Integration test for protecting clients*.google.com

Integration test for protecting clients*.google.com

BUG=715184
Review-Url: https://codereview.chromium.org/2876653003
Cr-Commit-Position: refs/heads/master@{#472141}
Committed: https://chromium.googlesource.com/chromium/src/+/d6a0a2e456e9f9e938d72c80a681ea701bdec8ec

[battre, 2017-05-11 16:41:44]

The CQ bit was checked by battre@chromium.org to run a CQ dry run

[battre, 2017-05-11 16:42:25]

Description was changed from

==========
Integration test for protecting clients*.google.com

BUG=715184
==========

to

==========
Integration test for protecting clients*.google.com

BUG=715184
==========

[battre, 2017-05-11 16:42:25]

battre@chromium.org changed reviewers:
+ mmenke@chromium.org, rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-05-11 16:42:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[battre, 2017-05-11 16:42:59]

Here is the requested integration test.

Thanks a lot for reviewing!

Best regards,
Dominic

[commit-bot: I haz the power, 2017-05-11 17:30:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 17:30:14]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[mmenke, 2017-05-11 18:39:46]

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
If the requests are failing due to CORS, does that mean we're actually observing
CORS requests, not the actual network requests?  If so, do we even send CORS
requests for chrome URLs?  If not, I'm very confused.

[mmenke, 2017-05-15 15:10:43]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 15:11:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 15:58:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 15:58:43]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Devlin, 2017-05-15 16:44:21]

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:679: int
expected_requests_observed = 0;
Maybe check the initial value, just for completeness?

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:704:
ASSERT_TRUE(runner);
Used?

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
On 2017/05/11 18:39:45, mmenke wrote:
> If the requests are failing due to CORS, does that mean we're actually
observing
> CORS requests, not the actual network requests?  If so, do we even send CORS
> requests for chrome URLs?  If not, I'm very confused.

We actually notify the extension *before* the request is sent, so I think this
is okay.  But it might be worth a comment.

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:728: class
TestURLFetcherDelegate : public net::URLFetcherDelegate {
I don't know if we have a specific rule about classes with logic being defined
inline, but it's rare enough in our codebase (i.e., I've never seen it) that I
think it breaks the "match surrounding code style" rule.  Can we define this
outside the test?

https://codereview.chromium.org/2876653003/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webrequest_clients_google_com/background.js
(right):

https://codereview.chromium.org/2876653003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webrequest_clients_google_com/background.js:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
not 2016 :)

[mmenke, 2017-05-15 16:47:27]

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> On 2017/05/11 18:39:45, mmenke wrote:
> > If the requests are failing due to CORS, does that mean we're actually
> observing
> > CORS requests, not the actual network requests?  If so, do we even send CORS
> > requests for chrome URLs?  If not, I'm very confused.
> 
> We actually notify the extension *before* the request is sent, so I think this
> is okay.  But it might be worth a comment.

CORS blocks requests from even reaching the network stack, which is where the
WebRequest API is hooked up.  So I'm not sure that resolves my concern.

[Devlin, 2017-05-15 17:03:35]

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
On 2017/05/15 16:47:26, mmenke wrote:
> On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> > On 2017/05/11 18:39:45, mmenke wrote:
> > > If the requests are failing due to CORS, does that mean we're actually
> > observing
> > > CORS requests, not the actual network requests?  If so, do we even send
CORS
> > > requests for chrome URLs?  If not, I'm very confused.
> > 
> > We actually notify the extension *before* the request is sent, so I think
this
> > is okay.  But it might be worth a comment.
> 
> CORS blocks requests from even reaching the network stack, which is where the
> WebRequest API is hooked up.  So I'm not sure that resolves my concern.

Hmm, interesting... Why does this succeed in intercepting the request from
example.com (which is also cross-origin)?

