Support Referrer Policy HTTP header

RESOLVED FIXED in Firefox 50

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: hchang, Assigned: tnguyen)

Tracking

({dev-doc-complete})

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(3 attachments, 10 obsolete attachments)

143.67 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
10.02 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
15.98 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
We have implemented <meta> and per-element referrer policy. The next one to do is referrer policy carried by HTTP header.
Reporter

Updated

3 years ago
Assignee: nobody → hchang
Whiteboard: btpp-active
Hi Franziskus,

Do we have any plan on the timeline of implementing this feature?
I ask this because Henry and I are figuring out our job priorities.
Flags: needinfo?(franziskuskiefer)
When bug 1223838 landed this should be one of the last bugs to support all core features from Referrer Policy spec. If we could get both into 49, that would be great. But bug 1223838 is higher priority.
Flags: needinfo?(franziskuskiefer)
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)
> When bug 1223838 landed this should be one of the last bugs to support all
> core features from Referrer Policy spec. If we could get both into 49, that
> would be great. But bug 1223838 is higher priority.

Franziskus, thanks for your feedback.
Thomas is doing his best on bug 1223838. Hope we can get it landed in FF 49.

But for this bug, we have less than three weeks before FF 49 (June 6).
Henry is still busy working on SafeBrowsing bugs and didn't start this one yet.
I don't think we can get this bug done in such limited time frame.

Can we make the target of this bug as FF 50?
(In reply to Ethan Tseng [:ethan] from comment #3)
> Can we make the target of this bug as FF 50?

Sure! There's no rush here. No other browser is supporting this yet. 50 sounds like a good target to me.
Assignee

Updated

3 years ago
Assignee: hchang → tnguyen
Assignee

Updated

3 years ago
Status: NEW → ASSIGNED
Assignee

Updated

3 years ago
Priority: -- → P2
Target Milestone: --- → mozilla50
Assignee

Comment 5

3 years ago
MozReview-Commit-ID: 2uMM7jXNVyF
Assignee

Comment 6

3 years ago
MozReview-Commit-ID: zIi6j3Gzmg
Assignee

Comment 7

3 years ago
MozReview-Commit-ID: 6KoTp9vuKCY
Assignee

Comment 8

3 years ago
MozReview-Commit-ID: 6KoTp9vuKCY
Assignee

Updated

3 years ago
Attachment #8771347 - Attachment is obsolete: true
Assignee

Comment 9

3 years ago
Hi Josh,
Could you please take a look at this bug? 
The patches that I've added mostly implement the support of Referrer-Policy header to window(document)/worker context.
Also, there's a change in fetch's request referrer policy with this header.This bug still needs a little work on Bug 1264792 to complete fetch changes regarding to referrer policy, but I think we should resolve this first.
Assignee

Updated

3 years ago
Attachment #8771345 - Flags: review?(josh)
Assignee

Updated

3 years ago
Attachment #8771346 - Flags: review?(josh)
Assignee

Updated

3 years ago
Attachment #8771366 - Flags: review?(josh)
Attachment #8771346 - Flags: review?(josh) → review+
Comment on attachment 8771366 [details] [diff] [review]
Support Referrer Policy HTTP header

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

::: dom/base/nsDocument.cpp
@@ +3782,5 @@
>    // Referrer policy spec says to ignore any empty referrer policies.
> +  if ((aHeaderField == nsGkAtoms::referrer ||
> +       aHeaderField == nsGkAtoms::headerReferrerPolicy) &&
> +      !aData.IsEmpty()) {
> +     ReferrerPolicy policy = nsContentUtils::GetReferrerPolicyFromHeader(aData);

Using the new method for `<meta name="referrer" content="...">` tags means that comma-separated values will be allowed, which is contrary to the specification: https://html.spec.whatwg.org/multipage/semantics.html#meta-referrer

@@ +8736,5 @@
>        "x-frame-options",
>        // add more http headers if you need
>        // XXXbz don't add content-location support without reading bug
>        // 238654 and its dependencies/dups first.
> +      "referrer-policy",

Let's add this above the comment.
Attachment #8771366 - Flags: review?(josh) → review+
Comment on attachment 8771345 [details] [diff] [review]
Add Referrer Policy HTTP header testcases

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

::: dom/workers/test/referrer_test_server.sjs
@@ +33,5 @@
> +  `;
> +}
> +
> +function handleRequest(request, response)
> +{

nit: { on the previous line.

@@ +38,5 @@
> +  var params = new URLSearchParams(request.queryString);
> +  var policy = params.get("Referrer-Policy");
> +  var type = params.get("TYPE");
> +  var action = params.get("ACTION");
> +  var csp = `default-src *`;

Why the `` here?

@@ +39,5 @@
> +  var policy = params.get("Referrer-Policy");
> +  var type = params.get("TYPE");
> +  var action = params.get("ACTION");
> +  var csp = `default-src *`;
> +  response.setHeader("Content-Security-Policy", csp, false);

Why not inline the header value?

::: dom/workers/test/referrer_worker.html
@@ +115,5 @@
> +  for (var type in testCases) {
> +    for (var policy in testCases[type]['Referrer-Policy']) {
> +      yield resetState();
> +      var searchParams = new URLSearchParams();
> +      searchParams.append("TYPE", escape(type));

Where's escape defined?

::: dom/workers/test/test_referrer_header_worker.html
@@ +7,5 @@
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test the referrer of workers</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /></head>

nit: move the </head> to the next line.

::: testing/web-platform/meta/MANIFEST.json
@@ +15831,5 @@
>          "path": "fetch/api/policies/referrer-origin.html",
>          "url": "/fetch/api/policies/referrer-origin.html"
>        },
>        {
> +        "path": "fetch/api/policies/referrer-origin-when-cross-origin.html",

Did these new tests get created using `./mach web-platform-tests-create`? I wouldn't expect to see these changes in this part of the file if so.

::: testing/web-platform/tests/fetch/api/policies/referrer-origin-when-cross-origin.js
@@ +3,5 @@
> +  importScripts("../resources/utils.js");
> +}
> +
> +var referrerOrigin = "http://{{host}}:{{ports[http][0]}}/";
> +var fetchedUrl = "http://www1.{{host}}:{{ports[http][0]}}" + dirname(location.pathname) + RESOURCES_DIR + "inspect-headers.py?cors&headers=referer";

Let's use "http://{{host}}:{{ports[http][1]}}" here instead.
Attachment #8771345 - Flags: review?(josh) → review+
Assignee

Comment 12

3 years ago
(In reply to Josh Matthews [:jdm] from comment #10)
> Comment on attachment 8771366 [details] [diff] [review]
> Support Referrer Policy HTTP header
> 
> Review of attachment 8771366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDocument.cpp
> @@ +3782,5 @@
> >    // Referrer policy spec says to ignore any empty referrer policies.
> > +  if ((aHeaderField == nsGkAtoms::referrer ||
> > +       aHeaderField == nsGkAtoms::headerReferrerPolicy) &&
> > +      !aData.IsEmpty()) {
> > +     ReferrerPolicy policy = nsContentUtils::GetReferrerPolicyFromHeader(aData);
> 
> Using the new method for `<meta name="referrer" content="...">` tags means
> that comma-separated values will be allowed, which is contrary to the
> specification:
> https://html.spec.whatwg.org/multipage/semantics.html#meta-referrer
> 
Oops, thanks for your prompt and great review.
Assignee

Comment 13

3 years ago
MozReview-Commit-ID: JMUr8DxAzVh
Assignee

Updated

3 years ago
Attachment #8771345 - Attachment is obsolete: true
Assignee

Comment 14

3 years ago
MozReview-Commit-ID: FKPCjpPjB41
Assignee

Updated

3 years ago
Attachment #8771346 - Attachment is obsolete: true
Comment hidden (obsolete)
Assignee

Updated

3 years ago
Attachment #8771366 - Attachment is obsolete: true
Comment hidden (obsolete)
Assignee

Updated

3 years ago
Attachment #8771827 - Attachment is obsolete: true
Assignee

Comment 17

3 years ago
MozReview-Commit-ID: F2PpxwZsSWH
Assignee

Updated

3 years ago
Attachment #8771829 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8771826 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8771825 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8771830 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Comment 19

3 years ago
MozReview-Commit-ID: GANRqgAZQ5C
Assignee

Updated

3 years ago
Attachment #8771830 - Attachment is obsolete: true
Assignee

Comment 20

3 years ago
MozReview-Commit-ID: JMUr8DxAzVh
Comment hidden (obsolete, typo)
Assignee

Updated

3 years ago
Attachment #8772318 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8772320 - Flags: review+
Assignee

Comment 22

3 years ago
Rebased to central/inbound branch today. :)

Comment 23

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2034653efbfd
Enable Referrer Policy HTTP header web-platform-tests. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/81ad4d06462d
Support Referrer Policy HTTP header. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/77fbb7ff85e5
Add Referrer Policy HTTP header testcases. r=jdm
Keywords: checkin-needed
Thomas, thanks for getting this done!
It's a big step for us!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8786588 - Attachment is obsolete: true
Attachment #8786588 - Flags: review?(ehsan)
Assignee

Updated

3 years ago
Attachment #8786589 - Attachment is obsolete: true
Attachment #8786589 - Flags: review?(ehsan)
Comment hidden (obsolete)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.