whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.1k stars 324 forks source link

Have CORP check rely on request's response tainting rather than request's mode #985

Closed yutakahirano closed 4 years ago

yutakahirano commented 4 years ago

Brought up at https://chromium-review.googlesource.com/c/chromium/src/+/1971810, related with https://github.com/w3c/ServiceWorker/issues/1490.

We would like to run the CORP check in CacheStorage for COEP (https://github.com/w3c/ServiceWorker/issues/1490), but currently the CORP check relies on the request mode which is not directly accessible there. request's response tainting is stored as response type and we can use it. What do you think about changing the first item as follows?

If request's response tainting is not "opaque", then return allowed.

In the CacheStorage spec, we will restore a request for the CORP check as follows:

@ArthurSonzogni @wanderview @annevk

annevk commented 4 years ago

Given the check the only caller does today is

If httpRequest’s response tainting is not "cors"

per https://fetch.spec.whatwg.org/#ref-for-cross-origin-resource-policy-check perhaps that initial step

If request’s mode is not "no-cors", then return allowed.

is even redundant?

I think we should investigate how this should change together with the change to enforce this for a service worker response.

yutakahirano commented 4 years ago

Yes, that is a possible approach. Do you think it's better?

Regarding the service worker response, it is now part of https://github.com/mikewest/corpp.

annevk commented 4 years ago

I think I would prefer putting all the checks in Fetch, if possible (Fetch gets the response from the service worker and can do it, after all), rather than putting security checks in Service Workers for this.

yutakahirano commented 4 years ago

I prefer running a CORP check in cache.match(https://github.com/w3c/ServiceWorker/issues/1490), which means calling it from the SW spec.

annevk commented 4 years ago

Fair, I guess that would be the exception, but I don't think we need it for respondWith().