Closed Bug 1334026 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, defect, P1)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Iteration:
54.2 - Feb 20
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 + verified
firefox-esr52 --- fixed
firefox53 --- verified
firefox54 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [FxPrivacy])

Attachments

(3 files)

Test case
645 bytes, text/html
Details
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.
59 bytes, text/x-review-board-request
mconley
: review+
lizzard
: approval-mozilla-aurora+
Details
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked.
59 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-beta+
Details
Attached file 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 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?
(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?
Flags: needinfo?(tanvi)
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?
Attachment #8830623 - Flags: review?(mconley) → review+
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.
(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).
Flags: needinfo?(tanvi)
Pushed by mozilla@noorenberghe.ca:
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
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9d7658b29d31
Backed out changeset 1ddaf704683d for browser_aboutHome.js crashes on a CLOSED TREE.
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
Pushed by mozilla@noorenberghe.ca:
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
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
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?
Tracking for 52 where the insecure password field warning is introduced.
tracking-firefox52: ? → +
Iteration: --- → 54.1 - Feb 6
Flags: qe-verify?
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
Flags: qe-verify? → qe-verify+
Pushed by mozilla@noorenberghe.ca:
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
https://hg.mozilla.org/mozilla-central/rev/2f89807ca30c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Attachment #8830623 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(MattN+bmo)
Attachment #8830623 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(MattN+bmo) → needinfo?(ovidiu.boca)
Just making sure this works as intended on aurora. Thanks Matt and 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).
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: needinfo?(ovidiu.boca)
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
Attachment #8833248 - Flags: review?(mconley)
Attachment #8833248 - Flags: review+
Attachment #8833248 - Flags: approval-mozilla-beta?
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.
Attachment #8833248 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1336391
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).
Flags: needinfo?(jcristau)
Backed out so the crash doesn't make it to beta4: https://hg.mozilla.org/releases/mozilla-beta/rev/06eda6a9a51f
status-firefox52: fixed → affected
Flags: needinfo?(jcristau)
Whiteboard: [beta uplift blocked by bug 1330111]
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Whiteboard: [beta uplift blocked by bug 1330111] → [FxPrivacy] [beta uplift blocked by bug 1330111]
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.
https://hg.mozilla.org/releases/mozilla-beta/rev/858a114711db
status-firefox52: affected → fixed
Whiteboard: [FxPrivacy] [beta uplift blocked by bug 1330111] → [FxPrivacy]
Verified fixed FX 52b6, Win 7.
status-firefox52: fixed → verified
Flags: qe-verify+
Depends on: 1348791
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: