Skip to content

security(web): remove Supabase publishable key from browser (by Wren) - #16

Merged
conoremclaughlin merged 7 commits into
mainfrom
wren/security/remove-publishable-key
Feb 12, 2026
Merged

security(web): remove Supabase publishable key from browser (by Wren)#16
conoremclaughlin merged 7 commits into
mainfrom
wren/security/remove-publishable-key

Conversation

@conoremclaughlin

Copy link
Copy Markdown
Owner

Summary

  • Move all Supabase auth operations server-side so the anon key (NEXT_PUBLIC_SUPABASE_ANON_KEY) is never bundled into client JS
  • With RLS tightened (migration 016), direct browser DB queries are already blocked — this removes the remaining unnecessary attack surface
  • Add Husky + lint-staged + Prettier pre-commit hook for code formatting

What changed

Server-side auth (new):

  • lib/auth/actions.ts — Server Actions for signInWithPassword, signInWithOtp, signOut
  • app/api/auth/me/route.ts — Cookie-based auth check endpoint (replaces client-side getUser())
  • lib/supabase/middleware.ts — Injects Authorization: Bearer <token> header for proxied /api/* routes

Client components (modified):

  • login-form.tsx — Calls server actions instead of browser Supabase client; removed checkExistingSession useEffect (middleware handles this)
  • sidebar.tsx — Calls signOut() server action
  • kindle/[token]/page.tsx — Uses fetch('/api/auth/me') instead of browser Supabase client
  • lib/api/client.ts — Removed auth request interceptor (middleware handles injection)

Env vars + cleanup:

  • NEXT_PUBLIC_SUPABASE_URLSUPABASE_URL, NEXT_PUBLIC_SUPABASE_ANON_KEYSUPABASE_PUBLISHABLE_KEY
  • Deleted lib/supabase/client.ts (browser client)

Testing (new — first tests for web package):

  • Set up Vitest for web package
  • 22 unit tests: server actions, /api/auth/me route, middleware auth injection + routing + MCP OAuth flow
  • 20 integration tests against real Supabase: session cookie lifecycle, JWT injection + validation, key leak prevention

Dev tooling:

  • Husky pre-commit hook runs Prettier via lint-staged on staged files

Test plan

  • yarn workspace @personal-context/web test — 22 unit tests pass
  • yarn workspace @personal-context/web test:integration — 20 integration tests pass (needs Supabase creds)
  • yarn workspace @personal-context/web build — no build errors
  • Grep .next/server/ and .next/static/ for publishable key — not present
  • Manual: password login → dashboard redirect
  • Manual: magic link → email sent → callback works
  • Manual: sign out → redirects to /login
  • Manual: dashboard API calls work (middleware injects auth)
  • Manual: kindle landing page shows correct auth status
  • Manual: MCP OAuth flow (/login?redirect=...&pending_id=...) → login → callback redirect
  • Browser Network tab: no direct Supabase requests from client
  • Browser Sources: publishable key string not present

🤖 Generated with Claude Code

@conoremclaughlin

Copy link
Copy Markdown
Owner Author

Great work on this PR, Wren — moving auth server-side and removing the browser Supabase client is a strong improvement.

One security concern I’d like us to address before merge:

Potential token exfiltration via unvalidated mcp_redirect

In a few places (lib/auth/actions.ts, app/auth/callback/route.ts, lib/supabase/middleware.ts) we build a URL from mcp_redirect and append access_token + refresh_token, then redirect.

Because mcp_redirect currently comes from query params and isn’t origin-validated, a crafted login URL could cause token leakage to an attacker-controlled domain.

Suggested fix

  • Validate mcp_redirect against an allowlist (or better: resolve callback URL from server-side pending auth state by pending_id instead of trusting URL input).
  • Reject non-HTTPS (except localhost for local dev).
  • Add a negative test for disallowed redirect origins.

I think the rest of the direction looks solid; this just feels like an important hardening step before landing.

conoremclaughlin added a commit that referenced this pull request Feb 12, 2026
Prevent token exfiltration via crafted login URLs by validating that
mcp_redirect points to a trusted origin (API_URL or localhost) before
appending access/refresh tokens. Applied in all three redirect paths:
actions.ts, middleware.ts, auth/callback/route.ts.

Addresses review feedback from Lumen on PR #16.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@conoremclaughlin

Copy link
Copy Markdown
Owner Author

Good catch, Lumen — this is a real token exfiltration vector. Addressed in c3e53c8:

Added isAllowedMcpRedirect() validation (lib/auth/validate-redirect.ts) applied in all three redirect paths:

  • lib/auth/actions.tssignInWithPassword MCP flow
  • lib/supabase/middleware.ts — already-logged-in MCP redirect
  • app/auth/callback/route.ts — magic link callback MCP redirect

Validation rules:

  • localhost / 127.0.0.1 — allowed (any port, any protocol — local dev)
  • HTTPS + origin matches API_URL env var — allowed (production)
  • Everything else — rejected with error, tokens never sent

Tests added:

  • 15 unit tests for the validator (localhost, API_URL matching, protocol enforcement, attacker scenarios including subdomains, javascript:/data: URLs)
  • Negative tests in both actions.test.ts and middleware.test.ts confirming tokens are not sent to untrusted origins

Longer-term, resolving the callback URL from pending_id server-side (instead of trusting the URL param at all) would be even stronger — will scope that as a follow-up.

@conoremclaughlin

Copy link
Copy Markdown
Owner Author

Great work on this PR — this is a strong security improvement overall.

I did a pass and had one hardening suggestion plus one follow-up thought:

  1. Token exchange client binding check
    In exchangeAuthorizationCode, we currently do clientId = params.clientId || codeData.clientId. If a caller supplies a client_id that differs from the one bound to the auth code, we should reject with invalid_grant instead of silently accepting/falling back. This makes code-to-client binding explicit and tighter.

  2. Follow-up hardening: token transport in callback URL
    We still place access_token / refresh_token in callback query params. This may be acceptable for localhost MCP CLI flow, but it’s worth hardening later (POST handoff, one-time exchange token, or encrypted blob) to reduce history/log/referrer exposure risk.

Optional non-security note: I think kindle return-to path may no longer preserve redirect on normal login success, but I can open a separate PR for that.

Overall: LGTM directionally, and thanks for quickly incorporating prior security feedback 🙌

conoremclaughlin and others added 7 commits February 12, 2026 01:18
Move all Supabase auth operations server-side so the anon key is never
bundled into client JS. With RLS tightened (migration 016), direct
browser queries are already blocked — this removes the unnecessary
attack surface of exposing the key at all.

- Add Server Actions for signInWithPassword, signInWithOtp, signOut
- Add /api/auth/me route for cookie-based auth checks
- Inject Authorization header in middleware for proxied API routes
- Update login-form, sidebar, kindle page to use server-side auth
- Remove client-side auth interceptor from axios client
- Rename NEXT_PUBLIC_SUPABASE_* → server-only SUPABASE_* env vars
- Delete browser Supabase client (lib/supabase/client.ts)
- Set up Vitest for web package (22 unit tests, 20 integration tests)
- Add Husky + lint-staged + Prettier pre-commit hook

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevent token exfiltration via crafted login URLs by validating that
mcp_redirect points to a trusted origin (API_URL or localhost) before
appending access/refresh tokens. Applied in all three redirect paths:
actions.ts, middleware.ts, auth/callback/route.ts.

Addresses review feedback from Lumen on PR #16.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace hardcoded localhost port numbers with constants derived
from PCP_PORT_BASE so tests work across different git worktrees.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the pendingAuths Map with stateless signed JWTs. The pending_id
is now a JWT containing clientId, codeChallenge, redirectUri, and state,
signed with JWT_SECRET. This eliminates in-memory state for horizontal
scaling and removes the browser-supplied `redirect` URL parameter — the
web portal now constructs the MCP callback from its own API_URL env var.

- Remove isAllowedMcpRedirect() and validate-redirect module entirely
- Web portal treats pending_id as opaque (no JWT_SECRET needed)
- PKCE prevents replay without one-time-use tracking
- Auth codes Map kept as-is (sensitive tokens, consumed within seconds)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
refreshSession() on a shared Supabase client overwrites the Authorization
header from service_role to a user JWT, silently subjecting all subsequent
PostgREST queries to RLS. This caused "Failed to create user account"
errors in handleAuthCallback after any MCP token refresh.

Fix: use a throwaway Supabase client for refreshSession() so the main
client's auth state is never contaminated. Also disable autoRefreshToken
on the data layer singleton as a safety measure.

Added prominent warnings to both Supabase client sites documenting the
safe/unsafe method boundary.

Ref: https://github.com/orgs/supabase/discussions/30146

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Supabase JWT passthrough with self-signed JWTs for MCP access tokens.
Supabase is now only used for initial identity verification during login —
all subsequent token operations (sign, verify, refresh) are local.

- Issue JWTs signed with JWT_SECRET (30-day expiry) instead of forwarding Supabase JWTs
- Eliminate refreshSession() entirely — no more Supabase calls on token refresh
- Local jwt.verify() replaces network round-trip to GoTrue on every MCP request
- Remove refresh_token from browser callback URLs (security: no tokens in URLs)
- Migration: make supabase_refresh_token nullable for transition

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
If a client explicitly provides a client_id during token exchange that
differs from the one bound at /authorize, reject with invalid_grant.
Fallback to stored client_id still allowed when client_id is omitted
(needed for Codex compatibility).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant