Closed
Bug 1420702
Opened 7 years ago
Closed 7 years ago
Referrer policy ignored in pinned tabs
Categories
(Core :: DOM: Security, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: oudomphe+devel, Assigned: tnguyen)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(4 files, 1 obsolete file)
(deleted),
text/x-review-board-request
|
ckerschb
:
review+
smaug
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ckerschb
:
review+
|
Details |
(deleted),
patch
|
gchang
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
(deleted),
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.
Updated•7 years ago
|
Component: Untriaged → DOM: Security
Product: Firefox → Core
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tnguyen
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [domsecurity-active]
Comment hidden (obsolete) |
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8932707 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
Christoph, smaug, do you mind taking a look at the patch?
Comment 10•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment 15•7 years ago
|
||
mozreview-review |
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 16•7 years ago
|
||
mozreview-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 17•7 years ago
|
||
mozreview-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+
Assignee | ||
Updated•7 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
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
Assignee | ||
Comment 20•7 years ago
|
||
MozReview-Commit-ID: 1kMssKbAm1l
Assignee | ||
Comment 21•7 years ago
|
||
MozReview-Commit-ID: DokwVqZcrx7
Comment hidden (typo) |
Assignee | ||
Comment 23•7 years ago
|
||
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
Assignee | ||
Comment 24•7 years ago
|
||
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?
Assignee | ||
Comment 25•7 years ago
|
||
(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 26•7 years ago
|
||
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-
Updated•7 years ago
|
Attachment #8939480 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•7 years ago
|
status-firefox58:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•