String-comparison against "href" + "xlink:href"

[otherdaniel]

https://wicg.github.io/sanitizer-api/#sanitize-core, step 1.5.9.5.3 reads:

"If the built-in animating URL attributes list contains «[elementName, attrName]» and attr’s value is "href" or "xlink:href", then remove attribute."

https://dom.spec.whatwg.org/#dom-element-setattributens, step 1, calls "validate and extract", which reads:

4. If qualifiedName contains a U+003A (:):
   1. Let splitResult be the result of running strictly split given qualifiedName and U+003A (:).
      [...]
   2. Set localName to splitResult[1].

For a qualifiedName "xlink:href:abc", this would set localName to "href". The ":abc" is discarded. Blink implements this as specified:

d = document.createElement("div");
d.setAttributeNS("https://www.w3.org/1999/xlink", "xlink:href:abc", "https://www.example.org");
d.attributes[0]  // xlink.href="https://www.example.org"
d.attributes[0].localName  // "href"

SVG Animations is surprisingly vague about what the attributeName value means. I guess in XML-world it didn't have to. Blink has implemented <svg:set attributeName> to also use "validate and extract" to parse its value as a qualified name, so that <svg:set attributeName="xlink:href:abc"> would animate the target element's href attribute. This is rather surprising and, worse, is a bypass of the Sanitizer API guarantees.

By my reading, Blink's implementation matches the specs. Admittedly, with the rather large caveat that it's not particularly clear how SVG, or actually SVG-in-HTML-syntax, wants attributeName to be interpreted. If my reading of the spec is correct, then the string comparison in "sanitizer core" is insufficient.

---

Original report here.

[evilpie]

I think the DOM spec is wrong here. The validate and extract definition was changed in whatwg/dom#1134 and I don't see any explicit desire there to allow more than one split.

[annevk]

I suspect the problematic change is whatwg/dom@e67d5fe as before multiple colons would not be valid.

I agree we need to either split on the first instance or concatenate the remainder afterwards.

cc @domenic

[sideshowbarker]

I agree we need to either split on the first instance

Yeah

or concatenate the remainder afterwards

It seems there’s no need to do further splitting and then concatenating/joining. It can be implemented by just splitting on the first instance — in other words, spec-wise, by not having “validate and extract” call “strictly split” at all, and instead just say:

1. Let prefix be the part of qualifiedName before the first U+003A (:).
2. Let localName be the part of qualifiedName after the first U+003A (:).

…because we don’t care about any other colons qualifiedName might have, and nothing else should be calling into this that needs it split any further than just on the first colon.

[otherdaniel]

From today's meeting: The decision is wait for whatwg/dom#1453 to resolve.

[sideshowbarker]

The validate and extract definition was changed in whatwg/dom#1134 and I don't see any explicit desire there to allow more than one split.

Yeah.

For the record here, what “validate and extract” said before the patch associated with whatwg/dom#1134 was this:

If qualifiedName contains a U+003A (:), then strictly split the string on it and set prefix to the part before and localName to the part after.

So, it was ambiguous — which is what whatwg/dom#1134 pointed out. But, one of the two ways it could have been read as intending is this:

If qualifiedName contains a U+003A (:), then strictly split the string on it and set prefix to the part before and localName to the part after.

In other words, it could have been read as actually intending what the new whatwg/dom#1455 change does. In other-other words, whatwg/dom#1455 is just reverting the spec back to what it had actually originally been intended to specify.

And nobody in the whatwg/dom#1134 discussion ever explicitly instead said anything like, “Throw away everything else in the name after splitting it into a list and taking just the first two list items is clearly what we had meant to require here.”

Instead, that’s just… effectively what it had ended up getting changed to state — without anybody ever having explicitly said there was some good reason for it needing to require throwing away everything else in the name.

[annevk]

The DOM specification has been updated. I suggest we add test coverage for this scenario before closing.