213207 - Security Certificate Mismatch Defaults To "OK"

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Lawrence Worth]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4) Gecko/20030624

While I agree that we need a shut-up-and-don't-ask-me-again option, as suggested
in 209299, the security mismatch warning should default to "Cancel", since it's
to easy to press Enter too many times.


Reproducible: Always

Steps to Reproduce:
1. Visit the URL.
2. See the mismatch warning as described.
3.

Actual Results:  
"OK" was highlighted.

Expected Results:  
"Cancel" should be highlighted (unless I've asked for the window never to come
up again for that site).

Comment 1

[Jo Hermans]

Yes, I agree. But it would make bug 209299 even more urgent to fix, since you
can't dismiss the dialog quickly anymore. So they need to be fixed together.

Comment 2

[Matthias Versen [:Matti]]

-> psm

Comment 3

[Lawrence Worth]

Yes, I agree that this should be fixed in the same
release as 209299, although they don't necessarily
need to be fixed at the same time, so long as they
both appear in the same release.

Comment 4

[John G. Myers]

Mass reassign ssaux bugs to nobody

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This will be easier to fix if bug 322155 is fixed.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This works, with the patch from bug 322155. Haven't double checked all these changes yet, though.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch 2

Now that bug 322155 is fixed, this bug is easy to fix. Relies on the changes in bug 284776 (defaultButton attribute).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

The only change in behavior is that the domain mismatch dialog button defaults to Cancel instead of OK. The other parts are just simplification.

Comment 9

[Kai Engert [:KaiE:]]

I'm not sure if I'm ok with this change.

In general, I think your change makes sense.
But I wonder what this will mean for consistency. What are the current defaults in our other cert warning dialogs? Do all off them default to the more secure way already, or is it mixed?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> I'm not sure if I'm ok with this change.
> 
> What are the current defaults in our other cert warning dialogs? Do all off
> them default to the more secure way already, or is it mixed?

Do you know of any test sites that I can use to test some of the other dialogs? I don't even really know which dialogs I should be looking at. If there are other dialogs that default to "unsafe" actions, then I can fix those too.

Comment 11

[Håkan Waara]

Comment on attachment 216673 [details] [diff] [review]
patch 2

I disagree with this patch.

The default behavior for focus in dialogs is platform specific (for example, on Mac OS X the first focusable item should always be focused, see bug 335089).

Also, the focus behavior  needs to be consistent across all our applications, and even in the XUL toolkit. There are good accessibility reasons for this. For example, users with low to no vision are often heavily reliant on the keyboard - and so they should be able to assume (based on platform user interface standards) which button that has focus.

Only in special exceptions should you call focus().

To sum up, the change is not correct on OS X, and is bad on all platforms for the reason of consistent accessibility.

I think the right way to go is simply remove the focus() call - and let the XBL binding do its default thing.

Comment 12

[Håkan Waara]

Sorry, a correction:  I agree with the patch, but it should also extend to either remove the explicit focus() call in newserver.js or ifndef XP_MACOSX it.

Comment 13

[Chandrasekar Kannan]

Gavin, are you working on this bug ?. any updates ?.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> Gavin, are you working on this bug ?. any updates ?.

The patch still works, it's just waiting for Kai's review.

To reply to comment 9, all this patch does is remove the focus() calls that interfere with the dialog binding's standard button focusing, so it is making the security dialogs behave as all other dialogs do. It also changes the default button of the domain mismatch dialog to cancel instead of "OK", per this bug's summary, which I believe to be consistent with other "warning" dialogs where there is a "safe" action.

One such example is the dialog you get when you try to navigate to a URL that specifies HTTP authentication information unnecessarily, like http://[e-mail]. It defaults to "No".

Comment 15

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

Comment on attachment 216673 [details] [diff] [review]
patch 2

Making sure that the default choice is always the safest makes sense to me.

Comment 16

[Kai Engert [:KaiE:]]

Comment on attachment 216673 [details] [diff] [review]
patch 2

ok, r=kengert

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 216673 [details] [diff] [review]
patch 2

This is a simple fix that I'd like to land on the 1.8 branch, see comment 14 for the patch description.

Comment 19

[Håkan Waara]

Was comment 12 addressed before check-in?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #19)
> Was comment 12 addressed before check-in?

No. The patch didn't make a change to that code, it only added a comment. Want to file a new bug about the change you think needs to be made?

Comment 21

[Håkan Waara]

bug 342862 filed