Fix web accessible resource checks in ShouldAllowOpenURL

chrome/browser/extensions/process_manager_browsertest.cc

Index: chrome/browser/extensions/process_manager_browsertest.cc
diff --git a/chrome/browser/extensions/process_manager_browsertest.cc b/chrome/browser/extensions/process_manager_browsertest.cc
index a455216ffda2f00eeb89620432e7f1b288d871d9..49957737775cc7ba919df7c25c218dc079665551 100644
--- a/chrome/browser/extensions/process_manager_browsertest.cc
+++ b/chrome/browser/extensions/process_manager_browsertest.cc
@@ -250,7 +250,8 @@ class ProcessManagerBrowserTest : public ExtensionBrowserTest {
     content::WindowedNotificationObserver popup_observer(
         chrome::NOTIFICATION_TAB_ADDED,
         content::NotificationService::AllSources());
-    EXPECT_TRUE(ExecuteScript(opener, "window.open('" + url.spec() + "')"));
+    EXPECT_TRUE(ExecuteScript(
+        opener, "window.popup = window.open('" + url.spec() + "')"));
     popup_observer.Wait();
     content::WebContents* popup =
         browser()->tab_strip_model()->GetActiveWebContents();
@@ -730,24 +731,27 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
       main_frame->GetProcess()->GetID(),
       GURL("chrome-extension://some-extension-id/resource.html")));
 
-  EXPECT_TRUE(policy->CanCommitURL(
-      extension_frame->GetProcess()->GetID(),
-      GURL("blob:chrome-extension://some-extension-id/some-guid")));
-  EXPECT_FALSE(policy->CanCommitURL(
-      main_frame->GetProcess()->GetID(),
-      GURL("blob:chrome-extension://some-extension-id/some-guid")));
-  EXPECT_TRUE(policy->CanCommitURL(
-      extension_frame->GetProcess()->GetID(),
-      GURL("chrome-extension://some-extension-id/resource.html")));
-  EXPECT_FALSE(policy->CanCommitURL(
-      main_frame->GetProcess()->GetID(),
-      GURL("chrome-extension://some-extension-id/resource.html")));
-  EXPECT_TRUE(policy->CanCommitURL(
-      extension_frame->GetProcess()->GetID(),
-      GURL("filesystem:chrome-extension://some-extension-id/some-path")));
-  EXPECT_FALSE(policy->CanCommitURL(
-      main_frame->GetProcess()->GetID(),
-      GURL("filesystem:chrome-extension://some-extension-id/some-path")));
+  if (IsIsolateExtensionsEnabled() ||
+      content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_TRUE(policy->CanCommitURL(
+        extension_frame->GetProcess()->GetID(),
+        GURL("blob:chrome-extension://some-extension-id/some-guid")));
+    EXPECT_FALSE(policy->CanCommitURL(
+        main_frame->GetProcess()->GetID(),
+        GURL("blob:chrome-extension://some-extension-id/some-guid")));
+    EXPECT_TRUE(policy->CanCommitURL(
+        extension_frame->GetProcess()->GetID(),
+        GURL("chrome-extension://some-extension-id/resource.html")));
+    EXPECT_FALSE(policy->CanCommitURL(
+        main_frame->GetProcess()->GetID(),
+        GURL("chrome-extension://some-extension-id/resource.html")));
+    EXPECT_TRUE(policy->CanCommitURL(
+        extension_frame->GetProcess()->GetID(),
+        GURL("filesystem:chrome-extension://some-extension-id/some-path")));
+    EXPECT_FALSE(policy->CanCommitURL(
+        main_frame->GetProcess()->GetID(),
+        GURL("filesystem:chrome-extension://some-extension-id/some-path")));
+  }
 
   // Open a new about:blank popup from main frame.  This should stay in the web
   // process.
@@ -788,21 +792,39 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   }
 
   // Navigate second subframe to each nested URL from the main frame (i.e.,
-  // from non-extension process).
+  // from non-extension process).  This should be blocked in
+  // --isolate-extensions, but allowed without --isolate-extensions due to
+  // unblessed extension frames.
   //
-  // TODO(alexmos): Currently, this is still allowed due to unblessed extension
-  // contexts, but in the future such subframe navigations from non-extension
-  // processes should be blocked when unblessed contexts go away with
-  // --isolate-extensions.
+  // TODO(alexmos): This is also temporarily allowed under PlzNavigate, because
+  // currently this particular blocking happens in
+  // ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL, which isn't
+  // triggered below under PlzNavigate (since there'll be no transfer).  Once
+  // the blob/filesystem URL checks in ExtensionNavigationThrottle are updated
+  // to apply to all frames and not just main frames, the PlzNavigate exception
+  // below can be removed.

[alexmos, 2016/10/28 00:29:41]

