setHTML() and declarative shadow roots

[annevk]

setHTML() which is being drafted in https://wicg.github.io/sanitizer-api/ is planned to be folded into HTML. It would be good to sort out the declarative shadow root interaction ahead of time so we don't run into any surprises.

By default setHTML() will sanitize and remove script elements. Is that problematic?

There's a number of further design questions here:

• Should they be opt-in given that this is a new API?
• Assuming yes, does the opt-in take the form of a boolean that allows them to be used or a boolean preventing them from being sanitized?

I personally kinda like the idea that they are parsed, but by default end up getting removed. But that would make this the only place where removal is possible, which is a bit suspect.

cc @mfreed7 @mozfreddyb @evilpie @otherdaniel @smaug---- @rniwa

[annevk]

I thought about this some more and also chatted with @mozfreddyb about it:

• We need an opt-in for declarative shadow roots on the parser side as setHTML() behaving differently from innerHTML in that respect would be surprising. This would be its own option that takes a boolean: allowShadowRoots.
• We need an option to opt out of the sanitizer, presumably something like sanitizer: "unsafe-none". This opens you up to XSS, but is a very useful tool when you know what you're doing.
• If the sanitizer is enabled and is configured to allow custom elements, it will need to traverse into their corresponding shadow roots, if any, and modify those trees in the same manner. (When custom elements are not allowed they will get removed as well as their corresponding shadow trees.)
• In particular, we don't see a need to allow custom elements but disallow declarative shadow roots from a sanitizer perspective. (You can still get there by not allowing them to be parsed in the first place.)

[mozfreddyb]

I believe there's value in having an opt-out: People want to invoke the fragment parser with new and exciting options (e.g., allowShadowRoots to enable declarative shadow DOM) and there currently is none. We've specified setHTML() to allow an options bag (rather than just a sanitizer) as a second argument for exactly this reason.

We've gone through great lengths to make sure that you can never construct a Sanitizer object that allows XSS and related badness (as per our thread model, not excluding DOM Clobbering, Script gadget attacks. terms & conditions apply). I would like to keep it that way. A constructed Sanitizer object should be guaranteed not to allow XSS.

imho, allowing to invoke setHTML with an explicit unsafe-none complements that idea: You can skip sanitizing, but you can't create an insecure Sanitizer object.

My only remaining concern is with the static analysis perspective. Until now, I would have set "use setHTML everywhere. Allow setHTML everywhere and you're guaranteed to be safe". Now that is "Allow setHTML but always check that it does not pass an object that has a sanitizer key with a value of "unsafe-none". That makes detection a bit more annoying (but not impossible!), as it has to trace parameters back to variable definition and such..

I suppose that's unavoidable.

Also adding @otherdaniel for visibility.

[mfreed7]

setHTML() which is being drafted in wicg.github.io/sanitizer-api is planned to be folded into HTML. It would be good to sort out the declarative shadow root interaction ahead of time so we don't run into any surprises.

So to start, I'm supportive of this proposal (allowing setHTML() to parse declarative shadow DOM). Thanks for opening this issue.

By default setHTML() will sanitize and remove script elements. Is that problematic?

I think <script>s should be treated the same within or outside DSD content. So whatever the normal Sanitizer policy is for scripts should apply the same way. DSD content is just HTML.

There's a number of further design questions here:

• Should they be opt-in given that this is a new API?

Wholeheartedly declarative shadow DOM should be opt-out. The only reason it needed to be opt-in for existing parser entrypoints is the possibility of badly-configured JS sanitizers. Given that the built-in Sanitizer should do the right thing, I see no reason why DSD should be treated any differently than any other HTML. Of course, unsafe content inside a declarative shadow root should be properly sanitized. But the shadow root itself isn't inherently dangerous.

• Assuming yes, does the opt-in take the form of a boolean that allows them to be used or a boolean preventing them from being sanitized?

I'm agnostic, as long (see above) as the default is to allow them.

[annevk]

@mfreed7 my first comment has a summary of what I think we ought to be doing here. Not sure why you skipped that one. 😊

[mfreed7]

@mfreed7 my first comment has a summary of what I think we ought to be doing here. Not sure why you skipped that one. 😊

I thought my reply quotes your first comment - does it not? Sorry, what did I miss?

[annevk]

Sorry, that's OP. I should have said first reply.

[mfreed7]

Sorry, that's OP. I should have said first reply.

Ahh ok. Sorry.

• We need an opt-in for declarative shadow roots on the parser side as setHTML() behaving differently from innerHTML in that respect would be surprising. This would be its own option that takes a boolean: allowShadowRoots.

My response to this is in the paragraph starting "Wholeheartedly declarative shadow DOM should be opt-out.".

• We need an option to opt out of the sanitizer, presumably something like sanitizer: "unsafe-none". This opens you up to XSS, but is a very useful tool when you know what you're doing.

This was discussed by @koto on whatwg/dom#912 (comment), with the viewpoint that the method name should indicate "unsafe" rather than one of the option values in the options bag. The rationale is that it's easier to grep for the unsafe method name than it is to follow argument values around.

• If the sanitizer is enabled and is configured to allow custom elements, it will need to traverse into their corresponding shadow roots, if any, and modify those trees in the same manner. (When custom elements are not allowed they will get removed as well as their corresponding shadow trees.)

Agree.

• In particular, we don't see a need to allow custom elements but disallow declarative shadow roots from a sanitizer perspective. (You can still get there by not allowing them to be parsed in the first place.)

Agree.

[annevk]

@mfreed7 interesting, depending on folks obfuscating invocation it might not be easier to grep, but if you can assume good faith I suppose that would work. We'd end up with quite a lot of methods though. I take it you agree we need some kind of unsafe method or are all known declarative shadow tree use cases served with sanitizing?

[annevk]

This was resolved by #9538.