Captive portal certificate list should be checked when name mismatch is the only error

chrome/browser/ssl/ssl_error_handler_unittest.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/ssl/ssl_error_handler.h"
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
@@ skipping 173 matching lines @@
   bool captive_portal_interstitial_shown_;
   bool redirected_to_suggested_url_;
   bool is_overridable_error_;
   CommonNameMismatchHandler::CheckUrlCallback suggested_url_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandlerDelegate);
 };
 
 }  // namespace
 
-template <net::CertStatus cert_status>
-class SSLErrorHandlerCertStatusTestBase
-    : public ChromeRenderViewHostTestHarness {
+// A class to test name mismatch errors. Creates an error handler with a name
+// mismatch error by default. The error handler can be recreated by calling
+// ResetErrorHandler() with an appropriate cert status.
+class SSLErrorHandlerNameMismatchTest : public ChromeRenderViewHostTestHarness {
  public:
-  SSLErrorHandlerCertStatusTestBase() : field_trial_list_(nullptr) {}
+  SSLErrorHandlerNameMismatchTest() : field_trial_list_(nullptr) {}
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     SSLErrorHandler::ResetConfigForTesting();
     SSLErrorHandler::SetInterstitialDelayForTesting(base::TimeDelta());
+    ResetErrorHandler(net::CERT_STATUS_COMMON_NAME_INVALID);
+  }
+
+  void TearDown() override {
+    EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+    error_handler_.reset(nullptr);
+    SSLErrorHandler::ResetConfigForTesting();
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  TestSSLErrorHandler* error_handler() { return error_handler_.get(); }
+  TestSSLErrorHandlerDelegate* delegate() { return delegate_; }
+
+  const net::SSLInfo& ssl_info() { return ssl_info_; }
+
+ protected:
+  // Deletes the current error handler and creates a new one with the given
+  // |cert_status|.
+  void ResetErrorHandler(net::CertStatus cert_status) {
+    ssl_info_.Reset();
     ssl_info_.cert =
         net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
     ssl_info_.cert_status = cert_status;
     ssl_info_.public_key_hashes.push_back(
         net::HashValue(kCertPublicKeyHashValue));
 
     delegate_ =
         new TestSSLErrorHandlerDelegate(profile(), web_contents(), ssl_info_);
     error_handler_.reset(new TestSSLErrorHandler(
         std::unique_ptr<SSLErrorHandler::Delegate>(delegate_), web_contents(),
         profile(), net::MapCertStatusToNetError(ssl_info_.cert_status),
         ssl_info_,
         GURL(),  // request_url
         base::Callback<void(content::CertificateRequestResultType)>()));
 
     // Enable finch experiment for captive portal interstitials.
     ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
                     "CaptivePortalInterstitial", "Enabled"));
     // Enable finch experiment for SSL common name mismatch handling.
     ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
                     "SSLCommonNameMismatchHandling", "Enabled"));
   }
 
