1370617 - browser provided Authorization header exposed to FetchEvent.request in non-e10s mode

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

While trying to fix some test failures in fetch-response-taint.https.html I discovered that we incorrectly expose the browser provided Authorization header to FetchEvent.request.  We only do this in non-e10s mode, though.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'm going to solve this the same way I did for other headers in bug 1200337.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Set Authorization header using eVarietyRequestDefault to avoid exposing it to service workers. r=mcmanus

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2aa469ad58d87f450cba1dc687167165897dec8

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Set Authorization header using eVarietyRequestDefault to avoid exposing it to service workers. r=mcmanus

It seems extensions and proxies can override the Authorization header.  We should still consider these the "browser default" when it comes to exposing things to the service worker, though.

I work around this by have SetWWWCredentials() clear the header before setting it again as the default.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6387cf405839269d5ace8c0a5588b11fdfc7c3

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8875393 [details] [diff] [review]
Set Authorization header using eVarietyRequestDefault to avoid exposing it to service workers. r=mcmanus

Patrick, in non-e10s mode we are exposing the browser-provided Authorization header to service worker FetchEvent handlers.  This is a problem we had before with other headers in bug 1200337.  We solved it there by using a "default" header type and letting the service worker code only expose non-default values.

This patch tries to fix the problem by setting the Authorization header as a "default" value.

Unfortunately, its a bit trickier in that proxies and extensions can trigger setting the authorization header more than once.  This is a problem because the "default" header values have an assert that they are only set once.

To work around this issue this patch removes the header before setting it as the default.  I think this is valid because anything calling SetWWWCredentials() is effectively the "browser" from content script's perspective.

I'm open to other solutions as well, of course.  Thanks!

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8875393 [details] [diff] [review]
Set Authorization header using eVarietyRequestDefault to avoid exposing it to service workers. r=mcmanus

Review of attachment 8875393 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is ok, but let's have dragana take a quick look

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7006eaac1b49
Set Authorization header using eVarietyRequestDefault to avoid exposing it to service workers. r=dragana

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7006eaac1b49