15483 - [Dogfood] Account Setup UI has problems disabling remember password

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P3)

Description

[laurel]

Using 10-04m11 build on mac OS 8.5.1
Also saw last week using 9-30 on NT 4.0 and linux

The Account Setup UI doesn't work properly when disabling the remember password
setting (Enabling the remember password setting when already disabled does
indeed work UI-wise and prefs.js-wise).

The UI's checkbox never unchecks to reflect the disabled state in any case.

Although the UI doesn't reflect the disabled state, the prefs.js file shows the
seamonkey style prefs line(s) removed ("mail.server1.remember_password" and its
associated password contents line )

1.  Launch seamonkey to account which has not had its remember password enabled.
2.  In seamonkey, Edit|Account Setup and checkmark/enable the remember password
for that account.  Confirm OK.
3.  Launch the dialog again and see the setting did indeed take.
4.  Look at the prefs.js file and find there is indeed a 5.0 pref for remember
password.
5.  In Account Setup dialog, disable/uncheck the check mail option. Confirm OK.

Result:  Account Setup UI still shows option enabled.  Prefs.js reflects the
option is indeed disabled.

Comment 1

[(not reading, please use [e-mail] instead)]

accepting

Comment 2

[(not reading, please use [e-mail] instead)]

accepting bug.

Comment 3

[Phil Peterson]

M12

Comment 4

[Phil Peterson]

Nominate for dogfood, due to potential security risk

Comment 5

[leger]

Putting on PDT+ radar.

Comment 6

[(not reading, please use [e-mail] instead)]

will look at this today / tomorrow.

Comment 7

[scottputterman]

I was looking for things I could help on and I have a fix for this.  I'll attach
the diff after this. Basically, when we would try to read in the pref in
GetRememberedPassword, it would return -2 since at this point, when the pref is
unset the pref code tries to read the default pref.  -2 is when the default
hasn't been initialized.  And the pref code would return success. So I changed
the pref code to return an error code when the default pref hasn't been
initialized.  I did this for int prefs as well.  I'm not sure what to do with
default character prefs that aren't set.

Comment 8

[scottputterman]

Attachment: patch for this bug

Comment 9

[scottputterman]

I'm not really sure how to verify if anyone else was depending on this to
succeed except to put a break point in there and see if it ever gets hit.

Comment 10

[daver]

So looks like Scott P. has a fix.  Is it in?  Can we mark fix?

Comment 11

[(not reading, please use [e-mail] instead)]

I'm about to investigate his fix.

talking with putterman, it's not really a security problem, because in the worst
case, the ui would be telling you it was remembering your password when it
wasn't (and not the other way around.)

I'm nervous about changing prefs like this so close to m12.  I will probably run
with his change for a while, look at other places where this code would affect
things, and check in for m13.

I'll report back with more info later today, 12-7

Comment 12

[(not reading, please use [e-mail] instead)]

ok, I feel confident about putterman's fix.  it shouldn't break others.

after his fix, we'll be returning the bogus default value, so if someone else
wrote code that didn't check the return value, the behaviour wouldn't change.

if they were checking the return value, but it was never != NS_OK, this fix will
fix them too.

there is a better way to fix this, and that is to remember if the pref has a had
it's default value set or not, instead of relying on -2 for bool prefs and -5236
for int prefs.  (even the original author of that code admits it's a hack)

I'll log that bug to keep track of it.

checking in the fix soon.

adding neeti to the cc list.

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: another implementation of the fix.

Comment 14

[(not reading, please use [e-mail] instead)]

hmm, I won't be checking this in yet.

this code used to return an NS_ERROR_* in this case, but it was removed.  I'm
looking for when and why now.

as alecf pointed out, if we return something other than NS_OK, it will cause any
JS callers to get an exception.

a while back, we kept having this problem, and when ever there wasn't a default
value for a pref, things would break, because usually the exception wasn't
properly handled with try & catch.

before I check this fix in, there is more investigating to do.

moving to m13.

removing 12-7 from the status whiteboard, adding alecf to the cc list.

Comment 15

[scottputterman]

I agree that this bug isn't worth fixing for M12 since I don't think the
security problem exists. But, I still think it should return an error value
regardless of the fact that there's an exception.  There's going to be an error
somewhere - either in the exception or with the value that's returned.

Comment 16

[Phil Peterson]

If the password isn't actually remembered, then we can demote this to PDT-, but
if the password is remembered when it shouldn't be, then I think this is a
legitimate dogfood security issue. Which is it?

Comment 17

[scottputterman]

I'm pretty sure we erase the password. At least it looked like it when stepping
through the code.  I'd have to verify it to be sure.

Comment 18

[scottputterman]

I'm not sure if this is part of this bug or not, but sure enough, using
yesterday's build, the password is not erased from the prefs when remember
password is turned off.  I still get the dialog, but my password is still there.

Comment 19

[(not reading, please use [e-mail] instead)]

putterman, I'm still working on this.  even with the fix to prefapi.c, the UI
checkbox doesn't always behave right.  still investigating.

marking m12, and claiming to fix this by 12-8.

Comment 20

[scottputterman]

My build with the patch doesn't save the password.

This is what I did:

1. made sure I was in remember password state and had saved off password
2. Quit and started up again.
3. Turned off remember password and exit account setup dialog
4. Brought up account set up dialog and exited it without doing anything.
5. Quit program.

In yesterday's build, my password stuck around in the prefs.  In my tree with
the patch it didn't.

However, this may be a rare situation. If I close the account setup dialog
without bringing it up in the same session then the password is not saved.

Comment 21

[(not reading, please use [e-mail] instead)]

ok, thanks to putterman's debugging my patch on windows, we figured out the
problem with my new patch.

the problem was I added a new error code, but didn't update the code that
converts a pref error to a nsresult.

I've fixed that, and now everything work ok.  I'll attach a new version of the
fix for review, which also includes a way to prevent this problem in the future
(some adds a new error code and forgets to update convertRes)

Comment 22

[(not reading, please use [e-mail] instead)]

Attachment: new patch, that actually works and prevents future convertRes problems.

Comment 23

[(not reading, please use [e-mail] instead)]

ok, here's the final patch, with the correct logic in the NS_ASSERTION()

Comment 24

[(not reading, please use [e-mail] instead)]

fixed.

laurel, I'm sure are other bool / int prefs in the
account setup dialog that have the same
behaviour that will be fixed by this bug.

If you can think of those bugs, you may want to re-test
those bugs, as they should be fixed, too.

Comment 25

[laurel]

OK using 12-09-10m12 commercial build on linux 6.0

Comment 26

[laurel]

OK using 12-09-10m12 commercial build on mac OS 8.5.1
OK using 12-09-15m12 commercial build on Win98