X-Chrome-Connected is stripped when it should not be in headers.

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc

Index: chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
diff --git a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
index 57388aaed81d6c3ad32f7421455e84acb653a35b..c34027a0ad8aef2402fe7445e4f1838f65d80e5b 100644
--- a/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
+++ b/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc
@@ -8,16 +8,21 @@
 
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/command_line.h"
 #include "base/macros.h"
+#include "base/path_service.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
+#include "base/test/scoped_command_line.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/policy/cloud/policy_header_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_host/chrome_navigation_data.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h"
@@ -25,18 +30,28 @@
 #include "components/policy/core/common/cloud/policy_header_io_helper.h"
 #include "components/policy/core/common/cloud/policy_header_service.h"
 #include "components/policy/core/common/policy_switches.h"
+#include "components/prefs/pref_service.h"
+#include "components/signin/core/browser/signin_header_helper.h"
+#include "components/signin/core/common/signin_pref_names.h"
+#include "components/signin/core/common/signin_switches.h"
 #include "content/public/browser/navigation_data.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/resource_dispatcher_host.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "net/http/http_request_headers.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "net/test/url_request/url_request_mock_http_job.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_filter.h"
+#include "testing/gmock/include/gmock/gmock-matchers.h"
 
 using content::ResourceType;
+using testing::HasSubstr;
+using testing::Not;
 
 namespace {
 static const char kTestPolicyHeader[] = "test_header";
@@ -251,3 +266,228 @@ IN_PROC_BROWSER_TEST_F(ChromeResourceDispatcherHostDelegateBrowserTest,
     ui_test_utils::NavigateToURL(browser(), embedded_test_server()->base_url());
   }
 }
+
+namespace {
+
+// A URLRequestMockHTTPJob to that reports HTTP request headers of outgoing
+// requests.
+class MirrorMockURLRequestJob : public net::URLRequestMockHTTPJob {
+ public:
+  // Callback function on the UI thread to report a (URL, request headers)
+  // pair. Indicating that the |request headers| will be sent for the |URL|.
+  using ReportResponseHeadersOnUI =
+      base::Callback<void(const std::string&, const std::string&)>;
+
+  MirrorMockURLRequestJob(net::URLRequest* request,
+                          net::NetworkDelegate* network_delegate,
+                          const base::FilePath& file_path,
+                          const scoped_refptr<base::TaskRunner>& task_runner,
+                          ReportResponseHeadersOnUI report_on_ui)
+      : net::URLRequestMockHTTPJob(request,
+                                   network_delegate,
+                                   file_path,
+                                   task_runner),
+        report_on_ui_(report_on_ui) {}
+
+  void Start() override {
+    // Report the observed request headers on the UI thread.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(report_on_ui_, request_->url().spec(),
+                   request_->extra_request_headers().ToString()));
+
+    URLRequestMockHTTPJob::Start();
+  }
+
+ protected:
+  const ReportResponseHeadersOnUI report_on_ui_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MirrorMockURLRequestJob);
+};

[mmenke, 2016/08/30 19:12:26]

Do the google checks care about port?  It doesn't look like they do.  If not,
then rather than create a URLRequestJob subclass for this, you can just use
embedded_test_server()->GetURL(echoheader?<header>), and check the contents of
the page using javascript.

  std::string command ="domAutomationController.send(document.body.innerText);";
  std::string body;
  EXPECT_TRUE(content::ExecuteScriptAndExtractString(
      browser->tab_strip_model()->GetActiveWebContents(), command, &body));
  return body;

Makes for a lot less boilerplate.

The embedded_test_server can also handle redirects (Which this file already
uses, higher up).

You made need to add rules to host_resolver() to map www.google.com and
www.redirect.com to 127.0.0.1.

[mmenke, 2016/08/30 19:23:39]

Actually, probably best to ignore this suggestion - we may want to restrict by
port in the future, even if we aren't now, and this current arrangement lets us
do that without modifying the tests.

[Ramin Halavati, 2016/09/01 10:41:44]

Acknowledged.

[Ramin Halavati, 2016/09/01 10:41:44]

Acknowledged.

