Hide requests in an extension from other extensions

Hide requests in an extension from other extensions

BUG=510802
Committed: https://crrev.com/86154b409eed6ea7f9177f6bf576e76533ab2f7b
Cr-Commit-Position: refs/heads/master@{#341939}

[robwu, 2015-08-04 11:37:29]

rob@robwu.nl changed reviewers:
+ battre@chromium.org, fsamuel@chromium.org, kalman@chromium.org

[robwu, 2015-08-04 11:37:29]

rob@robwu.nl changed required reviewers:
+ kalman@chromium.org

[robwu, 2015-08-04 11:37:30]

Dominic: Please review my logic.

Ben: Please review perform the OWNER review.

Fady: I have added a TODO in ExtensionWebRequestEventRouter::OnEventHandled.
Could you check whether the TODO is warranted, and if so pick it up? If I'm not
mistaken, the issue was introduced with
https://codereview.chromium.org/864893003.

[Fady Samuel, 2015-08-04 14:40:57]

fsamuel@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Fady Samuel, 2015-08-04 14:40:57]

I no longer work on <webview>. Passing along to Istiaque! Thanks!

[lazyboy, 2015-08-04 15:59:26]

FYI, before I can review, can you provide me access to crbug.com/510802. I don't
have much context w/o looking at the bug.

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js
(right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js:6:
var baseUrl = 'http://example.com:' + config.testServer.port;
nit: Indent 2 spaces in this CL.

[robwu, 2015-08-04 16:18:15]

On 2015/08/04 15:59:26, lazyboy wrote:
> FYI, before I can review, can you provide me access to crbug.com/510802. I
don't
> have much context w/o looking at the bug.

Thanks Istiaque for your input, but you don't need to fully review this CL
(battre@/kalman@ are better suited for that. I only asked Fady because of a
potential flaw that I found while developing this patch (see below).

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1250: // takes the webview
ID into account, which is not set on |listener|.
Fady / Istiaque: This new TODO item is completely unrelated to the patch itself.
I added you to this ticket because I was wondering whether 1) this item is in
fact a real problem, and 2) if it is, whether you'll fix the issue.

In retrospect I should have created a new ticket to avoid confusion, but
hindsight is 20/20.

[not at google - send to devlin, 2015-08-04 20:27:02]

Thanks for doing this.

https://codereview.chromium.org/1267183003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:330:
ExtensionTestMessageListener listener_extension("extension_done", false);
could you standardise on camelcase vs underscores?

https://codereview.chromium.org/1267183003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:337: // Perform
some network activity in an app and another extension.
1. The app and extension do slightly different things. They both fire XHR but
the extension is the one which triggers the navigation which ultimately
completes the test. This way of finishing the test seems arbitrary to me
actually; it's interesting to use the webRequest API to trigger it, but it would
be simpler to use extension messaging to communicate.

2. As I mentioned, it would be good to also have a test for a website that hosts
an extension's (web accessible of course) iframe, since that should bypass the
process ID check.

