Closed Bug 288054 Opened 20 years ago Closed 19 years ago

Unchecking "Allow web sites to install software" causes software update to hang

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Version:
1.7 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: chase, Assigned: dveditz)

References

Details

(Keywords: fixed1.8, late-l10n, Whiteboard: [asaP1][l10n impact])

Attachments

(1 file, 6 obsolete files)

label change as requested
3.16 KB, patch
dveditz
: review+
dveditz
: superreview+
asa
: approval1.8b5+
Details | Diff | Splinter Review 
Having "Allow web sites to install software" unchecked before starting the
update process causes auto-update to hang with no feedback given to the user
that the wait at the screen following selecting the update to install will be
indefinite.

Since there is a separate setting for whether or not to update Firefox, the
setting for "Allow web sites to install software" probably should not affect
whether or not an update can proceed if the process is initiated from the level
of the application.
Flags: blocking-aviary1.1?
Setting blocking-aviary1.1? for triage.
Change "auto-update" in the summary to "software update".
Summary: Unchecking "Allow web sites to install software" causes auto-update to hang → Unchecking "Allow web sites to install software" causes software update to hang
Javascript console shows:

Error: this.appUpdater has no properties
Source File: nsUpdateService.js

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/update/src/nsUpdateService.js.in#573
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Whiteboard: [asaP1]
*** Bug 284904 has been marked as a duplicate of this bug. ***
*** Bug 288086 has been marked as a duplicate of this bug. ***
*** Bug 261039 has been marked as a duplicate of this bug. ***
see also bug 248486
*** Bug 294098 has been marked as a duplicate of this bug. ***
At the very least, the "Firefox Update" dialog box should be smart enough to say
"This procedure can not work if the 'Allow web sites to install software' option
is disabled!"
It appears that nsXPInstallManager::InitManagerFromChrome is where this occurs
and it has a check for whether the "Allow web sites to install software" pref
(e.g. xpinstall.enabled) is enabled. There are two callers both of which appear
to be initiated from wizards that are user driven so this check should at the
least be performed earlier if the prevention of installs is desired when a user
initiates an extension / application update or installs a plugin. It does not
appear that this comes into play for automatic updates.
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/plugins/content/pluginInstallerService.js#51
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/content/update.js#910

If the pref check in the following was removed it may not be acceptable in the
case where it is desired to not allow plugin installation. In the case of
extensions and the application this is only called for software updates and
there are different prefs for extensions / application updates so the pref that
is checked isn't applicable.
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsXPInstallManager.cpp#148
Attached patch patch (obsolete) — — Splinter Review 
From what I can tell from the code the prevention of updates to extensions /
applications and installation of plugins should either not happen when a user
initiates it or it should be prevented earlier on. This is a simple patch which
allows user initiated. If it is preferable to prevent user initiated updates
and installs I suspect a new method would be needed to separate plugin installs
from extensions / application updates and a much more significant patch would
be required to then prevent it earlier on.
Attachment #183986 - Flags: review?(dveditz)
*** Bug 295495 has been marked as a duplicate of this bug. ***
Bringing in plugin-finder people: looking at the code they have a similar
problem of assuming XPInstall is always on. People writing code making that kind
of assumption is the main reason I'm reluctant to go with this patch -- if a
user turns off XPInstall it should be turned off, not "it's mostly off unless
you do x,y, or z".

InitManagerFromChrome() shouldn't return NS_OK if xpinstall is turned off, then
callers can't deal with it. Current callers of InitManagerFromChrome, however,
aren't checking for errors (granted, OOM is rare and we'll probably will die
elsewhere anyway).

Chase makes a good point that update has it's own enabling permission and it
should "just work". There's no pref to turn off the plugin finder, but the user
will have made several explicit choices to start a download; it should "just
work" too.

