Closed
Bug 1369862
Opened 8 years ago
Closed 8 years ago
fix synthesized response tainting to make fetch spec
Categories
(Core :: DOM: Service Workers, enhancement)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files, 5 obsolete files)
2.27 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
6.92 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
There is still an issue with non-e10s to figure out. We are getting DOM_BAD_URI in one of the WPT tests.
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
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.
Attachment #8873969 -
Flags: review?(ckerschb)
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #8873970 -
Flags: review?(bugmail)
Assignee | ||
Comment 11•8 years ago
|
||
Update the WPT test expectations.
Attachment #8873971 -
Attachment is obsolete: true
Attachment #8875011 -
Flags: review?(bugmail)
Assignee | ||
Comment 12•8 years ago
|
||
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.
Attachment #8874593 -
Flags: review?(bugmail)
Updated•8 years ago
|
Attachment #8873970 -
Flags: review?(bugmail) → review+
Comment 13•8 years ago
|
||
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".
Attachment #8874593 -
Flags: review?(bugmail) → review+
Comment 14•8 years ago
|
||
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"
Attachment #8875011 -
Flags: review?(bugmail) → review+
Comment 15•8 years ago
|
||
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
Attachment #8873969 -
Flags: review?(ckerschb) → feedback+
Assignee | ||
Comment 16•8 years ago
|
||
Updated per the suggestion in comment 15, so claiming r+.
Attachment #8873969 -
Attachment is obsolete: true
Attachment #8875310 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8873970 -
Attachment is obsolete: true
Attachment #8875311 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
I'm going to land this before bug 1370617.
No longer depends on: 1370617
Assignee | ||
Comment 19•8 years ago
|
||
Rebase for new bug landing order.
Attachment #8875011 -
Attachment is obsolete: true
Attachment #8875313 -
Flags: review+
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8874593 -
Attachment is obsolete: true
Attachment #8875314 -
Flags: review+
Comment 21•8 years ago
|
||
Pushed by bkelly@mozilla.com:
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
Assignee | ||
Comment 22•8 years ago
|
||
Backed out because I messed up the bug number on P1.
https://hg.mozilla.org/integration/mozilla-inbound/rev/24c5cff1eadb25aba955c18a509651921dc4831e
Comment 23•8 years ago
|
||
Pushed by bkelly@mozilla.com:
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•8 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•