This filter - according to its own description - is supposed to restrict the src attribute of img tags to local images . (period)
However at the end of _filter_html_image_secure_process() the php function getimagesize() decides whether the path being checked is......an image file that getimagesize() happens to support getting the size of, which excludes svg images.
if (@getimagesize($local_image_path))
Either:
- the filter's description should be updated to reflect that it only accepts supported image formats
- or the filter should simply check for the existence of a file: file_exists()
Comment | File | Size | Author |
---|---|---|---|
#31 | 2855653-nr-bot.txt | 144 bytes | needs-review-queue-bot |
#29 | 2855653-29.patch | 3.19 KB | smustgrave |
Issue fork drupal-2855653
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Neograph734I ran into this exact same problem when working on #2842780: Add a token for the site logo.
Since svg's are perfectly valid resources for a src attribute of an img element according to the HTML5 specification paragraph 4.7.1, naturally I'd suppose it should be supported by Drupal as well. (Though there are more issues ragarding svg files.)
I'd like to propose a third option; in addition to getimagesize(), also check if the file is a valid svg file. For instance by checking its MIME-type.
If fact, PHP does not even recommend using getimagesize to detect if a file is a valid image:
So we might want to get rid of that as well?
Comment #3
Neograph734What about replacing
with
Though this might not accept certain extensions that getimagesize() did accept (swf, jpc, jpf, jb2 and swc), I think the acceptance of svg over those could be a valid trade off. (getimagesize() would accept all IMAGETYPE_XXX constants, where this method would only accept a part of them http://php.net/manual/en/function.image-type-to-mime-type.php)
Comment #5
Neograph734I forgot the @ in case the file does not exist.
Comment #6
Neograph734Hmm, it appears that some webservers configurations will recognize the files as
text/plain
ortext/html
rather than theimage/svg+xml
I was aiming for. So this does not solve the problem...We'd probably need some other solution then...
Comment #7
Neograph734So I have done some additional searching, this actually has been requested as a PHP feature; Request #71517 Implement SVG support for getimagesize() and friends, but it has not received a lot of attention yet so it probably won't happen soon...
Furthermore my suggestion from #3 to rely on MIME-types is not safe because finfo_open only checks for known patterns in files, which can be deliberately added, making the files appear as another MIME-type. (Perhaps that should be added as a comment above getimagesize()?)
And third, SVG files can contain JavaScript (MDN example). I suppose we'd want to sanitize or block that.
So, the best I could come up with would be something like this:
Where $custom_svg_schema could be an adapted version of the svg schema, which does not allow script tags and would cause the validation to fail.
Basically everything described in this StackOverflow post.
But I really have no experience with this...
Comment #8
Karol Haltenberger CreditAttribution: Karol Haltenberger commentedI still think the filter should only check if the file is local and not bother with image formats and sizes.
Ensuring that it is an image should be the responsibility of whatever or whoever put the url there (plugin, widget, the person typing it in etc.).
Comment #11
amaisano CreditAttribution: amaisano commentedAny progress on this? Our authors can choose SVG files in the WYSIWYG editor, but when they preview or publish the page, they are seeing a red X icon due to this bug.
Comment #13
zolt_toth CreditAttribution: zolt_toth commentedIt is still an issue, with Drupal core 8.7.x.
Comment #16
Neograph734There are two problems here.
At the bottom of the article is a link to an issue in Wordpress which attempts to solve this by using a huge whitelist: https://core.trac.wordpress.org/attachment/ticket/24251/24251.2.diff
UPDATE, after reading #2868079-14: Sanitize OR add Content-Security-Policy for SVGs I understand that SVG's in HTML img tags should not pose a risk. That is great for this issue because the sanitizing process should not be a blocker then.
Comment #17
Neograph734Comment #18
Neograph734I think something like this could work.
Comment #19
Neograph734And I think this should suffice as a test
Comment #21
Neograph734Had a typo in classy :(
Switched to using the built in mime type guesser and also asserting that external SVG files are invalid.
Comment #23
Neograph734Built on latest dev.
For lower version one can use
MimeTypeGuesser::guess()
Instead.Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedPatch fails to apply to 9.5
Comment #28
longwaveComment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedRerolled
Comment #31
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #32
Pierre Paul Lefebvre CreditAttribution: Pierre Paul Lefebvre at Evolving Web commentedComment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedWill also need an issue summary update before review.
Comment #36
rpayanmPlease review.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks!
The issue summary still needs to be updated.
Comment #39
alisonI think this issue (#2855653) and #2998318: "Restrict images to this site" restricts images that, by definition, *are* on this site. may be duplicates of each other.