Remove custom element state when is attribute is blocked

[evilpie]

This depends on changes in #385 that haven't happened yet.

I am not really certain if there isn't some other data structure that we need to patch for this. Presumably this would be easier to specify when we do streaming parsing.

Fixes whatwg/html#12533.

---

Preview | Diff

[otherdaniel]

Looks good editorially, but I wonder whether this would execute the custom element constructor during parsing?

As I understand this, this would create the custom element, and then later remove it again. If so, I'd expect this to execute the custom element's constructor, and thus bypass sanitization in a sense.

[evilpie]

Looks good editorially, but I wonder whether this would execute the custom element constructor during parsing?

As I understand this, this would create the custom element, and then later remove it again. If so, I'd expect this to execute the custom element's constructor, and thus bypass sanitization in a sense.

No it doesn't AFAIK. The custom constructor (if not removed) is only invoked when inserting. IIRC because the custom elements registry is null for those documents we use for parsing?

You can actually try this out with div.setHTML('<div is="custom-div"></div>') in Firefox Nightly.

[evilpie]

Rebased. Should be ready for review, but can't land before whatwg/html#12533 of course.

[evilpie]

@otherdaniel I would appreciate your review on this!

[noamr]

Maybe wait with this until after the upstream?

[otherdaniel]

Looks good editorially, but I wonder whether this would execute the custom element constructor during parsing?
As I understand this, this would create the custom element, and then later remove it again. If so, I'd expect this to execute the custom element's constructor, and thus bypass sanitization in a sense.

No it doesn't AFAIK. The custom constructor (if not removed) is only invoked when inserting. IIRC because the custom elements registry is null for those documents we use for parsing?

You can actually try this out with div.setHTML('<div is="custom-div"></div>') in Firefox Nightly.

To be honest, I'm still a little confused here.

• https://wicg.github.io/sanitizer-api/#set-and-filter-html
  • Sanitizer's setAndFilter calls the HTML fragment parsing algorithm with the context element.
• https://html.spec.whatwg.org/#html-fragment-parsing-algorithm
  • The HTML Fragment Parsing Algorithm, step 3, gets the context node's context document.
  • I thought that's the Document that gets passed down to the "create element" operation.
• https://html.spec.whatwg.org/#create-an-element-for-the-token
  • steps 6 + 7 look up a custom element registry for |intendedParent|
  • step 10 calls https://dom.spec.whatwg.org/#concept-create-element, with a registry derived in step 7
• https://dom.spec.whatwg.org/#concept-create-element
  • step 2 looks up a custom element registry for |document| (if the registry from the previous step is "default")
  • steps 4.3 + 4.4 seem to always run an upgrade operation, either directly or as a task.

So... the way I read the specs, I'd still expect your example of div.setHTML('<div is="custom-div"></div>') to throw the custom-div constructor (if it's indeed a registered custom element for the document of that initial div).

As an additional point.. if the HTML fragment parsing algorithm doesn't run the custom element constructor anyhow, then why do we even bother with this whole thing? My understanding is that the sanitizer algorithm sits between the parsing and insertion in the document. If the whole logic only runs when inserting and thus after sanitization, wouldn't this whole point be moot?

---

As said above... I find the whole thing confusing, and am not very confident in my analysis above.

Our implementation seems to slightly deviate from what I find in the specs, in that a custom element registry is expressedly passed in to the parser, and when |scripting mode| is Inert that registry is forced to null. My gut feeling is that this is the better way: If we can guarantee that the parser doesn't execute custom element script during parsing (in this context), then that seems more reliable than fixing this up afterwards.

[evilpie]

I don't really feel confident with the behavior here either, but I do want to try and understand this:

If the whole logic only runs when inserting and thus after sanitization, wouldn't this whole point be moot?

I am not sure what you mean here. Do you think the whole idea of trying to prevent the usage of is is wrong? As in, should an user not be able to block the creation of a custom element, or should it just block the execution of the JS code?

From my point of view we definitely don't want to execute custom element callbacks with a default config, in fact I have landed patches in Firefox which already implement this proposal. But I do think the specification, Chrome and Firefox are probably all different in this area.

[annevk]

The problem is not that it executes while parsing, but that the callback fires when connected to the main document. And a null registry is patched at that point during connection. This is very similar to script elements which we also disallow.

[otherdaniel]

If the whole logic only runs when inserting and thus after sanitization, wouldn't this whole point be moot?

I am not sure what you mean here. Do you think the whole idea of trying to prevent the usage of is is wrong? As in, should an user not be able to block the creation of a custom element, or should it just block the execution of the JS code?

No. I meant this as a "proof by contradiction" type argument: Before sanitizing, we run the HTML fragment parsing algorithm + copying nodes into a fragment. If we were sure those steps, any pre-sanitize-core steps in fact, would never call or enque calling constructors, then we could just sanitize as normal and this whole issue wouldn't exist. Not sure that was a particularly compelling argument, so if this is too confusing please just ignore it... :-)

I do think everyone agrees on the desired behaviour here. I think the disagreement is merely about the mechanism, or maybe the spec-ification of the mechanism. If the proposed wording works, I'm happy!

---

I recently landed a fix on our side, which works by making sure is isn't processed, rather than processing it first and then undoing the effect later. I think all implementations pass all the WPT tests:

• https://wpt.fyi/results/sanitizer-api/sanitizer-custom-elements-is.tentative.html?label=experimental&label=master&aligned&q=sanitizer-api%2F
• https://wpt.fyi/results/sanitizer-api/sethtml-with-custom-elements.html?label=master&label=experimental&aligned&q=sanitizer-api%2F

(I expect Edge to pass 100% too, as soon as they pick up the patch. They're on a slightly different release cycle than us.)