1288721 - Password manager uses old password as username when changing password on accounts.firefox.com

Status: VERIFIED FIXED

(Toolkit :: Password Manager: Site Compatibility, defect)

Description

[Ada [:adalucinet]]

[Affected versions]:
- Firefox 48 beta 10
- latest Aurora 49.0a1
- latest Nightly 50.0a1

[Affected platforms]:
- Windows 10 64-bit
- Mac OS X 10.11.1
- Ubuntu 16.04 LTS 64-bit

[Steps to reproduce]:
1. Launch Firefox with a clean profile.
2. Log into FxA and select to remember credentials in the notification dialog.
3. Via Manage Account page, select to change password.
4. Click on 'Show' button next to 'Old password' field.
5. Enter a New password.
6. Click on 'Change' button.

[Expected result]: Notification dialog contains the correct values.

[Actual result]: The old password is displayed in the Notification dialog instead of the username.

[Regression range]:
- Not reproducible with Firefox 47.0.1; will investigate further.

[Additional notes]:
- Screenshot: https://i.imgur.com/7QFmVUu.png

Comment 1

[Ada [:adalucinet]]

Attachment: about:sync-log

Comment 2

[Mark Hammond [:markh] [:mhammond]]

It looks like this is the password manager mis-detecting the old password as the username?

Comment 3

[Ryan Kelly [:rfkelly]]

> It looks like this is the password manager mis-detecting the old password as the username

I guess this happens because the "old password" field has been turned into a type=text input, so the for has got a type=text input and a type=password input side-by-side, so it kinda looks like a username/password login form.

We have an open bug about smoothing out this flow [1], by e.g. morphing the "old password" field back into an actual type=password field before submitting the form.  This sounds like another reason to complete that work.

[1] https://github.com/mozilla/fxa-content-server/issues/3799

Comment 4

[Ryan Kelly [:rfkelly]]

This seems at least semi-related to Bug 1280294, and since that bug was marked security-sensitive, I'm marking this one similarly just in case.  In fact I wonder if that bug is the reason this is not reproducible on 47, since the patch in Bug 1280294 was uplifted all the way to 48.

Comment 5

[Shane Tomlinson [:stomlinson]]

I'm working on an FxA client side patch to convert password fields that are input[type=text] to input[type=password] in https://github.com/mozilla/fxa-content-server/pull/3969

Comment 6

[Matthew N. [:MattN]]

Does that pull request actually fix this bug? We capture the form data before the submit event occurs so if that PR is toggling to type=password on the submit event it may not work. It's hard for me to follow the patch though. I think you would need to toggle to type=password upon blur of the field.

Comment 7

[Sai Pc]

:MattN I agree that changing on submit event might not fire up before FF stores data, but I *think* Shane took care of that, I also disagree that we should change it on blur, that is not what other major websites do.

Comment 8

[Shane Tomlinson [:stomlinson]]

(In reply to Matthew N. [:MattN] from comment #6)
> Does that pull request actually fix this bug? We capture the form data
> before the submit event occurs so if that PR is toggling to type=password on
> the submit event it may not work. It's hard for me to follow the patch
> though. I think you would need to toggle to type=password upon blur of the
> field.

I have to admit that I read the proposed solution, became fixated on implementing
it, and did not test for the most important outcome - whether the problem is 
actually fixed. The bad news - no, the problem is not fixed.

Changing to onblur is problematic because the password input may not be
focused whenever the user clicks the "show" button. 

There are a couple of other alternatives:

* Only give the user an opportunity to show the password if the password field is currently focused. An onblur solution would then in theory work.
* Only show the password if the "show" button is depressed.
* Disable the feature until we have a content server fix in place, or a Firefox patch lands.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

surprised it took me so long to notice this typo!

Comment 10

[Shane Tomlinson [:stomlinson]]

[1] has just merged, which changes the behavior of the content server to only show the password while the user is depressing the "show" button. The password is no longer saved as the username after the user depresses the "show" button. Thanks for the earlier feedback MattN. This patch should make its way into production with train-67, which goes out in ~ 1.5 weeks.

[1] - https://github.com/mozilla/fxa-content-server/pull/3969

Comment 11

[Shane Tomlinson [:stomlinson]]

Pardon, 3969 was the original PR, the updated PR is https://github.com/mozilla/fxa-content-server/pull/3978

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Alexandra, can you verify the fix after it makes it to production? 
Shane can you let us know when that happens? Thanks.

Comment 14

[Shane Tomlinson [:stomlinson]]

:lizzard, the above referenced PR is now in production.

Comment 15

[Cristian Comorasu, QA [:ccomorasu], Release Desktop QA]

I could not reproduce this bug using Fx48.0b10, latest Aurora and lastet Nightly on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.9.5; I also tested some other scenarios. 
I think due to the modifications in Github the issues are resolved, in consequence I will mark them as verified.