917325 - Password manager shouldn't autofill into username fields with inappropriate @autocomplete values

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Craig Francis]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

Add a "saved password" for the website:

http://www.thriveapproach.co.uk/

For example, one for an admin account, then load the member register page:

http://www.thriveapproach.co.uk/member/register/


Actual results:

The postcode field (one back from the new password field) is assumed to be the username for the saved password.


Expected results:

Please see the HTML5 spec for the new autocomplete attribute:

http://www.w3.org/TR/html5/forms.html#autofilling-form-controls:-the-autocomplete-attribute

Ideally it the password manager would see this as being an inappropriate field for the username.

Comment 1

[Loic]

Do I need to create a fake account to test?

Comment 2

[Craig Francis]

I don't think so... I just don't know the FireFox password manager well enough to manually enter a username/password for a domain.

As in, getting a record to appear in "Preferences > Security > Saved Passwords".

Comment 3

[Loic]

Could you add clear STR to reproduce the bug, step by step, please.

Comment 4

[Craig Francis]

1) Use a fictional username/password on this dummy page:

http://www.thriveapproach.co.uk/test/

2) When you submit the form, use the password manager (little key in address bar if hidden) to "Remember password".

3) Now view the register page:

http://www.thriveapproach.co.uk/member/register/

The form should be blank, but the password manager has tried to fill out the postcode field with the username entered above.

Comment 5

[Loic]

It's clear now. :)
Same issue in old versions too.

Comment 6

[Matthew N. [:MattN]]

I can reproduce this (although there is now a maxlength=8 on the postcode field so you need to test with a username <= 8 characters).

Comment 7

[Edwin Wong [:edwong]]

Setting as P3 - is this something we want to fix this qtr?

Comment 8

[info]

I think what I've got here regards to this ticket. Therefor here another way trying to explain this faulty behavior.

Headline:
No or falls autocomplete and wrong autofill on wrongly detected username password field combinations

Login to foo

URL //example.org/foo/
<form action="foo/">
  <input type="text" name="username"> name
  <input type="password" name="password"> pass
</form>
Save password

Scenario 1:
There's only one saved password for foo

Next form has nothing to do with login above only the domain is the same.

URL //example.org/foo/bar.html
<form action="foo/bar.html">
  <input type="text" name="username">
  <input type="text" name="email"> name <false> <no autocomplete with emails>
  <input type="password" name="password"> pass <false>
  <input type="password" name="password_confirm">
</form>
In this form the email field gets wrongly filled with saved username from http://example.org/foo/.

Additionaly now there's no autocomplete functionality with former entered email addresses on field email.

Field "password" also gets wrongly filled with saved password.

Scenario 2:
There are multiple saved passwords for foo

URL //example.org/foo/bar.html
<form action="foo/bar.html">
  <input type="text" name="username">
  <input type="text" name="email"> <no autocomplete with emails>
  <input type="password" name="password">
  <input type="password" name="password_confirm">
</form>
Now the fields correctly does not get autofilled with values as there are multiple options.

But autocomplete functionality on email field shows values of saved usernames instead of former entered email addresses.

IMHO password manager should observe the URL or forms ACTION attribute before doing autofill and autocomplete.

Comment 9

[Dan]

Seen the same problem of username being placed in fields that have nothing to do with logging in.

For example, on a form for editing user details:

<form>
<input type='text' name='username'>
<input type='text' name='firstname'>
<input type='text' name='lastname'>
<input type='password' name='pw' autocomplete='off'>
</form>

The "lastname" gets filled with my login username, and the "pw" field gets filled with my password.  I realize the second is a result of more herculean efforts by Mozilla to make sure password fields can be filled in even when autocomplete is disabled, but putting my username into the "lastname" field is a bit much.

The password manager is trying too hard to fill in forms to the point that it fills out forms that aren't even login forms.  Restricting the saved password to those with the same ACTION would not prevent saved psswords from working, but it would fix the issue I describe.  It might lead to having to save your password multiple times on a site that used login forms with different ACTIONs, but that seems like an acceptable compromise.

This behavior has recently gotten much worse, and I'm not sure if I just noticed, or it was a result of https://bugzilla.mozilla.org/show_bug.cgi?id=956906

Comment 10

[vinnyjames]

This is a problem on all sites where you can edit your username/password and all sites where you can edit other users' credentials.  It appears the problem spawns from banks and the like that try to disable this on forms as they see fit instead of allowing the user to make that decision.

The proposed solution does not put the onus on the company that is misusing autocomplete but rather the random innocents like admins and developers who have access to many forms with username+passwords that are not their logins.

It would be more just to notify end-users that a site is trying to prevent them from saving their credentials and allowing them to override it.  This puts the blemish where it belongs instead of inconveniencing the rest of the world.

Comment 11

[Dan]

I agree with the idea of giving the user the option of overriding the behavior of autocomplete.  This however, does not solve the problem where the user has elected to save the password for a site, but then uses another form where password (and username) should not be autofilled.

Since it is often site admins that need this functionality, perhaps an option could be included to prevent it from applying to certain actions, rather than trying to restrict it to the original action of the login form (which could change for websites and annoy end users).

I think my preferred solution is to popup a box that asks if you want to autofill this form with your password when a new form with a new action is loaded.  The user can simply click "yes" and have this form added to their list of forms that are ok to autofill (the original having been automatically added when Fx asked if they wanted to save the password).  As a backwards compatibility measure, those saved passwords that had no action saved along with them could apply to all forms on a site until the user manually updates them.

Comment 12

[Matthew N. [:MattN]]

