From 98e98c3208b7386fd3935c3f7ba23a6667839e7d Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 5 Jul 2026 16:12:59 +0200 Subject: [PATCH] fix: cap client-driven traversal depth (var-length Cypher + MCP tools) 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 --- src/cypher/cypher.c | 18 +++++++++-- src/foundation/limits.c | 30 ++++++++++++++++++ src/foundation/limits.h | 13 ++++++++ src/mcp/mcp.c | 20 +++++++++++- tests/test_cypher.c | 69 +++++++++++++++++++++++++++++++++++++++++ tests/test_mcp.c | 57 ++++++++++++++++++++++++++++++++++ 6 files changed, 204 insertions(+), 3 deletions(-) diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index b211ae815..4f554ea4b 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -8,6 +8,8 @@ #include "cypher/cypher.h" #include "store/store.h" #include "foundation/platform.h" +#include "foundation/limits.h" +#include "foundation/log.h" enum { CYP_BUF_16 = 16, @@ -22,7 +24,6 @@ enum { CYP_MAX_VARS = 16, /* max Cypher variables in a query */ CYP_MAX_EDGE_VARS = 8, /* max edge variables */ CYP_GROWTH_10 = 10, /* binding growth factor */ - CYP_MAX_DEPTH = 10, /* max variable-length path depth */ CYP_CHAR_IDX1 = 1, /* second character index (e.g. op[1]) */ CYP_EBUF_MASK = 7, CYP_NODE_COLS = 4, /* columns per node var: name, qn, label, file */ @@ -2886,7 +2887,20 @@ static void expand_var_length(cbm_store_t *store, cbm_rel_pattern_t *rel, cbm_node_pattern_t *target_node, binding_t *b, cbm_node_t *src, const char *to_var, binding_t *new_bindings, int *new_count, int max_new, int *match_count) { - int max_depth = rel->max_hops > 0 ? rel->max_hops : CYP_MAX_DEPTH; + /* Clamp BOTH the explicit (`*1..N`) and unbounded (`*`, `*..m`) forms to the + * engine ceiling: an explicit N above the cap was previously honoured + * verbatim, driving cbm_store_bfs to an unbounded hop count (#887). WARN on + * clamp — never a silent truncation. */ + int depth_cap = cbm_cypher_max_depth(); + int max_depth = rel->max_hops > 0 ? rel->max_hops : depth_cap; + if (max_depth > depth_cap) { + char req_buf[16]; + char cap_buf[16]; + snprintf(req_buf, sizeof(req_buf), "%d", max_depth); + snprintf(cap_buf, sizeof(cap_buf), "%d", depth_cap); + cbm_log_warn("cypher.depth_capped", "requested", req_buf, "cap", cap_buf); + max_depth = depth_cap; + } cbm_traverse_result_t tr = {0}; const char *dir = rel->direction ? rel->direction : "outbound"; cbm_store_bfs(store, src->id, dir, rel->types, rel->type_count, max_depth, CBM_PERCENT, &tr); diff --git a/src/foundation/limits.c b/src/foundation/limits.c index b7ad14091..3e6325b36 100644 --- a/src/foundation/limits.c +++ b/src/foundation/limits.c @@ -4,6 +4,7 @@ #include "foundation/limits.h" #include +#include #include long cbm_max_file_bytes(void) { @@ -24,3 +25,32 @@ long cbm_max_file_bytes(void) { } return default_cap; } + +/* Shared env-int parser: a positive integer in [1, INT_MAX], else the fallback. + * Read fresh each call (see cbm_max_file_bytes rationale — cheap, test-friendly, + * no stale memoized copy across runs). */ +static int env_positive_int(const char *name, int fallback) { + const char *raw = getenv(name); + if (raw && raw[0]) { + errno = 0; + char *end = NULL; + long v = strtol(raw, &end, 10); + if (errno == 0 && end != raw && *end == '\0' && v > 0 && v <= INT_MAX) { + return (int)v; + } + /* Unparseable / non-positive / out-of-range → safe default. */ + } + return fallback; +} + +int cbm_cypher_max_depth(void) { + /* 10 — generous for a code call/def graph; an explicit `*1..N` above this is + * WARN-capped, never an unbounded (cyclic-graph DoS) traversal. */ + return env_positive_int("CBM_CYPHER_MAX_DEPTH", 10); +} + +int cbm_mcp_max_depth(void) { + /* 15 — ceiling for client-driven MCP graph traversals (trace_call_path, + * detect_changes); the caller's `depth` is WARN-clamped to this. */ + return env_positive_int("CBM_MCP_MAX_DEPTH", 15); +} diff --git a/src/foundation/limits.h b/src/foundation/limits.h index ed33f7044..b914c3f79 100644 --- a/src/foundation/limits.h +++ b/src/foundation/limits.h @@ -30,4 +30,17 @@ typedef enum { * leaking across runs. */ long cbm_max_file_bytes(void); +/* Maximum variable-length path depth for the Cypher engine (the `*min..max` + * hop ceiling). BOTH the explicit (`*1..N`) and unbounded (`*`, `*..m`) forms + * are clamped to this, so `[:CALLS*1..1000000]` degrades to a WARN-and-cap + * rather than an unbounded (cyclic-graph DoS) traversal. Override with + * CBM_CYPHER_MAX_DEPTH (a positive integer). Default 10. */ +int cbm_cypher_max_depth(void); + +/* Maximum traversal depth for client-driven MCP graph tools (trace_call_path, + * detect_changes): the client `depth` argument is WARN-clamped to this so an + * arbitrarily large value cannot drive an unbounded BFS over the shared store. + * Override with CBM_MCP_MAX_DEPTH (a positive integer). Default 15. */ +int cbm_mcp_max_depth(void); + #endif /* CBM_LIMITS_H */ diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 413a9d7aa..e855b4a18 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -14,7 +14,6 @@ enum { MCP_TIMEOUT_MS = 1000, MCP_HALF_SEC_US = 500000, MCP_MAX_ROWS = 100, - MCP_MAX_DEPTH = 15, MCP_COL_2 = 2, MCP_COL_3 = 3, MCP_COL_4 = 4, @@ -54,6 +53,7 @@ enum { #include "foundation/compat_fs.h" #include "foundation/compat_thread.h" #include "foundation/log.h" +#include "foundation/limits.h" #include "mcp/index_supervisor.h" #include "foundation/str_util.h" #include "foundation/dump_verify.h" @@ -2933,6 +2933,22 @@ static void bfs_union_same_name(cbm_store_t *store, const cbm_node_t *nodes, int } } +/* Clamp a client-supplied traversal depth to the MCP ceiling (cbm_mcp_max_depth), + * WARN-logging when it does so — never a silent truncation (#887). An unclamped + * `depth` would drive the shared cbm_store_bfs to an arbitrary hop count. */ +static int clamp_mcp_depth(int depth, const char *tool) { + int cap = cbm_mcp_max_depth(); + if (depth > cap) { + char req_buf[16]; + char cap_buf[16]; + snprintf(req_buf, sizeof(req_buf), "%d", depth); + snprintf(cap_buf, sizeof(cap_buf), "%d", cap); + cbm_log_warn("mcp.depth_capped", "tool", tool, "requested", req_buf, "cap", cap_buf); + return cap; + } + return depth; +} + static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) { char *func_name = cbm_mcp_get_string_arg(args, "function_name"); char *project = get_project_arg(args); @@ -2941,6 +2957,7 @@ static char *handle_trace_call_path(cbm_mcp_server_t *srv, const char *args) { char *mode = cbm_mcp_get_string_arg(args, "mode"); char *param_name = cbm_mcp_get_string_arg(args, "parameter_name"); int depth = cbm_mcp_get_int_arg(args, "depth", MCP_DEFAULT_DEPTH); + depth = clamp_mcp_depth(depth, "trace_call_path"); bool risk_labels = cbm_mcp_get_bool_arg(args, "risk_labels"); bool include_tests = cbm_mcp_get_bool_arg(args, "include_tests"); @@ -5201,6 +5218,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { char *since = cbm_mcp_get_string_arg(args, "since"); char *scope = cbm_mcp_get_string_arg(args, "scope"); int depth = cbm_mcp_get_int_arg(args, "depth", MCP_DEFAULT_BFS_DEPTH); + depth = clamp_mcp_depth(depth, "detect_changes"); /* scope: "files" = just changed files, "symbols" = files + symbols (default) */ bool want_symbols = !scope || strcmp(scope, "symbols") == 0 || strcmp(scope, "impact") == 0; diff --git a/tests/test_cypher.c b/tests/test_cypher.c index 2e79c8b4f..5fe3cd592 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -7,6 +7,7 @@ #include "test_framework.h" #include #include +#include #include #include @@ -834,6 +835,73 @@ TEST(cypher_exec_variable_length) { PASS(); } +/* Reproduce-first (#887): an EXPLICIT variable-length upper bound must still be + * capped at the engine ceiling (cbm_cypher_max_depth(), default 10). On + * origin/main, expand_var_length honoured an explicit `*1..N` verbatim (only the + * unbounded `*` / `*..m` forms were capped), so `[:CALLS*1..N]` passed N straight + * to cbm_store_bfs — an unbounded traversal (a DoS on cyclic graphs). RED before + * the clamp: a *1..12 walk over a 13-node chain + * returns all 12 hops (N01..N12). GREEN after: it stops at the depth-10 ceiling + * (N01..N10); N11/N12 are never emitted. max_rows=64 keeps the binding-expansion + * cap (bind_cap*10) well above the hop count, so DEPTH — not the binding cap — is + * the bound under test. */ +TEST(cypher_exec_var_length_explicit_bound_capped) { + cbm_store_t *s = cbm_store_open_memory(); + cbm_store_upsert_project(s, "test", "/tmp/test"); + + /* Linear chain N00 -CALLS-> N01 -> ... -> N12 (13 nodes, one node per hop). */ + int64_t ids[13]; + for (int i = 0; i < 13; i++) { + char name[8]; + char qn[24]; + snprintf(name, sizeof(name), "N%02d", i); + snprintf(qn, sizeof(qn), "test.N%02d", i); + cbm_node_t n = {.project = "test", + .label = "Function", + .name = name, + .qualified_name = qn, + .file_path = "chain.go"}; + ids[i] = cbm_store_upsert_node(s, &n); + } + for (int i = 0; i < 12; i++) { + cbm_edge_t e = { + .project = "test", .source_id = ids[i], .target_id = ids[i + 1], .type = "CALLS"}; + cbm_store_insert_edge(s, &e); + } + + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (a:Function {name: \"N00\"})-[:CALLS*1..12]->" + "(x:Function) RETURN x.name", + "test", 64, &r); + ASSERT_EQ(rc, 0); + + /* Capped at 10 hops → exactly N01..N10; N11/N12 are beyond the ceiling. */ + ASSERT_EQ(r.row_count, 10); + bool saw_n10 = false; + bool saw_n11 = false; + bool saw_n12 = false; + for (int i = 0; i < r.row_count; i++) { + const char *v = r.rows[i][0]; + if (v && strcmp(v, "N10") == 0) { + saw_n10 = true; + } + if (v && strcmp(v, "N11") == 0) { + saw_n11 = true; + } + if (v && strcmp(v, "N12") == 0) { + saw_n12 = true; + } + } + ASSERT_TRUE(saw_n10); /* within the ceiling — proves the traversal really ran */ + ASSERT_FALSE(saw_n11); /* clamped away */ + ASSERT_FALSE(saw_n12); + + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + TEST(cypher_exec_defines_edge) { cbm_store_t *s = setup_cypher_store(); cbm_cypher_result_t r = {0}; @@ -2581,6 +2649,7 @@ SUITE(cypher) { RUN_TEST(cypher_exec_limit); RUN_TEST(cypher_exec_order_by); RUN_TEST(cypher_exec_variable_length); + RUN_TEST(cypher_exec_var_length_explicit_bound_capped); RUN_TEST(cypher_exec_defines_edge); RUN_TEST(cypher_exec_no_results); RUN_TEST(cypher_exec_where_numeric); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index c1feebcf0..b358cd010 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -1001,6 +1001,62 @@ TEST(tool_trace_call_path_prefers_definition) { PASS(); } +/* Reproduce-first (#887): the client-supplied `depth` on trace_call_path must be + * clamped to the MCP ceiling (cbm_mcp_max_depth(), default 15). On origin/main + * an MCP_MAX_DEPTH=15 constant was defined but never applied — `depth` flowed + * straight into bfs_union_same_name, so an unbounded value drives the shared + * cbm_store_bfs to arbitrary depth. Over an 18-node call chain, depth=1000 + * reaches n16/n17 (RED); with the clamp the walk stops at hop 15, so n15 is + * reached but n16 is not (GREEN). Quoted tokens ("n15"/"n16") match only the + * node-name field, never the qualified_name (preceded by '.'), so the boundary + * check is exact. */ +TEST(tool_trace_call_path_depth_clamped) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + cbm_store_t *st = cbm_mcp_server_store(srv); + const char *proj = "depth-proj"; + cbm_mcp_server_set_project(srv, proj); + cbm_store_upsert_project(st, proj, "/tmp/depth"); + + /* Linear call chain n00 -CALLS-> n01 -> ... -> n17 (18 nodes). */ + int64_t ids[18]; + for (int i = 0; i < 18; i++) { + char name[8]; + char qn[32]; + snprintf(name, sizeof(name), "n%02d", i); + snprintf(qn, sizeof(qn), "depth-proj.n%02d", i); + cbm_node_t n = {.project = proj, + .label = "Function", + .name = name, + .qualified_name = qn, + .file_path = "chain.c", + .start_line = 1, + .end_line = 2}; + ids[i] = cbm_store_upsert_node(st, &n); + } + for (int i = 0; i < 17; i++) { + cbm_edge_t e = { + .project = proj, .source_id = ids[i], .target_id = ids[i + 1], .type = "CALLS"}; + cbm_store_insert_edge(st, &e); + } + + char *resp = cbm_mcp_server_handle( + srv, "{\"jsonrpc\":\"2.0\",\"id\":71,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"trace_call_path\",\"arguments\":{\"function_name\":\"n00\"," + "\"project\":\"depth-proj\",\"direction\":\"outbound\",\"depth\":1000}}}"); + ASSERT_NOT_NULL(resp); + char *inner = extract_text_content(resp); + ASSERT_NOT_NULL(inner); + + /* Reached within the ceiling (proves the traversal ran) but clamped at 15. */ + ASSERT_NOT_NULL(strstr(inner, "\"n15\"")); + ASSERT_NULL(strstr(inner, "\"n16\"")); + + free(inner); + free(resp); + cbm_mcp_server_free(srv); + PASS(); +} + /* Reproduce-first (#650, distilled): two GENUINELY-DIFFERENT same-named functions * whose bodies differ in length score differently, so the old exact-tie check did * not flag them ambiguous — and bfs_union_same_name (#546) then merged the caller @@ -4711,6 +4767,7 @@ SUITE(mcp) { RUN_TEST(tool_trace_missing_function_name); RUN_TEST(tool_trace_call_path_ambiguous); RUN_TEST(tool_trace_call_path_prefers_definition); + RUN_TEST(tool_trace_call_path_depth_clamped); RUN_TEST(tool_trace_call_path_distinct_defs_not_over_unioned); RUN_TEST(tool_trace_call_path_dts_stub_unions_with_impl); RUN_TEST(tool_delete_project_not_found);