diff --git a/apps/mcp/src/server/auth/index.test.ts b/apps/mcp/src/server/auth/index.test.ts index 8e8d043ee..02240f3dd 100644 --- a/apps/mcp/src/server/auth/index.test.ts +++ b/apps/mcp/src/server/auth/index.test.ts @@ -1,6 +1,11 @@ import { createLocalJWKSet, exportJWK, generateKeyPair, SignJWT } from "jose" import { afterEach, beforeAll, describe, expect, it, vi } from "vitest" -import { fetchSession, validateApiKey, validateOAuthToken } from "./index" +import { + fetchSession, + TransientAuthError, + validateApiKey, + validateOAuthToken, +} from "./index" const API_URL = "https://api.example.com" const ISSUER = `${API_URL}/api/auth` @@ -180,4 +185,34 @@ describe("MCP authentication", () => { await expect(validateApiKey("not_a_key", API_URL)).resolves.toBeNull() expect(fetchSpy).not.toHaveBeenCalled() }) + + it("surfaces a 500 from the session endpoint as TransientAuthError, not invalid token", async () => { + vi.spyOn(console, "error").mockImplementation(() => {}) + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue(new Response(null, { status: 500 })), + ) + + // fetchSession attaches the upstream status; validateApiKey must + // rethrow it as transient instead of collapsing to null (#1551). + await expect( + validateApiKey("sm_outage_key_0123456789abcdef", API_URL), + ).rejects.toThrow(TransientAuthError) + }) + + it("surfaces a session-endpoint timeout as TransientAuthError", async () => { + vi.spyOn(console, "error").mockImplementation(() => {}) + vi.stubGlobal( + "fetch", + vi.fn().mockRejectedValue( + Object.assign(new Error("The operation was aborted"), { + name: "TimeoutError", + }), + ), + ) + + await expect( + validateApiKey("sm_timeout_key_0123456789abcd", API_URL), + ).rejects.toThrow(TransientAuthError) + }) }) diff --git a/apps/mcp/src/server/auth/index.ts b/apps/mcp/src/server/auth/index.ts index 425f05970..37723fbfb 100644 --- a/apps/mcp/src/server/auth/index.ts +++ b/apps/mcp/src/server/auth/index.ts @@ -65,6 +65,23 @@ export function isApiKey(token: string): boolean { return API_KEY_PATTERN.test(token) } +/** + * API-key validation failed for a reason that is NOT an invalid key: + * network errors, timeouts, or a 5xx from the session endpoint. Callers must + * surface this as a temporary upstream failure instead of reporting + * "invalid token" (which makes MCP clients discard perfectly valid keys and + * push users through re-authentication during outages). + */ +export class TransientAuthError extends Error { + readonly status?: number + + constructor(message: string, status?: number) { + super(message) + this.name = "TransientAuthError" + this.status = status + } +} + export async function validateApiKey( token: string, apiUrl: string, @@ -93,6 +110,23 @@ export async function validateApiKey( return user } catch (error) { console.error("API key validation error:", error) + // Distinguish "bad key" from "session endpoint unavailable": only the + // former should collapse to null (-> 401 invalid_token). fetchSession + // attaches the upstream status to its errors; timeouts surface as + // AbortError/TimeoutError. + const status = (error as { status?: unknown } | null)?.status + if (typeof status === "number" && status !== 401 && status !== 403) { + throw new TransientAuthError( + `Session endpoint returned ${status}`, + status, + ) + } + if ( + error instanceof Error && + (error.name === "AbortError" || error.name === "TimeoutError") + ) { + throw new TransientAuthError("Session request timed out") + } return null } } diff --git a/apps/mcp/src/server/index.ts b/apps/mcp/src/server/index.ts index 0fac82c71..8da94432b 100644 --- a/apps/mcp/src/server/index.ts +++ b/apps/mcp/src/server/index.ts @@ -4,6 +4,7 @@ import { Hono, type Context } from "hono" import { cors } from "hono/cors" import { isApiKey, + TransientAuthError, validateApiKey, validateOAuthToken, type AuthUser, @@ -128,6 +129,33 @@ function authInfoFor( } } +type AuthResolution = + | { ok: true; user: AuthUser } + | { ok: false; reason: "invalid" } + | { ok: false; reason: "transient" } + +/** + * Resolve the request's bearer token to an AuthUser, keeping a transient + * upstream failure distinct from an invalid token (#1551). + */ +async function resolveAuthUser( + token: string, + apiUrl: string, + mcpResource: string, +): Promise { + try { + const user = isApiKey(token) + ? await validateApiKey(token, apiUrl) + : await validateOAuthToken(token, apiUrl, mcpResource) + return user ? { ok: true, user } : { ok: false, reason: "invalid" } + } catch (error) { + if (error instanceof TransientAuthError) { + return { ok: false, reason: "transient" } + } + throw error + } +} + function unauthorizedResponse( resourceMetadataUrl: string, invalidToken = false, @@ -181,10 +209,26 @@ async function handleMcpRequest( if (!token) return unauthorizedResponse(resourceMetadataUrl) - const authUser = isApiKey(token) - ? await validateApiKey(token, apiUrl) - : await validateOAuthToken(token, apiUrl, mcpResource) - if (!authUser) return unauthorizedResponse(resourceMetadataUrl, true) + const resolved = await resolveAuthUser(token, apiUrl, mcpResource) + if (!resolved.ok && resolved.reason === "transient") { + // A transient session-endpoint outage must not be reported as an + // invalid token — that makes clients discard valid sm_ keys and + // re-authenticate. Tell them to retry instead. + return Response.json( + { + jsonrpc: "2.0", + error: { + code: -32001, + message: + "Authentication backend temporarily unavailable, please retry", + }, + id: null, + }, + { status: 503, headers: { "Retry-After": "5" } }, + ) + } + if (!resolved.ok) return unauthorizedResponse(resourceMetadataUrl, true) + const authUser = resolved.user const actor: ActorContext = { userId: authUser.userId,