293826 - whitelist for software install remains empty until visiting a site triggers the notification bar

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[sairuh (rarely reading bugmail)]

found using 200505111x-1.0.x branch ffox builds on all platforms.

0. start with a fresh profile.

1. open Options/Prefs > Web Features > Allow websites to install software.

results: noticed that the whitelist is empty.

2. visit a site containing downloadable extensions, eg:

http://www.extensionsmirror.nl/
ftp://mozilla.isc.org/pub/mozilla.org/extensions/

3. click on a downloadable link.

4. when the software warning/notification bar appears, click Edit Options.

results: the whitelist is no longer empty and contains addons.mozilla.org and
update.mozilla.org --which it should have listed back at step (1)!

Comment 1

[sairuh (rarely reading bugmail)]

fwiw, this was also a problem in ffox 1.0.3.

need to investigate to see if this is also a problem in the trunk...

Comment 2

[sairuh (rarely reading bugmail)]

yes, this also occurs on the trunk. tested with 2005051011-trunk bits on fc3.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

The values of the xpinstall.whitelist.add and xpinstall.whitelist.add.103
default preferences get written to the whitelist in
nsInstallTrigger::AllowInstall():

http://lxr.mozilla.org/mozilla/source/xpinstall/src/nsInstallTrigger.cpp#349

Presumably, AllowInstall only gets called when the user tries to install an
extension, which is why the whitelist for a fresh profile is empty until then.

To fix the bug, we need to make the preferences dialog box update the whitelist
and blacklist when it gets opened just as AllowInstall() does when installing
software.  Alternately, you could make the app update the whitelist when it
starts to ensure that no alternate way of viewing the whitelist becomes subject
to the same problem.

Note that this bug is minor, since it doesn't prevent the whitelist from working
as intended, and few users will check the whitelist in their preferences or get
confused because our addons site isn't on it.

Comment 4

[Daniel Veditz [:dveditz]]

This was as intended, mostly. (it's certainly not blocking any branch security
release.) The xpinstall component is not normally loaded because installing is a
sometimes thing, and loading all the time just to detect preference changes
would be quite a bit of bloat.

Other options would be to move "install" permission logic into the permission
manager itself but that kind of fights its design as a generic service. Or, as
myk suggested, make the pref-reading code callable from the outside, and then
have the allow sites dialog call it during load.

That last is the best potential solution should we actually care about this
case. Very similar to the way the popup sitelist pref-read code used to work in
the Suite (and still might, dunno about that).

Comment 5

[Johnny Stenback (:jst)]

As intended or not, this is IMO confusing and can lead people to thinking that
their whitelist is empty when it really isn't. At the very least we should make
the preferences UI code trigger what's needed in XPInstall to get the whitelist
prefilled so that the dialog always reflects reality, anything else is IMO
simply broken and misleading.

I for one would love to see this fixed in 1.0.5.

Comment 6

[Marcia Knous [:marcia]]

I agree with jst's comments. As an end user, this is quite confusing.

(In reply to comment #5)
> As intended or not, this is IMO confusing and can lead people to thinking that
> their whitelist is empty when it really isn't. At the very least we should make
> the preferences UI code trigger what's needed in XPInstall to get the whitelist
> prefilled so that the dialog always reflects reality, anything else is IMO
> simply broken and misleading.
> 
> I for one would love to see this fixed in 1.0.5.

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch

A pretty simple patch since the code for adding perms is already available in
permissions.js. If nsIPermissionManager hasMoreElements returns false in init
for install permissions this reads the 3 prefs and updates the whitelist or
blacklist accordingly before showing the ui.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 184469 [details] [diff] [review]
patch

Teach me for not reading the code more... new patch coming up

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: better patch

I didn't realize the prefs are a comma separated list of hosts and this takes
that into account.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 184473 [details] [diff] [review]
better patch

Looks OK to me but I'm not a peer for the front-end code.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 184473 [details] [diff] [review]
better patch

>diff -u -r1.4 permissions.js

I see I haven't given you the -u8 or better speech yet.  Context is good!

>@@ -335,6 +341,50 @@
>     }  
>   },
>   
>+  _updateWhitelist: function ()

This should be _updatePermissions, to match nsSoftwareUpdate (though the
comment still refers to updateWhitelist there).

>+  {
>+    var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                            .getService(Components.interfaces.nsIIOService);
>+    var pbi = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefBranch2);

nit: indentation, the periods should align when breaking lines like this

getService can throw, of course, if this is called in startup we'll break the
UI.  Of course, it can be argued that by this point if either of these are
going to throw, we're already long-broken, or should know that these are
failing.  You are wrapping the bits that make use of these interfaces in a
try/catch already, maybe this should be wrapped as well?  Or the initial call
should wrap the whole function if its okay to fail (which I think it is, for
this case).

>+    var prefList = [["xpinstall.whitelist.add", nsIPermissionManager.ALLOW_ACTION],
>+                    ["xpinstall.whitelist.add.103", nsIPermissionManager.ALLOW_ACTION],
>+                    ["xpinstall.blacklist.add", nsIPermissionManager.DENY_ACTION]];
>+
>+    for (var i = 0; i < prefList.length; ++i) {
>+      try {
>+        var hosts = pbi.getCharPref(prefList[i][0]);
>+      } catch (e) {
>+        continue;
>+      }
>+
>+      hostList = hosts.split(",");
>+      var capability = prefList[i][1];
>+      for (var x = 0; x < hostList.length; ++x)

use j here, not x, to fit better with toolkit convention.

>+        try {
>+          var uri = ioService.newURI("http://" + host, null, null);
>+          host = uri.host;
>+
>+          var capabilityString = this._getCapabilityString(capability);
>+          var p = new Permission(host, 
>+                                 (host.charAt(0) == ".") ? host.substring(1,host.length) : host, 
>+                                 this._type, 
>+                                 capabilityString,
>+                                 capability);
>+          uri.spec = p.host;
>+          this._pm.add(uri, p.type, p.perm);

this code doesn't make sense to me.  You create a uri, use uri.host to create
the Permission object, then use p.host to set uri.spec (never do that to create
a URI, we fixed most/all of those recently).  All you should need is the
following:

host = (host.charAt(0) == ".") ? host.substring(1,host.length) : host;
var uri = ioService.newURI("http://" + host, null, null);
this._pm.add(uri, this._type, capability);

it seems other places in this code do similar bits...

>+          pbi.setCharPref(prefList[i][0], "");

this gets called once for each host in the pref, which I don't think you
intended, move it after the nested loop please

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch

Addresses mconnor's comments.

I removed the following since I am unable to think of a case where this would
apply to the values in the pref... is there one?
host = (host.charAt(0) == ".") ? host.substring(1,host.length) : host;

Thanks for the review!

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 185437 [details] [diff] [review]
patch

if we're going to fix up bad pref values, I'd rather do it completely, or not
at all, but this matches the existing implementation in nsSoftwareUpdate.

Comment 14

[Robert Strong (they/them - no direct email)]

Fixed on the trunk

Comment 15

[timeless]

Comment on attachment 185437 [details] [diff] [review]
patch

mozilla/browser/components/preferences/permissions.js	1.6