1444395 - Remove unsafeSetInnerHTML in gcli/util/util.js

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement, P2)

Description

[Johann Hofmann [:johannh]]

The setContents function in util.js is using unsafeSetInnerHTML to avoid our innerHTML sanitizer. We want to get rid of it.

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/shared/gcli/source/lib/gcli/util/util.js#503

This probably entails converting a lot of template files to inline JS. I'll file separate bugs to keep track of those.

Comment 1

[Johann Hofmann [:johannh]]

This could also get resolved by removing gcli, which sounds like the more practical and less painful step.

Comment 2

[Johann Hofmann [:johannh]]

Patrick, we are now at the point where GCLI is literally the last remaining user of unsafeSetInnerHTML. I don't want to pressure you into doing the unshipping work, but would you mind me breaking GCLI by removing the unsafeSetInnerHTML call (and potentially replacing it with a sanitized innerHTML assignment instead)?

https://bugzilla.mozilla.org/show_bug.cgi?id=1429421#c22 talks about disabling the feature in 60 or 61 and removing the code in 61 or 62, I assume it's fine to break and/or disable it now? (I'm happy to disable it before breaking if you feel more comfortable with that).

Thank you for the help!

Comment 3

[Patrick Brosset <:pbro>]

Let me forward this to Sole who is now the contact point in DevTools regarding GCLI.
We do want to remove it, and will, but we want to make sure we do it right, and our plan to disable in 60 and remove in 61 was probably too aggressive.
Sole, you've been gathering details about this project recently, do you have insights to share here?
What do you think our plan should be? I'd be willing to disable it in 61 (like right now, before the code freeze, it's just a pref anyway I believe, and tests still work), and migrating whatever needs to be migrated in 62.

Comment 4

[Johann Hofmann [:johannh]]

Considering that we're only a few days away from merge anyway, we can play it safe and just keep GCLI enabled until 62 comes around and then just break it as soon as possible after merge. There's no inherent security benefit of having unsafeSetInnerHTML removed in 61 vs 62 (all remaining consumers were removed anyway and I'm watching check-ins to make sure nobody is adding any).

Comment 5

[Soledad Penades [:sole] [:spenades]]

Sorry for the delay. We've identified that disabling GCLI now without an alternative for important commands is going to be quite disruptive. 

We've identified that restart is the command we need to absolutely migrate, once we do that we can disable and continue to removing the rest. Evidently this will start happening in 62. Hope that helps.

Comment 6

[Johann Hofmann [:johannh]]

Ok, so, considering that a lot of functionality of GCLI is broken at this point and `restart` doesn't require unsafeSetInnerHTML as far as I can tell, would it be okay if I just broke the remaining functionality (apart from `restart`) in 62?

Comment 7

[Soledad Penades [:sole] [:spenades]]

What do you mean with 'breaking' the functionality? Would you need to disable the tests too?

I'm a bit wary of "just doing it" as we'd need to inform people about the breakage too so they could use alternatives in the meantime, so simply breaking the functionality would not be acceptable.

Comment 8

[Joe Walker [:jwalker]]

We could go through the commands and working out which need unsafeSetInnerHTML and which don't, then removing all command that do?

The return type of the restart command is 'string', so it doesn't use unsafeSetInnerHTML.

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/restart.js#48

On the other hand the return type of the 'cookie' command is 'view', which does use unsafeSetInnerHTML

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/cookie.js#154

I think the list of commands that we would need to remove is roughly:

https://dxr.mozilla.org/mozilla-central/search?q=path%3Agcli+to%3A+view&redirect=false

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #8)
> I think the list of commands that we would need to remove is roughly:
> https://dxr.mozilla.org/mozilla-central/search?q=path%3Agcli+to%3A+view&redirect=false
All except 'security' (which some people have expressed interest about when I sent the intent to unship email) can be removed without any second thoughts.

Comment 10

[Joe Walker [:jwalker]]

We could re-write the security command to output plain text rather than an html view without too much difficulty. If we set the output font to be monospaced, then we could even retain the table.

Comment 11

[Frederik Braun [:freddy]]

FWIW 'security csp' is broken.

Comment 12

[Soledad Penades [:sole] [:spenades]]

Since we removed the UI in Bug 1461970 and the commits stayed in m-c, I tried creating a build without setUnsafeInnerHTML. I removed the usage of the method from devtools, and the method itself from Element.cpp and also from the WebIDL def. The DevTools tests pass and Firefox builds and runs the tests I selected: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88fbe687bdd3f38906539c76924074073fb1ecc

I might have done something wrong as I'm not overly familiar with changing C++ code let alone Element, but I feel we're pretty close to resolving this last usage of the method.

I think it's best if I leave it in your very capable hands, Johann :-)

Thank you for the patience!

Comment 13

[Johann Hofmann [:johannh]]

I can take care of it, thank you so much!

I'll make a removal patch for just the setUnsafeInnerHTML usage in this bug and remove the platform parts in bug 1444394, for posterity.

Cheers!

Comment 14

[Johann Hofmann [:johannh]]

Attachment: Bug 1444395 - Remove unsafeSetInnerHTML in gcli/util/util.js.

We intent to fully remove unsafeSetInnerHTML to be able to prevent any future
vulnerabilities that come from code injection. GCLI is the last remaining user of
this function, which is why we're removing it now. This would entirely break the
GCLI UI if it was not already removed in bug 1461970.

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

Comment 15

[Soledad Penades [:sole] [:spenades]]

Comment on attachment 8981149 [details]
Bug 1444395 - Remove unsafeSetInnerHTML in gcli/util/util.js.

https://reviewboard.mozilla.org/r/247244/#review253460

My tests showed there was no impact to devtools tests when removing the same code you removed, so it looks good to me :-)

Thank you!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/238eb0ed2874
Remove unsafeSetInnerHTML in gcli/util/util.js. r=sole

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/238eb0ed2874