content/browser/child_process_security_policy_impl.cc - Issue 1868763002: Remove URLRequest::IsHandledProtocol and IsHandledURL

Unified Diff: content/browser/child_process_security_policy_impl.cc

Issue 1868763002: Remove URLRequest::IsHandledProtocol and IsHandledURL (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: finish removing URLRequest::IsHandledProtocol() Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« android_webview/browser/net/aw_url_request_job_factory.cc ('K') | « components/omnibox/browser/test_scheme_classifier.cc ('k') | content/shell/browser/shell_content_browser_client.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/browser/child_process_security_policy_impl.cc

diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc

index 34bad5ad0e55ab8d4a4ce2b2b788f511271de1e8..be856ed42fa4c26bc994b7197230d48c8bf58ec1 100644

--- a/content/browser/child_process_security_policy_impl.cc

+++ b/content/browser/child_process_security_policy_impl.cc

@@ -612,8 +612,7 @@ bool ChildProcessSecurityPolicyImpl::CanRequestURL(

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/ embedde

mgersh 2016/04/28 22:30:13 Can you point me to an explanation of what the sec

Charlie Reis 2016/04/29 00:14:44 For reference, CanRequestURL is used in ResourceDi

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_reques

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 Should also note that doesn't work for all consume

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.

}

bool ChildProcessSecurityPolicyImpl::CanCommitURL(int child_id,