Almost all actions in Declarative Web Request require all_urls host permissions

Almost all actions in Declarative Web Request require all_urls host permissions

The whitelisted actions are:
 * ignoring a rule
 * cancelling a request
 * redirecting to a blank image or an empty document
+ SendMessageToExtension only checks the request's URL against the host permissions (does not require full permissions).

BUG=145456
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196959

[vabr (Chromium), 2013-04-19 20:00:00]

Hi Matt and Dominic,

You both took par in the discussion, so adding you here.

This CL is not complete yet, but a substantial part is ready for review.
Currently the CL contains these changes:

WebRequestAction
  * Changing the HostPermissionsStrategy to fit current needs.
  * Minor refactoring of storing the strategy (it's a life-time constant of the
action now, no virtual accessors).
  * Extending the unit tests to check the strategy handling. This required to
expose some helper functions from other tests (see below), and introducing a
test fixture to get the threads and message loops right for URL requests and
extension info map at the same time.

WebRequestPermissions
  * CanExtensionAccessURL: Changed the "enforce_host_permission" flag to hold
more values (no check, host check, all hosts check).
  * Added a unit test for that change of CanExtensionAccessURL (fixture needed
similarly as for the action tests).

(D)WR API tests:
  * Got rid of the WebRequestDeclarativePermissions API test -- what that test
checked in addition to the existing tests should now be fully covered by the WR
action unit tests.
  * Added "<all_urls>" to the common manifest. As a result, a single WR API test
stopped working (simpleLoadNonVisible, it tested exactly that host permissions
work for WebRequest). I removed that test, because the CanExtensionAccessURL
unit test now checks this.
  * I also removed "testPermission" from the second part of DWR API tests. That
feature is also covered by the new unit tests for action.

chrome/common/extensions/extension_test_util.h
chrome/common/extensions/extension_unittest.cc
chrome/common/extensions/permissions/permission_set_unittest.cc
permissions_updater_unittest.cc
  * I took some LoadManifest* helper functions out of extension_unittest and
exposed them here. Those functions were copied on a couple of places
(permissions_updater_unittest.cc), and I needed them on two more.
extension_test_util looked like the best place for them.

Extensions docs
  * Updated the declarativeWebRequest.html intro with the actual version of
permission requirements.


My TODO is:
  * Add error messages in cases where a rule tried to apply an action and was
denied to do so. 
  * Possibly also a unit test with an always-false mock of
CanExtensionAccessURL, checking that CanExtensionAccessURL is always called.

As usual, any comments you have are very welcome!

Thanks,
Vaclav

[Matt Perry, 2013-04-19 21:55:28]

Looks good so far.

https://codereview.chromium.org/14358004/diff/14001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/14358004/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:90:
WebRequestAction();
Can the default constructor be removed?

https://codereview.chromium.org/14358004/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html
(right):

https://codereview.chromium.org/14358004/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html:50:
"http://www.google.com", it also may set up a rule to send a message to this
nit: restruct a bit to, 'such an extension may set up a rule to
* cancel a request to "http://www.google.com" or "http://anything.else.com"'
* send a message when navigating to "http://www.google.com" but not to
"http://something.else.com"
The extension cannot set up a rule to redirect...'

[vabr (Chromium), 2013-04-25 19:08:03]

Hi Matt,
Please take another look.

PS1 -> PS2:
minor fixes, see the description of PS2

PS2 -> PS3:
permissions_updater_unittest.cc: the fix for compilation on Win
the rest is just rebasing

PS3->PS4:
declarative_rule.h:
Making ConsistencyChecker a callback.

content_rules_registry.cc, declarative_rule_unittest.cc:
Nececssary changes because of Making ConsistencyChecker a callback.

webrequest_action.*:
Added a type->name map for actions to have informative error messages. The most
logical place for that seemed to be the action factory, because this mapping is
something not tied to a particular action, but to their subtypes in general.

webrequest_rules_checker.*:
Putting the two consistency checks together. One was the existing check for
stages the other is a new test for permissions.
I optimised the former (stage checker), it has no more nested for loops, for
example. Also it's unittest was moved to webrequest_rules_checker_unittest.cc.
For the latter, I only test that an action requiring <all_urls> is not used
without that permission. I do not check that the SendMessageToExtension action
is used with the correct permissions, because of the following:
 * SMTE action requires host permissions for the URL it is triggered by. That's
the expected meaning of host permissions, no surprise for the developers there.
 * Checking that the corresponding conditions filter all URLs outside of host
permissions would be quite complex. Given the previous point, I don't think it's
worth it.

webrequest_rules_registry*:
CheckConsistency was moved to the WR rules checker. I removed also the unit
test, because:
1. there is a unittest in the checker for the stage checking, and
2. there is a unittest in declarative_rule_unittest.cc for the checker being
called (DeclarativeRuleTest.CheckConsistency)


Thanks,
Vaclav

https://codereview.chromium.org/14358004/diff/14001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/14358004/diff/14001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:90:
WebRequestAction();
On 2013/04/19 21:55:28, Matt Perry wrote:
> Can the default constructor be removed?

Done.

https://codereview.chromium.org/14358004/diff/14001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html
(right):

https://codereview.chromium.org/14358004/diff/14001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/intros/declarativeWebRequest.html:50:
"http://www.google.com", it also may set up a rule to send a message to this
On 2013/04/19 21:55:28, Matt Perry wrote:
> nit: restruct a bit to, 'such an extension may set up a rule to
> * cancel a request to "http://www.google.com" or "http://anything.else.com"'
> * send a message when navigating to "http://www.google.com" but not to
> "http://something.else.com"
> The extension cannot set up a rule to redirect...'

Done.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative/declarative_rule.h (right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative/declarative_rule.h:194: typedef
base::Callback<bool(const ConditionSet* conditions,
I switched to callback here from a bare function pointer so that new checker can
be made from methods or functions with additional arguments (the host permission
checker is one instance).

The alternatives I considered were:
1. Change the function pointer to have most generic list of arguments (adding
Extension* in this case).
That seemed wasteful and confusing.
2. Switching to Callback, and make it a Closure instead, binding the conditions,
actions and error in.
I don't like binding the error in particular, since it is not clear the target
string will be alive when the callback gets called. For leaving the conditions
and actions there I don't have a strong preference.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:381:
action_names.insert(ActionNames::value_type(type, name));
Inserting instead of using [] because I want the strings in the map stay const.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:99:
virtual int GetStages() const = 0;
In the same way permissions_strategy() went from a virtual function to a
construction parameter, I want to convert GetStages, GetType and
GetMinimumPriority. The stored value is offset by a shorter virtual function
table, and the invocation will be faster.
However, since this CL is long enough, I'll do it separately.

[Matt Perry, 2013-04-25 19:59:21]

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:341:
std::map<WebRequestAction::Type, const std::string> action_names;
use the typedef?

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:381:
action_names.insert(ActionNames::value_type(type, name));
On 2013/04/25 19:08:03, vabr (Chromium) wrote:
> Inserting instead of using [] because I want the strings in the map stay
const.

Weird... [] prevents that?

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:382:
std::ostringstream modify_request_cookie;
Don't use streams:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams

Just use std::string and concat to it.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:432:
const std::map<WebRequestAction::Type, const std::string>& names =
typedef?

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:76:
STRATEGY_NONE    = 1 << 0,  // Do not check host permissions.
Can you have multiple strategies? I don't see how...

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:18:
"To execute the action '*', host permissions for all URLs are needed";
nit: "To execute the action '*', you need to request host permission for all
URLs."

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:28:
return base::Bind(&WebRequestRulesChecker::Checker, base::Unretained(this));
Making this a class seems like overkill. Checker and the other methods can just
be static methods (or globals in this file) if you bind Extension instead of
|this|. Something like:

static GetChecker(Extension* ext) {
  return base::Bind(&Checker, base::Unretained(ext));
}

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:56:
if ((*action_iter)->host_permissions_strategy() & allowed_strategies)
Oh, I see why you made them bitmasks now. I'd still prefer to keep them singular
values, at the expense of making this code a little more verbose, to make it
clear you can only have one strategy at a time.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:92:
*error = kActionCannotBeExecuted;
Add the action's name in the error message.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/permissions_updater_unittest.cc (right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/permissions_updater_unittest.cc:101: #if
defined(OS_WIN)
Just use FilePath::AsUTF8Unsafe()

[vabr (Chromium), 2013-04-26 09:58:51]

Thanks a lot, Matt.

All the changes now are in response to your comments.

Additionally, in webrequest_rules_registry*, I had to amend the test fixture
with the extension map and some Extension objects, because it is now needed
sooner for the checks during AddRules.

Please take yet another look.

Vaclav

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:341:
std::map<WebRequestAction::Type, const std::string> action_names;
On 2013/04/25 19:59:21, Matt Perry wrote:
> use the typedef?

Done.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:381:
action_names.insert(ActionNames::value_type(type, name));
On 2013/04/25 19:59:21, Matt Perry wrote:
> On 2013/04/25 19:08:03, vabr (Chromium) wrote:
> > Inserting instead of using [] because I want the strings in the map stay
> const.
> 
> Weird... [] prevents that?

Yes, in
action_names[x] = "some string";
I would be assigning "some string" to an already constructed string.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:382:
std::ostringstream modify_request_cookie;
On 2013/04/25 19:59:21, Matt Perry wrote:
> Don't use streams:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams
> 
> Just use std::string and concat to it.

Done.

Using JoinString instead of plain +=, because const char* literals can only be
added one at a time (the left operand needs always be a std::string for + to
work). That would mean 5 lines instead of the 4 right now :).

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:432:
const std::map<WebRequestAction::Type, const std::string>& names =
On 2013/04/25 19:59:21, Matt Perry wrote:
> typedef?

Done.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.h:76:
STRATEGY_NONE    = 1 << 0,  // Do not check host permissions.
On 2013/04/25 19:59:21, Matt Perry wrote:
> Can you have multiple strategies? I don't see how...

Not for one action. But during host permissions checking, I had a bitmask
representing a set of strategies.

I rewrote the code in a way that that set is now a singleton, so the bit shifts
are not needed now.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:18:
"To execute the action '*', host permissions for all URLs are needed";
On 2013/04/25 19:59:21, Matt Perry wrote:
> nit: "To execute the action '*', you need to request host permission for all
> URLs."

Done, but should we actually say "for all hosts" instead of "for all URLs"? The
latter is not technically true.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:28:
return base::Bind(&WebRequestRulesChecker::Checker, base::Unretained(this));
On 2013/04/25 19:59:21, Matt Perry wrote:
> Making this a class seems like overkill. Checker and the other methods can
just
> be static methods (or globals in this file) if you bind Extension instead of
> |this|. Something like:
> 
> static GetChecker(Extension* ext) {
>   return base::Bind(&Checker, base::Unretained(ext));
> }

Done.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:56:
if ((*action_iter)->host_permissions_strategy() & allowed_strategies)
On 2013/04/25 19:59:21, Matt Perry wrote:
> Oh, I see why you made them bitmasks now. I'd still prefer to keep them
singular
> values, at the expense of making this code a little more verbose, to make it
> clear you can only have one strategy at a time.

Done.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:92:
*error = kActionCannotBeExecuted;
On 2013/04/25 19:59:21, Matt Perry wrote:
> Add the action's name in the error message.

Done.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry_unittest.cc:67:
return base::Time() + base::TimeDelta::FromDays(2);
The installation times are now set during SetUp when adding the extensions to
the extension info map.

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/permissions_updater_unittest.cc (right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/permissions_updater_unittest.cc:101: #if
defined(OS_WIN)
On 2013/04/25 19:59:21, Matt Perry wrote:
> Just use FilePath::AsUTF8Unsafe()

Done.

https://codereview.chromium.org/14358004/diff/91001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/declarative/api/manifest.json (right):

https://codereview.chromium.org/14358004/diff/91001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/declarative/api/manifest.json:10:
"declarativeWebRequest", "<all_urls>"
This is necessary for registering the Redirect rule in background.js. This
browser test does not care about checking permissions, only the rules loading,
so that should be OK.

[Matt Perry, 2013-04-26 20:37:44]

lgtm

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:18:
"To execute the action '*', host permissions for all URLs are needed";
On 2013/04/26 09:58:52, vabr (Chromium) wrote:
> On 2013/04/25 19:59:21, Matt Perry wrote:
> > nit: "To execute the action '*', you need to request host permission for all
> > URLs."
> 
> Done, but should we actually say "for all hosts" instead of "for all URLs"?
The
> latter is not technically true.

OK, how about "To execute the action '*', you need to request permission for all
hosts."

[vabr (Chromium), 2013-04-26 23:47:21]

Thanks Matt, for your patience with this review! :)

Your last comment was straightforward to implement, so I'm sending to CQ now.

Vaclav

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc
(right):

https://codereview.chromium.org/14358004/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_checker.cc:18:
"To execute the action '*', host permissions for all URLs are needed";
On 2013/04/26 20:37:44, Matt Perry wrote:
> On 2013/04/26 09:58:52, vabr (Chromium) wrote:
> > On 2013/04/25 19:59:21, Matt Perry wrote:
> > > nit: "To execute the action '*', you need to request host permission for
all
> > > URLs."
> > 
> > Done, but should we actually say "for all hosts" instead of "for all URLs"?
> The
> > latter is not technically true.
> 
> OK, how about "To execute the action '*', you need to request permission for
all
> hosts."

SGTM, done.

[commit-bot: I haz the power, 2013-04-26 23:47:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/14358004/73002

[commit-bot: I haz the power, 2013-04-27 00:02:35]

Retried try job too often on linux_chromeos for step(s) ash_unittests,
aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests,
check_deps, chromeos_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, dbus_unittests, device_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-27 06:57:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/14358004/73002

[commit-bot: I haz the power, 2013-04-27 07:44:27]

Change committed as 196959