fix(auth): PLT-1318 verify Supabase JWT signature before trusting accountId

[ashwinsingh2007]

Summary

Fixes PLT-1318 — a Supabase-JWT authentication bypass in the MCP server.

The JWT auth path (resolveApiKey → decodeJwtPayload) decoded the token payload client-side and trusted it without verifying the signature or expiry. A forged, unsigned token carrying an attacker-chosen app_metadata.accountId resolved straight to that account's Browserless API key via PostgREST — a full account takeover with no credentials.

Reproduction (before fix)

A 3-segment token with a real-looking header, an attacker-chosen accountId, and a garbage signature returned the victim's blsk_ key. No signature check existed anywhere in the path.

Fix

src/lib/account-resolver.ts:

• resolveApiKey now verifies the access token against Supabase Auth (GET ${SUPABASE_URL}/auth/v1/user) — hosted Supabase in prod, the local Supabase stack in dev; same REST surface. Supabase validates the signature, expiry, and revocation server-side.
• accountId is read only from the verified response, never from the client-decoded payload.
• Resolver cache is keyed on the token hash, so only tokens that already passed verification can populate it — a forged token can't hit a warm entry for an account it doesn't own.
• Also closes the pre-existing missing-exp gap: expired tokens are now rejected.

src/lib/utils.ts:

• Removed decodeJwtPayload (the decode-without-verify footgun) and its now-unused type import.

Verification

• Executable reproducer against the rebuilt code: forged token → rejected at Supabase Auth (401), PostgREST account lookup never reached, no key leak; a genuine token still resolves correctly.
• test/lib/account-resolver.spec.ts rewritten to model the verify-then-lookup flow, with a new explicit forged/rejected-token test asserting no account lookup and no key leak.
• Full suite 469 passing, npm run lint clean, npm run build clean.

Notes for reviewers

• Adds one round trip to Supabase Auth per cold token; warm tokens hit the existing 5-min cache (now keyed on token hash). No new dependencies.
• Dependency-free by design — did not pull in a JWT library (keeps the supply-chain surface flat; see PLT-1354).

🤖 Generated with Claude Code

Summary by CodeRabbit

• Security Improvements

  • Access tokens are now verified through Supabase Auth before using any account information (no client-side JWT payload decoding).
  • Verification requests use a bounded timeout and validate JWT structure early.
  • Verification and account resolution caching were improved; caches are cleared together.

• Bug Fixes

  • Account identifier is derived strictly from verified Auth results, ensuring safer account-to-key resolution.
  • Account lookup remains cached to reduce repeat PostgREST requests.

• Tests

  • Expanded coverage for endpoint headers, call ordering (Auth vs. lookup), abort/timeout behavior, and negative/error scenarios.

[coderabbitai]

Walkthrough

resolveApiKey now verifies Supabase access tokens through Auth, derives account IDs from verified metadata, caches verification and account lookups, and resolves account details through PostgREST. Client-side JWT decoding was removed, with tests covering requests, failures, caching, timeouts, and malformed tokens.

Changes

Account resolution

Layer / File(s)	Summary
Token verification and caching src/lib/account-resolver.ts, src/lib/utils.ts	Access tokens are shape-checked and verified through Supabase Auth with bounded fetches; verified account metadata drives PostgREST lookup and account-based caching, while client-side JWT decoding is removed.
Resolver flow validation test/lib/account-resolver.spec.ts	Tests validate Auth and PostgREST requests, cached verification and account lookups, timeout signals, rejected or malformed tokens, missing account IDs, and PostgREST failures.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Resolver
  participant SupabaseAuth
  participant PostgREST
  Client->>Resolver: Provide access token
  Resolver->>SupabaseAuth: Verify token at /auth/v1/user
  SupabaseAuth-->>Resolver: Return verified accountId metadata
  Resolver->>PostgREST: Query account by accountId
  PostgREST-->>Resolver: Return api_key and email
  Resolver-->>Client: Return resolved account details

Loading

Poem

I’m a rabbit guarding keys tonight,
Auth checks tokens by moonlit light.
Cached accounts rest along the trail,
PostgREST returns the matching detail.
Hop, hop—false JWTs fail!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly matches the main change: verifying Supabase JWTs before trusting accountId.
Description check	✅ Passed	The description covers summary, changes, verification, and notes; only template sections like related issues/test plan/checklist are missing.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch security/plt-1318-verify-supabase-jwt-signature

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/lib/account-resolver.ts (1)

2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

