Show red lock icon on captive portal SSL interstitial

chrome/browser/ssl/captive_portal_blocking_page.cc

 // Copyright 2014 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/captive_portal_blocking_page.h"
 
 #include <utility>
 
 #include "base/i18n/rtl.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
 #include "chrome/browser/interstitials/chrome_controller_client.h"
 #include "chrome/browser/interstitials/chrome_metrics_helper.h"
 #include "chrome/browser/ssl/cert_report_helper.h"
 #include "chrome/browser/ssl/ssl_cert_reporter.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/captive_portal/captive_portal_detector.h"
 #include "components/certificate_reporting/error_reporter.h"
 #include "components/security_interstitials/core/common_string_util.h"
 #include "components/security_interstitials/core/controller_client.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/url_formatter/url_formatter.h"
 #include "components/wifi/wifi_service.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/ssl_status.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/network_interfaces.h"
 #include "net/ssl/ssl_info.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace {
 
 const char kMetricsName[] = "captive_portal";
 
@@ skipping 31 matching lines @@
     content::WebContents* web_contents,
     const GURL& request_url,
     const GURL& login_url,
     std::unique_ptr<SSLCertReporter> ssl_cert_reporter,
     const net::SSLInfo& ssl_info,
     const base::Callback<void(content::CertificateRequestResultType)>& callback)
     : SecurityInterstitialPage(web_contents,
                                request_url,
                                CreateMetricsHelper(web_contents, request_url)),
       login_url_(login_url),
+      ssl_info_(ssl_info),
       callback_(callback) {
   DCHECK(login_url_.is_valid());
 
   if (ssl_cert_reporter) {
     cert_report_helper_.reset(new CertReportHelper(
         std::move(ssl_cert_reporter), web_contents, request_url, ssl_info,
         certificate_reporting::ErrorReport::INTERSTITIAL_CAPTIVE_PORTAL, false,
         nullptr));
   }
 
@@ skipping 143 matching lines @@
     case security_interstitials::CMD_TEXT_FOUND:
     case security_interstitials::CMD_TEXT_NOT_FOUND:
       // Commands are for testing.
       break;
     default:
       NOTREACHED() << "Command " << cmd
                    << " isn't handled by the captive portal interstitial.";
   }
 }
 
+void CaptivePortalBlockingPage::OverrideEntry(content::NavigationEntry* entry) {
+  entry->GetSSL() = content::SSLStatus(
+      content::SECURITY_STYLE_AUTHENTICATION_BROKEN, ssl_info_.cert, ssl_info_);
+}

[meacer, 2016/10/10 19:14:43]

Is it possible to push this call to the superclass? Safebrowsing also uses
SecurtyInterstitial, but I think we use a strikethrough for those as well?

[estark, 2016/10/10 21:41:38]

That's a surprisingly hard question to answer. :) We do show the red/dangerous
icon on safe browsing interstitials, but it's not implemented via
OverrideEntry(). Instead, it's implemented in
SecurityStateModel/ChromeSecurityStateModelClient (see
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...).

However, this whole OverrideEntry() thing is kind of weird and I don't see that
it's necessary at all. Instead of calling out to
InterstitialPageDelegate::OverrideEntry() from InterstitialPageImpl::Show(), we
should be able to just directly set the SSLStatus on the transient
NavigationEntry like we would for a normal navigation entry. I think we can set
it to content::SSLStatus(GetSecurityStyleForResource(...), ssl_info.cert,
ssl_info) -- no need to hardcode it to SECURITY_STYLE_AUTHENTICATION_BROKEN for
all interstitials, and no need to call out to the embedder to set it in a
special way.

WDYT of that idea? If you're on board, I'll do it in a separate CL (to separate
the bug fix from the refactoring.)

[meacer, 2016/10/10 22:08:50]

Sounds great! Thanks for looking into this.

+
 void CaptivePortalBlockingPage::OnProceed() {
   NOTREACHED()
       << "Cannot proceed through the error on a captive portal interstitial.";
 }
 
 void CaptivePortalBlockingPage::OnDontProceed() {
   if (cert_report_helper_) {
     // Finish collecting information about invalid certificates, if the
     // user opted in to.
     cert_report_helper_->FinishCertCollection(
         certificate_reporting::ErrorReport::USER_DID_NOT_PROCEED);
   }
 
   // Need to explicity deny the certificate via the callback, otherwise memory
   // is leaked.
   if (!callback_.is_null()) {
     callback_.Run(content::CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL);
     callback_.Reset();
   }
 }