(In reply to Dan from comment #9)
> <form>
> <input type='text' name='username'>
> <input type='text' name='firstname'>
> <input type='text' name='lastname'>
> <input type='password' name='pw' autocomplete='off'>
> </form>
> 
> The "lastname" gets filled with my login username, and the "pw" field gets
> filled with my password.  

This bug is about autocomplete attributes with values other than on/off e.g. autocomplete="family-name" / autocomplete="new-password" and since you're not using those this bug won't help you. You can add them to get the benefits when we fix this bug (which I hope to fix this year) but in the meantime I recommend reordering the fields so the username field is before the password field as that will help in the edit case since we will see a different username and not try to fill the user's password there.

(In reply to vinnyjames from comment #10)
Please use autocomplete="new-password" on fields where an admin is setting a new password for a user in order to get the benefit when this bug is fixed.

(In reply to Dan from comment #11)
> Since it is often site admins that need this functionality, perhaps an
> option could be included to prevent it from applying to certain actions,
> rather than trying to restrict it to the original action of the login form
> (which could change for websites and annoy end users).

This option already exists: You can flip the pref signon.autofillForms in about:config to false to disable autofill.

Comment 13

[vinnyjames]

Thanks for the clarification Matthew

Comment 14

[Dan]

(In reply to Matthew N. [:MattN] from comment #12)
> This bug is about autocomplete attributes with values other than on/off e.g.
> autocomplete="family-name" / autocomplete="new-password" and since you're
> not using those this bug won't help you. You can add them to get the
> benefits when we fix this bug (which I hope to fix this year) but in the
> meantime I recommend reordering the fields so the username field is before
> the password field as that will help in the edit case since we will see a
> different username and not try to fill the user's password there.

Well, I posted here for lack of a better bug to discuss this issue.  But, I can create a new bug if that's preferred.  Your idea would help a bit, but it doesn't help for new users.

> 
> (In reply to vinnyjames from comment #10)
> Please use autocomplete="new-password" on fields where an admin is setting a
> new password for a user in order to get the benefit when this bug is fixed.
 
How long before banks start using that to prevent us from saving passwords again?  It seems to me that autocomplete/autofill are two different things.  Why mess with autocomplete keyword further in this way?

> (In reply to Dan from comment #11)
> > Since it is often site admins that need this functionality, perhaps an
> > option could be included to prevent it from applying to certain actions,
> > rather than trying to restrict it to the original action of the login form
> > (which could change for websites and annoy end users).
> 
> This option already exists: You can flip the pref signon.autofillForms in
> about:config to false to disable autofill.

I think you must have misunderstood me.  Turning it off for the whole browser is not the same as restricting it to certain "actions" (by which I mean the "action" attribute of the "form" element) on a certain site.  The idea was that if there were two forms:

<form action="/login" />
<form action="/createuser" />

you could right click in the field/form somewhere, and indicate that for the "/createuser" action, autofill was not to occur (leaving it to occur on the "/login" action).  In this way, forms that annoyingly have the wrong data filled could be blacklisted.

However, I like my other idea better, that of asking users if they want to fill a form that has a different "action" attribute than the one that was saved.

Comment 15

[vinnyjames]

(In reply to Dan from comment #14)
> How long before banks start using that to prevent us from saving passwords
> again?  It seems to me that autocomplete/autofill are two different things. 
> Why mess with autocomplete keyword further in this way?

Is this chronology correct?

1. websites like banks disable autofill with autocomplete (not sure why we're picking on banks but seems like the most plausible culprit)
2. users grab pitchforks and demand autocomplete is no longer honored by the browsers to disable autofill
3. new feature to identify fields as specific types of fields gives websites the exact functionality they misused previously

Naturally I prefer my solution of putting the egg on the appropriate site's face and not in the laps of the masses.

Comment 16

[Dan]

(In reply to vinnyjames from comment #15)
> (In reply to Dan from comment #14)
> > How long before banks start using that to prevent us from saving passwords
> > again?  It seems to me that autocomplete/autofill are two different things. 
> > Why mess with autocomplete keyword further in this way?
> 
> Is this chronology correct?
> 
> 1. websites like banks disable autofill with autocomplete (not sure why
> we're picking on banks but seems like the most plausible culprit)
> 2. users grab pitchforks and demand autocomplete is no longer honored by the
> browsers to disable autofill
> 3. new feature to identify fields as specific types of fields gives websites
> the exact functionality they misused previously
> 
> Naturally I prefer my solution of putting the egg on the appropriate site's
> face and not in the laps of the masses.

Sounds about right. See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=956906

From the first post:

> This behavior is a concession to sites that think password managers are harmful and thus want to 
> prevent them from being effective. In aggregate, I think those sites are generally wrong, and 
> shouldn't have that much control over our behavior.
> I think we should investigate removing support for autocomplete="off" entirely, or at least the 
> portion of it that prevents us from saving passwords.

Then, see https://bugzilla.mozilla.org/show_bug.cgi?id=1119063

First post:

> A risk is that it will be used to prevent autofill on login forms i.e. a new way to do 
> @autocomplete=off that goes against the user's control

Perhaps we should take our crusade over there.  :)  However, that bug is specifically for adding the "new-password" option, so I don't see it as the right place to discuss the bad autofill behavior.

Comment 17

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

@Dan and @Vinnyjames: the discussion on this bug tailed of six months back. Do you have a way to move this forward, or should I moved this out of ASSIGNED and unassign it from @MattN?

Comment 18

[Matthew N. [:MattN]]

This is just blocked on the dependency (bug 1109188)

Comment 20

[:Gijs (he/him)]

Matt, looks like the dep for this got fixed (the dupe of the bug in comment #18, that is). What's the current status?

Comment 21

[Matthew N. [:MattN]]

We're going to look into this soon. It's complicated because we can't blindly trust @autocomplete.

Comment 22

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 917325 - Consider autocomplete fieldname when determining what is a username field. r?MattN

Comment 23

[Matthew N. [:MattN]]

Somehow I forgot "on"…

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64e2378a756b
Consider autocomplete fieldname when determining what is a username field. r=MattN

Comment 25

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on /test_autofill_autocomplete_types.html

backout: https://hg.mozilla.org/integration/autoland/rev/d4e6ec91e2c3f2d26205485b74e15e5320c728d2

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C4.3%2Capi16%2B%2Copt%2Cmochitests%2Ctest-android-em-4.3-arm7-api-16%2Fopt-mochitest-24%2Cm%2824%29&revision=64e2378a756b10054fc197bced32f8a587bee8f5

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=232779138&repo=autoland&lineNumber=2354

[task 2019-03-08T22:52:49.862Z] 22:52:49 INFO - 403 INFO TEST-START | toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html
[task 2019-03-08T22:53:00.085Z] 22:53:00 INFO - 404 INFO TEST-OK | toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html | took 11371ms
[task 2019-03-08T22:53:00.087Z] 22:53:00 INFO - 405 INFO TEST-START | Shutdown
[task 2019-03-08T22:53:00.088Z] 22:53:00 INFO - 406 INFO Passed: 25
[task 2019-03-08T22:53:00.088Z] 22:53:00 INFO - 407 INFO Failed: 0
[task 2019-03-08T22:53:00.088Z] 22:53:00 INFO - 408 INFO Todo: 0
[task 2019-03-08T22:53:00.088Z] 22:53:00 INFO - 409 INFO Mode: non-e10s
[task 2019-03-08T22:53:00.090Z] 22:53:00 INFO - 410 INFO Slowest: 11371ms - /tests/toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html
[task 2019-03-08T22:53:00.091Z] 22:53:00 INFO - 411 INFO SimpleTest FINISHED
[task 2019-03-08T22:53:01.710Z] 22:53:01 INFO - Failed to get top activity, retrying, once...
[task 2019-03-08T22:54:51.620Z] 22:54:51 INFO - 412 INFO TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - TypeError: SimpleTest.harnessParameters is undefined at SimpleTest_setTimeoutShim@https://example.com/tests/SimpleTest/SimpleTest.js:669:17
[task 2019-03-08T22:54:51.620Z] 22:54:51 INFO - add_task@https://example.com/tests/SimpleTest/AddTask.js:30:7
[task 2019-03-08T22:54:51.620Z] 22:54:51 INFO - @https://example.com/tests/toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html:79:1
[task 2019-03-08T22:54:51.620Z] 22:54:51 INFO - simpletestOnerror@https://example.com/tests/SimpleTest/SimpleTest.js:1665:24
[task 2019-03-08T22:54:57.196Z] 22:54:57 INFO - 413 INFO TEST-UNEXPECTED-FAIL | | /tests/toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html - finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()
[task 2019-03-08T22:54:57.197Z] 22:54:57 INFO - {u'loaded_test_url': u'/tests/toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html'}
[task 2019-03-08T22:54:57.197Z] 22:54:57 INFO - 414 INFO TEST-UNEXPECTED-ERROR | | Finished in 13144ms
[task 2019-03-08T22:54:57.197Z] 22:54:57 INFO - {u'runtime': 13144}
[task 2019-03-08T22:54:57.197Z] 22:54:57 INFO - TEST-INFO
[task 2019-03-08T22:54:57.197Z] 22:54:57 INFO - 415 INFO SimpleTest START
[task 2019-03-08T22:54:57.197Z] 22:54:57 INFO - 416 INFO TEST-START | toolkit/components/passwordmgr/test/mochitest/test_autofill_autocomplete_types.html
[task 2019-03-08T22:55:09.541Z] 22:55:09 INFO - Failed to get top activity, retrying, once...
[task 2019-03-08T22:55:09.957Z] 22:55:09 INFO - wait for org.mozilla.fennec_aurora complete; top activity=com.android.launcher
[task 2019-03-08T22:55:10.163Z] 22:55:10 INFO - remoteautomation.py | Application ran for: 0:03:27.098337
[task 2019-03-08T22:55:10.885Z] 22:55:10 INFO - Stopping web server
[task 2019-03-08T22:55:10.889Z] 22:55:10 INFO - Stopping web socket server
[task 2019-03-08T22:55:10.910Z] 22:55:10 INFO - Stopping ssltunnel
[task 2019-03-08T22:55:10.931Z] 22:55:10 INFO - leakcheck | refcount logging is off, so leaks can't be detected!
[task 2019-03-08T22:55:10.931Z] 22:55:10 INFO - runtests.py | Running tests: end.
[task 2019-03-08T22:55:11.764Z] 22:55:11 INFO - Buffered messages logged at 22:54:58
[task 2019-03-08T22:55:11.764Z] 22:55:11 INFO - 417 INFO TEST-START | Shutdown

Comment 26

[Sam Foster [:sfoster] (he/him)]

Thanks for the backout :nataliaCs. I thought my try push was clean on Android 4.3 API16, but looking again I see the same failure there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1564d10b11abac062b123267ad0df8bea4fc48c

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/004ff60a5824
Consider autocomplete fieldname when determining what is a username field. r=MattN

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/004ff60a5824

Comment 29

[Daniel Bodea [:danibodea]]

Using the STR from comment 0, this issue was fixed; However, If we were to use some test pages with username fields that have autocomplete= "anything but the valid values", then the username fields will get auto-filled. Is this intended? Does this cover this bug's verification?

Failing test page:

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
</head>
<body>

<form method="POST" action="./">
  <h1>Username field has autocomplete=ersahusername</h1>
  <p>
    <label>User: <input type="text" autocomplete="ersahusername"></label>
  </p>
  <p>
    <label>Password: <input type="password" name="password"></label>
  </p>
  <p>
    <input type="submit" value="Login">
  </p>
</form>

</body>
</html>

Comment 30

[Sam Foster [:sfoster] (he/him)]

That is my understanding of the spec, yes. An invalid autocomplete attribute should get our default behavior - which is to autofill.

Comment 31

[Matthew N. [:MattN]]

Yes, Sam is correct that the behaviour is intended.

If you saw improvements comparing before and after the patch then that would be enough for verification. The easiest way to verify would be to load a test page that has an address field with its @autocomplete value immediately before a password field when you have a saved login containing a username and password field. The address field shouldn't be filled anymore.

Comment 32

[Daniel Bodea [:danibodea]]

Considering comment 31, the STR found in comment 0 is now invalid because the registration page has changed. Do you know of any other test page that has the needed requirements?

If I were to build a test page, when using the test page from comment 9 with the addition of the autocomplete="last-name" in the field before the password field, mentioned in comment 12, like so:
<form>
<input type='text' name='username'>
<input type='text' name='firstname'>
<input type='text' name='lastname' autocomplete="family-name">
<input type='password' name='pw' autocomplete='off'>
</form>
the field with the autocomplete="last-name" attribute will NOT be autofilled; however, the field before that one (the first-name field) will get autofilled with the username saved in the Password Manager. The old behavior is still observable on builds before the patch. I understand that this is still intended. Please confirm?

Comment 33

[Matthew N. [:MattN]]

(In reply to Bodea Daniel [:danibodea] from comment #32)

Considering comment 31, the STR found in comment 0 is now invalid because the registration page has changed. Do you know of any other test page that has the needed requirements?

I don't know any off-hand but the ones you manually create are fine.

If I were to build a test page, when using the test page from comment 9 with the addition of the autocomplete="last-name" in the field before the password field, mentioned in comment 12, like so:
<form>
<input type='text' name='username'>
<input type='text' name='firstname'>
<input type='text' name='lastname' autocomplete="family-name">
<input type='password' name='pw' autocomplete='off'>
</form>
the field with the autocomplete="last-name" attribute will NOT be autofilled; however, the field before that one (the first-name field) will get autofilled with the username saved in the Password Manager. The old behavior is still observable on builds before the patch. I understand that this is still intended. Please confirm?

Yes, that's intended. All we're doing is ignoring valid @autocomplete values which aren't the ones listed in the user story. We will keep searching earlier in the form to find an eligible username field if an ineligible one is found.

Comment 34

[Daniel Bodea [:danibodea]]

Considering all the above, I will mark this bug as VERIFIED. Thank you.