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

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

Trunk
mozilla51

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

3 years ago
No description provided.
Comment hidden (mozreview-request)

Comment 3

3 years ago
mozreview-review
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
Attachment #8786080 - Flags: review?(lgreco) → review+

Comment 4

3 years ago
mozreview-review
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).
Attachment #8786079 - Flags: review?(lgreco) → review+
Assignee

Comment 5

3 years ago
mozreview-review-reply
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.
Assignee

Comment 6

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c4d0a9344729
https://hg.mozilla.org/mozilla-central/rev/3be5f41dcbd7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.