[Reland] Re-enable WebRequestApiTest.WebRequestClientsGoogleComProtection

This is a reland of r548137 which now includes a whitelist to allow
print preview WebUI to fetch cloud print device information directly
from the network. See https://crbug.com/829218 for additional context.
Original CL description follows.

==

Re-enables this test with the Network Service enabled.


This test did some things which the Network Service does not support and
does not need to support, or which otherwise made testing with the
Network Service more complex than necessary. The following changes have
been made:

  * We no longer attempt to load a network resource from a WebUI
    renderer. This will not be supported with Network Service and is no
    longer desirable to have working in production anyway. There should
    no longer be any WebUI pages making network requests.

  * Use http instead of https for the clients1.google.com requests. This
    allows such requests to be handled by the EmbeddedTestServer which
    already exists in BrowserTestBase, which is itself not configured to
    handle HTTPS requests. Since these requests are filtered by
    hostname, the scheme doesn't matter.

  * Switch the test to use a SimpleURLLoader instead of a
    FakeURLFetcher. This allows the test code to exercise the same
    behavior with or without the Network Service enabled.

TBR=karandeepb@chromium.org

Bug: 721414
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I57bf66dfd7e7e0726ac98c9571f7fa031243e328
Reviewed-on: https://chromium-review.googlesource.com/998449Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548673}

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

@@ -45,9 +45,11 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/page_type.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/simple_url_loader_test_helper.h"
 #include "content/public/test/url_loader_interceptor.h"
 #include "extensions/browser/api/web_request/web_request_api.h"
 #include "extensions/browser/blocked_action_type.h"
@@ -72,6 +74,8 @@
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_interceptor.h"
 #include "services/network/public/cpp/features.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 
 #if defined(OS_CHROMEOS)
@@ -908,12 +912,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
 }
 
 // Verify that requests to clientsX.google.com are protected properly.
