Make WebRequestProxyingURLLoaderFactory behavior match HTTP redirects.

In particular, have it invoke net::RedirectInfo::ComputeRedirectInfo()
when an extension injects a redirect so that site-for-cookies, referrer,
method, and referrer_policy are all calculated correctly, and have
it update NetworkIsolationKey according to the value in
request_.trusted_params->update_network_isolation_key_on_redirect.

Change-Id: Ied1e0d9825b75110143881d6aac806da1f13baab
Fixed: 1053573
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2057529
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746149}

chrome/browser/extensions/api/web_request/web_request_apitest.cc

@@ -2,8 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <map>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -12,11 +14,13 @@
 #include "base/memory/ptr_util.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
+#include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/lock.h"
 #include "base/task/post_task.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/thread_annotations.h"
 #include "base/time/time.h"
 #include "base/time/time_override.h"
 #include "base/values.h"
@@ -94,6 +98,8 @@
 #include "google_apis/gaia/gaia_switches.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "net/base/network_isolation_key.h"
+#include "net/cookies/site_for_cookies.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/http/http_util.h"
 #include "net/test/embedded_test_server/default_handlers.h"
@@ -112,6 +118,7 @@
 #include "services/network/test/test_url_loader_client.h"
 #include "third_party/blink/public/common/input/web_input_event.h"
 #include "ui/base/ui_base_features.h"
+#include "url/origin.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -279,6 +286,62 @@ bool HasSeenWebRequestInBackgroundPage(const Extension* extension,
   return seen;
 }
 
+// Class that monitors ResourceRequests sent to the network service during its
+// lifetime, and allows the caller to access information about them.
+class URLLoaderMonitor {
+ public:
+  struct RequestInfo {
+    net::SiteForCookies site_for_cookies;
+    base::Optional<net::NetworkIsolationKey> network_isolation_key;
+  };
+
+  URLLoaderMonitor()
+      : interceptor_(std::make_unique<content::URLLoaderInterceptor>(
+            base::BindRepeating(&URLLoaderMonitor::OnRequest,
+                                base::Unretained(this)))) {}
+
+  URLLoaderMonitor(const URLLoaderMonitor&) = delete;
+  URLLoaderMonitor& operator=(const URLLoaderMonitor&) = delete;
+
+  ~URLLoaderMonitor() {
+    // This is needed because |interceptor_| is a cross-thread object that may
+    // invoke the callback passed to it on the IO thread at any time until it's
+    // destroyed. Therefore, it must be destroyed before |this| is.
+    interceptor_.reset();
+  }
+
+  base::Optional<RequestInfo> GetRequestInfo(const GURL& url) {
+    base::AutoLock autolock(lock_);
+    const auto request_info = request_info_map_.find(url);
+    if (request_info == request_info_map_.end())
+      return base::nullopt;
+    return request_info->second;
+  }
+
+ private:
+  bool OnRequest(content::URLLoaderInterceptor::RequestParams* params) {
+    RequestInfo request_info;
+    request_info.site_for_cookies = params->url_request.site_for_cookies;
+    if (params->url_request.trusted_params) {
+      request_info.network_isolation_key =
+          params->url_request.trusted_params->network_isolation_key;
+    }
+
+    base::AutoLock autolock(lock_);
+    request_info_map_[params->url_request.url] = request_info;
+
+    // Don't override default handling of the request.
+    return false;
+  }
+
+  // This is needed to guard access to |request_info_map_|, as
+  // content::URLLoaderInterceptor can invoke its callback on the IO thread.
+  base::Lock lock_;
+  std::map<GURL, RequestInfo> GUARDED_BY(lock_) request_info_map_;
+
+  std::unique_ptr<content::URLLoaderInterceptor> interceptor_;
+};
+
 }  // namespace
 
 class ExtensionWebRequestApiTest : public ExtensionApiTest {
@@ -3191,4 +3254,136 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, HSTSUpgradeAfterRedirect) {
   EXPECT_EQ(final_url, web_contents->GetLastCommittedURL());
 }
 
+enum class RedirectType {
+  kOnBeforeRequest,
+  kOnHeadersReceived,
+};
+
+class RedirectInfoWebRequestApiTest
+    : public testing::WithParamInterface<RedirectType>,
+      public ExtensionApiTest {
+ public:
+  void SetUpOnMainThread() override {
+    ExtensionApiTest::SetUpOnMainThread();
+    host_resolver()->AddRule("*", "127.0.0.1");
+    embedded_test_server()->ServeFilesFromSourceDirectory("content/test/data");
+    ASSERT_TRUE(StartEmbeddedTestServer());
+  }
+
+  void InstallRequestRedirectingExtension(const std::string& resource_type) {
+    TestExtensionDir test_dir;
+    test_dir.WriteManifest(R"({
+          "name": "Simple Redirect",
+          "manifest_version": 2,
+          "version": "0.1",
+          "background": { "scripts": ["background.js"] },
+          "permissions": ["<all_urls>", "webRequest", "webRequestBlocking"]
+        })");
+    test_dir.WriteFile(
+        FILE_PATH_LITERAL("background.js"),
+        base::StringPrintf(R"(
+          chrome.webRequest.%s.addListener(function(details) {
+            if (details.type == '%s' &&
+                details.url.includes('hello.html')) {
+              var redirectUrl =
+                  details.url.replace('original.test', 'redirected.test');
+              return {redirectUrl: redirectUrl};
+            }
+          }, {urls: ['*://original.test/*']}, ['blocking']);
+          chrome.test.sendMessage('ready');
+        )",
+                           GetParam() == RedirectType::kOnBeforeRequest
+                               ? "onBeforeRequest"
+                               : "onHeadersReceived",
+                           resource_type.c_str()));
+    ExtensionTestMessageListener listener("ready", false);
+    const Extension* extension = LoadExtension(test_dir.UnpackedPath());
+    ASSERT_TRUE(extension);
+    EXPECT_TRUE(listener.WaitUntilSatisfied());
+  }
+
+ private:
+  TestExtensionDir test_dir_;
+};
+
+INSTANTIATE_TEST_SUITE_P(RedirectMode,
+                         RedirectInfoWebRequestApiTest,
+                         ::testing::Values(RedirectType::kOnBeforeRequest,
+                                           RedirectType::kOnHeadersReceived));
+
+// Test that a main frame request redirected by an extension has the correct
+// site_for_cookies and network_isolation_key parameters.
+IN_PROC_BROWSER_TEST_P(RedirectInfoWebRequestApiTest,
+                       VerifyRedirectInfoMainFrame) {
+  InstallRequestRedirectingExtension("main_frame");
+
+  URLLoaderMonitor monitor;
+
+  // Navigate to the URL that should be redirected, and check that the extension
+  // redirects it.
+  GURL url = embedded_test_server()->GetURL("original.test", "/hello.html");
+  EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), url));
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+  GURL redirected_url =
+      embedded_test_server()->GetURL("redirected.test", "/hello.html");
+  EXPECT_EQ(redirected_url, web_contents->GetLastCommittedURL());
+
+  // Check the parameters passed to the URLLoaderFactory.
+  base::Optional<URLLoaderMonitor::RequestInfo> request_info =
+      monitor.GetRequestInfo(redirected_url);
+  ASSERT_TRUE(request_info.has_value());
+  EXPECT_TRUE(request_info->site_for_cookies.IsFirstParty(redirected_url));
+  url::Origin redirected_origin = url::Origin::Create(redirected_url);
+  EXPECT_EQ(request_info->network_isolation_key,
+            net::NetworkIsolationKey(redirected_origin, redirected_origin));
+}
+
+// Test that a sub frame request redirected by an extension has the correct
+// site_for_cookies and network_isolation_key parameters.
+IN_PROC_BROWSER_TEST_P(RedirectInfoWebRequestApiTest,
+                       VerifyBeforeRequestRedirectInfoSubFrame) {
+  InstallRequestRedirectingExtension("sub_frame");
+
+  URLLoaderMonitor monitor;
+
+  // Navigate to page with an iframe that should be redirected, and check that
+  // the extension redirects it.
+  GURL original_iframed_url =
+      embedded_test_server()->GetURL("original.test", "/hello.html");
+  GURL page_with_iframe_url = embedded_test_server()->GetURL(
+      "somewhere-else.test",
+      net::test_server::GetFilePathWithReplacements(
+          "/page_with_iframe.html",
+          base::StringPairs{
+              {"title1.html", original_iframed_url.spec().c_str()}}));
+  EXPECT_TRUE(ui_test_utils::NavigateToURL(browser(), page_with_iframe_url));
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+  EXPECT_EQ(page_with_iframe_url, web_contents->GetLastCommittedURL());
+
+  // Since frames are returned in breadth first order, and there's only one
+  // iframe, the iframe should be the second frame in this vector.
+  std::vector<content::RenderFrameHost*> all_frames =
+      web_contents->GetAllFrames();
+  ASSERT_EQ(2u, all_frames.size());
+  GURL redirected_url =
+      embedded_test_server()->GetURL("redirected.test", "/hello.html");
+  ASSERT_EQ(redirected_url, all_frames[1]->GetLastCommittedURL());
+
+  // Check the parameters passed to the URLLoaderFactory.
+  base::Optional<URLLoaderMonitor::RequestInfo> request_info =
+      monitor.GetRequestInfo(redirected_url);
+  ASSERT_TRUE(request_info.has_value());
+  EXPECT_TRUE(
+      request_info->site_for_cookies.IsFirstParty(page_with_iframe_url));
+  EXPECT_FALSE(request_info->site_for_cookies.IsFirstParty(redirected_url));
+  url::Origin top_level_origin = url::Origin::Create(page_with_iframe_url);
+  url::Origin redirected_origin = url::Origin::Create(redirected_url);
+  EXPECT_EQ(request_info->network_isolation_key,
+            net::NetworkIsolationKey(top_level_origin, redirected_origin));
+}
+
 }  // namespace extensions

