Form history shows on password fields if password manager is disabled

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Form Manager
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Blocks: 1 bug, {regression})

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

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8824586 [details]
Test case

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

Expected result:
No autocomplete popup

Actual result:
Form history appears

I believe this is a regression from bug 1289913.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Tracking as new regression in 52.
tracking-firefox52: ? → +

Comment 4

5 months ago
mozreview-review
Comment on attachment 8824607 [details]
Bug 1329351 - Only autocomplete on password fields which were marked.

https://reviewboard.mozilla.org/r/103028/#review104292

This looks okay to me - one readability suggestion below.

Do we have a plan for removing entries from the FormHistory database for users that have already been hit by this?

::: toolkit/components/satchel/nsFormFillController.cpp:977
(Diff revision 2)
>    if (mPwmgrInputs.Get(inputNode))
>        isPwmgrInput = true;
>  
> +  // Don't show autocomplete on password fields regardless of datalist or
> +  // autocomplete being enabled as we don't want to show form history on them.
> +  if (!formControl->IsSingleLineTextControl(true) && !isPwmgrInput) {

Took a little to grok what `IsSingleLineTextControl(true)` meant (will return `false` if not a single line text control OR if is a single line text control and is a password input), and since we already checked `formControl->IsSingleLineTextControl(false)`, this is just asking if we're on a password input or not.

Instead of making the reader do the research / bool flips in their head, I wonder if this would be easier to read:

```JS
if (formControl->GetType() == NS_FORM_INPUT_PASSWORD && !isPwmgrInput) {
  // ...
}
```
Attachment #8824607 - Flags: review?(mconley) → review+
ni'ing MattN for question in comment 4 about the plan.
Flags: needinfo?(MattN+bmo)
(In reply to Mike Conley (:mconley) from comment #4)
> Do we have a plan for removing entries from the FormHistory database for
> users that have already been hit by this?

No, because that isn't a new issue. The accidental autocomplete on the password field just exposed the issue to the users. I confirmed we don't save type=password values to form history so I suspect the problem is sites that toggle to type=text to show the password in plaintext. What we need is to use an API requested in bug 1330228 at [1].

It would also be very hard to figure out what to cleanup and form history naturally expires.

[1] https://dxr.mozilla.org/mozilla-central/rev/e68cbc3b5b3d3fba4fe3e17e234713020f44e4a0/toolkit/components/satchel/formSubmitListener.js#129-131
Flags: needinfo?(MattN+bmo)
(In reply to Mike Conley (:mconley) - Backed up on review / needinfo's from comment #4)
> > +  // Don't show autocomplete on password fields regardless of datalist or
> > +  // autocomplete being enabled as we don't want to show form history on them.
> > +  if (!formControl->IsSingleLineTextControl(true) && !isPwmgrInput) {
> 
> Took a little to grok what `IsSingleLineTextControl(true)` meant (will
> return `false` if not a single line text control OR if is a single line text
> control and is a password input), and since we already checked
> `formControl->IsSingleLineTextControl(false)`, this is just asking if we're
> on a password input or not.
> 
> Instead of making the reader do the research / bool flips in their head, I
> wonder if this would be easier to read:
> 
> ```JS
> if (formControl->GetType() == NS_FORM_INPUT_PASSWORD && !isPwmgrInput) {
>   // ...
> }
> ```

I went back and forth on this myself while writing the patch but was balancing readability with consistency. My thinking was that I would rather it be less readable but correct/consistent than readable and inconsistent/incorrect if we expand/change the logic of the argument to `IsSingleLineTextControl`. I was hoping the comment would be add clarity but I guess not. I think I will switch to the more readable version checking the type but also mention the function in the comment so it gets noticed if someone is auditing changes to IsSingleLineTextControl.
I'm going to land this without the test for now (it depends on fixing bug 1317284) since I want to fix the information leak and may not have time to get bug 1317284 landed today.
Keywords: leave-open
Whiteboard: [test needs landing]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

5 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/cca78c809a20
Only autocomplete on password fields which were marked. r=mconley
I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=68589776&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/b3b5aae6e351
Flags: needinfo?(MattN+bmo)

Updated

4 months ago
Blocks: 1326022
Current suspicion is that the test failure may have just been the intermittent being fixed in bug 1317284.
Comment hidden (mozreview-request)
Attachment #8826319 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 15

4 months ago
bugherderlanding
https://hg.mozilla.org/integration/mozilla-inbound/rev/2365700d7988
Flags: in-testsuite+
Whiteboard: [test needs landing]
Duplicate of this bug: 1326022
https://hg.mozilla.org/mozilla-central/rev/2365700d7988
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: leave-open
Resolution: --- → FIXED
status-firefox53: affected → fixed
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you're comfortable doing so.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8824607 [details]
Bug 1329351 - Only autocomplete on password fields which were marked.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1289913
[User impact if declined]: Users will form history autocomplete on password fields (sometimes revealing saved pins/passwords)
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No, QE is already testing this feature as a whole
[List of other uplifts needed for the feature/fix]: This depends on bug 1317284's test fix
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple early return for an additional case which is close to what we did in 52.
[String changes made/needed]: No
Flags: needinfo?(MattN+bmo)
Attachment #8824607 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1318787
status-firefox54: --- → fixed
Comment on attachment 8824607 [details]
Bug 1329351 - Only autocomplete on password fields which were marked.

This now needs beta approval due to the merge.
Attachment #8824607 - Flags: approval-mozilla-beta?
Attachment #8824607 - Flags: approval-mozilla-aurora?
Comment on attachment 8824607 [details]
Bug 1329351 - Only autocomplete on password fields which were marked.

password autocomplete fix, beta52+, should be in b1.
Attachment #8824607 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
needs rebasing for beta
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 24

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4663bc61d861
status-firefox52: affected → fixed
Backed out from Beta for test_password_autocomplete.html failures on at least OSX/Win7/Win8.
https://treeherder.mozilla.org/logviewer.html#?job_id=71734395&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/8a1cdad0f52a
status-firefox52: fixed → affected
(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> Backed out from Beta for test_password_autocomplete.html failures on at
> least OSX/Win7/Win8.
> https://treeherder.mozilla.org/logviewer.html#?job_id=71734395&repo=mozilla-
> beta

The problem is that bug 1316311 wasn't in 52. I'll have a new push once I verify the test change would catch a regression.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 27

4 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/05453493c533
status-firefox52: affected → fixed
Depends on: 1334026
You need to log in before you can comment on or make changes to this bug.