Revert "Re-enable WebRequestApiTest.WebRequestClientsGoogleComProtection"

This reverts commit 7c2240a7.

Reason for revert: https://crbug.com/829218

Original change's description:
> 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}

TBR=jam@chromium.org,rockot@chromium.org,karandeepb@chromium.org

Change-Id: I054a7e11a4945e379cd45405546a90ee4d1e9577
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 721414
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/998076Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548440}

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

@@ -45,11 +45,9 @@
 #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"
@@ -74,8 +72,6 @@
 #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)
@@ -912,11 +908,12 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
 }
 
 // Verify that requests to clientsX.google.com are protected properly.
-// First test requests from a standard renderer and then a request from the
-// browser process.
+// First test requests from a standard renderer and a webui renderer.
+// Then test 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.
@@ -926,60 +923,96 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   ASSERT_TRUE(extension) << message_;
   EXPECT_TRUE(listener.WaitUntilSatisfied());
 
-  EXPECT_EQ(0, GetWebRequestCountFromBackgroundPage(extension, profile()));
+  // Perform requests to https://client1.google.com from renderer processes.
 
-  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);
+  struct TestCase {
+    const char* main_frame_url;
+    bool request_to_clients1_google_com_visible;
+  } testcases[] = {
+      {"http://www.example.com", true}, {"chrome://settings", false},
+  };
 
-  EXPECT_EQ(0, GetWebRequestCountFromBackgroundPage(extension, profile()));
+  // Expected number of requests to clients1.google.com observed so far.
+  int expected_requests_observed = 0;
+  EXPECT_EQ(expected_requests_observed,
+            GetWebRequestCountFromBackgroundPage(extension, profile()));
 
-  content::WebContents* web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  ASSERT_TRUE(web_contents);
+  for (const auto& testcase : testcases) {
+    SCOPED_TRACE(testcase.main_frame_url);
 
-  // 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()));
+    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()));
+  }
+
+  // Perform request to https://client1.google.com from browser process.
+
+  class TestURLFetcherDelegate : public net::URLFetcherDelegate {
+   public:
+    explicit TestURLFetcherDelegate(const base::Closure& quit_loop_func)
+        : quit_loop_func_(quit_loop_func) {}
+    ~TestURLFetcherDelegate() override {}
+
+    void OnURLFetchComplete(const net::URLFetcher* source) override {
+      EXPECT_EQ(net::HTTP_OK, source->GetResponseCode());
+      quit_loop_func_.Run();
+    }
+
+   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()));
 }
 
 // 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: ['http://clients1.google.com/']});
+}, {urls: ['https://clients1.google.com/']});
 
 chrome.test.sendMessage('ready');

extensions/browser/api/web_request/web_request_permissions.cc

@@ -84,18 +84,7 @@ 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,
-                    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();
-
+                    bool is_request_from_browser_or_webui_renderer) {
   // TODO(battre) Merge this, CanExtensionAccessURL and
   // PermissionsData::CanAccessPage into one function.
   bool sensitive_chrome_url = false;
@@ -114,7 +103,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_sensitive_source) {
+    if (is_request_from_browser_or_webui_renderer) {
       base::StringPiece::size_type pos = host.rfind(kClient);
       if (pos != base::StringPiece::npos) {
         bool match = true;
@@ -143,7 +132,7 @@ bool IsSensitiveURL(const GURL& url,
                                              base::CompareCase::SENSITIVE));
   }
 
-  if (is_request_from_sensitive_source) {
+  if (is_request_from_browser_or_webui_renderer) {
     sensitive_chrome_url =
         sensitive_chrome_url ||
         extensions::ExtensionsAPIClient::Get()->ShouldHideBrowserNetworkRequest(
@@ -196,8 +185,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,8 +22,7 @@ struct WebRequestInfo;
 
 // Exposed for unit testing.
 bool IsSensitiveURL(const GURL& url,
-                    bool is_request_from_browser,
-                    bool is_request_from_webui_renderer);
+                    bool is_request_from_browser_or_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,14 +58,12 @@ 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 */,
-                             false /* is_request_from_web_ui_renderer */))
+              IsSensitiveURL(
+                  url, false /* is_request_from_browser_or_webui_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 */,
-                             supported_in_webui_renderers))
+              IsSensitiveURL(
+                  url, true /* is_request_from_browser_or_webui_renderer */))
         << test.url;
   }
 }

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

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