Closed
Bug 1454045
Opened 6 years ago
Closed 6 years ago
Downgrade `openTrustedLinkIn` for DevTools where pages don't need special powers
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox62 fixed)
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jryans, Assigned: abhinav.koppula, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file)
As far as I know, DevTools doesn't open links to pages that need special access. Given that and the security reasoning from Christoph Kerschbaumer [:ckerschb] in bug 1453423 comment 26 (copied below), I think we should change `openTrustedLinkIn` calls to the lower priv `openWebLinkIn`. --- (In reply to Julian Descottes [:jdescottes][:julian] from comment #25) > For a link that we can trust (e.g. hardcoded, goes to a mozilla controlled > website) should we: > 1/ default to use openTrustedLinkIn, even if the page doesn't need any > special privilege? (maybe there are underlying benefits with > openTrustedLinkInto for regular webpages?) Ideally we should follow the principle of least privilege. In other words, if there is no need for any special privileges then ultimately it is better to rely on OpenWebLinkIn() wherever we can. > 2/ default to openWebLinkIn and only upgrade to openTrustedLinkIn when this > is really needed (eg. :jkt mentioned SUMO pages using priv. APIs)? To answer your question, (2) should be the default :-) > Also, is the target website important? In this bug we have MDN and > Discourse, should they be treated equally as long as the link is hardcoded? > Or should we trust MDN and not Discourse? To be honest, I am not entirely sure. I would imagine we can trust them equally, but the thing I am not sure about is, if Discourse has any special powers that I am not aware of. It's a good thing that you reached out again and obviously using openWebLinkIn() is the better choice if there is no need to load a trusted link.
Assignee | ||
Comment 1•6 years ago
|
||
@Ryan, for fixing this issue, should we change all 'openTrustedLinkIn' calls in devtools to 'openWebLinkIn' ? Is this what the bug is asking for?
Flags: needinfo?(jryans)
Reporter | ||
Comment 2•6 years ago
|
||
Let's break down each case to be sure. Looking at https://searchfox.org/mozilla-central/search?q=openTrustedLinkIn&case=true®exp=false&path=devtools, we have: devtools/client/inspector/rules/views/text-property-editor.js 301 browserWin.openTrustedLinkIn(target.href, "tab"); This seems to be a link from web content, please change to "Web". devtools/client/menus.js 151 window.openTrustedLinkIn("https://addons.mozilla.org/firefox/collections/mozilla/webdeveloper/", "tab"); This page shouldn't need privs, please change to "Web". devtools/client/scratchpad/scratchpad.js 1985 this.browserWindow.openTrustedLinkIn(url, "tab"); This page shouldn't need privs, please change to "Web". devtools/client/shared/AppCacheUtils.jsm 292 win.openTrustedLinkIn(url, "tab"); This links to an about page in the browser, leave as-is. devtools/client/shared/link.js 54 if (top && typeof top.openTrustedLinkIn === "function") { 55 top.openTrustedLinkIn(url, "tab", options); This exposes "Trusted" for others to use, leave as-is. devtools/client/aboutdebugging/test/browser_page_not_found.js 23 window.openTrustedLinkIn("about:debugging#invalid-hash", "current"); This links to an about page in the browser, leave as-is. devtools/client/webconsole/test/mochitest/head.js 499 let oldOpenTrustedLinkIn = window.openTrustedLinkIn; 503 window.openWebLinkIn = window.openTrustedLinkIn = function(link, where) { 504 window.openTrustedLinkIn = oldOpenTrustedLinkIn; 517 // Declare a timeout Promise that we can use to make sure openTrustedLinkIn or 522 window.openTrustedLinkIn = oldOpenTrustedLinkIn; This is testing the link functionality, leave as-is.
Flags: needinfo?(jryans)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
Hi Ryan, I have created a review request for this issue and have also pushed to TRY here https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b6a05c1fc18b2fe9c1879f9ccafe86db1459db
Reporter | ||
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8969349 [details] Bug 1454045 - Downgrade `openTrustedLinkIn` for DevTools where pages don't need special powers. https://reviewboard.mozilla.org/r/238086/#review243824 Thanks, this looks great to me! :)
Attachment #8969349 -
Flags: review?(jryans) → review+
Assignee | ||
Updated•6 years ago
|
Pushed by rgurzau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0ffcaeada9fc Downgrade `openTrustedLinkIn` for DevTools where pages don't need special powers. r=jryans
Keywords: checkin-needed
Comment 7•6 years ago
|
||
Backed out for devtools failures on browser_rules_edit-value-after-name_04.js backout: https://hg.mozilla.org/integration/autoland/rev/41d1450450f7 push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0ffcaeada9fc28141aacc8832888246b1d55040e failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174784277&repo=autoland&lineNumber=2481 [task 2018-04-20T14:46:33.551Z] 14:46:33 INFO - Console message: Security Error: Content at moz-nullprincipal:{30900eb8-7473-44e1-9e5a-1fb94e47073c} may not load or link to chrome://global/skin/icons/warning-64.png. [task 2018-04-20T14:46:33.551Z] 14:46:33 INFO - Console message: [JavaScript Error: "NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.loadURIWithOptions]"] [task 2018-04-20T14:46:33.552Z] 14:46:33 INFO - _loadURI@chrome://browser/content/browser.js:1061:7 [task 2018-04-20T14:46:33.552Z] 14:46:33 INFO - loadURI@chrome://browser/content/tabbrowser.xml:2266:13 [task 2018-04-20T14:46:33.552Z] 14:46:33 INFO - addTab@chrome://browser/content/tabbrowser.js:2440:9 [task 2018-04-20T14:46:33.553Z] 14:46:33 INFO - loadOneTab@chrome://browser/content/tabbrowser.js:1411:15 [task 2018-04-20T14:46:33.553Z] 14:46:33 INFO - openLinkIn@chrome://browser/content/utilityOverlay.js:536:26 [task 2018-04-20T14:46:33.557Z] 14:46:33 INFO - openUILinkIn@chrome://browser/content/utilityOverlay.js:273:3 [task 2018-04-20T14:46:33.558Z] 14:46:33 INFO - openWebLinkIn@chrome://browser/content/utilityOverlay.js:224:3 [task 2018-04-20T14:46:33.558Z] 14:46:33 INFO - _create/<@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/rules/views/text-property-editor.js:301:11 [task 2018-04-20T14:46:33.559Z] 14:46:33 INFO - update@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/rules/views/text-property-editor.js:566:7 [task 2018-04-20T14:46:33.560Z] 14:46:33 INFO - updateEditor@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/rules/models/text-property.js:60:7 [task 2018-04-20T14:46:33.561Z] 14:46:33 INFO - setName@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/rules/models/text-property.js:152:5 [task 2018-04-20T14:46:33.562Z] 14:46:33 INFO - _onNameDone@resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/rules/views/text-property-editor.js:833:7 [task 2018-04-20T14:46:33.563Z] 14:46:33 INFO - _apply@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/inplace-editor.js:972:14 [task 2018-04-20T14:46:33.564Z] 14:46:33 INFO - _onBlur@resource://devtools/shared/base-loader.js -> resource://devtools/client/shared/inplace-editor.js:999:7 [task 2018-04-20T14:46:33.565Z] 14:46:33 INFO - synthesizeMouseAtPoint@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:468:7 [task 2018-04-20T14:46:33.566Z] 14:46:33 INFO - synthesizeMouse@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:405:10 [task 2018-04-20T14:46:33.567Z] 14:46:33 INFO - @chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_edit-value-after-name_04.js:46:3 [task 2018-04-20T14:46:33.569Z] 14:46:33 INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1076:21 [task 2018-04-20T14:46:33.570Z] 14:46:33 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:326:42 [task 2018-04-20T14:46:33.571Z] 14:46:33 INFO - TaskImpl@resource://gre/modules/Task.jsm:275:3 [task 2018-04-20T14:46:33.573Z] 14:46:33 INFO - asyncFunction@resource://gre/modules/Task.jsm:247:14 [task 2018-04-20T14:46:33.574Z] 14:46:33 INFO - Task_spawn@resource://gre/modules/Task.jsm:161:12 [task 2018-04-20T14:46:33.575Z] 14:46:33 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:1067:9 [task 2018-04-20T14:46:33.577Z] 14:46:33 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:967:9 [task 2018-04-20T14:46:33.577Z] 14:46:33 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59 [task 2018-04-20T14:46:33.579Z] 14:46:33 INFO - [task 2018-04-20T14:46:33.581Z] 14:46:33 INFO - wait for the property value to be updated [task 2018-04-20T14:46:33.581Z] 14:46:33 INFO - wait for the image to be open in a new tab [task 2018-04-20T14:46:33.584Z] 14:46:33 INFO - Buffered messages finished [task 2018-04-20T14:46:33.585Z] 14:46:33 INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_edit-value-after-name_04.js | Test timed out -
Flags: needinfo?(jryans)
Reporter | ||
Comment 8•6 years ago
|
||
I think it's related to the chrome URI image in that test. I suggested to Abhinav on IRC that they replace it with http://example.com/browser/devtools/client/inspector/rules/test/doc_test_image.png.
Flags: needinfo?(jryans)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
Hi Ryan, I have pushed to TRY - can you check if the failures are not related? https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9dfa334e4f2ede8475f927732f6fa18623a3329
Flags: needinfo?(jryans)
Comment 11•6 years ago
|
||
Test devtools/startup/tests/browser/browser_shim_disable_devtools.js is a known intermittent on debug platforms, and not involving any of the methods used here. This should be safe to checkin.
Flags: needinfo?(jryans)
Keywords: checkin-needed
Comment 12•6 years ago
|
||
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/452b375b0e40 Downgrade `openTrustedLinkIn` for DevTools where pages don't need special powers. r=jryans
Keywords: checkin-needed
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/452b375b0e40
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•