Closed Bug 1314478 Opened 8 years ago Closed 8 years ago

Contextual Insecure Password Warning should show up for all username fields, even when there are no saved login

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Version:
52 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- verified
firefox53 --- verified

People

(Reporter: tanvi, Assigned: timdream)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1314478 - Always find and mark the usernameField on insecure pages,
58 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-aurora+
Details 
The Contextual Insecure Password Warning should show up on all username fields, even those where the user has no saved logins for a site.

The way the codebase works today (along with the work in bug 1217162), the warning is only useful to users who use Firefox's Password Manager, and only on the sites where that user has a stored password.  This is because we look for MarkAsLoginManagerField when deciding whether to show the dropdown.  That is only set on username fields where the Login Manager has a saved password.  It is not set on username fields if there are no Login Manager entries.

There could be various solutions to this, and perhaps MattN can chime in on them.  One is to create something like "MarkAsUsernameField"/ "MarkAsLoginField" / "MarkAsLoginFieldForInsecureWarning".  And I believe Matt has another idea as well.

We need to track this for Firefox 52.  Without this, we will show the contextual warning on less than half of the pages that we should.

Note that when we do https://bugzilla.mozilla.org/show_bug.cgi?id=1289913, we will show the warning on all password fields, regardless of whether there is a saved password.  But, by the time the user is typing their password, they have already revealed information.  Moreover, the user may look at their keyboard while entering the password and not look up to see the warning until their whole password is entered.
(In reply to Tanvi Vyas [:tanvi] from comment #0)
> There could be various solutions to this, and perhaps MattN can chime in on
> them.  One is to create something like "MarkAsUsernameField"/
> "MarkAsLoginField" / "MarkAsLoginFieldForInsecureWarning".  And I believe
> Matt has another idea as well.

A quick decision here can help a lot :) (I believe the engineering required is not that much, once we know what to do)
Flags: needinfo?(MattN+bmo)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #1)
> (In reply to Tanvi Vyas [:tanvi] from comment #0)
> > There could be various solutions to this, and perhaps MattN can chime in on
> > them.  One is to create something like "MarkAsUsernameField"/
> > "MarkAsLoginField" / "MarkAsLoginFieldForInsecureWarning".  And I believe
> > Matt has another idea as well.

We'll call the above options A and B: 

* Option A (MarkAsUsernameField): I think the problem with this approach (other than the obvious duplication of code) is that we are making the insecure password warning such that it can only show up on login manager controlled fields so MarkAsUsernameField would have to be a subset of MarkAsLoginManagerField meaning we would lose the autocomplete results when pwmgr isn't used.

* Option B (MarkAsLoginFieldForInsecureWarning): If MarkAsLoginFieldForInsecureWarning was only used on insecure forms then that would limit the downside mentioned above to only insecure forms which I think is okay.

> A quick decision here can help a lot :) (I believe the engineering required
> is not that much, once we know what to do)

The reason we only mark username fields if there are saved logins is so that if a user doesn't save logins for a site, we at least help them by autocompleting their username. The ideal solution in terms of functionality (perhaps not UX) is bug 1166112 but that has some other dependencies and won't be ready for 52 when we want to release the warning.

I had two other ideas to help show the warning more while still helping users with form history when pwmgr isn't used:
* Option C: mark all username fields as login manager fields (with the existing method) but if there are no password manager results from mLoginManager->AutoCompleteSearchAsync, use formAutoComplete->AutoCompleteSearchAsync the results from formAutoComplete->AutoCompleteSearchAsync instead. Because of the async nature of async autocomplete population I think this may be too messy.

* Option D (like B but without a new method): Mark all insecure username fields as login manager fields with the existing markAsLoginManagerField method but continue only marking secure username fields if there are saved logins. This requires adjusting code related to https://dxr.mozilla.org/mozilla-central/rev/3bfde35a0d18a643485ffd5073f3bc6a79e0ae48/toolkit/components/passwordmgr/LoginManagerContent.jsm#1017-1027 We need to move the markAsLoginManagerField earlier in the method before some of the early returns.

