feat: `web-ext sign` now supports extensions without explicit IDs #340

kumar303 · 2016-06-30T19:46:10Z

Supports #178

This feature also requires the patch from mozilla/sign-addon#114

andymckay · 2016-06-30T20:39:50Z

If I sign the add-on a second (or third or whatever) time, how do I specify the add-on id? Do I have to change the manifest, or could I specify an id on the command line as an argument?

kumar303 · 2016-07-01T15:53:02Z

Hmm, good point. As it stands now, each time you run the sign command it will create a new add-on which will start to make your DevHub add-on list pretty unbearable.

kumar303 · 2016-07-01T18:06:16Z

Good catch @andymckay ✋ fixed

rpl · 2016-07-01T19:46:50Z

src/cmd/sign.js

+        .then(({buildResult, manifestData}) => {
+          const manifestId = getManifestId(manifestData);
+          if (id && manifestId) {
+            log.warn(`Overriding manifest.json ID: ${id}`);


@kumar303 How about Overriding manifest.json ID "${id}" with "${manifestId}"?

rpl · 2016-07-01T19:57:00Z

@kumar303 how about the following tweak to the strategy used for signing the updates of an add-on without an extension id in the manifest?

warn the user when he submit a webextension without an extension id for the first time
save the addon id from the first successful signed xpi in an hidden file in the root of the add-on source dir
then we can use the hidden file to detect when the addon has been signed once, and even reuse it to automatically sign the next add-on XPIs with the same id, without the need for the user to remember to pass it to the sign command on every released update (and if the user re-creates its development environment, then it will be warned on the first web-ext sign, because the hidden file is missing in the newly created development environment)

kumar303 · 2016-07-01T20:55:22Z

Per our IRC conversation, I checked what AMO does in the case of a manifest.json and custom declared ID. It chooses the manifest.json ID so I made web-ext raise an error for that case (to avoid surprises!).

coveralls · 2016-07-01T20:56:57Z

Coverage remained the same at 100.0% when pulling 0b6b53f on kumar303:sign-without-id into e706145 on mozilla:master.

kumar303 · 2016-07-01T22:36:31Z

BTW, this is testable on https://addons-dev.allizom.org like web-ext sign --api-key ... --api-secret ... --api-url-prefix https://addons-dev.allizom.org/api/v3

kumar303 · 2016-07-01T22:39:02Z

@rpl really great idea about remembering the auto-generated ID! I added this feature. What it does is save a file to the artifacts directory. It will re-use the file if it exists but only as a fallback. Let me know how it looks.

rpl · 2016-07-04T12:52:14Z

src/cmd/sign.js

        .then((signingResult) => {
          // All information about the downloaded files would have
          // already been logged by signAddon().
-          log.info(signingResult.success ? 'SUCCESS' : 'FAIL');
+          if (signingResult.success) {
+            log.info(`Extension ID: ${signingResult.id}`);


@kumar303 at line 74, it seems to me that in some cases the id property can be missing from the signingResult (my guess is that it could happen when the extension id is assigned statically in the manifest.json), and in that case the above line is going to print a "Extension ID: undefined" log, which can be confusing for the the user.

If the above assumptions are true, we can probably tweak it a bit, to ensure that it is going to always print a log which contains the id (even if it is the same statically assigned in the manifest.json)

Did you install the latest sign-addon from this branch into web-ext? mozilla/sign-addon#114 What you're describing is the behavior before that patch is applied but not after.

rpl · 2016-07-04T13:09:59Z

@kumar303 besides the above comment (and the doubt below), this PR looks great now!

My only remaining doubt is potential side-effects of saving the auto-generated ID in the "artifact dir vs the project root":

my feeling is that is some cases, the addon developer is going to have its own building steps (e.g. for babel, webpack or browserify etc.) and it is pretty common that one of those building steps could remove the entire artifact dir (e.g. at every full rebuild), and the saved auto-generated ID will be lost (and a new one will be generated on the next web-ext sign run)

Given that the auto-generated ID is not a sensible information (and any user can get the addon id on an existent add-on pretty easily), I will probably prefer if it could be saved as an hidden file in the root of the project, because it ensures that it will not be removed by a build automation script (and by making it an hidden file, web-ext build will not include it in the generated XPI).

It is probably even reasonable to assume that the addon developer wants to add and commit the hidden file in the repository of the add-on sources, so that any of the developer that should be able to re-sign the addon will have access to the currently assigned id (but it can be overriden from the command line, if they need to test it as a separate add-on)

kumar303 · 2016-07-05T16:50:39Z

Ah, true, good point. I will make it a hidden file in the source dir. I was a little unsure about this when working on it. I think the artifacts dir is more ephemeral.

rpl · 2016-07-05T19:33:36Z

@kumar303 this PR looks great

r+

coveralls · 2016-07-05T19:53:35Z

Coverage remained the same at 100.0% when pulling fdf3d3b on kumar303:sign-without-id into ea378e5 on mozilla:master.

kumar303 added the state: pull request ready label Jun 30, 2016

kumar303 assigned rpl Jun 30, 2016

kumar303 force-pushed the sign-without-id branch 2 times, most recently from 32df77a to b0f238a Compare July 1, 2016 18:17

rpl reviewed Jul 1, 2016
View reviewed changes

kumar303 force-pushed the sign-without-id branch from b0f238a to 0b6b53f Compare July 1, 2016 20:54

kumar303 force-pushed the sign-without-id branch from 0b6b53f to 5196286 Compare July 1, 2016 22:39

rpl reviewed Jul 4, 2016
View reviewed changes

kumar303 force-pushed the sign-without-id branch 2 times, most recently from a62dda4 to a3b56f3 Compare July 5, 2016 19:19

kumar303 force-pushed the sign-without-id branch 3 times, most recently from c8978c5 to f911460 Compare July 5, 2016 19:47

feat: web-ext sign now supports extensions without explicit IDs

fdf3d3b

kumar303 force-pushed the sign-without-id branch from f911460 to fdf3d3b Compare July 5, 2016 19:49

kumar303 merged commit 21855ae into mozilla:master Jul 5, 2016

kumar303 deleted the sign-without-id branch July 5, 2016 20:06

kumar303 removed the state: pull request ready label Jul 5, 2016

kumar303 mentioned this pull request Jul 8, 2016

fix: Fixed incorrect help message for web-ext sign --id #356

Merged

This was referenced Jul 17, 2020

[Snyk] Security upgrade sign-addon from 0.3.1 to 1.0.0 TheLOGOGuy/web-ext#25

Open

[Snyk] Security upgrade sign-addon from 0.3.1 to 1.0.0 sirinartk/web-ext#7

Open

[Snyk] Security upgrade sign-addon from 0.3.1 to 1.0.0 dongseok1985/web-ext#11

Open

snyk-bot mentioned this pull request Oct 6, 2022

[Snyk] Fix for 47 vulnerabilities TheLOGOGuy/web-ext#60

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `web-ext sign` now supports extensions without explicit IDs #340

feat: `web-ext sign` now supports extensions without explicit IDs #340

kumar303 commented Jun 30, 2016 •

edited

Loading

andymckay commented Jun 30, 2016

kumar303 commented Jul 1, 2016

kumar303 commented Jul 1, 2016

rpl Jul 1, 2016

rpl commented Jul 1, 2016

kumar303 commented Jul 1, 2016

coveralls commented Jul 1, 2016 •

edited

Loading

kumar303 commented Jul 1, 2016

kumar303 commented Jul 1, 2016 •

edited

Loading

rpl Jul 4, 2016

kumar303 Jul 5, 2016

rpl commented Jul 4, 2016

kumar303 commented Jul 5, 2016

rpl commented Jul 5, 2016

coveralls commented Jul 5, 2016 •

edited

Loading

feat: web-ext sign now supports extensions without explicit IDs #340

feat: web-ext sign now supports extensions without explicit IDs #340

Conversation

kumar303 commented Jun 30, 2016 • edited Loading

andymckay commented Jun 30, 2016

kumar303 commented Jul 1, 2016

kumar303 commented Jul 1, 2016

rpl Jul 1, 2016

Choose a reason for hiding this comment

rpl commented Jul 1, 2016

kumar303 commented Jul 1, 2016

coveralls commented Jul 1, 2016 • edited Loading

kumar303 commented Jul 1, 2016

kumar303 commented Jul 1, 2016 • edited Loading

rpl Jul 4, 2016

Choose a reason for hiding this comment

kumar303 Jul 5, 2016

Choose a reason for hiding this comment

rpl commented Jul 4, 2016

kumar303 commented Jul 5, 2016

rpl commented Jul 5, 2016

coveralls commented Jul 5, 2016 • edited Loading

feat: `web-ext sign` now supports extensions without explicit IDs #340

feat: `web-ext sign` now supports extensions without explicit IDs #340

kumar303 commented Jun 30, 2016 •

edited

Loading

coveralls commented Jul 1, 2016 •

edited

Loading

kumar303 commented Jul 1, 2016 •

edited

Loading

coveralls commented Jul 5, 2016 •

edited

Loading