[Declarative Shadow DOM] How should we build the "opt-in" for fragment parsing of declarative Shadow DOM?

[mfreed7]

In #831, there is a rough consensus that, to protect against client-side XSS, the entry points to the fragment parser need to be guarded by a declarative Shadow DOM "opt-in". If some script doesn't enable this opt-in, then any declarative Shadow DOM (<template shadowroot>) in the markup being parsed will be treated as a "normal" <template> that happens to have an attribute called "shadowroot".

How should this opt-in work? There are multiple entry points to the parser, some of which are "good" for sanitization, in that they parse into an isolated document fragment that doesn't execute script:

1. DOMParser.parseFromString()
2. <template>.innerHTML
3. XMLHttpRequest with an HTML MIME type and a data URL
4. <iframe> using srcdoc, src, document open/write, etc.
5. createHTMLDocument and then use createContextualFragment()
6. createHTMLDocument and then use body.innerHTML

Are there others?

Of the list above, the most straightforward for most would seem to be just adding an opt-in attribute to the relevant object:
    1. DOMParser: let parser = new DOMParser(); parser.allowDeclarativeShadowDOM = true;
    3. XMLHttpRequest: let client = new XMLHttpRequest(); client.allowDeclarativeShadowDOM = true;
    4. HTMLIframeElement: let iframe = document.createElement('iframe'); iframe.allowDeclarativeShadowDOM = true;

For createContextualFragment, perhaps just add an options bag? Seems cleaner than an attribute on Range():
    5. createContextualFragment: createContextualFragment(fragment, {allowDeclarativeShadowDOM: true});

The most difficult, it would seem, is the innerHTML attribute. Perhaps an attribute on the owning document?
    2 and 6. innerHTML: element.ownerDocument.allowDeclarativeShadowDOM = true; element.innerHTML = html;

[inoas]

Why is any of this better than a http / meta equiv header?

Given that I hope for javascript to get isolations so SSR apps can use a subset of javascript and leave the user alone in terms of the rest of security issues revolving around javascript and DOM, a language (aka js-independent flag) for creating declarative shadow doms would be nice. Also it should not require the JS parser to run prior to the shadow dom being constructed. (this comment seems irrelevant to the topic)

Anyway I will not further disturb you on this discussion. I hope you come to a sane conclusion.

[prlbr]

Maybe I misunderstand, but don't all of these opt-in suggestions require JavaScript, which would be contrary to "compelling use cases such enabling scoped styles without requiring Javascript"?

[mfreed7]

Why is any of this better than a http / meta equiv header?

Maybe I misunderstand, but don't all of these opt-in suggestions require JavaScript, which would be contrary to "compelling use cases such enabling scoped styles without requiring Javascript"?

For both of these questions, the answer is that here, we're talking about the fragment parser. I.e. for things like div.innerHTML = contentContainingDSD. That (by definition) happens in Javascript only, and doesn't happen for document parsing of navigated pages. For these questions, see #913, which is more concerned with server-generated content.

[mfreed7]

So I've modified the Chromium implementation to disable declarative Shadow DOM, unless opted-in, for all six of the parser entry points listed in the OP here. I also added WPT tests for each of these.

In essence, the API is mostly just an allowDeclarativeShadowDom property added to most of the relevant APIs. For iframes, I've added a new allow-declarative-shadow-dom sandbox flag. In all cases, it seems relatively simple to use from a developer point of view, and it should completely mitigate the problem use case here for old client-side XSS sanitizers. Here's an example usage, for the innerHTML case:

  const div = document.createElement('div');
  const html = '<div><template shadowroot=open></template></div>';
  div.innerHTML = html;  // No Shadow DOM here!
  document.allowDeclarativeShadowDom = true;
  div.innerHTML = html;  // Declarative Shadow DOM enabled!

The implementation seems pretty clean, and I'm guessing (hoping?) that the spec changes should be similarly straightforward. Mostly, this is a new flag on the Document, which is set from the various parser entry points (e.g. DOMParser, XMLHttpRequest, etc.). And a check of this flag from the parser, for <template> start tags. My plan is to start modifying the declarative Shadow DOM spec PRs to incorporate this change. But if someone sees a problem with the above, now would be a great time to tell me!

