DevTools security panel: explain subresources with cert errors separately

chrome/browser/ssl/chrome_security_state_model_client.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ssl/chrome_security_state_model_client.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
@@ skipping 186 matching lines @@
             security_info.cert_id));
   } else if (security_info.sha1_deprecation_status ==
              SecurityStateModel::DEPRECATED_SHA1_MINOR) {
     security_style_explanations->unauthenticated_explanations.push_back(
         content::SecurityStyleExplanation(
             l10n_util::GetStringUTF8(IDS_MINOR_SHA1),
             l10n_util::GetStringUTF8(IDS_MINOR_SHA1_DESCRIPTION),
             security_info.cert_id));
   }
 
-  security_style_explanations->ran_insecure_content =
+  // Record the presence of mixed content (HTTP subresources on an HTTPS
+  // page).
+  security_style_explanations->ran_mixed_content =
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_RAN ||
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
-  security_style_explanations->displayed_insecure_content =
+  security_style_explanations->displayed_mixed_content =
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_DISPLAYED ||
       security_info.mixed_content_status ==
           SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
 
-  if (net::IsCertStatusError(security_info.cert_status)) {
+  bool is_cert_status_error = net::IsCertStatusError(security_info.cert_status);
+  bool is_cert_status_minor_error =
+      net::IsCertStatusMinorError(security_info.cert_status);
+
+  // Record the presence of content with certificate errors (HTTPS
+  // subresources that were loaded with certificate errors, on an HTTPS
+  // page that was loaded without major certificate errors).
+  if (!is_cert_status_error || is_cert_status_minor_error) {

[felt, 2016/08/30 16:28:52]

should that be an && instead of an ||?

[estark, 2016/08/30 16:36:57]

Nope, I think || is right. |is_cert_status_error| is true if there is any error
(major or minor). So this condition is supposed to be true if there were no cert
errors at all, or if there was only a minor cert error.

[felt, 2016/08/30 16:41:41]

That's what I'm confused about -- you want this to be populated if there were no
cert errors at all? The comment says that this is for recording cert errors,
which doesn't make sense when there are no cert errors. Could you update the
comment?

[estark, 2016/08/30 17:27:58]

Ahhh, ok, I think I understand the confusion now.
ran/displayed_content_with_cert_errors record the presence of subresources with
cert errors, but we only populate those fields if the main resource was loaded
without cert errors. I updated the comment to try to explain this better, lmk if
it makes sense or not.

+    security_style_explanations->ran_content_with_cert_errors =
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_RAN ||
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
+    security_style_explanations->displayed_content_with_cert_errors =
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_DISPLAYED ||
+        security_info.content_with_cert_errors_status ==
+            SecurityStateModel::CONTENT_STATUS_DISPLAYED_AND_RAN;
+  }
+
+  if (is_cert_status_error) {
     base::string16 error_string = base::UTF8ToUTF16(net::ErrorToString(
         net::MapCertStatusToNetError(security_info.cert_status)));
 
     content::SecurityStyleExplanation explanation(
         l10n_util::GetStringUTF8(IDS_CERTIFICATE_CHAIN_ERROR),
         l10n_util::GetStringFUTF8(
             IDS_CERTIFICATE_CHAIN_ERROR_DESCRIPTION_FORMAT, error_string),
         security_info.cert_id);
 
-    if (net::IsCertStatusMinorError(security_info.cert_status))
+    if (is_cert_status_minor_error) {
       security_style_explanations->unauthenticated_explanations.push_back(
           explanation);
-    else
+    } else {
       security_style_explanations->broken_explanations.push_back(explanation);
+    }
   } else {
     // If the certificate does not have errors and is not using
     // deprecated SHA1, then add an explanation that the certificate is
     // valid.
     if (security_info.sha1_deprecation_status ==
         SecurityStateModel::NO_DEPRECATED_SHA1) {
       security_style_explanations->secure_explanations.push_back(
           content::SecurityStyleExplanation(
               l10n_util::GetStringUTF8(IDS_VALID_SERVER_CERTIFICATE),
               l10n_util::GetStringUTF8(
@@ skipping 72 matching lines @@
   state->displayed_mixed_content =
       !!(ssl.content_status & content::SSLStatus::DISPLAYED_INSECURE_CONTENT);
   state->ran_mixed_content =
       !!(ssl.content_status & content::SSLStatus::RAN_INSECURE_CONTENT);
   state->displayed_content_with_cert_errors =
       !!(ssl.content_status &
          content::SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS);
   state->ran_content_with_cert_errors =
       !!(ssl.content_status & content::SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS);
 }