mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 20:42:43 +00:00
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.
This commit is contained in:
1 parent
324df70071
commit
3c6cb8b08d
3 files changed
+228
-24
No files matched your search
@@ -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<string, string | undefined>,
|
||||
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<Response> {
|
||||
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<Response> {
|
||||
|
||||
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;
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
+111
-15
@@ -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<ClientResolution> {
|
||||
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<ClientResolution> {
|
||||
{ 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<ClientResolution> {
|
||||
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 };
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in new issue
Block a user