From a41f5abfba9071202181d3974fb1b69bf36cec94 Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 1 Oct 2026 22:19:00 +0200 Subject: [PATCH] fix(hooks): file_write_branch clears only one exact-case key and a plain branch name Round-2 review of skills#52 committed to main through gitea-mcp with {"branch":"feat/y8","Branch":"main"}: Go's encoding/json matches keys case-insensitively and the last one wins, while the hook read only the lowercase key. Same trick on content emptied a file. The guard now asks unless exactly one key case-folds to "branch" and one to "content", both spelled exactly, both strings. The branch is cleared by shape ([A-Za-z0-9][A-Za-z0-9._/-]*, no "..", no refs/ prefix, no trailing slash) instead of a blocklist, since refs/heads/main created a second ref that wedged every API write to main. Whitespace-only content asks. 11 new cases; 6 were red before the change, 5 pinned untested-but-correct behaviour the reviewer's flips showed nothing covered. Refs mathias/infra#508 Bump-Type: minor Co-Authored-By: Claude Opus 5.5 --- hooks/mcp-destructive-guard.sh | 24 +++++++++++++++++++----- tests/mcp_destructive_guard_test.sh | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/hooks/mcp-destructive-guard.sh b/hooks/mcp-destructive-guard.sh index c5a60fb..3df075d 100755 --- a/hooks/mcp-destructive-guard.sh +++ b/hooks/mcp-destructive-guard.sh @@ -60,13 +60,27 @@ if printf '%s' "$verb" | grep -Eqix "($DESTRUCTIVE)"; then fi if printf '%s' "$verb" | grep -Eqix 'file_write_branch'; then + # Cleared only when there is exactly ONE key that case-folds to "branch" and + # one to "content": gitea-mcp decodes with Go encoding/json, which matches + # keys case-insensitively and lets the last one win, so {"branch":"feat/x", + # "Branch":"main"} landed on main while a lowercase-only read saw feat/x. + # The branch is cleared by SHAPE (allowlist), not by a blocklist of names: + # "refs/heads/main" created a second ref that wedged API writes to main. verdict="$(printf '%s' "$INPUT" | jq -r ' .tool_input as $i - | if ($i | type) != "object" then "ask" - elif (($i.branch // "") | ascii_downcase) as $b | ($b == "" or $b == "main" or $b == "master") then "ask" - elif (($i.content // "") == "") then "ask" - else "pass" end' 2>/dev/null || echo ask)" + | if ($i | type) != "object" then "ask" else + ([$i | to_entries[] | select(.key | ascii_downcase == "branch")]) as $b + | ([$i | to_entries[] | select(.key | ascii_downcase == "content")]) as $c + | if ($b | length) != 1 or ($c | length) != 1 then "ask" + elif ($b[0].key != "branch") or ($c[0].key != "content") then "ask" + elif ($b[0].value | type) != "string" or ($c[0].value | type) != "string" then "ask" + elif ($b[0].value | test("^[A-Za-z0-9][A-Za-z0-9._/-]*$") | not) then "ask" + elif ($b[0].value | test("\\.\\.|^refs/|/$")) then "ask" + elif ($b[0].value | ascii_downcase | . == "main" or . == "master") then "ask" + elif ($c[0].value | test("^\\s*$")) then "ask" + else "pass" end + end' 2>/dev/null || echo ask)" [ "$verdict" = "pass" ] \ - || ask "file_write_branch to main/master, with no branch, or emptying a file (infra ADR-0027): needs your approval." + || ask "file_write_branch to main/master, an unusual branch name, or emptying a file (infra ADR-0027): needs your approval." fi exit 0 diff --git a/tests/mcp_destructive_guard_test.sh b/tests/mcp_destructive_guard_test.sh index f542f3b..dfb3d2c 100755 --- a/tests/mcp_destructive_guard_test.sh +++ b/tests/mcp_destructive_guard_test.sh @@ -69,6 +69,25 @@ check ask "$(fwb feat/y '')" "file_write_branch emptying a file asks" check ask '{"tool_name":"mcp__plugin_skills_gitea__file_write_branch","tool_input":{"content":"x"}}' "file_write_branch without branch asks" check NONE "$(fwb feat/y x)" "file_write_branch to a feature branch passes" +# Go's encoding/json matches keys case-insensitively, last one wins: gitea-mcp +# wrote to main from {"branch":"feat/y8","Branch":"main"} while the hook read +# only `branch` (gatekeeper round 2, verified against a real Gitea). So the +# guard clears a write only when exactly one key case-folds to branch/content. +FWB=mcp__plugin_skills_gitea__file_write_branch +check ask "{\"tool_name\":\"$FWB\",\"tool_input\":{\"branch\":\"feat/y8\",\"Branch\":\"main\",\"content\":\"x\"}}" "case-variant Branch key asks" +check ask "{\"tool_name\":\"$FWB\",\"tool_input\":{\"branch\":\"feat/y\",\"content\":\"x\",\"Content\":\"\"}}" "case-variant Content key asks" +check ask "{\"tool_name\":\"$FWB\",\"tool_input\":{\"BRANCH\":\"feat/y\",\"content\":\"x\"}}" "only an uppercase BRANCH key asks" +check ask "$(fwb Main x)" "file_write_branch to Main asks" +check ask "{\"tool_name\":\"$FWB\",\"tool_input\":{\"branch\":\"feat/y\"}}" "missing content key asks" +check ask "$(fwb feat/y ' ')" "whitespace-only content asks" +check ask "{\"tool_name\":\"$FWB\",\"tool_input\":\"branch=feat/y\"}" "non-object tool_input asks" +# Branch names are cleared by shape, not by blocklist: refs/heads/main created a +# second ref that wedged every API write to main (gatekeeper side finding). +check ask "$(fwb refs/heads/main x)" "refs/ prefixed branch asks" +check ask "$(fwb ' main' x)" "branch with whitespace asks" +check ask "$(fwb '../main' x)" "branch with .. asks" +check NONE "$(fwb release/v1.2_x x)" "ordinary branch name passes" + # Reversible / read verbs pass through. for v in issue_close issue_create file_read repo_list pr_create; do check NONE "$(tool "mcp__plugin_skills_gitea__$v")" "$v passes through"