Enable fieldtrial testing config for HttpsUpgrades

The bulk of the tests failing due to Https Upgrades feature have been
handled in crbug.com/1424194.

This CL fixes the remaining tests by allowlisting affected hostnames
and enables the feature in test configs.

Bug: 1394910
Change-Id: Iac5de38af86eb3f9b0f3ac5ca89386938b63200a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312951
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1118919}

chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc

@@ -26,6 +26,7 @@
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
+#include "chrome/browser/ssl/https_upgrades_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -281,9 +282,6 @@ class WebNavigationApiPrerenderTestWithContextType
       const WebNavigationApiPrerenderTestWithContextType&) = delete;
   WebNavigationApiPrerenderTestWithContextType& operator=(
       const WebNavigationApiPrerenderTestWithContextType&) = delete;
-
- private:
-  content::test::ScopedPrerenderFeatureList prerender_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_P(WebNavigationApiTestWithContextType, Api) {
@@ -303,6 +301,10 @@ IN_PROC_BROWSER_TEST_P(WebNavigationApiPrerenderTestWithContextType, GetFrame) {
 
 IN_PROC_BROWSER_TEST_P(WebNavigationApiPrerenderTestWithContextType,
                        Prerendering) {
+  // TODO(crbug.com/1394910): Use https in the test and remove this allowlist
+  // entry.
+  AllowHttpForHostnamesForTesting({"a.test"}, browser()->profile()->GetPrefs());
+
   ASSERT_TRUE(StartEmbeddedTestServer());
   ASSERT_TRUE(RunExtensionTest("webnavigation/prerendering")) << message_;
 }
@@ -369,6 +371,11 @@ IN_PROC_BROWSER_TEST_P(WebNavigationApiTestWithContextType, MAYBE_Download) {
 
 IN_PROC_BROWSER_TEST_P(WebNavigationApiTestWithContextType,
                        ServerRedirectSingleProcess) {
+  // TODO(crbug.com/1394910): Use https in the test and remove these allowlist
+  // entries.
+  AllowHttpForHostnamesForTesting({"www.a.com", "www.b.com"},
+                                  browser()->profile()->GetPrefs());
+
   ASSERT_TRUE(StartEmbeddedTestServer());
 
   // Set max renderers to 1 to force running out of processes.
@@ -674,6 +681,11 @@ IN_PROC_BROWSER_TEST_P(WebNavigationApiTestWithContextType, PendingDeletion) {
 }
 
 IN_PROC_BROWSER_TEST_P(WebNavigationApiTestWithContextType, Crash) {
+  // TODO(crbug.com/1394910): Use https in the test and remove this allowlist
+  // entry.
+  AllowHttpForHostnamesForTesting({"www.a.com"},
+                                  browser()->profile()->GetPrefs());
+
   content::ScopedAllowRendererCrashes scoped_allow_renderer_crashes;
   ASSERT_TRUE(StartEmbeddedTestServer());
 

chrome/browser/extensions/content_script_apitest.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/extensions/identifiability_metrics_test_util.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/ssl/https_upgrades_interceptor.h"
+#include "chrome/browser/ssl/https_upgrades_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/search/ntp_test_utils.h"
@@ -145,6 +146,15 @@ class ContentScriptApiTest : public ExtensionApiTest {
         https_test_server_->port());
     HttpsUpgradesInterceptor::SetHttpPortForTesting(
         embedded_test_server()->port());
+
+    // Test extensions use these hostnames. Allow them to be loaded over
+    // HTTP so that HTTPS-Upgrades feature doesn't upgrade their URLs.
+    // TODO(crbug.com/1394910): Use https in these tests and remove these
+    // allowlist entries.
+    AllowHttpForHostnamesForTesting(
+        {"a.com", "b.com", "default.test", "bar.com", "path-test.example",
+         "example.com", "chromium.org", "example1.com"},
+        browser()->profile()->GetPrefs());
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -994,13 +1004,8 @@ IN_PROC_BROWSER_TEST_F(ContentScriptApiTest, CannotScriptTheNewTabPage) {
 
   // The extension should inject on "normal" urls.
 
-  // Test on an HTTP URL. Disable HTTPS upgrades on example1.com for this test
-  // to work.
-  auto* prefs = browser()->profile()->GetPrefs();
-  base::Value::List allowlist;
-  allowlist.Append("example1.com");
-  prefs->SetList(prefs::kHttpAllowlist, std::move(allowlist));
-
+  // Test on an HTTP URL. HTTPS upgrades is disabled on example1.com so it
+  // loads over http instead of https. example2.com loads over https.
   GURL unprotected_url1 = embedded_test_server()->GetURL(
       "example1.com", "/extensions/test_file.html");
   ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), unprotected_url1));

chrome/browser/ssl/https_only_mode_browsertest.cc

@@ -56,7 +56,8 @@ class HttpsOnlyModeBrowserTest : public InProcessBrowserTest {
   void SetUp() override {
     feature_list_.InitWithFeatures(
         /*enabled_features=*/{features::kHttpsOnlyMode},
-        /*disabled_features=*/{features::kHttpsFirstModeV2});
+        /*disabled_features=*/{features::kHttpsFirstModeV2,
+                               features::kHttpsUpgrades});
     InProcessBrowserTest::SetUp();
   }
 

chrome/browser/ssl/https_only_mode_upgrade_interceptor.cc

@@ -111,9 +111,8 @@ void HttpsOnlyModeUpgradeInterceptor::MaybeCreateLoader(
   }
 
   // Check if the hostname is in the enterprise policy HTTP allowlist.
-  PrefService* prefs = profile->GetPrefs();
-  if (IsHostnameInAllowlist(tentative_resource_request.url,
-                            prefs->GetList(prefs::kHttpAllowlist))) {
+  if (IsHostnameInHttpAllowlist(tentative_resource_request.url,
+                                profile->GetPrefs())) {
     std::move(callback).Run({});
     return;
   }

chrome/browser/ssl/https_upgrades_browsertest.cc

@@ -77,6 +77,10 @@ class HttpsUpgradesBrowserTest
           /*enabled_features=*/{features::kHttpsFirstModeV2,
                                 features::kHttpsUpgrades},
           /*disabled_features=*/{});
+    } else if (GetParam() == HttpsUpgradesTestType::kNeither) {
+      feature_list_.InitWithFeatures(
+          /*enabled_features=*/{features::kHttpsFirstModeV2},
+          /*disabled_features=*/{features::kHttpsUpgrades});
     } else {
       feature_list_.InitAndEnableFeature(features::kHttpsFirstModeV2);
     }

chrome/browser/ssl/https_upgrades_interceptor.cc

@@ -266,8 +266,8 @@ void HttpsUpgradesInterceptor::MaybeCreateLoaderOnHstsQueryCompleted(
   // Don't upgrade navigation if it is allowlisted.
   // First, check the enterprise policy HTTP allowlist.
   PrefService* prefs = profile->GetPrefs();
-  if (IsHostnameInAllowlist(tentative_resource_request.url,
-                            prefs->GetList(prefs::kHttpAllowlist))) {
+  if (IsHostnameInHttpAllowlist(tentative_resource_request.url,
+                                profile->GetPrefs())) {
     RecordNavigationRequestSecurityLevel(
         NavigationRequestSecurityLevel::kInsecure);
     std::move(callback).Run({});

chrome/browser/ssl/https_upgrades_util.cc

@@ -5,11 +5,15 @@
 #include "chrome/browser/ssl/https_upgrades_util.h"
 
 #include "base/values.h"
+#include "chrome/common/pref_names.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
+#include "components/prefs/pref_service.h"
 #include "url/gurl.h"
 
-bool IsHostnameInAllowlist(const GURL& url,
-                           const base::Value::List& allowed_hosts) {
+bool IsHostnameInHttpAllowlist(const GURL& url, PrefService* prefs) {
+  const base::Value::List& allowed_hosts =
+      prefs->GetList(prefs::kHttpAllowlist);
+
   // Though this is not technically a Content Setting, ContentSettingsPattern
   // aligns better than URLMatcher with the rules from
   // https://chromeenterprise.google/policies/url-patterns/.
@@ -28,3 +32,12 @@ bool IsHostnameInAllowlist(const GURL& url,
   }
   return false;
 }
+
+void AllowHttpForHostnamesForTesting(const std::vector<std::string>& hostnames,
+                                     PrefService* prefs) {
+  base::Value::List allowed_hosts;
+  for (const std::string& hostname : hostnames) {
+    allowed_hosts.Append(hostname);
+  }
+  prefs->SetList(prefs::kHttpAllowlist, std::move(allowed_hosts));
+}

chrome/browser/ssl/https_upgrades_util.h

@@ -8,14 +8,19 @@
 #include "base/values.h"
 #include "url/gurl.h"
 
+class PrefService;
+
 // Helper for applying the HttpAllowlist enterprise policy. Checks if the
 // hostname of `url` matches any of the hostnames or hostname patterns in the
 // `allowed_hosts` list. Does not allow blanket host wildcards (i.e., "*" which
 // matches all hosts), but does allow partial domain wildcards (e.g.,
 // "[*.]example.com"). Entries in `allowed_hosts` should follow the rules in
 // https://chromeenterprise.google/policies/url-patterns/ (or they'll be
 // ignored).
-bool IsHostnameInAllowlist(const GURL& url,
-                           const base::Value::List& allowed_hosts);
+bool IsHostnameInHttpAllowlist(const GURL& url, PrefService* prefs);
+
+// Adds `hostnames` to the HttpAllowlist enterprise policy for testing.
+void AllowHttpForHostnamesForTesting(const std::vector<std::string>& hostnames,
+                                     PrefService* prefs);
 
 #endif  // CHROME_BROWSER_SSL_HTTPS_UPGRADES_UTIL_H_

chrome/browser/ssl/security_state_tab_helper_browsertest.cc

@@ -1363,6 +1363,13 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperIncognitoTest, HttpErrorPage) {
       SecurityStateTabHelper::FromWebContents(contents);
   ASSERT_TRUE(helper);
 
+  // Disable HTTPS upgrades on nonexistent.test for this test to work.
+  auto* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
+  auto* prefs = profile->GetPrefs();
+  base::Value::List allowlist;
+  allowlist.Append("nonexistent.test");
+  prefs->SetList(prefs::kHttpAllowlist, std::move(allowlist));
+
   // Navigate to a URL that results in an error page. Even though the displayed
   // URL is http://, there shouldn't be a Not Secure warning because the browser
   // hasn't really navigated to an http:// page.

chrome/browser/ssl/ssl_browsertest.cc

@@ -109,6 +109,7 @@
 #include "components/security_interstitials/content/ssl_error_handler.h"
 #include "components/security_interstitials/content/stateful_ssl_host_state_delegate.h"
 #include "components/security_interstitials/core/controller_client.h"
+#include "components/security_interstitials/core/https_only_mode_metrics.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_interstitials/core/pref_names.h"
 #include "components/security_state/core/security_state.h"
@@ -6457,10 +6458,12 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   ASSERT_TRUE(ui_test_utils::NavigateToURL(
       browser(), https_server_.GetURL(replacement_path)));
+
   WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
   content::TestNavigationObserver nav_observer(tab, 1);
   ASSERT_TRUE(content::ExecuteScript(tab, "submitForm();"));
   nav_observer.Wait();
+
   security_interstitials::SecurityInterstitialTabHelper* helper =
       security_interstitials::SecurityInterstitialTabHelper::FromWebContents(
           tab);
@@ -6469,12 +6472,27 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   // Check this was logged correctly as a redirect mixed form that would not
   // expose form data.
-  histograms.ExpectTotalCount(interstitial_histogram, 1);
+  // Since Https Upgrades are enabled, metrics should be recorded twice, first
+  // for the original HTTP navigation and then for the fallback navigation.
+  histograms.ExpectTotalCount(interstitial_histogram, 2);
   histograms.ExpectBucketCount(
       interstitial_histogram,
       InsecureFormNavigationThrottle::InterstitialTriggeredState::
           kMixedFormRedirectNoFormData,
-      1);
+      2);
+
+  // Also check HTTPS upgrade metrics.
+  histograms.ExpectTotalCount(
+      security_interstitials::https_only_mode::kEventHistogram, 3);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeAttempted, 1);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeFailed, 1);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeNetError, 1);
 }
 
 // Checks no interstitial is shown for mixed forms caused for a POST form with a
@@ -6505,12 +6523,27 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   // Check this was logged correctly as a redirect mixed form that would not
   // expose form data.
-  histograms.ExpectTotalCount(interstitial_histogram, 1);
+  // Since Https Upgrades are enabled, metrics should be recorded twice, first
+  // for the original HTTP navigation and then for the fallback navigation.
+  histograms.ExpectTotalCount(interstitial_histogram, 2);
   histograms.ExpectBucketCount(
       interstitial_histogram,
       InsecureFormNavigationThrottle::InterstitialTriggeredState::
           kMixedFormRedirectNoFormData,
-      1);
+      2);
+
+  // Also check HTTPS upgrade metrics.
+  histograms.ExpectTotalCount(
+      security_interstitials::https_only_mode::kEventHistogram, 3);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeAttempted, 1);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeFailed, 1);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeNetError, 1);
 }
 
 namespace {
@@ -6569,12 +6602,27 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   // Check this was logged correctly as a redirect mixed form that would not
   // expose form data.
-  histograms.ExpectTotalCount(interstitial_histogram, 1);
+  // Since Https Upgrades are enabled, metrics should be recorded twice, first
+  // for the original HTTP navigation and then for the fallback navigation.
+  histograms.ExpectTotalCount(interstitial_histogram, 2);
   histograms.ExpectBucketCount(
       interstitial_histogram,
       InsecureFormNavigationThrottle::InterstitialTriggeredState::
           kMixedFormRedirectNoFormData,
-      1);
+      2);
+
+  // Also check HTTPS upgrade metrics.
+  histograms.ExpectTotalCount(
+      security_interstitials::https_only_mode::kEventHistogram, 3);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeAttempted, 1);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeFailed, 1);
+  histograms.ExpectBucketCount(
+      security_interstitials::https_only_mode::kEventHistogram,
+      security_interstitials::https_only_mode::Event::kUpgradeCertError, 1);
 }
 
 class MixedFormsPolicyTest : public policy::PolicyTest {};

testing/variations/fieldtrial_testing_config.json

@@ -6049,6 +6049,26 @@
             ]
         }
     ],
+    "HttpsUpgrades": [
+        {
+            "platforms": [
+                "android",
+                "windows",
+                "chromeos",
+                "chromeos_lacros",
+                "mac",
+                "linux"
+            ],
+            "experiments": [
+                {
+                    "name": "Enabled",
+                    "enable_features": [
+                        "HttpsUpgrades"
+                    ]
+                }
+            ]
+        }
+    ],
     "IOSAddToHomeScreen": [
         {
             "platforms": [