Cypher tools require an explicit opt-in, not just a connection string - #36
Merged
Merged
Conversation
Adam's instruction, on the way to publishing this MCP through the website's nginx: the public must not be able to run Cypher queries. Today they can, given one plausible misconfiguration. `registerAllTools` registers the Cypher tools whenever `NEO4J_URI` is set, so whether arbitrary graph queries are exposed is a side effect of a connection string rather than a decision anybody made. A public instance that set `NEO4J_URI` for any other reason -- the graph-schema warm-up on startup already wants it, and a future non-Cypher graph tool would -- would publish `reactome_cypher_query` by doing so, and nothing would look wrong. `MCP_ALLOW_CYPHER=1` is now required as well. Default-deny, and separate from the connection on purpose: forgetting it costs a missing tool on an internal instance, which is visible and harmless, where forgetting the inverse costs arbitrary query access on a public one. An instance with `NEO4J_URI` and no opt-in logs that the tools are off and how to turn them on, because an operator who expected them needs to know why they are absent. Silently present is the failure being prevented; silently absent would be a different one. Five tests over the combinations, including the two that matter: a connection alone registers nothing, and both together registers something. Without the second, the first three would pass against a build that never registers Cypher at all and prove nothing. Verified by reverting the gate: two fail. Note for anyone running the suite: the host's node is v18 and vitest needs node:util's styleText, so tests run under node:22 -- `docker run --rm -v "$PWD":/srv -w /srv node:22-slim npm run check`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…doors Reviewing the opt-in before merge, against its own claim: does a server told not to offer Cypher actually not offer it? Three surfaces answer that question and the first commit changed one. **The server instructions.** `buildServerInstructions()` still tested `isNeo4jConfigured()`, so an instance with a connection and no opt-in appended the "Graph database (Cypher) — enabled" section and told every client to call `reactome_cypher_schema` and `reactome_cypher_query` — tools it had just declined to register. Not an exposure, but a server confidently instructing clients to use what it does not have. **The `reactome://graph/schema` resource.** Also gated on the connection alone. It is not a query surface, but it runs `apoc.meta.schema()` on the caller's behalf and returns labels, counts, relationship cardinalities, property types, indexes and constraints — the internal graph model, handed to anyone who can read resources. If a connection string is not consent for the tools, it is not consent for this either. The fix is to stop asking the question in three places. `isCypherEnabled()` lives next to `isNeo4jConfigured()` and all three call it. Splitting one decision across independent tests is exactly how the first two got left behind. Also: the startup schema prefetch now waits for the opt-in too — without it nothing can read that cache, so it was opening a Neo4j connection for nobody — and `/health` and the startup log report `cypherEnabled` alongside `neo4jEnabled`, so an operator can see why the tools are absent rather than inferring it. Four tests, each asserted in both directions, because "the section is absent" and "the resource is not registered" both pass against a build that never offers them. Verified by reverting each gate separately: exactly the two new negative tests fail, the positive ones stay green. lint, format, typecheck, build and 116 tests clean under node:22. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adam's instruction, on the way to publishing this MCP through the website's nginx: the public must not be able to run Cypher queries.
Today they could, from one plausible misconfiguration
registerAllToolsregisters the Cypher tools wheneverNEO4J_URIis set:So whether arbitrary graph queries are exposed is a side effect of a connection string rather than a decision anybody made. A public instance that set
NEO4J_URIfor any other reason — the graph-schema warm-up on startup already wants it, and any future non-Cypher graph tool would — would publishreactome_cypher_queryby doing so, and nothing would look wrong.The change
MCP_ALLOW_CYPHER=1is now required as well. Default-deny, and separate from the connection on purpose: forgetting it costs a missing tool on an internal instance, which is visible and harmless. Forgetting the inverse would cost arbitrary query access on a public one.An instance with
NEO4J_URIand no opt-in logs that the tools are off and how to enable them — an operator who expected them needs to know why they are absent. Silently present is the failure this prevents; silently absent would be a different one.Tests
Five, over the combinations. The two that matter are "a connection alone registers nothing" and "both together registers something" — without the second, the others would pass against a build that never registers Cypher at all and prove nothing.
Verified by reverting the gate: two fail.
Running the suite
The host's node is v18 and vitest needs
node:util'sstyleText, so:112 tests pass, lint/format/typecheck/build clean.
🤖 Generated with Claude Code
Adversarial review, before merge
The guard above covers the tools. Reviewing it against its own claim — does a server told not to offer Cypher actually not offer it? — found two other surfaces answering the same question from their own copy of the condition:
buildServerInstructions()still appended "Graph database (Cypher) — enabled" and told clients to callreactome_cypher_schemaandreactome_cypher_query. Not an exposure — the tools genuinely aren't there — but a server confidently instructing clients to use what it does not have.reactome://graph/schemaresource. Also gated on the connection alone. Not a query surface, but it runsapoc.meta.schema()for the caller and returns labels, counts, relationship cardinalities, property types, indexes and constraints. If a connection string is not consent for the tools, it is not consent for this.Fixed by not asking the question in three places:
isCypherEnabled()sits next toisNeo4jConfigured()and all three call it. One decision split across independent tests is precisely how the other two got left behind.Two smaller things that followed: the startup schema prefetch waits for the opt-in too (without it nothing can read that cache, so it was opening a Neo4j connection for nobody), and
/healthand the startup log now reportcypherEnabledbesideneo4jEnabledso an operator can see why the tools are absent instead of inferring it.Four more tests, each in both directions — "the section is absent" and "the resource is not registered" both pass against a build that never offers them. Verified by reverting each gate separately: exactly the two new negative tests fail.
Confirmed surface, counted by driving
registerAllToolswith a recording stub rather than by grep: 59 tools without Neo4j, 62 with the opt-in. The three arereactome_cypher_query,reactome_cypher_sample,reactome_cypher_schema.116 tests, lint/format/typecheck/build clean under node:22.