[domenic]

allowDeclarativeShadowDom

Should be allowDeclarativeShadowDOM per https://w3ctag.github.io/design-principles/#casing-rules

allow-declarative-shadow-dom sandbox flag

In general I don't think we're adding new sandboxing flags; instead document policy seems to be the right replacement? That also gives you the right header-based opt-in for the main document, I believe. @clelland would know more.

[domenic]

Also when at all possible it'd be better to avoid global toggles in favor of method parameters. So e.g. instead of document.allowDeclarativeShadowDOM = true you'd do div.setInnerHTML(html, { allowDeclarativeShadowDOM: true }).

[mfreed7]

@domenic thanks for the feedback!

Should be allowDeclarativeShadowDOM per https://w3ctag.github.io/design-principles/#casing-rules

Somehow I always get that wrong, even when I try to get it right. Thanks - will change.

allow-declarative-shadow-dom sandbox flag

In general I don't think we're adding new sandboxing flags; instead document policy seems to be the right replacement? That also gives you the right header-based opt-in for the main document, I believe. @clelland would know more.

Ok, I wasn't sure what the situation was there. It doesn't seem like Document Policy is quite available everywhere (anywhere?). That's why I went with the more supported sandbox attribute, which does seem to have a natural linkage over to Document Policy when available, no? This issue just needs an imperative way to enable/disable declarative Shadow DOM for use by sanitizers, and doesn't need a header-based opt-in. (That's #913, still up for debate.) I'll read up on Document Policy more, but is there a way to implement this that would be supported today, without requiring other implementers to also implement Document Policy? I suppose they could just not support DSD for <iframe>s ever, but it seems like that would break some use cases. Maybe not.

Also when at all possible it'd be better to avoid global toggles in favor of method parameters. So e.g. instead of document.allowDeclarativeShadowDOM = true you'd do div.setInnerHTML(html, { allowDeclarativeShadowDOM: true }).

Ok, I avoided this because I didn't want to invent a new API (setInnerHTML()) just for this. But if that's the preferred way of doing things, I can make that change. I think it's important to point out that (at least from the implementation) it seems like we'll need a flag on Document anyway. Other APIs, like DOMParser and XMLHttpRequest, just create a document and then call the existing parser algorithm, so setting a flag on those new Documents before calling the algorithm seems natural. So the only question is whether to expose that flag directly (as I've done) or hide the flag and only expose it indirectly through other APIs. Can you help me understand why that's better?

[domenic]

In general shared mutable global state is problematic and makes code harder to reason about. E.g., you can set it, then call a framework method which isn't prepared to deal with the consequences. (Not security consequences; just, it was written before DSD, and it doesn't know how to deal with some nodes getting swept into a shadow tree.) You can think of it similarly to how we try to avoid global variables in C++ that control the behavior of methods, and instead we pass arguments to methods.

[mfreed7]

In general shared mutable global state is problematic and makes code harder to reason about. E.g., you can set it, then call a framework method which isn't prepared to deal with the consequences. (Not security consequences; just, it was written before DSD, and it doesn't know how to deal with some nodes getting swept into a shadow tree.) You can think of it similarly to how we try to avoid global variables in C++ that control the behavior of methods, and instead we pass arguments to methods.

Yep, totally fair. Ok, I’ll make that change. How do you feel about using properties on DOMParser and XMLHttpRequest?

[clelland]

Re: Document Policy, which is available in Chromium as of M86, and is currently used for the opt-out for scroll-to-text.

In general, I'd much rather see features like this defined in terms of Document Policy than Sandbox; sandboxing has some very un-ergonomic properties. Adding this to sandbox means that it is automatically disabled by default in every sandboxed frame; it also means that a developer wanting to disable just this feature would have to use <iframe sandbox> and then specifically re-enable every other sandbox feature. (And keep that list up to date if other sandbox flags are added).

