feat: consolidated auth + session management + multi-family isolation - #9
Conversation
Combines 4 PRs: 1. JWT_SECRET hardening 2. Auth gates on 4 API routes 3. Server-side session management 4. Multi-family isolation BREAKING: All existing JWTs invalidated. Migration required before merge.
📝 WalkthroughWalkthroughThis PR adds family-scoped data isolation, DB-backed session records with revocation support, auth checks across API routes, and a new bento-grid layout for the dashboard overview. ChangesFamily isolation and session security
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant auth_ts
participant Database
Client->>auth_ts: login or founder setup
auth_ts->>Database: create session record
Database-->>auth_ts: sessionId
auth_ts->>auth_ts: signSession(payload with sessionId)
auth_ts-->>Client: set session cookie
Client->>auth_ts: authenticated request
auth_ts->>Database: validate session and load user
Database-->>auth_ts: session valid
auth_ts-->>Client: AuthUser with familyId
sequenceDiagram
participant Founder
participant AdminSessionsRoute
participant auth_ts
participant Database
Founder->>AdminSessionsRoute: GET or POST /api/admin/sessions
AdminSessionsRoute->>auth_ts: getAuthUser()
auth_ts-->>AdminSessionsRoute: FOUNDER user
AdminSessionsRoute->>Database: list or revoke sessions
Database-->>AdminSessionsRoute: sessions or revoked count
AdminSessionsRoute-->>Founder: JSON response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/db-queries.ts (2)
372-382: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winScope
updateGoalbyfamilyIdfamilyIdis ignored here, so a caller who knows a goal ID can patch another family’s goal. Usewhere: { id, familyId }to matchdeleteGoaland block the IDOR.🤖 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 `@src/lib/db-queries.ts` around lines 372 - 382, The updateGoal function is ignoring familyId, which allows updating a goal by id alone and can cross family boundaries. Update the db.goal.update call in updateGoal so the where clause includes both id and familyId, matching the scoping used by deleteGoal. Keep the patch behavior the same, but ensure the lookup is restricted to the caller’s family context.
255-296: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftVerify
childIdbelongs tofamilyIdbefore mutating child data.
giveTokens,redeemTokens,addTransaction, andinvestNowacceptfamilyIdbut never confirm thatchildIdbelongs to that family. They write rows under the caller’sfamilyIdand updatechildbyidonly, so a mismatchedchildIdcan affect another family’s balances and mislabel records. Add afindFirst({ where: { id: childId, familyId } })guard before these writes, likesetChildName/setChildPhoto.🤖 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 `@src/lib/db-queries.ts` around lines 255 - 296, The token mutation helpers are missing a family ownership check, so `giveTokens`, `redeemTokens`, `addTransaction`, and `investNow` can write records for one `familyId` while updating a `child` from another family. Add a guard in each of these functions using `findFirst({ where: { id: childId, familyId } })` before any ledger, transaction, or child updates, and stop with an error if no matching child is found. Use the same pattern already used by `setChildName` and `setChildPhoto` so the `childId`/`familyId` pair is verified consistently.src/app/api/recommendations/route.ts (1)
8-21: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope recommendations to the caller’s family
familyIdstill comes from the query string,forParentignores it, andforChildaccepts anychildIdwithout checking ownership. Any authenticated user can read another family’s recommendations. Useuser.familyIdand reject child IDs outside that family before calling the service.🤖 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 `@src/app/api/recommendations/route.ts` around lines 8 - 21, The GET handler in recommendations/route.ts is not enforcing family scoping, so an authenticated user can query another family’s data. Update GET to use the authenticated user’s familyId instead of trusting the familyId query param, and validate any childId belongs to that same family before calling RecommendationService.forChild or RecommendationService.forParent. If the child is not in user.familyId, return an authorization/validation error rather than fetching recommendations.src/app/api/lessons/complete/route.ts (1)
17-28: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAuthorize lesson completion by the authenticated child, not caller-supplied
childId.
getAuthUser()only proves the caller is signed in;input.childIdis still arbitrary and is passed straight intoEducationService.completeLesson, which writes quiz attempts and progress for that userId. Resolve the child from the authenticated user or enforce a family/ownership check before calling the service.🤖 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 `@src/app/api/lessons/complete/route.ts` around lines 17 - 28, The lesson completion route is trusting caller-supplied childId from the request body instead of the authenticated user, which allows writing progress for an arbitrary child. Update the handler in the complete lesson route to derive the target child from getAuthUser() or validate ownership/family membership before calling EducationService.completeLesson, and keep the service call tied to that authenticated child identity rather than input.childId.
🧹 Nitpick comments (2)
prisma/schema.prisma (1)
90-93: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider composite indexes for family-scoped ordered reads.
getFullStatequeries these tables withwhere: { familyId }andorderBy: { timestamp: "desc" }(andcreatedAtfor goals). Separate single-column indexes onfamilyIdandtimestampcan't serve both the filter and the sort optimally; a composite index does.Applies equally to
SpendingEntry,TokenLedgerEntry(familyId+timestamp) andGoal(familyId+createdAt).♻️ Composite index for Transaction
- @@index([familyId]) @@index([childId]) - @@index([timestamp]) + @@index([familyId, timestamp])🤖 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 `@prisma/schema.prisma` around lines 90 - 93, The current single-column indexes on the Prisma models do not optimize family-scoped ordered reads in getFullState. Update the relevant model definitions in the schema to add composite indexes that match the query patterns: pair familyId with timestamp for SpendingEntry and TokenLedgerEntry, and pair familyId with createdAt for Goal. Keep the existing unique identifiers in the schema model blocks so the indexes are easy to locate and maintain.src/app/login/page.tsx (1)
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
next/safeNextderivation.The
searchParamsawait andsafeNextcomputation are repeated at Lines 10-11 and Lines 14-15. Extract once above theif (user)block to keep the validation single-sourced (and to avoid fixing the backslash check in only one place).🤖 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 `@src/app/login/page.tsx` around lines 10 - 15, The `next` and `safeNext` derivation in the login page is duplicated around the `user` redirect logic, so the validation is split across two places. Move the `searchParams` await and `safeNext` computation in `page.tsx` into a single shared block before the `if (user)` check, then reuse that value for the `redirect` call in the login flow to keep the safety check single-sourced.
🤖 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 `@prisma/migrations/20260625020000_consolidated_auth_isolation.sql`:
- Around line 26-52: The migration backfill in the DO block for founder_fam can
be skipped when no FOUNDER user exists, but the subsequent ALTER TABLE ... SET
NOT NULL statements for Child, ParentProfile, Account, Transaction,
SpendingCategory, SpendingEntry, Investment, TokenLedgerEntry, and Goal still
run unconditionally. Update the migration logic around the founder_fam lookup to
either fail fast with a clear error when no founder is found, or gate the NOT
NULL step behind a successful backfill path so the migration does not partially
apply. Use the existing DO $$ block and the Step 4 ALTER TABLE statements as the
main place to adjust this flow.
- Around line 84-85: The singleton delete in the migration is unconditional and
can remove the only FamilySettings row when no founder family exists. Update the
consolidated_auth_isolation migration so the delete is performed only after a
successful founder-family insert, by moving it into the IF founder_fam IS NOT
NULL block or otherwise gating it alongside the founder branch in the migration
logic.
In `@src/app/api/admin/sessions/route.ts`:
- Around line 20-24: The SESSION_REVOKE_ALL branch in the sessions route
currently calls revokeAllUserSessions(user.id) without preserving the caller’s
active session, so “revoke all” signs the requester out too. Update the handler
in the route’s all=true path to pass the current session id as exceptSessionId
to revokeAllUserSessions, using the existing session context available in this
handler, so the action becomes “sign out other devices” while keeping the
current login intact.
In `@src/app/login/page.tsx`:
- Around line 9-13: The safeNext validation in the login page still allows
backslash-prefixed paths like /\evil.com, which can be interpreted as an
external redirect. Update the redirect guard in the logic around searchParams,
safeNext, and redirect() to reject any path containing backslashes as well as
double slashes, so only true same-site relative paths are allowed. Use the
existing safeNext computation in page.tsx as the place to tighten the check.
In `@src/lib/auth-edge.ts`:
- Around line 9-19: getSessionFromCookieHeader currently only verifies the JWT,
so revoked but still-valid cookies continue to authenticate at the edge. Update
this edge-path logic to include a revocation/version check that can be enforced
before returning a SessionPayload, or otherwise shorten the JWT lifetime so
revocation is reflected quickly. Use the getSessionFromCookieHeader and
SessionPayload flow as the place to locate and apply the change, keeping
getAuthUser unchanged.
In `@src/lib/auth.ts`:
- Line 487: The audit payload in auth.ts is recording `backupCodesRemaining` as
a boolean instead of the remaining count, so update the 2FA success path in the
`auditLog` call to use the actual number of backup codes left after
`usedBackupIndex` is removed; if the field is meant to be boolean, rename it to
match the intent. Use the surrounding 2FA logic and the `auditLog` call to
locate the fix, and keep `LOGIN_2FA_BACKUP_SUCCESS`/`LOGIN_2FA_SUCCESS` behavior
unchanged.
- Around line 497-506: The logout flow in auth.ts incorrectly gates revocation
behind params.sessionId, so a revokeAll request with only params.userId skips
session revocation and only clears the cookie. Update logout to handle the
revokeAll path independently of sessionId, using revokeAllUserSessions when
revokeAll and userId are present, and keep revokeSession as the fallback for
single-session logout; make sure the existing auditLog and clearSessionCookie
behavior still run as appropriate.
In `@src/lib/db-queries.ts`:
- Around line 390-396: `contributeToGoal` currently ignores `familyId`, so the
atomic increment can target a goal from another family. Update the
`db.goal.update` call to scope by both `id` and `familyId`, matching the same
ownership check used in `updateGoal` (or add a `findFirst`/verify step before
updating). Keep the increment behavior, but ensure only the caller’s family goal
can be modified.
---
Outside diff comments:
In `@src/app/api/lessons/complete/route.ts`:
- Around line 17-28: The lesson completion route is trusting caller-supplied
childId from the request body instead of the authenticated user, which allows
writing progress for an arbitrary child. Update the handler in the complete
lesson route to derive the target child from getAuthUser() or validate
ownership/family membership before calling EducationService.completeLesson, and
keep the service call tied to that authenticated child identity rather than
input.childId.
In `@src/app/api/recommendations/route.ts`:
- Around line 8-21: The GET handler in recommendations/route.ts is not enforcing
family scoping, so an authenticated user can query another family’s data. Update
GET to use the authenticated user’s familyId instead of trusting the familyId
query param, and validate any childId belongs to that same family before calling
RecommendationService.forChild or RecommendationService.forParent. If the child
is not in user.familyId, return an authorization/validation error rather than
fetching recommendations.
In `@src/lib/db-queries.ts`:
- Around line 372-382: The updateGoal function is ignoring familyId, which
allows updating a goal by id alone and can cross family boundaries. Update the
db.goal.update call in updateGoal so the where clause includes both id and
familyId, matching the scoping used by deleteGoal. Keep the patch behavior the
same, but ensure the lookup is restricted to the caller’s family context.
- Around line 255-296: The token mutation helpers are missing a family ownership
check, so `giveTokens`, `redeemTokens`, `addTransaction`, and `investNow` can
write records for one `familyId` while updating a `child` from another family.
Add a guard in each of these functions using `findFirst({ where: { id: childId,
familyId } })` before any ledger, transaction, or child updates, and stop with
an error if no matching child is found. Use the same pattern already used by
`setChildName` and `setChildPhoto` so the `childId`/`familyId` pair is verified
consistently.
---
Nitpick comments:
In `@prisma/schema.prisma`:
- Around line 90-93: The current single-column indexes on the Prisma models do
not optimize family-scoped ordered reads in getFullState. Update the relevant
model definitions in the schema to add composite indexes that match the query
patterns: pair familyId with timestamp for SpendingEntry and TokenLedgerEntry,
and pair familyId with createdAt for Goal. Keep the existing unique identifiers
in the schema model blocks so the indexes are easy to locate and maintain.
In `@src/app/login/page.tsx`:
- Around line 10-15: The `next` and `safeNext` derivation in the login page is
duplicated around the `user` redirect logic, so the validation is split across
two places. Move the `searchParams` await and `safeNext` computation in
`page.tsx` into a single shared block before the `if (user)` check, then reuse
that value for the `redirect` call in the login flow to keep the safety check
single-sourced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 314f1d28-1940-4f00-b0d9-3cd454634146
📒 Files selected for processing (15)
prisma/migrations/20260625020000_consolidated_auth_isolation.sqlprisma/schema.prismasrc/app/api/admin/security/route.tssrc/app/api/admin/sessions/route.tssrc/app/api/auth/child-login/route.tssrc/app/api/auth/logout/route.tssrc/app/api/child/state/route.tssrc/app/api/lessons/complete/route.tssrc/app/api/mutations/route.tssrc/app/api/recommendations/route.tssrc/app/api/state/route.tssrc/app/login/page.tsxsrc/lib/auth-edge.tssrc/lib/auth.tssrc/lib/db-queries.ts
| DO $$ | ||
| DECLARE founder_fam TEXT; | ||
| BEGIN | ||
| SELECT "familyId" INTO founder_fam FROM "User" WHERE "platformRole" = 'FOUNDER' LIMIT 1; | ||
| IF founder_fam IS NOT NULL THEN | ||
| UPDATE "Child" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "ParentProfile" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "Account" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "Transaction" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "SpendingCategory" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "SpendingEntry" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "Investment" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "TokenLedgerEntry" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| UPDATE "Goal" SET "familyId" = founder_fam WHERE "familyId" IS NULL; | ||
| END IF; | ||
| END $$; | ||
|
|
||
| -- Step 4: Make familyId NOT NULL | ||
| ALTER TABLE "Child" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "ParentProfile" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "Account" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "Transaction" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "SpendingCategory" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "SpendingEntry" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "Investment" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "TokenLedgerEntry" ALTER COLUMN "familyId" SET NOT NULL; | ||
| ALTER TABLE "Goal" ALTER COLUMN "familyId" SET NOT NULL; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Migration silently corrupts/fails when no FOUNDER exists.
The whole backfill (Steps 3 and 6) is gated on founder_fam IS NOT NULL, but Step 4 (SET NOT NULL, lines 44-52) is unconditional. If there's no FOUNDER user, backfill is skipped and every SET NOT NULL aborts on the still-NULL rows, leaving the migration partially applied (columns added, indexes/constraints not).
Guard Step 4 on successful backfill, or fail fast with a clear error if no founder is found, so the DB isn't left in a half-migrated state.
🤖 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 `@prisma/migrations/20260625020000_consolidated_auth_isolation.sql` around
lines 26 - 52, The migration backfill in the DO block for founder_fam can be
skipped when no FOUNDER user exists, but the subsequent ALTER TABLE ... SET NOT
NULL statements for Child, ParentProfile, Account, Transaction,
SpendingCategory, SpendingEntry, Investment, TokenLedgerEntry, and Goal still
run unconditionally. Update the migration logic around the founder_fam lookup to
either fail fast with a clear error when no founder is found, or gate the NOT
NULL step behind a successful backfill path so the migration does not partially
apply. Use the existing DO $$ block and the Step 4 ALTER TABLE statements as the
main place to adjust this flow.
| -- Delete old singleton row | ||
| DELETE FROM "FamilySettings" WHERE "id" = 'singleton'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP "platformRole|FOUNDER|familyId" prisma/schema.prisma
rg -nP "SET NOT NULL|singleton|founder_fam" prisma/migrations/20260625020000_consolidated_auth_isolation.sqlRepository: isaJrKai/planned
Length of output: 3179
🏁 Script executed:
#!/bin/bash
sed -n '1,120p' prisma/migrations/20260625020000_consolidated_auth_isolation.sql | cat -nRepository: isaJrKai/planned
Length of output: 5317
🏁 Script executed:
#!/bin/bash
sed -n '180,230p' prisma/schema.prisma | cat -n
rg -n "FOUNDER|platformRole|familyId" prisma -g '!prisma/migrations/20260625020000_consolidated_auth_isolation.sql'Repository: isaJrKai/planned
Length of output: 3706
Guard the singleton delete on the founder family branch. User.familyId is nullable, so if no FOUNDER row exists this block is skipped, but the final DELETE FROM "FamilySettings" WHERE "id" = 'singleton' still runs and removes the only settings row. Move the delete inside the IF founder_fam IS NOT NULL block, or tie it to a successful insert.
🤖 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 `@prisma/migrations/20260625020000_consolidated_auth_isolation.sql` around
lines 84 - 85, The singleton delete in the migration is unconditional and can
remove the only FamilySettings row when no founder family exists. Update the
consolidated_auth_isolation migration so the delete is performed only after a
successful founder-family insert, by moving it into the IF founder_fam IS NOT
NULL block or otherwise gating it alongside the founder branch in the migration
logic.
| if (all === true) { | ||
| const count = await revokeAllUserSessions(user.id); | ||
| await auditLog({ userId: user.id, action: "SESSION_REVOKE_ALL", entityType: "user", entityId: user.id, ipAddress: ip, userAgent: ua, success: true, after: { count } }); | ||
| return NextResponse.json({ ok: true, revoked: count }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
all: true revokes the caller's current session as well.
revokeAllUserSessions(user.id) is called without exceptSessionId, so the founder issuing "revoke all" is logged out of their own current session too. If the intent is "sign out other devices," pass the current session id as the exception; otherwise this is fine but worth confirming the UX expectation.
🤖 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 `@src/app/api/admin/sessions/route.ts` around lines 20 - 24, The
SESSION_REVOKE_ALL branch in the sessions route currently calls
revokeAllUserSessions(user.id) without preserving the caller’s active session,
so “revoke all” signs the requester out too. Update the handler in the route’s
all=true path to pass the current session id as exceptSessionId to
revokeAllUserSessions, using the existing session context available in this
handler, so the action becomes “sign out other devices” while keeping the
current login intact.
| if (user) { | ||
| const { next } = await searchParams; | ||
| const safeNext = next && next.startsWith("/") && !next.startsWith("//") ? next : "/"; | ||
| redirect(user.familyRole === "CHILD" ? "/child" : safeNext); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
browser Location header backslash normalization open redirect bypass
💡 Result:
The browser backslash-normalization open redirect bypass is a vulnerability that occurs due to a discrepancy between how server-side URL validators and web browsers parse URLs containing backslashes () [1][2]. The vulnerability arises because the WHATWG URL Standard requires that, in special URL schemes like http and https, backslashes be treated as equivalent to forward slashes (path separators) [3][4]. Browsers strictly follow this standard, automatically normalizing backslashes to forward slashes before navigating [3][1]. However, many server-side URL validation libraries (such as those compliant with RFC 3986 or those that do not account for WHATWG rules) do not treat backslashes as path separators [1][4][5]. An attacker can exploit this by crafting a URL that bypasses server-side safety checks but is interpreted as a malicious redirect by the browser [1][4]. For example: 1. An application validates that a redirect URL is relative by checking if it starts with a single forward slash (e.g., /) and not two (e.g., //) [6][4]. 2. An attacker provides a URL like /\evil.com [6][4]. 3. The server-side validator sees this as a valid, same-origin path because it starts with a single / and does not recognize the \ as a directory separator [4]. 4. When the browser receives the resulting Location header (Location: /\evil.com), it normalizes the \ to a / [3][6]. 5. The browser interprets the resulting //evil.com as a protocol-relative absolute URL and redirects the user to the attacker-controlled domain [4][2]. This issue commonly affects OAuth implementations, login/logout redirect flows, and any application that uses custom URL parsing to enforce "same-origin" or "relative" redirect policies [6][1][4][7]. To mitigate this vulnerability, developers must ensure that their server-side URL validation logic mirrors the browser's behavior by explicitly normalizing or rejecting backslashes in redirect paths [6][4][8]. Common fixes include: - Rejecting any redirect URL containing a backslash character (or its encoded forms, like %5c) [4][7][2]. - Using URL parsing libraries that are compliant with the WHATWG URL Standard [1][5]. - Sanitizing leading slashes/backslashes to ensure they cannot form unintended protocol-relative URLs (e.g., replacing all leading [/\]+ with a single /) [6]. Top results: [3], [6], [1], [4]
Citations:
- 1: https://www.sonarsource.com/blog/security-implications-of-url-parsing-differentials
- 2: Uninett/mod_auth_mellon@6204142
- 3: https://guisso.dev/en/blog/cve-2024-29041-browser-parsers/
- 4: Open Redirect via Backslash Normalization in OAuth Redirect URI Validation apache/infrastructure-asfquart#90
- 5: Security vulnerability: Auth Bypass in Dart due to incorrect parsing of the backslash characters in the URL dart-lang/sdk#50075
- 6: Block open redirect via backslash in redirect Location simonw/datasette#2729
- 7: Prevent redirect bypasses via backslash-encoded paths (#36660) go-gitea/gitea#36716
- 8: Prevent redirect bypasses via backslash-encoded paths go-gitea/gitea#36660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file with line numbers.
sed -n '1,120p' src/app/login/page.tsx | cat -n
# Find other usages of the same parameter or redirect sanitization.
rg -n "searchParams.*next|next\.startsWith|redirect\(" src/app -g '!**/*.map' || trueRepository: isaJrKai/planned
Length of output: 2631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/app/login/page.tsx | cat -n
rg -n "searchParams.*next|next\.startsWith|redirect\(" src/app -g '!**/*.map' || trueRepository: isaJrKai/planned
Length of output: 2631
Reject backslashes in safeNext
/\evil.com still passes startsWith("/") && !startsWith("//"), but browsers can treat it as //evil.com and leave the site via redirect(). Reject \ alongside // (for example ^\/(?![\\/])).
🤖 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 `@src/app/login/page.tsx` around lines 9 - 13, The safeNext validation in the
login page still allows backslash-prefixed paths like /\evil.com, which can be
interpreted as an external redirect. Update the redirect guard in the logic
around searchParams, safeNext, and redirect() to reject any path containing
backslashes as well as double slashes, so only true same-site relative paths are
allowed. Use the existing safeNext computation in page.tsx as the place to
tighten the check.
| export async function getSessionFromCookieHeader(cookieHeader: string | null | undefined): Promise<SessionPayload | null> { | ||
| if (!cookieHeader) return null; | ||
| const cookies = Object.fromEntries(cookieHeader.split(";").map((c) => c.trim().split("=")).filter(([k]) => k).map(([k, ...rest]) => [k, decodeURIComponent(rest.join("="))])); | ||
| const token = cookies[SESSION_COOKIE]; | ||
| if (!token) return null; | ||
| try { | ||
| const { payload } = await jwtVerify(token, JWT_SECRET, { algorithms: ["HS256"] }); | ||
| if (typeof payload.sub !== "string" || typeof payload.email !== "string" || typeof payload.platformRole !== "string") return null; | ||
| return { sub: payload.sub, email: payload.email, platformRole: payload.platformRole as string, familyRole: (payload.familyRole as string) || "PARENT", iat: payload.iat, exp: payload.exp }; | ||
| if (typeof payload.sub !== "string" || typeof payload.email !== "string" || typeof payload.platformRole !== "string" || typeof payload.sessionId !== "string") return null; | ||
| return { sub: payload.sub, email: payload.email, platformRole: payload.platformRole as string, familyRole: (payload.familyRole as string) || "PARENT", sessionId: payload.sessionId as string, iat: payload.iat, exp: payload.exp }; | ||
| } catch { return null; } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find middleware and edge consumers of getSessionFromCookieHeader
rg -nP -C3 'getSessionFromCookieHeader' --type=ts
fd -i 'middleware.ts'Repository: isaJrKai/planned
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## files\n'
git ls-files 'src/lib/auth-edge.ts' 'src/**/middleware.ts' 'middleware.ts' | sed 's#^`#-` #'
printf '\n## auth-edge references\n'
rg -n --hidden --glob '!node_modules' 'getSessionFromCookieHeader' src . || true
printf '\n## session validation references\n'
rg -n --hidden --glob '!node_modules' 'isSessionValid|getAuthUser|Session table|revoked' src || trueRepository: isaJrKai/planned
Length of output: 6406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## outline auth-edge\n'
ast-grep outline src/lib/auth-edge.ts --view expanded || true
printf '\n## read auth-edge\n'
wc -l src/lib/auth-edge.ts
sed -n '1,220p' src/lib/auth-edge.tsRepository: isaJrKai/planned
Length of output: 2274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any edge or middleware entrypoints and inspect their imports/usages.
fd -a 'middleware.ts' .
printf '\n## possible edge entrypoints\n'
git ls-files | rg '(^|/)(middleware|edge|auth-edge)\.(ts|js|tsx|jsx)$' || true
printf '\n## direct import chain for auth-edge\n'
rg -n --hidden --glob '!node_modules' 'from .*/lib/auth-edge|from .*auth-edge|getSessionFromCookieHeader' src . || trueRepository: isaJrKai/planned
Length of output: 2197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/middleware.ts
sed -n '1,220p' src/middleware.tsRepository: isaJrKai/planned
Length of output: 5093
Edge middleware still bypasses session revocation. getSessionFromCookieHeader() only validates the JWT, so revoked but unexpired tokens still pass the /api/* and browser-route guards here. getAuthUser() is safe, but the edge layer cannot be the revocation boundary; add an edge-accessible revocation/version check or shorten the JWT lifetime.
🤖 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 `@src/lib/auth-edge.ts` around lines 9 - 19, getSessionFromCookieHeader
currently only verifies the JWT, so revoked but still-valid cookies continue to
authenticate at the edge. Update this edge-path logic to include a
revocation/version check that can be enforced before returning a SessionPayload,
or otherwise shorten the JWT lifetime so revocation is reflected quickly. Use
the getSessionFromCookieHeader and SessionPayload flow as the place to locate
and apply the change, keeping getAuthUser unchanged.
| await setSessionCookie(token); | ||
| await auditLog({ userId: user.id, action: usedBackupIndex !== null ? "LOGIN_2FA_BACKUP_SUCCESS" : "LOGIN_2FA_SUCCESS", entityType: "user", entityId: user.id, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true, after: { backupCodesRemaining: usedBackupIndex !== null } }); | ||
| return { ok: true, user: { id: user.id, email: user.email, platformRole: user.platformRole, familyRole: user.familyRole, name: user.name, twoFactorEnabled: user.twoFactorEnabled } }; | ||
| await auditLog({ userId: user.id, action: usedBackupIndex !== null ? "LOGIN_2FA_BACKUP_SUCCESS" : "LOGIN_2FA_SUCCESS", entityType: "user", entityId: user.id, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true, after: { backupCodesRemaining: usedBackupIndex !== null, sessionId: sessionRecord.id } }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
backupCodesRemaining logs a boolean, not a count.
usedBackupIndex !== null is truthy whenever a backup code was used, so this audit field records true/false rather than the remaining backup-code count. The remaining count is computable after splicing (see the 2FA path around Line 471-473). Consider logging the actual remaining number (or rename the field) so security forensics stay accurate.
🤖 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 `@src/lib/auth.ts` at line 487, The audit payload in auth.ts is recording
`backupCodesRemaining` as a boolean instead of the remaining count, so update
the 2FA success path in the `auditLog` call to use the actual number of backup
codes left after `usedBackupIndex` is removed; if the field is meant to be
boolean, rename it to match the intent. Use the surrounding 2FA logic and the
`auditLog` call to locate the fix, and keep
`LOGIN_2FA_BACKUP_SUCCESS`/`LOGIN_2FA_SUCCESS` behavior unchanged.
| export async function logout(params?: { userId?: string; sessionId?: string; ipAddress?: string; userAgent?: string; revokeAll?: boolean }): Promise<void> { | ||
| if (params?.sessionId) { | ||
| if (params.revokeAll && params.userId) await revokeAllUserSessions(params.userId); | ||
| else await revokeSession(params.sessionId); | ||
| } | ||
| await clearSessionCookie(); | ||
| if (params?.userId) { | ||
| await auditLog({ userId: params.userId, action: "LOGOUT", entityType: "user", entityId: params.userId, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true }); | ||
| await auditLog({ userId: params.userId, action: "LOGOUT", entityType: "user", entityId: params.userId, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true, after: { sessionId: params.sessionId } }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
revokeAll is gated behind sessionId presence.
Revocation only runs inside if (params?.sessionId), so a caller that requests revokeAll with userId but no sessionId clears the cookie without revoking any DB sessions. If "revoke all my sessions" can be invoked independently of the current sessionId, this leaves other devices' sessions active.
🔒 Proposed guard
- if (params?.sessionId) {
- if (params.revokeAll && params.userId) await revokeAllUserSessions(params.userId);
- else await revokeSession(params.sessionId);
- }
+ if (params?.revokeAll && params.userId) {
+ await revokeAllUserSessions(params.userId);
+ } else if (params?.sessionId) {
+ await revokeSession(params.sessionId);
+ }📝 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.
| export async function logout(params?: { userId?: string; sessionId?: string; ipAddress?: string; userAgent?: string; revokeAll?: boolean }): Promise<void> { | |
| if (params?.sessionId) { | |
| if (params.revokeAll && params.userId) await revokeAllUserSessions(params.userId); | |
| else await revokeSession(params.sessionId); | |
| } | |
| await clearSessionCookie(); | |
| if (params?.userId) { | |
| await auditLog({ userId: params.userId, action: "LOGOUT", entityType: "user", entityId: params.userId, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true }); | |
| await auditLog({ userId: params.userId, action: "LOGOUT", entityType: "user", entityId: params.userId, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true, after: { sessionId: params.sessionId } }); | |
| } | |
| } | |
| export async function logout(params?: { userId?: string; sessionId?: string; ipAddress?: string; userAgent?: string; revokeAll?: boolean }): Promise<void> { | |
| if (params?.revokeAll && params.userId) { | |
| await revokeAllUserSessions(params.userId); | |
| } else if (params?.sessionId) { | |
| await revokeSession(params.sessionId); | |
| } | |
| await clearSessionCookie(); | |
| if (params?.userId) { | |
| await auditLog({ userId: params.userId, action: "LOGOUT", entityType: "user", entityId: params.userId, ipAddress: params.ipAddress, userAgent: params.userAgent, success: true, after: { sessionId: params.sessionId } }); | |
| } | |
| } |
🤖 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 `@src/lib/auth.ts` around lines 497 - 506, The logout flow in auth.ts
incorrectly gates revocation behind params.sessionId, so a revokeAll request
with only params.userId skips session revocation and only clears the cookie.
Update logout to handle the revokeAll path independently of sessionId, using
revokeAllUserSessions when revokeAll and userId are present, and keep
revokeSession as the fallback for single-session logout; make sure the existing
auditLog and clearSessionCookie behavior still run as appropriate.
| export async function contributeToGoal(familyId: string, id: string, amount: number) { | ||
| // Atomic increment — no race condition | ||
| return db.goal.update({ | ||
| where: { id }, | ||
| data: { currentAmount: { increment: amount } }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Same isolation bypass in contributeToGoal.
familyId is unused; where: { id } lets a caller increment currentAmount on another family's goal. Apply the same { id, familyId } scoping (or findFirst verify) as suggested for updateGoal.
🛡️ Scope the increment by familyId
return db.goal.update({
- where: { id },
+ where: { id, familyId },
data: { currentAmount: { increment: amount } },
});📝 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.
| export async function contributeToGoal(familyId: string, id: string, amount: number) { | |
| // Atomic increment — no race condition | |
| return db.goal.update({ | |
| where: { id }, | |
| data: { currentAmount: { increment: amount } }, | |
| }); | |
| } | |
| export async function contributeToGoal(familyId: string, id: string, amount: number) { | |
| // Atomic increment — no race condition | |
| return db.goal.update({ | |
| where: { id, familyId }, | |
| data: { currentAmount: { increment: amount } }, | |
| }); | |
| } |
🤖 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 `@src/lib/db-queries.ts` around lines 390 - 396, `contributeToGoal` currently
ignores `familyId`, so the atomic increment can target a goal from another
family. Update the `db.goal.update` call to scope by both `id` and `familyId`,
matching the same ownership check used in `updateGoal` (or add a
`findFirst`/verify step before updating). Keep the increment behavior, but
ensure only the caller’s family goal can be modified.
- Schema provider: postgresql (was accidentally sqlite) - Bento grid: parent dashboard overview reorganized into modern grid - Hero cell: Total Family Savings (2x2) - Small cells: This Month, Tokens, Investments, Children - Wide cells: Savings Trend, Children Portfolios - Tall cell: Recent Activity - Full-width: Family Spending, Recommendations - Responsive: 3-col desktop, stacked mobile
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/DashboardClient.tsx (2)
347-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer existing store selectors over duplicated memo logic.
The
familySpendingandspendingBreakdownmemos duplicate the exact logic already exported from the store asfamilySpendingThisMonthandfamilySpendingBreakdown(store.ts:482-507). Using the store selectors eliminates duplication, ensures consistency if the month-scoping logic changes, and reduces re-renders since Zustand handles selector memoization internally.♻️ Proposed refactor using store selectors
- const allSpending = useStore((s) => s.spending); - const familySpending = useMemo( - () => allSpending.filter((e) => { const d = new Date(e.timestamp); const now = new Date(); return d.getFullYear() === now.getFullYear() && d.getMonth() === now.getMonth(); }).reduce((sum, e) => sum + e.amount, 0), - [allSpending] - ); - const spendingBreakdown = useMemo(() => { - const now = new Date(); - const map = new Map<string, { ownerKind: "parent" | "child"; ownerName: string; total: number }>(); - for (const e of allSpending) { - const d = new Date(e.timestamp); - if (d.getFullYear() !== now.getFullYear() || d.getMonth() !== now.getMonth()) continue; - const existing = map.get(e.ownerId); - if (existing) existing.total += e.amount; - else map.set(e.ownerId, { ownerKind: e.ownerKind, ownerName: e.ownerName, total: e.amount }); - } - return Array.from(map.entries()).map(([ownerId, v]) => ({ ownerId, ...v })).sort((a, b) => b.total - a.total); - }, [allSpending]); + const familySpending = useStore(familySpendingThisMonth); + const spendingBreakdown = useStore(familySpendingBreakdown);🤖 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 `@src/app/DashboardClient.tsx` around lines 347 - 363, The `DashboardClient` component is duplicating month-scoped spending calculations that already exist in the store. Replace the local `useMemo` logic for `familySpending` and `spendingBreakdown` with the existing store selectors `familySpendingThisMonth` and `familySpendingBreakdown` so the component stays consistent with `store.ts` and avoids redundant re-computation. Use the selector exports directly from `useStore` and remove the inline filtering/reduction logic in `DashboardClient`.
519-521: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTransaction feed shows "undefined" when child lookup fails.
When
childis not found inchildren(e.g., a transaction for a child that was removed),child?.name.split(" ")[0]renders as the string"undefined"in the UI. Add a fallback display name.✨ Proposed fix
- <div className="micro-label mt-0.5 truncate">{child?.name.split(" ")[0]} · {txTypeLabel[t.type]} · {timeAgo(t.timestamp)}</div> + <div className="micro-label mt-0.5 truncate">{child?.name.split(" ")[0] ?? "Unknown"} · {txTypeLabel[t.type]} · {timeAgo(t.timestamp)}</div>🤖 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 `@src/app/DashboardClient.tsx` around lines 519 - 521, The transaction feed item in DashboardClient renders "undefined" when the child lookup fails because the display name expression assumes a valid child name. Update the child-name formatting near the transaction row to safely handle a missing child by using a fallback label (for example, a generic removed/missing child name) before applying the first-name split. Keep the existing txTypeLabel and timeAgo rendering unchanged, and make the fallback logic easy to find around the child?.name usage in the feed item markup.
🤖 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 `@src/app/DashboardClient.tsx`:
- Around line 457-463: The Tokens action in DashboardClient is missing the same
undefined guard used by the Spend button, so `children[0]` can be passed as
`undefined` into `onGiveTokens`. Update the button’s `onClick` handler to check
for an existing child before calling `onGiveTokens`, and keep the behavior
aligned with the `parents[0]`/`onLogSpend` pattern in the same block.
---
Nitpick comments:
In `@src/app/DashboardClient.tsx`:
- Around line 347-363: The `DashboardClient` component is duplicating
month-scoped spending calculations that already exist in the store. Replace the
local `useMemo` logic for `familySpending` and `spendingBreakdown` with the
existing store selectors `familySpendingThisMonth` and `familySpendingBreakdown`
so the component stays consistent with `store.ts` and avoids redundant
re-computation. Use the selector exports directly from `useStore` and remove the
inline filtering/reduction logic in `DashboardClient`.
- Around line 519-521: The transaction feed item in DashboardClient renders
"undefined" when the child lookup fails because the display name expression
assumes a valid child name. Update the child-name formatting near the
transaction row to safely handle a missing child by using a fallback label (for
example, a generic removed/missing child name) before applying the first-name
split. Keep the existing txTypeLabel and timeAgo rendering unchanged, and make
the fallback logic easy to find around the child?.name usage in the feed item
markup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b65c8afe-2290-4b4f-993e-d575471ce8d5
📒 Files selected for processing (1)
src/app/DashboardClient.tsx
| <div className="flex gap-2"> | ||
| <button onClick={() => { const p = parents[0]; if (p) onLogSpend({ id: p.id, kind: "parent", name: p.name }); }} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5"> | ||
| <Wallet className="h-2.5 w-2.5" /> Spend | ||
| </button> | ||
| <button onClick={() => onGiveTokens(children[0])} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5"> | ||
| <Plus className="h-2.5 w-2.5" /> Tokens | ||
| </button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing guard on Tokens button when children array is empty.
The Spend button guards against parents[0] being undefined (if (p)), but the Tokens button calls onGiveTokens(children[0]) without a similar check. When there are no children, this passes undefined to a function typed (c: Child) => void, causing a runtime error.
🐛 Proposed fix
- <button onClick={() => onGiveTokens(children[0])} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5">
+ <button onClick={() => { const c = children[0]; if (c) onGiveTokens(c); }} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5">📝 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.
| <div className="flex gap-2"> | |
| <button onClick={() => { const p = parents[0]; if (p) onLogSpend({ id: p.id, kind: "parent", name: p.name }); }} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5"> | |
| <Wallet className="h-2.5 w-2.5" /> Spend | |
| </button> | |
| <button onClick={() => onGiveTokens(children[0])} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5"> | |
| <Plus className="h-2.5 w-2.5" /> Tokens | |
| </button> | |
| <div className="flex gap-2"> | |
| <button onClick={() => { const p = parents[0]; if (p) onLogSpend({ id: p.id, kind: "parent", name: p.name }); }} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5"> | |
| <Wallet className="h-2.5 w-2.5" /> Spend | |
| </button> | |
| <button onClick={() => { const c = children[0]; if (c) onGiveTokens(c); }} className="btn-outline px-2.5 py-1 rounded text-[10px] tracking-wider flex items-center gap-1.5"> | |
| <Plus className="h-2.5 w-2.5" /> Tokens | |
| </button> |
🤖 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 `@src/app/DashboardClient.tsx` around lines 457 - 463, The Tokens action in
DashboardClient is missing the same undefined guard used by the Spend button, so
`children[0]` can be passed as `undefined` into `onGiveTokens`. Update the
button’s `onClick` handler to check for an existing child before calling
`onGiveTokens`, and keep the behavior aligned with the `parents[0]`/`onLogSpend`
pattern in the same block.
1 participant
What this does
Combines 4 critical fixes into one clean PR:
Run
prisma/migrations/20260625020000_consolidated_auth_isolation.sqlon Neon BEFORE merging.All existing JWTs are auto-rejected (no sessionId). Every user must log in again.
After merge
Supersedes PRs #1, #2, #6, #7
Close those after merging this.
Verification
npx tsc --noEmit— cleanSummary by CodeRabbit
New Features
Bug Fixes