From 2381dd9ca6ff39678255345d721ada6ede0595f8 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 16:38:55 +0000 Subject: [PATCH 1/3] Nothing capped what a caller could send in `installToolWrapper` caps what a tool call returns, and argues for doing it in one place: "a per-tool guard is a guard somebody forgets to add to the fifty-seventh". Nothing capped the other direction. `reactome_analyze_identifiers` took `z.array(nonEmptyString)` with no maximum and posted `identifiers.join("\n")` to the Analysis Service, which does real work and stores a result against a token. A few bytes of MCP request could commission an arbitrarily large job, with no ceiling anywhere in the path. On a private instance that is academic. Fronted by a public nginx -- which is where this server is going -- it is an amplification. Every list argument now has an explicit ceiling: 10,000 identifiers (`MCP_MAX_ANALYSIS_IDENTIFIERS`, above a whole human proteome), 1,000 pathways for the filter POST, 100 export selections, 50 for each search filter. **The ceiling cannot live in the wrapper** the way the output cap does -- only the argument knows what a sane length is. So the *requirement* lives in a test: `tests/input-bounds.test.ts` walks every registered tool and fails on any argument that accepts an absurd array. A fifty-seventh tool with an unbounded list fails it without anybody remembering this commit. It found one immediately. I had capped five arrays by grepping `z.array`, which missed a sixth written as `z\n .array(...)` across two lines -- `reactome_search.types`. Grep matches how code is spelled; the sweep asks what the schema accepts. It probes by parsing rather than reading zod internals, so it survives a zod upgrade and cannot pass by misreading a private field. It does not reach an array nested inside an object argument; no tool has one, and the limit is stated in the file rather than implied. Adversarial review, before the PR: the first version put `.min(1)` on every list, including the optional search and export filters. Those were `z.array(...).optional()`, so a client sending `[]` to mean "no filter" worked, and I would have turned that into a validation error while bounding nothing -- a new way to fail with no harm prevented. Required bodies keep the floor (an empty identifier list asked the service to enrich nothing); optional filters keep accepting `[]`, pinned by its own test. Verified by sabotage, each against the specific test written for it: unbounding the identifier list fails three, reintroducing the floor on optional filters fails exactly the empty-list test. One earlier sabotage attempt only broke compilation and reported "no tests" -- that proves nothing, so it was redone as valid code. 122 tests, lint/format/typecheck/build clean under node:22. Co-Authored-By: Claude Opus 5 --- src/config.ts | 15 +++++ src/tools/analysis.ts | 7 +- src/tools/export.ts | 3 +- src/tools/limits.ts | 48 ++++++++++++++ src/tools/search.ts | 12 ++-- tests/input-bounds.test.ts | 130 +++++++++++++++++++++++++++++++++++++ 6 files changed, 203 insertions(+), 12 deletions(-) create mode 100644 src/tools/limits.ts create mode 100644 tests/input-bounds.test.ts diff --git a/src/config.ts b/src/config.ts index db14790..dbbf3cf 100644 --- a/src/config.ts +++ b/src/config.ts @@ -140,3 +140,18 @@ export const MCP_SESSION_TTL_MS = parsePositiveInt(process.env.MCP_SESSION_TTL_M /** Ceiling on concurrent sessions, so a client loop cannot exhaust memory. */ export const MCP_MAX_SESSIONS = parsePositiveInt(process.env.MCP_MAX_SESSIONS, 256); + +/** + * Most identifiers one `reactome_analyze_identifiers` call may submit. + * + * The list is POSTed to the Analysis Service, which does real work and stores + * a result against a token, so an uncapped list is an amplification: a few + * bytes of MCP request commissioning an unbounded job. 10,000 is well above a + * genuine enrichment (a whole human proteome is ~20,000) and far below what + * makes a useful weapon; raise it on a private instance if a real analysis + * needs more. + */ +export const MAX_ANALYSIS_IDENTIFIERS = parsePositiveInt( + process.env.MCP_MAX_ANALYSIS_IDENTIFIERS, + 10_000 +); diff --git a/src/tools/analysis.ts b/src/tools/analysis.ts index 8f4ffff..353028c 100644 --- a/src/tools/analysis.ts +++ b/src/tools/analysis.ts @@ -1,4 +1,5 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { boundedList, identifierList } from "./limits.js"; import { z } from "zod"; import { nonEmptyString } from "../schemas.js"; import { analysisClient } from "../clients/analysis.js"; @@ -95,9 +96,7 @@ export function registerAnalysisTools(server: McpServer) { "reactome_analyze_identifiers", "Perform pathway enrichment analysis on a list of gene/protein identifiers. Returns over-represented pathways sorted by p-value.", { - identifiers: z - .array(nonEmptyString) - .describe("List of gene symbols, UniProt IDs, or other identifiers"), + identifiers: identifierList("List of gene symbols, UniProt IDs, or other identifiers"), projection: z.boolean().optional().default(true).describe("Project results to Homo sapiens"), interactors: z .boolean() @@ -332,7 +331,7 @@ export function registerAnalysisTools(server: McpServer) { "Filter an analysis result to only include specific pathways.", { token: nonEmptyString.describe("Analysis token"), - pathways: z.array(nonEmptyString).describe("List of pathway stable IDs to include"), + pathways: boundedList(1_000, "List of pathway stable IDs to include"), resource: nonEmptyString.optional().default("TOTAL").describe("Resource filter"), p_value: z.number().optional().describe("p-value threshold"), }, diff --git a/src/tools/export.ts b/src/tools/export.ts index 8968fd3..3e95279 100644 --- a/src/tools/export.ts +++ b/src/tools/export.ts @@ -1,4 +1,5 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { optionalList } from "./limits.js"; import { z } from "zod"; import { nonEmptyString } from "../schemas.js"; import { contentClient } from "../clients/content.js"; @@ -22,7 +23,7 @@ export function registerExportTools(server: McpServer) { .default(5) .describe("Quality/scale factor (1-10, higher = larger image)"), flag: nonEmptyString.optional().describe("Identifier to highlight/flag in the diagram"), - sel: z.array(nonEmptyString).optional().describe("IDs to select/highlight"), + sel: optionalList(100, "IDs to select/highlight"), }, async ({ id, format, quality, flag, sel }) => { const params = new URLSearchParams(); diff --git a/src/tools/limits.ts b/src/tools/limits.ts new file mode 100644 index 0000000..323cc66 --- /dev/null +++ b/src/tools/limits.ts @@ -0,0 +1,48 @@ +import { z } from "zod"; +import { MAX_ANALYSIS_IDENTIFIERS } from "../config.js"; + +/** + * Bounds on how much a caller may send *in*. + * + * `installToolWrapper` already caps what a call returns, and says why it does + * so in one place: "a per-tool guard is a guard somebody forgets to add to the + * fifty-seventh". Nothing capped the other direction. On a private instance + * that was academic. Fronted by a public nginx it is not: + * `reactome_analyze_identifiers` took `z.array(nonEmptyString)` with no + * maximum and posted `identifiers.join("\n")` to the Analysis Service, so one + * small MCP call could commission an arbitrarily large analysis job and a + * stored result — an amplification with no ceiling anywhere in the path. + * + * An input cap cannot be applied centrally the way the output cap is, because + * only the caller knows what a sane length is for a given argument. What can + * be central is the *requirement*: `tests/input-bounds.test.ts` walks every + * registered tool schema and fails on any array without a maximum, so the + * fifty-seventh tool cannot quietly reintroduce this. + */ + +/** + * A required list argument: non-empty, with an explicit ceiling. + * + * `.min(1)` belongs only on the lists that become a request body. An empty + * identifier list POSTed an empty body and asked the Analysis Service to + * enrich nothing. + */ +export function boundedList(max: number, describe: string) { + return z.array(z.string().min(1)).min(1).max(max).describe(`${describe} (at most ${max})`); +} + +/** + * An optional filter list: the same ceiling, but `[]` stays legal. + * + * These were `z.array(...).optional()` before, so a client sending `[]` to + * mean "no filter" worked. Adding `.min(1)` here would have bounded nothing + * and turned that into a validation error -- a new way to fail with no harm + * prevented. The ceiling is the point; the floor was not. + */ +export function optionalList(max: number, describe: string) { + return z.array(z.string().min(1)).max(max).describe(`${describe} (at most ${max})`).optional(); +} + +/** Identifier lists posted to the Analysis Service. Configurable: a real + * enrichment can be large, and the right ceiling depends on the deployment. */ +export const identifierList = (describe: string) => boundedList(MAX_ANALYSIS_IDENTIFIERS, describe); diff --git a/src/tools/search.ts b/src/tools/search.ts index 9bc7280..001c93c 100644 --- a/src/tools/search.ts +++ b/src/tools/search.ts @@ -1,4 +1,5 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { optionalList } from "./limits.js"; import { z } from "zod"; import { contentClient } from "../clients/content.js"; import { nonEmptyString } from "../schemas.js"; @@ -97,12 +98,9 @@ export function registerSearchTools(server: McpServer) { species: nonEmptyString .optional() .describe("Filter by species (e.g., 'Homo sapiens', 'Mus musculus')"), - types: z - .array(nonEmptyString) - .optional() - .describe("Filter by type (Pathway, Reaction, Protein, Gene, Complex, etc.)"), - compartments: z.array(nonEmptyString).optional().describe("Filter by cellular compartment"), - keywords: z.array(nonEmptyString).optional().describe("Filter by keywords"), + types: optionalList(50, "Filter by type (Pathway, Reaction, Protein, Gene, Complex, etc.)"), + compartments: optionalList(50, "Filter by cellular compartment"), + keywords: optionalList(50, "Filter by keywords"), rows: z.number().optional().default(25).describe("Number of results to return"), cluster: z.boolean().optional().default(true).describe("Cluster related results"), }, @@ -153,7 +151,7 @@ export function registerSearchTools(server: McpServer) { page: z.number().optional().default(1).describe("Page number (1-based)"), rows_per_page: z.number().optional().default(20).describe("Results per page"), species: nonEmptyString.optional().describe("Filter by species"), - types: z.array(nonEmptyString).optional().describe("Filter by type"), + types: optionalList(50, "Filter by type"), }, async ({ query, page, rows_per_page, species, types }) => { const params: Record = { diff --git a/tests/input-bounds.test.ts b/tests/input-bounds.test.ts new file mode 100644 index 0000000..6b53332 --- /dev/null +++ b/tests/input-bounds.test.ts @@ -0,0 +1,130 @@ +import { describe, it, expect } from "vitest"; +import { registerAllTools } from "../src/tools/index.js"; +import { MAX_ANALYSIS_IDENTIFIERS } from "../src/config.js"; + +/** + * How much a caller may send *in*. + * + * `installToolWrapper` caps what a call returns, and argues for doing it in + * one place because "a per-tool guard is a guard somebody forgets to add to + * the fifty-seventh". The same was never true of inputs: + * `reactome_analyze_identifiers` accepted an unbounded array and posted it to + * the Analysis Service, so a few bytes of MCP request could commission an + * arbitrarily large job. Private, that is academic; behind a public nginx it + * is an amplification with no ceiling in the path. + * + * An input ceiling cannot live in the wrapper — only the individual argument + * knows what a sane length is. So the *requirement* lives here instead: this + * walks every registered tool and fails on any argument that will accept an + * absurd array. A new tool with an unbounded list fails this without anyone + * remembering it exists. + * + * It probes by parsing rather than by reading zod internals, so it keeps + * working across zod versions and cannot pass by misreading a private field. + * + * What it does not reach: an array nested inside an object argument. No tool + * has one today, and a sweep that silently covered less than it claims would + * be worse than this one, so the limit is stated rather than implied. + */ + +type Shape = Record { success: boolean } }>; + +function registeredSchemas(): Map { + const schemas = new Map(); + const server = { + tool: (...args: unknown[]) => { + const name = args[0]; + const shape = args[2]; + if (typeof name === "string" && shape && typeof shape === "object") { + schemas.set(name, shape as Shape); + } + return undefined; + }, + }; + registerAllTools(server as never); + return schemas; +} + +const ABSURD = Array.from({ length: 100_001 }, () => "R-HSA-109582"); +const ONE = ["R-HSA-109582"]; + +describe("tool input bounds", () => { + const schemas = registeredSchemas(); + + it("registers tools to check", () => { + // Without this the sweep below would pass vacuously against an empty map. + expect(schemas.size).toBeGreaterThan(50); + }); + + it("has no argument anywhere that accepts an unbounded list", () => { + const unbounded: string[] = []; + let arrayArgs = 0; + for (const [tool, shape] of schemas) { + for (const [arg, schema] of Object.entries(shape)) { + if (typeof schema?.safeParse !== "function") continue; + // Identify list arguments by behaviour: it takes a one-element array. + if (!schema.safeParse(ONE).success) continue; + arrayArgs++; + if (schema.safeParse(ABSURD).success) unbounded.push(`${tool}.${arg}`); + } + } + // The sweep is only meaningful if it found the list arguments at all. + expect(arrayArgs).toBeGreaterThan(0); + expect(unbounded).toEqual([]); + }); +}); + +describe("analysis identifier list", () => { + const identifiers = () => { + const shape = registeredSchemas().get("reactome_analyze_identifiers"); + if (!shape) throw new Error("reactome_analyze_identifiers is not registered"); + const arg = shape.identifiers; + if (!arg) throw new Error("reactome_analyze_identifiers has no identifiers argument"); + return arg; + }; + + it("accepts a list at the cap", () => { + const atCap = Array.from({ length: MAX_ANALYSIS_IDENTIFIERS }, () => "TP53"); + expect(identifiers().safeParse(atCap).success).toBe(true); + }); + + it("rejects one identifier past the cap", () => { + const overCap = Array.from({ length: MAX_ANALYSIS_IDENTIFIERS + 1 }, () => "TP53"); + expect(identifiers().safeParse(overCap).success).toBe(false); + }); + + it("rejects an empty list", () => { + // Previously posted an empty body to the Analysis Service. + expect(identifiers().safeParse([]).success).toBe(false); + }); + + it("still rejects a blank identifier", () => { + // The bound must not have replaced the emptiness check on each element. + expect(identifiers().safeParse(["TP53", ""]).success).toBe(false); + }); +}); + +describe("optional filter lists", () => { + // Capping these should not have changed what a working client may send. + // `.min(1)` here would have bounded nothing and only added a way to fail. + const filter = (tool: string, arg: string) => { + const shape = registeredSchemas().get(tool); + if (!shape) throw new Error(`${tool} is not registered`); + const schema = shape[arg]; + if (!schema) throw new Error(`${tool} has no ${arg} argument`); + return schema; + }; + + it("still accepts an empty list, as before the cap", () => { + expect(filter("reactome_search", "types").safeParse([]).success).toBe(true); + expect(filter("reactome_export_diagram", "sel").safeParse([]).success).toBe(true); + }); + + it("still accepts a normal list", () => { + expect(filter("reactome_search", "types").safeParse(["Pathway"]).success).toBe(true); + }); + + it("rejects an absurd one", () => { + expect(filter("reactome_search", "types").safeParse(ABSURD).success).toBe(false); + }); +}); From 08b8a8a02f1e1226e7132480766a7d4244113e48 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 16:50:15 +0000 Subject: [PATCH 2/3] The cap I just added was unreachable over HTTP The website session measured the deployed image and found the transport refuses a body over 100 KiB, so the 10,000-identifier cap in the previous commit could never be hit: 10,000 identifiers serialise to ~180 KB and a caller at exactly the documented cap got a bare 413 from express before any validation ran. Confirmed here independently by bisecting a real server -- 5,000 identifiers (90,125 bytes) are parsed, 6,000 (108,125) are refused, and the boundary is 102,400, express's `100kb` to the byte. **`express.json({ limit: "4mb" })` in `startHttpServer` never ran.** `createMcpExpressApp` mounts `express.json()` at its own line 29, and body-parser skips a body that has already been read, so the parser behind it sees nothing. The line was worse than absent: it was the number anyone reading that file would have believed, and it is four times larger than what applies. Removed, with the real ceiling and how it was measured written where it used to be. Two parsers where one was intended, and the redundant one is the invisible one -- it lives in a dependency, it is doing its job correctly, and the code that looks authoritative is the code that does nothing. `MAX_ANALYSIS_IDENTIFIERS` is now 3,000, chosen to fit rather than for its own sake: at 20 characters an identifier that is ~69 KB, a third of the ceiling spare. A caller at the cap now gets a validation error naming the limit instead of a 413 naming nothing. 4,000 was tried first and left 10% headroom. The headroom assertion rejected it, which is the assertion earning its place -- a cap that only just fits is one identifier-length change away from being unreachable again. `tests/body-limit.test.ts` holds the two ceilings together by asserting the *relationship*, not either number: it fails if the cap rises past what the transport carries, or if the transport tightens beneath the cap. The byte ceiling is express's default reached through the SDK, so nothing in this repo would otherwise mention it if an upgrade moved it. Verified by restoring the 10,000 cap: the at-the-cap test fails with 413 and the headroom test names 230,125 bytes. The over-the-cap control stays green, as it must -- which is why it is not the only test. 128 tests, lint/format/typecheck/build clean under node:22. Co-Authored-By: Claude Opus 5 --- src/config.ts | 20 ++++++-- src/http.ts | 18 ++++++- tests/body-limit.test.ts | 100 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 tests/body-limit.test.ts diff --git a/src/config.ts b/src/config.ts index dbbf3cf..f6cd14f 100644 --- a/src/config.ts +++ b/src/config.ts @@ -146,12 +146,22 @@ export const MCP_MAX_SESSIONS = parsePositiveInt(process.env.MCP_MAX_SESSIONS, 2 * * The list is POSTed to the Analysis Service, which does real work and stores * a result against a token, so an uncapped list is an amplification: a few - * bytes of MCP request commissioning an unbounded job. 10,000 is well above a - * genuine enrichment (a whole human proteome is ~20,000) and far below what - * makes a useful weapon; raise it on a private instance if a real analysis - * needs more. + * bytes of MCP request commissioning an unbounded job. + * + * 3,000 is chosen to fit, not for its own sake. The HTTP transport's body + * ceiling is express's 100 KiB default (see `startHttpServer`), and a + * `tools/call` carrying 3,000 identifiers of 20 characters comes to about + * 69 KB -- a third of the ceiling left spare, so a request at this cap gets + * a validation error naming the limit rather than a bare 413 from the + * transport. A cap the transport refuses to deliver is not a cap, it is two + * disagreeing ones. 4,000 was tried first and left only 10% headroom, which + * the test rejected: a cap that only just fits is one identifier-length + * change away from being unreachable again. + * + * `tests/body-limit.test.ts` asserts the two still agree. stdio has no such + * ceiling, so a private instance that needs a whole proteome can raise this. */ export const MAX_ANALYSIS_IDENTIFIERS = parsePositiveInt( process.env.MCP_MAX_ANALYSIS_IDENTIFIERS, - 10_000 + 3_000 ); diff --git a/src/http.ts b/src/http.ts index cd9a0dc..4145230 100644 --- a/src/http.ts +++ b/src/http.ts @@ -1,7 +1,6 @@ import { randomUUID } from "node:crypto"; import type { Server } from "node:http"; import type { Request, Response } from "express"; -import express from "express"; import { createMcpExpressApp } from "@modelcontextprotocol/sdk/server/express.js"; import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; @@ -41,7 +40,22 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro // hosts, which is what stops a web page in the user's browser from driving // this server. const app = createMcpExpressApp({ host }); - app.use(express.json({ limit: "4mb" })); + + // No second body parser here. `createMcpExpressApp` mounts `express.json()` + // with no limit of its own, so express's 100 KiB default is the real + // ceiling, and it is reached first: a parser added afterwards never sees a + // request, because body-parser skips a body that has already been read. + // + // An `express.json({ limit: "4mb" })` sat on this line and did nothing. It + // was worse than absent -- it was the number anyone reading this file would + // have believed, and it is four times larger than what actually applies. + // Measured, not read: 102,392 bytes are accepted and 102,992 are refused + // with 413, which is express's `100kb` exactly. + // + // 100 KiB is not a number anyone here chose, but it is a defensible one for + // a public instance, and MAX_ANALYSIS_IDENTIFIERS is set to fit inside it. + // `tests/body-limit.test.ts` holds the two together, so an SDK upgrade that + // moves this ceiling fails there rather than in production. const sessions = new Map(); diff --git a/tests/body-limit.test.ts b/tests/body-limit.test.ts new file mode 100644 index 0000000..b466456 --- /dev/null +++ b/tests/body-limit.test.ts @@ -0,0 +1,100 @@ +/** + * The two ceilings on one request, and whether they agree. + * + * `MAX_ANALYSIS_IDENTIFIERS` bounds how many identifiers a caller may submit. + * The HTTP transport separately bounds how many *bytes* it will read. Neither + * knows about the other, and for a while they disagreed: the cap was 10,000 + * identifiers, which serialise to about 180 KB, while the transport refuses + * anything over 100 KiB. A caller at exactly the documented cap got a bare + * 413 from express before any validation ran — so the error named the wrong + * thing, and the cap was unreachable by the transport this server is + * deployed behind. + * + * The byte ceiling is not ours: `createMcpExpressApp` mounts `express.json()` + * with no limit, so express's 100 KiB default applies, and an SDK upgrade + * could move it without anything here mentioning it. That is what this file + * is for. It asserts the relationship rather than either number, so it fails + * if the cap rises past what the transport will carry OR if the transport + * tightens beneath the cap. + * + * It speaks real HTTP to a real server for the same reason the transport + * tests do: the limit lives in middleware, and a mock of the middleware would + * be testing the mock. + */ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { request as httpRequest, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; +import { startHttpServer } from "../src/http.js"; +import { MAX_ANALYSIS_IDENTIFIERS } from "../src/config.js"; + +/** Longer than a gene symbol or a UniProt accession; an Ensembl gene ID is 15. */ +const PESSIMISTIC_IDENTIFIER = "X".repeat(20); + +function post(base: string, body: string): Promise { + return new Promise((resolve, reject) => { + const req = httpRequest( + `${base}/mcp`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + Accept: "application/json, text/event-stream", + "Content-Length": Buffer.byteLength(body), + }, + }, + res => { + res.resume(); + res.on("end", () => resolve(res.statusCode ?? 0)); + } + ); + req.on("error", reject); + req.end(body); + }); +} + +const analyzeCall = (count: number, identifier = PESSIMISTIC_IDENTIFIER) => + JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { + name: "reactome_analyze_identifiers", + arguments: { identifiers: Array.from({ length: count }, () => identifier) }, + }, + }); + +describe("request body ceiling", () => { + let server: Server; + let base: string; + + beforeAll(async () => { + server = await startHttpServer(0, "127.0.0.1"); + base = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; + }); + + afterAll(() => server.close()); + + it("carries a request at the identifier cap", async () => { + // 400 here is the session check refusing an uninitialised call — which + // means the body was read and parsed. The assertion is only that it was + // not refused for its size. + const status = await post(base, analyzeCall(MAX_ANALYSIS_IDENTIFIERS)); + expect(status).not.toBe(413); + expect(status).toBe(400); + }); + + it("refuses a body far above it", async () => { + // Without this the test above would pass against a server with no limit + // at all, and would stop being about anything. + const status = await post(base, analyzeCall(MAX_ANALYSIS_IDENTIFIERS * 10)); + expect(status).toBe(413); + }); + + it("leaves headroom rather than sitting on the boundary", async () => { + // A cap that only just fits is one identifier-length change away from + // being unreachable again. 20-character identifiers at the cap should use + // well under the ceiling. + const bytes = Buffer.byteLength(analyzeCall(MAX_ANALYSIS_IDENTIFIERS)); + expect(bytes).toBeLessThan(90_000); + }); +}); From 4761e21cdaaa47d83bcf76ed8fc2e7511971442c Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 18:24:50 +0000 Subject: [PATCH 3/3] Adversarial review: the sweep missed three tools and the threat was overstated Three findings against the two commits above, one of them about why they exist at all. **1. The justification was wrong on the transport that matters.** I said an uncapped identifier list was "an amplification with no ceiling anywhere in the path". Measured against main as it stood, before any of this: 12,000 identifiers (84,125 bytes) are accepted, 15,000 (105,125) are refused with 413. HTTP was already bounded near 14,600 short identifiers by express's 100 KiB default. So the public transport -- the one the exposure argument was about -- had a ceiling the whole time, and the ceiling I later called "not a number anyone chose" was the only thing actually bounding the thing I was alarmed about. stdio, which is not the public transport, is where nothing bounded it. The cap still earns its place, more modestly: over HTTP it makes the bound *predictable* -- 3,000 regardless of identifier length, rather than somewhere between 4,000 and 14,600 depending on how long they happen to be -- and turns an opaque 413 into a validation error naming the limit. Over stdio it is the only bound there is. Both comments now say that instead of the stronger thing. **2. The sweep walked 59 of 62 tools.** It called `registerAllTools` once, with no environment, so the three Cypher tools -- which need `NEO4J_URI` and `MCP_ALLOW_CYPHER` -- were never swept. A check whose entire value is completeness, quietly covering a subset. It is also the same shape as the bug that started this work: the thing that varies by configuration, checked in one configuration. It now merges both configurations and asserts it sees 62 tools and three Cypher names, so the coverage claim fails rather than shrinks. Verified by adding an unbounded array to `reactome_cypher_schema`: the widened sweep catches it, and the previous version could not have seen it at all. **3. The cap/transport agreement was only guarded at the default.** `MCP_MAX_ANALYSIS_IDENTIFIERS` can raise it at runtime, putting the two ceilings back into disagreement on a deployment no test ever runs against, with a bare 413 as the only symptom. `startHttpServer` now checks the configured value and warns, naming both numbers and the effect. It uses an estimate rather than a serialised request, so an estimate that under-stated the body would stay silent on exactly the misconfiguration it exists to report. Three tests hold it: it never under-states a real request at the cap, it fires on 10,000 (the cap that was actually wrong), and it stays quiet on the one in use. Confirmed against the built image -- `MCP_MAX_ANALYSIS_IDENTIFIERS=10000` warns with worstCaseBodyBytes 250256, the default logs nothing. 131 tests, lint/format/typecheck/build clean under node:22. Co-Authored-By: Claude Opus 5 --- src/config.ts | 16 ++++++++-- src/http.ts | 28 +++++++++++++++++ src/tools/limits.ts | 36 +++++++++++++++++----- tests/body-limit.test.ts | 25 ++++++++++++++++ tests/input-bounds.test.ts | 61 +++++++++++++++++++++++++++++++++----- 5 files changed, 149 insertions(+), 17 deletions(-) diff --git a/src/config.ts b/src/config.ts index f6cd14f..3a283a6 100644 --- a/src/config.ts +++ b/src/config.ts @@ -145,8 +145,20 @@ export const MCP_MAX_SESSIONS = parsePositiveInt(process.env.MCP_MAX_SESSIONS, 2 * Most identifiers one `reactome_analyze_identifiers` call may submit. * * The list is POSTed to the Analysis Service, which does real work and stores - * a result against a token, so an uncapped list is an amplification: a few - * bytes of MCP request commissioning an unbounded job. + * a result against a token. + * + * The first version of this cap was justified by calling that an unbounded + * amplification. **That was overstated for the transport it matters on.** + * Measured against the code as it stood, HTTP already refused a body over + * 100 KiB, so the list was in practice bounded near 14,600 short identifiers + * -- by express's default, not by anything anyone here decided. What is + * genuinely unbounded is stdio, which has no such ceiling. + * + * So this cap earns its place more modestly than first claimed: over HTTP it + * makes the bound *predictable* (3,000 regardless of identifier length, + * instead of somewhere between 4,000 and 14,600 depending on how long the + * identifiers happen to be) and turns an opaque 413 into a validation error + * naming the limit; over stdio it is the only bound there is. * * 3,000 is chosen to fit, not for its own sake. The HTTP transport's body * ceiling is express's 100 KiB default (see `startHttpServer`), and a diff --git a/src/http.ts b/src/http.ts index 4145230..84277ec 100644 --- a/src/http.ts +++ b/src/http.ts @@ -1,4 +1,5 @@ import { randomUUID } from "node:crypto"; +import { estimateAnalysisBodyBytes } from "./tools/limits.js"; import type { Server } from "node:http"; import type { Request, Response } from "express"; import { createMcpExpressApp } from "@modelcontextprotocol/sdk/server/express.js"; @@ -14,6 +15,7 @@ import { CONTENT_SERVICE_URL, ANALYSIS_SERVICE_URL, NEO4J_URI, + MAX_ANALYSIS_IDENTIFIERS, } from "./config.js"; interface Session { @@ -35,6 +37,16 @@ interface Session { * shared per-connection state; `createServer()` exists precisely so that * building one per session is cheap. */ +/** + * The request body ceiling that actually applies, in bytes. + * + * Not ours and not configurable: `createMcpExpressApp` mounts + * `express.json()` with no limit, so express's `100kb` default is what a + * request meets. Measured against a real server rather than read off the + * default -- 102,392 bytes are accepted, 102,992 are refused with 413. + */ +const EXPRESS_JSON_LIMIT_BYTES = 102_400; + export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Promise { // Defaults to 127.0.0.1 and turns on DNS-rebinding protection for localhost // hosts, which is what stops a web page in the user's browser from driving @@ -57,6 +69,22 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro // `tests/body-limit.test.ts` holds the two together, so an SDK upgrade that // moves this ceiling fails there rather than in production. + // That test guards the *default* cap. `MCP_MAX_ANALYSIS_IDENTIFIERS` can + // raise it at runtime, which puts the two ceilings back into disagreement + // on a deployment no test ever sees -- and the symptom is a bare 413 that + // names nothing. So the check is repeated here, against the configured + // value, where the operator who set it will read it. + const worstCaseBody = estimateAnalysisBodyBytes(MAX_ANALYSIS_IDENTIFIERS); + if (worstCaseBody > EXPRESS_JSON_LIMIT_BYTES) { + logger.warn("MCP_MAX_ANALYSIS_IDENTIFIERS is larger than this transport can carry", { + maxAnalysisIdentifiers: MAX_ANALYSIS_IDENTIFIERS, + worstCaseBodyBytes: worstCaseBody, + bodyLimitBytes: EXPRESS_JSON_LIMIT_BYTES, + effect: "a request at the cap is refused with 413 before validation runs", + hint: "lower the cap, or use stdio, which has no body limit", + }); + } + const sessions = new Map(); const closeSession = (sessionId: string, why: string) => { diff --git a/src/tools/limits.ts b/src/tools/limits.ts index 323cc66..558b143 100644 --- a/src/tools/limits.ts +++ b/src/tools/limits.ts @@ -6,18 +6,21 @@ import { MAX_ANALYSIS_IDENTIFIERS } from "../config.js"; * * `installToolWrapper` already caps what a call returns, and says why it does * so in one place: "a per-tool guard is a guard somebody forgets to add to the - * fifty-seventh". Nothing capped the other direction. On a private instance - * that was academic. Fronted by a public nginx it is not: + * fifty-seventh". Nothing capped the other direction: * `reactome_analyze_identifiers` took `z.array(nonEmptyString)` with no - * maximum and posted `identifiers.join("\n")` to the Analysis Service, so one - * small MCP call could commission an arbitrarily large analysis job and a - * stored result — an amplification with no ceiling anywhere in the path. + * maximum and posted `identifiers.join("\n")` to the Analysis Service. + * + * I first justified this as an unbounded amplification. Over HTTP that was + * overstated — express's 100 KiB default already bounded it, by accident + * rather than by anyone's decision. Over stdio nothing bounded it at all. + * `MAX_ANALYSIS_IDENTIFIERS` says what the cap actually buys. * * An input cap cannot be applied centrally the way the output cap is, because * only the caller knows what a sane length is for a given argument. What can - * be central is the *requirement*: `tests/input-bounds.test.ts` walks every - * registered tool schema and fails on any array without a maximum, so the - * fifty-seventh tool cannot quietly reintroduce this. + * be central is the *requirement*: `tests/input-bounds.test.ts` drives every + * registered tool, in every configuration, and fails on any argument that + * accepts an absurd array — so the fifty-seventh tool cannot quietly + * reintroduce this. */ /** @@ -46,3 +49,20 @@ export function optionalList(max: number, describe: string) { /** Identifier lists posted to the Analysis Service. Configurable: a real * enrichment can be large, and the right ceiling depends on the deployment. */ export const identifierList = (describe: string) => boundedList(MAX_ANALYSIS_IDENTIFIERS, describe); + +/** + * A deliberately pessimistic size for a `tools/call` carrying `count` + * identifiers, in bytes. + * + * Used to warn at startup when a configured cap cannot fit through the HTTP + * transport. It must never *under*-state the real body, or the warning is + * worse than none; `tests/body-limit.test.ts` asserts it stays at or above a + * real serialised request, so the margin cannot silently erode. + * + * 20 characters is longer than a gene symbol or a UniProt accession and + * longer than an Ensembl gene ID (15). 5 bytes per element covers the quotes, + * comma and JSON whitespace; 256 covers the JSON-RPC envelope. + */ +export function estimateAnalysisBodyBytes(count: number, identifierLength = 20): number { + return 256 + count * (identifierLength + 5); +} diff --git a/tests/body-limit.test.ts b/tests/body-limit.test.ts index b466456..25a698c 100644 --- a/tests/body-limit.test.ts +++ b/tests/body-limit.test.ts @@ -26,6 +26,7 @@ import { request as httpRequest, type Server } from "node:http"; import type { AddressInfo } from "node:net"; import { startHttpServer } from "../src/http.js"; import { MAX_ANALYSIS_IDENTIFIERS } from "../src/config.js"; +import { estimateAnalysisBodyBytes } from "../src/tools/limits.js"; /** Longer than a gene symbol or a UniProt accession; an Ensembl gene ID is 15. */ const PESSIMISTIC_IDENTIFIER = "X".repeat(20); @@ -98,3 +99,27 @@ describe("request body ceiling", () => { expect(bytes).toBeLessThan(90_000); }); }); + +describe("the startup estimate", () => { + /** + * `startHttpServer` warns when the *configured* cap cannot fit through the + * transport, using an estimate rather than a serialised request. An + * estimate that under-states the real body would make that warning worse + * than none: it would stay silent on exactly the misconfiguration it + * exists to report. + */ + it("never under-states a real request at the cap", () => { + const real = Buffer.byteLength(analyzeCall(MAX_ANALYSIS_IDENTIFIERS)); + expect(estimateAnalysisBodyBytes(MAX_ANALYSIS_IDENTIFIERS)).toBeGreaterThanOrEqual(real); + }); + + it("would fire on the cap that was actually wrong", () => { + // 10,000 was shipped in the previous commit and could not be delivered. + // If the estimate does not flag that, it flags nothing worth flagging. + expect(estimateAnalysisBodyBytes(10_000)).toBeGreaterThan(102_400); + }); + + it("does not fire on the cap in use", () => { + expect(estimateAnalysisBodyBytes(MAX_ANALYSIS_IDENTIFIERS)).toBeLessThan(102_400); + }); +}); diff --git a/tests/input-bounds.test.ts b/tests/input-bounds.test.ts index 6b53332..e8bb9fb 100644 --- a/tests/input-bounds.test.ts +++ b/tests/input-bounds.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; import { registerAllTools } from "../src/tools/index.js"; import { MAX_ANALYSIS_IDENTIFIERS } from "../src/config.js"; @@ -45,18 +45,65 @@ function registeredSchemas(): Map { return schemas; } +/** + * Every tool in *every* configuration, not just the default one. + * + * The first version of the sweep called `registerAllTools` once, with no + * environment, and so walked 59 of the 62 tools -- the three Cypher tools + * register only behind `NEO4J_URI` plus `MCP_ALLOW_CYPHER`. A sweep whose + * entire value is completeness, quietly covering a subset, is the exact + * failure it was written to prevent. It is also the same shape as the Cypher + * gate itself: the thing that varies by configuration, checked in one + * configuration. + */ +async function allSchemas(): Promise> { + const merged = new Map(); + const configs = [{}, { NEO4J_URI: "bolt://localhost:7690", MCP_ALLOW_CYPHER: "1" }]; + for (const env of configs) { + vi.resetModules(); + const previous: Record = {}; + for (const [k, v] of Object.entries(env)) { + previous[k] = process.env[k]; + process.env[k] = v; + } + try { + const mod = await import("../src/tools/index.js"); + const server = { + tool: (...args: unknown[]) => { + const name = args[0]; + const shape = args[2]; + if (typeof name === "string" && shape && typeof shape === "object") { + merged.set(name, shape as Shape); + } + return undefined; + }, + }; + mod.registerAllTools(server as never); + } finally { + for (const [k, v] of Object.entries(previous)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + vi.resetModules(); + } + } + return merged; +} + const ABSURD = Array.from({ length: 100_001 }, () => "R-HSA-109582"); const ONE = ["R-HSA-109582"]; describe("tool input bounds", () => { - const schemas = registeredSchemas(); - - it("registers tools to check", () => { - // Without this the sweep below would pass vacuously against an empty map. - expect(schemas.size).toBeGreaterThan(50); + it("covers every tool in every configuration, including Cypher", async () => { + const schemas = await allSchemas(); + // Without this the sweep would pass vacuously against an empty map, and + // without the Cypher names it would pass while missing three tools. + expect(schemas.size).toBe(62); + expect([...schemas.keys()].filter(n => n.startsWith("reactome_cypher"))).toHaveLength(3); }); - it("has no argument anywhere that accepts an unbounded list", () => { + it("has no argument anywhere that accepts an unbounded list", async () => { + const schemas = await allSchemas(); const unbounded: string[] = []; let arrayArgs = 0; for (const [tool, shape] of schemas) {