Fix remediation for HFM-for-Typically-Secure-Users

This adds a regression browser test and moves the "reset prefs" logic
out into a separate static method which we can call when creating the
HttpsUpgradesNavigationThrottle so it applies to all clients. The reset
will run until we enable the HFM-for-typically-secure-users feature
flag.

Bug: 1475747
Change-Id: I4233763cc18f906e304604b3477d15cb396cfaad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4866771
Reviewed-by: Joe DeBlasio <[email protected]>
Commit-Queue: Chris Thompson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1197422}

chrome/browser/ssl/https_first_mode_settings_tracker.cc

@@ -145,6 +145,27 @@ std::unique_ptr<KeyedService> BuildService(content::BrowserContext* context) {
 
 }  // namespace
 
+// static
+void HttpsFirstModeService::FixTypicallySecureUserPrefs(Profile* profile) {
+  if (!base::FeatureList::IsEnabled(
+          features::kHttpsFirstModeV2ForTypicallySecureUsers)) {
+    // HFM-for-typically-secure-users has never been enabled intentionally. If
+    // we see that the preference is enabled, that was by accident. Unset the
+    // relevant preferences to undo the damage.
+    if (profile->GetPrefs()->GetBoolean(prefs::kHttpsOnlyModeAutoEnabled)) {
+      // If HFM had already been enabled, the code wouldn't have toggled
+      // kHttpsOnlyModeAutoEnabled. That means it's safe to disable HFM here --
+      // HFM can only be enabled because we set it that way. We clear the pref
+      // here so it is treated as though the user has not explicitly set it.
+      profile->GetPrefs()->ClearPref(prefs::kHttpsOnlyModeEnabled);
+      // Clear the kHttpsOnlyModeAutoEnabled pref entirely, as some of the
+      // HFM-for-typically-secure users logic relies on checking whether it has
+      // ever been set.
+      profile->GetPrefs()->ClearPref(prefs::kHttpsOnlyModeAutoEnabled);
+    }
+  }
+}
+
 HttpsFirstModeService::HttpsFirstModeService(Profile* profile,
                                              base::Clock* clock)
     : profile_(profile), clock_(clock) {
@@ -227,18 +248,6 @@ bool HttpsFirstModeService::MaybeEnableHttpsFirstModeForUser(
 
   if (!base::FeatureList::IsEnabled(
           features::kHttpsFirstModeV2ForTypicallySecureUsers)) {
-    // Temporary fix for users impacted by crbug.com/1475747:
-    // HFM-for-typically-secure-users has never been enabled intentionally. If
-    // we see that the preference has been set, that was by accident. Unset the
-    // relevant preferences to undo the damage.
-    if (profile_->GetPrefs()->HasPrefPath(prefs::kHttpsOnlyModeAutoEnabled)) {
-      profile_->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeAutoEnabled, false);
-      // If HFM had already been enabled, the code wouldn't have toggled
-      // kHttpsOnlyModeAutoEnabled. That means it's safe to disable HFM here --
-      // HFM can only be enabled because we set it that way.
-      profile_->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeEnabled, false);
-    }
-
     return false;
   }
 
