Remove URLRequest::IsHandledProtocol and IsHandledURL

Remove URLRequest::IsHandledProtocol and IsHandledURL

These methods are being removed as part of a refactor, with the eventual
goal of deleting the URLRequestJobManager and handling http/https and
ws/wss in the same way as other protocols.

The call to URLRequest::IsHandledURL in
ChildProcessSecurityPolicyImpl::CanRequestURL was replaced by adding to
the hardcoded lists in the various ContentBrowserClient::IsHandledURL
overrides. This behavior change on the ContentBrowserClient side should
be safe because those methods have no other callers.

BUG=488166

[mgersh, 2016-04-21 17:49:58]

mgersh@chromium.org changed reviewers:
+ mmenke@chromium.org

[mgersh, 2016-04-21 18:00:06]

PTAL. (Other CL with more refactoring is also ready for review, sending it
next.)

[mmenke, 2016-04-21 18:24:19]

Thanks for doing this!  I started reviewing CLs aimed at removing the
URLRequestJobManager 3-4 years ago, great to see it finally getting ready to go.

https://codereview.chromium.org/1868763002/diff/60001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1868763002/diff/60001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:58: if
(net::URLRequest::IsHandledProtocol(scheme))
Can't this just be replaced with next_factory_->IsHandledProtocol(scheme)?  Note
that will make it URLRequestJobManager::SupportsScheme() as well as checking
schemes the URLRequestFactory supports itself.  The layering is a little weird. 
The JobManager is above the Factory when it comes to creating Jobs, but below it
when it comes to IsHandledProtocol, but that line should work now, and should
continue to work after the JobManager is removed.

https://codereview.chromium.org/1868763002/diff/60001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1868763002/diff/60001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.cc:217: return false;
I guess this didn't noticeably break anything because of the "&&
!net::URLRequest::IsHandledURL(url)"
ChildProcessSecurityPolicyImpl::CanRequestURL?

[mgersh, 2016-04-21 19:03:57]

Description was changed from

==========
Remove most calls to URLRequest::IsHandledProtocol and IsHandledURL

These methods will be removed as part of a refactor, with the eventual
goal of deleting the URLRequestJobManager and handling http/https and
ws/wss in the same way as other protocols.

The call to URLRequest::IsHandledURL in
ChildProcessSecurityPolicyImpl::CanRequestURL was replaced by adding to
the hardcoded lists in the various ContentBrowserClient::IsHandledURL
overrides. This behavior change on the ContentBrowserClient side should
be safe because those methods have no other callers.

BUG=488166
==========

to

==========
Remove URLRequest::IsHandledProtocol and IsHandledURL

These methods are being removed as part of a refactor, with the eventual
goal of deleting the URLRequestJobManager and handling http/https and
ws/wss in the same way as other protocols.

The call to URLRequest::IsHandledURL in
ChildProcessSecurityPolicyImpl::CanRequestURL was replaced by adding to
the hardcoded lists in the various ContentBrowserClient::IsHandledURL
overrides. This behavior change on the ContentBrowserClient side should
be safe because those methods have no other callers.

BUG=488166
==========

[mgersh, 2016-04-21 19:04:45]

https://codereview.chromium.org/1868763002/diff/60001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1868763002/diff/60001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:58: if
(net::URLRequest::IsHandledProtocol(scheme))
On 2016/04/21 18:24:19, mmenke wrote:
> Can't this just be replaced with next_factory_->IsHandledProtocol(scheme)? 
Note
> that will make it URLRequestJobManager::SupportsScheme() as well as checking
> schemes the URLRequestFactory supports itself.  The layering is a little
weird. 
> The JobManager is above the Factory when it comes to creating Jobs, but below
it
> when it comes to IsHandledProtocol, but that line should work now, and should
> continue to work after the JobManager is removed.

Oh, yeah, that would work. Did that, and also removed
URLRequest::IsHandledProtocol() entirely since this was the last caller.

https://codereview.chromium.org/1868763002/diff/60001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1868763002/diff/60001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.cc:217: return false;
On 2016/04/21 18:24:19, mmenke wrote:
> I guess this didn't noticeably break anything because of the "&&
> !net::URLRequest::IsHandledURL(url)"
> ChildProcessSecurityPolicyImpl::CanRequestURL?

Looks like it. I guess it's never been called anywhere that doesn't also call
URLRequest::IsHandledURL().

[mmenke, 2016-04-21 19:13:33]

LGTM!  Not sure I'll get to your other CL today, if not, definitely tomorrow.

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:56: // added to the
relevant URLRequestJobFactory.
Think you can remove this TODO?  We'll have to keep this check, because of the
AwURLRequestJobFactory::IsHandledProtocol weirdness.

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:59: return NULL;
nit:  While you're here, mind changing the NULL to nullptr?

[mgersh, 2016-04-22 14:33:18]

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:56: // added to the
relevant URLRequestJobFactory.
On 2016/04/21 19:13:33, mmenke wrote:
> Think you can remove this TODO?  We'll have to keep this check, because of the
> AwURLRequestJobFactory::IsHandledProtocol weirdness.

My understanding is that it's here only to allow the fall-through to the
URLRequestJobManager built-in factories, which is why it was originally
URLRequest::IsHandledProtocol(), and it won't be needed anymore once everything
is a ProtocolHandler. I actually ended up removing it in the other CL.

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:59: return NULL;
On 2016/04/21 19:13:33, mmenke wrote:
> nit:  While you're here, mind changing the NULL to nullptr?

Not going to bother here, since I still think it can be deleted in the next CL.
I'll fix it if I do end up keeping it.

[mmenke, 2016-04-22 14:54:36]

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:56: // added to the
relevant URLRequestJobFactory.
On 2016/04/22 14:33:18, mgersh wrote:
> On 2016/04/21 19:13:33, mmenke wrote:
> > Think you can remove this TODO?  We'll have to keep this check, because of
the
> > AwURLRequestJobFactory::IsHandledProtocol weirdness.
> 
> My understanding is that it's here only to allow the fall-through to the
> URLRequestJobManager built-in factories, which is why it was originally
> URLRequest::IsHandledProtocol(), and it won't be needed anymore once
everything
> is a ProtocolHandler. I actually ended up removing it in the other CL.

I thought it was also here to distinguished "handled" but not intercepted
(ERR_FAILED) with unknown schemes (ERR_UNKNOWN_SCHEME).  I guess your other CL
removes that distinction, on AW only?

[mmenke, 2016-04-22 14:56:00]

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.cc:56: // added to the
relevant URLRequestJobFactory.
On 2016/04/22 14:54:36, mmenke wrote:
> On 2016/04/22 14:33:18, mgersh wrote:
> > On 2016/04/21 19:13:33, mmenke wrote:
> > > Think you can remove this TODO?  We'll have to keep this check, because of
> the
> > > AwURLRequestJobFactory::IsHandledProtocol weirdness.
> > 
> > My understanding is that it's here only to allow the fall-through to the
> > URLRequestJobManager built-in factories, which is why it was originally
> > URLRequest::IsHandledProtocol(), and it won't be needed anymore once
> everything
> > is a ProtocolHandler. I actually ended up removing it in the other CL.
> 
> I thought it was also here to distinguished "handled" but not intercepted
> (ERR_FAILED) with unknown schemes (ERR_UNKNOWN_SCHEME).  I guess your other CL
> removes that distinction, on AW only?

Oh, I guess with that logic moved down to URLRequestJobFactoryImpl, we still get
that behavior on AW, too.

[mgersh, 2016-04-28 20:47:44]

mgersh@chromium.org changed reviewers:
+ creis@chromium.org, jamescook@chromium.org, sdefresne@chromium.org,
sgurun@chromium.org, slan@chromium.org

