Adapt MixedContentChecker for remote frames

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 f01788594efaabf87d8db98750fcba90a70c23fa..9d9ccd3af36caa9f556510a0ab27ef678db1d477 100644
--- a/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
+++ b/third_party/WebKit/Source/core/loader/MixedContentChecker.cpp
@@ -29,6 +29,7 @@
 #include "core/loader/MixedContentChecker.h"
 
 #include "core/dom/Document.h"
+#include "core/frame/Frame.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Settings.h"
 #include "core/frame/UseCounter.h"
@@ -44,22 +45,33 @@
 
 namespace blink {
 
-static void measureStricterVersionOfIsMixedContent(LocalFrame* frame, const KURL& url)
+namespace {
+
+    KURL mainResourceUrlForConsoleLog(Frame* frame)

[alexmos, 2016/01/11 22:02:24]

nit: comment explaining the need for this function?  

[alexmos, 2016/01/11 22:02:24]

nit: no need for indent.

[estark, 2016/01/14 23:05:48]

Hm, `git cl format` seems to undo it if I make that change.

[estark, 2016/01/14 23:05:48]

Done.

[alexmos, 2016/01/16 00:49:49]

That's interesting/weird.  The Blink coding style seems to imply that the indent
isn't needed in this case: (https://www.chromium.org/blink/coding-style, part 3
in Indentation).  I've also never seen it indented in other code. :)

I've asked dcheng@, and he suggests that clang-format hasn't really been
well-tested with Blink code and shouldn't be trusted.  He also agrees that there
shouldn't be indent here, so it seems this may be a bug in how git cl format is
set up for Blink.

[estark, 2016/01/20 05:56:22]

Oh ok, good to know! Done.

+    {
+        if (!frame->isLocalFrame())

[alexmos, 2016/01/11 22:02:24]

nit: frame->isRemoteFrame()

[estark, 2016/01/14 23:05:48]

Done.

+            return KURL(KURL(), frame->securityContext()->securityOrigin()->toString());
+        return toLocalFrame(frame)->document()->url();
+    }
+
+} // namespace
+
+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.
-    SecurityOrigin* origin = frame->document()->securityOrigin();
+    SecurityOrigin* origin = frame->securityContext()->securityOrigin();
     if (MixedContentChecker::isMixedContent(origin, url)) {
-        if (frame->document()->securityOrigin()->protocol() != "https")
+        if (origin->protocol() != "https")
             UseCounter::count(frame, UseCounter::MixedContentInNonHTTPSFrameThatRestrictsMixedContent);
     } else if (!SecurityOrigin::isSecure(url) && SchemeRegistry::shouldTreatURLSchemeAsSecure(origin->protocol())) {
         UseCounter::count(frame, UseCounter::MixedContentInSecureFrameThatDoesNotRestrictMixedContent);
     }
 }
 
-bool requestIsSubframeSubresource(LocalFrame* frame, WebURLRequest::FrameType frameType)
+bool requestIsSubframeSubresource(Frame* frame, WebURLRequest::FrameType frameType)
 {
     return (frame && frame != frame->tree().top() && frameType != WebURLRequest::FrameTypeNested);
 }
@@ -75,7 +87,7 @@ bool MixedContentChecker::isMixedContent(SecurityOrigin* securityOrigin, const K
 }
 
 // static
