[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]

Ok I almost have another patch landed in Chromium that makes the changes you've recommended here:

• Remove allowDeclarativeShadowDom state from Document, DocumentFragment, etc.
• Replace the above with call-site parameters for all but DOMParser, which gets a constructor parameter.
• innerHTML no longer supports Declarative Shadow DOM, ever.
• A new setInnerHTML() function allows opt-in access to DSD through a second options-bag argument.
• Several of the less-well-lit parser APIs do not have an opt-in for declarative Shadow DOM. For example XMLHttpRequest, createContextualFragment, and document.write() will not support DSD. (document.write() in a synchronous script as part of the main document will still be supported.)
• The sandbox flag has been removed from <iframe>s completely. My new plan is to use Document Policy to allow more fine-grained control over declarative Shadow DOM for <iframe>s. For now, <iframe>s always support declarative Shadow DOM. I will work on the Document Policy change in a subsequent CL, once I get the rest of this squared away.
• allowDeclarativeShadowDOM has become allowShadowRoot for all of the options above.

You can see how these look by reviewing the WPT test changes that were required.

As always, comments appreciated.

[domenic]

That all makes sense to me, with one exception:

Replace the above with call-site parameters for all but DOMParser, which gets a constructor parameter.

Sorry, I wasn't clear. I think it should be a third options argument to parseFromString(), not an option to the constructor. Currently DOMParser is completely stateless, and IMO it should stay that way.

[mfreed7]

That all makes sense to me, with one exception:

Replace the above with call-site parameters for all but DOMParser, which gets a constructor parameter.

Sorry, I wasn't clear. I think it should be a third options argument to parseFromString(), not an option to the constructor. Currently DOMParser is completely stateless, and IMO it should stay that way.

Ok, that's an easy enough change, I think. I'll try to get that landed tomorrow.

@annevk, any issues overall?

[annevk]

This sounds good overall, except that I'd wait with introducing setInnerHTML until we have a standardized sanitizer (see https://github.com/WICG/sanitizer-api) as not using the sanitizer should be an opt-out (labeled "unsafe"). Introducing another API that will incur XSS does not really seem acceptable to me.

[koto]

This sounds good overall, except that I'd wait with introducing setInnerHTML until we have a standardized sanitizer (see https://github.com/WICG/sanitizer-api) as not using the sanitizer should be an opt-out (labeled "unsafe"). Introducing another API that will incur XSS does not really seem acceptable to me.

Isn't this more of an argument for Trusted-Types-like guards on this sink? Sanitizer API leans into the direction of unconditionally removing JS code, and this sink needs to, at least optionally, support JS in HTML. So we'd either end up with two different sinks (unsafeSetInnerHTML(string), and setInnerHTML(stringToSanitize)), or an additional argument for both variants. It seems more elegant to just cover it with Trusted Types guards instead, or only require TrustedHTML with no option for a string.

[mfreed7]

This sounds good overall, except that I'd wait with introducing setInnerHTML until we have a standardized sanitizer (see https://github.com/WICG/sanitizer-api) as not using the sanitizer should be an opt-out (labeled "unsafe"). Introducing another API that will incur XSS does not really seem acceptable to me.

Isn't this more of an argument for Trusted-Types-like guards on this sink? Sanitizer API leans into the direction of unconditionally removing JS code, and this sink needs to, at least optionally, support JS in HTML. So we'd either end up with two different sinks (unsafeSetInnerHTML(string), and setInnerHTML(stringToSanitize)), or an additional argument for both variants. It seems more elegant to just cover it with Trusted Types guards instead, or only require TrustedHTML with no option for a string.

I tend to agree with @koto on this - there's not much point to a sanitized setInnerHTML() that removes all JS from the content. I've marked the new setInnerHTML API as an HTMLString, so it should play nicely with the Trusted Types system. Does that alleviate your concerns over a new API?

My original approach here was to add an opt-in to the Document, to avoid adding another setInnerHTML() API, but that was changed for what seemed like good reasons.

All of the changes in this comment and this one have now landed in Chromium 88+ (behind the Experimental Web Platform Features flag). I added a section to the explainer that touches on these changes. I'm going to get started modified the spec PRs (HTML/DOM) to incorporate these opt-in's.

