Remove URLs from chrome.tabs.executeScript permission warning.

Remove URLs from chrome.tabs.executeScript permission warning.

This CL removes the URL error messages triggered by chrome.tabs.executeScript
when the extension lacks the permission to access the respective host. This is
necessary to prevent that an extension can see the list of open URLs of all
tabs without having asked for appropriate permission.

BUG=551626
Committed: https://crrev.com/b807906072e191a10febe3f1e9b9fa46429abff9
Cr-Commit-Position: refs/heads/master@{#359392}

[battre, 2015-11-06 10:37:49]

Description was changed from

==========
Remove URLs from permission warning.

This CL removes the URL error messages triggered by chrome.tabs.executeScript
when the extension lacks the permission to access the respective host. This is
necessary to prevent that an extension can see the list of open URLs of all
tabs without having asked for appropriate permission.

BUG=551626
==========

to

==========
Remove URLs from chrome.tabs.executeScript permission warning.

This CL removes the URL error messages triggered by chrome.tabs.executeScript
when the extension lacks the permission to access the respective host. This is
necessary to prevent that an extension can see the list of open URLs of all
tabs without having asked for appropriate permission.

BUG=551626
==========

[battre, 2015-11-06 10:37:49]

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

[battre, 2015-11-06 10:39:12]

Devlin, could you please review this?

Thank you!

Best regards,
Dominic

[battre, 2015-11-06 10:39:55]

BTW: I did not update kCannotAccessAboutUrl because I don't expect confidential
information in those URLs.

[Devlin, 2015-11-06 20:00:39]

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/active_tab/background.js:59: 'Cannot access
contents of a page. ' +
nitty nit: the page, instead of a page?

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
extensions/common/manifest_constants.cc:269: "Cannot access contents of a page.
"
Hmm... I might prefer to have two error messages here - one for if the extension
has tabs permission, and for otherwise, because knowing the host is rather
helpful.  Since all the permission checks boil down to two places, it seems like
it should be pretty easy to do?

[battre, 2015-11-06 21:44:18]

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/active_tab/background.js:59: 'Cannot access
contents of a page. ' +
On 2015/11/06 20:00:39, Devlin wrote:
> nitty nit: the page, instead of a page?

I was thinking that if you check "chrome.runtime.lastError" you might not know
the context so that "a page" would be appropriate. Let me know if you want me to
change it to "the page", I am happy to do that.

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
extensions/common/manifest_constants.cc:269: "Cannot access contents of a page.
"
On 2015/11/06 20:00:39, Devlin wrote:
> Hmm... I might prefer to have two error messages here - one for if the
extension
> has tabs permission, and for otherwise, because knowing the host is rather
> helpful.  Since all the permission checks boil down to two places, it seems
like
> it should be pretty easy to do?

For PermissionsData::CanRunOnPage() that should be trivial. For the
programmatic_script_injector.cc, I am not sure how to go about this. What would
you suggest as a strategy? Should I send the tabs permission as an attribute to
ExtensionMsg_ExecuteCode_Params and memorize it?

[Devlin, 2015-11-06 22:06:13]

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/active_tab/background.js:59: 'Cannot access
contents of a page. ' +
On 2015/11/06 21:44:17, battre wrote:
> On 2015/11/06 20:00:39, Devlin wrote:
> > nitty nit: the page, instead of a page?
> 
> I was thinking that if you check "chrome.runtime.lastError" you might not know
> the context so that "a page" would be appropriate. Let me know if you want me
to
> change it to "the page", I am happy to do that.

last error is pretty much set in the context of a callback from the API call, so
I think it's fine to do "the".

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
extensions/common/manifest_constants.cc:269: "Cannot access contents of a page.
"
On 2015/11/06 21:44:17, battre wrote:
> On 2015/11/06 20:00:39, Devlin wrote:
> > Hmm... I might prefer to have two error messages here - one for if the
> extension
> > has tabs permission, and for otherwise, because knowing the host is rather
> > helpful.  Since all the permission checks boil down to two places, it seems
> like
> > it should be pretty easy to do?
> 
> For PermissionsData::CanRunOnPage() that should be trivial. For the
> programmatic_script_injector.cc, I am not sure how to go about this. What
would
> you suggest as a strategy? Should I send the tabs permission as an attribute
to
> ExtensionMsg_ExecuteCode_Params and memorize it?

Ah, right, I forgot how much of a rat's nest the injection code is. :(

We actually have extension permissions on the renderer side, so we can just
check them.  The problem is *getting* to them.  In some way or another, the
ProgrammaticScriptInjector should check the extension's permissions in order to
generate the error.  The right-est way of doing this might be to pass in an
error to OnWillNotInject that's set from the CanExecuteScriptOnFrame call.  Or
passing in the InjectionHost to OnWillNotInject, and putting a method
GetAccessDeniedError on InjectionHost.

Sorry for the hassle.  If you don't think it's worth it, feel free to add a
TODO.  It's a nice-to-have, but not necessary.

[battre, 2015-11-11 13:31:17]

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/active_tab/background.js (right):

https://codereview.chromium.org/1414223005/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/active_tab/background.js:59: 'Cannot access
contents of a page. ' +
On 2015/11/06 22:06:13, Devlin wrote:
> On 2015/11/06 21:44:17, battre wrote:
> > On 2015/11/06 20:00:39, Devlin wrote:
> > > nitty nit: the page, instead of a page?
> > 
> > I was thinking that if you check "chrome.runtime.lastError" you might not
know
> > the context so that "a page" would be appropriate. Let me know if you want
me
> to
> > change it to "the page", I am happy to do that.
> 
> last error is pretty much set in the context of a callback from the API call,
so
> I think it's fine to do "the".

Done.

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
extensions/common/manifest_constants.cc:269: "Cannot access contents of a page.
"
On 2015/11/06 22:06:13, Devlin wrote:
> On 2015/11/06 21:44:17, battre wrote:
> > On 2015/11/06 20:00:39, Devlin wrote:
> > > Hmm... I might prefer to have two error messages here - one for if the
> > extension
> > > has tabs permission, and for otherwise, because knowing the host is rather
> > > helpful.  Since all the permission checks boil down to two places, it
seems
> > like
> > > it should be pretty easy to do?
> > 
> > For PermissionsData::CanRunOnPage() that should be trivial. For the
> > programmatic_script_injector.cc, I am not sure how to go about this. What
> would
> > you suggest as a strategy? Should I send the tabs permission as an attribute
> to
> > ExtensionMsg_ExecuteCode_Params and memorize it?
> 
> Ah, right, I forgot how much of a rat's nest the injection code is. :(
> 
> We actually have extension permissions on the renderer side, so we can just
> check them.  The problem is *getting* to them.  In some way or another, the
> ProgrammaticScriptInjector should check the extension's permissions in order
to
> generate the error.  The right-est way of doing this might be to pass in an
> error to OnWillNotInject that's set from the CanExecuteScriptOnFrame call.  Or
> passing in the InjectionHost to OnWillNotInject, and putting a method
> GetAccessDeniedError on InjectionHost.
> 
> Sorry for the hassle.  If you don't think it's worth it, feel free to add a
> TODO.  It's a nice-to-have, but not necessary.

I have added a TODO. This is a slightly bigger undertaking. We should try to
make the M48 cutoff.

I think that passing the InjectionHost to OnWillNotInject could be an approach.
Either by having a "std::string GetAccessDeninedError(InjectFailureReason
reason, GURL url, GURL effective_url)" function which would cause some code
dupliction for all InjectionHosts or by adding a bool MayReportUrlInError()
function. I think that the latter would be simpler.

