Skip to content

[recipes] OAuth MCP upgrade and remote auth hardening#143

Closed
justfinethanku wants to merge 1 commit into
mainfrom
contrib/jonathanedwards/oauth-mcp-hardening
Closed

[recipes] OAuth MCP upgrade and remote auth hardening#143
justfinethanku wants to merge 1 commit into
mainfrom
contrib/jonathanedwards/oauth-mcp-hardening

Conversation

@justfinethanku
Copy link
Copy Markdown
Collaborator

Summary

  • add a standalone recipes/oauth-mcp-upgrade migration path for existing OB1 installs
  • add dashboards/open-brain-auth-portal, a minimal Next.js login and consent portal for Supabase-backed MCP OAuth flows
  • harden the core MCP server, extension MCP servers, and the meal-planning shared server around Authorization: Bearer <token> with Supabase owner-user validation and optional legacy key fallback
  • update the Svelte dashboard MCP proxy to forward the signed-in Supabase bearer token instead of a static MCP key
  • rewrite the core setup and remote-MCP docs to make OAuth the default path, with legacy key auth kept only as a migration appendix

Verification

  • deno check --config server/deno.json server/index.ts
  • deno check --config integrations/kubernetes-deployment/deno.json integrations/kubernetes-deployment/index.ts
  • deno check --config extensions/meal-planning/deno.json extensions/meal-planning/shared-server.ts
  • deno check --config extensions/<extension>/deno.json extensions/<extension>/index.ts for household-knowledge, home-maintenance, family-calendar, meal-planning, professional-crm, and job-hunt
  • npm install
  • NEXT_PUBLIC_SUPABASE_URL=https://example.supabase.co NEXT_PUBLIC_SUPABASE_PUBLISHABLE_KEY=sb_publishable_dummy npm run build
  • NEXT_PUBLIC_SUPABASE_URL=https://example.supabase.co NEXT_PUBLIC_SUPABASE_PUBLISHABLE_KEY=sb_publishable_dummy npm run lint
  • PUBLIC_SUPABASE_URL=https://example.supabase.co PUBLIC_SUPABASE_ANON_KEY=sb_publishable_dummy PUBLIC_MCP_URL=https://example.supabase.co/functions/v1/open-brain-mcp npm run check
  • PUBLIC_SUPABASE_URL=https://example.supabase.co PUBLIC_SUPABASE_ANON_KEY=sb_publishable_dummy PUBLIC_MCP_URL=https://example.supabase.co/functions/v1/open-brain-mcp npm run build

Notes

  • the self-hosted Kubernetes integration now prefers Authorization: Bearer ... but does not implement full Supabase OAuth; it keeps bearer-key auth as the scoped self-hosted variant
  • dashboards/open-brain-dashboard-next remains on its separate REST API-key auth model and is only documented as out of scope for this MCP migration

Tester Checklist

  • fresh install path tested
  • migration path tested
  • Claude tested
  • ChatGPT tested
  • legacy fallback disable tested

Follow-up

  • Full per-user RLS / user-scoped OB1 is intentionally deferred to a separate redesign.

@github-actions github-actions Bot added dashboard Contribution: frontend template documentation Improvements or additions to documentation extension Contribution: curated learning path build integration Contribution: MCP extension or capture source primitive Contribution: reusable concept guide recipe Contribution: step-by-step recipe labels Mar 31, 2026
Copy link
Copy Markdown
Collaborator

@matthallett1 matthallett1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: [recipes] OAuth MCP upgrade and remote auth hardening (#143)

48 files, ~8,400 additions. Single commit. Three-pass review: structured checklist, security adversarial, and completeness/logic analysis.

Scope: All files map to the 5 stated goals. No drift. Consider splitting into 2-3 PRs (auth code, auth portal, docs) for easier bisection.


CRITICAL — Fix before merge

1. Decision route has no auth check and no CSRF protection
dashboards/open-brain-auth-portal/app/api/oauth/decision/route.ts

The API route that approves/denies OAuth authorizations never calls getUser(). The consent page checks auth, but the endpoint doesn't. Combined with no CSRF protection, this is an account-takeover path:

  • Attacker initiates their own OAuth flow → gets authorization_id
  • Crafts a page with a hidden form that auto-POSTs decision=approve
  • If the owner visits that page while logged into the auth portal, browser sends session cookies → authorization approved → attacker gets OAuth token

Fix: Add const { data: { user } } = await supabase.auth.getUser() at the top of the handler, return 401 if no user. Add Origin header validation for CSRF.

2. Open redirect on login page
app/login/page.tsx:10-11 + LoginForm.tsx:39

The redirect query param flows directly to router.push(redirectTo) with zero validation. /login?redirect=https://evil.com/phish redirects to attacker site after login. Can be chained with a fake consent page.

Fix: Validate redirectTo starts with / and doesn't contain :// or //.

3. shared-server.ts missing Accept header patch (real bug)
extensions/meal-planning/shared-server.ts

Every other MCP server includes the Claude Desktop Accept header workaround (if (!c.req.header('accept')?.includes('text/event-stream'))...). This file doesn't. Claude Desktop connections to the shared meal-planning endpoint will fail with a transport error.

Fix: Add the same Accept header patching block after the auth check, before transport.handleRequest(c).

4. Module-scoped McpServer with per-request reconnection (concurrency bug)
server/index.ts + integrations/kubernetes-deployment/index.ts

Both create McpServer at module scope but call server.connect(transport) on every request with a new transport. Under concurrent load, the second request's connect() can clobber the first request's transport. The 6 extensions avoid this by creating both McpServer and transport inside the request handler.

Fix: Move McpServer instantiation inside the request handler (matching the extension pattern that already works).

5. Timing-unsafe secret comparison (all server files)
server/index.ts:469, all 6 extensions (~line 108), integrations/kubernetes-deployment/index.ts:451,463

=== is not constant-time. For k8s this is the primary auth mechanism (not a legacy fallback).

Fix: Use crypto.subtle.timingSafeEqual().

6. Dedup SQL silently removed from getting-started guide (regression)
docs/01-getting-started.md

Removes content_fingerprint column, upsert_thought function, and dedup index from database setup. Added by merged PR #116, unrelated to OAuth. New users get incomplete database setup.

Fix: Restore the dedup SQL section.


INFORMATIONAL — Worth addressing

I1. ~120 lines auth middleware duplicated across 8 files — Any auth bug needs 8 fixes. Copies aren't identical (server returns ownerUserId, extensions don't, k8s has different logic entirely). A shared module via Deno import maps or build-time copy would help.

