Block renderer-initiated main frame navigations to data URLs

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

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 8cd77648a4dd0116286c5cf0df7be66d44d27041..51e6558ae907e2ac4ac25ea3988c495941478401 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -89,6 +89,7 @@
 #include "platform/loader/fetch/ResourceFetcher.h"
 #include "platform/loader/fetch/ResourceRequest.h"
 #include "platform/network/HTTPParsers.h"
+#include "platform/network/NetworkUtils.h"
 #include "platform/scroll/ScrollAnimatorBase.h"
 #include "platform/weborigin/SchemeRegistry.h"
 #include "platform/weborigin/SecurityOrigin.h"
@@ -748,6 +749,20 @@ bool FrameLoader::PrepareRequestForThisFrame(FrameLoadRequest& request) {
     return false;
   }
 
+  // Block renderer-initiated loads of data URLs in the top frame. If the mime
+  // type of the data URL is supported, the URL will eventually be rendered, so
+  // block it here. Otherwise, the load might be handled by a plugin or end up
+  // as a download, so allow it to let the embedder figure out what to do with
+  // it.
+  if (frame_->IsMainFrame() &&
+      !request.GetResourceRequest().IsSameDocumentNavigation() &&
+      !frame_->Client()->AllowContentInitiatedDataUrlNavigations(

[dcheng, 2017/04/15 01:09:38]

Sorry, to follow up on my other question: would it make sense to move this check
into DecidePolicyForNavigation() rather than adding another entry point for
navigation policy checks?

[meacer, 2017/04/17 22:21:58]

Quick update: I moved these to DecidePolicyForNavigation() but it broke a large
number of tests that are using RenderFrameTest::LoadHTML. Currently looking into
it.

[meacer, 2017/04/19 00:05:54]

Okay, I've been looking at this, and there are tests that I don't know how to
fix. The latest CL that does these checks in DecidePolicyForNavigation is at
https://codereview.chromium.org/2787123005/. So I'd like to land this as is.

+          request.OriginDocument()->Url()) &&
+      url.ProtocolIsData() && NetworkUtils::IsDataURLMimeTypeSupported(url)) {
+    ReportTopLevelNavigationFailed(frame_, url.ElidedString());
+    return false;
+  }
+
   if (!request.Form() && request.FrameName().IsEmpty())
     request.SetFrameName(frame_->GetDocument()->BaseTarget());
   return true;
@@ -987,6 +1002,17 @@ void FrameLoader::ReportLocalLoadFailed(LocalFrame* frame, const String& url) {
                              "Not allowed to load local resource: " + url));
 }
 
+void FrameLoader::ReportTopLevelNavigationFailed(LocalFrame* frame,
+                                                 const String& url) {
+  DCHECK(!url.IsEmpty());
+  if (!frame)

[dcheng, 2017/04/19 11:59:49]

Nit: just make this a non-static method (unlike ReportLocalLoadFailed, we don't
need to call it from elsewhere--and probably ReportLocalLoadFailed should just
be non-static, but that's out of scope here)

And frame (and frame_) if not static can never be null here.

[meacer, 2017/04/21 01:31:21]

This used to be called from outside FrameLoader but not anymore, so I just
inlined it into PrepareRequestForThisFrame.

+    return;
+
+  frame->GetDocument()->AddConsoleMessage(ConsoleMessage::Create(
+      kSecurityMessageSource, kErrorMessageLevel,
+      "Not allowed to top-level navigate to resource: " + url));
+}
+
 void FrameLoader::StopAllLoaders() {
   if (frame_->GetDocument()->PageDismissalEventBeingDispatched() !=
       Document::kNoDismissal)