Repository navigation
Restore graph database access, off unless NEO4J_URI and MCP_ALLOW_CYPHER=1 - #44
Merged
Merged
Conversation
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 <noreply@anthropic.com>
…chema 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <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.
Puts the Cypher capability that #39 (22aec8b) removed back, switched off by default, so a publicly hosted instance can never run Cypher. Internal instances can point it at a dedicated Reactome graph database container.
The gate
Graph access exists only when both
NEO4J_URIis set (non-blank) andMCP_ALLOW_CYPHER=1. There is one predicate,isGraphAccessEnabled()insrc/graph/gate.ts, and it reads the environment at call time.#39 removed the tools because the old gate was checked in four places and the HTTP entrypoint checked the wrong thing. This time the driver checks as well, so a surface that forgets the check still cannot connect:
getDriver()(src/clients/neo4j.ts, the only file allowed to importneo4j-driver)registerCypherTools()registerGraphResources()permittedGroups(), applied byregisterAllTools,registerAllResourcesandbuildServerInstructionsto any group list they are givencypherresolveToolGroups()cypher;MCP_TOOL_GROUPS=…,cypherrefuses to startwarmGraphSchema(), called by both entrypoints instead of each deciding for itselfNEO4J_URIis set without the opt-in)/health, startup loggraphAccess: falseThe tools join the #40 tool groups as a new group,
cypher.Tests, written first
tests/graph-access-gate.test.tsreplacesno-graph-access.test.ts. It runs 6 env cases (neither variable, URI alone, opt-in alone,MCP_ALLOW_CYPHER=true, a blank URI, both) against 8 surfaces: the predicate,createServer, the registrars given every group explicitly,MCP_TOOL_GROUPS, the driver factory, the stdio entrypoint, the HTTP entrypoint, andstartHttpServerover real HTTP. Each test counts driver constructions on a fakeneo4j-driver. No test connects to Neo4j.git revert 22aec8b:NEO4J_URI alone > the HTTP entrypoint never constructs a driverfailed. That is the door the old gate missed, and the new test catches it.getDriver's check fails 2 testsNo APOC
The Reactome graph image runs Neo4j 4.4 Community without APOC, and the old schema code read
apoc.meta.*. The schema now comes from built-in procedures only:dbms.componentsdb.labels/db.relationshipTypes, with per-token counts from the count storedb.schema.nodeTypeProperties,relTypePropertiesandvisualizationSHOW INDEXES/SHOW CONSTRAINTS, falling back todb.indexes()/db.constraints()Each section fails on its own and is reported as unavailable instead of being shown empty. If no section answers, the fetch throws and nothing is cached.
tests/graph-schema.test.tscovers this with a fake driver that answers like 4.4 Community.Read-only safety
LOAD CSV. It is a read clause, so READ mode allows it, and it fetches URLs.//inside a string literal, by a comment that splits a procedure name, or by backtick-quoted names.Checked against a real Release 97 container (read-only)
CREATEwas refused by the READ session, and the timeout fired.NEO4J_URIset: no Cypher tools were listed and no driver was opened.Lockfile
Additions only.
neo4j-driver,neo4j-driver-coreandneo4j-driver-bolt-connection6.2.0 are added, along with their own dependencies:rxjs7.8.2,tslib2.8.1,buffer6.0.3,base64-js1.5.1,ieee7541.2.1,safe-buffer5.2.1 andstring_decoder1.3.0. Each entry is byte-identical to the lockfile before the removal, and no existing entry changed.Worth a look
NEO4J_DATABASEnow defaults to the server's default database (previouslygraph.db).NEO4J_PASSWORDnow defaults to none, i.e. no credentials (previouslyneo4j).reactome://graph/schemaresource is about 600 KB of JSON. Resources are not capped; this was also true before Remove graph database access rather than gating it #39.🤖 Generated with Claude Code