Do not pass URLs in onUpdated events to extensions unless they have the

Do not pass URLs in onUpdated events to extensions unless they have the
"tabs" permission.

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

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

[not at google - send to devlin, 2013-01-08 21:25:09]

is there someplace to put a test?

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:407:
ExtensionTabUtil::ScrubTabValue(contents, extension, changed_properties);
since DispatchTabUpdatedEvent is taking ownership of changed_properties it seems
like we can save a bit of work and take ownership here, and avoid copying the
value. 

in which case, take a scoped_ptr<DictionaryValue> for changed_properties

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:417: WebContents* contents,
DictionaryValue* changed_properties) {
cleanup: make this take a scoped_ptr<DictionaryValue> to indicate ownership

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:444:
base::Bind(&WillDispatchTabUpdatedEvent, contents, changed_properties);
changed_properties will have been deleted by the time the callback runs;
changing WillDispatchTabUpdatedEvent is to take a scoped_ptr might work, I can't
remember, maybe you need to use base::Owned in bind_helpers.h

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_tab_util.cc:140: DictionaryValue*
scrubbed_info = update_info->DeepCopy();
it doesn't seem necessary to return a copy here; we could just modify
update_info in-place and make callers DeepCopy it if they want.

also, it's not necessarily update_info, so call tab_info?

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_tab_util.cc:153: }
It would be nice to avoid the duplication here and in CreateTabValue. Could you
remove the permissions check in there (always set the field), then scrub them
out in the CreateTabValue caller?

[mvrable, 2013-01-09 01:46:59]

Thanks for the feedback; I've made some changes in response to your comments.

Let me know how this version of the patch looks.  One possible concern is that,
in moving all scrubbing code to one place and out of CreateTabList, there's no
longer an extra argument of INCLUDE_PRIVACY_SENSITIVE_FIELDS that gets set when
CreateTabList is called from a few other places in the code.  There's no
functional change to these, though the old code had the effect of making it
obvious to readers that the created value might include privacy-sensitive data,
and the new one does not.  Is it worth making further changes?

--Michael Vrable

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:407:
ExtensionTabUtil::ScrubTabValue(contents, extension, changed_properties);
On 2013/01/08 21:25:09, kalman wrote:
> since DispatchTabUpdatedEvent is taking ownership of changed_properties it
seems
> like we can save a bit of work and take ownership here, and avoid copying the
> value. 
> 
> in which case, take a scoped_ptr<DictionaryValue> for changed_properties

I believe the way this callback works is that it runs once before an event is
sent to an extension.  If there are multiple extensions listening for the event,
the callback will run multiple times--so we can't free changed_properties here.

