172091 - Encryption security warnings: "alert me whenever" should stay checked

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Soh Kam Yung]

User-Agent:       Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.2b) Gecko/20021001 Phoenix/0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.2b) Gecko/20021001 Phoenix/0.2

I like to be reminded by the pop-up security alert when I switch between a
normal (http URL) and secure (https URL) website.

In Phoenix, the preference to display this alert is not 'stickly'. The alert
will always be de-selected and I have to re-enable it to keep seeing the reminders.



Reproducible: Always

Steps to Reproduce:
1. go to https://login.yahoo.com/
2. a security warning window will pop-up, alerting me that I am going to a
secure web site
3. enable the "Alert me..." option
4. exit the website
5. re-enter the website
6. the security warning pops-up again
Actual Results:  
The "Alert me..." option is de-selected again

Expected Results:  
The "Alert me..." option should remain selected (as in Mozilla)

You may have to delete the user_pref("security.*") lines from the user.js file
first to get the alerts to appear.

Comment 1

[Asa Dotzler [:asa]]

blake, this is all you. we like the default but it shouldn't override user
selection. 

Comment 2

[Asa Dotzler [:asa]]

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

Comment 3

[Asa Dotzler [:asa]]

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

Comment 4

[William Stockall]

If one of the bugs marked as a duplicate of this one is marked for all platforms
and the reporter was using Windows, why is this bug showing as Linux specific? 
Just curious.

Comment 5

[Pierre Chanial]

as a reminder, the problem is here:
http://lxr.mozilla.org/seamonkey/source/security/manager/boot/src/nsSecurityWarningDialogs.cpp#155

Comment 6

[Jesse Ruderman]

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

Comment 7

[aynilove [So, Jae-yoon]]

> as a reminder, the problem is here:

then, simply delete line #155 to #157 will solve this problem - Why Not?

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

-> 0.7

Comment 9

[Mike Auty]

I'm sorry, I've been unable to create a diff file, however thanks to the
extremely useful link provided by pch, I can describe a patch which will still
give the intended default but will not override the user's choice.
Firstly (as pointed out by aynilove) remove lines #155 through to #157, then
replace line #135

if (NS_FAILED(rv)) prefValue = PR_TRUE;

with

#ifdef MOZ_PHOENIX
if (NS_FAILED(rv)) prefValue = PR_FALSE;
#else
if (NS_FAILED(rv)) prefValue = PR_TRUE;
#endif

(Again, I'm sorry I wasn't able to supply a diff.  I've read through a lot of
material on the site about helping and the requirements for patches to get
considered are difficult to achieve even though I know enough to offer a patch
for simple bugs like this.  Could anyone mail me help or ideas to simplify the
task?)

Comment 10

[Robert Parenton]

Attachment: Patch based on Mike Auty's fix

Comment 11

[Robert Parenton]

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Requesting review from Blake Ross

Comment 12

[Robert Parenton]

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Build Firebird with this patch included on both Windows and Linux.  On both
platforms it fixed the problem with no apparant side effects.  We should
probably try to get this in 0.6 if possible now that we have a working patch.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Actually... why don't we just set the default to false in firebird's all.js and
get rid of the #ifdef altogether?

Comment 14

[Brian Ryner (not reading)]

Attachment: get rid of the #ifdef

I think this should do the trick.

Comment 15

[Brian Ryner (not reading)]

checked in.

Comment 16

[Mike Auty]

The new proposed patch doesn't seem to have the same functionality.  This will
turn off the security dialogs, and require the user to specifically turn them on
before they can be seen.  The previous patch shows the dialog once, and then
defaults to false while allowing the user to choose.  This patch should be
backed out if possible.

Comment 17

[Sven Grull]

I agree with comment #16. Security warnings always should be activated by
default so the user has to think about it and then decide whether he will turn
them off.

Comment 18

[Owen Marshall (Not reading bugmail)]

Reopening based upon comment 16 and comment 17. 

If this is what the patch is causing, this isn't good at all. Opting in for the 
security info will not be welcomed.

Comment 19

[Mike Auty]

The first patch mentioned in Comment #10 (which has review+ from bryner), should
work, the second patch was causing the problems.  That needs backing out.  Can
anyone super-review the first patch please?

Comment 20

[Owen Marshall (Not reading bugmail)]

We also need a patch for backing out the one that was checked in. pch, can you 
take a look? Or bryner?

--> bryner

Comment 21

[Brian Ryner (not reading)]

Sorry, I did misunderstand the intent here.

I think the problem is that we actually need 3 states for the pref:

1. default: the dialog is shown once, but the default is to not show it again
2. the user chooses to display the dialog always
3. the user chooses to never display the dialog

