Problem/Motivation
Currently, it's not possible to use SVGs with image styles, because the image style system only works with raster graphics. The core Image module is unaware of the distinction, and may attempt to process SVGs anyway, which can result in errors and other user-facing ugliness. This also makes it impossible to display vector and raster images side-by-side in a single image field with an image style applied, since it will not be able to display all the images in the field consistently.
Proposed Resolution
The system should not try to render images whose format is unsupported by the image style in use. This wouldn't affect images displayed without a style (i.e., the "original image" formatter for image fields).
Remaining Tasks
Review and commit the patch.
API Changes
None.
UI Changes
None.
Security Implications
None. This patch does not introduce any way to upload or store SVGs, or any other hazardous image format. It is only to display images that are 100% trusted and legitimate -- for instance, SVGs generated by the system itself.
Comment | File | Size | Author |
---|---|---|---|
#59 | interdiff-2652138-47-59.txt | 611 bytes | phenaproxima |
#59 | 2652138-59.patch | 8.6 KB | phenaproxima |
#47 | interdiff-2652138-43-47.txt | 1.13 KB | phenaproxima |
#47 | 2652138-47.patch | 8.6 KB | phenaproxima |
#43 | interdiff-2652138-41-43.txt | 1.55 KB | phenaproxima |
Comments
Comment #2
phenaproximaComment #3
chx CreditAttribution: chx commentedComment #4
chx CreditAttribution: chx commentedAlso, plz file a parallel D7 issue as well.
Comment #5
Wim Leers+1 for principle
Comment #6
chx CreditAttribution: chx commentedRe hardcoding: yeah, ugly, but there are no other browser supported vector formats and given how long it took for SVG to finally work (with IE dead and buried) I can't imagine another vector format (what for?) rising and gaining widespread browser support. So: this is fine. We can always do more.
However, for the sake of extensibility please move it into a method so people can easily code around it.
Comment #7
mondrakeRelated, #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)
Comment #8
phenaproximaComment #9
phenaproximaHere's another approach, which moves the style selection logic into its own method as @chx requested in #6. No need for an interdiff, since the code is totally different.
Comment #10
mondrakeThis is only true if we consider core's GD image toolkit. With ImageMagick, it's perfectly legit to pass an SVG in input and apply an image style to it, as ImageMagick internally converts the image to raster. The only consideration is that ImageMagick is not good at outputting vector images (see http://www.imagemagick.org/Usage/formats/#vector), so image styles should have explicit image effects to convert them to a raster format.
Comment #11
claudiu.cristeaIf there are 4 .png files and one .svg, the we'll load the image style 4 times? No, you should get the image style outside foreach() or statically cache it. Also $cache_tags should be computed only once but should be empty if all no item will use an image style. Needs optimisation.
s/NULL/null
!==
Maybe these might be merged?
Also I would change the name to $image_style_name or .._id because we usually use $image_style for the object.
Also there's need for tests.
Comment #12
phenaproximaOK -- did some refactoring to optimize. Still no tests, but if this patch looks good I will write some :)
Comment #13
r.nabiullin CreditAttribution: r.nabiullin at Skilld commentedrerolled patch from comment #12, become outdated
a bit later, i will add tests
Comment #14
claudiu.cristeaComment #15
phenaproximaRerolled and rewritten. I'd like to get this into 8.2, since it'll help get a few cool features into the Media Entity suite of modules.
No interdiff because it's substantially different from the patch in #13.
Comment #16
mondrakeComment in #10 is not addressed. Hardcoding the MIME type check against
image/svg+xml
will forcefully prevent image styling also for toolkits that can process SVG images (I know about the security concerns related to letting users upload SVG files, but that's a separate discussion in #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)).BTW, beware of #2760797: MIME type guessing fails for remote stream wrapper URIs.
Alternative suggestion: what about loading from the
image.factory
the image file at$file->getFileUri()
, and check its validity via::isValid()
. The underlying toolkit will tell us if it can apply the style to the image or not. IMHO it will be more generic and avoid hardcoding.Comment #17
mondrakeOr, if we want to avoid the file I/O connected with getting the image from the factory, we could check if the file extension falls into one of those returned by
ImageFactory::getSupportedExtensions
. But for GD toolkit we would need #2477381: GDToolkit::getSupportedExtensions returns incomplete list before.Comment #18
mondrakeComment #19
phenaproximaAdded the ImageFactory-based check from #16. I'd rather have done something a little less I/O heavy, but I'd rather not postpone this issue on #2477381: GDToolkit::getSupportedExtensions returns incomplete list.
Comment #20
phenaproximaAdded tests!
Comment #21
mherchel+1 Great work!
Comment #22
mondrakeActually, #16 defeats the concept of processing the image style formatting without opening an Image object that was implemented through
ImageStyle::transformDimensions
which is called intemplate_preprocess_image_style()
. AFAICS the entire image style processing is split into HTML formatting, which occurs through #themes 'image_formatter' and 'image_style' within the request, and the actual delivery of the derivative image which occurs in separate requests. Sorry for not considering that before. #17 could do, though.Not sure we can change the constructor signature here, is it a BC break? Alternatively we can have a getter calling
\Drupal::service('image.factory')
Comment #23
phenaproximaI don't quite understand how this is a problem. This patch is about making ImageFormatter smarter, but nothing about it will prevent people from attempting to apply image styles to SVGs in the theme layer; trying to prevent that would be way out of scope for this patch.
Oooh, good catch. I will add an internal getter for image.factory, but I'll keep the constructor parameter as well and simply default it to NULL (in case anyone is weird enough to instantiate the formatter directly :)
Comment #24
mondrakePlease see #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O. It's all about getting the final dimensions of a derivative image without accessing the image file. With just the simple call to
$this->imageFactory->get($file->getFileUri())
we do access the image file at the very beginning of the process, so defeating the purpose of that issue.Comment #25
phenaproximaI'd rather avoid the extra I/O as well (as I said in #19), but if the only way to do that is to get #2477381: GDToolkit::getSupportedExtensions returns incomplete list in first and postpone this issue on that one, then that is what must be done.
Comment #26
phenaproxima#2477381: GDToolkit::getSupportedExtensions returns incomplete list has been committed to 8.2.x, so this is now unblocked. Full steam ahead!
Comment #27
Gábor HojtsyRemove duplicate tag.
Comment #28
mondrakeI have an alternative proposal.
The effect of the patch in #20 will be that if an image file is not supported by the toolkit, the original image will still be rendered, but on its full dimensions. The image style theme, however, can still determine (in some cases - if an image effect sets the dimensions and/or if the original dimensions are stored in the filed) the derivative image dimensions, via
ImageStyle::transformDimensions
, which is toolkit agnostic.We can work on a lower level and just let
template_preprocess_image_style
determine if the image can be derived or not. If yes, all OK; if no, still render the original image as in patch #20, but with the plus of adding width/height HTML attributes as determined by the image style if that is possible. Browsers will then do the sizing at runtime.With the patch here, the ImageFormatter code is in fact not touched at all. The interdiff is only interesting for the test code differences, I guess.
Comment #29
andypostLooks greaT!!!
Comment #30
andypostsad to miss a window
Comment #31
Wim Leers#28: great insight, great write-up! Thanks a lot! :)
Just reviewed the patch. I have zero remarks! (That's very rare!) I only have nits, but they're so minor that I'll just omit them. The solution is elegant, the test coverage is comprehensive. What more could we want? :)
Comment #32
alexpottThe solution on this patch gives me pause because there are many things an image style can do that are no related to width and size for example rotation and watermarks - this patch makes it look like it is working when it is not - which is a very confusing behaviour. Are we sure that this is the right approach?
Comment #33
phenaproximaEDIT: Never mind, misunderstood the patch.
Comment #34
phenaproximaI'm in favor of committing the patch as-is, personally, but I don't have commit access :) So I discussed with @alexpott on IRC and we arrived at a compromise.
Let's add a new method to ImageStyleInterface called supports() or similar. This method will accept a URI (or a file entity, from which it can extract a URI) and return a boolean indicating if that URI is supported by the image style. The default implementation in ImageStyle will, if the URI ends in .svg, loop through its effects and return false if any of its effects are not instances of ResizeImageEffect, since that, as far as I know, is the only effect we currently have that can work with SVGs. If the URI is not an SVG, supports() will just return true.
This method will be called by template_preprocess_image_style(), and if it returns false, $variables['#image'] will just be unset (or otherwise cut off) so that the image will not be rendered at all. Anything, in my opinion, is better than displaying a 404 (or worse, 500) error and a "broken image" icon where the image should be. template_preprocess_image_style() will also write something to the error log so that the site builder will not be mystified as to what happened.
Comment #35
phenaproximaFirst attempt to implement that approach. I changed a few things relative to my previous comment -- for one thing, I moved the ImageFactory::getSupportedExtensions() check into the new ImageStyle::supportsUri() method. And template_preprocess_image_style() will simply set $variables['images']['#access'] to FALSE if the style does not support the image, rather than unset $variables['image'], so as to give any further preprocess functions some wiggle room.
Comment #36
bkosborneSlightly off topic here, but can't we not use an image field to store SVGs until this is resolved anyway? #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)
Looks like image field has a hard coded set of file extensions it can allow.
Comment #37
phenaproximaImage fields can already store SVGs, but they cannot properly display them using the standard Image formatter. That's what this issue aims to resolve.
Comment #39
Gravypower CreditAttribution: Gravypower commentedHello I believe there may be an issue with the patch,
Some of the images on my site have file extensions that are upper case, this causes the in_array check on line 351 of ImageStyle.php to fail. Can I suggest that the result from the pathinfo is converted to lowercase before the in_array call.
patch attached.
Comment #40
phenaproxima@Gravypower, please post interdiffs when making patch changes; it helps everyone else review your good work :)
Comment #41
phenaproximaRe-rolled for 8.4.x.
Comment #42
Wim LeersWanted to RTBC, but…
Trailing spaces.
Should be fixed before this can be RTBC.
Comment #43
phenaproximaFixed #42.1, but I left #42.2 as-is because ImageStyle is an entity and entities don't support dependency injection. In fact, the ImageStyle entity already calls
\Drupal::service('image.factory')
elsewhere -- see the createDerivative() method. (I've seen \Drupal in many other entity classes as well.) So I just removed the @todo comment instead.Comment #44
Wim Leers#43.2: fine, but then let's instead add a
protected function getImageFactory()
method, which wraps the\Drupal::service()
call, so you can at least still do unit testing.Comment #45
phenaproximaSounds like a plan, but can you direct me to an existing example in a core entity class, so that I have something to pattern this after?
Comment #46
Wim LeersFor example:
\Drupal\Core\Config\Entity\ConfigEntityBase::getTypedConfig()
+\Drupal\image\Entity\ImageStyle::getImageEffectPluginManager()
.Comment #47
phenaproximaFixed! Thanks, @Wim Leers!
Comment #48
Wim LeersPerfect :)
Comment #49
phenaproximaComment #50
alexpottIt's not that they are a potential - it is that they are a security risk. If you give someone the ability to upload an SVG to your site then they can embed javascript on that site. I really think we need to be careful here. See https://stackoverflow.com/questions/5378559/including-javascript-in-svg
At the very least we should file a blocking issue that if someone adds svg to the allowed filetypes list they are made aware that this should not be granted to users you have no control over or trust of.
In #3 there was no security review.
Comment #51
alexpottAn alternative would be for Drupal to ship with a content security policy for SVG - something like what is discussed here https://github.com/github/markup/issues/289
Comment #52
phenaproximaI had initially requested security review for this issue, but because it does not give anyone the ability to upload SVGs in any way, shape, or form, @chx removed the tag. The point of this issue is to allow the use of legitimate SVG that has been generated by the system. (For example, Lightning generates thumbnails for text-only tweets and saves them as SVG, since the format lends itself well to that use case.)
Not only that, but you already cannot grant SVG upload capability on image fields at the moment. If you add it as an allowed extension, the core image widget will reject it anyway. I filed an issue for that as well -- not to allow SVGs, but to inform the user that this is the case. #2867494: File and image widgets are misleading about the extensions they accept
Comment #53
alexpott@phenaproxima good points - I agree with your assessment of the security implications of this patch. I'm removing the "needs security review" as a member of the security team. I'm removing it because the current patch does not enable SVG upload or directly support SVGs and thereby does not introduce new security concerns for core.
However I think that the issue title and summary need to be be updated to reflect how the final solution implemented here does not "Add SVG support to ImageFormatter". We also need a change record that explains the nature of the change and how to leverage it.
Comment #54
phenaproximaComment #55
phenaproximaComment #56
phenaproximaChange record written: https://www.drupal.org/node/2868331
Comment #57
phenaproximaOne thing I notice: It should say "Could not apply @style image style to @uri" (missing the 'to'). Could that be fixed on commit?
Comment #58
alexpott@phenaproxima committers have a lot to do on commit - the more that is done before the better.
Comment #59
phenaproximaDone. Leaving at RTBC because it was just a wording change.
Comment #60
alexpottUpdating issue credit.
Comment #61
alexpottCommitted 87c73d8 and pushed to 8.4.x. Thanks!