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

VERIFIED FIXED in Firebird0.8

Status

()

defect
VERIFIED FIXED
17 years ago
13 years ago

People

(Reporter: mozilla, Assigned: bryner)

Tracking

unspecified
Firebird0.8
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 years ago
blake, this is all you. we like the default but it shouldn't override user
selection. 
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

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

Updated

17 years ago
Target Milestone: --- → Phoenix0.6

Comment 3

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

Comment 4

17 years ago
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.

Updated

16 years ago
OS: Linux → All

Updated

16 years ago
Summary: Pop-up Security Warning Alert is not 'sticky' → Encryption security warnings: "alert me whenever" should stay checked

Comment 6

16 years ago
*** Bug 199826 has been marked as a duplicate of this bug. ***
> as a reminder, the problem is here:

then, simply delete line #155 to #157 will solve this problem - Why Not?
-> 0.7
Target Milestone: Phoenix0.6 → Phoenix0.7

Comment 9

16 years ago
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

16 years ago

Comment 11

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

Requesting review from Blake Ross
Attachment #123100 - Flags: review?(blaker)

Comment 12

16 years ago
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.
Attachment #123100 - Flags: review?(blaker) → review?(bryner)
(Assignee)

Updated

16 years ago
Attachment #123100 - Flags: review?(bryner) → review+
(Assignee)

Comment 13

16 years ago
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?
(Assignee)

Comment 14

16 years ago
I think this should do the trick.
Attachment #123100 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

16 years ago
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

16 years ago
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

16 years ago
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?
We also need a patch for backing out the one that was checked in. pch, can you 
take a look? Or bryner?

--> bryner
Assignee: blaker → bryner
Status: REOPENED → NEW
(Assignee)

Comment 21

16 years ago
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

16 years ago
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?
(Assignee)

Comment 23

16 years ago
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

16 years ago
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

16 years ago
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.
>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

16 years ago
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)
> 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

16 years ago
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?

Updated

16 years ago
Attachment #123100 - Attachment is obsolete: false

Comment 30

16 years ago
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

16 years ago
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

16 years ago
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.
Attachment #123100 - Attachment is obsolete: true

Comment 33

16 years ago
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

16 years ago
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?).
Attachment #127135 - Attachment is obsolete: true

Comment 35

16 years ago
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

16 years ago
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.
Attachment #127386 - Flags: review?(bryner)

Comment 37

16 years ago
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

16 years ago
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.
Attachment #127387 - Flags: review?(bryner)

Comment 39

16 years ago
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

16 years ago
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.
Attachment #127386 - Flags: review?(bryner) → review?(blakeross)

Comment 41

16 years ago
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.
Attachment #127387 - Flags: review?(bryner) → review?(blakeross)

Comment 42

16 years ago
NO #IFDEF PLEASE !!!

Comment 43

16 years ago
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?
taking QA contact, sorry about the bugspam
QA Contact: asa → 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

16 years ago
-> 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

16 years ago
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

16 years ago
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.
Attachment #127386 - Attachment is obsolete: true
Attachment #127386 - Flags: review?(blake)

Comment 49

16 years ago
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.
Attachment #127387 - Attachment is obsolete: true
Attachment #127387 - Flags: review?(blake)

Comment 51

16 years ago
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

16 years ago
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
Attachment #129948 - Flags: review?(bryner)

Updated

16 years ago
Target Milestone: Firebird0.7 → Firebird0.8

Comment 53

16 years ago
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

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

Comment 55

16 years ago
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.
(Assignee)

Comment 56

16 years ago
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.
Attachment #129948 - Flags: review?(bryner) → review-
(Assignee)

Comment 57

16 years ago
Posted patch proposed fixSplinter Review
(Assignee)

Updated

16 years ago
Attachment #137229 - Flags: superreview?(darin)
Attachment #137229 - Flags: review?(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.
Attachment #137229 - Flags: review?(kaie) → review-
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.

Updated

16 years ago
Attachment #137406 - Flags: review?(bryner)
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.
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.

Updated

16 years ago
Attachment #137406 - Flags: review?(bryner)
(Assignee)

Comment 63

16 years ago
> 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 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.)
Attachment #137229 - Flags: review- → review+
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.
(Assignee)

Comment 67

16 years ago
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.
(Assignee)

Comment 68

16 years ago
Comment on attachment 137229 [details] [diff] [review]
proposed fix

marking ben's sr= on the patch.
Attachment #137229 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 69

16 years ago
checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
verified fixed 2004-01-10
Status: RESOLVED → VERIFIED
2004-01-20 build in windows, it works exactly that reported to this article.
Should it reopened?
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → preferences
You need to log in before you can comment on or make changes to this bug.