From 4f2ab389f0557985f02a0313626674fe0ed973c1 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 4 Feb 2021 10:47:33 -0800 Subject: [PATCH] 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 --- .../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 service-workers/service-worker/navigation-redirect-resolution.https.html create mode 100644 service-workers/service-worker/resources/subdir/blank.html diff --git a/service-workers/service-worker/navigation-redirect-resolution.https.html b/service-workers/service-worker/navigation-redirect-resolution.https.html new file mode 100644 index 000000000000000..59e1cafec3484a8 --- /dev/null +++ b/service-workers/service-worker/navigation-redirect-resolution.https.html @@ -0,0 +1,58 @@ + +Service Worker: Navigation Redirect Resolution + + + + + + diff --git a/service-workers/service-worker/redirected-response.https.html b/service-workers/service-worker/redirected-response.https.html index d2c7858beddfcf4..71b35d0e1205062 100644 --- a/service-workers/service-worker/redirected-response.https.html +++ b/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/service-workers/service-worker/resources/fetch-rewrite-worker.js b/service-workers/service-worker/resources/fetch-rewrite-worker.js index f2d49e2706a40fb..4631e83e0ceaab5 100644 --- a/service-workers/service-worker/resources/fetch-rewrite-worker.js +++ b/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/service-workers/service-worker/resources/redirect-worker.js b/service-workers/service-worker/resources/redirect-worker.js index ddcc2cf54789422..82e21fc26fdb942 100644 --- a/service-workers/service-worker/resources/redirect-worker.js +++ b/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/service-workers/service-worker/resources/subdir/blank.html b/service-workers/service-worker/resources/subdir/blank.html new file mode 100644 index 000000000000000..a3c3a4689a62b45 --- /dev/null +++ b/service-workers/service-worker/resources/subdir/blank.html @@ -0,0 +1,2 @@ + +Empty doc