Error Uploading Some SVGs in Panel (attribute and namespace errors)

[neildaniels]

Describe the bug
I've run into situations where the Panel refused to accept uploads of certain SVGs. It seems like it's running SVGs through some kind of strict validation, where some extra attributes or "invalid" attributes cause an outright rejection.

Can this validation be removed or improved for SVGs?

Current workaround is I have to import and reexport SVGs from something like Sketch.

To Reproduce
Steps to reproduce the behavior:

1. Try to upload either of the attached SVGs to a Panel files section

Example SVGs.zip

Reproduces on https://trykirby.com

Expected behavior

Screenshots

Kirby Version
3.5.6

Console output

Desktop (please complete the following information):

• OS: macOS
• Browser: Safari
• Version: 14.1.1 (16611.2.7.1.4)

[bastianallgeier]

The new validation rules are a the result of a potentially severe security issue, we fixed in: https://github.com/getkirby/kirby/releases/tag/3.5.4

We will look into your examples and see what we can do. Our tip right now is to optimise the SVGs with https://jakearchibald.github.io/svgomg/ first. It's a lot more comfortable to import and reexport from Sketch.

[lukasbestle]

@neildaniels I've created a PR that allows to allowlist the custom namespace used in the second file. It's weird that this namespace is in there as it's not used at all, maybe it's just an ad for the optimization tool?

Regarding the first file I agree with Bastian that optimization before upload is the way to go: The xml:space property is deprecated. There are a few other deprecated properties that are in use in tools with a lot of history (like Illustrator or Inkscape). We didn't want to include them as we would have to manually review each of them for possible security risks. It's a good idea anyway to run SVGs through SVGO or SVGOMG to get an optimized file size for the web.

Also see #3433 (comment) for more details on our Sane implementation and how you can extend it.

[bastianallgeier]

✅

[rasteiner]

SVGOMG no longer removes xml:space since SVGO v2.1.0. This is because xml:space is actually useful (Illustrator uses it, many files online have it, and browsers still support it even if deprecated).

Could you point me in a direction that explains what the security implications of xml:space are?
The only source I've found is this, where the exploit consists of crafting a particular value for xml:space and making the file interact with XmlLite.dll

Did I miss another one or is it just that? Otherwise it could be useful to allow this attribute, but limit its value to "preserve" (there's also "default" as valid value, but that is removed by svgo because it's the [obvious] default).
I think I would be able to PR this if you want

[iskrisis]

I have the same issue. It's really to explain clients. Actually i myself can't seem to be able to upload svgs. One thing is xml:space but i also get errors like enable-background not allowed.

Can't i turn this off if i don't have malicious editors?

[lukasbestle]

@rasteiner @iskrisis We will add support for xml:space in 3.6.1. Missing it was an oversight. In a future 3.6.x release we will also add a mode that allows to sanitize instead of validate uploaded files. Then everything that isn't allowlisted will just be removed from the uploaded file.

@iskrisis You have two options: Either you can add the attributes you need to the allowlists (for example with Kirby\Sane\Svg::$allowedAttrs[] = 'enable-background';). If you want to disable validation completely, you can use:

unset(Kirby\Sane\Sane::$handlers['svg']);
unset(Kirby\Sane\Sane::$aliases['image/svg']);
unset(Kirby\Sane\Sane::$aliases['image/svg+xml']);

For everyone else reading along: Of course we do not recommend this. Only do it if you are absolutely sure all file uploaders can be trusted.

[alicericci]

@iskrisis You have two options: Either you can add the attributes you need to the allowlists (for example with Kirby\Sane\Svg::$allowedAttrs[] = 'enable-background';). If you want to disable validation completely, you can use:

unset(Kirby\Sane\Sane::$handlers['svg']);
unset(Kirby\Sane\Sane::$aliases['image/svg']);
unset(Kirby\Sane\Sane::$aliases['image/svg+xml']);

In which file do I need to put those lines to disable validation ?

[lukasbestle]

@alicericci Do you still have a need for this after the changes in Kirby 3.6.1? I‘m interested to hear what stops you from using Sane.

To answer your question: You can put these lines anywhere after Kirby is initialized, so in a plugin file or in site/config/config.php.

[alicericci]

@lukasbestle Thanks !

After the changes in Kirby 3.6.1, svg cleaned up with svgomg work, but svg straight from illustrator don't.

The doctype produced by illustrator
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

[lukasbestle]

@alicericci I don't have Illustrator at hand to test it, but I heard that checking "Minify" in the export dialog omits the doctype.

[8grad]

hej, 👋🏼 *.svg's cleaned up with svgomg does the trick for me.