Fix/auth#2425
Conversation
…P flow - Add passwordResetService: generates OTP, stores in verification table (15-min TTL), sends via Resend, verifies with timing-safe compare, updates credential account password (falls back to users.passwordHash for legacy users) - Add POST /api/password-reset/request and /verify routes - Wire forgotPassword/resetPassword in useAuthActions to custom endpoints - Update OTP screen from 5 to 6 characters (NIST SP 800-63B) - Update ResetPasswordRequestSchema code length to 6
…h banner getAccessToken now reads the expoClient cookie JSON from SecureStore directly instead of calling authClient.getSession() on every API request, eliminating concurrent network calls from Legend State polling intervals. onNeedsReauth now verifies the session is genuinely gone before setting needsReauthAtom, so transient 401s no longer trigger the sync-paused banner.
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
Coverage Report for API Unit Tests Coverage (./packages/api)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR implements end-to-end email OTP password reset across Expo and backend services. It updates the OTP input to 6 characters, adds a backend service with OTP generation and verification logic, creates two unauthenticated API routes for password reset requests and verification, and integrates the frontend to call these endpoints. It also optimizes the packrat API client to avoid repeated session-token fetches. ChangesEmail OTP Password Reset Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/expo/app/auth/one-time-password.tsx (1)
284-288:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix next-focus logic to account for the current field index.
Lines 284-288 use only
text.length, so partial pastes can dismiss the keyboard early and last-field typing can attempt focus on a non-existent next field.Suggested fix
- if (text.length < NUM_OF_CODE_CHARACTERS - 1) { - KeyboardController.setFocusTo('next'); - } else { - Keyboard.dismiss(); - } + const nextIndex = index + text.length; + if (nextIndex < NUM_OF_CODE_CHARACTERS) { + KeyboardController.setFocusTo('next'); + } else { + Keyboard.dismiss(); + }🤖 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/expo/app/auth/one-time-password.tsx` around lines 284 - 288, The next-focus logic wrongly uses only text.length; update the block around KeyboardController.setFocusTo to consider the current field index (e.g., the component's index/activeIndex) and NUM_OF_CODE_CHARACTERS so you only call KeyboardController.setFocusTo for a valid next field. Specifically, compute targetIndex = currentIndex + text.length (or +1 for single-char input), and if targetIndex < NUM_OF_CODE_CHARACTERS call KeyboardController.setFocusTo(targetIndex) (or the appropriate key name), otherwise call Keyboard.dismiss() and do not attempt to focus a non-existent field; also ensure partial pastes advance focus by the number of characters pasted rather than dismissing early.
🤖 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/expo/app/auth/index.tsx`:
- Line 122: The decorative Apple glyph Text element is being announced by screen
readers; mark that Text as non-accessible so only the button label is read. In
the Text component rendering the glyph (the Text with content ''), add the
accessibility props to hide it from assistive tech (e.g., accessible={false}
and/or accessibilityElementsHidden={true} and importantForAccessibility="no") so
the glyph remains visual-only while the surrounding button label is announced.
In `@apps/expo/features/auth/hooks/useAuthActions.ts`:
- Around line 191-212: Replace the raw fetch usage in forgotPassword and
resetPassword with the typed apiClient from 'expo-app/lib/api/packrat';
specifically call apiClient['password-reset'].request.post({ email }) in
forgotPassword and apiClient['password-reset'].verify.post({ email, code:
opts.token, newPassword: opts.newPassword }) in resetPassword, then inspect the
returned { error } and throw new Error((error.value as { error?: string
})?.error ?? 'Forgot password failed') or 'Reset password failed' as appropriate
— remove the await res.json() parsing and any direct fetch/URL usage so the
functions use the apiClient contract and avoid JSON parse errors on non-JSON
responses.
In `@apps/expo/lib/api/packrat.ts`:
- Around line 14-24: Remove the sensitive session-token logging: eliminate the
console.log calls that print cookieJson and the parsed
cookies['better-auth.session_token'] value inside the function in packrat.ts so
the raw cookie blob and session credential are not written to logs; keep the
existing try/catch flow and the console.warn on parse failure, but do not log
cookieJson, the cookies object, or cookies['better-auth.session_token'] anywhere
(retain cookieJson parsing logic into cookies and return
cookies['better-auth.session_token']?.value as before).
- Around line 13-27: parseSessionToken currently returns the cookie.value
without checking cookie.expires, so expired tokens get sent; update
parseSessionToken to inspect the parsed cookie object for the
'better-auth.session_token' entry and if it has an expires field, parse that
(accepting ISO string or numeric timestamp), compare to Date.now(), and return
null when expired; otherwise return the value as before; reference the
parseSessionToken function and the cookie shape { value, expires } (and later
fold the same check into the `@packrat/guards` validator as mentioned).
In `@packages/api/src/routes/passwordReset.ts`:
- Around line 29-37: The handler currently returns error.message to the client
on verifyOtpAndResetPassword failure which can leak differing failure signals;
change the catch block to always return a fixed generic message (e.g., "Password
reset failed") via the status(...) response instead of using error.message,
keeping the status(400, { error: ... }) shape and leaving
verifyOtpAndResetPassword and status usage unchanged.
In `@packages/api/src/services/passwordResetService.ts`:
- Around line 27-37: The verification record is storing OTPs in plaintext and
has no attempt limit; instead hash the OTP before inserting and track attempts
to invalidate after N failures. In the code that inserts into verification (the
block calling db.insert(verification).values({...}) and sendPasswordResetEmail),
replace storing value: code with a securely-derived hash (e.g., HMAC or
PBKDF2/bcrypt with per-code salt) and persist the salt/hashed field and an
attempts integer (default 0). In verifyOtpAndResetPassword, fetch the
verification row, compute the same hash from the provided code (using the stored
salt/secret), compare with timingSafeEqual, and on mismatch increment attempts
via db.update(verification).set({ attempts: attempts + 1 }) and
delete/invalidate the row once attempts >= MAX_ATTEMPTS (e.g., 5); on success
delete the row and continue reset. Use identifiers verification,
verifyOtpAndResetPassword, and sendPasswordResetEmail to locate where to apply
these changes.
- Around line 12-14: The generateOtp function uses Math.random() which is not
cryptographically secure; replace its RNG with a crypto-secure source (Node's
crypto.randomBytes or browser crypto.getRandomValues) in generateOtp so digits
are drawn from secure random bytes, using rejection sampling to avoid modulo
bias and produce exactly OTP_LENGTH numeric characters; ensure you
import/require the crypto module and preserve the function signature
generateOtp(): string and OTP_LENGTH usage.
- Around line 67-79: Remove the dead fallback that updates users.passwordHash:
the password store for Better Auth is the account table
(providerId='credential'), so delete the branch that updates users in
passwordResetService (the block that checks updated.length === 0 and calls
db.update(users).set(...)). Instead, if a credential account is missing, create
or upsert an account row for that user with providerId='credential' and the
hashed password (using the same db and account model used in the earlier
update), or simply remove the fallback logic entirely; update any related tests
to reflect that only the account table (account, providerId) is used for
password storage.
---
Outside diff comments:
In `@apps/expo/app/auth/one-time-password.tsx`:
- Around line 284-288: The next-focus logic wrongly uses only text.length;
update the block around KeyboardController.setFocusTo to consider the current
field index (e.g., the component's index/activeIndex) and NUM_OF_CODE_CHARACTERS
so you only call KeyboardController.setFocusTo for a valid next field.
Specifically, compute targetIndex = currentIndex + text.length (or +1 for
single-char input), and if targetIndex < NUM_OF_CODE_CHARACTERS call
KeyboardController.setFocusTo(targetIndex) (or the appropriate key name),
otherwise call Keyboard.dismiss() and do not attempt to focus a non-existent
field; also ensure partial pastes advance focus by the number of characters
pasted rather than dismissing early.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 01662021-2b0f-47f7-abec-a3c8583732e3
📒 Files selected for processing (8)
apps/expo/app/auth/index.tsxapps/expo/app/auth/one-time-password.tsxapps/expo/features/auth/hooks/useAuthActions.tsapps/expo/lib/api/packrat.tspackages/api/src/routes/index.tspackages/api/src/routes/passwordReset.tspackages/api/src/schemas/auth.tspackages/api/src/services/passwordResetService.ts
| onPress={signInWithApple} | ||
| > | ||
| <Text className="ios:text-foreground absolute left-4 text-[22px]"></Text> | ||
| <Text className="ios:text-foreground absolute left-4 text-[22px]">{''}</Text> |
There was a problem hiding this comment.
Hide the decorative Apple glyph from accessibility output.
Line 122 adds a private-use glyph that can be read aloud by VoiceOver. Keep it decorative so the button is announced cleanly from the text label.
Suggested fix
- <Text className="ios:text-foreground absolute left-4 text-[22px]">{''}</Text>
+ <Text accessible={false} className="ios:text-foreground absolute left-4 text-[22px]">
+ {''}
+ </Text>📝 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.
| <Text className="ios:text-foreground absolute left-4 text-[22px]">{''}</Text> | |
| <Text accessible={false} className="ios:text-foreground absolute left-4 text-[22px]"> | |
| {''} | |
| </Text> |
🤖 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/expo/app/auth/index.tsx` at line 122, The decorative Apple glyph Text
element is being announced by screen readers; mark that Text as non-accessible
so only the button label is read. In the Text component rendering the glyph (the
Text with content ''), add the accessibility props to hide it from assistive
tech (e.g., accessible={false} and/or accessibilityElementsHidden={true} and
importantForAccessibility="no") so the glyph remains visual-only while the
surrounding button label is announced.
| const res = await fetch(`${clientEnvs.EXPO_PUBLIC_API_URL}/api/password-reset/request`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ email }), | ||
| }); | ||
| if (error) throw new Error(error.message ?? 'Forgot password failed'); | ||
| if (!res.ok) { | ||
| const data = (await res.json()) as { error?: string }; | ||
| throw new Error(data.error ?? 'Forgot password failed'); | ||
| } | ||
| }; | ||
|
|
||
| const resetPassword = async (_email: string, opts: { token: string; newPassword: string }) => { | ||
| const { error } = await authClient.resetPassword({ | ||
| token: opts.token, | ||
| newPassword: opts.newPassword, | ||
| const resetPassword = async (email: string, opts: { token: string; newPassword: string }) => { | ||
| const res = await fetch(`${clientEnvs.EXPO_PUBLIC_API_URL}/api/password-reset/verify`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ email, code: opts.token, newPassword: opts.newPassword }), | ||
| }); | ||
| if (error) throw new Error(error.message ?? 'Reset password failed'); | ||
| if (!res.ok) { | ||
| const data = (await res.json()) as { error?: string }; | ||
| throw new Error(data.error ?? 'Reset password failed'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the Treaty path for the new password-reset routes (Elysia exposes the prefix as a key).
rg -nP -C2 "prefix:\s*'/password-reset'" packages/api/src/routes
rg -nP -C3 'apiClient\.' apps/expo --type=ts | head -50Repository: PackRat-AI/PackRat
Length of output: 4537
🏁 Script executed:
# Get the full passwordReset routes structure to understand endpoint paths
cat -n packages/api/src/routes/passwordReset.tsRepository: PackRat-AI/PackRat
Length of output: 1944
🏁 Script executed:
# Check the useAuthActions.ts file to confirm direct fetch() usage
sed -n '180,220p' apps/expo/features/auth/hooks/useAuthActions.ts | cat -nRepository: PackRat-AI/PackRat
Length of output: 2164
🏁 Script executed:
# Verify how the apiClient is imported and used in the auth module
grep -n "apiClient" apps/expo/features/auth/hooks/useAuthActions.ts | head -20Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Check imports and top of useAuthActions.ts file
head -40 apps/expo/features/auth/hooks/useAuthActions.ts | cat -nRepository: PackRat-AI/PackRat
Length of output: 1821
🏁 Script executed:
# Search for apiClient definition in the auth feature
grep -rn "apiClient" apps/expo/features/auth/ --type=tsRepository: PackRat-AI/PackRat
Length of output: 183
🏁 Script executed:
# Search for apiClient in the expo app using rg
rg -n "import.*apiClient" apps/expo/features/ | head -20Repository: PackRat-AI/PackRat
Length of output: 2277
🏁 Script executed:
# Check if apiClient exists and how it's initialized
rg -n "apiClient\s*=" apps/expo/lib --type ts -A 3 | head -40Repository: PackRat-AI/PackRat
Length of output: 365
🏁 Script executed:
# Verify passwordReset.ts export and how routes are structured
grep -n "export\|\.post\|\.get" packages/api/src/routes/passwordReset.tsRepository: PackRat-AI/PackRat
Length of output: 165
🏁 Script executed:
# Check the apiClient definition and what routes it has
cat -n apps/expo/lib/api/packrat.ts | head -60Repository: PackRat-AI/PackRat
Length of output: 2727
🏁 Script executed:
# Check what Treaty routes are available in the apiClient by looking at the type/implementation
rg -n "password-reset|passwordReset" apps/expo/lib/api/ --type ts -B 2 -A 2Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Verify the API schema for password reset
rg -n "ForgotPasswordRequestSchema|ResetPasswordRequestSchema" packages/api/src --type ts -B 1 -A 3Repository: PackRat-AI/PackRat
Length of output: 1711
🏁 Script executed:
# Check how createApiClient is implemented and what response format it uses
rg -n "createApiClient\|export.*function createApiClient" packages/api-client --type ts -A 10 | head -50Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Look at how other features handle apiClient error responses
rg -n "const.*error.*await apiClient" apps/expo/features --type ts -A 2 | head -40Repository: PackRat-AI/PackRat
Length of output: 3314
🏁 Script executed:
# Check how the apiClient is generated and if password-reset routes are registered
rg -n "password-reset|passwordReset" packages/api-client --type ts -B 3 -A 3Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
# Look at the main API index to see how routes are structured and exported
cat -n packages/api/src/index.ts | head -80Repository: PackRat-AI/PackRat
Length of output: 3517
🏁 Script executed:
# Verify the type of apiClient to see available routes
rg -n "typeof.*apiClient|type.*Api.*=" apps/expo/lib/api --type ts -B 2 -A 5Repository: PackRat-AI/PackRat
Length of output: 206
🏁 Script executed:
# Check the routes index to see if passwordResetRoutes are exported
cat -n packages/api/src/routes/index.tsRepository: PackRat-AI/PackRat
Length of output: 1966
🏁 Script executed:
# Look at how routes are structured and composed
rg -n "export.*routes\|passwordResetRoutes" packages/api/src/routes --type tsRepository: PackRat-AI/PackRat
Length of output: 44
Use the typed API client, not raw fetch.
The Expo app's coding guidelines explicitly forbid direct fetch() calls and require the Eden Treaty client. Both forgotPassword and resetPassword must use apiClient (imported from 'expo-app/lib/api/packrat') so they inherit the typed contract, base-URL handling, and standardized { data, error, status } response shape.
Update the functions to:
const { error } = await apiClient['password-reset'].request.post({ email });
if (error) throw new Error((error.value as { error?: string })?.error ?? 'Forgot password failed');const { error } = await apiClient['password-reset'].verify.post({
email,
code: opts.token,
newPassword: opts.newPassword,
});
if (error) throw new Error((error.value as { error?: string })?.error ?? 'Reset password failed');This also fixes a latent bug in the current code: await res.json() will throw on non-JSON error responses (proxy HTML, empty body, network appliance errors), masking the real failure with a SyntaxError.
🤖 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/expo/features/auth/hooks/useAuthActions.ts` around lines 191 - 212,
Replace the raw fetch usage in forgotPassword and resetPassword with the typed
apiClient from 'expo-app/lib/api/packrat'; specifically call
apiClient['password-reset'].request.post({ email }) in forgotPassword and
apiClient['password-reset'].verify.post({ email, code: opts.token, newPassword:
opts.newPassword }) in resetPassword, then inspect the returned { error } and
throw new Error((error.value as { error?: string })?.error ?? 'Forgot password
failed') or 'Reset password failed' as appropriate — remove the await res.json()
parsing and any direct fetch/URL usage so the functions use the apiClient
contract and avoid JSON parse errors on non-JSON responses.
| function parseSessionToken(cookieJson: string | null): string | null { | ||
| console.log('[auth] Parsing session token from cookie string:', cookieJson); | ||
| if (!cookieJson) return null; | ||
| try { | ||
| const cookies = JSON.parse(cookieJson) as Record<string, { value: string }>; | ||
| console.log( | ||
| '[auth] Parsed session token from cookie string:', | ||
| cookies['better-auth.session_token']?.value ?? 'null', | ||
| ); | ||
| return cookies['better-auth.session_token']?.value ?? null; | ||
| } catch (err) { | ||
| console.warn('[auth] Failed to parse session token from cookie string:', err); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Respect the cookie's expires field.
The stored payload includes { value, expires }, but parseSessionToken only reads value. An expired cookie will still be returned and sent to the server, which then 401s and triggers the onNeedsReauth round-trip — exactly the network call the rest of this refactor is trying to avoid. Cheap to fix at parse time.
♻️ Proposed fix
- const cookies = JSON.parse(cookieJson) as Record<string, { value: string }>;
- return cookies['better-auth.session_token']?.value ?? null;
+ const cookies = JSON.parse(cookieJson) as Record<string, { value: string; expires?: string }>;
+ const entry = cookies['better-auth.session_token'];
+ if (!entry) return null;
+ if (entry.expires && Date.parse(entry.expires) <= Date.now()) return null;
+ return entry.value ?? null;(Once you switch to a @packrat/guards validator per the CI failure above, fold the same check into the validated shape.)
🧰 Tools
🪛 GitHub Actions: Checks / 0_checks.txt
[error] 17-17: bun check:casts:strict failed. Unsafe type cast(s) found. Replace 'JSON.parse(cookieJson) as Record<string, { value: string }>' with guards from @packrat/guards.
🪛 GitHub Actions: Checks / checks
[error] 17-17: bun check:casts:strict failed. Found 1 unsafe type cast(s). Replace casts with guards from @packrat/guards. Unsafe cast: "JSON.parse(cookieJson) as Record<string, { value: string }>".
🤖 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/expo/lib/api/packrat.ts` around lines 13 - 27, parseSessionToken
currently returns the cookie.value without checking cookie.expires, so expired
tokens get sent; update parseSessionToken to inspect the parsed cookie object
for the 'better-auth.session_token' entry and if it has an expires field, parse
that (accepting ISO string or numeric timestamp), compare to Date.now(), and
return null when expired; otherwise return the value as before; reference the
parseSessionToken function and the cookie shape { value, expires } (and later
fold the same check into the `@packrat/guards` validator as mentioned).
| console.log('[auth] Parsing session token from cookie string:', cookieJson); | ||
| if (!cookieJson) return null; | ||
| try { | ||
| const cookies = JSON.parse(cookieJson) as Record<string, { value: string }>; | ||
| console.log( | ||
| '[auth] Parsed session token from cookie string:', | ||
| cookies['better-auth.session_token']?.value ?? 'null', | ||
| ); | ||
| return cookies['better-auth.session_token']?.value ?? null; | ||
| } catch (err) { | ||
| console.warn('[auth] Failed to parse session token from cookie string:', err); |
There was a problem hiding this comment.
Remove session-token logging — this is a secret leak.
console.log(...) of the raw cookie blob and the parsed better-auth.session_token.value writes the live session credential to device logs, Metro, and any crash/analytics tooling that captures console. Anyone with that log stream can replay the token and impersonate the user for 7 days. The console.warn on parse failure is fine; the other two must go before this ships.
🔒 Proposed fix
function parseSessionToken(cookieJson: string | null): string | null {
- console.log('[auth] Parsing session token from cookie string:', cookieJson);
if (!cookieJson) return null;
try {
const cookies = JSON.parse(cookieJson) as Record<string, { value: string }>;
- console.log(
- '[auth] Parsed session token from cookie string:',
- cookies['better-auth.session_token']?.value ?? 'null',
- );
return cookies['better-auth.session_token']?.value ?? null;
} catch (err) {
console.warn('[auth] Failed to parse session token from cookie string:', err);
return null;
}
}📝 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.
| console.log('[auth] Parsing session token from cookie string:', cookieJson); | |
| if (!cookieJson) return null; | |
| try { | |
| const cookies = JSON.parse(cookieJson) as Record<string, { value: string }>; | |
| console.log( | |
| '[auth] Parsed session token from cookie string:', | |
| cookies['better-auth.session_token']?.value ?? 'null', | |
| ); | |
| return cookies['better-auth.session_token']?.value ?? null; | |
| } catch (err) { | |
| console.warn('[auth] Failed to parse session token from cookie string:', err); | |
| function parseSessionToken(cookieJson: string | null): string | null { | |
| if (!cookieJson) return null; | |
| try { | |
| const cookies = JSON.parse(cookieJson) as Record<string, { value: string }>; | |
| return cookies['better-auth.session_token']?.value ?? null; | |
| } catch (err) { | |
| console.warn('[auth] Failed to parse session token from cookie string:', err); | |
| return null; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Checks / 0_checks.txt
[error] 17-17: bun check:casts:strict failed. Unsafe type cast(s) found. Replace 'JSON.parse(cookieJson) as Record<string, { value: string }>' with guards from @packrat/guards.
🪛 GitHub Actions: Checks / checks
[error] 17-17: bun check:casts:strict failed. Found 1 unsafe type cast(s). Replace casts with guards from @packrat/guards. Unsafe cast: "JSON.parse(cookieJson) as Record<string, { value: string }>".
🤖 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/expo/lib/api/packrat.ts` around lines 14 - 24, Remove the sensitive
session-token logging: eliminate the console.log calls that print cookieJson and
the parsed cookies['better-auth.session_token'] value inside the function in
packrat.ts so the raw cookie blob and session credential are not written to
logs; keep the existing try/catch flow and the console.warn on parse failure,
but do not log cookieJson, the cookies object, or
cookies['better-auth.session_token'] anywhere (retain cookieJson parsing logic
into cookies and return cookies['better-auth.session_token']?.value as before).
| async ({ body }) => { | ||
| try { | ||
| await verifyOtpAndResetPassword(body); | ||
| return { success: true, message: 'Password reset successfully.' }; | ||
| } catch (error) { | ||
| return status(400, { | ||
| error: error instanceof Error ? error.message : 'Password reset failed', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Return a generic error message on verify failure.
Reflecting error.message straight to the client works, but it leaks branchable signals: "Invalid or expired reset code" vs "User not found" vs whatever a future contributor throws. Since both paths are reachable only with a valid OTP (so the user-enumeration risk is small), this is minor — but a constant string keeps the surface stable and unambiguous.
🔒 Proposed fix
- } catch (error) {
- return status(400, {
- error: error instanceof Error ? error.message : 'Password reset failed',
- });
+ } catch (error) {
+ console.error('[password-reset] verify failed:', error);
+ return status(400, { error: 'Invalid or expired reset code' });
}🤖 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 `@packages/api/src/routes/passwordReset.ts` around lines 29 - 37, The handler
currently returns error.message to the client on verifyOtpAndResetPassword
failure which can leak differing failure signals; change the catch block to
always return a fixed generic message (e.g., "Password reset failed") via the
status(...) response instead of using error.message, keeping the status(400, {
error: ... }) shape and leaving verifyOtpAndResetPassword and status usage
unchanged.
| function generateOtp(): string { | ||
| return Array.from({ length: OTP_LENGTH }, () => Math.floor(Math.random() * 10)).join(''); | ||
| } |
There was a problem hiding this comment.
Use a cryptographically secure RNG for the OTP.
Math.random() is not cryptographically secure — its output is predictable and unsuitable for security tokens. With a 1,000,000-code space and a 15-minute window, a determined attacker doesn't need much luck either way, but the RNG should at minimum be unguessable.
🔒 Proposed fix using crypto.getRandomValues
function generateOtp(): string {
- return Array.from({ length: OTP_LENGTH }, () => Math.floor(Math.random() * 10)).join('');
+ const bytes = new Uint8Array(OTP_LENGTH);
+ crypto.getRandomValues(bytes);
+ return Array.from(bytes, (b) => (b % 10).toString()).join('');
}Note: b % 10 introduces a tiny modulo bias on a 256-value space (digits 0–5 are slightly more likely). If you want it perfectly uniform, rejection-sample bytes ≥ 250.
📝 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.
| function generateOtp(): string { | |
| return Array.from({ length: OTP_LENGTH }, () => Math.floor(Math.random() * 10)).join(''); | |
| } | |
| function generateOtp(): string { | |
| const bytes = new Uint8Array(OTP_LENGTH); | |
| crypto.getRandomValues(bytes); | |
| return Array.from(bytes, (b) => (b % 10).toString()).join(''); | |
| } |
🤖 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 `@packages/api/src/services/passwordResetService.ts` around lines 12 - 14, The
generateOtp function uses Math.random() which is not cryptographically secure;
replace its RNG with a crypto-secure source (Node's crypto.randomBytes or
browser crypto.getRandomValues) in generateOtp so digits are drawn from secure
random bytes, using rejection sampling to avoid modulo bias and produce exactly
OTP_LENGTH numeric characters; ensure you import/require the crypto module and
preserve the function signature generateOtp(): string and OTP_LENGTH usage.
| await db.delete(verification).where(eq(verification.identifier, identifier)); | ||
| await db.insert(verification).values({ | ||
| id: crypto.randomUUID(), | ||
| identifier, | ||
| value: code, | ||
| expiresAt, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }); | ||
|
|
||
| await sendPasswordResetEmail({ to: email, code }); |
There was a problem hiding this comment.
Store OTPs hashed and add a per-OTP attempt limit.
Two issues here:
- Plaintext OTP storage.
value: codewrites the raw 6-digit code intoverification. A read-only DB leak (logs, backups, a SQL-injection elsewhere) hands attackers live reset codes. Hash before storing andtimingSafeEqualthe hashes on verify. - Unbounded verify attempts.
verifyOtpAndResetPasswordonly deletes the record on success, so an attacker has the full 15 minutes to brute-force 1,000,000 codes (which is well within reach over HTTP). Trackattemptson the verification row and invalidate after N (e.g. 5) failures.
Together, these turn a small theoretical risk into a practical attack surface. Also consider rate-limiting /password-reset/verify per IP/email at the route layer.
🤖 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 `@packages/api/src/services/passwordResetService.ts` around lines 27 - 37, The
verification record is storing OTPs in plaintext and has no attempt limit;
instead hash the OTP before inserting and track attempts to invalidate after N
failures. In the code that inserts into verification (the block calling
db.insert(verification).values({...}) and sendPasswordResetEmail), replace
storing value: code with a securely-derived hash (e.g., HMAC or PBKDF2/bcrypt
with per-code salt) and persist the salt/hashed field and an attempts integer
(default 0). In verifyOtpAndResetPassword, fetch the verification row, compute
the same hash from the provided code (using the stored salt/secret), compare
with timingSafeEqual, and on mismatch increment attempts via
db.update(verification).set({ attempts: attempts + 1 }) and delete/invalidate
the row once attempts >= MAX_ATTEMPTS (e.g., 5); on success delete the row and
continue reset. Use identifiers verification, verifyOtpAndResetPassword, and
sendPasswordResetEmail to locate where to apply these changes.
| const updated = await db | ||
| .update(account) | ||
| .set({ password: hashedPassword, updatedAt: now }) | ||
| .where(and(eq(account.userId, user.id), eq(account.providerId, 'credential'))) | ||
| .returning({ id: account.id }); | ||
|
|
||
| // Fallback for legacy users whose password lives on the users row | ||
| if (updated.length === 0) { | ||
| await db | ||
| .update(users) | ||
| .set({ passwordHash: hashedPassword, updatedAt: now }) | ||
| .where(eq(users.id, user.id)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm passwordHash still exists on the users schema and find any consumers.
rg -nP -C3 'passwordHash' packages/api/src/db
rg -nP -C3 '\bpasswordHash\b' packages/api/src --type=tsRepository: PackRat-AI/PackRat
Length of output: 6773
🏁 Script executed:
rg -nP -C5 'passwordHash.*eq|passwordHash.*compare|verify.*passwordHash' packages/api/src --type=ts
rg -nP -C5 'credential.*account|provider.*credential' packages/api/src/services --type=ts
rg -nP 'from.*@better-auth' packages/api/src --type=ts | head -20Repository: PackRat-AI/PackRat
Length of output: 1887
🏁 Script executed:
rg -nP 'verifyPassword' packages/api/src --type=ts -A3 -B3
rg -nP 'signIn.*credential|credential.*signIn|login' packages/api/src --type=ts | head -30Repository: PackRat-AI/PackRat
Length of output: 4345
🏁 Script executed:
rg -nP 'password.*field|passwordHash.*schema' packages/api/src/auth --type=ts -A2 -B2
cat packages/api/src/auth/auth.config.tsRepository: PackRat-AI/PackRat
Length of output: 2970
Remove the users.passwordHash fallback — it will not work and is dead code.
Better Auth's emailAndPassword provider exclusively uses the account table (with providerId='credential') for password storage and verification. The passwordHash field on the users table is just a custom additional field — it's never consulted during login. If a user has no credential account row, they cannot authenticate, even if users.passwordHash is updated.
If you need to support legacy users without credential accounts, create the credential account instead of falling back to a field Better Auth won't check. Otherwise, remove this fallback entirely.
🤖 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 `@packages/api/src/services/passwordResetService.ts` around lines 67 - 79,
Remove the dead fallback that updates users.passwordHash: the password store for
Better Auth is the account table (providerId='credential'), so delete the branch
that updates users in passwordResetService (the block that checks updated.length
=== 0 and calls db.update(users).set(...)). Instead, if a credential account is
missing, create or upsert an account row for that user with
providerId='credential' and the hashed password (using the same db and account
model used in the earlier update), or simply remove the fallback logic entirely;
update any related tests to reflect that only the account table (account,
providerId) is used for password storage.
1 participant
Summary by CodeRabbit
New Features
Improvements
Style