Allow tabCapture API to be granted for chrome:// pages.

Allow tabCapture API to be granted for chrome:// pages.

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

[justinlin, 2013-06-17 18:07:16]

ptal, thanks.

[not at google - send to devlin, 2013-06-17 18:14:13]

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_granter.cc:54:
granted_extensions_.Insert(extension);
this means that an extension with the tab capture permission can get injection
access to chrome:// pages.

you probably want to check the active tab permission granter from the API
bindings instead.

[not at google - send to devlin, 2013-06-17 18:16:09]

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_granter.cc:54:
granted_extensions_.Insert(extension);
On 2013/06/17 18:14:13, kalman wrote:
> this means that an extension with the tab capture permission can get injection
> access to chrome:// pages.
> 
> you probably want to check the active tab permission granter from the API
> bindings instead.

checking it elsewhere wouldn't work either. maybe we need to just make it always
work for chrome:// pages and make sure we properly check it elsewhere. I think
that makes sense.

[justinlin, 2013-06-17 18:22:19]

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_granter.cc:54:
granted_extensions_.Insert(extension);
On 2013/06/17 18:14:13, kalman wrote:
> this means that an extension with the tab capture permission can get injection
> access to chrome:// pages.
> 
> you probably want to check the active tab permission granter from the API
> bindings instead.

Does it actually do that? I thought the injection stuff is only granted if the
below UpdateTabSpecificPermissions IPC is sent?

I'm not sure I understand what you're proposing?

[not at google - send to devlin, 2013-06-17 18:44:45]

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_granter.cc:54:
granted_extensions_.Insert(extension);
On 2013/06/17 18:22:20, justinlin wrote:
> On 2013/06/17 18:14:13, kalman wrote:
> > this means that an extension with the tab capture permission can get
injection
> > access to chrome:// pages.
> > 
> > you probably want to check the active tab permission granter from the API
> > bindings instead.
> 
> Does it actually do that? I thought the injection stuff is only granted if the
> below UpdateTabSpecificPermissions IPC is sent?
> 
> I'm not sure I understand what you're proposing?

I see; IsGranted is only used in the tab capture API. Yes you're right as it is
this wouldn't give script privileges.

It's nevertheless very strange to have IsGranted return true when in fact it
hasn't been granted, which is what will happen when an extension has tabCapture.
I can imagine a regression in which IsGranted *is* in fact used in that scenario
and a false sense of security is given by the permission granting logic.

This class is supposed to be general, not given API specific code; it just so
happens that the tabCapture API implies the activeTab permission. Come to think
of it a nicer way to do that now would be to use the features system, but,
another day.

I would like to suggest that we grant for chrome:// URLs regardless, and make
the chrome:// script injection logic part of the application logic, but I'm not
totally confident. We could test it easily enough. What would that take?

Or possibly activeTab is actually the wrong idiom for this after all and we
should make the process separate, e.g. make this class take a Grant callback
which takes a URL and returns the new permission data; then have both a
host_permission_granter and a tab_capture_permission_granter in TabHelper.

[justinlin, 2013-09-13 00:50:58]

Super late reply! Here's another proposal. What about adding an observer
interface to ActiveTabPermissionGranter which notifies when GrantIfRequested()
is triggered?

I could just add the tab_capture_registry as an observer and keep track of
extensions where a tab has been invoked since that's all tabCapture really cares
about (not really script injection). The only thing to consider with this is
lifecycle issues, but I think it works out. ActiveTabPermissionGranter should
outlive BrowserContextKeyedServices?

[not at google - send to devlin, 2013-09-13 01:26:29]

Heh, been a while. I have yet another proposal at this point.

- add a new (private within chrome) permission kTabCaptureForTab
- remove IsGranted, factor tests and code to not require it
- make ActiveTabPermissionGranter take different actions depending on what
permissions an extension has.

the GrantIfRequested (p.s. wish I'd called that just Grant now) would look
something like:

if (IsGranted)
  return;  // though I don't know what this actually achieves.

if (extension->HasPermission(kActiveTab)) {
  // should we grant tabs for the chrome:// tab? sure why not...
  extension->AddTabSpecificPermission(id, kTabs);
  if (<parse without chrome:// scheme>)
    extension->AddTabSpecificHostPermisisonOrWhatever(id, url);
}
if (extension->HasPermission(kTabCapture)) {
  extension->AddTabSpecificPermission(id, kTabCaptureForTab);
}

something like that.

[justinlin, 2013-09-13 02:54:12]

I'm a fan! Made those changes. PTAL

[not at google - send to devlin, 2013-09-13 03:22:43]

lg but there are a couple of other places IsGranted is used, you'll need to
replace them with checks for HasTabSpecificPermission or wahtever it is.

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:43: }
this check here isn't worth much anymore

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:80: new_permissions);
could you factor this so that each case shares more code? like

APIPermissionSet new_apis;
URLPatternSet new_hosts;

if (extension->HasPermission(kActiveTab)) {
 ...
}
if (extension->HasPermission(kTabCapture)) {
 ...
}

scoped_refptr<...>
PermissionsData::Update...();
Send(..)  // might as well

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:78: int routing_id
= rvh->GetRoutingID();
I don't see why you needed to move these up here, but, sure.

