Refactor of ExtensionNavigationThrottle

extensions/browser/extension_navigation_throttle.cc

 // Copyright 2016 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 "extensions/browser/extension_navigation_throttle.h"
 
 #include "components/guest_view/browser/guest_view_base.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
@@ skipping 16 matching lines @@
 
 ExtensionNavigationThrottle::ExtensionNavigationThrottle(
     content::NavigationHandle* navigation_handle)
     : content::NavigationThrottle(navigation_handle) {}
 
 ExtensionNavigationThrottle::~ExtensionNavigationThrottle() {}
 
 content::NavigationThrottle::ThrottleCheckResult
 ExtensionNavigationThrottle::WillStartRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  GURL url(navigation_handle()->GetURL());
+  const GURL& url = navigation_handle()->GetURL();
+  bool url_has_extension_scheme = url.SchemeIs(kExtensionScheme);

[Devlin, 2017/04/28 22:13:02]

Is it worth a brief comment explaining the significance of
url.SchemeIs(kExtensionScheme) vs Origin(url).scheme() == kExtensionScheme?

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

Done. I eliminated the is_nested_url variable and added comments on lines 47 and
51.

   content::WebContents* web_contents = navigation_handle()->GetWebContents();
   ExtensionRegistry* registry =
       ExtensionRegistry::Get(web_contents->GetBrowserContext());
 
+  // Is this navigation targeting an extension resource?
+  bool is_nested_url = url.SchemeIsFileSystem() || url.SchemeIsBlob();
+  url::Origin target_origin(url);
+  const Extension* target_extension = nullptr;
+  if (url_has_extension_scheme) {
+    target_extension =
+        registry->enabled_extensions().GetExtensionOrAppByURL(url);
+  } else if (is_nested_url && target_origin.scheme() == kExtensionScheme) {
+    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)
+    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 (is_nested_url && !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);
+    guest_view::GuestViewBase* guest =
+        guest_view::GuestViewBase::FromWebContents(web_contents);
+    if (content::IsBrowserSideNavigationEnabled() && url_has_extension_scheme &&
+        guest) {
+      // This logic is performed for PlzNavigate sub-resources and for non-
+      // PlzNavigate in url_request_util::AllowCrossRendererResourceLoad.

[Devlin, 2017/04/28 22:13:02]

"and for non-PlzNavigate" - non-PlzNavigate what?  Main resources?  All
resources?

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

Done.

+      std::string owner_extension_id = guest->owner_host();

[Devlin, 2017/04/28 22:13:02]

Can this be a const std::string&?

(Or even just inline the call in GetByID())

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

Done.

+      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);
+      std::string partition_domain, partition_id;

[Devlin, 2017/04/28 22:13:02]

one variable per line

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

Done.

+      bool in_memory;

[Devlin, 2017/04/28 22:13:02]

initialize

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

Done.

+      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;
-      }
+      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.
+  // 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());
 
-  // 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;
+  // Look to see if all ancestors belong to |target_extension|. If not,
+  // then the web_accessible_resource restriction applies.
+  bool external_ancestor = false;
+  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().IsSameOriginWith(target_origin))
+      continue;
+    // Look for an origin match with the last committed URL. This handles the
+    // case of sandboxed extension resources.

[Devlin, 2017/04/28 22:13:02]

Can you expand on this a bit to include briefly why sandboxed extension
resources need this separate check?

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

Done.

+    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;
 
-  // 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);
-
-  // 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();
-  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();
+    // Otherwise, we have an external ancestor.
+    external_ancestor = true;
+    break;
   }
 
-  if (!external_ancestor)
-    return content::NavigationThrottle::PROCEED;
+  if (external_ancestor) {
+    // Cancel navigations to nested URLs, to match the main frame behavior.
+    if (!url_has_extension_scheme)
+      return content::NavigationThrottle::CANCEL;
 
-  // 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;
-
-  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