-// First test requests from a standard renderer and a webui renderer.
-// Then test a request from the browser process.
+// First test requests from a standard renderer and then a request from the
+// browser process.
 IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
                        WebRequestClientsGoogleComProtection) {
   ASSERT_TRUE(embedded_test_server()->Start());
-  int port = embedded_test_server()->port();
 
   // Load an extension that registers a listener for webRequest events, and
   // wait until it's initialized.
@@ -923,96 +926,60 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   ASSERT_TRUE(extension) << message_;
   EXPECT_TRUE(listener.WaitUntilSatisfied());
 
-  // Perform requests to https://client1.google.com from renderer processes.
-
-  struct TestCase {
-    const char* main_frame_url;
-    bool request_to_clients1_google_com_visible;
-  } testcases[] = {
-      {"http://www.example.com", true}, {"chrome://settings", false},
-  };
-
-  // Expected number of requests to clients1.google.com observed so far.
-  int expected_requests_observed = 0;
-  EXPECT_EQ(expected_requests_observed,
-            GetWebRequestCountFromBackgroundPage(extension, profile()));
-
-  for (const auto& testcase : testcases) {
-    SCOPED_TRACE(testcase.main_frame_url);
-
-    GURL url;
-    if (base::StartsWith(testcase.main_frame_url, "chrome://",
-                         base::CompareCase::INSENSITIVE_ASCII)) {
-      url = GURL(testcase.main_frame_url);
-    } else {
-      url = GURL(base::StringPrintf("%s:%d/simple.html",
-                                    testcase.main_frame_url, port));
-    }
-
-    NavigateParams params(browser(), url, ui::PAGE_TRANSITION_TYPED);
-    ui_test_utils::NavigateToURL(&params);
-
-    EXPECT_EQ(expected_requests_observed,
-              GetWebRequestCountFromBackgroundPage(extension, profile()));
-
-    content::WebContents* web_contents =
-        browser()->tab_strip_model()->GetActiveWebContents();
-    ASSERT_TRUE(web_contents);
-
-    const char kRequest[] =
-        "var xhr = new XMLHttpRequest();\n"
-        "xhr.open('GET', 'https://clients1.google.com');\n"
-        "xhr.onload = () => {window.domAutomationController.send(true);};\n"
-        "xhr.onerror = () => {window.domAutomationController.send(false);};\n"
-        "xhr.send();\n";
-
-    bool success = false;
-    EXPECT_TRUE(ExecuteScriptAndExtractBool(web_contents->GetMainFrame(),
-                                            kRequest, &success));
-    // Requests always fail due to cross origin nature.
-    EXPECT_FALSE(success);
-
-    if (testcase.request_to_clients1_google_com_visible)
-      ++expected_requests_observed;
-
-    EXPECT_EQ(expected_requests_observed,
-              GetWebRequestCountFromBackgroundPage(extension, profile()));
-  }
+  EXPECT_EQ(0, GetWebRequestCountFromBackgroundPage(extension, profile()));
 
-  // Perform request to https://client1.google.com from browser process.
+  GURL main_frame_url =
+      embedded_test_server()->GetURL("www.example.com", "/simple.html");
+  NavigateParams params(browser(), main_frame_url, ui::PAGE_TRANSITION_TYPED);
+  ui_test_utils::NavigateToURL(&params);
 
-  class TestURLFetcherDelegate : public net::URLFetcherDelegate {
-   public:
-    explicit TestURLFetcherDelegate(const base::Closure& quit_loop_func)
-        : quit_loop_func_(quit_loop_func) {}
-    ~TestURLFetcherDelegate() override {}
+  EXPECT_EQ(0, GetWebRequestCountFromBackgroundPage(extension, profile()));
 
-    void OnURLFetchComplete(const net::URLFetcher* source) override {
-      EXPECT_EQ(net::HTTP_OK, source->GetResponseCode());
-      quit_loop_func_.Run();
-    }
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
 
-   private:
-    base::Closure quit_loop_func_;
-  };
-  base::RunLoop run_loop;
-  TestURLFetcherDelegate delegate(run_loop.QuitClosure());
-
-  net::URLFetcherImplFactory url_fetcher_impl_factory;
-  net::FakeURLFetcherFactory url_fetcher_factory(&url_fetcher_impl_factory);
-  url_fetcher_factory.SetFakeResponse(GURL("https://client1.google.com"),
-                                      "hello my friend", net::HTTP_OK,
-                                      net::URLRequestStatus::SUCCESS);
-  std::unique_ptr<net::URLFetcher> fetcher =
-      url_fetcher_factory.CreateURLFetcher(
-          1, GURL("https://client1.google.com"), net::URLFetcher::GET,
-          &delegate, TRAFFIC_ANNOTATION_FOR_TESTS);
-  fetcher->Start();
-  run_loop.Run();
-
-  // This request should not be observed by the extension.
-  EXPECT_EQ(expected_requests_observed,
-            GetWebRequestCountFromBackgroundPage(extension, profile()));
+  // Attempt to issue a request to clients1.google.com from the renderer. This
+  // will fail, but should still be visible to the WebRequest API.
+  const char kRequest[] = R"(
+      var xhr = new XMLHttpRequest();
+      xhr.open('GET', 'http://clients1.google.com');
+      xhr.onload = () => {window.domAutomationController.send(true);};
+      xhr.onerror = () => {window.domAutomationController.send(false);};
+      xhr.send();)";
+  bool success = false;
+  EXPECT_TRUE(ExecuteScriptAndExtractBool(web_contents->GetMainFrame(),
+                                          kRequest, &success));
+  // Requests always fail due to cross origin nature.
+  EXPECT_FALSE(success);
+
+  EXPECT_EQ(1, GetWebRequestCountFromBackgroundPage(extension, profile()));
+
+  // Now perform a request to client1.google.com from the browser process. This
+  // should *not* be visible to the WebRequest API.
+
+  auto request = std::make_unique<network::ResourceRequest>();
+  request->url =
+      embedded_test_server()->GetURL("clients1.google.com", "/simple.html");
+
+  auto* url_loader_factory =
+      content::BrowserContext::GetDefaultStoragePartition(profile())
+          ->GetURLLoaderFactoryForBrowserProcess()
+          .get();
+  content::SimpleURLLoaderTestHelper loader_helper;
+  auto loader = network::SimpleURLLoader::Create(std::move(request),
+                                                 TRAFFIC_ANNOTATION_FOR_TESTS);
+  loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory, loader_helper.GetCallback());
+
+  // Wait for the response to complete.
+  loader_helper.WaitForCallback();
+  EXPECT_TRUE(loader_helper.response_body());
+  EXPECT_EQ(200, loader->ResponseInfo()->headers->response_code());
+
+  // We should still have only seen the single render-initiated request from the
+  // first half of the test.
+  EXPECT_EQ(1, GetWebRequestCountFromBackgroundPage(extension, profile()));
 }
 
 // Verify that requests for PAC scripts are protected properly.

chrome/test/data/extensions/api_test/webrequest_clients_google_com/background.js

@@ -6,6 +6,6 @@ window.webRequestCount = 0;
 
 chrome.webRequest.onBeforeRequest.addListener(function(details) {
   ++window.webRequestCount;
-}, {urls: ['https://clients1.google.com/']});
+}, {urls: ['http://clients1.google.com/']});
 
 chrome.test.sendMessage('ready');

extensions/browser/api/web_request/web_request_permissions.cc

@@ -78,13 +78,40 @@ PermissionsData::AccessType GetMinimumAccessType(
   return access;
 }
 
+bool IsWebUIAllowedToMakeNetworkRequests(const url::Origin& origin) {
+  // Whitelist to work around exceptional cases. This is only used to elide a
+  // DCHECK.
+  return origin.host() == "print";
+}
+
 }  // namespace
 
 // Returns true if the URL is sensitive and requests to this URL must not be
 // modified/canceled by extensions, e.g. because it is targeted to the webstore
 // to check for updates, extension blacklisting, etc.
 bool IsSensitiveURL(const GURL& url,
-                    bool is_request_from_browser_or_webui_renderer) {
+                    base::Optional<url::Origin> initiator,
+                    bool is_request_from_browser,
+                    bool is_request_from_webui_renderer) {
+  const bool is_request_from_sensitive_source =
+      is_request_from_browser || is_request_from_webui_renderer;
+
+  const bool is_network_request =
+      url.SchemeIsHTTPOrHTTPS() || url.SchemeIsWSOrWSS();
+  if (is_network_request && is_request_from_webui_renderer) {
+    // WebUI renderers should never be making network requests, but we may make
+    // some exceptions for now. See https://crbug.com/829412 for details.
+    //
+    // The DCHECK helps avoid proliferation of such behavior. In any case, we
+    // treat the requests as sensitive to ensure that the Web Request API
+    // doesn't see them.
+    DCHECK(initiator.has_value());
+    DCHECK(IsWebUIAllowedToMakeNetworkRequests(*initiator))
+        << "Unsupported network request from " << initiator->GetURL().spec()
+        << " for " << url.spec();
+    return true;
+  }
+
   // TODO(battre) Merge this, CanExtensionAccessURL and
   // PermissionsData::CanAccessPage into one function.
   bool sensitive_chrome_url = false;
@@ -103,7 +130,7 @@ bool IsSensitiveURL(const GURL& url,
     // These URLs are only protected for requests from the browser and webui
     // renderers, not for requests from common renderers, because
     // clients*.google.com are also used by websites.
-    if (is_request_from_browser_or_webui_renderer) {
+    if (is_request_from_sensitive_source) {
       base::StringPiece::size_type pos = host.rfind(kClient);
       if (pos != base::StringPiece::npos) {
         bool match = true;
@@ -132,7 +159,7 @@ bool IsSensitiveURL(const GURL& url,
                                              base::CompareCase::SENSITIVE));
   }
 
-  if (is_request_from_browser_or_webui_renderer) {
+  if (is_request_from_sensitive_source) {
     sensitive_chrome_url =
         sensitive_chrome_url ||
         extensions::ExtensionsAPIClient::Get()->ShouldHideBrowserNetworkRequest(
@@ -185,8 +212,8 @@ bool WebRequestPermissions::HideRequest(
             request.render_process_id);
   }
 
-  return IsSensitiveURL(request.url, is_request_from_browser ||
-                                         is_request_from_webui_renderer) ||
+  return IsSensitiveURL(request.url, request.initiator, is_request_from_browser,
+                        is_request_from_webui_renderer) ||
          !HasWebRequestScheme(request.url);
 }
 

extensions/browser/api/web_request/web_request_permissions.h

@@ -22,7 +22,9 @@ struct WebRequestInfo;
 
 // Exposed for unit testing.
 bool IsSensitiveURL(const GURL& url,
-                    bool is_request_from_browser_or_webui_renderer);
+                    base::Optional<url::Origin> initiator,
+                    bool is_request_from_browser,
+                    bool is_request_from_webui_renderer);
 
 // This class is used to test whether extensions may modify web requests.
 class WebRequestPermissions {

extensions/browser/api/web_request/web_request_permissions_unittest.cc

@@ -58,12 +58,16 @@ TEST(ExtensionWebRequestPermissions, IsSensitiveURL) {
     GURL url(test.url);
     EXPECT_TRUE(url.is_valid()) << test.url;
     EXPECT_EQ(test.is_sensitive_if_request_from_common_renderer,
-              IsSensitiveURL(
-                  url, false /* is_request_from_browser_or_webui_renderer */))
+              IsSensitiveURL(url, url::Origin::Create(url),
+                             false /* is_request_from_browser */,
+                             false /* is_request_from_web_ui_renderer */))
         << test.url;
-    EXPECT_EQ(test.is_sensitive_if_request_from_browser_or_webui_renderer,
-              IsSensitiveURL(
-                  url, true /* is_request_from_browser_or_webui_renderer */))
+
+    const bool supported_in_webui_renderers = !url.SchemeIsHTTPOrHTTPS();
+    EXPECT_EQ(
+        test.is_sensitive_if_request_from_browser_or_webui_renderer,
+        IsSensitiveURL(url, base::nullopt, true /* is_request_from_browser */,
+                       supported_in_webui_renderers))
         << test.url;
   }
 }

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -181,7 +181,6 @@
 # http://crbug.com/721414
 # TODO(rockot): add support for webRequest API.
 -ExtensionWebRequestApiTest.WebRequestBlocking
--ExtensionWebRequestApiTest.WebRequestClientsGoogleComProtection
 -ExtensionWebRequestApiTest.WebRequestDeclarative2
 -ExtensionWebRequestApiTest.WebRequestDiceHeaderProtection
 -ExtensionWebRequestApiTest.WebRequestTypes