We could potentially reduce memory allocations by having a sanitized and
non-sanitized version of changed_properties and selecting the right one at each
step, but that adds extra complications and probably isn't worth it (especially
as tab_value is already being recreated on each call to
WillDispatchTabUpdatedEvent anyway).

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:417: WebContents* contents,
DictionaryValue* changed_properties) {
On 2013/01/08 21:25:09, kalman wrote:
> cleanup: make this take a scoped_ptr<DictionaryValue> to indicate ownership

Done.

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/bro...
chrome/browser/extensions/browser_event_router.cc:444:
base::Bind(&WillDispatchTabUpdatedEvent, contents, changed_properties);
On 2013/01/08 21:25:09, kalman wrote:
> changed_properties will have been deleted by the time the callback runs;
> changing WillDispatchTabUpdatedEvent is to take a scoped_ptr might work, I
can't
> remember, maybe you need to use base::Owned in bind_helpers.h

I believe that the callback is run from within BroadcastEvent below, and thus
will fall within the lifetime of the changed_properties object.

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_tab_util.cc:140: DictionaryValue*
scrubbed_info = update_info->DeepCopy();
On 2013/01/08 21:25:09, kalman wrote:
> it doesn't seem necessary to return a copy here; we could just modify
> update_info in-place and make callers DeepCopy it if they want.
> 
> also, it's not necessarily update_info, so call tab_info?

Done.

https://codereview.chromium.org/11824004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_tab_util.cc:153: }
On 2013/01/08 21:25:09, kalman wrote:
> It would be nice to avoid the duplication here and in CreateTabValue. Could
you
> remove the permissions check in there (always set the field), then scrub them
> out in the CreateTabValue caller?

Done, I think: do these changes look reasonable?

[not at google - send to devlin, 2013-01-09 02:04:29]

lgtm, what you have looks good, I think removing the enum is fine.

If you can find an easy way to add a regression test that would be nice too, but
browser_event_router.cc doesn't look like it has unit tests. Not a yak worth
shaving.

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:351: if
(changed_properties.get())
scoped_ptr works in conditionals without the .get()

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:400: DictionaryValue*
changed_properties,
const&?

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:419:
DCHECK(changed_properties.get());
can omit the .get()

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:143: return tab_info;
returning something here is unnecessary

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.h (right):

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.h:49: // status of a browser tab). 
Depending on the permissions of the extension,
"A Javascript dictionary..." is a bit misleading, though referencing its schema
in tabs.json would be good.

[mvrable, 2013-01-09 19:25:35]

A few more minor updates in response to your comments.  I'm also taking a quick
look at tests now to see if it's easy to make a test for the proper behavior
(but if it's non-trivial, I'll submit this without a test).

--Michael Vrable

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:351: if
(changed_properties.get())
On 2013/01/09 02:04:29, kalman wrote:
> scoped_ptr works in conditionals without the .get()

Done.

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:400: DictionaryValue*
changed_properties,
On 2013/01/09 02:04:29, kalman wrote:
> const&?

Changed to a const*.  I'm getting compiler errors from base::Bind when the
argument is a const&, and a const* is also consistent with the other arguments
(extension is also a pointer, not reference, type).

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:419:
DCHECK(changed_properties.get());
On 2013/01/09 02:04:29, kalman wrote:
> can omit the .get()

Done.

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:143: return tab_info;
On 2013/01/09 02:04:29, kalman wrote:
> returning something here is unnecessary

Done.

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.h (right):

https://codereview.chromium.org/11824004/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.h:49: // status of a browser tab). 
Depending on the permissions of the extension,
On 2013/01/09 02:04:29, kalman wrote:
> "A Javascript dictionary..." is a bit misleading, though referencing its
schema
> in tabs.json would be good.

Done.

[mvrable, 2013-01-09 21:40:12]

I made another update to the code, to add a new test case
(ExtensionApiTest.TabsNoPermissions in browser_tests)--this fails when run
against the original code (the URL is leaked) but passes with the fix.

This is the first extension test I've written, so any feedback on this is
welcome.  If it looks good, does this patch look ready to commit?

--Michael Vrable

[not at google - send to devlin, 2013-01-09 21:54:31]

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json
(right):

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json:7:
"page": "test.html"
use "script": "test.js", then you don't need test.html at all

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:6: var
capturedEventData;
instead of all this global stuff
(variables/onUpdateLIstener/checkExpectations/check), could you add a listener
within the test then remove it on the callback?

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:9:
console.log('---onUpdated: ' + info.status + ', ' + info.url);
remove logs

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:20:
JSON.stringify(capturedEventData));
I think assertEq works with objects?

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:31: function
noOnUpdateUrls() {
i'd call these just testOnUpdated/testQuery.

[mvrable, 2013-01-09 22:22:59]

Updated.

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json
(right):

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/manifest.json:7:
"page": "test.html"
On 2013/01/09 21:54:31, kalman wrote:
> use "script": "test.js", then you don't need test.html at all

Done.

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:6: var
capturedEventData;
On 2013/01/09 21:54:31, kalman wrote:
> instead of all this global stuff
> (variables/onUpdateLIstener/checkExpectations/check), could you add a listener
> within the test then remove it on the callback?

Done.

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:9:
console.log('---onUpdated: ' + info.status + ', ' + info.url);
On 2013/01/09 21:54:31, kalman wrote:
> remove logs

Done.

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:20:
JSON.stringify(capturedEventData));
On 2013/01/09 21:54:31, kalman wrote:
> I think assertEq works with objects?

