WKWebView(iOS9): correctly update SSL status for current navigation item

ios/web/web_state/ui/crw_wk_web_view_web_controller.mm

Index: ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
index e5c88664c2057d26c56a89ea4e4b249886d6e8fb..60dba928cf1531841fa495cfc82b429b94833f42 100644
--- a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
+++ b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
@@ -11,6 +11,7 @@
 #include "base/json/json_reader.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/values.h"
 #import "ios/net/http_response_headers_util.h"
@@ -82,6 +83,14 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
 }  // namespace
 
+#if !defined(__IPHONE_9_0) || __IPHONE_OS_VERSION_MAX_ALLOWED < __IPHONE_9_0
+// Predeclare iOS9 API so calls are compiled on iOS8 bots.
+// TODO(eugenebut): cleanup this (crbug.com/523365).
+@interface WKWebView (CRWIOS9API)
+@property(nonatomic, readonly, copy) NSArray* certificateChain;
+@end
+#endif
+
 @interface CRWWKWebViewWebController () <WKNavigationDelegate,
                                          WKScriptMessageHandler,
                                          WKUIDelegate> {
@@ -250,12 +259,20 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // Clears all activity indicator tasks for this web controller.
 - (void)clearActivityIndicatorTasks;
 
+// Obtains SSL status from given |certChain| and |host| and updates current
+// navigation item.
+- (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain
+                                                      host:(NSString*)host;
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 // Updates SSL status for the current navigation item based on the information
 // provided by web view.
 - (void)updateSSLStatusForCurrentNavigationItem;
 #endif
 
+// Reports "WebController.WKWebViewHasCertForSecureConnection" UMA.
+- (void)reportHasCertForSecureConnectionUMAWithValue:(bool)value;
+
 // Registers load request with empty referrer and link or client redirect
 // transition based on user interaction state.
 - (void)registerLoadRequest:(const GURL&)url;
@@ -864,12 +881,63 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
       clearNetworkTasksForGroup:[self activityIndicatorGroupID]];
 }
 
+- (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain

[davidben, 2015/10/07 20:16:30]

Nit: I'd maybe name this something with the word "schedule" in the name, so it's
clear this doesn't happen synchronously. Maybe
scheduleSSLStatusUpdateForCurrentNavigationItem?

[davidben, 2015/10/07 20:16:30]

Should this also be under the ifdef?

[stuartmorgan, 2015/10/07 22:30:14]

+1. "begin" could also work.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

We will drop ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW soon, so it does not really
matter.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

+                                                      host:(NSString*)host {
+  // Use Navigation Item's unique ID to locate requested item after
+  // obtaining cert status asynchronously.
+  web::NavigationItem* item =
+      self.webStateImpl->GetNavigationManager()->GetLastCommittedItem();
+  if (!item)

[stuartmorgan, 2015/10/07 22:30:14]

Is this something that should actually be possible? (I.e., is this really
something that should be handled vs DCHECKed?)

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Currently not possible. In Chromium DCHECKs are not used right before pointer
dereferencing, so I'm just removing this early return.

+    return;
+  int itemID = item->GetUniqueID();
+
+  base::WeakNSObject<CRWWKWebViewWebController> weakSelf(self);
+  void (^SSLStatusResponse)(web::SecurityStyle, net::CertStatus) = ^(
+      web::SecurityStyle style, net::CertStatus certStatus) {
+    base::scoped_nsobject<CRWWKWebViewWebController> strongSelf(
+        [weakSelf retain]);
+    if (!strongSelf || [strongSelf isBeingDestroyed]) {
+      return;
+    }

[stuartmorgan, 2015/10/07 22:30:14]

Could you pull everything after this into a helper method? Long blocks of code
after the strong/weak dance make me nervous, because it's *really* easy for
someone to accidentally slip an ivar reference in without anyone noticing.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

+
+    web::NavigationManager* navigationManager =
+        [strongSelf webStateImpl]->GetNavigationManager();
+
+    scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(chain));
+    int certID =
+        web::CertStore::GetInstance()->StoreCert(cert.get(), self.certGroupID);
+    std::string GURLHost = base::SysNSStringToUTF8(host);
+    for (int i = 0; i < navigationManager->GetEntryCount(); i++) {

[stuartmorgan, 2015/10/07 22:30:14]

It should almost always be the last entry, so walk backward rather than forward.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:33]

Done.

+      web::NavigationItem* item = navigationManager->GetItemAtIndex(i);
+      web::SSLStatus& SSLStatus = item->GetSSL();
+      // NavigationItem's UniqueID is preserved even after redirects, so
+      // checking that URL and cert matches is necessary.
+      if (itemID == item->GetUniqueID() && SSLStatus.cert_id == certID &&

[stuartmorgan, 2015/10/07 22:30:14]

Flip the first check around; it's backward relative to all the other ones which
makes it harder to read.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

+          item->GetURL().host() == GURLHost) {
+        web::SSLStatus previousSSLStatus = item->GetSSL();
+        SSLStatus.cert_status = certStatus;
+        SSLStatus.security_style = style;
+        if (navigationManager->GetCurrentEntryIndex() == i &&
+            !previousSSLStatus.Equals(SSLStatus)) {
+          [strongSelf didUpdateSSLStatusForCurrentNavigationItem];
+        }
+        return;

[stuartmorgan, 2015/10/07 22:30:14]

You actually want to return after finding an itemID match regardless of whether
the rest matched, so you'll need to break apart the conditional.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

+      }
+    }
+  };
+
+  [_certVerificationController
+      querySSLStatusForTrust:web::CreateServerTrustFromChain(chain, host)
+                        host:host
+           completionHandler:SSLStatusResponse];
+}
+
 #if !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
