1555209 - Use data from the autocomplete result in the parent to fill the correct password in content

Status: NEW

(Toolkit :: Password Manager, defect, P2)

Description

[Matthew N. [:MattN]]

+++ This bug was initially created as a clone of Bug #1166113 +++

When the Password Manager finds more than one login, the way we fill username and passwords is a bit odd (see bug 1127567 comment 15).

First we search for logins to fill the username field. Then we search for logins again to fill the password field (and make an additional request to the parent). Instead, we should take the data from autocomplete to populate the username, and also use it to populate the password field.

The proper way to do this for Fission would be to have all the autocomplete querying in the parent process and then only send the password to the content process to fill once a user selects it.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 2

[Matthew N. [:MattN]]

This is a defect from a security perspective.

Comment 3

[Matthew N. [:MattN]]

As part of this work we should probably unify the AutocompleteItem subclasses in the content process with the MozAutocompleteRichlistitem subclasses in the parent process, having the result in the parent process. The content process should mostly start/stop autocomplete searches in the parent process and potentially fill the selected value, not much else.

Comment 4

[Matthew N. [:MattN]]

We can maybe do this with onPopupClosed: https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/toolkit/components/passwordmgr/LoginAutoComplete.jsm#718

Comment 5

[Matthew N. [:MattN]]

The actual login filling could maybe use LoginManagerParent.fillForm if we include a content element identifier with the autocomplete message.

Comment 6

[Matthew N. [:MattN]]

The short term fix here (until we can rewrite how content autocomplete works) would be to:

1. not return login.password from the autocompleteLogins message
2. have the autocomplete row's value be "" for saved logins to signal to autocomplete that it shouldn't try to fill anything itself when the row is selected
3. Have some hook into AutoCompleteParent from LoginManagerParent to know that a row was selected and do the fill ourselves with LoginManagerParent.fillForm

Before trying this we should check if the method currently used for filling item.value from autocomplete are synchronous or async since switching to using LoginManagerParent.fillForm would make it async and affect the timing of when autocomplete values are filled. This can be a problem if the user hits enter quickly twice in a row: once to fill from autocomplete and again to submit the form. If the fill didn't happen before the submission it's not going to go over well.

https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1276,1279 looks to be the place where it happens and that looks synchronous in the content process so I don't think this plan as-is will work…

Maybe we can keep the autocomplete popup open (consuming the events) until the new value is received by content?

A less effective approach would be to send the password/value for the selected row to the content process when the row is selected… still leaking the password to the content process in advance of needing it but at least all logins for the baseDomain don't filter through the content process in advance…

Given that I don't think there is low-hanging fruit here I think we should unassign this for now.