From 6d38d75bf28b917c08305852b91735bcd6657d32 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 9 Feb 2021 04:36:50 +0000 Subject: [PATCH] Bug 1690306 [wpt PR 27444] - Resolve Service Worker redirects based on the response, a=testonly Automatic update from web-platform-tests Resolve Service Worker redirects based on the response We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in https://github.com/whatwg/fetch/pull/1149. See also discussion in https://github.com/whatwg/fetch/issues/1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871 Reviewed-by: Makoto Shimazu Reviewed-by: Kinuko Yasuda Reviewed-by: Ben Kelly Commit-Queue: David Benjamin Cr-Commit-Position: refs/heads/master@{#850874} -- wpt-commits: 020c59f0ae3ce0a5649c8e811faca2101d947d63 wpt-pr: 27444 --- .../navigation-redirect-resolution.https.html | 58 +++++++++++ .../redirected-response.https.html | 98 ++++++++++++++++++- .../resources/fetch-rewrite-worker.js | 4 + .../resources/redirect-worker.js | 7 ++ .../resources/subdir/blank.html | 2 + 5 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 testing/web-platform/tests/service-workers/service-worker/navigation-redirect-resolution.https.html create mode 100644 testing/web-platform/tests/service-workers/service-worker/resources/subdir/blank.html diff --git a/testing/web-platform/tests/service-workers/service-worker/navigation-redirect-resolution.https.html b/testing/web-platform/tests/service-workers/service-worker/navigation-redirect-resolution.https.html new file mode 100644 index 0000000000000..59e1cafec3484 --- /dev/null +++ b/testing/web-platform/tests/service-workers/service-worker/navigation-redirect-resolution.https.html @@ -0,0 +1,58 @@ + +Service Worker: Navigation Redirect Resolution + + + + + + diff --git a/testing/web-platform/tests/service-workers/service-worker/redirected-response.https.html b/testing/web-platform/tests/service-workers/service-worker/redirected-response.https.html index d2c7858beddfc..71b35d0e12050 100644 --- a/testing/web-platform/tests/service-workers/service-worker/redirected-response.https.html +++ b/testing/web-platform/tests/service-workers/service-worker/redirected-response.https.html @@ -297,7 +297,7 @@ .then(() => { const url = host_info['HTTPS_ORIGIN'] + base_path() + 'sample?url=' + encodeURIComponent(TARGET_URL) + - '&original-redirect-mode=follow&sw=gen'; + '&original-redirect-mode=manual&sw=gen'; return redirected_test({url: url, fetch_option: {redirect: 'manual'}, fetch_method: frame.contentWindow.fetch, @@ -307,6 +307,102 @@ }), 'mode: "manual", generated redirect response'); +// ======================================================= +// Tests for requests that are in-scope of the service worker. The service +// worker returns a generated redirect response manually with the Response +// constructor. +// ======================================================= +promise_test(t => setup_and_clean() + .then(() => { + const url = host_info['HTTPS_ORIGIN'] + base_path() + + 'sample?url=' + encodeURIComponent(TARGET_URL) + + '&original-redirect-mode=follow&sw=gen-manual'; + return redirected_test({url: url, + fetch_option: {redirect: 'follow'}, + fetch_method: frame.contentWindow.fetch, + expected_type: 'basic', + expected_redirected: true, + expected_intercepted_urls: [url, TARGET_URL]}) + }), + 'mode: "follow", manually-generated redirect response'); + +promise_test(t => setup_and_clean() + .then(() => { + const url = host_info['HTTPS_ORIGIN'] + base_path() + + 'sample?url=' + encodeURIComponent(TARGET_URL) + + '&original-redirect-mode=error&sw=gen-manual'; + return promise_rejects_js( + t, frame.contentWindow.TypeError, + frame.contentWindow.fetch(url, {redirect: 'error'}), + 'The generated redirect response from the service worker should ' + + 'be treated as an error when the redirect flag of request was' + + ' \'error\'.') + .then(() => check_intercepted_urls([url])); + }), + 'mode: "error", manually-generated redirect response'); + +promise_test(t => setup_and_clean() + .then(() => { + const url = host_info['HTTPS_ORIGIN'] + base_path() + + 'sample?url=' + encodeURIComponent(TARGET_URL) + + '&original-redirect-mode=manual&sw=gen-manual'; + return redirected_test({url: url, + fetch_option: {redirect: 'manual'}, + fetch_method: frame.contentWindow.fetch, + expected_type: 'opaqueredirect', + expected_redirected: false, + expected_intercepted_urls: [url]}) + }), + 'mode: "manual", manually-generated redirect response'); + +// ======================================================= +// Tests for requests that are in-scope of the service worker. The service +// worker returns a generated redirect response with a relative location header. +// Generated responses do not have URLs, so this should fail to resolve. +// ======================================================= +promise_test(t => setup_and_clean() + .then(() => { + const url = host_info['HTTPS_ORIGIN'] + base_path() + + 'sample?url=blank.html' + + '&original-redirect-mode=follow&sw=gen-manual'; + return promise_rejects_js( + t, frame.contentWindow.TypeError, + frame.contentWindow.fetch(url, {redirect: 'follow'}), + 'Following the generated redirect response from the service worker '+ + 'should result fail.') + .then(() => check_intercepted_urls([url])); + }), + 'mode: "follow", generated relative redirect response'); + +promise_test(t => setup_and_clean() + .then(() => { + const url = host_info['HTTPS_ORIGIN'] + base_path() + + 'sample?url=blank.html' + + '&original-redirect-mode=error&sw=gen-manual'; + return promise_rejects_js( + t, frame.contentWindow.TypeError, + frame.contentWindow.fetch(url, {redirect: 'error'}), + 'The generated redirect response from the service worker should ' + + 'be treated as an error when the redirect flag of request was' + + ' \'error\'.') + .then(() => check_intercepted_urls([url])); + }), + 'mode: "error", generated relative redirect response'); + +promise_test(t => setup_and_clean() + .then(() => { + const url = host_info['HTTPS_ORIGIN'] + base_path() + + 'sample?url=blank.html' + + '&original-redirect-mode=manual&sw=gen-manual'; + return redirected_test({url: url, + fetch_option: {redirect: 'manual'}, + fetch_method: frame.contentWindow.fetch, + expected_type: 'opaqueredirect', + expected_redirected: false, + expected_intercepted_urls: [url]}) + }), + 'mode: "manual", generated relative redirect response'); + // ======================================================= // Tests for requests that are in-scope of the service worker. The service // worker returns a generated redirect response. And the fetch follows the diff --git a/testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js b/testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js index f2d49e2706a40..4631e83e0ceaa 100644 --- a/testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js +++ b/testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js @@ -133,6 +133,10 @@ self.addEventListener('fetch', function(event) { } } + if (params['clone']) { + response = response.clone(); + } + // |cache| means to bounce responses through Cache Storage and back. if (params['cache']) { var cacheName = "cached-fetches-" + performance.now() + "-" + diff --git a/testing/web-platform/tests/service-workers/service-worker/resources/redirect-worker.js b/testing/web-platform/tests/service-workers/service-worker/resources/redirect-worker.js index ddcc2cf547894..82e21fc26fdb9 100644 --- a/testing/web-platform/tests/service-workers/service-worker/resources/redirect-worker.js +++ b/testing/web-platform/tests/service-workers/service-worker/resources/redirect-worker.js @@ -117,6 +117,13 @@ self.addEventListener('fetch', function(event) { event.respondWith(waitUntilPromise.then(async () => { if (params['sw'] == 'gen') { return Response.redirect(params['url']); + } else if (params['sw'] == 'gen-manual') { + // Note this differs from Response.redirect() in that relative URLs are + // preserved. + return new Response("", { + status: 301, + headers: {location: params['url']}, + }); } else if (params['sw'] == 'fetch') { return fetch(event.request); } else if (params['sw'] == 'fetch-url') { diff --git a/testing/web-platform/tests/service-workers/service-worker/resources/subdir/blank.html b/testing/web-platform/tests/service-workers/service-worker/resources/subdir/blank.html new file mode 100644 index 0000000000000..a3c3a4689a62b --- /dev/null +++ b/testing/web-platform/tests/service-workers/service-worker/resources/subdir/blank.html @@ -0,0 +1,2 @@ + +Empty doc