Closed
Bug 1280235
Opened 8 years ago
Closed 7 years ago
Allow APIs to be locked down
Categories
(WebExtensions :: General, defect, P2)
WebExtensions
General
Tracking
(Performance Impact:none)
People
(Reporter: andy+bugzilla, Assigned: aswan)
References
(Blocks 2 open bugs)
Details
(Keywords: perf, Whiteboard: [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.
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [telemetry] triaged
Reporter | ||
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: General
Reporter | ||
Updated•8 years ago
|
webextensions: --- → ?
Reporter | ||
Updated•8 years ago
|
webextensions: ? → ---
Updated•8 years ago
|
Whiteboard: [telemetry] triaged → [qf][telemetry] triaged
Updated•8 years ago
|
Whiteboard: [qf][telemetry] triaged → [qf-][telemetry] triaged
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
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)
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → aswan
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8917686 -
Flags: review?(kmaglione+bmo)
Comment 6•7 years ago
|
||
mozreview-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/#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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8917685 -
Flags: review?(kmaglione+bmo)
Attachment #8917686 -
Flags: review?(kmaglione+bmo)
Comment 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/71ea8cf2a217
https://hg.mozilla.org/mozilla-central/rev/d4961b5813af
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 17•7 years ago
|
||
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?
Assignee | ||
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
(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?
Comment 20•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(aswan) → qe-verify-
Assignee | ||
Comment 21•7 years ago
|
||
(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
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•6 years ago
|
status-firefox58:
fixed → ---
Updated•3 years ago
|
Performance Impact: --- → -
Whiteboard: [qf-][telemetry] triaged → [telemetry] triaged
You need to log in
before you can comment on or make changes to this bug.
Description
•