Closed Bug 1420702 Opened 7 years ago Closed 7 years ago

Referrer policy ignored in pinned tabs

Categories

(Core :: DOM: Security, defect, P1)

Product:
Core
Component:
DOM: Security
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: oudomphe+devel, Assigned: tnguyen)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(4 files, 1 obsolete file)

Bug 1420702 - Propagate referrer policy when creating window from pinned tab
59 bytes, text/x-review-board-request
ckerschb
: review+
smaug
: review+
Details
Bug 1420702 - Test referrer when clicking cross domain link from pinned tab
59 bytes, text/x-review-board-request
ckerschb
: review+
Details
Beta Part 1 : Propagate referrer policy when creating window from pinned tab. r=ckerschb
29.08 KB, patch
gchang
: approval-mozilla-beta-
Details | Diff | Splinter Review
Beta part 2: Test referrer when clicking cross domain link from pinned tab.
6.74 KB, patch
gchang
: approval-mozilla-beta-
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171121103731

Steps to reproduce:

Open a link with a referrer policy (meta tag or rel=noreferrer attribute) in a pinned tab.

Example:
* Open https://sympli.io/blog/2017/07/13/which-meta-tags-should-you-be-using-in-2017/ which contains <meta name="referrer" content="origin" />
* Pin tab
* Click on the "character encoding" link


Actual results:

Open URL https://www.w3schools.com/tags/att_meta_charset.asp
with Referrer https://sympli.io/blog/2017/07/13/which-meta-tags-should-you-be-using-in-2017/



Expected results:

Open URL https://www.w3schools.com/tags/att_meta_charset.asp
with Referrer https://sympli.io/

If the tab is not pinned, the behaviour is correct.
If the link is opened using Right-Click and "Open in a new Tab", the behaviour is correct, even if the tab is pinned.
Component: Untriaged → DOM: Security
Product: Firefox → Core
Assignee: nobody → tnguyen
Priority: -- → P1
Whiteboard: [domsecurity-active]
Never mind me -- was thinking of the character encoding menu -- I see you mean just a link in the page.

The problem is that pinned tabs can only navigate to the same origin; a different origin opens in a new window. That should be fine: we handle right-click "Open in new tab" correctly. But for some reason the mechanism that traps "oops, not same origin" and spawns the new window is carrying over the URL and not the context. More likely a bug in the front-end code and not in the DOM/security code.
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Never mind me -- was thinking of the character encoding menu -- I see you
> mean just a link in the page.
> 
> The problem is that pinned tabs can only navigate to the same origin; a
> different origin opens in a new window. That should be fine: we handle
> right-click "Open in new tab" correctly. But for some reason the mechanism
> that traps "oops, not same origin" and spawns the new window is carrying
> over the URL and not the context. More likely a bug in the front-end code
> and not in the DOM/security code.

I am biased to keep it in DOM/Security, I took a quick look and it appears the code paths are different between the 2 cases.
For the right-click, there's a click event listener and we run 
https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/browser/base/content/utilityOverlay.js#191
when the referrer policy is propagated correctly.
In the pinned tab
https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/browser/base/content/browser.js#5332
I wrote a patch that propagating referrer policy from child and it helps.
Obviously, referrer policy is missing in this case (which should be sent from child), then we are using the default instead.
(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #3)
> Created attachment 8932707 [details] [diff] [review]
> Add referrer policy when create window from pinned tab

Thanks for looking into this. That Patch looks good to me. Is there something missing or is it ready for review? Happy to help review if needed. Alternatively we could ask smaug for the dom/ bits.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #3)
> > Created attachment 8932707 [details] [diff] [review]
> > Add referrer policy when create window from pinned tab
> 
> Thanks for looking into this. That Patch looks good to me. Is there
> something missing or is it ready for review? Happy to help review if needed.
> Alternatively we could ask smaug for the dom/ bits.

Nice, thanks Christoph, obviously we are missing this sort of tests. I will make the request once the test is ready
Attachment #8932707 - Attachment is obsolete: true
Christoph, smaug, do you mind taking a look at the patch?
Comment on attachment 8933290 [details]
Bug 1420702 - Propagate referrer policy when creating window from pinned tab

https://reviewboard.mozilla.org/r/204228/#review209830

::: dom/base/nsOpenURIInFrameParams.cpp:23
(Diff revision 1)
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsOpenURIInFrameParams)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(nsOpenURIInFrameParams)
>  
>  nsOpenURIInFrameParams::nsOpenURIInFrameParams(const mozilla::OriginAttributes& aOriginAttributes,
>                                                 nsIFrameLoaderOwner* aOpener)
>    : mOpenerOriginAttributes(aOriginAttributes)

mReferrerPolicy should be initialized to some value.

