Firefox seems to send the referrer of the parent page when using fetch [1] instead of the current url. This breaks most W3C referrer policy tests using fetch. [1] https://fetch.spec.whatwg.org/

Do you have a small test case?

Created a very simple test case: click the link: https://www.franziskuskiefer.de/data/fetchTest.html expected: the referer for fetch is something with https://www.franziskuskiefer.de actual: the referer for fetch is something with https://bugzilla.mozilla.org copy&paste the link: https://www.franziskuskiefer.de/data/fetchTest.html expected: the referer for fetch is something with https://www.franziskuskiefer.de actual: fetch has no referer

Seems we don't quite implement all the steps right in the Determine Requests Referrer algorithm: https://w3c.github.io/webappsec/specs/referrer-policy/#determine-requests-referrer So this seems wrong: https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#422 And I don't see where we use an iframe's parent document. I think the null principal handling might deal with the sandboxed iframe case.

Bug 1169044 - Set correct referrer on load from window global. r=bkelly

https://reviewboard.mozilla.org/r/9599/#review8371 ::: dom/fetch/Fetch.cpp:448 (Diff revision 1) > + aRequest->SetReferrer(NS_ConvertUTF8toUTF16(referrer)); Uses logic similar to XHR to deal with pushState. The "determine referrer" algorithm talks about srcdoc attributes on iframes. Gecko deals with this internally so that the nsIDocument created from the srcdoc content has the correct referrer. I checked in jsfiddle.

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey Did all that complexity get copy/pasted from somewhere? If so, can we factor it out into a helper function?