+
 - (void)updateSSLStatusForCurrentNavigationItem {
   if ([self isBeingDestroyed])
     return;
 
-  DCHECK(self.webStateImpl);
   web::NavigationItem* item =
       self.webStateImpl->GetNavigationManagerImpl().GetLastCommittedItem();
   if (!item)
@@ -877,34 +945,60 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 
   web::SSLStatus previousSSLStatus = item->GetSSL();
   web::SSLStatus& SSLStatus = item->GetSSL();
-  if (item->GetURL().SchemeIsCryptographic()) {
-    // TODO(eugenebut): Do not set security style to authenticated once
-    // proceeding with bad ssl cert is implemented.
-    SSLStatus.security_style = web::SECURITY_STYLE_AUTHENTICATED;
-    SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
-                                   ? web::SSLStatus::NORMAL_CONTENT
-                                   : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
-
-    if (base::ios::IsRunningOnIOS9OrLater()) {
-      scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(
-          [_wkWebView performSelector:@selector(certificateChain)]));
-      if (cert) {
-        SSLStatus.cert_id = web::CertStore::GetInstance()->StoreCert(
-            cert.get(), self.certGroupID);
-      } else {
-        SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
-        SSLStatus.cert_id = 0;
+
+  // Starting from iOS9 WKWebView blocks active mixed content, so if
+  // |hasOnlySecureContent| returns NO it means passive content.
+  // On iOS8 there is no way to determine if web view has active mixed content.
+  SSLStatus.content_status = [_wkWebView hasOnlySecureContent]
+                                 ? web::SSLStatus::NORMAL_CONTENT
+                                 : web::SSLStatus::DISPLAYED_INSECURE_CONTENT;
+
+  // Try updating SSLStatus for current NavigationItem asynchronously.
+  scoped_refptr<net::X509Certificate> cert;
+  if (base::ios::IsRunningOnIOS9OrLater() &&
+      item->GetURL().SchemeIsCryptographic()) {
+    NSArray* chain = [_wkWebView certificateChain];
+    cert = web::CreateCertFromChain(chain);
+    if (cert) {
+      [self reportHasCertForSecureConnectionUMAWithValue:true];

[davidben, 2015/10/07 20:16:30]

Should this be in line 968? Seems you don't want to record anything when the
certificate hasn't changed.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

+      int oldCertID = SSLStatus.cert_id;
+      SSLStatus.cert_id = web::CertStore::GetInstance()->StoreCert(
+          cert.get(), self.certGroupID);
+      if (oldCertID != SSLStatus.cert_id ||

[davidben, 2015/10/07 20:16:30]

Could you add a comment here like:

  // Only recompute the SSLStatus information if the certificate has since
changed.

[davidben, 2015/10/07 20:16:30]

If events cause updateSSLStatusForCurrentNavigationItem to be called multiple
times in a row, I'm not sure this check quite works since SSLStatus only updates
when the asynchronous operation completes. Perhaps set cert_id synchronously and
then add some kind of pending state?

(Alternatively, see top-level comment.)

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

If |updateSSLStatusForCurrentNavigationItem| is called for the second time, then
both certID and URL will match and cert revalidation will not be started.

[Eugene But (OOO till 7-30), 2015/10/08 16:53:32]

Done.

[davidben, 2015/10/12 23:21:50]

Only if the asynchronous update process has completed already. If this function
is called twice in a row, the update process probably won't have completed yet,
and you'll trigger more and more of these.

[Eugene But (OOO till 7-30), 2015/10/13 20:40:33]

How come? This method (updateSSLStatusForCurrentNavigationItem) is actually
called twice in a raw.
However updateCurrentNavigationItemSSLStatusUsingCertChain:host is called only
once.

1st Call of updateSSLStatusForCurrentNavigationItem:
Changes SSLStatus.cert_id from 0 to 1, which leads to
updateCurrentNavigationItemSSLStatusUsingCertChain:host: invocation.

2nd Call of updateSSLStatusForCurrentNavigationItem:
Does not change SSLStatus.cert_id (because
web::CertStore::GetInstance()->StoreCert returns the same id for same cert);
certID and URLs are the same, hence
|updateCurrentNavigationItemSSLStatusUsingCertChain:host:| is not called.

Am I missing something?

[davidben, 2015/10/15 17:46:12]

Ah, no, you're right. I'd missed that line 965 actually updated things. See also
non-const references being very confusing. :-) So the pending state is about
ensuring the SSLStatus.security_status and SSLStatus.cert_id always match. If
you hadn't updated cert_id, then they would always match without the pending
state, but you'd trigger updates a lot.

+          item->GetURL().host() != _documentURL.host()) {
+        NSString* host = base::SysUTF8ToNSString(_documentURL.host());
+        [self updateCurrentNavigationItemSSLStatusUsingCertChain:chain
+                                                            host:host];
       }
     }
-  } else {
-    SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+  }
+
+  if (!cert) {
     SSLStatus.cert_id = 0;
+    if (!item->GetURL().SchemeIsCryptographic()) {
+      // HTTP or other non-secure connection.
+      SSLStatus.security_style = web::SECURITY_STYLE_UNAUTHENTICATED;
+    } else if (base::ios::IsRunningOnIOS9OrLater()) {
+      // HTTPS, iOS9 and no certificate (this use-case has not been observed).
+      [self reportHasCertForSecureConnectionUMAWithValue:false];
+      SSLStatus.security_style = web::SECURITY_STYLE_UNKNOWN;
+    } else {
+      // HTTPS, iOS8.
+      // iOS8 cannot load unauthenticated HTTPS content.
+      SSLStatus.security_style = web::SECURITY_STYLE_AUTHENTICATED;
+    }
   }
 
   if (!previousSSLStatus.Equals(SSLStatus)) {
     [self didUpdateSSLStatusForCurrentNavigationItem];
   }
 }
+
+- (void)reportHasCertForSecureConnectionUMAWithValue:(bool)value {
+  UMA_HISTOGRAM_BOOLEAN("WebController.WKWebViewHasCertForSecureConnection",
+                        value);
+}
+
 #endif  // !defined(ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW)
 
 - (void)registerLoadRequest:(const GURL&)url {