Closed Bug 1385057 Opened 2 years ago Closed 2 years ago

Remove support for unpacked side-loaded extensions

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox62 --- verified

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug, )

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(2 files)

No description provided.
Duplicate of this bug: 1385058
Priority: -- → P3
The longer we wait to do this, the harder it's going to become.
Priority: P3 → --
Assuming Kris is wanting to move this up in priority, going for a P1.
Flags: needinfo?(aswan)
what information is needed here?
Flags: needinfo?(aswan) → needinfo?(amckay)
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.
Flags: needinfo?(amckay)
(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/
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.
Priority: -- → P2
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?
Assignee: nobody → aswan
Flags: needinfo?(jorge)
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.
I don't think there's good way to identify those developers.
Flags: needinfo?(jorge)
(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.
Unassigning myself for now until we have a strategy for how to deploy this.
Assignee: aswan → nobody
Flags: needinfo?(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.
Flags: needinfo?(mconca)
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.
Assignee: nobody → kmaglione+bmo
Summary: Don't allow unpacked WebExtensions in global (e.g., registry) install locations → Remove support for unpacked side-loaded extensions
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.
Attachment #8974573 - Flags: review?(dkeeler) → review+
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 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 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 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 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 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 on attachment 8974573 [details]
Bug 1385057: Remove most code for handling unpacked side-loaded extensions.

https://reviewboard.mozilla.org/r/242912/#review249372
Attachment #8974573 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6caf1d4d23bc139562493ffbb8c0f199ed0b30
Bug 1385057: Remove most code for handling unpacked side-loaded extensions. r=aswan,keeler
https://hg.mozilla.org/mozilla-central/rev/ad6caf1d4d23
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1461308
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.
Status: RESOLVED → VERIFIED
Depends on: 1463625
Depends on: 1462500
No longer depends on: 1463625
: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.
Flags: needinfo?(aswan)
Looks good, thanks!
Flags: needinfo?(aswan)
Duplicate of this bug: 1493069
You need to log in before you can comment on or make changes to this bug.