Override `.innerHTML`, or define `safelySetInnerHTML(...)`?

[mikewest]

It might be difficult to audit sink assignments (e.g. el.innerHTML = whatever;), as bad usage looks exactly like good usage. The auditor needs to have knowledge of the context ("Is the restrictive flag set?") in order to know whether or not a given usage is safe.

Perhaps introducing a safe variant of the sink would be better: el.safelySetInnerHTML(whatever) or something similar.

[mozfreddyb]

What's the goal here auditing sinks or sources? How important is it to make this work seamlessly for existing, secure applications?

There are two roads I can envision, the first is to expose a context-sensitive escaper built upon streaming HTML that allows something like

safehtml`<a href="${url}">${text}</a>`

The other is to globally overwrite innerHTML with a thing that silently(?) sanitizes the input and removes all active code, given some sane defaults (yep, this is the hard part). The opt-out would be unsafelyAssignInnerHTML() that will reduce the amounts of code to be audited.

[koto]

Ideally, we should be able to audit sources, because that one scales better. However, refactoring existing applications will probably lead to quickly replacing any .innerHTML assignment with the types-based equivalent.

I'm fine if:

1. innerHTML accepts a TrustedHTML always,
2. There's a variant that escapes (or otherwise silently sanitizes using a hard-to-agree-on mechanism),

I'd rather not expose the unsafe variant of sink setters, as it promotes bad practice of putting validation close to the sink. We can't prevent developers from using .innerHTML = TrustedHTML.unsafelyCreate('foo'), but we can discourage other patterns like .unsafelySetInnerHTML('foo') by simply not adding them, and only exposing safe ones.

[koto]

But back to the original question though; Disabling innerHTML setter completely in the enforcement mode might be problematic because of templating frameworks that bind to element properties.

[mikesamuel]

Can we treat the below as separable questions?

• Should a FeaturePolicy flag opt-in cause innerHTML to only accept TrustedHTML
• Should a FeaturePolicy flag opt-out of innerHTML being present
• Should HTMLElement be extended with a safeInnerHTML that only accepts TrustedHTML

[koto]

We can, yes. But we might end up creating an overtly complex system (even when considering this single sink) I'd try to avoid. Should we need the answer to all of the above to be 'yes'?

My personal answers would be

Should a FeaturePolicy flag opt-in cause innerHTML to only accept TrustedHTML

yes

Should a FeaturePolicy flag opt-out of innerHTML being present

no, this will likely be an adoption killer and there are other ways of prohibiting innerHTML usage (polyfill, linter, ...)

Should HTMLElement be extended with a safeInnerHTML that only accepts TrustedHTML

maybe.

[mozfreddyb]

You could replace "innerHTML setter" with the fragment parsing algorithm.

This will magically catch innerHTML, outerHTML, insertAdjacentHTML, DOMParser, createContextualFragment.

[koto]

You could replace "innerHTML setter" with the fragment parsing algorithm.

Is it possible to hook into that algorithm from user land JS code? If not, the polyfill would need to override all of its callsites separately.

[mikesamuel]

@koto, I think <template> parses its body as a fragment, so

const myTemplate = document.createElement('template')
myTemplate.innerHTML = htmlParse
myTemplate.content  // A document fragment that won't have loaded images or executed scripts

https://html.spec.whatwg.org/multipage/scripting.html#dom-template-content

[mozfreddyb]

What @mikesamuel said, and you'd have to overwrite the HTMLElement's prototype.

[koto]

Take a look at the new design, with policies: https://github.com/WICG/trusted-types#policies. With it, we're trying to convince the developers there's a better way, and they should focus on the sources. Direct sink usage with a string is still allowed, but can go through a defined policy. 3e47991e90d47ae64fe2ae1413221272998ea039

[koto]

I think the current approach is to reuse the original sink names, with optionally the nw contract (i.e. they accept types). Closing this one then.