Conversation
There was a problem hiding this comment.
Review written by a Claude agent.
Same framing note as the AUTH-8 review: a few of the items below only bite once these actions are reachable (exported and imported somewhere), so if there's a planned follow-up ticket for authorization before this gets wired into a client component, deferring those specific items is reasonable — I've separated them out so that's a visible, deliberate choice.
Must-fix regardless of sequencing:
updateUser's unfiltereddataspread (see inline comment) — straightforward allowlist fix, independent of who's allowed to call it.- A likely merge collision with #3 (see inline comment on
src/lib/supabase.ts) — worth resolving which Supabase client module is canonical before both land. getUsers'swhere: filtersneeds field-level scoping, not just an auth check (see inline comment) — a valid authenticated caller in one project shouldn't be able to shape a filter that returns another project's users.- Raw Supabase Admin API error text (
error.message, line 48) is currently returned to the caller — worth routing through something like #3'sauthErrorToQueryCodepattern instead of surfacing provider text directly. deleteUserdeletes the Supabase auth identity (line 94) before running the Prisma transaction (line 102) — if the transaction fails, session/membership rows survive pointing at an identity that no longer resolves. Might be worth swapping the order or wrapping both in a compensating flow.
Pending authz ticket:
- None of the five exports currently check who's calling before reading/writing user data — most consequential combined with the
updateUserissue above. Flagging so it's confirmed rather than assumed before this is wired up.
Worth its own ticket, not introduced by this PR: same RLS gap noted on the other open PRs — no policies found on the underlying tables.
(Edit: corrected a couple of line references above and a couple of inline comment anchors below that were off in my first pass — same content, right lines now.)
| @@ -0,0 +1,17 @@ | |||
| import "server-only"; | |||
There was a problem hiding this comment.
Heads up on a collision with #3 (auth-11), independent of this PR's own scope: that branch adds a src/lib/supabase/ directory (server.ts, admin.ts, middleware.ts, etc.) exporting from an index.ts barrel. This file is src/lib/supabase.ts — same import specifier (@/lib/supabase), different shape. I tested merging both locally: git reports no conflict (different paths), but the file silently wins over the directory for that specifier, and tsc then fails on src/middleware.ts because updateSession can no longer be resolved — breaking session-cookie refresh app-wide with no build error to catch it before that point. Worth a quick sync with whoever owns #3 on which one is canonical before either merges — not a blocker on this PR in isolation, but a blocker on merging both.
| id: string, | ||
| data: { isAdmin?: boolean }, | ||
| ): Promise<User> { | ||
| return prisma.user.update({ where: { id }, data }); |
There was a problem hiding this comment.
data: { isAdmin?: boolean } is forwarded straight into prisma.user.update unfiltered. The TS type is compile-time only — Prisma will accept any field present on the object at runtime, including supabaseUserId (the only identity binding in this system, since it maps a User row to a specific Supabase auth account). Worth an explicit destructure (const { isAdmin } = data) regardless of what authorization ends up wrapping this, since even an authorized caller passing an unexpected extra field would silently be able to repoint identity fields on any row.
| isAdmin?: boolean; | ||
| }): Promise<User[]> { | ||
| return prisma.user.findMany({ | ||
| where: filters, |
There was a problem hiding this comment.
Even once there's a valid authenticated caller here, where: filters passes the caller's object straight into Prisma's query builder with no scoping — e.g. { isAdmin: true } today, but the type doesn't stop a caller from sending a differently-shaped filter, and there's no restriction to the caller's own project(s) either way. This is a data-scoping issue that authentication alone won't fix — worth deciding whether that scoping is part of the same follow-up work or needs to land here.
…llision
Addresses the "must-fix regardless of sequencing" findings from the AUTH-7
security review:
- updateUser now destructures isAdmin explicitly instead of spreading the
caller-supplied data object into Prisma, closing a mass-assignment path
that could otherwise write unexpected fields (e.g. supabaseUserId).
- getUsers now builds an explicit { isAdmin } where clause instead of
passing the caller's filter object straight to Prisma's query builder.
- getUser no longer returns raw Supabase Admin API error text to the
caller; details are logged server-side instead.
- deleteUser now runs the Prisma transaction before deleting the Supabase
auth identity, so a failed transaction can't leave live session/
membership rows pointing at an identity that no longer resolves.
- Renamed src/lib/supabase.ts to src/lib/supabase-admin.ts to avoid
colliding with the src/lib/supabase/ directory added by #3 (auth-11),
and added autoRefreshToken/persistSession: false plus clear env-var
errors to match that module's admin client pattern.
Caller-identity/authorization checks are intentionally deferred — this
codebase has no merged session mechanism yet (the only one, #3, is still
open) — and are tracked via a TODO in users.ts pending that follow-up.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Created
lib/users.tswith: