1444394 - [tracking] Remove Element::UnsafeSetInnerHTML

Status: RESOLVED FIXED

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

Description

[Johann Hofmann [:johannh]]

Bug 1432966 introduced the unsafeSetInnerHTML function to avoid dealing with all the cases where our sanitizer would have broken innerHTML functionality.

We should track removing all those calls so that we can unship that function again and make it impossible to bypass the sanitizer.

Comment 1

[Frederik Braun [:freddy]]

Element::UnsafeSetInnerHTML also introduced the parameter |bool aNeverSanitize| in SetInnerHTMLInternal().
Please remember to remove this parameter as well.

Source: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/dom/base/FragmentOrElement.h#271-272

Comment 2

[Frederik Braun [:freddy]]

and aSanitize NeverSanitize etc.  :P
https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/dom/base/nsContentUtils.cpp#5062,5190

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Bug 1444394 - Remove Element::UnsafeSetInnerHTML.

The last remaining user is already turned off and being removed
in bug 1444395 so that we can finally remove this unsafe code and
sleep a little better knowing that XSS through markup injections
will be impossible in chrome contexts.

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

Comment 4

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=304a76cc05afb9aa3db2b64b7ce289a160ec9441

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8981244 [details]
Bug 1444394 - Remove Element::UnsafeSetInnerHTML.

https://reviewboard.mozilla.org/r/247324/#review253386

\o/

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8981244 [details]
Bug 1444394 - Remove Element::UnsafeSetInnerHTML.

https://reviewboard.mozilla.org/r/247324/#review254160

::: dom/base/nsContentUtils.h
(Diff revision 1)
>     *
>     * @param aContextNode the node which is used to resolve namespaces
>     * @param aFragment the string which is parsed to a DocumentFragment
>     * @param aReturn the resulting fragment
>     * @param aPreventScriptExecution whether to mark scripts as already started
> -   * @param aSanitize whether the fragment should be sanitized prior to

We should probably document that the fragment will be sanitized if aContextNode is system, right?

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8981244 [details]
Bug 1444394 - Remove Element::UnsafeSetInnerHTML.

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

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8981244 [details]
Bug 1444394 - Remove Element::UnsafeSetInnerHTML.

https://reviewboard.mozilla.org/r/247324/#review254160

> We should probably document that the fragment will be sanitized if aContextNode is system, right?

Good idea, thanks!!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cab0dc8b7d9
Remove Element::UnsafeSetInnerHTML. r=bz,kmag

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4cab0dc8b7d9