Allow SVGs to be handled as regular images

maxime-rainville commented 2 months ago

We've historically not allowed SVGs to be uploaded in the CMS because they can contain XSS payload. We had an RFC back in the day about adding full SVG support. We opted against it mostly for security reason.

Security rational is still valid. However they are some SVG good sanitizers that can be used to mitigate the risk. And many developers are choosing to wear the risk of using SVG to provide a better experience for their users:

In this context, we could update asset-admin so that SVG images can be handled as regular images in the CMS ... if developers choose to add them to the list of allowed file extensions.

User story

As a content author, I want to be able to use SVG images in the CMS transparently if my project owner is willing to accept the risk.

Acceptance criteria

SVG uploads are disallowed by default.
If SVG uploads are allowed:
- SVG are treated as Images in asset-admin
- SVG can be used in upload fields expecting an image
- SVG can be inserted as images in WYSIWYG.
- SVGs are displayed as thumbnails in the CMS.
- SVGs report their height and width correctly.
Any official doc about this include warning about the risk of using SVGs.

Note

The original RFC had some high minded discussion about whether SVG should be treated has regular Image and whether you should be able to rasterise them. That still seems relevant today.
We may explore adding some way to do efficient post processing of file uploads in the future. Implementation detail here probably should account for that.

maxime-rainville commented 2 months ago

This bit of YML gets you part of the way there

SilverStripe\Assets\File:
  allowed_extensions:
    - svg
  app_categories:
    image:
      - svg
    image/supported:
      - svg
  class_for_file_extension:
    svg: SilverStripe\Assets\Image

Thumbnails don't work, but you can get the SVG in a WYSIWYG.

GuySartorelli commented 2 months ago

Thumbnails don't work

We could easily update the ThumbnailGenerator to simply return the original file if it's an SVG.

The bigger problem is image manipulation methods... if you have a relation to Image you expect to be able to call any image manipulation methods on the image stored in that relation, but Intervention Image doesn't support SVG so those will all fail. The discussion you linked to about rasterising SVG is very relevant here. I think we should either not treat them as images at all, or have separate rasterised vs vector image support with appropriate methods available for each.

maxime-rainville commented 2 months ago

Yeah ... image manipulation is the biggest question in my mind.

I'm not sure we want to create a SvgImage in core ... at least not while we are not sure if we want to support SVG out-of-the-box.

I'm thinking maybe we just update the image picker logic in the WYSIWYG so you can pick something that isn't an Image class. Then 3rd party module like restruct/silverstripe-svg-images can provide integration to provide first class support for SVG

GuySartorelli commented 2 months ago

I'm thinking maybe we just update the image picker logic in the WYSIWYG so you can pick something that isn't an Image class.

That sounds like a can of worms that I don't think we want to open tbh. Images should be images. If we're not treating SVG as an image, it shouldn't be selectable as an image.

adiwidjaja commented 2 months ago

Wouldn't it be possible to intercept the image manipulation methods so that they all just always return the original svg? It's really most of the time not the desired effect to get a rasterized svg and my acceptance criteria for an SVG-Image is "does not break". Not "works just like a raster image" ;-)

GuySartorelli commented 2 months ago

Wouldn't it be possible to intercept the image manipulation methods so that they all just always return the original svg?

Yes, but:

That means for any new methods like those we have to remember that SVG needs to be treated specially - which would be easy to forget
There are a lot of those methods - that feels like a lot of change for something that this card suggests still won't even be allowed by default
That will likely produce unexpected results in scenarios where the template expects a rasterised image to be scaled/cropped/whatever else and it ends up getting a vector image that doesn't behave the way the template expected

This isn't to say we absolutely won't go down that route, just pointing out potential problems if we choose to do so.

maxime-rainville commented 2 months ago

Images should be images. If we're not treating SVG as an image, it shouldn't be selectable as an image.

Us developers can argue about how the system should handle SVGs internally, whether you should rasterise them or whether they should be resizable.

But I think the average content author won't give a toss about all of this. The end goal of an SVG is to display something that my eye balls can see. From the end user's perspective, that's an image.

GuySartorelli commented 2 months ago

Text is an image by that definition :p My worry is not a pedantic or semantic one. There are some distinct differences between raster and vector images both from a manipulation pov in php-land as well as how they're treated in the browser.

Right now everything we say is an "image" works the same way in both of those contexts, so developers don't have to account for any differences. Adding svg to our definition of "image" would change how developers have to think about their usage and treatment of images.

maxime-rainville commented 2 months ago

To the extent that you have to install a third party module to get SVG support, I think it's fair to put it back on the developer installing the module to make sure that anywhere they expect an Image, they can account for the possibility that they will get an SVG.

On the "Insert an Image in WYSIWYG" front, the only concern we would have to consider is "how do we handle the resize of the SVG?". That could be handled by serving you the same SVG file no matter what, and relying on the width and height attribute of the <img> tag. Ultimately, I don't think we have to solve this problem ourselves.

I think that's the bit that stop you form inserting non-image as image in the WYSIWYG. https://github.com/silverstripe/silverstripe-asset-admin/blob/a9951320e37088b38373335838361f627ecbdac4/code/Forms/FileFormFactory.php#L45-L49

We could just stick a bit in there to make this configurable.

Looking back at the ACs:

If SVG uploads are allowed:

SVG are treated as Images in asset-admin

SVG can be used in upload fields expecting an image

SVG can be inserted as images in WYSIWYG.

SVGs are displayed as thumbnails in the CMS.

SVGs report their height and width correctly.

Maybe it becomes:

The type of files that can be inserted as images in WYSIWYG can be customised.
None image files can specify a thumbnail variant to display in the asset-admin area.

The rest of the ACs will be the responsibility of the third party who is implementing the actual SVG support.

SVG are treated as Images in asset-admin
SVG can be used in upload fields expecting an image
SVGs report their height and width correctly.

forsdahl commented 1 month ago

Couldn't the manipulate and manipulateImage methods in ImageManipulation just check if the Image_Backend implementation supports the current filetype or not? If not, it could just return the image as is (which would be the case for SVG with all current Intervention backends). That would be a bit of a more general solution, so that different image manipulation logic isn't hardcoded for SVG specifically. Personally I would really like to see SVGs handled as images, but with sanitization on upload and image manipulations other than resize just returning the SVG as is.

silverstripe / silverstripe-asset-admin