Closed Bug 1409226 Opened 5 years ago Closed 5 years ago

Referer is sent when opening a new private window

Tracking

(bug RESOLVED as FIXED found in Firefox _esr52)

RESOLVED FIXED

Firefox 58

Tracking	Status
firefox-esr52	---	affected
firefox56	---	wontfix
firefox57	---	wontfix
firefox58	---	fixed

People

(Reporter: poubelle06210, Assigned: groovecoder)

Details

	mooms Reporter
Description • 5 years 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

Reproducible on Firefox Nightly 58

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?

	Luke Crouch [:groovecoder] Assignee
Comment 6 • 5 years 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!