From a654acbc88006aeedcae748503c287f94b2e4c92 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 12 Aug 2026 11:57:35 +0200 Subject: [PATCH] fix(test-infra): grant the build-dir ACL by SID, not by name (#1532 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local Windows leg caught this within hours of merging #1532: on a machine that is not domain-joined, USERDOMAIN is the literal string WORKGROUP, and `WORKGROUP\test` is not a resolvable principal — WARN: build-dir DACL stamp (pre-wave) failed (user=WORKGROUP\test ...): WORKGROUP\test: No mapping between account names and security IDs was done. so the grant fails outright and the tree keeps its inherited `Authenticated Users:(M)`. The guard then correctly reports that cross-account mutation is still possible, and the whole leg stops. Since most Windows machines are not domain-joined, that is the common case, not the exotic one. #1532 was fixing a real bug in the other direction (a bare name resolves against the machine first, so a host named like its user grants to an empty principal). Both failures are name RESOLUTION failures, so this stops resolving names: the account is identified by SID, which is unambiguous everywhere. The SYSTEM and Administrators grants in these same commands have always used that form — the current user was the odd one out. Name lookup survives only as a fallback where PowerShell is unavailable. Applied to all three scripts carrying the stamp (run-tests-parallel, soak-test, memlab), since they share the failure and would otherwise drift apart. Co-Authored-By: Kiborgik Signed-off-by: Martin Vogel --- scripts/memlab.sh | 25 +++++++++++++--- scripts/run-tests-parallel.sh | 25 +++++++++++++--- scripts/soak-test.sh | 25 +++++++++++++--- src/mcp/mcp.c | 10 ++++++- tests/test_mcp.c | 56 +++++++++++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 13 deletions(-) diff --git a/scripts/memlab.sh b/scripts/memlab.sh index ffe5969f..37374255 100644 --- a/scripts/memlab.sh +++ b/scripts/memlab.sh @@ -38,10 +38,27 @@ if [[ "$BINARY" == *.exe ]] && command -v cygpath >/dev/null 2>&1 && # Qualify with the domain: a bare name from coreutils `whoami` resolves # against the machine first, so on a host whose name equals the user's the # grant lands on an empty principal. - ME="$(whoami | tr -d '\r')" - if [ -n "${USERDOMAIN:-}" ]; then - ME="${USERDOMAIN}\\${ME}" - fi + # Identify the account by SID, never by name. icacls resolves a bare name + # against the machine first, so a host whose name equals the user's grants + # to an empty principal (#1532); and a USERDOMAIN-qualified name is + # UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between + # account names and security IDs was done" — which fails the grant outright + # and silently leaves the tree writable by Authenticated Users. A SID has + # neither ambiguity, and the SYSTEM/Administrators grants below already use + # this form. Name lookup remains only as a fallback where PowerShell is + # unavailable. + ME="$(powershell.exe -NoProfile -NonInteractive -Command \ + '[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null | + tr -d '\r\n')" + case "${ME}" in + S-1-*) ME="*${ME}" ;; + *) + ME="$(whoami | tr -d '\r')" + if [ -n "${USERDOMAIN:-}" ]; then + ME="${USERDOMAIN}\\${ME}" + fi + ;; + esac MSYS2_ARG_CONV_EXCL='*' icacls "$WIN_ROOT_W" /reset /Q >/dev/null 2>&1 || true if ! MSYS2_ARG_CONV_EXCL='*' icacls "$WIN_ROOT_W" /inheritance:r \ /grant:r "${ME}:(OI)(CI)F" '*S-1-5-18:(OI)(CI)F' '*S-1-5-32-544:(OI)(CI)F' \ diff --git a/scripts/run-tests-parallel.sh b/scripts/run-tests-parallel.sh index d7f8c0e8..d2882954 100644 --- a/scripts/run-tests-parallel.sh +++ b/scripts/run-tests-parallel.sh @@ -64,10 +64,27 @@ stamp_windows_build_dir() { # (COMPUTERNAME=BUILD, user build) the grant lands on an empty principal # (BUILD\) and, combined with /inheritance:r above, locks this script out # of its own log directory. - me="$(whoami | tr -d '\r')" - if [ -n "${USERDOMAIN:-}" ]; then - me="${USERDOMAIN}\\${me}" - fi + # Identify the account by SID, never by name. icacls resolves a bare name + # against the machine first, so a host whose name equals the user's grants + # to an empty principal (#1532); and a USERDOMAIN-qualified name is + # UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between + # account names and security IDs was done" — which fails the grant outright + # and silently leaves the tree writable by Authenticated Users. A SID has + # neither ambiguity, and the SYSTEM/Administrators grants below already use + # this form. Name lookup remains only as a fallback where PowerShell is + # unavailable. + me="$(powershell.exe -NoProfile -NonInteractive -Command \ + '[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null | + tr -d '\r\n')" + case "${me}" in + S-1-*) me="*${me}" ;; + *) + me="$(whoami | tr -d '\r')" + if [ -n "${USERDOMAIN:-}" ]; then + me="${USERDOMAIN}\\${me}" + fi + ;; + esac # Normalize FIRST: some runner images stamp EXPLICIT (non-inherited) # Authenticated-Users ACEs onto the workspace tree, which /inheritance:r # cannot strip and /grant:r does not touch (it replaces only the granted diff --git a/scripts/soak-test.sh b/scripts/soak-test.sh index 12910bbd..a12029a3 100755 --- a/scripts/soak-test.sh +++ b/scripts/soak-test.sh @@ -80,10 +80,27 @@ soak_stamp_windows_dir() { # Qualify with the domain: a bare name from coreutils `whoami` resolves # against the machine first, so on a host whose name equals the user's the # grant lands on an empty principal. - me="$(whoami | tr -d '\r')" - if [ -n "${USERDOMAIN:-}" ]; then - me="${USERDOMAIN}\\${me}" - fi + # Identify the account by SID, never by name. icacls resolves a bare name + # against the machine first, so a host whose name equals the user's grants + # to an empty principal (#1532); and a USERDOMAIN-qualified name is + # UNRESOLVABLE on a workgroup machine — "WORKGROUP\test: No mapping between + # account names and security IDs was done" — which fails the grant outright + # and silently leaves the tree writable by Authenticated Users. A SID has + # neither ambiguity, and the SYSTEM/Administrators grants below already use + # this form. Name lookup remains only as a fallback where PowerShell is + # unavailable. + me="$(powershell.exe -NoProfile -NonInteractive -Command \ + '[System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value' 2>/dev/null | + tr -d '\r\n')" + case "${me}" in + S-1-*) me="*${me}" ;; + *) + me="$(whoami | tr -d '\r')" + if [ -n "${USERDOMAIN:-}" ]; then + me="${USERDOMAIN}\\${me}" + fi + ;; + esac if ! output=$(MSYS2_ARG_CONV_EXCL='*' icacls "$dir_w" /reset /Q 2>&1); then echo "FAIL: soak DACL normalize failed (dir=$dir_w user=$me)" >&2 printf '%s\n' "$output" >&2 diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index 7cbee0bf..a0d3269f 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -601,7 +601,7 @@ static const tool_def_t TOOLS[] = { "\"description\":\"Max enriched results per call. Default 10. Response includes " "'total_grep_matches' and 'total_results' so callers can detect truncation. No " "offset parameter — raise limit or narrow with file_pattern / path_filter to see more." - "\",\"default\":10}},\"required\":[\"pattern\",\"project\"]}"}, + "\",\"default\":10,\"minimum\":1}},\"required\":[\"pattern\",\"project\"]}"}, {"list_projects", "List projects", "List all indexed projects", "{\"type\":\"object\",\"properties\":{}}"}, @@ -9634,6 +9634,14 @@ static char *handle_search_code(cbm_mcp_server_t *srv, const char *args) { char *path_filter = cbm_mcp_get_string_arg(args, "path_filter"); char *mode_str = cbm_mcp_get_string_arg(args, "mode"); int limit = cbm_mcp_get_int_arg(args, "limit", MCP_DEFAULT_LIMIT); + /* #1511: a negative limit flowed straight into the result cap and came back + * as the reported count ("results: -5"), which reads to an agent as a real + * answer rather than a rejected argument. The schema now declares + * minimum:1, but a schema is a request to the client, never a guarantee to + * the server — clamp here too. */ + if (limit < 1) { + limit = MCP_DEFAULT_LIMIT; + } int context_lines = cbm_mcp_get_int_arg(args, "context", 0); bool use_regex = cbm_mcp_get_bool_arg(args, "regex"); uint64_t search_t0 = cbm_now_ms(); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index b83a8031..1d99d71a 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -4012,6 +4012,60 @@ TEST(tool_search_code_missing_pattern) { PASS(); } +/* #1511 (distilled from @lukiod's #1512): search_code echoed a negative limit + * back as the result count — "results: -5" — which an agent reads as an answer, + * not as a rejected argument. Both halves matter: the schema declares the bound + * so well-behaved clients never send it, and the handler clamps because a + * schema is a request to the client, never a guarantee to the server. */ +TEST(tool_search_code_negative_limit_is_not_echoed_issue1511) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + char *resp = + cbm_mcp_server_handle(srv, "{\"jsonrpc\":\"2.0\",\"id\":35,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"search_code\"," + "\"arguments\":{\"pattern\":\"func main\"," + "\"project\":\"nonexistent\",\"limit\":-5}}}"); + ASSERT_NOT_NULL(resp); + ASSERT_NULL(strstr(resp, "results: -5")); + free(resp); + cbm_mcp_server_free(srv); + PASS(); +} + +TEST(tool_search_code_limit_declares_a_minimum_issue1511) { + cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); + char *resp = cbm_mcp_server_handle( + srv, "{\"jsonrpc\":\"2.0\",\"id\":36,\"method\":\"tools/list\",\"params\":{}}"); + ASSERT_NOT_NULL(resp); + + yyjson_doc *doc = yyjson_read(resp, strlen(resp), 0); + yyjson_val *root = doc ? yyjson_doc_get_root(doc) : NULL; + yyjson_val *result = root ? yyjson_obj_get(root, "result") : NULL; + yyjson_val *tools = result ? yyjson_obj_get(result, "tools") : NULL; + yyjson_val *minimum = NULL; + if (tools && yyjson_is_arr(tools)) { + size_t index, max; + yyjson_val *tool; + yyjson_arr_foreach(tools, index, max, tool) { + yyjson_val *name = yyjson_obj_get(tool, "name"); + if (!name || !yyjson_is_str(name) || strcmp(yyjson_get_str(name), "search_code") != 0) { + continue; + } + yyjson_val *schema = yyjson_obj_get(tool, "inputSchema"); + yyjson_val *props = schema ? yyjson_obj_get(schema, "properties") : NULL; + yyjson_val *limit = props ? yyjson_obj_get(props, "limit") : NULL; + minimum = limit ? yyjson_obj_get(limit, "minimum") : NULL; + break; + } + } + bool declared = minimum && yyjson_is_int(minimum) && yyjson_get_int(minimum) >= 1; + yyjson_doc_free(doc); + free(resp); + cbm_mcp_server_free(srv); + + ASSERT_TRUE(declared); + PASS(); +} + TEST(tool_search_code_no_project) { cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); @@ -10591,6 +10645,8 @@ SUITE(mcp) { RUN_TEST(tool_get_code_snippet_missing_qn); RUN_TEST(tool_get_code_snippet_not_found); RUN_TEST(tool_search_code_missing_pattern); + RUN_TEST(tool_search_code_negative_limit_is_not_echoed_issue1511); + RUN_TEST(tool_search_code_limit_declares_a_minimum_issue1511); RUN_TEST(tool_search_code_no_project); RUN_TEST(search_code_multi_word); RUN_TEST(search_code_scoped_path_with_spaces_issue687);