Remove URLRequest::IsHandledProtocol and IsHandledURL

content/browser/child_process_security_policy_impl.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/child_process_security_policy_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
@@ skipping 594 matching lines @@
     // any child process.  Also, this case covers <javascript:...>, which should
     // be handled internally by the process and not kicked up to the browser.
     return false;
   }
 
   // If the process can commit the URL, it can request it.
   if (CanCommitURL(child_id, url))
     return true;
 
   // Also allow URLs destined for ShellExecute and not the browser itself.
-  return !GetContentClient()->browser()->IsHandledURL(url) &&
-         !net::URLRequest::IsHandledURL(url);
+  return !GetContentClient()->browser()->IsHandledURL(url);

[Charlie Reis, 2016/04/28 21:45:35]

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:13]

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.

[Charlie Reis, 2016/04/29 00:14:44]

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]

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]

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.

 }
 
 bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,
                                                   const GURL& url) {
   if (!url.is_valid())
     return false;  // Can't commit invalid URLs.
 
   // Of all the pseudo schemes, only about:blank is allowed to commit.
   if (IsPseudoScheme(url.scheme()))
     return base::LowerCaseEqualsASCII(url.spec(), url::kAboutBlankURL);
@@ skipping 240 matching lines @@
   base::AutoLock lock(lock_);
 
   SecurityStateMap::iterator state = security_state_.find(child_id);
   if (state == security_state_.end())
     return false;
 
   return state->second->can_send_midi_sysex();
 }
 
 }  // namespace content