Closed Bug 1264164 Opened 4 years ago Closed 4 years ago

Support Referrer Policy HTTP header

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: hchang, Assigned: tnguyen)

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active)

Attachments

(3 files, 10 obsolete files)

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
We have implemented <meta> and per-element referrer policy. The next one to do is referrer policy carried by HTTP header.
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: hchang → tnguyen
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla50
MozReview-Commit-ID: 2uMM7jXNVyF
MozReview-Commit-ID: zIi6j3Gzmg
MozReview-Commit-ID: 6KoTp9vuKCY
MozReview-Commit-ID: 6KoTp9vuKCY
Attachment #8771347 - Attachment is obsolete: true
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.
Attachment #8771345 - Flags: review?(josh)
Attachment #8771346 - Flags: review?(josh)
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+
(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.
MozReview-Commit-ID: JMUr8DxAzVh
Attachment #8771345 - Attachment is obsolete: true
MozReview-Commit-ID: FKPCjpPjB41
Attachment #8771346 - Attachment is obsolete: true
Attachment #8771366 - Attachment is obsolete: true
Attachment #8771827 - Attachment is obsolete: true
MozReview-Commit-ID: F2PpxwZsSWH
Attachment #8771829 - Attachment is obsolete: true
Attachment #8771826 - Flags: review+
Attachment #8771825 - Flags: review+
Attachment #8771830 - Flags: review+
Keywords: checkin-needed
MozReview-Commit-ID: GANRqgAZQ5C
Attachment #8771830 - Attachment is obsolete: true
MozReview-Commit-ID: JMUr8DxAzVh
Attachment #8772318 - Flags: review+
Attachment #8772320 - Flags: review+
Rebased to central/inbound branch today. :)
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!
Attachment #8786588 - Attachment is obsolete: true
Attachment #8786588 - Flags: review?(ehsan)
Attachment #8786589 - Attachment is obsolete: true
Attachment #8786589 - Flags: review?(ehsan)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.