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

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h"
 
 #include <stddef.h>
 
 #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"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #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";
 static const char kServerRedirectUrl[] = "/server-redirect";
 
 std::unique_ptr<net::test_server::HttpResponse> HandleTestRequest(
     const net::test_server::HttpRequest& request) {
   if (base::StartsWith(request.relative_url, kServerRedirectUrl,
                        base::CompareCase::SENSITIVE)) {
     // Extract the target URL and redirect there.
@@ skipping 194 matching lines @@
     ui_test_utils::NavigateToURL(
         browser(), embedded_test_server()->GetURL("/google/google.html"));
   }
   SetShouldAddDataReductionProxyData(true);
   {
     DidFinishNavigationObserver nav_observer(
         browser()->tab_strip_model()->GetActiveWebContents(), true);
     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);
+};
+
+// 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,

[mmenke, 2016/09/01 18:31:48]

include map

[mmenke, 2016/09/01 18:31:48]

Should document this.

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

+                          const std::string& url,
+                          const std::string& headers) {

[mmenke, 2016/09/01 18:31:48]

EXPECT_TRUE(request_header.find(url) == std::map::npos);?

[mmenke, 2016/09/01 18:31:48]

DCHECK_CURRENTLY_ON(BrowserThread::UI); (Or whatever the syntax is)

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

+  (*request_headers)[url] = headers;
+}
+
+void EmptyClosure() {}
+
+}  // namespace
+
+class HeaderTestDispatcherHostDelegate

[mmenke, 2016/09/01 18:31:48]

Should document this.

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

+    : 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);
+    }
+  }
+
+ private:
+  const GURL watch_url_;
+  DISALLOW_COPY_AND_ASSIGN(HeaderTestDispatcherHostDelegate);

[mmenke, 2016/09/01 18:31:48]

nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

+};
+
+// 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");
+
+  // 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);

[mmenke, 2016/09/01 18:31:48]

Move this stuff into the loop, so it's cleared between loop iterations?

[Ramin Halavati, 2016/09/02 13:41:44]

Done.

+
+  base::FilePath root_http;
+  PathService::Get(chrome::DIR_TEST_DATA, &root_http);
+  root_http = root_http.AppendASCII("mirror_request_header");
+
+  struct TestCase {
+    GURL urls[2];

[mmenke, 2016/09/01 18:31:48]

Think you should name these two URLs - you have to double-up the checking code
at the end, but think it's worth the clarity.

something like "original_url" and "redirected_to_url"

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

+    bool inject_header;
+    bool expect_header[2];
+  } all_tests[] = {
+      // Neither should have the header.
+      {{GURL("http://www.google.com"), GURL("http://www.redirected.com")},
+       false,
+       {false, false}},
+      // First one should have the header, but not transfered to second one.
+      {{GURL("https://www.google.com"), GURL("https://www.redirected.com")},
+       false,
+       {true, false}},
+      // First one adds the header and transfers it to the second.
+      {{GURL("http://www.header_adder.com"),
+        GURL("http://www.redirected_from_header_adder.com")},
+       true,
+       {true, true}}};
+
+  for (const auto& test_case : all_tests) {
+    SCOPED_TRACE(test_case.urls[0]);
+    // If test case requires adding header for the first url,
+    // change the delegate.
+    std::unique_ptr<HeaderTestDispatcherHostDelegate> dispatcher_host_delegate;
+    if (test_case.inject_header) {
+      dispatcher_host_delegate.reset(
+          new HeaderTestDispatcherHostDelegate(test_case.urls[0]));
+      content::ResourceDispatcherHost::Get()->SetDelegate(
+          dispatcher_host_delegate.get());

[mmenke, 2016/09/01 18:31:48]

Here and below, should set this on the IO thread (Below should also wait for it
to be set on the IO thread, before continuing)

[Ramin Halavati, 2016/09/02 13:41:43]

Done.

+    }
+
+    // Setup mockup interceptors.
+    for (const GURL& url : test_case.urls) {
+      content::BrowserThread::PostTask(
+          content::BrowserThread::IO, FROM_HERE,
+          base::Bind(&MirrorMockJobInterceptor::Register, url, root_http,
+                     report_request_headers));
+    }
+
+    // Navigate to first url.
+    ui_test_utils::NavigateToURL(browser(), test_case.urls[0]);
+
+    // Cleanup before verifying the observed headers.
+    for (const GURL& url : test_case.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();
+
+    if (test_case.inject_header)
+      content::ResourceDispatcherHost::Get()->SetDelegate(nullptr);

[mmenke, 2016/09/01 18:31:48]

We should restore the original RDHDelegate instead.

[Ramin Halavati, 2016/09/02 13:41:43]

I couldn't find anyway in public interface to get it, and in all other usages it
is set to nullptr. Can you suggest how it should be done?

[mmenke, 2016/09/02 16:29:43]

You're right.  Looks like the RDH mostly allows a nullptr delegate, so let's
just keep setting it to nullptr.

+
+    // Check if header exists and X-Chrome-Connected is correctly provided.
+    for (int i = 0; i < 2; i++) {
+      SCOPED_TRACE(test_case.urls[i]);
+      ASSERT_EQ(1u, request_headers.count(test_case.urls[i].spec()));
+      if (test_case.expect_header[i])
+        EXPECT_THAT(request_headers[test_case.urls[i].spec()],
+                    HasSubstr(signin::kChromeConnectedHeader));
+      else
+        EXPECT_THAT(request_headers[test_case.urls[i].spec()],
+                    Not(HasSubstr(signin::kChromeConnectedHeader)));
+    }
+  }
+}