Tabs API is usable without tabs permission.

Tabs API is usable without tabs permission.

All of the tabs api methods are now usable without needing to specify "tabs"
permission in the manifest. However privacy sensitive data is stripped from the
Tab object that is returned unless the permission is present.
The privacy sensitive data includes:
url,
title,
faviconURL

BUG=137404

[chebert, 2012-08-05 19:05:02]

Currently, it doesn't implement the console warnings yet.

[Matt Tytel, 2012-08-06 18:45:23]

https://chromiumcodereview.appspot.com/10829186/diff/3008/chrome/common/exten...
File chrome/common/extensions/permissions/permission_set.cc (left):

https://chromiumcodereview.appspot.com/10829186/diff/3008/chrome/common/exten...
chrome/common/extensions/permissions/permission_set.cc:60:
"management.getPermissionWarningsByManifest",
Ignorant Drive-by comment: Should executeScript should require permission since
they'll have access to the tab data? Or is that covered in the "http://*/*"
permissions?

Maybe insertCSS too for tab editing reasons.

[chebert, 2012-08-07 19:43:20]

Aaron,

I'm not sure how to incorporate console warnings into this set up.

Also, let me know your thoughts on Matt's point about executeScript and
insertCSS potentially being a security issue.

Thanks

[Aaron Boodman, 2012-08-10 06:44:51]

Yeah, executeScript() already requires host permissions.

