Functionality to allow blacklist and whitelist of custom schemes

components/policy/core/browser/url_blacklist_manager.cc

Index: components/policy/core/browser/url_blacklist_manager.cc
diff --git a/components/policy/core/browser/url_blacklist_manager.cc b/components/policy/core/browser/url_blacklist_manager.cc
index ffc8dacb602d8646f41a87fda11a62db2fe038d9..f2574fe2c7546d8950fc3c90d961998879433f39 100644
--- a/components/policy/core/browser/url_blacklist_manager.cc
+++ b/components/policy/core/browser/url_blacklist_manager.cc
@@ -26,6 +26,7 @@
 #include "net/base/net_errors.h"
 #include "url/third_party/mozilla/url_parse.h"
 #include "url/url_constants.h"
+#include "url/url_util.h"
 
 using url_matcher::URLMatcher;
 using url_matcher::URLMatcherCondition;
@@ -230,6 +231,36 @@ bool URLBlacklist::IsURLBlocked(const GURL& url) const {
   return !max->allow;
 }
 
+net::NetworkDelegate::URLBlacklistState URLBlacklist::GetURLBlacklistState(
+    const GURL& url) const {
+  std::set<URLMatcherConditionSet::ID> matching_ids =
+      url_matcher_->MatchURL(url);
+
+  const FilterComponents* max = NULL;

[Thiemo Nagel, 2016/03/08 18:44:34]

NULL --> nullptr

[igorcov1, 2016/04/12 07:01:45]

Done.

+  for (std::set<URLMatcherConditionSet::ID>::iterator id = matching_ids.begin();
+       id != matching_ids.end(); ++id) {
+    std::map<int, FilterComponents>::const_iterator it = filters_.find(*id);
+    DCHECK(it != filters_.end());
+    const FilterComponents& filter = it->second;
+    if (!max || FilterTakesPrecedence(filter, *max))
+      max = &filter;
+  }
+
+  // Default neutral.
+  if (!max)
+    return net::NetworkDelegate::URLBlacklistState::URL_NEUTRAL_STATE;
+
+  // Some of the internal Chrome URLs are not affected by the "*" in the
+  // blacklist. Note that the "*" is the lowest priority filter possible, so
+  // any higher priority filter will be applied first.
+  if (max->IsBlacklistWildcard() && BypassBlacklistWildcardForURL(url))
+    return net::NetworkDelegate::URLBlacklistState::URL_IN_WHITELIST;
+
+  return max->allow ?
+      net::NetworkDelegate::URLBlacklistState::URL_IN_WHITELIST :
+      net::NetworkDelegate::URLBlacklistState::URL_IN_BLACKLIST;
+}
+
 size_t URLBlacklist::Size() const {
   return filters_.size();
 }
@@ -245,7 +276,8 @@ bool URLBlacklist::FilterToComponents(SegmentURLCallback segment_url,
                                       std::string* query) {
   url::Parsed parsed;
 
-  if (segment_url(filter, &parsed) == url::kFileScheme) {
+  std::string url_scheme = segment_url(filter, &parsed);
+  if (url_scheme == url::kFileScheme) {
     base::FilePath file_path;
     if (!net::FileURLToFilePath(GURL(filter), &file_path))
       return false;
@@ -264,8 +296,19 @@ bool URLBlacklist::FilterToComponents(SegmentURLCallback segment_url,
     return true;
   }
 
-  if (!parsed.host.is_nonempty())
+  // Check if a custom scheme pattern is defined.
+  if (!url_scheme.empty() && !parsed.host.is_nonempty() &&

[Thiemo Nagel, 2016/03/08 18:44:34]

Why must the host be empty?  The IsStandard() call should be sufficient to
indicate whether it's a custom scheme or not.

[igorcov, 2016/03/09 15:21:06]

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.

[Thiemo Nagel, 2016/03/10 14:37:26]

I don't think that we should depend on undocumented implementation details of
SegmentURL().  This makes for brittle code.

[igorcov1, 2016/04/12 07:01:45]

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.

[Thiemo Nagel, 2016/04/12 13:35:47]

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.

[igorcov, 2016/04/12 16:09:30]

Move the code outside of checking the URL host. The tests pass and the
functionality that I checked remained the same.

[Thiemo Nagel, 2016/04/12 17:20:57]

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.

+      !url::IsStandard(url_scheme.c_str(),
+                       url::Component(0,
+                           static_cast<int>(url_scheme.length()))) &&
+      filter == url_scheme + "://*") {

[Thiemo Nagel, 2016/03/08 18:44:34]

This breaks normalization of URLs and schemes (eg. removal of leading whitespace
or converting to lower case).

[igorcov, 2016/03/09 15:21:06]

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.

[Thiemo Nagel, 2016/03/10 14:37:26]

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.

[igorcov1, 2016/04/12 07:01:45]

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://*".

[Thiemo Nagel, 2016/04/12 14:43:05]

The way I read GetValidScheme(), normalization only happens for "valid" schemes.
 Thus I'd assume that "FTP://*" would fail.

[igorcov, 2016/04/12 16:09:30]

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.

[Thiemo Nagel, 2016/04/12 17:20:57]

I don't think this is acceptable.  Please implement a proper solution, even if
it takes more effort.

[igorcov, 2016/04/18 15:16:35]

Implemented to have scheme, host and path case insensitive. The query will
remain
case-sensitive.

+    scheme->assign(url_scheme);
+    return true;
+  }
+
+  if (!parsed.host.is_nonempty()) {
     return false;
+  }
 
   if (parsed.scheme.is_nonempty())
     scheme->assign(filter, parsed.scheme.begin, parsed.scheme.len);
@@ -491,6 +534,12 @@ bool URLBlacklistManager::IsURLBlocked(const GURL& url) const {
   return blacklist_->IsURLBlocked(url);
 }
 
+net::NetworkDelegate::URLBlacklistState
+URLBlacklistManager::GetURLBlacklistState(const GURL& url) const {
+  DCHECK(io_task_runner_->RunsTasksOnCurrentThread());
+  return blacklist_->GetURLBlacklistState(url);
+}
+
 bool URLBlacklistManager::ShouldBlockRequestForFrame(const GURL& url,
                                                      int* reason) const {
   DCHECK(io_task_runner_->RunsTasksOnCurrentThread());