[annevk]

It does not unfortunately. Trusted Types doesn't have cross-browser agreement.

[mfreed7]

It does not unfortunately. Trusted Types doesn't have cross-browser agreement.

@annevk, I wonder if there's a way to shape the setInnerHTML() opt-in argument such that today, opting in allows all contained declarative Shadow DOM content, and in the future (when the sanitizer-api is available), not opting in still allows contained declarative content, but with a first pass through the sanitizer?

@clelland after your nice BlinkOn talk today about Permissions Policy and Document Policy, I'm actually thinking this feature belongs squarely in Permissions Policy, not Document Policy. The policy restricts a "powerful" feature that can potentially enable scripts to bypass sanitizers. Would you agree?

[annevk]

@mfreed7 maybe require passing unsafeWithShadowRoot or equivalent and throw otherwise. And in the future we could make not passing anything do the safe thing rather than throw. It's usually doable to migrate from throwing to not throwing. (I don't think Permissions Policy makes sense by the way, as the parent shouldn't be in control as to how the child parses without the child consenting.)

[mfreed7]

@mfreed7 maybe require passing unsafeWithShadowRoot or equivalent and throw otherwise. And in the future we could make not passing anything do the safe thing rather than throw. It's usually doable to migrate from throwing to not throwing. (I don't think Permissions Policy makes sense by the way, as the parent shouldn't be in control as to how the child parses without the child consenting.)

@annevk if I understand you (and I'm thinking I do not), you're just saying to make the existing proposal throw if declarative Shadow DOM content is encountered without the flag provided? I.e.

element.setInnerHTML(contentWithoutDSD,{includeShadowRoots:false}); // No exception
element.setInnerHTML(contentContainingDSD,{includeShadowRoots:false}); // Throws
element.setInnerHTML(contentContainingDSD,{includeShadowRoots:true}); // Parses DSD content, no exception

If that's not what you meant by "passing unsafeWithShadowRoot", can you let me know what you did mean?

Incidentally, I've renamed the setInnerHTML() and DOMParser.parseFromString() opt-in parameters to match the existing one on getInnerHTML(), namely "includeShadowRoots":

const html = element.getInnerHTML({ includeShadowRoots:true });
element.setInnerHTML(html, { includeShadowRoots:true });
(new DOMParser()).parseFromString(html, 'text/html', { includeShadowRoots:true });

It seemed better to make them all parallel, and plural, as there might be more than one shadow root present. LMK if you all are ok with this change.

[annevk]

I meant that in v1 of setInnerHTML() you have to pass unsafe to indicate you opt out of sanitization and otherwise it will throw. So for instance:

elm.setInnerHTML(content, { includeShadowRoots: true, unsafe: false }); // throws
elm.setInnerHTML(content, { includeShadowRoots: false, unsafe: false }); // throws
elm.setInnerHTML(content, { includeShadowRoots: true, unsafe: true }); // parses
elm.setInnerHTML(content, { includeShadowRoots: false, unsafe: true }); // parses

I don't think it should matter what's in content for the purposes of throwing or not throwing. It should do the same as the normal parser as much as possible.

[koto]

@annevk I like the safe-by-default aspect of it and how it enables incorporating sanitizer later. However, it still adds a new DOM XSS sink to the platform, it's just one that is more complicated to call, as it also requires control over the option bag argument. Granted, one of the keys in that bag suggests there's something security-related going on, but existing tools don't know that.

In some aspects it's even worse than innerHTML in that determining if the code causes XSS depends now on two separate values, both of them potentially dynamic. That makes automated analysis and manual review harder. For example:

el.setInnerHTML(componentHTML, config.componentRenderer.params)

To find out whether this is an XSS, we have to inspect the data flow for two different arguments, and changing either in the future has security consequences.

If DOM should add a new XSS sink (and this one looks to be potentially popular as a "better innerHTML"), I think the bar should be higher than the default (non-functional in v1) being safe, and the security controlled by just the object key names.

@mfreed7 how crucial is it to have the setInnerHTML case in v1? Do you expect the declarative shadow DOM to be commonly using this function? Can the other, already existing and unsafe sinks be used by the authors instead of setInnerHTML or does setInnerHTML have a unique behavior that you want to preserve?

[mfreed7]

@mfreed7 how crucial is it to have the setInnerHTML case in v1? Do you expect the declarative shadow DOM to be commonly using this function? Can the other, already existing and unsafe sinks be used by the authors instead of setInnerHTML or does setInnerHTML have a unique behavior that you want to preserve?

Hmm, I feel like we're going a bit in circles here. Recall that the original proposal was this:

document.allowDeclarativeShadowDOM = true;
element.innerHTML = contentIncludingDSD;

That had these pros:

• Avoids the sanitizer bypass, which again, is the entire goal of the "opt-in".
• No additional XSS sinks added.
• Easy to use.

And this con:

• It introduces state on the Document, which generally isn't great.

I don't particularly think this is ergonomic or readily understandable by developers:

element.setInnerHTML(content, { includeShadowRoots: true, unsafe: true });

and as @koto pointed out, it makes the XSS sanitization job more difficult.

I do think we need some innerHTML-like functionality, as this is a common pattern. Forcing all DSD fragment parsing through DOMParser feels quite painful. My own WPT tests, for example, make use of setInnerHTML() 25 times, not including the test itself for setInnerHTML().

Given that this is a new API, purely to avoid the risk of a sanitizer bypass for sanitizers that are unaware of declarative Shadow DOM, how about just this?

element.setInnerHTML(content, {unsafe: false}); // Always throws, regardless of content
element.setInnerHTML(content, {unsafe: true}); // Parses DSD, never throws

This clearly indicates that something about it is "unsafe". And the ability to not parse DSD isn't actually of value even to sanitizers, as closed shadow roots in the parsed output will be invisible to the sanitizer.

@annevk @koto what do you think?

@annevk as for the Permissions Policy, I'm beginning to think we should just skip adding permissions for iframes here at all. Iframes will continue to parse DSD, just as the main document does. Thoughts?

[koto]

My issue is with the introduction of a new sink, a variant of which needs to be unsafe to support DSD case (+ we don't actually have a way of adding a safe sink yet via sanitizer, Trusted Types or both; every new sink is unsafe).

I don't think naming a sink as unsafe is a sufficient bar. That said, unsafeSetInnerHTML(payload, options) is probably better than setInnerHTML(payload, optionsWithUnsafe); at least it's greppable and could be discovered easier. Adding new sinks is not great, as all existing infrastructure, education materials etc. authors use to help them secure their code needs to be updated. For example, in Google there's at least two static analysis frameworks that now need to cover setInnerHtml to stop our engineers from accidentally introducing XSS (and we know by now that scary names simply don't work as a vuln prevention mechanism). So, if one squints, a document state + existing sinks would be a safer choice here.

All that said, security is not the only aspect, and this is not the only way to evaluate the security of this choice either. I'm not the decider here - the DOM editors are.

[mfreed7]

@domenic you made the case against the Document state approach, which I did agree with. But in light of the seemingly more serious security concerns around adding a new unsafe sink above, perhaps this is the lesser of the evils?

document.allowDeclarativeShadowDOM = true;
element.innerHTML = contentIncludingDSD;

It does add state to Document. But it fixes the sanitizer bypass, it avoids adding a new XSS sink, and it's easy to use.

Again, I think it is important to remember that the entire motivation for this "opt-in" mechanism is to avoid the risk that existing sanitizer libraries don't know how to deal with DSD. After that, I'd prefer a world where DSD is just part of HTML, so it's natural to "allow" declarative shadow content anywhere you'd normally parse "normal" HTML. @domenic for example you said the global state was bad because you could create a Document, set the flag, and pass it to a framework that doesn't know how to deal with DSD content. But unless that framework is a sanitizer, that should be ok. That's the same thing as when other new HTML elements are added to the platform, and existing frameworks don't natively "understand" them. If the framework in your example is a sanitizer, it shouldn't be getting its parser document from outside, or it's already broken. Would you agree?

[domenic]

I don't agree, but unfortunately I'm on vacation for the next week. I'll be able to do a point by point reply afterward.

[mfreed7]

I don't agree, but unfortunately I'm on vacation for the next week. I'll be able to do a point by point reply afterward.

Ok, thanks for the heads-up. Enjoy the vacation!

[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.