Remove unsafeSetInnerHTML in gcli/util/util.js

RESOLVED FIXED in Firefox 62

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

60 Branch
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Depends on: 1444398
(Assignee)

Updated

a year ago
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
(Assignee)

Updated

a year ago
Depends on: 1444400
(Assignee)

Updated

a year ago
Depends on: 1444402
(Assignee)

Updated

a year ago
Depends on: 1444405
(Assignee)

Updated

a year ago
Depends on: 1444428
(Assignee)

Updated

a year ago
No longer depends on: 1444428
(Assignee)

Comment 1

a year ago
This could also get resolved by removing gcli, which sounds like the more practical and less painful step.
Depends on: gcli-removal
(Assignee)

Comment 2

a year ago
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!
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset) → needinfo?(spenades)
(Assignee)

Comment 4

a year ago
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).
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.
Flags: needinfo?(spenades)
(Assignee)

Comment 6

a year ago
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?
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.
Flags: needinfo?(jhofmann)
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
(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.
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.
FWIW 'security csp' is broken.
(Assignee)

Updated

11 months ago
Flags: needinfo?(jhofmann)
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!
Flags: needinfo?(jhofmann)
(Assignee)

Comment 13

11 months ago
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!
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
Comment hidden (mozreview-request)

Comment 15

11 months ago
mozreview-review
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!
Attachment #8981149 - Flags: review?(spenades) → review+

Comment 16

11 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/238eb0ed2874
Remove unsafeSetInnerHTML in gcli/util/util.js. r=sole

Comment 17

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/238eb0ed2874
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

10 months ago
Product: Firefox → DevTools

Updated

7 months ago
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.