1446571 - Remove support for update.rdf signing

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Kris Maglione [:kmag]]

This feature is essentially dead code, and maintaining tests for it is more trouble than it's worth. WebExtensions don't support specifying an update key, legacy extensions are no longer supported, and the update.rdf format is deprecated, in any case. It's time to remove this code.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1446571: Part 1 - Remove support for signed update.rdf files.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1446571: Part 2a - Stop throwing raw numeric values in https.js.

This isn't strictly related, but I ran into it for the nth time while updating
tests, and I got fed up with having my tests fail with a useless numeric value
with no indication of where it came from.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1446571: Part 2b - Update update tests to run sequentially.

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

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8959739 [details]
Bug 1446571: Part 2b - Update update tests to run sequentially.

https://reviewboard.mozilla.org/r/228590/#review234382


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/xpcshell/head.js:39
(Diff revision 1)
> -      server.stop(resolve);
> -    });
> -  });
> -
> -  return server;
>  }

Error: Missing semicolon. [eslint: semi]

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8959738 [details]
Bug 1446571: Part 2a - Stop throwing raw numeric values in https.js.

https://reviewboard.mozilla.org/r/228588/#review234474

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8959739 [details]
Bug 1446571: Part 2b - Update update tests to run sequentially.

https://reviewboard.mozilla.org/r/228590/#review234476

nice, thank you!

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8959739 [details]
Bug 1446571: Part 2b - Update update tests to run sequentially.

https://reviewboard.mozilla.org/r/228590/#review234666

::: commit-message-42b43:1
(Diff revision 1)
> +Bug 1446571: Part 2b - Update update tests to run sequentially. r?aswan

This description is wrong

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8959739 [details]
Bug 1446571: Part 2b - Update update tests to run sequentially.

https://reviewboard.mozilla.org/r/228590/#review234666

> This description is wrong

Yes, I noticed :p

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8959737 [details]
Bug 1446571: Part 1 - Remove support for signed update.rdf files.

https://reviewboard.mozilla.org/r/228586/#review234676

::: toolkit/mozapps/extensions/test/xpcshell/test_updatecheck.js:51
(Diff revision 1)
>  /*
>   * Tests that the security checks are applied correctly
>   *
>   * Test     signature   updateHash  updateLink   expected
>   *--------------------------------------------------------
>   * 2        absent      absent      http         fail
>   * 3        broken      absent      http         fail
>   * 4        correct     absent      http         no update
>   * 5        correct     sha1        http         update
>   * 6        corrent     absent      https        update
>   * 7        corrent     sha1        https        update
>   * 8        corrent     md2         http         no update
>   * 9        corrent     md2         https        update
>   */

Can we replace all the non-https test cases with a single "update fails if the update link is not https" test and get rid of a bunch of associated cruft

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8959737 [details]
Bug 1446571: Part 1 - Remove support for signed update.rdf files.

https://reviewboard.mozilla.org/r/228586/#review234676

> Can we replace all the non-https test cases with a single "update fails if the update link is not https" test and get rid of a bunch of associated cruft

Yes, but I was planning to do that as part of a larger refactor to all of the update tests when we remove support for non-restartless extensions. These simplifications just give us an easier starting point for that work.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8959737 [details]
Bug 1446571: Part 1 - Remove support for signed update.rdf files.

https://reviewboard.mozilla.org/r/228586/#review234676

> Yes, but I was planning to do that as part of a larger refactor to all of the update tests when we remove support for non-restartless extensions. These simplifications just give us an easier starting point for that work.

works for me

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8959737 [details]
Bug 1446571: Part 1 - Remove support for signed update.rdf files.

https://reviewboard.mozilla.org/r/228586/#review234716

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86939af02a1b6350f24fa54203df8ad234a10443
Bug 1446571: Part 1 - Remove support for signed update.rdf files. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1de28c600148249a6b0bd24dcadbc026890fb0
Bug 1446571: Part 2a - Stop throwing raw numeric values in https.js. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/063a77201774afe587f91a1a96f799814544ef50
Bug 1446571: Part 2b - Update update tests to run in parallel. r=aswan

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dadfe9ba6b93128b1de15a23b9954eb4405ad443
Bug 1446571: Add missing semicolon. r=eslint-fix DONTBUILD CLOSED TREE

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfea8df6eaef3e3830c533d9d3c3c52e4147630
Bug 1446571: Follow-up: Fix test that expected raw numeric errors. r=bustage

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afbc519c38563f44f66949df98706ebf4bf13555
Bug 1446571: Follow-up: Fix tests that try to initialize AddonTestUtils twice. r=bustage DONTBUILD

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d1f86caeaf0a7dd9356bae8d5d6fe059d29c30
Bug 1446571: Follow-up: Fix sync tests that misuse resources from add-on manager tests. r=bustage

Comment 18

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a95c9add03af42a289d00dda46bb021f51930254
Bug 1446571: Follow-up: Fix another test that expected raw numeric errors. r=bustage DONTBUILD

Comment 19

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/86939af02a1b
https://hg.mozilla.org/mozilla-central/rev/cf1de28c6001
https://hg.mozilla.org/mozilla-central/rev/063a77201774
https://hg.mozilla.org/mozilla-central/rev/dadfe9ba6b93
https://hg.mozilla.org/mozilla-central/rev/6cfea8df6eae
https://hg.mozilla.org/mozilla-central/rev/afbc519c3856
https://hg.mozilla.org/mozilla-central/rev/c2d1f86caeaf
https://hg.mozilla.org/mozilla-central/rev/a95c9add03af

Comment 20

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

Comment 21

[Kris Maglione [:kmag]]

This is just a removal of code that hasn't been usable for a long time.