vBulletin (message board) javascript causes wrong username/password to be remembered

VERIFIED FIXED in mozilla1.8.1beta1

Status

()

defect
--
minor
VERIFIED FIXED
15 years ago
11 years ago

People

(Reporter: bugzillamozillaorg, Assigned: mwu)

Tracking

({fixed1.8.1})

1.8 Branch
mozilla1.8.1beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [swag: 0d])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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
(Reporter)

Comment 2

15 years ago
(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?
(Reporter)

Updated

15 years ago
Summary: vBulletin message board username / password are not saved → vBulletin message board username / password are not remembered

Comment 3

15 years ago
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. ***

Comment 7

15 years ago
The point of the password manager is to emulate a user typing in the details. 
So it really shouldn't worry about onsubmit or any Javascript.  It should just
check what's in the password box when the button is clicked and save it.  Then
it can let the form continue with the submission.  Perhaps the summary for this
bug should be changed to something more general like 'password manager should
save passwords before javascript events'

Just my $.02

Comment 8

15 years ago
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.

Comment 9

14 years ago
You can get FF to remember the password by typing something like:

javascript:alert(document.forms[1].submit());

in the URL bar. Firefox will then ask you if you want to save the password.

However, this only works the first time: the next time you log in, Firefox
silently replaces the password with the empty password in the submit event.

Therefore, to fix this I would propose:

1. Fixing the password manager code so that it does not overwrite saved
passwords with empty passwords. Anyone know which files I have to look at?

2a. Changing the Firefox context menu on password fields from "Add a Keyword for
this search" to "Remember this Password"
OR
2b. Creating an extension that adds "Remember this Password" to the context menu
for password fields.
*** Bug 291116 has been marked as a duplicate of this bug. ***
*** Bug 293211 has been marked as a duplicate of this bug. ***

Comment 12

14 years ago
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)
A workaround for this behavior is to disable javascript before entering the
password. Seamonkey (and probably all mozilla-based browser) ask you if you want
to save the password. After a click on yes you can enable javascript again and
the password box will be prefilled like expected.

Bruno
*** 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]

Comment 18

14 years ago
*** Bug 235765 has been marked as a duplicate of this bug. ***

Comment 19

14 years ago
Martin: Would you mind if the summary changed to "vBulletin javascript causes
wrong password value to be remembered"

It seems like the password manager does remember the username, but the password
is blank because vB clears the password for the md5. Checked with Firefox 1.0.7
and Firefox 1.4 20050928 build (Mac).

Perhaps the password manager should try to catch the values before onsubmit
because otherwise forms that modify text/password input fields cause the
password manager to store the new value and not what the user typed in.

A couple possible problems that this currently causes:
- Passwords blanked out before submitting (with a md5-pass hidden field) store a
blank password
- Saved modified password will be loaded upon next visit and onsubmit
preprocessing will be triggered again resulting in a wrong password
Blocks: 212617
OS: Windows XP → All
Hardware: PC → All
Assignee: bryner → nobody
QA Contact: davidpjames → password.manager
Summary: vBulletin message board username / password are not remembered → vBulletin (message board) javascript causes wrong username/password to be remembered

Comment 20

14 years ago
*** Bug 315815 has been marked as a duplicate of this bug. ***

Comment 21

14 years ago
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!

Comment 22

13 years ago
(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? 

Comment 23

13 years ago
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.

Flags: blocking-firefox2?
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+
Keywords: helpwanted
Target Milestone: --- → Firefox 2 beta1
Assignee: nobody → michael.wu
(Assignee)

Updated

13 years ago
Whiteboard: [swag: 1d]
(Assignee)

Comment 25

13 years ago
(In reply to comment #24)
> 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.
> 
That is assuming we're dealing with a submit originating from a button or enter. If there is a custom made button or some other way of submitting the form, it is possible that the script will butcher the password and then call submit(). Hooking in the password manager before the submit event won't do anything because the javascript on the page has already butchered the password before calling submit() itself.
(Assignee)

Comment 26

13 years ago
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)
(Assignee)

Updated

13 years ago
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?
(Assignee)

Comment 28

13 years ago
(In reply to comment #27)
> (From update of attachment 224503 [details] [diff] [review] [edit])
> 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...
(Assignee)

Updated

13 years ago
Attachment #224503 - Flags: review?(bzbarsky) → review?(jst)
(Assignee)

Updated

13 years ago
Whiteboard: [swag: 1d] → [swag: 0d]
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+
Attachment #224503 - Flags: superreview?(bugmail)
(Assignee)

Comment 31

13 years ago
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+
(Assignee)

Comment 33

13 years ago
Sicking's suggestion applied.
Attachment #224503 - Attachment is obsolete: true
Attachment #226426 - Attachment is obsolete: true
(Assignee)

Comment 34

13 years ago
v3 checked in on trunk. Thanks for the reviews!
(Assignee)

Updated

13 years ago
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 35

13 years ago
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
(Assignee)

Comment 36

13 years ago
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 	1.186.4.1
Keywords: helpwantedfixed1.8.1
Version: unspecified → 2.0 Branch
(Assignee)

Updated

13 years ago
Depends on: 343182
(Assignee)

Comment 38

13 years ago
*** Bug 320661 has been marked as a duplicate of this bug. ***

Comment 39

13 years ago
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

Comment 40

13 years ago
Frits, please file a separate bug on that issue with steps to reproduce, etc. Thanks.

Comment 41

12 years ago
I have identified the nasty bit of code responsible for this issue. It's in
the vBulletin backwards compatibility engine, they had to leave in this
automatic form reset for some reason (it just says "for backwards compatibility
with old forum templates"). And although you are able to disable javascript and
thereby login and save the password, I find that troublesome (going into
Options and disabling it, then going back and enabling it). So instead, I
decided to use Greasemonkey to inject a fix that bypasses the form clearing.

Get it from here: http://userscripts.org/scripts/show/8021

It works seamlessly and transparently on all forums. The script has been setup to match sites containing the words *forum*, *board* and *bbs*, so in case your particular forum site doesn't have any of those in the name you can simply add it to the list of included sites.

Unlike the fixed 2.0 beta, this script works on all versions of Firefox that support Greasemonkey (1.5 and higher).
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.