1484263 - Clean up mozillaAddons permission parsing logic in _parseManifest

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Rob Wu [:robwu]]

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

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1484263 - Clean up manifest permission parser and add tests

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 2

[Andrew Swan [:aswan]]

Comment on attachment 9008053 [details]
Bug 1484263 - Clean up manifest permission parser and add tests

Andrew Swan [:aswan] has approved the revision.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14fee8b5f596
Clean up manifest permission parser and add tests r=aswan

Comment 4

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/14fee8b5f596