Add certificate error handling to devtools.

content/browser/devtools/protocol/security_handler.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 "content/browser/devtools/protocol/security_handler.h"
 
 #include <string>
 
 #include "content/browser/devtools/devtools_session.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
@@ skipping 128 matching lines @@
           .Build();
 
   frontend_->SecurityStateChanged(
       security_state,
       security_style_explanations.scheme_is_cryptographic,
       std::move(explanations),
       std::move(insecure_status),
       Maybe<std::string>(security_style_explanations.summary));
 }
 
+void SecurityHandler::DidFinishNavigation(NavigationHandle* navigation_handle) {
+  if (certificate_errors_overriden_)
+    FlushPendingRequests();
+}
+
+void SecurityHandler::FlushPendingRequests() {
+  for (auto callback : callbacks_)
+    callback.second.Run(content::CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL);
+  callbacks_.clear();
+}
+
+bool SecurityHandler::NotifyCertificateError(int cert_error,
+                                             const GURL& request_url,
+                                             CertErrorCallback handler) {
+  if (!enabled_)
+    return false;
+  frontend_->CertificateError(++last_cert_error_id_,

[estark, 2017/02/28 23:34:01]

Is it necessary to send the error even if |certificate_errors_overridden_| is
false?

[irisu, 2017/03/06 11:56:48]

@pfeldman earlier commented that the error should be sent if the security domain
is enabled. I don't think it's strictly necessary, although nice to have. It
could be removed - unless Pavel has an objection?

As it is, I can see it may be confusing as to whether a certificateError event
requires handling. If we want to continue sending the event regardless of
whether certificate_errors_overridden is set, then perhaps we should add a bool
to indicate whether handling is required and/or set eventID = -1. WYDT?

[estark, 2017/03/09 00:24:30]

Either way sounds fine to me; I'm not too worried about the specific behavior,
more that the code is a little confusing to read because it's not clear why
we're sending the error if we're not giving devtools any way to do something
about it. So, whatever you decide, please add a comment explaining why it is the
way it is.

[irisu, 2017/03/13 01:56:56]

Done.

+                              net::ErrorToShortString(cert_error),
+                              request_url.spec());
+  if (!certificate_errors_overriden_)
+    return false;
+  callbacks_[last_cert_error_id_] = handler;
+  return true;
+}
+
 Response SecurityHandler::Enable() {
   enabled_ = true;
   if (host_)
     AttachToRenderFrameHost();
 
   return Response::OK();
 }
 
 Response SecurityHandler::Disable() {
   enabled_ = false;
+  certificate_errors_overriden_ = false;
   WebContentsObserver::Observe(nullptr);
+  FlushPendingRequests();
   return Response::OK();
 }
 
 Response SecurityHandler::ShowCertificateViewer() {
   if (!host_)
     return Response::InternalError();
   WebContents* web_contents = WebContents::FromRenderFrameHost(host_);
   scoped_refptr<net::X509Certificate> certificate =
       web_contents->GetController().GetVisibleEntry()->GetSSL().certificate;
   if (!certificate)
     return Response::Error("Could not find certificate");
   web_contents->GetDelegate()->ShowCertificateViewerInDevTools(
       web_contents, certificate);
   return Response::OK();
 }
 
+Response SecurityHandler::HandleCertificateError(int event_id,
+                                                 const String& action) {
+  if (callbacks_.find(event_id) == callbacks_.end()) {

[estark, 2017/02/28 23:34:01]

Is it possible for one devtools process to send a message to the browser that
will be routed to the SecurityHandler corresponding to another devtools
instance? If so, I think that might be a security problem -- one devtools
process probably shouldn't be able to bypass certificate errors in another
unrelated renderer.

[irisu, 2017/03/06 11:56:48]

I'll defer this one to the devtools people: @dgozman, @pfeldman?

+    return Response::Error(
+        String("Unknown event id: " + std::to_string(event_id)));
+  }
+  content::CertificateRequestResultType type =
+      content::CERTIFICATE_REQUEST_RESULT_TYPE_DENY;
+  Response response = Response::OK();
+  if (action == Security::CertificateErrorActionEnum::Continue) {
+    type = content::CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE;
+  } else if (action == Security::CertificateErrorActionEnum::Cancel) {
+    type = content::CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL;

[estark, 2017/02/28 23:34:01]

Do we need to expose both CANCEL and DENY to devtools? I wonder if we could get
away with just exposing CANCEL, since that's what clicking through an
interstitial does.

[irisu, 2017/03/06 11:56:48]

Done.

+  } else if (action == Security::CertificateErrorActionEnum::Deny) {
+    type = content::CERTIFICATE_REQUEST_RESULT_TYPE_DENY;
+  } else {
+    response =
+        Response::Error(String("Unknown Certificate Error Action: " + action));
+  }
+  callbacks_[event_id].Run(type);
+  callbacks_.erase(event_id);
+  return response;
+}
+
+Response SecurityHandler::SetOverrideCertificateErrors(bool override) {
+  if (override && !enabled_)
+    return Response::Error("Security domain not enabled");
+  certificate_errors_overriden_ = override;
+  if (!override)
+    FlushPendingRequests();
+  return Response::OK();
+}
+
 }  // namespace protocol
 }  // namespace content