fix(mcp): accept customer wm_ API keys on /mcp + guard pro-token validate (#4859, #4860)

koala73 · koala73 · commit 470ea64dd86f · 2026-07-05T17:17:46.000+04:00
Two P0s from a paying-customer ticket (2026-07-05): 1. #4859 — /mcp validated X-WorldMonitor-Key ONLY against the WORLDMONITOR_VALID_KEYS env allowlist, so every dashboard-issued key (Convex userApiKeys) got 401 -32001 "Invalid API key" while the same keys worked on REST — and the endpoint's own 401 hint told users to send exactly that header. New `user_key` context kind: env-miss falls back to the gateway-shared validateUserApiKey hash lookup; gated methods enforce the OWNER's mcpAccess entitlement via the same fail-closed gate as the pro path (never mapped onto env_key, which skips pre-checks); per-user 60/min limiter + the pro daily quota (cache tools bypass downstream metering, so an unquota'd user_key would be an unmetered loophole); _execute fetches authenticate downstream with the raw key so REST metering attributes to the owner. 2. #4860 — `await deps.validateProMcpToken(...)` in runProPreChecks was the only unguarded await on the gated path with no top-level catch behind it: a rejection escaped as a raw 500 on every tools/call (tools/list unaffected, zero Sentry) — the exact fingerprint of the customer's second symptom. Now caught -> captureSilentError (step: pro-token-validate) -> structured 503 + Retry-After. Makes the documented contract (docs/mcp-overview.mdx auth method 2, mcp-quickstart curl examples) actually true for user-issued keys. Tests: tests/mcp-user-key-auth.test.mjs (10 cases, failing-first); mcp/quota/conformance/transport/telemetry/gateway/oauth suites green (360 tests); typecheck:api + biome clean. Claude-Session: https://claude.ai/code/session_01Ex9zkdxdbxYMogHmEK2YXZ
diff --git a/api/mcp/auth.ts b/api/mcp/auth.ts
@@ -16,6 +16,7 @@ import {
   signInternalMcpRequest,
 } from '../../server/_shared/mcp-internal-hmac';
 import { validateProMcpTokenOrNull } from '../../server/_shared/pro-mcp-token';
+import { validateUserApiKey } from '../../server/_shared/user-api-key';
 import { rpcError, withMcpNoStore } from './rpc';
 import type {
   AuthResolution,
@@ -101,7 +102,11 @@ export async function buildAuthHeaders(
   url: string,
   body: BodyInit | null | undefined,
 ): Promise<Record<string, string>> {
-  if (context.kind === 'env_key') {
+  if (context.kind === 'env_key' || context.kind === 'user_key') {
+    // user_key (#4859): the downstream REST gateway validates the raw key
+    // itself (Convex hash lookup + the #4611 apiAccess gate + per-account
+    // limits), so usage attributes to the key owner exactly like a direct
+    // REST call — no internal-HMAC identity smuggling needed.
     return { 'X-WorldMonitor-Key': context.apiKey };
   }
   // context.kind === 'pro'
@@ -131,6 +136,7 @@ export const PRODUCTION_DEPS: McpHandlerDeps = {
   // to distinguish revoked from transient (F3 of the U7+U8 review pass).
   validateProMcpToken: validateProMcpTokenOrNull,
   getEntitlements,
+  validateUserApiKey,
   redisPipeline: rawRedisPipeline,
 };
 
@@ -188,16 +194,44 @@ export async function resolveAuthContext(
     };
   }
   const validKeys = (process.env.WORLDMONITOR_VALID_KEYS || '').split(',').filter(Boolean);
-  if (!await timingSafeIncludes(candidateKey, validKeys)) {
-    return {
-      ok: false,
-      response: new Response(
-        JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32001, message: 'Invalid API key' } }),
-        { status: 401, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'WWW-Authenticate': wwwAuthHeader(resourceMetadataUrl, 'invalid_token'), ...corsHeaders }) },
-      ),
-    };
+  if (await timingSafeIncludes(candidateKey, validKeys)) {
+    return { ok: true, context: { kind: 'env_key', apiKey: candidateKey } };
   }