If we go with this patch's approach you should also remove the #define
PREF_XPINSTALL_ENABLED
(In reply to comment #13)
> Bringing in plugin-finder people: looking at the code they have a similar
> problem of assuming XPInstall is always on. People writing code making that kind
> of assumption is the main reason I'm reluctant to go with this patch -- if a
> user turns off XPInstall it should be turned off, not "it's mostly off unless
> you do x,y, or z".
> 
> InitManagerFromChrome() shouldn't return NS_OK if xpinstall is turned off, then
> callers can't deal with it. Current callers of InitManagerFromChrome, however,
> aren't checking for errors (granted, OOM is rare and we'll probably will die
> elsewhere anyway).
> 
> Chase makes a good point that update has it's own enabling permission and it
> should "just work". There's no pref to turn off the plugin finder, but the user
> will have made several explicit choices to start a download; it should "just
> work" too.
> 
> If we go with this patch's approach you should also remove the #define
> PREF_XPINSTALL_ENABLED

Actually, the user can turn off the plugin finder and get the old null plugin,
unless that was removed and I missed it.

I can make the plugin finder honor the pref and not show any UI if it is unset
(we have a bug filed on this exact issue actually).  This would also solve the
request for a way to disable the PFS UI without adding a new pref or UI.

The pref is labeled "allow websites to...", so software update probably
shouldn't care about the pref.  Plugins are requested by the website though.
no, the old netscape plugin finder couldn't be turned off. if you were lucky and
didn't have the old plugin finder (or replaced the null plugin w/ n4's) then
that's another story.
*** Bug 300546 has been marked as a duplicate of this bug. ***
(In reply to comment #14)
> The pref is labeled "allow websites to...", so software update probably
> shouldn't care about the pref.

The UI says that, but that's a misunderstanding or wishful thinking. The pref
it's flipping is xpinstall.enabled which simply means "turn off the install engine"

That said, the app software update has it's own pref and should still work based
on that.
*** Bug 298122 has been marked as a duplicate of this bug. ***
*** Bug 284992 has been marked as a duplicate of this bug. ***
Comment on attachment 183986 [details] [diff] [review]
patch

This just makes me nervous. Could you add a boolean argument to turn off the
checking? That way callers take an explicit "I know what I'm doing" action that
we can search for rather than distributing the permission checks out to all the
possible callers.
I could but it seems redundant... extension update calls this and extensions
always require the user to initiate an update installation. There was bug 304357
where it would have allowed sites to bypass this but that is now fixed so this
is only called when updating. That leaves the pfs where I am unsure of the
desired flow though I do know it will reach this point after going through its
wizard and then fail in the middle of the process.

If you really want the extra parm I'll add it but I think if someone more
familiar with pfs reviews it we will find that the only code paths are where the
user has initiated the install.

Personally, I believe we need the ui to honor the pref and allow the user to
open ui to change the pref if it isn't locked so this can be locked down
administratively, etc. when it is desired to not allow plugin / extension
installation per the end of comment #10.
Attachment #183986 - Attachment is obsolete: true
Attachment #183986 - Flags: review?(dveditz)
Attached patch patch in progress (obsolete) — — Splinter Review 
This fixes the extension update wizard that runs on upgrade. If the
xpinstall.enabled pref is false and locked it shows the incompatible extensions
and when the check is performed it goes to the finish page with text explaining
that software installation has been administratively disabled. If the
xpinstall.enabled pref is false and not locked it displays a checkbox on the
update found page to change the pref.

This does not fix the EM or the Plugin Finder Service wizard.
*** Bug 289274 has been marked as a duplicate of this bug. ***
Attached patch patch for the update wizard (obsolete) — — Splinter Review 
This fixes the extension update wizard that only runs on upgrade. I would
prefer to address the three areas affected separately since they all have
unique requirements, etc.
Attachment #192589 - Attachment is obsolete: true
Attachment #192639 - Flags: review?(benjamin)
Attachment #192639 - Attachment description: patch for the updae wizard → patch for the update wizard
I'm not sure what I think of this approach. Setting permanent preferences from
this wizard seems UI-wrong to me, *and* we end up duplicating the localized
string that is shared with the preferences panel (if decide to go this route, we
should probably take that string out of content.dtd and use this toolkit DTD
instead).
I agree but I also don't see a good solution for this issue especially since
this wizard is displayed during startup and it has to take into account if the
pref is locked as well. Another option would be to allow user initiated installs
from chrome when the pref is false and not locked... that would be the first
patch. We can add another param to say this is initiated from chrome as Daniel
asked though the only code paths that call it currently are from chrome so it
would be redundant... though adding it might make it clearer to anyone modifying
the code, etc. that it circumvents the pref when it isn't locked and false.
Actually the first patch doesn't take into account if the pref is locked. :/
The preference currently talks about "installing" software, not updating it, and
users see those as two very different concepts. I'm inclined to agree with
Chase, and say that if a user has left auto-update installed extensions on,
and/or if they kick off an explicit "Check now..." for updates from the
preferences panel, those should be intepreted as explicit requests to update.
Having a checkbox to switch this preference in the update dialog will just
result in that preference being flipped and users wondering why they had to bother.

(Really, also, this pref should be renamed to "Allow extensions and themes to be
installed" since that's what it's doing -- I was a little surprised that when
unchecked, I wasn't even able to install a local XPI!)
Daniel - I added a boolean aIgnore param to InitManagerFromChrome as you asked
for in comment #20 - I suspect a more descriptive name should be used.
InitManagerFromChrome is only called from user initiated installations (e.g.
PFS) and upgrades (e.g. EM) so all the call sites pass true.

In thinking this through I don't believe that this pref or any other pref needs
to be checked for being locked and false. The user will only be able to update
items that are installed and in a location they can write to and there is
already code to not check items for updates that are installed in a location
that can't be written to.
Attachment #192639 - Attachment is obsolete: true
Attachment #192865 - Flags: review?(dveditz)
Attachment #192639 - Flags: review?(benjamin)
Comment on attachment 192865 [details] [diff] [review]
patch w/ param added to InitManagerFromChrome

I just got my dev system back up after losing a hard drive and I'm gonna take a
deeper look at this first
Attachment #192865 - Attachment is obsolete: true
Attachment #192865 - Flags: review?(dveditz)
*** Bug 305013 has been marked as a duplicate of this bug. ***
Daniel - all of the call sites are from chrome and afaict adding a param instead
of just removing the check doesn't provide any more value than just adding a
comment to the affect that InitManagerFromChrome must only be called from chrome
initiated installs and updates. Adding the param might make people think twice
so I can see why you might want the extra param though they could just as easily
pass the value that makes it ignore the pref. Perhaps this is for future call sites?

I never assigned this bug to myself since there are several other people more
knowledgeable with the xpinstall code and I'm going to back off from this bug
and hope someone else is able to fix this.
*** Bug 305043 has been marked as a duplicate of this bug. ***
*** Bug 269532 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b4?
->Dan. I thikn we need this. If so, can you help us with a fix?
Assignee: nobody → dveditz
Flags: blocking1.8b4? → blocking1.8b4+
Flags: blocking-aviary1.5+
Flags: blocking-aviary2.0?
Flags: blocking-aviary2.0?
We ought to hide the checkbox tied to the xpinstall.enabled pref. The UI wording
is what people want, and the "Allowed Sites" button and whitelisting feature is
how you allow a site. They don't need the checkbox because the global setting
(at the site level) is to *not* allow it. Compare the popup blocking case on the
line above where the checkbox means the exact opposite, but it also has an
"Allow" button.

The back-end xpinstall.enable pref should be honored, but hidden away for
emergency or administrator use as was originally intended.

A similar option would be to switch the pref controlled by the checkbox to the
xpinstall.whitelist.required pref, and change the wording along the lines of the
popup pref to "Block sites from initiating software installs".
Status: NEW → ASSIGNED
Whiteboard: [asaP1] → [asaP1][l10n impact]
Removing UI really doesn't have l10n impact... you don't have to remove the strings.
Whiteboard: [asaP1][l10n impact] → [asaP1]
I don't think we can simply remove the string -- something has to go with the
"Allow Sites" button in order to get to the whitelist. It may be as simple as
removing the checkbox (no l10n), but if we change which pref is controlled or
where it is in the dialog (don't want a non-checkbox item in a list of checkbox
items) may need to change the string wording.
Whiteboard: [asaP1] → [asaP1][l10n impact]
So, a suggestion in the "easiest-to-do for 1.5 with minimal l10n impact" frame
of mind:
 _____________________________________________________________________
/                                                                     \
| [ ] Allow extensions to be installed         [ Allow Web Sites... ] |
| [ ] Block Popup Windows                      [ Exceptions...      ] |
| [ ] Load Images                              [ Exceptions...      ] |
|      [ ] for the originating website only                           |
| [ ] Enable Java                                                     |
| [ ] Enable Javascript                        [ Advanced...        ] |
|                                                                     |
\_____________________________________________________________________/

The new option wording is explicitly clear about what the pref does, and the
advanced button clearly indicates that it leads to advanced options that deal
with extensions from websites.

I also am suggesting that to reduce confusion / overload, we rename the "Allowed
Sites ..." button for "Block popups" to "Exceptions..." to bring it in line with
other web-content prsentation modifiers like "Load Images".

(As per many other comments, the "Allow extensions to be installed" pref should
not interfere with updating existing extensions.)
(In reply to comment #39)
> So, a suggestion in the "easiest-to-do for 1.5 with minimal l10n impact" frame
> of mind:
>  _____________________________________________________________________
> /                                                                     \
> | [ ] Allow extensions to be installed         [ Allow Web Sites... ] |
> | [ ] Block Popup Windows                      [ Exceptions...      ] |
> | [ ] Load Images                              [ Exceptions...      ] |
> |      [ ] for the originating website only                           |
> | [ ] Enable Java                                                     |
> | [ ] Enable Javascript                        [ Advanced...        ] |
> |                                                                     |
> \_____________________________________________________________________/
> 


a simplification:
 _____________________________________________________________________
/                                                                     \
| [ ] Allow extensions                         [ Allow Web Sites... ] |
| [ ] Block Popups                             [ Exceptions...      ] |
| [ ] Load Images                              [ Exceptions...      ] |
|      [ ] for originating website only                               |
| [ ] Enable Javascript                                               |
| [ ] Enable Java                              [ Advanced...        ] |
|                                                                     |
\_____________________________________________________________________/


I'm all for simplifications, but I fear that might be misleading. After all,
turning off the pref doesn't turn off extensions, which is what someone might
interpret the action of "do not 'allow extentions'" to mean.
Here's my suggestion for the allow websites to install software UI and the
update system.  Feel free to offer suggestions on my idea.

The checkbox next to "Allow websites to install software" should be removed,
since, as Daniel said, this is a whitelist already and you shouldn't have to
enable an option for it.  The text should be changed to something like, "Allow
the following websites to install software" or "Only these sites can install
software."  The word software could also be replaced by "extensions/themes",
although this might make the options box too wide.  The wording on the "Allowed
sites" button could either be left alone, or changed to a simple "Configure."  I
suppose "configure" might sound intimidating to some users, so that may not be
the best word choice, but “Allowed sites” seems kind of redundant since the
checkbox dialogue already says "Allow websites...".

xpinstall.enable should default to false for everything except sites on the
whitelist.  If the user tries to receive install something from a site not on
the whitelist, the bar should pop up, but the wording should be changed to,
"This site is not currently allowed to install software.  If you wish to allow
it, click here" and have a button that says "allow site" or something similar. 
Updating of extensions should work without xpinstall enabled because the user
already has the extension installed so they have allowed that software to be
installed already.  Updating extensions should be thought of in the same way as
installing updates to the browser itself, so the user should not have to have a
setting enabled for installing new software, when they are merely updating
existing software.  Basically, xpinstall should only affect the installation of
new extensions or themes.
(In reply to comment #42)
> enable an option for it.  The text should be changed to something like, "Allow
> the following websites to install software" or "Only these sites can install

I'm less comfortable with removing the checkbox, since keeping it provides a
preference for tin-foil hat types who want all XPI installation turned off. You
do point out a shortcoming of my proposed UI refactoring in comment 39, though,
which is that we should ensure the text covers extensions & themes. "Settings"
is a better term, I think, than "Configure" ..:
 _____________________________________________________________________
/                                                                     \
| [ ] Extensions & themes can be installed          [ Settings ... ]  |

> "This site is not currently allowed to install software.  If you wish to allow
> it, click here" and have a button that says "allow site" or something similar. 

The design of requiring explicit addition of a site to the whitelist is
intentional to ensure that a user doesn't allow, say, randomsitewithxpis.com for
a known-good extension and then end up getting burned on a secondary page with a
bad extension.

The enhancement to allow a one-time install from a site not on the whitelist is
bug 252830.

> Updating of extensions should work without xpinstall enabled because the user
> already has the extension installed so they have allowed that software to be

I believe that we're all already in agreement on this point, yes. :)
Ugh, looking back, I'm having sober second thought about that checkbox title.
Help! Maybe "Allow installation of extensions and themes"?
I think I like the wording "Allow installation of extensions and themes."  I
might have an even better idea, what if the wording is formatted like the
cookies options.  Like this:

[ ]  Allow installation of extensions and themes
      only for these sites              [ Settings ]

I don't think that there should be an option to allow all sites or a checkbox
next to the "only for these sites" because it's the only option. 

I don't know if this would be overkill, but another option might be.

[ ]  Allow installation of extensions and themes
      [ ]  Only for these sites         [ Settings ]
      [ ]  Through drag-and-drop into extension manager

I don't see drag-and-drop being a huge security concern, but I figured I'd throw
the idea out there anyway.

> The design of requiring explicit addition of a site to the whitelist is
> intentional to ensure that a user doesn't allow, say, randomsitewithxpis.com 
> for a known-good extension and then end up getting burned on a secondary page
> with a bad extension.

I'm not sure I understand what you're are saying.  Are you saying that there
should not be a prompt to add a site and the user should be forced to manually
type in the address into the whitelist box?  I guess I can understand this
because the prompt is more of an attempt to help less knowledgeable users, but
they are not usually installing a lot of extensions and the prompt would
probably just annoy more experienced users.  I still think that there should be
some kind of "installation blocked" message, even if there isn't a button that
easily adds the page to the whitelist.

I wasn't aware of bug 252830, but an "allow once" option is a great idea.
(In reply to comment #45)
> I don't think that there should be an option to allow all sites or a checkbox
> next to the "only for these sites" because it's the only option. 
> [ ]  Allow installation of extensions and themes
>       [ ]  Only for these sites         [ Settings ]
>       [ ]  Through drag-and-drop into extension manager

See comment 28 - it turns out that this preference actually controls the
installation of *any* XPI, local or from a website, so I don't think we have
this fidelity of control. We're not discussing a complete reworking of the
internal preference structure for the 1.5 release.

> some kind of "installation blocked" message, even if there isn't a button that
> easily adds the page to the whitelist.

There is. It appears at the top of the page. Test it yourself at some place like
http://toolbar.google.com/firefox/extensions/ What I'm saying is that we don't
want to make it super-easy to permanently add a site to the whitelist from this
browsermessage area, since doing so would almost certainly just train users to
continually click that "Add to whitelist" button without thinking or realizing
the implications. Thus, the request for "Add Once" ...

Anyway - I think, unless anyone has serious objections, we're now looking for a
patch that:

  - makes the following proposed UI string changes:
 _____________________________________________________________________
/                                                                     \
| [ ] Allow extensions & themes to be installed     [ Settings ... ]  |

  - allows the update engine to apply updates even if the xpinstall.enable pref
is off

(Note: IMO, if we wanted to take Daniel's first approach from comment 36, simply
removing the checkbox would be awkward. What we'd really want to do is create a
new groupbox and put in a string like "Firefox can always allow some web sites
to install extensions and themes" and a "Allow Sites ..." button. This actually
sounds like more work than we have time for. I'm not a big fan of the second
approach, as I really don't think we should ever have an "always allow
everything to be installed at all times" feature exposed in the primary UI.)
_____________________________________________________________________
/                                                                     \
| [ ] Allow extensions & themes to be installed     [ Settings ... ]  |

This seems like a relatively easy and workable fix, but "settings" is kind of
vague in this case.  It could mean that there is fidelity of control for things
like "only allow local install", or it could mean that there is a list of sites
that you can whitelist.  That was the reason that I proposed this, where all the
text and buttons would be greyed out until the checkbox is checked.
    
[ ]  Allow installation of extensions and themes
      only for these sites              [ Settings ]

I think my idea is probably too clunky, but I think it should be clearer that
the checkbox allows installation and the button is a whitelist for which
websites can install extensions and themes.  Unfortunately a lot of people don't
know what a whitelist is, so you can't just have the box say whitelist.  I don't
think the button should say "Exceptions" like the button for "Allow sites to set
cookies" option because, in the case of the cookies control, the "Exceptions"
button has options to block and to allow cookie setting, and allowing doesn't
seem like an exception to the rule of allowing sites to set cookies.

I'd appreciate any criticism of my idea, especially since I'm not a UI designer.
(In reply to comment #47)
> [ ]  Allow installation of extensions and themes
>       only for these sites              [ Settings ]

Um, please read comment 46 again, especially the bit where I point out that the
preference controlled by the checkbox does more than simply enable the
whitelist. If the xpinstall.enabled preference is off, then local installs of
XPIs (drag-and-drop, etc) is also turned off.

Settings isn't ideal, sure. I chose it to be consistent with other controls that
lead to secondary advanced settings dialogs, but maybe we just want to keep the
"Allowed Sites" label that already exists to minimize l10n impact. (I'd switched
the other labels to "Exceptions..." as it matched the checkbox label text a
little better)

Rob tells me that if he's feeling adventurous he might try for a patch 4tw this
weekend, so let's take it from there.
(In reply to comment #48)
> (In reply to comment #47)
> > [ ]  Allow installation of extensions and themes
> >       only for these sites              [ Settings ]
> 
> Um, please read comment 46 again, especially the bit where I point out that the
> preference controlled by the checkbox does more than simply enable the
> whitelist.

He was perhaps taking my earlier suggestion that we change this item so that it
*does* control only the whitelist pref (xpinstall.whitelist.required). Leave
xpinstall.enabled a hidden pref with no UI. There's no reason that ever needs to
be turned off, and if there *is* some reason (security hole?) then it's quite
possible it's the UI use of xpinstall that's gotten hacked so I wouldn't want
that to be an end-run.
Daniel, while I do agree that xpinstall.enable could probably left as a hidden
preference, I was saying something else.  I figured that hiding that preference
would be considered too large of a change for 1.5, so I was trying to change the
wording.  My example was trying to show that the whitelist is only a part of
what the checkbox affects by making the whitelist wording indented, like it's a
subset, of the theme extension install.  I realize that my example isn't very
good and there are more direct ways to explain the full function.  I had thought
that a popup tooltip when the cursor hovers over the checkbox or dialogue would
be good, but popup tooltips don't seem to be used anywhere else in the options
panel, so that's not a good idea.  Although the wording could definitely be
improved, here's a better version of my idea.  The spacing required may not be a
big issue, based strictly on appearance, since the content tab has more empty
space above ok, cancel and help than the privacy tab does

[ ] Allow installation of Extensions and Themes    [ Allowed Sites ]
     Note:  This option enables installation from websites and local
                installs

If the UI can be changed for 1.5 so that xpinstall.enable is a hidden pref and
the wording is improved, I think that would be the best solution.  If it can't
be finished in time, I think the wording should be changed to explain that
"software" means extensions and themes (I guess it also includes bugzilla
patches too, but extensions and themes is a better explanation for most users)
and that the checkbox enables all installations, not just whitelisted websites.  
> 
> [ ] Allow installation of Extensions and Themes    [ Allowed Sites ]
>      Note:  This option enables installation from websites and local
>                 installs
> 
Longer than necessary. How about:

  [ ] Allow Extensions and Themes to install         [ Allowed Sites ]  
       Note:  Enables installation from websites and local installs.

(In reply to comment #50)
> Created an attachment (id=196614) [edit]
> simply switching the pref (needs verbiage help)

I *still* think that this approach is dangerous, as it makes it very easy for
users to turn off the whitelist requirement without realizing the implications
of that action.

If we do it, though, the label should be something like:
 ____________________________________________________________________
/                                                                    \
| [ ] Block Popup Windows                          [ Allowed Sites ] |
| [ ] Block web sites from installing software     [ Allowed Sites ] |

where "software" can be replaced with extensions without making me shed any
tears, or "extensions & themes" to be fully accurate at the cost of making the
window wider than it already is.

Also:

>+      <preference id="xpinstall.whitelist.requiredd"  ...
                                                   ^^
I think you've got an extra "d" in there.
> I *still* think that this approach is dangerous, as it makes it very easy for
> users to turn off the whitelist requirement without realizing the implications
> of that action.

Turning off the whitelist is not dangerous, that's how Netscape 4-7 worked and
the  current Mozilla Suite/seaMonkey. It simply allows sites to annoy you with
unwanted prompts, and gives malicious sites an opportunity to badger you into
clicking OK. Not nice, but not a security hole.
> the  current Mozilla Suite/seaMonkey. It simply allows sites to annoy you with
> unwanted prompts, and gives malicious sites an opportunity to badger you into
> clicking OK. Not nice, but not a security hole.

Ah, yes, thanks for the reminder. I forgot that the prompt for install *still*
comes up. Mmkay, I guess I'm OK with this approach, then. I guess that it's the
simplest and least risky in terms of the original purpose of this bug.
*** Bug 285758 has been marked as a duplicate of this bug. ***
dveditz, I suggest the following verbiage: 

+<!ENTITY  enableXPInstall.label      "Warn me when web sites try to install
extensions or themes">

It can even use the existing "a" accesslabel. I went with "warn" over "block" to
prevent people from thinking that the pref turns off software installing altogether.
Attached patch switch pref and update label (obsolete) — — Splinter Review 

        Attachment #196614 -
        Attachment is obsolete: true

        Attachment #197898 -
        Flags: superreview?(mconnor)

        Attachment #197898 -
        Flags: review?(beltzner)

    
    

    
  
Comment on attachment 197898 [details] [diff] [review]
switch pref and update label

r=me on the change, but we need to s/enableXPInstall/enableXPInstallWarning/ or
something similar so l10n tinderboxes go orange.  Please coordinate with Axel
on the notifications to localizers as well.

        Attachment #197898 -
        Flags: superreview?(mconnor) → superreview+

    
    

    
  


  
(In reply to comment #59)
> (From update of attachment 197898 [details] [diff] [review] [edit])
> r=me on the change, but we need to s/enableXPInstall/enableXPInstallWarning/
or
> something similar so l10n tinderboxes go orange.

Done. I'll assume r=beltzner on the text change since he supplied it ;-)

        Attachment #197898 -
        Attachment is obsolete: true

        Attachment #197912 -
        Flags: superreview+

        Attachment #197912 -
        Flags: review+

        Attachment #197912 -
        Flags: approval1.8b5?

        Attachment #197912 -
        Flags: approval-l10n?

    
  

        Attachment #197898 -
        Flags: review?(beltzner)

    
  
Keywords: late-l10n

    
    

    
  
Comment on attachment 197912 [details] [diff] [review]
label change as requested

approval-l10n is only for changes inside localizations, but thanks for trying.
core l10n changes are evaluated by drivers.

        Attachment #197912 -
        Flags: approval-l10n?

    
    

    
  
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED

    
    

    
  
If I understand what just happened correctly, I'm disappointed with the security
stance this change takes.

Prior to this, users were able to restrict XPI installs all together.  Or, if
they wanted XPI's, they'd only be allowed from whitelisted sites and a warning
would be displayed.

Now it sounds like XPI installation can't be disabled from the UI and could even
be installed without any prompting.  OUCH!

Quite a step backward.

    
    

    
  
(In reply to comment #63)
> Prior to this, users were able to restrict XPI installs all together.

They still can, through about:config. If you can mess with about:config then you
can understand that turning off the xpinstall engine is turning off the install
engine, and that the original problem described in this bug (turn off the
install engine, but still expect parts of the program to use the install engine)
doesn't make sense.

>  Or, if they wanted XPI's, they'd only be allowed from whitelisted sites
> and a warning would be displayed.

That's the default behavior.

> Now it sounds like XPI installation can't be disabled from the UI.

Effectively, requiring the whitelist and not having any sites on it is disabling
installation. And the true "OFF" state can be reached through about:config.

> and could even be installed without any prompting.  OUCH!

No, turning off the checkbox would simply make all sites act as if they were
whitelisted, bringing up the install *prompt* rather than the infobar. In other
words exactly the behavior the Suite has always had and still has while
remaining perfectly safe.

> Quite a step backward.

Possibly.

It's conceivable that sites will then instruct users to turn off whitelisting
rather than whitelist their site (we *still* need the "allow just this once"
infobar option).

The complaint in comment 0 may not actually be true anymore, since Firefox
update uses .mar files and bypasses xpinstall. Then bug 307928 could be fixed to
disable the extension update buttons

    
  

        Attachment #197912 -
        Flags: approval1.8b5? → approval1.8b5+

    
    

    
  
dveditz,

Sorry to drop this so late, but I missed the fact that the button label needs to
change to "Exceptions" (not "Allowed Sites"). Pretty please? :)

    
  
Blocks: 310545

    
    

    
  
approval stands for that change as well.

    
    

    
  
Updated button as requested (picked 'x' as the accesskey, hope that's ok).
Checked into 1.8 branch
Keywords: fixed1.8

    
    

    
  
> +<!ENTITY  enableXPInstall.label      "Warn me when web sites try to install
> extensions or themes">

Themes are not affected by this pref. With this box checked and an empty
exception list, I still get the install prompt when I try to install a theme.
See bug 246375.

    
    

    
  
It seems there has been some concern on the forums about the fact that is now
harder to disable installation.  Is it possible to set the default value for
xpinstall.enabled to false?  Although extensions install isn't currently a
security issue, in case it does become one, it might be a good idea to prevent
users from installing extensions/themes, unless they are knowledgeable enough to
be able to enable the setting in about:config.

    
  
Depends on: xpinstall.enabled

    
    

    
  
(In reply to comment #64)
> turning off the xpinstall engine is turning off the install
> engine, and that the original problem described in this bug (turn off the
> install engine, but still expect parts of the program to use the install engine)
> doesn't make sense.

in line with that, i think that restricting xpinstall.enabled to only affecting
extension and theme prompts and not affecting app update would be the real
solution. then change the text to "allow whitelisted sites to prompt for
extensions and themes". that makes it clearer that putting an xpi address
directly into the address bar, dragging a link onto the extension manager, or
using the extension manager "find updates" feature will prompt to install
extensions without that site being on the white list.

    
    

    
  
OK, so, if the original issue in comment 0 isn't actually an issue anymore (Rob
Strong is apparently confirming this) and based on some of the early regressions
we're seeing (users who have xpinstall.enable set to false will lose the UI to
change it back / the browsermessage that appears when xpinstall.enable=false is
no longer accurate / EM update and compatability wizard still get hung when
xpinstall.enable=false) then I'm not sure that this solution is actually best
serving the user.

Instead, I'd recommend we back this patch out, and take the patches in bug
307928, bug 310737 and bug 310745 instead. That solves the two major problems:
 - the prefs are (finally) representative of what they do
 - when xpinstall.enabled=false, users aren't hung in the compatability wizard 
   & EM update

Rob said he'd submit a patch on bug 310737 ASAP. Sorry to churn on this.


    
    

    
  
I don't see it mentioned, but were this to remain as it is now, some cleanup is
probably necessary. If users update to 1.5b2 with software installation
disabled, they then currently have no way to re-enable it (to install extensions
etc.) without using about:config.


    
    

    
  
(In reply to comment #72)
> disabled, they then currently have no way to re-enable it (to install extensions
> etc.) without using about:config.

Covered in bug 310737.

Product: Firefox → Toolkit

    
    

    
  
(In reply to comment #72)
> I don't see it mentioned, but were this to remain as it is now, some cleanup is
> probably necessary. If users update to 1.5b2 with software installation
> disabled, they then currently have no way to re-enable it (to install extensions
> etc.) without using about:config.
> 
I know this is old news, but I'm cleaning out old bug votes. This one was just hanging out there as an unanswered issue. I'm cc'd on this bug, so I'll unvote on it, but just wondering if this was ever dealt with.


          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: