PlzNavigate: Move navigation-level mixed content checks to the browser.

third_party/WebKit/Source/core/loader/MixedContentChecker.cpp

Index: third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
diff --git a/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp b/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
index 51dfb21fc44a42e3c0a98e64b2da0780c1ed4dd3..4d7d6e70f1cae4d5627b8f181eba5183513f16e9 100644
--- a/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
+++ b/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
@@ -143,8 +143,8 @@ static void measureStricterVersionOfIsMixedContent(Frame* frame,
                                                    const KURL& url) {
   // We're currently only checking for mixed content in `https://*` contexts.
   // What about other "secure" contexts the SchemeRegistry knows about? We'll
-  // use this method to measure the occurance of non-webby mixed content to make
-  // sure we're not breaking the world without realizing it.
+  // use this method to measure the occurrence of non-webby mixed content to
+  // make sure we're not breaking the world without realizing it.
   SecurityOrigin* origin = frame->securityContext()->getSecurityOrigin();
   if (MixedContentChecker::isMixedContent(origin, url)) {
     if (origin->protocol() != "https") {
@@ -294,6 +294,13 @@ bool MixedContentChecker::shouldBlockFetch(
     ResourceRequest::RedirectStatus redirectStatus,
     const KURL& url,
     MixedContentChecker::ReportingStatus reportingStatus) {
+  // Frame-level loads are checked by the browser if PlzNavigate is enabled. No
+  // need to check them again here.
+  if (frame->settings()->getBrowserSideNavigationEnabled() &&
+      frameType != WebURLRequest::FrameTypeNone) {
+    return false;
+  }

[Mike West, 2017/02/13 13:17:44]

As we've discussed, landing this change will invert the web-visible behavior of
MIX and HSTS (for both PlzNavigate and status quo). What's the plan for
remedying that?

Could you add some justification to the CL description as to why we're not doing
an initial check on the Blink-side before sending things up to the browser?

[carlosk, 2017/02/14 01:42:59]

This change will not affect the current navigation implementation; it
falls-through in case PlzNavigate is not enabled. My understanding from our
meeting is that the remediation for PlzNavigate will be to have HSTS working as
a throttle implementation in the browser.
My CL description was outdated: it still said this implementation would be
active for both the current navigation implementation and for PlzNavigate. *That
is not true anymore!*. I updated it accordingly.

I might not be understanding what you mean but this is what I did:
* The comment above these lines already explains why there's this early return
here.
* And there's this statement in the description: "(...) note that this is only a
partial move of the checks currently performed by MixedContentChecker: only
frame navigation resource loads will be intercepted by the browser, by the newly
created MixedContentNavigationThrottle."

[carlosk, 2017/02/14 19:32:15]

Filed https://crbug.com/692157 to track the HSTS throttle work and added a TODO
to mixed_content_navigation_throttle.h.

+
   Frame* effectiveFrame = effectiveFrameForFrameType(frame, frameType);
   Frame* mixedFrame =
       inWhichFrameIsContentMixed(effectiveFrame, frameType, url);
@@ -563,6 +570,28 @@ void MixedContentChecker::handleCertificateError(
   }
 }
 
+// static
+void MixedContentChecker::mixedContentFound(
+    LocalFrame* frame,
+    const KURL& mainResourceUrl,
+    const KURL& mixedContentUrl,
+    WebURLRequest::RequestContext requestContext,
+    bool wasAllowed,
+    bool hadRedirect) {
+  // Logs to the frame console.
+  logToConsoleAboutFetch(frame, mainResourceUrl, mixedContentUrl,
+                         requestContext, wasAllowed);
+  // Reports to the CSP policy.
+  ContentSecurityPolicy* policy =
+      frame->securityContext()->contentSecurityPolicy();
+  if (policy) {

[Mike West, 2017/02/13 13:17:44]

Is it possible for this to be false? Shouldn't every security context have a
policy?

[carlosk, 2017/02/14 01:42:59]

This mimics the way these same reports are implemented in MixedContentChecker.

+    policy->reportMixedContent(
+        mixedContentUrl, hadRedirect
+                             ? ResourceRequest::RedirectStatus::FollowedRedirect
+                             : ResourceRequest::RedirectStatus::NoRedirect);
+  }
+}
+
 WebMixedContentContextType MixedContentChecker::contextTypeForInspector(
     LocalFrame* frame,
     const ResourceRequest& request) {