Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mfreed7 opened this issue Oct 30, 2020 · 35 comments
Labels
topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 30, 2020

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
Copy link

inoas commented Oct 31, 2020

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
Copy link

prlbr commented Oct 31, 2020

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"?

@annevk annevk added the topic: shadow Relates to shadow trees (as defined in DOM) label Nov 2, 2020
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 4, 2020
This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
@whatwg whatwg deleted a comment from nmonmontmon Nov 5, 2020
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 5, 2020
This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 5, 2020
This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 5, 2020
This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824591}
@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 5, 2020

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 5, 2020
This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824591}
@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 5, 2020

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
Copy link
Member

domenic commented Nov 5, 2020

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
Copy link
Member

domenic commented Nov 5, 2020

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
Copy link
Contributor Author

mfreed7 commented Nov 6, 2020

@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
Copy link
Member

domenic commented Nov 6, 2020

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
Copy link
Contributor Author

mfreed7 commented Nov 6, 2020

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
Copy link

clelland commented Nov 6, 2020

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
Copy link
Contributor Author

mfreed7 commented Nov 6, 2020

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
Copy link
Member

annevk commented Nov 6, 2020

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
Copy link
Contributor Author

mfreed7 commented Nov 6, 2020

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
Copy link
Member

domenic commented Nov 6, 2020

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
Copy link
Member

annevk commented Nov 6, 2020

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

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 7, 2020
…arsing to be opt-in, a=testonly

Automatic update from web-platform-tests
Change declarative Shadow DOM fragment parsing to be opt-in

This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824591}

--

wpt-commits: b13461b9a46b46eb1b092a58bde2b10418e7a73d
wpt-pr: 26398
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Nov 10, 2020
…arsing to be opt-in, a=testonly

Automatic update from web-platform-tests
Change declarative Shadow DOM fragment parsing to be opt-in

This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824591}

--

wpt-commits: b13461b9a46b46eb1b092a58bde2b10418e7a73d
wpt-pr: 26398
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 11, 2020
The issue thread [1] has had more discussion, after the initial draft
of declarative Shadow DOM opt-in landed [2]. This CL implements those
bits of feedback. In particular:
 - There is no public allowDeclarativeShadowDom state available on
   Document or DocumentFragment.
 - All APIs use call parameters to avoid state, with the exception
   of DOMParser.
 - innerHTML no longer supports Declarative Shadow DOM.
 - A new setInnerHTML() function allows opt-in access to DSD.
 - Several of the more obscure APIs do not have an opt-in for
   declarative Shadow DOM, such as XHR, createContextualFragment, and
   document.write.
 - The sandbox flag has been removed from iframes completely. The new
   plan is to use DocumentPolicy to enable declarative Shadow DOM for
   iframes. For now, iframes do not support declarative Shadow DOM.
 - allowDeclarativeShadowDOM has become allowShadowRoot.

[1] whatwg/dom#912 (comment)
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2513525

Bug: 1042130

Change-Id: I3a2becf2a113cc8647b29077d2efea1c990d4547
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 11, 2020
The issue thread [1] has had more discussion, after the initial draft
of declarative Shadow DOM opt-in landed [2]. This CL implements those
bits of feedback. In particular:
 - There is no public allowDeclarativeShadowDom state available on
   Document or DocumentFragment.
 - All APIs use call parameters to avoid state, with the exception
   of DOMParser.
 - innerHTML no longer supports Declarative Shadow DOM.
 - A new setInnerHTML() function allows opt-in access to DSD.
 - Several of the more obscure APIs do not have an opt-in for
   declarative Shadow DOM, such as XHR, createContextualFragment, and
   document.write.
 - The sandbox flag has been removed from iframes completely. The new
   plan is to use DocumentPolicy to enable declarative Shadow DOM for
   iframes. For now, iframes always support declarative Shadow DOM.
 - 'allowDeclarativeShadowDOM' has become 'allowShadowRoot'.

[1] whatwg/dom#912 (comment)
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2513525

Bug: 1042130

Change-Id: I3a2becf2a113cc8647b29077d2efea1c990d4547
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 11, 2020
The issue thread [1] has had more discussion, after the initial draft
of declarative Shadow DOM opt-in landed [2]. This CL implements those
bits of feedback. In particular:
 - There is no public allowDeclarativeShadowDom state available on
   Document or DocumentFragment.
 - All APIs use call parameters to avoid state, with the exception
   of DOMParser.
 - innerHTML no longer supports Declarative Shadow DOM.
 - A new setInnerHTML() function allows opt-in access to DSD.
 - Several of the more obscure APIs do not have an opt-in for
   declarative Shadow DOM, such as XHR, createContextualFragment, and
   document.write.
 - The sandbox flag has been removed from iframes completely. The new
   plan is to use DocumentPolicy to enable declarative Shadow DOM for
   iframes. For now, iframes always support declarative Shadow DOM.
 - 'allowDeclarativeShadowDOM' has become 'allowShadowRoot'.

[1] whatwg/dom#912 (comment)
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2513525

Bug: 1042130

Change-Id: I3a2becf2a113cc8647b29077d2efea1c990d4547
Cq-Do-Not-Cancel-Tryjobs: true
@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 23, 2020

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.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 24, 2020
The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 24, 2020
The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 24, 2020
The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}
pull bot pushed a commit to Alan-love/chromium that referenced this issue Nov 24, 2020
The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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


[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}
@mfreed7
Copy link
Contributor Author

mfreed7 commented Nov 24, 2020

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

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 1, 2020
Automatic update from web-platform-tests
Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}

