From e5e64004f8811792969f7a9789743d193afa88ba Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 14 Oct 2026 01:03:37 +0000 Subject: [PATCH 1/5] Restore graph database access, off unless two switches are on Adam: put the Cypher capability back, switched off by default, so a publicly hosted instance can never run Cypher -- internally it will be pointed at a dedicated Reactome graph database container. Restored from 22aec8b^: the three `reactome_cypher_*` tools, the `reactome://graph/schema` resource, the Cypher section of the instructions, the startup schema warm-up, `src/clients/neo4j.ts`, `src/graph/`, `neo4j-driver` 6.2.0, and their tests. **The gate.** Graph access exists only when `NEO4J_URI` is set (non-blank) AND `MCP_ALLOW_CYPHER=1`. One predicate, `isGraphAccessEnabled()` in src/graph/gate.ts, read from the environment at call time. 22aec8b removed the tools because the old gate was asked in four places and the HTTP entrypoint asked the wrong question. So this time the driver asks too: `getDriver()` refuses to construct a driver with the gate closed, and a structural test fails if any file but src/clients/neo4j.ts imports `neo4j-driver`. A surface that forgets the check still cannot connect. The predicate is checked by: getDriver(); registerCypherTools(); registerGraphResources(); permittedGroups(), which registerAllTools, registerAllResources and buildServerInstructions apply to whatever group list they are handed; resolveToolGroups() (unset excludes `cypher` when closed; naming `cypher` while closed refuses to start); and warmGraphSchema(), the one warm-up both entrypoints now call instead of deciding for themselves. `/health` and the startup log report `graphAccess`. Tools fit the #40 groups as a new `cypher` group. **Tests first.** tests/graph-access-gate.test.ts replaces no-graph-access.test.ts: six env cases (neither, URI alone, opt-in alone, opt-in "true", blank URI, both) x eight surfaces -- predicate, createServer, registrars with every group passed explicitly, MCP_TOOL_GROUPS, the driver factory, the stdio entrypoint, the HTTP entrypoint, and startHttpServer over real HTTP -- each counting driver constructions on a fake neo4j-driver. 34 tests failed against main before the code. Against a plain revert of 22aec8b, "NEO4J_URI alone > the HTTP entrypoint never constructs a driver" failed: the old door, caught. Sabotage after: weakening getDriver's check fails 2; adding the old URI-only prefetch on top fails 4; skipping the filter in the instructions fails 5; dropping the registrar's own check fails 2. **No APOC.** The Reactome graph image is Neo4j 4.4 Community without APOC, and the old schema code read apoc.meta.*. It now uses built-ins only -- dbms.components, db.labels/db.relationshipTypes with per-token count-store counts, db.schema.nodeTypeProperties/relTypeProperties/ visualization, SHOW INDEXES/CONSTRAINTS falling back to db.indexes()/db.constraints(). Each section fails on its own and is listed as unavailable rather than rendered empty; a fetch where nothing answers throws and is not cached. Tested with a fake driver that answers like 4.4 Community. **Read-only safety.** READ-mode sessions, a server-side timeout, row / width / total caps plus the global response cap. The guard now also refuses LOAD CSV (a read clause READ mode allows, which fetches URLs), and is no longer bypassed by '//' inside a string literal, a comment splitting a procedure name, or backtick-quoted names. **Checked against the real thing** (a Release 97 container, read-only): Neo4j 4.4.48 Community; the schema fetch read every section in ~65 s (2.96M nodes, 108 labels, 88 types, 5,669 label pairs); a CREATE was refused by the READ session; the timeout fired; over HTTP the gate closed (URI only) listed no Cypher tools and opened no driver, and open listed all three and answered a query. Rendered naively the schema digest was 87,000 characters, over the 40,000 cap; it is now 34,341, and the recorded row shapes are in src/graph/schema.ts. Lockfile: pure additions -- neo4j-driver, neo4j-driver-core, neo4j-driver-bolt-connection 6.2.0, rxjs 7.8.2, tslib 2.8.1, buffer 6.0.3, base64-js 1.5.1, ieee754 1.2.1, safe-buffer 5.2.1, string_decoder 1.3.0; each entry byte-identical to the pre-removal lock, no existing entry changed. Also: NEO4J_DATABASE defaults to the server's default database and NEO4J_PASSWORD to none, instead of graph.db / neo4j. The two smoke scripts, which had imported functions that no longer existed, work again. Constitution Principle IV redefined (2.0.0); README, SECURITY, spec 002 and CHANGELOG updated. 209 tests; lint, format, both typechecks and build clean on Node 24; tests also pass on Node 22. Co-Authored-By: Claude Opus 5.5 --- .specify/memory/constitution.md | 68 +++-- CHANGELOG.md | 20 +- README.md | 94 +++++- SECURITY.md | 15 +- package-lock.json | 140 +++++++++ package.json | 1 + scripts/smoke-guard-timeout.mjs | 34 ++- scripts/smoke-neo4j.mjs | 76 +++-- specs/002-transport-and-hosting/spec.md | 24 +- src/clients/neo4j.ts | 116 ++++++++ src/config.ts | 51 +++- src/graph/format-schema.ts | 185 ++++++++++++ src/graph/gate.ts | 35 +++ src/graph/schema.ts | 317 ++++++++++++++++++++ src/graph/warmup.ts | 44 +++ src/http-server.ts | 6 + src/http.ts | 10 +- src/index.ts | 5 + src/instructions.ts | 32 +- src/resources/graph.ts | 29 ++ src/resources/index.ts | 9 +- src/tools/cypher-guard.ts | 74 +++++ src/tools/cypher.ts | 212 +++++++++++++ src/tools/index.ts | 49 ++- tests/cypher-guard.test.ts | 128 ++++++++ tests/cypher-tools.test.ts | 110 +++++++ tests/cypher.test.ts | 46 +++ tests/graph-access-gate.test.ts | 369 +++++++++++++++++++++++ tests/graph-schema.test.ts | 376 ++++++++++++++++++++++++ tests/helpers/fake-neo4j.ts | 80 +++++ tests/input-bounds.test.ts | 12 +- tests/no-graph-access.test.ts | 100 ------- tests/readme-tools.test.ts | 15 + tests/server.test.ts | 27 +- tests/tool-groups.test.ts | 34 ++- 35 files changed, 2698 insertions(+), 245 deletions(-) create mode 100644 src/clients/neo4j.ts create mode 100644 src/graph/format-schema.ts create mode 100644 src/graph/gate.ts create mode 100644 src/graph/schema.ts create mode 100644 src/graph/warmup.ts create mode 100644 src/resources/graph.ts create mode 100644 src/tools/cypher-guard.ts create mode 100644 src/tools/cypher.ts create mode 100644 tests/cypher-guard.test.ts create mode 100644 tests/cypher-tools.test.ts create mode 100644 tests/cypher.test.ts create mode 100644 tests/graph-access-gate.test.ts create mode 100644 tests/graph-schema.test.ts create mode 100644 tests/helpers/fake-neo4j.ts delete mode 100644 tests/no-graph-access.test.ts diff --git a/.specify/memory/constitution.md b/.specify/memory/constitution.md index 26457bc..88fc398 100644 --- a/.specify/memory/constitution.md +++ b/.specify/memory/constitution.md @@ -1,18 +1,22 @@ # Reactome MCP Server Constitution @@ -62,24 +66,36 @@ shipped. ### IV. Reactome data, through the public services -Tools read the Content Service and the Analysis Service over HTTP. The server -holds no Neo4j connection, because there is no code that can open one: the Cypher -tools, the graph schema resource and the `neo4j-driver` dependency were removed on -2026-09-21. +Tools read the Content Service and the Analysis Service over HTTP. **A publicly +reachable instance holds no Neo4j connection.** An internal instance may be +given read-only Cypher access to a dedicated Reactome graph database, and only +when both `NEO4J_URI` and `MCP_ALLOW_CYPHER=1` are set. This is a decision about blast radius, not capability. A public MCP endpoint with -database credentials is a different security proposition from one that can only -make the same calls a browser can. - -**Why removed rather than gated.** This principle was enforced by a gate, which -made it a property of a configuration rather than of the code. The gate was -consulted in four places and one of them — `src/http-server.ts`, the entrypoint -that actually runs in the hosted deployment — still opened a connection on -`NEO4J_URI` alone after the other three were corrected. Neither of the two -consumers needed graph access in any case: the chatbot queries the graph directly -with its own driver when it builds embeddings, and the website needs the -REST-backed tools only. `tests/no-graph-access.test.ts` asserts the absence with -the old switches turned ON, so it cannot pass by being configured off. +database access is a different security proposition from one that can only +make the same calls a browser can: an arbitrary query from the internet can +take the database, and what depends on it, down. + +**History.** Until 2026-09-21 this was enforced by a gate that each surface +consulted for itself. It was consulted in four places and one of them -- +`src/http-server.ts`, the entrypoint that actually runs in the hosted +deployment -- still opened a connection on `NEO4J_URI` alone after the other +three were corrected. So the capability was removed outright. On 2026-10-14 it +was restored for internal use, on a different footing: + +- **One predicate**, `isGraphAccessEnabled()` in `src/graph/gate.ts`, read from + the environment at call time so there is no second copy of the answer. +- **The driver enforces it.** `getDriver()` asks the predicate itself, and + `src/clients/neo4j.ts` is the only file allowed to import `neo4j-driver`. A + surface that forgets to ask still cannot connect. +- **Every door is tested, both ways.** `tests/graph-access-gate.test.ts` drives + the stdio entrypoint, the HTTP entrypoint, `startHttpServer`, `createServer` + and the registrars in every combination of the two variables and counts + driver constructions. A new entrypoint belongs in that matrix. + +What would reverse this: graph access being needed on a public instance, or the +gate failing open anywhere. Either means removing the capability again, not +adding another check. ### V. Analysis runs in the service, not in the agent @@ -114,7 +130,7 @@ for model consumption, transports (stdio today; Streamable HTTP is the open question for hosted use), and the tests and sweeps that keep the above honest. **Out of scope by decision, not by oversight.** Direct Neo4j access from any -deployed instance (Principle IV). Reimplementing analysis (Principle V). +publicly reachable instance (Principle IV). Reimplementing analysis (Principle V). Non-Reactome data sources — the chatbot's cascade reaches Tavily and external search, and that logic belongs in the chatbot, not here. @@ -152,4 +168,4 @@ Amendments bump the version: MAJOR for a principle removed or redefined, MINOR for one added, PATCH for wording. Specs live in `specs/`; the ones that record why something is the way it is are as valuable as the ones that plan what is next. -**Version**: 1.0.0 | **Ratified**: 2026-09-14 | **Last amended**: 2026-09-14 +**Version**: 2.0.0 | **Ratified**: 2026-09-14 | **Last amended**: 2026-10-14 diff --git a/CHANGELOG.md b/CHANGELOG.md index 86fc0f4..0136790 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ All notable changes to this project are documented here. This project adheres to ### Added +- **Graph database access, restored and off by default.** The three `reactome_cypher_*` tools and the `reactome://graph/schema` resource are back for **internal** instances pointed at a dedicated Reactome graph database container. They exist only when **both** `NEO4J_URI` is set and `MCP_ALLOW_CYPHER=1`; a publicly reachable instance must set neither. + + One predicate, `isGraphAccessEnabled()`, is asked by every surface — tool registration (a new `cypher` tool group), the resource, the server instructions, the startup schema warm-up — and by `getDriver()` itself, which is the only code that imports `neo4j-driver`. With the gate closed no driver is ever constructed, in the stdio entrypoint, the HTTP entrypoint, `startHttpServer` or `createServer`. `MCP_TOOL_GROUPS` naming `cypher` with the gate closed refuses to start. `/health` reports `graphAccess`. + + Schema extraction now uses Neo4j's built-in procedures only (`db.schema.*`, `SHOW INDEXES`/`SHOW CONSTRAINTS` with `db.indexes()`/`db.constraints()` as fallback), because the Reactome graph database image runs Neo4j 4.4 Community without APOC. Each section degrades on its own and is reported as unavailable rather than rendered empty. + + The read guard now also refuses `LOAD CSV` (a read clause, so READ mode allows it, and it fetches URLs) and is no longer bypassed by a `//` inside a string literal or by backtick-quoted procedure names. + + `NEO4J_DATABASE` now defaults to the server's default database, and `NEO4J_PASSWORD` to none (connect without credentials), rather than `graph.db` and `neo4j`. + - **`MCP_TOOL_GROUPS` — choose which tools an instance publishes.** All 59 register by default, so a local stdio user is unaffected; a hosted instance names the groups it means to serve (`search`, `pathway`, `entity`, `analysis`, `export`, `interactors`, `gsa`, `utilities`). It is a **registration** switch, not a documentation one. An omitted group's tools are not registered, do not appear in `tools/list`, are not mentioned in the instructions the server sends on connection, and neither are its **resources** — omitting `analysis` withholds `reactome://analysis/{token}` as well, since a capability moved to a URI is not a capability withheld. A tool described nowhere but still answering is the divergence that let this server advertise Cypher tools it had not registered. @@ -16,15 +26,7 @@ All notable changes to this project are documented here. This project adheres to ### Removed -- **BREAKING: all graph database access.** The three `reactome_cypher_*` tools, the `reactome://graph/schema` resource, the Cypher section of the server instructions, the startup schema prefetch and the `neo4j-driver` dependency are gone. `NEO4J_URI`, `NEO4J_USER`, `NEO4J_PASSWORD`, `NEO4J_DATABASE`, `CYPHER_QUERY_TIMEOUT_MS` and `MCP_ALLOW_CYPHER` are inert — nothing reads them. - - **Who this affects:** a curator running this server locally over stdio against `reactome_neo4j_env`, which is who the tools were built for. There is no replacement in this server; query the graph directly, as the Reactome chatbot does with its own driver when it builds embeddings. - - **Why now.** This server is being hosted publicly, behind the website's nginx. Constitution Principle IV already said no deployment holds a Neo4j connection, and enforced it with a gate — which made the property true of a *configuration* rather than of the code. The gate was consulted in four places and one of them, `src/http-server.ts`, the entrypoint that actually runs in the hosted deployment, still opened a connection on `NEO4J_URI` alone after the other three were corrected. Neither consumer of this server needed graph access in the first place. - - The public tool surface is **unchanged at 59** — the Cypher tools were only ever the 60th to 62nd, and only with both switches on. - - `tests/no-graph-access.test.ts` asserts the absence with the old switches turned **on**, so it cannot pass by being configured off. +- ~~**BREAKING: all graph database access.**~~ Reverted before release — see *Graph database access, restored and off by default* above. The tools came back behind a gate that `getDriver()` enforces itself, rather than one each call site had to remember. ### Fixed - **Ten tools read field paths the Reactome services never return.** Each called the right endpoint and reported success, so nothing flagged them: `contentClient.get` asserts `T`, it does not verify it, and 51 of 56 tools had no test. `search_suggest` and `search_spellcheck` expected `{suggestions: []}` where the API returns a bare array; `entity_component_of` expected `Complex` objects where the API returns one entry per relationship type with parallel `names`/`stIds`/`schemaClasses` arrays; `participants` expected `stId` where the API returns `peDbId`; the four interactor tools read `score` one level above where it lives, throwing on `.toFixed`; `analysis_found_entities` read `mapsTo[].identifier` where the API returns `ids[]`. **`search_diagram` had never returned an answer** — it shared the grouped-results helper, but that endpoint returns a flat `entries` array, so every call threw `result.results is not iterable`. diff --git a/README.md b/README.md index b41b161..cb5bf15 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ An [MCP (Model Context Protocol)](https://modelcontextprotocol.io/) server that - **Species & disease** — list available species and disease annotations - **ID mapping** — map external identifiers (UniProt, Ensembl, CHEBI, etc.) to Reactome pathways and reactions -59 tools and 10 resources are registered — see [Tools](#tools) and [Resources](#resources) below for the full list. `tests/readme-tools.test.ts` fails if that count drifts or a tool goes undocumented. +59 tools and 10 resources are registered by default — see [Tools](#tools) and [Resources](#resources) below for the full list. An internal instance can additionally be given read-only Cypher access to a Reactome graph database, which makes 62 with graph access and 11 resources; it is **off** unless two variables are set, and a public instance must never set them (see [Graph database access](#graph-database-access-internal-instances-only)). `tests/readme-tools.test.ts` fails if those counts drift or a tool goes undocumented. ## Prerequisites @@ -57,6 +57,7 @@ MCP_TOOL_GROUPS=search,pathway,entity,utilities,analysis | `interactors` | 6 | | `gsa` | 5 | | `utilities` | 7 | +| `cypher` | 3 — only with [graph database access](#graph-database-access-internal-instances-only) on | This is a **registration** switch, not a documentation one. An omitted group's tools are not registered, are not listed by `tools/list`, and are not @@ -68,9 +69,83 @@ described nowhere but still answering is the failure this avoids. Three cases behave differently on purpose, because the dangerous one is a restriction that quietly becomes "everything": -- **unset** — all groups +- **unset** — all groups (`cypher` only if graph database access is on) - **set and empty** — the server refuses to start - **set with an unknown name** — the server refuses to start, naming the typo +- **names `cypher` while graph database access is off** — the server refuses to start, rather than quietly serving without it + +## Graph database access (internal instances only) + +Three extra tools — `reactome_cypher_schema`, `reactome_cypher_sample` and +`reactome_cypher_query` — and the `reactome://graph/schema` resource let an +agent query a Reactome Neo4j graph database directly. They exist for internal +instances pointed at a **dedicated** graph database container. + +**They are off by default, and a publicly reachable instance must never turn +them on.** An arbitrary Cypher query can run for a long time and hold the +database's memory and CPU while it does; on a public endpoint that is a way for +anyone on the internet to degrade the database and whatever else depends on it. +The read-only safeguards below limit the damage a query can do to the data, not +the load it can put on the server. + +Graph access exists only when **both** of these are set: + +| Variable | Value | | +|---|---|---| +| `NEO4J_URI` | e.g. `bolt://127.0.0.1:7687` | where the graph database listens | +| `MCP_ALLOW_CYPHER` | exactly `1` | the explicit opt-in; any other value is off | + +Either one alone does nothing: no tools, no schema resource, no mention in the +instructions the server sends on connection, and no database connection is +ever opened. `NEO4J_URI` alone is deliberately not enough — a connection string +is easy to inherit from a shared environment and is not a decision to publish +query access. The same check guards every surface (tool registration, the +resource, the instructions, the startup schema warm-up) and the driver factory +itself, in both the stdio and the HTTP entrypoints; +`tests/graph-access-gate.test.ts` drives each of them in every combination of +the two variables. `GET /health` on the HTTP transport reports `graphAccess`. + +Optional: + +| Variable | Default | | +|---|---|---| +| `NEO4J_USER` | `neo4j` | used only when `NEO4J_PASSWORD` is set | +| `NEO4J_PASSWORD` | _(unset)_ | unset connects without credentials, for a database with authentication disabled | +| `NEO4J_DATABASE` | _(server default)_ | Neo4j Community serves a single database, so this is rarely needed | +| `CYPHER_QUERY_TIMEOUT_MS` | `30000` | server-side transaction timeout for `reactome_cypher_*` queries | + +**Read-only safeguards.** Every query runs in a READ-mode session, so the +database refuses `CREATE`, `MERGE`, `DELETE`, `SET` and `REMOVE`. `LOAD CSV` and +APOC procedures that can write or fetch from outside the graph are refused +before they reach the database. Each query runs under a server-side timeout, +and rows, row width and total response size are capped. These are guardrails; +the boundary is that graph access is not enabled where the public can reach it. + +### Running against the Reactome graph database container + +Reactome publishes its graph database as an image. Run it bound to loopback, so +nothing but processes on the same machine can reach it: + +```bash +docker run -d --name reactome-graphdb \ + -p 127.0.0.1:7687:7687 \ + public.ecr.aws/reactome/graphdb:Release97 +``` + +Then point an internal instance at it: + +```bash +NEO4J_URI=bolt://127.0.0.1:7687 MCP_ALLOW_CYPHER=1 node dist/index.js +``` + +Add `NEO4J_USER` / `NEO4J_PASSWORD` if the container is configured with +authentication. The server reads the schema with Neo4j's built-in procedures +(`db.schema.nodeTypeProperties()`, `db.schema.relTypeProperties()`, +`db.schema.visualization()`, `SHOW INDEXES`, `SHOW CONSTRAINTS`), so APOC is not +required — the Reactome image runs Neo4j 4.4 Community without it. The schema +is read once in the background at startup and cached; on the full graph that +can take a while, and a section the database cannot provide is reported as +unavailable rather than shown empty. ## Usage @@ -235,6 +310,17 @@ Starts a local web UI with an MCP bridge for browser-based exploration. | `reactome_orthology` | Get orthologous events/entities in another species | | `reactome_query` | Query any Reactome database object by identifier | +### Graph database / Cypher (3 tools — internal instances only) + +Registered only when `NEO4J_URI` and `MCP_ALLOW_CYPHER=1` are both set; see +[Graph database access](#graph-database-access-internal-instances-only). + +| Tool | Description | +|------|-------------| +| `reactome_cypher_schema` | Labels, relationship types, connecting patterns, properties, indexes and constraints of the graph | +| `reactome_cypher_sample` | A few nodes of one label, to see their shape | +| `reactome_cypher_query` | Run a read-only Cypher query (READ session, timeout, capped output) | + ## Resources ### Static @@ -245,6 +331,7 @@ Starts a local web UI with an MCP bridge for browser-based exploration. | `reactome://species/main` | Main species with curated pathways | | `reactome://diseases` | All annotated diseases | | `reactome://database/info` | Database version and name | +| `reactome://graph/schema` | Graph database schema as JSON — only with graph database access on | ### Templates @@ -280,6 +367,9 @@ MCP_HTTP_PORT=4320 node dist/http-server.js | `MCP_SESSION_TTL_MS` | `1800000` | idle session reaped after 30 min | | `MCP_MAX_SESSIONS` | `256` | concurrent session ceiling | +A hosted instance must leave `NEO4J_URI` and `MCP_ALLOW_CYPHER` unset; see +[Graph database access](#graph-database-access-internal-instances-only). + Endpoints: `POST /mcp` (initialize, then requests), `GET /mcp` (server stream), `DELETE /mcp` (end session), `GET /health`. diff --git a/SECURITY.md b/SECURITY.md index abc4e03..5c7f7af 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -11,8 +11,8 @@ Include, where possible: - A description of the issue and the potential impact - Steps to reproduce (a minimal query, MCP config snippet, or command) -- Whether the issue affects the REST-client path, the HTTP transport, or the web demo -- The version of `reactome-mcp` (see `package.json`) and any relevant runtime versions (Node) +- Whether the issue affects the REST-client path, the HTTP transport, the graph database (Cypher) path, or the web demo +- The version of `reactome-mcp` (see `package.json`) and any relevant runtime versions (Node, Neo4j) You can expect an acknowledgement within a few business days. We will coordinate a fix and, where appropriate, credit you in the advisory. @@ -20,21 +20,24 @@ You can expect an acknowledgement within a few business days. We will coordinate In scope: -- The MCP server (`src/`, `dist/`) and its REST clients. +- The MCP server (`src/`, `dist/`), its REST clients and its optional Neo4j client. - The bundled web demo (`web/`). Out of scope (report upstream): - Vulnerabilities in `https://reactome.org` — report via the Reactome website. +- Vulnerabilities in Neo4j or `neo4j-driver` — report via those projects. - Vulnerabilities in Claude Desktop / Claude Code / the MCP SDK — report to the respective vendor. ## Threat Model (brief) -- The server reads the public Reactome Content and Analysis Services over HTTP and holds no database credentials. As of 2026-09-21 it has **no graph database access at all**: the Cypher tools, the graph schema resource and the `neo4j-driver` dependency were removed when the server began to be hosted publicly. `NEO4J_URI` and `MCP_ALLOW_CYPHER` are inert — nothing reads them. -- It is still **not** self-hardened for internet-facing deployment: it has no authentication, no TLS and no rate limiting of its own. A public deployment must put those at the reverse proxy, and should bind the server to loopback so the proxy is the only way in. What removing graph access changes is the blast radius if that proxy is misconfigured — a caller reaching the server directly can make the same calls a browser can make against reactome.org, and nothing more. +- By default the server reads the public Reactome Content and Analysis Services over HTTP and holds no database credentials. +- **Graph database access is off unless both `NEO4J_URI` and `MCP_ALLOW_CYPHER=1` are set**, and a publicly reachable instance must never set them. With either missing there are no Cypher tools, no graph schema resource, no mention of either in the server instructions, and no Neo4j driver is ever constructed — `getDriver()` checks the same predicate as every surface, so a surface that forgot to check still could not connect. `tests/graph-access-gate.test.ts` holds this for every entrypoint in every combination of the two variables. +- With graph access on, queries run in READ-mode sessions under a server-side timeout, `LOAD CSV` and APOC procedures that can write or fetch from outside the graph are refused before execution, and output is capped. That is a guardrail against accidents, not a boundary against a hostile caller: an expensive read can still load the database. Use a dedicated database, reachable on loopback only, and configure Neo4j's own authentication and plugin settings accordingly. +- It is **not** self-hardened for internet-facing deployment: it has no authentication, no TLS and no rate limiting of its own. A public deployment must put those at the reverse proxy, and should bind the server to loopback so the proxy is the only way in. With graph access off, a caller reaching the server directly can make the same calls a browser can make against reactome.org, and nothing more. - Tool inputs are bounded: every list argument has an explicit maximum, and `MAX_ANALYSIS_IDENTIFIERS` caps what one call may submit to the Analysis Service. The HTTP transport separately refuses a request body over 100 KiB. - The web demo (`web/mcp-bridge.js`) uses an allow-list CORS policy defaulting to localhost. Do not deploy it publicly without adding authentication. ## Handling of Secrets -- The server holds no credentials of its own. REST error bodies are logged verbatim on failure — if you deploy behind an authenticated proxy, ensure upstream errors do not echo credentials. +- With graph access off the server holds no credentials of its own. `NEO4J_PASSWORD`, when set, is read when the driver is built and passed to it; it is never logged. REST error bodies are logged verbatim on failure — if you deploy behind an authenticated proxy, ensure upstream errors do not echo credentials. diff --git a/package-lock.json b/package-lock.json index 133ea8b..65b2d0f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,6 +11,7 @@ "dependencies": { "@modelcontextprotocol/sdk": "^1.30.0", "express": "^5.2.1", + "neo4j-driver": "^6.2.0", "zod": "^4.6.5" }, "bin": { @@ -1441,6 +1442,26 @@ "node": "18 || 20 || >=22" } }, + "node_modules/base64-js": { + "version": "1.5.1", + "resolved": "https://registry.npmjs.org/base64-js/-/base64-js-1.5.1.tgz", + "integrity": "sha512-AKpaYlHn8t4SVbOHCy+b5+KKgvR4vrsD8vbvrbiQJps7fKDTkjkDry6ji0rUJjC0kzbNePLwzxq8iypo41qeWA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT" + }, "node_modules/body-parser": { "version": "2.3.0", "resolved": "https://registry.npmjs.org/body-parser/-/body-parser-2.3.0.tgz", @@ -1491,6 +1512,30 @@ "node": "20 || >=22" } }, + "node_modules/buffer": { + "version": "6.0.3", + "resolved": "https://registry.npmjs.org/buffer/-/buffer-6.0.3.tgz", + "integrity": "sha512-FTiCpNxtwiZZHEZbcbTIcZjERVICn9yq/pDFkTl95/AxzD1naBctN7YO68riM/gLSDY7sdrMby8hofADYuuqOA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT", + "dependencies": { + "base64-js": "^1.3.1", + "ieee754": "^1.2.1" + } + }, "node_modules/bytes": { "version": "3.1.2", "resolved": "https://registry.npmjs.org/bytes/-/bytes-3.1.2.tgz", @@ -2375,6 +2420,26 @@ "url": "https://opencollective.com/express" } }, + "node_modules/ieee754": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/ieee754/-/ieee754-1.2.1.tgz", + "integrity": "sha512-dcyqhDvX1C46lXZcVqCpK+FtMRQVdIMN6/Df5js2zouUsqG7I6sFxitIC+7KYK29KdXOLHdu9zL4sFnoVQnqaA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "BSD-3-Clause" + }, "node_modules/ignore": { "version": "5.3.2", "resolved": "https://registry.npmjs.org/ignore/-/ignore-5.3.2.tgz", @@ -2973,6 +3038,37 @@ "url": "https://opencollective.com/express" } }, + "node_modules/neo4j-driver": { + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/neo4j-driver/-/neo4j-driver-6.2.0.tgz", + "integrity": "sha512-9W/Tk7EyjZHtv87NFpqoIbl0mlQx8bX8phUFzAM9xmeJrRpGDvqkpdQzc11DjwzywQCdhifaZPSvokSnAp7YTg==", + "license": "Apache-2.0", + "dependencies": { + "neo4j-driver-bolt-connection": "6.2.0", + "neo4j-driver-core": "6.2.0", + "rxjs": "^7.8.2" + }, + "engines": { + "node": ">=18.0.0" + } + }, + "node_modules/neo4j-driver-bolt-connection": { + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/neo4j-driver-bolt-connection/-/neo4j-driver-bolt-connection-6.2.0.tgz", + "integrity": "sha512-SEnFSBfumleDxmDAZKkjXxZ5LgL2iZSTx08sO+qXX45ZKaxZJOcg9oxwoXACXZ1RbIq3XbDfHU8I5tExanYEOA==", + "license": "Apache-2.0", + "dependencies": { + "buffer": "^6.0.3", + "neo4j-driver-core": "6.2.0", + "string_decoder": "^1.3.0" + } + }, + "node_modules/neo4j-driver-core": { + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/neo4j-driver-core/-/neo4j-driver-core-6.2.0.tgz", + "integrity": "sha512-U8PSdvDECWmCgSWyhqNNVzMONXFHWPcTd5gqi12+EDxxt0aETA0m+XCXf9H7q6qQXdisc40rUJEOcVQOu6peEA==", + "license": "Apache-2.0" + }, "node_modules/object-assign": { "version": "4.1.1", "resolved": "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz", @@ -3350,6 +3446,35 @@ "node": ">= 18" } }, + "node_modules/rxjs": { + "version": "7.8.2", + "resolved": "https://registry.npmjs.org/rxjs/-/rxjs-7.8.2.tgz", + "integrity": "sha512-dhKf903U/PQZY6boNNtAGdWbG85WAbjT/1xYoZIC7FAY0yWapOBQVsVrDl58W86//e1VpMNBtRV4MaXfdMySFA==", + "license": "Apache-2.0", + "dependencies": { + "tslib": "^2.1.0" + } + }, + "node_modules/safe-buffer": { + "version": "5.2.1", + "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.2.1.tgz", + "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT" + }, "node_modules/safer-buffer": { "version": "2.1.2", "resolved": "https://registry.npmjs.org/safer-buffer/-/safer-buffer-2.1.2.tgz", @@ -3553,6 +3678,15 @@ "dev": true, "license": "MIT" }, + "node_modules/string_decoder": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.3.0.tgz", + "integrity": "sha512-hkRX8U1WjJFd8LsDJ2yQ/wWWxaopEsABU1XfkM8A+j0+85JAGppt16cr1Whg6KIbb4okU6Mql6BOj+uup/wKeA==", + "license": "MIT", + "dependencies": { + "safe-buffer": "~5.2.0" + } + }, "node_modules/tinybench": { "version": "6.1.4", "resolved": "https://registry.npmjs.org/tinybench/-/tinybench-6.1.4.tgz", @@ -3622,6 +3756,12 @@ "typescript": ">=4.8.4" } }, + "node_modules/tslib": { + "version": "2.8.1", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.8.1.tgz", + "integrity": "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==", + "license": "0BSD" + }, "node_modules/type-check": { "version": "0.4.0", "resolved": "https://registry.npmjs.org/type-check/-/type-check-0.4.0.tgz", diff --git a/package.json b/package.json index 1f4e68e..9932305 100644 --- a/package.json +++ b/package.json @@ -46,6 +46,7 @@ "dependencies": { "@modelcontextprotocol/sdk": "^1.30.0", "express": "^5.2.1", + "neo4j-driver": "^6.2.0", "zod": "^4.6.5" }, "devDependencies": { diff --git a/scripts/smoke-guard-timeout.mjs b/scripts/smoke-guard-timeout.mjs index 8239f4e..987cb35 100644 --- a/scripts/smoke-guard-timeout.mjs +++ b/scripts/smoke-guard-timeout.mjs @@ -1,18 +1,26 @@ #!/usr/bin/env node -// Verify the APOC write-through guard and the Cypher query timeout. -// Requires NEO4J_URI and a running Neo4j. Uses a short timeout to force it. +// Verify the write-through guard and the Cypher query timeout against a live +// database. Requires NEO4J_URI and MCP_ALLOW_CYPHER=1. Uses a short timeout on +// a deliberately slow read to force the server to terminate it. Needs no APOC. +// Keep the range modest: range() builds its list before the transaction guard +// gets a look in, so a huge one costs the database memory regardless. import { rejectWriteThroughCalls } from "../dist/tools/cypher-guard.js"; -import { runRead } from "../dist/clients/neo4j.js"; +import { runRead, closeDriver } from "../dist/clients/neo4j.js"; let failures = 0; -try { - rejectWriteThroughCalls("CALL apoc.cypher.runWrite('CREATE (n)', {})"); - console.log("FAIL: guard did not reject apoc.cypher.runWrite"); - failures++; -} catch (e) { - console.log("OK: guard rejected apoc.cypher.runWrite —", e.message.slice(0, 80)); +for (const query of [ + "CALL apoc.cypher.runWrite('CREATE (n)', {})", + "LOAD CSV FROM 'http://example.invalid/x.csv' AS row RETURN row", +]) { + try { + rejectWriteThroughCalls(query); + console.log("FAIL: guard did not reject:", query); + failures++; + } catch (e) { + console.log("OK: guard rejected —", e.message.slice(0, 80)); + } } try { @@ -23,9 +31,12 @@ try { failures++; } -// Force a server-side timeout via apoc.util.sleep which blocks for a known duration. try { - await runRead("CALL apoc.util.sleep(5000) RETURN 1", {}, { timeoutMs: 300 }); + await runRead( + "UNWIND range(1, 20000000) AS x WITH x WHERE x % 7 = 0 RETURN count(x)", + {}, + { timeoutMs: 300 } + ); console.log("FAIL: expected timeout but runRead returned"); failures++; } catch (e) { @@ -38,4 +49,5 @@ try { } } +await closeDriver(); process.exit(failures > 0 ? 1 : 0); diff --git a/scripts/smoke-neo4j.mjs b/scripts/smoke-neo4j.mjs index aac93be..11e258e 100644 --- a/scripts/smoke-neo4j.mjs +++ b/scripts/smoke-neo4j.mjs @@ -1,47 +1,63 @@ #!/usr/bin/env node -// Live smoke test for the Cypher tools against a running Reactome Neo4j. -// Requires: NEO4J_URI set (e.g. bolt://localhost:7687) and the DB reachable. -// Not part of the automated test suite — run manually after `npm run build`. - -import { isNeo4jConfigured, fetchGraphSchema, runRead } from "../dist/clients/neo4j.js"; -import { capCypherRows } from "../dist/tools/cypher.js"; - -if (!isNeo4jConfigured()) { - console.error("Set NEO4J_URI (e.g. bolt://localhost:7687) to run this smoke test."); +// Live smoke test for the Cypher tools against a running Reactome graph database. +// Requires: NEO4J_URI (e.g. bolt://127.0.0.1:7687) and MCP_ALLOW_CYPHER=1, and +// the database reachable. Read-only. Not part of the automated test suite -- +// run manually after `npm run build`. Never point it at a database you do not +// own. + +import { isGraphAccessEnabled } from "../dist/graph/gate.js"; +import { runRead, closeDriver } from "../dist/clients/neo4j.js"; +import { fetchGraphSchema } from "../dist/graph/schema.js"; +import { formatGraphSchemaMarkdown } from "../dist/graph/format-schema.js"; + +if (!isGraphAccessEnabled()) { + console.error("Set NEO4J_URI and MCP_ALLOW_CYPHER=1 to run this smoke test."); process.exit(2); } async function main() { - console.log("[smoke] fetching graph schema…"); - const schema = await fetchGraphSchema(); - console.log(` labels: ${schema.labels.length}`); - console.log(` relationship types: ${schema.relationshipTypes.length}`); - console.log(` labels with properties: ${Object.keys(schema.propertiesByLabel).length}`); - console.log(` first 5 labels: ${schema.labels.slice(0, 5).join(", ")}`); - - console.log("[smoke] database info query…"); - const info = await runRead( - "CALL dbms.components() YIELD name, versions, edition RETURN name, versions, edition" + console.log("[smoke] database components…"); + console.log( + " ", + JSON.stringify(await runRead("CALL dbms.components() YIELD name, versions, edition RETURN *")) ); - console.log(" components:", JSON.stringify(info)); + + console.log("[smoke] fetching graph schema (built-in procedures only)…"); + const started = Date.now(); + const schema = await fetchGraphSchema(); + console.log(` took ${Date.now() - started} ms`); + console.log(` nodes: ${schema.stats.nodeCount}, relationships: ${schema.stats.relCount}`); + console.log(` labels: ${Object.keys(schema.stats.labels).length}`); + console.log(` relationship types: ${Object.keys(schema.stats.relTypesCount).length}`); + console.log(` patterns: ${schema.patterns.length}`); + console.log(` node property rows: ${schema.nodeTypeProperties.length}`); + console.log(` rel property rows: ${schema.relTypeProperties.length}`); + console.log(` indexes: ${schema.indexes.length}, constraints: ${schema.constraints.length}`); + console.log(` unavailable: ${JSON.stringify(schema.unavailable)}`); + console.log(` digest: ${formatGraphSchemaMarkdown(schema).length} chars`); console.log("[smoke] sampling Pathway nodes…"); const pathways = await runRead( "MATCH (n:Pathway) RETURN n.stId AS stId, n.displayName AS name LIMIT $limit", { limit: 3 } ); - console.log(" pathways:", JSON.stringify(pathways, null, 2)); - - console.log("[smoke] exercising capCypherRows with a wide row…"); - const wide = Array.from({ length: 5 }, () => ({ big: "x".repeat(4000) })); - const { output, stats } = capCypherRows(wide, 10, 500, 40000); - console.log(` output rows: ${output.length}, stats: ${JSON.stringify(stats)}`); - - console.log("[smoke] OK"); - process.exit(0); + console.log(" pathways:", JSON.stringify(pathways)); + + console.log("[smoke] a write is refused by the READ-mode session…"); + try { + await runRead("CREATE (n:SmokeTestShouldNotExist) RETURN n"); + console.log(" FAIL: the write was accepted"); + process.exitCode = 1; + } catch (error) { + console.log(" OK:", String(error?.message ?? error).slice(0, 120)); + } + + await closeDriver(); + console.log(process.exitCode ? "[smoke] FAILED" : "[smoke] OK"); } -main().catch(err => { +main().catch(async err => { console.error("[smoke] FAILED:", err); + await closeDriver(); process.exit(1); }); diff --git a/specs/002-transport-and-hosting/spec.md b/specs/002-transport-and-hosting/spec.md index 9bcb87d..cc959dd 100644 --- a/specs/002-transport-and-hosting/spec.md +++ b/specs/002-transport-and-hosting/spec.md @@ -22,14 +22,22 @@ session and a second transport from the same registrations — neither is possib while the only instance is a module-scope constant. The idea is harvested from #5 by @adidev001. -**No Neo4j from a deployed instance.** Superseded 2026-09-21 by something -stronger: the graph tools were removed outright, along with the schema resource -and the `neo4j-driver` dependency, so there is no gate to hold. A public endpoint -holding database credentials is a different security proposition from one that -can only make the calls a browser can — and a gate makes that a property of the -configuration, which is how `src/http-server.ts` came to keep opening a -connection on `NEO4J_URI` alone after the other call sites were fixed. -`tests/no-graph-access.test.ts` asserts the absence with the old switches on. +**No Neo4j from a public instance.** History: graph tools sat behind a +`NEO4J_URI` gate, then behind `NEO4J_URI` plus `MCP_ALLOW_CYPHER=1`; on +2026-09-21 they were removed outright, because the gate had been asked in four +places and `src/http-server.ts` still opened a connection on `NEO4J_URI` alone. + +Restored 2026-10-14, switched off by default, for internal instances pointed at +a dedicated Reactome graph database container. The fix for the missed door is +structural rather than a fifth call site: one predicate, +`isGraphAccessEnabled()` in `src/graph/gate.ts`, asked by every surface **and** +by `getDriver()`, the only code that imports `neo4j-driver`. A surface that +forgets the check still gets no driver. `tests/graph-access-gate.test.ts` +drives both entrypoints, `startHttpServer`, `createServer` and the registrars in +every combination of the two variables, counting driver constructions on a fake +driver. A public endpoint holding database credentials is still a different +security proposition from one that can only make the calls a browser can; a +hosted instance does not set either variable. **Analysis runs in the Analysis Service.** The server submits identifiers, holds the token, and formats the reply. diff --git a/src/clients/neo4j.ts b/src/clients/neo4j.ts new file mode 100644 index 0000000..d7f49d8 --- /dev/null +++ b/src/clients/neo4j.ts @@ -0,0 +1,116 @@ +import neo4j, { type Driver } from "neo4j-driver"; +import { CYPHER_QUERY_TIMEOUT_MS, neo4jConnection } from "../config.js"; +import { GRAPH_ACCESS_OFF, isGraphAccessEnabled } from "../graph/gate.js"; +import { logger } from "../logger.js"; + +/** + * The only place in this server that constructs a Neo4j driver. + * + * `tests/graph-access-gate.test.ts` asserts no other source file imports + * `neo4j-driver`, so every connection comes through `getDriver()` -- and + * `getDriver()` asks the gate itself. A surface that forgot to check the gate + * would still get no driver. + */ + +let driverInstance: Driver | null = null; + +function isLoopback(uri: string): boolean { + try { + const host = new URL(uri).hostname; + return host === "localhost" || host === "127.0.0.1" || host === "[::1]" || host === "::1"; + } catch { + return false; + } +} + +export function getDriver(): Driver { + // Asked on every call, not only the first: a driver built while the gate + // was open is not a licence to keep using it once it is not. + if (!isGraphAccessEnabled()) throw new Error(GRAPH_ACCESS_OFF); + + if (!driverInstance) { + const { uri, user, password, database } = neo4jConnection(); + if (!uri) throw new Error(GRAPH_ACCESS_OFF); + + if (!isLoopback(uri) && !password) { + logger.warn("NEO4J_URI is not a loopback address and NEO4J_PASSWORD is unset", { + hint: "Keep the graph database on loopback, or require authentication.", + }); + } + + const auth = password === undefined ? undefined : neo4j.auth.basic(user, password); + driverInstance = neo4j.driver(uri, auth, { disableLosslessIntegers: true }); + logger.info("neo4j driver initialized", { database: database ?? "(server default)" }); + + // process.once expects a void-returning listener. Handing it an async + // function meant a failed close rejected with nobody listening, which Node + // turns into a hard exit on unhandled rejection. + const onShutdown = () => { + void closeDriver().catch((error: unknown) => { + logger.warn("neo4j driver close failed during shutdown", { + error: error instanceof Error ? error.message : String(error), + }); + }); + }; + process.once("SIGINT", onShutdown); + process.once("SIGTERM", onShutdown); + process.once("beforeExit", onShutdown); + } + return driverInstance; +} + +/** Close and forget the driver, if one was built. Safe to call when none was. */ +export async function closeDriver(): Promise { + const driver = driverInstance; + driverInstance = null; + if (driver) await driver.close(); +} + +// JS `number` always serializes to Float64 over Bolt. Cypher features like +// LIMIT / SKIP require an Integer type on the server side, so coerce any +// safe-integer number (and bigint) params into the driver's Integer wrapper. +function coerceIntParams(params: Record): Record { + const out: Record = {}; + for (const [k, v] of Object.entries(params)) { + if (typeof v === "number" && Number.isInteger(v) && Number.isSafeInteger(v)) { + out[k] = neo4j.int(v); + } else if (typeof v === "bigint") { + out[k] = neo4j.int(v.toString()); + } else { + out[k] = v; + } + } + return out; +} + +export interface RunReadOptions { + timeoutMs?: number; +} + +/** + * Run one query in a READ-mode session, under a server-side timeout. + * + * READ mode makes the database refuse native writes (CREATE, MERGE, DELETE, + * SET, REMOVE). The timeout is a transaction timeout: Neo4j terminates the + * transaction if it runs longer, so a runaway traversal ends on the server + * rather than merely being abandoned by this client. + */ +export async function runRead>( + cypher: string, + params: Record = {}, + options: RunReadOptions = {} +): Promise { + const driver = getDriver(); + const { database } = neo4jConnection(); + const session = driver.session({ + ...(database ? { database } : {}), + defaultAccessMode: neo4j.session.READ, + }); + const timeout = options.timeoutMs ?? CYPHER_QUERY_TIMEOUT_MS; + try { + const result = await session.run(cypher, coerceIntParams(params), { timeout }); + return result.records.map(r => r.toObject() as T); + } finally { + await session.close(); + } +} diff --git a/src/config.ts b/src/config.ts index 0da7109..31d2a4e 100644 --- a/src/config.ts +++ b/src/config.ts @@ -61,21 +61,6 @@ export const RESOURCE_TYPES = [ export const DIAGRAM_FORMATS = ["png", "jpg", "jpeg", "svg", "gif"] as const; -/** - * There is no Neo4j configuration here, deliberately. - * - * Constitution Principle IV said the server holds no Neo4j connection in any - * deployment the team runs, and enforced it with a gate. On 2026-09-21 the - * gate was removed along with the graph tools: this server is hosted - * publicly, and neither consumer needed it. The chatbot queries the graph - * directly with its own driver when it builds embeddings, and the website - * needs the REST-backed tools only. - * - * `NEO4J_URI`, `MCP_ALLOW_CYPHER` and the rest are now inert -- setting them - * does nothing, because nothing reads them and no code path opens a - * connection. `tests/no-graph-access.test.ts` keeps that true. - */ - function parsePositiveInt(raw: string | undefined, fallback: number): number { if (!raw) return fallback; const n = Number(raw); @@ -83,6 +68,42 @@ function parsePositiveInt(raw: string | undefined, fallback: number): number { return n; } +/** + * Graph database connection, for internal instances only. + * + * Whether any of this is used is decided by `isGraphAccessEnabled()` in + * src/graph/gate.ts -- `NEO4J_URI` **and** `MCP_ALLOW_CYPHER=1` -- and by + * nothing else. These values are read when a driver is built rather than + * captured at import, so the gate and the connection cannot disagree about + * what the environment says. + * + * `NEO4J_DATABASE` is optional. Neo4j Community serves one user database, and + * leaving it unset uses whichever that is. + */ +export function neo4jConnection(env: NodeJS.ProcessEnv = process.env) { + return { + uri: env.NEO4J_URI?.trim() || undefined, + user: env.NEO4J_USER ?? "neo4j", + password: env.NEO4J_PASSWORD, + database: env.NEO4J_DATABASE?.trim() || undefined, + }; +} + +/** + * Server-side timeout on every Cypher query, in milliseconds. The database + * terminates a transaction that runs longer. + * + * It bounds time, not memory, and not precisely: Neo4j checks the deadline + * periodically, so on the Release 97 graph a 300 ms timeout ended a query + * after about 4 s, and an expression it evaluates eagerly (a huge `range()`) + * can hold memory past the deadline. That is one more reason graph access is + * never enabled on a publicly reachable instance. + */ +export const CYPHER_QUERY_TIMEOUT_MS = parsePositiveInt( + process.env.CYPHER_QUERY_TIMEOUT_MS, + 30_000 +); + /** * Backstop on how much text one tool may return. * diff --git a/src/graph/format-schema.ts b/src/graph/format-schema.ts new file mode 100644 index 0000000..91948b1 --- /dev/null +++ b/src/graph/format-schema.ts @@ -0,0 +1,185 @@ +import type { GraphSchema } from "./schema.js"; + +/** How many labels to name on each side of a relationship type. */ +const LABELS_PER_SIDE = 4; + +type Prop = { propertyName: string; propertyTypes: string[]; mandatory: boolean }; + +/** `stId String!` -- `!` marks a property every node of the set has. */ +const propText = (p: Prop) => + `${p.propertyName} ${(p.propertyTypes ?? []).join("|") || "?"}${p.mandatory ? "!" : ""}`; + +/** + * Labels, most specific first, cut to a few. + * + * Reactome nodes carry their whole class hierarchy as labels -- a Pathway is + * also an Event and a DatabaseObject -- so `db.schema.visualization()` lists a + * relationship type once per pair of labels on its two ends: 749 pairs for one + * type on the Release 97 graph. Rarer labels are the specific classes, so + * sorting by node count puts the useful ones first. + */ +function labelList(labels: Set, counts: Record): string { + const sorted = [...labels].sort( + (a, b) => (counts[a] ?? Infinity) - (counts[b] ?? Infinity) || a.localeCompare(b) + ); + const shown = sorted.slice(0, LABELS_PER_SIDE).join("|"); + return sorted.length > LABELS_PER_SIDE ? `${shown}|… +${sorted.length - LABELS_PER_SIDE}` : shown; +} + +/** + * Render a GraphSchema as a compact markdown digest for a model to read before + * writing Cypher: labels and relationship types by count, which labels each + * type connects, property types, indexes and constraints. The full structure + * is the `reactome://graph/schema` resource. + * + * Sized for the tool response cap. Rendered naively, the Release 97 graph came + * to 87,000 characters -- more than twice the cap, so the cap would have cut + * it mid-way and dropped the indexes and constraints. The shape below came to + * 34,341 characters against the same graph, inside the default 40,000 cap: + * properties common to every label set are listed once, relationship types + * with the same properties share a line, and each type's endpoints are + * summarised as label sets rather than enumerated pair by pair. A much larger + * graph could still reach the cap, which announces its cut. + * + * Sections the database would not give up are named under "Unavailable" + * rather than left out, so a missing property list is not read as an empty + * one. + */ +export function formatGraphSchemaMarkdown(schema: GraphSchema): string { + const { stats, nodeTypeProperties, relTypeProperties, indexes, constraints, patterns } = schema; + + const labelEntries = Object.entries(stats.labels).sort(([, a], [, b]) => b - a); + const relEntries = Object.entries(stats.relTypesCount).sort(([, a], [, b]) => b - a); + + const lines: string[] = []; + lines.push(`## Reactome Graph Schema`); + const dbComp = schema.dbComponents[0]; + lines.push( + `**Neo4j:** ${dbComp?.versions?.[0] ?? "?"} ${dbComp?.edition ?? ""} · **Fetched:** ${schema.fetchedAt}` + ); + lines.push( + `**Totals:** ${stats.nodeCount.toLocaleString()} nodes · ${stats.relCount.toLocaleString()} relationships · ${labelEntries.length} labels · ${relEntries.length} relationship types` + ); + lines.push(""); + + if (schema.unavailable.length > 0) { + lines.push(`### Unavailable (${schema.unavailable.length})`); + lines.push("These sections could not be read; do not assume they are empty."); + for (const u of schema.unavailable) lines.push(`- \`${u.section}\` — ${u.reason}`); + lines.push(""); + } + + lines.push(`### Labels (${labelEntries.length}, by node count)`); + lines.push( + labelEntries.map(([label, count]) => `\`${label}\` ${count.toLocaleString()}`).join(" · ") + ); + lines.push(""); + + // Endpoints per relationship type. + const ends = new Map; end: Set }>(); + for (const p of patterns) { + const entry = ends.get(p.type) ?? { start: new Set(), end: new Set() }; + entry.start.add(p.start); + entry.end.add(p.end); + ends.set(p.type, entry); + } + + lines.push(`### Relationship types (${relEntries.length}, by relationship count)`); + if (ends.size > 0) { + lines.push("Each with the labels seen at its start and end, most specific first."); + } + const relTypes = new Set([...relEntries.map(([t]) => t), ...ends.keys()]); + for (const relType of relTypes) { + const count = stats.relTypesCount[relType]; + const side = ends.get(relType); + const shape = side + ? ` — (:${labelList(side.start, stats.labels)})-[:${relType}]->(:${labelList(side.end, stats.labels)})` + : ""; + lines.push(`- \`${relType}\` ${count === undefined ? "" : count.toLocaleString()}${shape}`); + } + lines.push(""); + + // Node properties by label set, with what every set shares listed once. + const bySet = new Map(); + for (const p of nodeTypeProperties) { + const key = p.nodeType; + const entry = bySet.get(key) ?? { labels: p.nodeLabels ?? [], props: [] }; + if (p.propertyName != null) entry.props.push(p); + bySet.set(key, entry); + } + if (bySet.size > 0) { + const sets = [...bySet.values()]; + const signature = (p: Prop) => propText(p); + const shared = (sets[0]?.props ?? []) + .map(signature) + .filter(sig => sets.every(s => s.props.some(p => signature(p) === sig))); + const sharedSet = new Set(shared); + const everywhereLabels = (sets[0]?.labels ?? []).filter(l => + sets.every(s => s.labels.includes(l)) + ); + + lines.push(`### Node properties (${bySet.size} label sets)`); + lines.push("`!` marks a property every node of that set has."); + if (shared.length > 0) lines.push(`**On every label set:** ${shared.join(", ")}`); + if (everywhereLabels.length > 0) { + lines.push(`**Labels on every node, omitted below:** ${everywhereLabels.join(", ")}`); + } + const rows = sets + .map(s => ({ + name: s.labels.filter(l => !everywhereLabels.includes(l)).join(":") || "(none)", + extra: s.props.map(signature).filter(sig => !sharedSet.has(sig)), + })) + .sort((a, b) => a.name.localeCompare(b.name)); + for (const row of rows) { + lines.push(`- **${row.name}**${row.extra.length ? `: ${row.extra.join(", ")}` : ""}`); + } + lines.push(""); + } + + // Relationship properties, grouped by identical property lists. + if (relTypeProperties.length > 0) { + const propsByRel = new Map(); + for (const p of relTypeProperties) { + const entry = propsByRel.get(p.relType) ?? []; + entry.push(propText(p)); + propsByRel.set(p.relType, entry); + } + const bySignature = new Map(); + for (const [rel, props] of propsByRel) { + const sig = [...props].sort().join(", "); + bySignature.set(sig, [...(bySignature.get(sig) ?? []), rel]); + } + lines.push(`### Relationship properties`); + for (const [sig, rels] of [...bySignature].sort(([a], [b]) => a.localeCompare(b))) { + lines.push(`- ${sig} — on ${rels.sort().join(", ")}`); + } + lines.push(""); + } + + if (indexes.length > 0) { + lines.push(`### Indexes (${indexes.length})`); + for (const ix of indexes) { + const labels = ix.labelsOrTypes?.join(",") ?? "?"; + const props = ix.properties?.join(",") ?? "?"; + lines.push(`- ${labels}(${props}) [${ix.type ?? "?"}, ${ix.state ?? "?"}]`); + } + lines.push(""); + } + + if (constraints.length > 0) { + lines.push(`### Constraints (${constraints.length})`); + for (const c of constraints) { + const what = + c.description ?? + `${c.type ?? "?"} on ${c.labelsOrTypes?.join(",") ?? "?"}(${c.properties?.join(",") ?? "?"})`; + lines.push(`- ${what}`); + } + lines.push(""); + } + + lines.push( + "_For the full structure as JSON, including every label pair each relationship type connects, read the `reactome://graph/schema` resource._" + ); + + return lines.join("\n"); +} diff --git a/src/graph/gate.ts b/src/graph/gate.ts new file mode 100644 index 0000000..8f21c8e --- /dev/null +++ b/src/graph/gate.ts @@ -0,0 +1,35 @@ +/** + * Whether this instance has graph database access at all. + * + * **Both** of these, or nothing: + * + * NEO4J_URI set to a non-blank connection string + * MCP_ALLOW_CYPHER exactly "1" + * + * This is the one predicate. Every surface that touches the graph asks it -- + * tool registration, the `reactome://graph/schema` resource, the server + * instructions, the startup schema warm-up, and `getDriver()` itself, which + * refuses to construct a driver when it is false. Asking it at the driver as + * well as at each surface is deliberate: the last gate this server had was + * consulted in four places, and the HTTP entrypoint -- the one actually + * hosted -- opened a connection on `NEO4J_URI` alone. A door that forgets to + * ask still cannot get a driver. + * + * Two variables rather than one, on purpose. A connection string can be set + * for many reasons and is easy to inherit from a shared environment file; + * it is not consent to publish arbitrary query access. Forgetting the opt-in + * costs a missing tool on an internal instance, which is visible and + * harmless. Forgetting it the other way round on a public instance would cost + * the database, and the website in front of it. + * + * Read from the environment on every call rather than captured at import, so + * there is one copy of the answer and it cannot go stale. + */ +export function isGraphAccessEnabled(env: NodeJS.ProcessEnv = process.env): boolean { + return Boolean(env.NEO4J_URI?.trim()) && env.MCP_ALLOW_CYPHER === "1"; +} + +/** Why a graph operation was refused, with what would change the answer. */ +export const GRAPH_ACCESS_OFF = + "Graph database access is off. It needs both NEO4J_URI and MCP_ALLOW_CYPHER=1, " + + "and must never be enabled on a publicly reachable instance."; diff --git a/src/graph/schema.ts b/src/graph/schema.ts new file mode 100644 index 0000000..890b6b6 --- /dev/null +++ b/src/graph/schema.ts @@ -0,0 +1,317 @@ +import { runRead } from "../clients/neo4j.js"; +import { logger } from "../logger.js"; + +/** + * The live graph schema, built from Neo4j's **built-in** procedures only. + * + * The Reactome graph database container runs Neo4j 4.4 Community with no + * APOC. The previous version of this file read `apoc.meta.stats()`, + * `apoc.meta.schema()` and `apoc.meta.nodeTypeProperties()`, and would have + * failed on its first query there. Everything below exists in 4.4 Community: + * + * dbms.components() version and edition + * db.labels(), db.relationshipTypes() + * MATCH (n:`L`) RETURN count(n) per-token counts, answered from the count + * store rather than by scanning + * db.schema.nodeTypeProperties() property names, types, mandatory flags + * db.schema.relTypeProperties() + * db.schema.visualization() which label connects to which, by type + * SHOW INDEXES / SHOW CONSTRAINTS, falling back to db.indexes() / + * db.constraints() where SHOW is not known + * + * Each section is fetched on its own and fails on its own. A section that + * cannot be read is listed in `unavailable` with the reason, rather than + * rendered empty -- an empty property list reads as "these labels have no + * properties", which is a confident wrong answer. + * + * Row shapes, recorded from the Reactome graph database (Neo4j 4.4.48 + * Community, Release 97), trimmed: + * db.schema.nodeTypeProperties {nodeType: ":`CandidateSet`:`DatabaseObject`:...", + * nodeLabels: ["CandidateSet", "DatabaseObject", ...], + * propertyName: "dbId", propertyTypes: ["Long"], + * mandatory: true} + * db.schema.relTypeProperties {relType: ":`author`", propertyName: "order", + * propertyTypes: ["Integer", "Long"], mandatory: true} + * db.schema.visualization {nodes: [{identity: -127, labels: ["TopLevelPathway"], + * properties: {name: "TopLevelPathway", ...}}], + * relationships: [{identity: -7911, start: -134, + * end: -179, type: "authored"}]} + * SHOW INDEXES {name, type: "BTREE", entityType: "NODE", + * labelsOrTypes: ["DatabaseObject"], + * properties: ["dbId"], state: "ONLINE"} + * SHOW CONSTRAINTS {name, type: "UNIQUENESS", entityType: "NODE", + * labelsOrTypes: ["DatabaseObject"], properties: ["dbId"]} + * + * On that graph the whole fetch took about 65 s (2.96M nodes, 11.5M + * relationships, 108 labels, 88 types, 5,669 label-level patterns), almost + * all of it `db.schema.nodeTypeProperties()`. Hence the warm-up, and the + * timeout below. + */ +export interface GraphSchema { + fetchedAt: string; + dbComponents: Array<{ name: string; versions: string[]; edition: string }>; + stats: { + nodeCount: number; + relCount: number; + labels: Record; + relTypesCount: Record; + }; + /** Label-level connections: (start)-[type]->(end). */ + patterns: Array<{ start: string; type: string; end: string }>; + nodeTypeProperties: Array<{ + nodeType: string; + nodeLabels: string[]; + propertyName: string; + propertyTypes: string[]; + mandatory: boolean; + }>; + relTypeProperties: Array<{ + relType: string; + propertyName: string; + propertyTypes: string[]; + mandatory: boolean; + }>; + indexes: Array<{ + name?: string; + type?: string; + entityType?: string; + labelsOrTypes?: string[] | null; + properties?: string[] | null; + state?: string; + }>; + constraints: Array<{ + name?: string; + type?: string; + entityType?: string; + labelsOrTypes?: string[] | null; + properties?: string[] | null; + description?: string; + }>; + /** Sections that could not be read, and why. Empty when complete. */ + unavailable: Array<{ section: string; reason: string }>; +} + +/** + * Schema queries get a longer budget than an ordinary Cypher query: + * `db.schema.nodeTypeProperties()` reads every node's property keys on 4.4, + * which on the Reactome graph is not quick. The warm-up at startup pays it + * once, so callers do not. + */ +const SCHEMA_FETCH_TIMEOUT_MS = 120_000; + +let schemaCache: GraphSchema | null = null; +let schemaPending: Promise | null = null; + +type Section = keyof Omit; + +class SectionUnavailable extends Error {} + +const message = (error: unknown) => (error instanceof Error ? error.message : String(error)); + +/** Run the first of `queries` that works; throw with every failure if none does. */ +async function firstThatWorks(queries: string[], params: Record = {}) { + const failures: string[] = []; + for (const query of queries) { + try { + return await runRead(query, params, { timeoutMs: SCHEMA_FETCH_TIMEOUT_MS }); + } catch (error) { + failures.push(message(error)); + } + } + throw new SectionUnavailable(failures.join(" | ")); +} + +/** Backticks, doubled, are how Cypher escapes a backtick inside a quoted name. */ +const quoteName = (name: string) => "`" + name.replace(/`/g, "``") + "`"; + +/** ":`input`" -> "input", as db.schema.relTypeProperties() reports it. */ +const bareRelType = (relType: string) => + relType + .replace(/^:/, "") + .replace(/^`(.*)`$/, "$1") + .replace(/``/g, "`"); + +/** + * One query that counts every token, answered from the count store. Names go + * in as parameters for the returned key and quoted for the pattern, so a label + * containing a backtick can neither break nor extend the query. + */ +async function countEach(tokens: string[], pattern: (quoted: string) => string) { + if (tokens.length === 0) return {}; + const params: Record = {}; + const parts = tokens.map((token, i) => { + params[`k${i}`] = token; + // The count sits alone in a subquery so the planner can answer it from + // the count store; with the key projected beside it, it might scan. + return `CALL { ${pattern(quoteName(token))} } RETURN $k${i} AS key, c AS count`; + }); + const rows = await firstThatWorks<{ key: string; count: number }>( + [parts.join(" UNION ALL ")], + params + ); + return Object.fromEntries(rows.map(r => [r.key, Number(r.count)])); +} + +async function fetchStats(): Promise { + const [labelRows, relRows, nodeTotal, relTotal] = await Promise.all([ + firstThatWorks<{ label: string }>(["CALL db.labels() YIELD label RETURN label"]), + firstThatWorks<{ relationshipType: string }>([ + "CALL db.relationshipTypes() YIELD relationshipType RETURN relationshipType", + ]), + firstThatWorks<{ count: number }>(["MATCH (n) RETURN count(n) AS count"]), + firstThatWorks<{ count: number }>(["MATCH ()-[r]->() RETURN count(r) AS count"]), + ]); + const [labels, relTypesCount] = await Promise.all([ + countEach( + labelRows.map(r => r.label), + q => `MATCH (n:${q}) RETURN count(n) AS c` + ), + countEach( + relRows.map(r => r.relationshipType), + q => `MATCH ()-[r:${q}]->() RETURN count(r) AS c` + ), + ]); + return { + nodeCount: Number(nodeTotal[0]?.count ?? 0), + relCount: Number(relTotal[0]?.count ?? 0), + labels, + relTypesCount, + }; +} + +interface VizNode { + identity: unknown; + labels?: string[]; + properties?: { name?: string }; +} +interface VizRel { + start: unknown; + end: unknown; + type: string; +} + +async function fetchPatterns(): Promise { + const rows = await firstThatWorks<{ nodes: VizNode[]; relationships: VizRel[] }>([ + "CALL db.schema.visualization() YIELD nodes, relationships RETURN nodes, relationships", + ]); + const row = rows[0]; + if (!row) return []; + const labelOf = new Map(); + for (const node of row.nodes ?? []) { + labelOf.set(String(node.identity), node.labels?.[0] ?? node.properties?.name ?? "?"); + } + return (row.relationships ?? []) + .map(rel => ({ + start: labelOf.get(String(rel.start)) ?? "?", + type: rel.type, + end: labelOf.get(String(rel.end)) ?? "?", + })) + .sort( + (a, b) => + a.type.localeCompare(b.type) || a.start.localeCompare(b.start) || a.end.localeCompare(b.end) + ); +} + +const SECTIONS: { [K in Section]: () => Promise } = { + dbComponents: () => + firstThatWorks([ + "CALL dbms.components() YIELD name, versions, edition RETURN name, versions, edition", + ]), + stats: fetchStats, + patterns: fetchPatterns, + nodeTypeProperties: () => + firstThatWorks([ + "CALL db.schema.nodeTypeProperties() YIELD nodeType, nodeLabels, propertyName, propertyTypes, mandatory RETURN nodeType, nodeLabels, propertyName, propertyTypes, mandatory", + ]), + relTypeProperties: async () => { + const rows = await firstThatWorks([ + "CALL db.schema.relTypeProperties() YIELD relType, propertyName, propertyTypes, mandatory RETURN relType, propertyName, propertyTypes, mandatory", + ]); + // A relationship type with no properties at all is reported once with a + // null propertyName; that is "no properties", not a property. + return rows + .filter(r => r.propertyName != null) + .map(r => ({ ...r, relType: bareRelType(r.relType) })); + }, + indexes: () => + firstThatWorks([ + "SHOW INDEXES YIELD name, type, entityType, labelsOrTypes, properties, state RETURN name, type, entityType, labelsOrTypes, properties, state", + "CALL db.indexes() YIELD name, type, entityType, labelsOrTypes, properties, state RETURN name, type, entityType, labelsOrTypes, properties, state", + ]), + constraints: () => + firstThatWorks([ + "SHOW CONSTRAINTS YIELD name, type, entityType, labelsOrTypes, properties RETURN name, type, entityType, labelsOrTypes, properties", + "CALL db.constraints() YIELD name, description RETURN name, description", + ]), +}; + +const EMPTY: Omit = { + dbComponents: [], + stats: { nodeCount: 0, relCount: 0, labels: {}, relTypesCount: {} }, + patterns: [], + nodeTypeProperties: [], + relTypeProperties: [], + indexes: [], + constraints: [], +}; + +/** + * Fetch the schema, once. Cached in memory after the first call that reads + * anything; concurrent first callers share one fetch. + * + * A fetch in which **no** section could be read -- the database is down, the + * credentials are wrong -- throws and is not cached, so the next call tries + * again instead of serving an empty schema for the life of the process. + */ +export async function fetchGraphSchema(): Promise { + if (schemaCache) return schemaCache; + if (schemaPending) return schemaPending; + + const start = Date.now(); + schemaPending = (async () => { + try { + const names = Object.keys(SECTIONS) as Section[]; + const settled = await Promise.allSettled(names.map(name => SECTIONS[name]())); + + const result: GraphSchema = { + fetchedAt: new Date().toISOString(), + ...structuredClone(EMPTY), + unavailable: [], + }; + settled.forEach((outcome, i) => { + const name = names[i] as Section; + if (outcome.status === "fulfilled") { + (result as unknown as Record)[name] = outcome.value; + } else { + result.unavailable.push({ section: name, reason: message(outcome.reason) }); + } + }); + + if (result.unavailable.length === names.length) { + throw new Error( + `Could not read the graph schema: ${result.unavailable[0]?.reason ?? "no section answered"}` + ); + } + + logger.info("graph schema fetched", { + durationMs: Date.now() - start, + nodeCount: result.stats.nodeCount, + labels: Object.keys(result.stats.labels).length, + unavailable: result.unavailable.map(u => u.section), + }); + + schemaCache = result; + return result; + } finally { + schemaPending = null; + } + })(); + + return schemaPending; +} + +/** For tests -- clears both the cached value and any in-flight fetch. */ +export function _resetGraphSchemaCache(): void { + schemaCache = null; + schemaPending = null; +} diff --git a/src/graph/warmup.ts b/src/graph/warmup.ts new file mode 100644 index 0000000..2439a27 --- /dev/null +++ b/src/graph/warmup.ts @@ -0,0 +1,44 @@ +import { logger } from "../logger.js"; +import { isGraphAccessEnabled } from "./gate.js"; +import { fetchGraphSchema } from "./schema.js"; +import { GRAPH_GROUP, resolveToolGroups } from "../tools/index.js"; + +/** + * Called by every entrypoint once it is serving. Never rejects. + * + * Warms the graph schema cache in the background when -- and only when -- + * this instance offers graph access, so the first `reactome_cypher_schema` + * call does not pay for the schema procedures. With the gate closed, or the + * `cypher` group withheld, nothing could read that cache, and warming it would + * open a database connection for nobody. + * + * The decision lives here rather than at each call site. The previous gate + * was asked by each entrypoint for itself, and the HTTP entrypoint asked the + * wrong question. + */ +export function warmGraphSchema(): void { + let offered: boolean; + try { + offered = resolveToolGroups().includes(GRAPH_GROUP); + } catch { + // A bad MCP_TOOL_GROUPS is reported where the server is built. + return; + } + + if (!offered) { + if (process.env.NEO4J_URI?.trim() && !isGraphAccessEnabled()) { + // Said out loud: an operator who set NEO4J_URI expecting the Cypher + // tools needs to know why they are absent. + logger.warn("graph database access is OFF: MCP_ALLOW_CYPHER is not 1", { + hint: "Set MCP_ALLOW_CYPHER=1 only on an instance that is not publicly reachable.", + }); + } + return; + } + + fetchGraphSchema().catch((error: unknown) => { + logger.warn("graph schema warm-up failed; will retry on first use", { + error: error instanceof Error ? error.message : String(error), + }); + }); +} diff --git a/src/http-server.ts b/src/http-server.ts index ab2a963..cef71b9 100644 --- a/src/http-server.ts +++ b/src/http-server.ts @@ -12,6 +12,7 @@ import { startHttpServer } from "./http.js"; import { logger } from "./logger.js"; import { MCP_HTTP_PORT, MCP_HTTP_HOST } from "./config.js"; +import { warmGraphSchema } from "./graph/warmup.js"; const port = MCP_HTTP_PORT; @@ -26,8 +27,13 @@ if (!port) { // validates the tool-group configuration synchronously, so a bad value threw // past the .catch below and killed the process with a raw stack trace. Dying // was correct; dying without the log line that says why was not. +// +// The graph schema warm-up decides for itself whether graph access is on -- +// this entrypoint once decided by NEO4J_URI alone, and opened a connection on +// an instance that had not opted in. Promise.resolve() .then(() => startHttpServer(port, MCP_HTTP_HOST)) + .then(() => warmGraphSchema()) .catch((error: unknown) => { logger.error("fatal error starting http server", { error: error instanceof Error ? error.message : String(error), diff --git a/src/http.ts b/src/http.ts index 224a68c..788495f 100644 --- a/src/http.ts +++ b/src/http.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { resolveToolGroups } from "./tools/index.js"; +import { GRAPH_GROUP, resolveToolGroups } from "./tools/index.js"; import { estimateAnalysisBodyBytes } from "./tools/limits.js"; import type { Server } from "node:http"; import type { Request, Response } from "express"; @@ -55,7 +55,11 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro // documentation said the server refuses to start. It does over stdio, // where createServer runs once at boot. It did not here, which is the // transport that is actually deployed. - resolveToolGroups(); + // + // Also what /health reports as `graphAccess`: whether sessions on this + // instance are offered the Cypher tools, so an operator can see which way + // the gate went without reading the environment. + const graphAccess = resolveToolGroups().includes(GRAPH_GROUP); // 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 @@ -229,6 +233,7 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro sessions: sessions.size, contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, + graphAccess, }); }); @@ -239,6 +244,7 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro port, contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, + graphAccess, }); resolve(http); }); diff --git a/src/index.ts b/src/index.ts index 60cbf43..c0fea30 100644 --- a/src/index.ts +++ b/src/index.ts @@ -4,6 +4,8 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { createServer } from "./server.js"; import { logger } from "./logger.js"; import { CONTENT_SERVICE_URL, ANALYSIS_SERVICE_URL } from "./config.js"; +import { GRAPH_GROUP, resolveToolGroups } from "./tools/index.js"; +import { warmGraphSchema } from "./graph/warmup.js"; async function main() { // Built here rather than at module scope, so importing this file does not @@ -14,7 +16,10 @@ async function main() { logger.info("reactome mcp server started", { contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, + // Whether this instance offers the Cypher tools -- the gate, and the group. + graphAccess: resolveToolGroups().includes(GRAPH_GROUP), }); + warmGraphSchema(); } main().catch(error => { diff --git a/src/instructions.ts b/src/instructions.ts index ddae761..901b5cb 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -1,4 +1,4 @@ -import { resolveToolGroups, type ToolGroup } from "./tools/index.js"; +import { GRAPH_GROUP, permittedGroups, resolveToolGroups, type ToolGroup } from "./tools/index.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. @@ -69,6 +69,10 @@ const RESOURCE_LINES: { group: ToolGroup; text: string }[] = [ { group: "pathway", text: "`reactome://pathway/{id}` — templated pathway details." }, { group: "entity", text: "`reactome://entity/{id}` — templated entity details." }, { group: "analysis", text: "`reactome://analysis/{token}` — templated analysis results." }, + { + group: "cypher", + text: "`reactome://graph/schema` — the graph database schema as JSON (labels, relationship types, properties, indexes, constraints).", + }, ]; const CATEGORY_LINES: Record = { @@ -79,9 +83,26 @@ const CATEGORY_LINES: Record = { interactors: `- **Interactors** (\`reactome_psicquic_*\`, \`reactome_static_interactors\`, \`reactome_interactor_pathways\`) — protein–protein interaction data.`, export: `- **Export** (\`reactome_export_*\`) — diagrams (PNG/SVG), SBGN, SBML, PDF reports, CSV/JSON analysis exports.`, utilities: `- **Utilities** (\`reactome_species\`, \`reactome_diseases\`, \`reactome_database_info\`, \`reactome_mapping_*\`, \`reactome_orthology\`, \`reactome_query\`).`, + cypher: `- **Graph database** (\`reactome_cypher_schema\`, \`reactome_cypher_sample\`, \`reactome_cypher_query\`) — read-only Cypher against the Reactome Neo4j graph. See the section below before using it.`, gsa: `- **Gene set analysis** (\`reactome_gsa_*\`) — ReactomeGSA: available methods, data types, and searchable public expression datasets. Distinct from Analysis, which is over-representation on a list of identifiers.`, }; +const CYPHER_INSTRUCTIONS = ` +# Graph database (Cypher) + +This instance can query the Reactome graph database directly. Use it for what the REST tools do not expose — arbitrary traversals, joins across relationship types, aggregate counts across labels. + +1. Call \`reactome_cypher_schema\` (or read \`reactome://graph/schema\`) **before writing any query**. It lists labels and relationship types with counts, which labels each relationship type connects, property types with mandatory flags, indexes and constraints. Never guess the schema. +2. Use \`reactome_cypher_sample\` on a label to see a representative node. +3. Run the query with \`reactome_cypher_query\`: + - It runs in a READ-mode session; write clauses are refused by the database. + - \`LOAD CSV\` and APOC procedures that write or fetch from outside the graph are refused before execution. + - A server-side timeout ends long queries. Always include a \`LIMIT\`, and anchor on an indexed property (for example \`stId\` or \`dbId\`) rather than scanning a whole label. + - Return specific properties (\`RETURN p.stId, p.displayName\`) rather than whole nodes; rows and response size are capped. + +**When not to use it:** lookups by ID or name, pathway hierarchies and enrichment — the REST tools are cached, cheaper, and format the answer for you. +`.trim(); + /** * The instructions a client reads on connecting. * @@ -95,7 +116,10 @@ const CATEGORY_LINES: Record = { * registration, and `tests/tool-groups.test.ts` asserts a restricted * instance does not mention what it withheld. */ -export function buildServerInstructions(groups: ToolGroup[] = resolveToolGroups()): string { +export function buildServerInstructions(requested: ToolGroup[] = resolveToolGroups()): string { + // The same filter the registrars apply, so the text cannot describe graph + // access the gate withheld -- even for a caller that passes its own list. + const groups = permittedGroups(requested); const categoryLines = groups.map(group => CATEGORY_LINES[group]).join("\n"); const steps = WORKFLOW_STEPS.filter( step => step.group === null || groups.includes(step.group) @@ -105,7 +129,9 @@ export function buildServerInstructions(groups: ToolGroup[] = resolveToolGroups( line => `- ${line.text}` ); - return CORE_INSTRUCTIONS.replace("__CATEGORY_LINES__", categoryLines) + const core = CORE_INSTRUCTIONS.replace("__CATEGORY_LINES__", categoryLines) .replace("__WORKFLOW_STEPS__", steps.join("\n")) .replace("__RESOURCE_LINES__", resourceLines.join("\n")); + + return groups.includes(GRAPH_GROUP) ? `${core}\n\n${CYPHER_INSTRUCTIONS}` : core; } diff --git a/src/resources/graph.ts b/src/resources/graph.ts new file mode 100644 index 0000000..69dc3a4 --- /dev/null +++ b/src/resources/graph.ts @@ -0,0 +1,29 @@ +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { isGraphAccessEnabled } from "../graph/gate.js"; +import { fetchGraphSchema } from "../graph/schema.js"; + +/** + * `reactome://graph/schema` -- the graph model as JSON. + * + * Not a query surface, but it runs schema procedures against the database on + * the caller's behalf and publishes the internal graph model, so it sits + * behind the same gate as the Cypher tools. Checked here as well as through + * the `cypher` group in registerAllResources, so calling this registrar + * directly cannot publish it either. + */ +export function registerGraphResources(server: McpServer) { + if (!isGraphAccessEnabled()) return; + + server.resource("reactome://graph/schema", "reactome://graph/schema", async () => { + const schema = await fetchGraphSchema(); + return { + contents: [ + { + uri: "reactome://graph/schema", + mimeType: "application/json", + text: JSON.stringify(schema, null, 2), + }, + ], + }; + }); +} diff --git a/src/resources/index.ts b/src/resources/index.ts index c393b83..251249b 100644 --- a/src/resources/index.ts +++ b/src/resources/index.ts @@ -1,7 +1,8 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { registerStaticResources } from "./static.js"; import { registerResourceTemplates } from "./templates.js"; -import { resolveToolGroups, type ToolGroup } from "../tools/index.js"; +import { registerGraphResources } from "./graph.js"; +import { permittedGroups, resolveToolGroups, type ToolGroup } from "../tools/index.js"; /** * Which group each resource belongs to. @@ -27,6 +28,7 @@ const RESOURCE_GROUPS: Record = { "reactome://analysis/{token}": "analysis", "reactome://top-pathways/{species}": "pathway", "reactome://events-hierarchy/{species}": "pathway", + "reactome://graph/schema": "cypher", }; export const RESOURCE_URIS = Object.keys(RESOURCE_GROUPS); @@ -41,6 +43,8 @@ function uriOf(args: unknown[]): string | undefined { } export function registerAllResources(server: McpServer, groups: ToolGroup[] = resolveToolGroups()) { + // The graph schema needs the graph access gate as well as its group. + const allowed = permittedGroups(groups); const original = server.resource.bind(server); (server as unknown as { resource: (...a: unknown[]) => unknown }).resource = ( ...args: unknown[] @@ -53,10 +57,11 @@ export function registerAllResources(server: McpServer, groups: ToolGroup[] = re `Add it to RESOURCE_GROUPS in src/resources/index.ts so MCP_TOOL_GROUPS can govern it.` ); } - if (!groups.includes(group)) return undefined; + if (!allowed.includes(group)) return undefined; return (original as (...a: unknown[]) => unknown)(...args); }; registerStaticResources(server); registerResourceTemplates(server); + registerGraphResources(server); } diff --git a/src/tools/cypher-guard.ts b/src/tools/cypher-guard.ts new file mode 100644 index 0000000..84fd45c --- /dev/null +++ b/src/tools/cypher-guard.ts @@ -0,0 +1,74 @@ +// Guard against queries that get around a READ-mode session. +// +// Neo4j's READ access mode rejects native write clauses (CREATE/MERGE/DELETE/ +// SET/REMOVE). It does not reject everything that reaches outside the graph: +// +// - APOC procedures open their own transactions and can write, load remote +// URLs or touch the filesystem regardless of the calling session's mode. +// The Reactome graph database container ships without APOC, but a +// database this server is pointed at might not, so they are refused here. +// - LOAD CSV is a *read* clause, so READ mode allows it -- and it fetches +// arbitrary URLs and, depending on server config, local files. +// +// This is a guardrail, not a security boundary. A determined caller can hide +// intent in ways a pattern cannot see, and the real boundary is that graph +// access is never enabled on a publicly reachable instance (see +// src/graph/gate.ts), plus Neo4j's own RBAC and plugin configuration. + +const WRITE_THROUGH_PATTERNS: Array<{ pattern: RegExp; label: string }> = [ + { + pattern: /\bapoc\.cypher\.(runWrite|doIt)\b/i, + label: "apoc.cypher.runWrite / apoc.cypher.doIt", + }, + { + pattern: /\bapoc\.periodic\.(iterate|commit|submit|countdown|repeat)\b/i, + label: "apoc.periodic.*", + }, + { + pattern: /\bapoc\.(create|merge|refactor)\.[a-z]\w*/i, + label: "apoc.create.* / apoc.merge.* / apoc.refactor.*", + }, + { pattern: /\bapoc\.nodes\.delete\b/i, label: "apoc.nodes.delete" }, + { + pattern: /\bapoc\.(load|import|export)\.[a-z]\w*/i, + label: "apoc.load.* / apoc.import.* / apoc.export.*", + }, + { pattern: /\bapoc\.trigger\.[a-z]\w*/i, label: "apoc.trigger.*" }, + { pattern: /\bload\s+csv\b/i, label: "LOAD CSV" }, +]; + +/** + * The forms of the query to test. All of them, not one: each normalisation + * closes a way of hiding a call and could open another. + * + * - The raw text. Comments used to be stripped first, but the stripper knows + * nothing of string literals, so a `'//'` inside a string swallowed the rest + * of the line -- call included. Testing the raw text as well means a + * commented-out call is refused too, which is a harmless false positive. + * - Comments removed, so `apoc.load/* x *\/.json` cannot split a name. + * - Backticks removed and whitespace around dots closed up, so + * `` `apoc`.`load`.`json` `` and `apoc . load . json` read as what they are. + */ +function candidates(query: string): string[] { + const noComments = query.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/[^\n]*/g, ""); + const canonical = (text: string) => text.replace(/`/g, "").replace(/\s*\.\s*/g, "."); + return [query, noComments, canonical(query), canonical(noComments)]; +} + +export class WriteThroughRejected extends Error { + constructor(public readonly matchedLabel: string) { + super( + `Query rejected: ${matchedLabel} can bypass READ-mode sessions and write to, or load data from, outside the graph. This server blocks it. If you have a legitimate need, run the query outside this server against a read replica.` + ); + this.name = "WriteThroughRejected"; + } +} + +export function rejectWriteThroughCalls(query: string): void { + const forms = candidates(query); + for (const { pattern, label } of WRITE_THROUGH_PATTERNS) { + if (forms.some(form => pattern.test(form))) { + throw new WriteThroughRejected(label); + } + } +} diff --git a/src/tools/cypher.ts b/src/tools/cypher.ts new file mode 100644 index 0000000..6cc0ca1 --- /dev/null +++ b/src/tools/cypher.ts @@ -0,0 +1,212 @@ +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; +import { runRead } from "../clients/neo4j.js"; +import { fetchGraphSchema } from "../graph/schema.js"; +import { formatGraphSchemaMarkdown } from "../graph/format-schema.js"; +import { logger } from "../logger.js"; +import { rejectWriteThroughCalls } from "./cypher-guard.js"; +import { isGraphAccessEnabled } from "../graph/gate.js"; + +const MAX_QUERY_CHARS = 50_000; +const MAX_ROWS_DEFAULT = 100; +const MAX_ROWS_CAP = 1000; +const MAX_ROW_CHARS_DEFAULT = 2000; +const MAX_ROW_CHARS_CAP = 20000; +const MAX_TOTAL_CHARS_DEFAULT = 40000; +const MAX_TOTAL_CHARS_CAP = 200000; + +interface RowSummary { + __truncated: true; + reason: "row_too_large"; + keys: string[]; + original_chars: number; +} + +function summarizeRow(row: Record, json: string): RowSummary { + return { + __truncated: true, + reason: "row_too_large", + keys: Object.keys(row), + original_chars: json.length, + }; +} + +export interface CapRowsStats { + rowCountTruncated: boolean; + rowsWidthTruncated: number; + stoppedAtTotal: boolean; +} + +export function capCypherRows( + rows: Record[], + maxRows: number, + maxRowChars: number, + maxTotalChars: number +): { output: unknown[]; stats: CapRowsStats } { + const rowCountTruncated = rows.length > maxRows; + const limitedRows = rowCountTruncated ? rows.slice(0, maxRows) : rows; + + const output: unknown[] = []; + let totalChars = 0; + let rowsWidthTruncated = 0; + let stoppedAtTotal = false; + + for (const row of limitedRows) { + const raw = JSON.stringify(row); + const entry: unknown = raw.length > maxRowChars ? summarizeRow(row, raw) : row; + const entryChars = JSON.stringify(entry).length; + if (totalChars + entryChars > maxTotalChars) { + stoppedAtTotal = true; + break; + } + if (entry !== row) rowsWidthTruncated++; + output.push(entry); + totalChars += entryChars; + } + + return { + output, + stats: { rowCountTruncated, rowsWidthTruncated, stoppedAtTotal }, + }; +} + +/** + * The `cypher` tool group. + * + * Registers nothing unless the graph access gate is open -- checked here as + * well as by the caller, so a path that reaches this registrar without going + * through `resolveToolGroups()` still cannot publish these tools. + */ +export function registerCypherTools(server: McpServer) { + if (!isGraphAccessEnabled()) return; + + server.tool( + "reactome_cypher_query", + "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. LOAD CSV and APOC procedures that can write or fetch from outside the graph are rejected before execution. A server-side timeout terminates long queries. Row count, per-row size and total response size are capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", + { + query: z + .string() + .min(1) + .max(MAX_QUERY_CHARS) + .describe(`Cypher query to execute (read-only; max ${MAX_QUERY_CHARS} chars)`), + params: z + .record(z.string(), z.unknown()) + .optional() + .describe("Optional parameter map passed to the query"), + max_rows: z + .number() + .int() + .positive() + .max(MAX_ROWS_CAP) + .optional() + .default(MAX_ROWS_DEFAULT) + .describe(`Maximum rows to return (default ${MAX_ROWS_DEFAULT}, cap ${MAX_ROWS_CAP})`), + max_row_chars: z + .number() + .int() + .positive() + .max(MAX_ROW_CHARS_CAP) + .optional() + .default(MAX_ROW_CHARS_DEFAULT) + .describe( + `Maximum JSON chars per row before replacing with a summary (default ${MAX_ROW_CHARS_DEFAULT}, cap ${MAX_ROW_CHARS_CAP})` + ), + max_total_chars: z + .number() + .int() + .positive() + .max(MAX_TOTAL_CHARS_CAP) + .optional() + .default(MAX_TOTAL_CHARS_DEFAULT) + .describe( + `Maximum total JSON chars across all rows before truncation (default ${MAX_TOTAL_CHARS_DEFAULT}, cap ${MAX_TOTAL_CHARS_CAP})` + ), + }, + async ({ query, params, max_rows, max_row_chars, max_total_chars }) => { + rejectWriteThroughCalls(query); + logger.info("cypher_query", { chars: query.length, max_rows }); + const rows = await runRead>(query, params ?? {}); + const { output, stats } = capCypherRows(rows, max_rows, max_row_chars, max_total_chars); + + const notes: string[] = []; + notes.push(`**Rows returned:** ${output.length} of ${rows.length}`); + if (stats.rowCountTruncated) notes.push(`(row-count cap: ${max_rows})`); + if (stats.rowsWidthTruncated > 0) + notes.push(`**Wide rows summarized:** ${stats.rowsWidthTruncated}`); + if (stats.stoppedAtTotal) + notes.push(`**Total size cap reached (${max_total_chars} chars); later rows omitted.**`); + + const body = [ + `## Cypher Result`, + ...notes, + "", + "```json", + JSON.stringify(output, null, 2), + "```", + ]; + + return { + content: [{ type: "text", text: body.join("\n") }], + }; + } + ); + + server.tool( + "reactome_cypher_schema", + "Introspect the Reactome graph schema: labels and relationship types with counts, which labels each relationship type connects, property names and types with mandatory flags, indexes and constraints. Read live from Neo4j's built-in schema procedures and cached for the life of the server (the server warms it at startup; a cold first call can take tens of seconds). Call this before writing Cypher. Sections the database could not provide are listed as unavailable rather than shown empty. For the full JSON, read the `reactome://graph/schema` resource.", + {}, + async () => { + logger.info("cypher_schema"); + const schema = await fetchGraphSchema(); + return { + content: [{ type: "text", text: formatGraphSchemaMarkdown(schema) }], + }; + } + ); + + server.tool( + "reactome_cypher_sample", + "Return a small sample of nodes for a given label, to inspect shape and typical property values.", + { + label: z + .string() + .min(1) + .max(200) + .describe("Node label to sample (e.g. 'Pathway', 'ReactionLikeEvent', 'PhysicalEntity')"), + limit: z + .number() + .int() + .positive() + .max(50) + .optional() + .default(5) + .describe("Number of nodes to return (default 5, max 50)"), + }, + async ({ label, limit }) => { + if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(label)) { + throw new Error(`Invalid label: ${label}`); + } + logger.info("cypher_sample", { label, limit }); + const rows = await runRead<{ n: Record }>( + `MATCH (n:\`${label}\`) RETURN n LIMIT $limit`, + { limit } + ); + + const lines = [ + `## Sample of \`${label}\` (${rows.length})`, + "", + "```json", + JSON.stringify( + rows.map(r => r.n), + null, + 2 + ), + "```", + ]; + + return { + content: [{ type: "text", text: lines.join("\n") }], + }; + } + ); +} diff --git a/src/tools/index.ts b/src/tools/index.ts index d011fee..7bfca28 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -12,6 +12,8 @@ import { registerEntityTools } from "./entity.js"; import { registerExportTools } from "./export.js"; import { registerInteractorTools } from "./interactors.js"; import { registerGsaTools } from "./gsa.js"; +import { registerCypherTools } from "./cypher.js"; +import { isGraphAccessEnabled } from "../graph/gate.js"; import { withNewRequestContext } from "../context.js"; import { capToolResult } from "../response-limits.js"; import { MAX_TOOL_RESPONSE_CHARS } from "../config.js"; @@ -70,12 +72,29 @@ export const TOOL_GROUPS = { interactors: registerInteractorTools, gsa: registerGsaTools, utilities: registerUtilityTools, + // Graph database access. Never registered unless the gate in + // src/graph/gate.ts is open -- see permittedGroups(). + cypher: registerCypherTools, } as const satisfies Record void>; export type ToolGroup = keyof typeof TOOL_GROUPS; export const ALL_TOOL_GROUPS = Object.keys(TOOL_GROUPS) as ToolGroup[]; +/** The one group that additionally needs the graph access gate open. */ +export const GRAPH_GROUP: ToolGroup = "cypher"; + +/** + * `groups`, less the graph group unless the graph access gate is open. + * + * Applied by every registrar and by the instructions -- not only by + * `resolveToolGroups()` -- so a caller that builds its own group list still + * cannot get past the gate. + */ +export function permittedGroups(groups: readonly ToolGroup[]): ToolGroup[] { + return isGraphAccessEnabled() ? [...groups] : groups.filter(g => g !== GRAPH_GROUP); +} + /** * Which groups of tools this instance registers, from `MCP_TOOL_GROUPS`. * @@ -106,7 +125,9 @@ export const ALL_TOOL_GROUPS = Object.keys(TOOL_GROUPS) as ToolGroup[]; export function resolveToolGroups( raw: string | undefined = process.env.MCP_TOOL_GROUPS ): ToolGroup[] { - if (raw === undefined) return ALL_TOOL_GROUPS; + // "Everything" means everything this instance is permitted: the graph + // group needs NEO4J_URI and MCP_ALLOW_CYPHER=1 as well. + if (raw === undefined) return permittedGroups(ALL_TOOL_GROUPS); const requested = raw .split(",") @@ -128,6 +149,17 @@ export function resolveToolGroups( ); } + // Asking for graph access and not getting it should be loud. The opposite + // -- quietly dropping the group -- would leave an operator wondering why + // the tools are missing; quietly *adding* it is what the gate prevents. + if (requested.includes(GRAPH_GROUP) && !isGraphAccessEnabled()) { + throw new Error( + `MCP_TOOL_GROUPS names "${GRAPH_GROUP}", but graph database access is off. ` + + `It needs both NEO4J_URI and MCP_ALLOW_CYPHER=1, and must never be enabled ` + + `on a publicly reachable instance.` + ); + } + // Deduplicated, and in the map's order rather than the caller's, so the // registration order does not depend on how the variable was written. return ALL_TOOL_GROUPS.filter(name => requested.includes(name)); @@ -136,17 +168,14 @@ export function resolveToolGroups( export function registerAllTools(server: McpServer, groups: ToolGroup[] = resolveToolGroups()) { installToolWrapper(server); - // No graph database tools. They were removed on 2026-09-21 when this - // server became publicly hosted: Constitution Principle IV already said no - // deployment holds a Neo4j connection, and a gate enforcing that is a gate - // somebody can flip. Nothing here opens one now. - - for (const group of groups) TOOL_GROUPS[group](server); + const registering = permittedGroups(groups); + for (const group of registering) TOOL_GROUPS[group](server); - if (groups.length < ALL_TOOL_GROUPS.length) { - const omitted = ALL_TOOL_GROUPS.filter(g => !groups.includes(g)); + const available = permittedGroups(ALL_TOOL_GROUPS); + if (registering.length < available.length) { + const omitted = available.filter(g => !registering.includes(g)); logger.info("registering a subset of tool groups", { - registered: groups, + registered: registering, omitted, source: "MCP_TOOL_GROUPS", }); diff --git a/tests/cypher-guard.test.ts b/tests/cypher-guard.test.ts new file mode 100644 index 0000000..98d1278 --- /dev/null +++ b/tests/cypher-guard.test.ts @@ -0,0 +1,128 @@ +import { describe, it, expect } from "vitest"; +import { rejectWriteThroughCalls, WriteThroughRejected } from "../src/tools/cypher-guard.js"; + +describe("rejectWriteThroughCalls", () => { + it("accepts a plain read query", () => { + expect(() => rejectWriteThroughCalls("MATCH (n:Pathway) RETURN n LIMIT 10")).not.toThrow(); + }); + + it("accepts queries that mention apoc in a string literal if no procedure call", () => { + // This is a query that reads only — "apoc" appears inside a string and + // the regex uses word-boundary procedure shape, so it won't match. + expect(() => + rejectWriteThroughCalls(`MATCH (n) WHERE n.note = 'see apoc docs' RETURN n`) + ).not.toThrow(); + }); + + it("rejects apoc.cypher.runWrite", () => { + expect(() => + rejectWriteThroughCalls("CALL apoc.cypher.runWrite('CREATE (n) RETURN n', {})") + ).toThrow(WriteThroughRejected); + }); + + it("rejects apoc.cypher.doIt regardless of case", () => { + expect(() => rejectWriteThroughCalls("CALL APOC.CYPHER.DOIT('CREATE (n)', {})")).toThrow( + WriteThroughRejected + ); + }); + + it("rejects apoc.periodic.iterate", () => { + expect(() => + rejectWriteThroughCalls( + "CALL apoc.periodic.iterate('MATCH (n) RETURN n', 'SET n.flag = 1', {})" + ) + ).toThrow(WriteThroughRejected); + }); + + it("rejects apoc.create.node", () => { + expect(() => + rejectWriteThroughCalls("CALL apoc.create.node(['X'], {}) YIELD node RETURN node") + ).toThrow(WriteThroughRejected); + }); + + it("rejects apoc.refactor.mergeNodes", () => { + expect(() => rejectWriteThroughCalls("CALL apoc.refactor.mergeNodes([n1, n2])")).toThrow( + WriteThroughRejected + ); + }); + + it("rejects apoc.load.json (SSRF risk)", () => { + expect(() => + rejectWriteThroughCalls( + "CALL apoc.load.json('http://evil.example/data') YIELD value RETURN value" + ) + ).toThrow(WriteThroughRejected); + }); + + it("rejects apoc.export.csv.all", () => { + expect(() => rejectWriteThroughCalls("CALL apoc.export.csv.all('/tmp/x.csv', {})")).toThrow( + WriteThroughRejected + ); + }); + + it("rejects apoc.nodes.delete", () => { + expect(() => rejectWriteThroughCalls("CALL apoc.nodes.delete([1,2,3], 100)")).toThrow( + WriteThroughRejected + ); + }); + + it("rejects a bad call even inside a comment", () => { + // Comments used to be stripped before matching, and the stripper does not + // know about string literals -- so a '//' inside a string swallowed the + // rest of the line, call included (see the next test). Refusing a + // commented-out write is a harmless false positive; the bypass was not. + expect(() => + rejectWriteThroughCalls("// CALL apoc.cypher.runWrite('...')\nMATCH (n) RETURN n") + ).toThrow(WriteThroughRejected); + }); + + it("is not bypassed by a '//' inside a string literal", () => { + expect(() => + rejectWriteThroughCalls( + "WITH 'http://x' AS u CALL apoc.load.json('http://evil.example/') YIELD value RETURN value" + ) + ).toThrow(WriteThroughRejected); + }); + + it("is not bypassed by a block comment splitting the procedure name", () => { + expect(() => + rejectWriteThroughCalls("CALL apoc.load/* x */.json('http://evil.example/') YIELD value") + ).toThrow(WriteThroughRejected); + }); + + it("is not bypassed by backtick-quoting the procedure name", () => { + expect(() => + rejectWriteThroughCalls("CALL `apoc`.`load`.`json`('http://evil.example/') YIELD value") + ).toThrow(WriteThroughRejected); + }); + + it("rejects LOAD CSV, which READ mode allows and which fetches URLs and files", () => { + expect(() => + rejectWriteThroughCalls("LOAD CSV FROM 'http://evil.example/x.csv' AS row RETURN row") + ).toThrow(WriteThroughRejected); + expect(() => + rejectWriteThroughCalls("load csv with headers from 'file:///etc/passwd' as r return r") + ).toThrow(WriteThroughRejected); + }); + + it("does not mistake a property called csv for LOAD CSV", () => { + expect(() => rejectWriteThroughCalls("MATCH (n) RETURN n.csv, n.load LIMIT 1")).not.toThrow(); + }); + + it("does not strip comments when the bad call is in active code", () => { + expect(() => + rejectWriteThroughCalls("// harmless comment\nCALL apoc.periodic.commit('DELETE n', {})") + ).toThrow(WriteThroughRejected); + }); + + it("accepts read-only apoc procedures (apoc.meta.schema, apoc.path.*)", () => { + expect(() => + rejectWriteThroughCalls("CALL apoc.meta.schema() YIELD value RETURN value") + ).not.toThrow(); + expect(() => + rejectWriteThroughCalls( + "MATCH (n) CALL apoc.path.subgraphAll(n, {}) YIELD nodes RETURN nodes" + ) + ).not.toThrow(); + }); +}); diff --git a/tests/cypher-tools.test.ts b/tests/cypher-tools.test.ts new file mode 100644 index 0000000..fbb42eb --- /dev/null +++ b/tests/cypher-tools.test.ts @@ -0,0 +1,110 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import type { FakeNeo4jState } from "./helpers/fake-neo4j.js"; + +/** + * The Cypher tools, driven through the registration path with a fake driver. + * + * What these hold is the read-only contract: every query runs in a READ + * session under a server-side timeout, the write-through guard refuses before + * a session is even opened, and the response caps apply. + */ + +const fake = vi.hoisted((): FakeNeo4jState => ({ drivers: [], sessions: [], respond: () => [] })); + +vi.mock("neo4j-driver", async () => { + const { fakeNeo4jModule } = await import("./helpers/fake-neo4j.js"); + return fakeNeo4jModule(fake); +}); + +import { createFakeServer, textOf } from "./helpers/fake-server.js"; +import { registerCypherTools } from "../src/tools/cypher.js"; +import { closeDriver } from "../src/clients/neo4j.js"; +import { CYPHER_QUERY_TIMEOUT_MS } from "../src/config.js"; + +describe("reactome_cypher_* tools", () => { + let fakeServer: ReturnType; + + beforeEach(() => { + process.env.NEO4J_URI = "bolt://127.0.0.1:7687"; + process.env.MCP_ALLOW_CYPHER = "1"; + fake.drivers.length = 0; + fake.sessions.length = 0; + fake.respond = () => []; + fakeServer = createFakeServer(); + registerCypherTools(fakeServer.server); + }); + + afterEach(async () => { + await closeDriver(); + delete process.env.NEO4J_URI; + delete process.env.MCP_ALLOW_CYPHER; + }); + + it("registers the three tools when the gate is open", () => { + expect(fakeServer.toolNames().sort()).toEqual([ + "reactome_cypher_query", + "reactome_cypher_sample", + "reactome_cypher_schema", + ]); + }); + + it("runs a query in a READ session, under the configured timeout", async () => { + fake.respond = () => [{ stId: "R-HSA-109582" }]; + const result = await fakeServer.invoke("reactome_cypher_query", { + query: "MATCH (p:Pathway) RETURN p.stId AS stId LIMIT 1", + }); + + expect(textOf(result)).toContain("R-HSA-109582"); + expect(fake.sessions).toHaveLength(1); + expect(fake.sessions[0]?.config.defaultAccessMode).toBe("READ"); + expect(fake.sessions[0]?.runs[0]?.config?.timeout).toBe(CYPHER_QUERY_TIMEOUT_MS); + expect(fake.sessions[0]?.closed).toBe(true); + }); + + it("refuses a write-through procedure before opening a session", async () => { + await expect( + fakeServer.invoke("reactome_cypher_query", { + query: "CALL apoc.periodic.iterate('MATCH (n) RETURN n', 'DETACH DELETE n', {})", + }) + ).rejects.toThrow(/bypass READ-mode/); + expect(fake.drivers).toHaveLength(0); + expect(fake.sessions).toHaveLength(0); + }); + + it("caps the rows it returns", async () => { + fake.respond = () => Array.from({ length: 500 }, (_, i) => ({ i })); + const result = await fakeServer.invoke("reactome_cypher_query", { + query: "UNWIND range(1, 500) AS i RETURN i", + max_rows: 10, + }); + expect(textOf(result)).toContain("**Rows returned:** 10 of 500"); + }); + + it("refuses a max_rows above the cap, before running anything", () => { + expect(() => + fakeServer.invoke("reactome_cypher_query", { query: "RETURN 1", max_rows: 1_000_000 }) + ).toThrow(/<=1000/); + expect(fake.sessions).toHaveLength(0); + }); + + it("samples a label read-only, and refuses a label that could inject Cypher", async () => { + fake.respond = () => [{ n: { stId: "R-HSA-1" } }]; + const ok = await fakeServer.invoke("reactome_cypher_sample", { label: "Pathway" }); + expect(textOf(ok)).toContain("R-HSA-1"); + expect(fake.sessions[0]?.config.defaultAccessMode).toBe("READ"); + + await expect( + fakeServer.invoke("reactome_cypher_sample", { label: "Pathway`) DETACH DELETE n //" }) + ).rejects.toThrow(/Invalid label/); + }); +}); + +describe("reactome_cypher_* tools with the gate closed", () => { + it("registers nothing, even when called directly", () => { + delete process.env.NEO4J_URI; + delete process.env.MCP_ALLOW_CYPHER; + const fakeServer = createFakeServer(); + registerCypherTools(fakeServer.server); + expect(fakeServer.toolNames()).toEqual([]); + }); +}); diff --git a/tests/cypher.test.ts b/tests/cypher.test.ts new file mode 100644 index 0000000..175d895 --- /dev/null +++ b/tests/cypher.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect } from "vitest"; +import { capCypherRows } from "../src/tools/cypher.js"; + +describe("capCypherRows", () => { + it("passes small rows through unchanged", () => { + const rows = [{ a: 1 }, { b: 2 }, { c: 3 }]; + const { output, stats } = capCypherRows(rows, 100, 2000, 40000); + expect(output).toEqual(rows); + expect(stats).toEqual({ + rowCountTruncated: false, + rowsWidthTruncated: 0, + stoppedAtTotal: false, + }); + }); + + it("truncates row count when rows.length exceeds max_rows", () => { + const rows = Array.from({ length: 10 }, (_, i) => ({ i })); + const { output, stats } = capCypherRows(rows, 3, 2000, 40000); + expect(output).toHaveLength(3); + expect(stats.rowCountTruncated).toBe(true); + }); + + it("replaces wide rows with a summary object", () => { + const wide = { big: "x".repeat(3000) }; + const { output, stats } = capCypherRows([wide], 10, 500, 40000); + expect(stats.rowsWidthTruncated).toBe(1); + const entry = output[0] as Record; + expect(entry.__truncated).toBe(true); + expect(entry.reason).toBe("row_too_large"); + expect(entry.keys).toEqual(["big"]); + expect(entry.original_chars).toBeGreaterThan(3000); + }); + + it("stops accumulating rows when total char budget is exhausted", () => { + const rows = Array.from({ length: 10 }, () => ({ payload: "y".repeat(100) })); + const { output, stats } = capCypherRows(rows, 100, 2000, 300); + expect(output.length).toBeLessThan(10); + expect(stats.stoppedAtTotal).toBe(true); + }); + + it("reports both width and total-cap truncation together", () => { + const rows = [{ big: "z".repeat(3000) }, { small: "ok" }, { small: "ok" }]; + const { stats } = capCypherRows(rows, 10, 500, 500); + expect(stats.rowsWidthTruncated).toBeGreaterThan(0); + }); +}); diff --git a/tests/graph-access-gate.test.ts b/tests/graph-access-gate.test.ts new file mode 100644 index 0000000..6a88233 --- /dev/null +++ b/tests/graph-access-gate.test.ts @@ -0,0 +1,369 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join, relative } from "node:path"; +import type { AddressInfo } from "node:net"; +import type { FakeNeo4jState } from "./helpers/fake-neo4j.js"; + +/** + * Graph database access is off unless an operator turns it on, twice. + * + * The Cypher tools exist for internal instances pointed at a dedicated + * Reactome graph database. A publicly hosted instance must never offer them: + * an arbitrary query from the internet can take the database -- and the + * website in front of it -- down. + * + * The gate is `NEO4J_URI` set AND `MCP_ALLOW_CYPHER=1`. The last time this + * server had a gate, it was consulted in four places and one of them -- the + * HTTP entrypoint, which is the one actually hosted -- still opened a + * connection on `NEO4J_URI` alone. So these tests do not check the predicate + * once and trust the call sites. They drive **every** surface, through every + * entrypoint and factory, in every combination of the two variables, and + * count driver constructions on a fake `neo4j-driver` that cannot open a + * socket. Each surface is asserted in both directions: "absent when closed" + * alone would pass against a build that never offers graph access at all. + */ + +const fake = vi.hoisted((): FakeNeo4jState => ({ drivers: [], sessions: [], respond: () => [] })); + +vi.mock("neo4j-driver", async () => { + const { fakeNeo4jModule } = await import("./helpers/fake-neo4j.js"); + return fakeNeo4jModule(fake); +}); + +// The stdio entrypoint connects a transport on import. This one talks to +// nothing, so importing `src/index.ts` runs the real startup path -- server +// construction, and the schema warm-up -- without touching stdin/stdout. +vi.mock("@modelcontextprotocol/sdk/server/stdio.js", () => ({ + StdioServerTransport: class { + onclose?: () => void; + onerror?: (e: Error) => void; + onmessage?: (m: unknown) => void; + async start() {} + async close() {} + async send() {} + }, +})); + +const URI = "bolt://127.0.0.1:7687"; +const GRAPH_VARS = ["NEO4J_URI", "MCP_ALLOW_CYPHER", "MCP_TOOL_GROUPS", "MCP_HTTP_PORT"] as const; + +interface GateCase { + name: string; + env: Partial>; + open: boolean; +} + +const CASES: GateCase[] = [ + { name: "neither variable set", env: {}, open: false }, + // The case that matters most: a connection string is not consent. This is + // the configuration a public instance would most plausibly arrive at. + { name: "NEO4J_URI alone", env: { NEO4J_URI: URI }, open: false }, + { name: "MCP_ALLOW_CYPHER=1 alone", env: { MCP_ALLOW_CYPHER: "1" }, open: false }, + { + name: "NEO4J_URI with MCP_ALLOW_CYPHER=true (only 1 counts)", + env: { NEO4J_URI: URI, MCP_ALLOW_CYPHER: "true" }, + open: false, + }, + { + name: "a blank NEO4J_URI with MCP_ALLOW_CYPHER=1", + env: { NEO4J_URI: " ", MCP_ALLOW_CYPHER: "1" }, + open: false, + }, + { name: "both set", env: { NEO4J_URI: URI, MCP_ALLOW_CYPHER: "1" }, open: true }, +]; + +const CYPHER_TOOLS = ["reactome_cypher_query", "reactome_cypher_schema", "reactome_cypher_sample"]; +const SCHEMA_URI = "reactome://graph/schema"; + +let saved: Record = {}; + +function applyEnv(env: GateCase["env"]) { + saved = {}; + for (const key of GRAPH_VARS) { + saved[key] = process.env[key]; + delete process.env[key]; + } + for (const [key, value] of Object.entries(env)) process.env[key] = value; +} + +function restoreEnv() { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } +} + +function resetFake() { + fake.drivers.length = 0; + fake.sessions.length = 0; + fake.respond = () => []; +} + +/** Let fire-and-forget startup work (the schema warm-up) run to completion. */ +async function settle() { + for (let i = 0; i < 5; i++) await new Promise(resolve => setTimeout(resolve, 5)); +} + +const cypherNames = (names: string[]) => names.filter(n => n.startsWith("reactome_cypher")); + +function recordingServer() { + const tools: string[] = []; + const resources: string[] = []; + const server = { + tool: (...args: unknown[]) => { + if (typeof args[0] === "string") tools.push(args[0]); + return undefined; + }, + resource: (...args: unknown[]) => { + const t = args[1] as { uriTemplate?: { toString(): string } } | undefined; + const uri = typeof args[1] === "string" ? args[1] : t?.uriTemplate?.toString(); + if (uri) resources.push(uri); + return undefined; + }, + }; + return { server: server as never, tools, resources }; +} + +describe.each(CASES)("graph access gate: $name", ({ env, open }) => { + beforeEach(() => { + applyEnv(env); + resetFake(); + vi.resetModules(); + }); + + afterEach(() => { + restoreEnv(); + vi.doUnmock("../src/http.js"); + vi.resetModules(); + }); + + it(`the shared predicate reports ${open ? "open" : "closed"}`, async () => { + const { isGraphAccessEnabled } = await import("../src/graph/gate.js"); + expect(isGraphAccessEnabled()).toBe(open); + }); + + it(`createServer ${open ? "offers" : "withholds"} the tools, the resource and the instructions, and constructs no driver`, async () => { + const { createServer } = await import("../src/server.js"); + const server = createServer() as unknown as { + _registeredTools: Record; + _registeredResources: Record; + server: { _instructions?: string }; + }; + + const tools = Object.keys(server._registeredTools); + const resources = Object.keys(server._registeredResources); + const instructions = server.server._instructions ?? ""; + + // The REST surface is there either way; the server is usable. + expect(tools).toContain("reactome_search"); + expect(instructions).toContain("reactome_search"); + + if (open) { + expect(cypherNames(tools).sort()).toEqual([...CYPHER_TOOLS].sort()); + expect(resources).toContain(SCHEMA_URI); + expect(instructions).toContain("reactome_cypher_query"); + } else { + expect(cypherNames(tools)).toEqual([]); + expect(resources).not.toContain(SCHEMA_URI); + expect(instructions.toLowerCase()).not.toContain("cypher"); + expect(instructions.toLowerCase()).not.toContain("neo4j"); + } + + // Building a server is not a reason to connect: the driver is lazy. + expect(fake.drivers).toHaveLength(0); + }); + + it(`registrars given every group explicitly still ${open ? "offer" : "withhold"} graph access`, async () => { + // Defence in depth: a caller that bypasses resolveToolGroups() and hands + // the registrars the full group list must not get past the gate either. + const { registerAllTools, TOOL_GROUPS } = await import("../src/tools/index.js"); + const { registerAllResources } = await import("../src/resources/index.js"); + const { buildServerInstructions } = await import("../src/instructions.js"); + const everyGroup = Object.keys(TOOL_GROUPS) as Parameters[1]; + + const rec = recordingServer(); + registerAllTools(rec.server, everyGroup); + registerAllResources(rec.server, everyGroup); + const text = buildServerInstructions(everyGroup); + + if (open) { + expect(cypherNames(rec.tools)).toHaveLength(3); + expect(rec.resources).toContain(SCHEMA_URI); + expect(text).toContain("reactome_cypher_schema"); + } else { + expect(cypherNames(rec.tools)).toEqual([]); + expect(rec.resources).not.toContain(SCHEMA_URI); + expect(text.toLowerCase()).not.toContain("cypher"); + } + expect(fake.drivers).toHaveLength(0); + }); + + it(`MCP_TOOL_GROUPS naming cypher ${open ? "is honoured" : "refuses to start"}`, async () => { + const { resolveToolGroups } = await import("../src/tools/index.js"); + if (open) { + expect(resolveToolGroups("search,cypher")).toEqual(["search", "cypher"]); + } else { + // Asking for graph access and not getting it is loud, not silent. + expect(() => resolveToolGroups("search,cypher")).toThrow(/MCP_ALLOW_CYPHER/); + } + }); + + it(`the driver factory ${open ? "constructs a driver" : "refuses to construct one"}`, async () => { + const { getDriver, runRead } = await import("../src/clients/neo4j.js"); + if (open) { + expect(getDriver()).toBeDefined(); + expect(fake.drivers).toHaveLength(1); + } else { + expect(() => getDriver()).toThrow(/MCP_ALLOW_CYPHER/); + await expect(runRead("RETURN 1")).rejects.toThrow(/MCP_ALLOW_CYPHER/); + expect(fake.drivers).toHaveLength(0); + } + }); + + it(`the stdio entrypoint ${open ? "warms the schema over one driver" : "never constructs a driver"}`, async () => { + await import("../src/index.js"); + await settle(); + expect(fake.drivers).toHaveLength(open ? 1 : 0); + if (open) { + // The warm-up ran, read-only, on the dedicated graph. + expect(fake.drivers[0]?.uri).toBe(URI); + expect(fake.sessions.length).toBeGreaterThan(0); + for (const s of fake.sessions) expect(s.config.defaultAccessMode).toBe("READ"); + } + }); + + it(`the HTTP entrypoint ${open ? "warms the schema over one driver" : "never constructs a driver"}`, async () => { + // src/http-server.ts is the entrypoint a hosted instance runs, and the one + // the previous gate missed. Its transport is stubbed so nothing binds; + // everything it does after the server starts is real. + vi.doMock("../src/http.js", () => ({ startHttpServer: vi.fn(async () => ({})) })); + process.env.MCP_HTTP_PORT = "4999"; + await import("../src/http-server.js"); + await settle(); + expect(fake.drivers).toHaveLength(open ? 1 : 0); + }); + + it(`startHttpServer sessions ${open ? "offer" : "withhold"} graph access, and construct no driver`, async () => { + const { startHttpServer } = await import("../src/http.js"); + const http = await startHttpServer(0, "127.0.0.1"); + try { + const { port } = http.address() as AddressInfo; + const base = `http://127.0.0.1:${port}`; + const headers = { + "Content-Type": "application/json", + Accept: "application/json, text/event-stream", + }; + + const init = await fetch(`${base}/mcp`, { + method: "POST", + headers, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: { + protocolVersion: "2024-11-05", + capabilities: {}, + clientInfo: { name: "vitest", version: "0" }, + }, + }), + }); + const initText = await init.text(); + const sessionId = init.headers.get("mcp-session-id"); + expect(sessionId).toBeTruthy(); + const session = { ...headers, "mcp-session-id": sessionId ?? "" }; + + await fetch(`${base}/mcp`, { + method: "POST", + headers: session, + body: JSON.stringify({ jsonrpc: "2.0", method: "notifications/initialized" }), + }); + + const list = async (method: string) => { + const res = await fetch(`${base}/mcp`, { + method: "POST", + headers: session, + body: JSON.stringify({ jsonrpc: "2.0", id: 2, method }), + }); + return res.text(); + }; + const toolsText = await list("tools/list"); + const resourcesText = await list("resources/list"); + const health = (await (await fetch(`${base}/health`)).json()) as Record; + + expect(toolsText).toContain("reactome_search"); + if (open) { + for (const name of CYPHER_TOOLS) expect(toolsText).toContain(`"${name}"`); + expect(resourcesText).toContain(SCHEMA_URI); + expect(initText).toContain("reactome_cypher_query"); + } else { + expect(toolsText).not.toContain("reactome_cypher"); + expect(resourcesText).not.toContain(SCHEMA_URI); + expect(initText.toLowerCase()).not.toContain("cypher"); + } + // An operator can see which way the gate went without reading the env. + expect(health.graphAccess).toBe(open); + // Serving sessions is not a reason to connect either. + expect(fake.drivers).toHaveLength(0); + } finally { + await new Promise(resolve => http.close(resolve)); + } + }); +}); + +describe("graph access gate: open, but the cypher group withheld", () => { + beforeEach(() => { + applyEnv({ NEO4J_URI: URI, MCP_ALLOW_CYPHER: "1", MCP_TOOL_GROUPS: "search,pathway" }); + resetFake(); + vi.resetModules(); + }); + + afterEach(() => { + restoreEnv(); + vi.resetModules(); + }); + + it("offers nothing graph-shaped and opens no connection for nobody", async () => { + const { createServer } = await import("../src/server.js"); + const server = createServer() as unknown as { + _registeredTools: Record; + _registeredResources: Record; + }; + expect(cypherNames(Object.keys(server._registeredTools))).toEqual([]); + expect(Object.keys(server._registeredResources)).not.toContain(SCHEMA_URI); + + // Without the tools or the resource nothing can read a warmed cache, so + // the warm-up must not connect. + await import("../src/index.js"); + await settle(); + expect(fake.drivers).toHaveLength(0); + }); +}); + +describe("graph access gate: structure", () => { + it("imports neo4j-driver from exactly one file, the gated factory", () => { + // Every connection has to go through getDriver(), which checks the gate + // itself. A second import would be a second door. + const importers: string[] = []; + const root = new URL("../src", import.meta.url).pathname; + const walk = (dir: string) => { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) walk(full); + else if (entry.endsWith(".ts")) { + const text = readFileSync(full, "utf8"); + if (/(from\s+|import\s*\(\s*)["']neo4j-driver["']/.test(text)) { + importers.push(relative(root, full)); + } + } + } + }; + walk(root); + expect(importers).toEqual(["clients/neo4j.ts"]); + }); + + it("keeps neo4j-driver as a declared dependency", () => { + const pkg = JSON.parse(readFileSync(new URL("../package.json", import.meta.url), "utf8")); + expect(pkg.dependencies["neo4j-driver"]).toBeDefined(); + }); +}); diff --git a/tests/graph-schema.test.ts b/tests/graph-schema.test.ts new file mode 100644 index 0000000..0eddd26 --- /dev/null +++ b/tests/graph-schema.test.ts @@ -0,0 +1,376 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import type { FakeNeo4jState } from "./helpers/fake-neo4j.js"; +import type { GraphSchema } from "../src/graph/schema.js"; + +/** + * Schema extraction against the database the tools are actually pointed at: + * the Reactome graph database container, which runs Neo4j 4.4 Community with + * **no APOC**. The previous version read `apoc.meta.*` and would have failed + * there on its first query. These script a fake driver that answers like that + * database -- APOC procedures do not exist -- and assert the schema is built + * from built-in procedures, with each optional section degrading on its own. + */ + +const fake = vi.hoisted((): FakeNeo4jState => ({ drivers: [], sessions: [], respond: () => [] })); + +vi.mock("neo4j-driver", async () => { + const { fakeNeo4jModule } = await import("./helpers/fake-neo4j.js"); + return fakeNeo4jModule(fake); +}); + +import { queriesRun } from "./helpers/fake-neo4j.js"; +import { fetchGraphSchema, _resetGraphSchemaCache } from "../src/graph/schema.js"; +import { formatGraphSchemaMarkdown } from "../src/graph/format-schema.js"; +import { closeDriver } from "../src/clients/neo4j.js"; + +const NO_PROCEDURE = (name: string) => + new Error( + `There is no procedure with the name \`${name}\` registered for this database instance.` + ); + +/** + * Answers the way Neo4j 4.4 Community without APOC does. `overrides` replaces + * the answer for any query containing its key. + */ +function community44( + overrides: Record) => unknown[]> = {} +) { + return (query: string, params: Record): unknown[] => { + for (const [needle, answer] of Object.entries(overrides)) { + if (query.includes(needle)) return answer(params); + } + const apoc = /\b(apoc\.[\w.]+)/.exec(query); + if (apoc?.[1]) throw NO_PROCEDURE(apoc[1]); + + if (query.includes("dbms.components")) + return [{ name: "Neo4j Kernel", versions: ["4.4.40"], edition: "community" }]; + if (query.includes("db.labels")) return [{ label: "Pathway" }, { label: "Reaction" }]; + if (query.includes("db.relationshipTypes")) + return [{ relationshipType: "hasEvent" }, { relationshipType: "input" }]; + if (query.includes("db.schema.nodeTypeProperties")) + return [ + { + nodeType: ":`Pathway`", + nodeLabels: ["Pathway"], + propertyName: "stId", + propertyTypes: ["String"], + mandatory: true, + }, + ]; + if (query.includes("db.schema.relTypeProperties")) + return [ + { + relType: ":`input`", + propertyName: "stoichiometry", + propertyTypes: ["Long"], + mandatory: true, + }, + ]; + if (query.includes("db.schema.visualization")) + return [ + { + nodes: [ + { identity: -1, labels: ["Pathway"], properties: { name: "Pathway" } }, + { identity: -2, labels: ["Reaction"], properties: { name: "Reaction" } }, + ], + relationships: [{ identity: -3, start: -1, end: -2, type: "hasEvent", properties: {} }], + }, + ]; + if (query.includes("SHOW INDEXES")) + return [ + { + name: "index_pathway_stId", + type: "BTREE", + entityType: "NODE", + labelsOrTypes: ["Pathway"], + properties: ["stId"], + state: "ONLINE", + }, + ]; + if (query.includes("SHOW CONSTRAINTS")) + return [ + { + name: "constraint_dbId", + type: "UNIQUENESS", + entityType: "NODE", + labelsOrTypes: ["DatabaseObject"], + properties: ["dbId"], + }, + ]; + // Per-token counts: one UNION ALL query each for labels and for + // relationship types, keyed by parameter so the names need no escaping. + if (query.includes("UNION ALL") || /RETURN \$\w+ AS key/.test(query)) { + return Object.entries(params).map(([, key]) => ({ + key, + count: key === "Pathway" ? 2_000 : key === "hasEvent" ? 9_000 : 500, + })); + } + if (/MATCH \(n\) RETURN count\(n\)/.test(query)) return [{ count: 3_000_000 }]; + if (/MATCH \(\)-\[r\]->\(\) RETURN count\(r\)/.test(query)) return [{ count: 12_000_000 }]; + throw new Error(`unscripted query: ${query}`); + }; +} + +describe("fetchGraphSchema on Neo4j 4.4 Community without APOC", () => { + beforeEach(() => { + process.env.NEO4J_URI = "bolt://127.0.0.1:7687"; + process.env.MCP_ALLOW_CYPHER = "1"; + fake.drivers.length = 0; + fake.sessions.length = 0; + fake.respond = community44(); + _resetGraphSchemaCache(); + }); + + afterEach(async () => { + _resetGraphSchemaCache(); + await closeDriver(); + delete process.env.NEO4J_URI; + delete process.env.MCP_ALLOW_CYPHER; + }); + + it("builds every section from built-in procedures and never calls APOC", async () => { + const schema = await fetchGraphSchema(); + + expect(queriesRun(fake).filter(q => /apoc\./i.test(q))).toEqual([]); + expect(schema.unavailable).toEqual([]); + expect(schema.dbComponents[0]?.edition).toBe("community"); + expect(schema.stats.nodeCount).toBe(3_000_000); + expect(schema.stats.relCount).toBe(12_000_000); + expect(schema.stats.labels).toEqual({ Pathway: 2_000, Reaction: 500 }); + expect(schema.stats.relTypesCount).toEqual({ hasEvent: 9_000, input: 500 }); + expect(schema.patterns).toEqual([{ start: "Pathway", type: "hasEvent", end: "Reaction" }]); + expect(schema.nodeTypeProperties[0]?.propertyName).toBe("stId"); + // The built-in reports ":`input`"; the schema carries the bare name. + expect(schema.relTypeProperties[0]?.relType).toBe("input"); + expect(schema.indexes[0]?.name).toBe("index_pathway_stId"); + expect(schema.constraints[0]?.name).toBe("constraint_dbId"); + }); + + it("reads only, and under a timeout", async () => { + await fetchGraphSchema(); + expect(fake.sessions.length).toBeGreaterThan(0); + for (const session of fake.sessions) { + expect(session.config.defaultAccessMode).toBe("READ"); + for (const run of session.runs) expect(run.config?.timeout).toBeGreaterThan(0); + } + }); + + it("falls back to db.indexes() and db.constraints() where SHOW is not understood", async () => { + const unsupported = () => { + throw new Error("Invalid input 'SHOW'"); + }; + fake.respond = community44({ + "SHOW INDEXES": unsupported, + "SHOW CONSTRAINTS": unsupported, + "db.indexes": () => [ + { + name: "legacy_index", + type: "BTREE", + entityType: "NODE", + labelsOrTypes: ["Pathway"], + properties: ["stId"], + state: "ONLINE", + }, + ], + "db.constraints": () => [ + { name: "legacy_constraint", description: "CONSTRAINT ON (d:DatabaseObject) ..." }, + ], + }); + + const schema = await fetchGraphSchema(); + expect(schema.indexes.map(i => i.name)).toEqual(["legacy_index"]); + expect(schema.constraints.map(c => c.name)).toEqual(["legacy_constraint"]); + expect(schema.unavailable).toEqual([]); + }); + + it("reports a section that fails everywhere, and still returns the rest", async () => { + fake.respond = community44({ + "db.schema.nodeTypeProperties": () => { + throw new Error("The transaction has been terminated (timeout)"); + }, + "db.indexes": () => { + throw NO_PROCEDURE("db.indexes"); + }, + "SHOW INDEXES": () => { + throw new Error("Invalid input 'SHOW'"); + }, + }); + + const schema = await fetchGraphSchema(); + expect(schema.nodeTypeProperties).toEqual([]); + expect(schema.indexes).toEqual([]); + expect(schema.unavailable.map(u => u.section).sort()).toEqual( + ["indexes", "nodeTypeProperties"].sort() + ); + // The rest is intact. + expect(schema.stats.labels.Pathway).toBe(2_000); + + // And the digest says so, rather than rendering an empty section as if + // the graph had no properties. + const md = formatGraphSchemaMarkdown(schema); + expect(md).toContain("Unavailable"); + expect(md).toContain("nodeTypeProperties"); + }); + + it("refuses rather than caching an empty schema when nothing answers", async () => { + fake.respond = () => { + throw new Error("Could not perform discovery. No routing servers available."); + }; + await expect(fetchGraphSchema()).rejects.toThrow(/schema/i); + + // Not cached: the next call tries again. + fake.respond = community44(); + const schema = await fetchGraphSchema(); + expect(schema.stats.labels.Pathway).toBe(2_000); + }); + + it("caches; a second call runs no queries", async () => { + const first = await fetchGraphSchema(); + const before = queriesRun(fake).length; + const second = await fetchGraphSchema(); + expect(second).toBe(first); + expect(queriesRun(fake).length).toBe(before); + }); + + it("shares one fetch between concurrent first callers", async () => { + const [a, b, c] = await Promise.all([ + fetchGraphSchema(), + fetchGraphSchema(), + fetchGraphSchema(), + ]); + expect(a).toBe(b); + expect(b).toBe(c); + expect(queriesRun(fake).filter(q => q.includes("dbms.components"))).toHaveLength(1); + }); +}); + +function minimalSchema(): GraphSchema { + return { + fetchedAt: "2026-10-14T15:00:00Z", + dbComponents: [{ name: "Neo4j Kernel", versions: ["4.4.40"], edition: "community" }], + stats: { + nodeCount: 100, + relCount: 200, + labels: { Pathway: 50, Reaction: 30, Entity: 20 }, + relTypesCount: { hasEvent: 80, input: 120 }, + }, + patterns: [{ start: "Pathway", type: "hasEvent", end: "Reaction" }], + nodeTypeProperties: [ + { + nodeType: ":`Pathway`", + nodeLabels: ["Pathway"], + propertyName: "stId", + propertyTypes: ["String"], + mandatory: true, + }, + { + nodeType: ":`Pathway`", + nodeLabels: ["Pathway"], + propertyName: "displayName", + propertyTypes: ["String"], + mandatory: false, + }, + ], + relTypeProperties: [ + { relType: "input", propertyName: "stoichiometry", propertyTypes: ["Long"], mandatory: true }, + ], + indexes: [ + { + name: "pathway_stId", + labelsOrTypes: ["Pathway"], + properties: ["stId"], + type: "BTREE", + state: "ONLINE", + }, + ], + constraints: [ + { name: "pathway_stId_unique", labelsOrTypes: ["Pathway"], properties: ["stId"] }, + ], + unavailable: [], + }; +} + +describe("formatGraphSchemaMarkdown", () => { + it("covers totals, labels, rel types, patterns, properties, indexes, constraints", () => { + const md = formatGraphSchemaMarkdown(minimalSchema()); + expect(md).toContain("Neo4j:** 4.4.40"); + expect(md).toContain("100 nodes"); + expect(md).toContain("`Pathway` 50"); + expect(md).toContain("`hasEvent` 80"); + expect(md).toContain("(:Pathway)-[:hasEvent]->(:Reaction)"); + expect(md).toContain("stId String!"); + expect(md).toContain("stoichiometry Long! — on input"); + expect(md).toContain("### Indexes (1)"); + expect(md).toContain("### Constraints (1)"); + expect(md).toContain("reactome://graph/schema"); + expect(md).not.toContain("Unavailable"); + // Nothing in the digest should send a reader looking for APOC. + expect(md.toLowerCase()).not.toContain("apoc"); + }); + + it("stays small enough for an LLM context window", () => { + expect(formatGraphSchemaMarkdown(minimalSchema()).length).toBeLessThan(10_000); + }); + + it("summarises a type's endpoints as label sets, most specific first", () => { + // Reactome nodes carry their class hierarchy as labels, so the real + // visualization lists hundreds of pairs per type. Listing them pair by + // pair took the digest past the response cap. + const md = formatGraphSchemaMarkdown({ + ...minimalSchema(), + patterns: [ + { start: "Pathway", type: "hasEvent", end: "Reaction" }, + { start: "Pathway", type: "hasEvent", end: "Entity" }, + { start: "Reaction", type: "hasEvent", end: "Reaction" }, + ], + }); + expect(md).toContain("(:Reaction|Pathway)-[:hasEvent]->(:Entity|Reaction)"); + }); + + it("lists properties every label set shares once", () => { + const md = formatGraphSchemaMarkdown({ + ...minimalSchema(), + nodeTypeProperties: [ + { + nodeType: ":`A`", + nodeLabels: ["A"], + propertyName: "dbId", + propertyTypes: ["Long"], + mandatory: true, + }, + { + nodeType: ":`A`", + nodeLabels: ["A"], + propertyName: "a", + propertyTypes: ["String"], + mandatory: false, + }, + { + nodeType: ":`B`", + nodeLabels: ["B"], + propertyName: "dbId", + propertyTypes: ["Long"], + mandatory: true, + }, + ], + }); + expect(md).toContain("**On every label set:** dbId Long!"); + expect(md).toContain("- **A**: a String"); + expect(md).toContain("- **B**\n"); + }); + + it("omits empty index and constraint sections", () => { + const md = formatGraphSchemaMarkdown({ ...minimalSchema(), indexes: [], constraints: [] }); + expect(md).not.toContain("### Indexes"); + expect(md).not.toContain("### Constraints"); + }); + + it("orders labels by count, descending", () => { + const md = formatGraphSchemaMarkdown(minimalSchema()); + const p = md.indexOf("`Pathway` 50"); + const r = md.indexOf("`Reaction` 30"); + const e = md.indexOf("`Entity` 20"); + expect(p).toBeLessThan(r); + expect(r).toBeLessThan(e); + }); +}); diff --git a/tests/helpers/fake-neo4j.ts b/tests/helpers/fake-neo4j.ts new file mode 100644 index 0000000..eaadaa4 --- /dev/null +++ b/tests/helpers/fake-neo4j.ts @@ -0,0 +1,80 @@ +/** + * A stand-in for `neo4j-driver`, so no test ever opens a socket to a database. + * + * Use it with `vi.mock("neo4j-driver", () => fakeNeo4jModule(state))`, where + * `state` comes from `vi.hoisted(newFakeNeo4jState)` so the mock factory and + * the test share it across `vi.resetModules()`. + * + * It records every driver constructed, every session opened (with its config) + * and every query run (with its transaction config), and answers queries + * through `state.respond`, which a test replaces to script the database. + */ + +export interface FakeRun { + query: string; + params: Record; + config: Record | undefined; +} + +export interface FakeSession { + config: Record; + runs: FakeRun[]; + closed: boolean; +} + +export interface FakeNeo4jState { + drivers: Array<{ uri: string; config: Record }>; + sessions: FakeSession[]; + /** Rows for a query; throw to simulate a database error. */ + respond: (query: string, params: Record) => unknown[] | Promise; +} + +export function newFakeNeo4jState(): FakeNeo4jState { + return { drivers: [], sessions: [], respond: () => [] }; +} + +export function resetFakeNeo4j(state: FakeNeo4jState): void { + state.drivers.length = 0; + state.sessions.length = 0; + state.respond = () => []; +} + +/** All query texts run so far, across every session, in order. */ +export function queriesRun(state: FakeNeo4jState): string[] { + return state.sessions.flatMap(s => s.runs.map(r => r.query)); +} + +export function fakeNeo4jModule(state: FakeNeo4jState) { + const neo4j = { + driver(uri: string, _auth: unknown, config: Record = {}) { + state.drivers.push({ uri, config }); + return { + session(sessionConfig: Record = {}) { + const session: FakeSession = { config: sessionConfig, runs: [], closed: false }; + state.sessions.push(session); + return { + async run( + query: string, + params: Record = {}, + txConfig?: Record + ) { + session.runs.push({ query, params, config: txConfig }); + const rows = await state.respond(query, params); + return { + records: rows.map(row => ({ toObject: () => row })), + }; + }, + async close() { + session.closed = true; + }, + }; + }, + async close() {}, + }; + }, + auth: { basic: (user: string, _password: string) => ({ scheme: "basic", principal: user }) }, + session: { READ: "READ", WRITE: "WRITE" }, + int: (value: number | string) => ({ __int: Number(value) }), + }; + return { default: neo4j, ...neo4j }; +} diff --git a/tests/input-bounds.test.ts b/tests/input-bounds.test.ts index 4eeb716..1440015 100644 --- a/tests/input-bounds.test.ts +++ b/tests/input-bounds.test.ts @@ -94,13 +94,13 @@ const ABSURD = Array.from({ length: 100_001 }, () => "R-HSA-109582"); const ONE = ["R-HSA-109582"]; describe("tool input bounds", () => { - it("covers every tool, and the set does not change with the environment", async () => { + 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. The - // exact count is asserted rather than a floor, so a tool that appears - // only under some environment shows up here as a number that moved. - expect(schemas.size).toBe(59); - expect([...schemas.keys()].filter(n => n.includes("cypher"))).toEqual([]); + // Without this the sweep would pass vacuously against an empty map, and + // without the Cypher names it would pass while missing the three tools + // that only register behind the graph access gate. + 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 () => { diff --git a/tests/no-graph-access.test.ts b/tests/no-graph-access.test.ts deleted file mode 100644 index 08f53c9..0000000 --- a/tests/no-graph-access.test.ts +++ /dev/null @@ -1,100 +0,0 @@ -import { describe, it, expect } from "vitest"; -import { readFileSync, readdirSync, statSync } from "node:fs"; -import { join } from "node:path"; -import { registerAllTools } from "../src/tools/index.js"; -import { registerStaticResources } from "../src/resources/static.js"; -import { buildServerInstructions } from "../src/instructions.js"; - -/** - * This server does not reach Neo4j, and cannot be configured to. - * - * It is hosted publicly. Constitution Principle IV always said no deployment - * holds a graph connection, and until 2026-09-21 that was enforced by a gate - * — which meant the property was true of a *configuration* rather than of the - * code. Gates get flipped, and this one was tested in four places and got - * three of them: `src/http-server.ts`, the entrypoint that actually runs in - * production, still opened a connection on `NEO4J_URI` alone. - * - * So the tools are gone instead. These tests set the environment that used to - * switch graph access ON, because asserting absence with the switch OFF would - * only confirm the old gate still works. - */ - -const GRAPH_ENV = { NEO4J_URI: "bolt://localhost:7690", MCP_ALLOW_CYPHER: "1" }; - -function withGraphEnv(body: () => T): T { - const previous: Record = {}; - for (const [k, v] of Object.entries(GRAPH_ENV)) { - previous[k] = process.env[k]; - process.env[k] = v; - } - try { - return body(); - } finally { - for (const [k, v] of Object.entries(previous)) { - if (v === undefined) delete process.env[k]; - else process.env[k] = v; - } - } -} - -function collect(register: (server: never) => void, key: "tool" | "resource"): string[] { - const names: string[] = []; - const server = { - [key]: (...args: unknown[]) => { - if (typeof args[0] === "string") names.push(args[0]); - return undefined; - }, - }; - register(server as never); - return names; -} - -describe("no graph access", () => { - it("registers no Cypher tool even with the old switches on", () => { - const names = withGraphEnv(() => collect(registerAllTools, "tool")); - expect(names.filter(n => n.includes("cypher"))).toEqual([]); - // The sweep is only meaningful if the other tools did register. - expect(names.length).toBeGreaterThan(50); - }); - - it("publishes no graph schema resource", () => { - const names = withGraphEnv(() => collect(registerStaticResources, "resource")); - expect(names).not.toContain("reactome://graph/schema"); - expect(names).toContain("reactome://species"); - }); - - it("does not tell clients a graph is available", () => { - // The instructions were the surface that survived the gate: a server - // advertising tools it had not registered is invisible to any check that - // asks the server what it can do, because it says it can. - const text = withGraphEnv(buildServerInstructions); - expect(text.toLowerCase()).not.toContain("cypher"); - expect(text.toLowerCase()).not.toContain("neo4j"); - expect(text).toContain("reactome_search"); - }); - - it("has no neo4j driver dependency", () => { - const pkg = JSON.parse(readFileSync(new URL("../package.json", import.meta.url), "utf8")); - const deps = { ...pkg.dependencies, ...pkg.devDependencies }; - expect(Object.keys(deps).filter(d => d.includes("neo4j"))).toEqual([]); - }); - - it("has no source file that imports a graph driver", () => { - // The tools could come back one import at a time. This is the check that - // notices before a tool exists to catch. - const offenders: string[] = []; - const walk = (dir: string) => { - for (const entry of readdirSync(dir)) { - const full = join(dir, entry); - if (statSync(full).isDirectory()) walk(full); - else if (entry.endsWith(".ts")) { - const text = readFileSync(full, "utf8"); - if (/from\s+["']neo4j-driver["']/.test(text)) offenders.push(full); - } - } - }; - walk(new URL("../src", import.meta.url).pathname); - expect(offenders).toEqual([]); - }); -}); diff --git a/tests/readme-tools.test.ts b/tests/readme-tools.test.ts index 94f4cfd..0bc10fd 100644 --- a/tests/readme-tools.test.ts +++ b/tests/readme-tools.test.ts @@ -33,6 +33,21 @@ describe("README tool coverage", () => { expect(missing).toEqual([]); }); + it("documents the graph tools, which only register behind the gate", () => { + process.env.NEO4J_URI = "bolt://127.0.0.1:7687"; + process.env.MCP_ALLOW_CYPHER = "1"; + try { + const names = registered(); + expect(names.filter(n => n.startsWith("reactome_cypher"))).toHaveLength(3); + expect(names.filter(name => !readme.includes(name))).toEqual([]); + expect(readme).toContain(`${names.length} with graph access`); + } finally { + delete process.env.NEO4J_URI; + delete process.env.MCP_ALLOW_CYPHER; + } + expect(readme).toContain("MCP_ALLOW_CYPHER"); + }); + it("states the tool count, and states it correctly", () => { const names = registered(); expect(names.length).toBeGreaterThan(0); diff --git a/tests/server.test.ts b/tests/server.test.ts index 7b078eb..6b27d16 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -43,20 +43,21 @@ describe("createServer", () => { expect(toolNames).toContain("reactome_analyze_identifiers"); }); - it("registers no graph tools, whatever the environment says", () => { - // Principle IV, now structural rather than configured: there are no graph - // tools to gate. The environment is set to what used to switch them ON, - // because a test that unsets it would be asserting the old gate still - // works rather than that the tools are gone. - process.env.NEO4J_URI = "bolt://localhost:7690"; - process.env.MCP_ALLOW_CYPHER = "1"; + it("offers graph tools only behind both switches", () => { + // The full matrix, every entrypoint, lives in graph-access-gate.test.ts. + // This is the factory's own check, in both directions: absent on a + // connection string alone, present once the opt-in is added too. + const cypherTools = () => { + const server = createServer() as unknown as { _registeredTools?: Record }; + return Object.keys(server._registeredTools ?? {}).filter(n => + n.startsWith("reactome_cypher") + ); + }; + process.env.NEO4J_URI = "bolt://127.0.0.1:7687"; try { - const server = createServer(); - const registered = server as unknown as { _registeredTools?: Record }; - const toolNames = Object.keys(registered._registeredTools ?? {}); - - expect(toolNames.filter(n => n.includes("cypher"))).toEqual([]); - expect(toolNames.length).toBeGreaterThan(40); + expect(cypherTools()).toEqual([]); + process.env.MCP_ALLOW_CYPHER = "1"; + expect(cypherTools()).toHaveLength(3); } finally { delete process.env.NEO4J_URI; delete process.env.MCP_ALLOW_CYPHER; diff --git a/tests/tool-groups.test.ts b/tests/tool-groups.test.ts index a39a669..40bcf6a 100644 --- a/tests/tool-groups.test.ts +++ b/tests/tool-groups.test.ts @@ -31,6 +31,17 @@ function namesFor(register: (server: never) => void): string[] { return names; } +function withGraphAccess(body: () => T): T { + process.env.NEO4J_URI = "bolt://127.0.0.1:7687"; + process.env.MCP_ALLOW_CYPHER = "1"; + try { + return body(); + } finally { + delete process.env.NEO4J_URI; + delete process.env.MCP_ALLOW_CYPHER; + } +} + function registeredWith(groups: string | undefined): string[] { const previous = process.env.MCP_TOOL_GROUPS; if (groups === undefined) delete process.env.MCP_TOOL_GROUPS; @@ -82,7 +93,10 @@ describe("MCP_TOOL_GROUPS parsing", () => { }); it("registers everything only when genuinely unset", () => { - expect(resolveToolGroups(undefined)).toEqual(ALL_TOOL_GROUPS); + // Everything the instance is allowed: `cypher` additionally needs the + // graph access gate open, and it is closed here. + expect(resolveToolGroups(undefined)).toEqual(ALL_TOOL_GROUPS.filter(g => g !== "cypher")); + withGraphAccess(() => expect(resolveToolGroups(undefined)).toEqual(ALL_TOOL_GROUPS)); }); it("refuses an empty value rather than falling back to everything", () => { @@ -154,6 +168,16 @@ describe("instructions match what is registered", () => { expect(dangling).toEqual([]); }); + it("names no tool or resource the full server does not register, with graph access on", () => { + withGraphAccess(() => { + const registered = registeredNames(ALL_TOOL_GROUPS); + const text = buildServerInstructions(ALL_TOOL_GROUPS); + expect(text).toContain("reactome_cypher_query"); + const dangling = referenced(text).filter(r => !satisfied(r, registered)); + expect(dangling).toEqual([]); + }); + }); + it("names no tool or resource a restricted server does not register", () => { const groups: ToolGroup[] = ["search", "pathway", "entity", "utilities"]; const registered = registeredNames(groups); @@ -182,7 +206,13 @@ describe("resources follow the same switch", () => { }; it("registers every known resource when all groups are on", () => { - expect([...resourceUris(ALL_TOOL_GROUPS)].sort()).toEqual([...RESOURCE_URIS].sort()); + withGraphAccess(() => + expect([...resourceUris(ALL_TOOL_GROUPS)].sort()).toEqual([...RESOURCE_URIS].sort()) + ); + // With the gate closed, everything but the graph schema. + expect([...resourceUris(ALL_TOOL_GROUPS)].sort()).toEqual( + RESOURCE_URIS.filter(u => u !== "reactome://graph/schema").sort() + ); }); it("withholds the resources of an omitted group", () => { From b768d24dc39dfc16a0c8d244937d46e3c9e0cf9e Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 14 Oct 2026 01:21:03 +0000 Subject: [PATCH 2/5] Adversarial review: allowlist the guard, stream the rows, retry the schema Five findings from review of #44. The gate held; these are about what an internal instance with graph access on lets a query do. Each fix has a test that was written first and failed against e5e6400. **1. LOAD CSV got past the guard with a comment (medium).** Comments were deleted, so `LOAD/**/CSV` became `LOADCSV` and matched nothing. A comment separates tokens, so it is now read as a space. Tests cover comment-split keywords generally: LOAD/**/CSV, LOAD//\nCSV, CALL/**/dbms..., USE/**/system, SHOW/**/TRANSACTIONS. **2. Row caps were applied after the whole result was read (medium).** `MATCH (n) RETURN n` pulled every node into the process before max_rows cut it down. `runReadLimited` now streams the result with a fetch size of at most max_rows + 1 and breaks out after max_rows + 1 rows; breaking out of the driver's iterator discards the rest on the server. The fake driver yields from a 3M-row generator, and the test asserts exactly 11 rows were pulled for max_rows 10 and the stream was cancelled. Against the Release 97 container, `MATCH (n) RETURN n.dbId` with max_rows 5 returned in 257 ms. The output now says "more rows exist" instead of a total it cannot know without reading everything. **3. The denylist missed eleven procedures (low).** apoc.cypher.runMany/runFile/runSchema/parallel, apoc.schema.assert, apoc.util.sleep, apoc.custom.*, apoc.systemdb.*, apoc.lock.*, dbms.killQuery/listQueries/listConfig and USE were all allowed. The guard is now an allowlist: - CALL only for db.labels, db.relationshipTypes, db.propertyKeys, db.schema.*, db.indexes, db.constraints and dbms.components; - SHOW only for indexes and constraints; - no APOC name anywhere, and no LOAD CSV, USE or TERMINATE. `rejectWriteThroughCalls`/`WriteThroughRejected` are renamed to `assertReadOnlyQuery`/`QueryRejected`. There is a test for each named bypass. Against e5e6400 every one of the review's examples was allowed; the new "what is allowed" tests failed there because the export did not exist. **4. A failed schema section stayed failed for the life of the process (low).** A complete schema is still cached for good. One with unavailable sections is served for SCHEMA_RETRY_AFTER_MS (5 min), then only the failed sections are read again. They are not retried on every call, because a section that timed out would make every call wait for the timeout again. **5. Also:** - `neo4j-driver` is imported dynamically inside the gated factory, so an instance with the gate closed never loads it. The gate test now counts module loads by re-registering the mock per test, because a hoisted vi.mock factory runs only once per file and could not count them. Closed cases assert zero loads in both entrypoints and in startHttpServer. - getDriver() is async now. - /health computes graphAccess per request, as sessions do. - Comments in gate.ts, warmup.ts, tools/index.ts, instructions.ts, http-server.ts and the gate test now state the rule rather than narrating the old leak. Failed first: 13 tests in the working tree, plus 22 in the gate test once module loads were counted: the five closed driver-factory, stdio, HTTP and startHttpServer cases each, "both set > startHttpServer ...", and "/health ... per request". Checked by sabotage: putting back comment deletion fails "a comment splitting a keyword". 207 tests; lint, format, both typechecks and build clean on Node 24. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- README.md | 15 +- SECURITY.md | 2 +- scripts/smoke-guard-timeout.mjs | 6 +- src/clients/neo4j.ts | 159 +++++++++++++------- src/graph/gate.ts | 12 +- src/graph/schema.ts | 46 ++++-- src/graph/warmup.ts | 5 +- src/http-server.ts | 5 +- src/http.ts | 21 ++- src/instructions.ts | 11 +- src/tools/cypher-guard.ts | 144 +++++++++++-------- src/tools/cypher.ts | 24 +++- src/tools/index.ts | 5 +- tests/cypher-guard.test.ts | 248 +++++++++++++++++--------------- tests/cypher-tools.test.ts | 38 ++++- tests/graph-access-gate.test.ts | 90 +++++++++--- tests/graph-schema.test.ts | 56 +++++++- tests/helpers/fake-neo4j.ts | 77 +++++++++- 19 files changed, 656 insertions(+), 310 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0136790..3783272 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ All notable changes to this project are documented here. This project adheres to Schema extraction now uses Neo4j's built-in procedures only (`db.schema.*`, `SHOW INDEXES`/`SHOW CONSTRAINTS` with `db.indexes()`/`db.constraints()` as fallback), because the Reactome graph database image runs Neo4j 4.4 Community without APOC. Each section degrades on its own and is reported as unavailable rather than rendered empty. - The read guard now also refuses `LOAD CSV` (a read clause, so READ mode allows it, and it fetches URLs) and is no longer bypassed by a `//` inside a string literal or by backtick-quoted procedure names. + The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. `NEO4J_DATABASE` now defaults to the server's default database, and `NEO4J_PASSWORD` to none (connect without credentials), rather than `graph.db` and `neo4j`. diff --git a/README.md b/README.md index cb5bf15..8ecefe7 100644 --- a/README.md +++ b/README.md @@ -115,11 +115,16 @@ Optional: | `CYPHER_QUERY_TIMEOUT_MS` | `30000` | server-side transaction timeout for `reactome_cypher_*` queries | **Read-only safeguards.** Every query runs in a READ-mode session, so the -database refuses `CREATE`, `MERGE`, `DELETE`, `SET` and `REMOVE`. `LOAD CSV` and -APOC procedures that can write or fetch from outside the graph are refused -before they reach the database. Each query runs under a server-side timeout, -and rows, row width and total response size are capped. These are guardrails; -the boundary is that graph access is not enabled where the public can reach it. +database refuses `CREATE`, `MERGE`, `DELETE`, `SET` and `REMOVE`. Before a +query reaches the database it is checked against an allowlist: `CALL` only for +the built-in schema procedures (`db.labels`, `db.relationshipTypes`, +`db.propertyKeys`, `db.schema.*`, `db.indexes`, `db.constraints`, +`dbms.components`), `SHOW` only for indexes and constraints, and no APOC, +`LOAD CSV`, `USE` or `TERMINATE`. Each query runs under a server-side timeout; +rows are streamed and reading stops at `max_rows`, with the rest discarded on +the server, and row width and total response size are capped. These are +guardrails; the boundary is that graph access is not enabled where the public +can reach it. ### Running against the Reactome graph database container diff --git a/SECURITY.md b/SECURITY.md index 5c7f7af..99b30da 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -33,7 +33,7 @@ Out of scope (report upstream): - By default the server reads the public Reactome Content and Analysis Services over HTTP and holds no database credentials. - **Graph database access is off unless both `NEO4J_URI` and `MCP_ALLOW_CYPHER=1` are set**, and a publicly reachable instance must never set them. With either missing there are no Cypher tools, no graph schema resource, no mention of either in the server instructions, and no Neo4j driver is ever constructed — `getDriver()` checks the same predicate as every surface, so a surface that forgot to check still could not connect. `tests/graph-access-gate.test.ts` holds this for every entrypoint in every combination of the two variables. -- With graph access on, queries run in READ-mode sessions under a server-side timeout, `LOAD CSV` and APOC procedures that can write or fetch from outside the graph are refused before execution, and output is capped. That is a guardrail against accidents, not a boundary against a hostile caller: an expensive read can still load the database. Use a dedicated database, reachable on loopback only, and configure Neo4j's own authentication and plugin settings accordingly. +- With graph access on, queries run in READ-mode sessions under a server-side timeout, queries are checked against an allowlist before execution (`CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, no APOC, `LOAD CSV`, `USE` or `TERMINATE`), and rows are streamed and cut off at the row cap rather than read whole. That is a guardrail against accidents, not a boundary against a hostile caller: an expensive read can still load the database. Use a dedicated database, reachable on loopback only, and configure Neo4j's own authentication and plugin settings accordingly. - It is **not** self-hardened for internet-facing deployment: it has no authentication, no TLS and no rate limiting of its own. A public deployment must put those at the reverse proxy, and should bind the server to loopback so the proxy is the only way in. With graph access off, a caller reaching the server directly can make the same calls a browser can make against reactome.org, and nothing more. - Tool inputs are bounded: every list argument has an explicit maximum, and `MAX_ANALYSIS_IDENTIFIERS` caps what one call may submit to the Analysis Service. The HTTP transport separately refuses a request body over 100 KiB. - The web demo (`web/mcp-bridge.js`) uses an allow-list CORS policy defaulting to localhost. Do not deploy it publicly without adding authentication. diff --git a/scripts/smoke-guard-timeout.mjs b/scripts/smoke-guard-timeout.mjs index 987cb35..adbd3d9 100644 --- a/scripts/smoke-guard-timeout.mjs +++ b/scripts/smoke-guard-timeout.mjs @@ -5,7 +5,7 @@ // Keep the range modest: range() builds its list before the transaction guard // gets a look in, so a huge one costs the database memory regardless. -import { rejectWriteThroughCalls } from "../dist/tools/cypher-guard.js"; +import { assertReadOnlyQuery } from "../dist/tools/cypher-guard.js"; import { runRead, closeDriver } from "../dist/clients/neo4j.js"; let failures = 0; @@ -15,7 +15,7 @@ for (const query of [ "LOAD CSV FROM 'http://example.invalid/x.csv' AS row RETURN row", ]) { try { - rejectWriteThroughCalls(query); + assertReadOnlyQuery(query); console.log("FAIL: guard did not reject:", query); failures++; } catch (e) { @@ -24,7 +24,7 @@ for (const query of [ } try { - rejectWriteThroughCalls("MATCH (n:Pathway) RETURN n LIMIT 1"); + assertReadOnlyQuery("MATCH (n:Pathway) RETURN n LIMIT 1"); console.log("OK: guard accepts a plain read query"); } catch (e) { console.log("FAIL: guard rejected a plain read query:", e.message); diff --git a/src/clients/neo4j.ts b/src/clients/neo4j.ts index d7f49d8..e272264 100644 --- a/src/clients/neo4j.ts +++ b/src/clients/neo4j.ts @@ -1,18 +1,24 @@ -import neo4j, { type Driver } from "neo4j-driver"; +import type { Driver, Session } from "neo4j-driver"; import { CYPHER_QUERY_TIMEOUT_MS, neo4jConnection } from "../config.js"; import { GRAPH_ACCESS_OFF, isGraphAccessEnabled } from "../graph/gate.js"; import { logger } from "../logger.js"; /** - * The only place in this server that constructs a Neo4j driver. + * The only place in this server that loads `neo4j-driver` or constructs a + * driver. * * `tests/graph-access-gate.test.ts` asserts no other source file imports - * `neo4j-driver`, so every connection comes through `getDriver()` -- and - * `getDriver()` asks the gate itself. A surface that forgot to check the gate - * would still get no driver. + * `neo4j-driver`, so every connection comes through `getDriver()`, and + * `getDriver()` asks the gate itself: a surface that forgot to check the gate + * still gets no driver. The library is imported dynamically, after that + * check, so an instance with graph access off never loads it at all. The type + * import above is erased at compile time. */ -let driverInstance: Driver | null = null; +/** The parts of the library this file uses. */ +type Neo4jModule = Pick; + +let driverPromise: Promise<{ neo4j: Neo4jModule; driver: Driver }> | null = null; function isLoopback(uri: string): boolean { try { @@ -23,53 +29,68 @@ function isLoopback(uri: string): boolean { } } -export function getDriver(): Driver { - // Asked on every call, not only the first: a driver built while the gate - // was open is not a licence to keep using it once it is not. - if (!isGraphAccessEnabled()) throw new Error(GRAPH_ACCESS_OFF); +async function build(): Promise<{ neo4j: Neo4jModule; driver: Driver }> { + const { uri, user, password, database } = neo4jConnection(); + if (!uri) throw new Error(GRAPH_ACCESS_OFF); - if (!driverInstance) { - const { uri, user, password, database } = neo4jConnection(); - if (!uri) throw new Error(GRAPH_ACCESS_OFF); + const neo4j: Neo4jModule = (await import("neo4j-driver")).default; - if (!isLoopback(uri) && !password) { - logger.warn("NEO4J_URI is not a loopback address and NEO4J_PASSWORD is unset", { - hint: "Keep the graph database on loopback, or require authentication.", - }); - } + if (!isLoopback(uri) && !password) { + logger.warn("NEO4J_URI is not a loopback address and NEO4J_PASSWORD is unset", { + hint: "Keep the graph database on loopback, or require authentication.", + }); + } + + const auth = password === undefined ? undefined : neo4j.auth.basic(user, password); + const driver = neo4j.driver(uri, auth, { disableLosslessIntegers: true }); + logger.info("neo4j driver initialized", { database: database ?? "(server default)" }); - const auth = password === undefined ? undefined : neo4j.auth.basic(user, password); - driverInstance = neo4j.driver(uri, auth, { disableLosslessIntegers: true }); - logger.info("neo4j driver initialized", { database: database ?? "(server default)" }); - - // process.once expects a void-returning listener. Handing it an async - // function meant a failed close rejected with nobody listening, which Node - // turns into a hard exit on unhandled rejection. - const onShutdown = () => { - void closeDriver().catch((error: unknown) => { - logger.warn("neo4j driver close failed during shutdown", { - error: error instanceof Error ? error.message : String(error), - }); + // process.once expects a void-returning listener. Handing it an async + // function meant a failed close rejected with nobody listening, which Node + // turns into a hard exit on unhandled rejection. + const onShutdown = () => { + void closeDriver().catch((error: unknown) => { + logger.warn("neo4j driver close failed during shutdown", { + error: error instanceof Error ? error.message : String(error), }); - }; - process.once("SIGINT", onShutdown); - process.once("SIGTERM", onShutdown); - process.once("beforeExit", onShutdown); - } - return driverInstance; + }); + }; + process.once("SIGINT", onShutdown); + process.once("SIGTERM", onShutdown); + process.once("beforeExit", onShutdown); + + return { neo4j, driver }; +} + +async function connection(): Promise<{ neo4j: Neo4jModule; driver: Driver }> { + // Asked on every call, not only the first: a driver built while the gate + // was open is not a licence to keep using it once it is not. + if (!isGraphAccessEnabled()) throw new Error(GRAPH_ACCESS_OFF); + // One promise, so concurrent first callers share one driver. + driverPromise ??= build().catch((error: unknown) => { + driverPromise = null; + throw error; + }); + return driverPromise; +} + +export async function getDriver(): Promise { + return (await connection()).driver; } /** Close and forget the driver, if one was built. Safe to call when none was. */ export async function closeDriver(): Promise { - const driver = driverInstance; - driverInstance = null; - if (driver) await driver.close(); + const pending = driverPromise; + driverPromise = null; + if (!pending) return; + const built = await pending.catch(() => null); + if (built) await built.driver.close(); } // JS `number` always serializes to Float64 over Bolt. Cypher features like // LIMIT / SKIP require an Integer type on the server side, so coerce any // safe-integer number (and bigint) params into the driver's Integer wrapper. -function coerceIntParams(params: Record): Record { +function coerceIntParams(neo4j: Neo4jModule, params: Record) { const out: Record = {}; for (const [k, v] of Object.entries(params)) { if (typeof v === "number" && Number.isInteger(v) && Number.isSafeInteger(v)) { @@ -87,8 +108,21 @@ export interface RunReadOptions { timeoutMs?: number; } +async function readSession(fetchSize?: number): Promise<{ neo4j: Neo4jModule; session: Session }> { + const { neo4j, driver } = await connection(); + const { database } = neo4jConnection(); + const session = driver.session({ + ...(database ? { database } : {}), + defaultAccessMode: neo4j.session.READ, + ...(fetchSize ? { fetchSize } : {}), + }); + return { neo4j, session }; +} + /** - * Run one query in a READ-mode session, under a server-side timeout. + * Run one query in a READ-mode session, under a server-side timeout, and + * return every row. For the schema queries, whose results are bounded by the + * schema; a caller's query goes through `runReadLimited`. * * READ mode makes the database refuse native writes (CREATE, MERGE, DELETE, * SET, REMOVE). The timeout is a transaction timeout: Neo4j terminates the @@ -100,17 +134,46 @@ export async function runRead>( params: Record = {}, options: RunReadOptions = {} ): Promise { - const driver = getDriver(); - const { database } = neo4jConnection(); - const session = driver.session({ - ...(database ? { database } : {}), - defaultAccessMode: neo4j.session.READ, - }); + const { neo4j, session } = await readSession(); const timeout = options.timeoutMs ?? CYPHER_QUERY_TIMEOUT_MS; try { - const result = await session.run(cypher, coerceIntParams(params), { timeout }); + const result = await session.run(cypher, coerceIntParams(neo4j, params), { timeout }); return result.records.map(r => r.toObject() as T); } finally { await session.close(); } } + +/** + * Like `runRead`, but reads at most `maxRows + 1` rows and then stops. + * + * The rows are streamed, in batches no larger than that, and the rest of the + * result is discarded on the server once enough have arrived -- so + * `MATCH (n) RETURN n` reads a handful of nodes into this process rather than + * every node in the graph. The extra row is how `more` is known without + * counting the rest. + */ +export async function runReadLimited>( + cypher: string, + params: Record, + maxRows: number, + options: RunReadOptions = {} +): Promise<{ rows: T[]; more: boolean }> { + const { neo4j, session } = await readSession(Math.min(maxRows + 1, 1000)); + const timeout = options.timeoutMs ?? CYPHER_QUERY_TIMEOUT_MS; + const rows: T[] = []; + let more = false; + try { + const result = session.run(cypher, coerceIntParams(neo4j, params), { timeout }); + for await (const record of result) { + if (rows.length === maxRows) { + more = true; + break; // ends the iteration, which discards the rest of the stream + } + rows.push(record.toObject()); + } + return { rows, more }; + } finally { + await session.close(); + } +} diff --git a/src/graph/gate.ts b/src/graph/gate.ts index 8f21c8e..06d4bb6 100644 --- a/src/graph/gate.ts +++ b/src/graph/gate.ts @@ -9,18 +9,16 @@ * This is the one predicate. Every surface that touches the graph asks it -- * tool registration, the `reactome://graph/schema` resource, the server * instructions, the startup schema warm-up, and `getDriver()` itself, which - * refuses to construct a driver when it is false. Asking it at the driver as - * well as at each surface is deliberate: the last gate this server had was - * consulted in four places, and the HTTP entrypoint -- the one actually - * hosted -- opened a connection on `NEO4J_URI` alone. A door that forgets to - * ask still cannot get a driver. + * refuses to construct a driver (or load the library) when it is false. + * Asking at the driver as well as at each surface means a surface that + * forgets to ask still cannot get a driver. Every entrypoint and factory is + * exercised against it in `tests/graph-access-gate.test.ts`. * * Two variables rather than one, on purpose. A connection string can be set * for many reasons and is easy to inherit from a shared environment file; * it is not consent to publish arbitrary query access. Forgetting the opt-in * costs a missing tool on an internal instance, which is visible and - * harmless. Forgetting it the other way round on a public instance would cost - * the database, and the website in front of it. + * harmless; the gate is designed so that the mistake goes that way. * * Read from the environment on every call rather than captured at import, so * there is one copy of the answer and it cannot go stale. diff --git a/src/graph/schema.ts b/src/graph/schema.ts index 890b6b6..41fce0a 100644 --- a/src/graph/schema.ts +++ b/src/graph/schema.ts @@ -99,7 +99,14 @@ export interface GraphSchema { */ const SCHEMA_FETCH_TIMEOUT_MS = 120_000; -let schemaCache: GraphSchema | null = null; +/** + * How long a schema with unavailable sections is served before those + * sections are tried again. Not on every call: a section that failed by + * timing out would make every call wait for the timeout again. + */ +export const SCHEMA_RETRY_AFTER_MS = 5 * 60_000; + +let schemaCache: { schema: GraphSchema; at: number } | null = null; let schemaPending: Promise | null = null; type Section = keyof Omit; @@ -256,28 +263,38 @@ const EMPTY: Omit = { }; /** - * Fetch the schema, once. Cached in memory after the first call that reads - * anything; concurrent first callers share one fetch. + * Fetch the schema. Cached in memory; concurrent callers share one fetch. + * + * A complete schema is cached for the life of the process. One with + * unavailable sections is served as it is for `SCHEMA_RETRY_AFTER_MS`, and + * then the failed sections -- only those -- are read again, so a section + * that timed out during the startup warm-up is not missing for good. * * A fetch in which **no** section could be read -- the database is down, the * credentials are wrong -- throws and is not cached, so the next call tries - * again instead of serving an empty schema for the life of the process. + * again instead of serving an empty schema. */ export async function fetchGraphSchema(): Promise { - if (schemaCache) return schemaCache; + if (schemaCache) { + const { schema, at } = schemaCache; + if (schema.unavailable.length === 0) return schema; + if (Date.now() - at < SCHEMA_RETRY_AFTER_MS) return schema; + } if (schemaPending) return schemaPending; + const previous = schemaCache?.schema; const start = Date.now(); schemaPending = (async () => { try { - const names = Object.keys(SECTIONS) as Section[]; + const names = previous + ? previous.unavailable.map(u => u.section as Section) + : (Object.keys(SECTIONS) as Section[]); const settled = await Promise.allSettled(names.map(name => SECTIONS[name]())); - const result: GraphSchema = { - fetchedAt: new Date().toISOString(), - ...structuredClone(EMPTY), - unavailable: [], - }; + const result: GraphSchema = previous + ? { ...previous, unavailable: [] } + : { fetchedAt: "", ...structuredClone(EMPTY), unavailable: [] }; + result.fetchedAt = new Date().toISOString(); settled.forEach((outcome, i) => { const name = names[i] as Section; if (outcome.status === "fulfilled") { @@ -287,20 +304,21 @@ export async function fetchGraphSchema(): Promise { } }); - if (result.unavailable.length === names.length) { + if (!previous && result.unavailable.length === names.length) { throw new Error( `Could not read the graph schema: ${result.unavailable[0]?.reason ?? "no section answered"}` ); } - logger.info("graph schema fetched", { + logger.info(previous ? "graph schema sections retried" : "graph schema fetched", { durationMs: Date.now() - start, + sections: names.length, nodeCount: result.stats.nodeCount, labels: Object.keys(result.stats.labels).length, unavailable: result.unavailable.map(u => u.section), }); - schemaCache = result; + schemaCache = { schema: result, at: Date.now() }; return result; } finally { schemaPending = null; diff --git a/src/graph/warmup.ts b/src/graph/warmup.ts index 2439a27..360f4ca 100644 --- a/src/graph/warmup.ts +++ b/src/graph/warmup.ts @@ -12,9 +12,8 @@ import { GRAPH_GROUP, resolveToolGroups } from "../tools/index.js"; * `cypher` group withheld, nothing could read that cache, and warming it would * open a database connection for nobody. * - * The decision lives here rather than at each call site. The previous gate - * was asked by each entrypoint for itself, and the HTTP entrypoint asked the - * wrong question. + * The decision lives here rather than at each call site, so every entrypoint + * makes the same one. */ export function warmGraphSchema(): void { let offered: boolean; diff --git a/src/http-server.ts b/src/http-server.ts index cef71b9..c21a542 100644 --- a/src/http-server.ts +++ b/src/http-server.ts @@ -28,9 +28,8 @@ if (!port) { // past the .catch below and killed the process with a raw stack trace. Dying // was correct; dying without the log line that says why was not. // -// The graph schema warm-up decides for itself whether graph access is on -- -// this entrypoint once decided by NEO4J_URI alone, and opened a connection on -// an instance that had not opted in. +// The graph schema warm-up decides for itself whether graph access is on; +// this entrypoint does not. Promise.resolve() .then(() => startHttpServer(port, MCP_HTTP_HOST)) .then(() => warmGraphSchema()) diff --git a/src/http.ts b/src/http.ts index 788495f..5bcec91 100644 --- a/src/http.ts +++ b/src/http.ts @@ -55,11 +55,18 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro // documentation said the server refuses to start. It does over stdio, // where createServer runs once at boot. It did not here, which is the // transport that is actually deployed. - // - // Also what /health reports as `graphAccess`: whether sessions on this - // instance are offered the Cypher tools, so an operator can see which way - // the gate went without reading the environment. - const graphAccess = resolveToolGroups().includes(GRAPH_GROUP); + resolveToolGroups(); + + // Whether a session opened now would be offered the Cypher tools. Asked per + // request, like the sessions themselves, so /health cannot report a value + // captured at startup that a new session no longer matches. + const graphAccess = (): boolean => { + try { + return resolveToolGroups().includes(GRAPH_GROUP); + } catch { + return false; + } + }; // 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 @@ -233,7 +240,7 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro sessions: sessions.size, contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, - graphAccess, + graphAccess: graphAccess(), }); }); @@ -244,7 +251,7 @@ export function startHttpServer(port: number, host: string = MCP_HTTP_HOST): Pro port, contentService: CONTENT_SERVICE_URL, analysisService: ANALYSIS_SERVICE_URL, - graphAccess, + graphAccess: graphAccess(), }); resolve(http); }); diff --git a/src/instructions.ts b/src/instructions.ts index 901b5cb..be47242 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -96,9 +96,9 @@ This instance can query the Reactome graph database directly. Use it for what th 2. Use \`reactome_cypher_sample\` on a label to see a representative node. 3. Run the query with \`reactome_cypher_query\`: - It runs in a READ-mode session; write clauses are refused by the database. - - \`LOAD CSV\` and APOC procedures that write or fetch from outside the graph are refused before execution. + - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...), and \`SHOW\` only for indexes and constraints. APOC, \`LOAD CSV\` and \`USE\` are refused. - A server-side timeout ends long queries. Always include a \`LIMIT\`, and anchor on an indexed property (for example \`stId\` or \`dbId\`) rather than scanning a whole label. - - Return specific properties (\`RETURN p.stId, p.displayName\`) rather than whole nodes; rows and response size are capped. + - Return specific properties (\`RETURN p.stId, p.displayName\`) rather than whole nodes; reading stops at \`max_rows\`, and response size is capped. **When not to use it:** lookups by ID or name, pathway hierarchies and enrichment — the REST tools are cached, cheaper, and format the answer for you. `.trim(); @@ -107,10 +107,9 @@ This instance can query the Reactome graph database directly. Use it for what th * The instructions a client reads on connecting. * * **It describes the groups this instance actually registered, and nothing - * else.** The last time these two facts were allowed to drift apart, the - * server told every client to call `reactome_cypher_query` on an instance - * that had declined to register it. That is invisible to any check which - * asks the server what it can do, because the answer *is* the claim. + * else.** A server describing tools it has not registered is invisible to + * any check which asks the server what it can do, because the answer *is* + * the claim. * * So the category list is built from the same `ToolGroup` values that drive * registration, and `tests/tool-groups.test.ts` asserts a restricted diff --git a/src/tools/cypher-guard.ts b/src/tools/cypher-guard.ts index 84fd45c..a6094b0 100644 --- a/src/tools/cypher-guard.ts +++ b/src/tools/cypher-guard.ts @@ -1,74 +1,102 @@ -// Guard against queries that get around a READ-mode session. +// What a `reactome_cypher_query` may ask the database to do. // -// Neo4j's READ access mode rejects native write clauses (CREATE/MERGE/DELETE/ -// SET/REMOVE). It does not reject everything that reaches outside the graph: +// Neo4j's READ access mode refuses native writes (CREATE/MERGE/DELETE/SET/ +// REMOVE). It does not refuse everything that reaches outside the graph or +// costs the server: procedures can open their own transactions, read server +// configuration, kill other queries or sleep; LOAD CSV is a read clause and +// fetches URLs; USE switches database, the system database included; SHOW +// lists transactions, users and procedures. // -// - APOC procedures open their own transactions and can write, load remote -// URLs or touch the filesystem regardless of the calling session's mode. -// The Reactome graph database container ships without APOC, but a -// database this server is pointed at might not, so they are refused here. -// - LOAD CSV is a *read* clause, so READ mode allows it -- and it fetches -// arbitrary URLs and, depending on server config, local files. +// So this is an allowlist, not a list of known offenders. A query may CALL +// only the read-only built-ins the schema tooling itself uses, may SHOW only +// indexes and constraints, and may not name APOC, use LOAD CSV, USE or +// TERMINATE at all. // -// This is a guardrail, not a security boundary. A determined caller can hide -// intent in ways a pattern cannot see, and the real boundary is that graph -// access is never enabled on a publicly reachable instance (see -// src/graph/gate.ts), plus Neo4j's own RBAC and plugin configuration. +// It is a guardrail, not the security boundary. A pattern cannot fully parse +// Cypher, and it errs towards refusing -- a string literal containing +// "apoc." is refused too. The boundary is that graph access is never enabled +// on a publicly reachable instance (src/graph/gate.ts), plus Neo4j's own +// configuration. -const WRITE_THROUGH_PATTERNS: Array<{ pattern: RegExp; label: string }> = [ - { - pattern: /\bapoc\.cypher\.(runWrite|doIt)\b/i, - label: "apoc.cypher.runWrite / apoc.cypher.doIt", - }, - { - pattern: /\bapoc\.periodic\.(iterate|commit|submit|countdown|repeat)\b/i, - label: "apoc.periodic.*", - }, - { - pattern: /\bapoc\.(create|merge|refactor)\.[a-z]\w*/i, - label: "apoc.create.* / apoc.merge.* / apoc.refactor.*", - }, - { pattern: /\bapoc\.nodes\.delete\b/i, label: "apoc.nodes.delete" }, - { - pattern: /\bapoc\.(load|import|export)\.[a-z]\w*/i, - label: "apoc.load.* / apoc.import.* / apoc.export.*", - }, - { pattern: /\bapoc\.trigger\.[a-z]\w*/i, label: "apoc.trigger.*" }, - { pattern: /\bload\s+csv\b/i, label: "LOAD CSV" }, -]; +/** Procedures a query may CALL, lower-cased. Everything else is refused. */ +const ALLOWED_PROCEDURES = new Set([ + "db.labels", + "db.relationshiptypes", + "db.propertykeys", + "db.schema.nodetypeproperties", + "db.schema.reltypeproperties", + "db.schema.visualization", + "db.indexes", + "db.constraints", + "dbms.components", +]); + +export class QueryRejected extends Error { + constructor(public readonly reason: string) { + super( + `Query rejected: ${reason}. This server runs read-only queries against the graph and allows ` + + `CALL only for ${[...ALLOWED_PROCEDURES].join(", ")}, and SHOW only for indexes and constraints.` + ); + this.name = "QueryRejected"; + } +} /** - * The forms of the query to test. All of them, not one: each normalisation - * closes a way of hiding a call and could open another. + * The forms of the query to check. All of them, because each normalisation + * closes one way of hiding a token and could open another: * - * - The raw text. Comments used to be stripped first, but the stripper knows - * nothing of string literals, so a `'//'` inside a string swallowed the rest - * of the line -- call included. Testing the raw text as well means a - * commented-out call is refused too, which is a harmless false positive. - * - Comments removed, so `apoc.load/* x *\/.json` cannot split a name. - * - Backticks removed and whitespace around dots closed up, so - * `` `apoc`.`load`.`json` `` and `apoc . load . json` read as what they are. + * - the raw text -- a comment stripper knows nothing of string literals, so a + * `'//'` inside a string would otherwise swallow the rest of its line; + * - comments replaced by a **space** -- a comment separates tokens, so + * `LOAD/**\/CSV` is `LOAD CSV` (deleting it made `LOADCSV`, which matched + * nothing); + * - each of those with backticks removed and whitespace around dots closed + * up, so `` `dbms`.`listConfig` `` and `dbms . listConfig` read as one name. */ -function candidates(query: string): string[] { - const noComments = query.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/[^\n]*/g, ""); +function forms(query: string): string[] { + const spaced = query.replace(/\/\*[\s\S]*?\*\//g, " ").replace(/\/\/[^\n]*/g, " "); const canonical = (text: string) => text.replace(/`/g, "").replace(/\s*\.\s*/g, "."); - return [query, noComments, canonical(query), canonical(noComments)]; + return [query, spaced, canonical(query), canonical(spaced)]; } -export class WriteThroughRejected extends Error { - constructor(public readonly matchedLabel: string) { - super( - `Query rejected: ${matchedLabel} can bypass READ-mode sessions and write to, or load data from, outside the graph. This server blocks it. If you have a legitimate need, run the query outside this server against a read replica.` - ); - this.name = "WriteThroughRejected"; +// A keyword at a token boundary: not part of a longer name, and not a +// property access like `n.use`. +const keyword = (word: string) => new RegExp(`(? pattern.test(form))) { - throw new WriteThroughRejected(label); + for (const match of form.matchAll(new RegExp(`${keyword("call").source}`, "gi"))) { + const rest = form.slice((match.index ?? 0) + match[0].length); + // CALL { ... } is a subquery, not a procedure. + if (/^\s*\{/.test(rest)) continue; + const name = /^\s*([A-Za-z_][\w.]*)/.exec(rest)?.[1]?.toLowerCase(); + if (!name || !ALLOWED_PROCEDURES.has(name)) { + return `CALL ${name ?? "(unrecognised target)"} is not allowed`; } } + return undefined; +} + +/** Throw `QueryRejected` unless every form of the query passes. */ +export function assertReadOnlyQuery(query: string): void { + for (const form of forms(query)) { + const reason = check(form); + if (reason) throw new QueryRejected(reason); + } } diff --git a/src/tools/cypher.ts b/src/tools/cypher.ts index 6cc0ca1..642692b 100644 --- a/src/tools/cypher.ts +++ b/src/tools/cypher.ts @@ -1,10 +1,10 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; -import { runRead } from "../clients/neo4j.js"; +import { runRead, runReadLimited } from "../clients/neo4j.js"; import { fetchGraphSchema } from "../graph/schema.js"; import { formatGraphSchemaMarkdown } from "../graph/format-schema.js"; import { logger } from "../logger.js"; -import { rejectWriteThroughCalls } from "./cypher-guard.js"; +import { assertReadOnlyQuery } from "./cypher-guard.js"; import { isGraphAccessEnabled } from "../graph/gate.js"; const MAX_QUERY_CHARS = 50_000; @@ -82,7 +82,7 @@ export function registerCypherTools(server: McpServer) { server.tool( "reactome_cypher_query", - "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. LOAD CSV and APOC procedures that can write or fetch from outside the graph are rejected before execution. A server-side timeout terminates long queries. Row count, per-row size and total response size are capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", + "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.schema.*, db.indexes, ...); APOC, other procedures, LOAD CSV, USE and SHOW other than indexes/constraints are rejected before execution. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", { query: z .string() @@ -123,14 +123,24 @@ export function registerCypherTools(server: McpServer) { ), }, async ({ query, params, max_rows, max_row_chars, max_total_chars }) => { - rejectWriteThroughCalls(query); + assertReadOnlyQuery(query); logger.info("cypher_query", { chars: query.length, max_rows }); - const rows = await runRead>(query, params ?? {}); + // Streamed and cut off at max_rows + 1 -- the cap is applied while + // reading, not after the whole result has been pulled into memory. + const { rows, more } = await runReadLimited>( + query, + params ?? {}, + max_rows + ); const { output, stats } = capCypherRows(rows, max_rows, max_row_chars, max_total_chars); const notes: string[] = []; - notes.push(`**Rows returned:** ${output.length} of ${rows.length}`); - if (stats.rowCountTruncated) notes.push(`(row-count cap: ${max_rows})`); + notes.push(`**Rows returned:** ${output.length}`); + if (more) { + notes.push( + `(more rows exist; reading stopped at the max_rows cap of ${max_rows} -- add a LIMIT, ORDER BY or a narrower MATCH)` + ); + } if (stats.rowsWidthTruncated > 0) notes.push(`**Wide rows summarized:** ${stats.rowsWidthTruncated}`); if (stats.stoppedAtTotal) diff --git a/src/tools/index.ts b/src/tools/index.ts index 7bfca28..63a29d5 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -102,9 +102,8 @@ export function permittedGroups(groups: readonly ToolGroup[]): ToolGroup[] { * deployment names the groups it means to publish. * * **This is a registration switch, not a documentation one.** A tool that is - * described nowhere but still answers is exactly the divergence that cost - * this repo a public Cypher surface and a server advertising tools it had not - * registered: what a server *offers* must be the same fact as what it *says*, + * described nowhere but still answers, or one described but not registered, + * is a divergence: what a server *offers* must be the same fact as what it *says*, * and the only way to be sure is for the unwanted tool not to exist on the * instance. * diff --git a/tests/cypher-guard.test.ts b/tests/cypher-guard.test.ts index 98d1278..8bf0706 100644 --- a/tests/cypher-guard.test.ts +++ b/tests/cypher-guard.test.ts @@ -1,128 +1,152 @@ import { describe, it, expect } from "vitest"; -import { rejectWriteThroughCalls, WriteThroughRejected } from "../src/tools/cypher-guard.js"; - -describe("rejectWriteThroughCalls", () => { - it("accepts a plain read query", () => { - expect(() => rejectWriteThroughCalls("MATCH (n:Pathway) RETURN n LIMIT 10")).not.toThrow(); - }); - - it("accepts queries that mention apoc in a string literal if no procedure call", () => { - // This is a query that reads only — "apoc" appears inside a string and - // the regex uses word-boundary procedure shape, so it won't match. - expect(() => - rejectWriteThroughCalls(`MATCH (n) WHERE n.note = 'see apoc docs' RETURN n`) - ).not.toThrow(); - }); - - it("rejects apoc.cypher.runWrite", () => { - expect(() => - rejectWriteThroughCalls("CALL apoc.cypher.runWrite('CREATE (n) RETURN n', {})") - ).toThrow(WriteThroughRejected); - }); - - it("rejects apoc.cypher.doIt regardless of case", () => { - expect(() => rejectWriteThroughCalls("CALL APOC.CYPHER.DOIT('CREATE (n)', {})")).toThrow( - WriteThroughRejected - ); - }); - - it("rejects apoc.periodic.iterate", () => { - expect(() => - rejectWriteThroughCalls( - "CALL apoc.periodic.iterate('MATCH (n) RETURN n', 'SET n.flag = 1', {})" - ) - ).toThrow(WriteThroughRejected); +import { assertReadOnlyQuery, QueryRejected } from "../src/tools/cypher-guard.js"; + +/** + * The query guard is an allowlist. A query may CALL only the read-only + * built-ins the schema tooling uses; every other procedure, every APOC name, + * LOAD CSV, USE, and any SHOW other than indexes and constraints is refused + * before it reaches the database. + * + * It is a guardrail, not the boundary -- the boundary is that graph access is + * off on any publicly reachable instance -- but a denylist is a list of the + * ways somebody already thought of, and the review found eleven it missed. + */ + +const accepted = (query: string) => expect(() => assertReadOnlyQuery(query)).not.toThrow(); +const refused = (query: string) => expect(() => assertReadOnlyQuery(query)).toThrow(QueryRejected); + +describe("assertReadOnlyQuery: what is allowed", () => { + it("accepts plain read queries", () => { + accepted("MATCH (n:Pathway) RETURN n LIMIT 10"); + accepted("MATCH (p:Pathway {stId: $id})-[:hasEvent]->(e) RETURN e.displayName LIMIT 25"); + accepted("MATCH (n) WITH n LIMIT 5 CALL { WITH n RETURN n.dbId AS id } RETURN id"); + }); + + it("accepts apoc, call, use and load as words inside data", () => { + accepted(`MATCH (n) WHERE n.note = 'see apoc docs' RETURN n`); + accepted("MATCH (n) RETURN n.csv, n.load, n.use LIMIT 1"); + }); + + it("accepts the read-only built-in procedures", () => { + for (const q of [ + "CALL db.labels() YIELD label RETURN label", + "CALL db.relationshipTypes()", + "CALL db.propertyKeys()", + "CALL db.schema.nodeTypeProperties()", + "CALL db.schema.relTypeProperties()", + "CALL db.schema.visualization()", + "CALL db.indexes()", + "CALL db.constraints()", + "CALL dbms.components() YIELD name, versions, edition RETURN *", + "call DB.LABELS()", + "SHOW INDEXES", + "SHOW ALL INDEXES YIELD name RETURN name", + "SHOW CONSTRAINTS", + "SHOW UNIQUE CONSTRAINTS", + ]) { + accepted(q); + } }); +}); - it("rejects apoc.create.node", () => { - expect(() => - rejectWriteThroughCalls("CALL apoc.create.node(['X'], {}) YIELD node RETURN node") - ).toThrow(WriteThroughRejected); +describe("assertReadOnlyQuery: refuses", () => { + it("every APOC name, called or not", () => { + for (const q of [ + "CALL apoc.cypher.runWrite('CREATE (n) RETURN n', {})", + "CALL APOC.CYPHER.DOIT('CREATE (n)', {})", + "CALL apoc.periodic.iterate('MATCH (n) RETURN n', 'SET n.flag = 1', {})", + "CALL apoc.create.node(['X'], {}) YIELD node RETURN node", + "CALL apoc.refactor.mergeNodes([n1, n2])", + "CALL apoc.load.json('http://evil.example/data') YIELD value RETURN value", + "CALL apoc.export.csv.all('/tmp/x.csv', {})", + "CALL apoc.nodes.delete([1,2,3], 100)", + "CALL apoc.meta.schema() YIELD value RETURN value", + "MATCH (n) CALL apoc.path.subgraphAll(n, {}) YIELD nodes RETURN nodes", + // The review's list: none of these was on the old denylist. + "CALL apoc.cypher.runMany('CREATE (n);', {})", + "CALL apoc.cypher.runFile('x.cypher')", + "CALL apoc.cypher.runSchema('CREATE INDEX FOR (n:X) ON (n.y)', {})", + "CALL apoc.cypher.parallel('MATCH (n) RETURN n', {}, 'x')", + "CALL apoc.schema.assert({}, {})", + "CALL apoc.util.sleep(600000)", + "CALL apoc.custom.asProcedure('x', 'CREATE (n)')", + "CALL apoc.systemdb.execute('SHOW USERS')", + "CALL apoc.lock.nodes([n])", + "RETURN apoc.text.join(['a'], ',')", + ]) { + refused(q); + } + }); + + it("dbms and db procedures outside the allowlist", () => { + for (const q of [ + "CALL dbms.killQuery('query-1')", + "CALL dbms.listQueries()", + "CALL dbms.listConfig()", + "CALL dbms.security.createUser('x', 'y', false)", + "CALL db.createLabel('X')", + "CALL db.index.fulltext.createNodeIndex('x', ['X'], ['y'])", + "CALL db.clearQueryCaches()", + "CALL db.stats.retrieve('GRAPH COUNTS')", + // A known name with a suffix is a different procedure. + "CALL db.labelsAndMore()", + ]) { + refused(q); + } + }); + + it("USE, which switches database -- system included", () => { + refused("USE system SHOW USERS"); + refused("use neo4j MATCH (n) RETURN n LIMIT 1"); + refused("CALL { USE system RETURN 1 AS x } RETURN x"); + }); + + it("SHOW and TERMINATE other than indexes and constraints", () => { + refused("SHOW TRANSACTIONS"); + refused("SHOW PROCEDURES"); + refused("SHOW DATABASES"); + refused("TERMINATE TRANSACTIONS 'neo4j-transaction-1'"); + }); + + it("LOAD CSV, which READ mode allows and which fetches URLs and files", () => { + refused("LOAD CSV FROM 'http://evil.example/x.csv' AS row RETURN row"); + refused("load csv with headers from 'file:///etc/passwd' as r return r"); }); +}); - it("rejects apoc.refactor.mergeNodes", () => { - expect(() => rejectWriteThroughCalls("CALL apoc.refactor.mergeNodes([n1, n2])")).toThrow( - WriteThroughRejected - ); +describe("assertReadOnlyQuery: is not bypassed by", () => { + it("a comment splitting a keyword", () => { + // Comments used to be deleted outright, so LOAD/**/CSV became LOADCSV and + // matched nothing. A comment separates tokens; it is read as a space. + refused("LOAD/**/CSV FROM 'http://x' AS r RETURN r"); + refused("LOAD/* a */CSV FROM 'http://x' AS r RETURN r"); + refused("LOAD//\nCSV FROM 'http://x' AS r RETURN r"); + refused("CALL/**/dbms.listConfig()"); + refused("CALL/**/apoc.util.sleep(1000)"); + refused("USE/**/system SHOW USERS"); + refused("SHOW/**/TRANSACTIONS"); }); - it("rejects apoc.load.json (SSRF risk)", () => { - expect(() => - rejectWriteThroughCalls( - "CALL apoc.load.json('http://evil.example/data') YIELD value RETURN value" - ) - ).toThrow(WriteThroughRejected); + it("a comment splitting a procedure name", () => { + refused("CALL apoc.load/* x */.json('http://evil.example/') YIELD value"); + refused("CALL dbms/* x */.listConfig()"); }); - it("rejects apoc.export.csv.all", () => { - expect(() => rejectWriteThroughCalls("CALL apoc.export.csv.all('/tmp/x.csv', {})")).toThrow( - WriteThroughRejected - ); + it("backtick-quoting or spacing the name", () => { + refused("CALL `apoc`.`load`.`json`('http://evil.example/') YIELD value"); + refused("CALL `dbms`.`listConfig`()"); + refused("CALL dbms . listConfig ( )"); + refused("CALL `dbms.listConfig`()"); }); - it("rejects apoc.nodes.delete", () => { - expect(() => rejectWriteThroughCalls("CALL apoc.nodes.delete([1,2,3], 100)")).toThrow( - WriteThroughRejected + it("a '//' inside a string literal", () => { + refused( + "WITH 'http://x' AS u CALL apoc.load.json('http://evil.example/') YIELD value RETURN value" ); + refused("WITH 'a//' AS s CALL dbms.listConfig() YIELD name RETURN name"); }); - it("rejects a bad call even inside a comment", () => { - // Comments used to be stripped before matching, and the stripper does not - // know about string literals -- so a '//' inside a string swallowed the - // rest of the line, call included (see the next test). Refusing a - // commented-out write is a harmless false positive; the bypass was not. - expect(() => - rejectWriteThroughCalls("// CALL apoc.cypher.runWrite('...')\nMATCH (n) RETURN n") - ).toThrow(WriteThroughRejected); - }); - - it("is not bypassed by a '//' inside a string literal", () => { - expect(() => - rejectWriteThroughCalls( - "WITH 'http://x' AS u CALL apoc.load.json('http://evil.example/') YIELD value RETURN value" - ) - ).toThrow(WriteThroughRejected); - }); - - it("is not bypassed by a block comment splitting the procedure name", () => { - expect(() => - rejectWriteThroughCalls("CALL apoc.load/* x */.json('http://evil.example/') YIELD value") - ).toThrow(WriteThroughRejected); - }); - - it("is not bypassed by backtick-quoting the procedure name", () => { - expect(() => - rejectWriteThroughCalls("CALL `apoc`.`load`.`json`('http://evil.example/') YIELD value") - ).toThrow(WriteThroughRejected); - }); - - it("rejects LOAD CSV, which READ mode allows and which fetches URLs and files", () => { - expect(() => - rejectWriteThroughCalls("LOAD CSV FROM 'http://evil.example/x.csv' AS row RETURN row") - ).toThrow(WriteThroughRejected); - expect(() => - rejectWriteThroughCalls("load csv with headers from 'file:///etc/passwd' as r return r") - ).toThrow(WriteThroughRejected); - }); - - it("does not mistake a property called csv for LOAD CSV", () => { - expect(() => rejectWriteThroughCalls("MATCH (n) RETURN n.csv, n.load LIMIT 1")).not.toThrow(); - }); - - it("does not strip comments when the bad call is in active code", () => { - expect(() => - rejectWriteThroughCalls("// harmless comment\nCALL apoc.periodic.commit('DELETE n', {})") - ).toThrow(WriteThroughRejected); - }); - - it("accepts read-only apoc procedures (apoc.meta.schema, apoc.path.*)", () => { - expect(() => - rejectWriteThroughCalls("CALL apoc.meta.schema() YIELD value RETURN value") - ).not.toThrow(); - expect(() => - rejectWriteThroughCalls( - "MATCH (n) CALL apoc.path.subgraphAll(n, {}) YIELD nodes RETURN nodes" - ) - ).not.toThrow(); + it("hiding the call inside a comment (a commented-out call is refused too)", () => { + refused("// CALL apoc.cypher.runWrite('...')\nMATCH (n) RETURN n"); }); }); diff --git a/tests/cypher-tools.test.ts b/tests/cypher-tools.test.ts index fbb42eb..f938441 100644 --- a/tests/cypher-tools.test.ts +++ b/tests/cypher-tools.test.ts @@ -66,18 +66,46 @@ describe("reactome_cypher_* tools", () => { fakeServer.invoke("reactome_cypher_query", { query: "CALL apoc.periodic.iterate('MATCH (n) RETURN n', 'DETACH DELETE n', {})", }) - ).rejects.toThrow(/bypass READ-mode/); + ).rejects.toThrow(/Query rejected/); expect(fake.drivers).toHaveLength(0); expect(fake.sessions).toHaveLength(0); }); - it("caps the rows it returns", async () => { - fake.respond = () => Array.from({ length: 500 }, (_, i) => ({ i })); + it("stops reading at max_rows + 1 rather than materialising the whole result", async () => { + // `MATCH (n) RETURN n` on the Reactome graph is ~3M nodes. Capping after + // the fact pulled every one of them into this process first. + let produced = 0; + fake.respond = function* () { + for (let i = 0; i < 3_000_000; i++) { + produced++; + yield { i }; + } + }; const result = await fakeServer.invoke("reactome_cypher_query", { - query: "UNWIND range(1, 500) AS i RETURN i", + query: "MATCH (n) RETURN n", max_rows: 10, }); - expect(textOf(result)).toContain("**Rows returned:** 10 of 500"); + + const run = fake.sessions[0]?.runs[0]; + expect(run?.pulled).toBe(11); + expect(run?.cancelled).toBe(true); + expect(produced).toBeLessThanOrEqual(11); + // And the driver is asked to fetch in batches no bigger than that. + expect(Number(fake.sessions[0]?.config.fetchSize)).toBeLessThanOrEqual(11); + + expect(textOf(result)).toContain("**Rows returned:** 10"); + expect(textOf(result)).toMatch(/more rows exist/); + }); + + it("reports an exact count, and reads to the end, when the result fits", async () => { + fake.respond = () => [{ i: 1 }, { i: 2 }, { i: 3 }]; + const result = await fakeServer.invoke("reactome_cypher_query", { + query: "UNWIND [1,2,3] AS i RETURN i", + max_rows: 10, + }); + expect(textOf(result)).toContain("**Rows returned:** 3"); + expect(textOf(result)).not.toMatch(/more rows exist/); + expect(fake.sessions[0]?.runs[0]?.cancelled).toBe(false); }); it("refuses a max_rows above the cap, before running anything", () => { diff --git a/tests/graph-access-gate.test.ts b/tests/graph-access-gate.test.ts index 6a88233..6a991d3 100644 --- a/tests/graph-access-gate.test.ts +++ b/tests/graph-access-gate.test.ts @@ -12,23 +12,30 @@ import type { FakeNeo4jState } from "./helpers/fake-neo4j.js"; * an arbitrary query from the internet can take the database -- and the * website in front of it -- down. * - * The gate is `NEO4J_URI` set AND `MCP_ALLOW_CYPHER=1`. The last time this - * server had a gate, it was consulted in four places and one of them -- the - * HTTP entrypoint, which is the one actually hosted -- still opened a - * connection on `NEO4J_URI` alone. So these tests do not check the predicate - * once and trust the call sites. They drive **every** surface, through every - * entrypoint and factory, in every combination of the two variables, and - * count driver constructions on a fake `neo4j-driver` that cannot open a - * socket. Each surface is asserted in both directions: "absent when closed" - * alone would pass against a build that never offers graph access at all. + * The gate is `NEO4J_URI` set AND `MCP_ALLOW_CYPHER=1`. These tests do not + * check the predicate once and trust the call sites: they drive **every** + * surface, through every entrypoint and factory, in every combination of the + * two variables, and count both loads of `neo4j-driver` and driver + * constructions on a fake that cannot open a socket. Each surface is asserted + * in both directions: "absent when closed" alone would pass against a build + * that never offers graph access at all. A new entrypoint belongs here. */ const fake = vi.hoisted((): FakeNeo4jState => ({ drivers: [], sessions: [], respond: () => [] })); -vi.mock("neo4j-driver", async () => { - const { fakeNeo4jModule } = await import("./helpers/fake-neo4j.js"); - return fakeNeo4jModule(fake); -}); +/** + * Registered per test, after `vi.resetModules()`, rather than once with + * `vi.mock`: a hoisted mock's factory runs once per file, so it could not + * count whether a given entrypoint loaded `neo4j-driver` at all. Re-registered + * like this, the factory runs on the first import after each reset, and + * `fake.moduleLoads` says whether anything imported it. + */ +function mockDriverModule() { + vi.doMock("neo4j-driver", async () => { + const { fakeNeo4jModule } = await import("./helpers/fake-neo4j.js"); + return fakeNeo4jModule(fake); + }); +} // The stdio entrypoint connects a transport on import. This one talks to // nothing, so importing `src/index.ts` runs the real startup path -- server @@ -97,6 +104,7 @@ function resetFake() { fake.drivers.length = 0; fake.sessions.length = 0; fake.respond = () => []; + fake.moduleLoads = 0; } /** Let fire-and-forget startup work (the schema warm-up) run to completion. */ @@ -129,6 +137,7 @@ describe.each(CASES)("graph access gate: $name", ({ env, open }) => { applyEnv(env); resetFake(); vi.resetModules(); + mockDriverModule(); }); afterEach(() => { @@ -211,12 +220,14 @@ describe.each(CASES)("graph access gate: $name", ({ env, open }) => { it(`the driver factory ${open ? "constructs a driver" : "refuses to construct one"}`, async () => { const { getDriver, runRead } = await import("../src/clients/neo4j.js"); if (open) { - expect(getDriver()).toBeDefined(); + expect(await getDriver()).toBeDefined(); expect(fake.drivers).toHaveLength(1); } else { - expect(() => getDriver()).toThrow(/MCP_ALLOW_CYPHER/); + await expect(getDriver()).rejects.toThrow(/MCP_ALLOW_CYPHER/); await expect(runRead("RETURN 1")).rejects.toThrow(/MCP_ALLOW_CYPHER/); expect(fake.drivers).toHaveLength(0); + // Refused before the library is even loaded. + expect(fake.moduleLoads).toBe(0); } }); @@ -224,6 +235,8 @@ describe.each(CASES)("graph access gate: $name", ({ env, open }) => { await import("../src/index.js"); await settle(); expect(fake.drivers).toHaveLength(open ? 1 : 0); + // A closed instance never loads neo4j-driver at all. + expect(fake.moduleLoads).toBe(open ? 1 : 0); if (open) { // The warm-up ran, read-only, on the dedicated graph. expect(fake.drivers[0]?.uri).toBe(URI); @@ -233,14 +246,15 @@ describe.each(CASES)("graph access gate: $name", ({ env, open }) => { }); it(`the HTTP entrypoint ${open ? "warms the schema over one driver" : "never constructs a driver"}`, async () => { - // src/http-server.ts is the entrypoint a hosted instance runs, and the one - // the previous gate missed. Its transport is stubbed so nothing binds; - // everything it does after the server starts is real. + // src/http-server.ts is the entrypoint a hosted instance runs. Its + // transport is stubbed so nothing binds; everything it does after the + // server starts is real. vi.doMock("../src/http.js", () => ({ startHttpServer: vi.fn(async () => ({})) })); process.env.MCP_HTTP_PORT = "4999"; await import("../src/http-server.js"); await settle(); expect(fake.drivers).toHaveLength(open ? 1 : 0); + expect(fake.moduleLoads).toBe(open ? 1 : 0); }); it(`startHttpServer sessions ${open ? "offer" : "withhold"} graph access, and construct no driver`, async () => { @@ -303,8 +317,9 @@ describe.each(CASES)("graph access gate: $name", ({ env, open }) => { } // An operator can see which way the gate went without reading the env. expect(health.graphAccess).toBe(open); - // Serving sessions is not a reason to connect either. + // Serving sessions is not a reason to connect, or to load the driver. expect(fake.drivers).toHaveLength(0); + expect(fake.moduleLoads).toBe(0); } finally { await new Promise(resolve => http.close(resolve)); } @@ -316,6 +331,7 @@ describe("graph access gate: open, but the cypher group withheld", () => { applyEnv({ NEO4J_URI: URI, MCP_ALLOW_CYPHER: "1", MCP_TOOL_GROUPS: "search,pathway" }); resetFake(); vi.resetModules(); + mockDriverModule(); }); afterEach(() => { @@ -367,3 +383,39 @@ describe("graph access gate: structure", () => { expect(pkg.dependencies["neo4j-driver"]).toBeDefined(); }); }); + +describe("graph access gate: /health reports the gate as it is now", () => { + beforeEach(() => { + applyEnv({}); + resetFake(); + vi.resetModules(); + mockDriverModule(); + }); + + afterEach(() => { + restoreEnv(); + vi.resetModules(); + }); + + it("follows the environment per request, as sessions do", async () => { + // Sessions are built per request and ask the gate each time, so a value + // captured at startup could disagree with what a new session is offered. + const { startHttpServer } = await import("../src/http.js"); + const http = await startHttpServer(0, "127.0.0.1"); + try { + const { port } = http.address() as AddressInfo; + const health = async () => + ((await (await fetch(`http://127.0.0.1:${port}/health`)).json()) as Record) + .graphAccess; + + expect(await health()).toBe(false); + process.env.NEO4J_URI = URI; + process.env.MCP_ALLOW_CYPHER = "1"; + expect(await health()).toBe(true); + delete process.env.MCP_ALLOW_CYPHER; + expect(await health()).toBe(false); + } finally { + await new Promise(resolve => http.close(resolve)); + } + }); +}); diff --git a/tests/graph-schema.test.ts b/tests/graph-schema.test.ts index 0eddd26..b5a8669 100644 --- a/tests/graph-schema.test.ts +++ b/tests/graph-schema.test.ts @@ -19,7 +19,11 @@ vi.mock("neo4j-driver", async () => { }); import { queriesRun } from "./helpers/fake-neo4j.js"; -import { fetchGraphSchema, _resetGraphSchemaCache } from "../src/graph/schema.js"; +import { + fetchGraphSchema, + _resetGraphSchemaCache, + SCHEMA_RETRY_AFTER_MS, +} from "../src/graph/schema.js"; import { formatGraphSchemaMarkdown } from "../src/graph/format-schema.js"; import { closeDriver } from "../src/clients/neo4j.js"; @@ -212,6 +216,56 @@ describe("fetchGraphSchema on Neo4j 4.4 Community without APOC", () => { expect(md).toContain("nodeTypeProperties"); }); + it("retries a failed section on a later fetch rather than caching the failure for good", async () => { + // A section that timed out during the startup warm-up used to stay + // "unavailable" for the life of the process. + vi.useFakeTimers({ toFake: ["Date"] }); + try { + let failing = true; + fake.respond = community44({ + "db.schema.nodeTypeProperties": () => { + if (failing) throw new Error("The transaction has been terminated (timeout)"); + return [ + { + nodeType: ":`Pathway`", + nodeLabels: ["Pathway"], + propertyName: "stId", + propertyTypes: ["String"], + mandatory: true, + }, + ]; + }, + }); + + const first = await fetchGraphSchema(); + expect(first.unavailable.map(u => u.section)).toEqual(["nodeTypeProperties"]); + + // Straight away: served from cache, not retried on every call. + const before = queriesRun(fake).length; + await fetchGraphSchema(); + expect(queriesRun(fake).length).toBe(before); + + // Later: only the failed section is read again, and it now succeeds. + failing = false; + vi.setSystemTime(Date.now() + SCHEMA_RETRY_AFTER_MS + 1); + const later = await fetchGraphSchema(); + expect(later.unavailable).toEqual([]); + expect(later.nodeTypeProperties).toHaveLength(1); + expect(later.stats.labels.Pathway).toBe(2_000); + const rerun = queriesRun(fake).slice(before); + expect(rerun.length).toBeGreaterThan(0); + expect(rerun.every(q => q.includes("db.schema.nodeTypeProperties"))).toBe(true); + + // Complete now, so cached for good. + vi.setSystemTime(Date.now() + SCHEMA_RETRY_AFTER_MS * 10); + const settledCount = queriesRun(fake).length; + expect(await fetchGraphSchema()).toBe(later); + expect(queriesRun(fake).length).toBe(settledCount); + } finally { + vi.useRealTimers(); + } + }); + it("refuses rather than caching an empty schema when nothing answers", async () => { fake.respond = () => { throw new Error("Could not perform discovery. No routing servers available."); diff --git a/tests/helpers/fake-neo4j.ts b/tests/helpers/fake-neo4j.ts index eaadaa4..6210517 100644 --- a/tests/helpers/fake-neo4j.ts +++ b/tests/helpers/fake-neo4j.ts @@ -14,6 +14,10 @@ export interface FakeRun { query: string; params: Record; config: Record | undefined; + /** Records handed to the caller so far. */ + pulled: number; + /** The caller stopped iterating early, which on a real driver discards the rest. */ + cancelled: boolean; } export interface FakeSession { @@ -22,21 +26,27 @@ export interface FakeSession { closed: boolean; } +/** Rows may be an array, or any iterable -- a generator stands in for a huge result. */ +export type FakeRows = Iterable; + export interface FakeNeo4jState { drivers: Array<{ uri: string; config: Record }>; sessions: FakeSession[]; /** Rows for a query; throw to simulate a database error. */ - respond: (query: string, params: Record) => unknown[] | Promise; + respond: (query: string, params: Record) => FakeRows | Promise; + /** How many times the module itself was loaded (set by fakeNeo4jModule). */ + moduleLoads?: number; } export function newFakeNeo4jState(): FakeNeo4jState { - return { drivers: [], sessions: [], respond: () => [] }; + return { drivers: [], sessions: [], respond: () => [], moduleLoads: 0 }; } export function resetFakeNeo4j(state: FakeNeo4jState): void { state.drivers.length = 0; state.sessions.length = 0; state.respond = () => []; + state.moduleLoads = 0; } /** All query texts run so far, across every session, in order. */ @@ -44,7 +54,56 @@ export function queriesRun(state: FakeNeo4jState): string[] { return state.sessions.flatMap(s => s.runs.map(r => r.query)); } +const toRecord = (row: unknown) => ({ toObject: () => row }); + +/** + * Like the driver's Result: awaitable for every record at once, or + * async-iterable one record at a time, where `return()` -- what `break` in a + * `for await` calls -- stops the stream. + */ +function fakeResult(state: FakeNeo4jState, run: FakeRun) { + let rows: Promise | undefined; + const produce = () => + (rows ??= Promise.resolve().then(() => state.respond(run.query, run.params))); + + return { + then( + onFulfilled?: (value: { records: Array<{ toObject: () => unknown }> }) => A, + onRejected?: (reason: unknown) => B + ) { + return produce() + .then(all => { + const records: Array<{ toObject: () => unknown }> = []; + for (const row of all) { + run.pulled++; + records.push(toRecord(row)); + } + return { records }; + }) + .then(onFulfilled, onRejected); + }, + [Symbol.asyncIterator]() { + let it: Iterator | undefined; + return { + async next() { + it ??= (await produce())[Symbol.iterator](); + const step = it.next(); + if (step.done) return { done: true as const, value: undefined }; + run.pulled++; + return { done: false as const, value: toRecord(step.value) }; + }, + async return() { + run.cancelled = true; + it?.return?.(); + return { done: true as const, value: undefined }; + }, + }; + }, + }; +} + export function fakeNeo4jModule(state: FakeNeo4jState) { + state.moduleLoads = (state.moduleLoads ?? 0) + 1; const neo4j = { driver(uri: string, _auth: unknown, config: Record = {}) { state.drivers.push({ uri, config }); @@ -53,16 +112,20 @@ export function fakeNeo4jModule(state: FakeNeo4jState) { const session: FakeSession = { config: sessionConfig, runs: [], closed: false }; state.sessions.push(session); return { - async run( + run( query: string, params: Record = {}, txConfig?: Record ) { - session.runs.push({ query, params, config: txConfig }); - const rows = await state.respond(query, params); - return { - records: rows.map(row => ({ toObject: () => row })), + const run: FakeRun = { + query, + params, + config: txConfig, + pulled: 0, + cancelled: false, }; + session.runs.push(run); + return fakeResult(state, run); }, async close() { session.closed = true; From 91ec0fb740f594795bd0976859b4b3b3fb13a419 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 14 Oct 2026 01:28:33 +0000 Subject: [PATCH 3/5] Check queries on one tokenisation instead of separate regex passes Re-review of b768d24 found the guard still bypassable, because regex passes over the raw text do not compose. A `/*` inside a `//` comment or a string literal made one pass see a block comment that Neo4j does not, which hid text from it. All three of these passed every form, while Neo4j ran LOAD CSV, USE and apoc.*: // /*\nLOAD/**/CSV ... // /*\nUSE/**/otherdb ... // /*\nRETURN apoc/**/.cypher.runFirstColumnSingle(...) forms()/check() are replaced by one left-to-right scanner. It reads: - single- and double-quoted strings with backslash escapes; their contents are dropped; - backtick names, with `` as a literal backtick, unquoted; - // and /* */ comments, which act as separators; - identifiers, numbers and punctuation. Separators are everything Unicode calls white space, plus every control and format character, so U+001C-U+001F included. A number is only its leading digits, so `1CALL` reads as a number followed by CALL. A query containing U+0085, U+2028 or U+2029 is refused, because where a // comment ends at one of those is exactly the kind of disagreement with the lexer that hides text. An unterminated string, comment or backtick name is refused. The rules are then checked on tokens: - an identifier `apoc` followed by `.` is refused; - LOAD CSV, USE, TERMINATE and PERIODIC are refused, as are CREATE, MERGE, SET, DELETE, REMOVE, DETACH, DROP, GRANT, DENY, REVOKE, ALTER, START, STOP and RENAME -- but only when used as keywords, not as a property, label, parameter or map key; - SHOW is allowed only for indexes and constraints; - CALL must be followed by a dotted name, compared after unquoting, that is on the allowlist. So CALL `db`.`labels`() is now allowed, which fixes that false refusal; Neo4j accepts it, checked read-only against the Release 97 container. CALL { ... } and CALL ( ... ) subqueries are refused. The tool description and the instructions now say that fulltext procedures and CALL subqueries are refused. Failed first against b768d24 (each test written before the code): - the comment opener inside a line comment or a string; - separators and run-together tokens; - unterminated input; - text inside comments and strings must be ignored; - write and admin keywords; - CALL subqueries; - the read-only built-ins, which failed on the backtick-quoted CALL; - plain read queries, which failed on keywords used as property and map names; - the line-separator case; - the description test. Every earlier bypass test is kept. A commented-out call is now accepted, because Neo4j ignores it -- that test changed direction on purpose. 215 tests; lint, format, both typechecks and build clean on Node 24. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- README.md | 7 +- SECURITY.md | 2 +- src/instructions.ts | 2 +- src/tools/cypher-guard.ts | 298 +++++++++++++++++++++++++++++-------- src/tools/cypher.ts | 2 +- tests/cypher-guard.test.ts | 82 +++++++++- tests/cypher-tools.test.ts | 7 + 8 files changed, 333 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3783272..ad8ff4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ All notable changes to this project are documented here. This project adheres to Schema extraction now uses Neo4j's built-in procedures only (`db.schema.*`, `SHOW INDEXES`/`SHOW CONSTRAINTS` with `db.indexes()`/`db.constraints()` as fallback), because the Reactome graph database image runs Neo4j 4.4 Community without APOC. Each section degrades on its own and is reported as unavailable rather than rendered empty. - The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. + The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). The check runs on one left-to-right tokenisation (strings, backtick names, line and block comments, Unicode separators), so a comment opener inside a comment or a string can no longer hide text, `CALL \`db\`.\`labels\`()` is allowed, and CALL subqueries and write/admin keywords are refused. Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. `NEO4J_DATABASE` now defaults to the server's default database, and `NEO4J_PASSWORD` to none (connect without credentials), rather than `graph.db` and `neo4j`. diff --git a/README.md b/README.md index 8ecefe7..584f7c7 100644 --- a/README.md +++ b/README.md @@ -119,8 +119,11 @@ database refuses `CREATE`, `MERGE`, `DELETE`, `SET` and `REMOVE`. Before a query reaches the database it is checked against an allowlist: `CALL` only for the built-in schema procedures (`db.labels`, `db.relationshipTypes`, `db.propertyKeys`, `db.schema.*`, `db.indexes`, `db.constraints`, -`dbms.components`), `SHOW` only for indexes and constraints, and no APOC, -`LOAD CSV`, `USE` or `TERMINATE`. Each query runs under a server-side timeout; +`dbms.components`) -- not fulltext procedures, and no `CALL { ... }` subqueries +-- `SHOW` only for indexes and constraints, and no APOC, `LOAD CSV`, `USE`, +`TERMINATE` or write/admin clause. The check runs on the query as Neo4j tokenises +it (comments, string literals and quoted names read as such), and anything it +cannot finish reading is refused. Each query runs under a server-side timeout; rows are streamed and reading stops at `max_rows`, with the rest discarded on the server, and row width and total response size are capped. These are guardrails; the boundary is that graph access is not enabled where the public diff --git a/SECURITY.md b/SECURITY.md index 99b30da..2346bc4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -33,7 +33,7 @@ Out of scope (report upstream): - By default the server reads the public Reactome Content and Analysis Services over HTTP and holds no database credentials. - **Graph database access is off unless both `NEO4J_URI` and `MCP_ALLOW_CYPHER=1` are set**, and a publicly reachable instance must never set them. With either missing there are no Cypher tools, no graph schema resource, no mention of either in the server instructions, and no Neo4j driver is ever constructed — `getDriver()` checks the same predicate as every surface, so a surface that forgot to check still could not connect. `tests/graph-access-gate.test.ts` holds this for every entrypoint in every combination of the two variables. -- With graph access on, queries run in READ-mode sessions under a server-side timeout, queries are checked against an allowlist before execution (`CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, no APOC, `LOAD CSV`, `USE` or `TERMINATE`), and rows are streamed and cut off at the row cap rather than read whole. That is a guardrail against accidents, not a boundary against a hostile caller: an expensive read can still load the database. Use a dedicated database, reachable on loopback only, and configure Neo4j's own authentication and plugin settings accordingly. +- With graph access on, queries run in READ-mode sessions under a server-side timeout, queries are checked against an allowlist before execution (`CALL` only for the built-in schema procedures and no `CALL` subqueries, `SHOW` only for indexes and constraints, no APOC, `LOAD CSV`, `USE`, `TERMINATE` or write/admin clause), checked on a tokenisation of the query rather than its raw text,, and rows are streamed and cut off at the row cap rather than read whole. That is a guardrail against accidents, not a boundary against a hostile caller: an expensive read can still load the database. Use a dedicated database, reachable on loopback only, and configure Neo4j's own authentication and plugin settings accordingly. - It is **not** self-hardened for internet-facing deployment: it has no authentication, no TLS and no rate limiting of its own. A public deployment must put those at the reverse proxy, and should bind the server to loopback so the proxy is the only way in. With graph access off, a caller reaching the server directly can make the same calls a browser can make against reactome.org, and nothing more. - Tool inputs are bounded: every list argument has an explicit maximum, and `MAX_ANALYSIS_IDENTIFIERS` caps what one call may submit to the Analysis Service. The HTTP transport separately refuses a request body over 100 KiB. - The web demo (`web/mcp-bridge.js`) uses an allow-list CORS policy defaulting to localhost. Do not deploy it publicly without adding authentication. diff --git a/src/instructions.ts b/src/instructions.ts index be47242..c3abc16 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -96,7 +96,7 @@ This instance can query the Reactome graph database directly. Use it for what th 2. Use \`reactome_cypher_sample\` on a label to see a representative node. 3. Run the query with \`reactome_cypher_query\`: - It runs in a READ-mode session; write clauses are refused by the database. - - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...), and \`SHOW\` only for indexes and constraints. APOC, \`LOAD CSV\` and \`USE\` are refused. + - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...), and \`SHOW\` only for indexes and constraints. Fulltext procedures, \`CALL { ... }\` subqueries, APOC, \`LOAD CSV\` and \`USE\` are refused. - A server-side timeout ends long queries. Always include a \`LIMIT\`, and anchor on an indexed property (for example \`stId\` or \`dbId\`) rather than scanning a whole label. - Return specific properties (\`RETURN p.stId, p.displayName\`) rather than whole nodes; reading stops at \`max_rows\`, and response size is capped. diff --git a/src/tools/cypher-guard.ts b/src/tools/cypher-guard.ts index a6094b0..d43d529 100644 --- a/src/tools/cypher-guard.ts +++ b/src/tools/cypher-guard.ts @@ -1,22 +1,33 @@ // What a `reactome_cypher_query` may ask the database to do. // -// Neo4j's READ access mode refuses native writes (CREATE/MERGE/DELETE/SET/ -// REMOVE). It does not refuse everything that reaches outside the graph or -// costs the server: procedures can open their own transactions, read server -// configuration, kill other queries or sleep; LOAD CSV is a read clause and -// fetches URLs; USE switches database, the system database included; SHOW -// lists transactions, users and procedures. +// Neo4j's READ access mode refuses native writes. It does not refuse +// everything that reaches outside the graph or costs the server: procedures +// can read server configuration, kill other queries or sleep; LOAD CSV is a +// read clause and fetches URLs; USE switches database, the system database +// included; SHOW lists transactions, users and procedures. // -// So this is an allowlist, not a list of known offenders. A query may CALL -// only the read-only built-ins the schema tooling itself uses, may SHOW only -// indexes and constraints, and may not name APOC, use LOAD CSV, USE or -// TERMINATE at all. +// So this is an allowlist, applied to the query as Neo4j will read it. One +// left-to-right scan splits the text into tokens the way the Cypher lexer +// does -- string literals, backtick-quoted names, line and block comments, +// identifiers, numbers, punctuation -- and the rules are checked on those +// tokens. Separate regex passes over the raw text did not compose: a `/*` +// inside a `//` comment or a string literal made one pass see a block comment +// that Neo4j does not, and hide the text after it. // -// It is a guardrail, not the security boundary. A pattern cannot fully parse -// Cypher, and it errs towards refusing -- a string literal containing -// "apoc." is refused too. The boundary is that graph access is never enabled -// on a publicly reachable instance (src/graph/gate.ts), plus Neo4j's own -// configuration. +// Rules: +// - CALL only for the built-in schema procedures below, compared by name +// after backticks are removed; no CALL subqueries. +// - SHOW only for indexes and constraints. +// - No identifier `apoc` followed by `.` -- no APOC procedure or function. +// - No LOAD CSV, USE, TERMINATE, PERIODIC COMMIT, and no write or +// administration keyword (CREATE, MERGE, SET, DELETE, REMOVE, DETACH, +// DROP, GRANT, DENY, REVOKE, ALTER, START, STOP, RENAME). +// - Text the scanner cannot finish reading -- an unterminated string, +// comment or quoted name -- is refused. +// +// It is a guardrail, not the security boundary, and it errs towards refusing. +// The boundary is that graph access is never enabled on a publicly reachable +// instance (src/graph/gate.ts), plus Neo4j's own configuration. /** Procedures a query may CALL, lower-cased. Everything else is refused. */ const ALLOWED_PROCEDURES = new Set([ @@ -31,72 +42,239 @@ const ALLOWED_PROCEDURES = new Set([ "dbms.components", ]); +/** Keywords refused wherever they appear as a keyword. */ +const FORBIDDEN_KEYWORDS = new Set([ + "use", + "terminate", + "periodic", + "create", + "merge", + "set", + "delete", + "remove", + "detach", + "drop", + "grant", + "deny", + "revoke", + "alter", + "start", + "stop", + "rename", +]); + +/** Words that may sit between SHOW and INDEXES / CONSTRAINTS. */ +const SHOW_MODIFIERS = new Set([ + "all", + "btree", + "fulltext", + "lookup", + "text", + "range", + "point", + "vector", + "unique", + "uniqueness", + "existence", + "exist", + "exists", + "node", + "relationship", + "rel", + "key", + "property", +]); +const SHOW_TARGETS = new Set(["index", "indexes", "constraint", "constraints"]); + export class QueryRejected extends Error { constructor(public readonly reason: string) { super( `Query rejected: ${reason}. This server runs read-only queries against the graph and allows ` + - `CALL only for ${[...ALLOWED_PROCEDURES].join(", ")}, and SHOW only for indexes and constraints.` + `CALL only for ${[...ALLOWED_PROCEDURES].join(", ")} (no CALL subqueries), ` + + `and SHOW only for indexes and constraints.` ); this.name = "QueryRejected"; } } +type Token = + | { kind: "word"; text: string } // an unquoted identifier or keyword + | { kind: "quoted"; text: string } // a backtick-quoted name, unquoted + | { kind: "string" } // a string literal; its contents are not kept + | { kind: "number" } + | { kind: "punct"; text: string }; + +const IDENTIFIER_START = /[\p{ID_Start}_]/u; +const IDENTIFIER_PART = /[\p{ID_Continue}_]/u; + /** - * The forms of the query to check. All of them, because each normalisation - * closes one way of hiding a token and could open another: - * - * - the raw text -- a comment stripper knows nothing of string literals, so a - * `'//'` inside a string would otherwise swallow the rest of its line; - * - comments replaced by a **space** -- a comment separates tokens, so - * `LOAD/**\/CSV` is `LOAD CSV` (deleting it made `LOADCSV`, which matched - * nothing); - * - each of those with backticks removed and whitespace around dots closed - * up, so `` `dbms`.`listConfig` `` and `dbms . listConfig` read as one name. + * Whitespace as Cypher reads it, and then some: everything Unicode calls + * white space, every control and format character (U+001C-U+001F included), + * and NEL. Treating more as a separator than Neo4j does can only split a + * token the guard then refuses; treating less could join two. */ -function forms(query: string): string[] { - const spaced = query.replace(/\/\*[\s\S]*?\*\//g, " ").replace(/\/\/[^\n]*/g, " "); - const canonical = (text: string) => text.replace(/`/g, "").replace(/\s*\.\s*/g, "."); - return [query, spaced, canonical(query), canonical(spaced)]; -} +const SEPARATOR = /[\p{White_Space}\p{Cc}\p{Cf}\p{Zs}\p{Zl}\p{Zp}]/u; -// A keyword at a token boundary: not part of a longer name, and not a -// property access like `n.use`. -const keyword = (word: string) => new RegExp(`(? { + throw new QueryRejected(`unterminated ${what}`); + }; - for (const match of form.matchAll(new RegExp(`${keyword("show").source}`, "gi"))) { - const rest = form.slice((match.index ?? 0) + match[0].length); - if (!SHOW_ALLOWED.test(rest)) return "SHOW is allowed only for indexes and constraints"; + while (i < chars.length) { + const c = chars[i] as string; + const next = chars[i + 1]; + + if (SEPARATOR.test(c)) { + i++; + continue; + } + + // Line comment: to the end of the line. + if (c === "/" && next === "/") { + i += 2; + while (i < chars.length && chars[i] !== "\n" && chars[i] !== "\r") i++; + continue; + } + + // Block comment: to the first */. Nothing inside it means anything. + if (c === "/" && next === "*") { + i += 2; + while (i < chars.length && !(chars[i] === "*" && chars[i + 1] === "/")) i++; + if (i >= chars.length) unterminated("comment"); + i += 2; + continue; + } + + // String literal, single or double quoted, with backslash escapes. + if (c === "'" || c === '"') { + i++; + while (i < chars.length && chars[i] !== c) i += chars[i] === "\\" ? 2 : 1; + if (i >= chars.length) unterminated("string literal"); + i++; + tokens.push({ kind: "string" }); + continue; + } + + // Backtick-quoted name; a doubled backtick is a literal one. + if (c === "`") { + let text = ""; + i++; + for (;;) { + if (i >= chars.length) unterminated("quoted name"); + if (chars[i] === "`") { + if (chars[i + 1] === "`") { + text += "`"; + i += 2; + continue; + } + i++; + break; + } + text += chars[i]; + i++; + } + tokens.push({ kind: "quoted", text }); + continue; + } + + // A number is its leading digits. Whatever follows is read as a new + // token, so `1CALL` is a number and then CALL. + if (/[0-9]/.test(c)) { + while (i < chars.length && /[0-9]/.test(chars[i] as string)) i++; + tokens.push({ kind: "number" }); + continue; + } + + if (IDENTIFIER_START.test(c)) { + let text = ""; + while (i < chars.length && IDENTIFIER_PART.test(chars[i] as string)) { + text += chars[i]; + i++; + } + tokens.push({ kind: "word", text }); + continue; + } + + tokens.push({ kind: "punct", text: c }); + i++; } - for (const match of form.matchAll(new RegExp(`${keyword("call").source}`, "gi"))) { - const rest = form.slice((match.index ?? 0) + match[0].length); - // CALL { ... } is a subquery, not a procedure. - if (/^\s*\{/.test(rest)) continue; - const name = /^\s*([A-Za-z_][\w.]*)/.exec(rest)?.[1]?.toLowerCase(); - if (!name || !ALLOWED_PROCEDURES.has(name)) { - return `CALL ${name ?? "(unrecognised target)"} is not allowed`; + return tokens; +} + +const isName = (t: Token | undefined): t is Extract => + t?.kind === "word" || t?.kind === "quoted"; +const isPunct = (t: Token | undefined, text: string) => t?.kind === "punct" && t.text === text; +const lower = (t: Token | undefined) => (isName(t) ? t.text.toLowerCase() : undefined); + +/** + * Whether the word at `i` is used as a keyword rather than as a name: not a + * property or label (`n.set`, `:Create`), a parameter (`$use`) or a map key + * (`{delete: 1}`). + */ +function isKeyword(tokens: Token[], i: number): boolean { + if (tokens[i]?.kind !== "word") return false; + const before = tokens[i - 1]; + if (isPunct(before, ".") || isPunct(before, ":") || isPunct(before, "$")) return false; + if (isPunct(tokens[i + 1], ":")) return false; + return true; +} + +function check(tokens: Token[]): string | undefined { + for (let i = 0; i < tokens.length; i++) { + const token = tokens[i]; + const word = lower(token); + if (word === undefined) continue; + + // Any APOC name, procedure or function, whatever it is called from. + if (word === "apoc" && isPunct(tokens[i + 1], ".")) { + return "APOC procedures and functions are not allowed"; + } + + if (!isKeyword(tokens, i)) continue; + + if (word === "load" && lower(tokens[i + 1]) === "csv") return "LOAD CSV is not allowed"; + if (FORBIDDEN_KEYWORDS.has(word)) return `${word.toUpperCase()} is not allowed`; + + if (word === "show") { + let j = i + 1; + while (SHOW_MODIFIERS.has(lower(tokens[j]) ?? "")) j++; + if (!SHOW_TARGETS.has(lower(tokens[j]) ?? "")) { + return "SHOW is allowed only for indexes and constraints"; + } + } + + if (word === "call") { + const parts: string[] = []; + let j = i + 1; + while (isName(tokens[j])) { + parts.push((tokens[j] as { text: string }).text.toLowerCase()); + if (!isPunct(tokens[j + 1], ".")) break; + j += 2; + } + if (parts.length === 0) return "CALL subqueries are not allowed"; + const name = parts.join("."); + if (!ALLOWED_PROCEDURES.has(name)) return `CALL ${name} is not allowed`; } } return undefined; } -/** Throw `QueryRejected` unless every form of the query passes. */ +/** Throw `QueryRejected` unless the query passes every rule. */ export function assertReadOnlyQuery(query: string): void { - for (const form of forms(query)) { - const reason = check(form); - if (reason) throw new QueryRejected(reason); - } + const reason = check(tokenize(query)); + if (reason) throw new QueryRejected(reason); } diff --git a/src/tools/cypher.ts b/src/tools/cypher.ts index 642692b..dc99b3b 100644 --- a/src/tools/cypher.ts +++ b/src/tools/cypher.ts @@ -82,7 +82,7 @@ export function registerCypherTools(server: McpServer) { server.tool( "reactome_cypher_query", - "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.schema.*, db.indexes, ...); APOC, other procedures, LOAD CSV, USE and SHOW other than indexes/constraints are rejected before execution. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", + "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.relationshipTypes, db.propertyKeys, db.schema.*, db.indexes, db.constraints, dbms.components). Rejected before execution: every other procedure (fulltext procedures such as db.index.fulltext.queryNodes included), CALL subqueries (CALL { ... }), APOC, LOAD CSV, USE, SHOW other than indexes/constraints, and write or admin clauses. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", { query: z .string() diff --git a/tests/cypher-guard.test.ts b/tests/cypher-guard.test.ts index 8bf0706..9597c76 100644 --- a/tests/cypher-guard.test.ts +++ b/tests/cypher-guard.test.ts @@ -19,7 +19,9 @@ describe("assertReadOnlyQuery: what is allowed", () => { it("accepts plain read queries", () => { accepted("MATCH (n:Pathway) RETURN n LIMIT 10"); accepted("MATCH (p:Pathway {stId: $id})-[:hasEvent]->(e) RETURN e.displayName LIMIT 25"); - accepted("MATCH (n) WITH n LIMIT 5 CALL { WITH n RETURN n.dbId AS id } RETURN id"); + accepted("MATCH (n) WHERE n.displayName STARTS WITH 'Signal' RETURN n.stId LIMIT 5"); + accepted("MATCH (n:Pathway) RETURN n.create, n.set, n.delete, n.use LIMIT 1"); + accepted("RETURN {set: 1, delete: 2, call: 3} AS m"); }); it("accepts apoc, call, use and load as words inside data", () => { @@ -39,6 +41,9 @@ describe("assertReadOnlyQuery: what is allowed", () => { "CALL db.constraints()", "CALL dbms.components() YIELD name, versions, edition RETURN *", "call DB.LABELS()", + // Backtick-quoted parts are the same name once unquoted. + "CALL `db`.`labels`() YIELD label RETURN label", + "CALL db.`schema`.nodeTypeProperties()", "SHOW INDEXES", "SHOW ALL INDEXES YIELD name RETURN name", "SHOW CONSTRAINTS", @@ -146,7 +151,78 @@ describe("assertReadOnlyQuery: is not bypassed by", () => { refused("WITH 'a//' AS s CALL dbms.listConfig() YIELD name RETURN name"); }); - it("hiding the call inside a comment (a commented-out call is refused too)", () => { - refused("// CALL apoc.cypher.runWrite('...')\nMATCH (n) RETURN n"); + it("a comment opener inside a line comment or a string", () => { + // Regex passes over the whole text treated `/*` inside a `//` comment as + // opening a block comment, and swallowed what Neo4j actually runs. + refused("// /*\nLOAD/**/CSV FROM 'http://x/' AS r RETURN r"); + refused("// /*\nUSE/**/otherdb MATCH (n) RETURN n"); + refused( + "// /*\nRETURN apoc/**/.cypher.runFirstColumnSingle('CA'+'LL dbms.listQueries() YIELD queryId RETURN queryId', {})" + ); + refused("RETURN '/*' AS a // */\nLOAD CSV FROM 'http://x/' AS r RETURN r"); + refused('RETURN "/*" AS a\nCALL dbms.listConfig() // */'); + refused("RETURN '\\'/*' AS a\nCALL dbms.listConfig()"); + }); + + it("separators Cypher treats as whitespace, and tokens run together", () => { + refused("LOAD\u001CCSV FROM 'http://x/' AS r RETURN r"); + refused("USE\u001Cdb MATCH (n) RETURN n"); + refused("RETURN apoc\u001C.text.join(['a'], ',')"); + refused("MATCH (n) RETURN n LIMIT 1CALL dbms.listConfig()"); + refused("LOAD\u00A0CSV FROM 'http://x/' AS r RETURN r"); + refused("LOAD\u2003CSV FROM 'http://x/' AS r RETURN r"); + refused("LOAD\u0085CSV FROM 'http://x/' AS r RETURN r"); + }); + + it("a line separator a comment might or might not end at", () => { + refused("// x\u2028LOAD CSV FROM 'http://x/' AS r RETURN r"); + refused("// it's\u2029MATCH (n) RETURN n"); + }); + + it("anything it cannot finish reading", () => { + refused("MATCH (n) RETURN 'unterminated"); + refused('MATCH (n) RETURN "unterminated'); + refused("MATCH (n) RETURN n /* unterminated"); + refused("MATCH (n) RETURN n.`unterminated"); + }); +}); + +describe("assertReadOnlyQuery: ignores what Neo4j ignores", () => { + it("text inside comments and string literals", () => { + accepted("// CALL apoc.cypher.runWrite('...')\nMATCH (n) RETURN n LIMIT 1"); + accepted("/* LOAD CSV FROM 'x' */ MATCH (n) RETURN n LIMIT 1"); + accepted("MATCH (n) WHERE n.name = 'USE system; CALL dbms.listConfig()' RETURN n LIMIT 1"); + accepted('MATCH (n) WHERE n.name = "it\\"s apoc.x" RETURN n LIMIT 1'); + }); +}); + +describe("assertReadOnlyQuery: refuses writes and admin as keywords", () => { + it("write and administration clauses", () => { + for (const q of [ + "CREATE (n:X) RETURN n", + "MATCH (n) SET n.x = 1", + "MATCH (n) DETACH DELETE n", + "MATCH (n) REMOVE n.x", + "MERGE (n:X {id: 1})", + "DROP INDEX x", + "GRANT ROLE admin TO bob", + "DENY READ {*} ON GRAPH * TO bob", + "REVOKE ROLE admin FROM bob", + "ALTER USER bob SET PASSWORD 'x'", + "START DATABASE neo4j", + "STOP DATABASE neo4j", + "USING PERIODIC COMMIT LOAD CSV FROM 'x' AS r RETURN r", + ]) { + refused(q); + } + }); + + it("CALL subqueries, which are not needed to read and can run in transactions", () => { + refused("MATCH (n) WITH n LIMIT 5 CALL { WITH n RETURN n.dbId AS id } RETURN id"); + refused("MATCH (n) CALL (n) { RETURN n.dbId AS id } RETURN id"); + }); + + it("fulltext procedures, which are not on the allowlist", () => { + refused("CALL db.index.fulltext.queryNodes('x', 'y')"); }); }); diff --git a/tests/cypher-tools.test.ts b/tests/cypher-tools.test.ts index f938441..113acdc 100644 --- a/tests/cypher-tools.test.ts +++ b/tests/cypher-tools.test.ts @@ -48,6 +48,13 @@ describe("reactome_cypher_* tools", () => { ]); }); + it("says in its description what the guard refuses", () => { + const description = fakeServer.tools.get("reactome_cypher_query")?.description ?? ""; + expect(description).toMatch(/fulltext/i); + expect(description).toContain("CALL subqueries"); + expect(description).toContain("LOAD CSV"); + }); + it("runs a query in a READ session, under the configured timeout", async () => { fake.respond = () => [{ stId: "R-HSA-109582" }]; const result = await fakeServer.invoke("reactome_cypher_query", { From 46ede92cd862fd6d5aa86bdc91f671115f660799 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 14 Oct 2026 01:34:34 +0000 Subject: [PATCH 4/5] Read number literals whole, refuse plugin names however quoted Review of 91ec0fb found one bypass class and several false refusals. Each change below has a test that was written first and failed against 91ec0fb. **Numbers (high).** The scanner read only the leading digits of a number. In `1e0CALL dbms.listQueries()` that left `e0CALL` as one name, so the CALL was never seen; Neo4j reads `1e0` and then CALL. Number literals are now read the way Cypher defines them: - decimal, with an optional fraction and an optional signed exponent; - a leading `.5`; - hexadecimal `0x...` and octal `0o...`. A name or backtick running straight on from a number is refused (`1e0CALL`, `1E0LOAD`, `0x1LOAD`, `0o7LOAD`, `1.5e3DELETE`, `.5e1DETACH`, `1_000`, `1e+5CALL`), so where the split falls no longer decides whether a keyword is seen. Range and arithmetic forms still pass: `[*1..3]`, `[0..2]`, `1E-3`, `0x1F`. **Plugin names (low).** `` `apoc.text.join`(...) `` was accepted. A backtick name that starts with `apoc.` or `gds.`, and `apoc` or `gds` followed by `.`, are now refused, for gds as well as apoc. **False refusals.** - START and STOP are refused only when followed by DATABASE, so `(start:Pathway) ... RETURN start.displayName` and `AS start` pass. - USE is refused only where a query part begins: at the start, or after EXPLAIN/PROFILE, UNION [ALL], `;` or `{`. That is where Neo4j allows the clause, and CALL subqueries are already refused. So `AS use` and `(use:Pathway)` pass, while `EXPLAIN USE`, `UNION ALL USE` and `EXISTS { USE ... }` are refused. - SET and SHOW stay refused even as variable names, because they can begin a clause in too many places to tell apart safely. This is now stated in the guard header, the tool description and the README. **Fulltext queries.** `db.index.fulltext.queryNodes` and `queryRelationships` are added to the allowlist; SHOW PROCEDURES on the Release 97 container (Neo4j 4.4.48) lists both as mode READ. Creating and dropping a fulltext index (mode SCHEMA) are still refused. The description, instructions, README and CHANGELOG say so. Failed first against 91ec0fb: - "are read whole, and refused when a name runs straight on from one"; - "refuses apoc and gds however the name is quoted"; - "accepts start and stop as variables; refuses only START/STOP DATABASE"; - "accepts use as a variable; refuses USE where a query or a UNION part starts"; - "fulltext procedures that create or drop, though querying is allowed"; - the tool-description test, which now requires the fulltext query procedures to be named. 220 tests; lint, format, both typechecks and build clean on Node 24. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- README.md | 8 ++-- src/instructions.ts | 2 +- src/tools/cypher-guard.ts | 92 ++++++++++++++++++++++++++++++++------ src/tools/cypher.ts | 2 +- tests/cypher-guard.test.ts | 81 ++++++++++++++++++++++++++++++++- tests/cypher-tools.test.ts | 2 +- 7 files changed, 166 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad8ff4a..9496052 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ All notable changes to this project are documented here. This project adheres to Schema extraction now uses Neo4j's built-in procedures only (`db.schema.*`, `SHOW INDEXES`/`SHOW CONSTRAINTS` with `db.indexes()`/`db.constraints()` as fallback), because the Reactome graph database image runs Neo4j 4.4 Community without APOC. Each section degrades on its own and is reported as unavailable rather than rendered empty. - The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). The check runs on one left-to-right tokenisation (strings, backtick names, line and block comments, Unicode separators), so a comment opener inside a comment or a string can no longer hide text, `CALL \`db\`.\`labels\`()` is allowed, and CALL subqueries and write/admin keywords are refused. Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. + The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). The check runs on one left-to-right tokenisation (strings, backtick names, line and block comments, Unicode separators), so a comment opener inside a comment or a string can no longer hide text, `CALL \`db\`.\`labels\`()` is allowed, and CALL subqueries and write/admin keywords are refused. Number literals are read whole (`1e0CALL` no longer hides a CALL) and a name running on from one is refused; `gds` is refused like `apoc`; `start`/`stop`/`use` work as variable names; the fulltext query procedures are allowed. Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. `NEO4J_DATABASE` now defaults to the server's default database, and `NEO4J_PASSWORD` to none (connect without credentials), rather than `graph.db` and `neo4j`. diff --git a/README.md b/README.md index 584f7c7..6380889 100644 --- a/README.md +++ b/README.md @@ -119,9 +119,11 @@ database refuses `CREATE`, `MERGE`, `DELETE`, `SET` and `REMOVE`. Before a query reaches the database it is checked against an allowlist: `CALL` only for the built-in schema procedures (`db.labels`, `db.relationshipTypes`, `db.propertyKeys`, `db.schema.*`, `db.indexes`, `db.constraints`, -`dbms.components`) -- not fulltext procedures, and no `CALL { ... }` subqueries --- `SHOW` only for indexes and constraints, and no APOC, `LOAD CSV`, `USE`, -`TERMINATE` or write/admin clause. The check runs on the query as Neo4j tokenises +`dbms.components`) and the fulltext query procedures +(`db.index.fulltext.queryNodes`, `db.index.fulltext.queryRelationships`), no +`CALL { ... }` subqueries, `SHOW` only for indexes and constraints, and no +APOC or GDS name, `LOAD CSV`, `USE`, `TERMINATE` or write/admin clause. `SET` +and `SHOW` cannot be used as variable names. The check runs on the query as Neo4j tokenises it (comments, string literals and quoted names read as such), and anything it cannot finish reading is refused. Each query runs under a server-side timeout; rows are streamed and reading stops at `max_rows`, with the rest discarded on diff --git a/src/instructions.ts b/src/instructions.ts index c3abc16..bc2324e 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -96,7 +96,7 @@ This instance can query the Reactome graph database directly. Use it for what th 2. Use \`reactome_cypher_sample\` on a label to see a representative node. 3. Run the query with \`reactome_cypher_query\`: - It runs in a READ-mode session; write clauses are refused by the database. - - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...), and \`SHOW\` only for indexes and constraints. Fulltext procedures, \`CALL { ... }\` subqueries, APOC, \`LOAD CSV\` and \`USE\` are refused. + - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...) and fulltext queries (\`db.index.fulltext.queryNodes\`, \`db.index.fulltext.queryRelationships\`), and \`SHOW\` only for indexes and constraints. Other procedures, \`CALL { ... }\` subqueries, APOC/GDS, \`LOAD CSV\` and \`USE\` are refused. - A server-side timeout ends long queries. Always include a \`LIMIT\`, and anchor on an indexed property (for example \`stId\` or \`dbId\`) rather than scanning a whole label. - Return specific properties (\`RETURN p.stId, p.displayName\`) rather than whole nodes; reading stops at \`max_rows\`, and response size is capped. diff --git a/src/tools/cypher-guard.ts b/src/tools/cypher-guard.ts index d43d529..c38a4c2 100644 --- a/src/tools/cypher-guard.ts +++ b/src/tools/cypher-guard.ts @@ -18,10 +18,13 @@ // - CALL only for the built-in schema procedures below, compared by name // after backticks are removed; no CALL subqueries. // - SHOW only for indexes and constraints. -// - No identifier `apoc` followed by `.` -- no APOC procedure or function. -// - No LOAD CSV, USE, TERMINATE, PERIODIC COMMIT, and no write or -// administration keyword (CREATE, MERGE, SET, DELETE, REMOVE, DETACH, -// DROP, GRANT, DENY, REVOKE, ALTER, START, STOP, RENAME). +// - No name in the apoc or gds namespaces, however it is quoted. +// - No LOAD CSV, TERMINATE, PERIODIC COMMIT, START/STOP DATABASE, no USE +// where a query part begins, and no write or administration keyword +// (CREATE, MERGE, SET, DELETE, REMOVE, DETACH, DROP, GRANT, DENY, REVOKE, +// ALTER, RENAME). SET and SHOW are refused even as variable names: they +// can begin a clause in too many places to tell apart safely. +// - A number literal with a name running straight on from it is refused. // - Text the scanner cannot finish reading -- an unterminated string, // comment or quoted name -- is refused. // @@ -40,11 +43,21 @@ const ALLOWED_PROCEDURES = new Set([ "db.indexes", "db.constraints", "dbms.components", + // READ-mode procedures in Neo4j 4.3/4.4: they query an existing fulltext + // index. Creating and dropping one are separate procedures, not listed. + "db.index.fulltext.querynodes", + "db.index.fulltext.queryrelationships", ]); +/** + * Plugin namespaces. Any name in them is refused, called or not: their + * procedures and functions are not part of what a read needs, and several + * write, load from outside the graph, or run other queries. + */ +const PLUGIN_NAMESPACES = ["apoc", "gds"]; + /** Keywords refused wherever they appear as a keyword. */ const FORBIDDEN_KEYWORDS = new Set([ - "use", "terminate", "periodic", "create", @@ -58,8 +71,6 @@ const FORBIDDEN_KEYWORDS = new Set([ "deny", "revoke", "alter", - "start", - "stop", "rename", ]); @@ -104,6 +115,7 @@ type Token = | { kind: "number" } | { kind: "punct"; text: string }; +const DIGIT = /[0-9]/; const IDENTIFIER_START = /[\p{ID_Start}_]/u; const IDENTIFIER_PART = /[\p{ID_Continue}_]/u; @@ -190,10 +202,35 @@ export function tokenize(query: string): Token[] { continue; } - // A number is its leading digits. Whatever follows is read as a new - // token, so `1CALL` is a number and then CALL. - if (/[0-9]/.test(c)) { - while (i < chars.length && /[0-9]/.test(chars[i] as string)) i++; + // A number literal, read the way Cypher defines one: decimal with an + // optional fraction and exponent (`1`, `1.5`, `.5`, `1e3`, `1.5E-3`), + // hexadecimal `0x1F`, octal `0o17` (or `017`, which the decimal rule + // covers). A name running straight on from one -- `1e0CALL`, `0x1LOAD`, + // `1_000` -- is refused rather than guessed at: where the lexer splits + // that is exactly what decides whether a keyword is seen. + if (DIGIT.test(c) || (c === "." && DIGIT.test(next ?? ""))) { + const at = (k: number) => chars[k] ?? ""; + if (c === "0" && /[xX]/.test(at(i + 1))) { + i += 2; + while (/[0-9a-fA-F]/.test(at(i))) i++; + } else if (c === "0" && /[oO]/.test(at(i + 1))) { + i += 2; + while (/[0-7]/.test(at(i))) i++; + } else { + while (DIGIT.test(at(i))) i++; + if (at(i) === "." && DIGIT.test(at(i + 1))) { + i++; + while (DIGIT.test(at(i))) i++; + } + const signed = /[+-]/.test(at(i + 1)); + if (/[eE]/.test(at(i)) && DIGIT.test(at(i + (signed ? 2 : 1)))) { + i += signed ? 2 : 1; + while (DIGIT.test(at(i))) i++; + } + } + if (IDENTIFIER_PART.test(at(i)) || at(i) === "`") { + throw new QueryRejected("a name runs straight on from a number literal"); + } tokens.push({ kind: "number" }); continue; } @@ -233,20 +270,47 @@ function isKeyword(tokens: Token[], i: number): boolean { return true; } +/** + * Whether token `i` is where a query part begins: the start of the text, or + * after EXPLAIN / PROFILE, UNION [ALL], `;` or `{`. + */ +function beginsQueryPart(tokens: Token[], i: number): boolean { + const before = tokens[i - 1]; + if (before === undefined) return true; + if (isPunct(before, ";") || isPunct(before, "{")) return true; + const word = before.kind === "word" ? before.text.toLowerCase() : undefined; + if (word === "explain" || word === "profile" || word === "union") return true; + return word === "all" && lower(tokens[i - 2]) === "union"; +} + function check(tokens: Token[]): string | undefined { for (let i = 0; i < tokens.length; i++) { const token = tokens[i]; const word = lower(token); if (word === undefined) continue; - // Any APOC name, procedure or function, whatever it is called from. - if (word === "apoc" && isPunct(tokens[i + 1], ".")) { - return "APOC procedures and functions are not allowed"; + // Any plugin name, procedure or function, however it is quoted: + // apoc.x(...), `apoc`.x(...) and `apoc.x`(...) are the same call. + for (const ns of PLUGIN_NAMESPACES) { + if ( + (word === ns && isPunct(tokens[i + 1], ".")) || + (token?.kind === "quoted" && word.startsWith(`${ns}.`)) + ) { + return `${ns.toUpperCase()} procedures and functions are not allowed`; + } } if (!isKeyword(tokens, i)) continue; if (word === "load" && lower(tokens[i + 1]) === "csv") return "LOAD CSV is not allowed"; + // Administration only when followed by DATABASE; otherwise start and + // stop are ordinary names, common in path queries. + if ((word === "start" || word === "stop") && lower(tokens[i + 1]) === "database") { + return `${word.toUpperCase()} DATABASE is not allowed`; + } + // USE is a clause only where a query, a UNION part or a subquery begins; + // anywhere else `use` is a name. + if (word === "use" && beginsQueryPart(tokens, i)) return "USE is not allowed"; if (FORBIDDEN_KEYWORDS.has(word)) return `${word.toUpperCase()} is not allowed`; if (word === "show") { diff --git a/src/tools/cypher.ts b/src/tools/cypher.ts index dc99b3b..2035f05 100644 --- a/src/tools/cypher.ts +++ b/src/tools/cypher.ts @@ -82,7 +82,7 @@ export function registerCypherTools(server: McpServer) { server.tool( "reactome_cypher_query", - "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.relationshipTypes, db.propertyKeys, db.schema.*, db.indexes, db.constraints, dbms.components). Rejected before execution: every other procedure (fulltext procedures such as db.index.fulltext.queryNodes included), CALL subqueries (CALL { ... }), APOC, LOAD CSV, USE, SHOW other than indexes/constraints, and write or admin clauses. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", + "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.relationshipTypes, db.propertyKeys, db.schema.*, db.indexes, db.constraints, dbms.components) and the fulltext query procedures (db.index.fulltext.queryNodes, db.index.fulltext.queryRelationships). Rejected before execution: every other procedure (creating or dropping a fulltext index included), CALL subqueries (CALL { ... }), APOC and GDS, LOAD CSV, USE, SHOW other than indexes/constraints, and write or admin clauses. SET and SHOW cannot be used as variable names. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", { query: z .string() diff --git a/tests/cypher-guard.test.ts b/tests/cypher-guard.test.ts index 9597c76..d719301 100644 --- a/tests/cypher-guard.test.ts +++ b/tests/cypher-guard.test.ts @@ -222,7 +222,84 @@ describe("assertReadOnlyQuery: refuses writes and admin as keywords", () => { refused("MATCH (n) CALL (n) { RETURN n.dbId AS id } RETURN id"); }); - it("fulltext procedures, which are not on the allowlist", () => { - refused("CALL db.index.fulltext.queryNodes('x', 'y')"); + it("fulltext procedures that create or drop, though querying is allowed", () => { + refused("CALL db.index.fulltext.createNodeIndex('x', ['X'], ['y'])"); + refused("CALL db.index.fulltext.drop('x')"); + accepted("CALL db.index.fulltext.queryNodes('x', 'y') YIELD node RETURN node LIMIT 5"); + accepted( + "CALL db.index.fulltext.queryRelationships('x', 'y') YIELD relationship RETURN relationship" + ); + }); +}); + +describe("assertReadOnlyQuery: number literals", () => { + it("are read whole, and refused when a name runs straight on from one", () => { + // Reading only the leading digits of 1e0CALL left `e0CALL` as one name, + // so the CALL was never seen. Neo4j reads 1e0 and then CALL. + for (const q of [ + "MATCH (n) WHERE n.x = 1e0CALL dbms.listQueries() YIELD queryId RETURN queryId", + "MATCH (n) WITH n LIMIT 1E0LOAD CSV FROM 'http://x/' AS r RETURN r", + "WITH 1 AS a LIMIT 0x1LOAD CSV FROM 'http://x/' AS r RETURN r", + "MATCH (n) WHERE n.x = 1.5e3DELETE n", + "RETURN 1e0CALL{ MATCH (n) RETURN n }", + "MATCH (n) WHERE n.x = .5e1DETACH n", + "MATCH (n) WHERE n.x = 0o7LOAD CSV FROM 'x' AS r RETURN r", + "MATCH (n) WHERE n.x = 1_000 RETURN n", + "MATCH (n) WHERE n.x = 1e+5CALL dbms.listConfig() RETURN n", + ]) { + refused(q); + } + }); + + it("in their ordinary forms", () => { + for (const q of [ + "MATCH (n) WHERE n.x = 1.5e3 RETURN n LIMIT 10", + "MATCH (n) WHERE n.x > .5 AND n.y < 1E-3 RETURN n", + "MATCH (n) WHERE n.x = 0x1F OR n.x = 0o17 OR n.x = 017 RETURN n", + "MATCH p = (a)-[*1..3]->(b) RETURN p LIMIT 5", + "RETURN [1,2,3][0..2] AS xs", + "RETURN 1+2-3*4/5%6^7 AS x", + ]) { + accepted(q); + } + }); +}); + +describe("assertReadOnlyQuery: plugin namespaces", () => { + it("refuses apoc and gds however the name is quoted", () => { + for (const q of [ + "RETURN `apoc.text.join`(['a'], ',')", + "CALL `apoc.util.sleep`(1000)", + "RETURN `APOC`.text.join(['a'], ',')", + "CALL gds.graph.project('g', '*', '*')", + "RETURN gds.util.asNode(1)", + "CALL `gds.graph.drop`('g')", + "RETURN `gds`.version()", + ]) { + refused(q); + } + }); +}); + +describe("assertReadOnlyQuery: names that are keywords elsewhere", () => { + it("accepts start and stop as variables; refuses only START/STOP DATABASE", () => { + accepted("MATCH p=(start:Pathway)-[:hasEvent]->(e) RETURN start.displayName LIMIT 5"); + accepted("MATCH (a:Pathway) WITH a AS start RETURN start.stId LIMIT 5"); + accepted("MATCH (stop:Pathway) RETURN stop LIMIT 1"); + refused("START DATABASE neo4j"); + refused("STOP DATABASE neo4j"); + refused("start database neo4j"); + }); + + it("accepts use as a variable; refuses USE where a query or a UNION part starts", () => { + accepted("MATCH (use:Pathway) RETURN use.stId LIMIT 1"); + accepted("WITH 1 AS use RETURN use"); + refused("USE system SHOW USERS"); + refused("EXPLAIN USE neo4j MATCH (n) RETURN n"); + refused("PROFILE USE neo4j MATCH (n) RETURN n"); + refused("MATCH (n) RETURN n.x AS x UNION USE other MATCH (n) RETURN n.x AS x"); + refused("MATCH (n) RETURN n.x AS x UNION ALL USE other MATCH (n) RETURN n.x AS x"); + refused("RETURN 1 AS x; USE system SHOW USERS"); + refused("MATCH (n) WHERE EXISTS { USE other MATCH (m) RETURN m } RETURN n"); }); }); diff --git a/tests/cypher-tools.test.ts b/tests/cypher-tools.test.ts index 113acdc..5ba223b 100644 --- a/tests/cypher-tools.test.ts +++ b/tests/cypher-tools.test.ts @@ -50,7 +50,7 @@ describe("reactome_cypher_* tools", () => { it("says in its description what the guard refuses", () => { const description = fakeServer.tools.get("reactome_cypher_query")?.description ?? ""; - expect(description).toMatch(/fulltext/i); + expect(description).toContain("db.index.fulltext.queryRelationships"); expect(description).toContain("CALL subqueries"); expect(description).toContain("LOAD CSV"); }); From a22cd80a6e4d93f369b45eda68a651f40fda34ff Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 14 Oct 2026 01:38:41 +0000 Subject: [PATCH 5/5] Refuse the CYPHER options prefix Review of 46ede92: Neo4j's pre-parser strips `CYPHER [version] [key=value]*` before parsing, so a USE after it begins the query. The guard decided whether USE begins a query by looking only at the token just before it, so all of these were accepted: CYPHER 4.4 USE system CALL dbms.components() CYPHER runtime=slotted USE neo4j MATCH (n) RETURN n CYPHER USE system SHOW USERS EXPLAIN CYPHER planner=cost USE x MATCH (n) RETURN n PROFILE CYPHER 4.4 USE x RETURN 1 The fix is the simple, fail-closed one. CYPHER is refused wherever it is used as a keyword. No tool needs planner or runtime options. `n.cypher` and `:Cypher` are still names; `cypher` as a variable name is refused. The guard header, tool description, instructions, README and CHANGELOG say so. Failed first against 46ede92: - "is refused, so options cannot sit between a query's start and USE" (the five queries above, plus a plain `CYPHER 4.4 MATCH ...`); - the tool-description test, which now requires CYPHER to be named. 222 tests; lint, format, both typechecks and build clean on Node 24. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- README.md | 5 +++-- src/instructions.ts | 2 +- src/tools/cypher-guard.ts | 9 +++++++++ src/tools/cypher.ts | 2 +- tests/cypher-guard.test.ts | 22 ++++++++++++++++++++++ tests/cypher-tools.test.ts | 1 + 7 files changed, 38 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9496052..6b90e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ All notable changes to this project are documented here. This project adheres to Schema extraction now uses Neo4j's built-in procedures only (`db.schema.*`, `SHOW INDEXES`/`SHOW CONSTRAINTS` with `db.indexes()`/`db.constraints()` as fallback), because the Reactome graph database image runs Neo4j 4.4 Community without APOC. Each section degrades on its own and is reported as unavailable rather than rendered empty. - The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). The check runs on one left-to-right tokenisation (strings, backtick names, line and block comments, Unicode separators), so a comment opener inside a comment or a string can no longer hide text, `CALL \`db\`.\`labels\`()` is allowed, and CALL subqueries and write/admin keywords are refused. Number literals are read whole (`1e0CALL` no longer hides a CALL) and a name running on from one is refused; `gds` is refused like `apoc`; `start`/`stop`/`use` work as variable names; the fulltext query procedures are allowed. Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. + The query guard is now an **allowlist**: `CALL` only for the built-in schema procedures, `SHOW` only for indexes and constraints, and no APOC name, `LOAD CSV`, `USE` or `TERMINATE`. The old denylist let through `apoc.cypher.runMany`/`runFile`/`runSchema`/`parallel`, `apoc.schema.assert`, `apoc.util.sleep`, `apoc.custom.*`, `apoc.systemdb.*`, `apoc.lock.*`, `dbms.killQuery`/`listQueries`/`listConfig` and `USE`, and a comment splitting a keyword (`LOAD/**/CSV`). The check runs on one left-to-right tokenisation (strings, backtick names, line and block comments, Unicode separators), so a comment opener inside a comment or a string can no longer hide text, `CALL \`db\`.\`labels\`()` is allowed, and CALL subqueries and write/admin keywords are refused. Number literals are read whole (`1e0CALL` no longer hides a CALL) and a name running on from one is refused; `gds` is refused like `apoc`; `start`/`stop`/`use` work as variable names; the fulltext query procedures are allowed. The `CYPHER` options prefix is refused, since the pre-parser strips it and a `USE` after it would begin the query. Query rows are streamed and reading stops at `max_rows`, rather than the whole result being read into memory and capped afterwards. A schema section that failed is retried after five minutes instead of staying unavailable for the life of the process. `neo4j-driver` is loaded only once the gate is open. `NEO4J_DATABASE` now defaults to the server's default database, and `NEO4J_PASSWORD` to none (connect without credentials), rather than `graph.db` and `neo4j`. diff --git a/README.md b/README.md index 6380889..4eb9b2f 100644 --- a/README.md +++ b/README.md @@ -122,8 +122,9 @@ the built-in schema procedures (`db.labels`, `db.relationshipTypes`, `dbms.components`) and the fulltext query procedures (`db.index.fulltext.queryNodes`, `db.index.fulltext.queryRelationships`), no `CALL { ... }` subqueries, `SHOW` only for indexes and constraints, and no -APOC or GDS name, `LOAD CSV`, `USE`, `TERMINATE` or write/admin clause. `SET` -and `SHOW` cannot be used as variable names. The check runs on the query as Neo4j tokenises +APOC or GDS name, `LOAD CSV`, `USE`, `TERMINATE`, `CYPHER` options prefix +(version, planner, runtime) or write/admin clause. `SET`, `SHOW` and `CYPHER` +cannot be used as variable names. The check runs on the query as Neo4j tokenises it (comments, string literals and quoted names read as such), and anything it cannot finish reading is refused. Each query runs under a server-side timeout; rows are streamed and reading stops at `max_rows`, with the rest discarded on diff --git a/src/instructions.ts b/src/instructions.ts index bc2324e..03a0b4a 100644 --- a/src/instructions.ts +++ b/src/instructions.ts @@ -96,7 +96,7 @@ This instance can query the Reactome graph database directly. Use it for what th 2. Use \`reactome_cypher_sample\` on a label to see a representative node. 3. Run the query with \`reactome_cypher_query\`: - It runs in a READ-mode session; write clauses are refused by the database. - - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...) and fulltext queries (\`db.index.fulltext.queryNodes\`, \`db.index.fulltext.queryRelationships\`), and \`SHOW\` only for indexes and constraints. Other procedures, \`CALL { ... }\` subqueries, APOC/GDS, \`LOAD CSV\` and \`USE\` are refused. + - \`CALL\` is allowed only for the built-in schema procedures (\`db.labels\`, \`db.schema.*\`, \`db.indexes\`, \`db.constraints\`, \`dbms.components\`, ...) and fulltext queries (\`db.index.fulltext.queryNodes\`, \`db.index.fulltext.queryRelationships\`), and \`SHOW\` only for indexes and constraints. Other procedures, \`CALL { ... }\` subqueries, APOC/GDS, \`LOAD CSV\`, \`USE\` and the \`CYPHER\` options prefix are refused. - A server-side timeout ends long queries. Always include a \`LIMIT\`, and anchor on an indexed property (for example \`stId\` or \`dbId\`) rather than scanning a whole label. - Return specific properties (\`RETURN p.stId, p.displayName\`) rather than whole nodes; reading stops at \`max_rows\`, and response size is capped. diff --git a/src/tools/cypher-guard.ts b/src/tools/cypher-guard.ts index c38a4c2..5f939ef 100644 --- a/src/tools/cypher-guard.ts +++ b/src/tools/cypher-guard.ts @@ -25,6 +25,9 @@ // ALTER, RENAME). SET and SHOW are refused even as variable names: they // can begin a clause in too many places to tell apart safely. // - A number literal with a name running straight on from it is refused. +// - No CYPHER options prefix (version, planner, runtime), which would +// otherwise move where a query begins; `cypher` is not usable as a +// variable name either. // - Text the scanner cannot finish reading -- an unterminated string, // comment or quoted name -- is refused. // @@ -303,6 +306,12 @@ function check(tokens: Token[]): string | undefined { if (!isKeyword(tokens, i)) continue; if (word === "load" && lower(tokens[i + 1]) === "csv") return "LOAD CSV is not allowed"; + // The pre-parser prefix `CYPHER [version] [key=value]*` is stripped before + // the query is parsed, so whatever follows it begins the query -- a USE + // included -- though the token before that is not where a query begins. + // No tool needs planner or runtime options, so it is refused outright, + // wherever it is a keyword (`n.cypher` and `:Cypher` are names). + if (word === "cypher") return "the CYPHER options prefix is not allowed"; // Administration only when followed by DATABASE; otherwise start and // stop are ordinary names, common in path queries. if ((word === "start" || word === "stop") && lower(tokens[i + 1]) === "database") { diff --git a/src/tools/cypher.ts b/src/tools/cypher.ts index 2035f05..830ab2d 100644 --- a/src/tools/cypher.ts +++ b/src/tools/cypher.ts @@ -82,7 +82,7 @@ export function registerCypherTools(server: McpServer) { server.tool( "reactome_cypher_query", - "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.relationshipTypes, db.propertyKeys, db.schema.*, db.indexes, db.constraints, dbms.components) and the fulltext query procedures (db.index.fulltext.queryNodes, db.index.fulltext.queryRelationships). Rejected before execution: every other procedure (creating or dropping a fulltext index included), CALL subqueries (CALL { ... }), APOC and GDS, LOAD CSV, USE, SHOW other than indexes/constraints, and write or admin clauses. SET and SHOW cannot be used as variable names. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", + "Run a read-only Cypher query against the Reactome graph database (Neo4j). Call reactome_cypher_schema first; never guess labels or properties. The session runs in READ mode, which rejects CREATE/MERGE/DELETE/SET/REMOVE. CALL is allowed only for the built-in schema procedures (db.labels, db.relationshipTypes, db.propertyKeys, db.schema.*, db.indexes, db.constraints, dbms.components) and the fulltext query procedures (db.index.fulltext.queryNodes, db.index.fulltext.queryRelationships). Rejected before execution: every other procedure (creating or dropping a fulltext index included), CALL subqueries (CALL { ... }), APOC and GDS, LOAD CSV, USE, SHOW other than indexes/constraints, and write or admin clauses. The CYPHER options prefix (version, planner, runtime) is refused. SET, SHOW and CYPHER cannot be used as variable names. A server-side timeout terminates long queries. Rows are streamed and reading stops at max_rows; per-row size and total response size are also capped, so always include a LIMIT and return specific properties (RETURN p.stId, p.displayName) rather than whole nodes.", { query: z .string() diff --git a/tests/cypher-guard.test.ts b/tests/cypher-guard.test.ts index d719301..311feff 100644 --- a/tests/cypher-guard.test.ts +++ b/tests/cypher-guard.test.ts @@ -303,3 +303,25 @@ describe("assertReadOnlyQuery: names that are keywords elsewhere", () => { refused("MATCH (n) WHERE EXISTS { USE other MATCH (m) RETURN m } RETURN n"); }); }); + +describe("assertReadOnlyQuery: the CYPHER pre-parser prefix", () => { + it("is refused, so options cannot sit between a query's start and USE", () => { + // Neo4j strips `CYPHER [version] [key=value]*` before parsing, so USE + // after it begins the query even though the token before it is not. + for (const q of [ + "CYPHER 4.4 USE system CALL dbms.components()", + "CYPHER runtime=slotted USE neo4j MATCH (n) RETURN n", + "CYPHER USE system SHOW USERS", + "EXPLAIN CYPHER planner=cost USE x MATCH (n) RETURN n", + "PROFILE CYPHER 4.4 USE x RETURN 1", + "CYPHER 4.4 MATCH (n) RETURN n LIMIT 1", + ]) { + refused(q); + } + }); + + it("does not refuse cypher as a property or label", () => { + accepted("MATCH (n:Pathway) RETURN n.cypher LIMIT 1"); + accepted("MATCH (n:Cypher) RETURN n LIMIT 1"); + }); +}); diff --git a/tests/cypher-tools.test.ts b/tests/cypher-tools.test.ts index 5ba223b..def3b90 100644 --- a/tests/cypher-tools.test.ts +++ b/tests/cypher-tools.test.ts @@ -53,6 +53,7 @@ describe("reactome_cypher_* tools", () => { expect(description).toContain("db.index.fulltext.queryRelationships"); expect(description).toContain("CALL subqueries"); expect(description).toContain("LOAD CSV"); + expect(description).toContain("CYPHER"); }); it("runs a query in a READ session, under the configured timeout", async () => {