Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug regarding token deletions upon successful password resets #5724

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

roberto-aguilar
Copy link

@roberto-aguilar roberto-aguilar commented Feb 14, 2024

In the default authentication system created by the mix phx.gen.auth, when a password has been successfully reset, it deletes all of the tokens regardless of their context, however, this is problematic in the following scenario:

  • A user has been registered, which creates a token with the confirm context and account confirmation instructions delivered via email.
  • The user has not clicked on the confirmation email message yet.
  • The user requests password reset instructions and gets them via email.
  • The user successfully follows the password reset instructions.
  • The user tries to click on the confirmation email, but it is no longer valid.

By scoping the deletion to only reset_password tokens, the bug is gone and the confirm token will still be valid regardless of the abovementioned process.

In the default authentication system created by the `mix phx.gen.auth`,
when a password has been successfully reset, it deletes all of the
tokens regardless of their context, however, this is problematic in the
following scenario:

- A user has been registered, which creates a token with the `confirm`
  context and account confirmation instructions delivered via email.
- The user has not clicked on the confirmation email message yet.
- The user requests password reset instructions and gets them via email.
- The user successfully follows the password reset instructions.
- The user tries to click on the confirmation email, but it is no longer
  valid.

By scoping the deletion to only `reset_password` tokens, the bug is gone
and the confirm token will still be valid regardless of the abovementioned
process.
@josevalim
Copy link
Member

Resetting the password should typically invalidate all other sessions, requests to change emails, etc. It doesn't need to reset the confirmation token indeed, but almost everything else is a must.

@ShPakvel
Copy link
Contributor

Resetting the password should typically invalidate all other sessions, requests to change emails, etc. It doesn't need to reset the confirmation token indeed, but almost everything else is a must.

@josevalim, this sounds reasonable for me as well. I've added necessary changes to keep confirmation token on this case. There is also other case (email change) that I think should be improved from security point of view. Some corrections. And optional idea described to consider as well. Could you please review it? Thank you in advance. 🙇🏼 #5951

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants