Add Password Reset Page#2420
Conversation
93f9a2a to
8f90cb0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds password reset functionality to Fishtest using the Resend email service. Users can now request a password reset via email, receive a secure token-based reset link, and set a new password through a dedicated interface.
Key Changes:
- Implements forgot password and reset password workflows with email-based token authentication
- Adds EmailSender class to handle email delivery via Resend API
- Includes proper token expiration (1 hour) and one-time use validation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| server/fishtest/views.py | Adds forgot_password and reset_password view handlers with token generation, email sending, and password validation logic |
| server/fishtest/emailer.py | New module implementing EmailSender class for sending emails via Resend API |
| server/fishtest/templates/forgot_password.mak | New template for the password reset request page where users enter their email |
| server/fishtest/templates/reset_password.mak | New template for the password reset form where users enter their new password |
| server/fishtest/templates/login.mak | Adds a "Reset password" button linking to the forgot password page |
| server/fishtest/routes.py | Adds routes for /forgot_password and /reset_password/{token} |
| server/fishtest/schemas.py | Updates user schema to include optional password_reset field with token and expiration |
| server/fishtest/init.py | Initializes EmailSender instance and makes it available to request handlers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I wonder if the mailer module by Pyramid can avoid writing some boilerplate. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def update_password_with_reset_token(self, user_id, token, new_password): | ||
| result = self.users.update_one( | ||
| {"_id": user_id, "password_reset.token": token}, | ||
| {"$set": {"password": new_password}, "$unset": {"password_reset": ""}}, | ||
| ) | ||
| if result.modified_count: | ||
| self.clear_cache() | ||
| return result |
There was a problem hiding this comment.
The update_password_with_reset_token method performs an atomic update that checks the token still exists, sets the new password, and removes the password_reset field in a single operation. However, there's a potential race condition if multiple reset password requests are made: if a user requests a password reset twice, the second request will overwrite the first token. When the first token is used successfully, it will invalidate both tokens. This is the expected behavior, but it should be documented or the error message at line 220 could be more specific about this scenario.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| user = request.userdb.users.find_one( | ||
| {"password_reset.token": token, "password_reset.expires_at": {"$gte": now}} | ||
| ) |
There was a problem hiding this comment.
The query uses find_one to look up a user by password_reset.token without any index on this field. If the user collection grows large, this could become a performance bottleneck. Consider adding a database index on password_reset.token to improve query performance, especially since password reset operations are time-sensitive from a user experience perspective.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not email_is_valid: | ||
| request.session.flash("Error! Invalid email: " + validated_email, "error") | ||
| return {} | ||
|
|
||
| user = request.userdb.find_by_email(validated_email) | ||
| if user is not None: | ||
| token = secrets.token_urlsafe(32) | ||
| expires_at = datetime.now(UTC) + timedelta( | ||
| hours=PASSWORD_RESET_EXPIRY_HOURS | ||
| ) | ||
| request.userdb.set_password_reset(user, token, expires_at) | ||
| reset_url = request.route_url("reset_password", token=token) | ||
| body = ( | ||
| "We received a request to reset your Fishtest password.\n\n" | ||
| f"Reset link: {reset_url}\n\n" | ||
| "This link will expire in 1 hour.\n" | ||
| "For your security, do not share this link with anyone, as it can " | ||
| "be used to change your password.\n\n" | ||
| "If you did not request a password reset, you can ignore this " | ||
| "email or contact the site administrators for assistance." | ||
| ) | ||
| try: | ||
| request.email_sender.send( | ||
| user["email"], | ||
| "Fishtest password reset", | ||
| body, | ||
| ) | ||
| except Exception as e: | ||
| print(f"failed to send password reset email to {validated_email}: {e}") | ||
|
|
||
| request.session.flash( | ||
| "If that email exists, a reset link has been sent.", "info" | ||
| ) |
There was a problem hiding this comment.
The forgot_password flow has a timing vulnerability that could allow attackers to enumerate valid email addresses. When an invalid email format is provided (line 196), the function returns immediately with an error. However, when a valid email format is provided (whether the user exists or not), the function proceeds to check the database (line 200) and potentially send an email (lines 217-222) before returning the same generic message.
This creates a measurable timing difference that could be exploited:
- Invalid email format: immediate return (~milliseconds)
- Valid email format with no user: database query (~tens of milliseconds)
- Valid email format with user: database query + email sending (~hundreds of milliseconds)
To prevent email enumeration through timing analysis, consider one of these approaches:
- Always perform the same operations regardless of whether the email exists (e.g., always generate a token and attempt to send an email to a dummy address, or add artificial delays)
- Move the email validation check to occur after the user lookup, so both invalid emails and non-existent users follow the same path
- Add a constant-time delay before returning in all cases to normalize response times
The most straightforward fix would be to restructure the code so that invalid email addresses also go through the database lookup path before returning, making timing analysis ineffective.
| if not email_is_valid: | |
| request.session.flash("Error! Invalid email: " + validated_email, "error") | |
| return {} | |
| user = request.userdb.find_by_email(validated_email) | |
| if user is not None: | |
| token = secrets.token_urlsafe(32) | |
| expires_at = datetime.now(UTC) + timedelta( | |
| hours=PASSWORD_RESET_EXPIRY_HOURS | |
| ) | |
| request.userdb.set_password_reset(user, token, expires_at) | |
| reset_url = request.route_url("reset_password", token=token) | |
| body = ( | |
| "We received a request to reset your Fishtest password.\n\n" | |
| f"Reset link: {reset_url}\n\n" | |
| "This link will expire in 1 hour.\n" | |
| "For your security, do not share this link with anyone, as it can " | |
| "be used to change your password.\n\n" | |
| "If you did not request a password reset, you can ignore this " | |
| "email or contact the site administrators for assistance." | |
| ) | |
| try: | |
| request.email_sender.send( | |
| user["email"], | |
| "Fishtest password reset", | |
| body, | |
| ) | |
| except Exception as e: | |
| print(f"failed to send password reset email to {validated_email}: {e}") | |
| request.session.flash( | |
| "If that email exists, a reset link has been sent.", "info" | |
| ) | |
| # Perform user lookup only for syntactically valid emails to avoid | |
| # leaking timing information about email existence while still | |
| # treating all syntactically invalid and valid emails similarly in | |
| # terms of work performed. | |
| user = None | |
| if email_is_valid: | |
| user = request.userdb.find_by_email(validated_email) | |
| # Generate a token and expiry regardless of whether a matching user | |
| # exists, but only store it when a user is found. | |
| token = secrets.token_urlsafe(32) | |
| expires_at = datetime.now(UTC) + timedelta( | |
| hours=PASSWORD_RESET_EXPIRY_HOURS | |
| ) | |
| body = ( | |
| "We received a request to reset your Fishtest password.\n\n" | |
| ) | |
| if user is not None: | |
| request.userdb.set_password_reset(user, token, expires_at) | |
| reset_url = request.route_url("reset_password", token=token) | |
| body += ( | |
| f"Reset link: {reset_url}\n\n" | |
| "This link will expire in 1 hour.\n" | |
| "For your security, do not share this link with anyone, as it can " | |
| "be used to change your password.\n\n" | |
| ) | |
| body += ( | |
| "If you did not request a password reset, you can ignore this " | |
| "email or contact the site administrators for assistance." | |
| ) | |
| # Always invoke the email-sending path to reduce timing differences. | |
| # For invalid emails or nonexistent users, send to a dummy address. | |
| recipient = ( | |
| user["email"] | |
| if email_is_valid and user is not None | |
| else "no-reply@example.invalid" | |
| ) | |
| try: | |
| request.email_sender.send( | |
| recipient, | |
| "Fishtest password reset", | |
| body, | |
| ) | |
| except Exception as e: | |
| # Log any failure, but do not reveal details to the client. | |
| print(f"failed to send password reset email for address '{email}': {e}") | |
| # Preserve original user-facing messages while normalizing the timing | |
| # of the underlying operations. | |
| if not email_is_valid: | |
| request.session.flash("Error! Invalid email: " + validated_email, "error") | |
| else: | |
| request.session.flash( | |
| "If that email exists, a reset link has been sent.", "info" | |
| ) |
|
okay.. made it so that once you open the link from the email it is immediately invalidated and cannot be used a second time, also since we don't have a background queue for sending emails this will inherently cause a "timing" attack if someone were dedicated enough to brute force through the captcha.. this was mainly coded up for the pw hashing since Pasquale mentioned "the code to deal with a forgotten pwd" |
|
@Disservin, @ppigazzini: any chance of this being merged soon? |
fishtest has changed a lot in the last 3 months, replacing pyramid/mako with fastapi/jinja2. If you need a password reset, please contact me on Discord with a DM. |
|
OK, thanks! |
Tested with
https://resend.com/homewhich offers a generous free term of 3000 emails per month (100 per day) maybe @ppigazzini prefers a different provider though, but if I find resend quite nice.Sends an email with a reset token and invalidates the token after use.
Needs
env vars
FISHTEST_RESEND_FROM_EMAIL, is for example fishtest@resend.dev
FISHTEST_RESEND_FROM_NAME, just Fishtest
reset page, from email
reset email (can be improved)