Refactor of ExtensionNavigationThrottle

extensions/browser/extension_navigation_throttle.cc

Index: extensions/browser/extension_navigation_throttle.cc
diff --git a/extensions/browser/extension_navigation_throttle.cc b/extensions/browser/extension_navigation_throttle.cc
index 0c06bd83cffa8d41a921eba9972088fcdd0a4561..9d207ab2f6974092c35a2b97df75423eef135591 100644
--- a/extensions/browser/extension_navigation_throttle.cc
+++ b/extensions/browser/extension_navigation_throttle.cc
@@ -34,130 +34,128 @@ ExtensionNavigationThrottle::~ExtensionNavigationThrottle() {}
 content::NavigationThrottle::ThrottleCheckResult
 ExtensionNavigationThrottle::WillStartRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  GURL url(navigation_handle()->GetURL());
   content::WebContents* web_contents = navigation_handle()->GetWebContents();
   ExtensionRegistry* registry =
       ExtensionRegistry::Get(web_contents->GetBrowserContext());
 
+  // Is this navigation targeting an extension resource?
+  const GURL& url = navigation_handle()->GetURL();
+  bool url_has_extension_scheme = url.SchemeIs(kExtensionScheme);
+  url::Origin target_origin(url);
+  const Extension* target_extension = nullptr;
+  if (url_has_extension_scheme) {
+    // "chrome-extension://" URL.
+    target_extension =
+        registry->enabled_extensions().GetExtensionOrAppByURL(url);
+  } else if (target_origin.scheme() == kExtensionScheme) {
+    // "blob:chrome-extension://" or "filesystem:chrome-extension://" URL.
+    DCHECK(url.SchemeIsFileSystem() || url.SchemeIsBlob());
+    target_extension =
+        registry->enabled_extensions().GetByID(target_origin.host());
+  } else {
+    // If the navigation is not to a chrome-extension resource, no need to
+    // perform any more checks; it's outside of the purview of this throttle.
+    return content::NavigationThrottle::PROCEED;
+  }
+
+  // If the navigation is to an unknown or disabled extension, block it.
+  if (!target_extension) {
+    if (url_has_extension_scheme) {
+      // Proceed in this case; the protocol handler will surface an error.
+      return content::NavigationThrottle::PROCEED;

[ncarter (slow), 2017/05/01 21:50:37]

Note that this PROCEED case is new vs. the previous patch set. This is to fix
the WindowOpenInvalidExtension browsertest under PlzNavigate, and it also
preserves the text content of the ERR_FAILED error page that results in this
case (at least, it preserves it when neterror.js is not blocked by CSP!). The
text on ERR_FAILED is more appropriate.

[Devlin, 2017/05/01 22:04:28]

Hmm... is there no way we could surface a useful error while keeping the check
here?  This just strikes me as unfortunate that it seems to amount to:
// We know this is unsafe, but we have another check that will hopefully catch
this.
return PROCEED;

If that other check ever changes, we're in bad shape.

[ncarter (slow), 2017/05/02 20:14:15]

Note that the PROCEED logic just matches the original behavior for main frame
navigations. So this behavior has existed for some time.

Having said that, I agree: we ought to block this here; let me tackle this in a
follow-on CL. I think there may be a PlzNavigate bug, and possibly a CSP bug,
confounding this. The ExtensionApiTest.WindowOpenInvalidExtension is sensitive
to both of these, and I'm still digging into what's going on there.

[ncarter (slow), 2017/05/02 23:53:15]

My prev comment was incorrect (since the PROCEED is in fact new for subframes,
which the unittests show). I'll figure out a way forward and ping this review
when I have a new CL ready.

+    }
+    // Explicitly block blob: and filesystem: URLs with unknown extensions.
+    // Nothing good can come of those.
+    return content::NavigationThrottle::BLOCK_REQUEST;
+  }
+
   if (navigation_handle()->IsInMainFrame()) {
     // Block top-level navigations to blob: or filesystem: URLs with extension
     // origin from non-extension processes.  See https://crbug.com/645028.
-    bool is_nested_url = url.SchemeIsFileSystem() || url.SchemeIsBlob();
-    bool is_extension = false;
-    if (registry) {
-      is_extension = !!registry->enabled_extensions().GetExtensionOrAppByURL(
-          navigation_handle()->GetStartingSiteInstance()->GetSiteURL());
-    }
+    bool current_frame_is_extension_process =
+        !!registry->enabled_extensions().GetExtensionOrAppByURL(
+            navigation_handle()->GetStartingSiteInstance()->GetSiteURL());
 
-    url::Origin origin(url);
-    if (is_nested_url && origin.scheme() == extensions::kExtensionScheme &&
-        !is_extension) {
+    if (!url_has_extension_scheme && !current_frame_is_extension_process) {
       // Relax this restriction for apps that use <webview>.  See
       // https://crbug.com/652077.
-      const extensions::Extension* extension =
-          registry->enabled_extensions().GetByID(origin.host());
       bool has_webview_permission =
-          extension &&
-          extension->permissions_data()->HasAPIPermission(
-              extensions::APIPermission::kWebView);
+          target_extension->permissions_data()->HasAPIPermission(
+              APIPermission::kWebView);
       if (!has_webview_permission)
         return content::NavigationThrottle::CANCEL;
     }
 
-    if (content::IsBrowserSideNavigationEnabled() &&
-        url.scheme() == extensions::kExtensionScheme) {
-      // This logic is performed for PlzNavigate sub-resources and for
-      // non-PlzNavigate in
-      // extensions::url_request_util::AllowCrossRendererResourceLoad.
-      const Extension* extension =
-          registry->enabled_extensions().GetExtensionOrAppByURL(url);
-      guest_view::GuestViewBase* guest =
-          guest_view::GuestViewBase::FromWebContents(web_contents);
-      if (guest) {
-        std::string owner_extension_id = guest->owner_host();
-        const Extension* owner_extension =
-            registry->enabled_extensions().GetByID(owner_extension_id);
-
-        std::string partition_domain, partition_id;
-        bool in_memory;
-        std::string resource_path = url.path();
-        bool is_guest = WebViewGuest::GetGuestPartitionConfigForSite(
-            navigation_handle()->GetStartingSiteInstance()->GetSiteURL(),
-            &partition_domain, &partition_id, &in_memory);
-
-        bool allowed = true;
-        url_request_util::AllowCrossRendererResourceLoadHelper(
-            is_guest, extension, owner_extension, partition_id, resource_path,
-            navigation_handle()->GetPageTransition(), &allowed);
-        if (!allowed)
-          return content::NavigationThrottle::BLOCK_REQUEST;
-      }
+    guest_view::GuestViewBase* guest =
+        guest_view::GuestViewBase::FromWebContents(web_contents);
+    if (content::IsBrowserSideNavigationEnabled() && url_has_extension_scheme &&
+        guest) {
+      // This variant of this logic applies to PlzNavigate top-level
+      // navigations. It is performed for subresources, and for non-PlzNavigate
+      // top navigations, in url_request_util::AllowCrossRendererResourceLoad.
+      const std::string& owner_extension_id = guest->owner_host();
+      const Extension* owner_extension =
+          registry->enabled_extensions().GetByID(owner_extension_id);
+
+      std::string partition_domain;
+      std::string partition_id;
+      bool in_memory = false;
+      bool is_guest = WebViewGuest::GetGuestPartitionConfigForSite(
+          navigation_handle()->GetStartingSiteInstance()->GetSiteURL(),
+          &partition_domain, &partition_id, &in_memory);
+
+      bool allowed = true;
+      url_request_util::AllowCrossRendererResourceLoadHelper(
+          is_guest, target_extension, owner_extension, partition_id, url.path(),
+          navigation_handle()->GetPageTransition(), &allowed);
+      if (!allowed)
+        return content::NavigationThrottle::BLOCK_REQUEST;
     }
 
     return content::NavigationThrottle::PROCEED;
   }
 
-  // Now enforce web_accessible_resources for navigations. Top-level navigations
-  // should always be allowed.
-
-  // If the navigation is not to a chrome-extension:// URL, no need to perform
-  // any more checks.
-  if (!url.SchemeIs(extensions::kExtensionScheme))
-    return content::NavigationThrottle::PROCEED;
-
-  // The subframe which is navigated needs to have all of its ancestors be
-  // at the same origin, otherwise the resource needs to be explicitly listed
-  // in web_accessible_resources.
-  // Since the RenderFrameHost is not known until navigation has committed,
-  // we can't get it from NavigationHandle. However, this code only cares about
-  // the ancestor chain, so find the current RenderFrameHost and use it to
-  // traverse up to the main frame.
-  content::RenderFrameHost* navigating_frame = nullptr;
-  for (auto* frame : web_contents->GetAllFrames()) {
-    if (frame->GetFrameTreeNodeId() ==
-        navigation_handle()->GetFrameTreeNodeId()) {
-      navigating_frame = frame;
-      break;
-    }
-  }
-  DCHECK(navigating_frame);
+  // This is a subframe navigation to a |target_extension| resource.
+  // Enforce the web_accessible_resources restriction.
+  content::RenderFrameHost* parent = web_contents->FindFrameByFrameTreeNodeId(
+      navigation_handle()->GetParentFrameTreeNodeId());
 
-  // Traverse the chain of parent frames, checking if they are the same origin
-  // as the URL of this navigation.
-  content::RenderFrameHost* ancestor = navigating_frame->GetParent();
+  // Look to see if all ancestors belong to |target_extension|. If not,
+  // then the web_accessible_resource restriction applies.
   bool external_ancestor = false;
-  while (ancestor) {
-    if (ancestor->GetLastCommittedURL().GetOrigin() != url.GetOrigin()) {
-      // Ignore DevTools, as it is allowed to embed extension pages.
-      if (!ancestor->GetLastCommittedURL().SchemeIs(
-              content::kChromeDevToolsScheme)) {
-        external_ancestor = true;
-        break;
-      }
-    }
-    ancestor = ancestor->GetParent();
+  for (auto* ancestor = parent; ancestor; ancestor = ancestor->GetParent()) {
+    // Look for a match on the last committed origin. This handles the
+    // common case, and the about:blank case.
+    if (ancestor->GetLastCommittedOrigin() == target_origin)
+      continue;
+    // Look for an origin match with the last committed URL. This handles the
+    // case of sandboxed extension resources, which commit with a null origin,
+    // but are permitted to load non-webaccessible extension resources in
+    // subframes.
+    if (url::Origin(ancestor->GetLastCommittedURL()) == target_origin)
+      continue;
+    // Ignore DevTools, as it is allowed to embed extension pages.
+    if (ancestor->GetLastCommittedURL().SchemeIs(
+            content::kChromeDevToolsScheme))
+      continue;
+
+    // Otherwise, we have an external ancestor.
+    external_ancestor = true;
+    break;
   }
 
-  if (!external_ancestor)
-    return content::NavigationThrottle::PROCEED;
-
-  // Since there was at least one origin different than the navigation URL,
-  // explicitly check for the resource in web_accessible_resources.
-  std::string resource_path = url.path();
-  if (!registry)
-    return content::NavigationThrottle::BLOCK_REQUEST;
+  if (external_ancestor) {
+    // Cancel navigations to nested URLs, to match the main frame behavior.

[Devlin, 2017/05/01 22:04:28]

Is this difference (CANCEL vs BLOCK_REQUEST) visible to script?  (We want to
make sure there's no way of detecting if an extension is installed by looking at
the result of a request to a non-web-accessible-resource)

[ncarter (slow), 2017/05/01 22:42:53]

Nested URLs (blob and filesystem) are classified as local only schemes -- blink
won't actually let you use them in any way, unless you're already in the context
of that origin:

% location.href = "blob:chrome-extension://lalalala/foo.html"
"blob:chrome-extension://lalalala/foo.html"
% (index):1 Not allowed to load local resource:
blob:chrome-extension://lalalala/foo.html

So in browsertests, these cases are exercisable only via --disable-web-security.

===

There are two reasons we might use CANCEL instead of BLOCK: 
 (1) when neterror.js executes in the context of an extension CSP, it shows a
blank page with 8+ CSP errors. I don't know if these CSP errors bubble (as
securitypolicyviolation events) to the parent document or not -- they might. You
can see this for yourself if you run
ExtensionBrowserTest.WindowOpenInvalidExtension. However, we really just ought
to fix the interaction of neterror.js with CSP.
 (2) When we're seeing something that a well-behaved renderer should never do,
or to a target URL that's inherently unsafe, CANCEL is a safer option than
BLOCK, because it suppresses the commit of the potentially dangerous URL (if we
commit the error page, we might still transfer it to an extensioney process). On
the other hand, CANCEL is the more developer-hostile behavior, since navigations
just disappear with no explanation.

FWIW, I floated the idea with Nasko of having the navigation throttles
optionally produce console messages, and he was on board with it.

[Devlin, 2017/05/02 01:52:09]

Okay, I think that makes sense, as long as there's no way (excluding a bug) for
a renderer to cause a navigation that results in a cancellation.  Thanks for the
explanation!

+    if (!url_has_extension_scheme)
+      return content::NavigationThrottle::CANCEL;
 
-  const extensions::Extension* extension =
-      registry->enabled_extensions().GetByID(url.host());
-  if (!extension)
-    return content::NavigationThrottle::BLOCK_REQUEST;
-
-  if (WebAccessibleResourcesInfo::IsResourceWebAccessible(extension,
-                                                          resource_path)) {
-    return content::NavigationThrottle::PROCEED;
+    // |url| must be in the manifest's "web_accessible_resources" section.
+    if (!WebAccessibleResourcesInfo::IsResourceWebAccessible(target_extension,
+                                                             url.path()))
+      return content::NavigationThrottle::BLOCK_REQUEST;
   }
 
-  return content::NavigationThrottle::BLOCK_REQUEST;
+  return content::NavigationThrottle::PROCEED;
 }
 
 }  // namespace extensions