::: dom/interfaces/base/nsIBrowserDOMWindow.idl:18
(Diff revision 1)
>  
>  [scriptable, uuid(e774db14-79ac-4156-a7a3-aa3fd0a22c10)]
>  interface nsIOpenURIInFrameParams : nsISupports
>  {
>    attribute DOMString referrer;
> +  attribute unsigned long referrerPolicy;

So I don't see anyone using the getter for referrerPolicy.
Is the patch missing some part or will there be another patch or what?

::: dom/ipc/ContentChild.cpp:744
(Diff revision 1)
>    }
>  
>    baseURI->GetSpec(aBaseURIString);
>  
> +  bool sendReferrer = true;
> +  aLoadInfo->GetSendReferrer(&sendReferrer);

I don't see anything guaranteeing aLoadInfo is non-null. I could miss something though.
Attachment #8933290 - Flags: review?(bugs) → review-
Comment on attachment 8933290 [details]
Bug 1420702 - Propagate referrer policy when creating window from pinned tab

https://reviewboard.mozilla.org/r/204228/#review209830

Thanks smaug for the review

> So I don't see anyone using the getter for referrerPolicy.
> Is the patch missing some part or will there be another patch or what?

This is called from js, but undefined for now (then we used default, that was incorrect)
https://searchfox.org/mozilla-central/rev/be78e6ea9b10b1f5b2b3b013f01d86e1062abb2b/browser/base/content/browser.js#5355
Flags: needinfo?(bugs)
oh, bizarre.
Flags: needinfo?(bugs)
Comment on attachment 8933290 [details]
Bug 1420702 - Propagate referrer policy when creating window from pinned tab

https://reviewboard.mozilla.org/r/204228/#review209862
Attachment #8933290 - Flags: review+
Comment on attachment 8933290 [details]
Bug 1420702 - Propagate referrer policy when creating window from pinned tab

https://reviewboard.mozilla.org/r/204228/#review209924

nice - thanks!
Attachment #8933290 - Flags: review?(ckerschb) → review+
Comment on attachment 8933291 [details]
Bug 1420702 - Test referrer when clicking cross domain link from pinned tab

https://reviewboard.mozilla.org/r/204230/#review209926

those tests are really nice, r=me
Attachment #8933291 - Flags: review?(ckerschb) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Pushed by tnguyen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a6f1cc40f27e
Propagate referrer policy when creating window from pinned tab r=ckerschb,smaug
https://hg.mozilla.org/integration/autoland/rev/0f6ee76c5393
Test referrer when clicking cross domain link from pinned tab r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/a6f1cc40f27e
https://hg.mozilla.org/mozilla-central/rev/0f6ee76c5393
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1426702
Comment on attachment 8939479 [details] [diff] [review] [diff] [review]
Beta Part 1 : Propagate referrer policy when creating window from pinned tab. r=ckerschb

The patches fix not only pinned tab, but also other cases which use the same code path (for example bug 1426702)
It would be qualified for fixing in beta, but I am not sure whether it is a bit late or not.

Approval Request Comment
[Feature/Bug causing the regression]: No
[User impact if declined]: Leak/incorrect referrer header 
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Low.
[Why is the change risky/not risky?]: The fix added new parameters to some calls and propagate referrer policy to nsIOpenURIInFrameParams idl interface. The policy value then will be passed between child/parent and then be passed to JS. The usage of the policy in js will not be changed. Low risk.
[String changes made/needed]: No
Comment on attachment 8939480 [details] [diff] [review]
Beta part 2: Test referrer when clicking cross domain link from pinned tab.

Approval Request Comment
[Feature/Bug causing the regression]: No
[User impact if declined]: No, automation test only
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Test only.
[String changes made/needed]: No
Attachment #8939480 - Flags: approval-mozilla-beta?
(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #23)
> Comment on attachment 8939479 [details] [diff] [review]
> Beta Part 1 : Propagate referrer policy when creating window from pinned
> tab. r=ckerschb
> 
> The patches fix not only pinned tab, but also other cases which use the same
> code path (for example bug 1426702)
> It would be qualified for fixing in beta, but I am not sure whether it is a
> bit late or not.
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: No
> [User impact if declined]: Leak/incorrect referrer header 
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: 
> [List of other uplifts needed for the feature/fix]: No.
> [Is the change risky?]: Low.
> [Why is the change risky/not risky?]: The fix added new parameters to some
> calls and propagate referrer policy to nsIOpenURIInFrameParams idl
> interface. The policy value then will be passed between child/parent and
> then be passed to JS. The usage of the policy in js will not be changed. Low
> risk.
> [String changes made/needed]: No

[Needs manual test from QE? If yes, steps to reproduce]: Follow comment 0 and 1426702 comment 0
Comment on attachment 8939479 [details] [diff] [review]
Beta Part 1 : Propagate referrer policy when creating window from pinned tab. r=ckerschb

Too late for 58. Let's let it ride the train. Beta58-
Attachment #8939479 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8939480 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
No longer blocks: 1426702
Blocks: 1370971
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: