fix: cap client-driven traversal depth (var-length Cypher + MCP tools)#888
Merged
Conversation
Explicit variable-length Cypher bounds ([:CALLS*1..N]) and the client `depth` on trace_call_path / detect_changes were passed straight into cbm_store_bfs, so an arbitrarily large value drove an unbounded traversal (a DoS on cyclic graphs). The MCP_MAX_DEPTH ceiling was defined but never applied, and CYP_MAX_DEPTH capped only the unbounded var-length forms. Add env-configurable ceilings cbm_cypher_max_depth() (CBM_CYPHER_MAX_DEPTH, default 10) and cbm_mcp_max_depth() (CBM_MCP_MAX_DEPTH, default 15), clamp both entry points to them, and WARN on clamp -- never a silent truncation. Remove the now-dead CYP_MAX_DEPTH / MCP_MAX_DEPTH constants. Reproduce-first: cypher_exec_var_length_explicit_bound_capped (12 -> 10 rows) and tool_trace_call_path_depth_clamped (depth 1000 -> hop 15). Closes #887. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.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.
Closes #887.
What
Two graph-traversal entry points let input drive an effectively unbounded BFS depth:
expand_var_lengthusedrel->max_hops > 0 ? rel->max_hops : CYP_MAX_DEPTH, so the ceiling (10) applied onlyto the unbounded forms (
*,*n..,*..m). An explicit[:CALLS*1..1000000]passed itsbound straight into
cbm_store_bfs— an unbounded traversal (on a cyclic call graph therecursive CTE emits up to
|V| × depthrows).trace_call_path/detect_changesdepthwas unclamped.MCP_MAX_DEPTH = 15wasdefined but never applied; the client
depthargument flowed straight into the sharedcbm_store_bfs.Fix
limits.c, mirroringcbm_max_file_bytes:cbm_cypher_max_depth()(CBM_CYPHER_MAX_DEPTH, default 10) andcbm_mcp_max_depth()(CBM_MCP_MAX_DEPTH, default 15).cypher.depth_capped/mcp.depth_capped) — never a silent truncation, per the project's limits philosophy.CYP_MAX_DEPTH/MCP_MAX_DEPTHconstants (the ceilings live inlimits.c).Reproduce-first
cypher_exec_var_length_explicit_bound_capped— a*1..12walk over a 13-node chain.RED = 12 rows (uncapped); GREEN = 10 (capped at the ceiling;
N11/N12never emitted).tool_trace_call_path_depth_clamped—depth:1000over an 18-node chain. RED = theresponse reaches
n16; GREEN = the walk stops at hop 15 (n15present,n16absent).Both fail on the pre-fix code and pass only with the clamp; full suite green, lint clean.
Notes
Companion to #883 (variable-length path semantics, still in review): that change rewrites
cbm_store_bfsto enumerate simple paths, which would amplify these unbounded-depth walks —bounding the depth here closes the surface regardless of that PR's outcome.