fix: remove auth bypass dev fallback in getUserIdFromRequest

[bmersereau]

Summary

• Removes the dev fallback in getUserIdFromRequest() that accepted any Bearer token verbatim as a user ID when env vars were absent
• Missing NEXT_PUBLIC_SUPABASE_URL or SUPABASE_SECRET_KEY now throws a 500 response instead of silently bypassing authentication
• createServerSupabase() also now throws when env vars are missing instead of silently creating a client with empty credentials
• Removes redundant || "" fallbacks from getUserIdFromRequest for consistency
• Adds vitest and unit tests covering missing-env-var cases for both functions

Closes #65
Closes #87
Closes #89
Closes #90

Changes

• frontend/src/lib/supabase-server.ts — createServerSupabase throws on missing env vars; getUserIdFromRequest removes || "" fallbacks; bypass fallback replaced with throw new Response(..., { status: 500 })
• frontend/src/lib/__tests__/supabase-server.test.ts — 10 unit tests covering both createServerSupabase and getUserIdFromRequest
• frontend/vitest.config.ts — minimal vitest config

Test plan

• Unit tests added and passing (10/10)
• TypeScript clean
• Build requires real Supabase env vars (pre-existing prerender failure without credentials)

[bmersereau]

PR Review: fix: remove auth bypass dev fallback from getUserIdFromRequest

Summary

This PR closes the dev-mode bypass that allowed any raw Bearer token to be treated as a valid user ID when NEXT_PUBLIC_SUPABASE_URL or SUPABASE_SECRET_KEY were absent. The fix is correct and the new error is the right behavior. Test coverage is present but has gaps described below.

Risk Assessment

• Risk Level: Low
• Blast Radius: Any environment that relied on the dev fallback (missing env vars) will now get a 500 instead of silently accepting the raw token — correct and intentional
• Rollback Plan: git revert the commit; fallback behavior is restored immediately

---

Review by Category

Security

• [severity:praise] Removing the fallback is the right call. The old code at lines 27–29 was a direct authentication bypass — any string passed as a Bearer token became the user ID. The new throw new Response("Server auth is not configured", { status: 500 }) is safe and fails loud.

Correctness

• [severity:minor] createServerSupabase() still constructs a client with empty strings when env vars are absent:

  const url = process.env.NEXT_PUBLIC_SUPABASE_URL || "";
  const key = process.env.SUPABASE_SECRET_KEY || "";
  return createClient(url, key, { auth: { persistSession: false } });

  This is now inconsistent with the new stricter behavior in getUserIdFromRequest. A misconfigured createServerSupabase() call will silently return a client that fails at query time rather than at construction. Not a blocker, but worth aligning in a follow-up.

Test Coverage

• [severity:minor] Missing happy-path test: a well-configured server with a valid token should return the user ID. Without this, a regression that breaks the success path won't be caught.

• [severity:minor] Missing invalid-token test: getUser returning { data: { user: null } } should yield a 401. This is the most security-relevant branch and has no test.

• [severity:nit] Test 1 ("throws 500 when NEXT_PUBLIC_SUPABASE_URL is missing") calls getUserIdFromRequest twice — once to assert the rejection, once to extract the status. The second call re-runs the async logic redundantly:

  await expect(getUserIdFromRequest(req)).rejects.toBeInstanceOf(Response);
  const err = await getUserIdFromRequest(req).catch((r: Response) => r); // second call

  Use a single call with rejects:

  const err = await getUserIdFromRequest(req).catch((r) => r);
  expect(err).toBeInstanceOf(Response);
  expect((err as Response).status).toBe(500);

---

Test Coverage Assessment

• Lines added (non-test): 1 (the throw replacing 2 lines net)
• Test lines added: ~57
• Missing test cases: happy path (valid token → returns user ID), invalid token (getUser returns null user → 401)

---

Verdict

• Approve - Ship it
• Approve with nits — Add the happy-path and invalid-token tests before or shortly after merge; fix the double-call in test 1
• Request changes - Blocking issues must be addressed
• Needs discussion - Architecture/approach concerns

What I Verified

• Read the full diff
• Ran tests locally (4/4 pass)
• Checked createServerSupabase() for consistency
• Reviewed linked issue [Security][Critical] Auth bypass dev fallback — unauthenticated account takeover #65

[bmersereau]

PR Review (follow-up): fix: remove auth bypass dev fallback from getUserIdFromRequest

All issues from the previous review have been addressed. This is a clean pass.

What changed since last review

• vi.hoisted() used for a stable mockGetUser reference that survives vi.resetModules() — correct vitest idiom
• Double-call in test 1 fixed: single catch + two assertions
• Happy-path test added: valid token → returns "real-user-id"
• Invalid-token 401 test added: mockGetUser returns null user → 401 response
• beforeEach resets mockGetUser to the default so tests don't bleed into each other

Remaining (non-blocking)

• [severity:nit] In beforeEach, placing mockGetUser.mockResolvedValue(...) before vi.resetModules() would read more naturally for someone unfamiliar with vi.hoisted — reset mock state first, then clear module registry. Behaviour is identical either way.

Test Coverage

All branches of getUserIdFromRequest are now covered: missing URL (500), missing key (500), both missing (500), no auth header (401), valid token → user ID, invalid/expired token (401). Nothing missing.

Verdict

• Approve — Ship it

What I Verified

• Read the full diff
• Ran tests locally — 6/6 pass
• Confirmed vi.hoisted() mock survives vi.resetModules() correctly

[LaurieScheepers]

These security enhancements should be merged.

[willchen96]

Thanks Beau. This has been fixed as frontend/src/lib/supabase-server.ts has now been deleted as supabase connection is moved to backend except for auth.