271097 - searchplugin auto update should ask user

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Axel Hecht]

Currently, searchplugins automatically update without getting a confirmation for
that download and install from the user.
This raises some privacy concerns, as the updated searchplugin could send user
data to a different server which could gather userdata that way.

There are at least two ways to do this.

One would be by adding a third line to
Extras - Preferences - Extended - Software Update
like
[ ] searchplugins
and check that pref before updating.

The next one would be to pop up a dialog, or a toolbar or something, if a 
update is available, and ask a "do you want to do now, not, [] don't ask again"
standard UI dialog.

Comment 1

[Daniel Veditz [:dveditz]]

One advantage for the global pref, from a privacy perspective, is that it would
stop any server pings that are not user-initiated. If a paranoid user turned it
off then only explicitly initiated searches would go to the search server
(which, as we've recently found, has its own privacy issues).

For the per-update dialog to be bearable it'd have to come up after we've
already checked and found an update available. Asking every 3 days whether we
can check for updates (times number of plugins) would quickly get the dialog
turned off, in which case there's not much point. I guess the truly paranoid
could simply remove the search plugins to stop the pings, after they find out
that's what we're doing.

The global option would be easier to implement, and easier for users to fit into
a consistent mental model of "things that check themselves for updates".

Or maybe we have to do both.

Comment 2

[Sandor Benczik]

Apart from privacy issues, there is another point in asking for update
confirmation. I edited one of the search plugins to better suit my needs and
today I found out to my surprize that it got overwritten by the original
version. (I cannot tell how happy I was.) It is the more strange, as the
searchplugin itself was not updated on mozdev.org for more than 8 months. Next
time I will rename the src file, but can I be sure that nobody will come up with
the same name for some future plugin?!?

Comment 3

[Sandor Benczik]

OK, i got it, please disregard previous post. If user is nosy enough to modify
plugins, he better remove the Update, UpdateIcon, and UpdateCheckDays entries
from the BROWSER section as well.

Comment 4

[Vectorspace]

I agree, User permission should be required.

I had modified not the src files, but the gif files (so I could tell the
different google plugins apart by the icon) and they replaced themselves with
the originals yesterday.

Comment 5

[Asa Dotzler [:asa]]

Who can own this issue? We really should fix it for 1.1

Comment 6

[Doug]

(In reply to comment #5)
> Who can own this issue? We really should fix it for 1.1

Agreed; this issue of self-updating software is infuriating, especially with 
extensions, but this is such a simple thing (on the face of it), and to 
overwrite local customisations is unforgiveable.

Comment 7

[O. Atsushi (Torisugari)]

IMHO, the ultimate blocker of this bug would be bug 191642

And if I (or somebody else) put the pref under
[Tools] > [Options] > [Advanced] > [Update], 
it might be misunderstanding to some extent.

A third party search plugin author can distribute his/her plugin
as a xpi package, but that pref has nothing to do with
Software Update nor Extension/Theme Update...

Comment 8

[O. Atsushi (Torisugari)]

Attachment: Patch

checkbox whether or not to send ping.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Mike+Mike: we need to decide what, if anything, we do with this in 1.5.  One
thing we want to think about is how behaviour is different between
user-installed search plugins (through the low-privilege installSearchPlugin
interface) and those that are pre-installed by us/localizers or by extensions.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm inclined to agree with the reporter. There's two UE problems here:
 1) we don't tell users anywhere in the UI that we update these things
 2) we don't give them a place to control that behaviour

Adding a confirmation dialog would be both irritating (and thus just ignored
quickly) and inconsistent with how we do other forms of updates. There's a
single area in the prefs for handling All Things Updating, and it seems that's
the best place for a direct control of this sort of thing. Default should be set
to "on". Here's some ASCII art:

.---------------------------------------------------------------.
|General|Update|Security|                                       |
|-------|      |------------------------------------------------|
|                                                               |
| Automatically check for updates to                            |
|   [x] Firefox                         [ Check now ...]        |
|   [x] Installed extensions & themes   [ Check now ...]        |
|   [x] Installed search engines        [ Check now ...]        |
|                                                               |
| When updates to Firefox are found,                            |
|   ( ) Ask me what I want to do                                |
|   (o) Automatically download and install the update           |
|        [x] Warn me if this will disable extensions or themes  |              
|                                                               |
| [Show Update History]                                         |
'---------------------------------------------------------------'

This doesn't cover the manual update of a searchplugin case, but IMO that's
pretty consistent with how we update anything (ie: extensions & themes.) 

note: mconnor and I have a list of things we want to do to better manage search
and searchplugins for 2.0, and this has been added to it.

Comment 11

[Doug]

