Prevent proxying browser-initiated requests with webRequest in NS

After crrev.com/c/1242296, we no longer need to proxy browser initiated
requests with webRequest. This cleans up the proxy for the
URLLoaderFactory used for browser initiated requests.

Bug: 888672
Change-Id: I96c55fa5849285ae8f86c4b6d7c24d47c8ac4c19
Reviewed-on: https://chromium-review.googlesource.com/c/1254725
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596324}

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

@@ -1594,23 +1594,27 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   // Create a profile that will be destroyed later.
   base::ScopedAllowBlockingForTesting allow_blocking;
   ProfileManager* profile_manager = g_browser_process->profile_manager();
-  Profile* tmp_profile = Profile::CreateProfile(
+  Profile* temp_profile = Profile::CreateProfile(
       profile_manager->user_data_dir().AppendASCII("profile"), nullptr,
       Profile::CreateMode::CREATE_MODE_SYNCHRONOUS);
 
   // Create a WebRequestAPI instance that we can control the lifetime of.
-  auto api = std::make_unique<WebRequestAPI>(tmp_profile);
-  // Make sure we are proxying for |tmp_profile|.
+  auto api = std::make_unique<WebRequestAPI>(temp_profile);
+  // Make sure we are proxying for |temp_profile|.
   api->ForceProxyForTesting();
-  content::BrowserContext::GetDefaultStoragePartition(tmp_profile)
+  content::BrowserContext::GetDefaultStoragePartition(temp_profile)
       ->FlushNetworkInterfaceForTesting();
 
   network::mojom::URLLoaderFactoryPtr factory;
   auto request = mojo::MakeRequest(&factory);
-  EXPECT_TRUE(api->MaybeProxyURLLoaderFactory(nullptr, false, &request));
+  auto temp_web_contents =
+      WebContents::Create(WebContents::CreateParams(temp_profile));
+  EXPECT_TRUE(api->MaybeProxyURLLoaderFactory(temp_web_contents->GetMainFrame(),
+                                              false, &request));
+  temp_web_contents.reset();
   auto params = network::mojom::URLLoaderFactoryParams::New();
   params->process_id = 0;
-  content::BrowserContext::GetDefaultStoragePartition(tmp_profile)
+  content::BrowserContext::GetDefaultStoragePartition(temp_profile)
       ->GetNetworkContext()
       ->CreateURLLoaderFactory(std::move(request), std::move(params));
 
@@ -1627,7 +1631,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   // WebRequestAPI. This will cause the connection error that will reach the
   // proxy before the ProxySet shutdown code runs on the IO thread.
   api->Shutdown();
-  ProfileDestroyer::DestroyProfileWhenAppropriate(tmp_profile);
+  ProfileDestroyer::DestroyProfileWhenAppropriate(temp_profile);
   client.Unbind();
   api.reset();
 }

chrome/browser/signin/chrome_signin_proxying_url_loader_factory.cc

@@ -452,10 +452,8 @@ bool ProxyingURLLoaderFactory::MaybeProxyRequest(
 
   // This proxy should only be installed for subresource requests from a frame
   // that is rendering the GAIA signon realm.
-  if (!render_frame_host ||
-      !gaia::IsGaiaSignonRealm(request_initiator.GetURL())) {
+  if (!gaia::IsGaiaSignonRealm(request_initiator.GetURL()))
     return false;
-  }
 
   auto* web_contents =
       content::WebContents::FromRenderFrameHost(render_frame_host);

content/browser/storage_partition_impl.cc

@@ -1350,12 +1350,6 @@ StoragePartitionImpl::GetURLLoaderFactoryForBrowserProcessInternal() {
           switches::kDisableWebSecurity);
   if (g_url_loader_factory_callback_for_test.Get().is_null()) {
     auto request = mojo::MakeRequest(&url_loader_factory_for_browser_process_);
-
-    if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
-      GetContentClient()->browser()->WillCreateURLLoaderFactory(
-          browser_context(), nullptr, false /* is_navigation */, url::Origin(),
-          &request, nullptr /* bypass_redirect_checks */);
-    }
     GetNetworkContext()->CreateURLLoaderFactory(std::move(request),
                                                 std::move(params));
     is_test_url_loader_factory_for_browser_process_ = false;

content/public/browser/content_browser_client.h

@@ -1114,9 +1114,7 @@ class CONTENT_EXPORT ContentBrowserClient {
       NonNetworkURLLoaderFactoryMap* factories);
 
   // Allows the embedder to intercept URLLoaderFactory interfaces used for
-  // navigation or being brokered on behalf of a renderer fetching subresources,
-  // or for non-navigation requests initiated by the browser on behalf of a
-  // BrowserContext.
+  // navigation or being brokered on behalf of a renderer fetching subresources.
   //
   // |is_navigation| is true when it's a request used for navigation.
   //
@@ -1126,9 +1124,7 @@ class CONTENT_EXPORT ContentBrowserClient {
   // it's a request for a renderer fetching subresources. It's not set when
   // creating a factory for navigation requests, because navigation requests are
   // made on behalf of the browser, rather than on behalf of any particular
-  // origin. It's not set in the case of browser-initiated, non-navigation
-  // requests, because in that case the factory is cached and it can be used for
-  // multiple URLs.
+  // origin.
   //
   // |*factory_request| is always valid upon entry and MUST be valid upon
   // return. The embedder may swap out the value of |*factory_request| for its
@@ -1141,10 +1137,6 @@ class CONTENT_EXPORT ContentBrowserClient {
   //
   // Always called on the UI thread and only when the Network Service is
   // enabled.
-  //
-  // Note that |frame| may be null if this is a browser-initiated,
-  // non-navigation request, e.g. a request made via
-  // |StoragePartition::GetURLLoaderFactoryForBrowserProcess()|.
   virtual bool WillCreateURLLoaderFactory(
       BrowserContext* browser_context,
       RenderFrameHost* frame,

extensions/browser/api/web_request/web_request_api.cc

@@ -600,20 +600,18 @@ bool WebRequestAPI::MaybeProxyURLLoaderFactory(
     bool skip_proxy = true;
     // There are a few internal WebUIs that use WebView tag that are whitelisted
     // for webRequest.
-    if (frame) {
-      auto* web_contents = content::WebContents::FromRenderFrameHost(frame);
-      if (web_contents && WebViewGuest::IsGuest(web_contents)) {
-        auto* guest_web_contents =
-            WebViewGuest::GetTopLevelWebContents(web_contents);
-        auto& guest_url = guest_web_contents->GetURL();
-        if (guest_url.SchemeIs(content::kChromeUIScheme)) {
-          auto* feature = FeatureProvider::GetAPIFeature("webRequestInternal");
-          if (feature
-                  ->IsAvailableToContext(nullptr, Feature::WEBUI_CONTEXT,
-                                         guest_url)
-                  .is_available()) {
-            skip_proxy = false;
-          }
+    auto* web_contents = content::WebContents::FromRenderFrameHost(frame);
+    if (web_contents && WebViewGuest::IsGuest(web_contents)) {
+      auto* guest_web_contents =
+          WebViewGuest::GetTopLevelWebContents(web_contents);
+      auto& guest_url = guest_web_contents->GetURL();
+      if (guest_url.SchemeIs(content::kChromeUIScheme)) {
+        auto* feature = FeatureProvider::GetAPIFeature("webRequestInternal");
+        if (feature
+                ->IsAvailableToContext(nullptr, Feature::WEBUI_CONTEXT,
+                                       guest_url)
+                .is_available()) {
+          skip_proxy = false;
         }
       }
     }
@@ -639,25 +637,22 @@ bool WebRequestAPI::MaybeProxyURLLoaderFactory(
   // NOTE: This request may be proxied on behalf of an incognito frame, but
   // |this| will always be bound to a regular profile (see
   // |BrowserContextKeyedAPI::kServiceRedirectedInIncognito|). As such, we use
-  // the frame's BrowserContext when |frame| is non-null.
-  auto* browser_context =
-      frame ? frame->GetProcess()->GetBrowserContext() : browser_context_;
+  // the frame's BrowserContext.
+  auto* browser_context = frame->GetProcess()->GetBrowserContext();
   DCHECK(browser_context == browser_context_ ||
          (browser_context->IsOffTheRecord() &&
           ExtensionsBrowserClient::Get()->GetOriginalContext(browser_context) ==
               browser_context_));
-  const bool is_for_browser_initiated_requests = is_navigation || !frame;
   base::PostTaskWithTraits(
       FROM_HERE, {BrowserThread::IO},
-      base::BindOnce(
-          &WebRequestProxyingURLLoaderFactory::StartProxying, browser_context,
-          browser_context->GetResourceContext(),
-          // Match the behavior of the WebRequestInfo constructor
-          // which takes a net::URLRequest*.
-          is_for_browser_initiated_requests ? -1 : frame->GetProcess()->GetID(),
-          request_id_generator_, std::move(navigation_ui_data),
-          base::Unretained(info_map_), std::move(proxied_request),
-          std::move(target_factory_info)));
+      base::BindOnce(&WebRequestProxyingURLLoaderFactory::StartProxying,
+                     browser_context, browser_context->GetResourceContext(),
+                     // Match the behavior of the WebRequestInfo constructor
+                     // which takes a net::URLRequest*.
+                     is_navigation ? -1 : frame->GetProcess()->GetID(),
+                     request_id_generator_, std::move(navigation_ui_data),
+                     base::Unretained(info_map_), std::move(proxied_request),
+                     std::move(target_factory_info)));
   return true;
 }
 

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.cc

@@ -25,7 +25,6 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
     int32_t network_service_request_id,
     int32_t routing_id,
     uint32_t options,
-    bool is_non_navigation_browser_request,
     const network::ResourceRequest& request,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
     network::mojom::URLLoaderRequest loader_request,
@@ -36,7 +35,6 @@ WebRequestProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
       network_service_request_id_(network_service_request_id),
       routing_id_(routing_id),
       options_(options),
-      is_non_navigation_browser_request_(is_non_navigation_browser_request),
       traffic_annotation_(traffic_annotation),
       proxied_loader_binding_(this, std::move(loader_request)),
       target_client_(std::move(client)),
@@ -69,22 +67,6 @@ void WebRequestProxyingURLLoaderFactory::InProgressRequest::Restart() {
       routing_id_, factory_->resource_context_, request_,
       !(options_ & network::mojom::kURLLoadOptionSynchronous));
 
-  if (is_non_navigation_browser_request_) {
-    // ResourceRequest always has a valid-looking ResourceType value since it's
-    // non-optional and defaults to 0 (i.e. MAIN_FRAME), even of the
-    // corresponding request didn't actually come from a renderer. Because
-    // |info_| was blindly constructed from that ResourceRequest, it also now
-    // appears to pertain to a main-frame request.
-    //
-    // Because we already know this is a browser-originated request, we
-    // explicitly reset |info_->type| to null. A request having no ResourceType
-    // effectively implies a browser-originated request to any subsequent
-    // WebRequest logic that cares, e.g. some permission checking to determine
-    // when to filter certain kinds of requests.
-    info_->type.reset();
-    info_->web_request_type = WebRequestResourceType::OTHER;
-  }
-
   auto continuation =
       base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders,
                           weak_factory_.GetWeakPtr());
@@ -617,6 +599,9 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart(
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
+  // Make sure we are not proxying a browser initiated non-navigation request.
+  DCHECK(render_process_id_ != -1 || navigation_ui_data_);
+
   // The request ID doesn't really matter in the Network Service path. It just
   // needs to be unique per-BrowserContext so extensions can make sense of it.
   // Note that |network_service_request_id_| by contrast is not necessarily
@@ -633,18 +618,11 @@ void WebRequestProxyingURLLoaderFactory::CreateLoaderAndStart(
     network_request_id_to_web_request_id_.emplace(request_id, web_request_id);
   }
 
-  // The WebRequest API treats browser-originated non-navigation requests with a
-  // few additional restrictions, so we deduce and propagate that information
-  // here.
-  const bool is_non_navigation_browser_request =
-      render_process_id_ == -1 && !navigation_ui_data_;
-
   auto result = requests_.emplace(
       web_request_id,
       std::make_unique<InProgressRequest>(
-          this, web_request_id, request_id, routing_id, options,
-          is_non_navigation_browser_request, request, traffic_annotation,
-          std::move(loader_request), std::move(client)));
+          this, web_request_id, request_id, routing_id, options, request,
+          traffic_annotation, std::move(loader_request), std::move(client)));
   result.first->second->Restart();
 }
 

extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h

@@ -55,7 +55,6 @@ class WebRequestProxyingURLLoaderFactory
         int32_t routing_id,
         int32_t network_service_request_id,
         uint32_t options,
-        bool is_non_navigation_browser_request,
         const network::ResourceRequest& request,
         const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
         network::mojom::URLLoaderRequest loader_request,
@@ -116,7 +115,6 @@ class WebRequestProxyingURLLoaderFactory
     const int32_t network_service_request_id_;
     const int32_t routing_id_;
     const uint32_t options_;
-    const bool is_non_navigation_browser_request_;
     const net::MutableNetworkTrafficAnnotationTag traffic_annotation_;
     mojo::Binding<network::mojom::URLLoader> proxied_loader_binding_;
     network::mojom::URLLoaderClientPtr target_client_;