Closed Bug 1264792 Opened 3 years ago Closed 3 years ago

Implement the recent changes to fetch with regards to referrer policy

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Ehsan, Assigned: tnguyen)

References

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 1 obsolete file)

See https://github.com/whatwg/fetch/issues/266.

Andrew, can you please find an owner for this?  We need to do this ASAP before we can ship the referrer API that's currently on Aurora.
Flags: needinfo?(overholt)
(taking per discussion with :overholt)
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(overholt)
Whiteboard: btpp-active
Thanks to both Andrews!  :-)
As a brief status update, I got hung up on the interaction of this bug with the worker referrer policy bug 1251378, especially since I didn't initially realize worker referrer policies basically don't work yet.  This bug may end up wanting to block on that one depending on where it's planned to stash the referrer policy for workers.  (Which could maybe interact with bug 1220346 if it's still relevant at all?)  I still need to investigate/understand a few more things before declaring that, hopefully late tomorrow.
Hmm, why do you think this is related to worker referrer policies?  I'm not sure if I understand...
Mainly because I thought the behavior was largely correctly implemented for documents clients already since in FetchDriver::HttpFetch:
- in the "about:client" referrer case, nsContentUtils::SetFetchReferrerURIWithPolicy already knows to ask the document for its referrer policy when the policy is net::RP_Default (which ReferrerPolicy::_empty maps to) but breaks for worker clients.
- in the non-"about:client", non-empty referrer case, the logic also knows to fallback to the document's policy if the policy was empty.  And again breaks for workers.

But in attempting to explain that, I now see that I missed that FetchDriver::HttpFetch's mapping from the WebIDL ReferrerPolicy enum to the net_referrerPolicy/nsIHttpChannel enums is actually destructive since net::RP_Default == 0 == net::RP_No_Referrer_When_Downgrade.  And although this helps make revised step 7 ('''If request's referrer policy is the empty string, then set request's referrer policy to "no-referrer-when-downgrade".''') work from the spec's perspective, nsContentUtils::SetFetchReferrerURIWithPolicy only has the net:ReferrerPolicy rep and not the binding ReferrerPolicy rep and so may erroneously clobber an explicitly set referrer policy of "no-referrer-when-downgrade" with the document's referrer policy when it does the following check:
  if (referrerPolicy == net::RP_Default) {
    referrerPolicy = aDoc->GetReferrerPolicy();
  }

So if we're pretending like worker clients don't exist, the fix for this bug becomes addressing that type ambiguity.  But back to my original confused concern, it seems that when cleaning up that logic, you'd want to deal with the worker case as well because the worker is never going to be/look like an nsIDocument.  And that depends on how the worker's referrer policy is stored/exposed.  (Admittedly, I'm still a little confused as to whether it's already available somehow; I did mozStorage/telemetry shutdown hang stuff today because I was hoping bug 1251378 comment 25 or related discussion would help answer these questions.  I have to review my notes and do a little more worker main script/channel life-cycle grokking to make sure it's not being stashed in there.)
(In reply to Andrew Sutherland [:asuth] from comment #5)

>   if (referrerPolicy == net::RP_Default) {
>     referrerPolicy = aDoc->GetReferrerPolicy();
>   }
> 
Agree, it seems that's not quite true at this current version. As spec [1] The Referrer Policy in Request can be used to override a referrer policy. We have
mRequest->GetEnvironmentReferrerPolicy(); return worker's or document's referrer policy in window/worker context. Probably, SetFetchReferrerURIWithPolicy could be changed according to that. And (referrerPolicy == net::RP_Default) also seems not be compliant to current specs (imho, specs now offers "Empty String" state to imply "Unset" policy)

Besides, landing Bug 1264164 will also add the support of Referrer-Policy header and propagate a policy to workers.
[1] https://fetch.spec.whatwg.org/#requests

Thanks for your looking.
It sounds like it might make sense for you to take over this bug.  Setting NI as a mutex; feel free to take and clear NI or not take and still clear NI :)
Flags: needinfo?(tnguyen)
Assignee: bugmail → tnguyen
Flags: needinfo?(tnguyen)
Priority: -- → P2
Duplicate of this bug: 1185019
Apologies for the late response. 
Hi Ehsan,
Could you please review the patch? The change is updating request's referrer, referrer policy when redirect and creating a test case for that.
Thanks
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review74204

::: netwerk/protocol/http/HttpBaseChannel.cpp:1284
(Diff revision 1)
>    mReferrer = nullptr;
>    nsresult rv = mRequestHead.ClearHeader(nsHttp::Referer);
>    if(NS_FAILED(rv)) {
>      return rv;
>    }
> -  mReferrerPolicy = REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE;
> +  mReferrerPolicy = referrerPolicy;

Why do you have to do this?  It seems like this is changing the behavior of SetReferrerWithPolicy in the cases where we currently end up taking an early branch...

::: netwerk/protocol/http/HttpChannelChild.cpp:1570
(Diff revision 1)
>  HttpChannelChild::OnRedirectVerifyCallback(nsresult result)
>  {
>    LOG(("HttpChannelChild::OnRedirectVerifyCallback [this=%p]\n", this));
>    OptionalURIParams redirectURI;
> +
> +  uint32_t referrerPolicy;

This will only get assigned to if you end up taking the branch at line 1583.  Is there anything that guarantees that you're not getting an uninitialized value when you use this on line 1658?

::: netwerk/protocol/http/HttpChannelChild.cpp:2053
(Diff revision 1)
> +  ENSURE_CALLED_BEFORE_CONNECT();
> +
> +  // remove old referrer if any
> +  for (uint32_t i = 0; i < mClientSetRequestHeaders.Length(); i++ ) {
> +    if (NS_LITERAL_CSTRING("Referer").Equals(mClientSetRequestHeaders[i].mHeader)) {
> +      mClientSetRequestHeaders.RemoveElementAt(i);

This loop's condition is wrong, since the first time you call RemoveElementAt, you're shifting all of the remaining indices, and i would end up one more than what you would expect, so you would be skipping the header right after Referer.

A very simple way to fix it would be to iterate from the end to the beginning.
(In reply to :Ehsan Akhgari from comment #12)
> Comment on attachment 8786616 [details]
> Bug 1264792 - Update request'referrer policy when redirect.
> 
> https://reviewboard.mozilla.org/r/75542/#review74204

Sorry, mozreview submitted this earlier than I expected!

I meant to add, I'm not sure if I understand your approach, can you please explain your solution and how the different parts of the patch fit together?  Here are a few open questions for me at this point:

* Why is the code which is being refactored out of FetchDriver::HttpFetch() into FetchUtil::SetRequestReferrer() being changed?  I'd have expected for it to move into the helper but not change its semantics.

* I'm not sure I understand why you needed to add code paths for e10s in Necko, and not something similar for nsHttpChannel?  Is it purely because the redirect callback is delivered to the child process?

* Why the changes to XHR and worker script loader?

Thanks!
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review74218

::: dom/fetch/FetchDriver.cpp:729
(Diff revision 1)
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
>      }
>    }
>  
> +  if (NS_SUCCEEDED(rv)) {

Why this check?
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review74226

Instead of a mochitest, please extend the web-platform tests that we originally added for this feature in fetch-event.https.html over in bug 1251872 so that other browser vendors would also pick up the same tests.
Attachment #8786617 - Flags: review?(ehsan)
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review74228

Also, I'd appreciate if you can run this patch by a Necko peer first for the Necko changes before I look at it again.  Thanks!
Attachment #8786616 - Flags: review?(ehsan)
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review74204

> Why do you have to do this?  It seems like this is changing the behavior of SetReferrerWithPolicy in the cases where we currently end up taking an early branch...

During redirect, if there's no referrer, mReferrerPolicy is unconditionally set to REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE. 
For example SetReferrerWithPolicy(nullptr, net::RP_No_Referrer);

Correctly, mReferrerPolicy should be set to argument referrerPolicy.

> This will only get assigned to if you end up taking the branch at line 1583.  Is there anything that guarantees that you're not getting an uninitialized value when you use this on line 1658?

Thanks you, will assign REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE as default

> This loop's condition is wrong, since the first time you call RemoveElementAt, you're shifting all of the remaining indices, and i would end up one more than what you would expect, so you would be skipping the header right after Referer.
> 
> A very simple way to fix it would be to iterate from the end to the beginning.

Oops, thanks for suggesting
(In reply to :Ehsan Akhgari from comment #13)
> (In reply to :Ehsan Akhgari from comment #12)
> > Comment on attachment 8786616 [details]
> > Bug 1264792 - Update request'referrer policy when redirect.
> > 
> > https://reviewboard.mozilla.org/r/75542/#review74204
> 
> Sorry, mozreview submitted this earlier than I expected!
> 
> I meant to add, I'm not sure if I understand your approach, can you please
> explain your solution and how the different parts of the patch fit together?
> Here are a few open questions for me at this point:
> 
> * Why is the code which is being refactored out of FetchDriver::HttpFetch()
> into FetchUtil::SetRequestReferrer() being changed?  I'd have expected for
> it to move into the helper but not change its semantics.

The change is adding to set request's referrer in step 8 [1].
  
> * I'm not sure I understand why you needed to add code paths for e10s in
> Necko, and not something similar for nsHttpChannel?  Is it purely because
> the redirect callback is delivered to the child process?
> 

Seems there's redirect referrer issue in e10s. That is: if referrer header is omitted during redirect (no-referrer in Referrer-Policy response header). But parent channel still keeps the header in [2]. 
I did ipc transferring referrerPolicy and referrerURI to set correct referrer policy and referrer header in parent channel
 
> * Why the changes to XHR and worker script loader?
> 
> Thanks!
The condition check in [3] seems to be compliant to the old spec when empty_string (Unset value policy) is not introduced.
Changed it to (aReferrerPolicy != net::RP_Unset) to make it compliant to new spec, then I have to update the caller of changed function, those are XHR and worker script loader.

[1] https://fetch.spec.whatwg.org/#main-fetch
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#652
[3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#8373
(In reply to :Ehsan Akhgari from comment #15)
> Comment on attachment 8786617 [details]
> Bug 1264792 - Add Fetch redirect referrer policy test case.
> 
> https://reviewboard.mozilla.org/r/75544/#review74226
> 
> Instead of a mochitest, please extend the web-platform tests that we
> originally added for this feature in fetch-event.https.html over in bug
> 1251872 so that other browser vendors would also pick up the same tests.

(In reply to :Ehsan Akhgari from comment #16)
> Comment on attachment 8786616 [details]
> Bug 1264792 - Update request'referrer policy when redirect.
> 
> https://reviewboard.mozilla.org/r/75542/#review74228
> 
> Also, I'd appreciate if you can run this patch by a Necko peer first for the
> Necko changes before I look at it again.  Thanks!

Will do that, thanks again for your review
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

Clear review until I add more web-platform tests.
Sorry if the reviewboard noticed you too many times :)
Attachment #8786617 - Flags: review?(ehsan)
Hi :dragana.
Sorry that I flagged you here, could you please take a look at this? Or do you know who could be good one to review this?
The main reason I need some changes in necko is mentioned in comment 17, comment 18, basically is to fix the incorrect referrer policy and referrer header are sent in parent channel (particularly in no-referrer case)

Thanks for your help
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review76912

This looks ok. I have reviewed only necko part.
Attachment #8786616 - Flags: review?(dd.mozilla) → review+
Blocks: 1304623
Hi Ben,
Since Ehsan is away until Nov 10, could you please take a look at these changes? Or do you know who would be good to review this?
The changes are :
- Update recent new referrer policy support in the Request.webidl and fetch, serviceworker, and web-platform test of that
- Update request's referrer, request's referrer policy when redirecting

There're some required changes in necko mentioned in commment 17 and comment 18, comment 25 which are reviewed my necko peer.

Thanks
There's still a fetch redirect test case should be added (Ehsan suggested me to expand using fetch in service worker in comment 15, instead of using mochitest).
The purpose of the test is that we can intercept and receive onfetch event on redirecting (after getting 302 and we do fetching the redirect location url) then check the referrer header, but I'm still confused and not sure a good way to intercept the second fetch event 
Afaik, if the document of fetch() is controlled by service worker, then we could get the first onFetch.
Service worker should do an "inner fetch" fetch(evt.request) to follow server redirect, and this fetch will not be intercepted by service worker again. 
Is there any way to intercept the redirect fetch request, not the original fetch request?
Plz correct me if I am wrong or missing something. Do you have any idea about the testing idea?
Ni :bkelly
Thanks for your help
Flags: needinfo?(bkelly)
There should be an easier way to test, clear ni for the question :)
Attachment #8805060 - Attachment is obsolete: true
Attachment #8805060 - Flags: review?(bkelly)
Updated the patch, adding redirect fetch test
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review88618

Sorry for the delay reviewing here.  I had to read up on the spec a bit.  I'm mainly concerned that we're missing step 7 in Main Fetch at the moment, but its possible its being handled somewhere else somehow.

::: dom/fetch/FetchDriver.cpp:275
(Diff revision 4)
> -    // from environment.
>      ReferrerPolicy referrerPolicy = mRequest->ReferrerPolicy_();
>      net::ReferrerPolicy net_referrerPolicy = mRequest->GetEnvironmentReferrerPolicy();
>      switch (referrerPolicy) {
> -    case ReferrerPolicy::_empty:
> +      case ReferrerPolicy::_empty:
> -      break;
> +        break;

nit: Please add a comment here that in the empty string case we use the request's referrer policy by default.

::: dom/fetch/FetchDriver.cpp:304
(Diff revision 4)
> +        break;
> -    default:
> +      default:
> -      MOZ_ASSERT_UNREACHABLE("Invalid ReferrerPolicy enum value?");
> +        MOZ_ASSERT_UNREACHABLE("Invalid ReferrerPolicy enum value?");
> -      break;
> +        break;
>      }
> -    if (referrer.EqualsLiteral(kFETCH_CLIENT_REFERRER_STR)) {
> +

This is missing step 7 of Main Fetch:

```
If request’s referrer policy is the empty string, then set request’s referrer policy to "no-referrer-when-downgrade". 
```

We need to check if the net_referrerPolicy is UNSET.  If so, use "no-referrer-when-downgrade".

::: dom/fetch/FetchUtil.cpp:117
(Diff revision 4)
>    return PushOverLine(aStart, aEnd);
>  }
>  
> +// static
> +nsresult
> +FetchUtil::SetRequestReferrer(nsIPrincipal* aPrincipal,

Please MOZ_ASSERT(NS_IsMainThread()) in this method since you use nsIURI.

::: dom/fetch/FetchUtil.cpp:133
(Diff revision 4)
> +                                                       aDoc,
> +                                                       aChannel,
> +                                                       aPolicy);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  } else if (referrer.IsEmpty()) {
> +    rv = aChannel->SetReferrerWithPolicy(nullptr, net::RP_No_Referrer);

To make it easier to read, can you move this condition to be first and then add a comment that its the "no-referrer" case.

::: dom/fetch/FetchUtil.cpp:150
(Diff revision 4)
> +  }
> +
> +  nsCOMPtr<nsIURI> referrerURI;
> +  aChannel->GetReferrer(getter_AddRefs(referrerURI));
> +
> +  // Step 2. Set the referrer.

Step 2 of what?

::: dom/fetch/FetchUtil.cpp:156
(Diff revision 4)
> +  if (referrerURI) {
> +    nsAutoString referrerURL;
> +    nsAutoCString spec;
> +
> +    rv = referrerURI->GetSpec(spec);
> +    CopyUTF8toUTF16(spec, referrerURL);

Just use:

```
aRequest->SetReferrer(NS_ConvertUTF8toUTF16(spec));
```

Creating another stack buffer with `nsAutoCString` just triggers more copying.  With `NS_ConvertUTF8toUTF16()` I believe the underlying string buffer will be shared without copying.

::: dom/fetch/FetchUtil.cpp:159
(Diff revision 4)
> +
> +    rv = referrerURI->GetSpec(spec);
> +    CopyUTF8toUTF16(spec, referrerURL);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    aRequest->SetReferrer(referrerURL);

The nsIChannel code guarantees we have no ref fragment, username, or password in the URL?

::: dom/xhr/XMLHttpRequestMainThread.cpp:2545
(Diff revision 4)
>  
>      if (!IsSystemXHR()) {
>        nsCOMPtr<nsPIDOMWindowInner> owner = GetOwner();
>        nsCOMPtr<nsIDocument> doc = owner ? owner->GetExtantDoc() : nullptr;
> +      mozilla::net::ReferrerPolicy referrerPolicy = doc ?
> +        doc->GetReferrerPolicy() : mozilla::net::RP_Default;

Again, does this implement step 7 of Main Fetch?  Is it possible for the document to provide an UNSET policy?

::: netwerk/protocol/http/HttpChannelChild.cpp:1601
(Diff revision 4)
>  {
>    LOG(("HttpChannelChild::OnRedirectVerifyCallback [this=%p]\n", this));
>    nsresult rv;
>    OptionalURIParams redirectURI;
> +
> +  uint32_t referrerPolicy = REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE;

Do we expect this default to actually be used, or are you just being safe here?

Any reason not to declare this down where its actually used?

::: netwerk/protocol/http/HttpChannelChild.cpp:2057
(Diff revision 4)
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
> +HttpChannelChild::SetReferrer(nsIURI *referrer)
> +{
> +  return SetReferrerWithPolicy(referrer, REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE);

Why is this necessary?  It overrides HttpBaseChannel::SetReferrer(), but is identical code.  The virtual SetReferrerWithPolicy() call should still execute the HttpChannelChild version.
Attachment #8786616 - Flags: review?(bkelly) → review-
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review88632

Nice tests!  I like all the new redirect test cases.  Very easy to read.

I'm concerned, though, that we are mapping blocked `no-referrer` values to `about:client` incorrectly.  I think we should be returning empty string there.

::: testing/web-platform/tests/fetch/api/redirect/redirect-referrer.js:61
(Diff revision 5)
> +testReferrerAfterRedirection("Cross origin redirection, empty redirect header, unsafe-url init ", redirectUrl, crossLocationUrl, "unsafe-url", "", referrerUrl);
> +testReferrerAfterRedirection("Cross origin redirection, empty redirect header, same-origin init ", redirectUrl, crossLocationUrl, "same-origin", "", null);
> +testReferrerAfterRedirection("Cross origin redirection, empty redirect header, origin init ", redirectUrl, crossLocationUrl, "origin", "", referrerOrigin);
> +testReferrerAfterRedirection("Cross origin redirection, empty redirect header, origin-when-cross-origin init ", redirectUrl, crossLocationUrl, "origin-when-cross-origin", "", referrerOrigin);
> +testReferrerAfterRedirection("Cross origin redirection, empty redirect header, no-referrer init ", redirectUrl, crossLocationUrl, "no-referrer", "", null);
> +testReferrerAfterRedirection("Cross origin redirection, empty redirect header, strict-origin init ", redirectUrl, crossLocationUrl, "strict-origin", "", referrerOrigin);

Do we have a test somewhere for strict-origin that triggers its mixed content condition?  If not, can you write a follow-up bug?

::: testing/web-platform/tests/service-workers/service-worker/fetch-event.https.html:222
(Diff revision 5)
> +        })
> +      .then(function(response) { return response.text(); })
> +      .then(function(response_text) {
> +          assert_equals(
> +            response_text,
> +            'Referrer: about:client\n' +

Why is this `about:client`? Shouldn't it be the empty string for a `no-referrer` value?

::: testing/web-platform/tests/service-workers/service-worker/fetch-event.https.html:256
(Diff revision 5)
> +        })
> +      .then(function(response) { return response.text(); })
> +      .then(function(response_text) {
> +          assert_equals(
> +            response_text,
> +            'Referrer: about:client\n' +

Again, why is this `about:client`.  It feels like if the referrer policy blocks the document's referrer then we should be showing an empty string for `no-referrer`.
Attachment #8786617 - Flags: review?(bkelly) → review-
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review89600

::: testing/web-platform/tests/service-workers/service-worker/fetch-event.https.html:256
(Diff revision 5)
> +        })
> +      .then(function(response) { return response.text(); })
> +      .then(function(response_text) {
> +          assert_equals(
> +            response_text,
> +            'Referrer: about:client\n' +

Thanks for your review.
Hmm, you are right, let me look into nsContentUtils::SetFetchReferrerURIWithPolicy to see what's wrong with this
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review88618

Thanks for help me to review this :)

> This is missing step 7 of Main Fetch:
> 
> ```
> If request’s referrer policy is the empty string, then set request’s referrer policy to "no-referrer-when-downgrade". 
> ```
> 
> We need to check if the net_referrerPolicy is UNSET.  If so, use "no-referrer-when-downgrade".

Added the step 7, although I think that "empty string" unset should be used, it's more reasonable (There's a note in specs "We use "no-referrer-when-downgrade" because it is the historical default.")
Even if we use UNSET, the policy will be changed to default in nsIChannel.

> The nsIChannel code guarantees we have no ref fragment, username, or password in the URL?

Yep, in HttpBaseChannel::SetReferrerWithPolicy

> Do we expect this default to actually be used, or are you just being safe here?
> 
> Any reason not to declare this down where its actually used?

Bascally, in e10s case, the policy is sent from parent to child. This is set for being safe if there's something wrong in ipc, need initial value.
The policy is used 2 times, so I put it here.
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review88632

> Do we have a test somewhere for strict-origin that triggers its mixed content condition?  If not, can you write a follow-up bug?

Add some cases related to mixed content, except mixed content in worker.
Afaik, downgrading in worker is blocked entirely without unblock option

> Again, why is this `about:client`.  It feels like if the referrer policy blocks the document's referrer then we should be showing an empty string for `no-referrer`.

Something wrong in ServiceWorker, Referer is set to about:client from no-referrer. In the case there's no referrer, we should show an empty string.
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review90470

This is better, thanks.  I think we need to make the mixed-content tests work without prefs, though.  Otherwise these tests will not work in other browsers and we lose the compat checking the test would otherwise provide.

::: testing/web-platform/meta/fetch/api/redirect/redirect-referrer.https.html.ini:3
(Diff revision 6)
> +[redirect-referrer.https.html]
> +  type: testharness
> +  prefs: [security.mixed_content.block_active_content:false, security.mixed_content.block_display_content:false]

What are these prefs needs for?  I'm not sure we should push a WPT test that requires non-standard behavior as that will likely not work in other browsers.

Rather than doing this I think you need to use something that allows mixed-content loading, like images.  Send the expected referrer in the query param of the image and have it return 500 if it doesn't match the header.
Attachment #8786617 - Flags: review?(bkelly) → review-
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review88618

> Added the step 7, although I think that "empty string" unset should be used, it's more reasonable (There's a note in specs "We use "no-referrer-when-downgrade" because it is the historical default.")
> Even if we use UNSET, the policy will be changed to default in nsIChannel.

I think you need to re-request review.  I don't see how to mark it r+ in mozreview's current state.
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review90470

> What are these prefs needs for?  I'm not sure we should push a WPT test that requires non-standard behavior as that will likely not work in other browsers.
> 
> Rather than doing this I think you need to use something that allows mixed-content loading, like images.  Send the expected referrer in the query param of the image and have it return 500 if it doesn't match the header.

The prefs are required to disable mixed content blocking in ff (allow https-> http). I see they are being used in lot of wb-platform-tests 
https://dxr.mozilla.org/mozilla-central/search?q=prefs%3A+%5Bsecurity.mixed_content.&redirect=false
I think, other browsers have the same idea to disable this kind of security mechanism, but I don't know exactly how other browsers disable that (for example chrome adds args : --allow-running-insecure-content).
Imho, I think we still need this kind of tests, because it tests global Fetch API. To avoid adding the prefs for only firefox, we could move the mixed content part to local firefox web-platform-test (somewhere in mozilla-central/testing/web-platform/mozilla/tests), or just use mochitest.
Do you agree with that, of do you have any other idea?
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

Re-push request for the first part only
Attachment #8786617 - Flags: review?(bkelly) → review-
James, can you confirm that writing tests that depend on disabling features like mixed content is ok for WPT tests?  That seems wrong to me.
Flags: needinfo?(james)
I… think that it would be a good idea to move the tests to testing/web-platform/mozilla/tests until we have a better idea of what other vendors want to do in this case.
Flags: needinfo?(james)
Rebased and move to internal mozilla tests
Comment on attachment 8786617 [details]
Bug 1264792 - Update web platform tests of fetch and serviceworker.

https://reviewboard.mozilla.org/r/75544/#review92020
Attachment #8786617 - Flags: review?(bkelly) → review+
Comment on attachment 8786616 [details]
Bug 1264792 - Update request'referrer policy when redirect..

https://reviewboard.mozilla.org/r/75542/#review92028
Attachment #8786616 - Flags: review?(bkelly) → review+
Rebased update
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/763507de3e4b
Update request'referrer policy when redirect.r=bkelly,dragana.
https://hg.mozilla.org/integration/autoland/rev/b9a3e66abdd1
Update web platform tests of fetch and serviceworker. r=bkelly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/763507de3e4b
https://hg.mozilla.org/mozilla-central/rev/b9a3e66abdd1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1319060
Hi there,

I'm trying to work out what to change in the MDN docs as a result of this bug, but not really getting very far. Can you explain what the change is in layperson's terms? Thanks!
Flags: needinfo?(ehsan)
Redirecting to Thomas who worked on the bug.  :-)
Flags: needinfo?(ehsan) → needinfo?(tnguyen)
The mdn docs which may refer to the changes is
https://developer.mozilla.org/en-US/docs/Web/API/Request/referrerPolicy
I see we referred the Fetch standard in this page.
This fix is only to correct request'referrer policy when redirect which is only an internal step of how Fetch works based on Fetch living standard, user may not know or have API to touch to them
https://fetch.spec.whatwg.org/#http-redirect-fetch. IMHO we may not need to add the explanation.
Flags: needinfo?(tnguyen)
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #65)
> The mdn docs which may refer to the changes is
> https://developer.mozilla.org/en-US/docs/Web/API/Request/referrerPolicy
> I see we referred the Fetch standard in this page.
> This fix is only to correct request'referrer policy when redirect which is
> only an internal step of how Fetch works based on Fetch living standard,
> user may not know or have API to touch to them
> https://fetch.spec.whatwg.org/#http-redirect-fetch. IMHO we may not need to
> add the explanation.

OK, thanks Thomas - I'll not worry about this then.
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.