fix: Remove username from verification emails

[dblythy]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

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]

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

[parse-github-assistant]

Thanks for opening this pull request!

[dblythy]

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]

Codecov Report

Attention: Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.16%. Comparing base (8f85ae2) to head (c09bc87).
Report is 2 commits behind head on alpha.

Files with missing lines	Patch %	Lines
src/Routers/PagesRouter.js	88.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #8488      +/-   ##
==========================================
- Coverage   93.50%   93.16%   -0.34%     
==========================================
  Files         186      186              
  Lines       14804    14807       +3     
==========================================
- Hits        13842    13795      -47     
- Misses        962     1012      +50     

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

[mtrezza]

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]

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]

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?

[mathieulb]

@mtrezza , if there isn't any code that specifically checks that the username is not in the link, and if tokens still work the same way, passing an old link to a new server will succeed. That is, if all cases of req.params.username have been removed and nothing does something like Object.keys(req.params).includes("username"). Right ?

[mtrezza]

@mathieulb That makes sense. Given that the existing tests have been changed, we'd need to maintain a test legacy test that uses the old link with username query parameter in the URL. Even though the param should be ignored, it would be good to test it.

[dblythy]

What is the purpose of token expiration for email verification?

When you attempt to reset with an expired token, the server will throw. It's up to the client to request a renewal - previously you could do it with the email that is in the query params, but now that will no longer work (there is a new mechanism where you can submit the expired token for renewal)

[mtrezza]

there is a new mechanism where you can submit the expired token for renewal

Is this new mechanism only Parse Server internal or is that a breaking change?

[dblythy]

This is a new mechanism, that expired tokens can be used trigger the resending of verification emails (previously you could use the email)

[mtrezza]

Ok, but is that mechanism only internal or does it imply any changes for developers or users?

[dblythy]

The mechanism is public and is breaking - if users have custom invalid pages that use the email from the query params, they will have to access expiredToken instead

[mtrezza]

Does this have to be breaking? Could it auto-generate a new token, based on the invalid token?

[dblythy]

As far as I can tell, the breaking part is that the invalid link URL currently contains the username, meaning someone could build a page with a button "resend email" that automatically calls Parse.User.requestVerifionEmail with the username from the url parameter. Not sure how likely that is though.

Or, they could have a templated message (e.g {{username}}, this link is invalid.

[mtrezza]

Good point, not having the username as URL param should indeed be considered breaking.

[mtrezza]

I’m a bit concerned about the breaking change. I think it warrants a migration document for PS7 to PS8, because describing the old vs. new process and the required changes for the developer, especially regarding the HTML files, will likely exceed a simply changelog entry. Could you please take the 6.0.0.md file as a template and add a 8.0.0.md file to this PR, and just briefly describing this change?

[parse-github-assistant]

The label block:major cannot be used here.

[mtrezza]

I'm wondering; how long until an expired token is deleted from the DB? Or isn't it deleted at all? If it is deleted, then this logic probably fails and the correct approach would be to present a form with an email field where the user has to enter the email address. Or, just show a website which instructs the user on how to request a new verification email from within the app, if login with unverified email is enabled.