Enable HTTPDangerWarning by default

Remove testing config and update tests to check for new default
behavior.

Launch bug: crbug.com/997972

Change-Id: I25672fa149cd73ee32ada35b6a374d0bcac47db4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2367742
Commit-Queue: Livvie Lin <livvielin@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802316}

chrome/browser/ssl/security_state_tab_helper_browsertest.cc

@@ -187,22 +187,6 @@ security_state::SecurityLevel GetLevelForPassiveMixedContent() {
              : security_state::NONE;
 }
 
-blink::SecurityStyle GetSecurityStyleForHttp() {
-  bool http_danger_warning_enabled = false;
-  if (base::FeatureList::IsEnabled(
-          security_state::features::kMarkHttpAsFeature)) {
-    std::string parameter = base::GetFieldTrialParamValueByFeature(
-        security_state::features::kMarkHttpAsFeature,
-        security_state::features::kMarkHttpAsFeatureParameterName);
-    if (parameter ==
-        security_state::features::kMarkHttpAsParameterDangerWarning) {
-      http_danger_warning_enabled = true;
-    }
-  }
-  return http_danger_warning_enabled ? blink::SecurityStyle::kInsecure
-                                     : blink::SecurityStyle::kNeutral;
-}
-
 // A delegate class that allows emulating selection of a file for an
 // INPUT TYPE=FILE form field.
 class FileChooserDelegate : public content::WebContentsDelegate {
@@ -1129,9 +1113,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest, SecurityStyleForHttpPage) {
   ui_test_utils::NavigateToURL(browser(), http_url);
   EXPECT_EQ(security_state::WARNING, helper->GetSecurityLevel());
   EXPECT_EQ(0u, observer.latest_explanations().neutral_explanations.size());
-  // The below expectation is based on the feature flag set in the field trial
-  // testing config.
-  EXPECT_EQ(GetSecurityStyleForHttp(), observer.latest_security_style());
+  EXPECT_EQ(blink::SecurityStyle::kInsecure, observer.latest_security_style());
 
   content::NavigationEntry* entry = contents->GetController().GetVisibleEntry();
   ASSERT_TRUE(entry);
@@ -1158,9 +1140,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
   // Ensure that WebContentsObservers don't show an incorrect Form Not Secure
   // explanation. Regression test for https://crbug.com/691412.
   EXPECT_EQ(0u, observer.latest_explanations().neutral_explanations.size());
-  // The below expectation is based on the feature flag set in the field trial
-  // testing config.
-  EXPECT_EQ(GetSecurityStyleForHttp(), observer.latest_security_style());
+  EXPECT_EQ(blink::SecurityStyle::kInsecure, observer.latest_security_style());
 
   content::NavigationEntry* entry = contents->GetController().GetVisibleEntry();
   ASSERT_TRUE(entry);
@@ -1296,9 +1276,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithFtpEnabled,
   // Ensure that WebContentsObservers don't show an incorrect Form Not Secure
   // explanation. Regression test for https://crbug.com/691412.
   EXPECT_EQ(0u, observer.latest_explanations().neutral_explanations.size());
-  // The below expectation is based on the feature flag set in the field trial
-  // testing config.
-  EXPECT_EQ(GetSecurityStyleForHttp(), observer.latest_security_style());
+  EXPECT_EQ(blink::SecurityStyle::kInsecure, observer.latest_security_style());
 
   content::NavigationEntry* entry = contents->GetController().GetVisibleEntry();
   ASSERT_TRUE(entry);
@@ -1523,22 +1501,10 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithFormsDangerous,
   ASSERT_EQ(security_state::WARNING, helper->GetSecurityLevel());
 }
 
-class SecurityStateTabHelperTestWithHttpWarningsDisabled
-    : public SecurityStateTabHelperTest {
- public:
-  SecurityStateTabHelperTestWithHttpWarningsDisabled() {
-    feature_list_.InitAndDisableFeature(
-        security_state::features::kMarkHttpAsFeature);
-  }
-
- private:
-  base::test::ScopedFeatureList feature_list_;
-};
-
 // Tests that the security level of a HTTP page is downgraded from
 // WARNING to DANGEROUS after editing a form field in the relevant
 // configurations.
-IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithHttpWarningsDisabled,
+IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithFormsDangerous,
                        SecurityLevelDowngradedAfterFileSelection) {
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();

components/omnibox/browser/location_bar_model_impl_unittest.cc

@@ -294,33 +294,8 @@ TEST_F(LocationBarModelImplTest, MAYBE_PreventElisionWorks) {
 }
 
 #if !defined(OS_ANDROID) && !defined(OS_IOS)
-// Tests GetVectorIcon returns the correct security indicator icon when the
-// danger-warning experiment is disabled.
-TEST_F(LocationBarModelImplTest, GetVectorIcon_DefaultWarning) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(
-      security_state::features::kMarkHttpAsFeature);
-
-  delegate()->SetSecurityLevel(security_state::SecurityLevel::WARNING);
-
-  gfx::ImageSkia expected_icon = gfx::CreateVectorIcon(
-      omnibox::kHttpIcon, gfx::kFaviconSize, gfx::kPlaceholderColor);
-
-  gfx::ImageSkia icon = gfx::CreateVectorIcon(
-      model()->GetVectorIcon(), gfx::kFaviconSize, gfx::kPlaceholderColor);
-
-  EXPECT_EQ(icon.bitmap(), expected_icon.bitmap());
-}
-
-// Tests GetVectorIcon returns the correct security indicator icon when the
-// danger-warning experiment is enabled.
-TEST_F(LocationBarModelImplTest, GetVectorIcon_DangerWarning) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeatureWithParameters(
-      security_state::features::kMarkHttpAsFeature,
-      {{security_state::features::kMarkHttpAsFeatureParameterName,
-        security_state::features::kMarkHttpAsParameterDangerWarning}});
-
+// Tests GetVectorIcon returns the correct security indicator icon.
+TEST_F(LocationBarModelImplTest, GetVectorIcon) {
   delegate()->SetSecurityLevel(security_state::SecurityLevel::WARNING);
 
   gfx::ImageSkia expected_icon =

components/security_state/content/content_utils_unittest.cc

@@ -587,14 +587,14 @@ TEST(SecurityStateContentUtilsTest, MixedContentAndCertErrorExplanations) {
 }
 
 // Tests that a security level of WARNING produces
-// blink::kSecurityStyleNeutral.
+// blink::kSecurityStyleInsecure.
 TEST(SecurityStateContentUtilsTest, HTTPWarning) {
   security_state::VisibleSecurityState visible_security_state;
   visible_security_state.url = GURL("http://scheme-is-not-cryptographic.test");
   content::SecurityStyleExplanations explanations;
   blink::SecurityStyle security_style = GetSecurityStyle(
       security_state::WARNING, visible_security_state, &explanations);
-  EXPECT_EQ(blink::SecurityStyle::kNeutral, security_style);
+  EXPECT_EQ(blink::SecurityStyle::kInsecure, security_style);
   // Verify no explanation was shown.
   EXPECT_EQ(0u, explanations.neutral_explanations.size());
 }

components/security_state/core/security_state.cc

@@ -24,24 +24,20 @@ namespace {
 // For nonsecure pages, returns a SecurityLevel based on the
 // provided information and the kMarkHttpAsFeature field trial.
 SecurityLevel GetSecurityLevelForNonSecureFieldTrial(
-    bool is_error_page,
     const InsecureInputEventData& input_events) {
   if (base::FeatureList::IsEnabled(features::kMarkHttpAsFeature)) {
     std::string parameter = base::GetFieldTrialParamValueByFeature(
         features::kMarkHttpAsFeature,
         features::kMarkHttpAsFeatureParameterName);
-
     if (parameter == features::kMarkHttpAsParameterDangerous) {
       return DANGEROUS;
     }
-    if (parameter == features::kMarkHttpAsParameterDangerWarning) {
-      return WARNING;
+    if (parameter ==
+        features::kMarkHttpAsParameterWarningAndDangerousOnFormEdits) {
+      return input_events.insecure_field_edited ? DANGEROUS : WARNING;
     }
   }
-
-  // Default to dangerous on editing form fields and otherwise
-  // warning.
-  return input_events.insecure_field_edited ? DANGEROUS : WARNING;
+  return WARNING;
 }
 
 std::string GetHistogramSuffixForSecurityLevel(
@@ -182,7 +178,6 @@ SecurityLevel GetSecurityLevel(
       }
 #endif  // !defined(OS_ANDROID)
       return GetSecurityLevelForNonSecureFieldTrial(
-          visible_security_state.is_error_page,
           visible_security_state.insecure_input_events);
     }
     return NONE;
@@ -333,10 +328,7 @@ bool IsSHA1InChain(const VisibleSecurityState& visible_security_state) {
 // TODO(crbug.com/1015626): Clean this up once the experiment is fully
 // launched.
 bool ShouldShowDangerTriangleForWarningLevel() {
-  return base::GetFieldTrialParamValueByFeature(
-             features::kMarkHttpAsFeature,
-             features::kMarkHttpAsFeatureParameterName) ==
-         security_state::features::kMarkHttpAsParameterDangerWarning;
+  return true;
 }
 
 }  // namespace security_state

components/security_state/core/security_state_unittest.cc

@@ -316,13 +316,17 @@ TEST(SecurityStateTest, MixedContentWithPolicyCertificate) {
 }
 
 // Tests that WARNING is set on normal http pages but DANGEROUS on
-// form edits with default feature enabled.
+// form edits when kMarkHttpAsFeature is set to lower security state on form
+// edits.
 TEST(SecurityStateTest, WarningAndDangerousOnFormEditsWhenFeatureEnabled) {
   TestSecurityStateHelper helper;
   helper.SetUrl(GURL(kHttpUrl));
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      security_state::features::kMarkHttpAsFeature);
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      security_state::features::kMarkHttpAsFeature,
+      {{security_state::features::kMarkHttpAsFeatureParameterName,
+        security_state::features::
+            kMarkHttpAsParameterWarningAndDangerousOnFormEdits}});
 
   EXPECT_EQ(security_state::WARNING, helper.GetSecurityLevel());
 
@@ -330,32 +334,31 @@ TEST(SecurityStateTest, WarningAndDangerousOnFormEditsWhenFeatureEnabled) {
   EXPECT_EQ(DANGEROUS, helper.GetSecurityLevel());
 }
 
-// Tests that WARNING is set on normal http pages but DANGEROUS on
-// form edits with default feature disabled.
-TEST(SecurityStateTest, WarningAndDangerousOnFormEditsWhenFeatureDisabled) {
+// Tests that WARNING is set on normal http pages regardless of form edits with
+// default feature enabled.
+TEST(SecurityStateTest,
+     AlwaysWarningWhenFeatureMarksWithTriangleWarningAndFeatureEnabled) {
   TestSecurityStateHelper helper;
   helper.SetUrl(GURL(kHttpUrl));
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(
+  scoped_feature_list.InitAndEnableFeature(
       security_state::features::kMarkHttpAsFeature);
 
   EXPECT_EQ(WARNING, helper.GetSecurityLevel());
 
   helper.set_insecure_field_edit(true);
-  EXPECT_EQ(DANGEROUS, helper.GetSecurityLevel());
+  EXPECT_EQ(WARNING, helper.GetSecurityLevel());
 }
 
-// Tests that WARNING is set on normal http pages regardless of form edits
-// when kMarkHttpAsFeature is set to mark non-secure connections with grey
-// triangle icon.
-TEST(SecurityStateTest, AlwaysWarningWhenFeatureMarksWithTriangleWarning) {
+// Tests that WARNING is set on normal http pages regardless of form edits with
+// default feature disabled.
+TEST(SecurityStateTest,
+     AlwaysWarningWhenFeatureMarksWithTriangleWarningAndFeatureDisabled) {
   TestSecurityStateHelper helper;
   helper.SetUrl(GURL(kHttpUrl));
   base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeatureWithParameters(
-      security_state::features::kMarkHttpAsFeature,
-      {{security_state::features::kMarkHttpAsFeatureParameterName,
-        security_state::features::kMarkHttpAsParameterDangerWarning}});
+  scoped_feature_list.InitAndDisableFeature(
+      security_state::features::kMarkHttpAsFeature);
 
   EXPECT_EQ(WARNING, helper.GetSecurityLevel());
 

testing/variations/fieldtrial_testing_config.json

@@ -2988,30 +2988,6 @@
             ]
         }
     ],
-    "HTTPDangerWarning": [
-        {
-            "platforms": [
-                "android",
-                "android_weblayer",
-                "chromeos",
-                "ios",
-                "linux",
-                "mac",
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "enabled",
-                    "params": {
-                        "treatment": "danger-warning"
-                    },
-                    "enable_features": [
-                        "MarkHttpAs"
-                    ]
-                }
-            ]
-        }
-    ],
     "HappinessTrackingSurveysForDesktopSettings": [
         {
             "platforms": [