Closed Bug 1374741 Opened 3 years ago Closed 2 years ago

Have openUILinkIn() provide the correct triggeringPrincipal

Categories

(Core :: DOM: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: ckerschb, Assigned: jkt)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(6 files, 38 obsolete files)

1.46 KB, patch
kmckinley
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
No description provided.
Assignee: nobody → ckerschb
Blocks: 1364392
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Hey Gijs, our efforts seem endless :-(

Do you think it makes sense to update all callsites of openUILinkIn() to actually pass a 'params' object as the third argument. Something similar to what I have done for one callsite within this patch (obviously passing the right triggeringPrincipal instead of using SystemPrincipal).

Using the temporary helper exception within openUILinkIn() we could identify all callsites. A quick DXR search revealed we would have to update something around 90 callsites [1].

What do you think? Makes sense?

[1] https://dxr.mozilla.org/mozilla-central/search?q=openUILinkIn(&redirect=false

PS: Once this bug is resolved we could then move on to Bug 1364392 :-)
Flags: needinfo?(gijskruitbosch+bugs)
I think in principle we could do this, but it would break add-ons, so we would either have to wait until 57 or make it only throw errors in automation / nightly only, or something.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #2)
> I think in principle we could do this, but it would break add-ons, so we
> would either have to wait until 57 or make it only throw errors in
> automation / nightly only, or something.

To be clear, I am not going to land those exceptions, I just use them temporarily to make sure I have updated all the callsites. After 57 I am fine with actually changing the signature of those functions, until then we should only update Gecko code to pass an 'params' argument and leave everything else unchanged.
Depends on: 1375023
Duplicate of this bug: 1379576
Let's give it a try and see how we do when running the entire test suite, locally already looks pretty good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=197badbf4c79dd8c4490eabc6ad3ce3222653183
Assignee: ckerschb → kmckinley
Attachment #8879672 - Attachment is obsolete: true
Updated to add triggering principal in test.
Attachment #8894906 - Attachment is obsolete: true
Updated with null checks
Attachment #8894904 - Attachment is obsolete: true
Attachment #8894902 - Attachment is obsolete: true
rebased patch
Attachment #8907337 - Attachment is obsolete: true
rebased patch
Attachment #8907339 - Attachment is obsolete: true
Added null checking around use of event.dataTransfer when choosing the appropriate triggering principal.
Attachment #8907340 - Attachment is obsolete: true
Attachment #8908814 - Flags: review?(pbrosset)
Added the triggering principal to the xpcshell test
Attachment #8907341 - Attachment is obsolete: true
Attachment #8908815 - Flags: review?(gijskruitbosch+bugs)
All openUILinkIn calls get a triggering principal.
Attachment #8907343 - Attachment is obsolete: true
Attachment #8908816 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8908815 [details] [diff] [review]
bug_1374741_openUILinkIn_browser_tests.patch

