xpinstall.enabled=false still allows to install xpi via. addon search

VERIFIED FIXED in Firefox 24

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23 wontfix, firefox24+ verified, firefox25 verified, firefox26 verified)

Details

(Whiteboard: [tracked b/c of ESR])

Attachments

(1 attachment, 7 obsolete attachments)

+++ This bug was initially created as a clone of Bug #668999 +++

xpinstall.enabled=false still allows to install xpi via. addon search toolbar.
No longer depends on: 668999
Posted patch v1 (obsolete) — Splinter Review
Blair, can you check this one please?
Attachment #688731 - Flags: feedback?(bmcbride)
Is it planned to solve this bug fast ?
I experienced this problem too, and would be very interrested in a fix.
I test your patch, which is working perfectly for me.
I compile it with 17.0.1esrpre.
Can a developper look at this issue and tell us if it can be introduced in a future release ?
Thanks !
Comment on attachment 688731 [details] [diff] [review]
v1

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

Thanks, and sorry again for the delay - as I mentioned in your other bug, I've been on extended leave.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +143,5 @@
>      gViewController.loadView(aViewId);
>    }
>  }
>  
> +function isXPIEnabled() {

This is going to end up being used in a variety of places, so could you implement this as a property of AddonManager/AddonManagerPrivate in AddonManager.jsm? Bug 715787 did the same for other similar properties for prefs, so should be a good guide. That bug also added the test_pref_properties.js test, which can be modified to test this.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +565,5 @@
>                  if (this.mControl.getAttribute("remote") != "true")
>                    break;
>  
>                  this._progress.hidden = true;
> +                showInstallRemote = isXPIEnabled();

With installs disabled, the remote search feature is pretty useless - it becomes UI noise. So rather than disabling the install button, remote searches should be disabled entirely (see gSearchView).
Attachment #688731 - Flags: feedback?(bmcbride) → feedback-
Posted patch v2 (obsolete) — Splinter Review
The updated one, utilizes AddonManager.jsm. If you're okay with that I'll write the test for it.
Attachment #688731 - Attachment is obsolete: true
Attachment #735248 - Flags: feedback?(bmcbride)
Comment on attachment 735248 [details] [diff] [review]
v2

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

Ugh, I just realised we're going about that API the wrong way :( Sorry, I should have realised this earlier.

The trouble is, we support many different types of add-ons. And it doesn't make sense for xpinstall.enabled to control all of those types - eg, we don't want lightweight themes to obey that pref. The Add-ons Manager UI doesn't really care about the different add-on types... except remote search, since we can safely assume (not now, at least) that AMO only hosts XPI-style add-ons. But as it turns out, we already have an API that handles this in a way that supports different providers - isInstallEnabled(). And XPIProvider already looks at xpinstall.enabled for the return value of that API.

So what we really want is...

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +25,5 @@
>  const PREF_EM_CERT_CHECKATTRIBUTES    = "extensions.hotfix.cert.checkAttributes";
>  const PREF_EM_HOTFIX_CERTS            = "extensions.hotfix.certs.";
>  const PREF_MATCH_OS_LOCALE            = "intl.locale.matchOS";
>  const PREF_SELECTED_LOCALE            = "general.useragent.locale";
> +const PREF_XPI_ENABLED                = "xpinstall.enabled";

Undo all changes in this file.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +974,5 @@
>      },
>  
>      cmd_installItem: {
>        isEnabled: function cmd_installItem_isEnabled(aAddon) {
> +        if (!aAddon || !AddonManager.extensionInstallEnabled)

Undo this change - with remote search disabled, this can't reached anyway (also, if it were possible, it's the wrong way to fix it - see bug 860186).

@@ +2015,5 @@
>      this._listBox = document.getElementById("search-list");
>      this._emptyNotice = document.getElementById("search-list-empty");
>      this._allResultsLink = document.getElementById("search-allresults-link");
>  
> +    if (!AddonManager.extensionInstallEnabled) {

This check should be:
  if (!AddonManager.isInstallEnabled("application/x-xpinstall")) {

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +565,5 @@
>                  if (this.mControl.getAttribute("remote") != "true")
>                    break;
>  
>                  this._progress.hidden = true;
> +                showInstallRemote = AddonManager.extensionInstallEnabled;

Undo this change - with remote search disabled, this can't be reached (and again, wrong way to fix it anyway).
Attachment #735248 - Flags: feedback?(bmcbride) → feedback-
It is ok for me. Maybe can you test it on you side before integrating this patch ?
(In reply to dcoutadeur from comment #7)
> It is ok for me. Maybe can you test it on you side before integrating this
> patch ?

Erm, I'm struggling to find context for this comment.
Posted patch v3 (obsolete) — Splinter Review
So something like this, right? Which kind of test do you want me to provide for this patch? Thanks!
Attachment #735248 - Attachment is obsolete: true
Attachment #736708 - Flags: feedback?(bmcbride)
I was just saying I had tested the latter patch on my test platform, and it was ok for me.
So if it is ok for you, it is ok for me !
Thank you for your work !
Comment on attachment 736708 [details] [diff] [review]
v3

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

Yep :)

Just need to modify the following test to check remote search option isn't visible:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_searching.js
Attachment #736708 - Flags: feedback?(bmcbride) → feedback+
Posted patch v3+tests (obsolete) — Splinter Review
A patch with tests.
Attachment #738500 - Flags: review?(bmcbride)
Posted patch v4+tests (obsolete) — Splinter Review
Sorry, better one.
Attachment #738500 - Attachment is obsolete: true
Attachment #738500 - Flags: review?(bmcbride)
Attachment #738512 - Flags: review?(bmcbride)
Attachment #738512 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/fx-team/rev/95067eec6bde

(Removing the sec whiteboard flag that was carried over by cloning bug 668999 - I don't think this needs any security review, as its not meant to fix the underlying issue of bug 668999.)
Status: NEW → ASSIGNED
Flags: in-testsuite+
Whiteboard: [sec-assigned:dveditz]
Attachment #736708 - Attachment is obsolete: true
Looks like this might be causing intermittent mochitest b-c failures on fx-team?
Backed out because we suspect it to be the source of mochitest-bc failures.

https://hg.mozilla.org/integration/fx-team/rev/81309cf14f61
Can you be more specific which tests fail? I did the testing with browser-chrome mochitest and with the patch applied I see only some extra failures in chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindow.js:

69:56.17 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindow.js | can't do any tests without this width
69:56.17 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindow.js | collapsed state as promised

But this one fails with or without the patch applied, only difference is that with the patch there are two extra ones.
This would be really nice to have in the next ESR. This is a major hole for enterprises blocking XPI installs.
I'll look at those mochitest failures, I see that on try builds.
Mochi failures:

08:25:59  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_eula.js | Should see the search result in the list
08:25:59  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_eula.js | uncaught exception - TypeError: parent is null at chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_eula.js:33
08:26:59  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_eula.js | Test timed out
08:27:00  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_eula.js | Should not have seen an install of http://example.com/browser/toolkit/mozapps/extensions/test/browser/addons/browser_install1_2.xpi in state 0
08:27:00  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_eula.js | Found a tab after previous test timed out: about:addons
My guess would be that somehow the xpinstall.enabled is still set to false after the tests run. That would then break the subsequent tests.
The mochitest failure seems to be caused by this._filter.selectedIndex = 0; - searching in installed addons only. Still investigating.
Whiteboard: [tracked b/c of ESR]
Any updates Martin? Let us know if you would like us to find another assignee.
Sorry for the delay, I'll try to finish it this week.
Posted patch wip v5 (obsolete) — Splinter Review
This one does not change the mentioned filter selectedIndex parameter. Unfortunately try servers are busted now.
Comment on attachment 771355 [details] [diff] [review]
wip v5

Try looks good. Blair, can you please check this one?
Attachment #771355 - Flags: review?(bmcbride)
Comment on attachment 771355 [details] [diff] [review]
wip v5

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

Sorry for the delay, been swamped with deadlines.
Attachment #771355 - Flags: review?(bmcbride) → review+
Posted patch v5 for check-in (obsolete) — Splinter Review
Thanks!
Attachment #792162 - Flags: checkin?
Keywords: checkin-needed
Attachment #738512 - Attachment is obsolete: true
Attachment #771355 - Attachment is obsolete: true
Attachment #792162 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/ebd79cd25bf5
Keywords: checkin-needed
Whiteboard: [tracked b/c of ESR] → [tracked b/c of ESR][fixed-in-fx-team]
Attachment #792162 - Flags: checkin+
Hi Martin any updates here ? Based on comment#30 looks like the change was backed out due to test failure ? Did you get a chance to investigate it ? We are close to shipping Firefox 24 and ESR in the next few weeks, and will be freezing on taking any code change later than this week.

Please feel free to needinfo me or if there is anything we can help with.
Flags: needinfo?(stransky)
I'm working on that. I'll do my best to catch the deadline.
Flags: needinfo?(stransky)
Posted patch patch, v6Splinter Review
The latest patch with updated tests. The search buttons are unused and hidden so we should not test them.

mochitest-bc try for this patch: https://tbpl.mozilla.org/?tree=Try&rev=3d51eb7a9a4b
Attachment #792162 - Attachment is obsolete: true
Attachment #796633 - Flags: review?(bmcbride)
Attachment #796633 - Flags: review?(bmcbride) → review+
Duplicate of this bug: 905786
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/01dbf8d69852
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [tracked b/c of ESR][fixed-in-fx-team] → [tracked b/c of ESR]
Target Milestone: --- → mozilla26
Needinfo'ing :Unfocused, Martin Stránský  here to request uplift if this is low risk enough and we have had the needed testing and baketime.We go to build with our second last beta tomorrow morning which will be the last opportunity to get this in Fx 24 or ESR .
Flags: needinfo?(stransky)
Flags: needinfo?(bmcbride)
Comment on attachment 796633 [details] [diff] [review]
patch, v6

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Blocking bug 720362.
User impact if declined: Mostly affects ESR, where's it's more likely that enterprises will want to disable extension installs. This fixes one of the major holes that still allows installing extensions. This is controlled via a hidden preference thhat's typically only used in enterprise environments - anyone not using that hidden preference will be unaffected by this change.
Testing completed (on m-c, etc.): m-c for a week, no known regressions. Covered by a browser-chrome test.
Risk to taking this patch (and alternatives if risky): Only real risk involved is when the hidden preference extensions.xpinstall is set to false. Not much in-the-field testing of this yet, since it's so uncommon (except in enterprise environments).
String or IDL/UUID changes made by this patch: None
Attachment #796633 - Flags: approval-mozilla-beta?
Attachment #796633 - Flags: approval-mozilla-aurora?
Eep, time flies.
Flags: needinfo?(stransky)
Flags: needinfo?(bmcbride)
Comment on attachment 796633 [details] [diff] [review]
patch, v6

Approving so this can get landed in our next beta going to build tomorrow.please land asap.

Also assigning QA contact as :Matt to help with some adhoc testing here  since this affects the esr mainly.
Attachment #796633 - Flags: approval-mozilla-beta?
Attachment #796633 - Flags: approval-mozilla-beta+
Attachment #796633 - Flags: approval-mozilla-aurora?
Attachment #796633 - Flags: approval-mozilla-aurora+
QA Contact: mwobensmith
(In reply to Blair McBride [:Unfocused] from comment #38)
> Comment on attachment 796633 [details] [diff] [review]
> patch, v6
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Blocking bug 720362.
> User impact if declined: Mostly affects ESR, where's it's more likely that
> enterprises will want to disable extension installs. This fixes one of the
> major holes that still allows installing extensions. This is controlled via
> a hidden preference thhat's typically only used in enterprise environments -
> anyone not using that hidden preference will be unaffected by this change.
> Testing completed (on m-c, etc.): m-c for a week, no known regressions.
> Covered by a browser-chrome test.
> Risk to taking this patch (and alternatives if risky): Only real risk
> involved is when the hidden preference extensions.xpinstall is set to false.
Can you please help QA of possible test cases/ideas that this change may affect ?
> Not much in-the-field testing of this yet, since it's so uncommon (except in
> enterprise environments).
> String or IDL/UUID changes made by this patch: None
Flags: needinfo?(bmcbride)
Also this affects enterprise users the most, request to all esr users on this bug to help verify the patch in our upcoming beta.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e15ddba58dc
https://hg.mozilla.org/releases/mozilla-beta/rev/ff5ae3399276


(In reply to Blair McBride [:Unfocused] from comment #38)
> the hidden preference extensions.xpinstall is set to false

Ugh, massive typo/brain-fart. The hidden preference is xpinstall.enabled, and it needs to be set to *true* to affect this change.


(In reply to bhavana bajaj [:bajaj] from comment #41)
> Can you please help QA of possible test cases/ideas that this change may
> affect ?

In theory, this is already covered by the browser-chrome test, but since it's so rarely used in practice:
1) Open Add-ons Manager
2) Search for an add-on that's listed on AMO (and is compatible)
3) Should be able to switch between "My Add-ons" and "Available Add-ons", and in "Available Add-ons", there should be an install button for each add-on listed - clicking that should install the add-on.
4) Close Add-ons Manager
5) Open about:config, set xpinstall.enabled to true (preference will not exist by default)
6) Open Add-ons Manager
7) Search for an add-on that's listed on AMO (and is compatible)
8) Should have *no* option to switch between "My Add-ons" and "Available Add-ons", and list of results should only display any matching add-on that's currently installed. This means there should be no way to install an add-on from the search results.
9) Close Add-ons Manager
10) In about:config, set xpinstall.enabled to false
11) repeat steps 1-4

