1385057 - Remove support for unpacked side-loaded extensions

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement, P2)

Comment 2

[Kris Maglione [:kmag]]

The longer we wait to do this, the harder it's going to become.

Comment 3

[Andy McKay]

Assuming Kris is wanting to move this up in priority, going for a P1.

Comment 4

[Andrew Swan [:aswan]]

what information is needed here?

Comment 5

[Andy McKay]

In a meeting that I think you were in, we assigned it to P1. In comment 1 Kris removed the P3 suggesting it should be a higher priority, but not setting it to one. I set it to P1 in comment 3 and since its on add-ons manager and a P1 I think your input would be helpful on if it is a P1.

Comment 6

[Andy McKay]

(In reply to Andy McKay [:andym] from comment #5)
> In a meeting that I think you were in, we assigned it to P1
s/P1/P3/

Comment 7

[Andrew Swan [:aswan]]

Ah.  I think its an important thing to do but not more important than all the other stuff on my plate for 57.  If Kris has the cycles to do it now, that's great otherwise, realistically, its going to have to wait.

Comment 8

[Andrew Swan [:aswan]]

How are we going to handle compatibility for this?  I just updated MDN but until a few minutes ago the documented method for sideloading on Windows involved unpacking.  Is there any practical way to notify all the authors of extensions that are installed in this way?

Comment 9

[Lakatos.Isti]

I'm asking just out of curiosity, but why shouldn't unpacked extensions be allowed to install via global install locations? I failed to find a concrete reason looking through the related issues.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

I don't think there's good way to identify those developers.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Lakatos.Isti from comment #9)
> I'm asking just out of curiosity, but why shouldn't unpacked extensions be
> allowed to install via global install locations? I failed to find a concrete
> reason looking through the related issues.

We don't support unpacked extensions in production environments anymore. We allow them in development environments, mainly as temporary extensions, but that comes with a performance penalty, and involves code paths we don't want to have to support in production.

Comment 12

[Andrew Swan [:aswan]]

Unassigning myself for now until we have a strategy for how to deploy this.

Comment 13

[Mike Conca [:mconca]]

The plan is to deprecate support for unpacked extensions loaded via the registry with the release of Firefox 62 (currently scheduled for 21-Aug-2018).  A communications plan is being formed to give notice to as many developers as possible during the six-month sunset window.

Comment 14

[Kris Maglione [:kmag]]

These extensions will no longer work after bug 1460350 lands, but that still leaves behind a bunch of dead code for verifying unpacked extensions, which can now go away.

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

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

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248762

Cool - security/ parts lgtm.

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248792

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:458
(Diff revision 1)
>   *        then the uri to the root of the contained files will be returned
>   * @returns {nsIURI}
>   *        An nsIURI pointing at the resource
>   */
>  function getURIForResourceInFile(aFile, aPath) {
> -  if (aFile.exists() && aFile.isDirectory()) {
> +  if (!aFile.leafName.toLowerCase().endsWith(".xpi")) {

What's the purpose of this change?  It is not unheard of for people to install .zip files (which I think you can only do from about:debugging, but people do do that...)

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248792

> What's the purpose of this change?  It is not unheard of for people to install .zip files (which I think you can only do from about:debugging, but people do do that...)

Calling .isFile() is expensive. Pretty sure we don't support installing .zip files. If we do, I expect we'd have tests. We use `.endsWith(".xpi")` or some variant thereof all over the place. But if we do support them, we probably shouldn't.

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248792

> Calling .isFile() is expensive. Pretty sure we don't support installing .zip files. If we do, I expect we'd have tests. We use `.endsWith(".xpi")` or some variant thereof all over the place. But if we do support them, we probably shouldn't.

Like I said it only works from about:debugging (see bug 1353861).
It would be nice to not break it but if isFile()/isDirectory() does a main thread stat() every time it is called then that sucks.  Could we do something like always stick a trailing / on directories and just check that?

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248792

> Like I said it only works from about:debugging (see bug 1353861).
> It would be nice to not break it but if isFile()/isDirectory() does a main thread stat() every time it is called then that sucks.  Could we do something like always stick a trailing / on directories and just check that?

Not really.

Honestly, supporting .zip files as extensions is probably a misfeature that I could not care less about.

That said, this probably doesn't break them. Or, at least, it probably doesn't break WebExtensions. It may break bootstrapped extensions, but I care even less about supporting .zip bootstrapped extensions.

If it *does* break them, then there should be tests that fail.

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248928

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update.js:44
(Diff revision 1)
>        minVersion: "1",
>        maxVersion: "1"
>      }],
>      name: "Test Delay Update Ignore",
> -  }, profileDir, IGNORE_ID, "bootstrap.js");
> +  }, profileDir, IGNORE_ID, {
> +    "bootstrap.js": String.raw`

Looks like `data/test_delay_*/bootstrap.js` can be removed now

::: toolkit/mozapps/extensions/test/xpcshell/test_proxy.js:13
(Diff revision 1)
>  
>  BootstrapMonitor.init();
>  
>  // Ensure that a proxy file to an add-on with a valid manifest works.
>  add_task(async function() {
> +  if (AppConstants.MOZ_REQUIRE_SIGNING) {

Looks like this can just be done from xpcshell.ini?

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:174
(Diff revision 1)
>          }),
>          "bootstrap.js": bootstrapJS,
>        };
>  
>        let target;
> -      if (packed) {
> +      if (!packed) {

ugh

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
(Diff revision 1)
>  tags = blocklist
>  [test_gmpProvider.js]
>  skip-if = appname != "firefox"
>  [test_harness.js]
>  [test_install.js]
> -[test_install_from_sources.js]

I'm in favor of just removing that function altogether.  But not a fan or removing the test and leaving untested code sitting around...

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review248928

> Looks like `data/test_delay_*/bootstrap.js` can be removed now

Yeah, sorry, I meant to remove them.

> Looks like this can just be done from xpcshell.ini?

Oh, yeah. I couldn't find any tests that used it, but we have mozconfig.require_signing.

> I'm in favor of just removing that function altogether.  But not a fan or removing the test and leaving untested code sitting around...

Oh. I thought it was still used internally, but I guess not. Yay. More code to delete.

Comment 23

[Kris Maglione [:kmag]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242912/diff/1-2/

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review249372

Comment 25

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6caf1d4d23bc139562493ffbb8c0f199ed0b30
Bug 1385057: Remove most code for handling unpacked side-loaded extensions. r=aswan,keeler

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ad6caf1d4d23

Comment 27

[marius.santa]

Attachment: sideload unpacked blocked.gif

Tested and reproduced issue on Firefox 61.0a1 (20180329222832).
Retested and verified in Firefox 62.0a1 (20180522100110) on Windows 10 x64 and Ubuntu 16.04 LTS.

Comment 28

[Will Bamberg [:wbamberg]]

:aswan, I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Alternative_distribution_options/Add-ons_in_the_enterprise#Installation_using_the_Windows_registry to try to clarify that the old way isn't supported, but AFAICT the docs only talk about packaged extensions already.

Let me know if there's anything else needed here.

Comment 29

[Andrew Swan [:aswan]]

Looks good, thanks!