Closed Bug 917325 Opened 6 years ago Closed 8 months ago

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

Categories

(Toolkit :: Password Manager, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla67
Tracking Status
firefox67 --- verified
firefox68 --- verified

People

(Reporter: craig, Assigned: sfoster)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [passwords:heuristics])

User Story

* The following `autocomplete` field names should be valid for username fields:
** "username"
** "email" (added in bug 1540154)
** "tel" (added in bug 1540154)
** "tel-national" (added in bug 1540154)
** "off"
** "on"
** ""

Use the API `usernameField.getAutocompleteInfo().fieldName` to get proper parsing. We can probably do this check in `LoginHelper.isUsernameFieldType`

Attachments

(1 file)

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.
Version: 23 Branch → 24 Branch
Do I need to create a fake account to test?
Flags: needinfo?(craig)
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".
Flags: needinfo?(craig)
Could you add clear STR to reproduce the bug, step by step, please.
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.
It's clear now. :)
Same issue in old versions too.
Component: Untriaged → Password Manager
Product: Firefox → Toolkit
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).
Assignee: nobody → MattN+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Summary: Incorrect autocomplete value (password manager) → Password manager shouldn't autocomplete into fields with inappropriate @autocomplete values
Depends on: 1061630
Blocks: 1061630
No longer depends on: 1061630
See Also: → 623910
Setting as P3 - is this something we want to fix this qtr?
Priority: -- → P3
Whiteboard: Heuristics+Recipes
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
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.
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.
(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.
Thanks for the clarification Matthew
(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.
(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.
(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.
@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?
Flags: needinfo?(vinnyjames)
Flags: needinfo?(dan_256)
This is just blocked on the dependency (bug 1109188)
Flags: needinfo?(vinnyjames)
Flags: needinfo?(dan_256)
Whiteboard: Heuristics+Recipes → [passwords:heuristics]
Matt, looks like the dep for this got fixed (the dupe of the bug in comment #18, that is). What's the current status?
Flags: needinfo?(MattN+bmo)

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

Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(MattN+bmo)
Summary: Password manager shouldn't autocomplete into fields with inappropriate @autocomplete values → Password manager shouldn't autofill into fields with inappropriate @autocomplete values
User Story: (updated)
Priority: P3 → P2
Summary: Password manager shouldn't autofill into fields with inappropriate @autocomplete values → Password manager shouldn't autofill into username fields with inappropriate @autocomplete values
Version: 24 Branch → Trunk
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Attachment #9049275 - Attachment description: Bug 917325 - Consider autocomplete fieldname when determining what is a username field → Bug 917325 - Consider autocomplete fieldname when determining what is a username field. r?MattN

Somehow I forgot "on"…

User Story: (updated)
Depends on: 1531135
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64e2378a756b
Consider autocomplete fieldname when determining what is a username field. r=MattN

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

Flags: needinfo?(sfoster)

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

Flags: needinfo?(sfoster)
Blocks: 1533965
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/004ff60a5824
Consider autocomplete fieldname when determining what is a username field. r=MattN
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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>
Flags: needinfo?(MattN+bmo)

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

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.

Flags: needinfo?(MattN+bmo)

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?

Flags: needinfo?(MattN+bmo)
No longer depends on: 1540154
Regressions: 1540154

(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.

User Story: (updated)
Flags: needinfo?(MattN+bmo)

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.