Do not immediately block cross-renderer extension resource loads for non-web-triggered transitions

extensions/browser/url_request_util.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 "extensions/browser/url_request_util.h"
 
 #include <string>
 
 #include "content/public/browser/resource_request_info.h"
 #include "extensions/browser/guest_view/web_view/web_view_renderer_state.h"
@@ skipping 22 matching lines @@
   int owner_process_id;
   WebViewRendererState::GetInstance()->GetOwnerInfo(
       info->GetChildID(), &owner_process_id, &owner_extension_id);
   const Extension* owner_extension =
       extension_info_map->extensions().GetByID(owner_extension_id);
   std::string partition_id;
   bool is_guest = WebViewRendererState::GetInstance()->GetPartitionID(
       info->GetChildID(), &partition_id);
   std::string resource_path = request->url().path();
 
-  // |owner_extension == extension| needs to be checked because extension
-  // resources should only be accessible to WebViews owned by that extension.
-  if (is_guest && owner_extension == extension &&
-      WebviewInfo::IsResourceWebviewAccessible(extension, partition_id,
-                                               resource_path)) {
-    *allowed = true;
+  if (is_guest) {
+    // Extension resources should only be accessible to WebViews owned by that
+    // extension.
+    if (owner_extension != extension)
+      return false;

[nasko, 2016/08/17 16:31:44]

Why this rewrite in this patch? I think the goal is to have it backported to M53
before it hits stable, which means it should be minimally invasive.

[robwu, 2016/08/18 08:09:58]

Without this change, the following test would fail when I remove the page
transition check:
WebViewTests/WebViewTest.Shim_TestLoadAbortChromeExtensionURLWrongPartition
WebViewTests/WebViewPluginTest.TestLoadPluginInternalResource

The transition in these tests is PAGE_TRANSITION_AUTO_TOPLEVEL, which explains
why the requests were previously rejected.

If I follow your suggestion of moving the transition check to the end of the
method, then these tests fail again. So I've restored the check, but limited it
to when |is_guest| is true.

+    *allowed = WebviewInfo::IsResourceWebviewAccessible(extension, partition_id,
+                                                        resource_path);
     return true;
   }
 
-  if (!ui::PageTransitionIsWebTriggerable(info->GetPageTransition())) {

[nasko, 2016/08/17 16:31:44]

As per my response in the bug, I think this should be moved at the end of all
the checks, not completely removed.

[robwu, 2016/08/18 08:09:58]

I restored this check and moved it to the end out of caution, but I do believe
that this check should be removed.
Or, if the check is to be kept, then the renderer-side check should be adjusted
to match the logic here.

-    *allowed = false;
-    return true;
-  }
-
   // The following checks require that we have an actual extension object. If we
   // don't have it, allow the request handling to continue with the rest of the
   // checks.
   if (!extension) {
     *allowed = true;
     return true;
   }
 
   // Disallow loading of packaged resources for hosted apps. We don't allow
   // hybrid hosted/packaged apps. The one exception is access to icons, since
@@ skipping 59 matching lines @@
   const content::ResourceRequestInfo* info =
       content::ResourceRequestInfo::ForRequest(request);
   // |info| can be NULL sometimes: http://crbug.com/370070.
   if (!info)
     return false;
   return WebViewRendererState::GetInstance()->IsGuest(info->GetChildID());
 }
 
 }  // namespace url_request_util
 }  // namespace extensions