Reset HTTP allowlist when the HTTPS-First Mode setting changes

Sites automatically allowlisted under HTTPS-Upgrades shouldn't affect HTTPS-First Mode (as the user has not explicitly allowed them as there was no warning shown), and vice versa. This CL updates the HttpsFirstModeService which tracks the HTTPS-First Mode pref to also clear the HTTP allowlist whenever that pref changes. Bug: 1446193 Change-Id: I6a044688ae7e37021fee9734fcd2aee308b1676d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4546210 Commit-Queue: Chris Thompson <[email protected]> Reviewed-by: Mustafa Emre Acer <[email protected]> Cr-Commit-Position: refs/heads/main@{#1146268}
Igalia · May 18, 2023 · 6f9c348 · 6f9c348
1 parent 86f4c4a
commit 6f9c348
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 0 deletions.
diff --git a/chrome/browser/ssl/https_first_mode_settings_tracker.cc b/chrome/browser/ssl/https_first_mode_settings_tracker.cc
@@ -165,6 +165,14 @@ void HttpsFirstModeService::OnHttpsFirstModePrefChanged() {
       kHttpsFirstModeSyntheticFieldTrialName,
       enabled ? kHttpsFirstModeSyntheticFieldTrialEnabledGroup
               : kHttpsFirstModeSyntheticFieldTrialDisabledGroup);
+
+  // Reset the allowlist when the pref changes. A user going from HTTPS-Upgrades
+  // to HTTPS-First Mode shouldn't inherit the set of allowlisted sites (or
+  // vice versa).
+  StatefulSSLHostStateDelegate* state =
+      static_cast<StatefulSSLHostStateDelegate*>(
+          profile_->GetSSLHostStateDelegate());
+  state->ClearHttpsOnlyModeAllowlist();
 }
 
 void HttpsFirstModeService::OnAdvancedProtectionStatusChanged(bool enabled) {

diff --git a/chrome/browser/ssl/https_upgrades_browsertest.cc b/chrome/browser/ssl/https_upgrades_browsertest.cc
@@ -1946,6 +1946,44 @@ IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest, crbug1431026) {
   }
 }
 
+// Tests that when the HTTPS-First Mode setting is toggled on or off, the
+// HTTP allowlist is cleared.
+IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
+                       TogglingSettingClearsAllowlist) {
+  auto http_url = http_server()->GetURL("bad-https.com", "/simple.html");
+  auto* contents = browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Start by enabling HTTPS-First Mode.
+  SetPref(true);
+
+  // Navigate to a URL that will fail upgrades, and click through the
+  // interstitial to add it to the allowlist.
+  EXPECT_FALSE(content::NavigateToURL(contents, http_url));
+  EXPECT_TRUE(chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
+      contents));
+  ProceedThroughInterstitial(contents);
+
+  // Disable the HTTPS-First Mode pref. This should clear the allowlist.
+  SetPref(false);
+
+  // If HTTPS-Upgrades are enabled, navigating again should cause the site to
+  // get added back to the allowlist. If not, the HTTP URL will load normally as
+  // HTTPS-First Mode is disabled.
+  EXPECT_TRUE(content::NavigateToURL(contents, http_url));
+  EXPECT_FALSE(
+      chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
+          contents));
+
+  // Re-enable the HTTPS-First Mode pref. The allowlist should be cleared again.
+  SetPref(true);
+
+  // Navigate to a URL that will fail upgrades, and the interstitial should be
+  // shown again as the allowlist was cleared.
+  EXPECT_FALSE(content::NavigateToURL(contents, http_url));
+  EXPECT_TRUE(chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
+      contents));
+}
+
 // A simple test fixture that ensures the kHttpsFirstModeV2 feature is enabled
 // and constructs a HistogramTester (so that it gets initialized before browser
 // startup). Used for testing pref tracking logic.

diff --git a/components/security_interstitials/content/stateful_ssl_host_state_delegate.cc b/components/security_interstitials/content/stateful_ssl_host_state_delegate.cc
@@ -397,6 +397,10 @@ bool StatefulSSLHostStateDelegate::IsHttpsEnforcedForHost(
                                                         is_nondefault_storage);
 }
 
+void StatefulSSLHostStateDelegate::ClearHttpsOnlyModeAllowlist() {
+  https_only_mode_allowlist_.ClearAllowlist(base::Time(), base::Time::Max());
+}
+
 void StatefulSSLHostStateDelegate::RevokeUserAllowExceptions(
     const std::string& host) {
   GURL url = GetSecureGURLForHost(host);

diff --git a/components/security_interstitials/content/stateful_ssl_host_state_delegate.h b/components/security_interstitials/content/stateful_ssl_host_state_delegate.h
@@ -67,12 +67,14 @@ class StatefulSSLHostStateDelegate : public content::SSLHostStateDelegate,
       const net::X509Certificate& cert,
       int error,
       content::StoragePartition* storage_partition) override;
+
   void HostRanInsecureContent(const std::string& host,
                               int child_id,
                               InsecureContentType content_type) override;
   bool DidHostRunInsecureContent(const std::string& host,
                                  int child_id,
                                  InsecureContentType content_type) override;
+
   void AllowHttpForHost(const std::string& host,
                         content::StoragePartition* storage_partition) override;
   bool IsHttpAllowedForHost(
@@ -90,6 +92,9 @@ class StatefulSSLHostStateDelegate : public content::SSLHostStateDelegate,
       const std::string& host,
       content::StoragePartition* storage_partition) override;
 
+  // Clears all entries from the HTTP allowlist.
+  void ClearHttpsOnlyModeAllowlist();
+
   // RevokeUserAllowExceptionsHard is the same as RevokeUserAllowExceptions but
   // additionally may close idle connections in the process. This should be used
   // *only* for rare events, such as a user controlled button, as it may be very