Closed Bug 257781 Opened 19 years ago Closed 18 years ago
590 bytes, text/html
4.67 KB, patch
|Details | Diff | Splinter Review|
4.44 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3 vBulletin is a popular message board system. Unfortunately, username and password used in such a message board are not saved by the password manager of Mozilla Firefox. Reproducible: Always Steps to Reproduce: 1. http://www.flightforum.ch/forum/ (a Swiss-German message board on aviation) 2. Login with username (Benutzername) and password (Kennwort) in the upper right corner Actual Results: Login is successful, however, Mozilla Firefox does NOT pop up the password manager window asking whether you want to save username and password Expected Results: Mozilla Firefox should pop up the standard password manager window asking whether you want to save username and password
WFM Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
(In reply to comment #1) > WFM You're right. I've just noticed that username and password are saved (including the password manager popup window), however, they are not remembered. After having logged out, I tried to login again and I had to enter the password by myself instead of getting it from the password manager. Can you confirm this problem?
Summary: vBulletin message board username / password are not saved → vBulletin message board username / password are not remembered
Mozilla is working as intended. vBulletin 3 uses clientside md5 prior attached to the onsubmit event. It hashes the value in the password field and stores it in a hidden variable and then clears the password field so the password manager never picks it up.
*** Bug 268716 has been marked as a duplicate of this bug. ***
I'd buy this as a wontfix|cantfix, i.e. "we can't change to pick up those without breaking this other thing" but it doesn't strike me as "it's working just the way it should" invalid. The only bad thing I can think of happening from having it remembered at the start of the onSubmit chain rather than at the end is that it would offer to remember things that would then be rejected by client-side validation, but I've only seen that in signups where it's not likely to be remembering anyway. Add in Live Journal, which uses the same login scheme, and there's several million more users, I think something like 7 million active.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 271494 has been marked as a duplicate of this bug. ***
Submit this form and agree to save the password. If you look in your saved passwords, the username is there, but the password is empty. It was cleared by an onsubmit event attached to the form.
*** Bug 291116 has been marked as a duplicate of this bug. ***
*** Bug 293211 has been marked as a duplicate of this bug. ***
I described in bug 293211 that a bookmarklet which neuters the onsubmit from the form is sufficient to have Firefox prompt you about saving your password. However, you must continue to run the bookmarklet EVERY time you log in, or Firefox will oh-so-helpfully update the password to null. (silently! as I ranted in the aforemented bug)
*** Bug 299497 has been marked as a duplicate of this bug. ***
This problem also occurs with Simple Machine Forums.
Simple Machines WFM: http://support.simplemachines.org/demo/index.php test/test, prompted to remember, remembers, and has no script hashing and removing the password before submitting (and if it doesn't do that, no matter whether or not it works it isn't this bug).
(In reply to comment #16) > Simple Machines WFM: http://support.simplemachines.org/demo/index.php test/test, > prompted to remember, remembers, and has no script hashing and removing the > password before submitting (and if it doesn't do that, no matter whether or not > it works it isn't this bug). That's a demo of SMF 1.0.5. This happens only in SMF 1.1 versions (which do onsubmit password hashing, using SHA-1.) I understand this is similar to the way vBulletin and LiveJournal do challenge authentication. Opera actually seems to handle it properly (saving the password before firing onsubmit events), but Internet Explorer does not. -[Unknown]
*** Bug 235765 has been marked as a duplicate of this bug. ***
OS: Windows XP → All
Hardware: PC → All
Assignee: bryner → nobody
QA Contact: davidpjames → password.manager
*** Bug 315815 has been marked as a duplicate of this bug. ***
I read a lot of posts saying that Firefox shows the prompt to save, but it will not work (blank password). This is true with Firefox 1.0, but with Firefox 1.5 it doesn't even show the prompt!
(In reply to comment #19) I am having a problem in FF 1.5 with the same symptoms: login is remembered but password isn't (blank). We have heavy JS on the page and in the form + two password fields with different names. Once you remove the second password field or set it to type="text", FF starts remembering the password as well. The interesting thing is that it did work with both password fields at some point, but I can't reproduce it. Any thoughts?
In the latest nightlys it is still the case that there is no longer a prompt to remember username and password at all. It seems that the password manage is simply broken when it comes to vBulletin sites.
We should try to hook into the password before the submit event gets fired, yes, since in theory when we fill the password later, the same client-side stuff gets run.
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
This attempts to call the password manager earlier when widgets call OnSubmitClickBegin. Both input and button call OnSubmitClickBegin early enough to do the job. Modifying OnSubmitClickBegin also allows us to keep the early password manager notify changes in one file.
Attachment #224503 - Flags: review?(jst)
Attachment #224503 - Flags: review?(jst) → review?(bzbarsky)
Comment on attachment 224503 [details] [diff] [review] Notify password manager about form earlier I'd really prefer, if possible, that someone more familiar than I with this code review this change. I guess jkeiser is not around, so I'd check the blame for who added the code and who reviewed? I'd guess jst and maybe bryner, but check the blame. That said, I'd maybe name "mEarlyNotify" something more like "mNotifiedObservers". I'm also concerned that this will miss whole classes of form submissions (eg submitting via hitting enter in a textfield on a form with no submit button or with just one textfield). Wouldn't it make more sense to notify right before firing the onsubmit event? Or are these sites doing weird stuff before that event even fires?
(In reply to comment #27) > (From update of attachment 224503 [details] [diff] [review] ) > I'd really prefer, if possible, that someone more familiar than I with this > code review this change. I guess jkeiser is not around, so I'd check the blame > for who added the code and who reviewed? I'd guess jst and maybe bryner, but > check the blame. > I originally asked jst for a review, but it seems that he's gonna be out, so I picked someone else from the cvs logs. > That said, I'd maybe name "mEarlyNotify" something more like > "mNotifiedObservers". > Sure. > I'm also concerned that this will miss whole classes of form submissions (eg > submitting via hitting enter in a textfield on a form with no submit button or > with just one textfield). > Hitting enter will work - we get the event before the page does. However, there is one class of form submission that this will miss, which I commented on earlier. A page can make a custom submit button which butchers the fields and calls submit() itself. This patch will only work for the cases where we submit via hitting enter or hitting a (real) submit button. If a page really wants to screw with the password manager, it can. > Wouldn't it make more sense to notify right before firing the onsubmit event? > Or are these sites doing weird stuff before that event even fires? > No big reason behind putting the early notify code where it is now. It's just that I get to put in the early notify changes in one file instead of three this way. Also, this file has a helper for notifying the password manager so I don't need to copy that helper code to the other files.
> Hitting enter will work - we get the event before the page does. We do? Why is that case different from the button-click case? > A page can make a custom submit button Yeah, I don't think we can really do much about that case. ;) You want reviewers from the blame for the code you'r changing, not just from the CVS logs for the files, basically. I know things about parts of these files, but not other parts...
Attachment #224503 - Flags: review?(bzbarsky) → review?(jst)
Comment on attachment 224503 [details] [diff] [review] Notify password manager about form earlier - In nsHTMLFormElement::OnSubmitClickBegin(): + PRBool aCancelSubmit = PR_FALSE; I know you're not the first one to do this in this file, but please don't name local variables _a_Something, that's the naming convention we use for function arguments, so name the above boolean cancelSubmit to avoid that confusion, and while you're at it, rename the same variable in nsHTMLFormElement::SubmitSubmission() as well. As for bz's concerns about whether this does the right thing if the user hits enter in a text field, that *will* trigger a synthesized left-click event, but that event is triggered after the enter key-up event is processed. So the synthetic click event is what triggers the submit, so that part should be fine as long as the page doesn't trigger any weird JS that laters the password etc from the enter key event(s). r=jst with the above naming change and bz's suggsted naming change. sicking should probably sr this one.
Attachment #224503 - Flags: review?(jst) → review+
Name changes by bz and jst. Otherwise, basically identical to before.
Comment on attachment 226426 [details] [diff] [review] Notify password manager about form earlier, v2 >+ // >+ // Notify observers of submit >+ // >+ PRBool cancelSubmit = PR_FALSE; >+ rv = NotifySubmitObservers(actionURI, &cancelSubmit); >+ if (NS_SUCCEEDED(rv)) >+ mNotifiedObservers = PR_TRUE; >+ >+ mNotifiedObserversResult = cancelSubmit; Nit: Put last statement inside the |if| as well for clarity (since we'll ignore the value anyway).
Attachment #226426 - Flags: superreview+
Sicking's suggestion applied.
v3 checked in on trunk. Thanks for the reviews!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060623 Minefield/3.0a1. The Save Password dialog now shows and when I choose "Remember Password" it gets saved.
Status: RESOLVED → VERIFIED
This patch should be fairly safe to put in and gives form/password managers more correct behavior when it comes to saving forms. (forms are saved before the page gets to change the form submitted)
Attachment #226853 - Flags: approval1.8.1?
Attachment #226853 - Flags: approval1.8.1? → approval1.8.1+
Checked in on the 1.8 branch. mozilla/content/html/content/src/nsHTMLFormElement.cpp 188.8.131.52
*** Bug 320661 has been marked as a duplicate of this bug. ***
Still got a problem with the password manager. If you go to www.dimeadozen.com and you give your username and password FF 2.0 says do you wanna keep this YES. The next time you wanna go to this page the username and password are not in the login scream. There has been a discussion on: http://www.mozbrowsec.php?t=8671&postdays=0&postorder=asc&start=0r.nl/forum/viewtopi
Frits, please file a separate bug on that issue with steps to reproduce, etc. Thanks.
You need to log in before you can comment on or make changes to this bug.