1298939 - APIs are initialized even if the extension has no permissions for them

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1298939: Don't initialize APIs that the extension does not have permissions for.

Review commit: https://reviewboard.mozilla.org/r/75044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75044/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1298939: Don't register cleanup functions for listeners until the first time they're added.

Review commit: https://reviewboard.mozilla.org/r/75046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75046/

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8786080 [details]
Bug 1298939: Don't register cleanup functions for listeners until the first time they're added.

https://reviewboard.mozilla.org/r/75046/#review72940

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8786079 [details]
Bug 1298939: Don't initialize APIs that the extension does not have permissions for.

https://reviewboard.mozilla.org/r/75044/#review72942

r+ with comments.

::: toolkit/components/extensions/Schemas.jsm:1812
(Diff revision 1)
> +  checkPermissions(namespace, wrapperFuncs) {
> +    let ns = this.namespaces.get(namespace);
> +    if (ns && ns.permissions) {
> +      return ns.permissions.some(perm => wrapperFuncs.hasPermission(perm));
> +    }
> +    return true;
> +  },

This patch looks good, my only doubt is about the name of the last parameter of this new `checkPermissions` function:

`wrapperFuncs` makes me assume that the parameter will contain a reference to the `schemaWrapper` object, but it looks like that we are actually assigning it to the `extension` instance, and `hasPermission` is the only method that they have in common, how about rename it to `extension`?

::: toolkit/components/extensions/test/xpcshell/test_ext_api_permissions.js:10
(Diff revision 1)
> +add_task(function* test_storage_api_without_permissions() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background() {},
> +
> +    manifest: {
> +      permissions: [],

How about adding an explicit test case with an addon without the `permissions` manifest property? 
(I know that currently "a missing `permissions` property" or "an empty array `permissions` property"  are the same thing for our implementation, but the additional test case is cheap enough and it would prevent such a regression).

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8786079 [details]
Bug 1298939: Don't initialize APIs that the extension does not have permissions for.

https://reviewboard.mozilla.org/r/75044/#review72942

> This patch looks good, my only doubt is about the name of the last parameter of this new `checkPermissions` function:
> 
> `wrapperFuncs` makes me assume that the parameter will contain a reference to the `schemaWrapper` object, but it looks like that we are actually assigning it to the `extension` instance, and `hasPermission` is the only method that they have in common, how about rename it to `extension`?

Urgh. I meant to add a docstring. I'd rather the schema code be agnostic to external types, but I'll document the parameter so it makes more sense.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d0a934472960be184bee8048a99c9c8c1d65a1
Bug 1298939: Don't initialize APIs that the extension does not have permissions for. r=rpl

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be5f41dcbd78ecfc12706d5de83cc3a429f9a42
Bug 1298939: Don't register cleanup functions for listeners until the first time they're added. r=rpl

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4d0a9344729
https://hg.mozilla.org/mozilla-central/rev/3be5f41dcbd7