Downgrade to insecure styling in DevTools for non-secure connections

Security UX is experimenting with using a grey triangle warning for
non-secure site connections (crbug.com/997972). When this icon is
shown, DevTools Security panel's security overview should match the
state shown in the omnibox.

crrev.com/c/1857047 swaps out the info icon for a grey triangle
for non-secure connections, so this cl uses the same logic
(factored out into a helper) to determine whether pages with
the NONE and WARNING security states should send Insecure or
Neutral over the DevTools protocol.

crrev.com/c/1881220 (DevTools frontend) should be submitted first.

Bug: 1008218
Change-Id: I524fc43bae2d86bbc5fbe394948c3c29cda21bee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1877653Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Livvie Lin <livvielin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710859}

chrome/browser/devtools/protocol/security_handler.cc

@@ -37,10 +37,15 @@ const char kIsErrorPageSecurityStateIssueId[] = "is-error-page";
 const char kInsecureInputEventsSecurityStateIssueId[] = "insecure-input-events";
 
 std::string SecurityLevelToProtocolSecurityState(
-    security_state::SecurityLevel security_level) {
+    security_state::SecurityLevel security_level,
+    GURL url) {
   switch (security_level) {
     case security_state::NONE:
     case security_state::WARNING:
+      if (security_state::ShouldDowngradeNeutralStyling(
+              security_level, url,
+              base::BindRepeating(&content::IsOriginSecure)))
+        return protocol::Security::SecurityStateEnum::Insecure;
       return protocol::Security::SecurityStateEnum::Neutral;
     case security_state::SECURE_WITH_POLICY_INSTALLED_CERT:
     case security_state::EV_SECURE:
@@ -146,8 +151,8 @@ CreateVisibleSecurityState(const security_state::VisibleSecurityState& state,
   SecurityStateTabHelper* helper =
       SecurityStateTabHelper::FromWebContents(web_contents);
   DCHECK(helper);
-  std::string security_state =
-      SecurityLevelToProtocolSecurityState(helper->GetSecurityLevel());
+  std::string security_state = SecurityLevelToProtocolSecurityState(
+      helper->GetSecurityLevel(), state.url);
 
   bool scheme_is_cryptographic =
       security_state::IsSchemeCryptographic(state.url);

components/omnibox/browser/location_bar_model_impl.cc

@@ -18,7 +18,6 @@
 #include "components/omnibox/common/omnibox_features.h"
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/template_url_service.h"
-#include "components/security_state/core/features.h"
 #include "components/security_state/core/security_state.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/common/origin_util.h"
@@ -181,36 +180,30 @@ LocationBarModelImpl::GetPageClassification(OmniboxFocusSource focus_source) {
 const gfx::VectorIcon& LocationBarModelImpl::GetVectorIcon() const {
 #if (!defined(OS_ANDROID) || BUILDFLAG(ENABLE_VR)) && !defined(OS_IOS)
   auto* const icon_override = delegate_->GetVectorIconOverride();
-  GURL url = GetURL();
-  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;
-    }
-  }
   if (icon_override)
     return *icon_override;
 
   if (IsOfflinePage())
     return omnibox::kOfflinePinIcon;
 
-  switch (GetSecurityLevel()) {
+  GURL url = GetURL();
+  security_state::SecurityLevel security_level = GetSecurityLevel();
+  switch (security_level) {
     case security_state::NONE:
       // Show a danger triangle icon on HTTPS pages with passive mixed content
       // when kMarkHttpAsParameterDangerWarning is enabled.
-      if (http_danger_warning_enabled && url.SchemeIsCryptographic()) {
+      if (security_state::ShouldDowngradeNeutralStyling(
+              security_level, url,
+              base::BindRepeating(&content::IsOriginSecure))) {
         return omnibox::kNotSecureWarningIcon;
       }
       return omnibox::kHttpIcon;
     case security_state::WARNING:
       // When kMarkHttpAsParameterDangerWarning is enabled, show a danger
       // triangle icon unless the page has a non-HTTPS secure origin.
-      if (http_danger_warning_enabled && !content::IsOriginSecure(url)) {
+      if (security_state::ShouldDowngradeNeutralStyling(
+              security_level, url,
+              base::BindRepeating(&content::IsOriginSecure))) {
         return omnibox::kNotSecureWarningIcon;
       }
       return omnibox::kHttpIcon;

components/security_state/content/content_utils.cc

@@ -39,10 +39,15 @@ namespace {
 // Note: This is a lossy operation. Not all of the policies that can be
 // expressed by a SecurityLevel can be expressed by a blink::SecurityStyle.
 blink::SecurityStyle SecurityLevelToSecurityStyle(
-    security_state::SecurityLevel security_level) {
+    security_state::SecurityLevel security_level,
+    GURL url) {
   switch (security_level) {
     case security_state::NONE:
     case security_state::WARNING:
+      if (security_state::ShouldDowngradeNeutralStyling(
+              security_level, url,
+              base::BindRepeating(&content::IsOriginSecure)))
+        return blink::SecurityStyle::kInsecure;
       return blink::SecurityStyle::kNeutral;
     case security_state::SECURE_WITH_POLICY_INSTALLED_CERT:
     case security_state::EV_SECURE:
@@ -468,7 +473,7 @@ blink::SecurityStyle GetSecurityStyle(
     const security_state::VisibleSecurityState& visible_security_state,
     content::SecurityStyleExplanations* security_style_explanations) {
   const blink::SecurityStyle security_style =
-      SecurityLevelToSecurityStyle(security_level);
+      SecurityLevelToSecurityStyle(security_level, visible_security_state.url);
 
   // Safety tips come after SafeBrowsing but before HTTP warnings.
   // ExplainSafeBrowsingSecurity always inserts warnings to the front, so

components/security_state/core/security_state.cc

@@ -320,4 +320,56 @@ bool IsSHA1InChain(const VisibleSecurityState& visible_security_state) {
           net::CERT_STATUS_SHA1_SIGNATURE_PRESENT);
 }
 
+// As an experiment, the info icon should be downgraded to a grey triangle for
+// non-secure connections (crbug.com/997972). This method helps distinguish
+// between cases where the NONE and WARNING security levels should map to
+// neutral (info) or insecure (triangle) for styling purposes.
+//
+// TODO(crbug.com/1015626): Clean this up once the experiment is fully
+// launched and security states refactored.
+bool ShouldDowngradeNeutralStyling(
+    security_state::SecurityLevel security_level,
+    GURL url,
+    IsOriginSecureCallback is_origin_secure_callback) {
+  // This method is only relevant if the info icon is shown, which only occurs
+  // for NONE and WARNING security states.
+  if (security_level != security_state::NONE &&
+      security_level != security_state::WARNING) {
+    return false;
+  }
+
+  // The state should not be downgraded unless the grey triangle experiment
+  // is enabled.
+  bool http_danger_warning_enabled = false;
+  if (base::FeatureList::IsEnabled(features::kMarkHttpAsFeature)) {
+    std::string parameter = base::GetFieldTrialParamValueByFeature(
+        features::kMarkHttpAsFeature,
+        features::kMarkHttpAsFeatureParameterName);
+    if (parameter ==
+        security_state::features::kMarkHttpAsParameterDangerWarning) {
+      http_danger_warning_enabled = true;
+    }
+  }
+  if (!http_danger_warning_enabled)
+    return false;
+
+  bool scheme_is_cryptographic = security_state::IsSchemeCryptographic(url);
+  bool origin_is_secure = scheme_is_cryptographic;
+  if (!scheme_is_cryptographic)
+    origin_is_secure = is_origin_secure_callback.Run(url);
+
+  // The grey danger triangle should be shown on HTTPS pages with passive
+  // mixed content. These pages currently use the NONE security state, but
+  // this is undergoing a refactor.
+  if (security_level == security_state::NONE && scheme_is_cryptographic)
+    return true;
+
+  // The info icon should be used on non-HTTPS secure origins, but other
+  // WARNING states should use they grey danger triangle.
+  if (security_level == security_state::WARNING && !origin_is_secure)
+    return true;
+
+  return false;
+}
+
 }  // namespace security_state

components/security_state/core/security_state.h

@@ -257,6 +257,14 @@ std::string GetLegacyTLSHistogramName(
 
 bool IsSHA1InChain(const VisibleSecurityState& visible_security_state);
 
+// Returns whether the NONE or WARNING state should downgrade styling from
+// neutral to insecure as part of an experiment to mark non-secure
+// connections with a grey triangle icon (crbug.com/997972).
+bool ShouldDowngradeNeutralStyling(
+    security_state::SecurityLevel security_level,
+    GURL url,
+    IsOriginSecureCallback is_origin_secure_callback);
+
 }  // namespace security_state
 
 #endif  // COMPONENTS_SECURITY_STATE_CORE_SECURITY_STATE_H_

components/security_state/core/security_state_unittest.cc

@@ -560,4 +560,35 @@ TEST(SecurityStateTest, MajorCertificateErrors) {
   EXPECT_TRUE(helper.HasMajorCertificateError());
 }
 
+// Tests that ShouldDowngradeNeutralStyling properly returns whether the
+// neutral styling should be downgraded to insecure styling.
+TEST(SecurityStateTest, ShouldDowngradeNeutralStyling) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      security_state::features::kMarkHttpAsFeature,
+      {{security_state::features::kMarkHttpAsFeatureParameterName,
+        security_state::features::kMarkHttpAsParameterDangerWarning}});
+
+  // Returns false when security state is something other than NONE or WARNING.
+  EXPECT_FALSE(security_state::ShouldDowngradeNeutralStyling(
+      security_state::SECURE, GURL("https://scheme-is-cryptographic.test"),
+      base::BindRepeating(&IsOriginSecure)));
+  EXPECT_FALSE(security_state::ShouldDowngradeNeutralStyling(
+      security_state::DANGEROUS, GURL("https://scheme-is-cryptographic.test"),
+      base::BindRepeating(&IsOriginSecure)));
+
+  // Returns false for non-cryptographic secure origins.
+  EXPECT_FALSE(security_state::ShouldDowngradeNeutralStyling(
+      security_state::NONE, GURL("chrome://test"),
+      base::BindRepeating(&IsOriginSecure)));
+
+  // Returns true for HTTP and some mixed content.
+  EXPECT_TRUE(security_state::ShouldDowngradeNeutralStyling(
+      security_state::WARNING, GURL("http://scheme-is-not-cryptographic.test"),
+      base::BindRepeating(&IsOriginSecure)));
+  EXPECT_TRUE(security_state::ShouldDowngradeNeutralStyling(
+      security_state::NONE, GURL("https://mixed-content.test"),
+      base::BindRepeating(&IsOriginSecure)));
+}
+
 }  // namespace security_state