-  return { ok: true, context: { kind: 'env_key', apiKey: candidateKey } };
+
+  // #4859: customer-issued dashboard keys (Convex userApiKeys). The env
+  // allowlist above holds only legacy operator keys; every key a user mints
+  // in the dashboard lives in Convex — before this fallback, ALL of them got
+  // "Invalid API key" here while the same keys worked on the REST gateway.
+  // Identity resolution only: the owner's mcpAccess entitlement is enforced
+  // at the gated-method pre-check (runUserKeyPreChecks), symmetric with the
+  // pro path, so a lapsed owner can still list tools but never call them.
+  if (candidateKey.startsWith('wm_')) {
+    let userKey: { userId: string } | null = null;
+    try {
+      userKey = await deps.validateUserApiKey(candidateKey);
+    } catch {
+      // Production validateUserApiKey fail-softs to null; a throw means the
+      // auth backend itself is unreachable — 503 mirrors the bearer path.
+      return {
+        ok: false,
+        response: new Response(
+          JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32603, message: 'Auth service temporarily unavailable. Try again.' } }),
+          { status: 503, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'Retry-After': '5', ...corsHeaders }) },
+        ),
+      };
+    }
+    if (userKey) {
+      return { ok: true, context: { kind: 'user_key', apiKey: candidateKey, userId: userKey.userId } };
+    }
+  }
+
+  return {
+    ok: false,
+    response: new Response(
+      JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32001, message: 'Invalid API key' } }),
+      { status: 401, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'WWW-Authenticate': wwwAuthHeader(resourceMetadataUrl, 'invalid_token'), ...corsHeaders }) },
+    ),
+  };
 }
 
 /**
@@ -227,40 +261,111 @@ export async function runProPreChecks(
     );
   }
 
-  const validation = await deps.validateProMcpToken(context.mcpTokenId);
+  // #4860: this await was the only unguarded step on the gated path — the
+  // wired helper never rejects today, but a rejection here previously escaped
+  // mcpHandler (no top-level catch) as a raw 500 with zero Sentry. Fail
+  // closed with the same retryable 503 shape as the bearer-resolve catch.
+  let validation: Awaited<ReturnType<typeof deps.validateProMcpToken>> = null;
+  try {
+    validation = await deps.validateProMcpToken(context.mcpTokenId);
+  } catch (err) {
+    captureSilentError(err, { tags: { route: 'api/mcp', step: 'pro-token-validate' }, ctx });
+    return new Response(
+      JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32603, message: 'Service temporarily unavailable, retry in a moment.' } }),
+      { status: 503, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'Retry-After': '5', ...corsHeaders }) },
+    );
+  }
   if (!validation || validation.userId !== context.userId) {
     return new Response(
       JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32001, message: 'MCP authorization revoked. Re-authorize at https://worldmonitor.app/mcp-grant.' } }),
       { status: 401, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'WWW-Authenticate': wwwAuthHeader(resourceMetadataUrl, 'invalid_token'), ...corsHeaders }) },
     );
   }
 
+  return checkMcpEntitlementGate(context.userId, deps, resourceMetadataUrl, corsHeaders, 'pro-entitlement-recheck', ctx);
+}
+
+/**
+ * Shared mcpAccess entitlement gate for identity-resolved contexts (pro AND
+ * user_key). Fail-closed per memory `entitlement-signal-server-outlier-sweep`.
+ * Returns null when the owner has an active tier>=1 + mcpAccess entitlement;
+ * a 401 Response otherwise.
+ */
+async function checkMcpEntitlementGate(
+  userId: string,
+  deps: McpHandlerDeps,
+  resourceMetadataUrl: string,
+  corsHeaders: Record<string, string>,
+  sentryStep: string,
+  ctx?: { waitUntil: (p: Promise<unknown>) => void },
+): Promise<Response | null> {
+  const rejected = () => new Response(
+    JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32001, message: 'Subscription not active.' } }),
+    { status: 401, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'WWW-Authenticate': wwwAuthHeader(resourceMetadataUrl, 'invalid_token'), ...corsHeaders }) },
+  );
+
   let ent: Awaited<ReturnType<typeof deps.getEntitlements>> = null;
   try {
-    ent = await deps.getEntitlements(context.userId);
+    ent = await deps.getEntitlements(userId);
   } catch (err) {
-    // Fail-closed per memory `entitlement-signal-server-outlier-sweep`.
-    captureSilentError(err, { tags: { route: 'api/mcp', step: 'pro-entitlement-recheck' }, ctx });
-    return new Response(
-      JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32001, message: 'Subscription not active.' } }),
-      { status: 401, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'WWW-Authenticate': wwwAuthHeader(resourceMetadataUrl, 'invalid_token'), ...corsHeaders }) },
-    );
+    captureSilentError(err, { tags: { route: 'api/mcp', step: sentryStep }, ctx });
+    return rejected();
   }
   const tier = ent?.features?.tier ?? 0;
   const mcpAccess = ent?.features?.mcpAccess === true;
   const validUntil = ent?.validUntil ?? 0;
   if (!ent || tier < 1 || !mcpAccess || validUntil < Date.now()) {
-    return new Response(
-      JSON.stringify({ jsonrpc: '2.0', id: null, error: { code: -32001, message: 'Subscription not active.' } }),
-      { status: 401, headers: withMcpNoStore({ 'Content-Type': 'application/json', 'WWW-Authenticate': wwwAuthHeader(resourceMetadataUrl, 'invalid_token'), ...corsHeaders }) },
-    );
+    return rejected();
+  }
+  return null;
+}
+
+/**
+ * user_key (#4859) pre-check: the key row proved identity at auth-resolution
+ * time; data methods must additionally verify the OWNER still has an active
+ * mcpAccess entitlement. Without this, a user_key context would be the one
+ * credential class that skips the entitlement gate (env_key is operator-owned
+ * and intentionally ungated; pro re-checks on every gated call).
+ */
+export async function runUserKeyPreChecks(
+  context: Extract<McpAuthContext, { kind: 'user_key' }>,
+  deps: McpHandlerDeps,
+  resourceMetadataUrl: string,
+  corsHeaders: Record<string, string>,
+  ctx?: { waitUntil: (p: Promise<unknown>) => void },
+): Promise<Response | null> {
+  return checkMcpEntitlementGate(context.userId, deps, resourceMetadataUrl, corsHeaders, 'user-key-entitlement', ctx);
+}
+
+/**
+ * Kind-dispatched pre-checks for gated (data/quota) methods. env_key needs
+ * none; pro and user_key each run their own. Single entry point so a future
+ * context kind can't silently ship without deciding its gate (the tracer
+ * finding on #4859: mapping user keys onto env_key would have bypassed
+ * entitlements entirely).
+ */
+export async function runContextPreChecks(
+  context: McpAuthContext,
+  deps: McpHandlerDeps,
+  resourceMetadataUrl: string,
+  corsHeaders: Record<string, string>,
+  ctx?: { waitUntil: (p: Promise<unknown>) => void },
+): Promise<Response | null> {
+  if (context.kind === 'pro') {
+    return runProPreChecks(context, deps, resourceMetadataUrl, corsHeaders, ctx);
+  }
+  if (context.kind === 'user_key') {
+    return runUserKeyPreChecks(context, deps, resourceMetadataUrl, corsHeaders, ctx);
   }
   return null;
 }
 
 /** Per-minute rate limit. Both paths fail-OPEN on Upstash error (graceful);
  *  the daily quota is the hard-cap fail-CLOSED gate. Returns null on success
- *  or pass-through, a Response on a real 60/min limit hit. */
+ *  or pass-through, a Response on a real 60/min limit hit.
+ *  user_key (#4859) shares the per-USER limiter with pro — the principal is
+ *  the key OWNER, so a user with an OAuth connection and a dashboard key gets
+ *  one combined 60/min budget instead of two stackable ones. */
 export async function applyPerMinuteLimit(context: McpAuthContext, headers: Record<string, string> = {}): Promise<Response | null> {
   if (context.kind === 'env_key') {
     const rl = getMcpRatelimit();
@@ -275,7 +380,7 @@ export async function applyPerMinuteLimit(context: McpAuthContext, headers: Reco
   if (!rl) return null;
   try {
     const { success } = await rl.limit(`pro-user:${context.userId}`);
-    if (!success) return rpcError(null, -32029, 'Rate limit exceeded. Max 60 requests per minute per Pro user.', headers);
+    if (!success) return rpcError(null, -32029, 'Rate limit exceeded. Max 60 requests per minute per user.', headers);
   } catch { /* graceful degradation */ }
   return null;
 }
diff --git a/api/mcp/dispatch.ts b/api/mcp/dispatch.ts
@@ -134,7 +134,12 @@ export async function dispatchToolsCall(
   // 50/day cap from even seeing tool definitions. Exempt by name; rate-
   // limiter (60/min) still applies as the abuse guard.
   const isMetadataTool = p.name === 'describe_tool';
-  if (context.kind === 'pro' && !isMetadataTool) {
+  // user_key (#4859) consumes the same per-user daily quota as pro: cache
+  // tools read Upstash directly (no downstream gateway metering), so an
+  // unquota'd user_key would be an unmetered data loophole bounded only by
+  // the 60/min limiter. Raising API-plan MCP allowances above the Pro cap is
+  // a deliberate follow-up, not a default.
+  if ((context.kind === 'pro' || context.kind === 'user_key') && !isMetadataTool) {
     const reservation = await reserveQuota(context.userId, deps.redisPipeline);
     if (!reservation.ok) {
       if (reservation.reason === 'cap-exceeded') {
diff --git a/api/mcp/handler.ts b/api/mcp/handler.ts
@@ -5,7 +5,7 @@ import {
   applyPerMinuteLimit,
   PRODUCTION_DEPS,
   resolveAuthContext,
-  runProPreChecks,
+  runContextPreChecks,
   wwwAuthHeader,
 } from './auth';
 import {
@@ -399,10 +399,8 @@ export async function mcpHandler(
     }
     const auth = await resolveAuthContext(req, deps, resourceMetadataUrl, corsHeaders);
     if (!auth.ok) return auth.response;
-    if (auth.context.kind === 'pro') {
-      const proCheck = await runProPreChecks(auth.context, deps, resourceMetadataUrl, corsHeaders, ctx);
-      if (proCheck) return proCheck;
-    }
+    const getPreCheck = await runContextPreChecks(auth.context, deps, resourceMetadataUrl, corsHeaders, ctx);
+    if (getPreCheck) return getPreCheck;
     const getLimited = await applyPerMinuteLimit(auth.context, corsHeaders);
     if (getLimited) return getLimited;
     return handleSseReplay(req, corsHeaders);
@@ -467,10 +465,8 @@ export async function mcpHandler(
     const auth = await resolveAuthContext(req, deps, resourceMetadataUrl, corsHeaders);
     if (!auth.ok) return auth.response;
     context = auth.context;
-    if (context.kind === 'pro') {
-      const proCheck = await runProPreChecks(context, deps, resourceMetadataUrl, corsHeaders, ctx);
-      if (proCheck) return proCheck;
-    }
+    const preCheck = await runContextPreChecks(context, deps, resourceMetadataUrl, corsHeaders, ctx);
+    if (preCheck) return preCheck;
     const limited = await applyPerMinuteLimit(context, corsHeaders);
     if (limited) return limited;
   }
diff --git a/api/mcp/telemetry.ts b/api/mcp/telemetry.ts
@@ -71,11 +71,12 @@ export const MCP_TOOLS_LIST_TELEMETRY_KEYS = Object.freeze([
 ] as const);
 
 // Log-safe principal id derived from the resolved auth context:
-//   - Pro:     raw Clerk `userId` (internal ID, not a secret; matches the
-//              REST gateway's `customer_id` convention).
+//   - Pro / user_key: raw Clerk `userId` (internal ID, not a secret; matches
+//              the REST gateway's `customer_id` convention — user_key carries
+//              the resolved key OWNER, #4859).
 //   - env_key: FNV-64 hash of the API key (secret — never log raw key
 //              material; mirrors `principal_id` in
 //              server/_shared/usage-identity.ts).
 export function principalIdForLog(context: McpAuthContext): string {
-  return context.kind === 'pro' ? context.userId : hashKeySync(context.apiKey);
+  return context.kind === 'env_key' ? hashKeySync(context.apiKey) : context.userId;
 }
diff --git a/api/mcp/types.ts b/api/mcp/types.ts
@@ -11,7 +11,14 @@
 
 export type McpAuthContext =
   | { kind: 'env_key'; apiKey: string }
-  | { kind: 'pro'; userId: string; mcpTokenId: string };
+  | { kind: 'pro'; userId: string; mcpTokenId: string }
+  // Customer-issued dashboard key (Convex userApiKeys, #4859). Carries BOTH
+  // the raw key (downstream _execute fetches authenticate as the owner via
+  // X-WorldMonitor-Key, so REST metering/limits attribute to them) AND the
+  // resolved owner userId (per-user rate limit + daily quota + the mcpAccess
+  // entitlement pre-check — a user_key context must NEVER skip that gate the
+  // way env_key does).
+  | { kind: 'user_key'; apiKey: string; userId: string };
 
 // ---------------------------------------------------------------------------
 // Tool registry types
@@ -229,6 +236,11 @@ export interface McpHandlerDeps {
   resolveBearerToContext: (token: string) => Promise<McpAuthContext | null>;
   validateProMcpToken: (tokenId: string) => Promise<{ userId: string } | null>;
   getEntitlements: (userId: string) => Promise<{ planKey?: string; features: { tier: number; mcpAccess?: boolean }; validUntil: number } | null>;
+  // #4859: Convex userApiKeys hash lookup (same shared helper as the REST
+  // gateway). Returns the key owner, or null for unknown/revoked keys. The
+  // production impl fail-softs to null internally; a THROW from a dep is
+  // treated as auth-backend-transient (503), mirroring resolveBearerToContext.
+  validateUserApiKey: (key: string) => Promise<{ userId: string } | null>;
   redisPipeline: PipelineFn;
 }
 
diff --git a/tests/helpers/mcp-pro-deps.mjs b/tests/helpers/mcp-pro-deps.mjs
@@ -89,6 +89,10 @@ export function makeProDeps(overrides = {}) {
         features: { tier: 1, mcpAccess: true },
         validUntil: Date.now() + 86_400_000,
       })),
+      // #4859 user-key path. Default rejects every key so pre-existing suites
+      // keep their exact 401 behaviour; user-key tests override with a
+      // fixture-matching resolver.
+      validateUserApiKey: overrides.validateUserApiKey ?? (async () => null),
       redisPipeline: pipe.pipeline,
     },
     pipe,
diff --git a/tests/mcp-user-key-auth.test.mjs b/tests/mcp-user-key-auth.test.mjs