[mmenke, 2017-05-15 17:09:51]

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
On 2017/05/15 17:03:35, Devlin (catching up) wrote:
> On 2017/05/15 16:47:26, mmenke wrote:
> > On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> > > On 2017/05/11 18:39:45, mmenke wrote:
> > > > If the requests are failing due to CORS, does that mean we're actually
> > > observing
> > > > CORS requests, not the actual network requests?  If so, do we even send
> CORS
> > > > requests for chrome URLs?  If not, I'm very confused.
> > > 
> > > We actually notify the extension *before* the request is sent, so I think
> this
> > > is okay.  But it might be worth a comment.
> > 
> > CORS blocks requests from even reaching the network stack, which is where
the
> > WebRequest API is hooked up.  So I'm not sure that resolves my concern.
> 
> Hmm, interesting... Why does this succeed in intercepting the request from
> http://example.com (which is also cross-origin)?

My guess is that it intercepts the CORS request.  I have not confirmed that.

[battre, 2017-05-15 17:32:56]

The CQ bit was checked by battre@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 17:33:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[battre, 2017-05-15 17:39:03]

The CQ bit was checked by battre@chromium.org to run a CQ dry run

[battre (please use the other), 2017-05-15 17:39:21]

battre@google.com changed reviewers:
+ battre@google.com

[battre (please use the other), 2017-05-15 17:39:22]

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:679: int
expected_requests_observed = 0;
On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> Maybe check the initial value, just for completeness?

Done.

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:704:
ASSERT_TRUE(runner);
On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> Used?

Done.

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
On 2017/05/15 17:09:51, mmenke wrote:
> On 2017/05/15 17:03:35, Devlin (catching up) wrote:
> > On 2017/05/15 16:47:26, mmenke wrote:
> > > On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> > > > On 2017/05/11 18:39:45, mmenke wrote:
> > > > > If the requests are failing due to CORS, does that mean we're actually
> > > > observing
> > > > > CORS requests, not the actual network requests?  If so, do we even
send
> > CORS
> > > > > requests for chrome URLs?  If not, I'm very confused.
> > > > 
> > > > We actually notify the extension *before* the request is sent, so I
think
> > this
> > > > is okay.  But it might be worth a comment.
> > > 
> > > CORS blocks requests from even reaching the network stack, which is where
> the
> > > WebRequest API is hooked up.  So I'm not sure that resolves my concern.
> > 
> > Hmm, interesting... Why does this succeed in intercepting the request from
> > http://example.com (which is also cross-origin)?
> 
> My guess is that it intercepts the CORS request.  I have not confirmed that.

My understanding is that Chrome sends the request but delivers it only if the
response contains an Access-Control-Allow-Origin header.

Here is what the console says:
XMLHttpRequest cannot load https://clients1.google.com/. No
'Access-Control-Allow-Origin' header is present on the requested resource.
Origin 'https://developer.mozilla.org' is therefore not allowed access. The
response had HTTP status code 404.

In this test we don't really care. What gets blocked (and counted) is whether
the GET is sent to the net.

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:728: class
TestURLFetcherDelegate : public net::URLFetcherDelegate {
On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> I don't know if we have a specific rule about classes with logic being defined
> inline, but it's rare enough in our codebase (i.e., I've never seen it) that I
> think it breaks the "match surrounding code style" rule.  Can we define this
> outside the test?

I actually copied this from code in Chrome. I like this here because it keeps
the context. If you don't mind, I'd prefer keeping it inline. But you are the
owner and can choose. :-)

[commit-bot: I haz the power, 2017-05-15 17:39:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-05-15 18:31:16]

