Closed Bug 1280235 Opened 3 years ago Closed 2 years ago

Allow APIs to be locked down

Categories

(WebExtensions :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla58

People

(Reporter: andy+bugzilla, Assigned: aswan)

References

(Blocks 3 open bugs)

Details

(Keywords: perf, Whiteboard: [qf-][telemetry] triaged)

Attachments

(2 files)

If we want to allow telemetry access, we'll need to limit that down to Mozilla only. I believe Google does this by having an allow list of acceptable add-on ids. There might be a better way, I think Kris has some ideas around this.
Priority: -- → P2
Whiteboard: [telemetry] triaged
Blocks: 1304021
Component: WebExtensions: Untriaged → WebExtensions: General
webextensions: --- → ?
Blocks: 1336576
webextensions: ? → ---
Whiteboard: [telemetry] triaged → [qf][telemetry] triaged
Whiteboard: [qf][telemetry] triaged → [qf-][telemetry] triaged
Blocks: 1247045
Hey Kris - Do you have a strategy in mind for locking down Mozilla-internal APIs? I'd be game to work on a patch, if you're willing to mentor (or can find someone to mentor).
Flags: needinfo?(kmaglione+bmo)
This requires that we start signing system and Test Pilot extensions with special certificates with a particular OU. I don't know how far along that effort is, at this point. Andrew might.
Flags: needinfo?(kmaglione+bmo) → needinfo?(aswan)
I do and we have regular meetings with Cory and Wil about the signing. We need to get the signing done first then we can proceed on with locking them down. I will ping you on IRC.
Flags: needinfo?(aswan)
Blocks: 1363624
No longer blocks: 1363624
Blocks: 1269288
Assignee: nobody → aswan
No longer blocks: 1336576
Keywords: perf
Attachment #8917686 - Flags: review?(kmaglione+bmo)
Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

https://reviewboard.mozilla.org/r/188618/#review194260

Let's just add a "mozillaAddons" permission and check the privileged flag before we accept it, the way we do for "geckoProfiler"
Attachment #8917686 - Flags: review?(kmaglione+bmo)
Attachment #8917685 - Flags: review?(kmaglione+bmo)
Attachment #8917686 - Flags: review?(kmaglione+bmo)
Comment on attachment 8917685 [details]
Bug 1280235 Part 1 remove createAddonDetails from XPIProvider

https://reviewboard.mozilla.org/r/188616/#review194646
Attachment #8917685 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

https://reviewboard.mozilla.org/r/188618/#review194648

::: toolkit/components/extensions/Extension.jsm:1220
(Diff revision 2)
>  
> +  async _parseManifest() {
> +    let manifest = await super.parseManifest();
> +    if (manifest.permissions.has("mozillaAddons") &&
> +        this.addonData.addon.signedState !== AddonManager.SIGNEDSTATE_PRIVILEGED) {
> +      manifest.permissions.delete("mozillaAddons");

We should emit a warning here, too.

::: toolkit/components/extensions/Extension.jsm:1222
(Diff revision 2)
> +    let manifest = await super.parseManifest();
> +    if (manifest.permissions.has("mozillaAddons") &&
> +        this.addonData.addon.signedState !== AddonManager.SIGNEDSTATE_PRIVILEGED) {
> +      manifest.permissions.delete("mozillaAddons");
> +      let i = manifest.manifest.permissions.indexOf("mozillaAddons");
> +      if (i >= 0) {

This is always true.

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_privileged.js:43
(Diff revision 2)
> +      paths: [["privileged"]],
> +    },
> +  };
> +
> +  const catMan = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
> +  catMan.addCategoryEntry("webextension-modules", "test",

s/test/&-privileged/

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_privileged.js:46
(Diff revision 2)
> +  AddonTestUtils.init(global);
> +  AddonTestUtils.overrideCertDB();

Nit: Please do these at the top-level.

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_privileged.js:59
(Diff revision 2)
> +      manifest: {
> +        applications: {gecko: {id: "privilegedapi@tests.mozilla.org"}},
> +        permissions: ["mozillaAddons"],
> +      },
> +      background() {
> +        browser.test.sendMessage("result", browser.privileged !== undefined);

Nit: `browser.privileged instanceof Object`, or something, please.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4464
(Diff revision 2)
>        let params = {
>          id: aAddon.id,
>          version: aAddon.version,
>          installPath: aFile.clone(),
> -        resourceURI: getURIForResourceInFile(aFile, "")
> +        resourceURI: getURIForResourceInFile(aFile, ""),
> +        addon: aAddon,

We should probably just pass the signed state here, or something. aAddon is usually an internal add-on object, of some sort or other, and the exact type will change from call to call. That's probably not a huge deal now that legacy add-ons are gone, but I still don't think we want to expose them externally.
Attachment #8917686 - Flags: review+
Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

https://reviewboard.mozilla.org/r/188618/#review194648

> This is always true.

You mean if it is in manifest.permissions that it also has to be in manifest.manifest.permissions?  I think the chances are slim but some future regression could upset the conditions that cause it to be true but that in that case just throwing here would be better than the alternative (splice()ing the last element off manifest.manifest.permissions!)
Are you proposing something different here?
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71ea8cf2a217
Part 1 remove createAddonDetails from XPIProvider r=kmag
https://hg.mozilla.org/integration/autoland/rev/d4961b5813af
Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions r=kmag
https://hg.mozilla.org/mozilla-central/rev/71ea8cf2a217
https://hg.mozilla.org/mozilla-central/rev/d4961b5813af
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Is this documented anywhere? I can't find it in

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions

If not, can we get a paragraph somewhere?
Flags: needinfo?(aswan)
Keywords: dev-doc-needed
OS: Unspecified → All
Hardware: Unspecified → All
I'm not sure that's the right place to document it, that permission is only usable from extensions that are signed as "Mozilla Extensions" and the "Browser Extensions" section on MDN is focused on cross-browser extension standards.  Where it covers Firefox-specific features, they are still features that are available to all extensions.
If you have an application for this, perhaps we can discuss it via email or IRC?
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #18)

> If you have an application for this, perhaps we can discuss it via email or
> IRC?

I'm mainly hoping that folks who want to expose APIs for use by Mozilla WebExtensions won't have to reach out to individual people, and will be able to discover this capability!

We have some specific experimentation/future work desires around storage and sync, but it would be awesome if I could point engineers to docs rather than people.

Is that a realistic expectation, or is this something we intentionally want to limit awareness of within Mozilla?
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(aswan)
Flags: needinfo?(aswan) → qe-verify-
(In reply to Richard Newman [:rnewman] from comment #17)
> If not, can we get a paragraph somewhere?

Added a note to https://wiki.mozilla.org/Add-ons/InternalSigning
Thanks, aswan!
Keywords: dev-doc-needed
Blocks: 1419884
Product: Toolkit → WebExtensions
See Also: → 1484263
You need to log in before you can comment on or make changes to this bug.