Revert "Enable fieldtrial testing config for HttpsUpgrades"

This reverts commit a1ec292. Reason for revert: Suspected cause of failures in SSLUITest on linux-chromeos-chrome: https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/31256/overview Original change's description: > 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} Bug: 1394910 Change-Id: Id055e29ba1fd451ae32cf4eb492600720fc41758 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4351849 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Tim Sergeant <tsergeant@chromium.org> Commit-Queue: Tim Sergeant <tsergeant@chromium.org> Cr-Commit-Position: refs/heads/main@{#1119137}
chromium · Mar 19, 2023 · 367ad16 · 367ad16
1 parent 1a79da6
commit 367ad16
Show file tree

Hide file tree

Showing 11 changed files with 26 additions and 140 deletions.
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
@@ -26,7 +26,6 @@
 #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"
@@ -282,6 +281,9 @@ class WebNavigationApiPrerenderTestWithContextType
       const WebNavigationApiPrerenderTestWithContextType&) = delete;
   WebNavigationApiPrerenderTestWithContextType& operator=(
       const WebNavigationApiPrerenderTestWithContextType&) = delete;
+
+ private:
+  content::test::ScopedPrerenderFeatureList prerender_feature_list_;
 };
 
 IN_PROC_BROWSER_TEST_P(WebNavigationApiTestWithContextType, Api) {
@@ -301,10 +303,6 @@ 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_;
 }
@@ -371,11 +369,6 @@ 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.
@@ -681,11 +674,6 @@ 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());
 

diff --git a/chrome/browser/extensions/content_script_apitest.cc b/chrome/browser/extensions/content_script_apitest.cc
@@ -23,7 +23,6 @@
 #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"
@@ -146,15 +145,6 @@ 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 {
@@ -1004,8 +994,13 @@ IN_PROC_BROWSER_TEST_F(ContentScriptApiTest, CannotScriptTheNewTabPage) {
 
   // The extension should inject on "normal" urls.
 
-  // 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.
+  // 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));
+
   GURL unprotected_url1 = embedded_test_server()->GetURL(
       "example1.com", "/extensions/test_file.html");
   ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), unprotected_url1));

diff --git a/chrome/browser/ssl/https_only_mode_browsertest.cc b/chrome/browser/ssl/https_only_mode_browsertest.cc
@@ -56,8 +56,7 @@ class HttpsOnlyModeBrowserTest : public InProcessBrowserTest {
   void SetUp() override {
     feature_list_.InitWithFeatures(
         /*enabled_features=*/{features::kHttpsOnlyMode},
-        /*disabled_features=*/{features::kHttpsFirstModeV2,
-                               features::kHttpsUpgrades});
+        /*disabled_features=*/{features::kHttpsFirstModeV2});
     InProcessBrowserTest::SetUp();
   }
 

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

diff --git a/chrome/browser/ssl/https_upgrades_browsertest.cc b/chrome/browser/ssl/https_upgrades_browsertest.cc
@@ -77,10 +77,6 @@ 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);
     }

diff --git a/chrome/browser/ssl/https_upgrades_interceptor.cc b/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 (IsHostnameInHttpAllowlist(tentative_resource_request.url,
-                                profile->GetPrefs())) {
+  if (IsHostnameInAllowlist(tentative_resource_request.url,
+                            prefs->GetList(prefs::kHttpAllowlist))) {
     RecordNavigationRequestSecurityLevel(
         NavigationRequestSecurityLevel::kInsecure);
     std::move(callback).Run({});

diff --git a/chrome/browser/ssl/https_upgrades_util.cc b/chrome/browser/ssl/https_upgrades_util.cc
@@ -5,15 +5,11 @@
 #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 IsHostnameInHttpAllowlist(const GURL& url, PrefService* prefs) {
-  const base::Value::List& allowed_hosts =
-      prefs->GetList(prefs::kHttpAllowlist);
-
+bool IsHostnameInAllowlist(const GURL& url,
+                           const base::Value::List& allowed_hosts) {
   // Though this is not technically a Content Setting, ContentSettingsPattern
   // aligns better than URLMatcher with the rules from
   // https://chromeenterprise.google/policies/url-patterns/.
@@ -32,12 +28,3 @@ bool IsHostnameInHttpAllowlist(const GURL& url, PrefService* prefs) {
   }
   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));
-}
diff --git a/chrome/browser/ssl/https_upgrades_util.h b/chrome/browser/ssl/https_upgrades_util.h
@@ -8,19 +8,14 @@
 #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 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);
+bool IsHostnameInAllowlist(const GURL& url,
+                           const base::Value::List& allowed_hosts);
 
 #endif  // CHROME_BROWSER_SSL_HTTPS_UPGRADES_UTIL_H_
diff --git a/chrome/browser/ssl/security_state_tab_helper_browsertest.cc b/chrome/browser/ssl/security_state_tab_helper_browsertest.cc
@@ -1363,13 +1363,6 @@ 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.

diff --git a/chrome/browser/ssl/ssl_browsertest.cc b/chrome/browser/ssl/ssl_browsertest.cc
@@ -109,7 +109,6 @@
 #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"
@@ -6458,12 +6457,10 @@ 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);
@@ -6472,27 +6469,12 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   // Check this was logged correctly as a redirect mixed form that would not
   // expose form data.
-  // 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.ExpectTotalCount(interstitial_histogram, 1);
   histograms.ExpectBucketCount(
       interstitial_histogram,
       InsecureFormNavigationThrottle::InterstitialTriggeredState::
           kMixedFormRedirectNoFormData,
-      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);
+      1);
 }
 
 // Checks no interstitial is shown for mixed forms caused for a POST form with a
@@ -6523,27 +6505,12 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   // Check this was logged correctly as a redirect mixed form that would not
   // expose form data.
-  // 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.ExpectTotalCount(interstitial_histogram, 1);
   histograms.ExpectBucketCount(
       interstitial_histogram,
       InsecureFormNavigationThrottle::InterstitialTriggeredState::
           kMixedFormRedirectNoFormData,
-      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);
+      1);
 }
 
 namespace {
@@ -6602,27 +6569,12 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
 
   // Check this was logged correctly as a redirect mixed form that would not
   // expose form data.
-  // 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.ExpectTotalCount(interstitial_histogram, 1);
   histograms.ExpectBucketCount(
       interstitial_histogram,
       InsecureFormNavigationThrottle::InterstitialTriggeredState::
           kMixedFormRedirectNoFormData,
-      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);
+      1);
 }
 
 class MixedFormsPolicyTest : public policy::PolicyTest {};

diff --git a/testing/variations/fieldtrial_testing_config.json b/testing/variations/fieldtrial_testing_config.json
@@ -6049,26 +6049,6 @@
             ]
         }
     ],
-    "HttpsUpgrades": [
-        {
-            "platforms": [
-                "android",
-                "windows",
-                "chromeos",
-                "chromeos_lacros",
-                "mac",
-                "linux"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "HttpsUpgrades"
-                    ]
-                }
-            ]
-        }
-    ],
     "IOSAddToHomeScreen": [
         {
             "platforms": [