extensions/browser/DEPS

@@ -23,6 +23,10 @@ include_rules = [
   "+grit/extensions_strings.h",
   "+net",
   "-net/url_request",
+  "+net/url_request/redirect_info.h",
+  "+net/url_request/redirect_util.h",
+  # Needed for a flag.
+  "+net/url_request/url_request.h",
   # This directory contains build flags and does not pull all of PPAPI in.
   "+ppapi/buildflags",
   "+services/data_decoder/public",

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

@@ -24,6 +24,9 @@
 #include "extensions/common/manifest_handlers/web_accessible_resources_info.h"
 #include "net/base/completion_repeating_callback.h"
 #include "net/http/http_util.h"
+#include "net/url_request/redirect_info.h"
+#include "net/url_request/redirect_util.h"
+#include "net/url_request/url_request.h"
 #include "services/network/public/cpp/features.h"
 #include "third_party/blink/public/common/loader/throttling_url_loader.h"
 #include "third_party/blink/public/platform/resource_request_blocked_reason.h"
@@ -56,6 +59,25 @@ class ShutdownNotifierFactory
   DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
 };
 
+// Creates simulated net::RedirectInfo when an extension redirects a request,
+// behaving like a redirect response was actually returned by the remote server.
+net::RedirectInfo CreateRedirectInfo(
+    const network::ResourceRequest& original_request,
+    const GURL& new_url,
+    int response_code,
+    const base::Optional<std::string>& referrer_policy_header) {
+  return net::RedirectInfo::ComputeRedirectInfo(
+      original_request.method, original_request.url,
+      original_request.site_for_cookies,
+      original_request.update_first_party_url_on_redirect
+          ? net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT
+          : net::URLRequest::NEVER_CHANGE_FIRST_PARTY_URL,
+      original_request.referrer_policy, original_request.referrer.spec(),
+      response_code, new_url, referrer_policy_header,
+      false /* insecure_scheme_was_upgraded */, false /* copy_fragment */,
+      false /* is_signed_exchange_fallback_redirect */);
+}
+
 }  // namespace
 
 WebRequestProxyingURLLoaderFactory::InProgressRequest::FollowRedirectParams::
@@ -464,12 +486,9 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::
 
   constexpr int kInternalRedirectStatusCode = 307;
 
-  net::RedirectInfo redirect_info;
-  redirect_info.status_code = kInternalRedirectStatusCode;
-  redirect_info.new_method = request_.method;
-  redirect_info.new_url = redirect_url_;
-  redirect_info.new_site_for_cookies =
-      net::SiteForCookies::FromUrl(redirect_url_);
+  net::RedirectInfo redirect_info =
+      CreateRedirectInfo(request_, redirect_url_, kInternalRedirectStatusCode,
+                         base::nullopt /* referrer_policy_header */);
 
   auto head = network::mojom::URLResponseHead::New();
   std::string headers = base::StringPrintf(
@@ -814,11 +833,9 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::
     // request to the Network Service. Our client shouldn't know the difference.
     GURL new_url(redirect_location);
 
-    net::RedirectInfo redirect_info;
-    redirect_info.status_code = override_headers_->response_code();
-    redirect_info.new_method = request_.method;
-    redirect_info.new_url = new_url;
-    redirect_info.new_site_for_cookies = net::SiteForCookies::FromUrl(new_url);
+    net::RedirectInfo redirect_info = CreateRedirectInfo(
+        request_, new_url, override_headers_->response_code(),
+        net::RedirectUtil::GetReferrerPolicyHeader(override_headers_.get()));
 
     // These will get re-bound if a new request is initiated by
     // |FollowRedirect()|.
@@ -860,6 +877,24 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::
   request_.site_for_cookies = redirect_info.new_site_for_cookies;
   request_.referrer = GURL(redirect_info.new_referrer);
   request_.referrer_policy = redirect_info.new_referrer_policy;
+  if (request_.trusted_params) {
+    url::Origin new_origin = url::Origin::Create(redirect_info.new_url);
+    switch (request_.trusted_params->update_network_isolation_key_on_redirect) {
+      case network::mojom::UpdateNetworkIsolationKeyOnRedirect::
+          kUpdateTopFrameAndFrameOrigin:
+        request_.trusted_params->network_isolation_key =
+            net::NetworkIsolationKey(new_origin, new_origin);
+        break;
+      case network::mojom::UpdateNetworkIsolationKeyOnRedirect::
+          kUpdateFrameOrigin:
+        request_.trusted_params->network_isolation_key =
+            request_.trusted_params->network_isolation_key
+                .CreateWithNewFrameOrigin(new_origin);
+        break;
+      case network::mojom::UpdateNetworkIsolationKeyOnRedirect::kDoNotUpdate:
+        break;
+    }
+  }
 
   // The request method can be changed to "GET". In this case we need to
   // reset the request body manually.