Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1692503002:
Functionality to allow blacklist and whitelist of custom schemes (Closed)
|
Created:
4 years, 10 months ago by igorcov Modified:
4 years, 7 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFunctionality to allow blacklist and whitelist of custom schemes.
These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies.
Only the formats scheme://* and scheme:* are accepted.
The URLs of other formats containing custom schemes will be considered invalid and ignored.
In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user.
In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs.
Most important changes done:
url_blacklist_manager.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that:
- if URL is whitelisted it will be launched without any warning/notification.
- if URL is blacklisted it will redirect user to blocked page information.
- if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL.
resource_dispatcher_host_impl.cc - Pass the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol.
resource_dispatcher_host_delegate.cc - uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted.
url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions.
BUG=158436
Committed: https://crrev.com/eccb40b717c49c968a87f91fba9b23c21f4f9a3e
Cr-Commit-Position: refs/heads/master@{#395305}
Patch Set 1 #Patch Set 2 : Initial implementation done #Patch Set 3 : Cleaned the code #Patch Set 4 : Implementation of schema only supported version #Patch Set 5 : Removed unused comments #
Total comments: 44
Patch Set 6 : Fixed review comments #Patch Set 7 : Another variable set as const #
Total comments: 17
Patch Set 8 : Fixed review comments #
Total comments: 3
Patch Set 9 : Implemented suggestions from Thiemo. #Patch Set 10 : Patterns for standard schemes are case-insensitive now #Patch Set 11 : Merged with changes from other CLs #Patch Set 12 : Added tests that cover basic documentation for url blacklist #Patch Set 13 : Fixed comments and method name #
Total comments: 36
Patch Set 14 : Fixed the latest review comments #
Total comments: 14
Patch Set 15 : Fixed review comments #
Total comments: 9
Patch Set 16 : Fixed review comments #Patch Set 17 : Fixed windows file path #Patch Set 18 : Included test with new scheme format #
Total comments: 19
Patch Set 19 : Fixed compile errors and latest review comments #Patch Set 20 : Replaced expect_eq(bool, bool) with expect_true/false(bool) #Patch Set 21 : Fixed file pattern issue #
Total comments: 14
Patch Set 22 : Fixed review comments #Patch Set 23 : Fixed review comments #
Total comments: 8
Patch Set 24 : Fixed review comments #Patch Set 25 : Fixed review comments #
Total comments: 2
Patch Set 26 : Fixed review comments #Patch Set 27 : Fixed review comments and merged #
Total comments: 6
Patch Set 28 : Fixed review comments. #
Total comments: 2
Patch Set 29 : Fixed review comments #Patch Set 30 : Improved doc #
Total comments: 2
Patch Set 31 : Removed redundant comments #
Total comments: 9
Patch Set 32 : Fixed review comments #Patch Set 33 : Fixed review comments #
Total comments: 1
Patch Set 34 : Changed code so that net package is not involved #Patch Set 35 : Fixed the comments and imports #Patch Set 36 : Identation fixed. #Patch Set 37 : Fixed variable name #Patch Set 38 : Indentation fixed #Patch Set 39 : Indentation fixed #
Total comments: 6
Patch Set 40 : Fixed review comments #
Total comments: 4
Patch Set 41 : Fixed review comments #Patch Set 42 : Fixed compile problem #Patch Set 43 : Fixed compile errors #Patch Set 44 : Fixed compile error #Patch Set 45 : Fixed compile error #Patch Set 46 : Fixed compile error #Messages
Total messages: 99 (30 generated)
Description was changed from ========== Partial implementation, waiting a decision for matcher functionality BUG=158436 ========== to ========== Partial implementation, waiting a decision for matcher functionality BUG=158436 ==========
igorcov@chromium.org changed reviewers: + atwilson@chromium.org, bartfab@chromium.org, tnagel@chromium.org
Please review changes made to allow admins to blacklist/whitelist custom schemes.
Description was changed from ========== Partial implementation, waiting a decision for matcher functionality BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes BUG=158436 ==========
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes BUG=158436 ========== to ========== Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
Please make the CL description more clear concerning * what you are trying to do and * how that is accomplished. Specifically, please specify your change to the the blacklist format.
Description was changed from ========== Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
Please repeat the CL title ("Functionality to allow blacklist and whitelist of
custom schemes") as the first line of the CL description, separated by a blank
line from the rest of the description.
Description was changed from ========== These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
https://codereview.chromium.org/1692503002/diff/80001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/80001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:763: net::NetworkDelegate::URLBlacklistState Please wrap in #if defined(ENABLE_CONFIGURATION_POLICY) ... #endif. https://codereview.chromium.org/1692503002/diff/80001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:764: ChromeNetworkDelegate::GetURLBlacklistState( Please don't wrap parameter if it would fit on a single line. https://codereview.chromium.org/1692503002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:241: if (is_whitelisted) { Please explain what is happening here and why! https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:207: bool URLBlacklist::IsURLBlocked(const GURL& url) const { Please don't duplicate code! Instead re-implement IsURLBlocked() using GetURLBlacklistState(). https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:239: const FilterComponents* max = NULL; NULL --> nullptr https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && Why must the host be empty? The IsStandard() call should be sufficient to indicate whether it's a custom scheme or not. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { This breaks normalization of URLs and schemes (eg. removal of leading whitespace or converting to lower case). https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.h:66: net::NetworkDelegate::URLBlacklistState GetURLBlacklistState(const GURL& url) I'd suggest to break the line after the opening brace. A single const on the next line might easily be overlooked, especially since it's not indented. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.h:167: net::NetworkDelegate::URLBlacklistState GetURLBlacklistState(const GURL& url) Same as above. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:394: // Filter custom schemes Please add a period at the end of the sentence. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:697: // check some types of custom schemes. Please start sentence with capital letter. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:699: blocked->AppendString("custom://*"); Please include tests with capital letters and leading whitespace. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:701: // this is bad format, we support only custom_scheme://* Please start sentence with capital letter and end it with a period. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:719: // shouldn't match with anything, since the format defined in allowed is not Same as above. https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:854: net::URLRequest* req = loader->request(); Can you make this const? https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:855: net::NetworkDelegate* network_delegate = req->network_delegate(); Can you make this const? https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:856: // get the state, if url is in blacklist, whitelist or in none of those. Please start comment with capital letter. https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:857: net::NetworkDelegate::URLBlacklistState urlState = const https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:860: if(urlState == net::NetworkDelegate::URLBlacklistState::URL_IN_BLACKLIST) { Please add a blank between "if" and "(". https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:861: // It's a link with custom scheme and it's blacklisted. Please wrap comment lines at 80 characters to save vertical space. https://codereview.chromium.org/1692503002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:863: // Eventually chrome_network_delegate will see it's in the blackist and blackist --> blacklist https://codereview.chromium.org/1692503002/diff/80001/net/base/network_delega... File net/base/network_delegate.h (right): https://codereview.chromium.org/1692503002/diff/80001/net/base/network_delega... net/base/network_delegate.h:58: // URLBlacklistState indicates if the URL was defined in blacklist, There's no need to repeat "URLBlacklistState". https://codereview.chromium.org/1692503002/diff/80001/net/base/network_delega... net/base/network_delegate.h:61: enum URLBlacklistState { Why do you need a tri-state? Please explain in CL description. https://codereview.chromium.org/1692503002/diff/80001/net/base/network_delega... net/base/network_delegate.h:131: // This function is to be used to check if the url is defined in Use pipes to refer to a parameter, such as |url|. https://codereview.chromium.org/1692503002/diff/80001/net/base/network_delega... net/base/network_delegate.h:133: virtual URLBlacklistState GetURLBlacklistState(const GURL& url); Please make the method const. https://codereview.chromium.org/1692503002/diff/80001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/1692503002/diff/80001/net/url_request/url_req... net/url_request/url_request.h:632: NetworkDelegate* network_delegate() { return network_delegate_; } Please make this const if possible.
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: network_delegate.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
Fixed comments from Thiemo. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && On 2016/03/08 18:44:34, Thiemo Nagel wrote: > Why must the host be empty? The IsStandard() call should be sufficient to > indicate whether it's a custom scheme or not. We want our code to check for custom scheme only when the host is empty, because that is the case when URL segmentation fails. A URL like chrome://policy also doesn't have a standard scheme, but it is segmented successfully and the host is not empty. And we want the behavior to remain the same for URLs that are understood by our segmentation code. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { On 2016/03/08 18:44:34, Thiemo Nagel wrote: > This breaks normalization of URLs and schemes (eg. removal of leading whitespace > or converting to lower case). The filter is defined by admin, and it has to be according to the format we allow, which is |scheme|://*. The url_scheme is taken from the filter. There's no external user input here. If the URL is defined with leading whitespaces, then it's not according to format we defined. Unit tests have been added to cover this.
https://codereview.chromium.org/1692503002/diff/120001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:242: // without any additional security checks. Since the URL is whitelisted, What security checks exactly are we skipping here (other than the user notification, which is fine)? https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:209: net::NetworkDelegate::URLBlacklistState::URL_IN_BLACKLIST; Nit: Indent. https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:257: std::string url_scheme = segment_url(filter, &parsed); Nit: const. https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:516: DCHECK(io_task_runner_->RunsTasksOnCurrentThread()); Nit: #include "base/logging.h" https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:67: const GURL& url) const; Nit: Indent. https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:395: blocked.reset(new base::ListValue); Nit: #include "base/values.h" https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:710: net::NetworkDelegate::URLBlacklistState::URL_IN_BLACKLIST); Nit: Here and below: Indent. https://codereview.chromium.org/1692503002/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1692503002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:853: const net::URLRequest* req = loader->request(); Nit: const pointer. https://codereview.chromium.org/1692503002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:854: const net::NetworkDelegate* network_delegate = req->network_delegate(); Nit 1: #include "net/base/network_delegate.h" Nit 2: const pointer. https://codereview.chromium.org/1692503002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1692503002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:84: bool is_whitelist); Nit: s/whitelist/whitelisted/ https://codereview.chromium.org/1692503002/diff/120001/net/base/network_deleg... File net/base/network_delegate.h (right): https://codereview.chromium.org/1692503002/diff/120001/net/base/network_deleg... net/base/network_delegate.h:58: // Indicates if the URL was defined in blacklist, in whitelist or is not Nit: URLs are not "defined" in the blacklist/whitelist. They may match the blacklist/whitelist.
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && > We want our code to check for custom scheme only when the host is empty, because > that is the case when URL segmentation fails. A URL like chrome://policy also > doesn't have a standard scheme, but it is segmented successfully and the host is > not empty. And we want the behavior to remain the same for URLs that are > understood by our segmentation code. I don't think that we should depend on undocumented implementation details of SegmentURL(). This makes for brittle code. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { > The filter is defined by admin, and it has to be according to the format we > allow, which is |scheme|://*. The url_scheme is taken from the filter. There's > no external user input here. If the URL is defined with leading whitespaces, > then it's not according to format we defined. Unit tests have been added to > cover this. We don't specify that schemes must be lower-case but the way I read your code it would not work for upper-case schemes. I don't think it's necessary or useful to be picky about the case. https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:67: const GURL& url) const; On 2016/03/10 13:49:41, bartfab (slow) wrote: > Nit: Indent. Please break the line after the return value. That seems more common. (Sorry for making wrong suggestion in previous review.) https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:167: net::NetworkDelegate::URLBlacklistState GetURLBlacklistState( Same as above. https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:722: // Shouldn't match with anything, since the format defined in allowed is not s/with// s/,// https://codereview.chromium.org/1692503002/diff/120001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1692503002/diff/120001/net/url_request/url_re... net/url_request/url_request.h:632: NetworkDelegate* network_delegate() const { return network_delegate_; } Please return const pointer.
https://codereview.chromium.org/1692503002/diff/120001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/1692503002/diff/120001/net/url_request/url_re... net/url_request/url_request.h:632: NetworkDelegate* network_delegate() const { return network_delegate_; } On 2016/03/10 14:37:26, Thiemo Nagel wrote: > Please return const pointer. I think you mean pointer to const :).
igorcov@google.com changed reviewers: + igorcov@google.com
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:207: bool URLBlacklist::IsURLBlocked(const GURL& url) const { On 2016/03/08 18:44:34, Thiemo Nagel (no reviews) wrote: > Please don't duplicate code! Instead re-implement IsURLBlocked() using > GetURLBlacklistState(). Done. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:239: const FilterComponents* max = NULL; On 2016/03/08 18:44:34, Thiemo Nagel (no reviews) wrote: > NULL --> nullptr Done. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && On 2016/03/10 14:37:26, Thiemo Nagel (no reviews) wrote: > > We want our code to check for custom scheme only when the host is empty, > because > > that is the case when URL segmentation fails. A URL like chrome://policy also > > doesn't have a standard scheme, but it is segmented successfully and the host > is > > not empty. And we want the behavior to remain the same for URLs that are > > understood by our segmentation code. > > I don't think that we should depend on undocumented implementation details of > SegmentURL(). This makes for brittle code. The code already relies on that implementation details - the line: if (!parsed.host.is_nonempty()) in the original code. It is used to validate the filter. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { On 2016/03/10 14:37:26, Thiemo Nagel (no reviews) wrote: > > The filter is defined by admin, and it has to be according to the format we > > allow, which is |scheme|://*. The url_scheme is taken from the filter. There's > > no external user input here. If the URL is defined with leading whitespaces, > > then it's not according to format we defined. Unit tests have been added to > > cover this. > > We don't specify that schemes must be lower-case but the way I read your code it > would not work for upper-case schemes. I don't think it's necessary or useful > to be picky about the case. It's case insensitive because the url_scheme is taken from filter as is. Works with mixed uppercase and lowercase letters too. This is covered in the tests, with filter like "AbC://*". https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:394: // Filter custom schemes On 2016/03/08 18:44:35, Thiemo Nagel (no reviews) wrote: > Please add a period at the end of the sentence. Done. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:697: // check some types of custom schemes. On 2016/03/08 18:44:35, Thiemo Nagel (no reviews) wrote: > Please start sentence with capital letter. Done. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager_unittest.cc:699: blocked->AppendString("custom://*"); On 2016/03/08 18:44:35, Thiemo Nagel (no reviews) wrote: > Please include tests with capital letters and leading whitespace. Done. https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/120001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:722: // Shouldn't match with anything, since the format defined in allowed is not On 2016/03/10 14:37:26, Thiemo Nagel (no reviews) wrote: > s/with// > s/,// Done.
LGTM if all of Thiemo's concerns have been addressed. https://codereview.chromium.org/1692503002/diff/140001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/140001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:67: GetURLBlacklistState(const GURL& url) const; Nit: This line should not be indented. https://codereview.chromium.org/1692503002/diff/140001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:168: GetURLBlacklistState(const GURL& url) const; Nit: THis line should not be indented. https://codereview.chromium.org/1692503002/diff/140001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/140001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:723: // Shouldn't match with anything since the format defined in |allowed| is not Nit: s/with //
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && > The code already relies on that implementation details - the line: > if (!parsed.host.is_nonempty()) > in the original code. It is used to validate the filter. While the fact that you state is true, I don't consider that a valid justification. We must hold new code to higher standards than existing ones, otherwise the code base would never improve.
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { > It's case insensitive because the url_scheme is taken from filter as is. Works > with mixed uppercase and lowercase letters too. This is covered in the tests, > with filter like "AbC://*". The way I read GetValidScheme(), normalization only happens for "valid" schemes. Thus I'd assume that "FTP://*" would fail.
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && On 2016/04/12 13:35:47, Thiemo Nagel (no reviews) wrote: > > The code already relies on that implementation details - the line: > > if (!parsed.host.is_nonempty()) > > in the original code. It is used to validate the filter. > > While the fact that you state is true, I don't consider that a valid > justification. We must hold new code to higher standards than existing ones, > otherwise the code base would never improve. Move the code outside of checking the URL host. The tests pass and the functionality that I checked remained the same. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { On 2016/04/12 14:43:05, Thiemo Nagel (no reviews) wrote: > > It's case insensitive because the url_scheme is taken from filter as is. Works > > with mixed uppercase and lowercase letters too. This is covered in the tests, > > with filter like "AbC://*". > > The way I read GetValidScheme(), normalization only happens for "valid" schemes. > Thus I'd assume that "FTP://*" would fail. I've fixed to compare ignore case. Unfortunately this doesn't solve the problem for known schemes, like ftp/chrome/file. Those should all be lower case in order to work. For all custom schemes though it will be case insensitive.
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:300: if (!url_scheme.empty() && !parsed.host.is_nonempty() && > Move the code outside of checking the URL host. The tests pass and the > functionality that I checked remained the same. What is the purpose of the remaining "if (!parsed.host.is_nonempty()) { return false; }" block Please replace that with something sane. My original comment that we should not depend on undocumented implementation details still applies. https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { > I've fixed to compare ignore case. Unfortunately this doesn't solve the problem > for known schemes, like ftp/chrome/file. Those should all be lower case in order > to work. For all custom schemes though it will be case insensitive. I don't think this is acceptable. Please implement a proper solution, even if it takes more effort.
https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:279: if (!url::IsStandard(url_scheme.c_str(), I think the IsStandard() condition should be dropped unless there is evidence that it is needed. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:282: base::LowerCaseEqualsASCII(base::StringPiece(filter.c_str()), .c_str() can be dropped. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:283: url_scheme + "://*")) { It cannot be assumed that the part after the scheme starts with //. I think the slashes should be dropped here and the documentation updated. Cf. https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:339: *path = base::ToLowerASCII(base::StringPiece(*path)); I don't think that the path can be converted to lower case. These tests must pass: CheckBlacklistMatch("http://example.com/aB", "http://example.com/aB", true); CheckBlacklistMatch("http://example.com/aB", "http://example.com/Ab", false); CheckBlacklistMatch("http://example.com/aB", "http://example.com/ab", false); CheckBlacklistMatch("http://example.com/aB", "http://example.com/AB", false);
Still LGTM assuming Thiemo's comments are addressed. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (left): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:21: #include "base/values.h" This is still used. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:175: // Will test if |url| matches the |pattern|, expecting the result to be the same This is generally not a good testing pattern. If the EXPECT_EQ() fails, test output will show you that there is an error in line 189 - and you will not know which invocation of CheckBlacklistMatch() failed. It is better to have this method return a |bool| and compare that to your expectation outside the method. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:177: void CheckBlacklistMatch(std::string pattern, std::string url, bool matches) { Nit 1: s/Check/check/ Nit 2: Pass |pattern| and |url| by const reference. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:181: scoped_ptr<base::ListValue> blocked(new base::ListValue); Nit: Use |std::unique_ptr| instead. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:186: if(is_blocked != matches) { Nit: s/if/if / https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:187: LOG(ERROR) << "failed " << pattern << ", " << url; Nit 1: #include "base/logging.h" Nit 2: Once you move the EXPECT_EQ() outside this method, the log will no longer be necessary. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:750: // An optional path can come after port. Any string can be used here. Nit: Not quite any string, e.g. "?" is not allowed. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:767: CheckBlacklistMatch("host.com/path", "http://user:pass@host.com:8080/path", true); Nit: Wrap. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:768: CheckBlacklistMatch("ftp://host.com/path", "ftp://user:pass@host.com:8080/path", true); Nit: Wrap. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:773: scoped_ptr<base::ListValue> blocked(new base::ListValue); Nit: Use |std::unique_ptr|. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:774: scoped_ptr<base::ListValue> allowed(new base::ListValue); Nit: Use |std::unique_ptr|. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:792: EXPECT_TRUE(blacklist.GetURLBlacklistState(GURL("http://www.youtube.com")) == Nit: Here and below: Use EXPECT_EQ(). https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:839: scoped_ptr<base::ListValue> blocked(new base::ListValue); Nit: Use |std::unique_ptr|. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:840: scoped_ptr<base::ListValue> allowed(new base::ListValue); Nit: Use |std::unique_ptr|. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:842: // Check recognized by chrome schemes with case insensitive scenarios. Nit: s/recognized by chrome schemes/schemes recognized by Chrome/
Still LGTM assuming Thiemo's comments are addressed.
https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:177: void CheckBlacklistMatch(std::string pattern, std::string url, bool matches) { On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit 1: s/Check/check/ > Nit 2: Pass |pattern| and |url| by const reference. NVM the s/Check/check/
https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/80001/components/policy/core/... components/policy/core/browser/url_blacklist_manager.cc:304: filter == url_scheme + "://*") { On 2016/04/12 17:20:57, Thiemo Nagel (no reviews) wrote: > > I've fixed to compare ignore case. Unfortunately this doesn't solve the > problem > > for known schemes, like ftp/chrome/file. Those should all be lower case in > order > > to work. For all custom schemes though it will be case insensitive. > > I don't think this is acceptable. Please implement a proper solution, even if > it takes more effort. Implemented to have scheme, host and path case insensitive. The query will remain case-sensitive. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:279: if (!url::IsStandard(url_scheme.c_str(), On 2016/04/18 13:12:39, Thiemo Nagel (no reviews) wrote: > I think the IsStandard() condition should be dropped unless there is evidence > that it is needed. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:282: base::LowerCaseEqualsASCII(base::StringPiece(filter.c_str()), On 2016/04/18 13:12:39, Thiemo Nagel (no reviews) wrote: > .c_str() can be dropped. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:283: url_scheme + "://*")) { On 2016/04/18 13:12:39, Thiemo Nagel (no reviews) wrote: > It cannot be assumed that the part after the scheme starts with //. I think the > slashes should be dropped here and the documentation updated. Cf. > https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax For the filter definition we need the // part because that's the only way for us to make distinction between a pattern like scheme:* and host:* where * is considered as a port. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:339: *path = base::ToLowerASCII(base::StringPiece(*path)); On 2016/04/18 13:12:39, Thiemo Nagel (no reviews) wrote: > I don't think that the path can be converted to lower case. These tests must > pass: > > CheckBlacklistMatch("http://example.com/aB", "http://example.com/aB", true); > CheckBlacklistMatch("http://example.com/aB", "http://example.com/Ab", false); > CheckBlacklistMatch("http://example.com/aB", "http://example.com/ab", false); > CheckBlacklistMatch("http://example.com/aB", "http://example.com/AB", false); Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:175: // Will test if |url| matches the |pattern|, expecting the result to be the same On 2016/04/18 13:27:41, bartfab (slow) wrote: > This is generally not a good testing pattern. If the EXPECT_EQ() fails, test > output will show you that there is an error in line 189 - and you will not know > which invocation of CheckBlacklistMatch() failed. It is better to have this > method return a |bool| and compare that to your expectation outside the method. Re-implemented as a bool function. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:177: void CheckBlacklistMatch(std::string pattern, std::string url, bool matches) { On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit 1: s/Check/check/ > Nit 2: Pass |pattern| and |url| by const reference. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:181: scoped_ptr<base::ListValue> blocked(new base::ListValue); On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: Use |std::unique_ptr| instead. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:186: if(is_blocked != matches) { On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: s/if/if / Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:187: LOG(ERROR) << "failed " << pattern << ", " << url; On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit 1: #include "base/logging.h" > Nit 2: Once you move the EXPECT_EQ() outside this method, the log will no longer > be necessary. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:750: // An optional path can come after port. Any string can be used here. On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: Not quite any string, e.g. "?" is not allowed. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:767: CheckBlacklistMatch("host.com/path", "http://user:pass@host.com:8080/path", true); On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: Wrap. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:768: CheckBlacklistMatch("ftp://host.com/path", "ftp://user:pass@host.com:8080/path", true); On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: Wrap. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:792: EXPECT_TRUE(blacklist.GetURLBlacklistState(GURL("http://www.youtube.com")) == On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: Here and below: Use EXPECT_EQ(). Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:839: scoped_ptr<base::ListValue> blocked(new base::ListValue); On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: Use |std::unique_ptr|. Done. https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:842: // Check recognized by chrome schemes with case insensitive scenarios. On 2016/04/18 13:27:41, bartfab (slow) wrote: > Nit: s/recognized by chrome schemes/schemes recognized by Chrome/ Done.
https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (left): https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:21: #include "base/values.h" Nit: This is still used. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:58: std::unique_ptr<base::ListValue> block(new base::ListValue); Nit: #include <memory> https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:175: // Will return if |url| matches the |pattern|. Nit: s/if/whether/ https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:176: bool IsBlacklistMatched(const std::string pattern, const std::string url) { Nit: Pass strings by const reference. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:665: blacklist(new URLBlacklist(GetSegmentURLCallback())); Nit: Indent. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:713: EXPECT_EQ(IsBlacklistMatched("file://*", "file:///abc.txt"), true); Nit: Here and below: Expected value first, value to be tested second. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:749: // An optional path can come after port. Any string not containing '?' can be If you want to get into the business of documenting what a URI path looks like, the rules are a bit more complex than that: https://tools.ietf.org/html/rfc3986#section-3.3 https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:875: GURL("http://examplE.com/Path?Query=1")), This is a really odd indent. I would have expected 4 spaces from the start of the line or from the start of |blacklist|.
https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:58: std::unique_ptr<base::ListValue> block(new base::ListValue); On 2016/04/18 15:26:30, bartfab (slow) wrote: > Nit: #include <memory> Done. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:175: // Will return if |url| matches the |pattern|. On 2016/04/18 15:26:30, bartfab (slow) wrote: > Nit: s/if/whether/ Done. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:176: bool IsBlacklistMatched(const std::string pattern, const std::string url) { On 2016/04/18 15:26:30, bartfab (slow) wrote: > Nit: Pass strings by const reference. Done. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:665: blacklist(new URLBlacklist(GetSegmentURLCallback())); On 2016/04/18 15:26:30, bartfab (slow) wrote: > Nit: Indent. Done. https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:749: // An optional path can come after port. Any string not containing '?' can be On 2016/04/18 15:26:30, bartfab (slow) wrote: > If you want to get into the business of documenting what a URI path looks like, > the rules are a bit more complex than that: > > https://tools.ietf.org/html/rfc3986#section-3.3 It's about the path from the filter, not the path from URI. In the code we don't do any validation on that. Users can put anything there and our parser will take all that until it encounters a '?' and consider it as a path! https://codereview.chromium.org/1692503002/diff/260001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:875: GURL("http://examplE.com/Path?Query=1")), On 2016/04/18 15:26:30, bartfab (slow) wrote: > This is a really odd indent. I would have expected 4 spaces from the start of > the line or from the start of |blacklist|. Done.
LGTM assuming Thiemo is OK.
https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/240001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:283: url_scheme + "://*")) { On 2016/04/18 15:16:36, igorcov wrote: > On 2016/04/18 13:12:39, Thiemo Nagel (no reviews) wrote: > > It cannot be assumed that the part after the scheme starts with //. I think > the > > slashes should be dropped here and the documentation updated. Cf. > > https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax > > For the filter definition we need the // part because that's the only way for us > to make distinction between a pattern like > scheme:* > and > host:* where * is considered as a port. * is not a valid port. Thus there is no ambiguity. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:279: // Check if it's a scheme pattern. "scheme pattern" doesn't seem precise enough. I'd suggest either "scheme:* pattern" or "scheme wildcard pattern". https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:280: if (base::LowerCaseEqualsASCII(base::StringPiece(filter.c_str()), I'd suggest to move this block above the kFileScheme block to make the code more easy to read and understand. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:287: if(query) Missing blank. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:408: // Filter custom schemes. Since this doesn't test interactions between different rules, it would be most straightforward to use IsBlacklistMatched(). Please also convert existing tests to IsBlacklistMatched() where it makes sense, i.e. where the test only applies to a single rule. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:707: TEST_F(URLBlacklistManagerTest, BlacklistBasicCoverage) { Please ensure that the following cases are covered (need to be converted to new syntax): + // Ommitting the scheme matches most standard schemes. + CheckBlacklistMatch("example.com", "chrome:example.com", true); + CheckBlacklistMatch("example.com", "chrome://example.com", true); + CheckBlacklistMatch("example.com", "file://example.com/", true); + CheckBlacklistMatch("example.com", "ftp://example.com", true); + CheckBlacklistMatch("example.com", "http://example.com", true); + CheckBlacklistMatch("example.com", "https://example.com", true); + CheckBlacklistMatch("example.com", "gopher://example.com", true); + CheckBlacklistMatch("example.com", "ws://example.com", true); + CheckBlacklistMatch("example.com", "wss://example.com", true); + + // Some schemes are not matched when the scheme is ommitted. + CheckBlacklistMatch("example.com", "about://example.com", false); + CheckBlacklistMatch("example.com", "about:example.com", false); + CheckBlacklistMatch("example.com/*", "filesystem:///something", false); + CheckBlacklistMatch("example.com", "custom://example.com", false); + CheckBlacklistMatch("example", "custom://example", false); + + // Case sensitivity. + CheckBlacklistMatch("http://example.com", "HTTP://example.com", true); + CheckBlacklistMatch("HTTP://example.com", "http://example.com", true); + CheckBlacklistMatch("http://EXAMPLE.COM", "http://example.com", true); + CheckBlacklistMatch("http://example.com/aB", "http://example.com/aB", true); + CheckBlacklistMatch("http://example.com/aB", "http://example.com/Ab", false); + CheckBlacklistMatch("http://example.com/aB", "http://example.com/ab", false); + CheckBlacklistMatch("http://example.com/aB", "http://example.com/AB", false); + CheckBlacklistMatch("*", "custom://anything", true); + // Star is not allowed in port numbers. + CheckBlacklistMatch("example.com:*", "http://example.com", false); + CheckBlacklistMatch("example.com:*", "http://example.com:8888", false);
https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:279: // Check if it's a scheme pattern. On 2016/04/19 12:59:01, Thiemo Nagel (no reviews) wrote: > "scheme pattern" doesn't seem precise enough. I'd suggest either "scheme:* > pattern" or "scheme wildcard pattern". Done. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:280: if (base::LowerCaseEqualsASCII(base::StringPiece(filter.c_str()), On 2016/04/19 12:59:01, Thiemo Nagel (no reviews) wrote: > I'd suggest to move this block above the kFileScheme block to make the code more > easy to read and understand. Done. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:287: if(query) On 2016/04/19 12:59:01, Thiemo Nagel (no reviews) wrote: > Missing blank. Done. https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/280001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:707: TEST_F(URLBlacklistManagerTest, BlacklistBasicCoverage) { On 2016/04/19 12:59:01, Thiemo Nagel (no reviews) wrote: > Please ensure that the following cases are covered (need to be converted to new > syntax): > > + // Ommitting the scheme matches most standard schemes. > + CheckBlacklistMatch("example.com", "chrome:example.com", true); > + CheckBlacklistMatch("example.com", "chrome://example.com", true); > + CheckBlacklistMatch("example.com", "file://example.com/", true); > + CheckBlacklistMatch("example.com", "ftp://example.com", true); > + CheckBlacklistMatch("example.com", "http://example.com", true); > + CheckBlacklistMatch("example.com", "https://example.com", true); > + CheckBlacklistMatch("example.com", "gopher://example.com", true); > + CheckBlacklistMatch("example.com", "ws://example.com", true); > + CheckBlacklistMatch("example.com", "wss://example.com", true); > + > + // Some schemes are not matched when the scheme is ommitted. > + CheckBlacklistMatch("example.com", "about://example.com", false); > + CheckBlacklistMatch("example.com", "about:example.com", false); > + CheckBlacklistMatch("example.com/*", "filesystem:///something", false); > + CheckBlacklistMatch("example.com", "custom://example.com", false); > + CheckBlacklistMatch("example", "custom://example", false); > + > + // Case sensitivity. > + CheckBlacklistMatch("http://example.com", "HTTP://example.com", true); > + CheckBlacklistMatch("HTTP://example.com", "http://example.com", true); > + CheckBlacklistMatch("http://EXAMPLE.COM", "http://example.com", true); > + CheckBlacklistMatch("http://example.com/aB", "http://example.com/aB", true); > + CheckBlacklistMatch("http://example.com/aB", "http://example.com/Ab", > false); > + CheckBlacklistMatch("http://example.com/aB", "http://example.com/ab", > false); > + CheckBlacklistMatch("http://example.com/aB", "http://example.com/AB", > false); > > + CheckBlacklistMatch("*", "custom://anything", true); > > + // Star is not allowed in port numbers. > + CheckBlacklistMatch("example.com:*", "http://example.com", false); > + CheckBlacklistMatch("example.com:*", "http://example.com:8888", false); Added, thanks for suggestions.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692503002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692503002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
On 2016/04/19 16:34:43, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1692503002/340001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1692503002/340001 It seems that the code doesn't compile on Android: FAILED: /b/build/slave/android/build/src/build/goma/client/gomacc /b/build/slave/android/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/android_webview/browser/android_webview_common.aw_content_browser_client.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DCR_CLANG_REVISION=266460-1 -DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_NOTIFICATIONS -DFIELDTRIAL_TESTING_ENABLED -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_SUPERVISED_USERS=1 -DVIDEO_HOLE=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_WEBVR -DSAFE_BROWSING_DB_REMOTE -DMOJO_USE_SYSTEM_IMPL -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DAPPCACHE_USE_SIMPLE_CACHE -DDISABLE_FFMPEG_VIDEO_DECODERS -DOPUS_FIXED_POINT -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_BUILD_FOR_ANDROID -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_NOEXCEPT= -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DCHROME_PNG_READ_PACK_SUPPORT -DMESA_EGL_NO_X11_HEADERS -DV8_SHARED -DUSING_V8_SHARED -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DANDROID -D__GNU_SOURCE=1 '-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -Igen -I../.. -I../../skia/config -Igen/ui/resources/ -I../../third_party/khronos -I../../gpu -Igen/angle -I../../third_party/WebKit/Source -Igen/protoc_out -I../../third_party/protobuf -I../../third_party/protobuf/src -I../../third_party/WebKit -Igen/third_party/WebKit -I../../third_party/opus/src/include -I../../skia/ext -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/WebKit -I../../third_party/icu/source/common -I../../third_party/libpng -I../../third_party/zlib -I../../third_party/libwebp -I../../third_party/ots/include -I../../third_party/qcms/src -I../../third_party/iccjpeg -I../../third_party/libjpeg_turbo -I../../third_party/mesa/src/include -I../../v8/include -Igen/policy -fstack-protector --param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall -Wno-extra -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wno-unused-local-typedefs -march=armv7-a -mtune=generic-armv7-a -mfpu=neon -mfloat-abi=softfp -mthumb -fno-tree-sra -fno-caller-saves -Wno-psabi -mthumb-interwork -ffunction-sections -funwind-tables -g -fstack-protector -fno-short-enums -finline-limit=64 --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm -Os -g -fdata-sections -ffunction-sections -fomit-frame-pointer -funwind-tables -g0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wno-abi -std=gnu++11 -Wno-narrowing -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk//sources/android/support/include -c ../../android_webview/browser/aw_content_browser_client.cc -o obj/android_webview/browser/android_webview_common.aw_content_browser_client.o In file included from ../../android_webview/browser/aw_content_browser_client.cc:21:0: ../../android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:51:8: error: 'bool android_webview::AwResourceDispatcherHostDelegate::HandleExternalProtocol(const GURL&, int, const WebContentsGetter&, bool, ui::PageTransition, bool)' marked override, but does not override bool HandleExternalProtocol( ^ FAILED: /b/build/slave/android/build/src/build/goma/client/gomacc /b/build/slave/android/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/android_webview/browser/renderer_host/android_webview_common.aw_resource_dispatcher_host_delegate.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DCR_CLANG_REVISION=266460-1 -DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_NOTIFICATIONS -DFIELDTRIAL_TESTING_ENABLED -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_SUPERVISED_USERS=1 -DVIDEO_HOLE=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_WEBVR -DSAFE_BROWSING_DB_REMOTE -DMOJO_USE_SYSTEM_IMPL -DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DAPPCACHE_USE_SIMPLE_CACHE -DDISABLE_FFMPEG_VIDEO_DECODERS -DOPUS_FIXED_POINT -DSKIA_DLL -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_BUILD_FOR_ANDROID -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_NOEXCEPT= -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DCHROME_PNG_READ_PACK_SUPPORT -DMESA_EGL_NO_X11_HEADERS -DV8_SHARED -DUSING_V8_SHARED -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DANDROID -D__GNU_SOURCE=1 '-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -Igen -I../.. -I../../skia/config -Igen/ui/resources/ -I../../third_party/khronos -I../../gpu -Igen/angle -I../../third_party/WebKit/Source -Igen/protoc_out -I../../third_party/protobuf -I../../third_party/protobuf/src -I../../third_party/WebKit -Igen/third_party/WebKit -I../../third_party/opus/src/include -I../../skia/ext -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/WebKit -I../../third_party/icu/source/common -I../../third_party/libpng -I../../third_party/zlib -I../../third_party/libwebp -I../../third_party/ots/include -I../../third_party/qcms/src -I../../third_party/iccjpeg -I../../third_party/libjpeg_turbo -I../../third_party/mesa/src/include -I../../v8/include -Igen/policy -fstack-protector --param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall -Wno-extra -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Wno-unused-local-typedefs -march=armv7-a -mtune=generic-armv7-a -mfpu=neon -mfloat-abi=softfp -mthumb -fno-tree-sra -fno-caller-saves -Wno-psabi -mthumb-interwork -ffunction-sections -funwind-tables -g -fstack-protector -fno-short-enums -finline-limit=64 --sysroot=../../third_party/android_tools/ndk//platforms/android-16/arch-arm -Os -g -fdata-sections -ffunction-sections -fomit-frame-pointer -funwind-tables -g0 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wno-abi -std=gnu++11 -Wno-narrowing -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk//sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk//sources/android/support/include -c ../../android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc -o obj/android_webview/browser/renderer_host/android_webview_common.aw_resource_dispatcher_host_delegate.o In file included from ../../android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:5:0: ../../android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:51:8: error: 'bool android_webview::AwResourceDispatcherHostDelegate::HandleExternalProtocol(const GURL&, int, const WebContentsGetter&, bool, ui::PageTransition, bool)' marked override, but does not override bool HandleExternalProtocol( ^
https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:256: std::string* query) { Please add DCHECK()s to ensure that none of the parameters is a nullptr. (Requires changing one call site in a unit test.) https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:258: That blank line seems superflous. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:260: // Check if it's a scheme wildcard pattern. You might want to add a blank line above the comment to make it more readable. Also, please wrap comments to 80 characters. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:270: *path = "/"; I don't understand why that is necessary. Why not simply leave |*path| blank as is done in line 350? https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:274: if (query) Once the DCHECK()s above are in place, the conditional can be removed. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:313: } else if ((*host)[0] == '.') { Direct array access scares me. Could you please replace by substr()? https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:72: // Splits a URL filter into its components. A GURL isn't used because these Please document that all parameters are mandatory, and that the return values supplied by the pointer parameters are only defined if the function returns true. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:176: // Will return whether |url| matches the |pattern|. Present tense is better style: "Returns whether ..." https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:374: // Filter only a certain path prefix. I think there's plenty of tests that could be converted to your new test style. The current layout of the code is difficult to read because similar tests are spread over different styles of writing tests and over different functions. Could you please organize the tests in a better way (group similar tests) and de-dupe? (Happy to discuss the details offline.) https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:739: EXPECT_EQ(true, IsBlacklistMatched(".example.com", I think it would be great if you could find a way to not break lines between the blacklist pattern and the URL to be matched. That would make the code much more readable, in my opinion. Some ideas: 1. Rename IsBlacklistMatched() --> IsMatch() 2. Break after EXPECT_EQ( 3. Break after EXPECT_EQ([bool], 4. Use the INSTANTIATE_TEST_CASE_P() mechanism to run test cases.
PTAL https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:256: std::string* query) { On 2016/04/19 18:18:31, Thiemo Nagel wrote: > Please add DCHECK()s to ensure that none of the parameters is a nullptr. > (Requires changing one call site in a unit test.) Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:258: On 2016/04/19 18:18:31, Thiemo Nagel wrote: > That blank line seems superflous. Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:260: // Check if it's a scheme wildcard pattern. On 2016/04/19 18:18:31, Thiemo Nagel wrote: > You might want to add a blank line above the comment to make it more readable. > Also, please wrap comments to 80 characters. Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:270: *path = "/"; On 2016/04/19 18:18:31, Thiemo Nagel (no reviews) wrote: > I don't understand why that is necessary. Why not simply leave |*path| blank as > is done in line 350? Because file paths for windows (see lines 289 - 293 which are part of old code). https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:274: if (query) On 2016/04/19 18:18:31, Thiemo Nagel wrote: > Once the DCHECK()s above are in place, the conditional can be removed. Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:313: } else if ((*host)[0] == '.') { On 2016/04/19 18:18:31, Thiemo Nagel wrote: > Direct array access scares me. Could you please replace by substr()? Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:176: // Will return whether |url| matches the |pattern|. On 2016/04/19 18:18:31, Thiemo Nagel wrote: > Present tense is better style: "Returns whether ..." Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:374: // Filter only a certain path prefix. On 2016/04/19 18:18:31, Thiemo Nagel wrote: > I think there's plenty of tests that could be converted to your new test style. > The current layout of the code is difficult to read because similar tests are > spread over different styles of writing tests and over different functions. > Could you please organize the tests in a better way (group similar tests) and > de-dupe? (Happy to discuss the details offline.) Done. https://codereview.chromium.org/1692503002/diff/340001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:739: EXPECT_EQ(true, IsBlacklistMatched(".example.com", On 2016/04/19 18:18:31, Thiemo Nagel wrote: > I think it would be great if you could find a way to not break lines between the > blacklist pattern and the URL to be matched. That would make the code much more > readable, in my opinion. > > Some ideas: > 1. Rename IsBlacklistMatched() --> IsMatch() > 2. Break after EXPECT_EQ( > 3. Break after EXPECT_EQ([bool], > 4. Use the INSTANTIATE_TEST_CASE_P() mechanism to run test cases. Done.
https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:249: // All arguments are mandatory. Please move method documentation to the header file. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:287: // Check if it's a scheme wildcard pattern. We will accept both versions Nit: Present tense is better style. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:74: // Returns false if the URL couldn't be parsed. Please mention that in the "false" case the values of the output parameters are undefined. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:177: bool IsMatched(const std::string& pattern, const std::string& url) { Nit: I think IsMatch() would be better English. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:182: blocked->Append(new base::StringValue(pattern)); Appending a raw pointer is deprecated (cf. link below). Please wrap it in a unique_ptr. (Feel free to convert other, existnig uses of that method if you feel like it.) https://code.google.com/p/chromium/codesearch#chromium/src/base/values.h&sq=p... https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:424: // Block an ip address. Please convert to IsMatch() style. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:786: TEST_F(URLBlacklistManagerTest, UseBlacklistState) { I think this should be converted to the IsMatch() style and probably moved to the place in BlacklistBasicCoverage that already tests schemes. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:851: TEST_F(URLBlacklistManagerTest, UseBlacklistCaseInsensitive) { I think this should be converted to the IsMatch() style: All the case sensitivity tests should be in one place. (There are more in BlacklistBasicCoverage.)
https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:182: blocked->Append(new base::StringValue(pattern)); BTW: std::WrapUnique seems to be the canonical way to wrap.
https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:249: // All arguments are mandatory. On 2016/04/20 15:28:09, Thiemo Nagel wrote: > Please move method documentation to the header file. Done. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:287: // Check if it's a scheme wildcard pattern. We will accept both versions On 2016/04/20 15:28:09, Thiemo Nagel wrote: > Nit: Present tense is better style. Done. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:74: // Returns false if the URL couldn't be parsed. On 2016/04/20 15:28:09, Thiemo Nagel wrote: > Please mention that in the "false" case the values of the output parameters are > undefined. Done. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:177: bool IsMatched(const std::string& pattern, const std::string& url) { On 2016/04/20 15:28:10, Thiemo Nagel wrote: > Nit: I think IsMatch() would be better English. Done. https://codereview.chromium.org/1692503002/diff/400001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:182: blocked->Append(new base::StringValue(pattern)); On 2016/04/20 16:16:52, Thiemo Nagel wrote: > BTW: std::WrapUnique seems to be the canonical way to wrap. Used AppendString instead. Makes code easier to read.
https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:197: std::unique_ptr<base::Value> pattern_ptr(new base::StringValue(pattern)); pattern_ptr is unused. https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:200: if(use_whitelist) { Please insert blank. https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:814: EXPECT_EQ(in_blacklist, GetMatch("custom://*", "custom://my_app", false)); Why not use IsMatch()? I don't see an advantage of using GetMatch() here. https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:836: // Host is case insensitive. Please move to the other case sensitivity test and use IsMatch(). (I don't see an advantage of using GetMatch() here.)
https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:197: std::unique_ptr<base::Value> pattern_ptr(new base::StringValue(pattern)); On 2016/04/21 11:25:58, Thiemo Nagel wrote: > pattern_ptr is unused. Done. https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:200: if(use_whitelist) { On 2016/04/21 11:25:58, Thiemo Nagel wrote: > Please insert blank. Done. https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:814: EXPECT_EQ(in_blacklist, GetMatch("custom://*", "custom://my_app", false)); On 2016/04/21 11:25:58, Thiemo Nagel wrote: > Why not use IsMatch()? I don't see an advantage of using GetMatch() here. Done. https://codereview.chromium.org/1692503002/diff/440001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:836: // Host is case insensitive. On 2016/04/21 11:25:58, Thiemo Nagel wrote: > Please move to the other case sensitivity test and use IsMatch(). (I don't see > an advantage of using GetMatch() here.) Done.
The changes don't apply to master. It seems that another rebase is needed. https://codereview.chromium.org/1692503002/diff/480001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/480001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:803: // Check schemes case insensitive scenarios. Some of these are duplicates. Please merge with the tests above.
https://codereview.chromium.org/1692503002/diff/480001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/480001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:803: // Check schemes case insensitive scenarios. On 2016/04/21 13:15:09, Thiemo Nagel wrote: > Some of these are duplicates. Please merge with the tests above. Done.
> Only the format |scheme|://* will be accepted. We also accept scheme:* now. Also it might be clearer to write scheme://* instead of |scheme|://*.
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the format |scheme|://* will be accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: network_delegate.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: network_delegate.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:277: *path = (lc_filter == "file://*") ? "" : file_path.AsUTF8Unsafe(); For consistency, we should also support "file:*" (and add that to unit tests). https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:412: // Filter custom schemes. Please merge with custom scheme tests in BlacklistBasicCoverage (below). https://codereview.chromium.org/1692503002/diff/520001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1692503002/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:925: // Get the state, if url is in blacklist, whitelist or in none of those. Please add pipes around url: |url|
https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:277: *path = (lc_filter == "file://*") ? "" : file_path.AsUTF8Unsafe(); On 2016/04/21 14:45:10, Thiemo Nagel wrote: > For consistency, we should also support "file:*" (and add that to unit tests). Done. https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... File components/policy/core/browser/url_blacklist_manager_unittest.cc (right): https://codereview.chromium.org/1692503002/diff/520001/components/policy/core... components/policy/core/browser/url_blacklist_manager_unittest.cc:412: // Filter custom schemes. On 2016/04/21 14:45:10, Thiemo Nagel wrote: > Please merge with custom scheme tests in BlacklistBasicCoverage (below). Done. https://codereview.chromium.org/1692503002/diff/520001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1692503002/diff/520001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:925: // Get the state, if url is in blacklist, whitelist or in none of those. On 2016/04/21 14:45:10, Thiemo Nagel wrote: > Please add pipes around url: |url| Done.
Please also update the documentation of URLBlacklist and URLWhitelist in policy_templates.json: Instead of describing the pattern format, just link to: https://www.chromium.org/administrators/url-blacklist-filter-format https://codereview.chromium.org/1692503002/diff/540001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/540001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:277: *path = (lc_filter == "file://*" || lc_filter == "file:*") ? Please use if then else. This ternary across two lines is really hard to read. Also, below you first test for :* and then for ://*. Please try to be consistent!
Updated policy_templates.json too. PTAL. https://codereview.chromium.org/1692503002/diff/540001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/540001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:277: *path = (lc_filter == "file://*" || lc_filter == "file:*") ? On 2016/04/21 16:49:30, Thiemo Nagel wrote: > Please use if then else. This ternary across two lines is really hard to read. > > Also, below you first test for :* and then for ://*. Please try to be > consistent! Done.
As usual, LGTM assuming Thiemo has no further concerns.
https://codereview.chromium.org/1692503002/diff/580001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1692503002/diff/580001/components/policy/reso... components/policy/resources/policy_templates.json:4317: Each URL pattern can either be a pattern for local files or a generic URL pattern. Local file patterns are of the format 'file://path', where path should be an absolute path to block. All file system locations for which that path is a prefix will be blocked. Please remove duplications. No need to re-phrase what is already explained in the linked docs.
https://codereview.chromium.org/1692503002/diff/580001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/1692503002/diff/580001/components/policy/reso... components/policy/resources/policy_templates.json:4317: Each URL pattern can either be a pattern for local files or a generic URL pattern. Local file patterns are of the format 'file://path', where path should be an absolute path to block. All file system locations for which that path is a prefix will be blocked. On 2016/04/25 09:11:10, Thiemo Nagel wrote: > Please remove duplications. No need to re-phrase what is already explained in > the linked docs. Done.
components/policy LGTM
igorcov@chromium.org changed reviewers: + asanka@chromium.org, boliu@chromium.org, kinuko@chromium.org, thakis@chromium.org
boliu@chromium.org: Please review changes in android_webview/* thakis@chromium.org: Please review changes in chrome/browser/* asanka@chromium.org: Please review changes in net/* kinuko@chromium.org: Please review changes in content/*
On 2016/04/25 13:35:57, igorcov wrote: > mailto:boliu@chromium.org: Please review changes in android_webview/* rubberstamp lgtm > > mailto:thakis@chromium.org: Please review changes in chrome/browser/* > > mailto:asanka@chromium.org: Please review changes in net/* > > mailto:kinuko@chromium.org: Please review changes in content/*
LGTM with some nits https://codereview.chromium.org/1692503002/diff/600001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:754: #if defined(ENABLE_CONFIGURATION_POLICY) Remove this #ifdef https://codereview.chromium.org/1692503002/diff/600001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/1692503002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.h:85: #if defined(ENABLE_CONFIGURATION_POLICY) We have deprecated ENABLE_CONFIGURATION_POLICY - please remove this #ifdef. We don't support non-policy builds any longer. https://codereview.chromium.org/1692503002/diff/600001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/600001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:67: GetURLBlacklistState(const GURL& url) const; consider wrapping after the ( rather than after the return value. https://codereview.chromium.org/1692503002/diff/600001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1692503002/diff/600001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:84: bool is_whitelisted); Document what this new parameter is (what is the effect of is_whitelisted) https://codereview.chromium.org/1692503002/diff/600001/net/base/network_deleg... File net/base/network_delegate.h (right): https://codereview.chromium.org/1692503002/diff/600001/net/base/network_deleg... net/base/network_delegate.h:57: // or doesn't match anything in either lists as defined in URLBlacklist and nit: either lists -> either list.
lgtm
https://codereview.chromium.org/1692503002/diff/600001/chrome/browser/net/chr... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/600001/chrome/browser/net/chr... chrome/browser/net/chrome_network_delegate.cc:754: #if defined(ENABLE_CONFIGURATION_POLICY) On 2016/05/02 09:32:11, Andrew T Wilson (Slow) wrote: > Remove this #ifdef Done. https://codereview.chromium.org/1692503002/diff/600001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.h (right): https://codereview.chromium.org/1692503002/diff/600001/components/policy/core... components/policy/core/browser/url_blacklist_manager.h:67: GetURLBlacklistState(const GURL& url) const; On 2016/05/02 09:32:11, Andrew T Wilson (Slow) wrote: > consider wrapping after the ( rather than after the return value. Done. https://codereview.chromium.org/1692503002/diff/600001/content/public/browser... File content/public/browser/resource_dispatcher_host_delegate.h (right): https://codereview.chromium.org/1692503002/diff/600001/content/public/browser... content/public/browser/resource_dispatcher_host_delegate.h:84: bool is_whitelisted); On 2016/05/02 09:32:11, Andrew T Wilson (Slow) wrote: > Document what this new parameter is (what is the effect of is_whitelisted) Done. https://codereview.chromium.org/1692503002/diff/600001/net/base/network_deleg... File net/base/network_delegate.h (right): https://codereview.chromium.org/1692503002/diff/600001/net/base/network_deleg... net/base/network_delegate.h:57: // or doesn't match anything in either lists as defined in URLBlacklist and On 2016/05/02 09:32:11, Andrew T Wilson (Slow) wrote: > nit: either lists -> either list. Done.
Why does //net have to be involved at all? //net doesn't take the blacklist state into account. Also, do you envision standard schemes being blacklisted?
chrome/ lgtm
https://codereview.chromium.org/1692503002/diff/640001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1692503002/diff/640001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:913: ResourceRequestInfoImpl* info = loader->GetRequestInfo(); So, we need some place to inject the url_blacklist_manager - maybe in ResourceLoader or ResourceRequestInfo, not sure which one is appropriate on the chrome side. We can't use ResourceDispatcherHostDelegate because it's a singleton and we need something that is per-profile.
Fixed the code to not involve net/ module. Main changes: ResourceDispatcherHostImpl is passing the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol. ResourceDispatcherHostDelegate uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted. Please take a look on new changes (versions 33 and 34).
content/ lgtm https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:265: const std::string lc_filter = base::ToLowerASCII(base::StringPiece(filter)); nit: do we need this explicit StringPiece cast? (here and below) https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:282: } nit: no need to have {} for one-line block (having them is ok too but it looks inconsistent in this file) https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:307: } ditto
https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:265: const std::string lc_filter = base::ToLowerASCII(base::StringPiece(filter)); On 2016/05/19 13:06:00, kinuko wrote: > nit: do we need this explicit StringPiece cast? (here and below) Removed that, thanks. https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:282: } On 2016/05/19 13:06:01, kinuko wrote: > nit: no need to have {} for one-line block (having them is ok too but it looks > inconsistent in this file) Done. https://codereview.chromium.org/1692503002/diff/760001/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:307: } On 2016/05/19 13:06:00, kinuko wrote: > ditto Done.
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: network_delegate.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - If url is not standard, use network_delegate from request to check the state of URL (whitelist, blacklist or neutral). url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: url_blacklist_manager.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - Pass the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol. resource_dispatcher_host_delegate.cc - uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted. url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
LGTM with a couple formatting nits. https://codereview.chromium.org/1692503002/diff/720002/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/720002/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:560: policy::URLBlacklist::URLBlacklistState::URL_IN_WHITELIST)); I'm pretty sure this needs to be indented https://codereview.chromium.org/1692503002/diff/720002/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/720002/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:280: } else Since the if statement has {} you need it for the else statement also.
https://codereview.chromium.org/1692503002/diff/720002/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1692503002/diff/720002/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:560: policy::URLBlacklist::URLBlacklistState::URL_IN_WHITELIST)); On 2016/05/19 15:02:43, Andrew T Wilson (Slow) wrote: > I'm pretty sure this needs to be indented Done. https://codereview.chromium.org/1692503002/diff/720002/components/policy/core... File components/policy/core/browser/url_blacklist_manager.cc (right): https://codereview.chromium.org/1692503002/diff/720002/components/policy/core... components/policy/core/browser/url_blacklist_manager.cc:280: } else On 2016/05/19 15:02:43, Andrew T Wilson (Slow) wrote: > Since the if statement has {} you need it for the else statement also. Done.
asanka@chromium.org changed reviewers: - asanka@chromium.org
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, boliu@chromium.org, tnagel@chromium.org, thakis@chromium.org, kinuko@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/1692503002/#ps790001 (title: "Fixed review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692503002/790001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692503002/790001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, kinuko@chromium.org, atwilson@chromium.org, boliu@chromium.org, tnagel@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1692503002/#ps810001 (title: "Fixed compile problem")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692503002/810001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692503002/810001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, kinuko@chromium.org, atwilson@chromium.org, boliu@chromium.org, tnagel@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1692503002/#ps830001 (title: "Fixed compile errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692503002/830001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692503002/830001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, kinuko@chromium.org, atwilson@chromium.org, boliu@chromium.org, tnagel@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1692503002/#ps890001 (title: "Fixed compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692503002/890001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692503002/890001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by igorcov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692503002/890001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692503002/890001
Message was sent while issue was closed.
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: url_blacklist_manager.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - Pass the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol. resource_dispatcher_host_delegate.cc - uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted. url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: url_blacklist_manager.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - Pass the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol. resource_dispatcher_host_delegate.cc - uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted. url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ==========
Message was sent while issue was closed.
Committed patchset #46 (id:890001)
Message was sent while issue was closed.
Description was changed from ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: url_blacklist_manager.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - Pass the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol. resource_dispatcher_host_delegate.cc - uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted. url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 ========== to ========== Functionality to allow blacklist and whitelist of custom schemes. These changes will allow admins to define custom schemes as part of URLBlacklist and URLWhitelist policies. Only the formats scheme://* and scheme:* are accepted. The URLs of other formats containing custom schemes will be considered invalid and ignored. In case a custom scheme is defined in whitelist, all the URLs for that scheme will proceed to be launched without asking the user. In case a custom scheme is defined in blacklist, all the URLs with that scheme will have the same behavior as standard blacklisted URLs. Most important changes done: url_blacklist_manager.h - New enum URLBlacklistState added, to identify if the URL is in blacklist, whitelist or none. It is used to separate behavior of custom scheme URLs, so that: - if URL is whitelisted it will be launched without any warning/notification. - if URL is blacklisted it will redirect user to blocked page information. - if URL is not matched with any schemes from above, the old behavior is preserved meaning the user is asked if he wants to launch the URL. resource_dispatcher_host_impl.cc - Pass the ResourceContext as additional parameter to ResourceDispatcherHostDelegate when handling external protocol. resource_dispatcher_host_delegate.cc - uses PorfileIOData based on ResourceContext to detect if URL is blacklisted or whitelisted. url_blacklist_manager.cc - Implement the new method to retrieve the URL state, and add code to allow the custom schemes in filter definitions. BUG=158436 Committed: https://crrev.com/eccb40b717c49c968a87f91fba9b23c21f4f9a3e Cr-Commit-Position: refs/heads/master@{#395305} ==========
Message was sent while issue was closed.
Patchset 46 (id:??) landed as https://crrev.com/eccb40b717c49c968a87f91fba9b23c21f4f9a3e Cr-Commit-Position: refs/heads/master@{#395305} |