Alternatively, to avoid the confusion with PlzNavigate, we could just skip this
for loop completely for PlzNav until we fix the ExtensionNavigationThrottle blob
checks (that jam@ moved there from ChromeExtensionsNetworkDelegate) to work for
subframes.

   for (size_t i = 0; i < arraysize(nested_urls); i++) {
     EXPECT_TRUE(content::NavigateIframeToURL(tab, "frame2", nested_urls[i]));
     content::RenderFrameHost* second_frame = ChildFrameAt(main_frame, 1);
-    EXPECT_EQ(nested_urls[i], second_frame->GetLastCommittedURL());
-    EXPECT_EQ(extension_origin, second_frame->GetLastCommittedOrigin());
-    EXPECT_EQ("foo", GetTextContent(second_frame));
-    EXPECT_EQ(IfExtensionsIsolated(2, 0),
-              pm->GetRenderFrameHostsForExtension(extension->id()).size());
-    EXPECT_EQ(IfExtensionsIsolated(2, 0), pm->GetAllFrames().size());
+    if (IsIsolateExtensionsEnabled() &&

[alexmos, 2016/10/28 00:29:41]

The reason this starts working (with --isolate-extensions), is because
ShouldAllowOpenURL enforces the block when navigating via a proxy for both main
frames and subframes.  Without --isolate-extensions, there's no proxy, so that
logic doesn't get triggered.

+        !content::IsBrowserSideNavigationEnabled()) {
+      EXPECT_NE(nested_urls[i], second_frame->GetLastCommittedURL());
+      EXPECT_FALSE(extension_origin.IsSameOriginWith(
+          second_frame->GetLastCommittedOrigin()));
+      EXPECT_NE("foo", GetTextContent(second_frame));
+      EXPECT_EQ(IfExtensionsIsolated(1, 0),
+                pm->GetRenderFrameHostsForExtension(extension->id()).size());
+      EXPECT_EQ(IfExtensionsIsolated(1, 0), pm->GetAllFrames().size());
+    } else {
+      EXPECT_EQ(nested_urls[i], second_frame->GetLastCommittedURL());
+      EXPECT_EQ(extension_origin, second_frame->GetLastCommittedOrigin());
+      EXPECT_EQ("foo", GetTextContent(second_frame));
+      EXPECT_EQ(IfExtensionsIsolated(2, 0),
+                pm->GetRenderFrameHostsForExtension(extension->id()).size());
+      EXPECT_EQ(IfExtensionsIsolated(2, 0), pm->GetAllFrames().size());
+    }
+    EXPECT_TRUE(
+        content::NavigateIframeToURL(tab, "frame2", GURL(url::kAboutBlankURL)));
   }
 }
 
@@ -970,6 +992,71 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   }
 }
 
+// Test that a web frame can't navigate a proxy for an extension frame to a
+// blob/filesystem extension URL.  See https://crbug.com/656752.
+IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
+                       NestedURLNavigationsViaProxyBlocked) {
+  // Create a simple extension without a background page.
+  const Extension* extension = CreateExtension("Extension", false);
+  embedded_test_server()->ServeFilesFromDirectory(extension->path());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Navigate main tab to an empty web page.  There should be no extension
+  // frames yet.
+  NavigateToURL(embedded_test_server()->GetURL("/empty.html"));
+  ProcessManager* pm = ProcessManager::Get(profile());
+  EXPECT_EQ(0u, pm->GetAllFrames().size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::RenderFrameHost* main_frame = tab->GetMainFrame();
+
+  // Open a new about:blank popup from main frame.  This should stay in the web
+  // process.
+  content::WebContents* popup =
+      OpenPopup(main_frame, GURL(url::kAboutBlankURL));
+  EXPECT_NE(popup, tab);
+  ASSERT_EQ(2, browser()->tab_strip_model()->count());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+  EXPECT_EQ(0u, pm->GetAllFrames().size());
+
+  // Navigate popup to an extension page.
+  const GURL extension_url(extension->url().Resolve("empty.html"));
+  content::TestNavigationObserver observer(popup);
+  EXPECT_TRUE(
+      ExecuteScript(popup, "location.href = '" + extension_url.spec() + "';"));
+  observer.Wait();
+  EXPECT_EQ(1u, pm->GetAllFrames().size());
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+  content::RenderFrameHost* extension_frame = popup->GetMainFrame();
+
+  // Create valid blob and filesystem URLs in the extension's origin.
+  url::Origin extension_origin(extension_frame->GetLastCommittedOrigin());
+  GURL blob_url(CreateBlobURL(extension_frame, "foo"));
+  EXPECT_EQ(extension_origin, url::Origin(blob_url));
+  GURL filesystem_url(CreateFileSystemURL(extension_frame, "foo"));
+  EXPECT_EQ(extension_origin, url::Origin(filesystem_url));
+
+  // Have the web page navigate the popup to each nested URL with extension
+  // origin via the window reference it obtained earlier from window.open.
+  GURL nested_urls[] = {blob_url, filesystem_url};
+  for (size_t i = 0; i < arraysize(nested_urls); i++) {
+    EXPECT_TRUE(ExecuteScript(
+        tab, "window.popup.location.href = '" + nested_urls[i].spec() + "';"));
+    WaitForLoadStop(popup);
+
+    // This is a top-level navigation that should be blocked since it
+    // originates from a non-extension process.  Ensure that the popup stays at
+    // the original page and doesn't navigate to the nested URL.
+    EXPECT_NE(nested_urls[i], popup->GetLastCommittedURL());
+    EXPECT_NE("foo", GetTextContent(popup->GetMainFrame()));
+
+    EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+    EXPECT_EQ(1u, pm->GetAllFrames().size());
+  }
+}
+
 // Verify that a web popup created via window.open from an extension page can
 // communicate with the extension page via window.opener.  See
 // https://crbug.com/590068.