<meta referrer> origin-when-crossorigin sets incorrect referrer

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: averstak, Assigned: geekboy)

Tracking

36 Branch
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.38 Safari/537.36

Steps to reproduce:

1. Open https://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin .
Click on "Creating reality: How TV news distorts events".
Open the console and type document.referrer to see the Referer header.

2. Repeat 1, but via plain HTTP: http://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin .

3. Open https://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin .
Click on "Cited by 739" under the second result (same origin link).
Open the console and type document.referrer.


Actual results:

1. document.referrer is empty - this is incorrect, should be origin.

2. document.referrer = "http://scholar.google.com" - this is correct, so it works over plain HTTP.

3. document.referrer = "https://scholar.google.com" - this is incorrect, it should be the full URL.  (Same issue over plain HTTP.)


Expected results:

1. document.referrer = "https://scholar.google.com".

2. document.referrer = "http://scholar.google.com".

3. document.referrer = "https://scholar.google.com/scholar?q=news&mref=origin-when-crossorigin".

It looks like the implementation of meta-referrer in Bug 704320 has two separate issues with origin-when-crossorigin:

A. It drops Referer entirely for cross-origin HTTPS -> HTTP links.
B. It limits Referer to origin for same-origin links, in both HTTP and HTTPS.

The appears to disagree with the spec:

http://w3c.github.io/webappsec/specs/referrer-policy/

"""
3.4. Origin When Cross-Origin

The Origin When Cross-Origin policy specifies that a full URL, stripped for use as a referrer, is sent as referrer information when making same-origin requests from a particular global environment, and only the ASCII serialization of the origin of the global environment from which a request is made is sent as referrer information when making cross-origin requests from a particular global environment.
"""

Updated

5 years ago
Component: General → DOM

Updated

