Adding thirdParty property to RequestMatcher

Adding thirdParty property to RequestMatcher

Including tests.
Also, corrected alphabetical order of keys.

BUG=135606
TEST=Install both extensions attached to BUG 135606 (but not both at the same time). If this feature is working correctly the extensions should block 3rd party requests on all pages. An example page to test this is http://www.corp.google.com/~vabr/no_crawl/third_party.html, where the first image is 1st-party, and the second is 3rd party.
TBR=yoz@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161861

[vabr (Chromium), 2012-09-26 15:44:46]

Hi Dominic,
Can you please take a look at this?
No need to hurry, I have now week off.
Once you are happy, I will ask Jochen and Adam about the use of
first_party_for_cookies.

Thanks,
Vaclav

http://codereview.chromium.org/10982044/diff/1/chrome/browser/extensions/api/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10982044/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:606:
int WebRequestConditionAttributeThirdParty::GetStages() const {
Do we want to restrict the stages?

http://codereview.chromium.org/10982044/diff/1/chrome/browser/extensions/api/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.cc
(right):

http://codereview.chromium.org/10982044/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.cc:38:
const char kThirdPartyKey[] = "thirdParty";
This is the only added key, others are just re-ordered.

http://codereview.chromium.org/10982044/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.h
(right):

http://codereview.chromium.org/10982044/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_constants.h:41:
extern const char kThirdPartyKey[];
This is the only added key, others are just re-ordered.

[battre, 2012-09-26 17:18:54]

LGTM, please check with Jochen and Adam. I think they were fine if we abused
first_party_for_cookies if we don't do it for security. Adam did not like it
when I added a top_frame_url field to URLRequests.

Best regards,
Dominic

[vabr (Chromium), 2012-09-26 19:37:30]

Thanks, Dominic!

Jochen, Adam,

Could you please check my use of URLRequest::first_party_for_cookies() in
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc,
around line 619?

My intention was to use it for a "thirdParty" flag at Declarative WebRequest's
RequestMatcher, i.e., as information for extensions using Declarative WebRequest
rules to set-up request redirects, cancellations, etc. Except for exposing to
extensions, the information is not used otherwise inside Chrome.

Thanks,
Vaclav

P.S. I apologise in advance for delayed responses on this, as I am OOO until
next Thursday. This also means that you don't have to rush with an answer. :)

[jochen (gone - plz use gerrit), 2012-09-26 19:50:19]

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/browser/exte...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:617:
DCHECK(request_data.request->first_party_for_cookies().is_valid());
first_party_for_cookies() is not necessarily valid

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:619:
request_data.request->first_party_for_cookies().GetOrigin();
why are you looking at the origin only?

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/browser/exte...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:622:
first_party_origin == request_data.request->url().GetOrigin();
You should use
RegistryControlledDomainService::SameDomainOrHost(request_data.request->url(),
request_data.request->first_party_for_cookies()) instead

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/common/exten...
File chrome/common/extensions/api/declarative_web_request.json (right):

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/common/exten...
chrome/common/extensions/api/declarative_web_request.json:103: "thirdParty": {
i don't really like this name. It should be thirdPartyForCookies

https://chromiumcodereview.appspot.com/10982044/diff/1001/chrome/common/exten...
chrome/common/extensions/api/declarative_web_request.json:106: "description":
"If set to true, matches requests with third-party origins. If set to false,
matches requests with first-party origins."
Should be something like "If set to true, matches requests that are subject to
third-party cookie policies. If set to false, matches all other requests."

[vabr (Chromium), 2012-10-04 11:20:41]

Thanks, Jochen,
I addressed your comments, PTAL.

Dominic,
Are you fine with renaming the filter to thirdPartyForCookies?

Thanks,
Vaclav

http://codereview.chromium.org/10982044/diff/1001/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10982044/diff/1001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:617:
DCHECK(request_data.request->first_party_for_cookies().is_valid());
On 2012/09/26 19:50:19, jochen wrote:
> first_party_for_cookies() is not necessarily valid

OK, I no longer check it and pass it directly to
RegistryControlledDomainService::SameDomainOrHost.

http://codereview.chromium.org/10982044/diff/1001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:619:
request_data.request->first_party_for_cookies().GetOrigin();
On 2012/09/26 19:50:19, jochen wrote:
> why are you looking at the origin only?

Looking no more, using RegistryControlledDomainService::SameDomainOrHost.

http://codereview.chromium.org/10982044/diff/1001/chrome/browser/extensions/a...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:622:
first_party_origin == request_data.request->url().GetOrigin();
On 2012/09/26 19:50:19, jochen wrote:
> You should use
> RegistryControlledDomainService::SameDomainOrHost(request_data.request->url(),
> request_data.request->first_party_for_cookies()) instead

Done.

http://codereview.chromium.org/10982044/diff/1001/chrome/common/extensions/ap...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10982044/diff/1001/chrome/common/extensions/ap...
chrome/common/extensions/api/declarative_web_request.json:103: "thirdParty": {
On 2012/09/26 19:50:19, jochen wrote:
> i don't really like this name. It should be thirdPartyForCookies

Done.

http://codereview.chromium.org/10982044/diff/1001/chrome/common/extensions/ap...
chrome/common/extensions/api/declarative_web_request.json:106: "description":
"If set to true, matches requests with third-party origins. If set to false,
matches requests with first-party origins."
On 2012/09/26 19:50:19, jochen wrote:
> Should be something like "If set to true, matches requests that are subject to
> third-party cookie policies. If set to false, matches all other requests."

Done.

[battre, 2012-10-05 12:57:32]

yes, renaming this sounds good to me.

http://codereview.chromium.org/10982044/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

http://codereview.chromium.org/10982044/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:59: function
getURLParties() {
Can you add a comment why this is called getURLParties?

[vabr (Chromium), 2012-10-05 16:13:26]

Addressed Dominic's comment.

Dominic and Jochen, PTAL.

http://codereview.chromium.org/10982044/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webrequest/test_declarative.js
(right):

http://codereview.chromium.org/10982044/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webrequest/test_declarative.js:59: function
getURLParties() {
On 2012/10/05 12:57:33, battre wrote:
> Can you add a comment why this is called getURLParties?

Done. On a second thought, the "parties" name sounds a bit silly, so I renamed
that file and this helper function.

[jochen (gone - plz use gerrit), 2012-10-08 07:30:35]

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:618:
const bool is_first_party =
if the first_party_for_cookies().is_empty(), it's also a first party.

you could also do something like

is_first_party =
(net::StaticCookiePolicy(BLOCK_ALL_THIRD_PARTY_COOKIES).CanGetCookies(url,
first_party_for_cookies) == net::OK)

then you don't have to copy the logic

[battre, 2012-10-15 07:19:51]

Still LGTM after addressing Jochen's comment.

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:207:
for (size_t i = 0; i < arraysize(request_stages); ++i) {
maybe you want to add a test case for empty first party for cookies url as per
Jochen's comment.

[vabr (Chromium), 2012-10-15 12:01:43]

Jochen,
Thanks for your suggestion.
PTAL.

Thanks all,
Vaclav

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute.cc:618:
const bool is_first_party =
On 2012/10/08 07:30:35, jochen wrote:
> if the first_party_for_cookies().is_empty(), it's also a first party.
> 
> you could also do something like
> 
> is_first_party =
> (net::StaticCookiePolicy(BLOCK_ALL_THIRD_PARTY_COOKIES).CanGetCookies(url,
> first_party_for_cookies) == net::OK)
> 
> then you don't have to copy the logic

Done.

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
File
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc
(right):

http://codereview.chromium.org/10982044/diff/23004/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_condition_attribute_unittest.cc:207:
for (size_t i = 0; i < arraysize(request_stages); ++i) {
On 2012/10/15 07:19:51, battre wrote:
> maybe you want to add a test case for empty first party for cookies url as per
> Jochen's comment.

Done.

[jochen (gone - plz use gerrit), 2012-10-15 13:08:52]

3rd party cookie check lgtm

[commit-bot: I haz the power, 2012-10-15 13:52:09]

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

[commit-bot: I haz the power, 2012-10-15 13:52:16]

Presubmit check for 10982044-35002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api

Presubmit checks took 2.6s to calculate.

[vabr (Chromium), 2012-10-15 13:53:49]

Adding Yoyo to TBR for chrome/common/extensions/api/declarative_web_request.json
OWNERS rubber stamp.

[commit-bot: I haz the power, 2012-10-15 13:54:05]

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

[commit-bot: I haz the power, 2012-10-15 15:54:17]

Change committed as 161861