PlzNavigate: fix issue preventing navigations to WebUIs

content/browser/webui/url_data_manager_backend.cc

Index: content/browser/webui/url_data_manager_backend.cc
diff --git a/content/browser/webui/url_data_manager_backend.cc b/content/browser/webui/url_data_manager_backend.cc
index fe1015f87e445c77ae396386df9182b94fc81cd0..d3d9c30ac4dd44792bcf8a590c665de998ed37f3 100644
--- a/content/browser/webui/url_data_manager_backend.cc
+++ b/content/browser/webui/url_data_manager_backend.cc
@@ -24,7 +24,11 @@
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/navigation_request.h"
+#include "content/browser/frame_host/navigator.h"
 #include "content/browser/histogram_internals_request_job.h"
+#include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/net/view_blob_internals_job_factory.h"
 #include "content/browser/net/view_http_cache_job_factory.h"
 #include "content/browser/resource_context_impl.h"
@@ -35,6 +39,7 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -185,6 +190,7 @@ class URLRequestChromeJob : public net::URLRequestJob {
   // Called on the UI thread to check if this request is allowed.
   static void CheckStoragePartitionMatches(
       int render_process_id,
+      int frame_tree_node_id,
       const GURL& url,
       const base::WeakPtr<URLRequestChromeJob>& job);
 
@@ -291,11 +297,13 @@ void URLRequestChromeJob::Start() {
     }
   }
 
+  const ResourceRequestInfoImpl* info =
+      ResourceRequestInfoImpl::ForRequest(request_);
+  DCHECK(info);
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
+      BrowserThread::UI, FROM_HERE,
       base::Bind(&URLRequestChromeJob::CheckStoragePartitionMatches,
-                 render_process_id, url,
+                 render_process_id, info->frame_tree_node_id(), request_->url(),
                  weak_factory_.GetWeakPtr()));
 }
 
@@ -408,6 +416,7 @@ int URLRequestChromeJob::CompleteRead(net::IOBuffer* buf, int buf_size) {
 
 void URLRequestChromeJob::CheckStoragePartitionMatches(
     int render_process_id,
+    int frame_tree_node_id,
     const GURL& url,
     const base::WeakPtr<URLRequestChromeJob>& job) {
   // The embedder could put some webui pages in separate storage partition.
@@ -421,6 +430,23 @@ void URLRequestChromeJob::CheckStoragePartitionMatches(
     StoragePartition* partition = BrowserContext::GetStoragePartitionForSite(
         process->GetBrowserContext(), url);
     allowed = partition == process->GetStoragePartition();
+  } else if (render_process_id == -1 && IsBrowserSideNavigationEnabled()) {

[Dan Beam, 2016/05/23 19:02:58]

when will render_process_id be -1 here?

[Charlie Reis, 2016/05/23 19:11:16]

With PlzNavigate, the renderer process may not be created until after the
network request is made and is ready to commit.  See https://crbug.com/368813.

[Dan Beam, 2016/05/23 19:17:09]

are you saying that ResourceRequestInfo::GetRenderFrameForRequest() can return
true and also set render_process_id = -1?

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...

[Charlie Reis, 2016/05/23 19:50:10]

That's what patch set 1 implies.  I agree that seems a bit unexpected, looking
at the implementation of GetRenderFrameForRequest.  Camille, does PlzNavigate
install the URLRequestUserData on the request with -1 for the process ID and
frame routing ID?

[Dan Beam, 2016/05/23 20:41:09]

I removed a constant in my previous CL that might be useful here because I
thought we'd never hit a case where we have a render_process_id of -1

https://codereview.chromium.org/1985983002/diff/60001/content/browser/webui/u...

[clamy, 2016/05/24 11:18:32]

PlzNavigate does indeed creates a URLRequestUserData for the request. It is
automatically created when we try to associate the ResourceRequestInfoImpl to
the request, by checking that we have a RFH and RPH id. The function checking
that always return true, and does not check that these ids are valid. Should I
change it?

[Charlie Reis, 2016/05/24 22:14:32]

Yeah, we could reintroduce that old path for allowing the request when the
process ID is -1 (as in patch set 1), but that was meant for the case that
"Request was not issued by renderer."  In PlzNavigate, the process ID will be -1
even when it was issued by the renderer, which is why I was hesitant to add it
back.
I don't know that code well enough to say.  It seems a bit surprising for
GetRenderFrameForRequest to return true and give back -1s, but I don't know if
that's a problem for any code.

[clamy, 2016/05/25 14:30:11]

I'm a bit wary of changing the behavior of
ResourceRequestInfo::GetRenderFrameForRequest() since there are othe r request
which can have ids of -1 (eg browser-initiated downloads).

My understanding is that the goal of the patch is to check using the storage
partition that a renderer is allowed to access data from a webui or not.

In PlzNavigate we have 2 use cases:
- the normal navigation request, which goes through the browser and creates a
NavigationRequest (which creates a ResourceRequest with rph id -1)
- the resource request made by the render to get the response body. We enforce
in ResourceDispatcherHost that any frame ResourceRequest made by a renderer has
to be for a blob url. So they woudln't be concerned by that check.

To handle case 1, I wrote a new version of the patch set, where when we're ready
to commit we check if the renderer we want to use has the right to display the
webui url, and cancel the navigation if not.

+    // PlzNavigate: there should be no process associated to the navigation.
+    // Instead, lookup the FrameTreeNode to access the NavigationRequest.
+    FrameTreeNode* frame_tree_node =
+        FrameTreeNode::GloballyFindByID(frame_tree_node_id);
+    if (frame_tree_node) {
+      NavigationRequest* request = frame_tree_node->navigation_request();
+      if (request) {
+        StoragePartition* partition =
+            BrowserContext::GetStoragePartitionForSite(
+                frame_tree_node->navigator()
+                    ->GetController()
+                    ->GetBrowserContext(),
+                url);
+        allowed = partition == request->GetStoragePartitionForNavigation();
+      }
+    }
   }
   BrowserThread::PostTask(
       BrowserThread::IO,