Fix web accessible resource checks in ShouldAllowOpenURL

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
 
 #include <stddef.h>
 
 #include <set>
 
 #include "base/command_line.h"
+#include "base/debug/alias.h"
+#include "base/debug/dump_without_crashing.h"
+#include "base/metrics/histogram_macros.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_web_ui.h"
 #include "chrome/browser/extensions/extension_webkit_preferences.h"
 #include "chrome/browser/media_galleries/fileapi/media_file_system_backend.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/chrome_extension_message_filter.h"
 #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h"
@@ skipping 54 matching lines @@
 // below.  Extension, and isolated apps require different privileges to be
 // granted to their RenderProcessHosts.  This classification allows us to make
 // sure URLs are served by hosts with the right set of privileges.
 enum RenderProcessHostPrivilege {
   PRIV_NORMAL,
   PRIV_HOSTED,
   PRIV_ISOLATED,
   PRIV_EXTENSION,
 };
 
+// Specifies reasons why web-accessible resource checks in ShouldAllowOpenURL
+// might fail.
+//
+// This enum backs an UMA histogram.  The order of existing values
+// should not be changed, and new values should only be added before
+// FAILURE_LAST.
+enum ShouldAllowOpenURLFailureReason {
+  FAILURE_FILE_SYSTEM_URL = 0,
+  FAILURE_BLOB_URL,
+  FAILURE_SCHEME_NOT_HTTP_OR_HTTPS_OR_EXTENSION,
+  FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE,
+  FAILURE_LAST,
+};
+
 RenderProcessHostPrivilege GetPrivilegeRequiredByUrl(
     const GURL& url,
     ExtensionRegistry* registry) {
   // Default to a normal renderer cause it is lower privileged. This should only
   // occur if the URL on a site instance is either malformed, or uninitialized.
   // If it is malformed, then there is no need for better privileges anyways.
   // If it is uninitialized, but eventually settles on being an a scheme other
   // than normal webrenderer, the navigation logic will correct us out of band
   // anyways.
   if (!url.is_valid())
@@ skipping 393 matching lines @@
   const Extension* extension =
       extension_info_map->extensions().GetExtensionOrAppByURL(first_party_url);
   // Don't allow a service worker for an extension url with no extension (this
   // could happen in the case of, e.g., an unloaded extension).
   return extension != nullptr;
 }
 
 // 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);

[Charlie Reis, 2016/11/01 18:12:19]

Maybe mention why this is important (for blob/filesystem URLs) in a comment, to
give other developers a reminder?

[alexmos, 2016/11/01 22:04:22]

Done.

