Closed Bug 15483 Opened 20 years ago Closed 20 years ago
[Dogfood] Account Setup UI has problems disabling remember password
1.13 KB, patch
|Details | Diff | Splinter Review|
4.03 KB, patch
|Details | Diff | Splinter Review|
5.10 KB, patch
|Details | Diff | Splinter Review|
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.
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
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
OK using 12-09-10m12 commercial build on mac OS 8.5.1 OK using 12-09-15m12 commercial build on Win98
You need to log in before you can comment on or make changes to this bug.