To explicitly point it out: This is only a UI fix involving search in the Add-ons Manager UI. It doesn't prevent other ways of installing add-ons, such as via installing an XPI file from the local hard drive.
Additionally:

1) Open Add-ons Manager
2) Search for an add-on that's listed on AMO (and is compatible)
3) Click "Available Add-ons" so it's selected, then close Add-ons Manager
4) Open Add-ons Manager
5) Search again
6) "Available Add-ons" should be pre-selected
7) Close Add-ons Manager
8) Open about:config, set xpinstall.enabled to true (preference will not exist by default)
9) Open Add-ons Manager
10) Search for an add-on that's listed on AMO (and is compatible)
11) Should have *no* option to switch between "My Add-ons" and "Available Add-ons", and list of results should only display any matching add-on that's currently installed (as though "My Add-ons was what was selected)
(In reply to Blair McBride [:Unfocused] from comment #43)
> To explicitly point it out: This is only a UI fix involving search in the
> Add-ons Manager UI. It doesn't prevent other ways of installing add-ons,
> such as via installing an XPI file from the local hard drive

IIRC The other ways are also covered (at least installation from local hard drive).
Hi all -

I've finished testing, and all looks good. Many thanks for the tests in comments 43 and 44.

FWIW, I can only assume that the steps that indicate "xpinstall.enabled=true" are inverted. Obviously, setting this pref to true does not disable XPI install. I assume you meant false.

In any case, my testing confirms that setting this pref to false disables the UI for browsing new add-ons as per scenarios above, as well as prevents installing XPIs locally. Setting it back to true re-enables XPI install.

Confirmed bad behavior on FF23.
Verified fixed on FF24, 25 and 26, builds from 2013-09-10.
(In reply to Matt Wobensmith from comment #46)
> FWIW, I can only assume that the steps that indicate
> "xpinstall.enabled=true" are inverted. Obviously, setting this pref to true
> does not disable XPI install. I assume you meant false.

*sigh* Yes. Please excuse my brain.
You need to log in before you can comment on or make changes to this bug.