Consider allowing installation of unpacked add-ons permanently (if signing verification disabled)

RESOLVED FIXED in Firefox 52
(Needinfo from 2 people)

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: krizsa, Assigned: ochameau, NeedInfo)

Tracking

(Blocks: 3 bugs, {dev-doc-needed})

34 Branch
mozilla52
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Developing add-ons for the new extension API should be simple. For that we need a load extension button and a package extension button from a local directory in the add-on page. Preferably hidden behind some checkbox... We also have to figure out when to enable this, since this is running unsigned extensions essentially.
(Reporter)

Updated

2 years ago
Blocks: 1161828
Priority: -- → P1
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Version: unspecified → 34 Branch
Created attachment 8646681 [details] [diff] [review]
patch

Does this look like the right approach Dave? I suspect there are some cases I'm missing, but I want to make sure I'm not totally off base.

Also, I'm not really sure how we'll address the signing issue. It would be pretty easy to disable signing along this path, but I don't know if it's a good idea :-). I've been testing it by toggling the pref.
Assignee: nobody → wmccloskey
Attachment #8646681 - Flags: feedback?(dtownsend)
Comment on attachment 8646681 [details] [diff] [review]
patch

Review of attachment 8646681 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2951,5 @@
>            }
>          }
>  
>          try {
> +          // FIXME: Do I need to handle the case where this is a dir add-on?

Yeah you probably do

@@ -5571,5 @@
>     *         XPI is incorrectly signed
>     */
>    loadManifest: Task.async(function* AI_loadManifest(file) {
> -    let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
> -                    createInstance(Ci.nsIZipReader);

This change will break things when we try to use the zipreader later
Attachment #8646681 - Flags: feedback?(dtownsend) → feedback+
(In reply to Dave Townsend [:mossop] from comment #2)
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
> @@ +2951,5 @@
> >            }
> >          }
> >  
> >          try {
> > +          // FIXME: Do I need to handle the case where this is a dir add-on?
> 
> Yeah you probably do

Actually, I looked over this code more carefully and I don't think we need to do anything. I think this code will only be invoked for restartless add-ons if we crash during installation. If that happens, I think we'll end up here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2825

That's maybe not the best outcome. However, I believe that we automatically skip unsigned restartless add-ons in the staging directory anyway:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2883

Given that people are very unlikely to be installing signed add-ons via this method, it seems reasonable to me not to change the current behavior. After all, what are the chances that we crash during installation?

> @@ -5571,5 @@
> >     *         XPI is incorrectly signed
> >     */
> >    loadManifest: Task.async(function* AI_loadManifest(file) {
> > -    let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"].
> > -                    createInstance(Ci.nsIZipReader);
> 
> This change will break things when we try to use the zipreader later

Maybe I missed something, but I tried to remove all uses of zipreader below.
(In reply to Bill McCloskey (:billm) from comment #3)
> Actually, I looked over this code more carefully and I don't think we need
> to do anything.

Oh, I forgot that people could install non-restartless add-ons this way :-(.
Blocks: 1209344

Updated

2 years ago
Blocks: 1214433

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Comment 5

2 years ago
Bill, Do you mind if I carry over your patch? I would like to see this feature in Firefox for Devtools needs.
(We are trying to support new development workflow involving an addon to easily reload devtools codebase)

I imagine we mostly miss some test for this?

Also, I was wondering if it make sense to introduce the UI (also or instead) in about:debugging,
which is more developers oriented than about:addons.
Flags: needinfo?(wmccloskey)
(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Also, I was wondering if it make sense to introduce the UI 
> (also or instead) in about:debugging,
> which is more developers oriented than about:addons.

I did not no about about:debug. Is this used by many add-on developers?
I would suggest "also" instead of "instead", if only for Chrome-parity…  ;)
I was hoping to get to this in the next couple days. I'll assign it over to you if I haven't made any progress by the end of next week.
Flags: needinfo?(wmccloskey)
rhelmer has the backend pieces for this almost done in bug 1209341, we probably only need UI hookups in bug 1209344 so I'm not sure if this bug is useful anymore.
(Assignee)

Comment 10

2 years ago
about:debugging just appeared during FF44 cycle.
It only targets Workers and Addons so far, but it should over time allow you to debug more things and especially contexts running on remote devices like apps and tabs on Fennec/FirefoxOS.
We haven't been communicating about this as it is brand new and certainly needs some polish.

If the backend is already ready, I can certainly add the UI in about:debugging, as I already now this codebase.

(In reply to Bill McCloskey (:billm) from comment #8)
> I was hoping to get to this in the next couple days. I'll assign it over to
> you if I haven't made any progress by the end of next week.

Ok. I won't have much time until Monday, so it sounds like a good plan!
Hmm, I didn't realize bug 1209341 will support unpacked add-ons.

I feel like there's value in being able to load unpacked add-ons in a non-temporary way if you disable the signing pref, especially for non-restartless add-ons. But maybe it's not a high priority.
(Assignee)

Comment 12

2 years ago
Let's try to move the "install from file" UI discussion to bug 1209344.
bill, can you close that bug or rename it to something more specific you have in mind?
OK, I re-titled the bug.
Summary: Add GUI elements for loading/packaging extensions from local directories → Consider allowing installation of unpacked add-ons permanently (if signing verification disabled)

Updated

2 years ago
Blocks: 1226743
No longer blocks: 1209344
(Assignee)

Comment 14

2 years ago
I would actually like to see that happen as well.
The usecase is quite simple:
 * you may break firefox with your addon and have to restart it often
 * you may want to see how your addon behaves during firefox startup

Mossop, First, are you ok with that?
Robert, You are not already working on that, right?

If yes, how would we implement this?

Codewise:
- should we make it so that existing installTemporaryAddon function install the addon permanently if the signature pref is off?
- or, should we come up with another function?

UI:
 - should the existing "load temporary addon" be permanent if signature is off?
 - should we display another action in about:debugging, if signature is off to load permanently?
Flags: needinfo?(dtownsend)
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> I would actually like to see that happen as well.
> The usecase is quite simple:
>  * you may break firefox with your addon and have to restart it often
>  * you may want to see how your addon behaves during firefox startup
> 
> Mossop, First, are you ok with that?

Yes I'm fine with this.

> If yes, how would we implement this?
> 
> Codewise:
> - should we make it so that existing installTemporaryAddon function install
> the addon permanently if the signature pref is off?
> - or, should we come up with another function?

I think we should use a different function and here's why. I think the temporary add-ons are something we can make special in ways that I wouldn't want to support for non-temporary add-ons. Things like watching the filesystem and reloading the add-on if something changes. So I'd rather keep them separated and make it possible to still use that even if you're also able to install unsigned add-ons.
Flags: needinfo?(dtownsend)

Updated

a year ago
Whiteboard: triaged

Updated

a year ago
Flags: blocking-webextensions+ → blocking-webextensions-
(Assignee)

Comment 16

11 months ago
Created attachment 8768030 [details] [diff] [review]
patch v1

Here is a tentative patch to do that.
It hacks the existing about:debugging feature to install addons permanently.
More work is needed in about:debugging to switch between temporary/permanent.

Note that working in chrome is simplier, local addons are installed permanently.
So I would like to prevent have to dance between temporary and permanent
and keep firefox as simple.
(Assignee)

Comment 17

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b64ffc993005

Comment 18

11 months ago
Just looping in security to check this is ok.
Flags: needinfo?(dveditz)

Comment 19

8 months ago
Checking in with Kev too on this.

BillM noticing this is assigned to you, make more sense to move to ochameau?
Flags: needinfo?(kev)
Priority: P1 → P2

Updated

8 months ago
Assignee: wmccloskey → poirot.alex
(Assignee)

Comment 20

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a58414a1041
(Assignee)

Comment 21

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f629d27fea74
Comment hidden (mozreview-request)
(Assignee)

Comment 23

8 months ago
Dave, I would like to focus on the AddonManager part of this bug. May be I should open a specific bug?
Here, I don't modify the behavior of about:addons nor about:debugging, I would like to keep that for another patch.

Otherwise, what do you think about handling permanently sideloaded addons via a specific InstallLocation, like what we do for temporary install?
Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

I'm going to let Rob handle your questions here, he owns the add-ons manager now.
Attachment #8797157 - Flags: review?(dtownsend) → review?(rhelmer)

Comment 25

8 months ago
mozreview-review
Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review81438

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4121
(Diff revision 1)
>  
>      addon = XPIDatabase.addAddonMetadata(addon, file.persistentDescriptor);
>  
>      XPIStates.addAddon(addon);
>      XPIDatabase.saveChanges();
> +    //XPIStates.save();

Debugging?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4136
(Diff revision 1)
>      AddonManagerPrivate.callAddonListeners("onInstalled", addon.wrapper);
>  
>      return addon.wrapper;
>    }),
>  
> +/*

This function is commented out, was this an earlier approach to the above replacement of `installTemporaryAddon` (which now calls `installAddonFromSources`?)
Attachment #8797157 - Flags: review?(rhelmer)

Comment 26

8 months ago
mozreview-review
Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review81448

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8846
(Diff revision 1)
>  }
>  
> +const SourceInstallLocation = {
> +  locked: false,
> +  name: KEY_APP_SOURCE,
> +  scope: AddonManager.SCOPE_TEMPORARY,

Hm `SCOPE_TEMPORARY` doesn't seem accurate anymore - not sure if any of the existing ones match exactly what you want to do, `SCOPE_USER` is probably closest.
(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Dave, I would like to focus on the AddonManager part of this bug. May be I
> should open a specific bug?
> Here, I don't modify the behavior of about:addons nor about:debugging, I
> would like to keep that for another patch.

I agree, UI should be handled in a separate patch. Temporary add-on loading is only supported via about:debugging and not about:addons, I'm not sure about where this one should go - this might be appropriate for about:addons gear->"Install Add-on From Folder..." or so to go with the existing "From File". I would be OK with it just being in about:debugging if that makes sense from the devtools POV though.

> Otherwise, what do you think about handling permanently sideloaded addons
> via a specific InstallLocation, like what we do for temporary install?

I see why we need a new API to trigger this since AddonManager supports loading extensions from a directory but only if they are present at startup (AFAICT), but I am not sure why a new InstallLocation is needed - wouldn't MutableDirectoryInstallLocation work?

It makes sense to have TemporaryInstallLocation mostly because there is a lot of behavior we don't wish to implement, like supporting updates. Is this the case too for "from source dir" add-ons, or do you want to support features like updates?

Overall I am fine with this approach, just would like to understand better why this should implement a new InstallLocation.
Comment hidden (mozreview-request)
(Assignee)

Comment 29

8 months ago
Just pushed a new patch addressing what you highlighted. I forgot that I kept work-in-progress things in the patch... I introduce a new dedicated scope.

I understand about the new InstallLocation. TBH, the AddonManager code is quite scary, I didn't knew exactly how to proceed and was very happy once I got a working patch!
I'm happy to go with a simplier alternative.
Wouldn't MutableDirectoryInstallLocation end up deleting the source folder on uninstall?
  http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8387
Also, I know the addon manager does scan the addon folders for modtime or checksum. Do you know if that kicks in with the new InstallLocation? What about the MutableDirectoryInstallLocation?

Regarding the signature, this features forces you to toggle the xpinstall.signatures.required pref to false. The new install location doesn't try to bypass any signature check.

Comment 30

8 months ago
mozreview-review
Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review81540

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4041
(Diff revision 2)
> +   *
>     * @return a Promise that resolves to an Addon object on success, or rejects
>     *         if the add-on is not a valid restartless add-on or if the
> -   *         same ID is already temporarily installed
> +   *         same ID is already installed.
>     */
> -  installTemporaryAddon: Task.async(function*(aFile) {
> +  installAddonFromSources: Task.async(function*(aFile, aInstallLocation = SourceInstallLocation) {

kmag suggested that you could use the existing "proxy file" feature, to avoid having to create a new type of install location:
https://developer.mozilla.org/en-US/Add-ons/Setting_up_extension_development_environment#Firefox_extension_proxy_file

This would require writing a text file to the extension directory in the profile, which contains the path to the addon.

You'd still need a way to activate it without a restart and without having to re-scan the whole extensions dir - I think you could easily modify `AddonInstall.initLocalInstall` (http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5411) to work with directories as well as files.

What do you think?
Attachment #8797157 - Flags: review?(rhelmer)
(Assignee)

Comment 31

8 months ago
I tried the MutableDirectoryInstallLocation and yes, it ends up deleting the original source directory:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8322
installAddon is doing that by trying to move 

I also tried the proxy file route but it forces to modify many things including startInstall:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6306
which stoped me pursuing this. Modifying initLocationInstall was reasonable but startInstall is much less.
(Assignee)

Comment 32

8 months ago
ALT+Space and bugzilla immediately send your comment even if it takes 10s to display any html :x

So. I'm not sure the proxy file is reasonable, it also sounds like a hack.
But I haven't spent too much time with the MutableDirectoryInstallLocation, I may be able to come up with something by still sharing some code with the temporary addon codepath.
(Assignee)

Comment 33

8 months ago
Created attachment 8797800 [details] [diff] [review]
alt patch using MutableDirectoryLocation

Here is an alternative patch that works, based on your suggestion to use MutableDirectoryLocation.
Note that I also used the proxy file trick.
f? to see if I should move forward with this patch instead?
If you agree, it may be good to refactor installAddon to accept a dictionnary as argument?
the aExistingId, aCopy, aSymLink are just messy and would be better agregated into an option object.
Attachment #8797800 - Flags: feedback?(rhelmer)
(Assignee)

Updated

8 months ago
Attachment #8646681 - Attachment is obsolete: true
Comment on attachment 8797800 [details] [diff] [review]
alt patch using MutableDirectoryLocation

This lgtm, although maybe I wouldn't use the term "symlink" if it's not an actual symbolic link and actually a text file, but I see what you mean :)
Attachment #8797800 - Flags: feedback?(rhelmer) → feedback+
(In reply to Alexandre Poirot [:ochameau] from comment #33)
> Created attachment 8797800 [details] [diff] [review]
> alt patch using MutableDirectoryLocation
> 
> Here is an alternative patch that works, based on your suggestion to use
> MutableDirectoryLocation.
> Note that I also used the proxy file trick.
> f? to see if I should move forward with this patch instead?
>
> If you agree, it may be good to refactor installAddon to accept a
> dictionnary as argument?
> the aExistingId, aCopy, aSymLink are just messy and would be better
> agregated into an option object.


Sure.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

8 months ago
mozreview-review
Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review82848

::: toolkit/mozapps/extensions/test/xpcshell/test_install_from_sources.js:6
(Diff revision 4)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +const ID = "bootstrap1@tests.mozilla.org";
> +const sampleRDFManifest = {

Is this used anywhere?
Attachment #8797157 - Flags: review?(rhelmer)

Comment 39

8 months ago
mozreview-review
Comment on attachment 8797157 [details]
Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently.

https://reviewboard.mozilla.org/r/82770/#review82852
Attachment #8797157 - Flags: review+
(Assignee)

Updated

8 months ago
Attachment #8797800 - Attachment is obsolete: true
(Assignee)

Updated

8 months ago
Attachment #8768030 - Attachment is obsolete: true
(Assignee)

Comment 40

8 months ago
(In reply to Robert Helmer [:rhelmer] from comment #38)
> Comment on attachment 8797157 [details]
> Bug 1185460 - Implements AddonManager.installAddonFromSources to install
> addon from sources permanently.
> 
> https://reviewboard.mozilla.org/r/82770/#review82848
> 
> ::: toolkit/mozapps/extensions/test/xpcshell/test_install_from_sources.js:6
> (Diff revision 4)
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/
> > + */
> > +
> > +const ID = "bootstrap1@tests.mozilla.org";
> > +const sampleRDFManifest = {
> 
> Is this used anywhere?

No, I removed it. It's a leftover from a copy paste from test_temporary.js.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 43

8 months ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/839c876d0dab
Implements AddonManager.installAddonFromSources to install addon from sources permanently. r=rhelmer

Comment 44

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/839c876d0dab
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

8 months ago
Blocks: 1309288
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.