From 46ffc37ae4b2b3fb4eeabe3724f75e194877a24f Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:53:24 +0000 Subject: [PATCH 1/2] Cypher tools require an explicit opt-in, not just a connection string Adam's instruction, on the way to publishing this MCP through the website's nginx: the public must not be able to run Cypher queries. Today they can, given one plausible misconfiguration. `registerAllTools` registers the Cypher tools whenever `NEO4J_URI` is set, so whether arbitrary graph queries are exposed is a side effect of a connection string rather than a decision anybody made. A public instance that set `NEO4J_URI` for any other reason -- the graph-schema warm-up on startup already wants it, and a future non-Cypher graph tool would -- would publish `reactome_cypher_query` by doing so, and nothing would look wrong. `MCP_ALLOW_CYPHER=1` is now required as well. Default-deny, and separate from the connection on purpose: forgetting it costs a missing tool on an internal instance, which is visible and harmless, where forgetting the inverse costs arbitrary query access on a public one. An instance with `NEO4J_URI` and no opt-in logs that the tools are off and how to turn them on, because an operator who expected them needs to know why they are absent. Silently present is the failure being prevented; silently absent would be a different one. Five tests over the combinations, including the two that matter: a connection alone registers nothing, and both together registers something. Without the second, the first three would pass against a build that never registers Cypher at all and prove nothing. Verified by reverting the gate: two fail. Note for anyone running the suite: the host's node is v18 and vitest needs node:util's styleText, so tests run under node:22 -- `docker run --rm -v "$PWD":/srv -w /srv node:22-slim npm run check`. Co-Authored-By: Claude Opus 5 --- src/config.ts | 18 ++++++ src/tools/index.ts | 18 +++++- tests/cypher-exposure.test.ts | 100 ++++++++++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 tests/cypher-exposure.test.ts diff --git a/src/config.ts b/src/config.ts index 421167b..db14790 100644 --- a/src/config.ts +++ b/src/config.ts @@ -66,6 +66,24 @@ export const NEO4J_USER = process.env.NEO4J_USER ?? "neo4j"; export const NEO4J_PASSWORD = process.env.NEO4J_PASSWORD ?? "neo4j"; export const NEO4J_DATABASE = process.env.NEO4J_DATABASE ?? "graph.db"; +/** + * Whether the Cypher tools may be registered at all. + * + * Default-deny, and deliberately a separate switch from `NEO4J_URI`. + * + * Until 2026-09-21 the Cypher tools appeared whenever `NEO4J_URI` was set, + * which made "can the public run arbitrary graph queries" a side effect of a + * connection string rather than a decision. That is the wrong shape for this + * particular capability: a deployment might set `NEO4J_URI` for any number of + * good reasons -- the graph-schema warm-up, a future non-Cypher graph tool -- + * and would silently publish `reactome_cypher_query` by doing so. + * + * So it is an opt-in. Forgetting it costs a missing tool on an internal + * instance, which is visible and harmless. Forgetting the inverse would have + * cost arbitrary query access on a public one. + */ +export const ALLOW_CYPHER_TOOLS = process.env.MCP_ALLOW_CYPHER === "1"; + function parsePositiveInt(raw: string | undefined, fallback: number): number { if (!raw) return fallback; const n = Number(raw); diff --git a/src/tools/index.ts b/src/tools/index.ts index a873662..41417df 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -13,9 +13,10 @@ import { registerInteractorTools } from "./interactors.js"; import { registerGsaTools } from "./gsa.js"; import { registerCypherTools } from "./cypher.js"; import { isNeo4jConfigured } from "../clients/neo4j.js"; +import { logger } from "../logger.js"; import { withNewRequestContext } from "../context.js"; import { capToolResult } from "../response-limits.js"; -import { MAX_TOOL_RESPONSE_CHARS } from "../config.js"; +import { ALLOW_CYPHER_TOOLS, MAX_TOOL_RESPONSE_CHARS } from "../config.js"; /** * Wrap `server.tool` so every handler runs inside a fresh request context. @@ -63,9 +64,20 @@ export function registerAllTools(server: McpServer) { registerInteractorTools(server); registerGsaTools(server); - // Graph database tools — only when NEO4J_URI is set - if (isNeo4jConfigured()) { + // Graph database tools — a connection AND an explicit opt-in. + // + // `NEO4J_URI` alone used to be enough, which made arbitrary query access a + // side effect of a connection string. A public instance that set it for any + // other reason would have published `reactome_cypher_query`. + if (isNeo4jConfigured() && ALLOW_CYPHER_TOOLS) { registerCypherTools(server); + } else if (isNeo4jConfigured()) { + // Said out loud, because an operator who set NEO4J_URI expecting these + // tools needs to know why they are absent. The reverse -- silently + // present -- is the failure this guard exists for. + logger.warn("Cypher tools are OFF: MCP_ALLOW_CYPHER is not 1", { + hint: "Set MCP_ALLOW_CYPHER=1 on an instance that is not publicly reachable.", + }); } // Register utility tools directly here diff --git a/tests/cypher-exposure.test.ts b/tests/cypher-exposure.test.ts new file mode 100644 index 0000000..a90f675 --- /dev/null +++ b/tests/cypher-exposure.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +/** + * Who can run arbitrary graph queries. + * + * Until 2026-09-21 the Cypher tools registered whenever `NEO4J_URI` was set, + * so "can the public run Cypher" was a side effect of a connection string. A + * public instance that set it for the graph-schema warm-up, or for a future + * non-Cypher graph tool, would have published `reactome_cypher_query` by + * doing so — and nothing would have looked wrong. + * + * These drive `registerAllTools` with a recording stub and assert on which + * tool names reach it, because that is the only place the answer is visible. + */ + +const TOOL_NAMES = () => { + const names: string[] = []; + const server = { + tool: (...args: unknown[]) => { + if (typeof args[0] === "string") names.push(args[0]); + return undefined; + }, + }; + return { server, names }; +}; + +async function registerWith(env: Record) { + vi.resetModules(); + const previous: Record = {}; + for (const [key, value] of Object.entries(env)) { + previous[key] = process.env[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + const { registerAllTools } = await import("../src/tools/index.js"); + const { server, names } = TOOL_NAMES(); + registerAllTools(server as never); + return names; + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } +} + +const isCypher = (name: string) => name.startsWith("reactome_cypher"); + +describe("Cypher tool exposure", () => { + beforeEach(() => vi.resetModules()); + afterEach(() => vi.resetModules()); + + it("registers no Cypher tools with neither variable set", async () => { + const names = await registerWith({ + NEO4J_URI: undefined, + MCP_ALLOW_CYPHER: undefined, + }); + expect(names.filter(isCypher)).toEqual([]); + expect(names.length).toBeGreaterThan(0); + }); + + it("registers no Cypher tools when NEO4J_URI alone is set", async () => { + // The case that mattered: a connection string is not consent. This is + // the configuration a public instance would most plausibly arrive at. + const names = await registerWith({ + NEO4J_URI: "bolt://localhost:7690", + MCP_ALLOW_CYPHER: undefined, + }); + expect(names.filter(isCypher)).toEqual([]); + }); + + it("registers no Cypher tools when the opt-in is set without a connection", async () => { + const names = await registerWith({ + NEO4J_URI: undefined, + MCP_ALLOW_CYPHER: "1", + }); + expect(names.filter(isCypher)).toEqual([]); + }); + + it("registers them only when both are set", async () => { + // Without this the three above would pass against a build that never + // registers Cypher at all, and prove nothing. + const names = await registerWith({ + NEO4J_URI: "bolt://localhost:7690", + MCP_ALLOW_CYPHER: "1", + }); + expect(names.filter(isCypher).length).toBeGreaterThan(0); + }); + + it("treats any value other than 1 as not opted in", async () => { + for (const value of ["", "0", "true", "yes", "TRUE"]) { + const names = await registerWith({ + NEO4J_URI: "bolt://localhost:7690", + MCP_ALLOW_CYPHER: value, + }); + expect(names.filter(isCypher), `MCP_ALLOW_CYPHER=${value}`).toEqual([]); + } + }); +}); From a838c94951a81f1ebe6063bb21843ea0253454f0 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 16:14:23 +0000 Subject: [PATCH 2/2] Adversarial review: the guard covered the tools and missed two other doors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing the opt-in before merge, against its own claim: does a server told not to offer Cypher actually not offer it? Three surfaces answer that question and the first commit changed one. **The server instructions.** `buildServerInstructions()` still tested `isNeo4jConfigured()`, so an instance with a connection and no opt-in appended the "Graph database (Cypher) — enabled" section and told every client to call `reactome_cypher_schema` and `reactome_cypher_query` — tools it had just declined to register. Not an exposure, but a server confidently instructing clients to use what it does not have. **The `reactome://graph/schema` resource.** Also gated on the connection alone. It is not a query surface, but it runs `apoc.meta.schema()` on the caller's behalf and returns labels, counts, relationship cardinalities, property types, indexes and constraints — the internal graph model, handed to anyone who can read resources. If a connection string is not consent for the tools, it is not consent for this either. The fix is to stop asking the question in three places. `isCypherEnabled()` lives next to `isNeo4jConfigured()` and all three call it. Splitting one decision across independent tests is exactly how the first two got left behind. Also: the startup schema prefetch now waits for the opt-in too — without it nothing can read that cache, so it was opening a Neo4j connection for nobody — and `/health` and the startup log report `cypherEnabled` alongside `neo4jEnabled`, so an operator can see why the tools are absent rather than inferring it. Four tests, each asserted in both directions, because "the section is absent" and "the resource is not registered" both pass against a build that never offers them. Verified by reverting each gate separately: exactly the two new negative tests fail, the positive ones stay green. lint, format, typecheck, build and 116 tests clean under node:22. Co-Authored-By: Claude Opus 5 --- src/clients/neo4j.ts | 16 +++++++ src/http.ts | 3 ++ src/index.ts | 7 ++- src/instructions.ts | 6 ++- src/resources/static.ts | 9 ++-- src/tools/index.ts | 6 +-- tests/cypher-exposure.test.ts | 85 +++++++++++++++++++++++++++++++++++ 7 files changed, 123 insertions(+), 9 deletions(-) diff --git a/src/clients/neo4j.ts b/src/clients/neo4j.ts index 997fb80..226bbff 100644 --- a/src/clients/neo4j.ts +++ b/src/clients/neo4j.ts @@ -5,6 +5,7 @@ import { NEO4J_PASSWORD, NEO4J_DATABASE, CYPHER_QUERY_TIMEOUT_MS, + ALLOW_CYPHER_TOOLS, } from "../config.js"; import { logger } from "../logger.js"; @@ -14,6 +15,21 @@ export function isNeo4jConfigured(): boolean { return Boolean(NEO4J_URI); } +/** + * Whether anything Cypher-shaped is offered to clients: the three + * `reactome_cypher_*` tools, the `reactome://graph/schema` resource, and the + * Cypher section of the server instructions. + * + * All three are the same decision, so they ask the same question here. They + * used to each test `isNeo4jConfigured()` independently, which is how the + * opt-in added on 2026-09-21 reached the tools and left the other two behind: + * a server that had been told not to offer Cypher still published the graph + * schema and still instructed clients to call tools it had not registered. + */ +export function isCypherEnabled(): boolean { + return isNeo4jConfigured() && ALLOW_CYPHER_TOOLS; +} + function isLocalhost(uri: string): boolean { try { const host = new URL(uri).hostname; diff --git a/src/http.ts b/src/http.ts index 1455de2..cd9a0dc 100644 --- a/src/http.ts +++ b/src/http.ts @@ -7,6 +7,7 @@ import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { createServer, SERVER_NAME, SERVER_VERSION } from "./server.js"; import { logger } from "./logger.js"; +import { isCypherEnabled } from "./clients/neo4j.js"; import { MCP_HTTP_HOST, MCP_MAX_SESSIONS, @@ -178,6 +179,7 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, neo4jEnabled: Boolean(NEO4J_URI), + cypherEnabled: isCypherEnabled(), }); }); @@ -189,6 +191,7 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, neo4jEnabled: Boolean(NEO4J_URI), + cypherEnabled: isCypherEnabled(), }); resolve(http); }); diff --git a/src/index.ts b/src/index.ts index f629510..8d99165 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,6 +5,7 @@ import { createServer } from "./server.js"; import { logger } from "./logger.js"; import { CONTENT_SERVICE_URL, ANALYSIS_SERVICE_URL, NEO4J_URI } from "./config.js"; import { fetchGraphSchema } from "./graph/schema.js"; +import { isCypherEnabled } from "./clients/neo4j.js"; async function main() { // Built here rather than at module scope, so importing this file does not @@ -16,13 +17,17 @@ async function main() { contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, neo4jEnabled: Boolean(NEO4J_URI), + cypherEnabled: isCypherEnabled(), }); // Warm the schema cache in the background so the first // reactome_cypher_schema call (or reactome://graph/schema read) doesn't // wait 15–30s on apoc.meta.schema(). Failures are logged; the cache // stays empty and the tool call will retry on demand. - if (NEO4J_URI) { + // Gated on the opt-in, not the connection: without it there is no schema + // tool and no schema resource, so the prefetch would warm a cache nothing + // can read and open a Neo4j connection for nobody. + if (isCypherEnabled()) { fetchGraphSchema().catch(err => { logger.warn("graph schema prefetch failed; will retry on first use", { error: err instanceof Error ? err.message : String(err), diff --git a/src/instructions.ts b/src/instructions.ts index c11f532..1505d65 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -1,4 +1,4 @@ -import { isNeo4jConfigured } from "./clients/neo4j.js"; +import { isCypherEnabled } from "./clients/neo4j.js"; const CORE_INSTRUCTIONS = ` This server exposes the Reactome pathway knowledgebase (https://reactome.org) to LLM clients. Reactome is a manually curated, peer-reviewed database of biological pathways: reactions grouped into pathways grouped into hierarchies, annotated with participants (proteins, complexes, small molecules), regulation, literature, species, and disease. @@ -53,6 +53,8 @@ A local Neo4j Reactome graph is available. Use it when the user wants a query th export function buildServerInstructions(): string { const parts = [CORE_INSTRUCTIONS]; - if (isNeo4jConfigured()) parts.push(CYPHER_INSTRUCTIONS); + // Not `isNeo4jConfigured()`: with a connection but no opt-in the tools this + // section tells the client to call do not exist. + if (isCypherEnabled()) parts.push(CYPHER_INSTRUCTIONS); return parts.join("\n\n"); } diff --git a/src/resources/static.ts b/src/resources/static.ts index c2137d7..f0762db 100644 --- a/src/resources/static.ts +++ b/src/resources/static.ts @@ -1,7 +1,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { contentClient } from "../clients/content.js"; import type { Species, Disease } from "../types/index.js"; -import { isNeo4jConfigured } from "../clients/neo4j.js"; +import { isCypherEnabled } from "../clients/neo4j.js"; import { fetchGraphSchema } from "../graph/schema.js"; export function registerStaticResources(server: McpServer) { @@ -70,8 +70,11 @@ export function registerStaticResources(server: McpServer) { }; }); - // Graph schema (opt-in, requires NEO4J_URI) - if (isNeo4jConfigured()) { + // Graph schema — the same opt-in as the Cypher tools. It is not a query + // surface, but it runs apoc.meta.schema() for the caller and publishes the + // internal graph model, so a server told not to offer Cypher should not be + // handing this out either. + if (isCypherEnabled()) { server.resource("reactome://graph/schema", "reactome://graph/schema", async () => { const schema = await fetchGraphSchema(); return { diff --git a/src/tools/index.ts b/src/tools/index.ts index 41417df..2ff6f03 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -12,11 +12,11 @@ import { registerExportTools } from "./export.js"; import { registerInteractorTools } from "./interactors.js"; import { registerGsaTools } from "./gsa.js"; import { registerCypherTools } from "./cypher.js"; -import { isNeo4jConfigured } from "../clients/neo4j.js"; +import { isCypherEnabled, isNeo4jConfigured } from "../clients/neo4j.js"; import { logger } from "../logger.js"; import { withNewRequestContext } from "../context.js"; import { capToolResult } from "../response-limits.js"; -import { ALLOW_CYPHER_TOOLS, MAX_TOOL_RESPONSE_CHARS } from "../config.js"; +import { MAX_TOOL_RESPONSE_CHARS } from "../config.js"; /** * Wrap `server.tool` so every handler runs inside a fresh request context. @@ -69,7 +69,7 @@ export function registerAllTools(server: McpServer) { // `NEO4J_URI` alone used to be enough, which made arbitrary query access a // side effect of a connection string. A public instance that set it for any // other reason would have published `reactome_cypher_query`. - if (isNeo4jConfigured() && ALLOW_CYPHER_TOOLS) { + if (isCypherEnabled()) { registerCypherTools(server); } else if (isNeo4jConfigured()) { // Said out loud, because an operator who set NEO4J_URI expecting these diff --git a/tests/cypher-exposure.test.ts b/tests/cypher-exposure.test.ts index a90f675..145d549 100644 --- a/tests/cypher-exposure.test.ts +++ b/tests/cypher-exposure.test.ts @@ -98,3 +98,88 @@ describe("Cypher tool exposure", () => { } }); }); + +/** + * The tools were the obvious surface. Two others answer the same question and + * were left behind by the first version of this guard: the server's own + * instructions, which tell a client Cypher is available and name the tools to + * call, and the `reactome://graph/schema` resource, which runs + * apoc.meta.schema() for the caller and returns the internal graph model. + * + * Each of these is asserted in both directions. The absent case alone would + * pass against a build that never offers the thing at all. + */ + +async function withEnv( + env: Record, + body: () => Promise +): Promise { + vi.resetModules(); + const previous: Record = {}; + for (const [key, value] of Object.entries(env)) { + previous[key] = process.env[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + return await body(); + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } +} + +const CONNECTED_ONLY = { NEO4J_URI: "bolt://localhost:7690", MCP_ALLOW_CYPHER: undefined }; +const OPTED_IN = { NEO4J_URI: "bolt://localhost:7690", MCP_ALLOW_CYPHER: "1" }; + +async function instructions(env: Record) { + return withEnv(env, async () => { + const { buildServerInstructions } = await import("../src/instructions.js"); + return buildServerInstructions(); + }); +} + +describe("server instructions", () => { + it("does not advertise Cypher on a connection alone", async () => { + const text = await instructions(CONNECTED_ONLY); + expect(text).not.toContain("reactome_cypher_query"); + expect(text).not.toContain("Graph database (Cypher)"); + // Still a usable server: the core instructions are there. + expect(text).toContain("reactome_search"); + }); + + it("advertises Cypher once opted in", async () => { + const text = await instructions(OPTED_IN); + expect(text).toContain("reactome_cypher_query"); + }); +}); + +async function resourceNames(env: Record) { + return withEnv(env, async () => { + const { registerStaticResources } = await import("../src/resources/static.js"); + const names: string[] = []; + const server = { + resource: (...args: unknown[]) => { + if (typeof args[0] === "string") names.push(args[0]); + return undefined; + }, + }; + registerStaticResources(server as never); + return names; + }); +} + +describe("graph schema resource", () => { + it("is not registered on a connection alone", async () => { + const names = await resourceNames(CONNECTED_ONLY); + expect(names).not.toContain("reactome://graph/schema"); + expect(names).toContain("reactome://species"); + }); + + it("is registered once opted in", async () => { + const names = await resourceNames(OPTED_IN); + expect(names).toContain("reactome://graph/schema"); + }); +});