Closed Bug 1277765 Opened 8 years ago Closed 8 years ago

No referrer when a link is opened by middle-click or the context menu and containers are involved

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox49 --- verified
firefox50 --- verified

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active][usercontextId][uplift49+])

Attachments

(2 files, 1 obsolete file)

Currently we are not consistent about propagating the referrers when a link is opened by a middle-click or from the context menu. This patch introduces several tests and a better policy.
Attached patch referrer.patch (obsolete) — Splinter Review
Mike, do you have time to review this patch?
Attachment #8759531 - Flags: review?(mconley)
Blocks: 1276880
Whiteboard: [domsecurity-active]
If a new link is opened in the same container tab, we should send referrer.  If a new link is opened in a different container tab, we should send no referrer.
Priority: -- → P2
Whiteboard: [domsecurity-active] → [domsecurity-active][userContextId]
Right. This is a information leak. To me it's P1 for the same reason why bug 1276880 is it.
Priority: P2 → P1
Comment on attachment 8759531 [details] [diff] [review]
referrer.patch

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

It's unfortunate that these tests doesn't use add_task - I feel like that'd help make it clearer what's happening when. However, since the pattern for these tests has already been established, I won't harp on it.

My only real issue here is the duplication of logic at the callsites for setting noReferrer - I feel like we should probably try to have that centralized at openLinkIn. See below.

::: browser/base/content/browser.js
@@ +5513,5 @@
>                   noReferrer: BrowserUtils.linkHasNoReferrer(linkNode) };
> +
> +  // The new tab/window will not use the same userContextId, so we don't have
> +  // to 'leak' the referrer.
> +  if (doc.nodePrincipal.originAttributes.userContextId) {

It's kind of nasty to have to do this at all of the call sites. Can we move this logic into openLinkIn?

::: browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js
@@ +16,5 @@
> +    });
> +
> +    let menu = gTestWindow.document.getElementById("context-openlinkinusercontext-menu");
> +    let menupopup = menu.menupopup;
> +    menupopup.showPopup();

This feels like it might be race-y. Are you sure you don't need to wait for popupshown to be fired on the menupopup?

::: browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab3.js
@@ +26,5 @@
> +    });
> +
> +    let menu = gTestWindow.document.getElementById("context-openlinkinusercontext-menu");
> +    let menupopup = menu.menupopup;
> +    menupopup.showPopup();

Same as the other test - need to wait for popupshown?

::: browser/base/content/test/referrer/browser_referrer_open_link_in_window_in_container.js
@@ +29,5 @@
> +  });
> +}
> +
> +function test() {
> +  requestLongerTimeout(10);  // slowwww shutdown on e10s

Do we know why it's such a slow shutdown?
Attachment #8759531 - Flags: review?(mconley) → review-
> It's kind of nasty to have to do this at all of the call sites. Can we move
> this logic into openLinkIn?

No. I don't think we can. The reason is that openLinkIn is used in too many places:

. contextMenu - and this is OK
. UI - and the callee has a totally different OriginAttributes
. Addons?
. ContentClick.jsm (parent to child)
. tests :)

> This feels like it might be race-y. Are you sure you don't need to wait for
> popupshown to be fired on the menupopup?

oh. yeah. maybe :) I copied that pattern from somewhere else.

> Do we know why it's such a slow shutdown?

Again, copy and paste.

NI: A part the popupshown issue, give me a feedback about openLinkIn.
Flags: needinfo?(mconley)
(In reply to Andrea Marchesini (:baku) from comment #6)
> 
> NI: A part the popupshown issue, give me a feedback about openLinkIn.

For popupshown, normally we wait for it to show up if we're going to interact with it. Similarly, we need to wait for popuphidden to fire on the menupopup to make sure it has fully gone away when it is closed. Popup behaviour is pretty finicky - I'd be interested in seeing a try run with these tests.

Regarding openLinkIn... huh, so what you're saying is, that we don't currently have access from params to the document or node principal of the thing requesting the link be opened? If so, perhaps we should change that, and pass the principal instead when we care about things like the user context.
Flags: needinfo?(mconley)
We don't have always the principal when openLinkIn is called. Or more precisely, it can be a systemPrincipal.
For instance, when openLinkIn is used to open a new tab or a new window.

I can introduce the principal (or the OriginAttributes) as optional param, but it could be quite fragile.
Flags: needinfo?(mconley)
Attached patch referrer.patchSplinter Review
The new UX is easier. We want to apply the same UCI in the new tab.
Attachment #8759531 - Attachment is obsolete: true
Attachment #8760741 - Flags: review?(mconley)
(In reply to Andrea Marchesini (:baku) from comment #8)
> We don't have always the principal when openLinkIn is called. Or more
> precisely, it can be a systemPrincipal.
> For instance, when openLinkIn is used to open a new tab or a new window.
> 
> I can introduce the principal (or the OriginAttributes) as optional param,
> but it could be quite fragile.

Can you elaborate on the fragility? I feel like having the nsIPrincipal of the originating requestor for the openLinkIn would be useful in general. I'm interested in understanding how this would be fragile.
Flags: needinfo?(mconley)
Flags: needinfo?(amarchesini)
Status: NEW → ASSIGNED
> > I can introduce the principal (or the OriginAttributes) as optional param,
> > but it could be quite fragile.


openLinkIn is used everywhere: our code and addons. If we add the principal of the callee, this will be a optional params because we don't want to break any existing addons. Plus, often, also in our code, the principal is 'system', in particular for top-level URI.

I prefer to have the check about userContextId closer to where we decide how to open the link: context menu or click handling.

This is my point of view, but I'm open to a different approach. I really would like you to take a look at the patch, because it is very different than what you saw last time. Thanks!
Flags: needinfo?(amarchesini) → needinfo?(mconley)
Comment on attachment 8760741 [details] [diff] [review]
referrer.patch

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

Yeah, I think this is better. I think we're centralizing the logic a lot better here - thanks for that. :)

Just a couple of notes to fix before landing, but nothing major. Also, I'm not going to tell you re-write all of the (awesome) tests you just added, but we should probably opt for using add_task and the BrowserTestUtils stuff where we acn. Tests like browser_referrer_open_link_in_tab_in_container.js are using the old-school testing mechanism so they don't get the task-y / BTU goodness. Would you mind following a follow-up to get all of the tests converted to the add_task style? Might be good volunteer contributor work.

::: browser/base/content/browser.js
@@ +5512,5 @@
>                   referrerPolicy: referrerPolicy,
>                   noReferrer: BrowserUtils.linkHasNoReferrer(linkNode) };
> +
> +  // The new tab/window must use the same userContextId
> +  if (doc.nodePrincipal.originAttributes.userContextId) {

Do we need to worry about the case when userContextId is 0 at all here?

::: browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js
@@ +33,5 @@
> +      is(firstContext.nodeType, Node.ELEMENT_NODE, "We have a first container entry.");
> +      ok(firstContext.hasAttribute('usercontextid'), "We have a usercontextid value.");
> +
> +      firstContext.doCommand();
> +      aContextMenu.hidePopup();

You might also want to wait for the popuphidden event to fire after hidePopup.

This will probably work as is, but waiting for completions helps to avoid event bleedover in other tests which can cause oranges.

::: browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab2.js
@@ +16,5 @@
> +    });
> +
> +    let menu = gTestWindow.document.getElementById("context-openlinkinusercontext-menu");
> +    let menupopup = menu.menupopup;
> +    menu.addEventListener('popupshown', function() {

Mixing single and double quotes.

::: browser/components/contextualidentity/test/browser/browser_middleClick.js
@@ +11,5 @@
> +  let tab = gBrowser.addTab(uri, {userContextId});
> +
> +  // select tab and make sure its browser is focused
> +  gBrowser.selectedTab = tab;
> +  tab.ownerDocument.defaultView.focus();

Focus isn't synchronous - you might need to do SimpleTest.waitForFocus.

Also, why are you needing to focus, out of curiosity?

Also, could this entire function be removed, and we just use the following in the test add_task:

```
let tab = gBrowser.addTab(uri, { userContextId: 1 });
yield BrowserTestUtils.browserLoaded(browser);
let browser = tab.linkedBrowser;
```

?

@@ +24,5 @@
> +  let data = yield* openTabInUserContext(URI, 1);
> +
> +  info("Create a HTMLAnchorElement...");
> +  let position = yield ContentTask.spawn(data.browser, URI,
> +    function(url) {

Seems a bit weird here to conflate URI and url... maybe be consistent and just use URI for this function argument as well.

@@ +25,5 @@
> +
> +  info("Create a HTMLAnchorElement...");
> +  let position = yield ContentTask.spawn(data.browser, URI,
> +    function(url) {
> +      let anchor = content.document.createElement('a');

Mixing single quotes and double. Let's go with double.

@@ +30,5 @@
> +      anchor.setAttribute('href', url);
> +      anchor.appendChild(content.document.createTextNode('click me!'));
> +      content.document.body.appendChild(anchor);
> +      let rect = anchor.getBoundingClientRect();
> +      return { x: (rect.x + rect.width / 2), y: (rect.y + rect.height / 2) };

I don't think you need the position - you can use BrowserTestUtils.synthesizeMouseAtCenter, can you not?

@@ +35,5 @@
> +    }
> +  );
> +
> +  info("Synthesize a mouse click and wait for a new tab...");
> +  let tab = yield new Promise((resolve, reject) => {

You can (and should) use BrowserTestUtils.waitForNewTab instead of rolling your own listener.

::: browser/modules/ContentClick.jsm
@@ +83,5 @@
>                     noReferrer: json.noReferrer,
>                     allowMixedContent: json.allowMixedContent };
>  
> +    // The new tab/window must use the same userContextId.
> +    if (json.originAttributes.userContextId) {

If the userContextId is 0, we're okay to not set it, right?
Attachment #8760741 - Flags: review?(mconley) → review+
Flags: needinfo?(mconley)
> Do we need to worry about the case when userContextId is 0 at all here?

Right. We don't pass 'userContextId' if it's == 0.

> I don't think you need the position - you can use
> BrowserTestUtils.synthesizeMouseAtCenter, can you not?

> You can (and should) use BrowserTestUtils.waitForNewTab instead of rolling
> your own listener.

If I do this, the test doesn't work and I see a lot JS errors related to event propagation.
I have to investigate again why I had such errors. I'll NI when I have an answer.

In the meantime I file the followup as you suggested.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07dc0094e76f
No referrer when a link is opened by middle-click or the context menu and containers are involved, r=mconley
https://hg.mozilla.org/mozilla-central/rev/07dc0094e76f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Please file bugs in the product / component where the relevant code lives.
Component: DOM: Security → General
Product: Core → Firefox
Target Milestone: mozilla50 → Firefox 50
Seems like this would be important for test pilot.
Whiteboard: [domsecurity-active][userContextId] → [domsecurity-active][usercontextId][uplift49+]
Comment on attachment 8760741 [details] [diff] [review]
referrer.patch

Approval Request Comment
[Feature/regressing bug #]: Testpilot for containers
[User impact if declined]: Referrer will net be set properly when opening links in containers
[Describe test coverage new/current, TreeHerder]: Patch includes test coverage of the issue fixed.
[Risks and why]: This involves platform changes that cannot be reprecated by an addon.  In order to do a Test Pilot experiment for Containers in September / Firefox 49 release, we need this code in release.  Without it, testing is delayed until November and getting the capabilities of the feature to users is delayed even further. Note that code has already landed in Nightly for a month now.
[String/UUID change made/needed]:None
Attachment #8760741 - Flags: approval-mozilla-aurora?
(In reply to Paul Theriault [:pauljt] from comment #18)
> Comment on attachment 8760741 [details] [diff] [review]
> referrer.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]: Testpilot for containers
> [User impact if declined]: Referrer will net be set properly when opening
> links in containers
> [Describe test coverage new/current, TreeHerder]: Patch includes test
> coverage of the issue fixed.
> [Risks and why]: This involves platform changes that cannot be reprecated by
> an addon.  In order to do a Test Pilot experiment for Containers in
> September / Firefox 49 release, we need this code in release.  Without it,
> testing is delayed until November and getting the capabilities of the
> feature to users is delayed even further. Note that code has already landed
> in Nightly for a month now.
> [String/UUID change made/needed]:None

Just adding more information to elaborate on the risk/benefit of this patch as requested via email. The benefit of this patch is that fixes middle click behavior when using Containers - it has no effect on regular users, only for people who have enabled containers, which will only be enabled in the Test Pilot. We _might_ be able to live without this for the test pilot (could we workaround perhaps with the addon, or maybe worst case maybe we highlight it as a known issue?). 

But the actual change is small, just a few lines in several files to propagate an origin value. Most of this patch is to add automated tests to confirm the fix. On that basis, it seems like this would be low risk change. Is that accurate Andrea?
> But the actual change is small, just a few lines in several files to
> propagate an origin value. Most of this patch is to add automated tests to
> confirm the fix. On that basis, it seems like this would be low risk change.
> Is that accurate Andrea?

I agree. The risk is low. The patch seems big because I touched many tests to see if the referrer propagation works correctly.
Comment on attachment 8760741 [details] [diff] [review]
referrer.patch

OK, let's take it to be ready in time for 49
Attachment #8760741 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
backed out from aurora, this breaks there bc tests like https://treeherder.mozilla.org/logviewer.html#?job_id=3132223&repo=mozilla-aurora
Flags: needinfo?(amarchesini)
Attached patch m-aSplinter Review
This issue happened because we landed bug 1276880 before this. This bug introduces some tests, but bug 1276880 changed part of them. Here we have the patch for this bug but with the tests merged with bug 1276880.
Flags: needinfo?(amarchesini)
Attachment #8775931 - Attachment is patch: true
While I was trying to verify this bug I encountered bug 1289001 . I updated it with STR and some crash reports. Comment 10 bug 1289001. bug 1289001#c10
I can confirm the issue is fixed, I verified on Fx 50.0(20161104212021) and Fx 49.0.2(20161019084923) on Windows 10 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.