Closed Bug 15483 Opened 20 years ago Closed 20 years ago

[Dogfood] Account Setup UI has problems disabling remember password

Categories

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

PowerPC
All
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: laurel, Assigned: sspitzer)

Details

(Whiteboard: [PDT+] 12-8)

Attachments

(3 files)

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.
QA Contact: lchiang → laurel
accepting bug.
Target Milestone: M12
M12
Summary: Account Setup UI has problems disabling remember password → [Dogfood] Account Setup UI has problems disabling remember password
Nominate for dogfood, due to potential security risk
Whiteboard: [PDT+]
Putting on PDT+ radar.
Whiteboard: [PDT+] → [PDT+] 12/7
will look at this today / tomorrow.
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.
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.
So looks like Scott P. has a fix.  Is it in?  Can we mark fix?
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
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.
Whiteboard: [PDT+] 12/7 → [PDT+]
Target Milestone: M12 → M13
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.
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.
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?
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.
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.
Whiteboard: [PDT+] → [PDT+] 12-8
Target Milestone: M13 → M12
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.
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.
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)
ok, here's the final patch, with the correct logic in the NS_ASSERTION()
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
OK using 12-09-10m12 commercial build on linux 6.0
Status: RESOLVED → VERIFIED
OK using 12-09-10m12 commercial build on mac OS 8.5.1
OK using 12-09-15m12 commercial build on Win98
Blocks: 21564
No longer blocks: 21564
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.