https://codereview.chromium.org/17298002/diff/11001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/tab_capture/experimental/active_tab_chrome_pages.js
(right):

https://codereview.chromium.org/17298002/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tab_capture/experimental/active_tab_chrome_pages.js:13:
chrome.test.notifyPass();
do you need this notifyPass? I don't know what it does...

[justinlin, 2013-09-13 19:27:32]

Thanks, PTAL. Added a test, and checked that it works, but can't turn it on
until tabcapture is moved to stable, which I'll do after this CL lands.

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:43: }
On 2013/09/13 03:22:44, kalman wrote:
> this check here isn't worth much anymore

Still need this, otherwise there will be some un-needed IPC's.

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:80: new_permissions);
On 2013/09/13 03:22:44, kalman wrote:
> could you factor this so that each case shares more code? like
> 
> APIPermissionSet new_apis;
> URLPatternSet new_hosts;
> 
> if (extension->HasPermission(kActiveTab)) {
>  ...
> }
> if (extension->HasPermission(kTabCapture)) {
>  ...
> }
> 
> scoped_refptr<...>
> PermissionsData::Update...();
> Send(..)  // might as well

Done. Fixed a kind of subtle bug too with this second patch. If both activeTab
and tabCapture permissions were present, it would early return in the activeTab
URL check, so you wouldn't get the tabCaptureForTab permission..

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:78: int routing_id
= rvh->GetRoutingID();
On 2013/09/13 03:22:44, kalman wrote:
> I don't see why you needed to move these up here, but, sure.

Needed the tab_id.

https://codereview.chromium.org/17298002/diff/11001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/tab_capture/experimental/active_tab_chrome_pages.js
(right):

https://codereview.chromium.org/17298002/diff/11001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/tab_capture/experimental/active_tab_chrome_pages.js:13:
chrome.test.notifyPass();
On 2013/09/13 03:22:44, kalman wrote:
> do you need this notifyPass? I don't know what it does...

It's to return control to C++ because the test is run with RunExtensionSubtest
which doesn't return until it gets a test success.

[not at google - send to devlin, 2013-09-13 22:09:45]

lgtm

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:43: }
On 2013/09/13 19:27:32, justinlin wrote:
> On 2013/09/13 03:22:44, kalman wrote:
> > this check here isn't worth much anymore
> 
> Still need this, otherwise there will be some un-needed IPC's.

why? you check new_apis.empty.

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:57:
new_apis.insert(APIPermission::kTab);
for consistency with tabCapture I think add this permission regardless of parse
success here. on the host permission thing matters for chrome:// URLs really.

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:131:
APIPermission::kTab);
can you just write this check specifically wherever it's needed? It's
effectively just a test utility now and not worth building into production code.

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_unittest.cc:334: // TODO(justinlin): Enable
when tabCapture API is moved to stable.
could you use ScopedCurrentChannel in feature_channel.h and enable it now?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[justinlin, 2013-09-13 22:47:14]

thanks

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:43: }
On 2013/09/13 22:09:45, kalman wrote:
> On 2013/09/13 19:27:32, justinlin wrote:
> > On 2013/09/13 03:22:44, kalman wrote:
> > > this check here isn't worth much anymore
> > 
> > Still need this, otherwise there will be some un-needed IPC's.
> 
> why? you check new_apis.empty.

Done. Well, now I do :)

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:57:
new_apis.insert(APIPermission::kTab);
On 2013/09/13 22:09:45, kalman wrote:
> for consistency with tabCapture I think add this permission regardless of
parse
> success here. on the host permission thing matters for chrome:// URLs really.

OK, I'll do this separately. I'm not really sure what the kTab permission
entails. Are you sure it should be set for chrome:// pages?

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:131:
APIPermission::kTab);
On 2013/09/13 22:09:45, kalman wrote:
> can you just write this check specifically wherever it's needed? It's
> effectively just a test utility now and not worth building into production
code.

Done.

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_unittest.cc:334: // TODO(justinlin): Enable
when tabCapture API is moved to stable.
On 2013/09/13 22:09:45, kalman wrote:
> could you use ScopedCurrentChannel in feature_channel.h and enable it now?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

Done.

[not at google - send to devlin, 2013-09-13 22:48:52]

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/17298002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/active_tab_permission_granter.cc:57:
new_apis.insert(APIPermission::kTab);
On 2013/09/13 22:47:15, justinlin wrote:
> On 2013/09/13 22:09:45, kalman wrote:
> > for consistency with tabCapture I think add this permission regardless of
> parse
> > success here. on the host permission thing matters for chrome:// URLs
really.
> 
> OK, I'll do this separately. I'm not really sure what the kTab permission
> entails. Are you sure it should be set for chrome:// pages?

It would mean that extensions could read the title of tabs that get clicked when
previously they couldn't. This is fine.

[commit-bot: I haz the power, 2013-09-13 22:50:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/17298002/42001

[commit-bot: I haz the power, 2013-09-14 01:24:30]

Retried try job too often on win7_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-14 20:36:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/17298002/56001

[commit-bot: I haz the power, 2013-09-15 00:50:02]

Change committed as 223269