From 19c2db2e60336c8df93a49e082e85d0c0219c526 Mon Sep 17 00:00:00 2001 From: Jonathan Haas Date: Tue, 4 Aug 2026 02:13:59 -0700 Subject: [PATCH] Survive recursive and hostile specs; add build_info/uptime metrics Recursive $refs (self-referencing schemas, common in real APIs) crashed the server two ways: the lint walker recursed the cyclic dereferenced document until stack overflow, and the compile cache JSON.stringify threw on cyclic operation models. The linter now tracks visited nodes, the schema normalizer cuts cycles to the permissive empty schema so compiled models are serializable and AJV/Zod-compilable, the cache write is best-effort, and the spec resource renders back-references as "[Circular]". Schema property names like __proto__ were silently dropped from Zod shapes (computed assignment hits the prototype setter); shapes and normalized schema nodes now use null-prototype objects so such properties are validated like any other. /metrics gains mcp_openapi_build_info{version} and mcp_openapi_uptime_seconds. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LqBsGC7xLvihtBxhCdWKz5 --- README.md | 2 +- src/compile-cache.ts | 11 ++- src/compiler.ts | 59 +++++++++------ src/lint.ts | 18 ++++- src/metrics.ts | 11 +++ src/server.ts | 28 ++++++- src/zod-schema.ts | 4 +- test/adversarial.test.ts | 156 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 257 insertions(+), 32 deletions(-) create mode 100644 test/adversarial.test.ts diff --git a/README.md b/README.md index 4a93656..48d3d65 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,7 @@ export default function transform({ operation, response }) { ## Observability -`/metrics` serves Prometheus text format: `mcp_openapi_tool_calls_total`, `_failed_total`, `_cancelled_total`, `_in_flight`, `_by_status_total{status}`, `mcp_openapi_retries_total`, `mcp_openapi_tool_call_latency_avg_ms`, and a latency histogram `mcp_openapi_tool_call_latency_ms_bucket`. Tool call start/completion and retry events are also emitted as MCP logging notifications. +`/metrics` serves Prometheus text format: `mcp_openapi_build_info{version}`, `mcp_openapi_uptime_seconds`, `mcp_openapi_tool_calls_total`, `_failed_total`, `_cancelled_total`, `_in_flight`, `_by_status_total{status}`, `mcp_openapi_retries_total`, `mcp_openapi_tool_call_latency_avg_ms`, and a latency histogram `mcp_openapi_tool_call_latency_ms_bucket`. Tool call start/completion and retry events are also emitted as MCP logging notifications. ## Library usage diff --git a/src/compile-cache.ts b/src/compile-cache.ts index 7ddbdd5..793e021 100644 --- a/src/compile-cache.ts +++ b/src/compile-cache.ts @@ -61,9 +61,14 @@ export async function compileDocumentWithCache( operations: [...operations.values()] }; - const abs = resolve(cachePath); - await mkdir(dirname(abs), { recursive: true }); - await writeFile(abs, JSON.stringify(entry), "utf8"); + // The cache is an optimization; a spec that cannot be cached must still serve. + try { + const abs = resolve(cachePath); + await mkdir(dirname(abs), { recursive: true }); + await writeFile(abs, JSON.stringify(entry), "utf8"); + } catch (error) { + process.stderr.write(`Compile cache write skipped: ${error instanceof Error ? error.message : String(error)}\n`); + } return operations; } diff --git a/src/compiler.ts b/src/compiler.ts index 91fa4af..eca0a84 100644 --- a/src/compiler.ts +++ b/src/compiler.ts @@ -343,7 +343,7 @@ function buildInputSchema(parameters: ParameterSpec[], bodySchema?: JsonSchema): continue; } - const groupProperties: Record = {}; + const groupProperties: Record = Object.create(null); const groupRequired: string[] = []; for (const param of items) { groupProperties[param.name] = param.schema ?? { type: "string" }; @@ -537,43 +537,58 @@ function pickContentWithSchema(content: Record): { contentType: } function normalizeJsonSchema(schema: JsonSchema): JsonSchema { - return normalizeNode(schema, new WeakMap()) as JsonSchema; + return normalizeNode(schema, new WeakMap(), new WeakSet()) as JsonSchema; } -function normalizeNode(node: T, seen: WeakMap): T { +function normalizeNode(node: T, seen: WeakMap, ancestors: WeakSet): T { if (!isObject(node)) { return node; } + // A node that contains itself (recursive $ref after dereferencing) cannot be + // JSON-serialized or compiled by AJV; cut the back-reference to the + // permissive empty schema. Non-cyclic reuse of shared subschemas is kept. + if (ancestors.has(node)) { + return {} as T; + } + if (seen.has(node)) { return seen.get(node) as T; } - if (Array.isArray(node)) { - const arr: unknown[] = []; - seen.set(node, arr); - for (const item of node) { - arr.push(normalizeNode(item, seen)); + ancestors.add(node); + try { + if (Array.isArray(node)) { + const arr: unknown[] = []; + for (const item of node) { + arr.push(normalizeNode(item, seen, ancestors)); + } + seen.set(node, arr); + return arr as T; } - return arr as T; - } - const source = node as Record; - const out: Record = {}; - seen.set(node, out); + const source = node as Record; + // Null prototype: schema keys are attacker-controlled and a "__proto__" + // key assigned to a plain object mutates its prototype instead of + // defining a property. + const out: Record = Object.create(null); - for (const [key, value] of Object.entries(source)) { - if (key === "nullable") { - continue; + for (const [key, value] of Object.entries(source)) { + if (key === "nullable") { + continue; + } + out[key] = normalizeNode(value, seen, ancestors); } - out[key] = normalizeNode(value, seen); - } - if (source.nullable === true) { - applyNullable(out); - } + if (source.nullable === true) { + applyNullable(out); + } - return out as T; + seen.set(node, out); + return out as T; + } finally { + ancestors.delete(node); + } } function applyNullable(schema: Record): void { diff --git a/src/lint.ts b/src/lint.ts index caf47be..8557c92 100644 --- a/src/lint.ts +++ b/src/lint.ts @@ -145,11 +145,16 @@ function collectBrokenInternalRefDiagnostics( value: unknown, root: Record, diagnostics: LintDiagnostic[], - location = "$" + location = "$", + visited: WeakSet = new WeakSet() ): void { if (Array.isArray(value)) { + if (visited.has(value)) { + return; + } + visited.add(value); for (let index = 0; index < value.length; index += 1) { - collectBrokenInternalRefDiagnostics(value[index], root, diagnostics, `${location}[${index}]`); + collectBrokenInternalRefDiagnostics(value[index], root, diagnostics, `${location}[${index}]`, visited); } return; } @@ -158,6 +163,13 @@ function collectBrokenInternalRefDiagnostics( return; } + // Dereferenced documents with recursive $refs are cyclic; each node is + // visited once. + if (visited.has(value)) { + return; + } + visited.add(value); + const record = value as Record; const ref = typeof record["$ref"] === "string" ? record["$ref"] : undefined; if (ref?.startsWith("#/") && resolveJsonPointer(root, ref) === undefined) { @@ -170,7 +182,7 @@ function collectBrokenInternalRefDiagnostics( } for (const [key, child] of Object.entries(record)) { - collectBrokenInternalRefDiagnostics(child, root, diagnostics, `${location}.${key}`); + collectBrokenInternalRefDiagnostics(child, root, diagnostics, `${location}.${key}`, visited); } } diff --git a/src/metrics.ts b/src/metrics.ts index dfce021..52df2b8 100644 --- a/src/metrics.ts +++ b/src/metrics.ts @@ -43,9 +43,20 @@ export function observeLatency(ms: number): void { metrics.latencyBuckets.le_inf += 1; } +let buildVersion = "0.0.0"; +const startedAtMs = Date.now(); + +export function setBuildInfo(version: string): void { + buildVersion = version; +} + export function renderPrometheus(): string { const avgLatency = metrics.toolCallsTotal > 0 ? metrics.toolCallLatencyMsTotal / metrics.toolCallsTotal : 0; return [ + "# TYPE mcp_openapi_build_info gauge", + `mcp_openapi_build_info{version="${buildVersion}"} 1`, + "# TYPE mcp_openapi_uptime_seconds gauge", + `mcp_openapi_uptime_seconds ${Math.floor((Date.now() - startedAtMs) / 1000)}`, "# TYPE mcp_openapi_tool_calls_total counter", `mcp_openapi_tool_calls_total ${metrics.toolCallsTotal}`, "# TYPE mcp_openapi_tool_calls_failed_total counter", diff --git a/src/server.ts b/src/server.ts index fe96d7a..6469e0b 100644 --- a/src/server.ts +++ b/src/server.ts @@ -34,7 +34,7 @@ import { import { executeOperation } from "./http.js"; import { STREAMABLE_TEST_HTML, SSE_TEST_HTML } from "./html.js"; import { paginateWithCursor } from "./pagination.js"; -import { observeLatency, observeStatus, renderPrometheus, metrics } from "./metrics.js"; +import { observeLatency, observeStatus, renderPrometheus, setBuildInfo, metrics } from "./metrics.js"; import type { CompileOptions, OperationModel, RuntimeOptions } from "./types.js"; import { zodFromJsonSchema } from "./zod-schema.js"; import { compileDocumentWithCache } from "./compile-cache.js"; @@ -128,6 +128,7 @@ let inFlightCalls = 0; let responseTransform: ((ctx: { operation: OperationModel; response: { body: unknown; status: number } }) => unknown | Promise) | undefined; async function main(): Promise { + setBuildInfo(PKG_VERSION); const cli = parseArgs(process.argv.slice(2)); if (cli.command === "init") { @@ -214,7 +215,7 @@ function createMcpServer(state: RuntimeState, cli: CliOptions): Server { for (const spec of state.specDocs) { if (uri === `openapi://${spec.name}/spec`) { return { - contents: [{ uri, mimeType: "application/json", text: JSON.stringify(spec.doc, null, 2) }] + contents: [{ uri, mimeType: "application/json", text: safeJsonStringify(spec.doc, 2) }] }; } if (uri === `openapi://${spec.name}/tools`) { @@ -1510,6 +1511,29 @@ function isObject(value: unknown): value is object { return value !== null && typeof value === "object"; } +// Dereferenced documents with recursive $refs contain object cycles; +// back-references are rendered as the string "[Circular]". +function safeJsonStringify(value: unknown, indent?: number): string { + const ancestors: object[] = []; + return JSON.stringify( + value, + function (this: unknown, _key: string, val: unknown) { + if (typeof val !== "object" || val === null) { + return val; + } + while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) { + ancestors.pop(); + } + if (ancestors.includes(val)) { + return "[Circular]"; + } + ancestors.push(val); + return val; + }, + indent + ); +} + main().catch((error) => { process.stderr.write(`Fatal: ${error instanceof Error ? error.message : String(error)}\n`); process.exit(1); diff --git a/src/zod-schema.ts b/src/zod-schema.ts index cc3d166..d75d8df 100644 --- a/src/zod-schema.ts +++ b/src/zod-schema.ts @@ -102,7 +102,9 @@ function convertSchema(schema: unknown, seen: WeakMap): ZodT const props = isObject(s.properties) ? (s.properties as Record) : {}; const required = Array.isArray(s.required) ? new Set(s.required.filter((x): x is string => typeof x === "string")) : new Set(); - const shape: Record = {}; + // Null prototype: a "__proto__" property name assigned to a plain object + // would replace the shape's prototype and silently skip validation. + const shape: Record = Object.create(null); for (const [key, value] of Object.entries(props)) { const inner = convertSchema(value, seen); shape[key] = required.has(key) ? inner : inner.optional(); diff --git a/test/adversarial.test.ts b/test/adversarial.test.ts new file mode 100644 index 0000000..3a02daa --- /dev/null +++ b/test/adversarial.test.ts @@ -0,0 +1,156 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import { ReadResourceResultSchema } from "@modelcontextprotocol/sdk/types.js"; +import { compileOperations } from "../src/compiler.js"; +import { loadOpenApiDocument } from "../src/openapi.js"; +import { zodFromJsonSchema } from "../src/zod-schema.js"; +import { renderPrometheus, setBuildInfo } from "../src/metrics.js"; + +const tsxCli = resolve("node_modules/tsx/dist/cli.mjs"); + +function cyclicSpec(): Record { + return { + openapi: "3.0.3", + info: { title: "Cyclic API", version: "1.0.0" }, + servers: [{ url: "https://cyclic.invalid" }], + paths: { + "/nodes": { + get: { + operationId: "listNodes", + responses: { + "200": { + description: "ok", + content: { + "application/json": { schema: { $ref: "#/components/schemas/Node" } } + } + } + } + } + } + }, + components: { + schemas: { + Node: { + type: "object", + properties: { + id: { type: "string" }, + child: { $ref: "#/components/schemas/Node" } + } + } + } + } + }; +} + +test("recursive $ref specs compile to serializable, validatable operations", async () => { + const dir = await mkdtemp(join(tmpdir(), "mcp-openapi-cyclic-")); + const specPath = join(dir, "cyclic.json"); + await writeFile(specPath, JSON.stringify(cyclicSpec()), "utf8"); + + const doc = await loadOpenApiDocument(specPath); + const operations = compileOperations(doc); + const op = operations.get("listNodes"); + assert.ok(op); + + // The compiled model must be cycle-free: cacheable and schema-compilable. + const serialized = JSON.stringify(op); + assert.ok(serialized.length > 0); + assert.ok(op.outputSchema); + const validator = zodFromJsonSchema(op.outputSchema); + const parsed = validator.safeParse({ id: "a", child: { id: "b", child: {} } }); + assert.equal(parsed.success, true); +}); + +test("server startup succeeds on a recursive spec (--validate-spec)", async () => { + const dir = await mkdtemp(join(tmpdir(), "mcp-openapi-cyclic-cli-")); + const specPath = join(dir, "cyclic.json"); + await writeFile(specPath, JSON.stringify(cyclicSpec()), "utf8"); + + const result = spawnSync( + process.execPath, + [tsxCli, "src/server.ts", "--spec", specPath, "--cache-path", join(dir, "cache.json"), "--validate-spec"], + { cwd: process.cwd(), encoding: "utf8" } + ); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Compiled 1 tools/); +}); + +test("spec resource for a recursive spec renders with [Circular] markers", async () => { + const dir = await mkdtemp(join(tmpdir(), "mcp-openapi-cyclic-res-")); + const specPath = join(dir, "cyclic.json"); + await writeFile(specPath, JSON.stringify(cyclicSpec()), "utf8"); + + const client = new Client({ name: "adversarial-test", version: "0.1.0" }, { capabilities: {} }); + const transport = new StdioClientTransport({ + command: process.execPath, + args: ["dist/server.js", "--spec", specPath, "--cache-path", join(dir, "cache.json")], + cwd: process.cwd(), + stderr: "pipe" + }); + + try { + await client.connect(transport); + const result = await client.request( + { method: "resources/read", params: { uri: "openapi://cyclic/spec" } }, + ReadResourceResultSchema + ); + const text = String(result.contents[0]?.text); + const doc = JSON.parse(text) as Record; + assert.equal(typeof doc.openapi, "string"); + assert.ok(text.includes("[Circular]")); + } finally { + await transport.close(); + } +}); + +test("__proto__ schema properties are validated, not silently dropped", () => { + const schema = { + type: "object", + additionalProperties: false, + properties: { + ["__proto__"]: { type: "string" }, + ok: { type: "boolean" } + } + }; + + const validator = zodFromJsonSchema(schema); + + const good = validator.safeParse(JSON.parse('{"__proto__": "s", "ok": true}')); + assert.equal(good.success, true); + + const bad = validator.safeParse(JSON.parse('{"__proto__": 5, "ok": true}')); + assert.equal(bad.success, false, "a __proto__ property violating its schema must fail validation"); + + assert.equal(({} as Record).polluted, undefined); + assert.equal(Object.prototype.hasOwnProperty.call(Object.prototype, "polluted"), false); +}); + +test("duplicate operationIds within a spec get numeric suffixes", () => { + const doc = { + openapi: "3.0.3", + info: { title: "Dup", version: "1.0.0" }, + servers: [{ url: "https://dup.invalid" }], + paths: { + "/a": { get: { operationId: "getThing", responses: { "200": { description: "ok" } } } }, + "/b": { get: { operationId: "getThing", responses: { "200": { description: "ok" } } } } + } + }; + + const operations = compileOperations(doc as Record); + assert.ok(operations.has("getThing")); + assert.ok(operations.has("getThing_2")); +}); + +test("metrics expose build_info and uptime", () => { + setBuildInfo("9.9.9-test"); + const rendered = renderPrometheus(); + assert.match(rendered, /mcp_openapi_build_info\{version="9\.9\.9-test"\} 1/); + assert.match(rendered, /mcp_openapi_uptime_seconds \d+/); +});