http://codereview.chromium.org/10829186/diff/13001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/10829186/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:822: DictionaryValue* result =
ExtensionTabUtil::CreateTabValue(
Can we pass Extension* into CreateTabValue and let it decide whether to add the
sensitive data.

That way people can't forget to do this check.

Or maybe this function is used places we don't want to have this behavior?

http://codereview.chromium.org/10829186/diff/13001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_set.cc (right):

http://codereview.chromium.org/10829186/diff/13001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_set.cc:61:
"tabs.captureVisibleTab",
Are there any functions that require tabs permission? If not, just add "tabs" to
kNonPermissionModuleNames, above.

[chebert, 2012-08-13 23:52:17]

https://chromiumcodereview.appspot.com/10829186/diff/13001/chrome/browser/ext...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/13001/chrome/browser/ext...
chrome/browser/extensions/api/tabs/tabs.cc:822: DictionaryValue* result =
ExtensionTabUtil::CreateTabValue(
An overloaded version is called from the MenuManager, the BrowserEventRouter,
and the MessageService.

I can make another overload that takes either the Extension* or a bool for
permissions, but I think that would be confusing.

Maybe a function name that describes that it is supposed to be used by the API
functions like CreateTabValueForExtension()?
CreateTabValueForAPIResult()?
CreateTabResultValue()?

On 2012/08/10 06:44:51, Aaron Boodman wrote:
> Can we pass Extension* into CreateTabValue and let it decide whether to add
the
> sensitive data.
> 
> That way people can't forget to do this check.
> 
> Or maybe this function is used places we don't want to have this behavior?

[Aaron Boodman, 2012-08-15 04:31:03]

So you're good on this right? (after our convo on irc)

[chebert, 2012-08-15 17:05:30]

On 2012/08/15 04:31:03, Aaron Boodman wrote:
> So you're good on this right? (after our convo on irc)

Yeah I'm still working through the details. Some of the classes that call
CreateTabValue() don't actually have access to the Extension(), so I'm trying to
figure out what to do for those.

[chebert, 2012-08-20 21:53:50]

Still doesn't display console warnings. I'm not sure how I would go about doing
that.

[Aaron Boodman, 2012-08-21 04:10:03]

+koz

The console warnings might be quite a bit more work and warrant a separate CL,
if we do them at all.

I think koz knows the most about the part of code where we'd likely add it. Koz,
see the bug for details, but basically we'd want to wrap every Tab object in
every API in a custom JS wrapper that throws special errors when certain
properties are accessed. Any idea how hard would such a thing be off hand?

(I'll look at the new patch soon)

[koz (OOO until 15th September), 2012-08-21 05:40:44]

On 2012/08/21 04:10:03, Aaron Boodman wrote:
> +koz
> 
> The console warnings might be quite a bit more work and warrant a separate CL,
> if we do them at all.
> 
> I think koz knows the most about the part of code where we'd likely add it.
Koz,
> see the bug for details, but basically we'd want to wrap every Tab object in
> every API in a custom JS wrapper that throws special errors when certain
> properties are accessed. Any idea how hard would such a thing be off hand?
> 
> (I'll look at the new patch soon)

I think the best chance at implementing something like that cleanly would be to
add something like a "permission_required": "tabs" to the relevant fields in
Tab's definition in tabs.json and then update tab objects in the validation
phase.

This would require making it so that validation occurs unconditionally in a few
situations, eg, currently we only do validation for data that we send to
callbacks and event listeners in debug mode.

So not necessarily a huge amount of work there, but any change to the bindings
is by default pretty tricky.

[Aaron Boodman, 2012-08-22 21:22:05]

http://codereview.chromium.org/10829186/diff/16027/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/10829186/diff/16027/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:80: return
ExtensionTabUtil::CreateTabValue(contents,
This was here before, but I think you can just call CreateTabValue() without
ExtensionTabUtil:: qualifier.

http://codereview.chromium.org/10829186/diff/16027/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:151:
!extension->HasAPIPermission(extensions::APIPermission::kTab)) {
You should use HasAPIPermissionForTab instead. It takes into account the new
activeTab permission.

http://codereview.chromium.org/10829186/diff/16027/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:152:
StripTabOfSensitiveData(result);
Rather than stripping sensitive data after the fact, it would be better to only
add it when permission is present.

[chebert, 2012-08-23 17:34:17]

https://chromiumcodereview.appspot.com/10829186/diff/16027/chrome/browser/ext...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/16027/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:80: return
ExtensionTabUtil::CreateTabValue(contents,
On 2012/08/22 21:22:05, Aaron Boodman wrote:
> This was here before, but I think you can just call CreateTabValue() without
> ExtensionTabUtil:: qualifier.

Done.

https://chromiumcodereview.appspot.com/10829186/diff/16027/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:151:
!extension->HasAPIPermission(extensions::APIPermission::kTab)) {
On 2012/08/22 21:22:05, Aaron Boodman wrote:
> You should use HasAPIPermissionForTab instead. It takes into account the new
> activeTab permission.

Done.

https://chromiumcodereview.appspot.com/10829186/diff/16027/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:152:
StripTabOfSensitiveData(result);
On 2012/08/22 21:22:05, Aaron Boodman wrote:
> Rather than stripping sensitive data after the fact, it would be better to
only
> add it when permission is present.

Done.

[Aaron Boodman, 2012-08-23 17:47:29]

Sweet.

Can you also please look through the samples that use the 'tabs' permission in
chrome/common/extensions/docs/examples/... and remove the tabs permission from
any that no longer require it?

Sorry, there are a lot to look through, but it's important that the samples are
good examples to follow. Perhaps you can devise a simple script to find the
candidates.

https://chromiumcodereview.appspot.com/10829186/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/21001/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:131:
result->SetString(keys::kUrlKey, std::string());
There's no reason to set these keys at all is there? The fields should be
optional.

https://chromiumcodereview.appspot.com/10829186/diff/21001/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:150: if
(entry->GetFavicon().valid) {
Can you rejigger things so that here is just one check for
HasAPIPermissionForTab?

[chebert, 2012-08-29 21:51:58]

Hey so since the docs server is up, I don't actually need to re-zip the samples,
right?


Because I didn't.

https://chromiumcodereview.appspot.com/10829186/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/21001/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:131:
result->SetString(keys::kUrlKey, std::string());
On 2012/08/23 17:47:29, Aaron Boodman wrote:
> There's no reason to set these keys at all is there? The fields should be
> optional.

Done.

https://chromiumcodereview.appspot.com/10829186/diff/21001/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:150: if
(entry->GetFavicon().valid) {
On 2012/08/23 17:47:29, Aaron Boodman wrote:
> Can you rejigger things so that here is just one check for
> HasAPIPermissionForTab?

Rejiggered.

[chebert, 2012-08-29 21:55:51]

Adding sky and jennb as reviewers for
chrome/browser/ui/views/ash/panel_view_aura.cc and
chrome/browser/ui/panels/panel.cc

[sky, 2012-08-29 22:11:30]

LGTM

[jennb, 2012-08-29 23:12:21]

LGTM for panels changes 

Hoping you'll land this soon as I'm making a change to the same method in
panel.cc that would benefit from having the new extension param. :-)

[Aaron Boodman, 2012-08-30 05:45:47]

If possible it would be really good to get this landed this week.

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
File chrome/browser/extensions/browser_event_router.cc (right):

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:402:
listeners().listeners(event_name));
The second 'listeners' should be GetListenersByName or some such.

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:447:
args->Append(ExtensionTabUtil::CreateTabValue(contents, NULL /*extension*/));
Why no extension here?

I think that CreateTabValue() may want to CHECK that it is given an extension to
ensure that we don't accidentally leak info via this function.

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
File chrome/browser/extensions/event_listener_map.h (right):

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
chrome/browser/extensions/event_listener_map.h:98: ListenerList& listeners(const
std::string& event_name) {
Can you return const&?

Also I noticed the call site was making a copy, even though you return by ref
here. Would be better to be an actual reference.

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
File chrome/browser/extensions/event_router.cc (right):

http://codereview.chromium.org/10829186/diff/34007/chrome/browser/extensions/...
chrome/browser/extensions/event_router.cc:449:
ExtensionTabUtil::MaybeStripEventArgsOfSensitiveData(
Can we go back to only adding it in the first place if needed?

http://codereview.chromium.org/10829186/diff/34007/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/active_tab/manifest.json (right):

http://codereview.chromium.org/10829186/diff/34007/chrome/test/data/extension...
chrome/test/data/extensions/api_test/active_tab/manifest.json:12: "tabs",
?

http://codereview.chromium.org/10829186/diff/34007/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json
(right):

http://codereview.chromium.org/10829186/diff/34007/chrome/test/data/extension...
chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json:9:
"permissions": [ "tabs" ],
?

[not at google - send to devlin, 2012-08-30 06:05:34]

drive-by: there is some code in chrome/renderer/extensions/dispatcher.cc that
can be cleaned up with this patch. Search for tabs.executeScript and
tabs.insertCSS.

[chebert, 2012-08-30 23:17:20]

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
File chrome/browser/extensions/browser_event_router.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
chrome/browser/extensions/browser_event_router.cc:402:
listeners().listeners(event_name));
On 2012/08/30 05:45:48, Aaron Boodman wrote:
> The second 'listeners' should be GetListenersByName or some such.

Done.

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
chrome/browser/extensions/browser_event_router.cc:447:
args->Append(ExtensionTabUtil::CreateTabValue(contents, NULL /*extension*/));
On 2012/08/30 05:45:48, Aaron Boodman wrote:
> Why no extension here?
> 
> I think that CreateTabValue() may want to CHECK that it is given an extension
to
> ensure that we don't accidentally leak info via this function.

Done.

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
File chrome/browser/extensions/event_listener_map.h (right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
chrome/browser/extensions/event_listener_map.h:98: ListenerList& listeners(const
std::string& event_name) {
On 2012/08/30 05:45:48, Aaron Boodman wrote:
> Can you return const&?
> 
> Also I noticed the call site was making a copy, even though you return by ref
> here. Would be better to be an actual reference.

Done.

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
File chrome/browser/extensions/event_router.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/browser/ext...
chrome/browser/extensions/event_router.cc:449:
ExtensionTabUtil::MaybeStripEventArgsOfSensitiveData(
On 2012/08/30 05:45:48, Aaron Boodman wrote:
> Can we go back to only adding it in the first place if needed?

Done.

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
File chrome/test/data/extensions/api_test/active_tab/manifest.json (right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
chrome/test/data/extensions/api_test/active_tab/manifest.json:12: "tabs",
Both of these extensions use the url of the tab value.

On 2012/08/30 05:45:48, Aaron Boodman wrote:
> ?

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
File chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json
(right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json:9:
"permissions": [ "tabs" ],
also gets the tab's url.

On 2012/08/30 05:45:48, Aaron Boodman wrote:
> ?

[chebert, 2012-08-30 23:31:14]

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
File chrome/test/data/extensions/api_test/active_tab/manifest.json (right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
chrome/test/data/extensions/api_test/active_tab/manifest.json:12: "tabs",
Ok. pretend I didn't actually say this.
On 2012/08/30 23:17:20, chebert wrote:
> Both of these extensions use the url of the tab value.
> 
> On 2012/08/30 05:45:48, Aaron Boodman wrote:
> > ?
>

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
File chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json
(right):

https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json:9:
"permissions": [ "tabs" ],
or this.
On 2012/08/30 23:17:20, chebert wrote:
> also gets the tab's url.
> 
> On 2012/08/30 05:45:48, Aaron Boodman wrote:
> > ?
>

[chebert, 2012-08-30 23:33:06]

err. no. wait. this is right. Pretend I didn't ask you to ignore it.

On 2012/08/30 23:31:14, chebert wrote:
>
https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
> File chrome/test/data/extensions/api_test/active_tab/manifest.json (right):
> 
>
https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
> chrome/test/data/extensions/api_test/active_tab/manifest.json:12: "tabs",
> Ok. pretend I didn't actually say this.
> On 2012/08/30 23:17:20, chebert wrote:
> > Both of these extensions use the url of the tab value.
> > 
> > On 2012/08/30 05:45:48, Aaron Boodman wrote:
> > > ?
> >
> 
>
https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
> File
chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json
> (right):
> 
>
https://chromiumcodereview.appspot.com/10829186/diff/34007/chrome/test/data/e...
> chrome/test/data/extensions/api_test/all_urls/content_script/manifest.json:9:
> "permissions": [ "tabs" ],
> or this.
> On 2012/08/30 23:17:20, chebert wrote:
> > also gets the tab's url.
> > 
> > On 2012/08/30 05:45:48, Aaron Boodman wrote:
> > > ?
> >

[Aaron Boodman, 2012-08-31 20:28:51]

http://codereview.chromium.org/10829186/diff/40002/chrome/browser/extensions/...
File chrome/browser/extensions/browser_event_router.cc (right):

http://codereview.chromium.org/10829186/diff/40002/chrome/browser/extensions/...
chrome/browser/extensions/browser_event_router.cc:400: const
EventListenerMap::ListenerList& listeners(
Nit: typical style would be to use equals here. I really only see Foo foo(...);
when an object is being constructed.

http://codereview.chromium.org/10829186/diff/40002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10829186/diff/40002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel.cc:92: // Safe to include info about the web
contents as this is only called
Hm, this is an example of a place where it should be OK to return url and title,
even though the extension doesn't have tabs permission. How do you plan to
handle this once this patch lands?

[Aaron Boodman, 2012-08-31 20:29:17]

Also, I just realized the 'windows' namespace should work the same way. But that
can be a separate CL.

[jennb, 2012-08-31 21:41:00]

http://codereview.chromium.org/10829186/diff/40002/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10829186/diff/40002/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel.cc:92: // Safe to include info about the web
contents as this is only called
On 2012/08/31 20:28:51, Aaron Boodman wrote:
> Hm, this is an example of a place where it should be OK to return url and
title,
> even though the extension doesn't have tabs permission. How do you plan to
> handle this once this patch lands?

Once I had an extension param, I want to add a
DCHECK(IsVisibleToExtension(extension);

Per IM conversation, chebert will replace the TODO with that DCHECK in this
patch before committing.

[chebert, 2012-09-01 00:40:30]

Aboodman: would like your lg before I commit this because I went back to setting
the keys to empty data.

https://chromiumcodereview.appspot.com/10829186/diff/40002/chrome/browser/ext...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://chromiumcodereview.appspot.com/10829186/diff/40002/chrome/browser/ext...
chrome/browser/extensions/extension_tab_util.cc:119: if
(extension->HasAPIPermissionForTab(tab_index,
OK so problem with only setting the keys if they are present.

chrome/renderer/resources/extensions/json_schema.js

the Json schema validator checks to make sure that the 'required' (meaning
optional: false) properties are there. so a couple of solutions I have in mind:

1) make a new term for something that is neither required nor optional 
'sensitive': true
and this data will be automatically stripped?
other possibility is
2) SetString(kUrlKey, std::string())

I have it set to #2 right now, because today is my last day for the summer (but
I'll still work remotely until I get this patch landed.)

[Aaron Boodman, 2012-09-01 01:03:09]

3) mark the fields optional in json schema and check that all the places we use
this structure it is being sent from chrome to extension. If there are places we
are sending from extension to chrome (can't think of any) then we'd have to
change them to use their own type.

[Aaron Boodman, 2012-09-04 07:36:34]

I landed this for you in crrev.com/154730 with a few minor changes. Thanks so
much for the patch, this will be a really nice improvement.