+  if (to_origin.scheme() != kExtensionScheme) {

[Charlie Reis, 2016/11/01 18:12:19]

I'm curious-- is |to_url| an effective URL or not?  I'm curious if hosted apps
were handled by this method before.  (The origin's scheme will only be
chrome-extension:// for hosted apps if we're given the effective URL.)

I'm guessing this method should always return true for hosted apps (where
everything is web accessible), but I want to be sure which part of the method is
handling that.  If |to_url| is not an effective URL, we can mention the hosted
app aspect here.  If it is an effective URL, we'll want to mention it on the
GetByID call below.

[alexmos, 2016/11/01 22:04:22]

I think |to_url| is not an effective URL.  For hosted apps, this is returning
false both before and after this CL (i.e., request not handled here, which ends
up returning true in ChromeContentBrowserClient::ShouldAllowOpenURL).  Before,
that happened on the very bottom of the method, because
to_url.SchemeIs(kExtensionScheme) was false.  Now, it happens here.  I added a
comment mentioning that hosted apps would fall into the case.  (I did a quick
sanity-check on this using the Drive app, and this is indeed what seems to
happen.)

[Charlie Reis, 2016/11/01 23:16:27]

Acknowledged.

+    // 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))) {
-    Profile* profile = Profile::FromBrowserContext(
-        site_instance->GetProcess()->GetBrowserContext());
-    ExtensionRegistry* registry = ExtensionRegistry::Get(profile);
-    if (!registry) {
-      *result = true;
-      return true;
+
+  Profile* profile = Profile::FromBrowserContext(
+      site_instance->GetProcess()->GetBrowserContext());
+  ExtensionRegistry* registry = ExtensionRegistry::Get(profile);

[Devlin, 2016/11/02 16:19:22]

Not your code, but ExtensionRegistry doesn't need a profile; you can just pass
BrowserContext in directly here.  Also, let's use
site_instance->GetBrowserContext() rather than going through the RPH.

[alexmos, 2016/11/02 17:32:46]

Done.

+  if (!registry) {

[Devlin, 2016/11/02 16:19:22]

|registry| should never ever be null (even in tests).  Let's make this a CHECK
(or DCHECK), especially since apparently the default behavior is to say "Sure,
this is fiiiiine." :)

[alexmos, 2016/11/02 17:32:46]

Thanks, good to learn about these. :)  I just removed the null check completely.
 Didn't add a CHECK since we'd crash accessing it on the next line anyway.

+    *result = true;
+    return true;
+  }
+
+  const Extension* extension =

[Charlie Reis, 2016/11/01 18:12:19]

nit: to_extension?

[alexmos, 2016/11/01 22:04:22]

Done.

+      registry->enabled_extensions().GetByID(to_origin.host());

[Charlie Reis, 2016/11/01 18:12:19]

Interesting.  As mentioned above, calling GetByID before GetExtensionOrAppByURL
will likely be different for hosted apps, which won't return their extension
object for GetByID.  (That's only if |to_url| is an effective URL.)

If so, let's mention the hosted app aspect here.

[alexmos, 2016/11/01 22:04:22]

We should never get here for hosted apps (we'd return false early above), and I
think the behavior of GetExtensionOrAppByURL and GetByID is the same for all
other cases.  I added a mention about hosted apps to the comment above the
"return false".

+  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()) {
+    if (to_url.SchemeIsFileSystem()) {
+      UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure",
+                                FAILURE_FILE_SYSTEM_URL, FAILURE_LAST);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure",
+                                FAILURE_BLOB_URL, FAILURE_LAST);
     }
-    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;
-    }
+    // 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();
 
-    if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(
-            extension, to_url.path())) {
-      *result = false;
-      return true;
-    }
+    *result = false;
+    return true;
   }
-  return false;
+
+  if (WebAccessibleResourcesInfo::IsResourceWebAccessible(extension,
+                                                          to_url.path())) {
+    *result = true;
+    return true;
+  }
+
+  if (!site_url.SchemeIsHTTPOrHTTPS() && !site_url.SchemeIs(kExtensionScheme)) {
+    UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure",
+                              FAILURE_SCHEME_NOT_HTTP_OR_HTTPS_OR_EXTENSION,
+                              FAILURE_LAST);
+    // 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();
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure",
+                              FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE,
+                              FAILURE_LAST);
+  }
+
+  *result = false;
+  return true;
 }
 
 // static
 std::unique_ptr<content::VpnServiceProxy>
 ChromeContentBrowserClientExtensionsPart::GetVpnServiceProxy(
     content::BrowserContext* browser_context) {
 #if defined(OS_CHROMEOS)
   chromeos::VpnService* vpn_service =
       chromeos::VpnServiceFactory::GetForBrowserContext(browser_context);
   if (!vpn_service)
@@ skipping 141 matching lines @@
     command_line->AppendSwitch(switches::kExtensionProcess);
   }
 }
 
 void ChromeContentBrowserClientExtensionsPart::ResourceDispatcherHostCreated() {
   content::ResourceDispatcherHost::Get()->RegisterInterceptor(
       "Origin", kExtensionScheme, base::Bind(&OnHttpHeaderReceived));
 }
 
 }  // namespace extensions