An origin referer header sent by Firefox does not include a trailing slash (neither does the origin header). While it is not clear to me whether this should be the case or not, this breaks W3C web platform tests for referer policies [1]. Also see [2] (not sure though why Mike says Firefox does include the trailing slash). [1] https://github.com/w3c/web-platform-tests/tree/master/referrer-policy [2] https://github.com/w3c/webappsec/issues/382

As far as the "Origin" header goes, the specification for that is at https://fetch.spec.whatwg.org/#origin-header and most definitely does NOT include the trailing slash in the BNF. If you look at the processing model, https://fetch.spec.whatwg.org/#http-network-or-cache-fetch step 7 uses https://html.spec.whatwg.org/multipage/browsers.html#ascii-serialisation-of-an-origin which also clearly does not include the trailing slash. I would be _very_ surprised if any browser includes a trailing slash in the "Origin" header. For "Referer", we could include the trailing "/" when it's not "null", sure.

Ok, agree. This seems to be a referrer policy bug [1], i.e. the referer header has a trailing slash if it is actually only an origin, but is missing one if a referrer policy is set that forces the referer header to be set to the origin. [1] https://github.com/w3c/webappsec/issues/382#issuecomment-123513647

This is a hot fix for adding a trailing slash to the referer header when a referrer policy is set and fixes tests accordingly. We might want to move this to nsStandardURL and use something else than nsStandardURL::GetPrePath here (add something like GetOrigin to nsIURI.idl that returns essentially the same thing as GetPrePath but with a trailing slash, I don't think we can use any of the existing functions for this).

this patch is already a bit old and would probably have to be rebased, but in order to get web-platform tests working we have to land something like this. Christoph/Steve, would you know a better place to do this or shall we do it in HttpBaseChannel?

After a quick glance through DXR and since it's an HTTP header, I think putting it in HttpBaseChannel makes sense. (Interestingly, the Origin header is special cased in nsHttpRequestHead - it has its own functions in that class, but Referer is just another header in the array. So it should be isolated from any changes you make to Referer).

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #4) > this patch is already a bit old and would probably have to be rebased, but > in order to get web-platform tests working we have to land something like > this. Christoph/Steve, would you know a better place to do this or shall we > do it in HttpBaseChannel? I agree, it can live in HttpBaseChannel. Please make sure to replace > spec = spec + NS_LITERAL_CSTRING("/"); with something like > spec.AppendLiteral("/"); [or whatever equivalent append functionality exists on nsAutoCString].

Review commit: https://reviewboard.mozilla.org/r/36639/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36639/

Comment on attachment 8723587 [details] MozReview Request: Bug 1186072 - add trailing slash to origin referer header when policy is set, r=sworkman https://reviewboard.mozilla.org/r/36639/#review33235 1 question, 1 nit. Otherwise r=me - thanks Franziskus! ::: dom/base/test/bug704320_counter.sjs:44 (Diff revision 1) > - } else if (referrer == "http://mochi.test:8888") { > + } else if (referrer.indexOf("http://mochi.test:8888") == 0) { What if the referrer has "http://mochi.test:8888" at the start of the string, but the path is wrong? Do you need to worry about that here? Is it better to look for "http://mochi.test:8888/" - i.e. origin with a trailing slash? ::: netwerk/protocol/http/HttpBaseChannel.cpp:1498 (Diff revision 1) > + spec.AppendLiteral("/"); It would be nice to be explicit in the preceding comment that a trailing slash is required per spec.

Comment on attachment 8723587 [details] MozReview Request: Bug 1186072 - add trailing slash to origin referer header when policy is set, r=sworkman https://reviewboard.mozilla.org/r/36639/#review33505 Looks good to me, please answers Steve's question. r=me

https://reviewboard.mozilla.org/r/36639/#review33235 > What if the referrer has "http://mochi.test:8888" at the start of the string, but the path is wrong? Do you need to worry about that here? Is it better to look for "http://mochi.test:8888/" - i.e. origin with a trailing slash? I can't remember why, but someone thought this would be a good idea and I let me convince to do it. Nevertheless, we should check == here.

Comment on attachment 8723587 [details] MozReview Request: Bug 1186072 - add trailing slash to origin referer header when policy is set, r=sworkman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36639/diff/1-2/

Comment on attachment 8723587 [details] MozReview Request: Bug 1186072 - add trailing slash to origin referer header when policy is set, r=sworkman https://reviewboard.mozilla.org/r/36639/#review33721 Thanks Franziskus! r=me.

Please note the test that I just landed, see for example: <https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5cc380ddb3#l2.60>. This test is doing this dance because of this bug and will probably (hopefully!) fail with your patch. Please switch <https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5cc380ddb3#l2.28> to compare against expectedReferrerSpec and drop expectedReferrerHeader completely. Thanks!

Comment on attachment 8723587 [details] MozReview Request: Bug 1186072 - add trailing slash to origin referer header when policy is set, r=sworkman Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36639/diff/2-3/

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=22926320&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7c00cae4ec

Those tests have been working before because they're wrong [1]. We can either be spec compliant or pass the web platform tests at the moment. I'm not sure if passing those tests is more important. [1] https://github.com/w3c/web-platform-tests/issues/2618

Tests, like specs, are not handed down on stone tablets; they can be wrong. You can either fix the tests in-tree and they'll get updated upstream automagically, or you can update the annotations. See testing/web-platform/README.md for more docs, or ping me any time.

There is also documentation on MDN these days [1], in case you like that better. But yes, what Ms2ger said. For preference fix the tests. If that isn't possible for some reason, change the metadata. [1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/web-platform-tests

I think it should be fairly straightforward to fix the tests here.

Review commit: https://reviewboard.mozilla.org/r/40041/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40041/

Comment on attachment 8730603 [details] MozReview Request: Bug 1186072 - web-platform-tests fetch origin referrer miss slash, r?fkiefer https://reviewboard.mozilla.org/r/40041/#review36571 lgtm. But could you do a try run with this to see if this fixes all web platform test errors? Could you also file a PR against the web platform tests upstream [1] to get this fixed? [1] https://github.com/w3c/web-platform-tests/tree/master/fetch

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26) > Could you also file a PR against the web platform tests upstream [1] to get > this fixed? The neat thing is that this will happen automagically once this commit lands into m-c.