I'm happy with separating updates to the Firefox core from updates to 
extensions, but I don't see why search plugins are treated differently from 
other extensions. For instance, why is it acceptable to install search plugins 
from any site, but other extensions require whitelisting? I appreciate the 
different level of access to the OS that search plugins have, but in terms of 
potential for abuse of privacy, its just as concerning, and this issue is all 
about privacy.
Treat them the same, call them all extensions, then there could be an option 
for each installed extension to autoupdate built in to the FF core. With those 
extensions I trust (or am interested in bleeding edge) I can enable 
autoupdate, for others, I won't.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #11)
> I'm happy with separating updates to the Firefox core from updates to 
> extensions, but I don't see why search plugins are treated differently from 
> other extensions. For instance, why is it acceptable to install search plugins 

Because they're not extensions, and aren't managed by the Extension Manager,
which as I mentioned about is a known problem but not fully containable for 1.5.
We can't simply equate them with extensions without doing a bunch of work to
give them proper treatment in the extension manager, and even then, it gets
confusing when you consider that searchplugins can be packaged as extensions.

Better searchplugin management to get it on par with themes & extensions is
something mconnor and I will be looking at for the next release.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 190943 [details] [diff] [review]
Patch


>+<!ENTITY enableSearchUpdate.label       "Search Plugins">
>+<!ENTITY enableSearchUpdate.accesskey   "P">

I guess Search Engines is better here, beltzner?

> InternetSearchDataSource::validateEngine(nsIRDFResource *engine)
> {
> 	nsresult	rv;
> 
>+	// confirm whether the user wants to update plugins.
>+	PRBool userAllowed = PR_TRUE;
>+	nsCOMPtr<nsIPrefBranch>
>+		prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>+	NS_ENSURE_SUCCESS(rv, rv);
>+
>+	rv = prefBranch->GetBoolPref("browser.search.update", &userAllowed);
>+	// if the pref value is not set or wrong type, don't stop here.
>+	NS_ENSURE_TRUE((NS_FAILED(rv) || userAllowed), NS_OK);
>+

Please move the PRBool decl after getting the prefbranch.  I really don't think
we want a noisy return just because someone disables the pref, so please change
the NS_ENSURE_TRUE.  What I think we want is:

if (NS_SUCCEEDED(rv) && !userAllowed)
  return NS_OK;

Comment 14

[O. Atsushi (Torisugari)]

Attachment: UI patch for Firefox

This patch has l10n impact.

Should I write a patch for "Check Now"?
It's hard, if possible, to be in 1.5, imho.

Comment 15

[O. Atsushi (Torisugari)]

Comment on attachment 192383 [details] [diff] [review]
UI patch for Firefox

Oops, accesskey confilct.
Sorry for bug spam.

Comment 16

[O. Atsushi (Torisugari)]

Attachment: UI patch for Firefox

Every char in "Search Engines" other than "h" and "i" is
already assinged. And according to
http://www.mozilla.org/access/keyboard/accesskey
, both don't seem good accesskeys.

However, -> "h"

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #14)
> Should I write a patch for "Check Now"?
> It's hard, if possible, to be in 1.5, imho.

If you can make it for 1.5, it'd be great to have for the sake of consistency.
Not *required* for this patch to land, though, IMO. Just kinda looks less
professional without it.

Comment 18

[Mike Connor [:mconnor]]

Check now would be nice, but the current service isn't set up to do that, and
the value is outweighed by the size/risk of a patch to make the service do that.
 Another log for the "Burn search to the ground" fire.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Is the update for the searchplugins kicked off only by the periodic update
check, or is it part of the extensions & themes check? One option is to do
something like:

 Automatically check for updates to
   [x] Firefox                         [ Check now ...]
   [x] Installed extensions & themes   [ Check now ...]
       [x] including search engines

(But I think that only makes sense / ends up making for better UI if the search
engines are part of the extension update, as otherwise we end up saying that
searchplugins are more closely associated with the core app than with extensions
.. ugh)

Comment 20

[O. Atsushi (Torisugari)]

Attachment: Patch for Core (no l10n impact)

comment #13 addressed.

Comment 21

[O. Atsushi (Torisugari)]

(In reply to comment #19)
> Is the update for the searchplugins kicked off only by the periodic 
> update check, or is it part of the extensions & themes check?

It has nothing to do with the extension update, at the moment.

But we can install search plugin via extension manager (this is a new
feature in 1.5). In that case, double update check would be held, as
they are completely independent from each other.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #21)
> It has nothing to do with the extension update, at the moment.

OK, forget it, then. Let's go with what we have.

Comment 23

[Chris Beard]

Comment on attachment 192395 [details] [diff] [review]
Patch for Core (no l10n impact)

r=shaver

Comment 24

[Mike Connor [:mconnor]]

bug 304100 is not really related to this issue, and the current updateURL is
mozilla.org anyway.

both patches landed, marking FIXED, thanks!