149478 - XPI install confirmation should default to cancel, not Install

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. Go to <http://www.beonex.com/communicator/version/0.8/add-ons/binaries/java/>
and click the install butten or by some other means trigger the install of an XPI.

Actual result:
The confirmation dialog has 2 buttons, Install and Cancel, and the install
button is the default.

Expected result:
The Cancel button is the default.

Additional Comments:
- I think this is a regression, it at some point defaulted to Cancel.

Rationale:
- XPIs have full local rights - the install scripts and/or binaries in them may
do everything the user can do.
- We know that many users just click the default button (without reading any
text), if they hit a msgbox they don't know. Since it is very easy to trigger
that dialog (we don't prevent that in any way - a simple link or JS will
suffice), it is insanely easy for attackers to take control over the machines of
naive users. Actually, even users who are aware of the risks, but just in a
hurry, might not recignize the dialog and just hit enter / click the highlighted
button.

Comment 1

[Daniel Veditz [:dveditz]]

There have been times when neither button was the default because we had a focus
problem, but I've always intended the Install button to be the default. I can
see your point, though. I think I'd prefer no default to a default of cancel.
It's just as much work to Install, but a lot harder to accidentally cancel out
on auto-pilot.

At some point we should re-think the wording and presentation of the dialog,
too. It says all the right things but it's just not scary enough.

Comment 2

[Daniel Veditz [:dveditz]]

Need UE input on this dialog, but we definitely need to change the default.

Comment 3

[Daniel Veditz [:dveditz]]

*** Bug 161713 has been marked as a duplicate of this bug. ***

Comment 4

[Paul Wyskoczka]

Adding adt1 rtm per the adt.

Comment 5

[Jesse Ruderman]

The skin install and enablePrivilege dialogs also have Install/Yes as the 
default buttons (Enter key), but they have checkboxes so bug 133582 keeps them 
from having inital focus (Space key).

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: (Av1) Make "cancel" the default [Checked in: Comment 13 and 14]

Comment 7

[Jesse Ruderman]

We also need to change the defaults for the enablePrivilege and skin-install
dialogs.  The same usability arguments and security holes apply to those
dialogs, but with "space" replaced with "enter" because of bug 133582.

Comment 8

[Syd Logan]

Comment on attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

r=syd

Comment 9

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on the ADT's behalf) approval for checkin to the 1.0 branch, pending
sr= and Drivers' approval. pls check this in asap, then replace "mozilla1.0.1+",
with "fixed1.0.1". thanks!

Comment 10

[Scott MacGregor]

Comment on attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

sr=mscott

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

Approved for 1.0 branch; change mozilla1.0.1+ to fixed1.0.1 when done.

Comment 12

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 1.1

Comment 13

[Daniel Veditz [:dveditz]]

Fixed on 1.0 and 1.1 branches

Comment 14

[Daniel Veditz [:dveditz]]

Checked into trunk

Comment 15

[Ben Bucksch (:BenB)]

Thanks! :-)

Comment 16

[Grace Bush]

verified on branch 1.0 (8/11) branch 1.1 (8/12)

Comment 17

[Jesse Ruderman]

The default is Install in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7a) Gecko/20040213.  Did this regress?

Comment 18

[Daniel Veditz [:dveditz]]

This regressed with sspitzer's fix for bug 214721 -- focus setting code got zapped

Comment 19

[Daniel Veditz [:dveditz]]

Should get fixed for the next release, marking blocker in case I forget

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <institems.js> (doing Av1 again)

Regression fix:
"Av1 like" patch, with "dialog syntax" instead of "window syntax".

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b) <institems.js> (doing Av1 again)

Regression fix:
"Av1 like" patch, with "dialog syntax" instead of "window syntax";
plus a little cleanup.

Comment 22

[Andrew Schultz]

Comment on attachment 142879 [details] [diff] [review]
(Bv1b) <institems.js> (doing Av1 again)

Serge: [e-mail] (Syd Logan) doesn't work on Mozilla anymore.

>    // set the okay button in the param block
>    if (gParam)
>-     gParam.SetInt(0, 0 );
>+   gParam.SetInt(0, 0);

the indenting here was fine.

>     // set the cancel button in the param block
>     if (gParam)
>-      gParam.SetInt(0, 1 );
>+    gParam.SetInt(0, 1);

same here.

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1c) <institems.*> (doing Av1 again) [Checked in: Comment 33]

Bv1b, with comment 22 suggestion(s).

(Enforces 2-car. indentation on the few remaining lines which were not.)

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

sr=dveditz, you can get r= from bsmedberg or ajschult

Comment 25

[Andrew Schultz]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

r=ajschult

Comment 26

[[e-mail]]

Has anyone tested this on a Mac?

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]


(In reply to comment #26)
> Has anyone tested this on a Mac?

Not me: PC/W98SE only.

Comment 28

[Andrew Schultz]

(In reply to comment #26)
> Has anyone tested this on a Mac?

I tested on Linux.  Are there specific Mac issues that might be exposed by this
patch?

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> (In reply to comment #26)
> > Has anyone tested this on a Mac?
> 
> I tested on Linux.  Are there specific Mac issues that might be exposed by this
> patch?

neil:
Is there a (potential) issue here, or could I seek approval1.7b ?

Comment 30

[[e-mail]]

The Mac plays by different rules: the Tab key doesn't work and the Enter key is
always OK. It would therefore have been incorrect for the patch to change the
default action on the Mac. As it is I asked a Mac owner and it does not.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]


approval1.7b?:
Solves blocking1.7b+ :-)

Comment 32

[chris hofmann]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

a=chofmann for 1.7b

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]


Check in: { 03/11/2004 13:03	neil%parkwaycc.co.uk }

Comment 34

[Grace Bush]

verified build 2004031708

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

a=blizzard for 1.4.3