LGTM

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:717:
EXPECT_FALSE(success);
On 2017/05/15 17:39:22, battre (please use the other) wrote:
> On 2017/05/15 17:09:51, mmenke wrote:
> > On 2017/05/15 17:03:35, Devlin (catching up) wrote:
> > > On 2017/05/15 16:47:26, mmenke wrote:
> > > > On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> > > > > On 2017/05/11 18:39:45, mmenke wrote:
> > > > > > If the requests are failing due to CORS, does that mean we're
actually
> > > > > observing
> > > > > > CORS requests, not the actual network requests?  If so, do we even
> send
> > > CORS
> > > > > > requests for chrome URLs?  If not, I'm very confused.
> > > > > 
> > > > > We actually notify the extension *before* the request is sent, so I
> think
> > > this
> > > > > is okay.  But it might be worth a comment.
> > > > 
> > > > CORS blocks requests from even reaching the network stack, which is
where
> > the
> > > > WebRequest API is hooked up.  So I'm not sure that resolves my concern.
> > > 
> > > Hmm, interesting... Why does this succeed in intercepting the request from
> > > http://example.com (which is also cross-origin)?
> > 
> > My guess is that it intercepts the CORS request.  I have not confirmed that.
> 
> My understanding is that Chrome sends the request but delivers it only if the
> response contains an Access-Control-Allow-Origin header.
> 
> Here is what the console says:
> XMLHttpRequest cannot load https://clients1.google.com/. No
> 'Access-Control-Allow-Origin' header is present on the requested resource.
> Origin 'https://developer.mozilla.org' is therefore not allowed access. The
> response had HTTP status code 404.
> 
> In this test we don't really care. What gets blocked (and counted) is whether
> the GET is sent to the net.

I was concenred that we wouldn't be making a request at all the in the settings
case, meaning this test wouldn't be checking anything.  After some
experimentation, looks like my concerns were unfounded.

[Devlin, 2017-05-15 18:40:39]

lgtm; thanks for adding this test!

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2876653003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:728: class
TestURLFetcherDelegate : public net::URLFetcherDelegate {
On 2017/05/15 17:39:22, battre (please use the other) wrote:
> On 2017/05/15 16:44:21, Devlin (catching up) wrote:
> > I don't know if we have a specific rule about classes with logic being
defined
> > inline, but it's rare enough in our codebase (i.e., I've never seen it) that
I
> > think it breaks the "match surrounding code style" rule.  Can we define this
> > outside the test?
> 
> I actually copied this from code in Chrome. I like this here because it keeps
> the context. If you don't mind, I'd prefer keeping it inline. But you are the
> owner and can choose. :-)

Heh fair enough.  I have a TODO around here somewhere to simplify these (I've
seen other places using this type of thing), so hopefully moot soon enough
anyway.

[commit-bot: I haz the power, 2017-05-15 19:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 19:04:44]

Dry run: This issue passed the CQ dry run.

[battre (please use the other), 2017-05-16 01:16:59]

The CQ bit was checked by battre@google.com

[commit-bot: I haz the power, 2017-05-16 01:18:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 01:45:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 01:46:00]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[battre, 2017-05-16 03:01:40]

Merge with ToT to resolve merge conflict

[battre (please use the other), 2017-05-16 03:02:12]

The CQ bit was checked by battre@google.com

[battre (please use the other), 2017-05-16 03:02:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2876653003/#ps60001
(title: "Merge with ToT to resolve merge conflict")

[battre (please use the other), 2017-05-16 03:02:13]

The CQ bit was checked by battre@google.com

[battre (please use the other), 2017-05-16 03:02:13]

The CQ bit was unchecked by battre@google.com

[battre (please use the other), 2017-05-16 03:02:31]

The CQ bit was checked by battre@google.com

[commit-bot: I haz the power, 2017-05-16 03:02:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 04:14:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 04:14:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[battre (please use the other), 2017-05-16 14:32:04]

The CQ bit was checked by battre@google.com

[commit-bot: I haz the power, 2017-05-16 14:32:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 17:13:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494945124691950,
"parent_rev": "8ea3a3dbe59eceb00d1a234767c74278f5bb03fd", "commit_rev":
"d6a0a2e456e9f9e938d72c80a681ea701bdec8ec"}

[commit-bot: I haz the power, 2017-05-16 17:14:05]

Description was changed from

==========
Integration test for protecting clients*.google.com

BUG=715184
==========

to

==========
Integration test for protecting clients*.google.com

BUG=715184

Review-Url: https://codereview.chromium.org/2876653003
Cr-Commit-Position: refs/heads/master@{#472141}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6a0a2e456e9f9e938d72c80a681...
==========

[commit-bot: I haz the power, 2017-05-16 17:14:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d6a0a2e456e9f9e938d72c80a681...