Closed
Bug 1444395
Opened 6 years ago
Closed 6 years ago
Remove unsafeSetInnerHTML in gcli/util/util.js
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, enhancement, P2)
Tracking
(firefox-esr60 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)
RESOLVED
FIXED
Firefox 62
People
(Reporter: johannh, Assigned: johannh)
References
Details
Attachments
(1 file)
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•6 years ago
|
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Assignee | ||
Comment 1•6 years 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•6 years 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)
Comment 3•6 years ago
|
||
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)
Priority: -- → P2
Assignee | ||
Comment 4•6 years 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).
Comment 5•6 years ago
|
||
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•6 years 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?
Comment 7•6 years ago
|
||
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)
Comment 8•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
FWIW 'security csp' is broken.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jhofmann)
Comment 12•6 years ago
|
||
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•6 years 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•6 years 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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/238eb0ed2874
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•6 years ago
|
status-firefox61:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•