Closed Bug 1409226 Opened 7 years ago Closed 7 years ago

Referer is sent when opening a new private window

Categories

(Firefox :: Private Browsing, defect)

Product:
Firefox
Component:
Private Browsing
Version:
56 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox-esr52 --- affected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: poubelle06210, Assigned: groovecoder)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1409226 - When opening a link into a new private window, remove Referer.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
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
Reproducible on Firefox Nightly 58
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 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 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?
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!
Attachment #8920321 - Flags: review?(mdeboer)
Attachment #8920321 - Attachment is obsolete: true
Attachment #8920321 - Flags: review?(mdeboer)
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 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 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 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.
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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/1a442cf4ecb7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: