1444443 - Remove unsafeSetInnerHTML in test_mutation.html

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement, P3)

Description

[Johann Hofmann [:johannh]]

We're using unsafeSetInnerHTML to inject HTML in the test_mutation.html test file. While that in itself is not dangerous, we're aiming to remove unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the test.

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/accessible/tests/mochitest/events/test_mutation.html#321

Comment 1

[alexander :surkov (:asurkov)]

unsafeSetInnnerHTML was introduced in bug 1432966, Kris, could please answer comment #0?

Comment 2

[Kris Maglione [:kmag]]

I'm not the right person to rewrite the test.

Assuming we want to remove test usage (rather than just adding CrashIfNotInAutomation() to unsafeSetInnerHTML), this will need to be rewritten to use DOM creation APIs.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #2)
> I'm not the right person to rewrite the test.

while it would resolve the bug, but I didn't mean that :)

> Assuming we want to remove test usage (rather than just adding
> CrashIfNotInAutomation() to unsafeSetInnerHTML), this will need to be
> rewritten to use DOM creation APIs.

If you could provide any pointers or give any examples, then it'd helpful, and the bug could be turned into mentored one. Honestly I don't have enough context on why innerHTML cannot be used in tests any longer.

Comment 4

[Kris Maglione [:kmag]]

(In reply to alexander :surkov from comment #3)
> If you could provide any pointers or give any examples, then it'd helpful,
> and the bug could be turned into mentored one.

I know someone wrote a tool to automatically rewrite simple innerHTML assignments into DOM calls, but I don't remember who. Maybe Johann does?

> Honestly I don't have enough context on why innerHTML cannot be used in tests any longer.

Assignments to innerHTML in chrome documents are now automatically sanitized, and the strings that these tests try to assign are no longer useful after sanitization.

When we turned on the automatic sanitization, we added unsafeSetInnerHTML as a temporary escape hatch, so that we could ship it more quickly to address a critical security vulnerability. But we want to remove that escape hatch, which means fixing all legacy callers that relied on unsanitized innerHTML assignments.

Comment 5

[Johann Hofmann [:johannh]]

Freddy wrote https://freddyb.github.io/html2dom/#, which is quite a nice tool, but I don't think it's really necessary in this simple case.

Sorry for causing confusion here, this bug is part of a larger cleanup tracked in bug 1444394. I did not intend to make anyone from your team work on this, in fact I think I can just take this one myself.

Comment 6

[Johann Hofmann [:johannh]]

Prathiksha offered to take this. Thanks!

Comment 7

[:prathiksha]

Attachment: Bug 1444443 - Remove unsafeSetInnerHTML in test_mutation.html.

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

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8962711 [details]
Bug 1444443 - Remove unsafeSetInnerHTML in test_mutation.html.

https://reviewboard.mozilla.org/r/231572/#review237426

This looks good, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a889ab29ddd4
Remove unsafeSetInnerHTML in test_mutation.html. r=johannh

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a889ab29ddd4