tab capture: Change the permissions for tabs.captureVisibleTab().

tab capture: Change the permissions for tabs.captureVisibleTab().

Require an extension to have '<all_urls>' permission, or been granted the
'activeTab' permission to be allowed to use tabs.captureVisibleTab().

BUG=83432
R=kalman@chromium.org

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

[sadrul, 2014-01-16 07:07:48]

I have only updated the existing tests, and not added any new test, since the
existing tests seem to have a pretty good coverage. If you can think of
additional cases I should cover with tests, please let me know.

[not at google - send to devlin, 2014-01-16 17:48:26]

https://codereview.chromium.org/140433003/diff/150001/chrome/browser/extensio...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/150001/chrome/browser/extensio...
chrome/browser/extensions/active_tab_unittest.cc:106: return
IsBothBlocked(extension, url, tab_id());
why do you need to separate these checks?

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:545:
all_urls.SetMatchAllURLs(true);
you can construct this in a single line with

URLPattern all_urls(URLPattern::SCHEME_ALL, URLPattern::kAllUrlsPattern);

since kAllUrlsPattern is guaranteed to parse.

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:546: if (tab_permissions.get()
&&
I think there will always be active permissions, so you don't need to check
this?

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:553:
GetTabSpecificPermissions(extension, tab_id)) {
I think the old check was what you want? the
tab_permissions->explicit_hosts().MatchesSecurityOrigin(page_url)?

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:558: *error =
errors::kAllURLOrActiveTabNeeded;
hopefully developers won't be confused... "but I have the activeTab
permission!". maybe this error message needs to be improved slightly to explain
that activeTab also needs to have been granted (or have 2 separate error
messages, one when neither is present, and one when activeTab is present but not
granted).

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
File extensions/common/permissions/permissions_data_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data_unittest.cc:342:
EXPECT_TRUE(CaptureOnly(extension.get(), settings_url));
Ugh this is tricky. Oh well I don't think there's realistically much we can do
about this besides on tab capture enumerate all of the frames on a page and make
sure none of them are file URLs (unless an extension has been granted file
access).

let me chat to the security people about this.

[not at google - send to devlin, 2014-01-16 18:57:13]

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
File extensions/common/permissions/permissions_data_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data_unittest.cc:342:
EXPECT_TRUE(CaptureOnly(extension.get(), settings_url));
On 2014/01/16 17:48:26, kalman wrote:
> Ugh this is tricky. Oh well I don't think there's realistically much we can do
> about this besides on tab capture enumerate all of the frames on a page and
make
> sure none of them are file URLs (unless an extension has been granted file
> access).
> 
> let me chat to the security people about this.

This is ok, turns out you can't put a file:// URL in an iframe. However
taptureVisibleTab should still respect the extension preference for enabling
access to file URLs in the top frame.

[sadrul, 2014-01-20 13:53:36]

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:545:
all_urls.SetMatchAllURLs(true);
On 2014/01/16 17:48:26, kalman wrote:
> you can construct this in a single line with
> 
> URLPattern all_urls(URLPattern::SCHEME_ALL, URLPattern::kAllUrlsPattern);
> 
> since kAllUrlsPattern is guaranteed to parse.

Done.

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:546: if (tab_permissions.get()
&&
On 2014/01/16 17:48:26, kalman wrote:
> I think there will always be active permissions, so you don't need to check
> this?

Done.

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:553:
GetTabSpecificPermissions(extension, tab_id)) {
On 2014/01/16 17:48:26, kalman wrote:
> I think the old check was what you want? the
> tab_permissions->explicit_hosts().MatchesSecurityOrigin(page_url)?

If the extension has the |activeTab| permission, and already been granted that
permission, does it still need to do MatchesSecurityOrigin()? I removed the
|page_url| parameter from CanCaptureVisiblePage() based on the reasoning that
this check would be superfluous. Let me know if I am mistaken about this.

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:558: *error =
errors::kAllURLOrActiveTabNeeded;
On 2014/01/16 17:48:26, kalman wrote:
> hopefully developers won't be confused... "but I have the activeTab
> permission!". maybe this error message needs to be improved slightly to
explain
> that activeTab also needs to have been granted (or have 2 separate error
> messages, one when neither is present, and one when activeTab is present but
not
> granted).

Done. (please let me know if you have suggestion about better error messages)

[not at google - send to devlin, 2014-01-21 21:44:20]

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:546: if (tab_permissions.get()
&&
On 2014/01/20 13:53:37, sadrul wrote:
> On 2014/01/16 17:48:26, kalman wrote:
> > I think there will always be active permissions, so you don't need to check
> > this?
> 
> Done.

I think I may have misread your code actually :\

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:553:
GetTabSpecificPermissions(extension, tab_id)) {
On 2014/01/20 13:53:37, sadrul wrote:
> On 2014/01/16 17:48:26, kalman wrote:
> > I think the old check was what you want? the
> > tab_permissions->explicit_hosts().MatchesSecurityOrigin(page_url)?
> 
> If the extension has the |activeTab| permission, and already been granted that
> permission, does it still need to do MatchesSecurityOrigin()? I removed the
> |page_url| parameter from CanCaptureVisiblePage() based on the reasoning that
> this check would be superfluous. Let me know if I am mistaken about this.

I think you're right; the checks are actually effectively equivalent since
- the only way to get tab specific origin permissions is via activeTab
- the origin is cleared each navigation.

as you say, removing the URL check from CaptureVisibleTab is kind of nice in
that it now rightly implies that it no longer matters.

Having the check be "does this extension have any tab specific permissions"
seems too flaky to me though. What if we have some kind of tab specific
permissions at some point which aren't triggered by a user action? This would
break then.

I see two options. First, easy; check the kTabs permission here, i.e., identical
to before except instead of MatchesSecurityOrigin use HasAPIPermission(kTabs). I
think that would work. Second, not as easy, introduce another permission like
kCaptureVisibleTabForTab like what the tabCapture API uses. That seems
unnecessary, so, the first one?

https://codereview.chromium.org/140433003/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/active_tab_unittest.cc:87: bool IsScriptAllowed(const
scoped_refptr<const Extension>& extension,
Comment for whole file: this is a pretty severe change to this file. The
splitting up of allowed into IsScriptAllowed and IsCaptureAllowed also feels a
bit unnatural.

Perhaps just keep them as IsAllowed/IsBlocked and in the IsAllowed case accept
some kind of enum? Like ALL vs SCRIPT_ONLY or something (default as ALL).

https://codereview.chromium.org/140433003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/active_tab_unittest.cc:116: extension.get(), tab_id,
NULL);
indentation around here needs updating

https://codereview.chromium.org/140433003/diff/240001/extensions/common/manif...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/140433003/diff/240001/extensions/common/manif...
extensions/common/manifest_constants.cc:225: "granted that permission by the
user for the current tab.";
The terminology we've been using for "active tab granted" is "invoked", so like,
"The 'activeTab' permission is not in effect because this extension has not been
invoked"

https://codereview.chromium.org/140433003/diff/240001/extensions/common/manif...
extensions/common/manifest_constants.cc:228: "This extension needs at least one
of '<all_urls>' and 'activeTab' "
Juts "Either the '<all_urls>' or 'activeTab' permission is required" ?

https://codereview.chromium.org/140433003/diff/240001/extensions/common/permi...
File extensions/common/permissions/permissions_data_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/240001/extensions/common/permi...
extensions/common/permissions/permissions_data_unittest.cc:367:
EXPECT_FALSE(Allowed(extension.get(), https_url));
maybe we need a "ScriptOnly" method?

[sadrul, 2014-01-23 20:11:14]

[ sorry about the long delay since last iteration ]

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:546: if (tab_permissions.get()
&&
On 2014/01/21 21:44:21, kalman wrote:
> On 2014/01/20 13:53:37, sadrul wrote:
> > On 2014/01/16 17:48:26, kalman wrote:
> > > I think there will always be active permissions, so you don't need to
check
> > > this?
> > 
> > Done.
> 
> I think I may have misread your code actually :\

Restored the null check for |tab_permissions|

https://codereview.chromium.org/140433003/diff/150001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:553:
GetTabSpecificPermissions(extension, tab_id)) {
On 2014/01/21 21:44:21, kalman wrote:
> On 2014/01/20 13:53:37, sadrul wrote:
> > On 2014/01/16 17:48:26, kalman wrote:
> > > I think the old check was what you want? the
> > > tab_permissions->explicit_hosts().MatchesSecurityOrigin(page_url)?
> > 
> > If the extension has the |activeTab| permission, and already been granted
that
> > permission, does it still need to do MatchesSecurityOrigin()? I removed the
> > |page_url| parameter from CanCaptureVisiblePage() based on the reasoning
that
> > this check would be superfluous. Let me know if I am mistaken about this.
> 
> I think you're right; the checks are actually effectively equivalent since
> - the only way to get tab specific origin permissions is via activeTab
> - the origin is cleared each navigation.
> 
> as you say, removing the URL check from CaptureVisibleTab is kind of nice in
> that it now rightly implies that it no longer matters.
> 
> Having the check be "does this extension have any tab specific permissions"
> seems too flaky to me though. What if we have some kind of tab specific
> permissions at some point which aren't triggered by a user action? This would
> break then.
> 
> I see two options. First, easy; check the kTabs permission here, i.e.,
identical
> to before except instead of MatchesSecurityOrigin use HasAPIPermission(kTabs).
I
> think that would work. Second, not as easy, introduce another permission like
> kCaptureVisibleTabForTab like what the tabCapture API uses. That seems
> unnecessary, so, the first one?

Yep, sounds good. Done.

https://codereview.chromium.org/140433003/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/active_tab_unittest.cc:87: bool IsScriptAllowed(const
scoped_refptr<const Extension>& extension,
On 2014/01/21 21:44:21, kalman wrote:
> Comment for whole file: this is a pretty severe change to this file. The
> splitting up of allowed into IsScriptAllowed and IsCaptureAllowed also feels a
> bit unnatural.
> 
> Perhaps just keep them as IsAllowed/IsBlocked and in the IsAllowed case accept
> some kind of enum? Like ALL vs SCRIPT_ONLY or something (default as ALL).

Done.

https://codereview.chromium.org/140433003/diff/240001/extensions/common/manif...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/140433003/diff/240001/extensions/common/manif...
extensions/common/manifest_constants.cc:225: "granted that permission by the
user for the current tab.";
On 2014/01/21 21:44:21, kalman wrote:
> The terminology we've been using for "active tab granted" is "invoked", so
like,
> "The 'activeTab' permission is not in effect because this extension has not
been
> invoked"

Done.

https://codereview.chromium.org/140433003/diff/240001/extensions/common/manif...
extensions/common/manifest_constants.cc:228: "This extension needs at least one
of '<all_urls>' and 'activeTab' "
On 2014/01/21 21:44:21, kalman wrote:
> Juts "Either the '<all_urls>' or 'activeTab' permission is required" ?

Done.

https://codereview.chromium.org/140433003/diff/240001/extensions/common/permi...
File extensions/common/permissions/permissions_data_unittest.cc (right):

https://codereview.chromium.org/140433003/diff/240001/extensions/common/permi...
extensions/common/permissions/permissions_data_unittest.cc:367:
EXPECT_FALSE(Allowed(extension.get(), https_url));
On 2014/01/21 21:44:21, kalman wrote:
> maybe we need a "ScriptOnly" method?

Done.

[not at google - send to devlin, 2014-01-23 20:16:35]

lgtm

https://codereview.chromium.org/140433003/diff/450001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/140433003/diff/450001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:546: if (active_permissions &&
it's the tab-specific permissions that need null checking, not the active
permissions. did I read this wrong... again?

https://codereview.chromium.org/140433003/diff/450001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:552:
HasAPIPermission(extension, APIPermission::kActiveTab)) {
I think this check is unnecessary. Just let the ActiveTabPermissionGranter do
its thing, and only check the tab specific permissions (if they exist) without
worrying about it.

[sadrul, 2014-01-23 20:23:49]

https://codereview.chromium.org/140433003/diff/450001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/140433003/diff/450001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:546: if (active_permissions &&
On 2014/01/23 20:16:35, kalman wrote:
> it's the tab-specific permissions that need null checking, not the active
> permissions. did I read this wrong... again?

More likely I misunderstood. Fixed

https://codereview.chromium.org/140433003/diff/450001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:552:
HasAPIPermission(extension, APIPermission::kActiveTab)) {
On 2014/01/23 20:16:35, kalman wrote:
> I think this check is unnecessary. Just let the ActiveTabPermissionGranter do
> its thing, and only check the tab specific permissions (if they exist) without
> worrying about it.

Done.

[sadrul, 2014-01-24 01:30:18]

Committed patchset #6 manually as r246766 (presubmit successful).

[meacer, 2014-01-24 01:42:58]

Drive by review: In the case of activeTab, the extension can inject cross origin
frames and can read them using captureVisibleTab. So activeTab +
captureVisibleTab effectively means all hosts permission rather than the active
tab, right? Not sure if this is something we want.

[sadrul, 2014-01-24 01:47:22]

On 2014/01/24 01:42:58, Mustafa Emre Acer wrote:
> Drive by review: In the case of activeTab, the extension can inject cross
origin
> frames and can read them using captureVisibleTab. So activeTab +
> captureVisibleTab effectively means all hosts permission rather than the
active
> tab, right? Not sure if this is something we want.

Perhaps leave the comment in the crbug instead? There are more people cc'ed
there. (an extension can currently do the same on a permitted host, right?)

[meacer, 2014-01-24 02:10:36]

On 2014/01/24 01:47:22, sadrul wrote:
> On 2014/01/24 01:42:58, Mustafa Emre Acer wrote:
> > Drive by review: In the case of activeTab, the extension can inject cross
> origin
> > frames and can read them using captureVisibleTab. So activeTab +
> > captureVisibleTab effectively means all hosts permission rather than the
> active
> > tab, right? Not sure if this is something we want.
> 
> Perhaps leave the comment in the crbug instead? There are more people cc'ed
> there. (an extension can currently do the same on a permitted host, right?)

Added the comment to the bug.

> (an extension can currently do the same on a permitted host, right?)

The extension can inject frames, but it can't read them if it doesn't have host
permissions. captureVisibleTab effectively allows bypassing same origin policy.

[mytele2, 2014-03-18 23:00:11]

> tab capture: Change the permissions for tabs.captureVisibleTab().

> Require an extension to have '<all_urls>' permission, or been granted the
> 'activeTab' permission to be allowed to use tabs.captureVisibleTab().


We have an extension with > 1M users, this patch breaks our and many other
extensions functionality.
The extension already have the following permissions "tabs" "http://*/*"
"https://*/*", 
if we change to the <all_urls>, extension will be automatically disabled for all
users!
This is abnormal situation. I'm sorry, but how long will you mock extensions
developers and 
what mn came up with these changes?
 
It has become impossible to make a stable products for Chrome platform, due to
the constant stupid changes, 
which are incompatible with a previous Chromium version. Please stop
incompatible changes, don't be evil.

[sadrul, 2014-03-18 23:05:14]

On 2014/03/18 23:00:11, mytele2 wrote:
> > tab capture: Change the permissions for tabs.captureVisibleTab().
> 
> > Require an extension to have '<all_urls>' permission, or been granted the
> > 'activeTab' permission to be allowed to use tabs.captureVisibleTab().
> 
> 
> We have an extension with > 1M users, this patch breaks our and many other
> extensions functionality.
> The extension already have the following permissions "tabs" "http://*/*"
> "https://*/*", 
> if we change to the <all_urls>, extension will be automatically disabled for
all
> users!

What if you use 'activeTab' permission instead
(http://developer.chrome.com/extensions/activeTab)? Would that work?

> This is abnormal situation. I'm sorry, but how long will you mock extensions
> developers and 
> what mn came up with these changes?
>  
> It has become impossible to make a stable products for Chrome platform, due to
> the constant stupid changes, 
> which are incompatible with a previous Chromium version. Please stop
> incompatible changes, don't be evil.

[mytele2, 2014-03-18 23:53:21]

> What if you use 'activeTab' permission instead
> http://developer.chrome.com/extensions/activeTab Would that work?

Thanks for qr. We have another choice? No.
Hope extension will not be auto-disabled due to the 'activeTab' permission.
We will try.

[mytele2, 2014-03-19 00:16:36]

No, doesn't work, only the following user gestures enable activeTab:

Executing a browser action
Executing a page action
Executing a context menu item
Executing a keyboard shortcut from the commands API
Accepting a suggestion from the omnibox API

In our case, a content script injects html element i.e button into a page :(

[not at google - send to devlin, 2014-03-19 15:52:43]

On 2014/03/19 00:16:36, mytele2 wrote:
> No, doesn't work, only the following user gestures enable activeTab:
> 
> Executing a browser action
> Executing a page action
> Executing a context menu item
> Executing a keyboard shortcut from the commands API
> Accepting a suggestion from the omnibox API
> 
> In our case, a content script injects html element i.e button into a page :(

mytele2, I'm not sure that changing to <all_urls> actually will cause your
extension to be disabled, it's the same warning ("all your tabs and browsing
history"). I apologise for not being able to say definitely, but I suggest
something like uploading a trusted-tester version of your extension with the new
permission to see what happens.

[not at google - send to devlin, 2014-03-19 15:53:20]

> extension to be disabled, it's the same warning ("all your tabs and browsing
> history").

"your data on all websites" rather.

[sandylamwork, 2014-04-13 09:16:32]

On 2014/03/19 15:53:20, kalman wrote:
> > extension to be disabled, it's the same warning ("all your tabs and browsing
> > history").
> 
> "your data on all websites" rather.

Hello, will this be fixed and when??? This bug seriously affects our users
(almost 400K). In our experience,  almost 1/3 of the users will forget to enable
the extension if it is disabled automatically due to permission change. This is
unfair since we did not change our permission at all. Please please please fix
this bug. Thank you.

[not at google - send to devlin, 2014-04-14 17:59:08]

On 2014/04/13 09:16:32, sandylamwork wrote:
> On 2014/03/19 15:53:20, kalman wrote:
> > > extension to be disabled, it's the same warning ("all your tabs and
browsing
> > > history").
> > 
> > "your data on all websites" rather.
> 
> Hello, will this be fixed and when??? This bug seriously affects our users
> (almost 400K). In our experience,  almost 1/3 of the users will forget to
enable
> the extension if it is disabled automatically due to permission change. This
is
> unfair since we did not change our permission at all. Please please please fix
> this bug. Thank you.

I don't know what bug you're referring to. Like I said above, http://*/* is the
same as <all_urls> for warnings and shouldn't cause the extension to be
disabled. If it does happen then file a bug rather than discussing it here.