search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.81k stars 4.82k forks source link

[Bug]: The new password is not recognized; Metamask only accepts the old password after wallet reset #25696

Closed anaamolnar closed 1 month ago

anaamolnar commented 1 month ago

Describe the bug

After going through the 'Forgot password' flow in Metamask and setting a new password, Metamask throws an 'Invalid password' warning when trying to login with the new password

Expected behavior

Metamask should only recognize the newly set password.

Screenshots/Recordings

https://github.com/MetaMask/metamask-extension/assets/82837486/0de00905-ee01-4241-a90e-da60c5eb0976

Steps to reproduce

  1. Open the extension
  2. Proceed to Forget password flow
  3. Paste your secret recovery phrase
  4. Set a new different password
  5. Proceed to Restore your Wallet
  6. After entering the account, lock it and try to login with the new password
  7. It will throw an Invalid password error

Error messages or log output

No response

Version

12.0.0

Build type

Beta

Browser

Chrome, Firefox

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response

owencraston commented 1 month ago

This issue was patch fixed in v12 but still needs to be addressed on the develop branch.

benjisclowder commented 2 weeks ago

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

**1. What PR fixed the issue?

  1. Can you pinpoint the commit from which the issue originated?
  2. Write a short explanation of the technical cause of the bug
  3. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?** 4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue? 4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue? 4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @owencraston

owencraston commented 2 weeks ago
  1. What PR fixed the issue?
  2. Can you pinpoint the commit from which the issue originated?
  3. Write a short explanation of the technical cause of the bug
    • Currently, when a user locks the app and then resets their password, they are not able to log in wth their new password.
    • This is because the encryption key was generated with the old password and not cleared.
    • To solve this we will remove the encryption key from state when the user locks the wallet as well as when #createNewVaultWithKeyring is called.
    • This allows for a new encryption key to be generated with the new password when the user logs in again and submitPassword is called.
  4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
    • Better end to end testing that tested the password reset flow 4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
    • yes. and I added new unit tests for this case here 4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
    • no 4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?
    • QA that went through this user journey.
    • Better E2E

cc @benjisclowder

metamaskbot commented 1 week ago

Missing release label release-12.0.0 on issue. Adding release label release-12.0.0 on issue, as issue is linked to PR #25787 which has this release label.