Referer is sent when opening a new private window

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: poubelle06210, Assigned: groovecoder)

Tracking

56 Branch
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox-esr52 affected, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171002220106

Steps to reproduce:

Right-click on a link and click on "Open this link in a new private window" (not sure on the terms because I use Firefox in French).

You can use thsi URL to test: https://www.jeffersonscher.com/res/jstest.php


Actual results:

Referer is sent


Expected results:

Referer should not be sent
Component: Untriaged → Private Browsing

Comment 1

a year ago
Reproducible on Firefox Nightly 58

Updated

a year ago
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → affected
Version: 56 Branch → 20 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 20 Branch → 56 Branch
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review195842

The changes look fine to me, but a Firefox reviewer should really sign off on this patch.
Attachment #8919522 - Flags: review?(josh) → review?(mdeboer)

Comment 4

a year ago
mozreview-review
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review195916

Thanks for working on this! I think we can apply this logic on an even deeper level!

::: browser/base/content/nsContextMenu.js:790
(Diff revision 1)
>  
>    // Open linked-to URL in a new private window.
>    openLinkInPrivateWindow() {
>      urlSecurityCheck(this.linkURL, this.principal);
>      openLinkIn(this.linkURL, "window",
> -               this._openLinkInParameters({ private: true }));
> +               this._openLinkInParameters({ private: true, noReferrer: true }));

Wouldn't it be even better if we were to make sure that `aNoReferer` is set to `true` when `aPrivate` is `true` in the `openLinkIn()` function inside utilityOverlay.js?
Because that's what we end up calling here too and in many other places.
That way we'll prevent this issue to ever occur again for other future callsites.
Attachment #8919522 - Flags: review?(mdeboer)
> ::: browser/base/content/nsContextMenu.js:790
> (Diff revision 1)
> >  
> >    // Open linked-to URL in a new private window.
> >    openLinkInPrivateWindow() {
> >      urlSecurityCheck(this.linkURL, this.principal);
> >      openLinkIn(this.linkURL, "window",
> > -               this._openLinkInParameters({ private: true }));
> > +               this._openLinkInParameters({ private: true, noReferrer: true }));
> 
> Wouldn't it be even better if we were to make sure that `aNoReferer` is set
> to `true` when `aPrivate` is `true` in the `openLinkIn()` function inside
> utilityOverlay.js?
> Because that's what we end up calling here too and in many other places.
> That way we'll prevent this issue to ever occur again for other future
> callsites.

Would that also cause referrers to be stripped from links clicked in private tabs?
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review195916

I've got a fix for `openLinkIn` but I'm not sure how best so submit for follow-up review? As a new commit, or squash into a single commit?

> Wouldn't it be even better if we were to make sure that `aNoReferer` is set to `true` when `aPrivate` is `true` in the `openLinkIn()` function inside utilityOverlay.js?
> Because that's what we end up calling here too and in many other places.
> That way we'll prevent this issue to ever occur again for other future callsites.

That makes good sense. I'll dig into that deeper level of code (too).
(In reply to Luke Crouch [:groovecoder] from comment #6)
> I've got a fix for `openLinkIn` but I'm not sure how best so submit for
> follow-up review? As a new commit, or squash into a single commit?

I'd squash it.

(In reply to Tanvi Vyas[:tanvi] from comment #5)
> Would that also cause referrers to be stripped from links clicked in private
> tabs?

Links clicked in private tabs open in the same tab or in a new private tab (because the window is private) and may have referrer info set.

Regardless, the code should be added in the `if (!w || where == "window") {` block, so that it only applies for new to open private windows, so that the referrer info doesn't leak from non-private to private contexts.

Does that make sense?
Assignee: nobody → lcrouch
Status: NEW → ASSIGNED
(In reply to Mike de Boer [:mikedeboer] from comment #7)
> (In reply to Tanvi Vyas[:tanvi] from comment #5)
> > Would that also cause referrers to be stripped from links clicked in private
> > tabs?
> 
> Links clicked in private tabs open in the same tab or in a new private tab
> (because the window is private) and may have referrer info set.
> 
> Regardless, the code should be added in the `if (!w || where == "window") {`
> block, so that it only applies for new to open private windows, so that the
> referrer info doesn't leak from non-private to private contexts.
> 
> Does that make sense?

Thanks for clarifying!
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8920321 - Flags: review?(mdeboer)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8920321 - Attachment is obsolete: true
Attachment #8920321 - Flags: review?(mdeboer)

Comment 11

a year ago
mozreview-review
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review197210

::: browser/base/content/utilityOverlay.js:241
(Diff revision 2)
>    var aTriggeringPrincipal  = params.triggeringPrincipal;
>    var aForceAboutBlankViewerInCurrent =
>        params.forceAboutBlankViewerInCurrent;
>  
> +  // Enforce aNoReferrer true when aIsPrivate is true
> +  if (aIsPrivate) {

This is incorrect, because this enforces the rule for all `where` targets.
We only want this when a new window is opened, so please look for the line `if (!w || where == "window") {` and add the code inside that block at the appropriate position.

Can you also make the comment above it more of an explainer of what it does and why it does it?

Thanks!
Attachment #8919522 - Flags: review?(mdeboer) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review197210

> This is incorrect, because this enforces the rule for all `where` targets.
> We only want this when a new window is opened, so please look for the line `if (!w || where == "window") {` and add the code inside that block at the appropriate position.
> 
> Can you also make the comment above it more of an explainer of what it does and why it does it?
> 
> Thanks!

Moved the code inside the `where == "window"` block, and improved the comment above it.

Comment 14

a year ago
mozreview-review
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review197300

r=me with nits fixed, because I think the code is already in the right place.

Please push the final changeset to Try prior to landing to catch any test failures that this patch may cause, even though we don't suspect any.
You can trigger a Try build using the 'Automation' menu in this ReviewBoard tool, just like you can trigger the automatic landing of this patch once all lights are green.

Thanks for working on this!

::: commit-message-7b754:1
(Diff revision 3)
> +Bug 1409226 - strip referer when opening link into private window

Please fix this commit message to explain what it's really doing.

::: browser/base/content/test/referrer/browser_referrer_open_link_in_private.js:4
(Diff revision 3)
>  // Tests referrer on context menu navigation - open link in new private window.
>  // Selects "open link in new private window" from the context menu.
>  
> +// The test runs from a regular browsing window

nit: missing dot.

::: browser/base/content/test/referrer/head.js:81
(Diff revision 3)
>  function getReferrerTest(aTestNumber) {
>    return _referrerTests[aTestNumber];
>  }
>  
>  /**
> + * Returns  shimmed test object for a given test number.

nit: Superfluous space between 'Returns' and 'shimmed'. You may also add an empty comment line below the description and '@param' lines.

::: browser/base/content/utilityOverlay.js:293
(Diff revision 3)
>    aTriggeringPrincipal = useOAForPrincipal(aTriggeringPrincipal);
>  
>    if (!w || where == "window") {
> +    // To prevent regular browsing data from leaking to private
> +    // browsing sites, strip the referrer when opening a new private
> +    // window (See Bug: 1409226)

nit: missing dot at the end and can you make this comment span two lines instead of three?
Attachment #8919522 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8919522 [details]
Bug 1409226 - When opening a link into a new private window, remove Referer.

https://reviewboard.mozilla.org/r/190356/#review197356

::: browser/base/content/nsContextMenu.js:790
(Diff revision 4)
>  
>    // Open linked-to URL in a new private window.
>    openLinkInPrivateWindow() {
>      urlSecurityCheck(this.linkURL, this.principal);
>      openLinkIn(this.linkURL, "window",
> -               this._openLinkInParameters({ private: true }));
> +               this._openLinkInParameters({ private: true, noReferrer: true }));

O I missed this! You can discard this change, since you fixed it in utilityOverlay.js.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

a year ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fb52bba8d5c
When opening a link into a new private window, remove Referer. r=mikedeboer
Backed out for eslint failure at browser/base/content/utilityOverlay.js:210: Function 'openLinkIn' has a complexity of 41:

https://hg.mozilla.org/integration/autoland/rev/5129a4cc02b95dd53f295027e000823a1f6050c5

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1fb52bba8d5c8880fb45cc197990243ad3c5190f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139221195&repo=autoland
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/utilityOverlay.js:210:1 | Function 'openLinkIn' has a complexity of 41. (complexity)
Flags: needinfo?(lcrouch)
(Assignee)

Comment 21

a year ago
I re-factored my code but when I ran the linter again, a different error comes up in a different file that I haven't touched?

(firefox-bootstrap)groovecoder:mozilla-unified lcrouch$ ./mach lint -l eslint -f treeherder
TEST-UNEXPECTED-ERROR | /Users/lcrouch/code/mozilla/src/mozilla-unified/devtools/server/tests/mochitest/inspector_getImageData.html:13:7 | Unexpected var, use let or const instead. (mozilla/var-only-at-top-level)
Flags: needinfo?(lcrouch) → needinfo?(aryx.bugmail)
If you haven't touched the file and not eslint stuff, feel free to ignore. Running |mach lint| against that file on m-c passes for me.
Flags: needinfo?(aryx.bugmail)
Comment hidden (mozreview-request)

Comment 24

a year ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a442cf4ecb7
When opening a link into a new private window, remove Referer. r=mikedeboer

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a442cf4ecb7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
status-firefox56: affected → wontfix
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.