Don't kill Chrome Apps that make XHRs from guests.

Don't kill Chrome Apps that make XHRs from guests.

It's possible for the app to make CORS requests from its guests without
any webview-accessible-resources, via injected content scripts.

BUG=613335
TEST=See bug comment 37 for a repro app.

Review-Url: https://codereview.chromium.org/2803963002
Cr-Commit-Position: refs/heads/master@{#462704}
Committed: https://chromium.googlesource.com/chromium/src/+/1d90c42584511fa8bf91f0eadddfe1634f9523fe

[Charlie Reis, 2017-04-06 08:06:57]

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

[commit-bot: I haz the power, 2017-04-06 08:07:20]

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-04-06 09:19:20]

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

[commit-bot: I haz the power, 2017-04-06 09:19:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Charlie Reis, 2017-04-06 16:18:17]

creis@chromium.org changed reviewers:
+ nick@chromium.org, rdevlin.cronin@chromium.org

[Charlie Reis, 2017-04-06 16:18:17]

Nick and Devlin, can you take a look?  Now that we've confirmed that apps don't
need webview-accessible-resources to make XHRs from their guests (thanks to
content scripts), we should be checking for the webview permission instead of
webview-accessible-resources.

For the test, I borrowed from Nick's repro app as well as two tests in
web_view_browsertest.cc: Shim_TestAddContentScript and
GeolocationAPIEmbedderHasNoAccessAllow (allowing me to use TestHelper outside
the larger shim test, which does have webview-accessible-resources).

[ncarter (slow), 2017-04-06 17:16:44]

nice work

lgtm with one question

https://codereview.chromium.org/2803963002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:213:
extensions::APIPermission::kWebView))
Do we actually need this check, or would just checking the GetOwnerInfo suffice?

(I'm fine with either way, mostly just curious if there's a meaningful
difference)

[Charlie Reis, 2017-04-06 18:15:46]

Thanks!  Devlin, can you take a look for owners approval?

https://codereview.chromium.org/2803963002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:213:
extensions::APIPermission::kWebView))
On 2017/04/06 17:16:44, ncarter wrote:
> Do we actually need this check, or would just checking the GetOwnerInfo
suffice?
> 
> (I'm fine with either way, mostly just curious if there's a meaningful
> difference)

In theory, the check below that you mention would be sufficient without also
checking for the webview permission.  I think it's probably reasonable to
enforce here that an app shouldn't have a webview without that permission,
though, so I'd like to keep it for now.

[Devlin, 2017-04-06 18:32:12]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Devlin, 2017-04-06 18:32:13]

+lazyboy knows the webview test framework better than I do, so he may wanna take
a quick look.  But otherwise, lgtm.

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/content_script.js
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/content_script.js:18:
case 'start-fetch': {
A switch for a single case is a little weird.  Can this just be an if?  And
should we fail if it's anything else, or are other messages expected?

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js:35:
chrome.test.log('Guest url is: ' + embedder.guestURL);
Some of these logs should probably be removed.

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js:79:
'testContentScriptFetch': testContentScriptFetch
no quotes around keys.

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/guest.html
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/guest.html:3:
* Copyright (c) 2017 The Chromium Authors. All rights reserved.  Use of this
no (c)

[Charlie Reis, 2017-04-06 19:21:59]

Thanks!  lazyboy@, any thoughts before I land?

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/content_script.js
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/content_script.js:18:
case 'start-fetch': {
On 2017/04/06 18:32:13, Devlin wrote:
> A switch for a single case is a little weird.  Can this just be an if?  And
> should we fail if it's anything else, or are other messages expected?

Done.

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js:35:
chrome.test.log('Guest url is: ' + embedder.guestURL);
On 2017/04/06 18:32:13, Devlin wrote:
> Some of these logs should probably be removed.

Sure, I removed some from here and content_script.js, and left the ones that
might help diagnose failures.

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js:79:
'testContentScriptFetch': testContentScriptFetch
On 2017/04/06 18:32:13, Devlin wrote:
> no quotes around keys.

Done.

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/guest.html
(right):

https://codereview.chromium.org/2803963002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/guest.html:3:
* Copyright (c) 2017 The Chromium Authors. All rights reserved.  Use of this
On 2017/04/06 18:32:13, Devlin wrote:
> no (c)

Done.

[Charlie Reis, 2017-04-06 19:22:08]

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

[commit-bot: I haz the power, 2017-04-06 19:22:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2017-04-06 19:28:09]

Drive-by nit

https://codereview.chromium.org/2803963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2803963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:205:
// itself, or by one if its guests if there are accessible_resources.
update this comment?

[Charlie Reis, 2017-04-06 20:20:47]

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

[Charlie Reis, 2017-04-06 20:21:03]

https://codereview.chromium.org/2803963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2803963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:205:
// itself, or by one if its guests if there are accessible_resources.
On 2017/04/06 19:28:09, alexmos (OOO on 4-7) wrote:
> update this comment?

Thanks!  Done.

[commit-bot: I haz the power, 2017-04-06 20:22:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-04-06 21:03:29]

lgtm with tiny nits.

https://codereview.chromium.org/2803963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2803963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:213:
extensions::APIPermission::kWebView))
nit: {}

https://codereview.chromium.org/2803963002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js
(right):

https://codereview.chromium.org/2803963002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js:66:
if (data == "fetch-success") {
nit: single quotes

[Charlie Reis, 2017-04-06 21:09:15]

Thanks!

https://codereview.chromium.org/2803963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2803963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:213:
extensions::APIPermission::kWebView))
On 2017/04/06 21:03:29, lazyboy wrote:
> nit: {}

Sure.  (Not strictly necessary since the body is still one line, but I agree
that it makes things easier to read here due to the various other indents
nearby.)

https://codereview.chromium.org/2803963002/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js
(right):

https://codereview.chromium.org/2803963002/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/content_script_fetch/embedder.js:66:
if (data == "fetch-success") {
On 2017/04/06 21:03:29, lazyboy wrote:
> nit: single quotes

Done.

[Charlie Reis, 2017-04-06 21:09:22]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2017-04-06 21:09:22]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
rdevlin.cronin@chromium.org, lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2803963002/#ps60001
(title: "Fix nits.")

[commit-bot: I haz the power, 2017-04-06 21:11:03]

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-04-07 00:21:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491512962287490,
"parent_rev": "99cc887ad000b29513c3d2613efa46f26199e504", "commit_rev":
"1d90c42584511fa8bf91f0eadddfe1634f9523fe"}

[commit-bot: I haz the power, 2017-04-07 00:23:04]

Description was changed from

==========
Don't kill Chrome Apps that make XHRs from guests.

It's possible for the app to make CORS requests from its guests without
any webview-accessible-resources, via injected content scripts.

BUG=613335
TEST=See bug comment 37 for a repro app.
==========

to

==========
Don't kill Chrome Apps that make XHRs from guests.

It's possible for the app to make CORS requests from its guests without
any webview-accessible-resources, via injected content scripts.

BUG=613335
TEST=See bug comment 37 for a repro app.

Review-Url: https://codereview.chromium.org/2803963002
Cr-Commit-Position: refs/heads/master@{#462704}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d90c42584511fa8bf91f0eadddf...
==========

[commit-bot: I haz the power, 2017-04-07 00:23:05]

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