svg / svgo

⚙️ Node.js tool for optimizing SVG files
https://svgo.dev/
MIT License
21.04k stars 1.39k forks source link

bug: removeScriptElement throws away xml:space=preserve for children elements #1960

Open SethFalco opened 9 months ago

SethFalco commented 9 months ago

Describe the bug

The removeScriptElement plugin will remove elements if they contain scripts. However, if the element that was removed had the xml:space="preserve" attribute, which applies to the children of the element too, this is unintentionally removed and spacing is no longer preserved.

To Reproduce Steps to reproduce the behavior:

Optimize the following with SVGO with only the removeScriptElement plugin:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
  <a href="javascript:(() => alert('uwu');)();" xml:space="preserve">
    <text x="10" y="35">
this is a test
    </text>
  </a>
</svg>

Expected behavior

Immediately, I'm thinking that xml:space="preserve" should be forwarded to children of the removed element. For example, in the scenario above, we'd remove the <a> element, and add xml:space="preserve" to the <text> element. In Firefox this resolves the visual difference.

Desktop (please complete the following information):

KTibow commented 9 months ago

Shouldn't this be generalized to all inheritable attributes, or is this a special case caused by it not being marked as inheritable or something?

SethFalco commented 9 months ago

Shouldn't this be generalized to all inheritable attributes

I haven't looked into it enough to say, but valid take.

It probably applies to all inheritable attributes, and probably should be handled in a utility that flattens the given node into its parent, which we can use everywhere we do this. We'd have to exclude children that set the same attribute as the removed parent did.

But then the annoying part, which is why I reported the issue instead. When testing xml:space before, it looks to me that while xml:space="preserve" applies to children, most browsers then ignore xml:space="default" from children so it continues to preserve whitespace despite being overridden, and I wanted to investigate this further.