1608074 - Add assertion to make sure referrer header matches the computed spec in referrerInfo

Status: REOPENED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1608074 - Add assertion to make sure referrer header matches the computed referrer in referrerInfo

Comment 2

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1608074 - Set channel ReferrerInfo in privileged code instead of referrer header

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Basti, may I ask you to drive this one over the finishing line please?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)

Basti, may I ask you to drive this one over the finishing line please?

302 to Julian - can you take a look at this one please?

Comment 5

[Julian Gaibler]

202 Accepted

Comment 6

[Shane Caraveo (:mixedpuppy)]

Because this bug has no description and no tests, I had to go back again to a conversation to find why this is being done. From a chat with ckerschb:

imagine a page sets a referrer-policy of 'no-referrer'
but we accidentally use a codepath that sets a referrer
the assertion is to make sure we can not accidentally create a new instance of a channel (or even protocol) without us realizing we are accidentally leaking the referrer value
I imagine that some devtools thing allows you to create a channel and set a referrer and then we don't have a valid referrerinfo for that

I'd like to see a test (the above makes a test sound possible), or an explanation why one is not necessary (or not possible), or (less desirable) a solid STR for QA to manually test.

Comment 7

[Julian Gaibler]

Thanks for digging up that message. Took me a while to understand what's going on but I think I got a clear picture now:

The first patch (D59314) adds debug assertions with MOZ_ASSERT to check if the ReferrerInfo object and the referer-header match. This means as long as there is any test that (A) sets the referer header and (B) sends the request, an implicit test that the referer was set correctly is also made.

To double-check that this is actually the case I ran three tests affected by the patch, with and without the changes from D59856:

./mach test devtools/client/netmonitor/test/browser_net_resend_headers.js
  (with)    OK
  (without) Assertion failure: referrerHeader.IsEmpty() (Referrer header should not be sent if there's no referrerInfo)

./mach test toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
  (with)    OK
  (without) Mozilla crash reason: MOZ_ASSERT(referrerHeader.IsEmpty()) (Referrer header should not be sent if there's no referrerInfo)

./mach test toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js 
  (with)    OK
  (without) Mozilla crash reason: MOZ_ASSERT(referrerHeader.IsEmpty()) (Referrer header should not be sent if there's no referrerInfo)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3fb1b5bbe34
Set channel ReferrerInfo in privileged code instead of referrer header r=nchevobbe,mixedpuppy,valentin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7aa5ac524911
Add assertion to make sure referrer header matches the computed referrer in referrerInfo r=ckerschb

Comment 10

[Alexandru Michis [:malexandru]]

Backed out changeset b3fb1b5bbe34 (Bug 1608074) for causing bustages in XMLHttpRequestMainThread.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/ba4e43d97863d39c19a7f261ba0d31bab88b64ee

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&tochange=ba4e43d97863d39c19a7f261ba0d31bab88b64ee&fromchange=f71a318e2dfcdd001ade192a5cdcde6419354a48&searchStr=build&selectedTaskRun=abrLwqvzRgOViUNK2B4jMw.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318475583&repo=autoland&lineNumber=12206

Comment 11

[Julian Gaibler]

Nrgg, sorry. I forgot to upload local changes :(

Comment 12

[mac198442]

It appears the other patch should also be backed out.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e44940803cd
Set channel ReferrerInfo in privileged code instead of referrer header r=nchevobbe,mixedpuppy,valentin

Comment 14

[Julian Gaibler]

If this one fails again, but I'm rather certain that it won't, then it's better to back both out, that's right! :)

Comment 15

[Narcis Beleuzu [:NarcisB]]

Backed out for assertion failure on nsContentSecurityUtils.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/e9abc628d101add1d024a6d5c668834477f532a7
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=318482733&repo=autoland&lineNumber=2397

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4e44940803cd

Comment 17

[Julian Gaibler]

The assertion patch still has to land. Before that, we need to figure out why and where it gets triggered and fix that as well.