RFC: Out-of-the-box SVG Support

[silbinarywolf]

I feel it's pretty common place nowadays for some clients to want to upload SVGs into the CMS. On all the older/closed issues, it states that until GD supports resizing of SVGs, this won't be added in.

Can't we just disable resizing on SVGs like so?
https://github.com/restruct/silverstripe-svg-images/blob/master/code/SVGImage.php#L90

Current solutions:
https://github.com/restruct/silverstripe-svg-images
https://github.com/stevie-mayhew/silverstripe-svg

Old issues:
silverstripe/silverstripe-cms#1469
#4670

[silbinarywolf]

@adrexia Has mentioned this was disabled due to JavaScript security issues. Assuming this is the case, we should just add some validation code to ensure a user cannot upload an SVG with a <script> tag inside it.

Perhaps use an existing sanitizing solution like:
https://github.com/darylldoyle/svg-sanitizer

[tractorcow]

The new standard "go to" advice for people wanting support for SVG is:

• Add a svg-supporting backend to intervention image (http://image.intervention.io/getting_started/configuration)
• Update config to support svg. Modifications required are to:
  • File.allowed_extensions
  • File.app_categories (both image and image/supported subkeys)
• Register custom intervention backend
• Manage all security concerns themselves. :)

YML config for intervention looks something like this:

SilverStripe\Core\Injector\Injector:
  Intervention\Image\ImageManager:
    constructor:
      driver: svgdriver

At this point we have no plans to support "out of the box" SVG support. However, what I would accept is a documentation guide for users wishing to build this support in, as-needed, as an alternative.

[chillu]

Add a svg-supporting backend to intervention image

So to be clear: It's not a matter of just selecting one - the Intervention library only supports GD and ImageMagick, and hence doesn't list SVG in it's supported formats. It looks like you need to move SVGImage.php logic down into a new Intervention backend, and create a couple of no-ops for unsupported features (return the original).

ImageMagick does support rasterizing of SVGs, but that's probably not the best course of action for most cases. There's a few discussions on the Intervention lib around SVG: https://github.com/Intervention/image/search?q=svg&type=Issues&utf8=%E2%9C%93.

Overall, it seems like a bit of a minefield in terms of full support for the ImageManipulation API in core. And I'm not keen to take on another complex third-party dependency (svg-sanitizer) for this use case into core. I think this functionality is well placed in the community, core just needs to ensure there's no hacks required to get it working.

[chillu]

@silverstripe/core-team Quick vote: Should we support SVGs as uploadable Image objects in core? Add your vote via 👍 or 👎 on this comment.

[sminnee]

I think that supporting SVGS and supporting manipulation of SVGs are two different things. Given that the critical use-case for image manipulation is resizing, and that I assume that SVGs don't require this, couldn't we support SVGs and not their manipulation?

If that's viable, that would get my vote.

[chillu]

You can already opt-in to SVG upload as File objects by setting allowed_extensions. I think even upload needs to be opt-in due to the security concerns. Enabling it by default would require the use of a sanitisation library. I'm not keen on the complexity, and the mentioned library doesn't exactly instill confidence ("my attempt at writing..."). Or you serve SVG files as text/plain by default, which can't be configured with reliable defaults (e.g. if you're serving with nginx instead of Apache), and probably defeats the purpose of allowing SVGs in the first place.

[jonom]

I think allowing uploading of SVGs could be okay but only if we can make sure they're safe. That's very different to supporting them as 'Images' in SilverStripe. Vector and Raster graphics are totally different and can't really be used interchangeably in web development. An SVG is closer to an iframe than a jpg in my opinion :)

I don't think SVG should be considered an Image in SilverStripe core unless we can reliably rasterise them and treat them consistently. The Image class may as well be named RasterImage, it's not appropriate for dealing with vector graphics in any way currently.

If we decided to support vector graphics I think we'd need something like a base Image class that defines common functionality, and a RasterImage and VectorImage class for format specific stuff. They're totally different graphic formats and behave differently when rendered in the browser too, so I don't think we can casually lump them together.

Note:

• A 5mb SVG file rendered at 10px by 10px is still a 5mb download (bandwidth / CMS thumbnail issue)
• Scaling SVGs can be tricky depending on viewport settings
• Will users expect they can choose an SVG file for something like a Facebook OG image?

