Can't autocomplete usernames on insecure pages.

VERIFIED FIXED in Firefox 52

Status

()

Toolkit
Password Manager
P1
normal
VERIFIED FIXED
8 months ago
6 months ago

People

(Reporter: tanvi, Assigned: timdream)

Tracking

(Blocks: 1 bug)

52 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 wontfix, firefox52 verified, firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
sadness
Go to about:config and set:
security.insecure_field_warning.contextual.enabled to true
signon.autofillForms.http to true

Go to an http:// page where you have one or more saved logins. (For example, you could go to http://mysqueezebox.com/user/login and enter a fake username/password.  Hit submit, and select for the password manager to save the credential).

Go to the login page again.  Focus on the username or password field.  You see the insecure password warning with a Learn More link.  But you don't not see your username underneath the insecure warning.  You cannot select your username and fill it.

Expected behavior: You should see a username underneath the insecure warning in both the username and password fields.

This happens when you have one or more saved logins.

I am using Nightly 53.0a1 from 2016-11-16 on OSX.
(In reply to Tanvi Vyas - behind on bugzilla [:tanvi] from comment #0)
> Go to about:config and set:
> security.insecure_field_warning.contextual.enabled to true
> signon.autofillForms.http to true

I can only reproduce this bug when I flip this pref to "false" (it's default to true currently). Is this a typo?
I've found the root cause and the patch incoming.
Assignee: nobody → timdream
Blocks: 1289913
Status: NEW → ASSIGNED
Hi Tanvi, I can not reproduce this bug based on the description. Please see the video link [1]. Do you think I miss any steps?

[1] https://youtu.be/dNxGgBESZks
Comment hidden (mozreview-request)
Talked to Sean offline. To be clear, my patch was based on comment 1, and the rationale that the user has to have access to saved login from the dropdown, regardless of the pref. Please disregard my patch if either is the case.
Comment hidden (mozreview-request)
Attachment #8811562 - Flags: feedback?(MattN+bmo)
Attachment #8811562 - Flags: feedback?(tanvi)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> I can only reproduce this bug when I flip this pref to "false" (it's default
> to true currently). Is this a typo?

Tanvi said yes over the e-mail.
Comment hidden (mozreview-request)
MattN and I are discussed this before at bug 1289913 comment 47.

> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1256
> (Diff revisions 3 - 7)
> > +  if (!prefShowInsecureAutoFillForms) {
> > +    matchingLogins = [];
> > +  }
> 
> Don't you only want to do this if `!prefShowInsecureAutoFillForms &&
> !isSecure`?
> 
> With what you have I think you're preventing autocomplete suggestions for
> both secure and insecure.
>
Hi Tanvi,

Hi Tanvi, to me this one is more like a UI decision instead of a bug. In the RST we already set the |signon.autofillForms.http| to |false|, which means we shouldn't display form input history under http connection. So to me it makes sense to not show the username underneath the insecure warning when the |signon.autofillForms.http| is set to |false|

Do you know who is the UX for this feature, maybe we should ni UX to have their comments as well.
Flags: needinfo?(tanvi)
Comment on attachment 8811562 [details]
Bug 1318194 - Always lists matching logins regardless of page security,

https://reviewboard.mozilla.org/r/93636/#review93930

I thought that Sean's patch before was using an empty array to replace the array with only the warning BEFORE adding the login. I didn't notice that it wasn't providing matching logins in this case. The pref is only about "autofill", not autocomplete so the plan was always for autocomplete to continue to work and that was part of the original UX discussion.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1297
(Diff revision 3)
>          seen.add(login.username);
>        }
>        return duplicates;
>      }
>  
> -    let currentMatchingLogins = (!LoginHelper.insecureAutofill && !this._isSecure) ?
> +    let currentMatchingLogins = this._matchingLogins;

Please get rid of this now unnecesarry redirection (inline `this._matchingLogins`)
Attachment #8811562 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 12

8 months ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> (In reply to Tanvi Vyas - behind on bugzilla [:tanvi] from comment #0)
> > Go to about:config and set:
> > security.insecure_field_warning.contextual.enabled to true
> > signon.autofillForms.http to true
> 
> I can only reproduce this bug when I flip this pref to "false" (it's default
> to true currently). Is this a typo?

signon.autoFillForms.http to false.  Sorry for the typo.
Flags: needinfo?(tanvi)
(Reporter)

Comment 13

8 months ago
(In reply to Vance Chen [:vchen][skype:vance.lucida][vchen@mozilla.com] from comment #10)
> Hi Tanvi,
> 
> Hi Tanvi, to me this one is more like a UI decision instead of a bug. In the
> RST we already set the |signon.autofillForms.http| to |false|, which means
> we shouldn't display form input history under http connection. So to me it
> makes sense to not show the username underneath the insecure warning when
> the |signon.autofillForms.http| is set to |false|
> 
> Do you know who is the UX for this feature, maybe we should ni UX to have
> their comments as well.

The UX design by Aislinn and Phillip is here - https://bug1217150.bmoattachments.org/attachment.cgi?id=8791926

We don't want to autofill the password.  But we want to give the user the option to autocomplete it (i.e. select it and fill it with user interaction) once they have seen the warning.  Hence, the usernames (whether there is just one saved, or more than one) should appear under the warning and a user should be able to select them.
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 15

8 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2e6a3c250e1
Always lists matching logins regardless of page security, r=MattN
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 17

8 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/e4cbefe6aefe
Always lists matching logins regardless of page security. Fix tests. r=bustage-fix

Comment 18

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2e6a3c250e1
https://hg.mozilla.org/mozilla-central/rev/e4cbefe6aefe
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 19

8 months ago
https://hg.mozilla.org/mozilla-central/rev/b2e6a3c250e1
https://hg.mozilla.org/mozilla-central/rev/e4cbefe6aefe
status-firefox51: --- → wontfix
status-firefox52: --- → affected
(Reporter)

Comment 20

8 months ago
Tim, can you please request uplift to 52?
Flags: needinfo?(timdream)
Comment on attachment 8811562 [details]
Bug 1318194 - Always lists matching logins regardless of page security,


Approval Request Comment
[Feature/Bug causing the regression]: Bug 1289913
[User impact if declined]: Incorrect behavior of bug 1289913 when enabled on Fx52
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: See dependency in bug 1289913
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a local local change and only affect the new feature in question.
[String changes made/needed]:
Flags: needinfo?(timdream)
Attachment #8811562 - Flags: approval-mozilla-aurora?
Attachment #8812156 - Flags: approval-mozilla-aurora?
Comment on attachment 8811562 [details]
Bug 1318194 - Always lists matching logins regardless of page security,

fix for login form autocomplete, take in aurora52
Attachment #8811562 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8812156 [details]
Bug 1318194 - Follow up, fix unit test

followup tests fix, for aurora52
Attachment #8812156 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 24

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c79a62854cb
https://hg.mozilla.org/releases/mozilla-aurora/rev/c642a065f38e
status-firefox52: affected → fixed
Verified as fixed on 53.0a1/20170116030326 and 52.0a2/20170112004017.
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.