1369862 - fix synthesized response tainting to make fetch spec

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

We are currently failing a bunch of tests in fetch-response-tainting.https.html.  This is because we set tainting on the nsIChannel immediately and then only maybe increase it when the service worker intercepts it.

Instead the fetch spec is written to return the response produced by the service worker directly.  So its exact tainting should be used even if its lower than the original nsIChannel.

So a cross-origin no-cors request that is intercepted with a basic response would result in a basic response.  Today we return an opaque in this circumstance.

See this spec issue for more discussion:

https://github.com/whatwg/fetch/issues/535

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose a SynthesizeServiceWorkerTainting() on nsILoadInfo. r=ckerschb

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update expectations on fetch-response-taint.https.html. r=asuth

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

There is still an issue with non-e10s to figure out.  We are getting DOM_BAD_URI in one of the WPT tests.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

The non-e10s issue is bug 1370617.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8873969 [details] [diff] [review]
P1 Expose a SynthesizeServiceWorkerTainting() on nsILoadInfo. r=ckerschb

Christoph, this adds an nsILoadInfo API to set the tainting level.  Normally we only allow the tainting to get more restrictive, but here we actually need to set to an explicit level.  It can go from more-tainted to less-tainted.

The situation where this is allowed is:

1. Page controlled by a service worker does a cross-origin no-cors request
2. Service worker intercepts and passes a basic Response to FetchEvent.respondWith()
3. Resulting page should see the basic Response instead of an opaque Response

It works this way because in the spec the Response passed to respondWith() should be passed directly back to the outer window.  Our implementation, however, has to synthesize the Response back on to the original nsIChannel.  So we end up overwriting the original tainting value.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8873970 [details] [diff] [review]
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth

This patch uses the new nsILoadInfo API to override the tainting value when we synthesize the response on the nsIChannel.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update expectations on fetch-response-taint.https.html. r=asuth

Update the WPT test expectations.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8874593 [details] [diff] [review]
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

Update our mochitests to expect the new behavior.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8874593 [details] [diff] [review]
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

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

restating: the "cors" response is now properly exposed as "cors" because we are able to lower the tainting from "no-cors"/"opaque".

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8875011 [details] [diff] [review]
P3 Update expectations on fetch-response-taint.https.html. r=asuth

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

Confirming that the removed failures were all cases of inability to lower tainting, specifically for (cross-origin) fetch requests against OTHER_ORIGIN (by a BASE_ORIGIN page):
* SW responds with "same-origin" mode, "omit" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "basic"
  * "cors", all 3 credential types: could not be lowered from "cors" to "basic"
* SW responds with "same-origin" mode, "same-origin" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "basic"
  * "cors", all 3 credential types: could not be lowered from "cors" to "basic"
* SW responds with (cors-approved) "cors" mode, "omit" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "cors"
* SW responds with (cros-approved) "cors" mode, "include" creds to page fetch with
  * "no-cors", all 3 credential types: could not be lowered from "opaque" to "cors"

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8873969 [details] [diff] [review]
P1 Expose a SynthesizeServiceWorkerTainting() on nsILoadInfo. r=ckerschb

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

Hey Ben, I am a little worried that someone will abuse that API. I would feel more confident to just add a method to the LoadInfo and then do the following on the callsite:
static_cast<mozilla::LoadInfo*>(loadInfo.get())->SynthesizeServiceWorkerTainting(tainting);

For example, we don't allow changing the security flags after creation of the loadInfo, but for one case we had to allow that. So we added a method CloneWithNewSecFlags [1]. I would prefer if we use similar semantics for your tainting mechanism. Please add a 'hands off!'-comment in LoadInfo.h that no one should use it unless they know exactly what they are doing!

(Please note that I will be on PTO starting tonight. I am adding f+ to this patch, you can consider this as an r+ if you incorporate my suggestion!)

[1] https://dxr.mozilla.org/mozilla-central/search?q=CloneWithNewSecFlags&redirect=false

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose LoadInfo::SynthesizeServiceWorkerTainting(). r=ckerschb

Updated per the suggestion in comment 15, so claiming r+.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

I'm going to land this before bug 1370617.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Update expectations on fetch-response-taint.https.html. r=asuth

Rebase for new bug landing order.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/674e032ea90c
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ecdf657881e
P3 Update expectations on fetch-response-taint.https.html. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe11fc40572
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Backed out because I messed up the bug number on P1.

https://hg.mozilla.org/integration/mozilla-inbound/rev/24c5cff1eadb25aba955c18a509651921dc4831e

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d61441b6594e
P1 Expose LoadInfo::SynthesizeServiceWorkerTainting(). r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7d7b7c2d29
P2 Make the FetchEvent.respondWith() synthesize the exact tainting on the outer nsIChannel. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4d1d742563
P3 Update expectations on fetch-response-taint.https.html. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a7b65a51a0b
P4 Update test_fetch_event.html expectations for synthesized CORS response returned for outer no-cors request. r=asuth

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d61441b6594e
https://hg.mozilla.org/mozilla-central/rev/7d7d7b7c2d29
https://hg.mozilla.org/mozilla-central/rev/7a4d1d742563
https://hg.mozilla.org/mozilla-central/rev/1a7b65a51a0b