Skip to content

Commit db47143

Browse files
davidbenchromium-wpt-export-bot
davidben
authored and
chromium-wpt-export-bot
committed
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 whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d
1 parent 284cfb0 commit db47143

5 files changed

Lines changed: 168 additions & 1 deletion

File tree

‎service-workers/service-worker/navigation-redirect-resolution.https.html‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<!DOCTYPE html>
2+
<title>Service Worker: Navigation Redirect Resolution</title>
3+
<script src="/resources/testharness.js"></script>
4+
<script src="/resources/testharnessreport.js"></script>
5+
<script src="resources/test-helpers.sub.js"></script>
6+
<body>
7+
<script>
8+
9+
function make_absolute(url) {
10+
return new URL(url, location).toString();
11+
}
12+
13+
const script = 'resources/fetch-rewrite-worker.js';
14+
15+
function redirect_result_test(scope, expected_url, description) {
16+
promise_test(async t => {
17+
const registration = await service_worker_unregister_and_register(
18+
t, script, scope);
19+
t.add_cleanup(() => {
20+
return service_worker_unregister(t, scope);
21+
})
22+
await wait_for_state(t, registration.installing, 'activated');
23+
24+
// The navigation to |scope| will be resolved by a fetch to |redirect_url|
25+
// which returns a relative Location header. If it is resolved relative to
26+
// |scope|, the result will be navigate-redirect-resolution/blank.html. If
27+
// relative to |redirect_url|, it will be resources/blank.html. The latter
28+
// is correct.
29+
const iframe = await with_iframe(scope);
30+
t.add_cleanup(() => { iframe.remove(); });
31+
assert_equals(iframe.contentWindow.location.href,
32+
make_absolute(expected_url));
33+
}, description);
34+
}
35+
36+
// |redirect_url| serves a relative redirect to resources/blank.html.
37+
const redirect_url = 'resources/redirect.py?Redirect=blank.html';
38+
39+
// |scope_base| does not exist but will be replaced with a fetch of
40+
// |redirect_url| by fetch-rewrite-worker.js.
41+
const scope_base = 'resources/subdir/navigation-redirect-resolution?' +
42+
'redirect-mode=manual&url=' +
43+
encodeURIComponent(make_absolute(redirect_url));
44+
45+
// When the Service Worker forwards the result of |redirect_url| as an
46+
// opaqueredirect response, the redirect uses the response's URL list as the
47+
// base URL, not the request.
48+
redirect_result_test(scope_base, 'resources/blank.html',
49+
'test relative opaqueredirect');
50+
51+
// The response's base URL should be preserved across CacheStorage and clone.
52+
redirect_result_test(scope_base + '&cache=1', 'resources/blank.html',
53+
'test relative opaqueredirect with CacheStorage');
54+
redirect_result_test(scope_base + '&clone=1', 'resources/blank.html',
55+
'test relative opaqueredirect with clone');
56+
57+
</script>
58+
</body>

‎service-workers/service-worker/redirected-response.https.html‎

