Fix Docs links from Files app on ChromeOS not working with PlzNavigate.

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index e9178424ff40acb438828b73b743ab27da641473..aae06d225c55596842442ab7e3e5fea9d6bca2c2 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -1148,9 +1148,13 @@ void ResourceDispatcherHostImpl::BeginRequest(
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(request_data.headers);
 
+  // PlzNavigate: no need to check ShouldServiceRequest at this point. The
+  // browser process already picked the right renderer for the final URL, and
+  // the stream URL is unguessable.
   if (is_shutdown_ ||
-      !ShouldServiceRequest(child_id, request_data, headers, requester_info,
-                            resource_context)) {
+      (!is_navigation_stream_request &&

[Charlie Reis, 2017/05/25 21:28:53]

Why are we turning off a security check rather than granting access to the
external: scheme for renderers that need it?

It's possible this might thread the needle and be safe, but it makes me awfully
nervous to say that the renderer can put whatever it wants in the request as
long as it has a blob URL, since we won't check the rest with
ShouldServiceRequest (e.g., origin header, uploaded files, etc).  It's not
obvious to me whether allowing arbitrary values on other params like that can
cause damage if we let them through, even if we know it's limited to blob URLs.

Is there some way that the external filesystem code could grant access to the
scheme instead when the browser process knows we need it?

[jam, 2017/05/25 21:45:17]

To be clear: this isn't skipped for "blob urls". This is only being skipped for
navigation stream URLS, which happen to use blob scheme. Origin header, uploaded
files etc are not relevant in this case. The code (which is split across two
methods) will simply return the data that maps to that url which has already
been fetched.

[jam, 2017/05/25 21:47:57]

also: once we use mojo for loading, and consequently data pipe for plznavigate,
then this IPC wouldn't exist. the data pipe would be sent in the commit IPC to
the renderer.

So right now, if all this IPC does is map stream blob URL->data, that is
functionally equivalent.

[jam, 2017/05/25 22:00:41]

Sorry forgot to reply to this: I think in this example, we would not want the
renderer to have access to the original scheme. The renderer is not getting data
from the original scheme; it's getting it from the new scheme after the
redirect. 

[Charlie Reis, 2017/05/25 23:11:01]

Interesting.  Then I'm confused about why the request is failing today.  Does
the stream URL itself contain the original external scheme rather than the Docs
URL?  If so, is that the bug, and could we make the stream URL have the Docs URL
inside it instead?

On 2017/05/25 21:47:57, jam wrote:
This is a great point.  There's no risk if we can eliminate the step where the
renderer asks for the stream URL.
Not sure I agree; see below.

On 2017/05/25 21:45:17, jam wrote:
I don't see how the is_navigation_stream_request case is automatically safe.  It
seems to go down the same ContinuePendingBeginRequest path with a few special
cases (e.g., referrer), but most of the other parameters on the request still
seem to be in place.

I agree with you now that we maybe shouldn't grant access to the scheme.  But
could we do a better job minimizing the renderer's ability to influence the
is_navigation_stream_request case?  That would make me much more comfortable
with the idea of skipping ShouldServiceRequest.

For example, in the is_navigation_stream_request case, could we construct a new
set of request parameters from scratch and copy over the bare minimum from the
renderer's request, such as the stream URL and request ID?  The rest of the
parameters would be blank, and thus wouldn't need sanitizing.

I'd be ok with either that, or fixing the renderer to not use the external
scheme in the stream request in the first place.

+       !ShouldServiceRequest(child_id, request_data, headers, requester_info,
+                             resource_context))) {
     AbortRequestBeforeItStarts(requester_info->filter(), sync_result_handler,
                                request_id, std::move(url_loader_client));
     return;