darylldoyle / svg-sanitizer

A PHP SVG/XML Sanitizer
GNU General Public License v2.0
456 stars 68 forks source link

Should xlink:href always be removed? #10

Closed niklashultstrom closed 7 years ago

niklashultstrom commented 7 years ago

Hello! Glad someone finally took on the SVG mess on WP :)

I ran in to this issue though, causing the SVG image not to display:

Before

<clipPath id="SVGID_2_">
 <use xlink:href="#SVGID_1_"  overflow="visible"/>
</clipPath>

After (upload)

<clipPath id="SVGID_2_">
</clipPath>

Similair with the image element, though just removing the xlink:href attribute:

Before

<image overflow="visible" width="66" height="77" xlink:href="data:image/jpeg;base64,/9j/4AA...jbf8ADaP/2Q==" transform="matrix(0.48 0 0 0.48 521.2959 384.499)">

After

<image overflow="visible" width="95" height="77" transform="matrix(0.48 0 0 0.48 638.2959 384.499)">

Which makes me wonder if xlink:href should be removed when containing just an #id or an base64 data-image.

darylldoyle commented 7 years ago

Hi @niklashultstrom,

The xlink:href attribute is not removed in the sanitiser.

In your first example, you're using the use element. This is stripped from the file, rather than the xlink:href attribute. This is inline with the sanitisation of DOMPurify.

In your second example, again the xlink:href attribute is not removed but the image element you've supplied is invalid XML and therefore the sanitiser fails to parse it and instead returns false. This can be fixed by either self closing the image element, or supplying a closing tag.

<image overflow="visible" width="66" height="77" xlink:href="data:image/jpeg;base64,/9j/4AA...jbf8ADaP/2Q==" transform="matrix(0.48 0 0 0.48 521.2959 384.499)" />

<image overflow="visible" width="66" height="77" xlink:href="data:image/jpeg;base64,/9j/4AA...jbf8ADaP/2Q==" transform="matrix(0.48 0 0 0.48 521.2959 384.499)"></image>

To see this in action try passing through the following strings at http://svg.enshrined.co.uk/ and you will see the difference:

`

` ` `
kent1D commented 6 years ago

Hmmm, that would be nice to make the same as this : https://github.com/cure53/DOMPurify/issues/233

The usage of <use... with relative content is frequent