Show red lock icon on captive portal SSL interstitial

chrome/browser/ssl/captive_portal_blocking_page.cc

Index: chrome/browser/ssl/captive_portal_blocking_page.cc
diff --git a/chrome/browser/ssl/captive_portal_blocking_page.cc b/chrome/browser/ssl/captive_portal_blocking_page.cc
index 7ec48333216851924003c9ded1a00717cdffc395..5ec37bd825e2a14317ee65eccf9e6753f41dcc4d 100644
--- a/chrome/browser/ssl/captive_portal_blocking_page.cc
+++ b/chrome/browser/ssl/captive_portal_blocking_page.cc
@@ -27,6 +27,8 @@
 #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"
@@ -78,6 +80,7 @@ CaptivePortalBlockingPage::CaptivePortalBlockingPage(
                                request_url,
                                CreateMetricsHelper(web_contents, request_url)),
       login_url_(login_url),
+      ssl_info_(ssl_info),
       callback_(callback) {
   DCHECK(login_url_.is_valid());
 
@@ -241,6 +244,11 @@ void CaptivePortalBlockingPage::CommandReceived(const std::string& command) {
   }
 }
 
+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.";