Review of attachment 8908815 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_bug579872.js
@@ +11,5 @@
>      gBrowser.pinTab(newTab);
>      gBrowser.selectedTab = newTab;
>  
> +    openUILinkIn("javascript:var x=0;", "current", {
> +      triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),

Nit:

let triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal();

openUILinkIn(..., "current", {triggeringPrincipal});

Avoids the repetition here.
Attachment #8908815 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8908816 [details] [diff] [review]
8907343: bug_1374741_openUILinkIn_browser.patch

Review of attachment 8908816 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh, I should have reviewed this first, clearly.

Can you or Christoph remind me why we're not just adding this as the fallback value into openUILinkIn ? Are there actual cases of openUILinkIn with a web URL or something where we can't use system principal? I see only the dnd code in here. At that point, does it really make sense to add this boilerplate everywhere?

I do understand that "secure defaults" would imply that in theory we shouldn't make anything use system principal unless we really need to, but equally the size and verbosity of this patch is very unfortunate, and it seems like 99% of callsites want the system principal anyway, so not making it the default seems wrong too.

If we feel that making it the default is inappropriate, we should instead make opting in less verbose, maybe with a bool like "userTriggered: true" or something.

Finally, unfortunately this patch already makes system principal the default for callers of openUILink() if no event is passed, and so as it is, the patch is a weird compromise between "secure defaults" for openUILinkIn *and* footgun-override-sorry-you-got-a-system-principal-by-accident defaults for openUILink. Whatever we do, we should be consistent.
Attachment #8908816 - Flags: review?(gijskruitbosch+bugs)
Attachment #8908818 - Flags: review?(amarchesini) → review+
(In reply to :Gijs from comment #23)
> Comment on attachment 8908816 [details] [diff] [review]
> 8907343: bug_1374741_openUILinkIn_browser.patch
> 
> Review of attachment 8908816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ugh, I should have reviewed this first, clearly.
> 
> Can you or Christoph remind me why we're not just adding this as the
> fallback value into openUILinkIn ? Are there actual cases of openUILinkIn
> with a web URL or something where we can't use system principal? I see only
> the dnd code in here. At that point, does it really make sense to add this
> boilerplate everywhere?

We shouldn't assume that the SystemPrincipal is correct in 100% of cases. For example, a common case in devtools/ is that there is an event which may have a triggeringPrincipal, and so we should use that if it is available.

> I do understand that "secure defaults" would imply that in theory we
> shouldn't make anything use system principal unless we really need to, but
> equally the size and verbosity of this patch is very unfortunate, and it
> seems like 99% of callsites want the system principal anyway, so not making
> it the default seems wrong too.
> 
> If we feel that making it the default is inappropriate, we should instead
> make opting in less verbose, maybe with a bool like "userTriggered: true" or
> something.

Since the SystemPrincipal is extra powerful, users of this method should be passing the principal, so I'm reluctant to make it the default. Using a marker like userTriggered should be acceptable. Since 3rd party addons can no longer call this code, internally we should always know the triggeringPrincipal.

> Finally, unfortunately this patch already makes system principal the default
> for callers of openUILink() if no event is passed, and so as it is, the
> patch is a weird compromise between "secure defaults" for openUILinkIn *and*
> footgun-override-sorry-you-got-a-system-principal-by-accident defaults for
> openUILink. Whatever we do, we should be consistent.

It looks like currently, other than tests, we pass an event for every invocation of openUILink. Since there are no more addon callers for this, could we change that to required? I don't know if every event has all of the data we need to get the right principal, though.
(In reply to Kate McKinley [:kmckinley, :Kate] from comment #24)
> > Finally, unfortunately this patch already makes system principal the default
> > for callers of openUILink() if no event is passed, and so as it is, the
> > patch is a weird compromise between "secure defaults" for openUILinkIn *and*
> > footgun-override-sorry-you-got-a-system-principal-by-accident defaults for
> > openUILink. Whatever we do, we should be consistent.
> 
> It looks like currently, other than tests, we pass an event for every
> invocation of openUILink. Since there are no more addon callers for this,
> could we change that to required?

I haven't checked that we always pass one, but in principle yes we could make it required, but...

> I don't know if every event has all of the
> data we need to get the right principal, though.

Right. I think right now we only use drag origin data (which is correct), but often the originating event will be a click or command event, which doesn't have such data. So I don't think that helps (much).

In *theory*, openUILink(In) is only supposed to be used for UI links, that is, links that were supplied by Firefox (rather than say, a random webpage) or the user's data (like bookmarks etc.). As a result, I think system principal is going to be the right principal in the majority of cases.

Now that add-on uses are gone, if we think it's helpful, we could potentially rename the method and come up with a separate one where we pass a principal? Have "openChromeLink" and "openUnsafeLink" or something. Perhaps devtools and the browser's drag and drop code shouldn't be using openUILink / openChromeLink if system principal is the wrong triggering principal...

FWIW,

(In reply to Kate McKinley [:kmckinley, :Kate] from comment #24)
> We shouldn't assume that the SystemPrincipal is correct in 100% of cases.
> For example, a common case in devtools/ is that there is an event which may
> have a triggeringPrincipal, and so we should use that if it is available.

Err, I'll leave pbro to properly review this, but I looked for an example of what you meant, and at least in devtools/client/inspector/computed/computed.js , the event is a click event and you're trying to get drag transferData off it. There won't (ever) be any, so in reality that code is using system principal, too. If you used event.target.nodePrincipal, you'd get the principal of the inspector node, but even that is likely to be system (or a null principal that we only use to render devtools or something, which is also not what you want). If anything, I guess we should get the principal of the *original* DOM node (ie not the one in the inspector, the one in the tab it points to).
(In reply to :Gijs from comment #25)
> Now that add-on uses are gone, if we think it's helpful, we could
> potentially rename the method and come up with a separate one where we pass
> a principal? Have "openChromeLink" and "openUnsafeLink" or something.

I like the idea of having "openChromeLink" and "openWeblink". For those cases the name would already imply what we are about to open. Further, it would allow us to remove the boilerplate from the callsite. Now that we already know all the callistes, it  should be fairly straight forward to do that kind of textual replacement.
Attachment #8908814 - Flags: review?(pbrosset) → review?(poirot.alex)
Comment on attachment 8908814 [details] [diff] [review]
bug_1374741_openUILinkIn_devtools.patch

(In reply to Kate McKinley [:kmckinley, :Kate] from comment #24)
> We shouldn't assume that the SystemPrincipal is correct in 100% of cases.
> For example, a common case in devtools/ is that there is an event which may
> have a triggeringPrincipal, and so we should use that if it is available.

Isn't triggeringPrincipal always going to be SystemPrincipal in devtools/?
The events are all coming from devtools UI which only made of chrome documents.
None of these events come from web content.

Like Gijs, I'm pretty reluctant to complexify devtools codebase everywhere,
whereas all callsites seems to be equivalent in devtools/.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> I like the idea of having "openChromeLink" and "openWeblink".

Could we use that from devtools instead?
Out of curiosity, what is the openUILinkIn behavior difference between content and chrome trigerringPrincipal?
This patch adds two methods, openTrustedLink and openWebLink which are intended to replace uses of openUILinkIn. OpenTrustedLink is intended for UI actions, and  uses the SystemPrincipal as the default if no triggeringPrincipal is provided. OpenWebLink is intended for use in other contexts, where the SystemPrincipal is not necessarily the correct principal to use. It uses the NullPrincipal if no triggeringPrincipal is provided. Both methods will prefer a triggeringPrincipal supplied by the caller.

Still todo is to remove debugging and cleanup code, but I wanted to get feedback on this.
Attachment #8908816 - Attachment is obsolete: true
Attachment #8935207 - Flags: review?(gijskruitbosch+bugs)
Attachment #8908811 - Attachment is obsolete: true
Attachment #8908814 - Attachment is obsolete: true
Attachment #8908814 - Flags: review?(poirot.alex)
Comment on attachment 8935207 [details] [diff] [review]
bug_1374741_openUILinkIn_browser.patch

Review of attachment 8935207 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great. Just a few comments. Note that I haven't checked if the patch is complete - I've just checked that all the things in this patch make sense to me.

I'm going to mark this r+ with the minor issues addressed and openUILink() (as opposed to openUILink*In*()) moved to a separate patch.

::: browser/base/content/pageinfo/feeds.js
@@ +17,5 @@
>  }
>  
>  function onSubscribeFeed() {
>    var listbox = document.getElementById("feedListbox");
> +  openTrustedLink(listbox.selectedItem.getAttribute("feedURL"), "current", {

This should be openWebLink. This URL is coming from the web page.

The principal will need to be passed through somehow. I don't know off-hand how hard this would be, but this might be an issue for other items in the page info dialog, too, so maybe it is already available somewhere?

::: browser/base/content/utilityOverlay.js
@@ +118,2 @@
>    let where = whereToOpenLink(event, aIgnoreButton, aIgnoreAlt);
> +  openTrustedLink(url, where, params);

I don't think this is right. If I look at the callsites for openUILink(), I think some of them should be using openWebLink. See e.g. https://dxr.mozilla.org/mozilla-central/rev/457b0fe91e0d49a5bc35014fb6f86729cd5bac9b/browser/base/content/browser-feeds.js#222 which just like the page info case is using a feed URI from the webpage.

Because this is a bit messy, can you split the openUILink() case out to a separate patch? Without this change, all the other comments on this patch are minor.

@@ +174,5 @@
> + * as the trigeringPrincipal, unless a more specific Principal is provided.
> + *
> + * See openUILinkIn for a discussion of parameters
> + */
> +function openTrustedLink(url, where, aParams) {

If these forward to `openUILinkIn`, then perhaps they need to be called open*LinkIn as well?

@@ +176,5 @@
> + * See openUILinkIn for a discussion of parameters
> + */
> +function openTrustedLink(url, where, aParams) {
> +  var params = aParams;
> +  dump("\n\n @@@@@ (openTrustedLink) : " + Components.stack.caller + "\n\n");

Nit: shouldn't land with this in the patch. :-)

@@ +194,5 @@
> + *
> + * See openUILinkIn for a discussion of parameters
> + */
> +function openWebLink(url, where, params) {
> +  dump("\n\n @@@@@ (openWebLink) : " + Components.stack.caller + "\n\n");

Same here.

@@ +217,5 @@
>   *  "window"      new window
>   *  "save"        save to disk (with no filename hint!)
>   *
> + * DEPRECATION WARNING:
> + * USE        -> openTrustedLink(url, where, aParams) if the source is always a user event (i.e., UI click)

I would say "user event on a user- or product-specified URL (as opposed to URLs provided by a webpage)".

@@ +218,5 @@
>   *  "save"        save to disk (with no filename hint!)
>   *
> + * DEPRECATION WARNING:
> + * USE        -> openTrustedLink(url, where, aParams) if the source is always a user event (i.e., UI click)
> + * USE        -> openWebLink(url, where, aParams) if the URI should be loaded with a specific triggeringPrincipal

maybe add ", for instance, if the url was supplied by web content."

@@ +247,2 @@
>    } else {
> +    dump("\n\n XXX (openUILinkIn) has no triggeringPrincipal: " + Components.stack.caller + "\n\n");

This probably wants to disappear before landing, too. :-)

@@ +252,4 @@
>        postData: aPostData,
>        referrerURI: aReferrerURI,
>        referrerPolicy: Components.interfaces.nsIHttpChannel.REFERRER_POLICY_UNSET,
>      };

And then we can drop this bit of code, I think, given the `throw` ?

@@ +258,5 @@
> +  if (!params.triggeringPrincipal) {
> +    dump("\n\n XXX (openUILinkIn) has no params.triggeringPrincipal: " + Components.stack.caller + "\n\n");
> +    throw new Error("Required argument params.triggeringPrincipal missing within openUILinkIn");
> +  }
> +  dump("\n\n @@@@@ (openUILinkIn) params.triggeringPrincipal: " + Components.stack.caller + "\n\n");

These 2 dumps should also disappear. :-)

::: browser/components/nsBrowserGlue.js
@@ +457,5 @@
>              // opening in a new selected tab.
>              if (where == "current") {
>                where = "tab";
>              }
> +            win.openTrustedLink(data.href, where);

Yuck. This code does its own security handling before it gets here. Can you file a followup so it works differently and passes a principal? We should be able to simplify it a bit that way...

::: browser/extensions/pocket/content/Pocket.jsm
@@ +25,5 @@
>      // Never override the current tab unless it's blank:
>      if (where == "current" && !win.isTabEmpty(win.gBrowser.selectedTab)) {
>        where = "tab";
>      }
> +    win.openWebLink(this.listURL, where);

This is hardcoded to the pocket site, so can be openTrustedLink, I think.
Attachment #8935207 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8935208 - Attachment is obsolete: true
Attachment #8938514 - Flags: review?(gijskruitbosch+bugs)
Attachment #8935212 - Attachment is obsolete: true
Attachment #8938519 - Flags: review?(gijskruitbosch+bugs)
Attachment #8935207 - Attachment is obsolete: true
Attachment #8938520 - Flags: review+
Attachment #8938520 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8908818 - Attachment is obsolete: true
Attachment #8935214 - Attachment is obsolete: true
Attachment #8938521 - Flags: review+
Can these reviews wait until after the end-of-year break?
Flags: needinfo?(kmckinley)
Yes, that should be fine.
Flags: needinfo?(kmckinley)
Comment on attachment 8938514 [details] [diff] [review]
provide correct triggering principal in toolkit

Review of attachment 8938514 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with openWebLinkIn for contentAreaUtils.js

::: toolkit/content/contentAreaUtils.js
@@ +1193,5 @@
>      protocolSvc.loadURI(uri);
>    } else {
>      var recentWindow = Services.wm.getMostRecentWindow("navigator:browser");
>      if (recentWindow) {
> +      recentWindow.openTrustedLinkIn(uri.spec, "tab");

This looks to me like it should be openWebLinkIn - some of the callsites in https://dxr.mozilla.org/mozilla-central/search?q=openURL(%20file%3A.js&=mozilla-central are using web-provided referrer URLs or add-on provided contribution/homepage URLs.
Attachment #8938514 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8938520 [details] [diff] [review]
provide the correct triggeringPrincipal within browser/

Review of attachment 8938520 [details] [diff] [review]:
-----------------------------------------------------------------

There's still a few bits here that need work, see below.

I'm also conscious we're not really consistent in how we treat history vs. open tabs in this patch. Synced tabs and switch-to-tab functionality is getting system principal here, but history isn't. But logically speaking, if it can end up in history it can be an open tab, right? So I suppose we should treat them the same.

I also kind of assumed all the openUILink() (as opposed to openUILinkIn) would end up in a separate patch, but it seems it's still in here? I feel bad clearing the review, but equally perhaps it makes sense to look at this again if those hunks stay in this patch...

::: browser/base/content/browser-feeds.js
@@ +215,5 @@
>      let feeds = gBrowser.selectedBrowser.feeds;
>      try {
> +      openUILink(href, event, {
> +        ignoreAlt: true,
> +        triggeringPrincipal: Services.scriptSecurityManger.createNullPrincipal({}),

I think this can use gBrowser.contentPrincipal (which forwards to gBrowser.selectedBrowser.contentPrincipal).

::: browser/base/content/browser-places.js
@@ +851,5 @@
>        if (!PrivateBrowsingUtils.isWindowPrivate(window))
>          PlacesUIUtils.markPageAsTyped(placesNode.uri);
> +      openUILink(placesNode.uri, aEvent, {
> +        ignoreAlt: true,
> +        triggeringPrincipal: Services.scriptSecurityManger.createNullPrincipal({})

This is a history item, so I guess this should be system? (see above).

::: browser/base/content/nsContextMenu.js
@@ +854,5 @@
>      let referrer = gContextMenuContentData.referrer;
> +    openWebLinkIn(gContextMenuContentData.docLocation, "current", {
> +      disallowInheritPrincipal: true,
> +      referrerURI: referrer ? makeURI(referrer) : null,
> +      triggeringPrincipal: this.principal,

You're using `this.principal`, but the previous urlSecurityCheck uses `contentPrincipal`. Why the difference?

Below, you use `contentPrincipal`. For the frame case (this one) I'm not sure which is best. For the image desc case below, it seems to me we should be using `this.principal`, assuming that's available.

@@ +909,5 @@
>                       this.browser.contentPrincipal,
>                       Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);
>      openUILink(this.imageDescURL, e, { disallowInheritPrincipal: true,
> +                                       referrerURI: gContextMenuContentData.documentURIObject,
> +                                       triggeringPrincipal: this.browser.contentPrincipal,

Can we omit the manual `urlSecurityCheck` calls here and elsewhere in this file given we're now passing a principal?

::: browser/base/content/utilityOverlay.js
@@ +89,5 @@
>  function openUILink(url, event, aIgnoreButton, aIgnoreAlt, aAllowThirdPartyFixup,
>                      aPostData, aReferrerURI) {
>    let params;
>  
> +  dump("\n\n @@@@@ (openUILink) params.triggeringPrincipal: caller: " + Components.stack.caller + "\n\n");

Nit: please omit the dump. :-)

@@ +113,5 @@
> +  if (!params.triggeringPrincipal) {
> +    let dt = event ? event.dataTransfer : null;
> +    if (!!dt && dt.mozSourceNode) {
> +      params.triggeringPrincipal = dt.mozSourceNode.nodePrincipal;
> +    }

There should be a fallback value in case there's no event, or no (datatransfer with) mozSourceNode, right? Can we make that null principal?

::: browser/components/places/content/browserPlacesViews.js
@@ +442,5 @@
>          aPopup._siteURIMenuitem.classList.add(this.options.extraClasses.entry);
>        }
>        aPopup._siteURIMenuitem.setAttribute("targetURI", siteUrl);
>        aPopup._siteURIMenuitem.setAttribute("oncommand",
> +        "openUILink(this.getAttribute('targetURI'), event, { triggeringPrincipal: Services.scriptSecurityManger.createNullPrincipal({})});");

Can you split this across two lines?

::: browser/extensions/pocket/content/main.js
@@ +508,5 @@
>      /**
>       * Open a new tab with a given url and notify the iframe panel that it was opened
>       */
>  
> +    function openTabWithUrl(url, aTriggeringPrincipal) {

This is a bit confusing. You're not actually using the triggering principal. Did you mean to do something else?

::: browser/extensions/screenshots/bootstrap.js
@@ +110,5 @@
>      let parent = libraryViewInsertionPoint.parentNode;
>      let {nextSibling} = libraryViewInsertionPoint;
>      let item = win.document.createElement("toolbarbutton");
>      item.className = "subviewbutton subviewbutton-iconic";
> +    item.addEventListener("command", () => win.openTrustedLinkIn(this.PAGE_TO_OPEN, "tab"));

Note that this will need upstreaming to the screenshots repo.
Attachment #8938520 - Flags: review+
Attachment #8938520 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8938519 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee: kate+bugzilla → cfu
Attachment #8938514 - Attachment is obsolete: true
Attachment #8941936 - Flags: review+
Attachment #8938517 - Attachment is obsolete: true
Attachment #8941939 - Flags: review?(pbrosset)
Attachment #8938518 - Attachment is obsolete: true
Attachment #8941940 - Flags: review?(pbrosset)
Updated based on comment 44.

I think it makes sense to keep the openUILink() changes in this patch since we can't necessarily determine within openUILink() which is the correct principal to use.
Attachment #8938520 - Attachment is obsolete: true
Attachment #8941944 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8941939 [details] [diff] [review]
provide the correct triggeringPrincipal within devtools/ tests

Review of attachment 8941939 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/styleeditor/test/browser_styleeditor_opentab.js
@@ +20,5 @@
>    let url = "https://example.com/browser/devtools/client/styleeditor/test/" +
>      "simple.css";
>    is(ui._contextMenuStyleSheet.href, url, "Correct URL for sheet");
>  
>    let originalOpenUILinkIn = ui._window.openUILinkIn;

I feel like there are more changes needed here. The code shadows the openUILinkIn, but really now it should shadow the openTrustedLinkIn. So all instances of openUILinkIn in this block of code should be replaced with openTrustedLinkIn.
Attachment #8941939 - Flags: review?(pbrosset)
Comment on attachment 8941940 [details] [diff] [review]
provide the correct triggeringPrincipal within devtools/

Review of attachment 8941940 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/debugger/content/views/sources-view.js
@@ +899,5 @@
>     */
>    _onNewTabCommand: function () {
>      let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
>      let selected = this.selectedItem.attachment;
> +    win.openTrustedLinkIn(selected.source.url, "tab", {

This will open a JS or CSS file coming from the website currently being debugged. Should we use the trusted version here? 

Disclaimer: I'm not aware of how principals work, and what the actual different between openTrustedLinkIn and openWebLinkIn is. But it feels like we should be using openTrustedLinkIn only for links we own, and openWebLinkIn for anything else (any URL that's coming from the untrusted website).
So I'll base my review on that. I hope that's correct.

::: devtools/client/dom/dom-panel.js
@@ +182,5 @@
>    openLink: function (url) {
>      let parentDoc = this._toolbox.doc;
>      let iframe = parentDoc.getElementById("this._toolbox");
>      let top = iframe.ownerDocument.defaultView.top;
> +    top.openTrustedLinkIn(url, "tab");

I think urls here come from the web page. So they can't be trusted.

::: devtools/client/inspector/computed/computed.js
@@ +700,5 @@
>        event.stopPropagation();
>        event.preventDefault();
>        let browserWin = this.inspector.target.tab.ownerDocument.defaultView;
> +      let dt = event.dataTransfer;
> +      let triggeringPrincipal = (!!dt && dt.mozSourceNode)

I don't understand what these few lines of code do. Why use the dataTransfer object, and in which case can it be null, or not.
Can you please add a line of comment above that explains what's going on here?

@@ +704,5 @@
> +      let triggeringPrincipal = (!!dt && dt.mozSourceNode)
> +          ? dt.mozSourceNode.nodePrincipal
> +          : browserWin.document.nodePrincipal;
> +      browserWin.openWebLinkIn(target.href, "tab", {
> +        triggeringPrincipal: triggeringPrincipal,

Nit: You can shorten slightly this:

browserWin.openWebLinkIn(target.href, "tab", { triggeringPrincipal });

::: devtools/client/inspector/inspector.js
@@ +2226,5 @@
>        this.inspector.resolveRelativeURL(
>          link, this.selection.nodeFront).then(url => {
>            if (type === "uri") {
>              let browserWin = this.target.tab.ownerDocument.defaultView;
> +            browserWin.openTrustedLinkIn(url, "tab");

This url is coming from the page being inspected here. It's not something we can trust.

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +297,5 @@
>          if (target.nodeName === "a") {
>            event.stopPropagation();
>            event.preventDefault();
>            let browserWin = this.ruleView.inspector.target.tab.ownerDocument.defaultView;
> +          let dt = event.dataTransfer;

Same comment than before about adding a line of comment here explaining what is going on here.

::: devtools/client/netmonitor/initializer.js
@@ +59,5 @@
>      const openLink = (link) => {
>        let parentDoc = toolbox.doc;
>        let iframe = parentDoc.getElementById("toolbox-panel-iframe-netmonitor");
>        let top = iframe.ownerDocument.defaultView.top;
> +      top.openTrustedLinkIn(link, "tab");

This link comes from the page itself. Can't be trusted.

::: devtools/client/netmonitor/src/components/MdnLink.js
@@ +34,5 @@
>    e.stopPropagation();
>    e.preventDefault();
>  
>    let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> +  let dt = event.dataTransfer;

Also, similar comment than before about explaining this code in a comment line.

@@ +42,3 @@
>    if (e.button === 1) {
> +    win.openWebLinkIn(url, "tabshifted", {
> +      triggeringPrincipal: triggeringPrincipal,

Nit: this can also be made shorter:

win.openWebLinkIn(url, "tabshifted", { triggeringPrincipal });

@@ +46,3 @@
>    } else {
> +    win.openWebLinkIn(url, "tab", {
> +      triggeringPrincipal: triggeringPrincipal,

ditto.

::: devtools/client/shared/AppCacheUtils.jsm
@@ +294,5 @@
>      let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
>                 .getService(Ci.nsIWindowMediator);
>      let win = wm.getMostRecentWindow(gDevTools.chromeWindowType);
>      let url = "about:cache-entry?storage=appcache&context=&eid=&uri=" + key;
> +    let triggeringPrincipal = undefined;

You initialize this variable, but don't use it later. This line should be removed.

::: devtools/client/styleeditor/StyleEditorUI.jsm
@@ +494,5 @@
>     * Open a particular stylesheet in a new tab.
>     */
>    _openLinkNewTab: function () {
>      if (this._contextMenuStyleSheet) {
> +      this._window.openTrustedLinkIn(this._contextMenuStyleSheet.href, "tab");

The URL here is a stylesheet coming from the site. Can't be trusted.
Attachment #8941940 - Flags: review?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #53)
> ::: devtools/client/inspector/computed/computed.js
> @@ +700,5 @@
> >        event.stopPropagation();
> >        event.preventDefault();
> >        let browserWin = this.inspector.target.tab.ownerDocument.defaultView;
> > +      let dt = event.dataTransfer;
> > +      let triggeringPrincipal = (!!dt && dt.mozSourceNode)
> 
> I don't understand what these few lines of code do. Why use the dataTransfer
> object, and in which case can it be null, or not.
> Can you please add a line of comment above that explains what's going on
> here?

DataTransfer and mozSourceNode is only useful for drag events. Because this method is called _onClick , I expect this is never a drag event so this doesn't work.
Comment on attachment 8941944 [details] [diff] [review]
provide the correct triggeringPrincipal within browser/

Review of attachment 8941944 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following minor issues addressed:

::: browser/base/content/nsContextMenu.js
@@ -1195,2 @@
>      let isContentWindowPrivate = this.isRemote ? this.ownerDoc.isPrivate : undefined;
>      this.saveHelper(this.linkURL, this.linkTextStr, null, true, this.ownerDoc,

So saveHelper checks principals, so this removal is OK...

@@ -1224,1 @@
>        saveImageURL(this.mediaURL, null, "SaveImageTitle", false,

but crucially, saveImageURL does not seem to check principals, so I think this specific check needs to stay, at least for now, based on my archaeology of the CVS repo...

::: browser/components/places/content/browserPlacesViews.js
@@ +443,5 @@
>        }
>        aPopup._siteURIMenuitem.setAttribute("targetURI", siteUrl);
>        aPopup._siteURIMenuitem.setAttribute("oncommand",
> +        "openUILink(this.getAttribute('targetURI'), event, {"
> +        + " triggeringPrincipal: Services.scriptSecurityManger.createNullPrincipal({})});");

Nit: operators go on the end of lines rather than the beginning.

::: browser/extensions/pocket/content/main.js
@@ +382,4 @@
>          var _openTabWithUrlMessageId = "openTabWithUrl";
>          pktUIMessaging.addMessageListener(iframe, _openTabWithUrlMessageId, function(panelId, data, contentPrincipal) {
>              try {
>                urlSecurityCheck(data.url, contentPrincipal, Services.scriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);

Do we still need this security check if we're passing the contentPrincipal anyway?
Attachment #8941944 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Patrick Brosset <:pbro> from comment #53)
> Disclaimer: I'm not aware of how principals work, and what the actual
> different between openTrustedLinkIn and openWebLinkIn is. But it feels like
> we should be using openTrustedLinkIn only for links we own, and
> openWebLinkIn for anything else (any URL that's coming from the untrusted
> website).
> So I'll base my review on that. I hope that's correct.

That assessment is absolutely correct. For all URLs to be loaded that can be influenced by the web, we should *never* use the SystemPrincipal. We should always try to get the documents nodePrincipal in such cases. If that is not possible in some cases we should do our best to find the accurate principal for the load, but still shouldn't use the Systemprincipal.
Assignee: cfu → nobody
Status: ASSIGNED → NEW
Christoph, I think these patches will bitrot rapidly, but they're mostly finished-ish, as far as I can tell from this bug (maybe 1 or 2 more review rounds for the non-r+'d bits, but the majority just want updating the last few nits + landing). Is someone available to work on this? This is an important part of our defense-in-depth/security-arch stuff and I'd hate for it to be dropped.
Flags: needinfo?(ckerschb)
(In reply to :Gijs from comment #57)
> Christoph, I think these patches will bitrot rapidly, but they're mostly
> finished-ish, as far as I can tell from this bug (maybe 1 or 2 more review
> rounds for the non-r+'d bits, but the majority just want updating the last
> few nits + landing). Is someone available to work on this? This is an
> important part of our defense-in-depth/security-arch stuff and I'd hate for
> it to be dropped.

Same here. I really want that to be landed. Trying hard to acquire resources for that. I'll see what I can do.
Flags: needinfo?(ckerschb)
Assignee: nobody → jkt
Status: NEW → ASSIGNED
Attachment #8941944 - Attachment is obsolete: true
Attachment #8941943 - Attachment is obsolete: true
Attachment #8941940 - Attachment is obsolete: true
The other patches rebased cleanly currently.

I addressed all comments I think besides:

Comment 55:
- Do we still need this security check if we're passing the contentPrincipal anyway?

I removed this however need to verify it works as expected.

Comment 53:
- I don't understand what these few lines of code do. Why use the dataTransfer object, and in which case can it be null, or not.
- Same comment than before about adding a line of comment here explaining what is going on here.
- Also, similar comment than before about explaining this code in a comment line.


Pushed to try to see where we are with the 3 patches I rebased and modified:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a07bc5310f9aa6a81d016ff9ce333487488f039e
Attachment #8941936 - Attachment is obsolete: true
Attachment #8941939 - Attachment is obsolete: true
Comment on attachment 8952774 [details]
Bug 1374741 - Within browser/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/221950/#review228226

I assume this basically hasn't changed apart from fixing the previous review's comments, so carrying over r=me.
Attachment #8952774 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952775 [details]
Bug 1374741 - Within browser/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/221952/#review228228
Attachment #8952775 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952886 [details]
Bug 1374741 - Within toolkit/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222120/#review228230

::: toolkit/content/contentAreaUtils.js:1181
(Diff revision 2)
>      // If we're not a browser, use the external protocol service to load the URI.
>      protocolSvc.loadURI(uri);
>    } else {
>      var recentWindow = Services.wm.getMostRecentWindow("navigator:browser");
>      if (recentWindow) {
> -      recentWindow.openUILinkIn(uri.spec, "tab");
> +      recentWindow.openTrustedLinkIn(uri.spec, "tab");

See https://bugzilla.mozilla.org/show_bug.cgi?id=1374741#c43 . I think this should be openWebLink. If that breaks something and it's hard to fix, we need to make sure that we have a follow-up and address it asap.
Attachment #8952886 - Flags: review?(gijskruitbosch+bugs) → review+
> I think this should be openWebLink. If that breaks something and it's hard to fix, we need to make sure that we have a follow-up and address it asap.

Agreed, also I found this https://bugzilla.mozilla.org/show_bug.cgi?id=1311145 which is one less instance to check if we remove. All the instances look like very low frequency use methods and perhaps this whole method could be removed eventually.

Restricting to web only seems safe and low impact if we mess it up anyway.
Comment on attachment 8952776 [details]
Bug 1374741 - Within devtools/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222000/#review228662

Apart from the odd usage of dataTransfer properties in non-drag event handlers, this looks good.

::: devtools/client/inspector/computed/computed.js:728
(Diff revision 3)
> -      browserWin.openUILinkIn(target.href, "tab");
> +      let dt = event.dataTransfer;
> +      let triggeringPrincipal = (!!dt && dt.mozSourceNode)
> +          ? dt.mozSourceNode.nodePrincipal
> +          : browserWin.document.nodePrincipal;

This is called on click, not on a drag event. Using the dataTransfer property would not work in this case, right?

::: devtools/client/inspector/computed/computed.js:1219
(Diff revision 3)
> -      browserWin.openUILinkIn(this.link, "tab");
> +      let dt = event.dataTransfer;
> +      let triggeringPrincipal = (!!dt && dt.mozSourceNode)
> +          ? dt.mozSourceNode.nodePrincipal
> +          : browserWin.document.nodePrincipal;

Same comment here.

::: devtools/client/inspector/rules/views/text-property-editor.js:302
(Diff revision 3)
> -          browserWin.openUILinkIn(target.href, "tab");
> +          let dt = event.dataTransfer;
> +          let triggeringPrincipal = (!!dt && dt.mozSourceNode)
> +              ? dt.mozSourceNode.nodePrincipal
> +              : undefined;
> +          browserWin.openTrustedLinkIn(target.href, "tab", {
> +            triggeringPrincipal: triggeringPrincipal,
> +          });

Also not a drag event, but a click event. Does using dataTransfer work here?

::: devtools/client/netmonitor/src/components/MdnLink.js:40
(Diff revision 3)
> +  let dt = event.dataTransfer;
> +  let triggeringPrincipal = (!!dt && dt.mozSourceNode)
> +      ? dt.mozSourceNode.nodePrincipal
> +      : Services.scriptSecurityManager.getSystemPrincipal();

Same comment about click event vs. drag event.
Attachment #8952776 - Flags: review?(pbrosset)
Comment on attachment 8952892 [details]
Bug 1374741 - Within devtools/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222124/#review228666

::: devtools/client/styleeditor/test/browser_styleeditor_opentab.js:26
(Diff revision 3)
> -    ui._window.openUILinkIn = newUrl => {
> -      // Reset the actual openUILinkIn function before proceeding.
> -      ui._window.openUILinkIn = originalOpenUILinkIn;
> +    ui._window.openWebLinkIn = newUrl => {
> +      // Reset the actual openTrustedLink function before proceeding.
> +      ui._window.openWebLinkIn = originalOpenTrustedLinkIn;

We're attempting to shadow the openTrustedLinkIn function here, but then the openWebLinkIn function is really being overriden instead, and finally it is being set to the value of openTrustedLinkIn.
I think there's a bit of confusion here, we should really just have openTrustedLinkIn here, not openWebLinkIn.
Attachment #8952892 - Flags: review?(pbrosset)
Comment on attachment 8952892 [details]
Bug 1374741 - Within devtools/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222124/#review228666

> We're attempting to shadow the openTrustedLinkIn function here, but then the openWebLinkIn function is really being overriden instead, and finally it is being set to the value of openTrustedLinkIn.
> I think there's a bit of confusion here, we should really just have openTrustedLinkIn here, not openWebLinkIn.

Sorry I have an update here which shadows both just-in-case. but I can switch it to trusted only.
:pbro I hadn't tackled those dataTransfer events yet, it appears these links open in a new tab which looks like it would always load correctly without the principal.

:ckerschb have I undeststood openWebLink correctly? If a link opens in a new tab to a https?: url do we ever need a principal for openWebLink?
Is there a concern if we somehow get a chrome document within the inspector this will cause an issue or just break?
Flags: needinfo?(pbrosset)
Flags: needinfo?(ckerschb)
(In reply to Jonathan Kingston [:jkt] from comment #80)
> :pbro I hadn't tackled those dataTransfer events yet, it appears these links
> open in a new tab which looks like it would always load correctly without
> the principal.
> 
> :ckerschb have I undeststood openWebLink correctly? If a link opens in a new
> tab to a https?: url do we ever need a principal for openWebLink?
> Is there a concern if we somehow get a chrome document within the inspector
> this will cause an issue or just break?

The 'point' here is down-privileging the loads that load fully-web-controlled URIs. If I have a webpage at https://foo.com and it includes a link to file:///etc/passwd, the user can't click that link to load it, or drag it to some bit of browser UI to load it, or open it in the inspector and click it in there to load it. There should be a clean barrier to loading it directly.

Users can work around this by copying the URL and then pasting it and then loading it, and that's OK.

Anyhow, the main point of passing the 'right' principal is to avoid situations where through clickjacking or window positioning, websites can trick people into loading and/or executing content that websites would ordinarily not be able to link to / load / execute.
... and so yes, if the target of the link is always http/https, then the null principal which (I think) is the default for openWebLink, will be fine. But I don't know that that's always true - e.g. if I use devtools on a chrome or file: page I'd like that to work, and I should hope that a null principal wouldn't be able to load file: or chrome: things.
Thanks Gijs that really helps clear up what I was thinking.

Many of these are MDN links which I think are fine however the code needs some fixing to work in toolbox. Pressing f1 on the computed style in inspector gets the following with openWebLink:

console.error: 
  Message: Error: Custom SelectorHighlighterhighlighter cannot be created in a XUL window
  Stack:
    initialize@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/highlighters.js:466:13
cls@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1173:5
getHighlighterByType@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/inspector/inspector.js:184:14
handler@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1106:19
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1767:15
_onJSONObjectReady/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:482:11
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14

initialize@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/highlighters.js:466:13

That said I can't get this to work in Nightly either, the [learn more] links in the console do work though.
Flags: needinfo?(pbrosset)
Flags: needinfo?(ckerschb)
Comment on attachment 8952776 [details]
Bug 1374741 - Within devtools/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222000/#review229132


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/general/browser_utilityOverlay.js:110
(Diff revision 4)
>      is(tab.linkedBrowser.currentURI.spec, "http://example.org/", "example.org loaded");
>      gBrowser.removeCurrentTab();
>      runNextTest();
>    });
>  
> -  openUILink("http://example.org/", undefined, {
> +  openWebLink("http://example.org/", undefined); // defaults to "current"

Error: 'openweblink' is not defined. [eslint: no-undef]
Comment on attachment 8952892 [details]
Bug 1374741 - Within devtools/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222124/#review229490
Attachment #8952892 - Flags: review?(pbrosset) → review+
Comment on attachment 8952776 [details]
Bug 1374741 - Within devtools/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222000/#review229500

Thanks Jonathan! Purely looking at the code changes, I'm good with them.
However, know that I really don't know enough about security principals and security in general to feel 100% sure about the approach here, so please consider this as a DevTools-peer rubberstamp more than anything else.
Attachment #8952776 - Flags: review?(pbrosset) → review+
Comment on attachment 8952776 [details]
Bug 1374741 - Within devtools/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222000/#review229598

::: browser/base/content/test/general/browser_utilityOverlay.js:110
(Diff revision 4)
>      is(tab.linkedBrowser.currentURI.spec, "http://example.org/", "example.org loaded");
>      gBrowser.removeCurrentTab();
>      runNextTest();
>    });
>  
> -  openUILink("http://example.org/", undefined, {
> +  openWebLink("http://example.org/", undefined); // defaults to "current"

Not obvious from the eslint thing, but this should be openWebLinkIn
Blocks: 1422284
Attachment #8952774 - Attachment is obsolete: true
Attachment #8952775 - Attachment is obsolete: true
Attachment #8952776 - Attachment is obsolete: true
Attachment #8952886 - Attachment is obsolete: true
Attachment #8952892 - Attachment is obsolete: true
Mozreview lost the diff because I obsoleted those first, the patches have only been rebased minus fixing a typo in one of the devtool failing tests which was shadowing the wrong open*LinkIn method.
Comment on attachment 8952892 [details]
Bug 1374741 - Within devtools/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222124/#review240640

Thanks, this looks reasonable to me!  I noticed a few small things to address before landing.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:505
(Diff revision 7)
> -  // Override openUILinkIn to prevent navigating.
> -  let oldOpenUILinkIn = window.openUILinkIn;
> +  // Override LinkIn methods to prevent navigating.
> +  let oldOpenTrustedLinkIn = window.openTrustedLinkIn;
> +  let oldOpenWebLinkIn = window.openWebLinkIn;
>  
>    const onOpenLink = new Promise((resolve) => {
> -    window.openUILinkIn = function(link, where) {
> +    window.openTrustedLinkIn = function(link, where) {

Hmm...  I assume this `simulateLinkClick` only triggers either `openTrustedLinkIn` OR `openWebLinkIn` but not both?

If that's right, I think it would be better to reset both of the `window` properties together, so that we don't leave one of them customized after this helper is done.

(I believe that would also allow assigning the same resolve function body to both `open*LinkIn` properties, rather than separate functions that each handle only one of two.)

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:522
(Diff revision 7)
>        // Click on the link.
>        element.click();
>      }
>    });
>  
>    // Declare a timeout Promise that we can use to make sure openUILinkIn was not called.

Please tweak the function name in this comment.

::: devtools/client/webconsole/test/head.js:1807
(Diff revision 7)
>  function simulateMessageLinkClick(element, expectedLink) {
>    let deferred = defer();
>  
>    // Invoke the click event and check if a new tab would
>    // open to the correct page.
> -  let oldOpenUILinkIn = window.openUILinkIn;
> +  let oldOpenWebLinkIn = window.openWebLinkIn;

I believe the comments from `simulateLinkClick` about resetting both properties together apply here as well.
Attachment #8952892 - Flags: review?(jryans) → review+
Comment on attachment 8966225 [details]
Bug 1374741 - Within devtools/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/234970/#review240672

Overall, it looks good! :) I only noticed one area for possible cleanup.

::: devtools/client/inspector/rules/views/text-property-editor.js:302
(Diff revision 2)
>  
>          if (target.nodeName === "a") {
>            event.stopPropagation();
>            event.preventDefault();
>            let browserWin = this.ruleView.inspector.target.tab.ownerDocument.defaultView;
> -          browserWin.openUILinkIn(target.href, "tab");
> +          let dt = event.dataTransfer;

As other comments seem to mention, this is a click event, so I don't think it will have `dataTransfer`?
Attachment #8966225 - Flags: review?(jryans) → review+
Comment on attachment 8952892 [details]
Bug 1374741 - Within devtools/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/222124/#review240686


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:519
(Diff revision 8)
>        // Click on the link.
>        element.click();
>      }
>    });
>  
> -  // Declare a timeout Promise that we can use to make sure openUILinkIn was not called.
> +  // Declare a timeout Promise that we can use to make sure openTrustedLinkIn or openWebLinkIn was not called.

Error: Line 519 exceeds the maximum line length of 90. [eslint: max-len]
(In reply to Jonathan Kingston [:jkt] from comment #99)
> Mozreview lost the diff because I obsoleted those first, the patches have
> only been rebased minus fixing a typo in one of the devtool failing tests
> which was shadowing the wrong open*LinkIn method.

So do you need me to review all 3 of these again? It sounds like the answer is "no", can you confirm? :-)
Flags: needinfo?(jkt)
I double checked the 3 diffs you reviewed and there isn't any changes on what you reviewed last time and gave an r+ for. The only differences I saw were related to the rebase only.
Flags: needinfo?(jkt) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 8966223 [details]
Bug 1374741 - Within browser/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/234966/#review240876

Stamping this and the other patches (sight unseen) per bug comments that this is just a rebase and I've already reviewed this.
Attachment #8966223 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8966224 [details]
Bug 1374741 - Within browser/ tests make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/234968/#review240878
Attachment #8966224 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8966226 [details]
Bug 1374741 - Within toolkit/ make openUILinkIn() provide the correct triggeringPrincipal.

https://reviewboard.mozilla.org/r/234972/#review240880
Attachment #8966226 - Flags: review?(gijskruitbosch+bugs) → review+
Flags: needinfo?(gijskruitbosch+bugs)
Checkin-needed for all patches including Kates which cleanly applied last night without a conflict.
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3182593eb59a
Within browser/ make openUILinkIn() provide the correct triggeringPrincipal. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e41a8d9dfd3
Within browser/ tests make openUILinkIn() provide the correct triggeringPrincipal. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e2b14dd0dc
Within devtools/ make openUILinkIn() provide the correct triggeringPrincipal. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d9ef19a7fc
Within toolkit/ make openUILinkIn() provide the correct triggeringPrincipal. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e73dd281210
Within devtools/ tests make openUILinkIn() provide the correct triggeringPrincipal. r=jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a13549c696a
Have openUILinkIn provide the correct principal in localstorage tests r=baku
Keywords: checkin-needed
See Also: → 1452678
Depends on: 1453528
Depends on: 1453638
Depends on: 1476234
Depends on: 1535032
You need to log in before you can comment on or make changes to this bug.