Closed Bug 1194784 Opened 4 years ago Closed 4 years ago

Sideloaded add-ons without full signing don't display an appropriate warning in the add-ons manager

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox41 + verified
firefox42 + verified
firefox43 + verified

People

(Reporter: drew, Assigned: mossop)

References

Details

(Keywords: uiwanted)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150812163655

Steps to reproduce:

As an example, within Firefox stable, I had a version of an extension installed that was not signed by Mozilla.  I then installed a version of the extension that was signed by Mozilla over top of it.  I then closed Firefox stable, opened Firefox beta, and went to about:addons.  


Actual results:

The extension didn't have the "could not be verified for use in Firefox and has been disabled" message, which was correct since it was signed by Mozilla.  However, it was disabled, and didn't have an "Enable" button.  I had to completely uninstall and reinstall the extension to get it out of this state.


Expected results:

The extension shouldn't have been disabled.

I haven't been able to reproduce thus far, but it's issues like these that make me doubt this stuff will be ready in less than 6 weeks.
Was this a specific extension? Did it need a restart to install and did you do that restart with Firefox stable or just quit and start beta?
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
Version: 40 Branch → unspecified
Yes, LastPass, which still does require a restart to install.  I'm fairly certain I let stable restart to finish the installation before closing and starting beta.  I just tried reproducing both ways, though, with no luck.
I've done some testing around this and while I found bug 1196797 I've not been able to spot the kind of thing you're talking about, nor have I heard of any similar reports. Unfortunately without more information to go on, ideally a way to reproduce we can't make much more progress here so I'm closing this out.
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
In the process of testing unsigned => signed upgrade this morning, I managed to reproduce this again (on 2 computers).  Please see the screenshot I just attached.  LastPass is disabled, with no button to enable, and no error message.

Unfortunately, I'm still unable to reproduce this reliably.  However, I still have a Firefox profile where it's happening.  Is there anything I can do to debug this on my side?  I see it as rather serious.
Ok, I think I've gotten to the bottom of this, and it's related to side-loading.  If you download the following signed version of LastPass:

https://lastpass.com/lp.xpi

Then, sideload it by creating a support@lastpass.com directory within your Firefox profile's extensions directory