3. It would be good to test what functionality is supposed to work; intercepting
requests from yourself. It should be pretty straightforward to modify the "main"
extension to do an XHR before continuing with its test assertion.

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js
(right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js:1:
// Copyright (c) 2015 The Chromium Authors. All rights reserved.
(c) is not needed

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js:6:
var baseUrl = 'http://example.com:' + config.testServer.port;
On 2015/08/04 15:59:26, lazyboy wrote:
> nit: Indent 2 spaces in this CL.

yep, here and the JS in the other files as well.

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/extension/background.js
(right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/extension/background.js:19:
chrome.test.sendMessage('extension_done');
good practice to remove listener after this.

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:454: // of find() without
specifying the process ID of the extension.
Huh, funky. Where does this actually make a difference? I *would* rather we be
principled and set the process ID everywhere, unless there is a good reason not
to.

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1483: if
(is_request_from_extension &&
Comment this ("allow requests from the extension"), because it's subtle.

Also note that this won't work for iframes right now, but once site isolation is
in place, it will. I am fine with this behavior, but consider testing that.

[robwu, 2015-08-04 21:57:24]

https://codereview.chromium.org/1267183003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:330:
ExtensionTestMessageListener listener_extension("extension_done", false);
On 2015/08/04 20:27:02, kalman wrote:
> could you standardise on camelcase vs underscores?

Done.

https://codereview.chromium.org/1267183003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:337: // Perform
some network activity in an app and another extension.
On 2015/08/04 20:27:02, kalman wrote:
> 1. The app and extension do slightly different things. They both fire XHR but
> the extension is the one which triggers the navigation which ultimately
> completes the test. This way of finishing the test seems arbitrary to me
> actually; it's interesting to use the webRequest API to trigger it, but it
would
> be simpler to use extension messaging to communicate.

Good point, done.

> 
> 2. As I mentioned, it would be good to also have a test for a website that
hosts
> an extension's (web accessible of course) iframe, since that should bypass the
> process ID check.

Done (with a content script).

> 
> 3. It would be good to test what functionality is supposed to work;
intercepting
> requests from yourself. It should be pretty straightforward to modify the
"main"
> extension to do an XHR before continuing with its test assertion.

This is already covered by other tests (e.g. BackgroundXhrTest.TlsClientAuth and
BackgroundXhrTest.HttpAuth).

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js
(right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js:1:
// Copyright (c) 2015 The Chromium Authors. All rights reserved.
On 2015/08/04 20:27:02, kalman wrote:
> (c) is not needed

Done.

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/app/background.js:6:
var baseUrl = 'http://example.com:' + config.testServer.port;
On 2015/08/04 20:27:02, kalman wrote:
> On 2015/08/04 15:59:26, lazyboy wrote:
> > nit: Indent 2 spaces in this CL.
> 
> yep, here and the JS in the other files as well.

Done.

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/extension/background.js
(right):

https://codereview.chromium.org/1267183003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/extension/background.js:19:
chrome.test.sendMessage('extension_done');
On 2015/08/04 20:27:02, kalman wrote:
> good practice to remove listener after this.

I've removed the tab-load detection from the page, now I'm relying on a message
from the content script.

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:454: // of find() without
specifying the process ID of the extension.
On 2015/08/04 20:27:02, kalman wrote:
> Huh, funky. Where does this actually make a difference? I *would* rather we be
> principled and set the process ID everywhere, unless there is a good reason
not
> to.

WebRequestAPI::OnListenerRemoved doesn't provide any information about the
reason for event removal. I don't want to rely on the process ID for removal, to
avoid getting bit by the following scenario:
1. Extension unloads.
2. OnListenerRemoved is called.
3. Meanwhile the extension process is gone.
4. The browser process tries to look up the process ID for the given extension
ID, but doesn't find any.

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1483: if
(is_request_from_extension &&
On 2015/08/04 20:27:02, kalman wrote:
> Comment this ("allow requests from the extension"), because it's subtle.
> 
> Also note that this won't work for iframes right now, but once site isolation
is
> in place, it will. I am fine with this behavior, but consider testing that.

Done.

[not at google - send to devlin, 2015-08-04 23:12:02]

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

[not at google - send to devlin, 2015-08-04 23:12:03]

lgtm

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:454: // of find() without
specifying the process ID of the extension.
On 2015/08/04 21:57:24, robwu wrote:
> On 2015/08/04 20:27:02, kalman wrote:
> > Huh, funky. Where does this actually make a difference? I *would* rather we
be
> > principled and set the process ID everywhere, unless there is a good reason
> not
> > to.
> 
> WebRequestAPI::OnListenerRemoved doesn't provide any information about the
> reason for event removal. I don't want to rely on the process ID for removal,
to
> avoid getting bit by the following scenario:
> 1. Extension unloads.
> 2. OnListenerRemoved is called.
> 3. Meanwhile the extension process is gone.
> 4. The browser process tries to look up the process ID for the given extension
> ID, but doesn't find any.

Makes sense that you're guarding against a bug here. The way the comment is
phrased makes it sound like an optimisation. Perhaps fix it up a bit.

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:362: 
extra blank line

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:377: //
extension.
+rdcronin

Like I mentioned this will change with site isolation, and this test will
probably start failing for that as well. Devlin could you double check this?

https://codereview.chromium.org/1267183003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/extension/framescript.js
(right):

https://codereview.chromium.org/1267183003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/extension/framescript.js:16:
x.send();
For the record, the ideal test factoring would probably have been to use a
TestExtensionDir from the C++ then manufacture the app, extension, and extension
iframe. This pattern of send XHR, wait for response, check there was response,
notify success is pretty common.

but no point rewriting things for that.

[Devlin, 2015-08-04 23:19:20]

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:377: //
extension.
On 2015/08/04 23:12:03, kalman wrote:
> +rdcronin
> 
> Like I mentioned this will change with site isolation, and this test will
> probably start failing for that as well. Devlin could you double check this?

Yeah, this looks like it will (or at least should) fail. Can we have two
different versions of this check, one for site isolation and one without? 
Should be sufficient just to check the commandline for the presence of the
site-per-process flag.

[robwu, 2015-08-05 12:51:14]

The CQ bit was checked by rob@robwu.nl

[robwu, 2015-08-05 12:51:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1267183003/#ps60001
(title: "Update comment in operator<, remove blank line, OOPIF-friendly test")

[commit-bot: I haz the power, 2015-08-05 12:51:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267183003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267183003/60001

[battre, 2015-08-05 13:49:47]

https://codereview.chromium.org/1267183003/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1485: // content scripts,
or extension pages in non-extension processes.
Also extensions can be hosted in the same process. Would these be filtered?

[robwu, 2015-08-05 16:38:09]

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:454: // of find() without
specifying the process ID of the extension.
On 2015/08/04 23:12:02, kalman wrote:
> On 2015/08/04 21:57:24, robwu wrote:
> > On 2015/08/04 20:27:02, kalman wrote:
> > > Huh, funky. Where does this actually make a difference? I *would* rather
we
> be
> > > principled and set the process ID everywhere, unless there is a good
reason
> > not
> > > to.
> > 
> > WebRequestAPI::OnListenerRemoved doesn't provide any information about the
> > reason for event removal. I don't want to rely on the process ID for
removal,
> to
> > avoid getting bit by the following scenario:
> > 1. Extension unloads.
> > 2. OnListenerRemoved is called.
> > 3. Meanwhile the extension process is gone.
> > 4. The browser process tries to look up the process ID for the given
extension
> > ID, but doesn't find any.
> 
> Makes sense that you're guarding against a bug here. The way the comment is
> phrased makes it sound like an optimisation. Perhaps fix it up a bit.

Done.

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:362: 
On 2015/08/04 23:12:03, kalman wrote:
> extra blank line

Done.

https://codereview.chromium.org/1267183003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:377: //
extension.
On 2015/08/04 23:19:20, Devlin wrote:
> On 2015/08/04 23:12:03, kalman wrote:
> > +rdcronin
> > 
> > Like I mentioned this will change with site isolation, and this test will
> > probably start failing for that as well. Devlin could you double check this?
> 
> Yeah, this looks like it will (or at least should) fail. Can we have two
> different versions of this check, one for site isolation and one without? 
> Should be sufficient just to check the commandline for the presence of the
> site-per-process flag.

Done (and ran the test locally, with and without --site-per-process flag).

https://codereview.chromium.org/1267183003/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/webrequest_extensions/extension/framescript.js
(right):

https://codereview.chromium.org/1267183003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest_extensions/extension/framescript.js:16:
x.send();
On 2015/08/04 23:12:03, kalman wrote:
> For the record, the ideal test factoring would probably have been to use a
> TestExtensionDir from the C++ then manufacture the app, extension, and
extension
> iframe. This pattern of send XHR, wait for response, check there was response,
> notify success is pretty common.
> 
> but no point rewriting things for that.

Good to know for the future. I'll keep the current CL as is though.

https://codereview.chromium.org/1267183003/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1485: // content scripts,
or extension pages in non-extension processes.
On 2015/08/05 13:49:46, battre wrote:
> Also extensions can be hosted in the same process. Would these be filtered?

You mean another extension within some webRequest extension?
With site isolation: Yes.
Without site isolation: No, but the frame cannot be embedded anyway unless the
page is in web_accessible_resources.

[not at google - send to devlin, 2015-08-05 16:49:59]

https://codereview.chromium.org/1267183003/diff/60001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/1267183003/diff/60001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1485: // content scripts,
or extension pages in non-extension processes.
On 2015/08/05 16:38:09, robwu wrote:
> On 2015/08/05 13:49:46, battre wrote:
> > Also extensions can be hosted in the same process. Would these be filtered?
> 
> You mean another extension within some webRequest extension?
> With site isolation: Yes.
> Without site isolation: No, but the frame cannot be embedded anyway unless the
> page is in web_accessible_resources.

I think the question is can extensions share a render process host ID?

I had always assumed that an RPH is an abstraction of a process, so the process
manager infrastructure can choose to host two RPHs in the same OS process but
that's an implementation detail.

However come to think of it I've never actually asked anybody that question.

[commit-bot: I haz the power, 2015-08-05 18:53:36]

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

[commit-bot: I haz the power, 2015-08-05 18:53:36]

Try jobs failed on following builders:
  mac_chromium_10.10_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.1...)

[robwu, 2015-08-05 18:55:40]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-08-05 18:57:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267183003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267183003/60001

[commit-bot: I haz the power, 2015-08-05 19:30:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-05 19:30:52]

Patchset 4 (id:??) landed as
https://crrev.com/86154b409eed6ea7f9177f6bf576e76533ab2f7b
Cr-Commit-Position: refs/heads/master@{#341939}