I think option D is the simplest while limiting the regression of autocomplete on username fields without a saved login to only insecure ones so I recommend that we implement that.
Flags: needinfo?(MattN+bmo)
A testcase can be added to test_insecure_form_field_autocomplete.html
I spoke with Matt and agree we should go with Option D.  This means:

1) Mark all insecure username fields with MarkAsLoginManagerField.
This requires an update to this code.
https://dxr.mozilla.org/mozilla-central/rev/3bfde35a0d18a643485ffd5073f3bc6a79e0ae48/toolkit/components/passwordmgr/LoginManagerContent.jsm#1017-1027
We shouldn't return early if no logins are found.  Instead, we should check if the page is insecure.  If the page is insecure, we should mark the field with "this._formFillService.markAsLoginManagerField(usernameField);" and then return.

I'm not sure if we should check for security with hasInsecureLoginForms or isFormSecure:
https://dxr.mozilla.org/mozilla-central/rev/3bfde35a0d18a643485ffd5073f3bc6a79e0ae48/toolkit/components/passwordmgr/LoginManagerContent.jsm#454
http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/InsecurePasswordUtils.jsm#78

2) Add a test to test_insecure_form_field_autocomplete.html

3) Implications: When a user visits and insecure page with no saved logins, they previously could use the form history autocomplete to autocomplete a username.  Now, the user will have access to form history autocomplete on insecure pages with no saved logins.

We need someone to pick up this bug.
Flags: needinfo?(timdream)
Yeah, this is something I should be able to do this week as long as my schedule is not filled with meetings!
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
I got the WIP ready and I am now working on the tests.

The required fix on where comment 4 points at but is not exactly what's suggested here.
I have trouble figure out creating a valid test in test_insecure_form_field_autocomplete.html because the reproduction of this bug requires no saved login at the time the document is loaded. My newly added test reduces to the same form1 test case on that file. We'll probably need to add a new file (something I try to avoid...)
:tanvi, it would be great if you could review this before :MattN. He might be OOO this week. Thanks!
Flags: needinfo?(tanvi)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7)
> I have trouble figure out creating a valid test in
> test_insecure_form_field_autocomplete.html because the reproduction of this
> bug requires no saved login at the time the document is loaded. My newly
> added test reduces to the same form1 test case on that file. We'll probably
> need to add a new file (something I try to avoid...)

You just need to change the form action so that the login doesn't match.
I'm going to review this today
Flags: needinfo?(tanvi)
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review91312

Please double-check that the test fails without your code changes.

r=me with the code movement below:

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:975
(Diff revision 3)
> +      if (foundLogins.length == 0 && !usernameField) {
> +        // We don't log() here since this is a very common case.
> +        recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
> +        return;
> +      }

Hey Tim, Why did you move this so high? It's going to change our telemetry data for no clear reason as it seems like it should only be moved right above "// Prevent autofilling insecure forms."
Attachment #8808087 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

