Reuse password reset tokens until they expire

[fredden]

Description (*)

This avoids a race condition where email delivery is slow and user generates
new token before original (now invalid) token is received.

Consider the following scenario:

1. User requests token (1)
2. User waits, but does not yet receive the token
3. User requests another token (2)
4. User receives token (1), but this is invalid
5. User requests another token (3)
6. User receives token (2), but this is invalid
7. [repeat]

This also has the added bonus of not giving an attacker sequential tokens upon request. While the tokens are understood to be suitably opaque, any information disclosures identified in future will be at least partially mitigated by reusing the same token rather than giving out another token data point.

Fixed Issues (if relevant)

Manual testing scenarios (*)

1. Generate password reset email (admin & frontend)
2. Wait for time-out
3. Generate another password reset email
4. Observe same token in both emails

Questions or comments

I am unsure if this change requires a unit test.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Reuse password reset tokens until they expire #29640: Reuse password reset tokens until they expire

[m2-assistant]

Hi @fredden. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[lenaorobei]

Hi @fredden.

Thank you for this contribution, however we've had the discussion with the product owner on security track and it was confirmed that token regeneration is the expected behaviour here. Unfortunately I need to close this PR.
Please do not hesitate to open issue if you will experience any problems with that.

Thank you.

[m2-assistant]

Hi @fredden, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[fredden]

@lenaorobei Where/how can I participate in such discussions? This change introduces a much better user experience with minimal real-world negative security impact. Eg, https://www.youtube.com/watch?v=-gpfKW_8EJw

[m2-assistant]

Hi @fredden. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[fredden]

Update: on 15th April I reached out to @lenaorobei on Magento Community Engineering Slack instance, who helpfully set up an open conversation with "the product owner." That conversation is here, but as yet (23th April) there is no response from the product owner.
https://magentocommeng.slack.com/archives/CBSL1DF8B/p1586979797058900

[fredden]

Update: product owner suggests another approach which has the same benefits achieved here. I will update the code to match this method.

1. User clicks on forgot password - asks for token1 => token 1 is generated and sent over email
2. Internally it should create a mapping of user -> token => active or not , email sent or not
3. User did not receive the email and asks for token2 =>
4. Internally it should check => if there is a valid token already there against that user and if email sent => if yes => just ask the user in the UI to refresh his email , token has already been sent.
5. New token should only be created when a token life time is passed,
6. And yes there should be an upper limit number of requests

[sidolov]

@magento create issue

[engcom-Bravo]

Hello @fredden

Thank you for your contribution

I'm not able to reproduce the original issue on the latest 2.4-develop

I have generated a password reset e-mail, did not use it. Waited for the link to expire

Checked the token in dev console Elements

And the token in the link while redirect page is loading

Then, I have created another Reset password e-mail for the same user (also tried with a different user), the token is different.

[fredden]

@engcom-Bravo I believe the problem here is that you've waited for the link to expire. Expired links should not be restored, but links that have not yet expired should have their validity extended.

[ihor-sviziev]

Hi @engcom-Bravo,
Could you check the reply from the PR author?

[ihor-sviziev]

Moving back to "ready for testing" as we got an answer from the author of this PR.

[ihor-sviziev]

@magento run Functional Tests B2B, Functional Tests EE

[ihor-sviziev]

@magento run Functional Tests B2B, Functional Tests EE

[ihor-sviziev]

@magento run Functional Tests B2B, Functional Tests EE

[gabrieldagama]

Hi @fredden, thanks for contributing! We have discussed this issue at public triage meeting and EngCom team and contributors that were present agree that we should not reduce security (keeping the link valid) for a better user experience.

Also, we agreed that the way that this feature works is a standard in the market, pretty much all other platforms or services have this practice.

I will be closing this PR for now, but please, feel free to reopen it and we can discuss it further if you like.

Thanks!

[m2-assistant]

Hi @fredden, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.