-LocalFrame* MixedContentChecker::inWhichFrameIsContentMixed(LocalFrame* frame, WebURLRequest::FrameType frameType, const KURL& url)
+Frame* MixedContentChecker::inWhichFrameIsContentMixed(Frame* frame, WebURLRequest::FrameType frameType, const KURL& url)
 {
     // We only care about subresource loads; top-level navigations cannot be mixed content. Neither can frameless requests.
     if (frameType == WebURLRequest::FrameTypeTopLevel || !frame)
@@ -83,19 +95,13 @@ LocalFrame* MixedContentChecker::inWhichFrameIsContentMixed(LocalFrame* frame, W
 
     // Check the top frame first.
     if (Frame* top = frame->tree().top()) {
-        // FIXME: We need a way to access the top-level frame's SecurityOrigin when that frame
-        // is in a different process from the current frame. Until that is done, we bail out.
-        if (!top->isLocalFrame())
-            return nullptr;
-
-        LocalFrame* localTop = toLocalFrame(top);
-        measureStricterVersionOfIsMixedContent(localTop, url);
-        if (isMixedContent(localTop->document()->securityOrigin(), url))
-            return localTop;
+        measureStricterVersionOfIsMixedContent(top, url);
+        if (isMixedContent(top->securityContext()->securityOrigin(), url))
+            return top;
     }
 
     measureStricterVersionOfIsMixedContent(frame, url);
-    if (isMixedContent(frame->document()->securityOrigin(), url))
+    if (isMixedContent(frame->securityContext()->securityOrigin(), url))
         return frame;
 
     // No mixed content, no problem.
@@ -103,7 +109,7 @@ LocalFrame* MixedContentChecker::inWhichFrameIsContentMixed(LocalFrame* frame, W
 }
 
 // static
-MixedContentChecker::ContextType MixedContentChecker::contextTypeFromContext(WebURLRequest::RequestContext context, LocalFrame* frame)
+MixedContentChecker::ContextType MixedContentChecker::contextTypeFromContext(WebURLRequest::RequestContext context, Frame* frame)
 {
     switch (context) {
     // "Optionally-blockable" mixed content
@@ -238,18 +244,18 @@ const char* MixedContentChecker::typeNameFromContext(WebURLRequest::RequestConte
 }
 
 // static
-void MixedContentChecker::logToConsoleAboutFetch(LocalFrame* frame, const KURL& url, WebURLRequest::RequestContext requestContext, bool allowed)
+void MixedContentChecker::logToConsoleAboutFetch(LocalFrame* frame, const KURL& mainResourceUrl, const KURL& url, WebURLRequest::RequestContext requestContext, bool allowed)
 {
     String message = String::format(
         "Mixed Content: The page at '%s' was loaded over HTTPS, but requested an insecure %s '%s'. %s",
-        frame->document()->url().elidedString().utf8().data(), typeNameFromContext(requestContext), url.elidedString().utf8().data(),
+        mainResourceUrl.elidedString().utf8().data(), typeNameFromContext(requestContext), url.elidedString().utf8().data(),
         allowed ? "This content should also be served over HTTPS." : "This request has been blocked; the content must be served over HTTPS.");
     MessageLevel messageLevel = allowed ? WarningMessageLevel : ErrorMessageLevel;
     frame->document()->addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, messageLevel, message));
 }
 
 // static
-void MixedContentChecker::count(LocalFrame* frame, WebURLRequest::RequestContext requestContext)
+void MixedContentChecker::count(Frame* frame, WebURLRequest::RequestContext requestContext)
 {
     UseCounter::count(frame, UseCounter::MixedContentPresent);
 
@@ -298,15 +304,19 @@ void MixedContentChecker::count(LocalFrame* frame, WebURLRequest::RequestContext
 // static
 bool MixedContentChecker::shouldBlockFetch(LocalFrame* frame, WebURLRequest::RequestContext requestContext, WebURLRequest::FrameType frameType, const KURL& url, MixedContentChecker::ReportingStatus reportingStatus)
 {
-    LocalFrame* mixedFrame = inWhichFrameIsContentMixed(frame, frameType, url);
+    Frame* effectiveFrame = effectiveFrameForFrameType(frame, frameType);

[alexmos, 2016/01/11 22:02:24]

Do you know why this didn't call effectiveFrameForFrameType previously?  Was
this a bug?

[estark, 2016/01/14 23:05:48]

The callsite (in FrameFetchContext.cpp) used to call it. I moved it in here so
that I'd always have access to a LocalFrame in here. Otherwise, |frame| might be
remote and we wouldn't have any LocalFrame in here.

[alexmos, 2016/01/16 00:49:50]

Ah yes, for FrameFetchContext that makes sense, but are other call sites ok with
this change?  E.g., the one in WebLocalFrameImpl::checkIfRunInsecureContent
seems like it may end up using the parent frame in new code but not in old code
- is this ok?  (There's also a comment there about that function used only for
NPAPI and going away, so this may not matter much).

[estark, 2016/01/20 05:56:22]

Ahh, I missed that one, thanks. (I think it shouldn't matter for the other call
sites).

I'm not sure about this because I don't quite understand why that call site uses
FrameTypeNested; I'll ask Mike when he takes a look at this CL.

[alexmos, 2016/01/20 22:43:55]

Sounds good.

+    Frame* mixedFrame = inWhichFrameIsContentMixed(effectiveFrame, frameType, url);
     if (!mixedFrame)
         return false;
 
     MixedContentChecker::count(mixedFrame, requestContext);
 
     Settings* settings = mixedFrame->settings();
+    // Use the current local frame's client; the embedder doesn't
+    // distinguish mixed content signals from different frames on the
+    // same page.
     FrameLoaderClient* client = frame->loader().client();
-    SecurityOrigin* securityOrigin = mixedFrame->document()->securityOrigin();
+    SecurityOrigin* securityOrigin = mixedFrame->securityContext()->securityOrigin();
     bool allowed = false;
 
     // If we're in strict mode, we'll automagically fail everything, and intentionally skip
@@ -336,7 +346,7 @@ bool MixedContentChecker::shouldBlockFetch(LocalFrame* frame, WebURLRequest::Req
     case ContextTypeBlockable: {
         // Strictly block subresources in subframes, unless all insecure
         // content is allowed.
-        if (!settings->allowRunningOfInsecureContent() && requestIsSubframeSubresource(frame, frameType)) {
+        if (!settings->allowRunningOfInsecureContent() && requestIsSubframeSubresource(effectiveFrame, frameType)) {
             UseCounter::count(mixedFrame, UseCounter::BlockableMixedContentInSubframeBlocked);
             allowed = false;
             break;
@@ -364,16 +374,16 @@ bool MixedContentChecker::shouldBlockFetch(LocalFrame* frame, WebURLRequest::Req
     };
 
     if (reportingStatus == SendReport)
-        logToConsoleAboutFetch(frame, url, requestContext, allowed);
+        logToConsoleAboutFetch(effectiveFrame->isLocalFrame() ? toLocalFrame(effectiveFrame) : frame, mainResourceUrlForConsoleLog(mixedFrame), url, requestContext, allowed);

[alexmos, 2016/01/11 22:02:24]

This feels a bit weird, in that the sometimes the message ends up on the parent
frame and sometimes on current frame (depending on whether OOPIFs are used). 
What was wrong with just logging on |frame|?

[estark, 2016/01/14 23:05:48]

I suppose always using |frame| makes sense, but it causes a slight regression:
when https://foo.com has an iframe to http://bar.com, the console message won't
say the line number, I guess because there is no line number for loading the
main resource for a frame. So e.g. the layout test results will be "CONSOLE
ERROR" instead of "CONSOLE ERROR (line X)". What do you think?

[alexmos, 2016/01/16 00:49:49]

Ah, I see.  It would be nice if the test expectations matched up with and
without site isolation, as this will make running layout tests with OOPIFs
easier.  So my own slight preference would be to keep this as-is, but I'm not
sure we can get away with it (I'd also be ok if we restored the line number
behavior).  Maybe run this by mkwst@ to see if he's ok removing this?  Also, how
many layout tests are affected (I don't see this coming up in the expectation
changes you have so far)?

[estark, 2016/01/20 05:56:22]

Ok, yeah, I agree that it would be nice if the test expectations were
consistent, so I'll add this to my ask-Mike list. I think it'll affect about 8
layout tests (I didn't make the changes yet in case we want to change it back),
but I'm kicking off a trybot run to make sure there aren't any others that I
didn't notice.

[alexmos, 2016/01/20 22:43:55]

Acknowledged.

     return !allowed;
 }
 
 // static
-void MixedContentChecker::logToConsoleAboutWebSocket(LocalFrame* frame, const KURL& url, bool allowed)
+void MixedContentChecker::logToConsoleAboutWebSocket(LocalFrame* frame, const KURL& mainResourceUrl, const KURL& url, bool allowed)
 {
     String message = String::format(
         "Mixed Content: The page at '%s' was loaded over HTTPS, but attempted to connect to the insecure WebSocket endpoint '%s'. %s",
-        frame->document()->url().elidedString().utf8().data(), url.elidedString().utf8().data(),
+        mainResourceUrl.elidedString().utf8().data(), url.elidedString().utf8().data(),
         allowed ? "This endpoint should be available via WSS. Insecure access is deprecated." : "This request has been blocked; this endpoint must be available over WSS.");
     MessageLevel messageLevel = allowed ? WarningMessageLevel : ErrorMessageLevel;
     frame->document()->addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, messageLevel, message));
@@ -382,7 +392,7 @@ void MixedContentChecker::logToConsoleAboutWebSocket(LocalFrame* frame, const KU
 // static
 bool MixedContentChecker::shouldBlockWebSocket(LocalFrame* frame, const KURL& url, MixedContentChecker::ReportingStatus reportingStatus)
 {
-    LocalFrame* mixedFrame = inWhichFrameIsContentMixed(frame, WebURLRequest::FrameTypeNone, url);
+    Frame* mixedFrame = inWhichFrameIsContentMixed(frame, WebURLRequest::FrameTypeNone, url);
     if (!mixedFrame)
         return false;
 
@@ -390,13 +400,13 @@ bool MixedContentChecker::shouldBlockWebSocket(LocalFrame* frame, const KURL& ur
     UseCounter::count(mixedFrame, UseCounter::MixedContentWebSocket);
 
     Settings* settings = mixedFrame->settings();
-    FrameLoaderClient* client = mixedFrame->loader().client();
-    SecurityOrigin* securityOrigin = mixedFrame->document()->securityOrigin();
+    FrameLoaderClient* client = frame->loader().client();
+    SecurityOrigin* securityOrigin = mixedFrame->securityContext()->securityOrigin();
     bool allowed = false;
 
     // If we're in strict mode, we'll automagically fail everything, and intentionally skip
     // the client checks in order to prevent degrading the site's security UI.
-    bool strictMode = mixedFrame->document()->shouldEnforceStrictMixedContentChecking() || settings->strictMixedContentChecking();
+    bool strictMode = mixedFrame->securityContext()->shouldEnforceStrictMixedContentChecking() || settings->strictMixedContentChecking();
     if (!strictMode) {
         bool allowedPerSettings = settings && settings->allowRunningOfInsecureContent();
         mixedFrame->client()->triedRunningInsecureContent(securityOrigin, url);
@@ -407,7 +417,7 @@ bool MixedContentChecker::shouldBlockWebSocket(LocalFrame* frame, const KURL& ur
         client->didRunInsecureContent(securityOrigin, url);
 
     if (reportingStatus == SendReport)
-        logToConsoleAboutWebSocket(frame, url, allowed);
+        logToConsoleAboutWebSocket(frame, mainResourceUrlForConsoleLog(mixedFrame), url, allowed);
     return !allowed;
 }
 
@@ -419,19 +429,19 @@ bool MixedContentChecker::isMixedFormAction(LocalFrame* frame, const KURL& url,
     if (url.protocolIs("javascript"))
         return false;
 
-    LocalFrame* mixedFrame = inWhichFrameIsContentMixed(frame, WebURLRequest::FrameTypeNone, url);
+    Frame* mixedFrame = inWhichFrameIsContentMixed(frame, WebURLRequest::FrameTypeNone, url);
     if (!mixedFrame)
         return false;
 
     UseCounter::count(mixedFrame, UseCounter::MixedContentPresent);
 
-    mixedFrame->loader().client()->didDisplayInsecureContent();
+    frame->loader().client()->didDisplayInsecureContent();
 
     if (reportingStatus == SendReport) {
         String message = String::format(
             "Mixed Content: The page at '%s' was loaded over a secure connection, but contains a form which targets an insecure endpoint '%s'. This endpoint should be made available over a secure connection.",
-            frame->document()->url().elidedString().utf8().data(), url.elidedString().utf8().data());
-        mixedFrame->document()->addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, WarningMessageLevel, message));
+            mainResourceUrlForConsoleLog(mixedFrame).elidedString().utf8().data(), url.elidedString().utf8().data());
+        frame->document()->addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, WarningMessageLevel, message));
     }
 
     return true;
@@ -447,47 +457,50 @@ void MixedContentChecker::checkMixedPrivatePublic(LocalFrame* frame, const Atomi
         UseCounter::count(frame->document(), UseCounter::MixedContentPrivateHostnameInPublicHostname);
 }
 
-LocalFrame* MixedContentChecker::effectiveFrameForFrameType(LocalFrame* frame, WebURLRequest::FrameType frameType)
+Frame* MixedContentChecker::effectiveFrameForFrameType(LocalFrame* frame, WebURLRequest::FrameType frameType)
 {
     // If we're loading the main resource of a subframe, ensure that we check
     // against the parent of the active frame, rather than the frame itself.
-    LocalFrame* effectiveFrame = frame;
-    if (frameType == WebURLRequest::FrameTypeNested) {
-        // FIXME: Deal with RemoteFrames.
-        Frame* parentFrame = effectiveFrame->tree().parent();
-        ASSERT(parentFrame);
-        if (parentFrame->isLocalFrame())
-            effectiveFrame = toLocalFrame(parentFrame);
-    }
-    return effectiveFrame;
+    if (frameType != WebURLRequest::FrameTypeNested)
+        return frame;
+
+    Frame* parentFrame = frame->tree().parent();
+    ASSERT(parentFrame);
+    return parentFrame;
 }
 
 void MixedContentChecker::handleCertificateError(LocalFrame* frame, const ResourceRequest& request, const ResourceResponse& response)
 {
     WebURLRequest::FrameType frameType = request.frameType();
-    LocalFrame* effectiveFrame = effectiveFrameForFrameType(frame, frameType);
+    Frame* effectiveFrame = effectiveFrameForFrameType(frame, frameType);
     if (frameType == WebURLRequest::FrameTypeTopLevel || !effectiveFrame)
         return;
 
-    FrameLoaderClient* client = effectiveFrame->loader().client();
+    // TODO(estark): handle remote frames, perhaps by omitting security info when the effective frame is remote.
+    if (!effectiveFrame->isLocalFrame())
+        return;
+
+    LocalFrame* localEffectiveFrame = toLocalFrame(effectiveFrame);
+
+    FrameLoaderClient* client = frame->loader().client();

[alexmos, 2016/01/11 22:02:24]

nit: comment about why this changed from effectiveFrame to frame (elsewhere as
well?)

[estark, 2016/01/14 23:05:48]

Done.

     WebURLRequest::RequestContext requestContext = request.requestContext();
-    ContextType contextType = MixedContentChecker::contextTypeFromContext(requestContext, frame);
+    ContextType contextType = MixedContentChecker::contextTypeFromContext(requestContext, effectiveFrame);
     if (contextType == ContextTypeBlockable) {
-        client->didRunContentWithCertificateErrors(response.url(), response.getSecurityInfo(), effectiveFrame->document()->url(), effectiveFrame->loader().documentLoader()->response().getSecurityInfo());
+        client->didRunContentWithCertificateErrors(response.url(), response.getSecurityInfo(), KURL(KURL(), effectiveFrame->securityContext()->securityOrigin()->toString()), localEffectiveFrame->loader().documentLoader()->response().getSecurityInfo());

[alexmos, 2016/01/11 22:02:24]

Is it possible to reuse mainResourceUrlForConsoleLog here?

[estark, 2016/01/14 23:05:48]

Done.

     } else {
         // contextTypeFromContext() never returns NotMixedContent (it
         // computes the type of mixed content, given that the content is
         // mixed).
         ASSERT(contextType != ContextTypeNotMixedContent);
-        client->didDisplayContentWithCertificateErrors(response.url(), response.getSecurityInfo(), effectiveFrame->document()->url(), effectiveFrame->loader().documentLoader()->response().getSecurityInfo());
+        client->didDisplayContentWithCertificateErrors(response.url(), response.getSecurityInfo(), KURL(KURL(), effectiveFrame->securityContext()->securityOrigin()->toString()), localEffectiveFrame->loader().documentLoader()->response().getSecurityInfo());

[alexmos, 2016/01/11 22:02:24]

Ditto

[estark, 2016/01/14 23:05:48]

Done.

     }
 }
 
 MixedContentChecker::ContextType MixedContentChecker::contextTypeForInspector(LocalFrame* frame, const ResourceRequest& request)
 {
-    LocalFrame* effectiveFrame = effectiveFrameForFrameType(frame, request.frameType());
+    Frame* effectiveFrame = effectiveFrameForFrameType(frame, request.frameType());
 
-    LocalFrame* mixedFrame = inWhichFrameIsContentMixed(effectiveFrame, request.frameType(), request.url());
+    Frame* mixedFrame = inWhichFrameIsContentMixed(effectiveFrame, request.frameType(), request.url());
     if (!mixedFrame)
         return ContextTypeNotMixedContent;