Add certificate error handling to devtools.

content/browser/devtools/protocol/security_handler.cc

Index: content/browser/devtools/protocol/security_handler.cc
diff --git a/content/browser/devtools/protocol/security_handler.cc b/content/browser/devtools/protocol/security_handler.cc
index 6b6808e4367c2719ae13e36f62823be48c4487f1..c617835f48616a3adeb397511ca132237e9c94ad 100644
--- a/content/browser/devtools/protocol/security_handler.cc
+++ b/content/browser/devtools/protocol/security_handler.cc
@@ -146,6 +146,31 @@ void SecurityHandler::DidChangeVisibleSecurityState() {
       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_)
@@ -156,7 +181,9 @@ Response SecurityHandler::Enable() {
 
 Response SecurityHandler::Disable() {
   enabled_ = false;
+  certificate_errors_overriden_ = false;
   WebContentsObserver::Observe(nullptr);
+  FlushPendingRequests();
   return Response::OK();
 }
 
@@ -173,5 +200,38 @@ Response SecurityHandler::ShowCertificateViewer() {
   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