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
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#850874}

content/browser/service_worker/service_worker_browsertest.cc

@@ -63,6 +63,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/navigation_handle_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/public/test/url_loader_interceptor.h"
 #include "content/shell/browser/shell.h"
@@ -1128,6 +1129,36 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest,
   base::RunLoop().RunUntilIdle();
 }
 
+// Check that, if the worker synthesizes a redirect with an invalid URL, the
+// navigation fails. See https://crbug.com/1170379.
+IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest, RedirectInvalid) {
+  StartServerAndNavigateToSetup();
+  GURL page_url =
+      embedded_test_server()->GetURL("/service_worker/redirect.html");
+  GURL worker_url = embedded_test_server()->GetURL(
+      "/service_worker/fetch_event_invalid_redirect.js");
+  auto observer = base::MakeRefCounted<WorkerStateObserver>(
+      wrapper(), ServiceWorkerVersion::ACTIVATED);
+  observer->Init();
+  blink::mojom::ServiceWorkerRegistrationOptions options(
+      page_url, blink::mojom::ScriptType::kClassic,
+      blink::mojom::ServiceWorkerUpdateViaCache::kImports);
+  public_context()->RegisterServiceWorker(
+      worker_url, options,
+      base::BindOnce(&ExpectRegisterResultAndRun,
+                     blink::ServiceWorkerStatusCode::kOk, base::DoNothing()));
+  observer->Wait();
+
+  // If the service worker responds with an invalid redirect, the navigation to
+  // |page_url| should ultimately fail.
+  NavigationHandleObserver navigation_observer(shell()->web_contents(),
+                                               page_url);
+  EXPECT_FALSE(NavigateToURL(shell(), page_url));
+  // ERR_UNSAFE_REDIRECT or ERR_INVALID_REDIRECT would be a more natural error,
+  // but the workaround for https://crbug.com/941653 ends up applying here too.
+  EXPECT_EQ(net::ERR_ABORTED, navigation_observer.net_error_code());
+}
+
 class ServiceWorkerEagerCacheStorageSetupTest
     : public ServiceWorkerBrowserTest {
  public:

content/browser/service_worker/service_worker_main_resource_loader_unittest.cc

@@ -152,9 +152,9 @@ class FetchEventServiceWorker : public FakeServiceWorker {
   }
 
   // Tells this worker to respond to fetch events with the redirect response.
-  void RespondWithRedirectResponse(const GURL& new_url) {
+  void RespondWithRedirectResponse(const std::string& location_header) {
     response_mode_ = ResponseMode::kRedirect;
-    redirected_url_ = new_url;
+    location_header_ = location_header;
   }
 
   // Tells this worker to simulate failure to dispatch the fetch event to the
@@ -314,7 +314,7 @@ class FetchEventServiceWorker : public FakeServiceWorker {
         // Now the caller must call FinishWaitUntil() to finish the event.
         break;
       case ResponseMode::kRedirect:
-        response_callback->OnResponse(RedirectResponse(redirected_url_.spec()),
+        response_callback->OnResponse(RedirectResponse(location_header_),
                                       std::move(timing));
         std::move(finish_callback)
             .Run(blink::mojom::ServiceWorkerEventStatus::COMPLETED);
@@ -364,7 +364,7 @@ class FetchEventServiceWorker : public FakeServiceWorker {
       response_callback_;
 
   // For ResponseMode::kRedirect.
-  GURL redirected_url_;
+  std::string location_header_;
 
   // For ResponseMode::kHeaders
   base::flat_map<std::string, std::string> headers_;
@@ -1005,7 +1005,7 @@ TEST_F(ServiceWorkerMainResourceLoaderTest, EarlyResponse) {
 TEST_F(ServiceWorkerMainResourceLoaderTest, Redirect) {
   base::HistogramTester histogram_tester;
   GURL new_url("https://example.com/redirected");
-  service_worker_->RespondWithRedirectResponse(new_url);
+  service_worker_->RespondWithRedirectResponse(new_url.spec());
 
   // Perform the request.
   StartRequest(CreateRequest());
@@ -1024,6 +1024,29 @@ TEST_F(ServiceWorkerMainResourceLoaderTest, Redirect) {
                                       blink::ServiceWorkerStatusCode::kOk, 1);
 }
 
+// Synthetic response lack a base URL, so relative redirects turn into a
+// redirect to an invalid URL. See https://crbug.com/1170379.
+TEST_F(ServiceWorkerMainResourceLoaderTest, RedirectRelativeNoBaseURL) {
+  base::HistogramTester histogram_tester;
+  service_worker_->RespondWithRedirectResponse("/foo.html");
+
+  // Perform the request.
+  StartRequest(CreateRequest());
+  client_.RunUntilRedirectReceived();
+
+  auto& info = client_.response_head();
+  EXPECT_EQ(301, info->headers->response_code());
+  ExpectResponseInfo(*info, *CreateResponseInfoFromServiceWorker());
+
+  const net::RedirectInfo& redirect_info = client_.redirect_info();
+  EXPECT_EQ(301, redirect_info.status_code);
+  EXPECT_EQ("GET", redirect_info.new_method);
+  EXPECT_FALSE(redirect_info.new_url.is_valid());
+
+  histogram_tester.ExpectUniqueSample(kHistogramMainResourceFetchEvent,
+                                      blink::ServiceWorkerStatusCode::kOk, 1);
+}
+
 TEST_F(ServiceWorkerMainResourceLoaderTest, Lifetime) {
   StartRequest(CreateRequest());
   base::WeakPtr<ServiceWorkerMainResourceLoader> loader = loader_->AsWeakPtr();

content/common/service_worker/service_worker_loader_helpers.cc

@@ -133,6 +133,18 @@ ServiceWorkerLoaderHelpers::ComputeRedirectInfo(
   if (!response_head.headers->IsRedirect(&new_location))
     return base::nullopt;
 
+  // Note Service Workers resolve redirects based on the resource's URL list,
+  // not the request's URL. There also may not be a base URL if the resource was
+  // script-constructed. See https://github.com/whatwg/fetch/issues/1146 and
+  // https://github.com/whatwg/fetch/pull/1149.
+  GURL location_url;
+  if (response_head.url_list_via_service_worker.empty()) {
+    location_url = GURL(new_location);
+  } else {
+    location_url =
+        response_head.url_list_via_service_worker.back().Resolve(new_location);
+  }
+
   // If the request is a MAIN_FRAME request, the first-party URL gets
   // updated on redirects.
   const net::RedirectInfo::FirstPartyURLPolicy first_party_url_policy =
@@ -145,8 +157,7 @@ ServiceWorkerLoaderHelpers::ComputeRedirectInfo(
       original_request.site_for_cookies, first_party_url_policy,
       original_request.referrer_policy,
       original_request.referrer.GetAsReferrer().spec(),
-      response_head.headers->response_code(),
-      original_request.url.Resolve(new_location),
+      response_head.headers->response_code(), location_url,
       net::RedirectUtil::GetReferrerPolicyHeader(response_head.headers.get()),
       false /* insecure_scheme_was_upgraded */);
 }

content/common/service_worker/service_worker_loader_helpers.h

@@ -36,7 +36,9 @@ class ServiceWorkerLoaderHelpers {
                                network::mojom::URLResponseHead* out_head);
 
   // Returns a redirect info if |response_head| is an redirect response.
-  // Otherwise returns base::nullopt.
+  // Otherwise returns base::nullopt. Note, if the location URL fails to parse,
+  // this function will return a RedirectInfo with an invalid |new_url|
+  // (is_valid() will return false).
   static base::Optional<net::RedirectInfo> ComputeRedirectInfo(
       const network::ResourceRequest& original_request,
       const network::mojom::URLResponseHead& response_head);

content/test/data/service_worker/fetch_event_invalid_redirect.js

@@ -0,0 +1,13 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+this.onfetch = function(event) {
+  // Redirects are resolved relative to the response's URL list. Synthetic
+  // responses don't have a URL list, so a relative location header should fail
+  // to redirect.
+  event.respondWith(new Response('', {
+    status: 302,
+    headers: {location: 'foo.html'},
+  }));
+};

third_party/blink/web_tests/external/wpt/service-workers/service-worker/navigation-redirect-resolution.https.html

@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<title>Service Worker: Navigation Redirect Resolution</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/test-helpers.sub.js"></script>
+<body>
+<script>
+
+function make_absolute(url) {
+  return new URL(url, location).toString();
+}
+
+const script = 'resources/fetch-rewrite-worker.js';
+
+function redirect_result_test(scope, expected_url, description) {
+  promise_test(async t => {
+    const registration = await service_worker_unregister_and_register(
+          t, script, scope);
+      t.add_cleanup(() => {
+        return service_worker_unregister(t, scope);
+      })
+      await wait_for_state(t, registration.installing, 'activated');
+
+      // The navigation to |scope| will be resolved by a fetch to |redirect_url|
+      // which returns a relative Location header. If it is resolved relative to
+      // |scope|, the result will be navigate-redirect-resolution/blank.html. If
+      // relative to |redirect_url|, it will be resources/blank.html. The latter
+      // is correct.
+      const iframe = await with_iframe(scope);
+      t.add_cleanup(() => { iframe.remove(); });
+      assert_equals(iframe.contentWindow.location.href,
+                    make_absolute(expected_url));
+  }, description);
+}
+
+// |redirect_url| serves a relative redirect to resources/blank.html.
+const redirect_url = 'resources/redirect.py?Redirect=blank.html';
+
+// |scope_base| does not exist but will be replaced with a fetch of
+// |redirect_url| by fetch-rewrite-worker.js.
+const scope_base = 'resources/subdir/navigation-redirect-resolution?' +
+    'redirect-mode=manual&url=' +
+    encodeURIComponent(make_absolute(redirect_url));
+
+// When the Service Worker forwards the result of |redirect_url| as an
+// opaqueredirect response, the redirect uses the response's URL list as the
+// base URL, not the request.
+redirect_result_test(scope_base, 'resources/blank.html',
+                     'test relative opaqueredirect');
+
+// The response's base URL should be preserved across CacheStorage and clone.
+redirect_result_test(scope_base + '&cache=1', 'resources/blank.html',
+                     'test relative opaqueredirect with CacheStorage');
+redirect_result_test(scope_base + '&clone=1', 'resources/blank.html',
+                     'test relative opaqueredirect with clone');
+
+</script>
+</body>

third_party/blink/web_tests/external/wpt/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

third_party/blink/web_tests/external/wpt/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() + "-" +

third_party/blink/web_tests/external/wpt/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') {

third_party/blink/web_tests/external/wpt/service-workers/service-worker/resources/subdir/blank.html

@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<title>Empty doc</title>