diff --git a/src/config.ts b/src/config.ts index db14790..3a283a6 100644 --- a/src/config.ts +++ b/src/config.ts @@ -140,3 +140,40 @@ 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. + * + * 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 + * `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, + 3_000 +); diff --git a/src/http.ts b/src/http.ts index cd9a0dc..84277ec 100644 --- a/src/http.ts +++ b/src/http.ts @@ -1,7 +1,7 @@ import { randomUUID } from "node:crypto"; +import { estimateAnalysisBodyBytes } from "./tools/limits.js"; 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"; @@ -15,6 +15,7 @@ import { CONTENT_SERVICE_URL, ANALYSIS_SERVICE_URL, NEO4J_URI, + MAX_ANALYSIS_IDENTIFIERS, } from "./config.js"; interface Session { @@ -36,12 +37,53 @@ 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 // 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. + + // 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(); 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..558b143 --- /dev/null +++ b/src/tools/limits.ts @@ -0,0 +1,68 @@ +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: + * `reactome_analyze_identifiers` took `z.array(nonEmptyString)` with no + * 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` 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. + */ + +/** + * 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); + +/** + * 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/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/body-limit.test.ts b/tests/body-limit.test.ts new file mode 100644 index 0000000..25a698c --- /dev/null +++ b/tests/body-limit.test.ts @@ -0,0 +1,125 @@ +/** + * 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"; +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); + +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); + }); +}); + +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 new file mode 100644 index 0000000..e8bb9fb --- /dev/null +++ b/tests/input-bounds.test.ts @@ -0,0 +1,177 @@ +import { describe, it, expect, vi } 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; +} + +/** + * 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", () => { + 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", async () => { + const schemas = await allSchemas(); + 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); + }); +});