This is an update based on https://wiki.mozilla.org/WebExtensions/Applications, which allows a WebExtension id to be specified in one of the following places: * browser_specific_settings in manifest.json * applications in manifest.json * in the signature itself If one of those doesn't work, it will try to assign a temporary id, if appropriate as mentioned in bug 1255564. If that fails, then it just throws an error and fails to load the add-on.

dveditz, does this sound ok to you? Basically we are allowing a WebExtension to not have an id and use (and trust) the id from the signature.

Is there a more detailed spec available for browser_specific_settings? There are a few other things beside id that are currently under applications.gecko that will need to be addressed as part of this bug (including update url, min/max versions)

Everything thats in applications can be in browser_specific_settings. Consider it just a rename of the value.

(In reply to Andy McKay [:andym] from comment #1) > dveditz, does this sound ok to you? Basically we are allowing a WebExtension > to not have an id and use (and trust) the id from the signature. The crucial bit is that the ID in the signature match the extension, so one signature can't be used to sign something that will overwrite/replace someone else's extension. If the ID in the signature IS the ID then that issue is solved. It does remove a source of sanity checks for the signing infrastructure. That means the ID has to be passed in as a parameter and we just have to trust the caller of the signing API. The review/signing path and code might need to be re-reviewed with this taken into account -- it won't be enough to make sure the code that was reviewed matches what's presented for signing, we have to make sure the metadata can't be tampered with as well. I didn't review that bit, I think it was :kang? I'm more concerned about the fact that there might be two other places to get an ID, and what do we do if they don't match? But as long as we predictably favor one over the other, and whichever one we do use has to match the ID in the signature then that's OK in the end, too (assuming we've validated the signature so we know the contents aren't tampered with).

(In reply to Daniel Veditz [:dveditz] from comment #4) > It does remove a source of sanity checks for the signing infrastructure. > That means the ID has to be passed in as a parameter and we just have to > trust the caller of the signing API. We already pass the ID as a parameter. That's the reason that signing multi-package XPIs never worked: The inner XPIs were signed with the wrong ID. > The review/signing path and code might need to be re-reviewed with this > taken into account -- it won't be enough to make sure the code that was > reviewed matches what's presented for signing, we have to make sure the > metadata can't be tampered with as well. Did we make sure of that before? > I'm more concerned about the fact that there might be two other places to > get an ID, and what do we do if they don't match? But as long as we > predictably favor one over the other, and whichever one we do use has to > match the ID in the signature then that's OK in the end, too (assuming we've > validated the signature so we know the contents aren't tampered with). I think that if the ID is specified in multiple places, they should all be required to match. The "applications" property is meant to be phased out. Support should eventually be removed.

@comment 4 I did not find a review per se - just the RRA of the service itself (https://docs.google.com/spreadsheets/d/1M-Fg2DH2ka_lwfB2y-YiyuxwXqqMTU5DBV8MlSXKmnY/edit#gid=0) It sounds like it depends on the container of the add-on and the signature. If the signature does not cover metadata, and metadata can be misused when modified, that seems dangerous though

(In reply to Daniel Veditz [:dveditz] from comment #4) > It does remove a source of sanity checks for the signing infrastructure. > That means the ID has to be passed in as a parameter and we just have to > trust the caller of the signing API. The review/signing path and code might > need to be re-reviewed with this taken into account -- it won't be enough to > make sure the code that was reviewed matches what's presented for signing, > we have to make sure the metadata can't be tampered with as well. I didn't > review that bit, I think it was :kang? The add-on id is passed as a parameter to trunion from the AMO process. In the past if you wanted to sign an add-on with a different id, you'd have to change the add-on id passed to trunion and the manifest.json file. The AMO process is the only thing that can call trunion. It feels like the attack vector is still the same, you have to break into AMO to get it sign the add-on with the right id.

To clarify, the plan is: - The ID for a signed XPI will come directly from the signature (from the CN in the cert specifically) - If a signed XPI also has an ID in manifest.json in either applications.gecko or browser_specific_settings.gecko.id, that ID *must* match the ID from the signature or we will fail to load - For a temporary install of an unsigned extension that does not have an ID in the manifest, we will just generate a temporary ID on the fly.

web-ext (https://github.com/mozilla/web-ext) currently installs a temporary add-on for development like this: - creates a temp profile - flips some settings - copies a file/directory/symlink to <profile>/extensions/<addon-id> - starts Firefox - the add-on gets installed automatically as long as the ID in the filename matches the add-on manifest. I'd like to preserve this behavior. One idea is that Firefox could see that the extension manifest has no ID and simply use its filename as the ID instead of auto-generating a new one. This should be easy enough since the manager already expects the add-on files to live in a directory named after its ID.

Review commit: https://reviewboard.mozilla.org/r/45163/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45163/

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer I got a bunch of test failures, this isn't ready for review yet, I'll push a new version when it is.

> - flips some settings to clarify, web-ext does this: // Only load extensions from the application and user profile. // AddonManager.SCOPE_PROFILE + AddonManager.SCOPE_APPLICATION 'extensions.enabledScopes' : 5, // Allow installing extensions dropped into the profile folder. 'extensions.autoDisableScopes' : 10,

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45163/diff/1-2/

Here is the try run, the one failure appears to me to be unrelated? https://treeherder.mozilla.org/#/jobs?repo=try&revision=96a4ac65e381

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer https://reviewboard.mozilla.org/r/45163/#review41889 I haven't done a full review here but there are some issues that need to be fixed before this can be looked at in depth. First you're doing different things depending on whether the add-on is packed (using the certificate ID) or unpacked (generating a random ID or using the directory name). Unless we have a compelling reason otherwise we should be doing the same for both. Second I'm seeing test failures on windows right now. It's possible that that is because I haven't got my new laptop set up right yet so I've spun some try builds to verify. Third can you look at writeWebManifestForExtension and getIDForManifest in head_addons.js and decide if they need to be updated for the new json format. Finally this is a pretty big change to the kind of add-ons we accept so it is going to need more than just two testcases to verify that it is working as expected. There are now (ignoring temporary add-ons for the moment) four different places that might determine an ID. We don't need to test every combination but we should at least verify that we prefer the new json location and that the cases where the ID doesn't match causes failures. For temporary add-ons we need to make sure that installing a temporary add-on that is signed still works, I don't think it does right now. ::: toolkit/components/extensions/schemas/manifest.json:227 (Diff revision 2) > + "strict_max_version": { > + "type": "string", > + "optional": true > + } > + } > + Nit: whitespace ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1324 (Diff revision 2) > > let uri = Services.io.newFileURI(file).QueryInterface(Ci.nsIFileURL); > > let addon = file.leafName == FILE_WEB_MANIFEST ? > yield loadManifestFromWebManifest(uri) : > - loadFromRDF(uri); > + loadFromRDF(uri); Nit: You've unindented this line. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1327 (Diff revision 2) > let addon = file.leafName == FILE_WEB_MANIFEST ? > yield loadManifestFromWebManifest(uri) : > - loadFromRDF(uri); > + loadFromRDF(uri); > > + if (!addon.id) { > + if (aInstallLocation == TemporaryInstallLocation) { Also do this for the registry install locations since we can't rely on those using sane names. ::: toolkit/mozapps/extensions/test/xpcshell/test_webextensions_install.js:5 (Diff revision 2) > +function run_test() { > + run_next_test(); > +} > + > +function promiseInstallAddon(filename) { This already exists, promiseInstallAllFiles ::: toolkit/mozapps/extensions/test/xpcshell/test_webextensions_install.js:19 (Diff revision 2) > + install.install(); > + }, "application/x-xpinstall"); > + }); > +} > + > +function promiseGetAddonByID(id) { This already exists, promiseAddonByID ::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini:38 (Diff revision 2) > [test_XPIStates.js] > [test_temporary.js] > [test_proxies.js] > [test_proxy.js] > [test_pass_symbol.js] > +[test_webextensions_install.js] Please move this to xpcshell-shared.ini

(In reply to Dave Townsend [:mossop] from comment #16) > First you're doing different things depending on whether the add-on is > packed (using the certificate ID) or unpacked (generating a random ID or > using the directory name). Unless we have a compelling reason otherwise we > should be doing the same for both. Right, I merged bug 1255564 into this one, I think that is creating some unnecessary confusion here, but there were 3 different requirements this was meant to handle: - Use the certificate ID if the extension is signed and there is no ID in the manifest. This was the original topic of this bug and per comment 9 the existing behavior is unchanged when the extension is signed and there is an ID in the manifest. - Generate a temporary ID if the extension is unpacked and this is a temporary install (this was originally bug 1255564). - Use the directory name for unpacked installs from other directories. This was Kumar's request from comment 10 and note that this code path will not be followed for the great majority of users who have not overridden extensions.enabledScopes since an unsigned install from a non-temporary location will be rejected due to the lack of signing. I'm not seeing an obvious way to simplify given those requirements and the requirements all seem reasonable to me. > Second I'm seeing test failures on windows right now. It's possible that > that is because I haven't got my new laptop set up right yet so I've spun > some try builds to verify. Do you have links? The try build I referenced above was clean on 64-bit windows, though I didn't do 32-bit. > Third can you look at writeWebManifestForExtension and getIDForManifest in > head_addons.js and decide if they need to be updated for the new json format. I don't think there's an immediate need to update them. Existing tests all include an ID in the manifest and these functions work for them as-is. We can easily write new tests that use writeWebManifestForWebExtension without including an ID in the manifest by passing the aId argument. As I understanding getIDForManifest, it exists so that we can easily create what look like signed packed extensions for xpcshell tests without the hassle of actually signing them. If we want to develop new tests like this in which we don't include an ID in the manifest, we'll need to do something different. I don't have any great ideas, but it also doesn't feel like a problem that needs to be solved right now? > Finally this is a pretty big change to the kind of add-ons we accept so it > is going to need more than just two testcases to verify that it is working > as expected. There are now (ignoring temporary add-ons for the moment) four > different places that might determine an ID. We don't need to test every > combination but we should at least verify that we prefer the new json > location and that the cases where the ID doesn't match causes failures. For > temporary add-ons we need to make sure that installing a temporary add-on > that is signed still works, I don't think it does right now. I'll add further tests (though I'll wait until we get the requirements nailed down from the beginning of this comment). Note also that this change only applies to webextensions (since the new code is in blocks beginning with `if !addon.id` and the rdf parser throws before we reach that code if there is no ID). I will rewrite the logic though to make that flow of control more obvious and not dependent on that particular behavior of rdf parsing. > ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1327 > (Diff revision 2) > > let addon = file.leafName == FILE_WEB_MANIFEST ? > > yield loadManifestFromWebManifest(uri) : > > - loadFromRDF(uri); > > + loadFromRDF(uri); > > > > + if (!addon.id) { > > + if (aInstallLocation == TemporaryInstallLocation) { > > Also do this for the registry install locations since we can't rely on those > using sane names. My understanding of windows is limited, but this code (to choose a temporary ID) was meant so that an add-on developer could do a temporary install on a webextension without an ID in the manifest, with the idea that eventually they will get the extension signed and we will get the ID from the signature, but before it is signed they should be able to develop/test it without changing the manifest. So this path is limited to the developer's workflow of a temporary install from about:debugging. I hope I explained that sensibly, given that does it make sense to apply the same logic to registry install locations?

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45163/diff/2-3/

Updated to make it more explicit that the new logic applies to WebExtensions only, added some more tests, and various cleanups in response to earlier feedback. We don't have a test of the scenario where a signed WebExtension has an ID in the manifest that is different from the one with which it is signed. But we do have such tests for bootstrap and non-bootstrap add-ons. Should we add a corresponding one for WebExtensions?

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer https://reviewboard.mozilla.org/r/45163/#review42729 I think the only other thing I'd note here is that it stands out as inconsistent to do different things depending on whether the add-on is packed or unpacked. For example an unpacked signed webextension won't get its ID from the signature. That said I don't know if we are ever going to see that case so maybe it isn't worth worrying about. It just might trip us up if we forget that. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1333 (Diff revision 3) > + .getService(Ci.nsIUUIDGenerator) > + .generateUUID().toString(); > + logger.info(`Generated temporary id ${id} for ${aDir.path}`); > + addon.id = id; > + } else { > + addon.id = aDir.leafName; So this will allow us to use any characters the filesystem accepts as part of the ID. I can't think of any problem with that but can you include a test which uses some characters in the path that we wouldn't normally see in an ID. Given that we serialise to JSON I guess any JS delimeters that work in the filesystem would be good to include. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1685 (Diff revision 3) > * @param aFile > * the xpi file to check > * @param aAddon > * the add-on object to verify > - * @return a Promise that resolves to an AddonManager.SIGNEDSTATE_* constant. > + * @return a Promise that resolves to an array with two elements: > +* an AddonManager.SIGNEDSTATE_* constant and an nsIX509Cert. I'd prefer for this to be an object with state and cert properties. Just as easy to destruct but more self-documenting. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1756 (Diff revision 3) > * @return a Promise that resolves to an AddonManager.SIGNEDSTATE_* constant. > */ > function verifyBundleSignedState(aBundle, aAddon) { > if (aBundle.isFile()) > - return verifyZipSignedState(aBundle, aAddon); > + return verifyZipSignedState(aBundle, aAddon).then(([state, cert]) => state); > return verifyDirSignedState(aBundle, aAddon); While you don't use it right now please make these methods have consistent signatures.

https://reviewboard.mozilla.org/r/45163/#review42729 This probably a naive question but I don't understand the "unpacked webextension" scenario. From what I can gather (mostly from https://developer.mozilla.org/en-US/Add-ons/Install_Manifests#unpack), unpacking appears to be relevant for things that largely don't apply in webextensions (with windows icons maybe being an exception?) Would it be feasible to just make unpacked (non-temporary) webextensions unsupported?

https://reviewboard.mozilla.org/r/45163/#review42729 > So this will allow us to use any characters the filesystem accepts as part of the ID. I can't think of any problem with that but can you include a test which uses some characters in the path that we wouldn't normally see in an ID. Given that we serialise to JSON I guess any JS delimeters that work in the filesystem would be good to include. The ID is still validated so bad characters etc. will cause the addon to fail to load. I'll add a test for this though.

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45163/diff/3-4/

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45163/diff/4-5/

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45163/diff/5-6/

Comment on attachment 8739268 [details] MozReview Request: Bug 1262005 Rework how WebExtensions IDs are determined r?rhelmer https://reviewboard.mozilla.org/r/45163/#review44933 ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_paths.js:41 (Diff revision 6) > + ]; > + > + for (let dir of directories) { > + try { > + writeWebManifestForExtension(manifest, profileDir, dir); > + } catch (ex) { Is it possible to be more explicit here, e.g. look at the exception and make sure it's not something unexpected?

https://reviewboard.mozilla.org/r/45163/#review44933 > Is it possible to be more explicit here, e.g. look at the exception and make sure it's not something unexpected? I'm not sure, here's an example of a failure: https://treeherder.mozilla.org/logviewer.html#?job_id=19683517&repo=try#L3206 It looks to me like the exception contains a generic FAILED code, but I maybe there is some other way that I don't know of to distinguish this error?

https://reviewboard.mozilla.org/r/45163/#review44933 > I'm not sure, here's an example of a failure: https://treeherder.mozilla.org/logviewer.html#?job_id=19683517&repo=try#L3206 > It looks to me like the exception contains a generic FAILED code, but I maybe there is some other way that I don't know of to distinguish this error? Hm well it looks like nsIFile.create() throws more specific exceptions in some cases: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#create() So I expect that it's just throwing a more generic exception in this case, how about running `continue` only these `if ex.name == "NS_ERROR_FAILURE"` so if a more specific exception (permissions etc.) is thrown it isn't masked?

I was not able to install a web-extension without an ID set in the manifest.json. This is happening for old or fresh submitted web-extensions on FF 46.0.1(Win 7) and FF 47.0b3(Win 7); NOT reproducing on FF49.0a1(Win 7). Used web-extension: https://addons-dev.allizom.org/en-US/firefox/addon/flickr-tab-dev/ Screenshot for this issue: http://screencast.com/t/WMjJMV7NPs What should we do in this situation?

We have not uplifted the Firefox fixes up to 46 and 47 (and don't intend to) so add-ons without the id should not work there. We'll put this in the documentation, so that's expected. I noticed you tested on 46, 47 and 49. The most important release for this to happen is 48, could you test that one please?

Verified as fixed on FF48.0a2 (Win 7) Please see the screenshot: http://screencast.com/t/yW297Vjv