[Devlin, 2015-11-11 16:50:47]

Looks like some tests are failing, so holding off on the LG in case more changes
are needed (but otherwise good).

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/1414223005/diff/1/extensions/common/manifest_...
extensions/common/manifest_constants.cc:269: "Cannot access contents of a page.
"
On 2015/11/11 13:31:17, battre wrote:
> On 2015/11/06 22:06:13, Devlin wrote:
> > On 2015/11/06 21:44:17, battre wrote:
> > > On 2015/11/06 20:00:39, Devlin wrote:
> > > > Hmm... I might prefer to have two error messages here - one for if the
> > > extension
> > > > has tabs permission, and for otherwise, because knowing the host is
rather
> > > > helpful.  Since all the permission checks boil down to two places, it
> seems
> > > like
> > > > it should be pretty easy to do?
> > > 
> > > For PermissionsData::CanRunOnPage() that should be trivial. For the
> > > programmatic_script_injector.cc, I am not sure how to go about this. What
> > would
> > > you suggest as a strategy? Should I send the tabs permission as an
attribute
> > to
> > > ExtensionMsg_ExecuteCode_Params and memorize it?
> > 
> > Ah, right, I forgot how much of a rat's nest the injection code is. :(
> > 
> > We actually have extension permissions on the renderer side, so we can just
> > check them.  The problem is *getting* to them.  In some way or another, the
> > ProgrammaticScriptInjector should check the extension's permissions in order
> to
> > generate the error.  The right-est way of doing this might be to pass in an
> > error to OnWillNotInject that's set from the CanExecuteScriptOnFrame call. 
Or
> > passing in the InjectionHost to OnWillNotInject, and putting a method
> > GetAccessDeniedError on InjectionHost.
> > 
> > Sorry for the hassle.  If you don't think it's worth it, feel free to add a
> > TODO.  It's a nice-to-have, but not necessary.
> 
> I have added a TODO. This is a slightly bigger undertaking. We should try to
> make the M48 cutoff.
> 
> I think that passing the InjectionHost to OnWillNotInject could be an
approach.
> Either by having a "std::string GetAccessDeninedError(InjectFailureReason
> reason, GURL url, GURL effective_url)" function which would cause some code
> dupliction for all InjectionHosts or by adding a bool MayReportUrlInError()
> function. I think that the latter would be simpler.

Okay, TODO is fine with me.

[battre, 2015-11-11 18:11:21]

Fixed test case

[battre, 2015-11-11 18:12:00]

The failure was caused by the TODO. I have adapted the test case accordingly.

Feel free to send this to the CQ if it LGTY.

Thanks,
Dominic

[battre, 2015-11-12 16:41:45]

On 2015/11/11 18:12:00, battre wrote:
> The failure was caused by the TODO. I have adapted the test case accordingly.
> 
> Feel free to send this to the CQ if it LGTY.
> 
> Thanks,
> Dominic

Trybots are now green.

[Devlin, 2015-11-12 16:54:56]

Whoops, sorry - had this open and forgot to send.  LGTM.

[battre, 2015-11-12 17:48:48]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2015-11-12 17:49:17]

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

[commit-bot: I haz the power, 2015-11-12 21:25:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-12 21:27:05]

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