Closed Bug 1318203 Opened 8 years ago Closed 7 years ago

Don't show autocomplete UI on Password Field if it is already populated with text

Categories

(Toolkit :: Password Manager, defect)

Product:
Toolkit
Component:
Password Manager
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 + verified
firefox53 --- verified

People

(Reporter: tanvi, Assigned: timdream)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1318203 - Login manager autoCompleteSearchAsync helper to not complete canceled searches.
59 bytes, text/x-review-board-request
mossop
: review+
jcristau
: approval-mozilla-aurora+
Details
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,
59 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-aurora+
Details 
Go to an HTTPS page with a password field.  Create two fake login entries for that domain.

Focus the username field.  Press the down arrow twice (the first of your two login entries will be selected).  Hit tab.  The tab moves you to the password field.  The password field is autofilled.  But the password field autocomplete also shows up with your two saved usernames.  This is confusing, because the field is already filled and these usernames shouldn't show up.
Hi Tanvi,

I suppose the behavior you expected in comment 0 is "Hide the password drop-down when the username and password are both matched with the exist login." Correct me if it's wrong.

Another case is the password field is not matched but also not empty. IMHO, the behavior is to show all possible logins for a not-matched password.

BTW, both [1] and [2] are relative to the behavior mentioned in comment 0.

[1] http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/toolkit/components/passwordmgr/nsLoginManager.js#490
[2] http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/toolkit/components/passwordmgr/LoginManagerParent.jsm#267
Flags: needinfo?(tanvi)
Hi Tanvi, maybe we also need UX's input on this one, does that make sense to you?
Adding Ryan from UX.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> Hi Tanvi,
> 
> I suppose the behavior you expected in comment 0 is "Hide the password
> drop-down when the username and password are both matched with the exist
> login." Correct me if it's wrong.

That sounds right to me.

> 
> Another case is the password field is not matched but also not empty. IMHO,
> the behavior is to show all possible logins for a not-matched password.
> 
Can you elaborate on what you mean?  Do you mean:
If username field is populated, and password field is populated with a password that does not match the password stored for that username, then show the username list dropdown in the password field?

I wonder if this could leak data for users who have a master password.  Someone could try to continuoulsy enter passwords until the drop down disappears, revealing the password that did match.  I'm not sure if that is a real threat though.  How often does a user have to enter master password in order for passwords to fill?  Once every X minutes, or once per session?  If once per session (as I suspect), then this would leak data.

Also, maybe the user changed their password and hasn't stored the new one in Firefox yet.  In that case, the drop down on the password field could be confusing and an annoyance.

So I would propose:
If the username field is populated and the password field is populated, don't show the drop down in the password field (regardless of whether the password field matches the password for that username).

Also needinfo'ing Matt for his thoughts.
Flags: needinfo?(tanvi)
Flags: needinfo?(rfeeley)
Flags: needinfo?(MattN+bmo)
For one thing, I think this is somewhat of an edge case as I think many users will autofill from the username field and then submit with enter or with a mouse click on the submit button and thus not see this issue.

(In reply to Tanvi Vyas - behind on bugzilla [:tanvi] from comment #3)
> Adding Ryan from UX.
> 
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #1)
> > Hi Tanvi,
> > 
> > I suppose the behavior you expected in comment 0 is "Hide the password
> > drop-down when the username and password are both matched with the exist
> > login." Correct me if it's wrong.
> 
> That sounds right to me.

The reason we don't take the password field's value into account for autocomplete results is because it leaks info about the password (which you figured out below). My interpretation of what Tanvi wants is: Don't show autocomplete on the password field if you just tabbed to the password field and caused the password to be filled.

> > Another case is the password field is not matched but also not empty. IMHO,
> > the behavior is to show all possible logins for a not-matched password.
> > 
> Can you elaborate on what you mean?  Do you mean:
> If username field is populated, and password field is populated with a
> password that does not match the password stored for that username, then
> show the username list dropdown in the password field?
> 
> I wonder if this could leak data for users who have a master password. 
> Someone could try to continuoulsy enter passwords until the drop down
> disappears, revealing the password that did match.  I'm not sure if that is
> a real threat though.  How often does a user have to enter master password
> in order for passwords to fill?  Once every X minutes, or once per session? 
> If once per session (as I suspect), then this would leak data.
> 
> Also, maybe the user changed their password and hasn't stored the new one in
> Firefox yet.  In that case, the drop down on the password field could be
> confusing and an annoyance.
> 
> So I would propose:
> If the username field is populated and the password field is populated,
> don't show the drop down in the password field (regardless of whether the
> password field matches the password for that username).

