AUTH-11 Implement Supabase SSR clients, OTP helpers, and auth callback - #3
AUTH-11 Implement Supabase SSR clients, OTP helpers, and auth callback#3loganravin4 wants to merge 4 commits into
Conversation
…ed by supabase limit for creating project
b-at-neu
left a comment
b-at-neu
left a comment
There was a problem hiding this comment.
Looking good! Just a few questions and minor suggestions. Also remember to prefix your commits with the linear tag!!
…, admin client, updateSession, edge middleware
pataniaeli
left a comment
pataniaeli
left a comment
There was a problem hiding this comment.
Security review (auth/session/secret handling — mandatory pre-merge gate per repo policy).
Must fix before merge:
- HIGH-1 — session cookies missing
httpOnly/secure(server.ts, middleware.ts) - HIGH-2 —
env.tsmissingserver-onlyguard, service-role key reachable from a client component if one ever imports it directly (verified via build: leaks into SSR HTML) - MEDIUM-4 —
getSafeNextPathdoesn't block backslash-based open-redirect payloads despite its docstring; not exploitable at today's single call site but exported for reuse by future adapters
Track for follow-up (not necessarily blocking this PR):
- HIGH-3 — no RLS found on
Session/Projecttables (plaintext token/apiKey) in the Prisma migrations; needs confirmation against the live Supabase project before this holds real traffic - MEDIUM-5 — middleware matcher excludes any path ending in an image extension, unanchored to route structure — a bypass shape once this middleware does real authorization
- MEDIUM-6 —
sendOtp'sdataoption writes to user-editableuser_metadata/JWT claims; fine today (unused) but a trap for future authorization-relevant fields - MEDIUM-7 — no rate limiting on
/auth/callbackor OTP verify onceverifyOtpgets a caller
Verified safe: service-role key never appears in any client bundle; admin.ts is tree-shaken out of the Edge middleware chunk; cookie get/set ordering matches Supabase's documented SSR pattern; CRLF/header injection via next is blocked by Next's URL parsing; .env.example has no real secrets; PKCE makes login-CSRF via a stolen code fail closed.
| export async function createServerSupabaseClient() { | ||
| const cookieStore = await cookies(); | ||
|
|
||
| return createServerClient(getSupabaseUrl(), getSupabaseAnonKey(), { |
There was a problem hiding this comment.
HIGH — session cookies missing httpOnly/secure
createServerClient() is called with only { cookies: {...} } — no cookieOptions. @supabase/ssr's DEFAULT_COOKIE_OPTIONS is { httpOnly: false, sameSite: "lax" } with no secure, so the sb-<ref>-auth-token cookies (access + refresh token) are written readable by JS and over plaintext HTTP, with a 400-day lifetime.
Any XSS on this origin can exfiltrate the refresh token and mint access tokens for all 6 downstream projects for over a year. The docstring above claims "HTTP-only cookies" — currently false.
Fix: pass cookieOptions: { httpOnly: true, secure: process.env.NODE_ENV === "production", sameSite: "lax", path: "/" }.
| request, | ||
| }); | ||
|
|
||
| const supabase = createServerClient(getSupabaseUrl(), getSupabaseAnonKey(), { |
There was a problem hiding this comment.
HIGH — same missing cookieOptions as server.ts
Same issue as server.ts: no cookieOptions passed to createServerClient(), so session cookies default to httpOnly: false with no secure. Fix both call sites together.
| @@ -0,0 +1,28 @@ | |||
| /** | |||
There was a problem hiding this comment.
HIGH — missing server-only guard
Every other module in src/lib/supabase/ (admin.ts, otp.ts, server.ts) starts with import "server-only";. This file doesn't, even though it exports getSupabaseServiceRoleKey().
Verified with a real build: importing the barrel (./index) from a "use client" component fails closed (Turbopack errors on the transitive server-only import). But importing @/lib/supabase/env directly from a client component builds successfully, and the service-role key gets rendered into SSR HTML output (confirmed with a canary value in .next/server/app/*.html). Not exploited today since nothing does this yet, but it's a one-line mistake away from a full identity-service compromise.
Fix: add import "server-only"; to this file, or move getSupabaseServiceRoleKey into a separately-guarded module and drop it from the shared barrel.
| if (trimmed === "") { | ||
| return fallback; | ||
| } | ||
| if (trimmed.includes("://") || trimmed.startsWith("//")) { |
There was a problem hiding this comment.
MEDIUM — doesn't actually block open redirects, despite the docstring
trimmed.includes("://") || trimmed.startsWith("//") misses backslash-based authority injection. Under WHATWG URL parsing, a backslash behaves like a forward slash for special schemes, so a value like /\evil.com (or \evil.com, which this function rewrites to /\evil.com) resolves with host = evil.com when passed to redirect()/NextResponse.redirect() without an origin prefix.
Not exploitable today — auth/callback/route.ts always prefixes ${origin} before this path, which pins the authority. But this function is exported from the shared barrel specifically for reuse by the other 5 project adapters, and its docstring promises same-origin-only redirects — the first adapter author who calls redirect(getSafeNextPath(x)) without an origin prefix gets a working open redirect on the login flow.
Fix: reject backslashes and control characters before the existing checks, or validate positively via new URL(trimmed, "https://placeholder.invalid") and require the resolved origin to match the placeholder.
…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
Add server-side createServerSupabaseClient (cookies + @supabase/ssr), service-role
admin client, sendOtp / verifyOtp / getAuthUser, root middleware session refresh,
and PKCE exchange on /auth/callback.