Closed
Bug 1264792
Opened 9 years ago
Closed 8 years ago
Implement the recent changes to fetch with regards to referrer policy
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ehsan.akhgari, 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)
Comment 1•9 years ago
|
||
(taking per discussion with :overholt)
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(overholt)
Updated•9 years ago
|
Whiteboard: btpp-active
Reporter | ||
Comment 2•9 years ago
|
||
Thanks to both Andrews! :-)
Comment 3•9 years ago
|
||
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.
Reporter | ||
Comment 4•9 years ago
|
||
Hmm, why do you think this is related to worker referrer policies? I'm not sure if I understand...
Comment 5•9 years ago
|
||
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.)
Assignee | ||
Comment 6•8 years ago
|
||
(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.
Comment 7•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: bugmail → tnguyen
Flags: needinfo?(tnguyen)
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•8 years ago
|
||
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
Reporter | ||
Comment 12•8 years ago
|
||
mozreview-review |
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.
Reporter | ||
Comment 13•8 years ago
|
||
(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!
Reporter | ||
Comment 14•8 years ago
|
||
mozreview-review |
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?
Reporter | ||
Comment 15•8 years ago
|
||
mozreview-review |
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)
Reporter | ||
Comment 16•8 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 18•8 years ago
|
||
(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
Assignee | ||
Comment 19•8 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•8 years ago
|
||
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
Assignee | ||
Comment 31•8 years ago
|
||
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?
Comment hidden (typo) |
Assignee | ||
Comment 34•8 years ago
|
||
There should be an easier way to test, clear ni for the question :)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8805060 -
Attachment is obsolete: true
Attachment #8805060 -
Flags: review?(bkelly)
Assignee | ||
Comment 36•8 years ago
|
||
Updated the patch, adding redirect fetch test
Comment 37•8 years ago
|
||
mozreview-review |
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 38•8 years ago
|
||
mozreview-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-
Assignee | ||
Comment 39•8 years ago
|
||
mozreview-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
Assignee | ||
Comment 40•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 41•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 44•8 years ago
|
||
mozreview-review |
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 45•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 46•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 49•8 years ago
|
||
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-
Comment 50•8 years ago
|
||
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)
Comment 51•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 54•8 years ago
|
||
Rebased and move to internal mozilla tests
Comment 55•8 years ago
|
||
mozreview-review |
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 56•8 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 59•8 years ago
|
||
Rebased update
Assignee | ||
Comment 60•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 61•8 years ago
|
||
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
Comment 62•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/763507de3e4b
https://hg.mozilla.org/mozilla-central/rev/b9a3e66abdd1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 63•8 years ago
|
||
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)
Reporter | ||
Comment 64•8 years ago
|
||
Redirecting to Thomas who worked on the bug. :-)
Flags: needinfo?(ehsan) → needinfo?(tnguyen)
Assignee | ||
Comment 65•8 years ago
|
||
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)
Comment 66•8 years ago
|
||
(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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•