Custom element registry can call constructor of sanitizer-removed elements

[noamr]

Envision the following script:

const scoped_registry = new CustomElementRegistry();
scoped_registry.define("forbidden-element", class ForbiddenElement : HTMLElement { 
  constructor() {
    super();
    console.log("This should not be called");
  }
});

const container = document.createElement("div", {customElementRegistry: scoped_registry });
container.setHTMLUnsafe(
  "<forbidden-element></forbidden-element>", {
  sanitizer: {removeElements: ["forbidden-element"]}
});

According to the current spec, the console would log "This should not be called".

That is because:

• the sanitizer API calls the fragment parser
• In turn, the fragment parser uses the custom element registry of the context element
• Parsing happens before sanitization
• Custom element constructors happen straight away when an element is created.

[noamr]

cc @zcorpan
I think this might make us want to move to a streaming mode sooner rather than later, where we can perhaps avoid creating removed elements in the first place.

[noamr]

I am not sure if this is a security flaw btw, it's a matter of a judgement call -
should the sanitizer prevent calling these constructors on some disconnected element that's about to be removed?

The caller can call document.createElement("x-element", {customElementRegistry: element.customElementRegistry}) and invoke that constructor anyway and it would have the same effect

[noamr]

Filed whatwg/html#12274 as well

[noamr]

Some options of how to solve this:

• Change the behavior of those registries (as per Passing the scoped custom element registry to the fragment parser has non-obvious side effects whatwg/html#12274)
• Add some state to elments when parsing about whether they are "really" going to be constructed or just provisionally, and only queue the CE reactions after sanitization
• Specifically don't schedule those CE reactions for custom elements not allowed by the registry, or block lookup of those elements in the registry.

[sorvell]

I am not sure if this is a security flaw btw, it's a matter of a judgement call -

Trying to make a steel-form argument for this...

• the element must already be defined for it to do anything harmful, meaning the harmful code has already run and therefore the harm is already done.
• connectedCallback will not run and this is generally when code that interacts with the document is placed
• built-in elements have similar (?) behavior

Some options of how to solve this:

The spec related to this is really complicated so I don't know specifically what is viable:

• won't fix based on steel-form argument above.
• avoid creating elements that are to be removed.
• change the parsing behavior to: (1) create the elements with a null registry, (2) run sanitization, (3) adopt the (remaining) elements with the proper registry. Note, this might require adding a fallback registry to the adoption process which seems useful.

[noamr]

I am not sure if this is a security flaw btw, it's a matter of a judgement call -

Trying to make a steel-form argument for this...

• the element must already be defined for it to do anything harmful, meaning the harmful code has already run and therefore the harm is already done.
• connectedCallback will not run and this is generally when code that interacts with the document is placed
• built-in elements have similar (?) behavior

Yea that doesn't work. Untrusted code can confuse the host to run code at an unexpected time.

Some options of how to solve this:

The spec related to this is really complicated so I don't know specifically what is viable:

• won't fix based on steel-form argument above.
• avoid creating elements that are to be removed.

We probably can't exactly do that because we need them in the stack of open elemetns. But we might be able to supress their CE reactions.

• change the parsing behavior to: (1) create the elements with a null registry, (2) run sanitization, (3) adopt the (remaining) elements with the proper registry. Note, this might require adding a fallback registry to the adoption process which seems useful.

Sure

[sorvell]

@noamr

Envision the following script:

Can you confirm that the behavior here isn't related to scoped custom element registries? Is the issue that the spec changed to accommodate them?

This also logs.

customElements.define(
    "forbidden-element", 
    class ForbiddenElement extends HTMLElement { 
      constructor() {
      super();
      console.log("This should not be called");
      }
    }
  );

  const container = document.createElement("div");
  container.setHTMLUnsafe(
  "<forbidden-element></forbidden-element>", {
  sanitizer: {removeElements: ["forbidden-element"]}
  });

[noamr]

@noamr

Envision the following script:

Can you confirm that the behavior here isn't related to scoped custom element registries? Is the issue that the spec changed to accommodate them?

This also logs.

customElements.define(
"forbidden-element",
class ForbiddenElement extends HTMLElement {
constructor() {
super();
console.log("This should not be called");
}
}
);

const container = document.createElement("div");
container.setHTMLUnsafe(
"", {
sanitizer: {removeElements: ["forbidden-element"]}
});

I believe that this would be an implementation bug.

[sorvell]

I believe that this would be an implementation bug.

Mind explaining why it's an implementation bug?

Based on what you said:

In turn, the fragment parser uses the custom element registry of the context element

and the text in step 8, it seems like this is expected behavior and not a bug?

[sorvell]

@annevk and @ja-y-son and @noamr

Do you think it could make sense to modify the steps here to something like this (additions are bold underlined):

3. Let newChildren be the result of the HTML fragment parsing algorithm given contextElement with its customElementRegistry as null, html, and true.
4. Let fragment be a new DocumentFragment whose node document is contextElement’s node document.
5. For each node in newChildren, append node to fragment.
6. Run sanitize on fragment using sanitizer and safe.
7. Using contextElement's customElementRegistry, run customElementRegistry.initialize on fragment.
8. Replace all with fragment within target.

The idea is that elements would not customize before step 6 when they may be removed.

[noamr]

I believe that this would be an implementation bug.

Mind explaining why it's an implementation bug?

Based on what you said:

In turn, the fragment parser uses the custom element registry of the context element

and the text in step 8, it seems like this is expected behavior and not a bug?

Ah sorry, you're right - that registry could be the document's registry and is not necesarily scoped.
However, without scoped registries, the solution is relaltively simple - set it to null and let the document's registry take effect when adopting.

Maybe the solution here could be to always pass a null registry, and then iterate on the fragment after sanitation to invoke those reactions.

[annevk]

I like @sorvell's idea. I think it's equivalent until we add the customelementregistry attribute. (When we add that we'll need to figure out what behavior we want in this case.) Would be good if someone could run it against the tests to see if there are any surprises.