(In reply to :Ms2ger from comment #27) > (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26) > > Could you also file a PR against the web platform tests upstream [1] to get > > this fixed? > > The neat thing is that this will happen automagically once this commit lands > into m-c. oh, I didn't know about that. That's cool. Well, then ignore that part of the comment Thomas.

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26) > Comment on attachment 8730603 [details] > MozReview Request: Bug 1186072 - web-platform-tests fetch origin referrer > miss slash, r?fkiefer > > https://reviewboard.mozilla.org/r/40041/#review36571 > > lgtm. But could you do a try run with this to see if this fixes all web > platform test errors? > Thanks for looking into the patch. referrer-policy test cases still fail for example in the try run. https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb9c620463a. Failure example TEST-UNEXPECTED-PASS | /referrer-policy/origin-only/http-csp/cross-origin/http-http/fetch-request/generic.keep-origin-redirect.http.html | The referrer URL is origin when a This is due to FAIL expectation data in metadata file. I think it should be set to PASS instead of. Let me change and run a new try. Thanks

(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #29) > This is due to FAIL expectation data in metadata file. I think it should be > set to PASS instead of. No. Since that is the only expected failure in that ini file, you can remove the ini file completely since after getting rid of that expected failure, there's nothing more left in the file that is useful. Note that you can also run these tests locally after removing that ini file (and other similar ones), for example by running: ./mach web-platform-tests /referrer-policy/

Comment on attachment 8730603 [details] MozReview Request: Bug 1186072 - web-platform-tests fetch origin referrer miss slash, r?fkiefer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40041/diff/1-2/

Review commit: https://reviewboard.mozilla.org/r/40729/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40729/

Review commit: https://reviewboard.mozilla.org/r/40731/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40731/

(In reply to :Ehsan Akhgari from comment #30) > Since that is the only expected failure in that ini file, you can > remove the ini file completely since after getting rid of that expected > failure, there's nothing more left in the file that is useful. Thanks Ehsan Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34eaffa946ed

Comment on attachment 8731584 [details] MozReview Request: Bug 1186072 - fix web-platform-tests service worker fetch , r?fkiefer https://reviewboard.mozilla.org/r/40729/#review37639 lgtm

Comment on attachment 8731585 [details] MozReview Request: Bug 1186072 - Run referrer-policy web platform tests expected pass, r?fkiefer https://reviewboard.mozilla.org/r/40731/#review37641 I presume that's all files, didn't check them individually. But judging from the try run it's all good.

Franziskus, looks like we need to get someone else (or more than one person) to do final reviews on these patches, right?

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #37) > Franziskus, looks like we need to get someone else (or more than one person) > to do final reviews on these patches, right? r=me. :-)

Awesome! Thanks Ehsan :)

https://reviewboard.mozilla.org/r/40731/#review37641 I only changed expected=pass for expected=fail to correct failed test cases. Some "disabled" test cases still remains (for example image tag test cases will be disabled due to https://github.com/w3c/web-platform-tests/issues/1874)

https://hg.mozilla.org/integration/fx-team/rev/768bd20b25ae https://hg.mozilla.org/integration/fx-team/rev/87b00f4843cc https://hg.mozilla.org/integration/fx-team/rev/ca466d057591 https://hg.mozilla.org/integration/fx-team/rev/e10424cf620f

https://hg.mozilla.org/mozilla-central/rev/768bd20b25ae https://hg.mozilla.org/mozilla-central/rev/87b00f4843cc https://hg.mozilla.org/mozilla-central/rev/ca466d057591 https://hg.mozilla.org/mozilla-central/rev/e10424cf620f