One question to ask right now is whether frames should be allowed to set this flag independently of their parent frames; sandbox does not allow that, ever -- if disabled in one frame, it is necessarily disabled in all of its subframes. Document policy allows a frame to disable a feature, without necessarily imposing the same restrictions on embedded content. (The spec does include a way to impose restrictions like that on subframes, like sandbox does, but that mode is not shipped yet.)

[mfreed7]

Re: Document Policy, which is available in Chromium as of M86, and is currently used for the opt-out for scroll-to-text.

@clelland thanks for this context - that's very helpful. I can check out the scroll-to-text spec to use as a template for this change.

In general, I'd much rather see features like this defined in terms of Document Policy than Sandbox; sandboxing has some very un-ergonomic properties. Adding this to sandbox means that it is automatically disabled by default in every sandboxed frame; it also means that a developer wanting to disable just this feature would have to use <iframe sandbox> and then specifically re-enable every other sandbox feature. (And keep that list up to date if other sandbox flags are added).

One question to ask right now is whether frames should be allowed to set this flag independently of their parent frames; sandbox does not allow that, ever -- if disabled in one frame, it is necessarily disabled in all of its subframes. Document policy allows a frame to disable a feature, without necessarily imposing the same restrictions on embedded content. (The spec does include a way to impose restrictions like that on subframes, like sandbox does, but that mode is not shipped yet.)

Ok, this sounds promising. I'll take a look at Document Policy for this feature.

Side note, it actually sounds like the sandbox feature itself runs afoul of #913 in some way. But I'll leave that as a pure side note for now.

[annevk]

I don't think we should call it "shadow DOM" in APIs. Using "DOM" in API names is a legacy construct. allowShadowRoot is probably sufficient since it's pretty clear from context it's about the parser.

[mfreed7]

I don't think we should call it "shadow DOM" in APIs. Using "DOM" in API names is a legacy construct. allowShadowRoot is probably sufficient since it's pretty clear from context it's about the parser.

Ok, makes sense to me. Anything to avoid an uppercased acronym is a win for me.

[domenic]

Yep, totally fair. Ok, I’ll make that change. How do you feel about using properties on DOMParser and XMLHttpRequest?

It'd be better to use an options argument for DOMParser IMO.

For XHR, I can't see anywhere nice to slot it in, and everything uses properties anyway. Although maybe you'd want it to be .responseType = "document-allowing-shadow-trees" instead of .responseType = "document" and then a allowShadowRoot property that only works when responseType === "document"? I dunno, that API is so terrible anyway. An alternative would be just never allowing DSD with XHR since we've generally been trying to avoid adding features to XHR; maybe @annevk has thoughts there.

[annevk]

Yeah, let's not add this feature to XHR.

[mfreed7]

I'm thinking that given the issues here, it might be best to just not add a setInnerHTML() type API at this point, and leave DOMParser as the only fragment parser entry point that (optionally) accepts declarative Shadow DOM. We can always add a setInnerHTML() type capability later, if it turns out to be a key use case. Given that the primary motivating use case for declarative Shadow DOM is SSR, which doesn't need an imperative interface at all, perhaps that's not too much of a limitation. It also seems like it could be relatively easily polyfilled:

Element.prototype.setInnerHTML = function(content) {
  const fragment = (new DOMParser()).parseFromString(`<pre>${content}</pre>`,
        'text/html', {includeShadowRoots: true});
  this.replaceChildren(...fragment.body.firstChild.childNodes);
};

I'm going to move forward with this approach for now, but please feel free to chime in with comments.

[mfreed7]

Ok, the PRs (including a new one against XHR) have been updated accordingly. Comments and advice appreciated - spec writing is still quite opaque to me.

PRs: HTML, DOM, XHR

[ByteEater-pl]

In general shared mutable global state is problematic and makes code harder to reason about. E.g., you can set it, then call a framework method which isn't prepared to deal with the consequences.

This concern could be discharged by adding a method allowDeclarativeShadowDOM setting a flag which is automatically unset after each task.