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

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

The cause was that there was a redirect from externalfile scheme to https. The PlzNavigate code chose the right ending process based on the final URL. However when the renderer made a request for the stream URL, it does attach the original URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was incorrectly aborting the URL because that process rightly does not have access to the externalfile scheme.

The fix is to skip the ChildProcessSecurityPolicy::CanRequestURL check in ShouldServiceRequest for the PlzNavigate stream request. The browser already picked the right process, and the stream URL is unguessable. The change also skips most of ContinuePendingBeginRequest() and CreateResourceHandler() for PlzNavigate stream requests to minimize the risk that they contain code that could be exploited.

BUG=717644
Review-Url: https://codereview.chromium.org/2908593002
Cr-Commit-Position: refs/heads/master@{#475097}
Committed: https://chromium.googlesource.com/chromium/src/+/ecb25eb8968a45120ee434af2d3da30baebc6b63

[jam, 2017-05-25 20:37:46]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 20:38:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-05-25 20:57:02]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[jam, 2017-05-25 20:57:03]

pta, working on a test in meantime

[jam, 2017-05-25 21:00:44]

Description was changed from

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

The cause was that there was a redirect from externalfile scheme to https. The
PlzNavigate code chose the right ending process based on the final URL. However
when the renderer made a request for the stream URL, it does attach the original
URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was
incorrectly aborting the URL because that process rightly does not have access
to the externalfile scheme.

The fix is to skip the ShouldServiceRequest for the PlzNavigate stream request.
The browser already picked the right process, and the stream URL is unguessable.

BUG=717644
==========

to

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

The cause was that there was a redirect from externalfile scheme to https. The
PlzNavigate code chose the right ending process based on the final URL. However
when the renderer made a request for the stream URL, it does attach the original
URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was
incorrectly aborting the URL because that process rightly does not have access
to the externalfile scheme.

The fix is to skip the ShouldServiceRequest for the PlzNavigate stream request.
The browser already picked the right process, and the stream URL is unguessable.

BUG=717644
==========

[jam, 2017-05-25 21:00:45]

jam@chromium.org changed reviewers:
+ creis@chromium.org

[jam, 2017-05-25 21:00:57]

+Charlie as well, since this is a replacement fix for
https://codereview.chromium.org/2897963003/

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

Thanks for looking into this!  One concern below.

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1155:
(!is_navigation_stream_request &&
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]

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1155:
(!is_navigation_stream_request &&
On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> 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.

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.

> 
> 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:47:57]

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1155:
(!is_navigation_stream_request &&
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]

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1155:
(!is_navigation_stream_request &&
On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> 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?

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.

[nasko, 2017-05-25 22:03:36]

On 2017/05/25 22:00:41, jam wrote:
>
https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
> content/browser/loader/resource_dispatcher_host_impl.cc:1155:
> (!is_navigation_stream_request &&
> On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> > 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?
> 
> 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.

We need to better understand why we see the original URL in the ResourceRequest
since we have had redirects. Are we applying the redirects after creating the
ResourceRequest?
I guess I'm find it strange to get a stream request for the original URL, not
the final redirect. If the two were to match, we wouldn't have this problem,
correct?

[commit-bot: I haz the power, 2017-05-25 22:23:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 22:23:55]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-25 22:46:07]

On 2017/05/25 22:03:36, nasko wrote:
> On 2017/05/25 22:00:41, jam wrote:
> >
>
https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
> > content/browser/loader/resource_dispatcher_host_impl.cc:1155:
> > (!is_navigation_stream_request &&
> > On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> > > 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?
> > 
> > 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.
> 
> We need to better understand why we see the original URL in the
ResourceRequest
> since we have had redirects. Are we applying the redirects after creating the
> ResourceRequest?
> I guess I'm find it strange to get a stream request for the original URL, not
> the final redirect. If the two were to match, we wouldn't have this problem,
> correct?

Per our VC earlier, this is sent by blink (which is what makes the request) when
it's told a navigation for foo.com started. afterwards the plznavigate code
replays the redirects until it ends at the final url. 

The original URL is not actually used in the browser when it comes for the
stream request. The only url that is used is the stream URL.

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

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:1155:
(!is_navigation_stream_request &&
On 2017/05/25 22:00:41, jam wrote:
> On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> > 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?
> 
> 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. 

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:
> 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.

This is a great point.  There's no risk if we can eliminate the step where the
renderer asks for the stream URL.

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

Not sure I agree; see below.

On 2017/05/25 21:45:17, jam wrote:
> On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> > 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.
> 
> 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.

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.

[Charlie Reis, 2017-05-25 23:12:18]

On 2017/05/25 22:46:07, jam wrote:
> On 2017/05/25 22:03:36, nasko wrote:
> > On 2017/05/25 22:00:41, jam wrote:
> > >
> >
>
https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
> > > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2908593002/diff/1/content/browser/loader/reso...
> > > content/browser/loader/resource_dispatcher_host_impl.cc:1155:
> > > (!is_navigation_stream_request &&
> > > On 2017/05/25 21:28:53, Charlie Reis (overloaded) wrote:
> > > > 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?
> > > 
> > > 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.
> > 
> > We need to better understand why we see the original URL in the
> ResourceRequest
> > since we have had redirects. Are we applying the redirects after creating
the
> > ResourceRequest?
> > I guess I'm find it strange to get a stream request for the original URL,
not
> > the final redirect. If the two were to match, we wouldn't have this problem,
> > correct?
> 
> Per our VC earlier, this is sent by blink (which is what makes the request)
when
> it's told a navigation for http://foo.com started. afterwards the plznavigate
code
> replays the redirects until it ends at the final url. 
> 
> The original URL is not actually used in the browser when it comes for the
> stream request. The only url that is used is the stream URL.

Ah, sorry, ignore my question about the stream URL, since I didn't see this part
before my reply.

[jam, 2017-05-26 01:19:06]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 01:20:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-05-26 02:05:04]

On 2017/05/25 23:11:01, Charlie Reis (OOO until 5-30) wrote:
> 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.

It turns out that a number of minor fields are used, i.e. IDs, load flags etc..
So what I've done is
1) call most of ShouldServiceRequest (other than the CanRequestURL call)
2) skipped most of the code in ContinuePendingBeginRequest and
CreateResourceHandler

That restores more of the security checks, and skips most of the code that can
be using parts of |request_data|.

[jam, 2017-05-26 02:05:16]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 02:06:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 06:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 06:02:42]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-26 17:25:57]

Description was changed from

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

The cause was that there was a redirect from externalfile scheme to https. The
PlzNavigate code chose the right ending process based on the final URL. However
when the renderer made a request for the stream URL, it does attach the original
URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was
incorrectly aborting the URL because that process rightly does not have access
to the externalfile scheme.

The fix is to skip the ShouldServiceRequest for the PlzNavigate stream request.
The browser already picked the right process, and the stream URL is unguessable.

BUG=717644
==========

to

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

The cause was that there was a redirect from externalfile scheme to https. The
PlzNavigate code chose the right ending process based on the final URL. However
when the renderer made a request for the stream URL, it does attach the original
URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was
incorrectly aborting the URL because that process rightly does not have access
to the externalfile scheme.

The fix is to skip the ChildProcessSecurityPolicy::CanRequestURL check in
ShouldServiceRequest for the PlzNavigate stream request. The browser already
picked the right process, and the stream URL is unguessable. The change also
skips most of ContinuePendingBeginRequest() and CreateResourceHandler() for
PlzNavigate stream requests to minimize the risk that they contain code that
could be exploited.

BUG=717644
==========

[nasko, 2017-05-26 18:18:14]

Couple of notes that make the code a bit easier to follow, but overall I think
it does the right thing.

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (left):

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1113: }
Why not keep this early return here? It will make sure no code further down
executes if that's the case.
Then the blocks you refactored below can be just made "if
(!is_navigation_stream_request) { ... }".

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1103: bool
is_navigation_stream_request =
nit: Moving this down to where it is checked will make it easier to find.
Nothing between this line and the block that moved is using it.

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1525: return handler;
Do we still need this block here? It is checking for the case of
is_navigation_stream_request, which if true will not enter this function at all.

[jam, 2017-05-26 18:26:00]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 18:26:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-05-26 18:27:05]

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (left):

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1113: }
On 2017/05/26 18:18:14, nasko wrote:
> Why not keep this early return here? It will make sure no code further down
> executes if that's the case.
> Then the blocks you refactored below can be just made "if
> (!is_navigation_stream_request) { ... }".

I thought it's slightly better to have 1 if statement than 2. but it didn't end
up making much of a difference, so restored. Done.

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1103: bool
is_navigation_stream_request =
On 2017/05/26 18:18:14, nasko wrote:
> nit: Moving this down to where it is checked will make it easier to find.
> Nothing between this line and the block that moved is using it.

(not relevant anymore that I did your other comment)

https://codereview.chromium.org/2908593002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1525: return handler;
On 2017/05/26 18:18:14, nasko wrote:
> Do we still need this block here? It is checking for the case of
> is_navigation_stream_request, which if true will not enter this function at
all.

Done.

[jam, 2017-05-26 19:32:51]

The CQ bit was unchecked by jam@chromium.org

[nasko, 2017-05-26 19:39:30]

LGTM

[jam, 2017-05-26 19:44:09]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-05-26 19:44:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 20:06:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495827849104960,
"parent_rev": "ae5decc21d826351ebb953cab11c0014ea35585d", "commit_rev":
"ecb25eb8968a45120ee434af2d3da30baebc6b63"}

[commit-bot: I haz the power, 2017-05-26 20:06:28]

Description was changed from

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

The cause was that there was a redirect from externalfile scheme to https. The
PlzNavigate code chose the right ending process based on the final URL. However
when the renderer made a request for the stream URL, it does attach the original
URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was
incorrectly aborting the URL because that process rightly does not have access
to the externalfile scheme.

The fix is to skip the ChildProcessSecurityPolicy::CanRequestURL check in
ShouldServiceRequest for the PlzNavigate stream request. The browser already
picked the right process, and the stream URL is unguessable. The change also
skips most of ContinuePendingBeginRequest() and CreateResourceHandler() for
PlzNavigate stream requests to minimize the risk that they contain code that
could be exploited.

BUG=717644
==========

to

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

The cause was that there was a redirect from externalfile scheme to https. The
PlzNavigate code chose the right ending process based on the final URL. However
when the renderer made a request for the stream URL, it does attach the original
URL (pre-redirect) along. ResourceDispatcherHostImpl::ShouldServiceRequest was
incorrectly aborting the URL because that process rightly does not have access
to the externalfile scheme.

The fix is to skip the ChildProcessSecurityPolicy::CanRequestURL check in
ShouldServiceRequest for the PlzNavigate stream request. The browser already
picked the right process, and the stream URL is unguessable. The change also
skips most of ContinuePendingBeginRequest() and CreateResourceHandler() for
PlzNavigate stream requests to minimize the risk that they contain code that
could be exploited.

BUG=717644

Review-Url: https://codereview.chromium.org/2908593002
Cr-Commit-Position: refs/heads/master@{#475097}
Committed:
https://chromium.googlesource.com/chromium/src/+/ecb25eb8968a45120ee434af2d3d...
==========

[commit-bot: I haz the power, 2017-05-26 20:06:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ecb25eb8968a45120ee434af2d3d...