Do we need to consider the username field for 52? How about only showing login suggestions on password fields if they are empty for now? We should still show the insecure warning, if appropriate.

I think having the key icon on the autocomplete suggestions (and ideally the footer) would make it more clear that the popup is listing logins though and that the visible test (usually the username) isn't what's going to get filled in the password field like most other autocomplete widgets (though datalist also supports a different value and label).

FWIW Safari doesn't seem to auto-show the popup as often (or ever?) on password fields (though perhaps that's because they have the in-field key icon).
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #4)
> Do we need to consider the username field for 52? How about only showing
> login suggestions on password fields if they are empty for now? We should
> still show the insecure warning, if appropriate.
> 

That sounds good.  For this bug, if the username field is populated with anything at all, don't show the autocomplete drop down or the insecure password warning.

Sean, Tim, jkt, Dale, Matt - can any of you take this bug?  Per Matt, it shoudl be pretty simple; just need to add a conditional to check if the password field has text in it.
Summary: Password Field drop down odd behavior → Don't show autocomplete UI on Password Field if it is populated with text
Summary: Don't show autocomplete UI on Password Field if it is populated with text → Don't show autocomplete UI on Password Field if it is already populated with text
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Actually, if I couldn't submit a patch in the next 6 hours (remaining working hours available this week), please go ahead and steal this from me.
Comment on attachment 8816386 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

There will be a lot of test cases to remove (which I don't have the time to get into today unfortunately) so please give feedback on the actual code change first.

If no one can pick this up on Friday EU/US time, we (or I) would have to do this between meetings in Hawaii...
Attachment #8816386 - Flags: feedback?(tanvi)
Attachment #8816386 - Flags: feedback?(MattN+bmo)
I think the warning should still appear but the logins should not in the case described in the summary but I think the patch is also hiding the warning (from a quick skim).
Yes, the patch removes the autocomplete "UI" as described in the summary. Revert LoginManagerContent.jsm if we want to keep the warning and just remove the logins.

Tanvi, could you clarify?
Flags: needinfo?(tanvi)
I tried this on an HTTP page and see what Matt and Tim are talking about.  We can keep the warning on the password field, and just remove the logins.
Flags: needinfo?(tanvi)
The patch is incomplete. I've just found out that the following STR does not trigger a new search and thus does not remove the items for a password field already populated with text.

1. Focus on the empty password field
2. Press down to show the autocomplete menu
3. Select one of the saved login, observe the password being filled
4. (very important) press down to show the autocomplete UI again (instead of click on the focused field again)

This is probably required a deep dive into C++...
Attached file comment-13-wip.patch (obsolete) — 
So I've located where the problem in comment 13 originated. It try to reuse the result in the down keypress case without starting the new search. By inserting a literal |false| there I can fix it. I try to compare the current mSearchString with the input value but it looks like mSearchString gets updated to the input value after the input is updated WITHOUT starting a new search.

I wonder what's the right things to put into that expression there to *not* always re-search (returns |true|) while correctly invoke a new search in our case (returns |false|).
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14)
> I try to compare the current
> mSearchString with the input value but it looks like mSearchString gets
> updated to the input value after the input is updated WITHOUT starting a new
> search.

It's being changed in

http://searchfox.org/mozilla-central/rev/dc8cf05768b83a6ef0b4039edd6efddd56ee4109/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1553

Blame shows bug 165955 in 2002 :-/
Attachment #8817052 - Attachment is obsolete: true
Patch updated to address the case where the login is selected (in comment 13 step 3) by right array key.

I however doubt this is the right approach as it would change the behavior of the username input as well (the user would previously being able to select another login even if she/he selected the wrong login, and with the patch only the previously-selected login will show up in the new search).

Tanvi, would you be able to clarify if comment 13 is a problematic behavior, or it's a non-issue?
Flags: needinfo?(tanvi)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #19)
> Patch updated to address the case where the login is selected (in comment 13
> step 3) by right array key.
> 
> I however doubt this is the right approach as it would change the behavior
> of the username input as well (the user would previously being able to
> select another login even if she/he selected the wrong login, and with the
> patch only the previously-selected login will show up in the new search).
> 
> Tanvi, would you be able to clarify if comment 13 is a problematic behavior,
> or it's a non-issue?

Let me see if I understand this correctly.  The problem in comment 13 where we show the autocomplete UI again only occurs if 1) the password field starts off empty and 2) the user clicks the down arrow again to show the autocomplete UI (because maybe they changed their mind and want to use a different password).  I think that is okay.

What I don't want is the autocomplete UI to show up randomly, after the user has selected a username and a password has been autofilled.  If the user explicitly hits the down arrow and is searching for another password, then its okay if the autocomplete UI shows up.

