Re-enable WebRequestApiTest.WebRequestClientsGoogleComProtection

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.

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

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

@@ -84,7 +84,18 @@ PermissionsData::AccessType GetMinimumAccessType(
 // 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) {
+                    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;
+
+  // WebUI renderers should never be making network requests. We assert that
+  // here just to be sure.
+  const bool is_network_request =
+      url.SchemeIsHTTPOrHTTPS() || url.SchemeIsWSOrWSS();
+  DCHECK(!is_network_request || !is_request_from_webui_renderer)
+      << "Unsupported network request from WebUI for " << url.spec();
+
   // TODO(battre) Merge this, CanExtensionAccessURL and
   // PermissionsData::CanAccessPage into one function.
   bool sensitive_chrome_url = false;
@@ -103,7 +114,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 +143,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 +196,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, is_request_from_browser,
+                        is_request_from_webui_renderer) ||
          !HasWebRequestScheme(request.url);
 }
 

extensions/browser/api/web_request/web_request_permissions.h

@@ -22,7 +22,8 @@ struct WebRequestInfo;
 
 // Exposed for unit testing.
 bool IsSensitiveURL(const GURL& url,
-                    bool is_request_from_browser_or_webui_renderer);
+                    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,14 @@ 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, false /* is_request_from_browser */,
+                             false /* is_request_from_web_ui_renderer */))
         << test.url;
+
+    const bool supported_in_webui_renderers = !url.SchemeIsHTTPOrHTTPS();
     EXPECT_EQ(test.is_sensitive_if_request_from_browser_or_webui_renderer,
-              IsSensitiveURL(
-                  url, true /* is_request_from_browser_or_webui_renderer */))
+              IsSensitiveURL(url, true /* is_request_from_browser */,
+                             supported_in_webui_renderers))
         << test.url;
   }
 }

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

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