Repository navigation
feat(auth): add email-based forgot/reset password flow - #56
Conversation
Adds a self-service password recovery path for users who are locked out (previously UpdatePasswordUseCase required knowing the current password). - New PasswordResetToken model: a hashed, single-use, 1-hour-expiring token (mirrors the existing ApiToken hashing pattern) - RequestPasswordResetUseCase always resolves silently regardless of whether the email is known, to prevent account enumeration - New requestPasswordReset/resetPassword GraphQL mutations - New /forgot-password and /reset-password web routes, linked from /login - Reuses the existing Brevo email infrastructure via a new sendPasswordReset method on IEmailService
WalkthroughThis PR implements a full password reset flow: a new ChangesPassword reset feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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 |
mankatcheung
left a comment
mankatcheung
left a comment
There was a problem hiding this comment.
Overview
Adds a self-service forgot/reset password flow: new PasswordResetToken table (hashed, single-use, 1h TTL), RequestPasswordResetUseCase / ResetPasswordUseCase, requestPasswordReset/resetPassword GraphQL mutations, Brevo email template, and /forgot-password + /reset-password web routes. Good test coverage across use cases, the Prisma repository (real in-memory DB), the email service/template, the resolver, and both new web pages.
Security Analysis
Token design — solid. 32 random bytes (256 bits) hex-encoded, SHA-256 hashed at rest, unique index lookup (no naive string compare), single-use via usedAt, 1h expiry checked server-side. This mirrors the existing ApiToken pattern appropriately.
HIGH — no session/refresh-token invalidation on reset. ResetPasswordUseCase updates passwordHash but does nothing to the auth session. Refresh tokens in this app are stateless JWTs (FastifyJwtTokenService, 7‑day expiry) with no revocation store, so an attacker holding a stolen refresh token (the exact scenario a password reset is meant to remediate) keeps full access for up to 7 days after the legitimate user resets their password. This defeats the primary purpose of the feature. Recommend adding a tokenVersion (or passwordChangedAt) claim on User, bumping it in ResetPasswordUseCase, and checking it in verifyRefresh/context auth so old refresh tokens are rejected immediately.
MEDIUM — enumeration leak via error propagation on requestPasswordReset. In authMutations.ts, resetPassword wraps the use case in try/catch + fromCodedError, but requestPasswordReset does not. Since emailService.sendPasswordReset is only invoked for known emails (unknown emails short-circuit before it), any transient failure from Brevo (timeout, 5xx, rate limit) surfaces as a GraphQL error only for existing accounts — letting an attacker distinguish valid from invalid emails. Awaiting the email send inline also adds a timing side-channel (network call vs. instant no-op) on top of the identical response body. Suggest: catch/swallow (with logging) errors from the email send inside RequestPasswordResetUseCase, and consider not awaiting the send (fire-and-forget with error logging) so the mutation always returns in roughly constant time regardless of email existence.
MEDIUM — no server-side password strength validation in ResetPasswordUseCase. newPassword goes straight to bcrypt.hash with no length/complexity check; only the web Zod schema enforces min(8). A direct GraphQL call (bypassing the web UI) could set a 1-character or empty password. Worth adding the same minimum-length check server-side (this gap also exists in RegisterUseCase, but the recovery path is a good place to close it now).
MEDIUM — no rate limiting on requestPasswordReset. There's no rate-limiting plugin anywhere in the API (@fastify/rate-limit isn't a dependency), so this new endpoint can be hit repeatedly to email-bomb a victim or hammer the Brevo API. This is a pre-existing gap across all auth mutations, but it's most relevant here since this is the first mutation that triggers an external send per call.
Good: enumeration is otherwise well handled at the use-case level (identical return value/timing-independent response body for known vs. unknown email in the happy path), tokens are deleted on each new request (no stale valid tokens lying around), and the reset email copy correctly states the link is single-purpose and safely ignorable.
Code Quality
- Follows existing Clean Architecture layering precisely: domain entity → port → use case → Prisma repo → resolver → Pothos mutation → container wiring → web route. Consistent with
ApiToken's existing hashing convention. webAppOriginis derived by splittingCORS_ORIGINand taking the first entry — functional, but repurposing a CORS setting to build user-facing email links is an implicit coupling; a dedicatedWEB_APP_URLenv var would be clearer and less fragile ifCORS_ORIGINever changes shape/order.- Files are small and focused; DI wiring in
container.tscorrectly uses TRANSIENT for the two new use cases and SINGLETON for the repository, matching convention. - Web pages (
forgot-password.tsx,reset-password.tsx) are consistent in style with the existinglogin.tsx/register.tsx.
Issues & Risks
- Stolen refresh tokens survive a password reset (HIGH, see above).
- Enumeration via error-path asymmetry on
requestPasswordReset(MEDIUM). - No server-side password validation on reset (MEDIUM).
- No rate limiting on the reset-request endpoint (MEDIUM, pre-existing pattern app-wide).
- Minor: requesting a second reset link invalidates the first (
deleteAllForUserruns on every request) — intentional single-active-token design, just flagging as a UX consideration, not a bug.
Suggestions
- Add a
tokenVersion/passwordChangedAtcheck to the refresh-token verification path and bump it inResetPasswordUseCase. - Wrap/guard the
sendPasswordResetcall inRequestPasswordResetUseCaseso email-provider failures never leak through as errors distinguishable from the no-op path. - Add a minimum length (and ideally reuse whatever complexity policy exists/gets added) check in
ResetPasswordUseCasebefore hashing. - Consider a basic per-IP/per-email rate limit on
requestPasswordReset(e.g.@fastify/rate-limit) before this ships broadly. - Replace the
CORS_ORIGIN-derivedwebAppOriginwith a dedicated env var.
Test Coverage
Strong: RequestPasswordResetUseCase and ResetPasswordUseCase unit tests cover the enumeration no-op, expiry math, and all UNAUTHORIZED paths (missing/used/expired token); PrismaPasswordResetTokenRepository is tested against a real in-memory DB including cross-user isolation; BrevoEmailService.sendPasswordReset and the email template are covered; AuthResolver delegation and error propagation are tested; both new web pages have solid RTL coverage including the "always shows the same confirmation" enumeration-resistance assertion. No test currently exercises the session-invalidation gap or the email-failure error-propagation asymmetry noted above — worth adding once those are addressed.
Verdict
Requesting changes primarily for the session-invalidation gap and the enumeration side-channel on email-service failure — both go to the core security guarantees this feature is meant to provide. The token design itself (entropy, hashing, single-use, expiry) is solid and the test coverage is otherwise thorough.
- Invalidate all existing sessions when a password reset succeeds, so a refresh token stolen before the reset can't outlive it (JEF-24). - Stop letting email-provider failures distinguish known vs. unknown email addresses on requestPasswordReset — swallow send failures the same way the silent unknown-email no-op already does (JEF-25). - Add a minimum password length check to ResetPasswordUseCase, and add an in-process fixed-window rate limiter (by email and by IP) on requestPasswordReset to blunt email-bombing and enumeration probing (JEF-26).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/prisma/schema.prisma`:
- Around line 67-76: The reset-token claim flow must be atomic against
concurrent requests. Update the repository’s markUsed() implementation to
conditionally update only records whose usedAt is null, and throw when the
conditional update does not claim exactly one row; then change
ResetPasswordUseCase.execute() to perform the password update and session
revocation within the same database transaction as the token claim.
In `@apps/api/src/constants.ts`:
- Around line 104-109: Update the password-reset rate limiting flow using
RATE_LIMIT.PASSWORD_RESET_REQUEST so its counters and windows are enforced
through a shared atomic store such as Redis, rather than RateLimiter.buckets;
alternatively, explicitly enforce and document a single-instance deployment
guarantee before retaining the in-process implementation.
In `@apps/api/src/http/container.ts`:
- Around line 247-252: Update the passwordResetRateLimiter registration to use a
bounded, TTL-evicting store so entries for inactive email keys are removed and
total state remains capped. Preserve the existing
RATE_LIMIT.PASSWORD_RESET_REQUEST limits while configuring the RateLimiter
instance for expiration and bounded storage.
In `@apps/api/src/infrastructure/rateLimit/RateLimiter.ts`:
- Around line 26-28: Update RateLimiter’s bucket-expiry condition to use an
inclusive resetAt comparison, and adjust the related RateLimiter test to advance
exactly 60_000 ms so it verifies the boundary reset behavior.
- Line 15: Update the RateLimiter bucket storage around the private buckets Map
and its related rate-limit logic to use a bounded TTL cache or shared store with
explicit expiry and capacity limits. Ensure expired one-off email/IP keys are
removed and new entries cannot grow the singleton store beyond the configured
capacity, while preserving existing rate-limit behavior.
In `@apps/api/src/use-cases/auth/RequestPasswordResetUseCase.ts`:
- Around line 40-64: Update RequestPasswordResetUseCase so email delivery is
queued or dispatched outside the awaited request path, while preserving the
existing silent behavior for unknown emails and provider failures. Minimize work
and timing differences between the no-user branch and the user branch, including
avoiding awaited token writes or otherwise equalizing the response path; use the
existing email/queue abstractions rather than introducing synchronous delivery.
In `@apps/api/src/use-cases/auth/ResetPasswordUseCase.ts`:
- Around line 29-43: The ResetPasswordUseCase currently reads and later marks
the reset token, allowing concurrent redemption and partial updates. Replace the
findByTokenHash/markUsed sequence with a conditional atomic consume requiring
usedAt IS NULL and an unexpired token, then execute token consumption, password
update, and revokeAllForUser within one transaction; preserve the unauthorized
error when consumption fails and add a test covering concurrent redemption.
In `@apps/api/src/use-cases/ports/IPasswordResetTokenRepository.ts`:
- Around line 10-11: Replace markUsed in IPasswordResetTokenRepository with an
atomic claim method that returns whether the token was unused and unexpired. In
PrismaPasswordResetTokenRepository, implement the claim using a guarded update
on token ID, unused status, and expiration, and require exactly one updated row.
In ResetPasswordUseCase, perform the claim before changing the password or
revoking sessions, aborting when it fails. Apply these changes at
apps/api/src/use-cases/ports/IPasswordResetTokenRepository.ts lines 10-11 and
apps/api/src/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.ts
lines 41-43; ResetPasswordUseCase requires the corresponding call-site update.
In `@apps/web/src/routes/forgot-password.tsx`:
- Around line 31-35: Update onSubmit in the forgot-password form to catch
gqlClient.request failures and surface them through the form’s root error using
setError, including RATE_LIMITED and transport errors without exposing account
existence. Render errors.root?.message in the form, mirroring
reset-password.tsx, and reuse or add the shared extractGqlError helper for the
displayed message.
🪄 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 Plus
Run ID: 72cb9c85-83fd-4a9a-80de-a7cae58cad5b
📒 Files selected for processing (44)
apps/api/prisma/migrations/20260722213047_add_password_reset_tokens/migration.sqlapps/api/prisma/schema.prismaapps/api/src/__tests__/application/auth/RequestPasswordResetUseCase.test.tsapps/api/src/__tests__/application/auth/ResetPasswordUseCase.test.tsapps/api/src/__tests__/application/auth/SendEmailVerificationUseCase.test.tsapps/api/src/__tests__/application/reminders/SendFollowUpRemindersUseCase.test.tsapps/api/src/__tests__/digest/SendWeeklyDigestUseCase.test.tsapps/api/src/__tests__/helpers/createTestDb.tsapps/api/src/__tests__/helpers/mocks.tsapps/api/src/__tests__/http/errors/AppError.test.tsapps/api/src/__tests__/http/errors/formatError.test.tsapps/api/src/__tests__/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.test.tsapps/api/src/__tests__/infrastructure/db/repositories/PrismaSessionRepository.test.tsapps/api/src/__tests__/infrastructure/email/BrevoEmailService.test.tsapps/api/src/__tests__/infrastructure/email/templates/passwordResetTemplate.test.tsapps/api/src/__tests__/infrastructure/rateLimit/RateLimiter.test.tsapps/api/src/__tests__/interface-adapters/resolvers/AuthResolver.test.tsapps/api/src/constants.tsapps/api/src/domain/passwordResetToken/PasswordResetToken.tsapps/api/src/http/container.tsapps/api/src/http/errors/AppError.tsapps/api/src/http/errors/formatError.tsapps/api/src/http/schema/mutations/authMutations.tsapps/api/src/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.tsapps/api/src/infrastructure/db/repositories/PrismaSessionRepository.tsapps/api/src/infrastructure/email/BrevoEmailService.tsapps/api/src/infrastructure/email/templates/passwordResetTemplate.tsapps/api/src/infrastructure/rateLimit/RateLimiter.tsapps/api/src/interface-adapters/resolvers/AuthResolver.tsapps/api/src/use-cases/auth/IRequestPasswordResetUseCase.tsapps/api/src/use-cases/auth/IResetPasswordUseCase.tsapps/api/src/use-cases/auth/RequestPasswordResetUseCase.tsapps/api/src/use-cases/auth/ResetPasswordUseCase.tsapps/api/src/use-cases/ports/IEmailService.tsapps/api/src/use-cases/ports/IPasswordResetTokenRepository.tsapps/api/src/use-cases/ports/IRateLimiter.tsapps/api/src/use-cases/ports/ISessionRepository.tsapps/web/src/__tests__/components/ForgotPasswordPage.test.tsxapps/web/src/__tests__/components/LoginPage.test.tsxapps/web/src/__tests__/components/ResetPasswordPage.test.tsxapps/web/src/routeTree.gen.tsapps/web/src/routes/forgot-password.tsxapps/web/src/routes/login.tsxapps/web/src/routes/reset-password.tsx
| model PasswordResetToken { | ||
| id String @id | ||
| userId String | ||
| tokenHash String @unique | ||
| expiresAt DateTime | ||
| usedAt DateTime? | ||
| createdAt DateTime @default(now()) | ||
| user User @relation(fields: [userId], references: [id], onDelete: Cascade) | ||
|
|
||
| @@index([userId]) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate prisma schema and reset token related files =="
git ls-files | rg '(^|/)schema\.prisma$|reset|password|auth|otp|token' | head -200
echo
echo "== schema excerpt around PasswordResetToken =="
if [ -f apps/api/prisma/schema.prisma ]; then
nl -ba apps/api/prisma/schema.prisma | sed -n '1,130p' | rg -n -C 8 'model PasswordResetToken|PasswordResetToken|Token' || true
fi
echo
echo "== search relevant symbols =="
rg -n "PasswordResetToken|resetPassword|markUsed|usedAt|resetPasswordToken|createPasswordReset|usePasswordReset|PasswordReset" -S .Repository: mankatcheung/job-finder
Length of output: 26895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ResetPasswordUseCase implementation =="
cat -n apps/api/src/use-cases/auth/ResetPasswordUseCase.ts
echo
echo "== PasswordResetTokenRepository implementation =="
cat -n apps/api/src/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.ts
echo
echo "== PasswordResetToken interface =="
cat -n apps/api/src/domain/passwordResetToken/PasswordResetToken.ts
echo
echo "== IPasswordResetTokenRepository port =="
cat -n apps/api/src/use-cases/ports/IPasswordResetTokenRepository.ts
echo
echo "== AuthResolver resetPassword request flow =="
cat -n apps/api/src/interface-adapters/resolvers/AuthResolver.ts | sed -n '55,70p'
echo
echo "== package versions relevant to Prisma =="
cat package.json | rg -i 'prisma|`@prisma/client`|client' || true
if [ -f apps/api/package.json ]; then cat apps/api/package.json | rg -i 'prisma|`@prisma/client`|client' || true; fiRepository: mankatcheung/job-finder
Length of output: 6232
Make reset-token claims atomic.
ResetPasswordUseCase.execute() reads the token, then separately calls markUsed(), while the repository only does update({ id }, { usedAt }) with no where: { usedAt: null }. Concurrent reset requests can both pass the usedAt guard and complete updates. Change markUsed() into a conditional update that updates only when usedAt IS NULL, throw if only one row is claimed, and wrap the password update/session revocation in the same transaction.
🤖 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/prisma/schema.prisma` around lines 67 - 76, The reset-token claim
flow must be atomic against concurrent requests. Update the repository’s
markUsed() implementation to conditionally update only records whose usedAt is
null, and throw when the conditional update does not claim exactly one row; then
change ResetPasswordUseCase.execute() to perform the password update and session
revocation within the same database transaction as the token claim.
| /** Rate limits for auth endpoints prone to abuse (in-process, fixed-window). */ | ||
| export const RATE_LIMIT = { | ||
| PASSWORD_RESET_REQUEST: { | ||
| MAX_ATTEMPTS: 5, | ||
| WINDOW_MS: 15 * 60 * 1000, // 15 minutes | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching constants.ts:\n'
fd -a 'constants\.ts$' . | sed 's#^\./##'
printf '\nRelevant constants.ts lines:\n'
if [ -f apps/api/src/constants.ts ]; then
nl -ba apps/api/src/constants.ts | sed -n '88,125p'
fi
printf '\nSearch for RATE_LIMIT usage:\n'
rg -n "RATE_LIMIT|PASSWORD_RESET_REQUEST|MAX_ATTEMPTS|WINDOW_MS" .
printf '\nSearch for password reset endpoint/mutation names:\n'
rg -n "password.*reset|PASSWORD|resetPassword|PasswordReset|forgot|forgotPassword" apps/api -SRepository: mankatcheung/job-finder
Length of output: 446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant constants.ts lines:\n'
awk 'NR>=88 && NR<=125 { printf "%5d %s\n", NR, $0 }' apps/api/src/constants.ts
printf '\nSearch for RATE_LIMIT usage:\n'
rg -n "RATE_LIMIT|PASSWORD_RESET_REQUEST|MAX_ATTEMPTS|WINDOW_MS|rateLimit|rate limit" . || true
printf '\nSearch for password reset endpoint/mutation names:\n'
rg -n "password.*reset|PASSWORD|resetPassword|PasswordReset|forgot|forgotPassword" apps/api -S || trueRepository: mankatcheung/job-finder
Length of output: 26717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'RateLimiter files:\n'
fd -a 'RateLimiter\.(ts|js)$' . | sed 's#^\./##'
printf '\nRateLimiter implementation:\n'
if [ -f apps/api/src/infrastructure/rateLimit/RateLimiter.ts ]; then
awk 'NR>=1 && NR<=260 { printf "%5d %s\n", NR, $0 }' apps/api/src/infrastructure/rateLimit/RateLimiter.ts
fi
printf '\nDI container rate-limit construction area:\n'
if [ -f apps/api/src/http/container.ts ]; then
awk 'NR>=230 && NR<=260 { printf "%5d %s\n", NR, $0 }' apps/api/src/http/container.ts
fi
printf '\nRequestPasswordResetUseCase implementation:\n'
if [ -f apps/api/src/use-cases/auth/RequestPasswordResetUseCase.ts ]; then
awk 'NR>=1 && NR<=80 { printf "%5d %s\n", NR, $0 }' apps/api/src/use-cases/auth/RequestPasswordResetUseCase.ts
fiRepository: mankatcheung/job-finder
Length of output: 6318
Move the password-reset rate limit out of in-process storage.
The passwordResetRateLimiter is instantiated from RATE_LIMIT.PASSWORD_RESET_REQUEST limits and persists buckets only in RateLimiter.buckets, so horizontally scaled API instances each allow the same five attempts and a restart clears the window. Use a shared atomic store such as Redis for this security limit, or make the single-instance guarantee explicit before relying on this protection.
🤖 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/constants.ts` around lines 104 - 109, Update the password-reset
rate limiting flow using RATE_LIMIT.PASSWORD_RESET_REQUEST so its counters and
windows are enforced through a shared atomic store such as Redis, rather than
RateLimiter.buckets; alternatively, explicitly enforce and document a
single-instance deployment guarantee before retaining the in-process
implementation.
| passwordResetRateLimiter: asValue( | ||
| new RateLimiter( | ||
| RATE_LIMIT.PASSWORD_RESET_REQUEST.MAX_ATTEMPTS, | ||
| RATE_LIMIT.PASSWORD_RESET_REQUEST.WINDOW_MS, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound rate-limiter state for arbitrary email keys.
This singleton retains one Map entry per unique reset email; expired buckets are not removed unless that exact key is used again. An unauthenticated caller can submit random addresses until process memory is exhausted. Use a limiter with TTL eviction and a bounded store.
🤖 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/http/container.ts` around lines 247 - 252, Update the
passwordResetRateLimiter registration to use a bounded, TTL-evicting store so
entries for inactive email keys are removed and total state remains capped.
Preserve the existing RATE_LIMIT.PASSWORD_RESET_REQUEST limits while configuring
the RateLimiter instance for expiration and bounded storage.
| * horizontally-scaled instances. | ||
| */ | ||
| export class RateLimiter implements IRateLimiter { | ||
| private readonly buckets = new Map<string, Bucket>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound retained rate-limit buckets.
Expired entries for one-off keys are never removed. Password-reset requests can supply unlimited distinct email/IP keys, causing this singleton Map to grow until process memory is exhausted. Use a bounded TTL cache or shared store with expiry and capacity controls.
Also applies to: 22-28
🤖 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/rateLimit/RateLimiter.ts` at line 15, Update the
RateLimiter bucket storage around the private buckets Map and its related
rate-limit logic to use a bounded TTL cache or shared store with explicit expiry
and capacity limits. Ensure expired one-off email/IP keys are removed and new
entries cannot grow the singleton store beyond the configured capacity, while
preserving existing rate-limit behavior.
| if (!bucket || now > bucket.resetAt) { | ||
| this.buckets.set(key, { count: 1, resetAt: now + this.windowMs }); | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the fixed window at resetAt. The strict comparison keeps the previous bucket active at the exact expiry timestamp.
apps/api/src/infrastructure/rateLimit/RateLimiter.ts#L26-L28: changenow > bucket.resetAttonow >= bucket.resetAt.apps/api/src/__tests__/infrastructure/rateLimit/RateLimiter.test.ts#L39-L47: advance exactly60_000ms to cover the boundary.
📍 Affects 2 files
apps/api/src/infrastructure/rateLimit/RateLimiter.ts#L26-L28(this comment)apps/api/src/__tests__/infrastructure/rateLimit/RateLimiter.test.ts#L39-L47
🤖 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/rateLimit/RateLimiter.ts` around lines 26 - 28,
Update RateLimiter’s bucket-expiry condition to use an inclusive resetAt
comparison, and adjust the related RateLimiter test to advance exactly 60_000 ms
so it verifies the boundary reset behavior.
| const user = await this.deps.userRepository.findByEmail(input.email); | ||
| // Silently no-op for unknown emails so this endpoint can't be used to enumerate accounts. | ||
| if (!user) return; | ||
|
|
||
| await this.deps.passwordResetTokenRepository.deleteAllForUser(user.id); | ||
|
|
||
| const rawToken = randomBytes(PASSWORD_RESET_TOKEN.RANDOM_BYTES).toString('hex'); | ||
| const tokenHash = createHash('sha256').update(rawToken).digest('hex'); | ||
| const expiresAt = new Date(Date.now() + PASSWORD_RESET_TOKEN.TTL_MS); | ||
|
|
||
| await this.deps.passwordResetTokenRepository.create({ | ||
| id: this.deps.generateId(), | ||
| userId: user.id, | ||
| tokenHash, | ||
| expiresAt, | ||
| }); | ||
|
|
||
| const resetUrl = `${this.deps.webAppOrigin}/reset-password?token=${rawToken}`; | ||
| try { | ||
| await this.deps.emailService.sendPasswordReset(user.email, resetUrl); | ||
| } catch { | ||
| // An email-provider failure must not surface differently than the | ||
| // silent no-op above for unknown emails — otherwise it becomes an | ||
| // enumeration oracle (errors only ever occur for real accounts). | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid an account-enumeration timing oracle.
Unknown accounts return after lookup, while real accounts perform token writes and await the email provider. The GraphQL mutation awaits this use case, so identical successful responses still have measurably different latency. Queue delivery outside the request path and minimise timing differences between account-present and account-absent flows.
🤖 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/auth/RequestPasswordResetUseCase.ts` around lines 40 -
64, Update RequestPasswordResetUseCase so email delivery is queued or dispatched
outside the awaited request path, while preserving the existing silent behavior
for unknown emails and provider failures. Minimize work and timing differences
between the no-user branch and the user branch, including avoiding awaited token
writes or otherwise equalizing the response path; use the existing email/queue
abstractions rather than introducing synchronous delivery.
| const tokenHash = createHash('sha256').update(input.token).digest('hex'); | ||
| const resetToken = await this.deps.passwordResetTokenRepository.findByTokenHash(tokenHash); | ||
|
|
||
| if (!resetToken || resetToken.usedAt || resetToken.expiresAt < new Date()) { | ||
| throw Object.assign(new Error('Invalid or expired reset link'), { | ||
| code: ERROR_CODES.UNAUTHORIZED, | ||
| }); | ||
| } | ||
|
|
||
| const passwordHash = await bcrypt.hash(input.newPassword, 12); | ||
| await this.deps.userRepository.update(resetToken.userId, { passwordHash }); | ||
| await this.deps.passwordResetTokenRepository.markUsed(resetToken.id); | ||
| // Invalidate every existing session so a refresh token stolen before the | ||
| // reset can't survive it — otherwise the whole point of the reset is defeated. | ||
| await this.deps.sessionRepository.revokeAllForUser(resetToken.userId); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Atomically consume the reset token.
Two concurrent requests can both pass the usedAt check, update the password, then mark the same token used. A failure after Line 39 also leaves a valid token after changing credentials. Replace the read-then-mark sequence with a conditional atomic consume (usedAt IS NULL and unexpired), and wrap token consumption, password update, and session revocation in one transaction. Add a concurrent-redemption test.
🤖 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/auth/ResetPasswordUseCase.ts` around lines 29 - 43,
The ResetPasswordUseCase currently reads and later marks the reset token,
allowing concurrent redemption and partial updates. Replace the
findByTokenHash/markUsed sequence with a conditional atomic consume requiring
usedAt IS NULL and an unexpired token, then execute token consumption, password
update, and revokeAllForUser within one transaction; preserve the unauthorized
error when consumption fails and add a test covering concurrent redemption.
| findByTokenHash(tokenHash: string): Promise<PasswordResetToken | null>; | ||
| markUsed(id: string): Promise<void>; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make password-reset token use an atomic claim. The current read-then-update flow permits concurrent requests to validate the same token and both change the password.
apps/api/src/use-cases/ports/IPasswordResetTokenRepository.ts#L10-L11: replacemarkUsedwith a conditional claim contract returning whether the unused, unexpired token was claimed.apps/api/src/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.ts#L41-L43: implement that contract with a guarded update and require exactly one updated row.apps/api/src/use-cases/auth/ResetPasswordUseCase.ts: claim before updating the password or revoking sessions.
📍 Affects 2 files
apps/api/src/use-cases/ports/IPasswordResetTokenRepository.ts#L10-L11(this comment)apps/api/src/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.ts#L41-L43
🤖 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/ports/IPasswordResetTokenRepository.ts` around lines
10 - 11, Replace markUsed in IPasswordResetTokenRepository with an atomic claim
method that returns whether the token was unused and unexpired. In
PrismaPasswordResetTokenRepository, implement the claim using a guarded update
on token ID, unused status, and expiration, and require exactly one updated row.
In ResetPasswordUseCase, perform the claim before changing the password or
revoking sessions, aborting when it fails. Apply these changes at
apps/api/src/use-cases/ports/IPasswordResetTokenRepository.ts lines 10-11 and
apps/api/src/infrastructure/db/repositories/PrismaPasswordResetTokenRepository.ts
lines 41-43; ResetPasswordUseCase requires the corresponding call-site update.
| const onSubmit = async (data: FormValues) => { | ||
| // Always resolves — the backend responds identically for known and unknown | ||
| // emails so this form can't be used to enumerate accounts. | ||
| await gqlClient.request(REQUEST_PASSWORD_RESET_MUTATION, data); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unhandled failure path leaves the user with no feedback.
onSubmit never catches errors from gqlClient.request. The backend's RequestPasswordResetUseCase legitimately throws a RATE_LIMITED error (and any transport/network failure will also reject) — this is safe to surface since rate-limiting is applied uniformly regardless of whether the account exists, so it doesn't create an enumeration oracle. As written, any such failure becomes an unhandled promise rejection, isSubmitSuccessful never becomes true, and the user sees no success or error message at all — the button just stops spinning with no indication of what happened.
🐛 Proposed fix: surface non-enumerating errors via setError
const {
register,
handleSubmit,
- formState: { errors, isSubmitting, isSubmitSuccessful },
+ formState: { errors, isSubmitting, isSubmitSuccessful },
+ setError,
} = useForm<FormValues>({
resolver: zodResolver(schema),
});
const onSubmit = async (data: FormValues) => {
// Always resolves — the backend responds identically for known and unknown
- // emails so this form can't be used to enumerate accounts.
- await gqlClient.request(REQUEST_PASSWORD_RESET_MUTATION, data);
+ // emails so this form can't be used to enumerate accounts. Rate-limit and
+ // network failures are still safe to surface since they're independent of
+ // whether the account exists.
+ try {
+ await gqlClient.request(REQUEST_PASSWORD_RESET_MUTATION, data);
+ } catch (err: unknown) {
+ setError('root', { message: extractGqlError(err) ?? 'Something went wrong. Please try again.' });
+ }
};Then render errors.root?.message in the form, mirroring reset-password.tsx's pattern, and add an extractGqlError helper (ideally shared, see related comment).
🤖 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/forgot-password.tsx` around lines 31 - 35, Update
onSubmit in the forgot-password form to catch gqlClient.request failures and
surface them through the form’s root error using setError, including
RATE_LIMITED and transport errors without exposing account existence. Render
errors.root?.message in the form, mirroring reset-password.tsx, and reuse or add
the shared extractGqlError helper for the displayed message.
1 participant
Summary
UpdatePasswordUseCaserequired knowing the current password, with no recovery optionPasswordResetTokenmodel: a hashed (sha256), single-use, 1-hour-expiring token, following the existingApiTokenhashing patternRequestPasswordResetUseCasealways resolves silently regardless of whether the email is known, to prevent account enumerationrequestPasswordReset/resetPasswordGraphQL mutations, wired throughAuthResolver/forgot-passwordand/reset-passwordweb routes, linked from/loginsendPasswordResetmethod onIEmailServiceFixes JEF-16
Test plan
pnpm typecheck(api + web)pnpm lint(api + web)pnpm test— 419 API tests + 85 web tests passing, including new coverage forRequestPasswordResetUseCase,ResetPasswordUseCase,PrismaPasswordResetTokenRepository,BrevoEmailService.sendPasswordReset, the password reset email template,AuthResolver, and the newForgotPasswordPage/ResetPasswordPagepnpm build(all packages)🤖 Generated with Claude Code
https://claude.ai/code/session_01DdEiNRTcUnM6kE5AdFQ3n8
Summary by CodeRabbit