--

wpt-commits: 60d87a5d19f5cf033f96b26f9597b32ad2732792
wpt-pr: 26631
sylvestre pushed a commit to sylvestre/gecko-dev that referenced this issue Dec 1, 2020
Automatic update from web-platform-tests
Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}

--

wpt-commits: 60d87a5d19f5cf033f96b26f9597b32ad2732792
wpt-pr: 26631
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 3, 2020
Automatic update from web-platform-tests
Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreedchromium.org>
Commit-Queue: Kouhei Ueno <kouheichromium.org>
Reviewed-by: Kouhei Ueno <kouheichromium.org>
Cr-Commit-Position: refs/heads/master{#830501}

--

wpt-commits: 60d87a5d19f5cf033f96b26f9597b32ad2732792
wpt-pr: 26631

UltraBlame original commit: 1046b32a6c9d31383f782745ed88a9d1fd4bc71e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 3, 2020
Automatic update from web-platform-tests
Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreedchromium.org>
Commit-Queue: Kouhei Ueno <kouheichromium.org>
Reviewed-by: Kouhei Ueno <kouheichromium.org>
Cr-Commit-Position: refs/heads/master{#830501}

--

wpt-commits: 60d87a5d19f5cf033f96b26f9597b32ad2732792
wpt-pr: 26631

UltraBlame original commit: 1046b32a6c9d31383f782745ed88a9d1fd4bc71e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 3, 2020
Automatic update from web-platform-tests
Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreedchromium.org>
Commit-Queue: Kouhei Ueno <kouheichromium.org>
Reviewed-by: Kouhei Ueno <kouheichromium.org>
Cr-Commit-Position: refs/heads/master{#830501}

--

wpt-commits: 60d87a5d19f5cf033f96b26f9597b32ad2732792
wpt-pr: 26631

UltraBlame original commit: 1046b32a6c9d31383f782745ed88a9d1fd4bc71e
@ByteEater-pl
Copy link

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.

sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Dec 10, 2020
Automatic update from web-platform-tests
Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}

--

wpt-commits: 60d87a5d19f5cf033f96b26f9597b32ad2732792
wpt-pr: 26631
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL implements most of the suggestions from [1], which effectively
block declarative Shadow DOM from being used by any fragment parser
entry point, unless an explicit opt-in is toggled.

The opt-ins include:
 - DOMParser.allowDeclarativeShadowDom = true;
 - HTMLTemplateElement.allowDeclarativeShadowDom = true;
 - XMLHttpRequest.allowDeclarativeShadowDom = true;
 - DocumentFragment.allowDeclarativeShadowDom = true;
 - Document.allowDeclarativeShadowDom = true; // For innerHTML
 - A new <iframe> sandbox flag: allow-declarative-shadow-dom

This mitigates the potential client-side XSS sanitizer bypass detailed
in the explainer and at [1]. Assuming these changes are functional,
and mitigate the issue, this new behavior will be folded into the
spec PRs at [2] and [3]. But given the security implications of the
existing code, I'd like to get this landed first.

[1] whatwg/dom#912 (comment)
[2] whatwg/html#5465
[3] whatwg/dom#892

Bug: 1042130
Change-Id: I088f28f63078a0d26e354a4442494c0132b47ffc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513525
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824591}
GitOrigin-RevId: 83080e44d00b0a9321b54bcca7bbe0eb2f9d5e43
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
The issue thread [1] has had more discussion, after the initial draft
of declarative Shadow DOM opt-in landed [2]. This CL implements those
bits of feedback. In particular:
 - There is no public allowDeclarativeShadowDom state available on
   Document or DocumentFragment.
 - All APIs use call parameters to avoid state, with the exception
   of DOMParser.
 - innerHTML no longer supports Declarative Shadow DOM.
 - A new setInnerHTML() function allows opt-in access to DSD.
 - Several of the more obscure APIs do not have an opt-in for
   declarative Shadow DOM, such as XHR, createContextualFragment, and
   document.write.
 - The sandbox flag has been removed from iframes completely. The new
   plan is to use DocumentPolicy to enable declarative Shadow DOM for
   iframes. For now, iframes always support declarative Shadow DOM.
 - 'allowDeclarativeShadowDOM' has become 'allowShadowRoot'.

[1] whatwg/dom#912 (comment)
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2513525

Bug: 1042130

Change-Id: I3a2becf2a113cc8647b29077d2efea1c990d4547
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2530222
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826643}
GitOrigin-RevId: d5bf4401a47f04cbfbcca5d7fb4be5f99fa41e19
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Per more feedback [1] on the issue thread, it would be preferred
to remove the state from DOMParser and add it instead to the
parseFromString method.

[1] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: I316c008b80436bdacf7d0548e4ccd891a5c15411
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533848
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826693}
GitOrigin-RevId: f5504c3fcaea2608e33fa834a243905c869621dc
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
The conversation [1] about the recent changes to setInnerHTML have led
to the conclusion [2] that perhaps we shouldn't add a new XSS sink
method at all. That would "fix" the declarative Shadow DOM problem, but
would create a new sink that all security libraries would need to
know about and handle. Seems like not a good trade.

In the meantime, a polyfill can stand in for setInnerHTML:

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

[1] whatwg/dom#912
[2] whatwg/dom#912 (comment)

Bug: 1042130
Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}
GitOrigin-RevId: af023967d0ff024b3008265de09a68d5041f919c
@annevk annevk closed this as completed in ca756e0 Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: shadow Relates to shadow trees (as defined in DOM)
Development

No branches or pull requests

8 participants