-  void TearDown() override {
+  void TestNoCaptivePortalInterstitial(int expected_cert_error) {
+    base::HistogramTester histograms;
+
+    EXPECT_EQ(expected_cert_error, error_handler()->cert_error_for_testing());

[estark, 2017/02/15 00:31:41]

I feel meh about having this cert_error_for_testing() getter. Is it just to
sanity-check that the SSLErrorHandler was recreated with the appropriate error?
If so, I don't think it's worth exposing the implementation detail just for a
sanity-check. (especially because you can effectively do the same sanity-check
by just checking that MapCertStatusToNetError is what you expect)

[meacer, 2017/02/23 23:28:19]

Correct.
I was trying to avoid doing that explicit check, in case SSLErrorHandler does
some transformation to cert status other than MapCertStatusToNetError which we
would miss. But okay, let's be consistent with the browser tests and do the same
here.

Note that I'm only doing this for name mismatch + weak signature case since the
other two (name mismatch only or authority invalid only) are pretty
straightforward (i.e. adding EXPECT_EQ(CERT_ERR_COMMON_NAME_INVALID,
MapCertStatusToNetError(CERT_STATUS_COMMON_NAME_INVALID)) seems redundant)

     EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-    error_handler_.reset(nullptr);
-    SSLErrorHandler::ResetConfigForTesting();
-    ChromeRenderViewHostTestHarness::TearDown();
+    EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
+
+    auto config_proto =
+        base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
+    config_proto->add_captive_portal_cert()->set_sha256_hash(
+        "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
+    config_proto->add_captive_portal_cert()->set_sha256_hash(
+        ssl_info().public_key_hashes[0].ToString());
+    config_proto->add_captive_portal_cert()->set_sha256_hash(
+        "sha256/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
+    SSLErrorHandler::SetErrorAssistantProto(std::move(config_proto));
+
+    error_handler()->StartHandlingError();
+
+    // Timer should start for captive portal detection.
+    EXPECT_TRUE(error_handler()->IsTimerRunningForTesting());
+    EXPECT_TRUE(delegate()->captive_portal_checked());
+    EXPECT_FALSE(delegate()->ssl_interstitial_shown());
+    EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
+    EXPECT_FALSE(delegate()->suggested_url_checked());
+
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
+    EXPECT_TRUE(delegate()->captive_portal_checked());
+    EXPECT_TRUE(delegate()->ssl_interstitial_shown());
+    EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
+    EXPECT_FALSE(delegate()->suggested_url_checked());
+
+    // Check that the histogram for the captive portal cert was recorded.
+    histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                                2);
+    histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
+                                 SSLErrorHandler::HANDLE_ALL, 1);
+    histograms.ExpectBucketCount(
+        SSLErrorHandler::GetHistogramNameForTesting(),
+        SSLErrorHandler::SHOW_SSL_INTERSTITIAL_OVERRIDABLE, 1);
   }
 
-  TestSSLErrorHandler* error_handler() { return error_handler_.get(); }
-  TestSSLErrorHandlerDelegate* delegate() { return delegate_; }
-
-  const net::SSLInfo& ssl_info() { return ssl_info_; }
-
  private:
   net::SSLInfo ssl_info_;
   std::unique_ptr<TestSSLErrorHandler> error_handler_;
   TestSSLErrorHandlerDelegate* delegate_;
   base::FieldTrialList field_trial_list_;
 
-  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerCertStatusTestBase);
+  DISALLOW_COPY_AND_ASSIGN(SSLErrorHandlerNameMismatchTest);
 };
 
-using SSLErrorHandlerNameMismatchTest =
-    SSLErrorHandlerCertStatusTestBase<net::CERT_STATUS_COMMON_NAME_INVALID>;
-using SSLErrorHandlerAuthorityInvalidTest =
-    SSLErrorHandlerCertStatusTestBase<net::CERT_STATUS_AUTHORITY_INVALID>;
-
 class SSLErrorHandlerDateInvalidTest : public ChromeRenderViewHostTestHarness {
  public:
   SSLErrorHandlerDateInvalidTest()
       : field_trial_test_(new network_time::FieldTrialTest()),
         clock_(new base::SimpleTestClock),
         tick_clock_(new base::SimpleTestTickClock),
         test_server_(new net::EmbeddedTestServer) {
     SetThreadBundleOptions(content::TestBrowserThreadBundle::REAL_IO_THREAD);
     network_time::NetworkTimeTracker::RegisterPrefs(pref_service_.registry());
   }
@@ skipping 492 matching lines @@
 
 // Tests that a certificate marked as a known captive portal certificate does
 // not cause the captive portal interstitial to be shown, if the feature is
 // disabled.
 TEST_F(SSLErrorHandlerNameMismatchTest, CaptivePortalCertificateList_Disabled) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitFromCommandLine(
       std::string() /* enabled */,
       "CaptivePortalCertificateList" /* disabled */);
 
-  base::HistogramTester histograms;
-
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
-
-  auto config_proto =
-      base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      ssl_info().public_key_hashes[0].ToString());
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
-  SSLErrorHandler::SetErrorAssistantProto(std::move(config_proto));
-
-  error_handler()->StartHandlingError();
-
-  // Timer should start since captive portal certificate list feature is
-  // disabled.
-  EXPECT_TRUE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_FALSE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
-
-  // A buggy SSL error handler might have incorrectly started the timer. Run to
-  // completion to ensure the timer is expired.
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_TRUE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
-
-  // Check that the histogram for the captive portal cert was recorded.
-  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 2);
-  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
-                               SSLErrorHandler::HANDLE_ALL, 1);
-  histograms.ExpectBucketCount(
-      SSLErrorHandler::GetHistogramNameForTesting(),
-      SSLErrorHandler::SHOW_SSL_INTERSTITIAL_OVERRIDABLE, 1);
+  TestNoCaptivePortalInterstitial(net::ERR_CERT_COMMON_NAME_INVALID);
 }
 
 // Tests that an error other than name mismatch does not cause a captive portal
 // interstitial to be shown, even if the certificate is marked as a known
 // captive portal certificate.
-TEST_F(SSLErrorHandlerAuthorityInvalidTest,
-       CaptivePortalCertificateList_ShouldShowGenericInterstitial) {
+TEST_F(SSLErrorHandlerNameMismatchTest,
+       CaptivePortalCertificateList_AuthorityInvalid) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitFromCommandLine(
       "CaptivePortalCertificateList" /* enabled */,
       std::string() /* disabled */);
 
-  base::HistogramTester histograms;
+  ResetErrorHandler(net::CERT_STATUS_AUTHORITY_INVALID);
+  TestNoCaptivePortalInterstitial(net::ERR_CERT_AUTHORITY_INVALID);
+}
 
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_EQ(1u, ssl_info().public_key_hashes.size());
+// Tests that another error in addition to name mismatch error does not cause a
+// captive portal interstitial to be shown, even if the certificate is marked as
+// a known captive portal certificate. This is similar to
+// SSLErrorHandlerNameMismatchTest.CaptivePortalCertificateList_AuthorityInvalid
+// except there are two errors here (name mismatch + weak key).
+TEST_F(SSLErrorHandlerNameMismatchTest,
+       CaptivePortalCertificateList_NameMismatchAndWeakKey) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitFromCommandLine(
+      "CaptivePortalCertificateList" /* enabled */,
+      std::string() /* disabled */);
 
-  auto config_proto =
-      base::MakeUnique<chrome_browser_ssl::SSLErrorAssistantConfig>();
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa");
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      ssl_info().public_key_hashes[0].ToString());
-  config_proto->add_captive_portal_cert()->set_sha256_hash(
-      "sha256/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
-  SSLErrorHandler::SetErrorAssistantProto(std::move(config_proto));
-
-  error_handler()->StartHandlingError();
-
-  // Timer should start for captive portal detection.
-  EXPECT_TRUE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_FALSE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
-
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_FALSE(error_handler()->IsTimerRunningForTesting());
-  EXPECT_TRUE(delegate()->captive_portal_checked());
-  EXPECT_TRUE(delegate()->ssl_interstitial_shown());
-  EXPECT_FALSE(delegate()->captive_portal_interstitial_shown());
-  EXPECT_FALSE(delegate()->suggested_url_checked());
-
-  // Check that the histogram for the captive portal cert was recorded.
-  histograms.ExpectTotalCount(SSLErrorHandler::GetHistogramNameForTesting(), 2);
-  histograms.ExpectBucketCount(SSLErrorHandler::GetHistogramNameForTesting(),
-                               SSLErrorHandler::HANDLE_ALL, 1);
-  histograms.ExpectBucketCount(
-      SSLErrorHandler::GetHistogramNameForTesting(),
-      SSLErrorHandler::SHOW_SSL_INTERSTITIAL_OVERRIDABLE, 1);
+  ResetErrorHandler(net::CERT_STATUS_COMMON_NAME_INVALID |

[estark, 2017/02/15 00:31:41]

same nit about sanity-checking to make sure the code isn't only looking at the
most serious error

[meacer, 2017/02/23 23:28:20]

Done.

+                    net::CERT_STATUS_WEAK_KEY);
+  TestNoCaptivePortalInterstitial(net::ERR_CERT_COMMON_NAME_INVALID);
 }
 
 #endif  // BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)