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 <path>), 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user