Closed Bug 1166995 Opened 9 years ago Closed 3 years ago

Run login capture code when a form or a formless password field is removed from the document tree

Categories

(Toolkit :: Password Manager, enhancement, P3)

Product:
Toolkit
Component:
Password Manager
Type:
enhancement
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- verified
firefox89 --- verified

People

(Reporter: MattN, Assigned: dimi)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [passwords:heuristics] [passwords:capture-UI] )

Attachments

(6 files, 2 obsolete files)

Bug 1166995 - P1. Notify DOMDocFetchSuccess when a fetch or XHR request completes successfully
48 bytes, text/x-phabricator-request
Details | Review
Bug 1166995 - P2. Call onFormSubmit when a form or a password field is removed from the DOM tree
48 bytes, text/x-phabricator-request
Details | Review
Bug 1166995 - P3. Do not check isConnected when onFormSubmit is triggered by form removal inference
48 bytes, text/x-phabricator-request
Details | Review
Bug 1166995 - P4. Add testcases
48 bytes, text/x-phabricator-request
Details | Review
removeFormLoginCapture.html test case
1023 bytes, text/html
Details
Bug 1166995 - P5. Add a preference to control whether to capture login when a form is removed
48 bytes, text/x-phabricator-request
Details | Review 
Pages can use JS to handle logins instead of using form submission events and we should try to capture the saved values in those cases. One heuristic is if a password field is removed from a document.

We can likely use a custom DOM event (like bug 1132211) or Mutation Observers/Events for this but whatever we use will likely need to have access to the removed element and the associated username field before it gets deleted.

Some edge cases to consider:
* Do we want to capture if the user closes a tab? What about the page doing window.close (e.g. Persona)? These may also trigger the input to be removed when the document gets destroyed.
* We don't want to capture when the document gets deleted from the bfcache.
* We probably don't want to cause a second capture doorhanger to appear if the values were already captured via other methods (e.g. onbeforesubmit or other heuristics).
* We probably don't want to display multiple capture doorhangers (replacing the previous one) when there were multiple password fields in the form (e.g. a password change or registration form with double-entry) as the user may have already dismissed the first one and reopening would interrupt the user.
See Also: → 1166998
Blocks: 442524
No longer blocks: 442524
Priority: -- → P3
Whiteboard: [passwords:heuristics]
Whiteboard: [passwords:heuristics] → [passwords:heuristics] [passwords:capture-UI]
Flags: qe-verify+
Blocks: 1592800
Blocks: 1686043
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Blocks: 1691015
Blocks: 1691377
Blocks: 1691969
Blocks: 1691970
Blocks: 1692212

The goal of this series of patches is to address the cases where sites
don't use standard form submission event.

The basic idea is inferring a form is submitted when the form is removed
from the DOM tree, but with one premise:
There must be a successful ajax request sent in the document before the
form is removed.

This is because websites usually send the credentials with an ajax
request (fetch or xhr) and then removes the form.

In summary, this patch does the following:

  1. Add NotifyAjaxSuccess API in Document. The API sends a
    "DOMDocAjaxSuccess" event to who registers the event listener.

  2. When a fetch request or a XMLHttpReuqest completes and succeeds
    (status code between 200~299), call NotifyAjaxSuccess().

  3. Add SetAjaxSuccessEventListener API in Document.webidl.
    LoginMangerChild calls the API when there is user interaction on a
    form (or a form-less password field) to listen to AjaxSuccess event.

  4. When LoginMangerChild receives an AjaxSuccess event, it resets the event
    listener to null (because the DOM mutation observer will be set up,
    we don't have to listen to the event anymore)

This patch uses MutationObserver to check whether a form or a form-less
password is removed from the DOM tree. If any node on the parent chain
of the form (or the form-less password filed) is removed, we consider
the node is removed.

This patch also adds a WeakFieldSet formlessModifiedPasswordFields to
record all the form-less password fields that users have interacted
with. This is because before this patch, we only record the "root
element" of a form or a password field, but in order to walk through
the parent chain of a form-less password field to observe DOM tree
changes, we need to have a data structure knowing the password field.

Depends on D106024

In Bug 1386283, we add isConnected to make sure we don't prompt saved
login doorhanger when a form is removed, and then a page navigation
happens afterward. However, the isConnected prevents the form removal
heuristic from working because the username/password fields are not connected
when a form is "removed".

This patch adds an additional parameter to ignore the isConnceted check
when _onFormSubmit is triggered by the form removal inference.

Depends on D106025

Depends on D106026

Attachment #9204677 - Attachment description: Bug 1166995 - P1. Support registering AjaxSuccess event handler in Document → Bug 1166995 - P1. Notify DOMDocFetchSuccess when a fetch or XHR request completes successfully
Summary: Run login capture code when a password field is removed from the document tree → Run login capture code when a form or a formless password field is removed from the document tree
Attached file removeFormLoginCapture.html (obsolete) —

I think this should be a valid test case?

  • If you click either button without interacting with the form it shouldn't put up the capture prompt
  • If you put in a password value and click "Just remove form" it shouldn't put up the capture prompt
  • If you put in a password value and click "Fetch and remove form" it should prompt to save the password.
Attached file removeFormLoginCapture.html (obsolete) —

Adding a valid url to ensure the fetch is successful.

Attachment #9208831 - Attachment is obsolete: true

:dimi, can you verify attachment 9208834 [details] is a valid test case? That's not working for me... I did confirm the patch works as expected on bigcommerce.com, but I'd like to have a reduced case for this bug.

Flags: needinfo?(dlee)

Add preference signon.formRemovalCapture.enabled, default on

Depends on D106027

Blocks: 1698498

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

:dimi, can you verify attachment 9208834 [details] is a valid test case? That's not working for me... I did confirm the patch works as expected on bigcommerce.com, but I'd like to have a reduced case for this bug.

Hi Sam,
Yes I have verified the patch worked by using the attachment with all the testing scenarios listed in Comment 5.
Not sure why it doesn't work in your environment. Did you include patch P3 while testing?
Anyway, I rebased and updated all the patches, could you help use the latest one and check again? thank you!

Flags: needinfo?(dlee)

(In reply to Dimi Lee [:dimi][:dlee] from comment #10)

Yes I have verified the patch worked by using the attachment with all the testing scenarios listed in Comment 5.
Not sure why it doesn't work in your environment. Did you include patch P3 while testing?
Anyway, I rebased and updated all the patches, could you help use the latest one and check again? thank you!

Ok, I pulled everything down and built again (I had clobbered earlier in the day). This time attachment 9208834 [details] seemed to work ok.
I also checked the login page on bigcommerce.com - I found that my breakpoint in LoginManagerChild.jsm#_onFormSubmit showed that was a regular form submission now? Maybe they redesigned the page..

I've added my r+ to the patches. If you have r+ from :smaug for the parts he had been looking at, I think we are ready to land this.

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

Ok, I pulled everything down and built again (I had clobbered earlier in the day). This time attachment 9208834 [details] seemed to work ok.
I also checked the login page on bigcommerce.com - I found that my breakpoint in LoginManagerChild.jsm#_onFormSubmit showed that was a regular form submission now? Maybe they redesigned the page..

bigcommerce uses regular form submission in their login page, but not in registration page. So to test this bug, we'll have to test the registration page.

Attachment #9204678 - Attachment description: Bug 1166995 - P2. Call _onFormSubmit when a form or a password field is removed from the DOM tree → Bug 1166995 - P2. Call onFormSubmit when a form or a password field is removed from the DOM tree
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c02d83b9352
P1. Notify DOMDocFetchSuccess when a fetch or XHR request completes successfully r=smaug,sfoster
https://hg.mozilla.org/integration/autoland/rev/42db612fa2c8
P2. Call onFormSubmit when a form or a password field is removed from the DOM tree r=sfoster,tgiles,smaug
https://hg.mozilla.org/integration/autoland/rev/883288ee1291
P3. Do not check isConnected when onFormSubmit is triggered by form removal inference r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/29072892d937
P4. Add testcases r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/b932f18c2551
P5. Add a preference to control whether to capture login when a form is removed r=sfoster,tgiles

NI? myself to not forget QA verification.

Flags: needinfo?(timea.babos)

Marking this as verified-fixed based on the verifications done on the latest Nightly 89 and Beta 88.0b2 in the blocking bugs.

Status: RESOLVED → VERIFIED
status-firefox88: fixed → verified
status-firefox89: --- → verified
Flags: qe-verify+
Flags: needinfo?(timea.babos)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: