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: Remove username from verification emails #8488

Draft
wants to merge 7 commits into
base: alpha
Choose a base branch
from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Mar 30, 2023

Pull Request

Issue

Currently, Parse Server exposes username via verification email urls. All that should be needed to perform a reset request is a valid token

Closes: #7137

Approach

Tasks

  • Add tests

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title fix: remove username from verification emails fix: Remove username from verification emails Mar 30, 2023
@parse-github-assistant
Copy link

Thanks for opening this pull request!

@dblythy
Copy link
Member Author

dblythy commented Mar 30, 2023

How should this handle expired tokens? With the old implementation the invalid verification link page could re-generate an email link.

I'm also wondering if removing the username parameter opens up to brute forcing, as previously to change a token you would need a valid username and token combination, whereas now you could brute for the endpoint with different tokens until one matches.

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (6a4a00c) 94.37% compared to head (7ba6521) 94.33%.

❗ Current head 7ba6521 differs from pull request most recent head b960a1a. Consider uploading reports for the commit b960a1a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8488      +/-   ##
==========================================
- Coverage   94.37%   94.33%   -0.04%     
==========================================
  Files         185      183       -2     
  Lines       14761    14501     -260     
==========================================
- Hits        13930    13680     -250     
+ Misses        831      821      -10     
Files Changed Coverage Δ
src/Controllers/UserController.js 96.06% <100.00%> (+0.14%) ⬆️
src/GraphQL/loaders/usersMutations.js 93.39% <100.00%> (+0.80%) ⬆️
src/Routers/PagesRouter.js 97.66% <100.00%> (-0.06%) ⬇️
src/Routers/PublicAPIRouter.js 93.16% <100.00%> (-0.23%) ⬇️

... and 36 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtrezza
Copy link
Member

mtrezza commented Jun 17, 2023

How should this handle expired tokens? With the old implementation the invalid verification link page could re-generate an email link.

What is the purpose of token expiration for email verification? If an expired token leads to a website where one can request a new token (sent via verification email) without login, the expiration seems useless. Is there any scenario in which expiration makes sense? Maybe the existing tests related to token expiration give clues about the intentions of expiration?

I'm also wondering if removing the username parameter opens up to brute forcing, as previously to change a token you would need a valid username and token combination, whereas now you could brute for the endpoint with different tokens until one matches.

The difference of brute forcing two fields (email + token) vs one field (token) is just the amount of possible combinations. If we make the token a longer string, the difficulty should be the same.

What happens in the following scenario:

  1. User signs-up with email1
  2. Verification is sent to email1
  3. User changes email from email1 to email2
  4. Another verification email is set to email2
  5. Account of email1 is compromised
  6. Attacker open email1 verification link
  • Will the token for email1 be invalid?
  • Will that validate email2 with the token from email1?
  • Will that have any effect (changes) on the user object?

@dblythy
Copy link
Member Author

dblythy commented Jul 27, 2023

This PR:

  • Removes email from verification links
  • Makes it possible to regenerate links with a valid expired token (as previously this flow could be invoked with the username)

This all feels like pretty breaking changes to me, which I think we would have to phase in @mtrezza thoughts?

@mtrezza
Copy link
Member

mtrezza commented Jul 28, 2023

Did you find out any hinds regarding:

What is the purpose of token expiration for email verification? If an expired token leads to a website where one can request a new token (sent via verification email) without login, the expiration seems useless. Is there any scenario in which expiration makes sense? Maybe the existing tests related to token expiration give clues about the intentions of expiration?

Regarding breaking change:

  • I assume it would be breaking if someone has a verification email with an "old style" link and tried to open it after the developer upgraded Parse Server using the "new style" link. How should that be handled?
  • Are there any other breaking changes?

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.

Remove username from password reset / email verification links
2 participants