1586304 - Dismissed door-hanger is lost when there is delay introduced before submitting the action

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Timea Cernea [:tbabos][inactive]]

Attachment: Video of the issue

Affected versions
Nightly71
Beta70.0b12

Affected platforms
All

Steps to reproduce

• Launch Firefox
• Go to reddit.com and log in, save credentials
• Go to change password form
• Fill in both password field with generated password (notice the blue key icon)
• Click on Save

Expected result
After the save process is completed and the page reloaded (or redirected) the blue key icon should still be displayed for the user to access the doorhanger

Actual Result
The Blue key icon is not displayed when Reddit redirects to a different page, thus the user cannot save the changes of the password from the doorhanger.

Note
This only happens when Reddit redirects to a different page, otherwise it will work correctly - check the video.

Comment 1

[Timea Cernea [:tbabos][inactive]]

Attachment: LogExport.txt

Comment 2

[Timea Cernea [:tbabos][inactive]]

Hey Sam,

We have 2 different behaviors here for the key icon and dismissed dorhanger:

1. Blue key icon is shown and the user can access the doorhanger -> this is when the page is not redirected
2. The reddit settings page is redirected to a different one (check video) and the key icon is no longer displayed, thus users can't access the doorhanger.

Could you please take a look over the log and confirm if this is indeed a reddit problem or its an issue that could affect other sites as well?

Comment 3

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

Investigating this today. I cant' rule out that this might happen on other sites and there's the potential for data loss here as the password change is not reflected in the saved login, only the auto-saved log - which we no longer have the prompt for.

Comment 4

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

Here's what I know so far:

• The change password form on https://www.reddit.com/prefs/update/ has a form field for the old password and 2 password fields for the new value
• When you fill with the generated password, we create a new empty-username auto-saved login for https://www.reddit.com/ with that generated password value
• When you submit the form, onFormSubmit in LMP gets a formLogin with no username (there was no username field in the form) and with the new password - which is the generated password we auto-saved.
• So, in LoginManagerParent, we find the matching login - which is the auto-saved login - see that the password is already the new generated password value and we just record the use of the login and return.
• Which leaves the auto-saved login with the new/correct password, and your existing username'd login with an obsolete password

So far so good. This is actually expected behavior. The problem is that the mitigation - the dismissed "attention"-state doorhanger which would allow the user to add their username and so update their reddit saved login - gets removed somehow.

Comment 5

[Matthew N. [:MattN]]

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

Investigating this today. I cant' rule out that this might happen on other sites and there's the potential for data loss here as the password change is not reflected in the saved login, only the auto-saved log - which we no longer have the prompt for.

This is only data loss if the user already has an empty-username login saved for the site, right?

Comment 6

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

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

This is only data loss if the user already has an empty-username login saved for the site, right?

Yeah if there's already an empty-username login (e.g. using generated password in a previous session). The other case is that following these STR, the new password which has been sent to reddit.com is saved under the empty-username login. The existing login with your reddit username has your old password. So if you pick that from the autocomplete menu next time you login - which is the reasonable and logical thing to do - you'll get a wrong-password error from the site. So the new password data isn't lost as such, its just misplaced, which amounts to close to the same thing from the user's point of view.

Comment 7

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

The rest of the story:

When you submit the form, an XHR request is made to https://www.reddit.com/api/update_password
On response, the current page (https://www.reddit.com/prefs/update/) reloads
The LoginManagerContent onStateChange handler gets loadType LOAD_CMD_RELOAD, which it does not treat as navigation
In PopupNotifications locationChange, we remove all the non-persistent notifications.

Comment 8

[Adrian Florinescu [:aflorinescu]]

Doing further investigation as well after discussing with MattN the other night, I think the problem here is from the timer and it applies only to the scenario in which the door-hanger is in the dismissed state.

mnoorenberghe 12:43 AM:

that has been the behaviour of the pwmgr doorhanger for a long time
https://searchfox.org/mozilla-central/rev/5cb522c7baba24e55874809e0e206b001494c1e9/toolkit/components/passwordmgr/LoginManagerPrompter.jsm#1298"

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

This is only data loss if the user already has an empty-username login saved for the site, right?

In this case, on the update door-hanger will not be in dismissed state, it will be displayed hence it will persist on reload or redirect.

-- the issue will be caught mostly on change password forms or forms which miss the username
Here's reduced STR.: (use clean profile or delete all about:login entries for the test site)

Case A: (proper functionality)

1. Open https://bugs.mattn.ca/pwmgr/login_and_change_form.html
2. In "Login form" input u/p and press "log in" / save credentials.
3. Open in new tab https://bugs.mattn.ca/pwmgr/login_and_change_form.html
4. Delete the autofill entries, leaving only the autofilled current password in the "Password change form".
5. In the "Password change form" generate secure passwords for both fields and press "change password".

A.R.
On "change password", redirects to success.html page, blue key /dismissed door-hanger is present.

Case B: (dismissed update door-hanger gone) - same steps as in case A, just introduces delay at str 5

1. Open https://bugs.mattn.ca/pwmgr/login_and_change_form.html
2. In "Login form" input u/p and press "log in" / save credentials.
3. Open in new tab https://bugs.mattn.ca/pwmgr/login_and_change_form.html
4. Delete the autofill entries, leaving only the autofilled current password in the "Password change form".
5. In the "Password change form" generate secure passwords for both fields and wait(20s) then press "change password".

A.R.
On "change password", redirects to success.html page, blue key /dismissed door-hanger is gone.

So, AIUI, I guess this explains why we caught this issue so late and why we thought initially it is intermittent.

Comment 9

[Matthew N. [:MattN]]

We will try to fix this for 70 if we can find a low-risk patch (like changing the timeout) but product and eng. doesn't feel that this is a blocker.

Comment 10

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

Attachment: Bug 1586304 - Increase timeout for dismissed attention notifications. r?MattN

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a212d1a84594
Increase timeout for dismissed attention notifications. r=MattN

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a212d1a84594

Comment 13

[Adrian Florinescu [:aflorinescu]]

With the risk of being annoying, the timer here was 10s, now the fix is pushing it to 60s. Granted the +50s increase will reduce the number of occurrences of this particular scenario, but in my view, the noUsername scenario with dismissed door-hanger will only be encountered in a generation scenario, in which the end purpose would be to have the user take an action with the dismissed door-hanger, so I feel that until we advance the feature to version2, we shouldn't expire at all in this particular case, or have something much higher than 60s.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Let me know if this is something you want to uplift to 70.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Oh, looks like it is. If you can fill out the uplift request this can make it into tomorrow's beta 14 build.

Comment 16

[Matthew N. [:MattN]]

(In reply to Adrian Florinescu [:adrian_sv] from comment #13)

With the risk of being annoying, the timer here was 10s, now the fix is pushing it to 60s. Granted the +50s increase will reduce the number of occurrences of this particular scenario, but in my view, the noUsername scenario with dismissed door-hanger will only be encountered in a generation scenario, in which the end purpose would be to have the user take an action with the dismissed door-hanger, so I feel that until we advance the feature to version2, we shouldn't expire at all in this particular case, or have something much higher than 60s.

It's only a minimum delay so as long as their isn't another navigation (probably triggered by the user) between 60s and when the user wants to save then it should be fine. I understand it's not perfect but having a plaintext password linger around too long is also a problem.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 9099696 [details]
Bug 1586304 - Increase timeout for dismissed attention notifications. r?MattN

Beta/Release Uplift Approval Request

• User impact if declined: User won't be able to easily add a username to a generated password (and have it merge with an existing one).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): All we did was increase the minimum time of dismissal in this case.
• String changes made/needed: None

Comment 18

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

(In reply to Adrian Florinescu [:adrian_sv] from comment #13)

Yeah this is a stop-gap fix that reduces the likelihood of running into this bug. But we should think about a bettter way to handle this eventually. One idea was to only expire the doorhanger when the origin changes - but as some sites redirect login form submissions through 3rd party services that isn't perfect either. And as Matt points out, we don't want to leave the doorhanger there indefinitely.

Comment 19

[Adrian Florinescu [:aflorinescu]]

Forgot to mention in comment 13 that we have verified the fix on Nightly 71 on Ubuntu and Windows 10 and even though I concur with comment 17 assessment that there is a need for further manual verification once uplifted, let's just have it verified anyways since there's only a small effort to it.

Changing component since this issue is not a site compat issue anymore.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9099696 [details]
Bug 1586304 - Increase timeout for dismissed attention notifications. r?MattN

Fix for skyline issue, verified in nightly, OK for beta 14 uplift.

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3c8eae8eea98

Comment 22

[Adrian Florinescu [:aflorinescu]]

Verified the fix on Firefox 70.0b14 2019-10-10 on Ubuntu 16.04/ Windows 10 / Mac 10.14.6: inside the increased 60s timer, we persist the dismissed attention notification.