+
+// A URLRequestInterceptor to inject MirrorMockURLRequestJobs.
+class MirrorMockJobInterceptor : public net::URLRequestInterceptor {
+ public:
+  using ReportResponseHeadersOnUI =
+      MirrorMockURLRequestJob::ReportResponseHeadersOnUI;
+
+  MirrorMockJobInterceptor(const base::FilePath& root_http,
+                           ReportResponseHeadersOnUI report_on_ui)
+      : root_http_(root_http), report_on_ui_(report_on_ui) {}
+  ~MirrorMockJobInterceptor() override = default;
+
+  // URLRequestInterceptor implementation
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new MirrorMockURLRequestJob(
+        request, network_delegate, root_http_,
+        content::BrowserThread::GetBlockingPool()
+            ->GetTaskRunnerWithShutdownBehavior(
+                base::SequencedWorkerPool::SKIP_ON_SHUTDOWN),
+        report_on_ui_);
+  }
+
+  static void Register(const GURL& url,
+                       const base::FilePath& root_http,
+                       ReportResponseHeadersOnUI report_on_ui) {
+    EXPECT_TRUE(
+        content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+    base::FilePath file_path(root_http);
+    file_path =
+        file_path.AppendASCII(url.scheme() + "." + url.host() + ".html");
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        url.scheme(), url.host(), base::WrapUnique(new MirrorMockJobInterceptor(
+                                      file_path, report_on_ui)));
+  }
+
+  static void Unregister(const GURL& url) {
+    EXPECT_TRUE(
+        content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+    net::URLRequestFilter::GetInstance()->RemoveHostnameHandler(url.scheme(),
+                                                                url.host());
+  }
+
+ private:
+  const base::FilePath root_http_;
+  ReportResponseHeadersOnUI report_on_ui_;
+
+  DISALLOW_COPY_AND_ASSIGN(MirrorMockJobInterceptor);
+};
+
+void ReportRequestHeaders(std::map<std::string, std::string>* request_headers,
+                          const std::string& url,
+                          const std::string& headers) {
+  (*request_headers)[url] = headers;
+}
+
+void EmptyClosure() {}
+
+}  // namespace
+
+class HeaderTestDispatcherHostDelegate
+    : public ChromeResourceDispatcherHostDelegate {
+ public:
+  explicit HeaderTestDispatcherHostDelegate(const GURL& watch_url)
+      : watch_url_(watch_url) {}
+  ~HeaderTestDispatcherHostDelegate() override {}
+
+  void RequestBeginning(
+      net::URLRequest* request,
+      content::ResourceContext* resource_context,
+      content::AppCacheService* appcache_service,
+      ResourceType resource_type,
+      ScopedVector<content::ResourceThrottle>* throttles) override {
+    ChromeResourceDispatcherHostDelegate::RequestBeginning(
+        request, resource_context, appcache_service, resource_type, throttles);
+    if (request->url() == watch_url_) {
+      request->SetExtraRequestHeaderByName(signin::kChromeConnectedHeader,
+                                           "User Data", false);

[mmenke, 2016/08/30 19:12:26]

Why are we appending this?  Shouldn't the RDH do that on its own?

[Ramin Halavati, 2016/09/01 10:41:44]

We added a new test in the last CL in which a website not owned by Google my add
this header, and then redirects to another, and we check that the header should
not be removed in this case. This part adds the header for this website.

+    }
+  }
+
+ private:
+  const GURL watch_url_;
+  DISALLOW_COPY_AND_ASSIGN(HeaderTestDispatcherHostDelegate);
+};
+
+// Verify the following items:
+// 1- X-Chrome-Connected is appended on Google domains if account
+//    consistency is enabled and access is secure.
+// 2- The header is stripped in case a request is redirected from a Gooogle
+//    domain to non-google domain.
+// 3- The header is NOT stripped in case it is added directly by the page
+//    and not because it was on a secure Google domain.
+// This is a regression test for crbug.com/588492.
+IN_PROC_BROWSER_TEST_F(ChromeResourceDispatcherHostDelegateBrowserTest,
+                       MirrorRequestHeader) {
+  // Enable account consistency so that mirror actually sets the
+  // X-Chrome-Connected header in requests to Google.
+  base::test::ScopedCommandLine command_line;
+  command_line.GetProcessCommandLine()->AppendSwitch(
+      switches::kEnableAccountConsistency);
+
+  browser()->profile()->GetPrefs()->SetString(prefs::kGoogleServicesUsername,
+                                              "user@gmail.com");
+  browser()->profile()->GetPrefs()->SetString(
+      prefs::kGoogleServicesUserAccountId, "account_id");
+
+  // Set URLS:
+  // The one that needs X-Chrome-Header injection:
+  GURL http_header_adder_com("http://www.header_adder.com");
+  // The ones that should have the X-Chrome-Header:
+  std::vector<GURL> urls_with_header = {
+      http_header_adder_com, GURL("https://www.google.com"),
+      GURL("http://www.redirected_from_header_adder.com")};
+  // All others:
+  std::vector<GURL> all_urls = {GURL("http://www.google.com"),
+                                GURL("https://www.redirected.com"),
+                                GURL("http://www.redirected.com")};
+  // Merge:
+  all_urls.insert(all_urls.end(), urls_with_header.begin(),
+                  urls_with_header.end());

[mmenke, 2016/08/30 19:12:26]

This seems like it would be much cleaner as single loop that navigates, waits to
hear back from other thread, checks the result and then continues.

[Ramin Halavati, 2016/09/01 10:41:44]

Done.

+
+  // Change the delegate to the one that adds the header for
+  // http_header_adder_com.
+  HeaderTestDispatcherHostDelegate dispatcher_host_delegate(
+      http_header_adder_com);
+  content::ResourceDispatcherHost::Get()->SetDelegate(
+      &dispatcher_host_delegate);
+
+  // The HTTP Request headers (i.e. the ones that are managed on the URLRequest
+  // layer, not on the URLRequestJob layer) sent from the browser are collected
+  // in this map. The keys are URLs the values the request headers.
+  std::map<std::string, std::string> request_headers;
+  MirrorMockURLRequestJob::ReportResponseHeadersOnUI report_request_headers =
+      base::Bind(&ReportRequestHeaders, &request_headers);
+
+  base::FilePath root_http;
+  PathService::Get(chrome::DIR_TEST_DATA, &root_http);
+  root_http = root_http.AppendASCII("mirror_request_header");
+
+  for (const GURL& url : all_urls) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&MirrorMockJobInterceptor::Register, url, root_http,
+                   report_request_headers));
+  }
+
+  // Perform a navigation to all none-redirected urls to observe
+  // the request headers.
+  for (const GURL& url : all_urls) {
+    if (url.host().find("redirect") == -1)
+      ui_test_utils::NavigateToURL(browser(), url);
+  }

