PlzNavigate: fix issue preventing navigations to WebUIs

content/browser/webui/url_data_manager_backend.cc

 // Copyright (c) 2012 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 "content/browser/webui/url_data_manager_backend.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/debug/alias.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #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"
 #include "content/browser/webui/shared_resources_data_source.h"
 #include "content/browser/webui/url_data_source_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #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"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/log/net_log_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_error_job.h"
 #include "net/url_request/url_request_job.h"
@@ skipping 130 matching lines @@
  private:
   ~URLRequestChromeJob() override;
 
   // Helper for Start(), to let us start asynchronously.
   // (This pattern is shared by most net::URLRequestJob implementations.)
   void StartAsync(bool allowed);
 
   // 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);
 
   // Specific resources require unsafe-eval in the Content Security Policy.
   bool RequiresUnsafeEval() const;
 
   // Do the actual copy from data_ (the data we're serving) into |buf|.
   // Separate from ReadRawData so we can handle async I/O. Returns the number of
   // bytes read.
   int CompleteRead(net::IOBuffer* buf, int buf_size);
@@ skipping 86 matching lines @@
     std::vector<std::string> hosts;
     hosts.push_back(content::kChromeUIResourcesHost);
     GetContentClient()->
         browser()->GetAdditionalWebUIHostsToIgnoreParititionCheck(&hosts);
     if (std::find(hosts.begin(), hosts.end(), url.host()) != hosts.end()) {
       StartAsync(true);
       return;
     }
   }
 
+  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()));
 }
 
 void URLRequestChromeJob::Kill() {
   weak_factory_.InvalidateWeakPtrs();
   backend_->RemoveRequest(this);
   URLRequestJob::Kill();
 }
 
 bool URLRequestChromeJob::GetMimeType(std::string* mime_type) const {
@@ skipping 92 matching lines @@
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "455423 URLRequestChromeJob::CompleteRead memcpy"));
     memcpy(buf->data(), data_->front() + data_offset_, buf_size);
     data_offset_ += buf_size;
   }
   return 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.
   // RenderProcessHostImpl::IsSuitableHost would guard against top level pages
   // being in the same process. We do an extra check to guard against an
   // exploited renderer pretending to add them as a subframe. We skip this check
   // for resources.
   bool allowed = false;
   RenderProcessHost* process = RenderProcessHost::FromID(render_process_id);
   if (process) {
     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,
       FROM_HERE,
       base::Bind(&URLRequestChromeJob::StartAsync, job, allowed));
 }
 
 void URLRequestChromeJob::StartAsync(bool allowed) {
   if (!request_)
     return;
@@ skipping 369 matching lines @@
 
 }  // namespace
 
 net::URLRequestJobFactory::ProtocolHandler*
 CreateDevToolsProtocolHandler(content::ResourceContext* resource_context,
                               bool is_incognito) {
   return new DevToolsJobFactory(resource_context, is_incognito);
 }
 
 }  // namespace content