Nothing capped what a caller could send in

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
+);

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<Server> {
   // 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<string, Session>();
 

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"),
     },

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();

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);
+}

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<string, string | number | boolean | undefined> = {

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<number> {
+  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);
+  });
+});