[dhensby]

I've not actually come across users who want to upload SVGs, I'd be interested in hearing the usecase for it, are they expecting to treat them like first class images?

Like @sminnee I'd support allowing SVGs to be uploaded and skip over manipulations.

Developers looking to support SVG manipulation would likely need to implement a different Image_Backend which relied on a library that supported SVGs, rather than Intervention Image.

[silbinarywolf]

The use cases I've experienced are:

• Client with designer knowledge wanting to upload SVG's to their portfolio and display them in a gallery of sorts.
• Client wants the ability to replace their logo / affiliates images in the CMS. They push for SVG because it looks crisp on their iPhone/iPad/etc. (I've suggested just scaling down a high quality image for this, but they dislike that option)

I can definitely live without SVG manipulation.

[adrexia]

I've had similar experiences to @silbinarywolf. Logos are a very common requirement. Occasionally also vector illustrations for campaigns and the like, and cms editable iconography (eg a Type->Icon relationship). Its very uncommon for us to have a build that doesn't require svg upload. Though we can sometimes get around it by providing 2x image upload.

Saying that though, I don't require svgs to act as images. I use the silverstripe-svg module (https://github.com/stevie-mayhew/silverstripe-svg) for the behaviour I need there. What would be good would be some sort of sanitisation on svgs uploaded through the cms. But potentially that could be done in a module, rather than in core (maybe even in the silverstripe-svg module?).

[stevie-mayhew]

@adrexia I'd be happy for the silverstripe-svg module to do more if you need it. It was always just a quick and easy way for us to manage SVG through a theme, rather than uploaded assets but I can see the use case there and would be happy to help build out the functionality which would make it a more complete, CMS facing system

[silbinarywolf]

I'd prefer if SVG's were allowed to be uploaded as an "Image" as originally a logo for a client was PNG and they tried to replace it with an SVG.

Perhaps just don't show thumbnails of SVG in the CMS if an image is of that type?

[tractorcow]

Perhaps just don't show thumbnails of SVG in the CMS if an image is of that type?

An implementation of an SVG backend which simply returned the original in all cases could meet this need, if necessary.

The challenge here is to implement this as an InterventionImage backend; We don't have the ability (yet) to have different Image_Backend implementations for different file types; Maybe that would be easier?

[sminnee]

Related: silverstripe/silverstripe-assets#64

[sminnee]

My view is that the most important missing feature is that SVGs are handled specially by the image processing engine. I've written a ticket on that specific point here: silverstripe/silverstripe-assets#65

[micschk]

My two cents as the author of the restruct/SVG module; SVG's shouldn't be supported as if they're raster images, they are not. They are markup/vector (line) images, much like .html, .pdf, .dwg (CAD drawings) & .ai (Illustrator) and in that respect should simply be treated as files by default.

What makes SVG special compared to other vector formats is that their markup can be included right into html (inline, or as img src) and browsers will render these parts just fine. This makes for a special use case, like for example the situation with client logos described above, or say a set of custom designed icons.

I think rasterizing SVGs would be backwards, as that doesn't maintain any of the advantages (small file size, crisp rendering, etc) and all the disadvantages of raster images.

Where @stevie-mayhew's module is geared to SVG's as theme-assets (eg an SVG with a collection of custom designed icons which can simply be included one-by-one, inline in templates), our module complements it and is more geared to allow clients to upload/update vector based parts of their website.

Both use cases assume the developer to foresee the use of SVG images as 'special assets', which they are. (You wouldn't upload an HTML file or Markup file and expect the CMS to know how to rasterize/handle it it out of the box either, right?)

[Friksel]

Totally agree with @silbinarywolf. Doing a lot of (code based) graphics I'm very much surprised in 2017 there are still people wondering why SVG should be supported as an uploadable asset and people are still looking for usecases. The web is full of svgs. That they are supersharp and scalable and can be animated is a good reason, but also a lot of times filesizes are much smaller then even jpg while remaining quality on large (endless) scales. And they are able to contain animations / responsive content inside without the need to have multiple jpgs/pngs for all kind of screen sizes. They could also have javascript inside for animation purposes so this means javascript inside a svg isn't always unwanted and must not always be cleanedout. In my opinion this should be up to the developer (made optional).

I understand the current graphics lib is holding it from putting this in Silverstripe, because it doesn't support scaling of SVGs. But like others said before here most of the time that's not even needed, because it's a Scalable Vector Graphic. And if it's hard to generate a thumbnail I'd say just don't show a thumbnail in the meantime, or an SVG-icon instead would be enough for me.

Treating it as a file instead of an image is fine for me too. It would be great if it would show a SVG-icon as a thumbnail though, but if that's hard, just leave it. But at the moment SVG is not supported at all. Why? Like said above: it's almost 2018, all modern websites use SVG nowadays. Why no support for it in Silverstripe 4? Even Silverstripe 3 let users upload svg.

I understand we can add 'svg' to the allowed extensions inside File.php ourselves. But in my opinion you just can't leave out svg as a supported file in a state of the art CMS like Silverstripe 4 and have us add 'svg' to the allowed extensions ourselves everytime we upgrade to a new version of Silverstripe. Next to this it's not very clean changing Silverstripe core files.

In ss3 there was exactly the same discussion. d9b3982
After that the 'svg' was added to the allowed-extensions per default. But in v4 it's stripped out again. I don't understand why. What's the harm in having svg supported as an file-extension per default? At least for the mean time untill there is a better (Image) solution?

SVG is nothing to be scared about. It's a great friend making our websites beautiful and clean responsive looking on all our devices instead of those blocky, blurry jpgs and pngs!! Respect the SVG! Let the friend in!
My 2 cents :)

[tractorcow]

I totally agree; Any time you are rasterising an SVG you are probably doing something wrong.

What I'd like to see is native support for SVG though the SilverStripe Image_Backend API. It would be fine in my eyes to restrict image format translations; We could restrict raster / vector backends safely.

ImageBackendFactory.php could look something like the below:

// Verify file exists before creating backend
 $backend = null;
 if ($store->exists() && $store->getIsImage()) {
     if ($store->getIsVector() {
     	$backend = $this->creator->create($service.'.vector', $params);
     } else {
        $backend = $this->creator->create($service.'.raster', $params);
    }
 }

Where our vector Image_Backend would be something entirely separate to our InterventionImage backend;

For reference, intervention doesn't have any plans to support vector images in the future Intervention/image#254 (comment)

Looking at @micschk 's module https://github.com/restruct/silverstripe-svg-images/blob/master/code/SVGImage.php I would use this as a simple proof of concept for such a backend. :) We may want to explore other third party alternatives, however.

[dhensby]

@Friksel SVGs present a very real and high impact security issue if non-trusted sources are able to upload them. As a responsible vendor we need to make sure that SVGs can't be uploaded via Forms or, at the very least, be loaded by accident via browsers. Arbitrary JS execution in the same domain context is a real risk.

There's also a contradiction in your post where you say that you think developers should be able to configure if SVGs have JS stripped but you object to developers being able to opt-in to SVGs.

Whilst I'd like to see a more complete and secure approach to SVGs in SilverStripe, at the moment we have to accept that making them opt-in is a better for security than allowing them to be uploaded with no protection.

[jonom]

This RFC could be split in two :)

1. Allow SVG uploads out of the box

I think there's a consensus that manipulation isn't expected or possible on SVGs, so "supporting SVGs" really just means allowing them to be uploaded. There's no need for the Image class to include SVGs in that case, File does just fine. Developers just want the option to upload SVGs, but it's clear we can't do that safely (yet). Until it can be done safely, developers should just opt in if they need SVG (opting in is easy).

2. Treat SVGs as more than a File

There may be a case to have a limited common API between raster and vector graphics, and perhaps videos and other graphical content as well. Common methods could be things like getThumbnail, getTag, getLink etc., so we have a standard way of rendering those assets on the front-end and in the CMS. I don't think there is much demand for this so personally I don't think this would be worth the effort right now unless a third party component can provide the common API and take care of security concerns.

[Friksel]

@dhensby Thanks for your response. I understand where you're coming from and I agree I didn't think that much about website-users (vs cms-editors). I totally understand you want the core of Silverstripe to be safe from injections. I think so too. On the other side in my opinion there should be a balance between safety and being flexible for more experienced users. On the cms-side I do think we should keep in mind that a lot of uploading is done by experienced en responsible content editors or even developers themselfs maintaining their own company-websites (like myself).

So I think we should make a distinction between website-users and cms-users. In my opinion website-users shouldn't be able to upload svg's with javascript in it, but cms-users should (at least optional).
To be able to please both I suggested to make the 'stripout-javascript-from-svg' optional, perhaps with default to true so that Silverstripe is safe per default.

To please both, why not make make this configurable in the uploadfield (or simular) itself (like myUploadField->sanatiseSvg = false) and put this to true per default. Than we can have both: Silverstripe is safe per default and sanitise of svg's can be set on a per field basis in the website code to make a distinction between website-users and cms-users and even per field.
If svg's are supported to be uploaded per default by Silverstripe developers don't have to change Silverstripe core-files anymore to be able to upload svg's (having to change core-files, especially in a vendor package installed with composer, is a real no-go to me).

There's also a contradiction in your post where you say that you think developers should be able to configure if SVGs have JS stripped but you object to developers being able to opt-in to SVGs.

I don't get what you find is contradicting. Maybe I didn't explain well. Hopefully this post helps to clearify.

[tractorcow]

I feel that SVG probably feels like a non-core module that could address all of the concerns raised by @jonom ; It would need to not only provide SVG image resampling / modification capabilities, but also implement some kind of sanitisation to protect from scripts injections, to ensure that any uploaded SVG file is safe to use.

[jonom]

If svg's are supported to be uploaded per default by Silverstripe developers don't have to change Silverstripe core-files anymore to be able to upload svg's (having to change core-files, especially in a vendor package installed with composer, is a real no-go to me).

@Friksel I just want to expand on something @chillu said earlier - you shouldn't have to modify any core files to allow uploading of SVGs for all file fields by default. Just add this bit of yml to your project's config file:

SilverStripe\Assets\File:
  allowed_extensions:
    - svg

Note that if your upload field has been set to allow only types from the Image category, SVGs would still be rejected in that case.

[dhensby]

Note that if your upload field has been set to allow only types from the Image category, SVGs would still be rejected in that case.

But you can add SVG to the image types via config, I think if you really want that

[Friksel]

@jonom and @dhensby : I thought it was only possible by changing a core file. Being able to add the extension using config helps a lot. Thanks

[micschk]

Revisiting this today and going through some possible attack vectors, it is my opinion that SVGs are a snakepit and there's no way to securely allow general (untrusted) SVG uploads by default any time soon.

I've added some comments to the README;
...

SVGs are vulnerable to a lot of possible attack vectors, most of which are widely known and unpatched. Basically you should consider SVG a browser-executable format comparable to HTML/JS, but with virtually no exploit-protection built into browsers. In some circumstances, eg when parsing XML server side, SVGs could also pose server side risks like file inclusion (XML External Entity attack), fork bombs (Billion laughs) and probably dozens more.

...
Currently I don't know of any way to fully sanitize untrusted SVGs. Regular expressions are not suitable for the job and any PHP XML parsers are vulnerable to at least some attack vectors (like file inclusion). Here's a thorough listing of known attack vectors.

DOMPurify is a browser/JS based library that seems to do a pretty good job (but it's JS/NodeJS, not PHP). PHP based libraries which provide some protection but use (possibly dangerous) XML parsing are svg-sanitizer & SVG Sanitizer.

As a general rule of thumb, only work with trusted SVGs (created & uploaded by trusted users). SVGs loaded through an img tag provide a bit more security (eg no script execution) than inline SVG code.

[robbieaverill]

Re-visiting this as an RFC, which it should've been labelled as originally, here's the current status of core team votes to add this (as of 1.5 years ago):

In favour (2): @dhensby @sminnee
Against (6): @chillu @flamerohr @tractorcow @unclecheese @stevie-mayhew @robbieaverill

It's worth noting that there are also a number of community member votes in favour of implementing this.

On this basis of the core team vote, I'm going to close this. If developers want to add support for SVG file uploads, they can do so with configuration in their project, but the core team has voted not to support this feature by default with new SilverStripe projects for security reasons.