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)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 50
People
(Reporter: baku, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-active][usercontextId][uplift49+])
Attachments
(2 files, 1 obsolete file)
22.36 KB,
patch
|
mconley
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
20.54 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Mike, do you have time to review this patch?
Attachment #8759531 -
Flags: review?(mconley)
Updated•8 years ago
|
Whiteboard: [domsecurity-active]
Comment 2•8 years ago
|
||
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]
Assignee | ||
Comment 3•8 years ago
|
||
Right. This is a information leak. To me it's P1 for the same reason why bug 1276880 is it.
Priority: P2 → P1
Comment 5•8 years ago
|
||
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-
Assignee | ||
Comment 6•8 years ago
|
||
> 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)
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
(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)
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•8 years ago
|
||
> > 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 12•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: needinfo?(mconley)
Assignee | ||
Comment 13•8 years ago
|
||
> 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.
Comment 14•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/07dc0094e76f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 16•8 years ago
|
||
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?
Assignee | ||
Comment 20•8 years ago
|
||
> 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.
Updated•8 years ago
|
status-firefox49:
--- → affected
Comment 21•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: qe-verify+
Comment 22•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf366a3b54d2
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8775931 -
Attachment is patch: true
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e62247fa043
Comment 26•8 years ago
|
||
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
Comment 27•8 years ago
|
||
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.
Description
•