270558 - Password manager should not fill in username and password if the username field is pre-filled by the server to a different username. (frequent problem with phpBB admin interface)

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Thom Unger]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

The password manager is filling in text fields on pages where it's not been
asked to.

I discovered this problem while administrating a phpBB (www.phpbb.com) message
board.


Reproducible: Always
Steps to Reproduce:
1. When using phpBB, I log on to the board using the "admin" account and save
the username and password for future use.
2. I then go to the admin panel to lookup user information (Admin Panel->User
Admin->Management).  
3. I enter a username, and the form appears with the user information, but the
username data in the username text field is immediately overwritten with the
word "admin".  
4. If I view the source of the page, the correct username is there.


Actual Results:  
The incorrect username is being displayed, even though the html source has the
correct username.  If I then hit "Submit" to save the data, a box pops up asking
me to "Confirm Password Change".  When I click "Cancel", it goes ahead and
submits the page, but with the incorrect username.

Expected Results:  
Left the username field alone and not prompted me to "Confirm Password Change"
when the page has nothing to do with that.

It is a potentially major bug, given that valid data could be easily overwritten
in this example, and presumably elsewhere as well.

I suspect it has to do with the following: the userdata is being displayed in a
frame that was opened by the original
http://localhost/components/com_forum/admin/index.php that handled the initial
logon.  Even though the sub-frame? is a different url
(http://localhost/components/com_forum/admin/admin_users.php), the password
manager thinks it's still on the index.php page, and the textfield is presumably
named the same as the logon text field.

Comment 1

[Mike Connor [:mconnor]]

Its more than password manager saves passwords by domain, not by page.  And of
course the phpBB form has the names "username" and "password" iirc, which just
makes for great fun.  Quick'n'dirty hack is to hack the board to use different
fieldnames and make sure to unhack them after the form submit, but I'll admit
that's kinda annoying.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

The password "memory" is per domain, not per page. Whether it should be a per
page setting is debatable, since sometimes you'd prefer the password to remain
regardless of any site changes (ie. login page is changed from login.htm to
login.html on the site). Using the full URL isn't really reliable anyways, since
they're often dynamic and can vary.

Comment 3

[Phil Ringnalda (:philor)]

*** Bug 273305 has been marked as a duplicate of this bug. ***

Comment 4

[Phil Ringnalda (:philor)]

*** Bug 259760 has been marked as a duplicate of this bug. ***

Comment 5

[Tom Brennan]

Someone with sufficient authorisation might want to compare to bug 112260, as it
appears to be the same problem

Comment 6

[:Gavin Sharp [email: [e-mail]]]

*** Bug 276514 has been marked as a duplicate of this bug. ***

Comment 7

[Phil Ringnalda (:philor)]

Attachment: testcase for saving

Comment 8

[Phil Ringnalda (:philor)]

Attachment: testcase for clobbering

Comment 9

[Phil Ringnalda (:philor)]

This is actually a regression of this particular case, from bug 229762 - the fix
there didn't quite match Ben's suggested logic, which was

if (page default value)
  if (page default value == any of the saved signons)
    pwmgr fill password for that signon
  else
    do nothing, attach autocomplete widget.
else
  if (more than one signon)
    do nothing, attach autocomplete widget
  else
    prefill username and password

what we have now instead is

if (page default value)
  if (page default value == any of the saved signons)
    pwmgr fill password for that signon
  else
    if (more than one signon)
      do nothing, attach autocomplete widget.
    else
      prefill username and password
else
  if (more than one signon)
    do nothing, attach autocomplete widget
  else
    prefill username and password

which makes for a rather odd workaround for forum admins: save two logins, and
when you log in you'll have to click, down-arrow in the username textbox to use
autocomplete to pick the real one, but then on user admin pages pwdmgr won't
clobber the HTML-supplied username and password.

STR:

1. Load attachment 170787 [details] and submit any user/pass, and let pwdmgr save it
2. Load attachment 170788 [details] and note that your user/pass is filled in, clobbering
what the HTML supplied.
3. Back to attachment 170787 [details] and have pwdmgr save a different user/pass.
4. Back to attachment 170788 [details] and note that the "HTML supplied" user/pass is
filled in.

Comment 10

[Phil Ringnalda (:philor)]

*** Bug 277687 has been marked as a duplicate of this bug. ***

Comment 11

[Måns Jonasson]

I have to chip in on the problems with this. I seems that it would be a lot
smarter for the password autofill to only kick in if the form fields on the page
are blank - not when they have a value set by the HTML code. 

This is creating severe problems with several of the applications we use here at
work. 

Comment 12

[Thomas K. (:tom)]

*** Bug 296841 has been marked as a duplicate of this bug. ***

Comment 13

[Mike Connor [:mconnor]]

this may be related/a dupe, but if bryner has cycles I'd like to get this.

Comment 14

[Phil Ringnalda (:philor)]

*** Bug 306121 has been marked as a duplicate of this bug. ***

Comment 15

[Dave Townsend [:mossop]]

*** Bug 290298 has been marked as a duplicate of this bug. ***

Comment 16

[Dave Townsend [:mossop]]

*** Bug 306298 has been marked as a duplicate of this bug. ***

Comment 17

[Phil Ringnalda (:philor)]

*** Bug 259841 has been marked as a duplicate of this bug. ***

Comment 18

[Josh Watson]

This bug is still present in Firefox 1.5 Beta 1:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

Comment 19

[Christian Schmidt]

*** Bug 309312 has been marked as a duplicate of this bug. ***

Comment 20

[Edward Z. Yang]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Confirming, I see it too. Doesn't seem to have disappeared in Firefox 1.5.

Comment 21

[tech]

(In reply to comment #19)
> *** Bug 309312 has been marked as a duplicate of this bug. ***
> 

I'm sorry to insist but these bugs can't be duplicates because 270558 is a mozilla-firefox bug and 309312 is on Mozilla Application Suite

Comment 22

[Phil Ringnalda (:philor)]

Attachment: fix

Remember whether it was prefilled until the "fill one un/pw pair" step.

Comment 23

[Jesse Ruderman]

*** Bug 324411 has been marked as a duplicate of this bug. ***

Comment 24

[Phil Ringnalda (:philor)]

Attachment: unrotted

Comment 25

[Brian Ryner (not reading)]

Comment on attachment 212419 [details] [diff] [review]
unrotted

Looks ok, but can you add a comment about what prefilledUser indicates?

Post a new patch with the added comment and I'll go ahead and check it in if you'd like.  Thanks!

Comment 26

[Phil Ringnalda (:philor)]

Gah, just like it should, writing documentation (down at the "if (firstMatch && !attachedToInput" where I was most confused) made me realize I'd done it wrong: if the page prefills, and we have one saved username that doesn't match, we still need to attach the autocomplete listener so we can fill the password if the user changes to our username. New patch once I sort that out.

Comment 27

[Phil Ringnalda (:philor)]

Attachment: Fixed and commented

Filed bug 327977 on overwriting a password-only form, because that's either unsolvable or needs a different solution. This patch should cover every case for username/password forms, if I didn't miss anything in:

not prefilled
  no saved
  one saved
  two saved
    after filling first, change to second
user prefilled, pass not
  no saved
  one (same as prefill) saved
  one (not same as prefill) saved
  two (one is prefill) saved
    change to second
  two (neither is prefill) saved
    change to first
    change to second
user and pass prefilled
  no saved
  one (same as prefill) saved
  one (not same as prefill) saved
  two (one is prefill) saved
    change to second
  two (neither is prefill) saved
    change to first
    change to second

Comment 28

[Brian Ryner (not reading)]

checked in, thanks.

Comment 29

[Brian Ryner (not reading)]

Comment on attachment 212538 [details] [diff] [review]
Fixed and commented

branch? i think so

Comment 30

[:Gavin Sharp [email: [e-mail]]]

patch doesn't seem to apply cleanly to the branch, didn't really look into it

Comment 31

[Phil Ringnalda (:philor)]

It should just need bug 235336 to bitrot it again on the branch, and then the current patch should apply fine over that.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

See also bug 112260 (Seamonkey)

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #27)
> Created an attachment (id=212538) [edit]
> +    // and fill in the username and password  only if the HTML didn't prefill
> +    // the username.

This is really annyoing on sites that prefill their login field with "name", "login" or s.th. like that; which is quite common, I think.
Some days ago I thought this new behaviour was a bug. Now I'm kind of irritated to see that it's a bug fix.

Comment 34

[Phil Ringnalda (:philor)]

*** Bug 331441 has been marked as a duplicate of this bug. ***

Comment 35

[Phil Ringnalda (:philor)]

*** Bug 334237 has been marked as a duplicate of this bug. ***

Comment 36

[Vasya Pupkin]

Bloscks 1.5.0.2

Comment 37

[HyperHacker]

(In reply to comment #33)
> (In reply to comment #27)
> > Created an attachment (id=212538) [edit]
> > +    // and fill in the username and password  only if the HTML didn't prefill
> > +    // the username.
> 
> This is really annyoing on sites that prefill their login field with "name",
> "login" or s.th. like that; which is quite common, I think.
> Some days ago I thought this new behaviour was a bug. Now I'm kind of irritated
> to see that it's a bug fix.
> 

Yes, quite a number of different forum softwares have "username" and "password" prefilled in the login form.
This bug can be especially troublesome on forums that use the same field name for multiple purposes. Acmlmboard demonstrates this; the names that are used for the username and password on the login page are also used to change a user's name and password on the user editing admin panel. Having your password remembered means any time you use the user editor panel, you have to remember to clear out the password box and enter their username, or else their name/pass is changed to yours. I suspect many similar apps do the same.

Comment 38

[tech]

I would like to know when the fixed version be released

Comment 39

[Phil Ringnalda (:philor)]

*** Bug 335719 has been marked as a duplicate of this bug. ***

Comment 40

[bart]

There are still major bugs in the behaviour of the password manager.. on our customer extranet, the password manager pops up whenever a customer tries to change the password of a mailbox, ftp password, database password or other password in an html form with password fields.  These passwords have nothing to do with the password manager, that keeps popping up just because the password manager has an entry in the same domain... In my opinion the password manager should look at the domain IN COMBINATION with the names of the input fields.. The page url is not an option because this can change often and there can be multiple html password form on the same url... One other thing : when I have a form with 3 password fields (one for the current password, 2 for the new passwords) which field is considered ? The password manager pops up even if the new passwords do not match... 

> The password "memory" is per domain, not per page. Whether it should be a per
> page setting is debatable, since sometimes you'd prefer the password to remain
> regardless of any site changes (ie. login page is changed from login.htm to
> login.html on the site). Using the full URL isn't really reliable anyways, since
> they're often dynamic and can vary.

Comment 41

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060803 BonEcho/2.0b1.

Using attachment 170788 [details] the password manager does not override the pre-filled text with my saved password.

Comment 42

[Cri]

(In reply to comment #2)
> The password "memory" is per domain, not per page. Whether it should be a per
> page setting is debatable, since sometimes you'd prefer the password to remain
> regardless of any site changes (ie. login page is changed from login.htm to
> login.html on the site). Using the full URL isn't really reliable anyways, since
> they're often dynamic and can vary.

What about adding an option to let the user choose whether to remember passwords per page or per domain?

There are cases when remembering per domain is just not useful, and remembering per page is required, and doesn't have side-effects (e.g. mailman administration of multiple mailing lists on the same domain: only the password of one list is currently remembered)

Comment 43

[Daniel Kabs, reporting bugs since 2002]

(In reply to Adam Guthrie from comment #41)
> Using attachment 170788 [details] the password manager does not override the
> pre-filled text with my saved password.

Not always.

PW-mgr clobbers the pre-filled password with the saved password as soon as I click the username field and press submit. Why is this? Does this warrant a reopen or even a new bug?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0) Gecko/20100101 Firefox/11.0