1334026 - Insecure password field warning doesn't appear on password fields if password manager is disabled

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect, P1)

Description

[Matthew N. [:MattN]]

Attachment: Test case

I totally did the wrong fix for bug 1329351, the popup was supposed to show on INSECURE password fields even if password manager is off, instead bug 1329351 made it so that it never appears. This is bad because I don't want users to turn off password manager (because sites suggest it) in order to stop the insecure warning. It also means some users wouldn't be protected by this feature we're promoting heavily.

STR:
1) Set signon.rememberSignons = false (Maps to checkbox on the Security tab for pwmgr)
2) Open the testcase
3) Submit some form history for a given <input> @name.
4) Open the autocomplete popup (e.g. down arrow or focus) for a password field with the same @name.

Expected result:
Insecure password field warning appears in the dropdown with no other result.

Actual result:
No popup appears

[Tracking Requested - why for this release]: No insecure field warning will appear if password manager is disabled

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

Review commit: https://reviewboard.mozilla.org/r/107370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107370/

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

https://reviewboard.mozilla.org/r/107370/#review108490

::: toolkit/components/satchel/nsFormFillController.cpp
(Diff revision 1)
> -  if (formControl->GetType() == NS_FORM_INPUT_PASSWORD && !isPwmgrInput) {
> -    return;
> -  }

Note that this is just a backout of bug 1329351

