Cache control site status in SecurityStateTabHelper

This caches the results of IsTLSDeprecationConfigControlSite() in
SecurityStateTabHelper, ensuring that all callers get a consistent
result for |is_legacy_tls_control_site| for the duration of the page
visit.

Bug: 1011089
Change-Id: I7564e54c509ab927d0e3a3212859444ae4b02804
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838302
Commit-Queue: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704775}

chrome/browser/ssl/security_state_tab_helper.cc

@@ -122,22 +122,34 @@ SecurityStateTabHelper::SecurityStateTabHelper(
 
 SecurityStateTabHelper::~SecurityStateTabHelper() {}
 
-security_state::SecurityLevel SecurityStateTabHelper::GetSecurityLevel() const {
+security_state::SecurityLevel SecurityStateTabHelper::GetSecurityLevel() {
   return security_state::GetSecurityLevel(
       *GetVisibleSecurityState(), UsedPolicyInstalledCertificate(),
       base::BindRepeating(&content::IsOriginSecure));
 }
 
 std::unique_ptr<security_state::VisibleSecurityState>
-SecurityStateTabHelper::GetVisibleSecurityState() const {
+SecurityStateTabHelper::GetVisibleSecurityState() {
   auto state = security_state::GetVisibleSecurityState(web_contents());
 
   if (state->connection_info_initialized) {
     state->connection_used_legacy_tls =
         IsLegacyTLS(state->url, state->connection_status);
     if (state->connection_used_legacy_tls) {
-      state->is_legacy_tls_control_site =
-          IsTLSDeprecationConfigControlSite(state->url);
+      // We cache the results of the lookup for the duration of a navigation
+      // entry.
+      int navigation_id =
+          web_contents()->GetController().GetVisibleEntry()->GetUniqueID();
+      if (cached_is_legacy_tls_control_site_ &&
+          cached_is_legacy_tls_control_site_.value().first == navigation_id) {
+        state->is_legacy_tls_control_site =
+            cached_is_legacy_tls_control_site_.value().second;
+      } else {
+        state->is_legacy_tls_control_site =
+            IsTLSDeprecationConfigControlSite(state->url);
+        cached_is_legacy_tls_control_site_ = std::pair<int, bool>(
+            navigation_id, state->is_legacy_tls_control_site);
+      }
     }
   }
 

chrome/browser/ssl/security_state_tab_helper.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/optional.h"
 #include "components/security_state/core/security_state.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -26,9 +27,9 @@ class SecurityStateTabHelper
   ~SecurityStateTabHelper() override;
 
   // See security_state::GetSecurityLevel.
-  security_state::SecurityLevel GetSecurityLevel() const;
+  security_state::SecurityLevel GetSecurityLevel();
   std::unique_ptr<security_state::VisibleSecurityState>
-  GetVisibleSecurityState() const;
+  GetVisibleSecurityState();
 
   // content::WebContentsObserver:
   void DidStartNavigation(
@@ -44,6 +45,16 @@ class SecurityStateTabHelper
   bool UsedPolicyInstalledCertificate() const;
   security_state::MaliciousContentStatus GetMaliciousContentStatus() const;
 
+  // Caches the legacy TLS control site status for the duration of a page load
+  // (bound to a specific navigation ID) to ensure that we show consistent
+  // security UI (e.g., security indicator and page info). This is because the
+  // control site status depends on external state (a component loading from
+  // disk), which can cause inconsistent state across a page load if it isn't
+  // cached.
+  base::Optional<std::pair<int /* navigation entry ID */,
+                           bool /* is_legacy_tls_control_site */>>
+      cached_is_legacy_tls_control_site_;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
   DISALLOW_COPY_AND_ASSIGN(SecurityStateTabHelper);

chrome/browser/ssl/security_state_tab_helper_browsertest.cc

@@ -1779,6 +1779,11 @@ const char kResponseFilePath[] = "chrome/test/data/title1.html";
 const int kObsoleteTLSVersion = net::SSL_CONNECTION_VERSION_TLS1_1;
 // ECDHE_RSA + AES_128_CBC with HMAC-SHA1
 const uint16_t kObsoleteCipherSuite = 0xc013;
+// SHA-256 hash of kMockNonsecureHostname for use in setting a control site in
+// the LegacyTLSExperimentConfig for Legacy TLS tests. Generated with
+// `echo -n "example-nonsecure.test" | openssl sha256`.
+const char kMockControlSiteHash[] =
+    "aaa334d67e96314a14d5679b2309e72f96bf30f9fe9b218e5db3d57be8baa94c";
 
 class BrowserTestNonsecureURLRequest : public InProcessBrowserTest {
  public:
@@ -1897,9 +1902,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
   auto config =
       std::make_unique<chrome_browser_ssl::LegacyTLSExperimentConfig>();
   GURL control_site(std::string("https://") + kMockNonsecureHostname);
-  std::string control_site_hash =
-      "aaa334d67e96314a14d5679b2309e72f96bf30f9fe9b218e5db3d57be8baa94c";
-  config->add_control_site_hashes(control_site_hash);
+  config->add_control_site_hashes(kMockControlSiteHash);
   SetRemoteTLSDeprecationConfigProto(std::move(config));
 
   auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
@@ -1946,6 +1949,44 @@ IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
                                               original_pref);
 }
 
+// Tests that a page has consistent security state despite the config proto
+// getting loaded during the page visit lifetime (see crbug.com/1011089).
+IN_PROC_BROWSER_TEST_F(BrowserTestNonsecureURLRequest,
+                       LegacyTLSDelayedConfigLoad) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      security_state::features::kLegacyTLSWarnings);
+
+  // Navigate to an affected page before the config proto has been set.
+  auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+  auto* helper = SecurityStateTabHelper::FromWebContents(web_contents);
+
+  GURL control_site(std::string("https://") + kMockNonsecureHostname);
+  ui_test_utils::NavigateToURL(browser(), control_site);
+
+  EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
+  EXPECT_FALSE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
+  EXPECT_EQ(helper->GetSecurityLevel(), security_state::WARNING);
+
+  // Set the config proto.
+  auto config =
+      std::make_unique<chrome_browser_ssl::LegacyTLSExperimentConfig>();
+  config->add_control_site_hashes(kMockControlSiteHash);
+  SetRemoteTLSDeprecationConfigProto(std::move(config));
+
+  // Security state for the current page should not change.
+  EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
+  EXPECT_FALSE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
+  EXPECT_EQ(helper->GetSecurityLevel(), security_state::WARNING);
+
+  // Refreshing the page should update the page's security state to now be
+  // treated as control group.
+  ui_test_utils::NavigateToURL(browser(), control_site);
+  EXPECT_TRUE(helper->GetVisibleSecurityState()->connection_used_legacy_tls);
+  EXPECT_TRUE(helper->GetVisibleSecurityState()->is_legacy_tls_control_site);
+  EXPECT_EQ(helper->GetSecurityLevel(), security_state::SECURE);
+}
+
 // Tests that the Not Secure chip does not show for error pages on http:// URLs.
 // Regression test for https://crbug.com/760647.
 IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperIncognitoTest, HttpErrorPage) {