(In reply to Not doing reviews right now from comment #6) > Comment on attachment 8612572 [details] > MozReview Request: Bug 1169044 - Set correct referrer on load from window > global. r=bkelly > > Did all that complexity get copy/pasted from somewhere? If so, can we > factor it out into a helper function? XHR. I could probably refactor a few lines from 'get the principal' and 'get the doc' onwards into a function that accepts a principal and a doc and returns the referrer. The way XHR obtains its doc from the global is different though (I'm not sure why it does a combination of GetContextForEventHandlers + GetDocumentFromScriptContext when it has access to the window global.) If I can change that I can do a more significant refactoring.

Hmm. smaug might know why xhr is doing what it's doing...

I would say use of GetDocumentFromScriptContext is a leftover. And since XHR used to have mScriptContext, something was needed to get nsIScriptContext and GetContextForEventHandlers was the easiest way. GetContextForEventHandlers() does do innerwindow correctness check, so one can't open or send XHR to http(s) url if it is not from the current inner window. We should probably tighten that up and do similar checks also for other networking stuff for consistency. But if you want to factor out referrer handling, just add some CheckInnerWindowCorrectness(); to XHR (to keep the existing behavior) and otherwise use GetOwner()->GetExtantDoc() or something?

Though, out setup for security checks in general has changed quite a bit, that perhaps we should remove most of the inner window checks, at least the ones from Bug 403168. Loadgroup usage can still be an issue though. (using loadgroup which is now used for some window)

but if we do any generic changes to loadgroup handling or innerwindow checks, those clearly don't belong to this bug.

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey https://reviewboard.mozilla.org/r/9599/#review8397 ::: dom/fetch/Fetch.cpp:448 (Diff revision 1) > + aRequest->SetReferrer(NS_ConvertUTF8toUTF16(referrer)); Can we unify this logic in a single shared method? Maybe something like nsContentUtils::GetReferrerURI(aGlobal, aDoc)? Also, can you add a comment that iframe srcdoc referrer handling is done transparently by the document?

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey https://reviewboard.mozilla.org/r/9599/#review8401 Overall looks reasonable. It would be nice to factory out the common code. Also, please add a test that catches the main failure modes from this bug. Thanks!

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey Bug 1169044 - Set correct referrer on load from window global. r?bkelly,smaug

https://reviewboard.mozilla.org/r/9597/#review8471 ::: dom/base/nsContentUtils.cpp:7815 (Diff revision 2) > + referrerURI = principalURI; Please add { } around the if block. ::: dom/base/nsContentUtils.cpp:7786 (Diff revision 2) > + sop->GetPrincipal()->GetURI(getter_AddRefs(principalURI)); Is GetPrincipal() guaranteed to return non-nullptr? I don't see anything in the API guaranteeing this and its a bare pointer deref. Maybe store it in an nsCOMPtr<> before you access it here and in the IsSystemPrincipal?

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey https://reviewboard.mozilla.org/r/9599/#review8477 Ship It!

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey Why Fetch doesn't need to deal with SetReferrerWithPolicy, but XHR and other code does need to? I don't like this inconsistency. I'd prefer also that part being moved to the helper method. Or perhaps there is some reason why Fetch handles this all differently to any other code, but I'm not familiar with Fetch at all. Anyhow, I'd prefer the helper to deal with SetReferrerWithPolicy at least if one explicitly passes an httpChannel to the method.

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?smaug

Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku

Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r?bkelly The ServiceWorkerRegistrationInfo's principal is the principal of the document that called register(). If we create WorkerPrivate instances based off of this, they have a valid principal in terms of security and same-origin-ness, but the URI path is wrong. When fetching the script from the network, the channel's principal is used to update the worker principal. We need to do the same when the script is loaded from Cache. This patch adds support to store the channel principal in the cache.

Bug 1169044 - Patch 4 - Set referrer on intercepted request. r?jdm

Ehsan, if you are comfortable with cache code, could you take patch 3?

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey Kyle said he could review some more.

Fernando, Patch 3 in this bug adds an assert that the principal of the channel loading an SW is same-origin to the principal of the registration. Obviously, jar is violating that somehow https://treeherder.mozilla.org/logviewer.html#?job_id=8229547&repo=try Could you tell me if this requires a relaxed assertion or something? Is the jar trying to load a non-jar resource?

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #24) > Ehsan, if you are comfortable with cache code, could you take patch 3? Yes. Can you please reset the flag to me? I will review tomorrow.

Comment on attachment 8615688 [details] [diff] [review] Patch 2 bugzilla Review of attachment 8615688 [details] [diff] [review]: ----------------------------------------------------------------- I prefer to have URLSearchParam inheriting from URLParams instead having a mParams obj. Any reason why you decied to go for this approach? ::: dom/base/URLSearchParams.cpp @@ +116,3 @@ > > +void > +URLSearchParams::ParseInput(const nsACString& aInput, move it next to the URLSearchParams ctor. ::: dom/base/URLSearchParams.h @@ +26,5 @@ > > virtual void URLSearchParamsUpdated(URLSearchParams* aFromThis) = 0; > }; > > +class URLParams final What about using this class as base class for URLSearchParams? @@ +71,5 @@ > + void Serialize(nsAString& aValue) const; > + > + void Get(const nsAString& aName, nsString& aRetval); > + > + void GetAll(const nsAString& aName, nsTArray<nsString >& aRetval); I've always wanted to remove this extra space between nsString and '>'. Can you do it in this patch? :)

Comment on attachment 8615685 [details] MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan https://reviewboard.mozilla.org/r/10345/#review9089 Ship It! ::: dom/cache/DBSchema.cpp:1747 (Diff revision 1) > + } Are we supposed to parse the contents of the string manually like this? You may want to check that with bholley. ::: dom/workers/ServiceWorkerScriptCache.cpp:102 (Diff revision 1) > + loadGroup); // FIXME(nsm): TYPE_SERVICEWORKER Nit: please move the comment back up. ::: dom/workers/ServiceWorkerScriptCache.cpp:470 (Diff revision 1) > + return rv; You Cancel()ed the channel above in the case of an error. Why not do the same here?

Comment on attachment 8615686 [details] MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm https://reviewboard.mozilla.org/r/10347/#review9093 Ship It!

(In reply to Andrea Marchesini (:baku) from comment #30) > Comment on attachment 8615688 [details] [diff] [review] > Patch 2 bugzilla > > Review of attachment 8615688 [details] [diff] [review]: > ----------------------------------------------------------------- > > I prefer to have URLSearchParam inheriting from URLParams instead having a > mParams obj. Any reason why you decied to go for this approach? To me, "URLSearchParams is a webidl object with observer support that *has* a bag of name value pairs" reads better than "Here is this bag of name value pairs, but we are going to specialize it so it also *is* this WebIDL object with observer support". I'd like to keep using composition unless there is a strong reason to use inheritance. > > ::: dom/base/URLSearchParams.cpp > @@ +116,3 @@ > > > > +void > > +URLSearchParams::ParseInput(const nsACString& aInput, > > move it next to the URLSearchParams ctor. > > ::: dom/base/URLSearchParams.h > @@ +26,5 @@ > > > > virtual void URLSearchParamsUpdated(URLSearchParams* aFromThis) = 0; > > }; > > > > +class URLParams final > > What about using this class as base class for URLSearchParams? > > @@ +71,5 @@ > > + void Serialize(nsAString& aValue) const; > > + > > + void Get(const nsAString& aName, nsString& aRetval); > > + > > + void GetAll(const nsAString& aName, nsTArray<nsString >& aRetval); > > I've always wanted to remove this extra space between nsString and '>'. Can > you do it in this patch? :)

https://reviewboard.mozilla.org/r/10345/#review9121 > You Cancel()ed the channel above in the case of an error. Why not do the same here? `SetPrincipalInfo(nsIChannel*)` is called from OnStartRequest() where returning an error automatically leads to cancel.

Comment on attachment 8615685 [details] MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan Bobby, is this sort of principal parsing in DBSchema.cpp ok? This is on a background thread with no CC etc, so I can't use any main thread only or webidl helpers.

FindChar('!') looks wrong to me, since a URI can contain '!' in it, no?

Ah, but the serialization format may not include the ! if it's not present. That seems broken to me, since it means it can't be unambiguously parsed. Consider a codebase principal for file:///tmp/foo!appId=5&inBrowser=1

Comment on attachment 8615685 [details] MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan We shouldn't have any manual parsing logic scattered throughout the tree - anything we do should live with all the other code in CAPS.

(In reply to Not doing reviews right now from comment #37) > Ah, but the serialization format may not include the ! if it's not present. Yes, so that the origins match up with the old behavior when no special OriginAttributes are present. > That seems broken to me, since it means it can't be unambiguously parsed. > Consider a codebase principal for file:///tmp/foo!appId=5&inBrowser=1 Hm, that's a good point. Maybe we should aways include the '!'? What do you think Jonas?

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey

Comment on attachment 8615684 [details] MozReview Request: Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku

Comment on attachment 8615685 [details] MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan The ServiceWorkerRegistrationInfo's principal is the principal of the document that called register(). If we create WorkerPrivate instances based off of this, they have a valid principal in terms of security and same-origin-ness, but the URI path is wrong. When fetching the script from the network, the channel's principal is used to update the worker principal. We need to do the same when the script is loaded from Cache. This patch adds support to store the channel principal in the cache.

Comment on attachment 8615686 [details] MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

(In reply to Bobby Holley (:bholley) from comment #39) > Hm, that's a good point. Maybe we should aways include the '!'? What do you > think Jonas? Happy to leave this to you. Generally I think it's good if we don't ever need to parse these strings. But clearly they need to always be unique. Another thing that's tricky is that on one hand we want don't want a filename to be able to collide with the origin suffix. On the other hand we want to be able to use the origin string as a filename for things like indexedDB...

Comment on attachment 8615685 [details] MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan The ServiceWorkerRegistrationInfo's principal is the principal of the document that called register(). If we create WorkerPrivate instances based off of this, they have a valid principal in terms of security and same-origin-ness, but the URI path is wrong. When fetching the script from the network, the channel's principal is used to update the worker principal. We need to do the same when the script is loaded from Cache. This patch adds support to store the channel principal in the cache.

Comment on attachment 8615686 [details] MozReview Request: Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm Bug 1169044 - Patch 4 - Set referrer on intercepted request. r=jdm

Bobby, only the caps changes.

Unrelatedly, I think it would be really great if we didn't have to replicate this "grab the current principal, unless pushState has been called, in that case grab the document URI" code everywhere. It would be great if we: 1. Added a field for "document URI" to nsIPrincipal. Update this field any time pushState is called. (possibly we could even mutate the existing URI on the principal, but that feels somewhat scary. Audit would be needed) 2. Add a loadinfo security-flag for explicitly preventing any referrer from being sent. 3. Make the http-channel code get the referrer from loadInfo.triggeringPincipal.documentURI unless a different referrer is explicitly set, or unless the above flag has been set. 4. Remove the explicitly set referrer in all/most cases.

> Added a field for "document URI" to nsIPrincipal Multiple documents that have distinct document URIs can share a principal in general, no? Now if we're doing this for referrers that would be because some cases alias another document's referrer, but... It's not quite clear to me what should happen if a page has an about:blank or data: or javascript: subframe or a document.opened subframe and then does pushState. Should the referrer be the one from before or after the pushState? Note that afaict per spec it should be about:blank or data: or the URI the document it had when he javascript: ran, respectively. But note https://www.w3.org/Bugs/Public/show_bug.cgi?id=24613

(In reply to Not doing reviews right now from comment #50) > > Added a field for "document URI" to nsIPrincipal > > Multiple documents that have distinct document URIs can share a principal in > general, no? file:// aside for a second, can this happen in other cases than when a principal is inherited? I.e. other than for data:, javascript:, about:srcdoc and about:blank? > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24613 Not much action there the last few months :( Early on in the bug it did seem like people were fine with "walking up the iframe chain" for cases where we currently inherit. I just hate the idea of having to pass in a "triggering document" in addition to "triggering principal" any time a channel is created. Though I guess it's always a possibility.

Comment on attachment 8615685 [details] MozReview Request: Bug 1169044 - Patch 3 - Store and set principal with script URI on ServiceWorkers. r=ehsan https://reviewboard.mozilla.org/r/10345/#review9139 r=me on the caps changes with that. ::: caps/BasePrincipal.cpp:122 (Diff revision 3) > + int32_t pos = aOrigin.FindChar('!'); > + Can you make this an rfind, which will make it more robust per bug 1172080?

> file:// aside for a second, can this happen in other cases than when a principal is > inherited? Not really. Possibly add to your list document.open(); I don't recall what that does with principals. > I just hate the idea of having to pass in a "triggering document" in addition to > "triggering principal" So one option here is to go ahead and store the actual origin information in some other data structure pointed to by an principal, so that we can share _that_ across documents but not share the principal pointer itself. Note that we really need to do something like that anyway to correctly implement hixie's spec's asymmetric handling of document.domain. Assuming we want to implement that, of course.

I think it depends on what design we want. If we want to implement the "walk up the chain to find a referrer" then I think the current design of sharing nsIPrincipal makes sense. Otherwise yeah, we could share something else instead. So maybe getting that settled in the spec w3c bugzilla thread would be a good idea. I don't have an opinion about document.domain.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #27) > Fernando, > > Patch 3 in this bug adds an assert that the principal of the channel loading > an SW is same-origin to the principal of the registration. Obviously, jar is > violating that somehow > https://treeherder.mozilla.org/logviewer.html#?job_id=8229547&repo=try > Could you tell me if this requires a relaxed assertion or something? Is the > jar trying to load a non-jar resource? Sorry, I cannot tell why that assertion is failing :( The JAR channel should only be loading JAR resources. FWIW I did a quick test and it seems that every request from that test goes to a URL like jar:file:///var/folders/zh/bl0hypv16pb3x474nvy2f3yh0000gp/T/tmpYYGc1w.mozrunner/webapps/%7B2bab62a9-9532-b04b-9a64-79da1d822aac%7D/application.zip!/* So that seems to be fine.

Andrea, please see comment 33 and reconsider the review of patch 2. Thanks.

Comment on attachment 8615688 [details] [diff] [review] Patch 2 bugzilla Review of attachment 8615688 [details] [diff] [review]: ----------------------------------------------------------------- Ok to have URLSearchParams not inheriting URLParams but in the source code can you split the 2 implementations? In the same file is ok but not mix URLSearchParams and URLParams methods. Thanks.

Comment on attachment 8615684 [details] MozReview Request: Bug 1169044 - Patch 2 - Split URLSearchParams parsing logic into non-CCed URLParams. r?baku https://reviewboard.mozilla.org/r/10343/#review9373 Ship It!

Per comment https://bugzilla.mozilla.org/show_bug.cgi?id=1155153#c58, setting v2 dependency here. Thanks!

Comment on attachment 8624921 [details] [diff] [review] Patch 6 - wpt expectations update Review of attachment 8624921 [details] [diff] [review]: ----------------------------------------------------------------- I think you didn't mean to delete the __dir__.ini files; that's a bug in the -update script (which is fixed upstream). Please restore these.

Comment on attachment 8612572 [details] MozReview Request: Bug 1169044 - Patch 1 - Refactor setting referrer and referrer policy between fetch and XHR. r?khuey https://reviewboard.mozilla.org/r/9599/#review10435 Ship It!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8473627ca5f https://hg.mozilla.org/integration/mozilla-inbound/rev/67b6e55eb4ac https://hg.mozilla.org/integration/mozilla-inbound/rev/66ea7374c23d https://hg.mozilla.org/integration/mozilla-inbound/rev/7000726b44f8 https://hg.mozilla.org/integration/mozilla-inbound/rev/08fe55879ba7 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c599a027376

https://hg.mozilla.org/mozilla-central/rev/a8473627ca5f https://hg.mozilla.org/mozilla-central/rev/67b6e55eb4ac https://hg.mozilla.org/mozilla-central/rev/66ea7374c23d https://hg.mozilla.org/mozilla-central/rev/7000726b44f8 https://hg.mozilla.org/mozilla-central/rev/08fe55879ba7 https://hg.mozilla.org/mozilla-central/rev/3c599a027376 https://hg.mozilla.org/mozilla-central/rev/a5cd9f18f1b6

Checking possible android regression as here: Bug 1179706 - Can't load page https://medium.com/starts-with-a-bang

(In reply to Mark Capella [:capella] from comment #77) > Checking possible android regression as here: > Bug 1179706 - Can't load page https://medium.com/starts-with-a-bang Separate issue. See bug 1178508. I will be landing a fix as soon as the trees re-open.