feat(auth): add email verification on signup and email change - #57
Merged
Merged
Conversation
Adds an email confirmation step so RegisterUseCase no longer accepts any address with no follow-up. Also re-verifies on email change via UpdateEmailUseCase, since a changed address shouldn't inherit the old address's verified status. - New EmailVerificationToken model: hashed, single-use, 24-hour-expiring token (same pattern as the ApiToken/PasswordResetToken hashing) - New emailVerifiedAt column on User, cleared whenever the email changes - SendEmailVerificationUseCase issues the token and emails the link; both RegisterUseCase and UpdateEmailUseCase call it but swallow delivery failures so a Brevo outage or missing local-dev credentials never blocks account creation or an email change - New verifyEmail GraphQL mutation and VerifyEmailUseCase to consume the token - New /verify-email web route that auto-verifies on load
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughAdds end-to-end email verification: token persistence and hashing, verification email delivery, registration and email-change integration, a GraphQL mutation, and a ChangesEmail verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VerifyEmailPage
participant GraphQLAPI
participant AuthResolver
participant VerifyEmailUseCase
participant TokenRepository
participant UserRepository
VerifyEmailPage->>GraphQLAPI: Submit verification token
GraphQLAPI->>AuthResolver: Call verifyEmail(token)
AuthResolver->>VerifyEmailUseCase: Execute token verification
VerifyEmailUseCase->>TokenRepository: Find hashed token
VerifyEmailUseCase->>UserRepository: Set emailVerifiedAt
VerifyEmailUseCase->>TokenRepository: Mark token used
GraphQLAPI-->>VerifyEmailPage: Return success or error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🤖 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
`@apps/api/src/infrastructure/db/repositories/PrismaEmailVerificationTokenRepository.ts`:
- Around line 41-45: The markUsed method in
PrismaEmailVerificationTokenRepository must consume tokens atomically: replace
the unconditional update with a conditional updateMany requiring usedAt to be
null, and return whether exactly one row was updated. Update the repository port
and verification use case to propagate this result and reject verification when
consumption fails.
In `@apps/api/src/infrastructure/email/BrevoEmailService.ts`:
- Around line 70-79: Update sendEmailVerification’s Brevo fetch request to use a
bounded request-level timeout, ensuring blocked connections reject so existing
error handling can proceed. Use the project’s established timeout mechanism or
define an appropriate request timeout, and add service tests covering the
timeout rejection path.
In `@apps/api/src/use-cases/user/UpdateEmailUseCase.ts`:
- Around line 31-36: The UpdateEmailUseCase must treat an unchanged email as a
no-op: compare the existing and new addresses, and only clear emailVerifiedAt
and send verification when they differ. Update
apps/api/src/use-cases/user/UpdateEmailUseCase.ts lines 31-36 accordingly, and
add coverage in
apps/api/src/__tests__/application/user/UpdateEmailUseCase.test.ts lines 119-125
asserting verification is preserved and no new token is sent for an unchanged
address.
- Around line 33-43: Update apps/api/src/use-cases/user/UpdateEmailUseCase.ts
lines 33-43 to atomically update the email and invalidate prior verification
tokens. Update apps/api/src/use-cases/auth/VerifyEmailUseCase.ts lines 18-36 to
atomically claim an unused token and verify the user only when the token’s
captured email or address version still matches, preventing reuse and
stale-token verification.
In `@apps/web/src/routes/verify-email.tsx`:
- Around line 8-12: Replace the inline VERIFY_EMAIL_MUTATION definition in the
verify-email route with a generated GraphQL operation. Add the VerifyEmail
mutation to the appropriate web .graphql file, regenerate the document and
variable/result types, and update the route to use those generated symbols while
preserving the existing token input and response behavior.
- Around line 26-44: Update the token-dependent useEffect to reset verification
state whenever token changes: clear the prior error, set status to verifying,
and immediately set the invalid-link error and error status when token is
absent. Preserve the existing request, cancellation, and success/error handling
for present tokens.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa5b8467-a323-4092-9701-b588e7960ed0
📒 Files selected for processing (38)
apps/api/prisma/migrations/20260722221759_add_email_verification/migration.sqlapps/api/prisma/schema.prismaapps/api/src/__tests__/application/auth/RegisterUseCase.test.tsapps/api/src/__tests__/application/auth/SendEmailVerificationUseCase.test.tsapps/api/src/__tests__/application/auth/VerifyEmailUseCase.test.tsapps/api/src/__tests__/application/reminders/SendFollowUpRemindersUseCase.test.tsapps/api/src/__tests__/application/user/UpdateEmailUseCase.test.tsapps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.tsapps/api/src/__tests__/helpers/createTestDb.tsapps/api/src/__tests__/helpers/mocks.tsapps/api/src/__tests__/infrastructure/db/repositories/PrismaEmailVerificationTokenRepository.test.tsapps/api/src/__tests__/infrastructure/db/repositories/PrismaUserRepository.test.tsapps/api/src/__tests__/infrastructure/email/BrevoEmailService.test.tsapps/api/src/__tests__/infrastructure/email/templates/emailVerificationTemplate.test.tsapps/api/src/__tests__/interface-adapters/resolvers/AuthResolver.test.tsapps/api/src/__tests__/security/authorizationGuards.test.tsapps/api/src/constants.tsapps/api/src/domain/emailVerificationToken/EmailVerificationToken.tsapps/api/src/domain/user/User.tsapps/api/src/http/container.tsapps/api/src/http/schema/mutations/authMutations.tsapps/api/src/infrastructure/db/repositories/PrismaEmailVerificationTokenRepository.tsapps/api/src/infrastructure/db/repositories/PrismaUserRepository.tsapps/api/src/infrastructure/email/BrevoEmailService.tsapps/api/src/infrastructure/email/templates/emailVerificationTemplate.tsapps/api/src/interface-adapters/resolvers/AuthResolver.tsapps/api/src/use-cases/auth/ISendEmailVerificationUseCase.tsapps/api/src/use-cases/auth/IVerifyEmailUseCase.tsapps/api/src/use-cases/auth/RegisterUseCase.tsapps/api/src/use-cases/auth/SendEmailVerificationUseCase.tsapps/api/src/use-cases/auth/VerifyEmailUseCase.tsapps/api/src/use-cases/ports/IEmailService.tsapps/api/src/use-cases/ports/IEmailVerificationTokenRepository.tsapps/api/src/use-cases/ports/IUserRepository.tsapps/api/src/use-cases/user/UpdateEmailUseCase.tsapps/web/src/__tests__/components/VerifyEmailPage.test.tsxapps/web/src/routeTree.gen.tsapps/web/src/routes/verify-email.tsx
Comment on lines
+41
to
+45
| async markUsed(id: string): Promise<void> { | ||
| await this.db.emailVerificationToken.update({ | ||
| where: { id }, | ||
| data: { usedAt: new Date() }, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make token consumption atomic.
Two concurrent verifications can both observe an unused token before either reaches this unconditional update. Use a conditional updateMany with usedAt: null, return whether exactly one row changed, and reject the verification when it did not; update the port and use case accordingly.
🤖 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
`@apps/api/src/infrastructure/db/repositories/PrismaEmailVerificationTokenRepository.ts`
around lines 41 - 45, The markUsed method in
PrismaEmailVerificationTokenRepository must consume tokens atomically: replace
the unconditional update with a conditional updateMany requiring usedAt to be
null, and return whether exactly one row was updated. Update the repository port
and verification use case to propagate this result and reject verification when
consumption fails.
Comment on lines
+70
to
+79
| const response = await fetch(EMAIL.BREVO_API_URL, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json', 'api-key': this.apiKey }, | ||
| body: JSON.stringify({ | ||
| sender: { name: this.fromName, email: this.fromEmail }, | ||
| to: [{ email: to }], | ||
| subject: 'Verify your Job Finder email', | ||
| htmlContent, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate file and relevant usages =="
git ls-files | rg '^apps/api/src/infrastructure/email/BrevoEmailService\.ts$|sendEmailVerification|BrevoEmailService|EmailService'
echo
echo "== File outline =="
ast-grep outline apps/api/src/infrastructure/email/BrevoEmailService.ts || true
echo
echo "== Relevant BrevoEmailService.ts lines =="
cat -n apps/api/src/infrastructure/email/BrevoEmailService.ts | sed -n '1,140p'
echo
echo "== Fetch usages in email service / callers =="
rg -n "sendEmailVerification|class .*Email|interface .*Email|createEmail|EmailService|BrevoEmailService|fetch\\(" apps/api/src -g '*.ts' -g '*.tsx' | head -200Repository: mankatcheung/job-finder
Length of output: 17306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant sender/caller use cases =="
cat -n apps/api/src/use-cases/auth/SendEmailVerificationUseCase.ts | sed -n '1,90p'
echo
cat -n apps/api/src/use-cases/auth/RegisterUseCase.ts | sed -n '1,120p'
echo
cat -n apps/api/src/use-cases/user/UpdateEmailUseCase.ts | sed -n '1,70p'
echo
echo "== Current BrevoEmailService tests =="
cat -n apps/api/src/__tests__/infrastructure/email/BrevoEmailService.test.ts | sed -n '1,230p'
echo
echo "== Node fetch timeout/signal availability probe =="
node - <<'JS'
console.log({
fetchAvailable: typeof fetch,
AbortSignalAvailable: typeof AbortSignal,
AbortSignalTimeoutAvailable: typeof AbortSignal?.timeout,
});
JSRepository: mankatcheung/job-finder
Length of output: 14660
Add a bounded timeout to the Brevo request.
sendEmailVerification is awaited in registration and email updates, and try/catch only activates once the promise rejects; an unbounded fetch can still leave those operations stalled on a blocked Brevo connection. Add a request-level timeout and cover the timeout path in the service tests.
Proposed fix
const response = await fetch(EMAIL.BREVO_API_URL, {
method: 'POST',
+ signal: AbortSignal.timeout(10_000),
headers: { 'Content-Type': 'application/json', 'api-bar': this.apiKey },🤖 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 `@apps/api/src/infrastructure/email/BrevoEmailService.ts` around lines 70 - 79,
Update sendEmailVerification’s Brevo fetch request to use a bounded
request-level timeout, ensuring blocked connections reject so existing error
handling can proceed. Use the project’s established timeout mechanism or define
an appropriate request timeout, and add service tests covering the timeout
rejection path.
Comment on lines
+31
to
+36
| // Changing the email address invalidates verification of the old one — | ||
| // the new address must be re-confirmed before it counts as verified. | ||
| await this.deps.userRepository.update(input.userId, { | ||
| email: input.newEmail, | ||
| emailVerifiedAt: null, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not revoke verification for a no-op email update. The current-user email is explicitly allowed, but this path now clears emailVerifiedAt and sends another verification email even when the address is unchanged.
apps/api/src/use-cases/user/UpdateEmailUseCase.ts#L31-L36: only clear verification and trigger delivery when the address actually changes.apps/api/src/__tests__/application/user/UpdateEmailUseCase.test.ts#L119-L125: assert that an unchanged address preserves verification and does not send a new token.
📍 Affects 2 files
apps/api/src/use-cases/user/UpdateEmailUseCase.ts#L31-L36(this comment)apps/api/src/__tests__/application/user/UpdateEmailUseCase.test.ts#L119-L125
🤖 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 `@apps/api/src/use-cases/user/UpdateEmailUseCase.ts` around lines 31 - 36, The
UpdateEmailUseCase must treat an unchanged email as a no-op: compare the
existing and new addresses, and only clear emailVerifiedAt and send verification
when they differ. Update apps/api/src/use-cases/user/UpdateEmailUseCase.ts lines
31-36 accordingly, and add coverage in
apps/api/src/__tests__/application/user/UpdateEmailUseCase.test.ts lines 119-125
asserting verification is preserved and no new token is sent for an unchanged
address.
Comment on lines
+33
to
+43
| await this.deps.userRepository.update(input.userId, { | ||
| email: input.newEmail, | ||
| emailVerifiedAt: null, | ||
| }); | ||
|
|
||
| try { | ||
| await this.deps.sendEmailVerificationUseCase.execute(input.userId); | ||
| } catch { | ||
| // Verification email delivery is non-critical — don't block the email | ||
| // change if the email provider is down or unconfigured (e.g. local dev). | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Bind verification tokens to the email version and consume them atomically. After Line 33 updates the address, an old token can be verified before SendEmailVerificationUseCase deletes it; VerifyEmailUseCase then marks the new address verified because tokens only carry userId. Concurrent verification requests can also both pass the unused-token check.
apps/api/src/use-cases/user/UpdateEmailUseCase.ts#L33-L43: invalidate prior tokens atomically with the address change.apps/api/src/use-cases/auth/VerifyEmailUseCase.ts#L18-L36: atomically claim the token and verify only when its captured email/address-version still matches the user.
📍 Affects 2 files
apps/api/src/use-cases/user/UpdateEmailUseCase.ts#L33-L43(this comment)apps/api/src/use-cases/auth/VerifyEmailUseCase.ts#L18-L36
🤖 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 `@apps/api/src/use-cases/user/UpdateEmailUseCase.ts` around lines 33 - 43,
Update apps/api/src/use-cases/user/UpdateEmailUseCase.ts lines 33-43 to
atomically update the email and invalidate prior verification tokens. Update
apps/api/src/use-cases/auth/VerifyEmailUseCase.ts lines 18-36 to atomically
claim an unused token and verify the user only when the token’s captured email
or address version still matches, preventing reuse and stale-token verification.
Comment on lines
+8
to
+12
| const VERIFY_EMAIL_MUTATION = ` | ||
| mutation VerifyEmail($token: String!) { | ||
| verifyEmail(token: $token) | ||
| } | ||
| `; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a generated GraphQL operation instead of an inline mutation.
Move this operation to a web .graphql file and call the generated document/types from the route. As per coding guidelines, when adding a feature, follow the layer order: “web .graphql file → code-generated types → UI.”
🤖 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 `@apps/web/src/routes/verify-email.tsx` around lines 8 - 12, Replace the inline
VERIFY_EMAIL_MUTATION definition in the verify-email route with a generated
GraphQL operation. Add the VerifyEmail mutation to the appropriate web .graphql
file, regenerate the document and variable/result types, and update the route to
use those generated symbols while preserving the existing token input and
response behavior.
Source: Coding guidelines
Comment on lines
+26
to
+44
| useEffect(() => { | ||
| if (!token) return; | ||
| let cancelled = false; | ||
|
|
||
| gqlClient | ||
| .request(VERIFY_EMAIL_MUTATION, { token }) | ||
| .then(() => { | ||
| if (!cancelled) setStatus('success'); | ||
| }) | ||
| .catch((err: unknown) => { | ||
| if (cancelled) return; | ||
| setErrorMessage(extractGqlError(err) ?? 'Failed to verify email.'); | ||
| setStatus('error'); | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [token]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching verify-email.tsx:"
fd -a 'verify-email\.tsx$' . || true
echo
echo "Relevant file outline and contents:"
if [ -f apps/web/src/routes/verify-email.tsx ]; then
wc -l apps/web/src/routes/verify-email.tsx
cat -n apps/web/src/routes/verify-email.tsx
fi
echo
echo "Search for useVerifyEmail / VerifyEmail route references:"
rg -n "verify-email|useVerifyEmail|VerifyEmail|VERIFY_EMAIL_MUTATION|verifying|success|Failed to verify email" apps/web/src -S || true
echo
echo "Git status/stat:"
git status --short
git diff --stat || true
git diff -- apps/web/src/routes/verify-email.tsx || trueRepository: mankatcheung/job-finder
Length of output: 8339
Reset verification UI state when the token changes.
TanStack Router preserves route state while the route component remains mounted, so if the URL changes from ?token=abc to ?token=xyz or drops the token, the page can keep showing the previous success/error result. Reset through verifying, clear the previous error, and show the invalid-link error whenever token is absent.
Proposed fix
useEffect(() => {
- if (!token) return;
+ if (!token) {
+ setErrorMessage(null);
+ setStatus('error');
+ return;
+ }
+
+ setErrorMessage(null);
+ setStatus('verifying');
let cancelled = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (!token) return; | |
| let cancelled = false; | |
| gqlClient | |
| .request(VERIFY_EMAIL_MUTATION, { token }) | |
| .then(() => { | |
| if (!cancelled) setStatus('success'); | |
| }) | |
| .catch((err: unknown) => { | |
| if (cancelled) return; | |
| setErrorMessage(extractGqlError(err) ?? 'Failed to verify email.'); | |
| setStatus('error'); | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [token]); | |
| useEffect(() => { | |
| if (!token) { | |
| setErrorMessage(null); | |
| setStatus('error'); | |
| return; | |
| } | |
| setErrorMessage(null); | |
| setStatus('verifying'); | |
| let cancelled = false; | |
| gqlClient | |
| .request(VERIFY_EMAIL_MUTATION, { token }) | |
| .then(() => { | |
| if (!cancelled) setStatus('success'); | |
| }) | |
| .catch((err: unknown) => { | |
| if (cancelled) return; | |
| setErrorMessage(extractGqlError(err) ?? 'Failed to verify email.'); | |
| setStatus('error'); | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [token]); |
🤖 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 `@apps/web/src/routes/verify-email.tsx` around lines 26 - 44, Update the
token-dependent useEffect to reset verification state whenever token changes:
clear the prior error, set status to verifying, and immediately set the
invalid-link error and error status when token is absent. Preserve the existing
request, cancellation, and success/error handling for present tokens.
5 tasks done
5 tasks done
mankatcheung
commented
Jul 23, 2026
mankatcheung
left a comment
mankatcheung
left a comment
Owner
Author
There was a problem hiding this comment.
Overview
Adds an EmailVerificationToken model + flow: RegisterUseCase and UpdateEmailUseCase now issue a hashed, single-use, 24h-expiring token and email a /verify-email?token=... link; a new verifyEmail mutation consumes it. Clean layering (domain → port → use case → Prisma repo → resolver → mutation → web route), consistent with the existing Clean Architecture conventions, and each new unit has focused test coverage (use cases, Prisma repo against a real SQLite test DB, Brevo service, email template, resolver, web page).
Security Analysis
- Token generation/storage: good.
randomBytes(32)→ sha256 hash stored intokenHash(unique), raw value only ever in the emailed URL — mirrorsApiToken's pattern. Single-use (usedAt) and 24h TTL (expiresAt) are both checked inVerifyEmailUseCase. Old tokens are deleted (deleteAllForUser) before a new one is issued, so only one valid token exists per user at a time. - Correction on PR description: the description says this token follows "the same pattern as
ApiToken/PasswordResetToken(#56)" — I could not find anyPasswordResetTokenmodel, repository, or forgot-password use case anywhere in the repo (checkedschema.prisma, use-cases, and a repo-wide code search). There is no forgot-password flow yet, so that reference appears to be aspirational/incorrect — worth fixing the description or confirming #56 hasn't landed. - No user enumeration:
verifyEmailtakes only a token, never an email, so it can't be used to probe for account existence. - Not actually enforced anywhere.
emailVerifiedAtis set but never read:LoginUseCasedoesn't check it (unverified users can log in and use the app fully), andUserType.tsdoesn't exposeemailVerifiedAtto the GraphQL API at all, so the web app has no way to show a "please verify" banner even if it wanted to. Combined with no "resend verification" mutation, a user who misses/loses the one 24h-expiring email currently has no way to get a fresh one short of changing their email address again. This may well be intentional scope-for-this-ticket (JEF-17 is literally "add verification on signup/email change"), but as shipped the feature has no teeth yet — flagging so it's a deliberate, not accidental, gap. - No notification to the old address on email change.
UpdateEmailUseCasere-verifies the new address but never emails the old one to say "your account email was changed." Low risk today since there's no password-reset-via-email flow yet, but this becomes an account-takeover vector (attacker with a live session silently repoints the account to their own inbox) the moment such a flow exists — worth a follow-up ticket. - No rate limiting on registration/email-change triggering an email send — but this is a pre-existing, app-wide gap (no
fastify-rate-limitor similar anywhere in the codebase), not something newly introduced here. - Swallowing email-delivery failures in both call sites is intentional, documented with comments, and tested (including the failure path) — reasonable given Brevo isn't configured in local dev.
Code Quality
- Follows repo conventions well:
I*UseCaseport interfaces, Awilix registration (TRANSIENTfor use cases,SINGLETONfor the repo) incontainer.ts, mapper-free domain type sinceEmailVerificationTokenis simple. SendEmailVerificationUseCase/VerifyEmailUseCaseare small, single-purpose, easy to follow.container.ts'swebAppOriginderives fromCORS_ORIGIN.split(',')[0]— ifCORS_ORIGINis ever a genuine multi-origin list (e.g., staging + prod sharing one API), verification links would always point at whichever origin happens to be first, potentially the wrong one for some users. Minor, but worth a dedicatedWEB_APP_ORIGINenv var instead of overloading the CORS allow-list.VerifyEmailUseCase.executedoes the "mark user verified" and "mark token used" as two separate non-transactional writes; a crash between them would leave a used-looking-unused token but a verified user (harmless) or vice versa. Not worth blocking on, but aprisma.$transactionwould be more correct given the rest of the app already uses atransactionContext/getClientpattern.
Issues & Risks
- Feature is currently inert: no login gating, no GraphQL exposure of
emailVerifiedAt, no resend path, no UI indicator (confirmedaccount.tsxhas zero mention of verification). Recommend a fast follow-up ticket so this doesn't get lost. - PR description references a nonexistent
PasswordResetToken— please correct or confirm. - Old-email notification on email change is missing (future account-takeover concern once password reset exists).
webAppOriginpicks the first CORS origin — fine for single-origin deployments, fragile for multi-origin ones.
Test Coverage
Strong: new unit tests for SendEmailVerificationUseCase (not-found, happy path, TTL math), VerifyEmailUseCase (missing/used/expired/valid token), PrismaEmailVerificationTokenRepository against a real in-memory DB (including cross-user isolation for deleteAllForUser), BrevoEmailService.sendEmailVerification, the email template, AuthResolver.verifyEmail, updated RegisterUseCase/UpdateEmailUseCase tests covering the non-blocking-failure path, and VerifyEmailPage (no-token / verifying / success / error states). createTestDb.ts and mocks.ts updated consistently. No coverage gaps stood out.
Verdict
Approving — the token issuance/consumption implementation itself is secure and well-tested, and nothing here regresses existing behavior. The findings above (enforcement/UX gaps, old-email notification, PR description inaccuracy, CORS-origin edge case) are follow-up items rather than blockers for this PR's stated scope.
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
1 participant
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.
Summary
RegisterUseCasepreviously accepted any email with no confirmation step — this adds a verification token sent on registrationUpdateEmailUseCase, clearingemailVerifiedAtsince a changed address shouldn't inherit the old address's verified statusEmailVerificationTokenmodel: hashed (sha256), single-use, 24-hour-expiring token, same pattern asApiToken/PasswordResetToken(feat(auth): add email-based forgot/reset password flow #56)SendEmailVerificationUseCaseissues the token and sends the email; bothRegisterUseCaseandUpdateEmailUseCasecall it but swallow delivery failures (matching the precedent inaccount.tsx's export handler) so a Brevo outage or missing local-dev credentials never blocks account creation or an email change — verified via smoke test: registration succeeds with noBREVO_API_KEYconfiguredverifyEmailGraphQL mutation +VerifyEmailUseCaseto consume the token/verify-emailweb route that auto-verifies on page loadFixes JEF-17
Test plan
pnpm typecheck(api + web)pnpm lint(api + web)pnpm test— 425 API tests + 79 web tests passing, including new coverage forSendEmailVerificationUseCase,VerifyEmailUseCase,PrismaEmailVerificationTokenRepository,BrevoEmailService.sendEmailVerification, the verification email template,AuthResolver, updatedRegisterUseCase/UpdateEmailUseCase(including the non-blocking-email-failure path), and the newVerifyEmailPagepnpm build(all packages)verifyEmailmutation present in schema, andregisterstill succeeds end-to-end with no Brevo credentials configured🤖 Generated with Claude Code
https://claude.ai/code/session_01DdEiNRTcUnM6kE5AdFQ3n8
Summary by CodeRabbit
verifyEmailmutation./verify-emailpage that guides users through verifying, success, and error states.