5 years ago
Flags: needinfo?(sstamm)
(In reply to Alex Verstak from comment #0)
> It looks like the implementation of meta-referrer in Bug 704320 has two
> separate issues with origin-when-crossorigin:
> 
> A. It drops Referer entirely for cross-origin HTTPS -> HTTP links.
> B. It limits Referer to origin for same-origin links, in both HTTP and HTTPS.

Hm, it looks like somehow we missed covering origin-when-crossorigin in the tests:
http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug704320.html?force=1

I can understand why A may make sense.  It deviates from the spec, but the user agent may not want to leak any referrer data from https to http.  But if it's origin, it probably doesn't leak much and we should likely fix A.

B needs to be fixed.  Let me dig into this.
Assignee: nobody → sstamm
Blocks: 704320
(In reply to Alex Verstak from comment #0)
> 1. document.referrer is empty - this is incorrect, should be origin.

I can reproduce the first case.  This is https to http, here's why the referrer gets dropped:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1041

The key part of the spec is this:
"Note: The Origin When Cross-Origin policy causes the origin of HTTPS referrers to be sent over the network as part of unencrypted HTTP requests."

> 2. document.referrer = "http://scholar.google.com" - this is correct, so it
> works over plain HTTP.

Works as intended.

> 3. document.referrer = "https://scholar.google.com" - this is incorrect, it
> should be the full URL.  (Same issue over plain HTTP.)

I cannot reproduce this, I get a full referrer here on Nightly (seems to work as intended).
Flags: needinfo?(sstamm)
Posted patch fix (obsolete) — Splinter Review
This seems to fix the https->http xorigin referrer issue.  Needs tests... if someone else wants to take a stab at testing this, go for it.  Otherwise, I will try to get to it in a week (taking some time off for the holidays).
Reporter

Comment 4

5 years ago
Posted patch Tests that don't pass (obsolete) — Splinter Review
Added tests, but some cases don't pass:
1. window.open with http->http and https->https - loadingURI is null, so it treats this as cross-origin and sends an origin referrer - http://example.com or https://example.com;
2. link with http->http and https->https - loadingURI is the top frame, http://mochi.test:8888/..., so it thinks this is cross-origin and sends an origin referrer - http://example.com or https://example.com.

Perhaps it should use the referrer, not the loading principal, for the origin-when-crossorigin check?  The referring page here is the iframe, not the top-level frame, and the Referer header is based on the iframe.  I'm not sure - it's best to ask someone competent.

(Without frames, this fix works just fine.)
Reporter

Comment 5

5 years ago
The iframes work as expected (by me :) using the triggering principal for the cross-origin check, as suggested in bug 1091883.

Sid: back to you.
Attachment #8541517 - Attachment is obsolete: true
Thanks, Alex!  Just quickly, your patch looks right.  We landed the initial meta referrer patch before triggeringPrincipal was available, and you're right in switching this implementation to use it!  

I'll look closely and see if we can't get this landed early next week.  Do you mind if I assign the bug to you (for credit) since you did all the hard work?
Flags: needinfo?(averstak)
Assignee

Updated

5 years ago
Attachment #8541586 - Flags: review?(sstamm)
Assignee

Updated

5 years ago
Attachment #8539364 - Attachment is obsolete: true
Reporter

Comment 7

5 years ago
Great, thanks for the quick responses, Sid!

Re bug ownership - either way is fine by me, though it'll probably be up to you to do a try run and to get it landed.  Your call.
Flags: needinfo?(averstak)
Comment on attachment 8541586 [details] [diff] [review]
Fix and tests, using triggering principal for cross-origin check

Review of attachment 8541586 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great from my perspective.  The tests are what I expected and the use of triggering principal seems right.  I'm not a necko peer, so I'm gonna flag mcmanus for a review (he reviewed the other meta referrer stuff in necko) and flag Tanvi just to make sure the use of triggering principal makes sense here.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1083,3 @@
>      isCrossOrigin = NS_FAILED(rv);
>    } else {
> +    NS_WARNING("no triggering principal available via loadInfo, assumming load is cross-origin");

I know, I spelled this wrong originally, but assuming should only have one "m".  Please fix while you're modifying this.
Attachment #8541586 - Flags: review?(sstamm)
Attachment #8541586 - Flags: review?(mcmanus)
Attachment #8541586 - Flags: review+
Attachment #8541586 - Flags: feedback?(tanvi)
Comment on attachment 8541586 [details] [diff] [review]
Fix and tests, using triggering principal for cross-origin check

TriggeringPrincipal looks right here.  (And I've actually been looking at some behavior in nsDocShell that supports this change - https://etherpad.mozilla.org/docShellPrincipals line 38.)

How does this relate to bug 1091883?  I still think we should test what happens in the scenario described in that bug.
Attachment #8541586 - Flags: feedback?(tanvi) → feedback+
Reporter

Comment 10

5 years ago
Yep, this also fixes the case in bug 1091883.  Added a test to that bug.
Comment on attachment 8541586 [details] [diff] [review]
Fix and tests, using triggering principal for cross-origin check

Review of attachment 8541586 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1075,5 @@
> +  if (triggeringURI) {
> +    if (LOG_ENABLED()) {
> +      nsAutoCString triggeringURISpec;
> +      rv = triggeringURI->GetAsciiSpec(triggeringURISpec);
> +      if (!NS_FAILED(rv)) LOG(("triggeringURI=%s\n", triggeringURISpec.get()));

nit:
if(!foo) {
 log();
}
Attachment #8541586 - Flags: review?(mcmanus) → review+
Component: DOM → DOM: Security
Reporter

Comment 12

5 years ago
Addressed both review comments. Ran mach test {dom,netwerk} and updated a similar test under CSP.

Sid: may I ask you to do a try bot run with this?  I don't have access.

Is there anything else I need to do to get this checked in?
Attachment #8541586 - Attachment is obsolete: true
Attachment #8545798 - Flags: review+
Attachment #8545798 - Flags: feedback+
I don't think a try run will uncover problems with this patch (it's pretty straightforward), but since I can't stick around and help fix anything in case it does cause a regression, gonna push to try anyway:

(if the try run looks good, feel free to set the checkin-needed keyword to get someone's attention for landing).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e7d2d94dc666

Great work, Alex!  This is really helpful!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed280f6c7b39
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1275247
You need to log in before you can comment on or make changes to this bug.