Closed Bug 1388674 Opened 6 years ago Closed 3 years ago

Only prompt to save logins if a field in a login form was modified (off-by-default)

Categories

(Toolkit :: Password Manager, defect, P2)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla73
Tracking Flags:
Tracking Status
relnote-firefox --- 73+
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- verified

People

(Reporter: jukka.lindgren, Assigned: sfoster)

References

(Depends on 1 open bug, Blocks 3 open bugs, Regressed 1 open bug)

Details

(Keywords: perf-alert, Whiteboard: [passwords:heuristics])

User Story

Preference: signon.userInputRequiredToCapture.enabled

* Use a one-time `input` event listener on the detected un/pw fields to update `fieldModified` below. Check isTrusted.
* Ensure that an autofill by Firefox doesn't count as a user modification.
* Check event.isTrusted
* Since LoginManagerContent._getFormFields can return different field depending on whether we're submitting (`isSubmission`), check if the event listener was added to the field upon submission and if it wasn't tracked then prompt anyways (see `trackedFields` below). 

```js
stateForDocument(doc).fieldModificationsByRootElement.set(formLike.rootElement, {
  fieldModified: false, // true if one of the tracked fields was modified
  trackedFields: new WeakSet(),
});
```

Attachments

(8 files, 4 obsolete files)

Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r?MattN
47 bytes, text/x-phabricator-request
Details | Review
Testcase 2: From bug 1568940 without JS
317 bytes, text/html
Details
mochitest-plain-audit.md
11.37 KB, text/plain
Details
browser-chrome-audit.md
6.94 KB, text/plain
Details
Bug 1388674 - Use document state to track generated password fields, not just the event handlers. r?MattN
47 bytes, text/x-phabricator-request
Details | Review
Bug 1388674 - Set signon.userInputRequiredToCapture.enabled in AutofillDelegateTest.kt. r=sfoster
47 bytes, text/x-phabricator-request
Details | Review
Log for "In a form" testcase fail
10.78 KB, text/plain
Details
Testcase 3: Multiple forms without JS
678 bytes, text/html
Details
Attached file Password_displayed.zip — 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170803105720

Steps to reproduce:

1. Browse to any web page, where a Login + Password fields are shown. The existing password is shown as dots, to stop a passer-by to see the password.

2. Do not edit any of the Login / Password fields. 

3. Click any link to browse to another page. Do not type a new address.

4. The "Would you like Firefox to save this login for [domain name]?" dialog will pop up - as if the user had entered a new password to the appropriate field.

5. Click "Show Password" on that dialog.

6. The password from that page is displayed in clear text. 
If the page contains a hash value (as opposed to actual password in text), the hash value is displayed.



Actual results:

A third-party user's password may be revealed, if a page containing that users Login + Password is visited.
If the page has saved the password hash value, instead of the actual password, the hash value is displayed. It may concievably be used to obtain the actual password.


Expected results:

Leaving a page containing an existing password in hidden form should not pop up the "Save password?" dialog, making the saved password visible to other user.

Please note: Attached screenshots (in zip file) show the case with a hashed password only. Due to privacy/security concerns, I do not wish to attach screenshot with a clear-text password.
This is an expected part of how the web browser works. The same values could be inspected with e.g. the developer tools or a purpose-built add-on. It's not clear to me how you believe this feature poses a greater risk to the user or their machine. As a result, removing the 'security sensitive' flag from this bugreport (as it keeps it hidden from the rest of the world).

(In reply to jukka.lindgren from comment #0)
> A third-party user's password may be revealed, if a page containing that
> users Login + Password is visited.

A webpage should never include a third-party user password in its markup or password fields if the entity requesting the page should not see said password. If it does, the website must assume that the password is compromised from that point onwards.

> If the page has saved the password hash value, instead of the actual
> password, the hash value is displayed. It may concievably be used to obtain
> the actual password.

There are two cases here:
1) the hash is reversible or not salted. Consider using a better hash algorithm and a salt. As above, the website should consider the password compromised from the point where it sent the compromisable hash.
2) the hash is not reversible and properly salted. In this case, exposing the hash has limited risk - but that risk is incurred by the website as soon as it exposes the hash, not by the browser for displaying the hash to the user.

