Conversation
|
Claude finished @hamza-56's task in 3m 49s —— View job Review: feat(auth): add email verification on registration
OverviewWell-structured feature that adds email verification gating on login. The architecture is sound: SHA-256 hashed tokens, proper invalidation of prior tokens, Redis-based rate limiting, background email sends, Google OAuth auto-verification, and an existing-user backfill migration. Test coverage is thorough (38 new/updated tests) and the frontend handles all verification states cleanly. Issues Found🔴 High
🟡 Medium
🔵 Low
What looks good
RecommendationAddress #1 (HTML injection) before merging — it's a straightforward fix. Issues #2-4 are worth fixing but could be follow-up if needed. |
React DoctorScore: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Prevents HTML/anchor injection from a user-supplied display name (e.g. registering as '<a href="evil.site">click</a>'). Plain-text body is unaffected. Also returns (token, name) from request_resend so the resend endpoint no longer needs a second User query after commit.
…edis - Lowercase + strip email at registration and on Google OAuth (was case-sensitive, so 'Alice@Example.com' wouldn't match the resend lookup that lowercases). Also closes a duplicate-account door where case-different emails could both register. - Replace double-commit in register_user with flush + single commit so user creation and verification-token creation are atomic — a failure in token creation no longer leaves an orphaned user row. - Drop into redis.asyncio.from_url directly for the resend cooldown instead of reaching into CacheService._redis. CacheService wraps a different API (string get/set with ttl); we just need SET NX EX. - Resend endpoint uses (token, name) returned by request_resend, removing the redundant User query after commit.
|
Claude finished @hamza-56's task in 2m 9s —— View job Re-review: feat(auth): add email verification on registration
Previous Review CommentsAll 7 threads from the initial review have been resolved:
All fixes are correct and well-implemented. The new tests ( New Issue Found
Overall AssessmentThe fix commits cleanly address all critical and medium issues from the initial review. The code is in good shape:
The Redis connection leak (above) is the only remaining issue worth addressing before merge — it's a straightforward fix. |
aioredis.from_url() created a fresh client (and its connection pool) on every /verify-email/resend request, with no aclose() — connections leaked over time. Wrap the constructor in an lru_cache(maxsize=1) singleton so we build one client per process, matching the existing get_cache_service pattern in app/core/cache.py. _get_resend_redis stays as the test-override seam.
|
Claude finished @hamza-56's task in 3m 28s —— View job Re-review: feat(auth): add email verification on registration
Previous Review CommentsAll 8 threads from the prior two reviews have been resolved:
All fixes are correct and well-implemented. The regression tests ( New Issues FoundAll low priority — none are blocking.
What Looks Good
RecommendationReady to merge. All critical and medium issues from prior reviews are addressed. The remaining low-priority items are non-blocking suggestions for follow-up. |
Replaces the lru_cache singleton from 4067fa4 with a build-then-close pattern. The endpoint is rate-limited to ~1 request per email per cooldown window, so per-request pool setup is negligible — and this removes the shared mutable module state. aclose() guarantees cleanup. Adds a no-op aclose() to the FakeRedis test shim to match the real client's lifecycle.
|
Claude finished @hamza-56's task in 3m 26s —— View job Re-review: feat(auth): add email verification on registration
Previous Review CommentsAll 8 threads from prior reviews have been resolved:
All fixes are correct and well-implemented, with regression tests locking the key fixes ( New Issues
What Looks Good
Overall AssessmentThe PR is in excellent shape. All critical and medium issues from prior reviews have been addressed with clean, correct fixes. The remaining observations are all low-priority and non-blocking. The architecture is solid — hashed tokens, atomic transactions, proper rate limiting, background email sends, and thorough test coverage. Ready to merge. The three remaining low-priority suggestions (formataddr, trailing slash, used_at semantics) are all follow-up material. |
|
|
||
|
|
||
| def _build_verify_url(raw_token: str) -> str: | ||
| return f"{settings.FRONTEND_BASE_URL}/verify-email?token={raw_token}" |
There was a problem hiding this comment.
🔵 Low — Trailing slash in FRONTEND_BASE_URL produces double-slash URL
If FRONTEND_BASE_URL is configured as https://app.example.com/, this yields https://app.example.com//verify-email?token=.... Functionally harmless (browsers and most email clients normalize it) but looks unprofessional in the email and could confuse link scanners.
| return f"{settings.FRONTEND_BASE_URL}/verify-email?token={raw_token}" | |
| return f"{settings.FRONTEND_BASE_URL.rstrip('/')}/verify-email?token={raw_token}" |
There was a problem hiding this comment.
Fixed in f74cd0c. _build_verify_url now does settings.FRONTEND_BASE_URL.rstrip("/") so configs ending in / no longer produce //verify-email. Added test_strips_trailing_slash_in_frontend_base_url to lock it.
| ) | ||
| ) | ||
| for prior in result.all(): | ||
| prior.used_at = now |
There was a problem hiding this comment.
🔵 Low — used_at conflates "verified" with "superseded"
Setting used_at = now on prior tokens reuses the same column for two different semantics: "the user actually clicked this token" vs. "this token was invalidated because a newer one was minted." If you ever need to audit which token was actually verified (vs. just invalidated), you can't distinguish the two.
Not a blocking issue — the current behavior is correct. Just noting that if auditing becomes important, a separate invalidated_at column or a status enum would be cleaner.
There was a problem hiding this comment.
Keeping as-is. The reviewer notes this is non-blocking, and the design intent is "any token row not currently active." For audit, the workflow is to find the token whose used_at is closest to the user record's email_verified_at — they are written together inside verify_token. Adding an invalidated_at column or a status enum would be cheap if a future flow needs to distinguish (e.g. analytics on resend churn), but YAGNI today.
| "1", | ||
| ex=settings.EMAIL_VERIFICATION_RESEND_COOLDOWN_SECONDS, | ||
| nx=True, | ||
| ) | ||
| finally: | ||
| await redis.aclose() | ||
| if not accepted: | ||
| raise HTTPException(status_code=429, detail="rate_limited") | ||
|
|
||
| result = await EmailVerificationService.request_resend(session, email=email_lower) | ||
| if result is None: |
There was a problem hiding this comment.
🔵 Low — Rate-limit lock persists if the DB operation fails
If session.commit() on line 330 raises (e.g. transient DB error), the Redis SET NX has already been consumed. The user gets a "rate_limited" response on their next attempt even though no email was sent.
The 60-second default cooldown makes this very low impact — the user just waits a minute. But if the cooldown is raised significantly, consider wrapping in a try/except that deletes the Redis key on DB failure.
There was a problem hiding this comment.
Keeping as-is. With the 60s default cooldown the worst case is "user waits one extra minute," which is below the threshold worth a compensating delete + the failure-mode complexity that introduces (e.g. the delete itself can fail and now we have two paths leaking state). If we ever raise the cooldown to, say, an hour, this would be worth revisiting — flagging in the follow-up that addresses the rate-limit shape mentioned in #289.
Replaces the explicit try/finally + aclose() in the endpoint with `async with _get_resend_redis() as redis: ...`. `_get_resend_redis` is now an @asynccontextmanager that owns the open/close pair, so the caller never has to remember to close, and the test FakeRedis no longer needs an aclose() shim.
|
Claude finished @hamza-56's task in 1m 50s —— View job Re-review: feat(auth): add email verification on registration
Previous Review CommentsAll 8 threads from the initial and second reviews have been resolved:
3 low-priority suggestions from the third review remain unresolved — these are non-blocking:
Latest Commit Review (e2a2c35)The refactor from
No issues with this commit. No New Issues FoundThe code is in good shape across all layers. No new critical, high, or medium issues. What Looks Good
RecommendationReady to merge. All critical and medium issues from prior reviews are addressed. The 3 remaining open threads are low-priority suggestions suitable for follow-up. |
| raise RuntimeError("SMTP not configured: set SMTP_HOST") | ||
|
|
||
| message = EmailMessage() | ||
| message["From"] = f'"{settings.SMTP_FROM_NAME}" <{settings.SMTP_FROM_EMAIL}>' |
There was a problem hiding this comment.
🔵 Low — Manual From header may produce malformed RFC 5322 if display name contains special characters
The f-string f'"{name}" <{email}>' works for simple names like "Sparkth", but will break subtly if SMTP_FROM_NAME contains double quotes, commas, or non-ASCII characters (e.g. Sparkth "Beta" → malformed header).
Python's email.utils.formataddr handles quoting, escaping, and RFC 2047 encoding automatically:
| message["From"] = f'"{settings.SMTP_FROM_NAME}" <{settings.SMTP_FROM_EMAIL}>' | |
| message["From"] = formataddr((settings.SMTP_FROM_NAME, settings.SMTP_FROM_EMAIL)) |
with from email.utils import formataddr at the top.
Low risk since this is config-controlled, but formataddr is the safer default.
There was a problem hiding this comment.
Fixed in f74cd0c. From is now built with email.utils.formataddr((SMTP_FROM_NAME, SMTP_FROM_EMAIL)), which handles RFC 5322 quoting and RFC 2047 encoding for non-ASCII names. Existing email-composition test still passes (the simple "Test" <…> case round-trips identically through formataddr).
Adds validate_password_complexity() in app/core/security: rejects passwords shorter than 8 chars or missing an uppercase letter, a digit, or a non-alphanumeric character. UserCreate schema runs it via a field_validator so it's enforced at the API boundary and surfaces as a 422 with a field-level error. Frontend register form shows the rule under the password input as a hint, hiding it once a server-side error replaces it. Updates the few existing register-flow tests that submitted weak passwords; adds parametrized tests for each rule plus a strong-password happy-path.
|
Claude finished @hamza-56's task in 2m 36s —— View job Re-review: feat(auth): add email verification on registration
Previous Review CommentsAll 8 critical/medium threads from prior reviews are resolved:
4 low-priority threads from prior reviews remain open (non-blocking):
Latest Commit Review (ff3998f — password complexity)Clean addition of server-side password complexity enforcement:
New IssuesAll low priority — none blocking.
What Looks Good
RecommendationReady to merge. All critical and medium issues from prior reviews are addressed. The latest commit (password complexity) is clean and well-tested. The remaining open threads are all low-priority suggestions suitable for follow-up. |
| if ( | ||
| len(password) < 8 | ||
| or not re.search(r"[A-Z]", password) | ||
| or not re.search(r"\d", password) | ||
| or not re.search(r"[^A-Za-z0-9]", password) | ||
| ): |
There was a problem hiding this comment.
🔵 Low — Single compound condition obscures which rule failed
The validator returns one generic message regardless of which rule the password violated. Splitting into individual checks would let you return a specific message (e.g. "must include a digit") — better for UX and for the 422 response the frontend renders.
Not blocking — the current approach is simpler and the frontend already shows a static hint. But if you want per-rule feedback:
| if ( | |
| len(password) < 8 | |
| or not re.search(r"[A-Z]", password) | |
| or not re.search(r"\d", password) | |
| or not re.search(r"[^A-Za-z0-9]", password) | |
| ): | |
| if len(password) < 8: | |
| raise ValueError("Password must be at least 8 characters.") | |
| if not re.search(r"[A-Z]", password): | |
| raise ValueError("Password must include at least one uppercase letter.") | |
| if not re.search(r"\d", password): | |
| raise ValueError("Password must include at least one number.") | |
| if not re.search(r"[^A-Za-z0-9]", password): | |
| raise ValueError("Password must include at least one special character.") |
There was a problem hiding this comment.
Keeping as-is. The frontend already shows the rules as inline hint text under the password input, so by the time a user submits a weak password they have the full ruleset visible. Splitting into per-rule messages would mean the user sees the same information in two places phrased two different ways — net negative for clarity. Happy to revisit if a UX requirement comes up that benefits from "you missed exactly X" feedback.
| len(password) < 8 | ||
| or not re.search(r"[A-Z]", password) | ||
| or not re.search(r"\d", password) | ||
| or not re.search(r"[^A-Za-z0-9]", password) |
There was a problem hiding this comment.
🔵 Low — No maximum password length
There's no upper bound on password length. Without one, an attacker can submit a multi-MB password to force the bcrypt hash to chew through it (HashDoS). Most frameworks cap at 72 bytes (bcrypt's internal limit) or a reasonable upper bound like 128 chars. Consider adding:
if len(password) > 128:
raise ValueError("Password must not exceed 128 characters.")Low priority since registration is already gated behind whitelist + rate limits, but it's a defense-in-depth measure.
There was a problem hiding this comment.
Fixed in f74cd0c. validate_password_complexity now rejects any password longer than MAX_PASSWORD_LENGTH = 128 chars before running the complexity regexes. Added a parametrized case ("A1!" + "a"*200) that hits the cap. Defense-in-depth even though we already have the registration whitelist + auth rate limits.
- app/core/email.py: build From with email.utils.formataddr instead of a manual f-string. Handles RFC 5322 quoting/encoding so non-ASCII or punctuated SMTP_FROM_NAME values can't produce malformed headers. - _build_verify_url: rstrip the trailing slash off FRONTEND_BASE_URL so configs ending in '/' don't produce '...//verify-email?token=...'. - validate_password_complexity: reject passwords longer than 128 characters as defense-in-depth against hash-DoS via large inputs. Adds two tests: trailing-slash URL building, and a 128+ char password rejected.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
…r hooks
- Add fallback={null} to the Suspense wrappers in app/login/page.tsx
and app/verify-email/page.tsx so the omission is intentional and the
React Doctor / linter sees a fallback at the boundary.
- Destructure useRouter() and useSearchParams() up front so each
component's router/searchParams dependency surface is explicit and
React Compiler can memoize without re-walking the wrapper objects.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
1 similar comment
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
…d Suspense
- VerifyEmailClient: collapse the verify useEffect's three setStatus
branches into a single computed-then-set call, with a cancelled flag
so an unmount mid-fetch can't update unmounted state. Read-then-write
is now exactly one setState per resolution.
- Co-locate the <Suspense fallback={null}> boundary with the
useSearchParams hook by splitting LoginPage and VerifyEmailClient
into a Suspense-wrapping shell + an inner component that owns the
hook. Parent page.tsx wrappers stay (nested Suspense is harmless),
but the boundary is now visible to per-file static analysis.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
1 similar comment
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
hamza-56
changed the title
1 participant
What
Adds email confirmation on account registration. New email/password signups create unverified accounts; users must click a tokenized link before they can log in. Existing users are auto-verified via migration backfill, and Google OAuth signups are auto-verified on creation/link. Also enforces password complexity at registration.
Summary
Backend
email_verification_tokentable; sha256-hashed tokens (raw never persisted), 24h expiry, single-use, prior-token invalidation on resend.EmailVerificationServicemints / verifies / resends;send_verification_emailcomposes a multipart text + HTML body (HTML-escaped name to prevent injection).app/core/email.pywrapsaiosmtplib— works with Amazon SES SMTP, Mailgun, Gmail, MailHog locally.POST /auth/verify-email(400 invalid/expired/used),POST /auth/verify-email/resend(Redis-cooldowned viaSET NX EX, async-context-manager lifecycle).POST /auth/registerschedules the email viaBackgroundTasks(atomic flush + single commit).POST /auth/loginreturns403 {code:"email_not_verified", email}for unverified accounts (only reachable post-password-verification).validate_password_complexityinapp/core/security.pyenforces ≥8 chars + uppercase + digit + special; runs viaUserCreate.field_validator.strip().lower()) at register and Google OAuth callback to prevent case-sensitive lookup mismatches and duplicate accounts.sa.func.now()).Frontend
/verify-emailpage with success / expired / invalid states and a resend form.How to Test
make test— 45 new/updated tests cover token lifecycle, all four endpoints, Google OAuth verification, SMTP composition, resend rate limiting, and password complexity.make frontend.build— clean Next.js build with/verify-emailroute generated.abc,alllowercase1!,NoDigits!!,NoSpecial1A) → 422 with field-level error; submitSup3rSecret!→ 200.email_verified=trueon the resulting user row.Closes #289