[mmenke, 2016/08/30 19:12:26]

I'm unable to figure out how we visit these URLs.  It doesn't look to me like
the one URL we visit redirects to the other two.

[Ramin Halavati, 2016/09/01 10:41:44]

We have 3 starting URLs here and each one redirects to another one:
1) http://www.google.com --> http://www.redirected.com
2) https://www.google.com --> https://www.redirected.com
3) http:/www.header_adder.com --> http://www.redirected_from_header_adder.com

In the first case, we just check that the header is not added to any of them.
In the second case the original site should have the header, but it should not
be sent to the redirected site.
In the third case, header is added to the original site and we check that the
redirected site also receives that.

+
+  // Cleanup before verifying the observed headers.
+  for (const GURL& url : all_urls) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&MirrorMockJobInterceptor::Unregister, url));
+  }
+
+  // Ensure that the response headers have been reported to the UI thread
+  // and unregistration has been processed on the IO thread.
+  base::RunLoop run_loop;
+  content::BrowserThread::PostTaskAndReply(content::BrowserThread::IO,
+                                           FROM_HERE,
+                                           // Flush IO thread...
+                                           base::Bind(&EmptyClosure),
+                                           // ... and UI thread.
+                                           run_loop.QuitClosure());
+  run_loop.Run();
+
+  content::ResourceDispatcherHost::Get()->SetDelegate(nullptr);
+
+  // Check if header exists and X-Chrome-Connected is correctly provided.
+  for (const GURL url : all_urls) {
+    ASSERT_EQ(1u, request_headers.count(url.spec()));
+    if (base::ContainsValue(urls_with_header, url))
+      EXPECT_THAT(request_headers[url.spec()],
+                  HasSubstr(signin::kChromeConnectedHeader));
+    else
+      EXPECT_THAT(request_headers[url.spec()],
+                  Not(HasSubstr(signin::kChromeConnectedHeader)));
+  }
+}