Fix web accessible resource checks in ShouldAllowOpenURL

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

Index: chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
diff --git a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc b/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
index ef6ee6b46d3390770c47ec4b385629628cf736a9..2758ad314da3e420f27d20868fa0927d534d3d2a 100644
--- a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
+++ b/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
@@ -9,6 +9,8 @@
 #include <set>
 
 #include "base/command_line.h"
+#include "base/debug/alias.h"
+#include "base/debug/dump_without_crashing.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
@@ -496,43 +498,85 @@ bool ChromeContentBrowserClientExtensionsPart::AllowServiceWorker(
 // static
 bool ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL(
     content::SiteInstance* site_instance,
-    const GURL& from_url,
     const GURL& to_url,
     bool* result) {
   DCHECK(result);
 
+  url::Origin to_origin(to_url);
+  if (to_origin.scheme() != kExtensionScheme) {
+    // We're not responsible for protecting this resource.
+    return false;
+  }
+
   // Do not allow pages from the web or other extensions navigate to
   // non-web-accessible extension resources.
-  if (to_url.SchemeIs(kExtensionScheme) &&
-      (from_url.SchemeIsHTTPOrHTTPS() || from_url.SchemeIs(kExtensionScheme))) {

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

Lifting the (bogus) scheme restriction on from_url only affected
ExtensionBrowserTest.WindowOpenNoPrivileges, which was just an invalid test
(attempted to window.open a non-WAR page; the fix is to make it WAR).  However,
I also added a DWOC for cases that we wouldn't have blocked previously due to
this below, so we can catch any regressions.

-    Profile* profile = Profile::FromBrowserContext(
-        site_instance->GetProcess()->GetBrowserContext());
-    ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
-    if (!registry) {
-      *result = true;
-      return true;
-    }
-    const Extension* extension =
-        registry->enabled_extensions().GetExtensionOrAppByURL(to_url);
-    if (!extension) {
-      *result = true;
-      return true;
-    }
-    const Extension* from_extension =
-        registry->enabled_extensions().GetExtensionOrAppByURL(
-            site_instance->GetSiteURL());
-    if (from_extension && from_extension->id() == extension->id()) {
-      *result = true;
-      return true;
-    }
 
-    if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(
-            extension, to_url.path())) {
-      *result = false;
-      return true;
-    }
+  Profile* profile = Profile::FromBrowserContext(
+      site_instance->GetProcess()->GetBrowserContext());
+  ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
+  if (!registry) {
+    *result = true;
+    return true;
   }
-  return false;
+
+  const Extension* extension =
+      registry->enabled_extensions().GetByID(to_origin.host());
+  if (!extension) {
+    *result = true;
+    return true;
+  }
+
+  GURL site_url(site_instance->GetSiteURL());
+  const Extension* from_extension =
+      registry->enabled_extensions().GetExtensionOrAppByURL(site_url);
+  if (from_extension && from_extension->id() == extension->id()) {
+    *result = true;
+    return true;
+  }
+
+  // Blob and filesystem URLs are never considered web-accessible.  See
+  // https://crbug.com/656752.
+  if (to_url.SchemeIsFileSystem() || to_url.SchemeIsBlob()) {
+    // TODO(alexmos): Temporary instrumentation to find any regressions for
+    // this blocking.  Remove after verifying that this is not breaking any
+    // legitimate use cases.
+    char site_url_copy[256];
+    base::strlcpy(site_url_copy, site_url.spec().c_str(),
+                  arraysize(site_url_copy));
+    base::debug::Alias(&site_url_copy);
+    char to_origin_copy[256];
+    base::strlcpy(to_origin_copy, to_origin.Serialize().c_str(),
+                  arraysize(to_origin_copy));
+    base::debug::Alias(&to_origin_copy);
+    base::debug::DumpWithoutCrashing();
+
+    *result = false;
+    return true;
+  }
+
+  if (WebAccessibleResourcesInfo::IsResourceWebAccessible(extension,
+                                                          to_url.path())) {
+    *result = true;
+    return true;
+  }
+
+  if (!site_url.SchemeIsHTTPOrHTTPS() && !site_url.SchemeIs(kExtensionScheme)) {
+    // TODO(alexmos): Previous version of this function skipped the
+    // web-accessible resource checks in this case.  Collect data to catch
+    // any regressions, and then remove this.
+    char site_url_copy[256];
+    base::strlcpy(site_url_copy, site_url.spec().c_str(),
+                  arraysize(site_url_copy));
+    base::debug::Alias(&site_url_copy);
+    char to_origin_copy[256];
+    base::strlcpy(to_origin_copy, to_origin.Serialize().c_str(),
+                  arraysize(to_origin_copy));
+    base::debug::Alias(&to_origin_copy);
+    base::debug::DumpWithoutCrashing();

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

Should we also collect UMA stats for some of these cases, or is the DWOC
sufficient?

[ncarter (slow), 2016/10/28 21:45:03]

Let's add UMA metrics too, since they will be mergeable, unlike these DWOCs.

(An extension-id-valued Rappor trial would actually be ideal for this, but I
don't think we have a drop-in way to create them.)

This is really on the edge of what DWOC is for, but I'm okay with it. In general
we shouldn't be using DWOC to collect metrics about webpage behavior, but the
second DWOC here targets a rare and suspicious enough case that I think it is
okay for canary, and the first is targeted to exactly the behavior we're locking
down.

[alexmos, 2016/10/31 23:34:47]

Metrics added, PTAL.

+  }
+
+  *result = false;
+  return true;
 }
 
 // static