Stop on redirects while checking for www mismatches

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index 5992e360a97fb24398efe4aa442bdf16ac886d94..99f1526a4c05effb3bac0fa07743a791e9978050 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -104,6 +104,7 @@
 #include "net/ssl/ssl_info.h"
 #include "net/test/cert_test_util.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/request_handler_util.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/test/test_certificate_data.h"
@@ -3573,10 +3574,50 @@ IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest,
   TriggerTimeResponse();
 }
 
+// Fails with a CHECK for all requests over HTTP except for favicons. This is to
+// ensure that name mismatch redirect feature's suggest URL ping stops on
+// redirects and never hits an HTTP URL.
+class HttpNameMismatchPingInterceptor : public net::URLRequestInterceptor {
+ public:
+  HttpNameMismatchPingInterceptor() {}
+  ~HttpNameMismatchPingInterceptor() override {}
+
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* delegate) const override {
+    if (request->url().path() == "/favicon.ico") {
+      // Ignore favicon requests.

[elawrence, 2017/06/03 18:45:11]

As someone new to this code, it would be nice if the comment explained why
ignoring favicon requests is desirable.

[meacer, 2017/06/05 21:57:59]

Done.

+      return nullptr;
+    }
+
+    CHECK(request->url().SchemeIsCryptographic())

[elawrence, 2017/06/03 18:45:11]

Can this check ever pass? Below, AddHostnameInterceptor is always called with
the HTTP scheme.

[meacer, 2017/06/05 21:57:59]

No, it can never pass. Converted to CHECK(false).

+        << "Name mismatch pings must never be over HTTP. This request was for "
+        << request->url();
+    return nullptr;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(HttpNameMismatchPingInterceptor);
+};
+
 class CommonNameMismatchBrowserTest : public CertVerifierBrowserTest {
  public:
-  CommonNameMismatchBrowserTest() : CertVerifierBrowserTest() {}
-  ~CommonNameMismatchBrowserTest() override {}
+  CommonNameMismatchBrowserTest() : CertVerifierBrowserTest() {
+    // Add interceptors for HTTP versions of example.org and www.example.org.
+    // These are the hostnames used in the tests, and we never want them to be
+    // contacted over HTTP.
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", "example.org",
+        std::unique_ptr<HttpNameMismatchPingInterceptor>(
+            new HttpNameMismatchPingInterceptor()));
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", "www.example.org",
+        std::unique_ptr<HttpNameMismatchPingInterceptor>(
+            new HttpNameMismatchPingInterceptor()));
+  }
+  ~CommonNameMismatchBrowserTest() override {
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+  }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // Enable finch experiment for SSL common name mismatch handling.
@@ -3595,14 +3636,14 @@ class CommonNameMismatchBrowserTest : public CertVerifierBrowserTest {
 // mail.example.com.
 IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
                        ShouldShowWWWSubdomainMismatchInterstitial) {
-  net::EmbeddedTestServer https_server_example_domain_(
+  net::EmbeddedTestServer https_server_example_domain(
       net::EmbeddedTestServer::TYPE_HTTPS);
-  https_server_example_domain_.ServeFilesFromSourceDirectory(
+  https_server_example_domain.ServeFilesFromSourceDirectory(
       base::FilePath(kDocRoot));
-  ASSERT_TRUE(https_server_example_domain_.Start());
+  ASSERT_TRUE(https_server_example_domain.Start());
 
   scoped_refptr<net::X509Certificate> cert =
-      https_server_example_domain_.GetCertificate();
+      https_server_example_domain.GetCertificate();
 
   // Use the "spdy_pooling.pem" cert which has "mail.example.com"
   // as one of its SANs.
@@ -3626,11 +3667,11 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
 
   // Use a complex URL to ensure the path, etc., are preserved. The path itself
   // does not matter.
-  GURL https_server_url =
-      https_server_example_domain_.GetURL("/ssl/google.html?a=b#anchor");
+  const GURL https_server_url =
+      https_server_example_domain.GetURL("/ssl/google.html?a=b#anchor");
   GURL::Replacements replacements;
   replacements.SetHostStr("www.mail.example.com");
-  GURL https_server_mismatched_url =
+  const GURL https_server_mismatched_url =
       https_server_url.ReplaceComponents(replacements);
 
   WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
@@ -3655,14 +3696,14 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
 // for www.example.org. Verify that the page redirects to www.example.org.
 IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
                        CheckWWWSubdomainMismatchInverse) {
-  net::EmbeddedTestServer https_server_example_domain_(
+  net::EmbeddedTestServer https_server_example_domain(
       net::EmbeddedTestServer::TYPE_HTTPS);
-  https_server_example_domain_.ServeFilesFromSourceDirectory(
+  https_server_example_domain.ServeFilesFromSourceDirectory(
       base::FilePath(kDocRoot));
-  ASSERT_TRUE(https_server_example_domain_.Start());
+  ASSERT_TRUE(https_server_example_domain.Start());
 
   scoped_refptr<net::X509Certificate> cert =
-      https_server_example_domain_.GetCertificate();
+      https_server_example_domain.GetCertificate();
 
   net::CertVerifyResult verify_result;
   verify_result.verified_cert =
@@ -3679,11 +3720,11 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
   mock_cert_verifier()->AddResultForCertAndHost(cert.get(), "www.example.org",
                                                 verify_result_valid, net::OK);
 
-  GURL https_server_url =
-      https_server_example_domain_.GetURL("/ssl/google.html?a=b");
+  const GURL https_server_url =
+      https_server_example_domain.GetURL("/ssl/google.html?a=b");
   GURL::Replacements replacements;
   replacements.SetHostStr("example.org");
-  GURL https_server_mismatched_url =
+  const GURL https_server_mismatched_url =
       https_server_url.ReplaceComponents(replacements);
 
   WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
@@ -3699,6 +3740,85 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
                      AuthState::NONE);
 }
 
+namespace {
+// Redirects incoming request to http://example.org.
+std::unique_ptr<net::test_server::HttpResponse> HTTPSToHTTPRedirectHandler(
+    const net::EmbeddedTestServer* test_server,
+    const net::test_server::HttpRequest& request) {
+  GURL::Replacements replacements;
+  replacements.SetHostStr("example.org");
+  replacements.SetSchemeStr("http");
+  const GURL redirect_url =
+      test_server->base_url().ReplaceComponents(replacements);
+
+  std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
+      new net::test_server::BasicHttpResponse);
+  http_response->set_code(net::HTTP_MOVED_PERMANENTLY);
+  http_response->AddCustomHeader("Location", redirect_url.spec());
+  return std::move(http_response);
+}
+}  // namespace
+
+// Common name mismatch handling feature should ignore redirects when pinging
+// the suggested hostname. Visit the URL example.org on a server that presents a
+// valid certificate for www.example.org. In this case, www.example.org
+// redirects to http://example.org, and the SSL error should not be redirected
+// to this URL.
+IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
+                       WWWSubdomainMismatch_StopOnRedirects) {
+  net::EmbeddedTestServer https_server_example_domain(
+      net::EmbeddedTestServer::TYPE_HTTPS);
+
+  // Redirect all URLs to http://example.org. Since this test will trigger only
+  // one request to check the suggested URL, redirecting all requests is OK.
+  // We would normally use content::SetupCrossSiteRedirector here, but that
+  // function does not support https to http redirects.
+  // This must be done before ServeFilesFromSourceDirectory(), otherwise the
+  // test server will serve files instead of redirecting requests to them.
+  https_server_example_domain.RegisterRequestHandler(
+      base::Bind(&HTTPSToHTTPRedirectHandler, &https_server_example_domain));
+
+  https_server_example_domain.ServeFilesFromSourceDirectory(
+      base::FilePath(kDocRoot));
+
+  ASSERT_TRUE(https_server_example_domain.Start());
+
+  scoped_refptr<net::X509Certificate> cert =
+      https_server_example_domain.GetCertificate();
+
+  net::CertVerifyResult verify_result;
+  verify_result.verified_cert =
+      net::ImportCertFromFile(net::GetTestCertsDirectory(), "spdy_pooling.pem");
+  verify_result.cert_status = net::CERT_STATUS_COMMON_NAME_INVALID;
+
+  mock_cert_verifier()->AddResultForCertAndHost(
+      cert.get(), "example.org", verify_result,
+      net::ERR_CERT_COMMON_NAME_INVALID);
+
+  net::CertVerifyResult verify_result_valid;
+  verify_result_valid.verified_cert =
+      net::ImportCertFromFile(net::GetTestCertsDirectory(), "spdy_pooling.pem");
+  mock_cert_verifier()->AddResultForCertAndHost(cert.get(), "www.example.org",
+                                                verify_result_valid, net::OK);
+
+  // The user will visit https://example.org:port/ssl/blank.html.
+  GURL::Replacements replacements;
+  replacements.SetHostStr("example.org");
+  const GURL https_server_mismatched_url =
+      https_server_example_domain.GetURL("/ssl/blank.html")
+          .ReplaceComponents(replacements);
+
+  // Should simply show an interstitial, because the suggested URL
+  // (https://www.example.org) redirected to http://example.org.
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(browser(), https_server_mismatched_url);
+  WaitForInterstitialAttach(contents);
+
+  CheckSecurityState(contents, net::CERT_STATUS_COMMON_NAME_INVALID,
+                     security_state::DANGEROUS,
+                     AuthState::SHOWING_INTERSTITIAL);
+}
+
 // Tests this scenario:
 // - |CommonNameMismatchHandler| does not give a callback as it's set into the
 //   state |IGNORE_REQUESTS_FOR_TESTING|. So no suggested URL check result can
@@ -3709,14 +3829,14 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
 // - Stopping the page load shouldn't result in any interstitials.
 IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
                        InterstitialStopNavigationWhileLoading) {
-  net::EmbeddedTestServer https_server_example_domain_(
+  net::EmbeddedTestServer https_server_example_domain(
       net::EmbeddedTestServer::TYPE_HTTPS);
-  https_server_example_domain_.ServeFilesFromSourceDirectory(
+  https_server_example_domain.ServeFilesFromSourceDirectory(
       base::FilePath(kDocRoot));
-  ASSERT_TRUE(https_server_example_domain_.Start());
+  ASSERT_TRUE(https_server_example_domain.Start());
 
   scoped_refptr<net::X509Certificate> cert =
-      https_server_example_domain_.GetCertificate();
+      https_server_example_domain.GetCertificate();
 
   net::CertVerifyResult verify_result;
   verify_result.verified_cert =
@@ -3733,11 +3853,11 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
   mock_cert_verifier()->AddResultForCertAndHost(cert.get(), "mail.example.com",
                                                 verify_result_valid, net::OK);
 
-  GURL https_server_url =
-      https_server_example_domain_.GetURL("/ssl/google.html?a=b");
+  const GURL https_server_url =
+      https_server_example_domain.GetURL("/ssl/google.html?a=b");
   GURL::Replacements replacements;
   replacements.SetHostStr("www.mail.example.com");
-  GURL https_server_mismatched_url =
+  const GURL https_server_mismatched_url =
       https_server_url.ReplaceComponents(replacements);
 
   WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
@@ -3772,14 +3892,14 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
 // result is the same. (i.e. page load stops, no interstitials shown)
 IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
                        InterstitialReloadNavigationWhileLoading) {
-  net::EmbeddedTestServer https_server_example_domain_(
+  net::EmbeddedTestServer https_server_example_domain(
       net::EmbeddedTestServer::TYPE_HTTPS);
-  https_server_example_domain_.ServeFilesFromSourceDirectory(
+  https_server_example_domain.ServeFilesFromSourceDirectory(
       base::FilePath(kDocRoot));
-  ASSERT_TRUE(https_server_example_domain_.Start());
+  ASSERT_TRUE(https_server_example_domain.Start());
 
   scoped_refptr<net::X509Certificate> cert =
-      https_server_example_domain_.GetCertificate();
+      https_server_example_domain.GetCertificate();
 
   net::CertVerifyResult verify_result;
   verify_result.verified_cert =
@@ -3796,11 +3916,11 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
   mock_cert_verifier()->AddResultForCertAndHost(cert.get(), "mail.example.com",
                                                 verify_result_valid, net::OK);
 
-  GURL https_server_url =
-      https_server_example_domain_.GetURL("/ssl/google.html?a=b");
+  const GURL https_server_url =
+      https_server_example_domain.GetURL("/ssl/google.html?a=b");
   GURL::Replacements replacements;
   replacements.SetHostStr("www.mail.example.com");
-  GURL https_server_mismatched_url =
+  const GURL https_server_mismatched_url =
       https_server_url.ReplaceComponents(replacements);
 
   WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
@@ -3833,14 +3953,14 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
 // new page should load, and no interstitials should be shown.
 IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
                        InterstitialNavigateAwayWhileLoading) {
-  net::EmbeddedTestServer https_server_example_domain_(
+  net::EmbeddedTestServer https_server_example_domain(
       net::EmbeddedTestServer::TYPE_HTTPS);
-  https_server_example_domain_.ServeFilesFromSourceDirectory(
+  https_server_example_domain.ServeFilesFromSourceDirectory(
       base::FilePath(kDocRoot));
-  ASSERT_TRUE(https_server_example_domain_.Start());
+  ASSERT_TRUE(https_server_example_domain.Start());
 
   scoped_refptr<net::X509Certificate> cert =
-      https_server_example_domain_.GetCertificate();
+      https_server_example_domain.GetCertificate();
 
   net::CertVerifyResult verify_result;
   verify_result.verified_cert =
@@ -3857,11 +3977,11 @@ IN_PROC_BROWSER_TEST_F(CommonNameMismatchBrowserTest,
   mock_cert_verifier()->AddResultForCertAndHost(cert.get(), "mail.example.com",
                                                 verify_result_valid, net::OK);
 
-  GURL https_server_url =
-      https_server_example_domain_.GetURL("/ssl/google.html?a=b");
+  const GURL https_server_url =
+      https_server_example_domain.GetURL("/ssl/google.html?a=b");
   GURL::Replacements replacements;
   replacements.SetHostStr("www.mail.example.com");
-  GURL https_server_mismatched_url =
+  const GURL https_server_mismatched_url =
       https_server_url.ReplaceComponents(replacements);
 
   WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();