From 3c6cb8b08df2fa941b903ee1ded9046ef4c82603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 28 Jun 2026 15:18:12 +0200 Subject: [PATCH] fix(mcp): record why a client fails to resolve at /authorize CIMD client resolution swallowed every fetch failure as ssrf_blocked and /authorize logged a bare invalid event, so an unreachable metadata host was indistinguishable from a genuine SSRF block and the consent flow returned invalid_client with no diagnosable cause. Split the resolution failure into ssrf_blocked (a real private-host refusal) and fetch_failed (timeout / connect error), carrying the concrete SafeFetchError kind and cause as a short non-secret detail. Thread the resolution reason and detail into the authorize.invalid annotation behind a pinned meta shape, and cache a resolved CIMD document for up to 24h so the consent flow does not re-fetch on every hit. --- src/app/api/mcp/oauth/authorize/route.ts | 47 ++++++- .../mcp/oauth/__tests__/oauth-bridge.test.ts | 79 ++++++++++- src/lib/mcp/oauth/clients.ts | 126 +++++++++++++++--- 3 files changed, 228 insertions(+), 24 deletions(-) diff --git a/src/app/api/mcp/oauth/authorize/route.ts b/src/app/api/mcp/oauth/authorize/route.ts index 906d4677a..1abc2c847 100644 --- a/src/app/api/mcp/oauth/authorize/route.ts +++ b/src/app/api/mcp/oauth/authorize/route.ts @@ -118,16 +118,42 @@ interface ValidatedRequest { resource: string; } +/** + * The gate a request failed, pinned to a stable enum so the + * `mcp.oauth.authorize.invalid` annotation stays dashboard-friendly. The + * swallowed-reason gap (a bare `invalid` event) is exactly why the prod CIMD + * failure was undiagnosable. + */ +type ValidateFailure = + | "invalid_params" + | "client_unresolved" + | "redirect_mismatch" + | "bad_response_type" + | "pkce_required" + | "audience_mismatch"; + +type ValidateMeta = { + reason: ValidateFailure; + /** For `client_unresolved`: the `resolveClient` reason. */ + clientReason?: string; + /** For `client_unresolved`: a short, non-secret transport diagnostic. */ + detail?: string; +}; + /** Shared validation for GET + POST. Returns a ready error Response on failure. */ async function validate( params: Record, requestUrl: string, -): Promise<{ ok: true; v: ValidatedRequest } | { ok: false; res: Response }> { +): Promise< + | { ok: true; v: ValidatedRequest } + | { ok: false; res: Response; meta: ValidateMeta } +> { const parsed = z.object(baseParams).safeParse(params); if (!parsed.success) { return { ok: false, res: badRequest("invalid_request", "Missing or malformed parameters"), + meta: { reason: "invalid_params" }, }; } const p = parsed.data; @@ -139,6 +165,11 @@ async function validate( return { ok: false, res: badRequest("invalid_client", "Unknown or unresolvable client"), + meta: { + reason: "client_unresolved", + clientReason: resolved.reason, + ...(resolved.detail ? { detail: resolved.detail } : {}), + }, }; } @@ -150,6 +181,7 @@ async function validate( "invalid_request", "redirect_uri does not match the registered client", ), + meta: { reason: "redirect_mismatch" }, }; } @@ -161,6 +193,7 @@ async function validate( "unsupported_response_type", "Only response_type=code is supported", ), + meta: { reason: "bad_response_type" }, }; } @@ -175,6 +208,7 @@ async function validate( "invalid_request", "PKCE with code_challenge_method=S256 is required", ), + meta: { reason: "pkce_required" }, }; } @@ -186,6 +220,7 @@ async function validate( "invalid_target", "resource must equal this server's MCP endpoint", ), + meta: { reason: "audience_mismatch" }, }; } @@ -280,7 +315,10 @@ export async function GET(request: NextRequest): Promise { const params = Object.fromEntries(url.searchParams.entries()); const result = await validate(params, request.url); if (!result.ok) { - annotate({ action: { name: "mcp.oauth.authorize.invalid" } }); + annotate({ + action: { name: "mcp.oauth.authorize.invalid" }, + meta: result.meta, + }); return result.res; } const { v } = result; @@ -409,7 +447,10 @@ export async function POST(request: NextRequest): Promise { const result = await validate(params, request.url); if (!result.ok) { - annotate({ action: { name: "mcp.oauth.authorize.invalid" } }); + annotate({ + action: { name: "mcp.oauth.authorize.invalid" }, + meta: result.meta, + }); return result.res; } const { v } = result; diff --git a/src/lib/mcp/oauth/__tests__/oauth-bridge.test.ts b/src/lib/mcp/oauth/__tests__/oauth-bridge.test.ts index 29b6a6a99..4e952589b 100644 --- a/src/lib/mcp/oauth/__tests__/oauth-bridge.test.ts +++ b/src/lib/mcp/oauth/__tests__/oauth-bridge.test.ts @@ -8,18 +8,31 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; process.env.API_TOKEN_HMAC_KEY = "x".repeat(48); process.env.APP_URL = "https://health.example"; +// Mirror the real SafeFetchError shape (message + kind + cause) so the +// `instanceof` branch in `resolveCimdClient` exercises the kind→reason mapping. +type SafeFetchErrorKind = "private_host" | "timeout" | "network"; vi.mock("@/lib/safe-fetch", () => ({ safeFetch: vi.fn(), - SafeFetchError: class SafeFetchError extends Error {}, + SafeFetchError: class SafeFetchError extends Error { + readonly kind: SafeFetchErrorKind; + readonly cause?: unknown; + constructor(message: string, kind: SafeFetchErrorKind, cause?: unknown) { + super(message); + this.name = "SafeFetchError"; + this.kind = kind; + if (cause !== undefined) this.cause = cause; + } + }, })); -import { safeFetch } from "@/lib/safe-fetch"; +import { safeFetch, SafeFetchError } from "@/lib/safe-fetch"; import { signArtifact, verifyArtifact } from "../artifacts"; import { s256Challenge, verifyPkceS256, isValidVerifier } from "../pkce"; import { registerDcrClient, resolveClient, redirectUriAllowed, + _resetCimdCacheForTests, } from "../clients"; import { protectedResourceMetadata, @@ -105,7 +118,10 @@ describe("clients — DCR (stateless) round-trip", () => { }); describe("clients — CIMD via safeFetch (SSRF-safe)", () => { - beforeEach(() => vi.clearAllMocks()); + beforeEach(() => { + vi.clearAllMocks(); + _resetCimdCacheForTests(); + }); it("fetches a CIMD document through safeFetch with requirePublicHost", async () => { const clientId = "https://app.example/mcp-client.json"; @@ -144,10 +160,61 @@ describe("clients — CIMD via safeFetch (SSRF-safe)", () => { expect(res).toEqual({ ok: false, reason: "invalid_metadata" }); }); - it("surfaces an SSRF block as ssrf_blocked", async () => { - vi.mocked(safeFetch).mockRejectedValue(new Error("private host")); + it("surfaces a private-host SSRF block as ssrf_blocked", async () => { + vi.mocked(safeFetch).mockRejectedValue( + new SafeFetchError("private host", "private_host"), + ); const res = await resolveClient("https://169.254.169.254/meta.json"); - expect(res).toEqual({ ok: false, reason: "ssrf_blocked" }); + expect(res).toMatchObject({ ok: false, reason: "ssrf_blocked" }); + }); + + it("surfaces an unreachable host as fetch_failed (not ssrf_blocked)", async () => { + // The prod CIMD outage: a transport failure (no IPv6 route) that was + // mislabelled `ssrf_blocked`. It must now resolve to `fetch_failed` and + // carry the concrete kind + cause for the `/authorize` annotation. + vi.mocked(safeFetch).mockRejectedValue( + new SafeFetchError( + "safeFetch network error: connect ENETUNREACH", + "network", + new Error("connect ENETUNREACH 2607:6bc0::1:443"), + ), + ); + const res = await resolveClient("https://claude.ai/.well-known/mcp.json"); + expect(res.ok).toBe(false); + if (!res.ok) { + expect(res.reason).toBe("fetch_failed"); + expect(res.detail).toContain("network"); + expect(res.detail).toContain("ENETUNREACH"); + } + }); + + it("surfaces a timeout as fetch_failed", async () => { + vi.mocked(safeFetch).mockRejectedValue( + new SafeFetchError("safeFetch aborted (TimeoutError)", "timeout"), + ); + const res = await resolveClient("https://slow.example/mcp.json"); + expect(res).toMatchObject({ ok: false, reason: "fetch_failed" }); + }); + + it("caches a resolved CIMD document and does not re-fetch within the TTL", async () => { + const clientId = "https://app.example/mcp-client.json"; + vi.mocked(safeFetch).mockResolvedValue( + new Response( + JSON.stringify({ + client_id: clientId, + client_name: "Cached Client", + redirect_uris: ["https://app.example/cb"], + }), + { status: 200 }, + ), + ); + + const first = await resolveClient(clientId); + const second = await resolveClient(clientId); + expect(first.ok).toBe(true); + expect(second.ok).toBe(true); + // Only the first call hits the network; the second is served from cache. + expect(vi.mocked(safeFetch)).toHaveBeenCalledTimes(1); }); it("rejects a CIMD doc with a non-https / non-loopback redirect (L2)", async () => { diff --git a/src/lib/mcp/oauth/clients.ts b/src/lib/mcp/oauth/clients.ts index 01717c1c1..e6060ddfb 100644 --- a/src/lib/mcp/oauth/clients.ts +++ b/src/lib/mcp/oauth/clients.ts @@ -22,7 +22,7 @@ * `https://claude.ai/api/mcp/auth_callback` hosted callback is matched as an * ordinary registered entry. */ -import { safeFetch } from "@/lib/safe-fetch"; +import { safeFetch, SafeFetchError } from "@/lib/safe-fetch"; import { readBodyCapped } from "@/lib/http/read-capped"; import { ARTIFACT_KINDS, signArtifact, verifyArtifact } from "./artifacts"; @@ -35,16 +35,82 @@ export interface ResolvedClient { source: "cimd" | "dcr"; } +/** + * Why a `client_id` failed to resolve. + * + * - `unknown_client` — not a CIMD URL / DCR id, or the document 404'd. + * - `invalid_metadata` — fetched, but the document is malformed / oversized / + * fails a SEP-991 constraint. + * - `ssrf_blocked` — the SSRF floor refused the host (private / metadata + * range): a genuine policy block. + * - `fetch_failed` — the metadata host could not be reached (timeout, + * connect error, version skew). This is NOT an SSRF + * block; conflating the two is exactly what made the + * prod CIMD outage undiagnosable — every transport + * failure surfaced as `ssrf_blocked / invalid_client`. + * + * `detail` carries a short, non-secret diagnostic string (the + * `SafeFetchError.kind` + message + cause) for the `/authorize` annotation. + */ +export type ClientResolutionReason = + | "unknown_client" + | "invalid_metadata" + | "ssrf_blocked" + | "fetch_failed"; + export type ClientResolution = | { ok: true; client: ResolvedClient } | { ok: false; - reason: "unknown_client" | "invalid_metadata" | "ssrf_blocked"; + reason: ClientResolutionReason; + detail?: string; }; const MAX_REDIRECT_URIS = 12; const CIMD_MAX_BYTES = 16 * 1024; +/** + * SEP-991 recommends caching a resolved CIMD document (≤24h). Re-resolving on + * every `/authorize` hit would re-pay the outbound fetch and widen the SSRF + * surface needlessly; a short cache keeps the consent flow snappy without + * letting a stale document linger. Only SUCCESSFUL resolutions are cached — + * failures must re-try so a transient outage self-heals. + */ +const CIMD_CACHE_TTL_MS = 24 * 60 * 60 * 1000; +const CIMD_CACHE_MAX_ENTRIES = 256; +const cimdCache = new Map< + string, + { client: ResolvedClient; expiresAt: number } +>(); + +function getCachedCimd(clientId: string): ResolvedClient | null { + const hit = cimdCache.get(clientId); + if (!hit) return null; + if (hit.expiresAt <= Date.now()) { + cimdCache.delete(clientId); + return null; + } + return hit.client; +} + +function putCachedCimd(clientId: string, client: ResolvedClient): void { + // Cheap bound: a flood of distinct attacker URLs cannot grow the map without + // limit. When full, drop the oldest insertion (Map preserves insertion order). + if (cimdCache.size >= CIMD_CACHE_MAX_ENTRIES) { + const oldest = cimdCache.keys().next().value; + if (oldest !== undefined) cimdCache.delete(oldest); + } + cimdCache.set(clientId, { + client, + expiresAt: Date.now() + CIMD_CACHE_TTL_MS, + }); +} + +/** Test helper — clears the resolved-CIMD cache between cases. */ +export function _resetCimdCacheForTests(): void { + cimdCache.clear(); +} + function asStringArray(value: unknown): string[] | null { if (!Array.isArray(value)) return null; const out: string[] = []; @@ -142,6 +208,9 @@ function resolveDcrClient(clientId: string): ClientResolution { } async function resolveCimdClient(clientId: string): Promise { + const cached = getCachedCimd(clientId); + if (cached) return { ok: true, client: cached }; + let res: Response; try { // SSRF floor: the metadata host is fully user-controlled, so pin the @@ -151,10 +220,38 @@ async function resolveCimdClient(clientId: string): Promise { { method: "GET", headers: { accept: "application/json" } }, { requirePublicHost: true, timeoutMs: 8000 }, ); - } catch { - return { ok: false, reason: "ssrf_blocked" }; + } catch (err) { + // Distinguish a genuine SSRF policy block from an unreachable host. The + // prod CIMD outage was a transport failure (no IPv6 route to claude.ai) + // mislabelled `ssrf_blocked`; carry the concrete kind + cause so the + // `/authorize` annotation records WHY, instead of a blanket label. + if (err instanceof SafeFetchError) { + const causeMsg = + err.cause instanceof Error + ? err.cause.message + : err.cause !== undefined + ? String(err.cause) + : undefined; + const detail = causeMsg ? `${err.kind}: ${causeMsg}` : err.kind; + return { + ok: false, + reason: err.kind === "private_host" ? "ssrf_blocked" : "fetch_failed", + detail, + }; + } + return { + ok: false, + reason: "fetch_failed", + detail: err instanceof Error ? err.message : String(err), + }; + } + if (!res.ok) { + return { + ok: false, + reason: "unknown_client", + detail: `http_${res.status}`, + }; } - if (!res.ok) return { ok: false, reason: "unknown_client" }; // M3 — enforce the byte cap WHILE reading: reject up front on an oversized // Content-Length and abort the stream the moment it overflows, so a hostile @@ -181,17 +278,16 @@ async function resolveCimdClient(clientId: string): Promise { return { ok: false, reason: "invalid_metadata" }; } - return { - ok: true, - client: { - clientId, - clientName: - typeof doc.client_name === "string" ? doc.client_name : "MCP client", - redirectUris, - isPublic: true, - source: "cimd", - }, + const client: ResolvedClient = { + clientId, + clientName: + typeof doc.client_name === "string" ? doc.client_name : "MCP client", + redirectUris, + isPublic: true, + source: "cimd", }; + putCachedCimd(clientId, client); + return { ok: true, client }; } /**