Done.

https://codereview.chromium.org/11824004/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:31: function
noOnUpdateUrls() {
On 2013/01/09 21:54:31, kalman wrote:
> i'd call these just testOnUpdated/testQuery.

Done.

[not at google - send to devlin, 2013-01-09 22:30:15]

lgtm with those couple of test changes

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:14: }
nit: single-line ifs omit {}

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:21:
chrome.tabs.create({ url: 'chrome://newtab/' });
mm, you should also test changing the URL or title and making sure that property
doesn't get set.

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:28:
chrome.test.assertEq(undefined, tabs[0].url);
you could add the other sensitive properties here too?

['url', 'title', 'faviconUrl'].forEach(...)

[mvrable, 2013-01-09 23:03:14]

A few more updates, plus a couple of JavaScript style fixes.

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:14: }
On 2013/01/09 22:30:15, kalman wrote:
> nit: single-line ifs omit {}

Done.

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:21:
chrome.tabs.create({ url: 'chrome://newtab/' });
On 2013/01/09 22:30:15, kalman wrote:
> mm, you should also test changing the URL or title and making sure that
property
> doesn't get set.

Done.

https://codereview.chromium.org/11824004/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:28:
chrome.test.assertEq(undefined, tabs[0].url);
On 2013/01/09 22:30:15, kalman wrote:
> you could add the other sensitive properties here too?
> 
> ['url', 'title', 'faviconUrl'].forEach(...)

Done (implemented as a separate check function so it can be re-used).

[not at google - send to devlin, 2013-01-09 23:04:38]

lgtm

[commit-bot: I haz the power, 2013-01-10 00:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/11824004/3011

[commit-bot: I haz the power, 2013-01-10 01:09:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-10 04:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/11824004/2015

[commit-bot: I haz the power, 2013-01-10 05:06:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/11824004/9019

[commit-bot: I haz the power, 2013-01-10 09:23:05]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-11 17:49:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/11824004/9019

[commit-bot: I haz the power, 2013-01-11 19:19:28]

Change committed as 176406

[mvrable, 2013-01-11 21:37:04]

Updated patch to fix a test failure.  Does the updated test look reasonable to
you?  (The only change is to the test.js file; a few other files show up as
changed because I rebased the patch against the current head.)

[not at google - send to devlin, 2013-01-11 21:44:41]

lgtm

https://codereview.chromium.org/11824004/diff/25001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/25001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:33:
eventHandled();
nitty comment: I was briefly confused here, this isn't actually an event being
handled, perhaps call the event countDown() or something?

[not at google - send to devlin, 2013-01-11 21:45:22]

https://codereview.chromium.org/11824004/diff/25001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/25001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:33:
eventHandled();
On 2013/01/11 21:44:41, kalman wrote:
> nitty comment: I was briefly confused here, this isn't actually an event being
> handled, perhaps call the event countDown() or something?

s/event/function/

[mvrable, 2013-01-11 21:57:06]

Changed the names and added a missing call to assertNoSensitiveFields(info), so
we check both arguments to the onUpdated event callback.

The test passes with the patch, and fails with the tabs permission present or
against the old code (as expected).

--Michael Vrable

https://codereview.chromium.org/11824004/diff/25001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/tabs/no_permissions/test.js (right):

https://codereview.chromium.org/11824004/diff/25001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tabs/no_permissions/test.js:33:
eventHandled();
On 2013/01/11 21:44:41, kalman wrote:
> nitty comment: I was briefly confused here, this isn't actually an event being
> handled, perhaps call the event countDown() or something?

Done.

[commit-bot: I haz the power, 2013-01-11 22:03:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/11824004/35001

[commit-bot: I haz the power, 2013-01-12 16:54:53]

Change committed as 176546