> Please note: Attached screenshots (in zip file) show the case with a hashed
> password only. Due to privacy/security concerns, I do not wish to attach
> screenshot with a clear-text password.

I've marked your attachment as private just in case.
Group: firefox-core-security
Component: Untriaged → Password Manager
Product: Firefox → Toolkit
(In reply to :Gijs from comment #1)
This phenomenon (dialog box displayed when leaving the page) only appeared on Firefox 55.0 on Mac. This is new.
I cannot see any rationale on displaying this dialog, if no new Login or Password has been entered - and there is nothing to save.
On all previous versions of Firefox, only entering something onto Login/Password fields triggered the "Save?" dialog.

> This is an expected part of how the web browser works. The same values could
> be inspected with e.g. the developer tools or a purpose-built add-on. It's
> not clear to me how you believe this feature poses a greater risk to the
> user or their machine.

While it is possible to find the hash value via e.g. "Inspect element" function, any additional layer of security is desirable. Revealing it so obviously to a common, non-expert user gives this issue much vider audience. In terms of 'security through obscurity', this is never a good idea.
(In reply to jukka.lindgren from comment #2)
> (In reply to :Gijs from comment #1)
> This phenomenon (dialog box displayed when leaving the page) only appeared
> on Firefox 55.0 on Mac. This is new.
> I cannot see any rationale on displaying this dialog, if no new Login or
> Password has been entered - and there is nothing to save.
> On all previous versions of Firefox, only entering something onto
> Login/Password fields triggered the "Save?" dialog.

It's impossible to comment on this precisely without insight into what pages you're seeing this on and what their markup looks like. However, on earlier versions of Firefox we didn't do a good job of allowing users to save passwords in password fields outside <form> elements. Given that webpages do use such password fields, that got fixed.

If, as a side-effect, we are now showing the prompt when we shouldn't, that is interesting and potentially fixable because of correctness rather than security concerns. However, we can't really diagnose this without a testcase. Can you provide one, either a link or an attachment, that you think shouldn't prompt but does?
Flags: needinfo?(jukka.lindgren)
(In reply to :Gijs from comment #3)

> Can you provide one, either a link or an attachment, that you
> think shouldn't prompt but does?

Unfortunately I found this on an FTP server administrator page.
This is CrushFTP 7.7.0_69, which is available as a trial version (I believe).

The problem occurs on the "User Manager" on the admin section of that software's web UI.

If you cannot find a way to reproduce this on any other web site, I will try to arrange access under high confidentiality.
Flags: needinfo?(jukka.lindgren)
I thought we had a bug on not asking to save for fields that weren't changed but I can't find it. I think that would mostly address this.

Otherwise the prompt and toggling visibility are working as expected. If you don't want the password to be revealed in the prompt, you can set a Master Password.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Summary: Hidden password on a visited page revealed by "Save login?" dialog → Only prompt to save logins if a login field was modified
Whiteboard: [passwords:heuristics]
See Also: → 1298952
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Blocks: 1533383
Assignee: prathikshaprasadsuman → nobody
Status: ASSIGNED → NEW
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
User Story: (updated)
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Only prompt to save logins if a login field was modified → Only prompt to save logins if a login field was modified by the user
Version: 55 Branch → Trunk
User Story: (updated)

Only prompt to save logins if a login field was modified by the user.

Assignee: prathikshaprasadsuman → chengy12

Another test case is attachment 9063653 [details] from bug 1298952 which this bug will probably address.

Assignee: chengy12 → nobody
Status: ASSIGNED → NEW
Attachment #9080750 - Attachment description: Test case from bug 1568940 → Testcase 2: From bug 1568940 without JS
Attachment #9080750 - Attachment filename: file_1388674.txt → file_1388674.htm
Attachment #9080750 - Attachment mime type: text/plain → text/html
status-firefox68: --- → wontfix
status-firefox69: --- → affected
status-firefox70: --- → affected
Priority: P3 → P2
Blocks: 1572371
Flags: qe-verify+
Blocks: 1583150
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
  • Lots of tests updated to use setUserInput so we get the input event, so the message is sent to the parent to create the doorhanger
  • TODO: perhaps the observer.handleEvent input handling could be consolidated with these new input handlers?

I've run all the tests at some point, but here's a windows try run to shake out any more issues.

I'm not entirely clear why it was necessary to import WrapPrivileged.jsm in contentSubmitForm, but not for other functions in the same suite that also run in the content process?

As I noted in the commit message, it seems like we are sprouting event listeners left and right and these could probably be consolidated at some point - perhaps but not necessarily in this bug.

I've got that try run and more manual testing to do and going back over the patch before its ready for review. I may split out all those test changes into their own patch as they should pass with or without the functional changes in LoginManagerChild.

Attachment #9105940 - Attachment is obsolete: true
Attachment #9052970 - Attachment description: Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r?jaws → Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r?MattN

The changes for this bug have implications for how we test form interactions and login capture. To ensure we aren't missing something via false positives or gaps in test coverage, I've done an audit of all the passwordmgr tests, looking at what data is used and if it will cross paths with the new user-modification requirements being implemented here. In most cases where we would previously assign to an input's value property, we just need to use setUserInput(). In some cases, forms with declarative field values would be submitted, expecting a formsubmit message to be sent to the parent process.

A list of the changes to toolkit/components/passwordmgr/test/browser-chrome tests, and notes on what was changed/not changed and potential gotchas.

Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee452cb16fac
Use document state to track generated password fields, not just the event handlers. r=MattN
https://hg.mozilla.org/integration/autoland/rev/5caf9e933738
Update tests to use setUserInput, ensuring we get an input event for field modifications. r=MattN
https://hg.mozilla.org/integration/autoland/rev/09e3e82fb439
Only prompt to save logins if a login field was modified by the user. r=MattN

Backed out 3 changesets (bug 1388674) for mochitest failures e.g browser_doorhanger_toggles.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/dbb0fff9e65ce321b9bf2a7df7edb78cdc4288e2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=09e3e82fb4396ae250cc5e92c7af8447ff5a5683&selectedJob=276890294

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276890294&repo=autoland&lineNumber=21965

Log snippet:
[task 2019-11-19T01:31:48.050Z] 01:31:48 INFO - TEST-START | toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js
[task 2019-11-19T01:31:48.086Z] 01:31:48 INFO - GECKO(1966) | ++DOCSHELL 0x114545800 == 2 [pid = 1970] [id = {67554b95-6129-bf41-934a-625981fe02f3}]
[task 2019-11-19T01:31:48.087Z] 01:31:48 INFO - GECKO(1966) | ++DOMWINDOW == 19 (0x11b67d200) [pid = 1970] [serial = 201] [outer = 0x0]
[task 2019-11-19T01:31:48.087Z] 01:31:48 INFO - GECKO(1966) | ++DOMWINDOW == 20 (0x114e39c00) [pid = 1970] [serial = 202] [outer = 0x11b67d200]
[task 2019-11-19T01:31:48.160Z] 01:31:48 INFO - GECKO(1966) | ++DOMWINDOW == 21 (0x11b51dc00) [pid = 1970] [serial = 203] [outer = 0x11b67d200]
[task 2019-11-19T01:31:48.235Z] 01:31:48 INFO - GECKO(1966) | console.warn: LoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
[task 2019-11-19T01:31:48.451Z] 01:31:48 INFO - GECKO(1966) | [Child 1970, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /builds/worker/workspace/build/src/layout/forms/nsTextControlFrame.cpp, line 300
[task 2019-11-19T01:31:48.451Z] 01:31:48 INFO - GECKO(1966) | [Child 1970, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /builds/worker/workspace/build/src/layout/forms/nsTextControlFrame.cpp, line 300
[task 2019-11-19T01:31:48.565Z] 01:31:48 INFO - GECKO(1966) | ++DOMWINDOW == 22 (0x11b52ac00) [pid = 1970] [serial = 204] [outer = 0x11b67d200]
[task 2019-11-19T01:31:48.632Z] 01:31:48 INFO - GECKO(1966) | console.warn: LoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
[task 2019-11-19T01:31:48.656Z] 01:31:48 INFO - TEST-INFO | started process screencapture
[task 2019-11-19T01:31:48.819Z] 01:31:48 INFO - TEST-INFO | screencapture: exit 0
[task 2019-11-19T01:31:48.819Z] 01:31:48 INFO - Buffered messages logged at 01:31:48
[task 2019-11-19T01:31:48.819Z] 01:31:48 INFO - Entering test bound common_initialize
[task 2019-11-19T01:31:48.819Z] 01:31:48 INFO - Leaving test bound common_initialize
[task 2019-11-19T01:31:48.819Z] 01:31:48 INFO - Entering test bound setup
[task 2019-11-19T01:31:48.819Z] 01:31:48 INFO - Leaving test bound setup
[task 2019-11-19T01:31:48.820Z] 01:31:48 INFO - Entering test bound test_toggle_password
[task 2019-11-19T01:31:48.820Z] 01:31:48 INFO - TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js | Check doorhanger username -
[task 2019-11-19T01:31:48.820Z] 01:31:48 INFO - TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js | Check doorhanger password -
[task 2019-11-19T01:31:48.820Z] 01:31:48 INFO - Buffered messages finished
[task 2019-11-19T01:31:48.820Z] 01:31:48 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js | false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js :: test_toggle_password/< :: line 39
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - Stack trace:
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js:test_toggle_password/<:39
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - resource://testing-common/BrowserTestUtils.jsm:withNewTab:151
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_doorhanger_toggles.js:test_toggle_password:12
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1069
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1104
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:932
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-11-19T01:31:48.821Z] 01:31:48 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(sfoster)

Thanks :RaulG, looks like it only failed on OSX. I'll investigate.

Flags: needinfo?(sfoster)

Looks like I exacerbated an existing problem with browser_doorhanger_toggles.js. The updated patch should fix this and reduce the intermittents this test sees.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a68132a0780e246379bc8366445c207435d09b8&selectedJob=277091331

Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b723ff1791cd
Use document state to track generated password fields, not just the event handlers. r=MattN
https://hg.mozilla.org/integration/autoland/rev/74fb61b252bb
Update tests to use setUserInput, ensuring we get an input event for field modifications. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2bbba46b221c
Only prompt to save logins if a login field was modified by the user. r=MattN

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=277127623&resultStatus=testfailed%2Cbusted%2Cexception&revision=2bbba46b221cfe512fe851778ede9e905e6e44f6&searchStr=mochitest

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=277127623&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/fcd9570d85ee3a11d687d28ca871dddad9bc565d

[task 2019-11-20T07:22:06.191Z] 07:22:06 INFO - TEST-PASS | toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html | Reason cannot be empty
[task 2019-11-20T07:22:06.192Z] 07:22:06 INFO - add_task | Entering test test_init
[task 2019-11-20T07:22:06.193Z] 07:22:06 INFO - add_task | Leaving test test_init
[task 2019-11-20T07:22:06.195Z] 07:22:06 INFO - add_task | Entering test test_no_message_on_navigation
[task 2019-11-20T07:22:06.195Z] 07:22:06 INFO - Buffered messages finished
[task 2019-11-20T07:22:06.196Z] 07:22:06 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html | SecurityError: Permission denied to access property "document" on cross-origin object - Should not throw any errors
[task 2019-11-20T07:22:06.197Z] 07:22:06 INFO - get@resource://specialpowers/WrapPrivileged.jsm:228:23
[task 2019-11-20T07:22:06.198Z] 07:22:06 INFO - setup@http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html:42:19
[task 2019-11-20T07:22:06.198Z] 07:22:06 INFO - asynctest_no_message_on_navigation@http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html:66:24
[task 2019-11-20T07:22:06.199Z] 07:22:06 INFO - nextTick/<@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:1797:34
[task 2019-11-20T07:22:06.200Z] 07:22:06 INFO - asyncnextTick@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:1813:11
[task 2019-11-20T07:22:06.200Z] 07:22:06 INFO - setTimeout handler*SimpleTest_setTimeoutShim@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:686:43
[task 2019-11-20T07:22:06.202Z] 07:22:06 INFO - add_task@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:1757:17
[task 2019-11-20T07:22:06.202Z] 07:22:06 INFO - @http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html:57:9
[task 2019-11-20T07:22:06.203Z] 07:22:06 INFO - GECKO(6676) | console.warn: LoginManager: "searchLogins: formActionOrigin or httpRealm is recommended"
[task 2019-11-20T07:22:06.204Z] 07:22:06 INFO - GECKO(6676) | MEMORY STAT | vsize 2574MB | residentFast 172MB | heapAllocated 23MB
[task 2019-11-20T07:22:06.204Z] 07:22:06 INFO - GECKO(6676) | Removing 1 popup notifications.
[task 2019-11-20T07:22:06.208Z] 07:22:06 INFO - TEST-OK | toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html | took 1166ms

Also these screenshot failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Cshippable%2Copt%2Cmochitests%2Ctest-linux64-shippable%2Fopt-browser-screenshots-e10s%2Cm%28ss%29&fromchange=2bbba46b221cfe512fe851778ede9e905e6e44f6&tochange=fcd9570d85ee3a11d687d28ca871dddad9bc565d&selectedJob=277135351

https://treeherder.mozilla.org/logviewer.html#?job_id=277135351&repo=autoland

Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10dde27b429c
Use document state to track generated password fields, not just the event handlers. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2c96c192a576
Update tests to use setUserInput, ensuring we get an input event for field modifications. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2e38cf3d0d27
Only prompt to save logins if a login field was modified by the user. r=MattN

(In reply to Cosmin Sabou [:CosminS] from comment #24)

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=277127623&resultStatus=testfailed%2Cbusted%2Cexception&revision=2bbba46b221cfe512fe851778ede9e905e6e44f6&searchStr=mochitest

Not sure if I missed this Fission test failure in the previous landing attempt, but this is fixed now and fingers crossed that 3rd time is a charm.

Flags: needinfo?(sfoster)
Blocks: 1536728
Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05c8232e7dc5
Use document state to track generated password fields, not just the event handlers. r=MattN
https://hg.mozilla.org/integration/autoland/rev/d1e436047e1e
Update tests to use setUserInput, ensuring we get an input event for field modifications. r=MattN
https://hg.mozilla.org/integration/autoland/rev/d9b1730b8cb3
Only prompt to save logins if a login field was modified by the user. r=MattN
Blocks: 1287202
Depends on: 1599315

Comment on attachment 9106340 [details]
Bug 1388674 - Update tests to use setUserInput, ensuring we get an input event for field modifications. r?MattN

Revision D51718 was moved to bug 1599315. Setting attachment 9106340 [details] to obsolete.

Attachment #9106340 - Attachment is obsolete: true
Flags: needinfo?(sfoster)
Attachment #9052970 - Attachment description: Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r?MattN → Bug 1388674 - (WIP) Only prompt to save logins if a login field was modified by the user. r?MattN
Attachment #9052970 - Attachment description: Bug 1388674 - (WIP) Only prompt to save logins if a login field was modified by the user. r?MattN → Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r?MattN
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a8165f1e03c
Use document state to track generated password fields, not just the event handlers. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2a5b202965c5
Only prompt to save logins if a login field was modified by the user. r=MattN

https://hg.mozilla.org/mozilla-central/rev/4a8165f1e03c
https://hg.mozilla.org/mozilla-central/rev/2a5b202965c5

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox73: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla73
User Story: (updated)
Summary: Only prompt to save logins if a login field was modified by the user → Only prompt to save logins if a login field was modified by the user (off-by-default)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59d09fb05cf9
Set signon.userInputRequiredToCapture.enabled in AutofillDelegateTest.kt. r=sfoster
Blocks: 1603226

Updating summary to reflect what we enabled in bug 1603226. Any edit in the LoginForm allows a capture still, in the future we can look specifically for login fields being edited.

Summary: Only prompt to save logins if a login field was modified by the user (off-by-default) → Only prompt to save logins if a field in a login form was modified by the user (off-by-default)

Hey Matt and Sam,

On latest Nightly 73.0a1 (2020-01-03) (64-bit) with "signon.userInputRequiredToCapture.enabled" enabled by default:

  • Both pre-filled (in a form and outside) fields from Testcase 1: Username and password fields filled by JS will toggle the save prompt once the user hits submit without editing the field. -> doesn't work
  • The save prompt will not be toggled upon clicking the link from Testcase 2: From bug 1568940 without JS -> this is fixed

Attached Log for "In a form" case.
Also mentioned this in Bug 1603226 where the pref was enabled by default.

Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)

(In reply to Timea Cernea [:tbabos] from comment #38)

Hey Matt and Sam,

On latest Nightly 73.0a1 (2020-01-03) (64-bit) with "signon.userInputRequiredToCapture.enabled" enabled by default:

  • Both pre-filled (in a form and outside) fields from Testcase 1: Username and password fields filled by JS will toggle the save prompt once the user hits submit without editing the field. -> doesn't work

I think by "doesnt work" you mean that if the field values were filled by JS and fields in the form had no actual user input, the test case expects there will not be a prompt to save/update the login. We ended up getting not enforcing that, and treat a form with a JS-modified value as being modified and allow the save/update doorhanger to show. We were concerned we didn't really understand the impact not prompting for js-modified fields would have, so elected for the more incremental, conservative implementation. I'll update the bug title to clarify, and file a follow-up to revisit this decision in the future.

Flags: needinfo?(sfoster)
Flags: needinfo?(MattN+bmo)
Summary: Only prompt to save logins if a field in a login form was modified by the user (off-by-default) → Only prompt to save logins if a field in a login form was modified (off-by-default)

Thanks for the explaining Sam! In this case, we can mark it as verified-fixed on Windows 10, MacOS 10.14 and Ubuntu 18.04 given its expected for the save prompt to appear for fields pre-filled by JS.

Status: RESOLVED → VERIFIED
status-firefox73: fixed → verified
Flags: qe-verify+
Depends on: 1607346

Added to the final Fx73 relnotes.

relnote-firefox: --- → 73+

Just updated to Firefox 73 excited to finally have this fixed but it's still broken. :-( Do I need to change a setting or something? To be 100% clear:

a) visit a form with a password field
b) do nothing
c) navigate away from page

Firefox still prompts me if I want to save password.

OSX 10.11.6
Firefox 73.0

(In reply to billynoah from comment #43)

Just updated to Firefox 73 excited to finally have this fixed but it's still broken. :-( Do I need to change a setting or something? To be 100% clear:

a) visit a form with a password field
b) do nothing
c) navigate away from page

Firefox still prompts me if I want to save password.

If javascript on the page sets/changes form field values, it will still treat that as modified and offer to save the password. Do you have an example form? I can look to verify that is what is going on.

Flags: needinfo?(billynoah)

(In reply to Sam Foster [:sfoster] (he/him) from comment #44)

(In reply to billynoah from comment #43)

Just updated to Firefox 73 excited to finally have this fixed but it's still broken. :-( Do I need to change a setting or something? To be 100% clear:

a) visit a form with a password field
b) do nothing
c) navigate away from page

Firefox still prompts me if I want to save password.

If javascript on the page sets/changes form field values, it will still treat that as modified and offer to save the password. Do you have an example form? I can look to verify that is what is going on.

Thanks Sam,

No, no javascript here. I've created a simple page to demonstrate here:
http://mozilla-1388674.dev.zuma-design.com/

There is nothing there other than a form with some values populated and some buttons. When I click the "Cancel" link, I get the prompted to save my password.

Flags: needinfo?(billynoah)

Since this bug doesn't appear to be fixed and I've provided a reproducible example, what is the protocol here? Open a new bug or change status and re-open this one?

Sam, how do you best want to handle this potentially new issue?

Flags: needinfo?(sfoster)
Depends on: 1616945

(In reply to billynoah from comment #46)

Since this bug doesn't appear to be fixed and I've provided a reproducible example, what is the protocol here? Open a new bug or change status and re-open this one?

Thank you very much for the test case and report. We never re-open bugs unless the code gets reverted so I filed bug 1616945 for you.

Flags: needinfo?(sfoster)
Attachment #9063651 - Attachment is obsolete: true
Attachment #9127977 - Attachment mime type: text/plain → text/html
Attachment #9127977 - Attachment is obsolete: true
Regressions: 1641942
Keywords: perf-alert
You need to log in before you can comment on or make changes to this bug.