Skip to content

Secure chat authentication and billing#39

Draft
MaxGhenis wants to merge 1 commit intomainfrom
codex/security-audit-fixes
Draft

Secure chat authentication and billing#39
MaxGhenis wants to merge 1 commit intomainfrom
codex/security-audit-fixes

Conversation

@MaxGhenis
Copy link
Copy Markdown
Contributor

Summary

  • require a Supabase bearer token for chat, title, billing, and private conversation routes
  • derive user_id and email from the verified token instead of trusting request bodies or query strings
  • scope conversation CRUD/share/report and billing usage to the authenticated user
  • send the Supabase access token from the frontend and stop sending client-supplied user IDs

Security impact

Closes the audit findings where callers could impersonate users, bypass billing with anonymous chat requests, and access or mutate other users' conversation records.

Validation

  • python -m py_compile routes/auth.py routes/supabase_client.py routes/billing.py routes/chatbot.py routes/conversations.py tests/test_api.py
  • Targeted pytest was attempted, but local execution is blocked by missing pydantic_ai in the checkout environment.

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
policyengine-uk-chat Ready Ready Preview, Comment Apr 23, 2026 10:08pm

Request Review

@github-actions
Copy link
Copy Markdown

Beta preview is ready.

@vahid-ahmadi
Copy link
Copy Markdown
Contributor

vahid-ahmadi commented Apr 24, 2026

Reviewed the diff and a couple of neighbouring files. The direction is clearly right and I'd merge this after the blocker below is fixed — without it, production (and local dev without NEXT_PUBLIC_BACKEND_URL) will 401 on every authenticated call.

🔴 Blocker — Next.js proxy drops the Authorization header

frontend/src/app/api/proxy/[...slug]/route.ts line 59 hardcodes the forwarded headers:

const fetchOptions: RequestInit = { method, headers: { \"Content-Type\": \"application/json\" }, redirect: \"follow\" };

In frontend/src/utils/backend.ts, getBackendBase() falls back to /api/proxy for any hostname that doesn't match the Vercel preview regex. That means:

  • Preview (policyengine-uk-chat-git-*-policy-engine.vercel.app) → direct to Modal, Authorization passes through, tests green. ✅
  • Production / any host not matching the regex / local dev without NEXT_PUBLIC_BACKEND_URL/api/proxy, proxy strips Authorization, backend returns 401 on every protected route. ❌

That's why this PR's preview checks pass despite the bug — the preview URL bypasses the proxy. Suggested fix:

const incomingAuth = request.headers.get(\"authorization\");
const headers: Record<string, string> = { \"Content-Type\": \"application/json\" };
if (incomingAuth) headers.Authorization = incomingAuth;
const fetchOptions: RequestInit = { method, headers, redirect: \"follow\" };

🟡 Sync Supabase get_user() in async handler

routes/auth.py:require_user calls get_supabase().auth.get_user(token) — the supabase-py auth client uses a sync httpx.Client. chat_message is async def, so every request now blocks the event loop for the round-trip to Supabase (~50–200 ms) before any chat work starts. Under concurrency this serialises the whole streaming endpoint.

Options, roughly cheapest first:

  1. await asyncio.to_thread(get_supabase().auth.get_user, token) inside require_user (make it async).
  2. Verify the Supabase-issued JWT locally with PyJWT + SUPABASE_JWT_SECRET — avoids the network call entirely and is what most production backends do. Happy to scope this as a follow-up since it's an optimisation, not a correctness bug.

🟡 save_conversation silently claims legacy anonymous rows

routes/conversations.py:107:

if existing.user_id and existing.user_id != current_user.id:
    raise HTTPException(status_code=403, detail=\"Not your conversation\")
...
existing.user_id = current_user.id

If existing.user_id is None (a pre-PR anonymous row), the check short-circuits and the row gets reassigned to whoever authenticates with that session_id first. share/report/delete/get all use the stricter row.user_id != current_user.id (None → 403). Risk is low — session_id is a UUID — but the asymmetry is surprising, and a legacy row's original anonymous author can't complain. I'd either:

  • apply the same strict check everywhere, or
  • if the intent is "first authenticated saver claims their own legacy thread", leave a comment explaining the migration intent.

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.

2 participants