Closed
Bug 15483
Opened 25 years ago
Closed 25 years ago
[Dogfood] Account Setup UI has problems disabling remember password
Categories
(SeaMonkey :: MailNews: Message Display, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
M12
People
(Reporter: laurel, Assigned: sspitzer)
Details
(Whiteboard: [PDT+] 12-8)
Attachments
(3 files)
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.
Assignee | ||
Comment 1•25 years ago
|
||
accepting
Assignee | ||
Comment 2•25 years ago
|
||
accepting bug.
Updated•25 years ago
|
Target Milestone: M12
Comment 3•25 years ago
|
||
M12
Updated•25 years ago
|
Summary: Account Setup UI has problems disabling remember password → [Dogfood] Account Setup UI has problems disabling remember password
Comment 4•25 years ago
|
||
Nominate for dogfood, due to potential security risk
Assignee | ||
Updated•25 years ago
|
Whiteboard: [PDT+] → [PDT+] 12/7
Assignee | ||
Comment 6•25 years ago
|
||
will look at this today / tomorrow.
Comment 7•25 years ago
|
||
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•25 years ago
|
||
Comment 9•25 years ago
|
||
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•25 years ago
|
||
So looks like Scott P. has a fix. Is it in? Can we mark fix?
Assignee | ||
Comment 11•25 years ago
|
||
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
Assignee | ||
Comment 12•25 years ago
|
||
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.
Assignee | ||
Comment 13•25 years ago
|
||
Assignee | ||
Updated•25 years ago
|
Whiteboard: [PDT+] 12/7 → [PDT+]
Target Milestone: M12 → M13
Assignee | ||
Comment 14•25 years ago
|
||
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•25 years ago
|
||
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•25 years ago
|
||
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•25 years ago
|
||
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•25 years ago
|
||
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.
Assignee | ||
Updated•25 years ago
|
Whiteboard: [PDT+] → [PDT+] 12-8
Target Milestone: M13 → M12
Assignee | ||
Comment 19•25 years ago
|
||
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•25 years ago
|
||
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.
Assignee | ||
Comment 21•25 years ago
|
||
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)
Assignee | ||
Comment 22•25 years ago
|
||
Assignee | ||
Comment 23•25 years ago
|
||
ok, here's the final patch, with the correct logic in the NS_ASSERTION()
Assignee | ||
Updated•25 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 24•25 years ago
|
||
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.
Reporter | ||
Comment 25•25 years ago
|
||
OK using 12-09-10m12 commercial build on linux 6.0
Reporter | ||
Comment 26•25 years ago
|
||
OK using 12-09-10m12 commercial build on mac OS 8.5.1 OK using 12-09-15m12 commercial build on Win98
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•