I2. K8s auth diverges from OAuth claim — Uses static Bearer token, no Supabase Auth, no .well-known endpoint. Reports auth: "bearer-token". Not wrong given k8s uses direct PostgreSQL, but needs explicit documentation.

I3. mode and ownerUserId return values are dead code — Never consumed by any caller. Remove or wire up.

I4. Error messages leak env var names"SUPABASE_PUBLISHABLE_KEY not configured" tells attackers about infrastructure. Use generic errors in production.

I5. Client setup instructions gutted without replacements — Specific claude mcp add command, config.toml example, and bridge configs removed. Replaced with vague guidance.

I6. SQL interpolation for days in k8sINTERVAL '${days} days' — Zod-validated but not parameterized. Defense in depth.

I7. "redirect_url" in authDetails duck-type check — Relies on undocumented Supabase return type discrimination. Fragile.

I8. shared-server.ts routes on /mcp while others use * — Undocumented divergence.

I9. Dashboard proxy forwards upstream error bodiesdetails: text in 502 responses could leak stack traces.


Overall

The core auth design is solid. Using authClient.auth.getUser(token) for server-side validation (checks revocation, not just JWT decode) is the right call. The legacy fallback gated by ALLOW_LEGACY_MCP_KEY is a clean migration path. The auth portal layout is clean and minimal.

The consent portal needs the auth check + CSRF fix (C1) and open redirect fix (C2) before merge. The shared-server.ts missing Accept patch (C3) will break Claude Desktop. The McpServer concurrency issue (C4) will manifest under any concurrent usage.

Nice contribution — the OAuth migration recipe is well-written and the credential tracker pattern is a good UX touch.

@matthallett1
Copy link
Copy Markdown
Collaborator

Review tooling: This review was produced by Claude Code (Opus 4.6) using the gstack /review workflow. Three parallel analysis passes ran against the full diff:

  1. Structured checklist pass — Two-pass review (Critical + Informational) against the gstack pre-landing checklist, focused on SQL safety, race conditions, auth trust boundaries, and enum completeness
  2. Security adversarial agent — Claude subagent tasked with breaking the auth code. Found the CSRF/missing-auth on the consent endpoint, the open redirect, and the timing-unsafe comparisons
  3. Completeness/logic agent — Claude subagent checking for copy-paste divergences, missing error handling, and dead code across all 8 server files. Found the shared-server.ts Accept header bug and the McpServer concurrency issue

A fourth agent (architecture/DRY review) was still running at time of posting. Codex cross-model review was not used (worktree execution issues in this session).

@matthallett1
Copy link
Copy Markdown
Collaborator

Follow-up: Architecture review (additional findings)

A fourth analysis pass (architecture/DRY) completed after the main review was posted. Three new findings not covered above:

I10. SUPABASE_PUBLISHABLE_KEY vs SUPABASE_ANON_KEY naming conflict
The PR introduces SUPABASE_PUBLISHABLE_KEY across 8 server files, the auth portal, and docs. But the existing Svelte dashboard in the same repo still uses PUBLIC_SUPABASE_ANON_KEY, and the Supabase ecosystem universally calls this the "anon" key. Users setting up OB1 will see "publishable key" in the guide but "anon public" in the Supabase Dashboard. Pick one name and use it everywhere, or add a mapping note: "This is the key Supabase calls the 'anon public' key in the Dashboard."

I11. Migration recipe doesn't mention redeploying extensions
recipes/oauth-mcp-upgrade/README.md Step 5 only shows redeploying the core server (server/index.ts), but this PR also updates all 6 extension servers with the same auth middleware. Users who have deployed extensions need to redeploy those too with the same new secrets (SUPABASE_PUBLISHABLE_KEY, OB1_OWNER_USER_ID, ALLOW_LEGACY_MCP_KEY). Add a note.

I12. shared-server.ts name is misleading
extensions/meal-planning/shared-server.ts is not a shared module — nothing imports it. It's a standalone deployable Edge Function with its own Deno.serve() call, intended for household members with limited access. The name suggests it's the shared auth module that other files should import. Rename to household-server.ts or similar to avoid confusion.

@justfinethanku
Copy link
Copy Markdown
Collaborator Author

Thanks for the work on this. I’m going to close this PR because it isn’t a fit for OB1 in this form.

The main blocker is scope: this rewrites core MCP infrastructure and spans recipes, dashboards, integrations, primitives, and curated extensions in one PR. OB1’s contribution model doesn’t support community PRs that modify the core server in that way, and the contribution boundary here is too broad to merge safely.

There are ideas in here worth salvaging, but not as a single PR against this repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dashboard Contribution: frontend template documentation Improvements or additions to documentation extension Contribution: curated learning path build integration Contribution: MCP extension or capture source primitive Contribution: reusable concept guide recipe Contribution: step-by-step recipe

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants