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

Remove username from password reset / email verification links #7137

Open
3 tasks done
mtrezza opened this issue Jan 21, 2021 · 7 comments · May be fixed by #8488
Open
3 tasks done

Remove username from password reset / email verification links #7137

mtrezza opened this issue Jan 21, 2021 · 7 comments · May be fixed by #8488
Labels
type:feature New feature or improvement of existing feature

Comments

@mtrezza
Copy link
Member

mtrezza commented Jan 21, 2021

New Feature / Enhancement Checklist

Current Limitation

The link sent in emails to reset the password / verify the email address currently contains the username which

  • allows to fully compromise an account if the link is shared
  • may expose personally identifiable information in the logs

Feature / Enhancement Description

Link should not contain email address of user.

The link already contains the perishable token. The token is enough to identify the user whose password should be reset. The user controller should accept the password reset with just the token. It should not make it less secure, as the link already contains both (username and token) and once the link is exposed as it currently is, the password can be reset.

⚠️ It can be argued that the username should never be included in a password reset email together with the token, because it allows to fully compromise an account if the link is shared. If instead only the token is included, and the password reset page does not show the username, even if the link is shared, a malicious user could reset the password, but would not know for which account. So the account cannot be compromised, it would only lock out the real user from their account. Which can be self-resolved by requesting a password reset.

Example Use Case

  1. Sign-up user with email and password where username is the email address
  2. Request password reset for user
  3. Link contains email address of user

Alternatives / Workarounds

  • Hash the email address in the link and compare it to the username as a second measure of security. However, as the link already contains token and email address, this would be stricter than the current security measure and there seems to be no basis for tightening this.

3rd Party References

none

@mtrezza mtrezza changed the title Remove username from password reset link Remove username from password reset / email verification links Jan 21, 2021
@mtrezza
Copy link
Member Author

mtrezza commented Jan 21, 2021

@davimacedo, @dplewis Do you have any security concerns about this?

@davimacedo
Copy link
Member

@mtrezza It makes sense to me. I believe we should remove the username from the link.

@mtrezza
Copy link
Member Author

mtrezza commented Jan 23, 2021

If we remove the username from the link for email verification, the following process does not work anymore:

  1. User signs up
  2. Server sends email verification link
  3. User clicks on link with invalid token (e.g. token expired)
  4. Server responds with redirect to invalid_verification_link.html?username=... which lets the user request a new link with the click on a button because the username is passed in the URL
  5. User clicks on link with valid token and can verify email address

We could either
a) automatically send a new email when the user tries to verify the email but the token is expired and then redirect to a page token expired, we've sent a new link, but what is the token expiration then good for?
b) remove the "re-request link" feature; the developer would have to provide a resend verification email feature themselves, which means calling sendVerificationEmail(user) in the UserController; we may have to implement a dedicated endpoint for that. I am not sure what the purpose of the token expiration for verifying an email address is.
c) Only change the password reset route, but leave the username in the email verification route --> maybe not a good idea because the username would still appear in logs
d) Remove the expiration of the email verification token.
e) ?

This challenge only exists for email verification, not for password reset.

@mtrezza mtrezza mentioned this issue Jan 23, 2021
11 tasks
@davimacedo
Copy link
Member

I'd go with letter a)

@mtrezza
Copy link
Member Author

mtrezza commented Jan 25, 2021

Okay, I don't understand why the token has to have an expiration date? If we remove that, then we could simply show an invalid link page, it would be the same process as for password reset.

@mtrezza
Copy link
Member Author

mtrezza commented Feb 16, 2021

It seems the fact that the password reset / email verification token have an expiration date is simply a matter of security policy. It does not seem to have any other practical significance.

If we want to preserve this choice, and unless we want to make it all configurable:

  • the above options a) and d) are not viable
  • option c) would be half-baked because it does not solve the issue for both routes
  • option b) would be consistent with the password reset option, it is also a common design pattern, see for example the npm banner:
    image

The issue with option b) is that this is a breaking change and developers would have to implement a new UI to re-request the email verification link, just like it already works for the password reset link.

Since we want to avoid sudden breaking changes and follow a more phased deprecation policy, we could add a temporary option that enables a), which is not a breaking change because the webpage for "we have sent you a new verification link" already exists in the current user flow. Plus it actually saves the user the click on the re-request button compared to the current flow.

@davimacedo
Copy link
Member

It sounds like a plan.

@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
@dblythy dblythy linked a pull request Mar 30, 2023 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants