Remember user decisions on invalid certificates behind a flag

content/browser/ssl/ssl_host_state.cc

Index: content/browser/ssl/ssl_host_state.cc
diff --git a/content/browser/ssl/ssl_host_state.cc b/content/browser/ssl/ssl_host_state.cc
index 06c600205fa8b1277b5252e91cbbf56580320c21..6820a7d30da6d52c9866a974753abd6d0ca08123 100644
--- a/content/browser/ssl/ssl_host_state.cc
+++ b/content/browser/ssl/ssl_host_state.cc
@@ -7,6 +7,10 @@
 #include "base/logging.h"
 #include "base/lazy_instance.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/ssl_host_state_delegate.h"
+#include "net/http/http_transaction_factory.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 
 const char kKeyName[] = "content_ssl_host_state";
 
@@ -16,7 +20,10 @@ SSLHostState* SSLHostState::GetFor(BrowserContext* context) {
   SSLHostState* rv = static_cast<SSLHostState*>(context->GetUserData(kKeyName));
   if (!rv) {
     rv = new SSLHostState();
-    context->SetUserData(kKeyName, rv);
+    rv->delegate_ = context->GetSSLHostStateDelegate();
+    // |context| may be NULL, implementing the default storage strategy.
+    if (context)

[benm (inactive), 2014/12/10 15:01:42]

should this check go up above? If context might be null, we are blindly derefing
it on line 23, right? Should this be rv->delegate we check?

[jww, 2014/12/10 23:20:06]

Good catch, although this code was significantly refactored in
https://codereview.chromium.org/473643002 and this particular path doesn't exist
anymore.

+      context->SetUserData(kKeyName, rv);
   }
   return rv;
 }
@@ -43,7 +50,10 @@ void SSLHostState::DenyCertForHost(net::X509Certificate* cert,
                                    net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_[host].Deny(cert, error);
+  if (!delegate_)
+    return;
+
+  delegate_->DenyCert(host, cert, error);
 }
 
 void SSLHostState::AllowCertForHost(net::X509Certificate* cert,
@@ -51,13 +61,47 @@ void SSLHostState::AllowCertForHost(net::X509Certificate* cert,
                                     net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_[host].Allow(cert, error);
+  if (!delegate_)
+    return;
+
+  delegate_->AllowCert(host, cert, error);
 }
 
-void SSLHostState::Clear() {
+void SSLHostState::RevokeAllowAndDenyPreferences(const std::string& host) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_.clear();
+  if (!delegate_)
+    return;
+
+  // TODO(jww): This will revoke all of the decisions in the browser context.
+  // However, the networking stack actually keeps track of its own list of
+  // exceptions per-HttpNetworkTransaction in the SSLConfig structure (see the
+  // allowed_bad_certs Vector in net/ssl/ssl_config.h). This dual-tracking of
+  // exceptions introduces a problem where the browser context can revoke a
+  // certificate, but if a transaction reuses a cached version of the SSLConfig
+  // (probably from a pooled socket), it may bypass the intestitial layer.
+  //
+  // Over time, the cached versions should expire and it should converge on
+  // showing the interstitial. We probably need to
+  // introduce into the networking stack a way revoke SSLConfig's
+  // allowed_bad_certs lists per socket.
+  delegate_->RevokeAllowAndDenyPreferences(host);
+}
+
+bool SSLHostState::HasAllowedOrDeniedCert(const std::string& host) {
+  DCHECK(CalledOnValidThread());
+
+  if (!delegate_)
+    return false;
+
+  return delegate_->HasAllowedOrDeniedCert(host);
+}
+
+void SSLHostState::Clear() {
+  if (!delegate_)
+    return;
+
+  delegate_->Clear();
 }
 
 net::CertPolicy::Judgment SSLHostState::QueryPolicy(net::X509Certificate* cert,
@@ -65,7 +109,10 @@ net::CertPolicy::Judgment SSLHostState::QueryPolicy(net::X509Certificate* cert,
                                                     net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  return cert_policy_for_host_[host].Check(cert, error);
+  if (!delegate_)
+    return net::CertPolicy::Judgment::UNKNOWN;
+
+  return delegate_->QueryPolicy(host, cert, error);
 }
 
 }  // namespace content