The patch that I marked review+ on attempts to express these 3 states by making
failure to get the default pref value correspond to state #1.  I don't think
that will work unless we also removed the default pref values from
security-prefs.js.

Comment 22

[Mike Auty]

I think you're right, it probably won't work without removing the defaults from
security-prefs.js.  Sorry I didn't catch that first time round, I'm a bit new to
the code as you can probably tell.

It feels a bit like we're mangling the code to get it to do what we want. 
Perhaps the preference should be an integer:

0 - No alert
1 - Alert
2 - Alert once, then default to off
Anything else - Alert

The reason for this ordering is to keep the 0=false, 1=true idea.  Is this the
only code that changing this would affect, or will there be more changes
required throughout the source?  I have no idea which is better coding practice,
hardcode the default, or turn the preference into a 3 state option?  Any
suggestions?

Comment 23

[Brian Ryner (not reading)]

I suspect changing it to an integer pref may cause problems with upgrades. 
Conrad, can you confirm that?

If that's problematic, we may have to make another set of "first time" prefs (ugh).

Comment 24

[Pierre Chanial]

or we could have only two integer prefs (first time + let's stick the alert) and
use their bits instead for the set of warnings.

Comment 25

[Sven Grull]

Re Comment #21:

>I don't think that will work unless we also removed the default pref
>values from security-prefs.js.

I created a new profile and visited a secure site, there was no popup. So I
checked about:config and all security.warn_* were set to false. 
I don't know which pref is higher rated, all.js or security-prefs.js. But after
my try it seems that all.js overrules the other prefs.

Comment 26

[aynilove [So, Jae-yoon]]

>Perhaps the preference should be an integer:
>
>0 - No alert
>1 - Alert
>2 - Alert once, then default to off
>Anything else - Alert
>
>The reason for this ordering is to keep the 0=false, 1=true idea.

In same, it can seperated in two pref.

 One pref. determine (Show dialog or Not)
 Other pref. determines (Checked or Unchecked)

Like IE does, Dialog is open only once, and  "alert me whenever" is NOT checked.
But user check it and click a command button, this decision will saved,
and next time dialog showed it should checked.

It's not a simple question than I supposed before.

BUT! Why we not use mozilla's defult dialog form that works well?

Comment 27

[Mike Auty]

We could separate it into two prefs, but that would be two prefs for each of the
five dialogs.  Requiring it to store 10 prefs, rather than 5.  Also, after the
first time the dialog shows up, the "show tick mark" pref is rather pointless. 
If they kept it once (show dialog), they're likely to want to keep it again
(checked), and if not then it doesn't matter what the checked or unchecked pref
is, the checkbox won't be seen again.

The mozilla's dialog default is to check the box (making the problem far
simpler) but most people only want the box once and would like it not to show
again (as you said IE behaves).

It may work out that the easiest course of action would be to remove the prefs
from security-prefs.js and all.js, and apply the first patch.  The only thing
we're waiting on is for someone to decide if this is a good decision or whether
it would be better to rewrite the thing to use some sort of preference
mechanism, and if so what the best way of doing that would be.

In the short term, could someone reverse the diff in the second patch, so that
it doesn't affect the nightlies.  Also does anyone know how to sort out
security-prefs.js so that it only affects firebird?  Thanks...

File reference:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/security-prefs.js
Lines 39 through 43 (the security.warn prefs)

Comment 28

[Conrad Carlen (not reading bugmail)]

> Also does anyone know how to sort out
security-prefs.js so that it only affects firebird?

Yes. The default pref files are interpreted in reverse-sort order. See
http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/nsPrefService.cpp#684.
The pref which is read last overrides the previous value. So, for firebird, make
a file called "security-prefs-firebird.js" and it will do what the 1st patch did
but without an #ifdef. This is what Camino does in order to differ from
mozilla's all.js.

Comment 29

[Mike Auty]

I'm not sure that will achieve what we need.  The problem is to not specify a
default (ie have no value, either true or false) in *any* of the prefs files. 
Obviously it would be best only to have this for Firebird, so I was wondering
how to pack firebird with a different security-prefs.js and all.js than any
other package?  The other packages *should* cope without defaults, but it's
probably nicer to just remove the prefs for firebird.  Has anybody taken out the
changes made to all.js yet?

Comment 30

[Robert Parenton]

Comment on attachment 123100 [details] [diff] [review]
Patch based on Mike Auty's fix

Removed the obsolete flag on the first patch as it worked as desired.

Comment 31

[Robert Parenton]

Brian/Blake,

Can you review the first patch (Patch based on Mike Auty's fix) and check it in
if if looks ok.  I previously built FB with it (first patch only) on Windows and
Linux and it achieved the desired results on both platforms when creating a new
profile.

Also, it appears that the second patch (get rid of the #ifdef) has not been
backed out.  For the first patch to work, it is necessary to back out the second
patch.

I can build this on Windows and Linux if you want me to test it again before
rolling back the second patch and checking in the first.  Also, I would be glad
to test on both Windows and Linux after it is checked in.

Comment 32

[Robert Parenton]

Attachment: Patch rolling back the second patch and applying the first (Dialogs will appear by default and be checked by default)

This patch rolls back the second patch and applies the first.  The result is
that the dialogs will appear by default, and will be checked by default.  This
has been tested on Linux and will be tested on Windows.  I will report back if
the effects are different on Windows.

>It may work out that the easiest course of action would be to remove the prefs

>from security-prefs.js and all.js, and apply the first patch.

I just tried this and built FB.  The result was that the dialogs never
appeared.  It looks like it treats not having a pref set the same as having one
set as false.

Comment 33

[Robert Parenton]

After rereading this entire bug and getting a better understanding about the
desired effect of removing the security.warn prefs from all.js and
security-prefs.js, I rebuilt FB from scratch again, using the third patch, and
also removing the relevant lines from security-prefs.js.  Once again, I got the
same result as in Comment #32, that is no warning dialogs were displayed at all.
 I tested this on Linux and am building it on Windows right now.  I'm thinking
that some other code elsewhere changed that is changing the behavior of Mike
Auty's proposed fix.

Comment 34

[Robert Parenton]

Comment on attachment 127135 [details] [diff] [review]
Patch rolling back the second patch and applying the first (Dialogs will appear by default and be checked by default)

I have almost completed a patch that makes the default unchecked, but will not
override the user's preference (too tired to finish it tonight).  The only
caveat is that the security.warn prefs must be removed from all.js and
security-prefs.js.  This should not cause a problem for the Mozilla Suite, but
I don't know how it would effect Camino.

I was thinking that we could add a flag for the makefile (or where ever it
needs to be) that would use a different security-prefs.js for FB.  Something
similar to the way Windows and Unix get different versions of radio.css:

http://lxr.mozilla.org/mozilla/source/toolkit/skin/win/radio.css
http://lxr.mozilla.org/mozilla/source/toolkit/skin/unix/radio.css

If this isn't possible, I think we should try to get approval to remove the
security.warn prefs from security-prefs.js (Brian?, Asa?, Conrad?).

Comment 35

[Robert Parenton]

Attachment: Patch that rolls back the addition of prefs to all.js and completely fixes the bug

This patch should fix the problem.  It uses the method Mike Auty suggested for
determining the state of the checkbox and whether or not to show the warning
dialogs.

To do this, I created netwerk/base/public/firebird/security-prefs.js, which is
identical to netwerk/base/public/security-prefs.js, except the security.warn
prefs have been removed.  I then modified netwerk/base/public/Makefile.in to
use netwerk/base/public/firebird/security-prefs.js when MOZ_PHOENIX is defined.
 If MOZ_PHOENIX is not defined, it uses netwerk/base/public/security-prefs.js
as usual.  This should cause the removal of security.warn prefs in
security-prefs.js for Firebird only.

I am attaching netwerk/base/public/firebird/security-prefs.js since CVS gave an
error when I included it in the diff.  The netwerk/base/public/firebird
directory will need to be created and that file will need to be added to it in
addition to checking in the diff.

Comment 36

[Robert Parenton]

Comment on attachment 127386 [details] [diff] [review]
Patch that rolls back the addition of prefs to all.js and completely fixes the bug

Requesting patch review from Brian Ryner.

Comment 37

[Robert Parenton]

Attachment: netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

This file is the security-prefs.js that should be added to
netwerk/base/public/firebird (after that directory is created).  This needs to
be done in addition to checking in the above patch.

Comment 38

[Robert Parenton]

Comment on attachment 127387 [details]
netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

Requesting review of addition required to Firebird by the patch for this bug
from Brian Ryner.

Comment 39

[Robert Parenton]

Just finished building and testing on both Linux and Windows.  The
patch+security-prefs.js combination fixed the bug on both platforms.

If anybody else could test with this patch combination, it would be greatly
appreciated.

Comment 40

[Robert Parenton]

Comment on attachment 127386 [details] [diff] [review]
Patch that rolls back the addition of prefs to all.js and completely fixes the bug

Requesting review from Blake Ross.

Comment 41

[Robert Parenton]

Comment on attachment 127387 [details]
netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

Requesting review from Blake Ross.

Comment 42

[Pierre Chanial]

NO #IFDEF PLEASE !!!

Comment 43

[Robert Parenton]

Pch, I don't quite understand which #ifdef you are referring to.  Are you
talking about the one in nsSecurityWarningDialogs.cpp or the ifdef in Makefile.in?

Since you said #ifdef, I'm assuming that you mean the ones in
nsSecurityWarningDialogs.cpp.  If it is undesirable to have the #ifdef is that
file, it seems like the only way to make this work would be to fork the
nsSecurityWarningDialogs.cpp and add an ifdef to the appropriate Makefile.in. 
Is this correct and a more desirable solution?

Comment 44

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 45

[Mike Connor [:mconnor]]

While I like, in theory, the idea of user opt-in on first warning, I don't think
that we can proceed with that without having UI to enable it.

The question of how to implement this properly without #ifdefs isn't trivial, so
I don't think we're going to make these changes before 0.7, and leaving things
as-is would be detrimental to the release.  In the short term, can we reverse
the defaults to true on the warning prefs, and let users opt out?

bryner, your thoughts on this?

Comment 46

[Robert Parenton]

-> Mike

I have a working version of the patch using a fork of
nsSecurityWarningDialogs.cpp and security-prefs.js.  However, the only fork I
could get working was when I forked the entire directory in which
nsSecurityWarningDialogs.cpp resides, which doesn't seem to be a very good solution.

If anybody can help me with modifying the Makefile.in files and doing the
xpinstall modifications (if they're necessary), I think I could finish it up in
time for 0.7.

Comment 47

[Robert Parenton]

I have a working build of FB built from the current trunk with my updated fix
(only fork the nsSecurityWarningDialogs.cpp instead of its entire directory)
included.

It is a standard optimized Linux build, except it requires a PentiumII or higher
to run (built with --march=pentium2).

If anybody can help test it out while I get the diff for the patch made, I would
appreciate it.  YOU MUST USE A NEW PROFILE.

It is posted at www.louisianaada.org/MozillaFirebird-i686-pc-linux-gnu.tar.gz

Comment 48

[Robert Parenton]

Comment on attachment 127386 [details] [diff] [review]
Patch that rolls back the addition of prefs to all.js and completely fixes the bug

Removing obsolete patch review request.

Comment 49

[Robert Parenton]

Comment on attachment 127387 [details]
netwerk/base/public/firebird/security-prefs.js that goes with the above patch (needs to be copied to netwerk/base/public/firebird)

Removing obsolete patch review request.

Comment 50

[Robert Parenton]

Correct link for the link in Comment #47 :

http://www.louisianaada.org/MozillaFirebird-i686-pc-linux-gnu.tar.gz

Comment 51

[Robert Parenton]

Attachment: Patch that forks nsSecurityWarningDialogs.cpp and security-prefs.js (includes new files so it may need modifications before being checked in)

This is the patch for the changes I made to make the security warning dialogs
work in the build I posted in Comment #47.

-> Mike, Bryner

Can you take a look at this patch?

Comment 52

[Robert Parenton]

Comment on attachment 129948 [details] [diff] [review]
Patch that forks nsSecurityWarningDialogs.cpp and security-prefs.js (includes new files so it may need modifications before being checked in)

Requesting patch review from Brian Ryner

Comment 53

[Michael Gall]

It probably belongs in a new bug, but I can't see a problem with forking those
preference files. There is a hell of a lot of unneeded preferences in FB from
Mozilla suite.

Comment 54

[Az]

michael: it would be helpful if ceated a new bug in which you listed all of
these unneeded preferences.

Comment 55

[Robert Parenton]

If a fork similar to this is the way the devs want to go, I can create a patch
with the desired directory structure.  If another method is preferred, I can
probably create a patch with whatever method is desired.  I just need some input
from the devs.

Comment 56

[Brian Ryner (not reading)]

Comment on attachment 129948 [details] [diff] [review]
Patch that forks nsSecurityWarningDialogs.cpp and security-prefs.js (includes new files so it may need modifications before being checked in)

I think I have something better/safer.

Comment 57

[Brian Ryner (not reading)]

Attachment: proposed fix

Comment 58

[Kai Engert [:KaiE:]]

Comment on attachment 137229 [details] [diff] [review]
proposed fix

Err. I think the user choice simply does not get stored because of the current
code.

Ignore the patch, look at current code: SetBoolPref is only being called if the
new preference is false. If the user toggles the checkbox to checked, the
preference value is true, and the conditional call to SetBoolPref is not
reached.

I think, if you changed the default, all you need is either invert the
condition check, or remove the condition altogether and always call
SetBoolPref.

Comment 59

[Kai Engert [:KaiE:]]

Since the default in Seamonkey is still "true" 
  (which in my personal opinion is the right thing 
   and I would like to remain that way),

and the default in Firebird is "false" 
  (which I don't like, but I can live with),

and the code is shared between both

=> I think the conclusion is to remove the condition altogether.

Comment 60

[Kai Engert [:KaiE:]]

Attachment: The real fix?

Comment 61

[Kai Engert [:KaiE:]]

By always storing the user decision, ignoring the default value, we also make
sure the user's past preference will remain active, whatever default other
versions of the software might think as being appropriate.

Comment 62

[Kai Engert [:KaiE:]]

Err, are we sure this bug is still valid?

I might be doing something wrong, but what I have just tried and seen is
shocking me. Let me say that I'm not a Firebird user, but still stick with
Seamonkey.

I removed my Thunderbird profile directory.
I started Thunderbird.
I went to a secure page (e.g. https://login.yahoo.com)
I did not get a security dialog (for entering a secure site)
I clicked an insecure link in the upper right corner
I again did not get a security warning (for leaving a secure site)

Since we don't even get a warning, I wonder why we bother about checkboxes.

I'm shocked.
I vote for changing the defaults in the preferences to true.

Please tell me I'm doing something wrong.

Comment 63

[Brian Ryner (not reading)]

> I removed my Thunderbird profile directory.
> I started Thunderbird.
> I went to a secure page (e.g. https://login.yahoo.com)

Do you mean Firebird here, not Thunderbird?

What you pointed out is exactly what I'm trying to fix.  The previous fix for
this bug actually disabled the dialogs altogether.  That's not the behavior we
_want_ to have for Firebird.  What we want is for the dialog to show up the
first time, but the default value of the checkbox is to not show the dialog
again.  If the user checks the checkbox, the dialog should show every time, and
the "show every time" checkbox should be initially checked when the dialog shows.

The patch you posted looks to me like it will always turn off the dialogs after
showing them once, even if the user wanted them to keep showing up.  That's not
what either SeaMonkey or Firebird wants.  You could instead change it to set
prefValue instead of PR_FALSE and it would remember the choice as it should, but
that doesn't let us get the behavior I described above for Firebird -- the key
difference is to have a way for the initial value of the checkbox to differ from
the current value of the pref.

Comment 64

[Brendan Eich [:brendan]]

Comment on attachment 137229 [details] [diff] [review]
proposed fix

>@@ -260,6 +286,8 @@ nsSecurityWarningDialogs::ConfirmDialog(
> 
>   if (!prefValue && prefName != nsnull) {
>     mPref->SetBoolPref(prefName, PR_FALSE);
>+  } else if (prefValue && showOnce) {
>+    mPref->SetBoolPref(showOncePref.get(), PR_TRUE);
>   }
> 
>   return rv;

Nit: how about this instead?

  if (!prefValue) {
    if (prefName != nsnull) {
      mPref->SetBoolPref(prefName, PR_FALSE);
    }
  } else {
    if (showOnce) {
      mPref->SetBoolPref(showOncePref.get(), PR_TRUE);
    }
  }

(Hope I'm following the over-braced-one-line-then/else style correctly.)

Comment 65

[Ben Goodger (use ben at mozilla dot org for email)]

sr=[e-mail]

Comment 66

[Bill Gianopoulos [:WG9s]]

I don't really understand any of the proposed patch code.  It seems to add a
bunch of show this only once preferences, none of which are required.  What we
need is a seperate preference for if we should show each type of security
warning.  All of these need to default to true.  Then we need to have the pop-up
warning for each of these include a checkbox which should NOT be checked by
default.  The box should be labeled "Do not show this warning again".  If you
check this box before dismissing the window, then the appropriate show this
specific security warning prefence should be turned off.

Additionally, in the advanced preferences, there needs to be a button added in
the security section to turn all the awarnings back on.

Comment 67

[Brian Ryner (not reading)]

The show_once preferences are required because SeaMonkey and Firebird want to
have different behavior for whether the "show this warning again" checkbox is
checked the first time the dialog appears.

Comment 68

[Brian Ryner (not reading)]

Comment on attachment 137229 [details] [diff] [review]
proposed fix

marking ben's sr= on the patch.

Comment 69

[Brian Ryner (not reading)]

checked in.

Comment 70

[Simon Paquet [:sipaq]]

verified fixed 2004-01-10

Comment 71

[aynilove [So, Jae-yoon]]

2004-01-20 build in windows, it works exactly that reported to this article.
Should it reopened?

Comment 72

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change