SupabaseJwtPayload is misapplied to the verified Auth API response, not a JWT payload.

SupabaseJwtPayload (per src/@types/types.d.ts) models a decoded JWT payload, but here it types the JSON body returned by /auth/v1/user — a different shape (e.g. the real field is id, not sub). Happens to work because only app_metadata.accountId is read, but it's misleading given the doc comment's explicit warning against trusting decoded JWT payloads — a future reader could mistake this typed value for an unverified decode.

♻️ Suggested fix: introduce a dedicated response type

-import type { SupabaseJwtPayload } from '../@types/types.js';
+import type { SupabaseAuthUser } from '../@types/types.js';

-  const user = (await response.json()) as SupabaseJwtPayload;
+  const user = (await response.json()) as SupabaseAuthUser;

Also applies to: 49-58

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/account-resolver.ts` around lines 2 - 3, Replace the
SupabaseJwtPayload annotation used for the verified Auth API user response in
the account-resolution flow with a dedicated type representing the
`/auth/v1/user` response, including its actual `id` and nested `app_metadata`
shape. Define or reuse this response type near the resolver and update the
related response variable and `app_metadata.accountId` access without changing
the existing resolution behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/account-resolver.ts`:
- Around line 34-40: Update the Supabase Auth verification fetch in
resolveApiKey to use an AbortController-based timeout, aborting the request
after the configured timeout and preserving the existing request headers and
response handling. Ensure the timeout is cleaned up when the fetch completes so
resources are not retained.
- Around line 14-22: The account resolver’s token cache must not bypass Supabase
validation or rely on a truncated hash key. Update the cache-hit path in the
account resolution logic to revalidate the access token with Supabase Auth, and
use the full SHA-256 token fingerprint for cache keys; preserve returning the
verified account/API key only after successful validation.

---

Nitpick comments:
In `@src/lib/account-resolver.ts`:
- Around line 2-3: Replace the SupabaseJwtPayload annotation used for the
verified Auth API user response in the account-resolution flow with a dedicated
type representing the `/auth/v1/user` response, including its actual `id` and
nested `app_metadata` shape. Define or reuse this response type near the
resolver and update the related response variable and `app_metadata.accountId`
access without changing the existing resolution behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e562507-d961-4541-bfc8-ebdafdf40660

📥 Commits

Reviewing files that changed from the base of the PR and between 89d254f and c8e312f.

📒 Files selected for processing (3)

• src/lib/account-resolver.ts
• src/lib/utils.ts
• test/lib/account-resolver.spec.ts

💤 Files with no reviewable changes (1)

• src/lib/utils.ts

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/account-resolver.ts (1)

12-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Timeout guard now in place — resolves prior major issue.

fetchWithTimeout correctly bounds every Supabase call with an AbortController and clears the timer in finally, addressing the previously flagged unbounded fetch to /auth/v1/user.

One optional improvement: an aborted call surfaces as a raw AbortError to callers, indistinguishable from other network failures. Consider mapping it to an explicit timeout error for clearer diagnostics.

♻️ Optional: surface a clear timeout error

 async function fetchWithTimeout(
   url: string,
   init: RequestInit,
 ): Promise<Response> {
   const controller = new AbortController();
   const timer = setTimeout(() => controller.abort(), SUPABASE_TIMEOUT_MS);
   try {
-    return await fetch(url, { ...init, signal: controller.signal });
+    return await fetch(url, { ...init, signal: controller.signal });
+  } catch (err) {
+    if (err instanceof Error && err.name === 'AbortError') {
+      throw new Error(`Request to ${url} timed out after ${SUPABASE_TIMEOUT_MS}ms`);
+    }
+    throw err;
   } finally {
     clearTimeout(timer);
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/account-resolver.ts` around lines 12 - 29, Optionally update
fetchWithTimeout to detect when its AbortController timeout aborts the fetch and
rethrow a clear timeout-specific error, while preserving existing error
propagation for non-timeout failures and timer cleanup.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/account-resolver.ts`:
- Around line 12-29: Optionally update fetchWithTimeout to detect when its
AbortController timeout aborts the fetch and rethrow a clear timeout-specific
error, while preserving existing error propagation for non-timeout failures and
timer cleanup.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5b6c13aa-fd69-4c1b-9f7e-73c39e2220ae

📥 Commits

Reviewing files that changed from the base of the PR and between c8e312f and ffa9a33.

📒 Files selected for processing (2)

• src/lib/account-resolver.ts
• test/lib/account-resolver.spec.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• test/lib/account-resolver.spec.ts