1642531 - Fix expected fail on html/cross-origin-embedder-policy/require-corp-load-from-cache-storage.https.html

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Eden Chuang[:edenchuang]]

Fix web-platform test fail on html/cross-origin-embedder-policy/require-corp-load-from-cache-storage.https.html

Comment 1

[Eden Chuang[:edenchuang]]

I get confused with this line in the test.

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/html/cross-origin-embedder-policy/require-corp-load-from-cache-storage.https.html#113

When cache.match() is called with a URL string, according to the https://fetch.spec.whatwg.org/#dom-request step 6.5 and step 17, a fetch request is generated with cors mode. And according to https://wicg.github.io/cross-origin-embedder-policy/#corp-check step 1. CORP checking will return "Allowed" when the mode is cors. This is the reason why we get fail on these sub-tests.

So I expect we should create a request by the passed in request_mode. That means the test will be modified to

const request = new Request(url, {mode: request_mode}); 
if (response_type === 'error') {
  await promise_rejects_js(t, TypeError, cache.match(request));
  return;
}
const response = await cache.match(request);

But if the above modification is applied, the following sub-tests will fail.

Fetch cross-origin no-cors from service-worker and CacheStorage.
Fetch same-origin no-cors from service-worker and CacheStorage.
Fetch same-origin no-cors cors-disabled corp-undefined from network and CacheStorage.
Fetch same-origin no-cors cors-enabled corp-undefined from network and CacheStorage.

Because https://github.com/w3c/ServiceWorker/issues/1490 mentions if the response of cache.match() has no CORP header should return a TypeError. So these sub-tests should be updated with response_type "error".

Anne, could you help to confirm my thought?

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1642531 - Instead of using RequestMode, using ResponseType for CORP checking in cache APIs

Comment 3

[Anne (:annevk)]

The service worker script stores the responses and it has new Request(url, {mode: e.data.mode}). And if I'm reading https://w3c.github.io/ServiceWorker/#request-matches-cached-item correctly cache.match() doesn't take mode into account. So I don't think that analysis is correct.

Comment 4

[Eden Chuang[:edenchuang]]

Thanks for explaining, but I am still confused.

https://w3c.github.io/ServiceWorker/#request-matches-cached-item only mentioned the input of cache.match is a request. But it doesn't mention if the mode is taken into account. In fact, the input request could have a specific mode.

If the input is a string, new Request(input) is called implicitly, and cors mode is set in default.

The problem is we need to apply CORP checking on the response of cache.match(). If we don't take the input request's mode into account, that means https://wicg.github.io/cross-origin-embedder-policy/#corp-check step 1 would be ignored when cache.match()/cache.matchAll() is called. And it also could make a TypeError for the following code.

try {
const request = new Request(url, {mode: "cors"});
let responst = cache.match(request);
} catch(e) {
// got an TypeError, since CORP checking failed.
}

And I am not sure if this TypeError is expected.

And the following sub-tests fail in CORP checking if we ignore the input request's mode.
Fetch cross-origin cors from service-worker and CacheStorage.
Fetch same-origin cors from service-worker and CacheStorage
Fetch cross-origin cors cors-enabled corp-undefined from network and CacheStorage
Fetch cross-origin cors cors-enabled corp-undefined from network and CacheStorage

These fail tests make no sense to me since it specifies the mode is cors, and according to CORP checking spec, the checking should be skipped by step 1.

Comment 5

[Anne (:annevk)]

The problem here is a non-COEP process storing something using the cache API and a COEP-process trying to retrieve it using the same API. match() is only for retrieval and as defined does not care about request's mode. What needs to happen is that once match() is invoked in a COEP process and it finds something, we do an appropriate CORP check on the response found, and only if that is successful do we return the response to the process.

Comment 6

[Eden Chuang[:edenchuang]]

(In reply to Anne (:annevk) from comment #5)

The problem here is a non-COEP process storing something using the cache API and a COEP-process trying to retrieve it using the same API. match() is only for retrieval and as defined does not care about request's mode. What needs to happen is that once match() is invoked in a COEP process and it finds something, we do an appropriate CORP check on the response found, and only if that is successful do we return the response to the process.

If I understand this correctly, you mean we should only do CORP checking of cache.match() in COEP process. Otherwise, CORP checking should be ignored.
But if I understand correctly, it doesn't explain why sub-test "Fetch cross-origin no-cors cors-enabled corp-undefined from network and CacheStorage" should be error response type.

https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/testing/web-platform/tests/html/cross-origin-embedder-policy/require-corp-load-from-cache-storage.https.html#172

We can not consider it as no-cors in cache.match(), since it passed into cache.match() is cors mode. We make it by new Request(url) implicitly.

Comment 7

[Anne (:annevk)]

cache.match() doesn't put things into the cache, it just looks for things in the cache. cache.put() puts things in the cache and that code is a in different file (the service worker) and that does use the mode (via a Request object). See https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/testing/web-platform/tests/html/cross-origin-embedder-policy/resources/sw-store-to-cache-storage.js#9,20,28.

Comment 8

[Eden Chuang[:edenchuang]]

Let's look the sub-test "Fetch cross-origin no-cors cors-enabled corp-undefined from network and CacheStorage" deeply step by step.

The following creates a command for ServiceWorker for loading a cross-origin URL by no-cors mode, and the corp header is undefined.
Then sending it to the non-COEP ServiceWorker.

https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/testing/web-platform/tests/html/cross-origin-embedder-policy/require-corp-load-from-cache-storage.https.html#92-107

In the ServiceWorker message event handler, it creates the request by the cross-origin url and no-cors mode.
ServiceWorker gets the response by calling fetch(request). Then put the response into the cache v1.

https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/testing/web-platform/tests/html/cross-origin-embedder-policy/resources/sw-store-to-cache-storage.js#9,20,28

Notice that the cached response doesn't have the request_mode information, and it is not supposed to have the request_mode information.

Back to require-corp-load-from-cache-storage.https.html. In COEP process, opening the cache v1 and calling catch.match(url) to get the response, and it expects the type of the response is an Error.

https://searchfox.org/mozilla-central/rev/8ccea36c4fb09412609fb738c722830d7098602b/testing/web-platform/tests/html/cross-origin-embedder-policy/require-corp-load-from-cache-storage.https.html#110-115

Since the cached response has no request_mode information, it makes no difference between calling with/without setting request mode for the input request of cache.match() in the non-COEP process. They all get the cached response.
Since the cache.match() is in COEP process, Applying CORP checking on the cached response.
For CORP checking, we need the request_mode. Since the response has no request_mode information, we only can get it from the input request of cache.match(), I think the input request of cache.match is a reasonable source since it is the same with what applied on fetch().

If we send a cors request in fetch() the CORP checking is supposed to return Allowed, and I assume the same concept on cache.match(). That means if a cors mode request is sent into cache.match(), CORP checking is supposed to return Allowed.

Now we meet the problem when we call cache.match(url_string) since its input request will be cors mode. It makes the CORP checking passed, but we expected it to fail since it is no_cors mode test.

Since the test fails, here are some questions.
Should the cached response have request_mode information? I guess it should not, otherwise other sub-tests fails since they can not get response.
If it should not, why this sub-test expect an error response? Or why not this sub-test calling cache.match() with a no_cors mode request, but an URL string?

Comment 9

[Anne (:annevk)]

https://github.com/w3c/ServiceWorker/pull/1516 defines how this is supposed to work. It does seem that the patch there creates a special kind of request for the purposes of the CORP check. (Perhaps there's a better way to accomplish that though, it hasn't had much review yet.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2e0f62377bc
Instead of using RequestMode, using ResponseType for CORP checking in cache APIs r=dom-workers-and-storage-reviewers,perry

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d2e0f62377bc