[mgersh, 2016-04-28 20:47:45]

creis: PTAL at content/
jamescook: PTAL at extensions/shell/
sdefresne: PTAL at components/ and ios/
sgurun: PTAL at android_webview/
slan: PTAL at chromecast/

Thanks!

[James Cook, 2016-04-28 21:07:27]

extensions/shell lgtm

[Charlie Reis, 2016-04-28 21:45:36]

One concern about the CanRequestURL change below.

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:615: return
!GetContentClient()->browser()->IsHandledURL(url);
This seems like it depends on the content/ embedder to always include http(s)
and ws(s) in IsHandledURL to get the same outcome.  Can you update the default
implementation in ContentBrowserClient, in case some content/ embedders don't
override that?

I'm also concerned about the ones that already override it and don't know about
this change, but I'm not sure how to address that.  What will happen to Opera,
CEF, etc?

[mgersh, 2016-04-28 22:30:14]

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:615: return
!GetContentClient()->browser()->IsHandledURL(url);
On 2016/04/28 21:45:35, Charlie Reis wrote:
> This seems like it depends on the content/ embedder to always include http(s)
> and ws(s) in IsHandledURL to get the same outcome.  Can you update the default
> implementation in ContentBrowserClient, in case some content/ embedders don't
> override that?
> 
> I'm also concerned about the ones that already override it and don't know
about
> this change, but I'm not sure how to address that.  What will happen to Opera,
> CEF, etc?

Can you point me to an explanation of what the security issue is here, so I can
think about other ways around the problem? Looking closer at this, I notice that
http/https are always allowed anyway because they're in the list of web-safe
schemes, but ws/wss aren't, and I don't know why that is.

[halliwell, 2016-04-29 00:06:34]

On 2016/04/28 22:30:14, mgersh wrote:
>
https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
> File content/browser/child_process_security_policy_impl.cc (right):
> 
>
https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
> content/browser/child_process_security_policy_impl.cc:615: return
> !GetContentClient()->browser()->IsHandledURL(url);
> On 2016/04/28 21:45:35, Charlie Reis wrote:
> > This seems like it depends on the content/ embedder to always include
http(s)
> > and ws(s) in IsHandledURL to get the same outcome.  Can you update the
default
> > implementation in ContentBrowserClient, in case some content/ embedders
don't
> > override that?
> > 
> > I'm also concerned about the ones that already override it and don't know
> about
> > this change, but I'm not sure how to address that.  What will happen to
Opera,
> > CEF, etc?
> 
> Can you point me to an explanation of what the security issue is here, so I
can
> think about other ways around the problem? Looking closer at this, I notice
that
> http/https are always allowed anyway because they're in the list of web-safe
> schemes, but ws/wss aren't, and I don't know why that is.

chromecast/ lgtm

[Charlie Reis, 2016-04-29 00:14:45]

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:615: return
!GetContentClient()->browser()->IsHandledURL(url);
On 2016/04/28 22:30:13, mgersh wrote:
> On 2016/04/28 21:45:35, Charlie Reis wrote:
> > This seems like it depends on the content/ embedder to always include
http(s)
> > and ws(s) in IsHandledURL to get the same outcome.  Can you update the
default
> > implementation in ContentBrowserClient, in case some content/ embedders
don't
> > override that?
> > 
> > I'm also concerned about the ones that already override it and don't know
> about
> > this change, but I'm not sure how to address that.  What will happen to
Opera,
> > CEF, etc?
> 
> Can you point me to an explanation of what the security issue is here, so I
can
> think about other ways around the problem? Looking closer at this, I notice
that
> http/https are always allowed anyway because they're in the list of web-safe
> schemes, but ws/wss aren't, and I don't know why that is.

For reference, CanRequestURL is used in ResourceDispatcherHost and elsewhere to
know whether a given renderer is allowed to ask for a given resource, whether
for navigation or other cases.  Many things are off limits (e.g., chrome://
URLs).

My concern is that this particular line is supposed to return true if |url| will
be handled by something outside the browser.  Any content/ embedders that we
don't modify in this CL will act as if HTTP URLs aren't handled by the browser,
which seems wrong.  That may affect this code, and it would likely affect any
future callers of IsHandledURL.  I'd like to find a way to make that
clearer/safer.

That said, I agree with you that it gets a little less clear looking at the
surrounding code here in CanRequestURL.  HTTP(s) URLs will cause us to return
true from CanCommitURL above (at least currently; let's ignore the TODO there
above the IsWebSafeScheme call), so we won't get to IsHandledURL for them.  I'm
not sure why ws/wss URLs aren't included in IsWebSafeScheme (or whether they
should be), but that does appear to mean that CanRequestURL returns false for
ws/wss URLs today and true with your change.

It does appear that you can't navigate to ws URLs today, such as
ws://echo.websocket.org.  (That's also true in Safari and Firefox.)  I'm not
sure if this check is the only reason-- do we behave the same after your CL?

Anyway, I'm open to other ways to checking whether a URL will be handled
externally to the browser, but that's what this line is looking for.  (I'm also
open to finding out more about websocket URLs, in case that simplifies this.)

[mmenke, 2016-04-29 00:27:24]

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:615: return
!GetContentClient()->browser()->IsHandledURL(url);
On 2016/04/29 00:14:44, Charlie Reis wrote:
> On 2016/04/28 22:30:13, mgersh wrote:
> > On 2016/04/28 21:45:35, Charlie Reis wrote:
> > > This seems like it depends on the content/ embedder to always include
> http(s)
> > > and ws(s) in IsHandledURL to get the same outcome.  Can you update the
> default
> > > implementation in ContentBrowserClient, in case some content/ embedders
> don't
> > > override that?
> > > 
> > > I'm also concerned about the ones that already override it and don't know
> > about
> > > this change, but I'm not sure how to address that.  What will happen to
> Opera,
> > > CEF, etc?
> > 
> > Can you point me to an explanation of what the security issue is here, so I
> can
> > think about other ways around the problem? Looking closer at this, I notice
> that
> > http/https are always allowed anyway because they're in the list of web-safe
> > schemes, but ws/wss aren't, and I don't know why that is.
> 
> For reference, CanRequestURL is used in ResourceDispatcherHost and elsewhere
to
> know whether a given renderer is allowed to ask for a given resource, whether
> for navigation or other cases.  Many things are off limits (e.g., chrome://
> URLs).
> 
> My concern is that this particular line is supposed to return true if |url|
will
> be handled by something outside the browser.  Any content/ embedders that we
> don't modify in this CL will act as if HTTP URLs aren't handled by the
browser,
> which seems wrong.  That may affect this code, and it would likely affect any
> future callers of IsHandledURL.  I'd like to find a way to make that
> clearer/safer.
> 
> That said, I agree with you that it gets a little less clear looking at the
> surrounding code here in CanRequestURL.  HTTP(s) URLs will cause us to return
> true from CanCommitURL above (at least currently; let's ignore the TODO there
> above the IsWebSafeScheme call), so we won't get to IsHandledURL for them. 
I'm
> not sure why ws/wss URLs aren't included in IsWebSafeScheme (or whether they
> should be), but that does appear to mean that CanRequestURL returns false for
> ws/wss URLs today and true with your change.
> 
> It does appear that you can't navigate to ws URLs today, such as
> http://ws://echo.websocket.org.  (That's also true in Safari and Firefox.) 
I'm not
> sure if this check is the only reason-- do we behave the same after your CL?
> 
> Anyway, I'm open to other ways to checking whether a URL will be handled
> externally to the browser, but that's what this line is looking for.  (I'm
also
> open to finding out more about websocket URLs, in case that simplifies this.)

If we're on the IOTHread, we could call
url_request_context->job_factory()->IsHandledProtocol, but we'd need to be able
to get the main URLRequestContext for the ResourceContext to do that.

Or just pass in the URLRequestContext, though that seems a little weird.

[mmenke, 2016-04-29 00:32:15]

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
File content/browser/child_process_security_policy_impl.cc (right):

https://codereview.chromium.org/1868763002/diff/80001/content/browser/child_p...
content/browser/child_process_security_policy_impl.cc:615: return
!GetContentClient()->browser()->IsHandledURL(url);
On 2016/04/29 00:27:24, mmenke wrote:
> On 2016/04/29 00:14:44, Charlie Reis wrote:
> > On 2016/04/28 22:30:13, mgersh wrote:
> > > On 2016/04/28 21:45:35, Charlie Reis wrote:
> > > > This seems like it depends on the content/ embedder to always include
> > http(s)
> > > > and ws(s) in IsHandledURL to get the same outcome.  Can you update the
> > default
> > > > implementation in ContentBrowserClient, in case some content/ embedders
> > don't
> > > > override that?
> > > > 
> > > > I'm also concerned about the ones that already override it and don't
know
> > > about
> > > > this change, but I'm not sure how to address that.  What will happen to
> > Opera,
> > > > CEF, etc?
> > > 
> > > Can you point me to an explanation of what the security issue is here, so
I
> > can
> > > think about other ways around the problem? Looking closer at this, I
notice
> > that
> > > http/https are always allowed anyway because they're in the list of
web-safe
> > > schemes, but ws/wss aren't, and I don't know why that is.
> > 
> > For reference, CanRequestURL is used in ResourceDispatcherHost and elsewhere
> to
> > know whether a given renderer is allowed to ask for a given resource,
whether
> > for navigation or other cases.  Many things are off limits (e.g., chrome://
> > URLs).
> > 
> > My concern is that this particular line is supposed to return true if |url|
> will
> > be handled by something outside the browser.  Any content/ embedders that we
> > don't modify in this CL will act as if HTTP URLs aren't handled by the
> browser,
> > which seems wrong.  That may affect this code, and it would likely affect
any
> > future callers of IsHandledURL.  I'd like to find a way to make that
> > clearer/safer.
> > 
> > That said, I agree with you that it gets a little less clear looking at the
> > surrounding code here in CanRequestURL.  HTTP(s) URLs will cause us to
return
> > true from CanCommitURL above (at least currently; let's ignore the TODO
there
> > above the IsWebSafeScheme call), so we won't get to IsHandledURL for them. 
> I'm
> > not sure why ws/wss URLs aren't included in IsWebSafeScheme (or whether they
> > should be), but that does appear to mean that CanRequestURL returns false
for
> > ws/wss URLs today and true with your change.
> > 
> > It does appear that you can't navigate to ws URLs today, such as
> > http://ws://echo.websocket.org.  (That's also true in Safari and Firefox.) 
> I'm not
> > sure if this check is the only reason-- do we behave the same after your CL?
> > 
> > Anyway, I'm open to other ways to checking whether a URL will be handled
> > externally to the browser, but that's what this line is looking for.  (I'm
> also
> > open to finding out more about websocket URLs, in case that simplifies
this.)
> 
> If we're on the IOTHread, we could call
> url_request_context->job_factory()->IsHandledProtocol, but we'd need to be
able
> to get the main URLRequestContext for the ResourceContext to do that.
> 
> Or just pass in the URLRequestContext, though that seems a little weird.

Should also note that doesn't work for all consumers of
ContentBrowserClient::BrowserIsHandledURL - a bunch of consumers aren't on the
network thread, unfortunately.  Would like to have all consumers switch over to
it, longer term, but involves a fair bit of work.

[sgurun-gerrit only, 2016-04-29 22:25:06]

aw lgtm

[sdefresne, 2017-05-03 09:08:42]

sdefresne@chromium.org changed reviewers:
- sdefresne@chromium.org