Patch changed so we don't trigger different telemetry flags.
Attachment #8808087 - Flags: review+ → review?(MattN+bmo)
(In reply to Tanvi Vyas [:tanvi] from comment #4)
> I'm not sure if we should check for security with hasInsecureLoginForms or
> isFormSecure:
> https://dxr.mozilla.org/mozilla-central/rev/
> 3bfde35a0d18a643485ffd5073f3bc6a79e0ae48/toolkit/components/passwordmgr/
> LoginManagerContent.jsm#454
> http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/
> InsecurePasswordUtils.jsm#78

By the way, |hasInsecureLoginForms| checks the top window and |isFormSecure| checks the actual form.

We already have the formLike in _fillForm and it's possible to have an insecure from within a secure page, so the correct method to use is |isFormSecure|.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #17)
> Comment on attachment 8808087 [details]
> Bug 1314478 - Always find and mark the usernameField on insecure pages,
> 
> Patch changed so we don't trigger different telemetry flags.

Interdiff showed the only change is moving the lines of code for telemetry:
https://reviewboard.mozilla.org/r/91018/diff/3-4/

Matt, is this moved to the right place, or does it need to be two if blocks later, after:
if (passwordField.disabled || passwordField.readOnly) {



Tim, can you push to try again?
(In reply to Tanvi Vyas [:tanvi] from comment #19)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #17)
> Interdiff showed the only change is moving the lines of code for telemetry:
> https://reviewboard.mozilla.org/r/91018/diff/3-4/

Please don't look at the interdiff but the actual patch. It has reduced to a very-easy-to-understand place.

> Tim, can you push to try again?

I did, through MozReview, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b1746bf247b

Though the bc7 errors should probably be investigated ...
The patch might be in conflict with bug 1289913. Even if it's not, I should manually test the patch on top of that bug, after it lands.
Depends on: 1289913
(In reply to Tanvi Vyas [:tanvi] from comment #19)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #17)
> > Comment on attachment 8808087 [details]
> > Bug 1314478 - Always find and mark the usernameField on insecure pages,
> > 
> > Patch changed so we don't trigger different telemetry flags.
>
> Matt, is this moved to the right place, or does it need to be two if blocks
> later, after:
> if (passwordField.disabled || passwordField.readOnly) {

You're right. I believe it should be two if blocks later like I said before.
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92208
Attachment #8808087 - Flags: review?(MattN+bmo)
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92220

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:978
(Diff revision 5)
> +      // one. This is normally used to select from multiple accounts,
> +      // but even with one account we should refill if the user edits.
> +      // We would also need this attached to show the insecure login
> +      // warning, regardless of saved login.
> +      if (usernameField) {
> +        this._formFillService.markAsLoginManagerField(usernameField);

This should move after the passwordField and readOnly/disabled checks too unless you're sure that it won't cause any undesired effects once we handle the first focus. Keeping it closer to where it was is less risky so that's what I would prefer.
Attachment #8808087 - Flags: review?(MattN+bmo)
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92320

Thanks
Attachment #8808087 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92322

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:943
(Diff revision 6)
>  
>      try {
> -      // Nothing to do if we have no matching logins available.
> -      if (foundLogins.length == 0) {
> +      // Nothing to do if we have no matching logins available,
> +      // and there isn't a need to show the insecure form warning.
> +      if (foundLogins.length == 0 &&
> +          InsecurePasswordUtils.isFormSecure(form)) {

Really the pref should be taken into account here so we don't regress behaviour when the insecure warning is disabled:

`foundLogins.length == 0 && (InsecurePasswordUtils.isFormSecure(form) || !warningEnabled)
(In reply to Matthew N. [:MattN] (away Nov. 3–7) from comment #30)
> Really the pref should be taken into account here so we don't regress
> behaviour when the insecure warning is disabled:
> 
> `foundLogins.length == 0 && (InsecurePasswordUtils.isFormSecure(form) ||
> !warningEnabled)

I added that, but this requires the pref to be flipped *before* the mochitest test page loads. Do you know if we have mechanics to do that? If we don't have something readily available, I would try to test this with within an iframe.
Flags: needinfo?(MattN+bmo)
Never mind, just found SpecialPowers.setBoolPref()
Flags: needinfo?(MattN+bmo)
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92542

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:940
(Diff revisions 6 - 8)
> +      let prefShowInsecureFieldWarning =
> +        Preferences.get(PREF_INSECURE_FIELD_WARNING_ENABLED, false);

See the first part of bug 1289913 comment 47 and the patch there about how to make this pref live.

::: toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_no_saved_login.html:37
(Diff revisions 6 - 8)
> +// Set to pref before the document loads.
> +SpecialPowers.setBoolPref(
> +  "security.insecure_field_warning.contextual.enabled", true);

If you make the pref live using LoginHelper this may not need to change anymore. Does SpecialPowers.setBoolPref automatically clean up the pref at the end of the test file like pushPrefEnv does? If not, that needs to be handled.
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92544

::: toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_no_saved_login.html
(Diff revisions 6 - 8)
>    return Promise.resolve();
>  }
>  
> -add_task(function* setup() {
> -  yield SpecialPowers.pushPrefEnv({"set": [["security.insecure_field_warning.contextual.enabled", true]]});
> -  listenForUnexpectedPopupShown();

Also, is it intentional that you removed `listenForUnexpectedPopupShown()`?
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92542

> See the first part of bug 1289913 comment 47 and the patch there about how to make this pref live.

Do you really want to touch this in this bug? I will ended up touching other part of LoginManagerContent.jsm that is unrelated to this bug.

> If you make the pref live using LoginHelper this may not need to change anymore. Does SpecialPowers.setBoolPref automatically clean up the pref at the end of the test file like pushPrefEnv does? If not, that needs to be handled.

> If you make the pref live using LoginHelper this may not need to change anymore.

How? LoginHelper would still rely on actually flipping the pref right?

> Does SpecialPowers.setBoolPref automatically clean up the pref at the end of the test file like pushPrefEnv does? If not, that needs to be handled.

Fixed.
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92542

> Do you really want to touch this in this bug? I will ended up touching other part of LoginManagerContent.jsm that is unrelated to this bug.

Yeah, after rebasing you got this for free.

> > If you make the pref live using LoginHelper this may not need to change anymore.
> 
> How? LoginHelper would still rely on actually flipping the pref right?
> 
> > Does SpecialPowers.setBoolPref automatically clean up the pref at the end of the test file like pushPrefEnv does? If not, that needs to be handled.
> 
> Fixed.

Yeah, my point is that you can go back to the normal way of doing this with 
`yield SpecialPowers.pushPrefEnv({"set": [["security.insecure_field_warning.contextual.enabled", true]]});` in the "setup" task so you get cleanup for free (like you had in revision 7).
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

https://reviewboard.mozilla.org/r/91018/#review92542

> Yeah, my point is that you can go back to the normal way of doing this with 
> `yield SpecialPowers.pushPrefEnv({"set": [["security.insecure_field_warning.contextual.enabled", true]]});` in the "setup" task so you get cleanup for free (like you had in revision 7).

Ah, I forgot this check is in `_fillForm` so you would still need to set the pref before the field is found (like you are) unless you change the test to create the password field dynamically.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be95aa5249cf
Always find and mark the usernameField on insecure pages, r=MattN
https://hg.mozilla.org/mozilla-central/rev/be95aa5249cf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Tim, please request uplift to 52.
Flags: needinfo?(timdream)
Sorry, wrong bug: 

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1289913
[User impact if declined]: Incorrect behavior of bug 1289913 when enabled on Fx52. User will not be able to select previously saved login on HTTP page without this fix.
[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?]: This changes make sure we still allow user to select a saved login on HTTP pages, when the feature is enabled.
[String changes made/needed]: No
Sorry, I don't understand what "See dependency in bug 1289913" means in terms of other needed uplifts.
Flags: needinfo?(timdream)
Blocks: 1289913
No longer depends on: 1289913
(In reply to Julien Cristau [:jcristau] from comment #51)
> Sorry, I don't understand what "See dependency in bug 1289913" means in
> terms of other needed uplifts.

Sorry, the answer to that question should be "none" since other patches should have their own uplift requests requested.
Flags: needinfo?(timdream)
Comment on attachment 8808087 [details]
Bug 1314478 - Always find and mark the usernameField on insecure pages,

show insecure password warning on more forms, for aurora52
Attachment #8808087 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.

Attachment

General

Creator:
Created:
Updated:
Size: