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/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/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 a873662..2ff6f03 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -12,7 +12,8 @@ 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 { MAX_TOOL_RESPONSE_CHARS } from "../config.js"; @@ -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 (isCypherEnabled()) { 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..145d549 --- /dev/null +++ b/tests/cypher-exposure.test.ts @@ -0,0 +1,185 @@ +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([]); + } + }); +}); + +/** + * 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"); + }); +});