From 9ba7cb2dcd11ffb6be94091c0e4b58a0bad8abbf Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 1 Oct 2026 20:37:46 +0200 Subject: [PATCH 1/4] feat(hooks): mcp-destructive-guard asks before irreversible MCP verbs gitea-mcp authenticates by PAT pass-through, so an agent holds the caller's full PAT scope, and repo_delete's confirm field is filled in by the model. The infra-mcp README said client-side deny lists covered destructive verbs; on 2026-10-01 none existed in any settings file. PreToolUse hook on mcp__.*: repo_delete, file_delete, branch_delete, pr_merge, tbd_ship, repo_update and repo_mirror_push get "ask". Matched on the verb, not the server name, so the connector namespace or a renamed registration cannot bypass it. Never emits allow; malformed input fails closed. Scope limit: MCP calls only. The Gitea REST API via Bash with the PAT in env is not covered (recorded in infra ADR-0027). Refs mathias/infra#508 Bump-Type: minor Co-Authored-By: Claude Opus 5.5 --- hooks/hooks.json | 10 ++++ hooks/mcp-destructive-guard.sh | 46 ++++++++++++++++++ tests/mcp_destructive_guard_test.sh | 75 +++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+) create mode 100755 hooks/mcp-destructive-guard.sh create mode 100755 tests/mcp_destructive_guard_test.sh diff --git a/hooks/hooks.json b/hooks/hooks.json index 8db511f..5eed7bf 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -33,6 +33,16 @@ "timeout": 5 } ] + }, + { + "matcher": "mcp__.*", + "hooks": [ + { + "type": "command", + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/mcp-destructive-guard.sh\"", + "timeout": 5 + } + ] } ] } diff --git a/hooks/mcp-destructive-guard.sh b/hooks/mcp-destructive-guard.sh new file mode 100755 index 0000000..d3f3e23 --- /dev/null +++ b/hooks/mcp-destructive-guard.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash +# PreToolUse hook (mcp__.* matcher) — makes a human approve irreversible MCP +# verbs instead of letting the model run them unattended. +# +# Why (infra#508, infra ADR-0027): gitea-mcp authenticates by PAT pass-through, +# so an agent holds the full scope of the caller's PAT. repo_delete's own guard +# (confirm == repo) is filled in by the model on both sides: it stops typos, +# not an injected or mistaken agent. The infra-mcp README claimed client-side +# deny lists covered this; on 2026-10-01 none existed on any settings file. +# +# Policy: match the VERB, not the server. The same tool is reachable as +# mcp__plugin_skills_gitea__*, mcp__claude_ai_gitea__* (connector) or under +# any name a host registers it with, so keying on the server name would let a +# rename bypass the gate. +# +# Emits "ask" or nothing. Never "allow": a hook allow would override the +# user's own deny/ask rules. Malformed input fails closed (ask). +# +# Scope limit, recorded in the ADR: this gates MCP calls only. A session with +# the PAT in its environment can still reach the Gitea REST API via Bash. + +set -uo pipefail + +DESTRUCTIVE='repo_delete|file_delete|branch_delete|pr_merge|tbd_ship|repo_update|repo_mirror_push' + +ask() { + jq -n --arg reason "$1" \ + '{hookSpecificOutput: {hookEventName: "PreToolUse", permissionDecision: "ask", permissionDecisionReason: $reason}}' + exit 0 +} + +INPUT="$(cat)" +if ! TOOL_NAME="$(printf '%s' "$INPUT" | jq -er '.tool_name' 2>/dev/null)"; then + ask "mcp-destructive-guard: could not parse the tool call, so it cannot be shown safe. Approve only if you expected it." +fi + +verb="${TOOL_NAME##*__}" +case "$TOOL_NAME" in + mcp__*__*) ;; + *) exit 0 ;; +esac + +if printf '%s' "$verb" | grep -Eqix "($DESTRUCTIVE)"; then + ask "Irreversible MCP verb '$verb' (infra ADR-0027): needs your approval. The model filled in every argument, including any confirm field." +fi +exit 0 diff --git a/tests/mcp_destructive_guard_test.sh b/tests/mcp_destructive_guard_test.sh new file mode 100755 index 0000000..21fdf8f --- /dev/null +++ b/tests/mcp_destructive_guard_test.sh @@ -0,0 +1,75 @@ +#!/bin/sh +# Tests for hooks/mcp-destructive-guard.sh — the PreToolUse gate that makes a +# human approve irreversible MCP verbs (infra#508, infra ADR-0027). +# +# Contract: reads PreToolUse hook JSON on stdin, ALWAYS exits 0, and answers +# in hookSpecificOutput.permissionDecision. "ask" = a human must approve; +# no decision emitted ("NONE") = the guard stays out of the way and normal +# permission rules apply. The guard never emits "allow": an allow from a hook +# would bypass the user's own deny/ask rules. + +set -u + +ROOT=$(CDPATH= cd -- "$(dirname -- "$0")/.." && pwd) +GUARD="$ROOT/hooks/mcp-destructive-guard.sh" + +pass=0 +fail=0 + +# An empty stdout is a valid hook answer ("no opinion"), reported as NONE. +decode() { + _out=$(cat) + [ -z "$_out" ] && { echo NONE; return; } + printf '%s' "$_out" | jq -r '.hookSpecificOutput.permissionDecision // "NONE"' 2>/dev/null || echo BROKEN +} + +decision_for() { + jq -n --arg t "$1" '{tool_name: $t, tool_input: {owner: "mathias", repo: "x"}}' \ + | "$GUARD" | decode +} + +expect() { + _want=$1 _tool=$2 _label=$3 + _got=$(decision_for "$_tool") + if [ "$_got" = "$_want" ]; then + pass=$((pass + 1)); printf ' ok %s\n' "$_label" + else + fail=$((fail + 1)); printf ' FAIL %s\n want=%s got=%s tool=%s\n' "$_label" "$_want" "$_got" "$_tool" + fi +} + +echo "mcp-destructive-guard tests" + +# Irreversible verbs, on the plugin namespace. +for v in repo_delete file_delete branch_delete pr_merge tbd_ship repo_update repo_mirror_push; do + expect ask "mcp__plugin_skills_gitea__$v" "plugin gitea $v asks" +done + +# The same verb reached through a different server name must not slip past: +# the claude.ai connector, a bare local registration, or any future rename. +expect ask "mcp__claude_ai_gitea__repo_delete" "connector namespace asks" +expect ask "mcp__gitea__repo_delete" "bare server name asks" +expect ask "mcp__git__repo_delete" "renamed server still asks" + +# Case variants must not bypass (tool names are matched exactly by the host, +# but a guard that lowercases nothing should not depend on that). +expect ask "mcp__plugin_skills_gitea__REPO_DELETE" "uppercase verb asks" + +# Reversible / read verbs pass through untouched. +for v in issue_close issue_create file_read repo_list pr_create file_write_branch; do + expect NONE "mcp__plugin_skills_gitea__$v" "$v passes through" +done + +# Near-miss names are not the guarded verb. +expect NONE "mcp__plugin_skills_gitea__repo_delete_dry_run_list" "suffix near-miss passes" +expect NONE "Bash" "non-MCP tool passes" + +# Malformed input fails CLOSED (ask), never open. +_got=$(printf 'not json' | "$GUARD" | decode) +if [ "$_got" = "ask" ]; then pass=$((pass+1)); echo " ok malformed input asks"; else fail=$((fail+1)); echo " FAIL malformed input: got=$_got"; fi + +# Executed as a program, like Claude Code does (shebang decides interpreter). +[ -x "$GUARD" ] && { pass=$((pass+1)); echo " ok guard is executable"; } || { fail=$((fail+1)); echo " FAIL guard not executable"; } + +printf '\n%d passed, %d failed\n' "$pass" "$fail" +[ "$fail" -eq 0 ] -- 2.54.0 From d068cbc279fc872f43ba6d860f07b5ee0d405e0b Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 1 Oct 2026 20:37:54 +0200 Subject: [PATCH 2/4] docs(readme): list mcp-destructive-guard among shipped hooks Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c4dacf7..de91b11 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ descriptions and "use when" triggers. /plugin install skills@mathias ``` -The plugin ships skills, the `secret-guard` + `pre-task-ritual` hooks, and the +The plugin ships skills, the `secret-guard`, `mcp-destructive-guard` and `pre-task-ritual` hooks, and the brain/gitea/infra MCP servers as one versioned unit. Skills are namespaced (`skills:tdd`). Updates land when `.claude-plugin/plugin.json`'s `version` moves — CI bumps it on every release. -- 2.54.0 From 4a82206393494cc834eaab5808caa0e74e44cb2e Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 1 Oct 2026 20:45:19 +0200 Subject: [PATCH 3/4] fix(hooks): close the five escapes the skills#52 review found - Fail-closed path no longer needs jq: the ask JSON is a printf literal, and a host without jq asks instead of staying silent. - A tool_name that is not a non-empty string asks (was: no opinion). - Gate workflow_run_trigger, tag_create, release_create, and file_write_branch when it lands on main/master, has no branch, or empties a file (same effect as pr_merge / file_delete). - Header claim narrowed: gitea-mcp spellings on any server name, not other servers' spellings of the same operation. - Tests run the hook as hooks.json does (bash ), assert exit 0 on every case, and cover null/missing/wrong-type tool_name, last-segment verb parsing and a PATH without jq. 13 new cases were red first. Refs mathias/infra#508 Bump-Type: minor Co-Authored-By: Claude Opus 5.5 --- hooks/mcp-destructive-guard.sh | 60 +++++++++++----- tests/mcp_destructive_guard_test.sh | 106 +++++++++++++++++----------- 2 files changed, 108 insertions(+), 58 deletions(-) diff --git a/hooks/mcp-destructive-guard.sh b/hooks/mcp-destructive-guard.sh index d3f3e23..c5a60fb 100755 --- a/hooks/mcp-destructive-guard.sh +++ b/hooks/mcp-destructive-guard.sh @@ -6,41 +6,67 @@ # so an agent holds the full scope of the caller's PAT. repo_delete's own guard # (confirm == repo) is filled in by the model on both sides: it stops typos, # not an injected or mistaken agent. The infra-mcp README claimed client-side -# deny lists covered this; on 2026-10-01 none existed on any settings file. +# deny lists covered this; on 2026-10-01 none existed in any settings file. # -# Policy: match the VERB, not the server. The same tool is reachable as -# mcp__plugin_skills_gitea__*, mcp__claude_ai_gitea__* (connector) or under -# any name a host registers it with, so keying on the server name would let a -# rename bypass the gate. +# Policy: match gitea-mcp's verb spellings on ANY server name. The same tool is +# reachable as mcp__plugin_skills_gitea__*, mcp__claude_ai_gitea__* or under +# whatever name a host registers it with, so keying on the server name would +# let a rename bypass the gate. It does NOT recognise other servers' spellings +# of the same operation (e.g. a third-party `delete_repo`). # -# Emits "ask" or nothing. Never "allow": a hook allow would override the -# user's own deny/ask rules. Malformed input fails closed (ask). +# file_write_branch is gated when it would land on main/master (on infra that +# is a Flux deploy within ~60s — the same effect as pr_merge) or empty a file +# (the same effect as file_delete). Writes to other branches pass. # -# Scope limit, recorded in the ADR: this gates MCP calls only. A session with -# the PAT in its environment can still reach the Gitea REST API via Bash. +# Emits "ask" or nothing; never "allow" (a hook allow would override the +# user's own deny/ask rules). Always exits 0: Claude Code ignores a hook's +# JSON on a non-zero exit. Anything it cannot parse — including a host +# without jq — fails closed to "ask". The ask JSON is a literal, so the +# fail-closed path needs no tool that might be missing. +# +# Scope limit, recorded in infra ADR-0027: this gates MCP calls only. A +# session with the PAT in its environment can still reach the Gitea REST API +# through a shell. -set -uo pipefail +set -u -DESTRUCTIVE='repo_delete|file_delete|branch_delete|pr_merge|tbd_ship|repo_update|repo_mirror_push' +DESTRUCTIVE='repo_delete|file_delete|branch_delete|pr_merge|tbd_ship|repo_update|repo_mirror_push|workflow_run_trigger|tag_create|release_create' ask() { - jq -n --arg reason "$1" \ - '{hookSpecificOutput: {hookEventName: "PreToolUse", permissionDecision: "ask", permissionDecisionReason: $reason}}' + # $1 must contain only [A-Za-z0-9 _.:()'/-] — it is spliced into JSON verbatim. + printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"ask","permissionDecisionReason":"%s"}}\n' "$1" exit 0 } INPUT="$(cat)" -if ! TOOL_NAME="$(printf '%s' "$INPUT" | jq -er '.tool_name' 2>/dev/null)"; then - ask "mcp-destructive-guard: could not parse the tool call, so it cannot be shown safe. Approve only if you expected it." + +command -v jq >/dev/null 2>&1 \ + || ask "mcp-destructive-guard: jq is not installed, so this MCP call cannot be checked. Approve only if you expected it." + +if ! TOOL_NAME="$(printf '%s' "$INPUT" | jq -er '.tool_name | strings | select(length > 0)' 2>/dev/null)"; then + ask "mcp-destructive-guard: could not read a tool name from this call, so it cannot be shown safe. Approve only if you expected it." fi -verb="${TOOL_NAME##*__}" case "$TOOL_NAME" in mcp__*__*) ;; *) exit 0 ;; esac +verb="${TOOL_NAME##*__}" +safe_verb="$(printf '%s' "$verb" | tr -cd 'A-Za-z0-9_-')" + if printf '%s' "$verb" | grep -Eqix "($DESTRUCTIVE)"; then - ask "Irreversible MCP verb '$verb' (infra ADR-0027): needs your approval. The model filled in every argument, including any confirm field." + ask "Irreversible MCP verb '$safe_verb' (infra ADR-0027): needs your approval. The model filled in every argument, including any confirm field." +fi + +if printf '%s' "$verb" | grep -Eqix 'file_write_branch'; then + 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)" + [ "$verdict" = "pass" ] \ + || ask "file_write_branch to main/master, with no branch, 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 21fdf8f..f542f3b 100755 --- a/tests/mcp_destructive_guard_test.sh +++ b/tests/mcp_destructive_guard_test.sh @@ -4,9 +4,11 @@ # # Contract: reads PreToolUse hook JSON on stdin, ALWAYS exits 0, and answers # in hookSpecificOutput.permissionDecision. "ask" = a human must approve; -# no decision emitted ("NONE") = the guard stays out of the way and normal -# permission rules apply. The guard never emits "allow": an allow from a hook -# would bypass the user's own deny/ask rules. +# empty stdout ("NONE") = no opinion, normal permission rules apply. The guard +# never emits "allow": an allow from a hook would bypass the user's own rules. +# +# Cases marked (gatekeeper) are escapes found by the skills#52 review: each +# passed the first version of this suite. set -u @@ -16,60 +18,82 @@ GUARD="$ROOT/hooks/mcp-destructive-guard.sh" pass=0 fail=0 -# An empty stdout is a valid hook answer ("no opinion"), reported as NONE. -decode() { - _out=$(cat) - [ -z "$_out" ] && { echo NONE; return; } - printf '%s' "$_out" | jq -r '.hookSpecificOutput.permissionDecision // "NONE"' 2>/dev/null || echo BROKEN +# run_guard [PATH] — invoked exactly as hooks.json does: `bash `. +# Prints " rc=", decision NONE for an empty answer. +run_guard() { + if [ $# -ge 2 ]; then + _out=$(printf '%s' "$1" | PATH="$2" "$BASH_BIN" "$GUARD"); _rc=$? + else + _out=$(printf '%s' "$1" | bash "$GUARD"); _rc=$? + fi + if [ -z "$_out" ]; then _d=NONE + else _d=$(printf '%s' "$_out" | jq -r '.hookSpecificOutput.permissionDecision // "NONE"' 2>/dev/null || echo BROKEN) + fi + printf '%s rc=%s' "$_d" "$_rc" } +BASH_BIN=$(command -v bash) -decision_for() { - jq -n --arg t "$1" '{tool_name: $t, tool_input: {owner: "mathias", repo: "x"}}' \ - | "$GUARD" | decode -} - -expect() { - _want=$1 _tool=$2 _label=$3 - _got=$(decision_for "$_tool") - if [ "$_got" = "$_want" ]; then +check() { + _want=$1 _json=$2 _label=$3 + _got=$(run_guard "$_json") + if [ "$_got" = "$_want rc=0" ]; then pass=$((pass + 1)); printf ' ok %s\n' "$_label" else - fail=$((fail + 1)); printf ' FAIL %s\n want=%s got=%s tool=%s\n' "$_label" "$_want" "$_got" "$_tool" + fail=$((fail + 1)); printf ' FAIL %s\n want=%s rc=0 got=%s\n in=%s\n' "$_label" "$_want" "$_got" "$_json" fi } +tool() { printf '{"tool_name":"%s","tool_input":{"owner":"mathias","repo":"x"}}' "$1"; } echo "mcp-destructive-guard tests" -# Irreversible verbs, on the plugin namespace. -for v in repo_delete file_delete branch_delete pr_merge tbd_ship repo_update repo_mirror_push; do - expect ask "mcp__plugin_skills_gitea__$v" "plugin gitea $v asks" +for v in repo_delete file_delete branch_delete pr_merge tbd_ship repo_update repo_mirror_push \ + workflow_run_trigger tag_create release_create; do + check ask "$(tool "mcp__plugin_skills_gitea__$v")" "plugin gitea $v asks" done -# The same verb reached through a different server name must not slip past: -# the claude.ai connector, a bare local registration, or any future rename. -expect ask "mcp__claude_ai_gitea__repo_delete" "connector namespace asks" -expect ask "mcp__gitea__repo_delete" "bare server name asks" -expect ask "mcp__git__repo_delete" "renamed server still asks" +# The verb is matched on any server name: connector, bare, or renamed. +check ask "$(tool mcp__claude_ai_gitea__repo_delete)" "connector namespace asks" +check ask "$(tool mcp__gitea__repo_delete)" "bare server name asks" +check ask "$(tool mcp__git__repo_delete)" "renamed server still asks" +check ask "$(tool mcp__plugin_skills_gitea__REPO_DELETE)" "uppercase verb asks" +# The verb is the LAST segment (gatekeeper: parsing the second segment stayed green). +check ask "$(tool mcp__a__b__repo_delete)" "verb taken from last segment" +check NONE "$(tool mcp__repo_delete__issue_get)" "server named like a verb does not trigger" -# Case variants must not bypass (tool names are matched exactly by the host, -# but a guard that lowercases nothing should not depend on that). -expect ask "mcp__plugin_skills_gitea__REPO_DELETE" "uppercase verb asks" +# file_write_branch: content landing on main, or emptying a file, is the same +# effect as the gated pr_merge / file_delete (gatekeeper). +fwb() { printf '{"tool_name":"mcp__plugin_skills_gitea__file_write_branch","tool_input":{"branch":"%s","content":"%s"}}' "$1" "$2"; } +check ask "$(fwb main x)" "file_write_branch to main asks" +check ask "$(fwb master x)" "file_write_branch to master asks" +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" -# Reversible / read verbs pass through untouched. -for v in issue_close issue_create file_read repo_list pr_create file_write_branch; do - expect NONE "mcp__plugin_skills_gitea__$v" "$v passes through" +# 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" done +check NONE "$(tool mcp__plugin_skills_gitea__repo_delete_dry_run_list)" "suffix near-miss passes" +check NONE "$(tool Bash)" "non-MCP tool passes" +check NONE "$(tool repo_delete)" "non-MCP tool named like a verb passes" -# Near-miss names are not the guarded verb. -expect NONE "mcp__plugin_skills_gitea__repo_delete_dry_run_list" "suffix near-miss passes" -expect NONE "Bash" "non-MCP tool passes" +# Malformed input fails CLOSED (gatekeeper: non-string tool_name got NONE). +check ask 'not json' "non-JSON asks" +check ask '{}' "missing tool_name asks" +check ask '{"tool_name":null}' "null tool_name asks" +check ask '{"tool_name":""}' "empty tool_name asks" +check ask '{"tool_name":5}' "number tool_name asks" +check ask '{"tool_name":["mcp__x__repo_delete"]}' "array tool_name asks" +check ask '{"tool_name":{"a":1}}' "object tool_name asks" -# Malformed input fails CLOSED (ask), never open. -_got=$(printf 'not json' | "$GUARD" | decode) -if [ "$_got" = "ask" ]; then pass=$((pass+1)); echo " ok malformed input asks"; else fail=$((fail+1)); echo " FAIL malformed input: got=$_got"; fi - -# Executed as a program, like Claude Code does (shebang decides interpreter). -[ -x "$GUARD" ] && { pass=$((pass+1)); echo " ok guard is executable"; } || { fail=$((fail+1)); echo " FAIL guard not executable"; } +# Without jq on PATH the guard must still answer ask, not stay silent +# (gatekeeper: the fail-closed path itself needed jq). +nojq=$(mktemp -d); trap 'rm -rf "$nojq"' EXIT +for b in bash cat grep tr sed; do ln -s "$(command -v $b)" "$nojq/$b"; done +_got=$(run_guard "$(tool mcp__plugin_skills_gitea__repo_delete)" "$nojq") +if [ "$_got" = "ask rc=0" ]; then pass=$((pass+1)); echo " ok no jq on PATH still asks"; else fail=$((fail+1)); echo " FAIL no jq on PATH: got=$_got"; fi +_got=$(run_guard "$(tool mcp__plugin_skills_gitea__issue_get)" "$nojq") +if [ "$_got" = "ask rc=0" ]; then pass=$((pass+1)); echo " ok no jq on PATH asks even for a read verb (cannot parse = cannot clear)"; else fail=$((fail+1)); echo " FAIL no jq read verb: got=$_got"; fi printf '\n%d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ] -- 2.54.0 From a41f5abfba9071202181d3974fb1b69bf36cec94 Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 1 Oct 2026 22:19:00 +0200 Subject: [PATCH 4/4] 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" -- 2.54.0