1618587 - Don't allow revealing autofilled passwords in the dismissed login capture doorhanger from an edit

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

(Quoting Matthew N. [:MattN] from bug 1536728 comment #0)

Potential issues:
…

• We probably should not show the doorhanger toggle for edits to autofilled passwords since that makes them too easily revealed to users. We should check how Chrome handles this. We could either not show the doorhanger when it was autofilled previously (we already have that some of that state) or we should hide the checkbox in that case.

(Quoting katieC from bug 1536728 comment #4)

Hey Sam, based on our conversation, we decided:
• remove show password toggle from doorhanger for autofill +edit (for pages where we can't detect fields)

I don't exactly understand what "(for pages where we can't detect fields)" is referring to here.

Sam, did you confirm how Chrome handles this case? I think we need to look at that before decide if this is a blocker. Marking as P1 until we have that.

Comment 1

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #0)

Sam, did you confirm how Chrome handles this case? I think we need to look at that before decide if this is a blocker. Marking as P1 until we have that.

In Chrome, the toggle icon is there in the dismissed doorhanger, but clicking it puts up the OS auth prompt (on windows at least). Once authorized, it toggles on/off without further prompting.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Currently the decision to show/not show the password visibility toggle is made in LoginManagerPrompter.jsm, in the "showing" event handler on the doorhanger's popup panel. So we need to pass either a boolean through from LoginManagerParent, or the autofilled login guid which when non-empty/true will be used to set the hidden attribute on that checkbox.

This means adding another argument to both promptToChangePassword and promptToSavePassword (because editing the username of an autofilled login in a form would otherwise be a trivial way to reveal the password in the save-password doorhanger.)

It would be good to refactor this growing list of arguments into a dictionary object, but that has larger scope and as this bug exists today in the (non-dismissed) form capture I'd like to end up with a potentially upliftable patch.

Comment 3

[Sam Foster [:sfoster] (he/him)]

I have a WIP for this. Will run it through try before requesting review

Comment 4

[Matthew N. [:MattN]]

I think we decided:

• Factor a helper out of AboutLoginsParent (patch not landed) to LoginHelper to re-auth the user with the appropriate mechanism (MP or OS)
  ** Ideally that helper would be able to tell the consumer if the user was on a platform without MP and without OS re-auth: LInux (e.g. maybe it will reject?)
• When the user toggles password visibility, use the helper to prompt for re-auth and if successful then toggle visibility, otherwise don't.

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #4)

• When the user toggles password visibility, use the helper to prompt for re-auth and if successful then toggle visibility, otherwise don't.

The logic looks like:

• only show a toggle at all if signon.rememberSignons.visibilityToggle is true
• But hide toggle (prevent the password from being revealed) if
  • master password is set.
  • the doorhanger was created as a dimissed doorhanger which has been opened
  • we're adding a username to a password-only login which was not created/changed super recently
  • the login was autofilled

I wonder if we need all this? What if we always showed the toggle (provided signon.rememberSignons.visibilityToggle is true) and clicking it always calls LoginHelper.requestReauth() before doing anything? I.e. ditch the autofilled check, ditch the special case for username-only editing, ditch the special case for dismissed-by-default doorhangers.

As it stands that would simplify things, but remove some perceived protection for cases when MP isn't set or the native OSKeyStore isnt supported (e.g. Linux.) I can keep the existing logic (and add the autofilled check) for these cases if necessary but it would be nice to have a plan to make this simplification later if not now.

Comment 6

[Matthew N. [:MattN]]

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

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #4)

• When the user toggles password visibility, use the helper to prompt for re-auth and if successful then toggle visibility, otherwise don't.

The logic looks like:

• only show a toggle at all if signon.rememberSignons.visibilityToggle is true
• But hide toggle (prevent the password from being revealed) if
  • master password is set.

One reason for this is because I think on some OS or window managers, opening the MP dialog would cause the doorhanger to close, thus making it impossible to toggle the visibility and see the result. If that's not an issue then we can remove this restriction.

• the doorhanger was created as a dimissed doorhanger which has been opened

This was to avoid someone coming by your computer a long time later and then peaking at the password that you forgot you submitted hours ago.

• we're adding a username to a password-only login which was not created/changed super recently
• the login was autofilled

I wonder if we need all this?

I don't think so.

What if we always showed the toggle (provided signon.rememberSignons.visibilityToggle is true) and clicking it always calls LoginHelper.requestReauth() before doing anything? I.e. ditch the autofilled check, ditch the special case for username-only editing, ditch the special case for dismissed-by-default doorhangers.

I think that sounds good for me. I would ditch signon.rememberSignons.visibilityToggle first… there will be little need to disable it if we have the re-auth.

As it stands that would simplify things, but remove some perceived protection for cases when MP isn't set or the native OSKeyStore isnt supported (e.g. Linux.)

Hmm… I wonder if we can assume that most Linux users are tech-savvy enough to lock their computer if they care about protecting something like this…

I can keep the existing logic (and add the autofilled check) for these cases if necessary but it would be nice to have a plan to make this simplification later if not now.

I would just run your decision by Sandy. If possible it would be good to do some of the simplification is separate commits for easier reviewing.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1618587 - Refactor the OSKeyStore reauthentication into a LoginHelper method, add canReauth method on OSKeyStore. r?Jaws

• New canReauth method on OSKeyStore
• Moved MP and OKKeyStore.ensureLoggedIn checks from AboutLoginParent to new requestReauth method on LoginHelper
• Use canReauth in LoginManagerPrompter when displaying/hiding the password visiblity toggle in the doorhanger to use requestReauth when possible, and checking it in onVisibilityToggle.
• Added autoFilledLoginGuid argument to promptToSavePassword and prompToChangePassword

Comment 8

[Julien Cristau [:jcristau]]

Does something need to happen for 75 here?

Comment 9

[Matthew N. [:MattN]]

No, the feature got delayed to 76.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1618587 - Refactor the browser_doorhanger_toggles.js test. r?MattN

Depends on D67681

Comment 11

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1618587 - Add autoFilledLoginGuid argument to promptToSavePassword and promptToChangePassword. r?MattN

• Add autofilledLoginGuid argument to the .idl and toolkit implementations of promptToSavePassword and promptToChangePassword
• Pass the guid of the autofilled login along to the prompter methods in LoginManagerParent
• Don't show the password visibility toggle in the capture doorhanger when the login was autofilled

Depends on D68965

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1618587 - (WIP) Use OS reauth when possible to toggle password visibility in the doorhanger.

Depends on D68966

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38bf18564421
Refactor the browser_doorhanger_toggles.js test. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7774b7056528
Add autoFilledLoginGuid argument to promptToSavePassword and promptToChangePassword. r=MattN

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/38bf18564421
https://hg.mozilla.org/mozilla-central/rev/7774b7056528

Comment 15

[Takanori MATSUURA]

There are "Needs Revision" and "Needs Review" patches.
Missing leave-open?

Comment 16

[Matthew N. [:MattN]]

No, they will move to bug 1626472. Thanks for checking.

Comment 17

[Phabricator Automation]

Comment on attachment 9134772 [details]
Bug 1618587 - Refactor the OSKeyStore reauthentication into a LoginHelper method, add canReauth method on OSKeyStore. r?Jaws

Revision D67681 was moved to bug 1626472. Setting attachment 9134772 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9137062 [details]
Bug 1618587 - (WIP) Use OS reauth when possible to toggle password visibility in the doorhanger.

Revision D68967 was moved to bug 1626472. Setting attachment 9137062 [details] to obsolete.

Comment 19

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Nightly 76.0a1 (2020-04-02) on Windows 10 x64, MacOS 10.13 and Ubuntu 16.04.
The "Show Password" toggle is not displayed in the doorhanger for edits on autofilled credentials.
Ensured MP functionality did not regress, "Show Password" toggle is not displayed when MP is set up.