Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(318)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 7716003: WIP: URL blacklisting by policy. (Closed)

Created:
9 years, 4 months ago by Joao da Silva
Modified:
9 years, 3 months ago
Visibility:
Public.

Description

WIP: URL blacklisting by policy. Introduces two new policies: a blacklist and a whitelist. Matching is done by searching for the most specific matching domain that has a filter. Only HTTP, HTTPS and FTP schemes can be filtered. Paths are matched by longest prefix. BUG=49612 TEST=Set the policies. URLs matching the filters should be blocked. unit_tests:TestHostBlacklistManager.*

Patch Set 1 #

Patch Set 2 : More progress, cleanups #

Patch Set 3 : Refactored a lot of stuff, incomplete #

Patch Set 4 : Added path filtering #

Patch Set 5 : Always filter including paths #

Total comments: 14

Patch Set 6 : Reviewed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1071 lines, -5 lines) Patch
M chrome/browser/extensions/extension_webrequest_api_unittest.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.h View 1 2 3 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.cc View 1 2 3 4 5 3 chunks +10 lines, -1 line 0 comments Download
A chrome/browser/policy/url_blacklist_manager.h View 1 2 3 4 5 1 chunk +190 lines, -0 lines 0 comments Download
A chrome/browser/policy/url_blacklist_manager.cc View 1 2 3 4 5 1 chunk +439 lines, -0 lines 0 comments Download
A chrome/browser/policy/url_blacklist_manager_unittest.cc View 1 2 3 4 1 chunk +382 lines, -0 lines 0 comments Download
M chrome/browser/prefs/browser_prefs.cc View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.h View 1 2 3 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.h View 1 2 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 2 6 chunks +13 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Joao da Silva
Here's the current WIP. Feel free to comment on it :-) This is built upon ...
9 years, 3 months ago (2011-08-30 17:56:12 UTC) #1
Mattias Nissler (ping if slow)
http://codereview.chromium.org/7716003/diff/14001/chrome/browser/net/chrome_network_delegate.cc File chrome/browser/net/chrome_network_delegate.cc (right): http://codereview.chromium.org/7716003/diff/14001/chrome/browser/net/chrome_network_delegate.cc#newcode77 chrome/browser/net/chrome_network_delegate.cc:77: // TODO(joaodasilva): this error code isn't quite correct period ...
9 years, 3 months ago (2011-08-31 14:58:09 UTC) #2
Joao da Silva
9 years, 3 months ago (2011-09-01 12:47:36 UTC) #3 
Reviewed

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:77: // TODO(joaodasilva): this
error code isn't quite correct
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> period
> 
> Can we just define a new error code then?

@willchan suggests using ERR_NETWORK_ACCESS_DENIED. Having a policy-specific
error code would be a layering violation in net/...

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.cc (right):

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:74: PathFilter* f;
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> use a better name instead of f, maybe filter?

Done. |filter| is taken, renamed to |path_filter|.

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:161: }
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:165: break;
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.cc:238: namespace {
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> move anonymous namespace to top of file.

Done.

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
File chrome/browser/policy/url_blacklist_manager.h (right):

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:64: // aren't canonical URLs.
Returns false if the URL couldn't be parsed.
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> Actually, isn't the problem with GURL that it _does_ canonicalize the URL?

Indeed it does. The problem is that GURL doesn't accept invalid URLs, like
"google.com". The comments has been changed to clarify that.

http://codereview.chromium.org/7716003/diff/14001/chrome/browser/policy/url_b...
chrome/browser/policy/url_blacklist_manager.h:82: match_subdomains(match)  { }
On 2011/08/31 14:58:09, Mattias Nissler wrote:
> remove space after ) and between {}

Done.

Powered by Google App Engine
This is Rietveld 408576698