1450315 - Remove document.allowUnsafeHTML

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

Bug 1434155 seems to have removed the last consumer of this, so I think we can get rid of it again!

https://searchfox.org/mozilla-central/search?q=allowUnsafeHTML&case=false&regexp=false&path=

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1450315 - Remove document.allowUnsafeHTML.

This attribute was added in bug 1432966 when we started sanitizing
chrome usage of innerHTML to allow devtools to use custom attributes
in React via dangerouslySetInnerHTML.

Devtools removed its usage of dangerouslySetInnerHTML in bug 1434155, so
document.allowUnsafeHTML is no longer used and can be removed.

Review commit: https://reviewboard.mozilla.org/r/232890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232890/

Comment 2

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6149a5a235088f59608faa4847cf349eaa4955c4

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8964099 [details]
Bug 1450315 - Remove document.allowUnsafeHTML.

https://reviewboard.mozilla.org/r/232890/#review238564

r=me.  Thank you!

::: dom/base/nsContentUtils.cpp:5092
(Diff revision 1)
>  
>    // If this is a chrome-privileged document, create a fragment first, and
>    // sanitize it before insertion.
>    RefPtr<DocumentFragment> fragment;
> -  if (aSanitize != NeverSanitize && !aTargetNode->OwnerDoc()->AllowUnsafeHTML()) {
> +  if (aSanitize != NeverSanitize &&
> +      IsSystemPrincipal(aTargetNode->OwnerDoc()->NodePrincipal())) {

Just aTargetNode->NodePrincipal()

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8964099 [details]
Bug 1450315 - Remove document.allowUnsafeHTML.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232890/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76f66b8cea93
Remove document.allowUnsafeHTML. r=bz

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, we're using this in TB and also JQuery uses .innerHTML = "..." which is in turn used in add-ons :-(
We can replace the use of .innerHTML, but JQuery won't. For example see:
https://github.com/micz/ThunderStats/commit/75452e94cfc5211e10982a15ada9f8769ea3816a

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Jorg K (GMT+1) from comment #6)
> Hmm, we're using this in TB and also JQuery uses .innerHTML = "..." which is
> in turn used in add-ons :-(
> We can replace the use of .innerHTML, but JQuery won't. For example see:
> https://github.com/micz/ThunderStats/commit/
> 75452e94cfc5211e10982a15ada9f8769ea3816a

I'm happy to help fix up Thunderbird itself in bug 1433871 but we do not give any guarantees to old-style privileged add-ons anymore, even if they are still supported in Thunderbird (especially for such a security sensitive matter). Sorry for that.

Comment 8

[Kris Maglione [:kmag]]

Solution: Don't use jQuery in chrome contexts. Especially, do not use jQuery to inject HTML strings into chrome contexts.

You can, of course, get around this by replacing Element.prototype.innerHTML. But that is a terrible idea.

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/76f66b8cea93