Clean up mozillaAddons permission parsing logic in _parseManifest

RESOLVED FIXED in Firefox 66

Status

enhancement
P3
normal
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

unspecified
mozilla66
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
The current mozillaAddons permission parsing logic is as follows:
https://searchfox.org/mozilla-central/rev/246f2b4fab2/toolkit/components/extensions/Extension.jsm#1491-1497

    if (manifest && manifest.permissions.has("mozillaAddons") &&
        !this.isPrivileged) {
      Cu.reportError(`Stripping mozillaAddons permission from ${this.id}`);
      manifest.permissions.delete("mozillaAddons");
      let i = manifest.manifest.permissions.indexOf("mozillaAddons");
      if (i >= 0) {
        manifest.manifest.permissions.splice(i, 1);

The code suggests that it removes the "mozillaAddons" permission from add-ons which are not supposed to have that permission. However, it does not account for the possibility of having multiple "mozillaAddons" entries in the list.

Fortunately, this flaw has no negative consequences because the raw manifest.manifest.permissions is not used anywhere, except potentially in onUpdate:
https://searchfox.org/mozilla-central/rev/246f2b4fab/toolkit/components/extensions/ExtensionParent.jsm#98
https://searchfox.org/mozilla-central/rev/246f2b4fab/browser/components/extensions/parent/ext-tabs.js#342

We should clean up the logic, either by removing the unneeded partial "sanitization" (or by removing all duplicate entries). I believe that removing the unneded partial sanitization is preferred because internal API consumers should always use extension.permissions.has / extension.hasPermission instead of extension.manifest.permissions.includes

Updated

9 months ago
Priority: -- → P3
(Assignee)

Updated

8 months ago
Assignee: nobody → rob
Status: NEW → ASSIGNED
Flags: qe-verify-
(Assignee)

Comment 1

8 months ago
The "permissions" array of the raw manifest is not (and should not) be
used for permission checking, so it is not necessary to strip the
"mozillaAddons" permission from it, and hence that part of the code has
been removed.

New tests has been added to verify the permission warnings for some
combinations of permissions. This also includes tests that verify
that only privileged extensions can use "mozillaAddons" to unlock
unrestricted schemes.
Comment on attachment 9008053 [details]
Bug 1484263 - Clean up manifest permission parser and add tests

Andrew Swan [:aswan] has approved the revision.
Attachment #9008053 - Flags: review+

Comment 3

4 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/14fee8b5f596
Clean up manifest permission parser and add tests r=aswan

Comment 4

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.