search

phoenixframework / phoenix

Peace of mind from prototype to production
https://www.phoenixframework.org
MIT License
21.45k stars 2.88k forks source link

Keep email confirmation token on password update. #5951

Open ShPakvel opened 1 month ago

ShPakvel commented 1 month ago

When user decides to change password, mail can still be waiting confirmation.

Reason why user changes password is not important and does not lead to any security issue related to email confirmation. Logic is the same for update and reset of password.

josevalim commented 1 month ago

Thank you for the PR. Just one tiny correction:

Fix security issue on email update.

I don't believe there is a security issue here because, when we reset and confirm password, we validate that user email matches the token email.

This PR should be exclusively about improving the user experience by not deleting confirmation tokens. All other aspects are protected today.

josevalim commented 1 month ago

To be clear, I believe the PR should only change this line: https://github.com/phoenixframework/phoenix/pull/5951/files#diff-501abec1a66c387cce697c48f3c433e58c4fecd77bcd39765d0f3f982d96c39cL202

And update one of the existing tests. :)

ShPakvel commented 4 weeks ago

@josevalim, thanks! I see I missed protection we have by comparison of user emails in verify_email_token_query. I've reverted this part of code.

Regarding keeping email confirmation token. I think it should be done in both cases "password update" and "password reset". Removing "confirm" email token is inconvenience for user experience in both cases, and I don't see any security issue with keeping token.