Then, run Firefox beta, it will end up in this state (the usual prompt to enable the extension doesn't show up).
Thanks for figuring that out. The root problem here is that in order to sideload an add-on it must have undergone full review and the XPI you link to has only been preliminarily reviewed. So Firefox is disabling it for that reason, but we're not displaying an appropriate message in the UI here so that need to be fixed.

Markus, what kind of message should we use here, just the regular message about the add-on not being signed properly? That would save us l10n for the uplift.
Flags: needinfo?(mjaritz)
Keywords: uiwanted
Summary: I'm occasionally seeing some seriously buggy behavior with regards to signing. → Sideloaded add-ons without full signing don't display an appropriate warning in the add-ons manager
Ugh, that's unfortunate.  This is a beta channel XPI from a fully reviewed extension.  Is it expected that beta channel XPIs from fully reviewed extensions only get preliminary review status, and thus aren't able to be side-loaded?  If so, is that documented anywhere?  It's quite unfortunate to not find this out until now, especially since Jorge is the one who made the recommendation to use the beta channel to work around the issue of not being able to sign both a listed and an unlisted version of an extension with the same ID.  This was an important part of our signing strategy going forward, and it sounds like it's not going to work.

What about the case where an extension has been sideloaded sometime in the distant past, has successfully upgraded to a signed extension, but will now be disabled because it was sideloaded at some point?  Is that really the behavior you want?  As far as I can tell, it will force us to upgrade all our users to a fully-reviewed build, since we don't have a way to determine whether they sideloaded or installed via web.
(In reply to Andrew Zitnay from comment #8)
> Ugh, that's unfortunate.  This is a beta channel XPI from a fully reviewed
> extension.  Is it expected that beta channel XPIs from fully reviewed
> extensions only get preliminary review status, and thus aren't able to be
> side-loaded?  If so, is that documented anywhere?  It's quite unfortunate to
> not find this out until now, especially since Jorge is the one who made the
> recommendation to use the beta channel to work around the issue of not being
> able to sign both a listed and an unlisted version of an extension with the
> same ID.  This was an important part of our signing strategy going forward,
> and it sounds like it's not going to work.

I'm not sure of the details here, Jorge?

> What about the case where an extension has been sideloaded sometime in the
> distant past, has successfully upgraded to a signed extension, but will now
> be disabled because it was sideloaded at some point?  Is that really the
> behavior you want?  As far as I can tell, it will force us to upgrade all
> our users to a fully-reviewed build, since we don't have a way to determine
> whether they sideloaded or installed via web.

As long as it updated to a fully reviewed add-on then it should work.
Flags: needinfo?(jorge)
(In reply to Dave Townsend [:mossop] from comment #9)
> As long as it updated to a fully reviewed add-on then it should work.

I understand that, but our plan wasn't to upgrade everyone to a fully reviewed extension, since they take far too long to be reviewed.  We want to keep as many people as possible on our beta channel, but I don't see how we can do that, given that they might have originally sideloaded, even years in the past.
(In reply to Dave Townsend [:mossop] from comment #9)
> (In reply to Andrew Zitnay from comment #8)
> > Ugh, that's unfortunate.  This is a beta channel XPI from a fully reviewed
> > extension.  Is it expected that beta channel XPIs from fully reviewed
> > extensions only get preliminary review status, and thus aren't able to be
> > side-loaded?  If so, is that documented anywhere?  It's quite unfortunate to
> > not find this out until now, especially since Jorge is the one who made the
> > recommendation to use the beta channel to work around the issue of not being
> > able to sign both a listed and an unlisted version of an extension with the
> > same ID.  This was an important part of our signing strategy going forward,
> > and it sounds like it's not going to work.
> 
> I'm not sure of the details here, Jorge?

I suggested beta versions as a workaround for AMO not supporting simultaneous listed and unlisted versions for the same add-on ID. That was wrong on my part, since beta versions can't have an updateURL (given they are listed). I hadn't even considered the side-loading part, but that's another limitation, yes. We can't give beta versions side-loading privileges because they aren't code-reviewed.
Flags: needinfo?(jorge)
This bug screwed me over pretty badly...  A few months ago, back when Nightly was first displaying signing warnings in about:addons, I tested sideloading a beta channel XPI.  Everything worked as expected, and I didn't see a message in about:addons due to this bug, so I thought we were good.

Due to this, we'll probably need a fully reviewed build rushed through our support@lastpass.com listed submission before signing is enforced on 9/22, so we can be sure anyone who's sideloaded over the past 7 years gets upgraded to a fully reviewed build.  I'm hoping you guys are amenable to that.  I have made numerous changes over the past few weeks to work around some of the more serious AMO validation warnings, and I'm currently attempting to have it reviewed on our dev@lastpass.com unlisted submission (which is of course taking forever, because the unlisted queue is far from empty).
(In reply to Dave Townsend [:mossop] from comment #7)
> Markus, what kind of message should we use here, just the regular message
> about the add-on not being signed properly? That would save us l10n for the
> uplift.

Yes, the regular message should be fine. Thanks.
Flags: needinfo?(mjaritz)
Reopening to fix the warning
Assignee: nobody → dtownsend
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
[Tracking Requested - why for this release]:

This should be tracked too, it can give users and developers misleading messaging about whether an add-on is verified or not
Tracked, we do want to iron out all issues related to add-on signing.
Attached patch patchSplinter Review
Attachment #8656764 - Flags: review?(dao)
Comment on attachment 8656764 [details] [diff] [review]
patch

>--- a/toolkit/mozapps/extensions/test/browser/browser_list.js
>+++ b/toolkit/mozapps/extensions/test/browser/browser_list.js
>@@ -441,41 +455,19 @@ add_task(function*() {
>   is_element_hidden(get_node(addon, "error"), "Error message should be hidden");
>   is_element_hidden(get_node(addon, "error-link"), "Error link should be hidden");
>   is_element_hidden(get_node(addon, "pending"), "Pending message should be hidden");
> 
>   info("Filter for disabled unsigned extensions");
>   let filterButton = gManagerWindow.document.getElementById("show-disabled-unsigned-extensions");
>   let showAllButton = gManagerWindow.document.getElementById("show-all-extensions");
>   let signingInfoUI = gManagerWindow.document.getElementById("disabled-unsigned-addons-info");
>-  is_element_visible(filterButton, "Button for showing disabled unsigned extensions should be visible");
>+  is_element_hidden(filterButton, "Button for showing disabled unsigned extensions should be hidden");
>   is_element_hidden(showAllButton, "Button for showing all extensions should be hidden");
>   is_element_hidden(signingInfoUI, "Signing info UI should be hidden");
>-
>-  filterButton.click();
>-
>-  yield new Promise(resolve => wait_for_view_load(gManagerWindow, resolve));
>-
>-  is_element_hidden(filterButton, "Button for showing disabled unsigned extensions should be hidden");
>-  is_element_visible(showAllButton, "Button for showing all extensions should be visible");
>-  is_element_visible(signingInfoUI, "Signing info UI should be visible");
>-
>-  items = get_test_items();
>-  is(Object.keys(items).length, 1, "Only one add-on should be shown");
>-  is(Object.keys(items)[0], "Test add-on 11", "The disabled unsigned extension should be shown");
>-
>-  showAllButton.click();
>-
>-  yield new Promise(resolve => wait_for_view_load(gManagerWindow, resolve));
>-
>-  items = get_test_items();
>-  is(Object.keys(items).length, 11, "All add-ons should be shown again");
>-  is_element_visible(filterButton, "Button for showing disabled unsigned extensions should be visible again");
>-  is_element_hidden(showAllButton, "Button for showing all extensions should be hidden again");
>-  is_element_hidden(signingInfoUI, "Signing info UI should be hidden again");
> });

Not sure why filterButton should now be hidden here. But if so, please adjust info("Filter for disabled unsigned extensions");.
Attachment #8656764 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/3d945fc88b01
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
So this is only fixed in Firefox 43?  That seems unfortunate, unless the signing enforcement timeline has changed.
Comment on attachment 8656764 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Add-on signing
[User impact if declined]: Users may see confusing messaging in the add-ons manager when sideloaded add-ons are disabled for not being fully signed
[Describe test coverage new/current, TreeHerder]: On mozilla-central with automated tests running
[Risks and why]: Low risk, this is a straightforward change to some logic, well contained to the add-ons manager UI
[String/UUID change made/needed]: None
Attachment #8656764 - Flags: approval-mozilla-beta?
Attachment #8656764 - Flags: approval-mozilla-aurora?
(In reply to Andrew Zitnay from comment #21)
> So this is only fixed in Firefox 43?  That seems unfortunate, unless the
> signing enforcement timeline has changed.

We always let fixes sit on the main development branch for at least a few days before attempting uplift to the other channels.
While testing, I also noticed that nightly doesn't seem to display the "One or more installed add-ons cannot be verified and have been disabled" notification bar for a sideloaded extension without full signing.
(In reply to Andrew Zitnay from comment #24)
> While testing, I also noticed that nightly doesn't seem to display the "One
> or more installed add-ons cannot be verified and have been disabled"
> notification bar for a sideloaded extension without full signing.

This patch is meant to take care of that, but please file a bug if not
I'm not filing another bug, but I didn't see it when testing.
Comment on attachment 8656764 [details] [diff] [review]
patch

Given that add on signing is required by default in FF41, we need to uplift to Beta41 and Aurora42. It's good to see automated tests.
Attachment #8656764 - Flags: approval-mozilla-beta?
Attachment #8656764 - Flags: approval-mozilla-beta+
Attachment #8656764 - Flags: approval-mozilla-aurora?
Attachment #8656764 - Flags: approval-mozilla-aurora+
Assigning for verification in Nightly 43 and Beta 41.
Flags: qe-verify+
QA Contact: vasilica.mihasca
I was able to reproduce this issue on Firefox 43.0a1 (2015-08-24) using GNotifier preliminarily reviewed add-on under Windows 7 64-bit.

Verified fixed on Firefox 43.0a1 (2015-09-13), Firefox 42.0a2 (2015-09-14) and Firefox 41 Beta 9 (20150910171927) under Windows 7 64-bit and Ubuntu 14.04 32-bit. The add-on is disabled and accompanied by the regular message in about:addons.

But, I have noticed that the yellow notification bar, about disabling some of the unsigned add-ons does not appear in this case. Is this an issue or the notification bar shouldn’t be displayed in this situation?

And here is another question: A preliminarily reviewed add-on is disabled in Add-ons manager if it is installed via sideloaded method, but it can be successfully installed from addons.mozilla.org. Why there are different behaviors for the same add-on?
Flags: needinfo?(dtownsend)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #31)
> I was able to reproduce this issue on Firefox 43.0a1 (2015-08-24) using
> GNotifier preliminarily reviewed add-on under Windows 7 64-bit.
> 
> Verified fixed on Firefox 43.0a1 (2015-09-13), Firefox 42.0a2 (2015-09-14)
> and Firefox 41 Beta 9 (20150910171927) under Windows 7 64-bit and Ubuntu
> 14.04 32-bit. The add-on is disabled and accompanied by the regular message
> in about:addons.
> 
> But, I have noticed that the yellow notification bar, about disabling some
> of the unsigned add-ons does not appear in this case. Is this an issue or
> the notification bar shouldn’t be displayed in this situation?

Please file a bug with STR.

> And here is another question: A preliminarily reviewed add-on is disabled in
> Add-ons manager if it is installed via sideloaded method, but it can be
> successfully installed from addons.mozilla.org. Why there are different
> behaviors for the same add-on?

Because the policy is that an add-on has to be fully reviewed before it can be sideloaded into the app.
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #32)

> Please file a bug with STR.

Filed Bug 1204814

Based on my previous testing from Comment 31, I am marking this bug as Verified since the other issue is tracked separately.
You need to log in before you can comment on or make changes to this bug.