1734987 - Introduce reliable isPrivileged flag in ExtensionData

Status: RESOLVED FIXED

(WebExtensions :: General, task, P2)

Description

[Rob Wu [:robwu]]

In bug 1733466, a certain manifest feature (l10n_resources) is limited to privileged extensions. The ExtensionData class is not aware of the concept of privileged extensions, only the Extension subclass is.

In bug 1675858, the lack of "isPrivileged" led to the introduction of hacks to avoid depending on the privileged flag for permission parsing, and resulted in skipping some warning messages when related to privileged API permissions. This however does not account for privileged host permissions, and one can still see errors like Reading manifest: Invalid extension permission: about:reader* from the built-in Screenshots extension on the first run of Firefox.

To reliably support privileged APIs, we need to set the privileged flag before the manifest is parsed by ExtensionData's parseManifest. Determing the privileged state depends on the extension's signedState, which in turn depends on having at least a partially parsed manifest. This cyclic dependency can be broken by separating parseManifest in two stages:

• A minimal stage that parses the extension type and ID from the manifest.
• The rest of parseManifest.

In between, the add-on signature can be determined, which can then be used to set the privileged flag before parsing the rest of the manifest. That means that the computation of the signedState member needs to move to between ExtensionData construction and loadManifest (which calls parseManifest).

By doing so, we can also simplify Extension.jsm by removing the hacks from bug 1675858.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1734987 - Part 1: Replace isPrivileged with ExtensionData check for l10n_resources

l10n_resources has bugs that results in errors when enabled for
ExtensionData. In this patch stack we are going to introduce the
isPrivileged property to ExtensionData.

To prevent these errors from being triggered when isPrivileged is added,
replace the "isPrivileged" in this check with a check against
ExtensionData.

l10n_resources support in ExtensionData will be added in bug 1733466.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1734987 - Part 2: Refactor verifySignedState to not require AddonInternal

This refactor replaces the aAddon parameter of verifySignedState
with the minimal parameters needed to perform its functionality.

This enables us to compute signedState without requiring a fully
initialized AddonInternal instance.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1734987 - Part 3: Add isPrivileged to ExtensionData + test

See https://bugzilla.mozilla.org/show_bug.cgi?id=1734987#c0 for the
description of the why and how of this patch.

Since isPrivileged is now supported on ExtensionData, this patch also
removes the work-arounds from bug 1675858.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1734987 - Part 4: Make addon.isPrivileged consistent with extension.isPrivileged

addon.isPrivileged and extension.isPrivileged are currently almost
identical, except in the case of temporary extensions when experiments
are enabled.

This difference is inexplicable for those unfamiliar with the history,
so this patch makes them identical.

addon.isPrivileged was introduced in bug 1543204 to support hidden
add-ons, which was intended to be used by privileged add-ons only.
Hence, this patch updates the "hidden" getter to continue ignoring the
hidden flag for temporary add-ons.

With this change, the addon.isPrivileged check can be removed from
loadManifestFromWebManifest, in favor of checking extension.isPrivileged
at the location where addon.hidden is initialized for the first (and now
only) time.

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1734987 - Part 3.1: Correct isPrivileged during updates

During updates, isPrivileged was not set correctly, which resulted in
incorrect error messages (warnings about unsupported permissions) when a
privileged/builtin extension was updated (a variant of bug 1675858).

In the previous patch (part 3), isPrivileged was added to the
ExtensionData constructor, and also passed to BootstrapScope calls,
so we can use that here to.

The unit test here serves two purposes:

1. Primarily: test coverage for the correctness of isPrivileged in the
   update scenario.
2. Test coverage for the existence and order of ExtensionAPI's onUpdate
   calls. There is no unit test for this mechanism right now, only
   indirect tests.

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1734987 - Part 3.2 - Pass explicit isPrivileged flag to ExtensionData calls

Although the default value for isPrivileged is false in the
ExtensionData constructor, let's add the explicit value (false) so that
it's obvious that the default value is intended.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0152446140c4
Part 1: Replace isPrivileged with ExtensionData check for l10n_resources r=rpl
https://hg.mozilla.org/integration/autoland/rev/01047f2fcc85
Part 2: Refactor verifySignedState to not require AddonInternal r=rpl
https://hg.mozilla.org/integration/autoland/rev/5ed31aa8b156
Part 3: Add isPrivileged to ExtensionData + test r=rpl
https://hg.mozilla.org/integration/autoland/rev/3c50643923f0
Part 3.1: Correct isPrivileged during updates r=rpl
https://hg.mozilla.org/integration/autoland/rev/d02e0c5d2f3e
Part 3.2 - Pass explicit isPrivileged flag to ExtensionData calls r=rpl,geckoview-reviewers,calu
https://hg.mozilla.org/integration/autoland/rev/ab302ff25a5f
Part 4: Make addon.isPrivileged consistent with extension.isPrivileged r=rpl

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0152446140c4
https://hg.mozilla.org/mozilla-central/rev/01047f2fcc85
https://hg.mozilla.org/mozilla-central/rev/5ed31aa8b156
https://hg.mozilla.org/mozilla-central/rev/3c50643923f0
https://hg.mozilla.org/mozilla-central/rev/d02e0c5d2f3e
https://hg.mozilla.org/mozilla-central/rev/ab302ff25a5f