@@ -302,6 +311,8 @@ bool HttpsFirstModeService::MaybeEnableHttpsFirstModeForUser(
   if (enable_https_first_mode &&
       !profile_->GetPrefs()->HasPrefPath(prefs::kHttpsOnlyModeEnabled) &&
       !profile_->GetPrefs()->HasPrefPath(prefs::kHttpsOnlyModeAutoEnabled)) {
+    // The prefs must be set in this order, as setting kHttpsOnlyModeEnabled
+    // will cause kHttpsOnlyModeAutoEnabled to be reset to false.
     profile_->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeEnabled,
                                      enable_https_first_mode);
     profile_->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeAutoEnabled,

chrome/browser/ssl/https_first_mode_settings_tracker.h

@@ -29,6 +29,11 @@ class HttpsFirstModeService
       public safe_browsing::AdvancedProtectionStatusManager::
           StatusChangedObserver {
  public:
+  // Reset user prefs if they were accidentally enabled previously. See
+  // crbug.com/1475747 for details. Only has as effect if
+  // kHttpsFirstModeV2ForTypicallySecureUsers is not enabled.
+  static void FixTypicallySecureUserPrefs(Profile* profile);
+
   explicit HttpsFirstModeService(Profile* profile, base::Clock* clock);
   ~HttpsFirstModeService() override;
 

chrome/browser/ssl/https_first_mode_settings_tracker_unittest.cc

@@ -303,16 +303,11 @@ TEST_F(HttpsFirstModeSettingsTrackerTest, UndoTypicallySecureUser) {
   feature_list.InitAndDisableFeature(
       features::kHttpsFirstModeV2ForTypicallySecureUsers);
 
-  HttpsFirstModeService* service =
-      HttpsFirstModeServiceFactory::GetForProfile(profile());
-  ASSERT_TRUE(service);
-
   // Pretend that the feature had been erroneously enabled previously.
   profile()->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeAutoEnabled, true);
   profile()->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeEnabled, true);
 
-  EXPECT_FALSE(
-      service->MaybeEnableHttpsFirstModeForUser(/*add_fallback_entry=*/true));
+  HttpsFirstModeService::FixTypicallySecureUserPrefs(profile());
   EXPECT_FALSE(profile()->GetPrefs()->GetBoolean(prefs::kHttpsOnlyModeEnabled));
   EXPECT_FALSE(
       profile()->GetPrefs()->GetBoolean(prefs::kHttpsOnlyModeAutoEnabled));

chrome/browser/ssl/https_upgrades_browsertest.cc

@@ -2643,6 +2643,47 @@ IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
       http_url.host(), contents->GetPrimaryMainFrame()->GetStoragePartition()));
 }
 
+// Regression test for crbug.com/1475747. With HTTPS-Upgrades enabled but
+// HFM-for-Typically-Secure-Users disabled, sets the prefs as though
+// HFM-for-Typically-Secure-Users had triggered accidentally and tests whether
+// the remediation that resets this works correctly.
+IN_PROC_BROWSER_TEST_P(HttpsUpgradesBrowserTest,
+                       AccidentalTypicallySecureUsersRemediation) {
+  // Just test for HTTPS-Upgrades (we want HFM-for-Typically-Secure-Users to be
+  // disabled).
+  if (https_upgrades_test_type() != HttpsUpgradesTestType::kHttpsUpgradesOnly) {
+    return;
+  }
+
+  // Pretend that the feature had been erroneously enabled previously. The prefs
+  // must be set in this order, as setting kHttpsOnlyModeEnabled will cause
+  // kHttpsOnlyModeAutoEnabled to be reset to false by the pref observer
+  // in HttpsFirstModeService.
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeEnabled,
+                                               true);
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kHttpsOnlyModeAutoEnabled,
+                                               true);
+
+  // Start a navigation to a URL that will be upgraded but fail, which should
+  // cause the prefs to be reset and no interstitial should be shown.
+  GURL http_url = http_server()->GetURL("bad-https.com", "/simple.html");
+  GURL https_url = https_server()->GetURL("bad-https.com", "/simple.html");
+
+  auto* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  NavigateAndWaitForFallback(contents, http_url);
+  EXPECT_EQ(http_url, contents->GetLastCommittedURL());
+
+  EXPECT_FALSE(
+      chrome_browser_interstitials::IsShowingHttpsFirstModeInterstitial(
+          contents));
+
+  // Check that the prefs have been reset.
+  EXPECT_FALSE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kHttpsOnlyModeEnabled));
+  EXPECT_FALSE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kHttpsOnlyModeAutoEnabled));
+}
+
 // A simple test fixture that constructs a HistogramTester (so that it gets
 // initialized before browser startup). Used for testing pref tracking logic.
 class HttpsUpgradesPrefsBrowserTest : public InProcessBrowserTest {

chrome/browser/ssl/https_upgrades_navigation_throttle.cc

@@ -71,6 +71,13 @@ HttpsUpgradesNavigationThrottle::MaybeCreateThrottleFor(
     return nullptr;
   }
 
+  // Repair prefs if the user was previously affected by crbug.com/1475747. This
+  // will reset the affected prefs, before setting up the state for the Throttle
+  // for this navigation.
+  // TODO(crbug.com/1475747): Remove this after M120 (or after
+  // kHttpsFirstModeV2ForTypicallySecureUsers is enabled by default).
+  HttpsFirstModeService::FixTypicallySecureUserPrefs(profile);
+
   PrefService* prefs = profile->GetPrefs();
   security_interstitials::https_only_mode::HttpInterstitialState
       interstitial_state;