Lines changed: 97 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@
297297
.then(() => {
298298
const url = host_info['HTTPS_ORIGIN'] + base_path() +
299299
'sample?url=' + encodeURIComponent(TARGET_URL) +
300-
'&original-redirect-mode=follow&sw=gen';
300+
'&original-redirect-mode=manual&sw=gen';
301301
return redirected_test({url: url,
302302
fetch_option: {redirect: 'manual'},
303303
fetch_method: frame.contentWindow.fetch,
@@ -307,6 +307,102 @@
307307
}),
308308
'mode: "manual", generated redirect response');
309309

310+
// =======================================================
311+
// Tests for requests that are in-scope of the service worker. The service
312+
// worker returns a generated redirect response manually with the Response
313+
// constructor.
314+
// =======================================================
315+
promise_test(t => setup_and_clean()
316+
.then(() => {
317+
const url = host_info['HTTPS_ORIGIN'] + base_path() +
318+
'sample?url=' + encodeURIComponent(TARGET_URL) +
319+
'&original-redirect-mode=follow&sw=gen-manual';
320+
return redirected_test({url: url,
321+
fetch_option: {redirect: 'follow'},
322+
fetch_method: frame.contentWindow.fetch,
323+
expected_type: 'basic',
324+
expected_redirected: true,
325+
expected_intercepted_urls: [url, TARGET_URL]})
326+
}),
327+
'mode: "follow", manually-generated redirect response');
328+
329+
promise_test(t => setup_and_clean()
330+
.then(() => {
331+
const url = host_info['HTTPS_ORIGIN'] + base_path() +
332+
'sample?url=' + encodeURIComponent(TARGET_URL) +
333+
'&original-redirect-mode=error&sw=gen-manual';
334+
return promise_rejects_js(
335+
t, frame.contentWindow.TypeError,
336+
frame.contentWindow.fetch(url, {redirect: 'error'}),
337+
'The generated redirect response from the service worker should ' +
338+
'be treated as an error when the redirect flag of request was' +
339+
' \'error\'.')
340+
.then(() => check_intercepted_urls([url]));
341+
}),
342+
'mode: "error", manually-generated redirect response');
343+
344+
promise_test(t => setup_and_clean()
345+
.then(() => {
346+
const url = host_info['HTTPS_ORIGIN'] + base_path() +
347+
'sample?url=' + encodeURIComponent(TARGET_URL) +
348+
'&original-redirect-mode=manual&sw=gen-manual';
349+
return redirected_test({url: url,
350+
fetch_option: {redirect: 'manual'},
351+
fetch_method: frame.contentWindow.fetch,
352+
expected_type: 'opaqueredirect',
353+
expected_redirected: false,
354+
expected_intercepted_urls: [url]})
355+
}),
356+
'mode: "manual", manually-generated redirect response');
357+
358+
// =======================================================
359+
// Tests for requests that are in-scope of the service worker. The service
360+
// worker returns a generated redirect response with a relative location header.
361+
// Generated responses do not have URLs, so this should fail to resolve.
362+
// =======================================================
363+
promise_test(t => setup_and_clean()
364+
.then(() => {
365+
const url = host_info['HTTPS_ORIGIN'] + base_path() +
366+
'sample?url=blank.html' +
367+
'&original-redirect-mode=follow&sw=gen-manual';
368+
return promise_rejects_js(
369+
t, frame.contentWindow.TypeError,
370+
frame.contentWindow.fetch(url, {redirect: 'follow'}),
371+
'Following the generated redirect response from the service worker '+
372+
'should result fail.')
373+
.then(() => check_intercepted_urls([url]));
374+
}),
375+
'mode: "follow", generated relative redirect response');
376+
377+
promise_test(t => setup_and_clean()
378+
.then(() => {
379+
const url = host_info['HTTPS_ORIGIN'] + base_path() +
380+
'sample?url=blank.html' +
381+
'&original-redirect-mode=error&sw=gen-manual';
382+
return promise_rejects_js(
383+
t, frame.contentWindow.TypeError,
384+
frame.contentWindow.fetch(url, {redirect: 'error'}),
385+
'The generated redirect response from the service worker should ' +
386+
'be treated as an error when the redirect flag of request was' +
387+
' \'error\'.')
388+
.then(() => check_intercepted_urls([url]));
389+
}),
390+
'mode: "error", generated relative redirect response');
391+
392+
promise_test(t => setup_and_clean()
393+
.then(() => {
394+
const url = host_info['HTTPS_ORIGIN'] + base_path() +
395+
'sample?url=blank.html' +
396+
'&original-redirect-mode=manual&sw=gen-manual';
397+
return redirected_test({url: url,
398+
fetch_option: {redirect: 'manual'},
399+
fetch_method: frame.contentWindow.fetch,
400+
expected_type: 'opaqueredirect',
401+
expected_redirected: false,
402+
expected_intercepted_urls: [url]})
403+
}),
404+
'mode: "manual", generated relative redirect response');
405+
310406
// =======================================================
311407
// Tests for requests that are in-scope of the service worker. The service
312408
// worker returns a generated redirect response. And the fetch follows the

‎service-workers/service-worker/resources/fetch-rewrite-worker.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,10 @@ self.addEventListener('fetch', function(event) {
133133
}
134134
}
135135

136+
if (params['clone']) {
137+
response = response.clone();
138+
}
139+
136140
// |cache| means to bounce responses through Cache Storage and back.
137141
if (params['cache']) {
138142
var cacheName = "cached-fetches-" + performance.now() + "-" +

‎service-workers/service-worker/resources/redirect-worker.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,13 @@ self.addEventListener('fetch', function(event) {
117117
event.respondWith(waitUntilPromise.then(async () => {
118118
if (params['sw'] == 'gen') {
119119
return Response.redirect(params['url']);
120+
} else if (params['sw'] == 'gen-manual') {
121+
// Note this differs from Response.redirect() in that relative URLs are
122+
// preserved.
123+
return new Response("", {
124+
status: 301,
125+
headers: {location: params['url']},
126+
});
120127
} else if (params['sw'] == 'fetch') {
121128
return fetch(event.request);
122129
} else if (params['sw'] == 'fetch-url') {

‎service-workers/service-worker/resources/subdir/blank.html‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<!DOCTYPE html>
2+
<title>Empty doc</title>

0 commit comments

Comments
 (0)