Implement email verification and password recovery with Mailgun#62
Merged
A1L13N merged 7 commits intoJun 3, 2026
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds end-to-end email verification and password recovery: DB schema and migrations, secure token generation and hashed storage, Mailgun email helpers, verification/resend/forgot/reset API handlers and routes, registration/login changes to require verification, and three new frontend pages with client flows. ChangesEmail Verification & Password Reset Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/login.html`:
- Around line 440-456: The current showRegisterSuccess function injects the
whole HTML via panel.innerHTML with the user-facing msg interpolated, which
risks XSS if msg source changes; instead build the static structure and assign
the dynamic message using textContent (or create the message <p> via
document.createElement and set its textContent) so the element with id or class
for the message receives msg as text only; keep the same container (panel =
document.getElementById('panel-register')) and preserve the sign-in button
onclick="switchTab('login')" behavior while ensuring only static markup is added
via innerHTML or createElement and the msg is set via textContent.
In `@schema.sql`:
- Around line 123-125: Make the schema change idempotent: either remove the raw
ALTER TABLE line from schema.sql and rely on the init_db function in worker.py
to perform the migration, or update the SQL to avoid failing when re-run (e.g.,
use a conditional ADD COLUMN form supported by your SQL engine such as "ADD
COLUMN IF NOT EXISTS email_verified ..." or wrap the change in a check against
pragma/table_info) so that executing schema.sql via wrangler d1 execute won't
error; reference the ALTER TABLE users ADD COLUMN email_verified statement in
schema.sql and the init_db function in worker.py when making the change.
In `@src/worker.py`:
- Around line 988-990: The code uses datetime.datetime.utcnow() to build
expires_at (and the similar occurrence around lines 1140-1142); replace utcnow()
with a timezone-aware call using datetime.datetime.now(datetime.timezone.utc) so
the resulting timestamp is timezone-aware before applying timedelta and
strftime. Update the expressions that set expires_at (and the other similar
variable) to call datetime.datetime.now(datetime.timezone.utc) +
datetime.timedelta(...) and then format as before, ensuring you import
datetime.timezone where needed and adjust any tests or downstream code that
expect naive datetimes.
- Line 564: The send_password_reset_email function declares a username parameter
that is never used; to silence linters and make intent clear, rename the
parameter to _username (or prefix it with an underscore) in the async def
signature and update any internal references if present, leaving the rest of
send_password_reset_email unchanged.
- Around line 991-999: The token insert failure currently only logs the error
(capture_exception in api_register.insert_verification_token) leaving a created
user without a token; update the except block to either remove the
partially-created user or mark them so a resend flow can be used: specifically,
in the except for env.DB.prepare(...).bind(...).run() call, run a compensating
delete (e.g., DELETE FROM users WHERE id = uid using
env.DB.prepare(...).bind(uid).run()) and then re-raise or return an error to the
caller, and separately implement a "resend verification" endpoint that uses
send_verification_email to issue a new token when requested (lookup by email or
user id and create+store a new token); ensure capture_exception still logs the
original error and that the new delete/mark and resend logic reference uid,
send_verification_email, and the token insertion path.
- Around line 727-736: The migration block that runs env.DB.prepare(...) to
ALTER TABLE users and UPDATE users swallows all errors with a bare except, so
add logging around it: wrap the existing try around env.DB.prepare(... "ALTER
TABLE users ADD COLUMN email_verified INTEGER NOT NULL DEFAULT 0") and the
subsequent UPDATE in the same try but on success call your logger (e.g.,
processLogger.info or the module's logger) to record that the email_verified
column was added or already existed and the UPDATE ran; in the except block
catch Exception as e and log a warning/error including the exception object and
a short message mentioning "email_verified" and the migration to aid
observability while preserving idempotency.
- Around line 549-551: The HTML email template is interpolating the
user-controlled variable username directly into the message; update the code
that builds the email body (the lines concatenating "<p>You&`#39`;re receiving
this email..." with {username}) to escape user input before interpolation (use
html.escape on username) so any markup or scripts are rendered inert; locate the
template construction in src/worker.py where username is used and replace direct
insertion with an escaped_username variable (or call html.escape inline) when
composing the HTML email.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75536195-157b-49b7-998d-86a63520c097
📒 Files selected for processing (6)
public/forgot-password.htmlpublic/login.htmlpublic/reset-password.htmlpublic/verify-email.htmlschema.sqlsrc/worker.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
public/login.html (1)
201-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMatch the client password policy to the API's 8-character minimum.
The form still advertises and validates 6 characters, but
api_registernow rejects anything under 8. Right now users can submit a password the UI says is valid and only fail after the request comes back.Also applies to: 402-404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/login.html` around lines 201 - 203, The password input with id "r-password" currently uses minlength="6" and placeholder "min 6 characters" which conflicts with the API's 8-character minimum; update the client-side validation to require at least 8 characters by changing minlength to "8", update the placeholder text to "min 8 characters", and make the same change for the related registration password fields referenced at lines 402-404 (any other inputs with id or name indicating registration password) so the UI validation matches api_register's 8-char policy.src/worker.py (3)
1091-1097:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConsume tokens atomically with the account mutation.
Both flows mutate the user record and delete the token in separate statements. If the second statement fails, the token remains usable even though the account change already happened. That breaks the single-use guarantee, and on the password-reset path it means the same reset link can be replayed until expiry.
Also applies to: 1276-1282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/worker.py` around lines 1091 - 1097, Wrap the user mutation and token deletion in a single atomic transaction so both succeed or both roll back: instead of calling env.DB.prepare(...) twice, begin a transaction (via your DB client's transaction/connection API), execute the UPDATE "UPDATE users SET email_verified=1 WHERE id=?" bound to row.user_id and the DELETE "DELETE FROM email_verification_tokens WHERE id=?" bound to row.id inside that transaction, then commit; on error rollback and surface the error. Apply the same transactional fix to the analogous code around the password-reset path (the block referenced at 1276-1282).
1029-1048:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the login row contract in sync with the new verification field.
CI is currently red here: the new
row.email_verifiedaccess raises on the existingMockRowfixtures before login completes. Please update the login mocks/fixtures alongside this change, or normalize the field access intentionally, so the verification gate doesn't break the test path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/worker.py` around lines 1029 - 1048, The login code now expects row.email_verified which breaks existing MockRow fixtures; update either the fixtures or the login access to be resilient. Fix options: (A) add email_verified to your test/mock objects (MockRow) with appropriate boolean values so the login path using row.email_verified works; or (B) make the login check tolerant by replacing direct access to row.email_verified with a safe lookup (e.g., getattr(row, "email_verified", False) or row.get("email_verified", False)) inside the login function where row is used, so tests that use MockRow won’t raise. Ensure you update references in the login flow (the code block that binds u_hash, sets password_hash/user_id/etc., and performs the verification gate) and any MockRow/test fixtures that construct returned rows.
1008-1013:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail registration if the verification email was not actually queued.
send_verification_email()returns a boolean, but this path always reports success. If Mailgun is misconfigured or temporarily failing, the user is created as unverified and immediately locked out with no guarantee they ever received the link the UI tells them to check.Suggested fix
- await send_verification_email(email, username, v_token, env) + sent = await send_verification_email(email, username, v_token, env) + if not sent: + try: + await env.DB.prepare("DELETE FROM users WHERE id=?").bind(uid).run() + except Exception as rollback_exc: + await capture_exception( + rollback_exc, req, env, "api_register.rollback_user_after_email_failure" + ) + return err("Registration failed — please try again", 503)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/worker.py` around lines 1008 - 1013, The registration flow calls send_verification_email(email, username, v_token, env) but ignores its boolean result and always returns ok(...); update the registration handler to check the return value of send_verification_email and treat False as a failure: roll back the created user (or mark the user for deletion/unverified cleanup), log the failure with context, and return an error response instead of the success ok message so the user is not told to check email when the message wasn't queued; reference send_verification_email and the registration return ok(...) path to locate where to add the check and rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/verify-email.html`:
- Around line 80-83: The resend email input (id="resend-email" inside form
id="resend-form") lacks an accessible name; add a proper <label
for="resend-email"> (can be visually hidden with a utility class) or add an
explicit aria-label="Email address for resend" on the input so screen readers
can identify it; ensure the label’s for value exactly matches the input id and
that no placeholder-only approach remains.
- Around line 164-170: The fetch handler for the resend flow unconditionally
shows a success message; update the logic around the fetch response (the res,
data, msgEl variables and the POST to `${API}/api/resend-verification`) to check
res.ok first and, on non-2xx, set msgEl.textContent to data.error || a generic
failure string instead of the success text; on res.ok keep the existing success
message (data.message || 'Check your inbox for a new link.'). Ensure you still
await res.json() to read the error message and handle network/parse failures
with a catch that displays a generic error.
In `@src/worker.py`:
- Around line 1105-1165: Add a short per-account and per-IP cooldown check
before generating tokens and calling send_verification_email in
api_resend_verification (and similarly in api_forgot_password): query the
email_verification_tokens table (or a new rate_limit table) for a recent token
for the same user_id or request IP, compare its created_at/expires_at to a
configured cooldown (e.g., 1–15 minutes), and if the cooldown is still active
return ok(None, _GENERIC_OK) without creating a new token or calling Mailgun;
log/capture rate-limit hits as needed and ensure the check happens before the
DELETE/INSERT and send_verification_email call so no email is sent while
throttled.
---
Outside diff comments:
In `@public/login.html`:
- Around line 201-203: The password input with id "r-password" currently uses
minlength="6" and placeholder "min 6 characters" which conflicts with the API's
8-character minimum; update the client-side validation to require at least 8
characters by changing minlength to "8", update the placeholder text to "min 8
characters", and make the same change for the related registration password
fields referenced at lines 402-404 (any other inputs with id or name indicating
registration password) so the UI validation matches api_register's 8-char
policy.
In `@src/worker.py`:
- Around line 1091-1097: Wrap the user mutation and token deletion in a single
atomic transaction so both succeed or both roll back: instead of calling
env.DB.prepare(...) twice, begin a transaction (via your DB client's
transaction/connection API), execute the UPDATE "UPDATE users SET
email_verified=1 WHERE id=?" bound to row.user_id and the DELETE "DELETE FROM
email_verification_tokens WHERE id=?" bound to row.id inside that transaction,
then commit; on error rollback and surface the error. Apply the same
transactional fix to the analogous code around the password-reset path (the
block referenced at 1276-1282).
- Around line 1029-1048: The login code now expects row.email_verified which
breaks existing MockRow fixtures; update either the fixtures or the login access
to be resilient. Fix options: (A) add email_verified to your test/mock objects
(MockRow) with appropriate boolean values so the login path using
row.email_verified works; or (B) make the login check tolerant by replacing
direct access to row.email_verified with a safe lookup (e.g., getattr(row,
"email_verified", False) or row.get("email_verified", False)) inside the login
function where row is used, so tests that use MockRow won’t raise. Ensure you
update references in the login flow (the code block that binds u_hash, sets
password_hash/user_id/etc., and performs the verification gate) and any
MockRow/test fixtures that construct returned rows.
- Around line 1008-1013: The registration flow calls
send_verification_email(email, username, v_token, env) but ignores its boolean
result and always returns ok(...); update the registration handler to check the
return value of send_verification_email and treat False as a failure: roll back
the created user (or mark the user for deletion/unverified cleanup), log the
failure with context, and return an error response instead of the success ok
message so the user is not told to check email when the message wasn't queued;
reference send_verification_email and the registration return ok(...) path to
locate where to add the check and rollback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 695392d7-c430-421e-8b83-0e0135e366d0
📒 Files selected for processing (4)
public/login.htmlpublic/verify-email.htmlschema.sqlsrc/worker.py
💤 Files with no reviewable changes (1)
- schema.sql
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_api_auth.py (2)
77-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThese tests no longer verify the behavior in their names.
After the response contract changed, both cases now only assert
"success", so neither one proves thatnamedefaults tousernameor that a custom name is preserved. Please assert the stored INSERT-bound value instead so these tests stay meaningful. As per coding guidelines,**/tests/**: "Ensure tests are descriptive, cover edge cases, and follow the project's existing test patterns."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_api_auth.py` around lines 77 - 93, The tests test_name_defaults_to_username and test_custom_name_preserved only assert data["success"] now; update them to inspect the MockDB INSERT parameters captured by MockDB/make_stmt after calling worker.api_register (via env) and assert the stored name value: for test_name_defaults_to_username assert the INSERT-bound "name" equals the provided "username" ("bob"), and for test_custom_name_preserved assert the INSERT-bound "name" equals the provided "name" ("Robert"); use whatever property or method MockDB/make_stmt exposes to read the last statement parameters instead of relying solely on _parse(r).
141-150: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd the unverified-login test alongside this fixture.
The fixture now hardcodes
email_verified=1, but there is no companion case asserting the new 403/message when it is0. That verification gate is the core behavior change in this PR, so it should be locked down explicitly. As per coding guidelines,**/*.py: "Verify tests cover the key logic paths."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_api_auth.py` around lines 141 - 150, Add a new unit test next to the existing auth tests that uses the _make_user_row fixture but with email_verified=0 (call _make_user_row(..., email_verified=0)) to simulate an unverified account, perform the same login request flow used by the other tests, and assert the response status is 403 and the response body contains the expected "email not verified" message; implement it as a separate test function (e.g., test_unverified_login_returns_403) so the unverified branch introduced by the PR is explicitly covered.src/worker.py (1)
1298-1309:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPassword reset does not invalidate existing auth tokens.
This flow updates
users.password_hash, but the bearer tokens created bycreate_token()and accepted byverify_token()in this file are fully stateless and carry no expiry, version, or password-change check. Any previously issued token therefore remains valid after a reset, which defeats the account-recovery security goal. Please add a server-checked revocation mechanism such as a token/session version orpassword_changed_atclaim. As per coding guidelines,**/*.py: "Check for security vulnerabilities."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/worker.py` around lines 1298 - 1309, The current password-reset flow updates users.password_hash but leaves stateless tokens valid; to fix, add a server-side revocation field (e.g., users.password_changed_at TIMESTAMP or users.session_version INT) and wire it through token creation/verification: when issuing tokens in create_token() embed the current users.password_changed_at or session_version into the token payload, update the chosen field when resetting the password in the password-reset block that currently calls env.DB.prepare("UPDATE users SET password_hash=? WHERE id=?"), and modify verify_token() to fetch the current users.password_changed_at/session_version for the token's user_id and reject the token if it does not match the value embedded in the token; ensure the password-reset handler (the try block shown) updates the revocation field atomically with password_hash so existing tokens are invalidated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/worker.py`:
- Around line 1139-1148: The cooldown lookup currently swallows exceptions and
proceeds, which disables the abuse guard; change the except block around the
env.DB.prepare(...) call that queries email_verification_tokens (the `recent`
lookup) so that on any Exception you log the error and immediately return
ok(None, _GENERIC_OK) instead of `pass`; update the identical pattern in the
other occurrence (lines ~1212-1221) so both per-account cooldown checks
fail-closed by returning via ok rather than continuing.
---
Outside diff comments:
In `@src/worker.py`:
- Around line 1298-1309: The current password-reset flow updates
users.password_hash but leaves stateless tokens valid; to fix, add a server-side
revocation field (e.g., users.password_changed_at TIMESTAMP or
users.session_version INT) and wire it through token creation/verification: when
issuing tokens in create_token() embed the current users.password_changed_at or
session_version into the token payload, update the chosen field when resetting
the password in the password-reset block that currently calls
env.DB.prepare("UPDATE users SET password_hash=? WHERE id=?"), and modify
verify_token() to fetch the current users.password_changed_at/session_version
for the token's user_id and reject the token if it does not match the value
embedded in the token; ensure the password-reset handler (the try block shown)
updates the revocation field atomically with password_hash so existing tokens
are invalidated.
In `@tests/test_api_auth.py`:
- Around line 77-93: The tests test_name_defaults_to_username and
test_custom_name_preserved only assert data["success"] now; update them to
inspect the MockDB INSERT parameters captured by MockDB/make_stmt after calling
worker.api_register (via env) and assert the stored name value: for
test_name_defaults_to_username assert the INSERT-bound "name" equals the
provided "username" ("bob"), and for test_custom_name_preserved assert the
INSERT-bound "name" equals the provided "name" ("Robert"); use whatever property
or method MockDB/make_stmt exposes to read the last statement parameters instead
of relying solely on _parse(r).
- Around line 141-150: Add a new unit test next to the existing auth tests that
uses the _make_user_row fixture but with email_verified=0 (call
_make_user_row(..., email_verified=0)) to simulate an unverified account,
perform the same login request flow used by the other tests, and assert the
response status is 403 and the response body contains the expected "email not
verified" message; implement it as a separate test function (e.g.,
test_unverified_login_returns_403) so the unverified branch introduced by the PR
is explicitly covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d96df30e-6433-46bb-bb41-ef1077d46345
📒 Files selected for processing (5)
public/verify-email.htmlsrc/worker.pytests/test_api_activities.pytests/test_api_admin.pytests/test_api_auth.py
A1L13N
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Authentication
Email Integration
Replaced Resend integration with Mailgun
Added Mailgun email service helper
Added transactional email templates for:
Added environment variable support for:
Security
Added cryptographically secure token generation using
os.urandom(32)Added SHA-256 token hashing before database storage
Verification and reset tokens are never stored in plaintext
Added token expiration support:
Added single-use token consumption
Continued use of AES-256-GCM encrypted user data
Preserved existing PBKDF2 password hashing implementation
Database
Added new tables:
email_verification_tokenspassword_reset_tokensAdded:
User Experience
Testing
Tested the following scenarios:
Email Verification and Password Recovery Implementation with Mailgun
This PR adds mandatory email verification for new accounts, a complete password-recovery flow, and replaces the Resend integration with Mailgun.
Purpose
Require users to verify their email addresses and provide secure, single-use password reset functionality delivered via Mailgun transactional emails.
Key Changes
Frontend
Backend (src/worker.py)
Database (schema.sql)
Security
User Experience / Impact
Testing