Update "javascript:" handling; esp. SVG animation and MathML href.

[otherdaniel]

• Add SVG & to list of javascript:-attributes.
• Add a list for SVG animations.
• Minor edits when using those lists.

---

Preview | Diff

[annevk]

@cure53 do you know why DOMPurify bans all SVG animation? Do you know other maintainers we can reach out to?

[annevk]

We should double check that xlink:href is actually meaningful as it supposedly only works when xmlns puts xlink in scope which is never the case in HTML.

[zcorpan]

xlink:href works in HTML regardless of namespace declarations. https://html.spec.whatwg.org/multipage/parsing.html#adjust-foreign-attributes

[annevk]

@zcorpan that is xlink:href as an attribute, but not as an attribute value of these SVG attributes.

[zcorpan]

Ah, I see. Still, xmlns:xlink is also an adjusted attribute.

[annevk]

Good point, so it should work when xmlns:xlink is also specified. In which case we want to always ban xlink:href as is suggested here. (Having tests for all these combinations would be good though.)

I guess the other piece of feedback I forgot to note down here is that we should make sure we have the same processing model as these attributes have today, which regards to splitting on whitespace and such.

[securityMB]

@cure53 do you know why DOMPurify bans all SVG animation? Do you know other maintainers we can reach out to?

I believe the reason is that you can use SVG animation to animate href attribute which can lead to XSS.

For example:

<svg>
<animate xlink:href=#x 
         attributeName=href 
         values=javascript:alert(1) />
<a id=x>
  <text x=10 y=10>
    Click me
  </text>
</a>

[annevk]

@securityMB thanks for weighing in! This PR attempts to tackle that in a more specific way, allowing generic animation features to continue to work. We are wondering if that is good enough or if we'll run into other trouble.

[lukewarlow]

Trusted types covers the SVGAnimatedString IDL type, which covers you updating an SVGScriptElement hrefs baseVal, and it also includes the javascript: url pre-navigation handling for when a javascript URL is defined in markup and navigated to.

So I think if sanitizer is stripping the javascript: value for href (and probably it's xlink variant?) from any markup via the safe sync, along with probably stripping out the script element from SVG entirely (aka follow the same model as happens with HTML), that should be sufficient?

[annevk]

@lukewarlow no, see the example above: #212 (comment). We cannot rely on the navigation check Trusted Types has here. We need something along the lines of this PR or ban SVG animation entirely, but that seems less well founded.

[lukewarlow]

Sorry to clarify I didn't mean to rely on TT for anything I was just providing context on what TT does for this area.

[otherdaniel]

I wrote a test to figure out what browsers will actually do...

• Animating "href" to a javascript:-url is implemented by all browsers.
• Chrome & Safari, but not Firefox, will animate "xlink:href", but only if there's an explicit xmlns:xlink declaration.
• There seems to be no pre-processing (like strippping whitespace) involved. I couldn't get " href " to animate.

(Source is here and there.)

---

[Edit: In HTML documents, Chrome & Safari will animate "xlink:href", but only if there's an explicit xmlns:xlink declaration. They will not animate another prefix, even with a declaration. In XML documents, both will animate whatever prefix has the proper xmlns: declaration, e.g. bla:href with an xmlns:bla=.... It seems FF will not animate any name with any namespace prefix in any document mode.]

[otherdaniel]

The current version now does this:

• Remove the following attributes, if their value has a `javascript:' prefix:
  • <a href>
  • <area href>
  • <form action>
  • <input formaction>
  • <button formaction>
  • <a href> (SVG namespace)
  • <a xlink:href> (SVG namespace)
  • href in any element in the MathML namespace
• Remove attributeName attribute, if its value is href or xlink:href from:
  • <animate attributeName> (SVG namespace; also all below)
  • <animateMotion attributeName> (as above)
  • <animateTransform attributeName> (as above)
  • <set attributeName> (as above)

I think this covers all cases we have discovered.

Presently, this just non-chalantly states the algorithm. Should I add a note and/or appendix that explains how we got there, in case someone else wants to understand what the point is?

[annevk]

I would make this javascript: URLs throughout (including the :), including in definitions. I think that's more consistent with how we approach this elsewhere.

[annevk]

I think this looks good now, modulo nits. It's unfortunate we have to invoke the URL parser though, but I guess so be it. We never said that safe was going to be cheap.

[otherdaniel]

In the meeting, we had briefly discussed what the support for MathML + href + javascript: was. It seems there's already WPT tests for this: https://wpt.fyi/results/mathml/relations/html5-tree?label=master&label=experimental&aligned&q=mathml%2Frelations%2Fhtml5-tree%2Fhref-click

href-click-003.tentative.html tests navigating to a javascript:-url.

[otherdaniel]

Since this PR was quasi-abandoned, I've rebased and (partially) re-written it. Also, instead of the usual chronological commit history, this is now seperated out in semantically meaninfgul, seperate commits, that can also be reviewed seperately.

1. A handful of editorial changes that are likely uncontroversial. Mainly, moving if-then clauses with tiny then actions into a single line.
2. Complete hte list of built-in navigating URL attributes.
3. Handle SVG animation of navigating attributes.
4. Handle MathML xlink:href support.
5. Add an explanatory note.

[annevk]

Looks good to me modulo some nits.