::: toolkit/components/satchel/nsFormFillController.cpp:1006
(Diff revision 1)
> +      (mPwmgrInputs.Get(mFocusedInputNode) ||
> +       formControl->GetType() == NS_FORM_INPUT_PASSWORD)) {

This makes us delegate to password manager for all password fields regardless of whether they're marked. Password manager already handles checking the pwmgr pref before searching the login DB.

Note that this fix won't cause us to show the warning on insecure username fields if password manager is disabled as that requires more password manager logic to run but I think that's probably fine as least for 52 since the password field is the more sensitive one.

Tanvi, is that fine?

Comment 3

[Matthew N. [:MattN]]

(Quoting Matthew N. [:MattN] from comment #2)
> Note that this fix won't cause us to show the warning on insecure username
> fields if password manager is disabled as that requires more password
> manager logic to run but I think that's probably fine as least for 52 since
> the password field is the more sensitive one.
> 
> Tanvi, is that fine?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

https://reviewboard.mozilla.org/r/107370/#review108622

Good catch MattN - I didn't even consider this scenario during my review of bug 1329351. :/

::: toolkit/components/satchel/nsFormFillController.cpp:706
(Diff revision 1)
> +    if (!mLoginManager)
> +      mLoginManager = do_GetService("@mozilla.org/login-manager;1");

I haven't done a comprehensive search through the rest of the file, but at least `nsFormFillController::RevalidateDataList` has us putting braces around one-liners. I know this bracing thing is always contentious, but it'd be great if we could be consistent within this file.

::: toolkit/components/satchel/nsFormFillController.cpp:1005
(Diff revision 1)
> -  if (!mContextMenuFiredBeforeFocus && mPwmgrInputs.Get(mFocusedInputNode)) {
> +  if (!mContextMenuFiredBeforeFocus && formControl &&
> +      (mPwmgrInputs.Get(mFocusedInputNode) ||

I guess there's no way for `formControl` to be `nullptr` and still have `mFocusedInputNode` inside `mPwmgrInputs`? If there is, we've changed the logic here, since we check for `formControl` first.

If it's possible, this should probably be:

```
if (!mContextMenuFiredBeforeFocus &&
    (mPwmgrInputs.Get(mFocusedInputNode) ||
     (formControl && formControl->GetType() == NS_FORM_INPUT_PASSWORD))) {
```

Or, even clearer:

```
nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
bool isPasswordInput = mPwmgrInputs.Get(mFocusedInputNode) ||
                       formControl && formControl->GetType() == NS_FORM_INPUT_PASSWORD;
                    
// If this focus doesn't immediately follow a contextmenu event then show
// the autocomplete popup

if (!mContextMenuFiredBeforeFocus && isOnPassword) {
  ShowPopup();
}
```

Of course, if it's non-sensical for formControl to be `nullptr` in this scenario, your way is fine (though maybe it makes sense in that case to add a MOZ_ASSERT on formControl after we QI it).

::: toolkit/components/satchel/test/mochitest.ini:18
(Diff revision 1)
>  [test_form_autocomplete_with_list.html]
>  [test_form_submission.html]
>  [test_form_submission_cap.html]
>  [test_form_submission_cap2.html]
>  [test_password_autocomplete.html]
> +scheme = https

TIL about scheme. That's handy.

::: toolkit/components/satchel/test/test_password_autocomplete.html:99
(Diff revision 1)
>      { op : "add", fieldname : "field1", value : "value8" },
>      { op : "add", fieldname : "field1", value : "value9" },
>    ], resolve));
>  });
>  
> -add_task(function* test_no_form_history() {
> +add_task(function* test_secure_noFormHistoryOrWarning() {

Super small nit: Why this mix of underscore and camelcase?

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

https://reviewboard.mozilla.org/r/107370/#review108622

> I haven't done a comprehensive search through the rest of the file, but at least `nsFormFillController::RevalidateDataList` has us putting braces around one-liners. I know this bracing thing is always contentious, but it'd be great if we could be consistent within this file.

I copied these lines as-is from another part of the file so didn't think about the braces. I prefer braces and they're the recommended style so will add them.

> I guess there's no way for `formControl` to be `nullptr` and still have `mFocusedInputNode` inside `mPwmgrInputs`? If there is, we've changed the logic here, since we check for `formControl` first.
> 
> If it's possible, this should probably be:
> 
> ```
> if (!mContextMenuFiredBeforeFocus &&
>     (mPwmgrInputs.Get(mFocusedInputNode) ||
>      (formControl && formControl->GetType() == NS_FORM_INPUT_PASSWORD))) {
> ```
> 
> Or, even clearer:
> 
> ```
> nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
> bool isPasswordInput = mPwmgrInputs.Get(mFocusedInputNode) ||
>                        formControl && formControl->GetType() == NS_FORM_INPUT_PASSWORD;
>                     
> // If this focus doesn't immediately follow a contextmenu event then show
> // the autocomplete popup
> 
> if (!mContextMenuFiredBeforeFocus && isOnPassword) {
>   ShowPopup();
> }
> ```
> 
> Of course, if it's non-sensical for formControl to be `nullptr` in this scenario, your way is fine (though maybe it makes sense in that case to add a MOZ_ASSERT on formControl after we QI it).

> I guess there's no way for formControl to be nullptr and still have mFocusedInputNode inside mPwmgrInputs? 

Correct, the case I'm handling is if we don't control the input because Maybe… decides not to. I added an early return `if (!mFocusedInputNode)` and a MOZ_ASSERT after the QI now.

> Super small nit: Why this mix of underscore and camelcase?

I was just trying to empasize the secure vs. insecure aspect of the two tests. Maybe I'm the only one who does this in test task functions? i.e. it's a form of grouping.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107370/diff/1-2/

Comment 7

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #2)
> Note that this fix won't cause us to show the warning on insecure username
> fields if password manager is disabled as that requires more password
> manager logic to run but I think that's probably fine as least for 52 since
> the password field is the more sensitive one.
> 
> Tanvi, is that fine?

I'm fine with this for now.  We can file a followup for the username field when the password manager is disabled.  And then talk to pdol about whether we need to fix that followup or just defer it (P5 it).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ddaf704683d
Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d7658b29d31
Backed out changeset 1ddaf704683d for browser_aboutHome.js crashes on a CLOSED TREE.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107370/diff/2-3/

Comment 11

[Matthew N. [:MattN]]

I got fooled by our hash table implementation gracefully accepting a null key which caused me to think that it wasn't possible for that key variable to be null.

Try push looks much better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1da3f81eb36d7495e093add57ddf7f018e61cb

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25c931f199e8
Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley

Comment 13

[Phil Ringnalda (:philor)]

Went a bit better that time, only crashing Android reftests like https://treeherder.mozilla.org/logviewer.html#?job_id=72460740&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/dad46f412588

Comment 14

[Matthew N. [:MattN]]

Fun… now the problem is related to master password on Android from the pwmgr do_GetService:

https://treeherder.mozilla.org/logviewer.html#?job_id=72460740&repo=autoland&lineNumber=7337-7330

0x805a1f9b maps to SEC_ERROR_INVALID_PASSWORD

Comment 15

[Matthew N. [:MattN]]

This may be be another reason to fix bug 1334012 but the nsLoginManager autoCompleteSearchAsync method would need access to storage anyways so maybe that would just move the problem?

Comment 16

[Julien Cristau [:jcristau]]

Tracking for 52 where the insecure password field warning is introduced.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107370/diff/3-4/

Comment 18

[Matthew N. [:MattN]]

Android Try is now green with the ifdef, just going to double-check Linux is still green too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=165b99f46db379a17ac12fbde9cfe868f918da38

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f89807ca30c
Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f89807ca30c

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1289913 / bug 1329351
[User impact if declined]: Users won't be warned about insecure password fields if they disabled password manager.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: No, not specifically
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Riskier than related JS-only changes
[Why is the change risky/not risky?]: I did hit some crashes on landings, some specifically due to different behaviour on Android. We have a work item to get QE to verify the Android is working properly for the feature in general.
[String changes made/needed]: None

The test depends on bug 1286312 which is only in 53 so Beta will need a separate patch.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8830623 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

OK, sounds risky but if you need to uplift to aurora to diagnose and fix possible fallout, let's bring this to aurora now. 
Can you follow up after it lands in aurora (tomorrow)? 
Do you need QA  help?

Comment 23

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d5fd88afc55

Comment 24

[Matthew N. [:MattN]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> Comment on attachment 8830623 [details]
> Bug 1334026 - Show the the insecure field warning on insecure password
> fields even if they're not marked.
> 
> OK, sounds risky but if you need to uplift to aurora to diagnose and fix
> possible fallout, let's bring this to aurora now. 
> Can you follow up after it lands in aurora (tomorrow)? 

Sorry, what do you want me to follow-up on? Getting a Beta patch ready or something else?

> Do you need QA  help?

Since Adrian is on PTO I think Ovidiu can help.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Just making sure this works as intended on aurora. Thanks Matt and Ovidiu!

Comment 26

[ovidiu boca[:Ovidiu]]

Hi,

I tested on Mac 10.10, Windows 10 x64 and Ubuntu 16.04 following the steps from description. I can confirm the fix on Nighlty 54.0a1(2017-02-01) and Aurora 53.0a2(2017-02-01).

Comment 27

[Matthew N. [:MattN]]

Attachment: Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

Review commit: https://reviewboard.mozilla.org/r/109484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109484/

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8833248 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

This is the beta patch with only a test change.

Approval Request Comment:
See comment 21

Comment 29

[Matthew N. [:MattN]]

Beta try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20a3769c54e878363fd9c53f1277563785775bbc

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8833248 [details]
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.

insecure password field warning fix, tested in nightly and aurora

This should be in 52.0b4.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cb874e871110

Comment 32

[Matthew N. [:MattN]]

I seems like this bug caused the crash on Aurora in bug 1336391. Julien, can you make sure that no beta goes out with this fix? I see that FIREFOX_52_0b3_BUILD1 was already tagged on a revision without this crasher. The plan is to have the fix in bug 1330111 in Beta ASAP (beta 4 or 5).

Comment 33

[Julien Cristau [:jcristau]]

Backed out so the crash doesn't make it to beta4: https://hg.mozilla.org/releases/mozilla-beta/rev/06eda6a9a51f

Comment 34

[Matthew N. [:MattN]]

The beta series to land including bug 1330111:

(Quoting Matthew N. [:MattN] from bug 1330111 comment #100)
> Rebased beta try push v2:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=34544281b93e19af235589158e9689bb5c04413b
> 
> Please land the following commits from the try push for "Bug 1334026" and
> "Bug 1330111" once approved if Try is green:
> https://hg.mozilla.org/try/shortlog?rev=7ebc860b2307%3A%3A43884f68cd61

Try is green.

Comment 35

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/858a114711db

Comment 36

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52b6, Win 7.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/858a114711db