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)

enhancement

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.
@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)
Let's break down each case to be sure.  Looking at https://searchfox.org/mozilla-central/search?q=openTrustedLinkIn&case=true&regexp=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)
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
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: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Keywords: checkin-needed
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
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)
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)
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)
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
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
https://hg.mozilla.org/mozilla-central/rev/452b375b0e40
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.