Add password reset flow#234
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a complete password-reset feature: DB table + migration, server token creation/verification and rate-limiting, Resend-backed email delivery with dev fallback, request/confirm API routes, web reset page and Auth UI link, iOS UI/API changes, tests, and fixture updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Web as Web App
participant API as PasswordReset API
participant DB as Database
participant Mail as Resend
participant IOS as iOS App
User->>Web: Visit /reset-password (no token) submit email
Web->>API: POST /api/auth/password-reset/request { email, ipHash }
API->>DB: Lookup user by email
alt user exists
API->>DB: TX: lock user, find/create password_reset_tokens row
DB-->>API: tokenHash (server retains plaintext token to email)
API->>Mail: send password reset email with token link
Mail-->>API: delivered:true/false
else user missing
DB-->>API: no user
end
API-->>Web: { message: generic success }
Web-->>User: show generic success message
User->>Web: Visit /reset-password?token=... submit new password
Web->>API: POST /api/auth/password-reset/confirm { token, password }
API->>DB: TX: consume token row (usedAt null & not expired) and update users.passwordHash
DB-->>API: rowsUpdated (1) / 0
alt rowsUpdated == 1
API-->>Web: { ok: true }
else
API-->>Web: 400 invalid/expired token
end
Web-->>User: show success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 6 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f46cb87. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/middleware.ts (1)
10-24:⚠️ Potential issue | 🟠 MajorOver-broad prefix allowlist can unintentionally bypass auth.
Line 23 uses
startsWith, so adding"/api/auth/password-reset"at Line 10 also whitelists any path beginning with that prefix (not just/requestand/confirm). Prefer explicit endpoint allowlisting here.Safer route matching
const publicPaths = [ "/api/auth/signup", "/api/auth/login", "/api/auth/logout", - "/api/auth/password-reset", "/api/board", ]; +const publicExactPaths = new Set([ + "/api/auth/password-reset/request", + "/api/auth/password-reset/confirm", +]); // Allow public routes -if (publicPaths.some((p) => pathname.startsWith(p))) { +if (publicExactPaths.has(pathname) || publicPaths.some((p) => pathname.startsWith(p))) { return NextResponse.next(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware.ts` around lines 10 - 24, The allowlist in publicPaths plus the middleware check in middleware(...) uses pathname.startsWith(p) which over-broadly matches prefixes (e.g., "/api/auth/password-reset" will match "/api/auth/password-reset-extra"); change the matching to exact-path checks (or a Set/includes) or use anchored regexes so only exact endpoints are allowed: update the publicPaths handling in middleware to compare pathname === p (or test ^p$) for each entry in publicPaths instead of startsWith, and ensure publicPaths entries list the specific endpoints (e.g., "/api/auth/password-reset/request" and "/api/auth/password-reset/confirm") so only those exact routes bypass auth.ios/StillPointApp/ViewModels/AuthViewModel.swift (1)
5-75:⚠️ Potential issue | 🟠 MajorAdd
@MainActortoAuthViewModeland use the API-returned message.The class mutates observable properties (
isSubmitting,error,resetMessage,isRequestingPasswordReset) across async boundaries insubmit()andrequestPasswordReset(). Mark the class as@MainActorto ensure explicit main-thread isolation for these mutations:`@Observable` `@MainActor` final class AuthViewModel { // ... }Additionally,
requestPasswordReset()should use the message returned by the API instead of hardcoding a response. TheAPIClient.requestPasswordReset(email:)method already returns aString, so assign it toresetMessage:try await APIClient.shared.requestPasswordReset(email: trimmedEmail) resetMessage = try await APIClient.shared.requestPasswordReset(email: trimmedEmail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/StillPointApp/ViewModels/AuthViewModel.swift` around lines 5 - 75, Mark AuthViewModel as `@MainActor` to ensure main-thread isolation for properties mutated across async boundaries (email, isSubmitting, error, resetMessage, isRequestingPasswordReset), e.g. add `@MainActor` above the final class AuthViewModel declaration; and in requestPasswordReset() capture and assign the API-returned message to resetMessage by calling APIClient.shared.requestPasswordReset(email: trimmedEmail) once and storing its returned String instead of using the hardcoded message (refer to the requestPasswordReset() method and APIClient.shared.requestPasswordReset).
🧹 Nitpick comments (5)
.env.example (1)
18-21:EMAIL_PROVIDERis documented but not used by the runtime.Line 19 implies provider selection, but current server code (
src/lib/email.ts) always targets Resend and only gates onEMAIL_FROM+RESEND_API_KEY. Consider either implementing provider switching or removing/clarifyingEMAIL_PROVIDERto avoid misleading configuration.Suggested doc-only alignment
-# Optional email provider config for password reset delivery. If unset, reset links are logged server-side in non-production only. -# EMAIL_PROVIDER=console +# Optional email config for password reset delivery (Resend currently implemented). +# If EMAIL_FROM or RESEND_API_KEY is unset, reset links are logged server-side in non-production only. +# EMAIL_PROVIDER=resend🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 18 - 21, The .env.example documents EMAIL_PROVIDER but the runtime in src/lib/email.ts ignores it and always uses Resend (gated only by EMAIL_FROM and RESEND_API_KEY); either remove/clarify EMAIL_PROVIDER from the example or make src/lib/email.ts honor it. Fix by updating src/lib/email.ts to read process.env.EMAIL_PROVIDER and branch the sending logic (e.g., support 'resend' and 'console' modes), wiring the existing Resend client initialization (RESEND_API_KEY, EMAIL_FROM) when EMAIL_PROVIDER === 'resend' and falling back to the current console/log behavior when EMAIL_PROVIDER === 'console' (update the sendEmail/sendPasswordResetEmail functions to consult EMAIL_PROVIDER). Ensure env vars EMAIL_FROM and RESEND_API_KEY remain used for Resend mode and update .env.example accordingly if you choose the doc-only path.ios/StillPointApp/Views/AuthView.swift (1)
101-121: Consider scoping reset confirmation text to login mode only.Right now the message at Lines 101-107 can persist even after switching to sign-up. Limiting it to login mode avoids cross-flow UI noise.
Small conditional tweak
- if let resetMessage = vm.resetMessage { + if !vm.isSignUp, let resetMessage = vm.resetMessage { Text(resetMessage) .font(SPFont.mono(12)) .foregroundStyle(SPColor.greenText) .multilineTextAlignment(.center) .accessibilityIdentifier("auth.passwordResetMessage") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/StillPointApp/Views/AuthView.swift` around lines 101 - 121, The reset confirmation Text currently shown by checking vm.resetMessage can persist when the view switches to sign-up; restrict it to login mode by rendering the Text only when vm.resetMessage is non-nil AND vm.isSignUp is false (or move the entire vm.resetMessage Text block inside the existing if !vm.isSignUp block). Update the view so the UI uses the combined condition (e.g., vm.resetMessage != nil && !vm.isSignUp) to avoid showing resetMessage during sign-up flows and keep the Button and its disabled/label behavior unchanged.src/db/schema.ts (1)
34-42: Avoid duplicate uniqueness definitions ontokenHash.Line 34 already marks
tokenHashunique, and Line 41 adds another unique index on the same column. Keep one to avoid redundant constraints/index overhead.Suggested fix
- tokenHash: varchar("token_hash", { length: 64 }).unique().notNull(), + tokenHash: varchar("token_hash", { length: 64 }).notNull(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/schema.ts` around lines 34 - 42, The tokenHash column is declared unique via tokenHash: varchar("token_hash", { length: 64 }).unique().notNull(), and you also create a uniqueIndex("password_reset_tokens_token_hash_unique").on(table.tokenHash) — remove the redundant unique index definition (the uniqueIndex call) so there is only one uniqueness constraint on tokenHash; keep the column-level .unique() on tokenHash (or if you prefer index-style uniqueness, remove .unique() and keep the uniqueIndex) and update the table indexes block to omit the duplicate uniqueIndex reference.ios/StillPointApp/ViewModels/AuthViewModel.swift (1)
68-70: Use the API’s returned reset message instead of hardcoding.Line 68-70 discards
APIClient.shared.requestPasswordReset(email:)’s returned message and duplicates the copy locally. Prefer using the returned string as the single source of truth.Suggested fix
- try await APIClient.shared.requestPasswordReset(email: trimmedEmail) - resetMessage = "If an account exists for that email, a reset link will arrive shortly." + resetMessage = try await APIClient.shared.requestPasswordReset(email: trimmedEmail)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/StillPointApp/ViewModels/AuthViewModel.swift` around lines 68 - 70, The reset handler in AuthViewModel currently ignores the string returned by APIClient.shared.requestPasswordReset(email:) and instead assigns a hardcoded message to resetMessage; change it to capture and assign the API's returned string (e.g., let message = try await APIClient.shared.requestPasswordReset(email: trimmedEmail); resetMessage = message) so resetMessage uses the API response as the single source of truth and remove the duplicate hardcoded text.src/lib/passwordReset.ts (1)
118-144: Consider periodic cleanup of expired rate-limit entries.The in-memory Map grows unbounded since expired entries are never removed—they're only replaced when the same key is reused. For long-running processes with diverse email/IP combinations, this could accumulate stale entries.
Given the PR notes recommend WAF throttling for production, this is acceptable as a lightweight safeguard, but adding periodic cleanup would improve memory hygiene.
♻️ Optional: Add lazy cleanup of expired entries
function attemptsStore() { if (!globalForPasswordReset.__passwordResetAttempts) { globalForPasswordReset.__passwordResetAttempts = new Map(); } return globalForPasswordReset.__passwordResetAttempts; } +function cleanupExpiredAttempts() { + const now = Date.now(); + const store = attemptsStore(); + for (const [key, attempt] of store) { + if (attempt.resetAt <= now) { + store.delete(key); + } + } +} export function isPasswordResetRateLimited(email: string, ipHash: string | null) { const now = Date.now(); + // Opportunistic cleanup on read + if (Math.random() < 0.01) cleanupExpiredAttempts(); const attempt = attemptsStore().get(rateLimitKey(email, ipHash)); return Boolean(attempt && attempt.resetAt > now && attempt.count >= RATE_LIMIT_MAX_ATTEMPTS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/passwordReset.ts` around lines 118 - 144, The attempts Map in attemptsStore can grow unbounded because expired entries are never removed; add periodic cleanup to prune entries whose resetAt <= Date.now() (e.g., in recordPasswordResetAttempt and/or isPasswordResetRateLimited) by scanning attemptsStore() and deleting expired keys, but do it lazily and cheaply (run a full scan only occasionally — e.g., probabilistically with Math.random() or every N calls tracked by a small counter) so you avoid CPU spikes; reference attemptsStore(), recordPasswordResetAttempt(), isPasswordResetRateLimited(), and RATE_LIMIT_WINDOW_MS when implementing the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/password_reset_tokens_incremental.sql`:
- Around line 15-16: The ALTER TABLE ADD CONSTRAINT line for
password_reset_tokens_user_id_users_id_fk is not idempotent and will error if
the constraint already exists; replace it with an existence-guarded creation
(e.g. a small DO $$ BEGIN ... END $$; block or a conditional SELECT against
pg_constraint/pg_class/pg_namespace) that checks for the constraint name
"password_reset_tokens_user_id_users_id_fk" on table "password_reset_tokens" and
only executes the ADD CONSTRAINT ... FOREIGN KEY ("user_id") REFERENCES
"public"."users"("id") ON DELETE cascade ON UPDATE no action when absent,
preserving the exact constraint definition.
In `@e2e/fixtures/auth.fixture.ts`:
- Around line 170-174: The mock for POST "/api/auth/password-reset/confirm"
conflates short-password validation with token expiry; change the branching in
the handler (where pathname, method, state.resetToken and body are used) so that
if body.password is present but shorter than 8 you return a validation error
response (match the real endpoint's status/code and error shape) instead of the
generic "Reset link is invalid or expired", and only return the generic
token/expiry error when body.token does not match state.resetToken or
state.resetToken is missing; keep uses of json(route, ...) but adjust the status
and payload for the validation branch to mirror the real API.
In `@README.md`:
- Around line 86-88: The README and runtime disagree: the code in
src/lib/email.ts currently enables Resend based only on EMAIL_FROM and
RESEND_API_KEY, but the docs say EMAIL_PROVIDER=resend should be the switch;
update src/lib/email.ts to require EMAIL_PROVIDER === 'resend' (in addition to
validating EMAIL_FROM and RESEND_API_KEY) before enabling the Resend path (the
function/initializer that constructs the mail client or sendPasswordResetEmail
logic), or alternatively update the README to state that both EMAIL_FROM and
RESEND_API_KEY are required and that EMAIL_PROVIDER is not used; pick one
approach and make the change consistently and update comments/validation
messages accordingly.
In `@src/app/api/auth/password-reset/confirm/route.ts`:
- Around line 10-19: Wrap the call to await request.json() in a try/catch and
treat JSON parsing errors as a 400 client error: change the code around the
body/token/password extraction so that if request.json() throws (e.g., a
SyntaxError or other parse error) you return NextResponse.json({ error:
"Malformed JSON" }, { status: 400 }) instead of letting the outer catch convert
it to a 500; keep the existing validation for token and password (the token
trimming and password length check) after successful parsing.
In `@src/app/api/auth/password-reset/request/route.ts`:
- Around line 45-56: The current read-before-write check using recentCutoff and
activeToken against the passwordResetTokens table is race-prone: concurrent
requests can both see no active token and both insert new ones. Fix by making
issuance atomic for a given user: wrap the logic in a transaction and acquire a
per-user serialization point (e.g., SELECT FOR UPDATE on the user's row or an
advisory lock keyed by user.id) or atomically update existing tokens (set usedAt
or revoked) and then insert the new token in the same transaction; ensure you
reference passwordResetTokens.createdAt / usedAt and user.id when performing the
lock/update so no two transactions can both succeed.
- Around line 69-74: The issue: throwing from sendPasswordResetEmail makes
requests for existing emails return 500 while non-existing emails return the
normal non-enumeration message; fix by preserving the same response shape for
both cases. Wrap the call to sendPasswordResetEmail (in the route handler where
PASSWORD_RESET_REQUEST_MESSAGE is returned) in its own try/catch, log the
provider/send error (console.error or processLogger) but do not change the HTTP
response—always return NextResponse.json({ message:
PASSWORD_RESET_REQUEST_MESSAGE }) for both non-existing emails and send
failures; keep the outer catch for truly unexpected errors only.
In `@src/app/reset-password/page.tsx`:
- Around line 158-213: Wrap the inputs and button in a semantic <form> so the
flow uses form submission rather than onClick on a div; make the submit button
type="submit" and move the current onClick logic (calling submit or
requestReset) into a form onSubmit handler that calls event.preventDefault()
then calls submit() or requestReset() based on token. Add visible <label>
elements with htmlFor and matching id attributes for the email, password and
confirmPassword inputs (use the password, confirmPassword and email state
bindings), and keep the existing status/message rendering but add aria-live
(e.g., aria-live="polite" or "assertive" depending on status) to the message
container so screen readers are notified; preserve styling via
inputStyle/buttonStyle and keep disabling logic using status === "submitting".
In `@src/components/AuthScreen.tsx`:
- Around line 53-55: The resetHref variable in AuthScreen.tsx currently appends
the user's email to the query string (resetHref); remove the email from the URL
and always navigate to "/reset-password" to avoid leaking PII. Replace
occurrences that build `/reset-password?email=...` with a plain
"/reset-password", and instead pass the email securely (e.g., via in-memory
state/context, navigation state, or a POST/secure store) to prefill the reset
form; update any consumers of resetHref and the reset form to read email from
the chosen secure mechanism rather than the query string.
In `@src/lib/email.ts`:
- Around line 22-35: The fetch call that posts to
"https://api.resend.com/emails" must be wrapped with an AbortController-based
timeout: create an AbortController, pass controller.signal into fetch, start a
setTimeout that calls controller.abort() after a bounded period (e.g. 5–15s),
and clearTimeout once fetch returns; update the surrounding code that awaits the
response (where response is created and uses resendApiKey, fromAddress, to,
subject, text, html) to handle the abort/fetch rejection cleanly (propagate or
translate the error and ensure resources/timeouts are cleared).
- Around line 17-20: The current check using fromAddress and resendApiKey (in
the email sending flow) silently returns { delivered: false } in all
environments; change it so that when either fromAddress or resendApiKey is
missing and process.env.NODE_ENV === 'production' the code throws a clear Error
(e.g., "Email delivery not configured: EMAIL_FROM and RESEND_API_KEY required")
to fail fast in prod, while retaining the console.info + return { delivered:
false } behavior for non-production environments; update the conditional around
fromAddress and resendApiKey in the function that performs sending (referencing
those variables) to implement this branching.
---
Outside diff comments:
In `@ios/StillPointApp/ViewModels/AuthViewModel.swift`:
- Around line 5-75: Mark AuthViewModel as `@MainActor` to ensure main-thread
isolation for properties mutated across async boundaries (email, isSubmitting,
error, resetMessage, isRequestingPasswordReset), e.g. add `@MainActor` above the
final class AuthViewModel declaration; and in requestPasswordReset() capture and
assign the API-returned message to resetMessage by calling
APIClient.shared.requestPasswordReset(email: trimmedEmail) once and storing its
returned String instead of using the hardcoded message (refer to the
requestPasswordReset() method and APIClient.shared.requestPasswordReset).
In `@src/middleware.ts`:
- Around line 10-24: The allowlist in publicPaths plus the middleware check in
middleware(...) uses pathname.startsWith(p) which over-broadly matches prefixes
(e.g., "/api/auth/password-reset" will match "/api/auth/password-reset-extra");
change the matching to exact-path checks (or a Set/includes) or use anchored
regexes so only exact endpoints are allowed: update the publicPaths handling in
middleware to compare pathname === p (or test ^p$) for each entry in publicPaths
instead of startsWith, and ensure publicPaths entries list the specific
endpoints (e.g., "/api/auth/password-reset/request" and
"/api/auth/password-reset/confirm") so only those exact routes bypass auth.
---
Nitpick comments:
In @.env.example:
- Around line 18-21: The .env.example documents EMAIL_PROVIDER but the runtime
in src/lib/email.ts ignores it and always uses Resend (gated only by EMAIL_FROM
and RESEND_API_KEY); either remove/clarify EMAIL_PROVIDER from the example or
make src/lib/email.ts honor it. Fix by updating src/lib/email.ts to read
process.env.EMAIL_PROVIDER and branch the sending logic (e.g., support 'resend'
and 'console' modes), wiring the existing Resend client initialization
(RESEND_API_KEY, EMAIL_FROM) when EMAIL_PROVIDER === 'resend' and falling back
to the current console/log behavior when EMAIL_PROVIDER === 'console' (update
the sendEmail/sendPasswordResetEmail functions to consult EMAIL_PROVIDER).
Ensure env vars EMAIL_FROM and RESEND_API_KEY remain used for Resend mode and
update .env.example accordingly if you choose the doc-only path.
In `@ios/StillPointApp/ViewModels/AuthViewModel.swift`:
- Around line 68-70: The reset handler in AuthViewModel currently ignores the
string returned by APIClient.shared.requestPasswordReset(email:) and instead
assigns a hardcoded message to resetMessage; change it to capture and assign the
API's returned string (e.g., let message = try await
APIClient.shared.requestPasswordReset(email: trimmedEmail); resetMessage =
message) so resetMessage uses the API response as the single source of truth and
remove the duplicate hardcoded text.
In `@ios/StillPointApp/Views/AuthView.swift`:
- Around line 101-121: The reset confirmation Text currently shown by checking
vm.resetMessage can persist when the view switches to sign-up; restrict it to
login mode by rendering the Text only when vm.resetMessage is non-nil AND
vm.isSignUp is false (or move the entire vm.resetMessage Text block inside the
existing if !vm.isSignUp block). Update the view so the UI uses the combined
condition (e.g., vm.resetMessage != nil && !vm.isSignUp) to avoid showing
resetMessage during sign-up flows and keep the Button and its disabled/label
behavior unchanged.
In `@src/db/schema.ts`:
- Around line 34-42: The tokenHash column is declared unique via tokenHash:
varchar("token_hash", { length: 64 }).unique().notNull(), and you also create a
uniqueIndex("password_reset_tokens_token_hash_unique").on(table.tokenHash) —
remove the redundant unique index definition (the uniqueIndex call) so there is
only one uniqueness constraint on tokenHash; keep the column-level .unique() on
tokenHash (or if you prefer index-style uniqueness, remove .unique() and keep
the uniqueIndex) and update the table indexes block to omit the duplicate
uniqueIndex reference.
In `@src/lib/passwordReset.ts`:
- Around line 118-144: The attempts Map in attemptsStore can grow unbounded
because expired entries are never removed; add periodic cleanup to prune entries
whose resetAt <= Date.now() (e.g., in recordPasswordResetAttempt and/or
isPasswordResetRateLimited) by scanning attemptsStore() and deleting expired
keys, but do it lazily and cheaply (run a full scan only occasionally — e.g.,
probabilistically with Math.random() or every N calls tracked by a small
counter) so you avoid CPU spikes; reference attemptsStore(),
recordPasswordResetAttempt(), isPasswordResetRateLimited(), and
RATE_LIMIT_WINDOW_MS when implementing the cleanup.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7736e785-ee37-4bee-a9a0-743bd3497feb
📒 Files selected for processing (18)
.env.exampleREADME.mddrizzle/password_reset_tokens_incremental.sqle2e/auth/password-reset.spec.tse2e/fixtures/auth.fixture.tsios/StillPointApp/ViewModels/AuthViewModel.swiftios/StillPointApp/Views/AuthView.swiftios/StillPointAppUITests/StillPointAppUITests.swiftios/StillPointShared/Sources/StillPointShared/APIClient.swiftios/StillPointShared/Sources/StillPointShared/DTOs/DTOs.swiftsrc/app/api/auth/password-reset/confirm/route.tssrc/app/api/auth/password-reset/request/route.tssrc/app/reset-password/page.tsxsrc/components/AuthScreen.tsxsrc/db/schema.tssrc/lib/email.tssrc/lib/passwordReset.tssrc/middleware.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/password-reset-ad4b
branch
from
3064513 to
a989afc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/app/reset-password/page.tsx (1)
43-74: Consider clearing the email field on successful request.After a successful reset link request, the email field retains its value. For UX consistency with the confirm flow (which clears password fields on success at lines 111-112), consider clearing the email field when
status === "success".Optional enhancement
setStatus("success"); + setEmail(""); setMessage(data.message || "If an account exists for that email, a reset link will arrive shortly.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reset-password/page.tsx` around lines 43 - 74, After a successful password-reset request, clear the email input for UX consistency: in the requestReset function, after you call setStatus("success") and setMessage(...), also call setEmail("") (or the state setter managing the email input) to reset the field; ensure setEmail is in scope (the same state used to populate the email variable) before adding this call.src/lib/passwordReset.ts (2)
107-144: Rate limiting is process-local; document or mitigate production limitations.The in-memory rate limiter (using
globalThis) won't share state across serverless instances. The README (line 97) already mentions this limitation and recommends WAF throttling for production. This is acceptable given the documentation, but consider adding a code comment here for maintainability.Add inline documentation
+/** + * In-memory rate limiting for password reset requests. + * NOTE: This is per-process and does not share state across serverless instances. + * Production deployments should rely on platform/WAF rate limiting for stronger protection. + */ const RATE_LIMIT_WINDOW_MS = 15 * 60 * 1000; const RATE_LIMIT_MAX_ATTEMPTS = 5;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/passwordReset.ts` around lines 107 - 144, Add an inline comment near the in-memory rate limiter definitions (globalForPasswordReset, attemptsStore, RATE_LIMIT_WINDOW_MS, RATE_LIMIT_MAX_ATTEMPTS) explaining that the rate limiting is process-local and will not be shared across serverless instances or multiple servers; mention that isPasswordResetRateLimited and recordPasswordResetAttempt rely on this process-local Map and recommend using a centralized store (Redis, DB, or WAF throttling) for production to ensure global rate limiting and to avoid relying solely on this in-memory mechanism.
81-105:consumePasswordResetTokenis unused - extract token consumption logic to avoid duplication.The helper function at lines 81-105 wraps the token consumption pattern, but
src/app/api/auth/password-reset/confirm/route.ts(lines 34-45) duplicates this logic inline. Refactor the confirm route to call this helper and centralize the token consumption logic:Refactor confirm route to use consumePasswordResetToken
In
src/app/api/auth/password-reset/confirm/route.ts:const passwordHash = await hashPassword(password); const updated = await poolDb.transaction(async (tx) => { - const [resetToken] = await tx - .update(passwordResetTokens) - .set({ usedAt: new Date() }) - .where( - and( - eq(passwordResetTokens.userId, resetPayload.userId), - eq(passwordResetTokens.tokenHash, resetPayload.tokenHash), - isNull(passwordResetTokens.usedAt), - gt(passwordResetTokens.expiresAt, new Date()), - ), - ) - .returning({ userId: passwordResetTokens.userId }); - if (!resetToken) { + const result = await consumePasswordResetToken(tx, token); + if (!result.ok) { return false; } await tx.update(users) .set({ passwordHash, updatedAt: new Date() }) - .where(eq(users.id, resetToken.userId)); + .where(eq(users.id, result.userId)); return true; });Don't forget to import
consumePasswordResetTokenfrom@/lib/passwordReset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/passwordReset.ts` around lines 81 - 105, The password reset token consumption logic is duplicated in the confirm route; replace the inline logic with a call to the existing helper consumePasswordResetToken(tx, token): import consumePasswordResetToken from the passwordReset module, pass the same transactional db instance (tx) and the provided token, then use its returned shape ({ ok, userId }) to handle success/failure exactly as the inline code did (return the same error responses when ok is false and continue flow when ok is true). Ensure you remove the duplicated update/where block from the confirm route and rely solely on consumePasswordResetToken for marking tokens as used and retrieving userId.src/lib/email.ts (1)
50-63: Consider HTML-escaping the link in the email body.The
linkvariable is constructed from user-controlledtokenand embedded directly into HTML at line 59. While the token is base64url-encoded JWT (safe characters), future changes could introduce XSS risk in email clients. HTML-escaping the href is a defensive measure.Defensive escaping suggestion
+function escapeHtml(str: string) { + return str.replace(/&/g, "&").replace(/"/g, """).replace(/</g, "<").replace(/>/g, ">"); +} export async function sendPasswordResetEmail({ to, token }: { to: string; token: string }) { const resetUrl = new URL("/reset-password", appUrl); resetUrl.searchParams.set("token", token); const link = resetUrl.toString(); + const safeLink = escapeHtml(link); const result = await sendEmail({ to, subject: "Reset your Still Point password", text: `Use this link to reset your Still Point password. It expires in ${PASSWORD_RESET_TTL_MINUTES} minutes:\n\n${link}\n\nIf you did not request this, you can ignore this email.`, - html: `<p>Use this link to reset your Still Point password. It expires in ${PASSWORD_RESET_TTL_MINUTES} minutes:</p><p><a href="${link}">Reset your password</a></p><p>If you did not request this, you can ignore this email.</p>`, + html: `<p>Use this link to reset your Still Point password. It expires in ${PASSWORD_RESET_TTL_MINUTES} minutes:</p><p><a href="${safeLink}">Reset your password</a></p><p>If you did not request this, you can ignore this email.</p>`, devLogMessage: `Password reset link for local development: ${link}`, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/email.ts` around lines 50 - 63, The HTML email embeds the user-controlled link directly in sendPasswordResetEmail (variable link) which could allow XSS if token handling changes; update the function to HTML-escape the link before inserting it into the html template (escape characters like &, <, >, ", and ') by using an existing escapeHtml helper or adding one, then use the escaped value in the <a href="..."> and any other HTML contexts (keep the text/plain body unchanged). Ensure you reference sendPasswordResetEmail, link, and the sendEmail call when making the change so the escaped value is passed into sendEmail's html field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/reset-password/page.tsx`:
- Around line 43-74: After a successful password-reset request, clear the email
input for UX consistency: in the requestReset function, after you call
setStatus("success") and setMessage(...), also call setEmail("") (or the state
setter managing the email input) to reset the field; ensure setEmail is in scope
(the same state used to populate the email variable) before adding this call.
In `@src/lib/email.ts`:
- Around line 50-63: The HTML email embeds the user-controlled link directly in
sendPasswordResetEmail (variable link) which could allow XSS if token handling
changes; update the function to HTML-escape the link before inserting it into
the html template (escape characters like &, <, >, ", and ') by using an
existing escapeHtml helper or adding one, then use the escaped value in the <a
href="..."> and any other HTML contexts (keep the text/plain body unchanged).
Ensure you reference sendPasswordResetEmail, link, and the sendEmail call when
making the change so the escaped value is passed into sendEmail's html field.
In `@src/lib/passwordReset.ts`:
- Around line 107-144: Add an inline comment near the in-memory rate limiter
definitions (globalForPasswordReset, attemptsStore, RATE_LIMIT_WINDOW_MS,
RATE_LIMIT_MAX_ATTEMPTS) explaining that the rate limiting is process-local and
will not be shared across serverless instances or multiple servers; mention that
isPasswordResetRateLimited and recordPasswordResetAttempt rely on this
process-local Map and recommend using a centralized store (Redis, DB, or WAF
throttling) for production to ensure global rate limiting and to avoid relying
solely on this in-memory mechanism.
- Around line 81-105: The password reset token consumption logic is duplicated
in the confirm route; replace the inline logic with a call to the existing
helper consumePasswordResetToken(tx, token): import consumePasswordResetToken
from the passwordReset module, pass the same transactional db instance (tx) and
the provided token, then use its returned shape ({ ok, userId }) to handle
success/failure exactly as the inline code did (return the same error responses
when ok is false and continue flow when ok is true). Ensure you remove the
duplicated update/where block from the confirm route and rely solely on
consumePasswordResetToken for marking tokens as used and retrieving userId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 366e9156-154e-426f-92fe-54c4668fedaf
📒 Files selected for processing (18)
.env.exampleREADME.mddrizzle/password_reset_tokens_incremental.sqle2e/auth/password-reset.spec.tse2e/fixtures/auth.fixture.tsios/StillPointApp/ViewModels/AuthViewModel.swiftios/StillPointApp/Views/AuthView.swiftios/StillPointAppUITests/StillPointAppUITests.swiftios/StillPointShared/Sources/StillPointShared/APIClient.swiftios/StillPointShared/Sources/StillPointShared/DTOs/DTOs.swiftsrc/app/api/auth/password-reset/confirm/route.tssrc/app/api/auth/password-reset/request/route.tssrc/app/reset-password/page.tsxsrc/components/AuthScreen.tsxsrc/db/schema.tssrc/lib/email.tssrc/lib/passwordReset.tssrc/middleware.ts
✅ Files skipped from review due to trivial changes (4)
- src/middleware.ts
- ios/StillPointShared/Sources/StillPointShared/DTOs/DTOs.swift
- .env.example
- src/db/schema.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/components/AuthScreen.tsx
- ios/StillPointApp/Views/AuthView.swift
- ios/StillPointShared/Sources/StillPointShared/APIClient.swift
- drizzle/password_reset_tokens_incremental.sql
- ios/StillPointApp/ViewModels/AuthViewModel.swift
- ios/StillPointAppUITests/StillPointAppUITests.swift
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/auth/password-reset.spec.ts`:
- Around line 22-27: The mock route in password-reset.spec.ts conflates token
expiration and password validation errors; update the handler that currently
checks body.token !== "valid-reset-token" || !body.password ||
body.password.length < 8 so that token failures still return the existing "Reset
link is invalid or expired" response (via route.fulfill) but short/malformed
passwords return the validation-specific error payload the real API returns
(e.g., a 4xx validation status and a JSON body indicating a password field
error). Locate the conditional in the test mock and split it into two branches:
one that checks token validity and one that checks password presence/length and
returns the validation error accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5a5d4a9-158e-45ef-b55d-f8067a0280a6
📒 Files selected for processing (2)
e2e/auth/password-reset.spec.tssrc/app/reset-password/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/reset-password/page.tsx
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/app/reset-password/page.tsx (1)
169-197: Extract repeated visually-hidden label style into one constant.The same inline hidden-label style is repeated three times; centralizing it will reduce duplication and keep future a11y tweaks consistent.
♻️ Suggested refactor
+const visuallyHiddenLabelStyle: React.CSSProperties = { + position: "absolute", + width: 1, + height: 1, + overflow: "hidden", +}; ... -<label htmlFor="new-password" style={{ position: "absolute", width: 1, height: 1, overflow: "hidden" }}> +<label htmlFor="new-password" style={visuallyHiddenLabelStyle}> ... -<label htmlFor="confirm-new-password" style={{ position: "absolute", width: 1, height: 1, overflow: "hidden" }}> +<label htmlFor="confirm-new-password" style={visuallyHiddenLabelStyle}> ... -<label htmlFor="reset-email" style={{ position: "absolute", width: 1, height: 1, overflow: "hidden" }}> +<label htmlFor="reset-email" style={visuallyHiddenLabelStyle}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/reset-password/page.tsx` around lines 169 - 197, Create a single reusable constant (e.g., hiddenLabelStyle) for the visually-hidden label style and replace the three inline label style objects with that constant; add the constant near the top of the Reset Password component in src/app/reset-password/page.tsx and update the labels with htmlFor values "new-password", "confirm-new-password", and "reset-email" to use hiddenLabelStyle so the style is centralized and no inline duplicates remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/passwordReset.ts`:
- Around line 48-57: The JWT created by createPasswordResetToken currently
includes email (PII) in its payload; remove email from the SignJWT payload so it
only embeds nonce (and any non-PII needed) and adjust the verification flow
(verifyPasswordResetToken / the code that reads the JWT) to stop expecting
email; then update the caller in the password-reset request route to call
createPasswordResetToken with only userId (remove passing email). Ensure all
references to the JWT payload within createPasswordResetToken and the
corresponding verify/consume function use only the nonce/userId and do not rely
on email.
- Around line 123-149: The attempts Map created by attemptsStore() never prunes
expired entries, causing unbounded memory growth; update the logic to remove
stale entries by either (a) adding a periodic cleanup (setInterval) that
iterates attemptsStore() and deletes entries with resetAt <= Date.now(), or (b)
pruning on access inside recordPasswordResetAttempt() and
isPasswordResetRateLimited() by scanning and deleting expired keys before
reading/updating; use RATE_LIMIT_WINDOW_MS/resetAt to determine expiry and keep
the existing
rateLimitKey()/attemptsStore()/recordPasswordResetAttempt()/isPasswordResetRateLimited()
functions as the hook points for this cleanup so the Map stays bounded.
---
Nitpick comments:
In `@src/app/reset-password/page.tsx`:
- Around line 169-197: Create a single reusable constant (e.g.,
hiddenLabelStyle) for the visually-hidden label style and replace the three
inline label style objects with that constant; add the constant near the top of
the Reset Password component in src/app/reset-password/page.tsx and update the
labels with htmlFor values "new-password", "confirm-new-password", and
"reset-email" to use hiddenLabelStyle so the style is centralized and no inline
duplicates remain.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5e5ee83-fed2-45ea-85ba-e1bf1a7549bb
📒 Files selected for processing (4)
e2e/auth/password-reset.spec.tssrc/app/reset-password/page.tsxsrc/lib/email.tssrc/lib/passwordReset.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/auth/password-reset.spec.ts
Co-authored-by: Bretton Auerbach <auerbachb@users.noreply.github.com>
This was referenced Apr 27, 2026
This was referenced Apr 27, 2026
5 tasks
This was referenced May 1, 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.
Summary
mainand address all visible Bugbot and CodeRabbit comments, including latest follow-up feedback.Review comments addressed
Walkthrough
password_reset_web_flow.mp4
Reset request page with prefilled email
Set new password page
Testing
npm run buildpasses.npx tsc --noEmitpasses.PORT=3012 npx playwright test e2e/auth/password-reset.spec.tspasses across mobile Chromium and WebKit iPhone projects.npm run test:unitcurrently fails in pre-existing rebasedsrc/lib/thoughtSaving.test.tsbecause it usesnode:testunder the new Vitest runner; unrelated to this password reset PR.Notes
.env.exampledocuments placeholders only.swift testcould not run because Swift is not installed on this Linux image; GitHub iOS workflows will rerun on push.To show artifacts inline, enable in settings.
Summary by CodeRabbit
New Features
Tests
Documentation