Integration test for protecting clients*.google.com

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

Index: chrome/browser/extensions/api/web_request/web_request_apitest.cc
diff --git a/chrome/browser/extensions/api/web_request/web_request_apitest.cc b/chrome/browser/extensions/api/web_request/web_request_apitest.cc
index ef400b6a0e2ec76571b6887d8d6f5d70535f2775..b07b827b2081d1186f003ae6a93708212e9216bf 100644
--- a/chrome/browser/extensions/api/web_request/web_request_apitest.cc
+++ b/chrome/browser/extensions/api/web_request/web_request_apitest.cc
@@ -39,6 +39,8 @@
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/test_data_directory.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_fetcher_delegate.h"
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 
 #if defined(OS_CHROMEOS)
@@ -648,6 +650,115 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
   EXPECT_EQ(BLOCKED_ACTION_WEB_REQUEST, runner->GetBlockedActions(extension));
 }
 
+// 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.
+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 'til it's initialized.
+  ExtensionTestMessageListener listener("ready", false);
+  const Extension* extension = LoadExtension(
+      test_data_dir_.AppendASCII("webrequest_clients_google_com"));
+  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;

[Devlin, 2017/05/15 16:44:21]

Maybe check the initial value, just for completeness?

[battre (please use the other), 2017/05/15 17:39:22]

Done.

+
+  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));
+    }
+
+    chrome::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);
+    ExtensionActionRunner* runner =
+        ExtensionActionRunner::GetForWebContents(web_contents);
+    ASSERT_TRUE(runner);

[Devlin, 2017/05/15 16:44:21]

Used?

[battre (please use the other), 2017/05/15 17:39:22]

Done.

+
+    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);

[mmenke, 2017/05/11 18:39:45]

If the requests are failing due to CORS, does that mean we're actually observing
CORS requests, not the actual network requests?  If so, do we even send CORS
requests for chrome URLs?  If not, I'm very confused.

[Devlin, 2017/05/15 16:44:21]

We actually notify the extension *before* the request is sent, so I think this
is okay.  But it might be worth a comment.

[mmenke, 2017/05/15 16:47:26]

CORS blocks requests from even reaching the network stack, which is where the
WebRequest API is hooked up.  So I'm not sure that resolves my concern.

[Devlin, 2017/05/15 17:03:35]

Hmm, interesting... Why does this succeed in intercepting the request from
example.com (which is also cross-origin)?

[mmenke, 2017/05/15 17:09:51]

My guess is that it intercepts the CORS request.  I have not confirmed that.

[battre (please use the other), 2017/05/15 17:39:22]

My understanding is that Chrome sends the request but delivers it only if the
response contains an Access-Control-Allow-Origin header.

Here is what the console says:
XMLHttpRequest cannot load https://clients1.google.com/. No
'Access-Control-Allow-Origin' header is present on the requested resource.
Origin 'https://developer.mozilla.org' is therefore not allowed access. The
response had HTTP status code 404.

In this test we don't really care. What gets blocked (and counted) is whether
the GET is sent to the net.

[mmenke, 2017/05/15 18:31:16]

I was concenred that we wouldn't be making a request at all the in the settings
case, meaning this test wouldn't be checking anything.  After some
experimentation, looks like my concerns were unfounded.

+
+    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 {

[Devlin, 2017/05/15 16:44:21]

I don't know if we have a specific rule about classes with logic being defined
inline, but it's rare enough in our codebase (i.e., I've never seen it) that I
think it breaks the "match surrounding code style" rule.  Can we define this
outside the test?

[battre (please use the other), 2017/05/15 17:39:22]

I actually copied this from code in Chrome. I like this here because it keeps
the context. If you don't mind, I'd prefer keeping it inline. But you are the
owner and can choose. :-)

[Devlin, 2017/05/15 18:40:39]

Heh fair enough.  I have a TODO around here somewhere to simplify these (I've
seen other places using this type of thing), so hopefully moot soon enough
anyway.

+   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_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);
+  fetcher->Start();
+  run_loop.Run();
+
+  // This request should not be observed by the extension.
+  EXPECT_EQ(expected_requests_observed,
+            GetWebRequestCountFromBackgroundPage(extension, profile()));
+}
+
 // Test that the webRequest events are dispatched for the WebSocket handshake
 // requests.
 IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, WebSocketRequest) {