A patch to this bug may mean that sometimes the down arrow on a password field triggers an autocomplete UI and sometimes it doesn't.  And I'm okay with that too.
Flags: needinfo?(tanvi)
Attachment #8817059 - Attachment is obsolete: true
Attachment #8817059 - Flags: review?(adw)
Comment on attachment 8816386 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

https://reviewboard.mozilla.org/r/97130/#review98920

Thanks. A test would be good if it's easy to add a case.
Attachment #8816386 - Flags: review?(MattN+bmo) → review+
I'm going to land this so we can get Nightly testing.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a41f5558f860
Don't show autocomplete UI on Password Field if it is already populated with text, r=MattN
I am getting

133 INFO Testing username field: test-username-11
JavaScript error: chrome://browser/content/content.js, line 113: TypeError: doc is null
134 INFO Console message: [JavaScript Error: "TypeError: doc is null" {file: "chrome://browser/content/content.js" line: 113}]

On my Linux machine so I couldn't really debug my way out of this. Below is a try push to desperately proof that the issue here is probably intermittent...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ee3400cc9d68f457f944a9c32e9e2ebdd9c7773
Are you testing with a debug build?
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #32)
> Are you testing with a debug build?

Yes, and it timeout at the same place instead of the error on try.
I have also not yet understand what that assertion guards...
See Also: → 1325437
[Tracking Requested - why for this release]: Usability regression that we should make sure to not ship.
tracking-firefox52: --- → ?
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #34)
> I have also not yet understand what that assertion guards...

An idea: maybe we should cancel the setTimeout() added there...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #36)
> An idea: maybe we should cancel the setTimeout() added there...

Let's see if this works:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2be41451fbae8142e029246628b6772da4f13683
Comment on attachment 8816386 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

... and that fixes it! Matt, could you re-review this?
Attachment #8816386 - Flags: review+ → review?(MattN+bmo)
Tracking this new UX regression for 52
tracking-firefox52: ? → +
Comment on attachment 8816386 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

https://reviewboard.mozilla.org/r/97130/#review101352

It's getting very messy with the same logic duplicated in three places so I'm attaching a patch which a helper to use instead.
Attachment #8816386 - Flags: review?(MattN+bmo) → review-
Comment on attachment 8821593 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

https://reviewboard.mozilla.org/r/100830/#review101356
Attachment #8821593 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8821592 [details]
Bug 1318203 - Login manager autoCompleteSearchAsync helper to not complete canceled searches.

https://reviewboard.mozilla.org/r/100828/#review101364
Attachment #8821592 - Flags: review+
Attachment #8821592 - Flags: review?(timdream)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/8db1a1e5c46c
Login manager autoCompleteSearchAsync helper to not complete canceled searches. r=mossop
https://hg.mozilla.org/integration/autoland/rev/7581a71c1230
Don't show autocomplete UI on Password Field if it is already populated with text, r=MattN
Attachment #8816386 - Attachment is obsolete: true
Comment on attachment 8821593 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

Approval Request Comment
[Feature/Bug causing the regression]: Insecure field warning
[User impact if declined]: Users will see an autocomplete popup appear on passwords fields to suggest logins even when the password field is already filled.
[Is this code covered by automated tests?]: Somewhat but this change isn't
[Has the fix been verified in Nightly?]: Not yet. I'm going to get QA to do a pass when we flip the pref to enable the warning (bug 1217152).
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: Other commit in this bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Straightforward conditions added
[String changes made/needed]: None
Flags: needinfo?(rfeeley)
Attachment #8821593 - Flags: approval-mozilla-aurora?
Attachment #8821592 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8db1a1e5c46c
https://hg.mozilla.org/mozilla-central/rev/7581a71c1230
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1326022
People are firefox-dev were complaining about this over a week ago. Could we please get this uplifted ASAP to not annoy Aurora users more?
No longer depends on: 1326022
Flags: needinfo?(jcristau)
Comment on attachment 8821592 [details]
Bug 1318203 - Login manager autoCompleteSearchAsync helper to not complete canceled searches.

refactoring in login manager, aurora52+
Flags: needinfo?(jcristau)
Attachment #8821592 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8821593 [details]
Bug 1318203 - Don't show autocomplete UI on Password Field if it is already populated with text,

no password autocomplete for populated field, aurora52+
Attachment #8821593 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on Mac OS X 10.10, Ubuntu 16.04 and Windows 7 x32 with FF Nighlty 53.0a1(2017-01-15) and DeveloperEdition 52.0a2(2017-01-15).
status-firefox52: fixed → verified
status-firefox53: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: