From be4397c523b437316f1ab8d9222ea7bcefd688a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:05:22 +0000 Subject: [PATCH 1/8] fix(hooks): fail closed on missing/malformed jq in four PreToolUse gates Ports the guard prologue already proven in check-pr-issue-acm-disclosure.sh and check-pr-title-convention.sh into the four hooks a deterministic-gate- quality audit found still fail open when jq is missing from PATH or the PreToolUse payload is malformed/wrong-shaped: check-bash-safety.sh, check-template-overwrite.sh, check-pr-skill-audit-disclosure.sh, and check-merge-pull-request-block.sh (this repository's own unconditional merge-block "no override" deny, highest priority per the issue). Live-reproduced before the fix (missing jq -> exit 127 "command not found"; malformed JSON -> exit 5, jq's own parse-error code -- neither is exit 2, so Claude Code's PreToolUse contract treats both as non-blocking and the guarded tool call proceeds) and live-confirmed after (exit 2 + deny JSON) for all four scripts, plus two additional malformed shapes (valid-JSON-non-object, tool_input-non-object). Adds a regression test for the new guard to each hook's existing pytest suite, plus a new suite for check-template-overwrite.sh, which had none before. Self-checked all four post-fix scripts against skills/evaluating-deterministic-gate-quality/scripts/gitapex_check_gate_shape.py. Refs #1208 --- hooks/check-bash-safety.sh | 71 ++++-- hooks/check-merge-pull-request-block.sh | 46 +++- hooks/check-pr-skill-audit-disclosure.sh | 48 +++- hooks/check-template-overwrite.sh | 45 +++- hooks/test_gitapex_check_bash_safety.py | 77 +++++++ ..._gitapex_check_merge_pull_request_block.py | 77 +++++++ ...x_check_pr_skill_audit_disclosure_shell.py | 82 +++++++ .../test_gitapex_check_template_overwrite.py | 207 ++++++++++++++++++ 8 files changed, 620 insertions(+), 33 deletions(-) create mode 100644 hooks/test_gitapex_check_template_overwrite.py diff --git a/hooks/check-bash-safety.sh b/hooks/check-bash-safety.sh index 2672a6f6..6cf1348c 100755 --- a/hooks/check-bash-safety.sh +++ b/hooks/check-bash-safety.sh @@ -19,28 +19,26 @@ set -euo pipefail -input=$(cat) - -tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') - -# Defense in depth: the hooks.json matcher already restricts this hook to -# Bash, but never trust that alone. -if [ "$tool_name" != "Bash" ]; then - exit 0 -fi - -command=$(printf '%s' "$input" | jq -r '.tool_input.command // empty') - -if [ -z "$command" ]; then - exit 0 +# Issue #1208: this deny path must not itself depend on jq -- if jq is +# missing from PATH entirely (a broken environment, not a malformed +# payload), every jq call below would crash under `set -e` with exit 127 +# ("command not found"), an exit code Claude Code's PreToolUse contract +# treats as non-blocking (the tool call proceeds unchecked). Checked first, +# via a fixed, statically-escaped JSON literal (no interpolation, so no +# JSON-escaping risk), same pattern as +# hooks/check-pr-issue-acm-disclosure.sh's own jq-missing guard. +if ! command -v jq >/dev/null 2>&1; then + printf '%s\n' "{\"hookSpecificOutput\": {\"permissionDecision\": \"deny\"}, \"systemMessage\": \"Blocked by hooks/check-bash-safety.sh: jq is not available on PATH -- cannot verify the Bash command. Failing closed.\"}" >&2 + exit 2 fi -lc_command=$(printf '%s' "$command" | tr '[:upper:]' '[:lower:]') - deny() { local reason="$1" - jq -n --arg msg "$reason" \ - '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": $msg}' >&2 + # Piped via stdin (jq -Rs: raw input, slurped to one string), not + # `--arg` -- same ARG_MAX-avoidance reason as + # hooks/check-pr-issue-acm-disclosure.sh's own deny(). + printf '%s' "$reason" | jq -Rs \ + '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": .}' >&2 exit 2 } @@ -55,6 +53,43 @@ warn() { exit 0 } +input=$(cat) + +# Issue #1208: a malformed payload (invalid JSON, or valid JSON that isn't +# an object) would otherwise make every field-extraction jq call below exit +# non-zero, crashing past deny() under `set -e` with an exit code Claude +# Code's PreToolUse contract treats as non-blocking -- the same fail-open +# class hooks/check-pr-issue-acm-disclosure.sh's own adversarial review +# found and fixed. Validate the shape up front instead. +if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/dev/null 2>&1; then + deny "Blocked by hooks/check-bash-safety.sh: the tool-call payload on stdin is not a JSON object. Failing closed." +fi + +tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') + +# Defense in depth: the hooks.json matcher already restricts this hook to +# Bash, but never trust that alone. +if [ "$tool_name" != "Bash" ]; then + exit 0 +fi + +# Issue #1208: tool_input could be a non-object (array/string/number/bool) +# in an otherwise well-formed payload, which would crash the +# `.tool_input.command` access below with jq's own "Cannot index X with +# string" runtime error -- same fail-open class as the top-level check +# above. +if ! printf '%s' "$input" | jq -e '(.tool_input // {}) | type == "object"' >/dev/null 2>&1; then + deny "Blocked by hooks/check-bash-safety.sh: tool_input in the payload is not a JSON object. Failing closed." +fi + +command=$(printf '%s' "$input" | jq -r '.tool_input.command // empty') + +if [ -z "$command" ]; then + exit 0 +fi + +lc_command=$(printf '%s' "$command" | tr '[:upper:]' '[:lower:]') + # --- Shared boundary: pre-command anchor that also swallows an absolute or # relative path prefix ----------------------------------------------------- # The boundary is "start of string, or any character that cannot be part of a diff --git a/hooks/check-merge-pull-request-block.sh b/hooks/check-merge-pull-request-block.sh index 3a7a7691..d6f89c2a 100755 --- a/hooks/check-merge-pull-request-block.sh +++ b/hooks/check-merge-pull-request-block.sh @@ -25,8 +25,48 @@ set -euo pipefail +# Issue #1208: per the audit that found this, "the repository's most +# categorical deny ('no override') does not fire in an environment without +# jq" -- this deny path must not itself depend on jq. If jq is missing from +# PATH entirely, every jq call below would crash under `set -e` with exit +# 127 ("command not found"), an exit code Claude Code's PreToolUse contract +# treats as non-blocking (mcp__github__merge_pull_request would proceed +# unchecked). Checked first, via a fixed, statically-escaped JSON literal +# (no interpolation, so no JSON-escaping risk), same pattern as +# hooks/check-pr-issue-acm-disclosure.sh's own jq-missing guard. +if ! command -v jq >/dev/null 2>&1; then + printf '%s\n' "{\"hookSpecificOutput\": {\"permissionDecision\": \"deny\"}, \"systemMessage\": \"Blocked by hooks/check-merge-pull-request-block.sh: jq is not available on PATH -- cannot verify the tool-call payload, and mcp__github__merge_pull_request is never a valid agent action in this repository regardless. Failing closed.\"}" >&2 + exit 2 +fi + +deny() { + local reason="$1" + # Piped via stdin (jq -Rs: raw input, slurped to one string), not + # `--arg` -- same ARG_MAX-avoidance reason as + # hooks/check-pr-issue-acm-disclosure.sh's own deny(). + printf '%s' "$reason" | jq -Rs \ + '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": .}' >&2 + exit 2 +} + input=$(cat) +# Issue #1208: a malformed payload (invalid JSON, or valid JSON that isn't +# an object) would otherwise make the `.tool_name` extraction below exit +# non-zero, crashing past deny() under `set -e` with an exit code Claude +# Code's PreToolUse contract treats as non-blocking -- the same fail-open +# class hooks/check-pr-issue-acm-disclosure.sh's own adversarial review +# found and fixed. This hook's own "no override" categorical deny is the +# highest-priority target in issue #1208, so an unparseable payload here +# fails closed too, rather than falling through on an indeterminate +# tool_name: this hook cannot tell whether an unparseable payload is in +# fact a disguised mcp__github__merge_pull_request call, and the +# repository's fail-closed-on-INDETERMINATE posture answers that +# uncertainty with deny, not allow. +if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/dev/null 2>&1; then + deny "Blocked by hooks/check-merge-pull-request-block.sh: the tool-call payload on stdin is not a JSON object, and mcp__github__merge_pull_request is never a valid agent action in this repository regardless. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matcher already restricts this hook to @@ -35,8 +75,4 @@ if [ "$tool_name" != "mcp__github__merge_pull_request" ]; then exit 0 fi -deny_msg='Blocked by hooks/check-merge-pull-request-block.sh: mcp__github__merge_pull_request is never a valid agent action in this repository, no override. Per planning-a-branch-from-an-issue/SKILL.md, drafting-a-pr-to-merge/SKILL.md, and the ranking-the-open-queue Routine specs'"'"' "100% human review of any pull request merge" policy, merging a PR is always a separate, explicit human or CI decision. hooks/check-bash-safety.sh already blocks the equivalent "gh pr merge" shell command; this hook blocks the platform-integrated tool-call form the same way.' - -jq -n --arg msg "$deny_msg" \ - '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": $msg}' >&2 -exit 2 +deny "Blocked by hooks/check-merge-pull-request-block.sh: mcp__github__merge_pull_request is never a valid agent action in this repository, no override. Per planning-a-branch-from-an-issue/SKILL.md, drafting-a-pr-to-merge/SKILL.md, and the ranking-the-open-queue Routine specs' \"100% human review of any pull request merge\" policy, merging a PR is always a separate, explicit human or CI decision. hooks/check-bash-safety.sh already blocks the equivalent \"gh pr merge\" shell command; this hook blocks the platform-integrated tool-call form the same way." diff --git a/hooks/check-pr-skill-audit-disclosure.sh b/hooks/check-pr-skill-audit-disclosure.sh index 5873a04b..abe9c2d3 100755 --- a/hooks/check-pr-skill-audit-disclosure.sh +++ b/hooks/check-pr-skill-audit-disclosure.sh @@ -59,8 +59,40 @@ set -euo pipefail +# Issue #1208: this deny path must not itself depend on jq -- if jq is +# missing from PATH entirely, every jq call below would crash under +# `set -e` with exit 127 ("command not found"), an exit code Claude Code's +# PreToolUse contract treats as non-blocking (the tool call proceeds +# unchecked). Checked first, via a fixed, statically-escaped JSON literal +# (no interpolation, so no JSON-escaping risk), same pattern as +# hooks/check-pr-issue-acm-disclosure.sh's own jq-missing guard. +if ! command -v jq >/dev/null 2>&1; then + printf '%s\n' "{\"hookSpecificOutput\": {\"permissionDecision\": \"deny\"}, \"systemMessage\": \"Blocked by hooks/check-pr-skill-audit-disclosure.sh: jq is not available on PATH -- cannot verify skill audit disclosure. Failing closed.\"}" >&2 + exit 2 +fi + +deny() { + local reason="$1" + # Piped via stdin (jq -Rs: raw input, slurped to one string), not + # `--arg` -- same ARG_MAX-avoidance reason as + # hooks/check-pr-issue-acm-disclosure.sh's own deny(). + printf '%s' "$reason" | jq -Rs \ + '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": .}' >&2 + exit 2 +} + input=$(cat) +# Issue #1208: a malformed payload (invalid JSON, or valid JSON that isn't +# an object) would otherwise make every field-extraction jq call below exit +# non-zero, crashing past deny() under `set -e` with an exit code Claude +# Code's PreToolUse contract treats as non-blocking -- the same fail-open +# class hooks/check-pr-issue-acm-disclosure.sh's own adversarial review +# found and fixed. Validate the shape up front instead. +if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/dev/null 2>&1; then + deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: the tool-call payload on stdin is not a JSON object. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matchers already restrict this hook to @@ -70,6 +102,15 @@ case "$tool_name" in *) exit 0 ;; esac +# Issue #1208: tool_input could be a non-object (array/string/number/bool) +# in an otherwise well-formed payload, which would crash the +# `.tool_input.body`/`.tool_input.base` accesses below with jq's own +# "Cannot index X with string" runtime error -- same fail-open class as the +# top-level check above. +if ! printf '%s' "$input" | jq -e '(.tool_input // {}) | type == "object"' >/dev/null 2>&1; then + deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: tool_input in the payload is not a JSON object. Failing closed." +fi + body=$(printf '%s' "$input" | jq -r '.tool_input.body // empty') # An update_pull_request call that isn't setting a body has nothing new @@ -82,13 +123,6 @@ if [ "$tool_name" = "mcp__github__update_pull_request" ] && [ -z "$body" ]; then exit 0 fi -deny() { - local reason="$1" - jq -n --arg msg "$reason" \ - '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": $msg}' >&2 - exit 2 -} - script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" check_script="$script_dir/gitapex_check_skill_audit_disclosure_or_waiver.py" diff --git a/hooks/check-template-overwrite.sh b/hooks/check-template-overwrite.sh index 7dddd6e4..ea8ab417 100755 --- a/hooks/check-template-overwrite.sh +++ b/hooks/check-template-overwrite.sh @@ -11,8 +11,40 @@ set -euo pipefail +# Issue #1208: this deny path must not itself depend on jq -- if jq is +# missing from PATH entirely, every jq call below would crash under +# `set -e` with exit 127 ("command not found"), an exit code Claude Code's +# PreToolUse contract treats as non-blocking (the tool call proceeds +# unchecked). Checked first, via a fixed, statically-escaped JSON literal +# (no interpolation, so no JSON-escaping risk), same pattern as +# hooks/check-pr-issue-acm-disclosure.sh's own jq-missing guard. +if ! command -v jq >/dev/null 2>&1; then + printf '%s\n' "{\"hookSpecificOutput\": {\"permissionDecision\": \"deny\"}, \"systemMessage\": \"Blocked by hooks/check-template-overwrite.sh: jq is not available on PATH -- cannot verify the write target. Failing closed.\"}" >&2 + exit 2 +fi + +deny() { + local reason="$1" + # Piped via stdin (jq -Rs: raw input, slurped to one string), not + # `--arg` -- same ARG_MAX-avoidance reason as + # hooks/check-pr-issue-acm-disclosure.sh's own deny(). + printf '%s' "$reason" | jq -Rs \ + '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": .}' >&2 + exit 2 +} + input=$(cat) +# Issue #1208: a malformed payload (invalid JSON, or valid JSON that isn't +# an object) would otherwise make every field-extraction jq call below exit +# non-zero, crashing past deny() under `set -e` with an exit code Claude +# Code's PreToolUse contract treats as non-blocking -- the same fail-open +# class hooks/check-pr-issue-acm-disclosure.sh's own adversarial review +# found and fixed. Validate the shape up front instead. +if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/dev/null 2>&1; then + deny "Blocked by hooks/check-template-overwrite.sh: the tool-call payload on stdin is not a JSON object. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matcher already restricts this hook to @@ -21,6 +53,15 @@ if [ "$tool_name" != "Write" ]; then exit 0 fi +# Issue #1208: tool_input could be a non-object (array/string/number/bool) +# in an otherwise well-formed payload, which would crash the +# `.tool_input.file_path` access below with jq's own "Cannot index X with +# string" runtime error -- same fail-open class as the top-level check +# above. +if ! printf '%s' "$input" | jq -e '(.tool_input // {}) | type == "object"' >/dev/null 2>&1; then + deny "Blocked by hooks/check-template-overwrite.sh: tool_input in the payload is not a JSON object. Failing closed." +fi + file_path=$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty') if [ -z "$file_path" ]; then @@ -57,9 +98,7 @@ is_template_path() { } if is_template_path "$file_path" && [ -f "$file_path" ]; then - jq -n --arg path "$file_path" \ - '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": ("Blocked by hooks/check-template-overwrite.sh: Write would overwrite an existing template file at " + $path + ". Never overwrite or \"improve\" an existing template via Write -- their presence ends automated generation unless the owner names specific additions; use Edit for a deliberate, reviewed change instead.")}' >&2 - exit 2 + deny "Blocked by hooks/check-template-overwrite.sh: Write would overwrite an existing template file at $file_path. Never overwrite or \"improve\" an existing template via Write -- their presence ends automated generation unless the owner names specific additions; use Edit for a deliberate, reviewed change instead." fi exit 0 diff --git a/hooks/test_gitapex_check_bash_safety.py b/hooks/test_gitapex_check_bash_safety.py index a9d215c0..cbb1ae19 100644 --- a/hooks/test_gitapex_check_bash_safety.py +++ b/hooks/test_gitapex_check_bash_safety.py @@ -19,6 +19,7 @@ import json import os +import shutil import subprocess from pathlib import Path @@ -192,6 +193,82 @@ def test_empty_command_is_allowed() -> None: assert_allowed("") +# --------------------------------------------------------------------------- +# Issue #1208: fail closed, not open, when jq is missing or the payload is +# malformed. Ported guard prologue, same one hooks/check-pr-issue-acm- +# disclosure.sh and hooks/check-pr-title-convention.sh already carried. +# --------------------------------------------------------------------------- + + +def _no_jq_path(tmp_path: Path) -> str: + """A PATH directory holding every tool this script needs except jq, so + `command -v jq` genuinely fails the way it would in an environment + without jq installed -- rather than mocking that condition.""" + bin_dir = tmp_path / "no-jq-path" + bin_dir.mkdir() + for tool in ("bash", "cat", "tr", "grep", "sed", "git", "python3", "dirname"): + real = shutil.which(tool) + if real: + (bin_dir / tool).symlink_to(real) + return str(bin_dir) + + +def test_denied_when_jq_missing(tmp_path: Path) -> None: + """Live-reproduced before this fix: with jq absent, the very first jq + call (extracting tool_name) crashed under `set -e` with exit 127 + ("command not found") -- before deny() was even defined, and non- + blocking per Claude Code's PreToolUse contract, so an arbitrary Bash + command (including `gh pr merge`) would have proceeded unchecked. Must + now deny (exit 2) instead.""" + result = run("gh pr merge 1", extra_env={"PATH": _no_jq_path(tmp_path)}) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + assert "jq is not available" in payload["systemMessage"] + + +def test_denied_on_malformed_json_stdin() -> None: + """Live-reproduced before this fix: jq's own parse-error exit (5) + propagated past deny() under `set -e` -- non-blocking per Claude Code's + PreToolUse contract. Must now deny (exit 2) instead.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input="not valid json{{{", + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_denied_when_tool_input_is_not_an_object() -> None: + """A well-formed top-level payload whose tool_input is itself a + non-object (array/string/number/bool) would otherwise crash the + `.tool_input.command` access with jq's own "Cannot index" error. Must + deny.""" + payload = json.dumps({"tool_name": "Bash", "tool_input": ["not", "an", "object"]}) + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2 + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + # --------------------------------------------------------------------------- # Finding 4: git push gated (warn, not deny) on gitapex_scan_provenance.py # --------------------------------------------------------------------------- diff --git a/hooks/test_gitapex_check_merge_pull_request_block.py b/hooks/test_gitapex_check_merge_pull_request_block.py index 18b610ba..70e6ba49 100644 --- a/hooks/test_gitapex_check_merge_pull_request_block.py +++ b/hooks/test_gitapex_check_merge_pull_request_block.py @@ -19,6 +19,7 @@ import json import os +import shutil import subprocess from pathlib import Path @@ -91,3 +92,79 @@ def test_bash_gh_pr_merge_is_ignored_by_this_hook() -> None: assert result.returncode == 0 assert result.stdout == "" assert result.stderr == "" + + +# --------------------------------------------------------------------------- +# Issue #1208: fail closed, not open, when jq is missing or the payload is +# malformed -- highest priority in that issue, since this hook backs the +# repository's single most categorical "no override" deny. +# --------------------------------------------------------------------------- + + +def _no_jq_path(tmp_path: Path) -> str: + """A PATH directory holding every tool this script needs except jq, so + `command -v jq` genuinely fails the way it would in an environment + without jq installed -- rather than mocking that condition.""" + bin_dir = tmp_path / "no-jq-path" + bin_dir.mkdir() + for tool in ("bash", "cat"): + real = shutil.which(tool) + if real: + (bin_dir / tool).symlink_to(real) + return str(bin_dir) + + +def test_denied_when_jq_missing(tmp_path: Path) -> None: + """Live-reproduced before this fix: with jq absent, the very first jq + call (extracting tool_name) crashed under `set -e` with exit 127 + ("command not found") -- before deny() was even defined, and non- + blocking per Claude Code's PreToolUse contract, so + mcp__github__merge_pull_request would have proceeded unchecked: the + repository's own "no override" categorical deny did not fire. Must now + deny (exit 2) instead.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + env.pop("CLAUDE_PLUGIN_ROOT", None) + env["PATH"] = _no_jq_path(tmp_path) + payload = json.dumps( + { + "tool_name": "mcp__github__merge_pull_request", + "tool_input": {"owner": "tvna", "repo": "gitapex", "pullNumber": 1}, + } + ) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + assert "jq is not available" in parsed["systemMessage"] + + +def test_denied_on_malformed_json_stdin() -> None: + """Live-reproduced before this fix: jq's own parse-error exit (5) + propagated past deny() under `set -e` -- non-blocking per Claude Code's + PreToolUse contract. This hook cannot then tell whether the malformed + payload was a disguised merge_pull_request call, so it must deny + (exit 2) rather than fall through on an indeterminate tool_name.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + env.pop("CLAUDE_PLUGIN_ROOT", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input="not valid json{{{", + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" diff --git a/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py index 6781a61a..9d5ebcaa 100644 --- a/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py +++ b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py @@ -28,6 +28,7 @@ import json import os +import shutil import subprocess from pathlib import Path @@ -340,6 +341,87 @@ def test_an_update_call_with_no_body_is_ignored(repo: Path) -> None: assert result.returncode == 0 +# --------------------------------------------------------------------------- +# Issue #1208: fail closed, not open, when jq is missing or the payload is +# malformed. +# --------------------------------------------------------------------------- + + +def _no_jq_path(tmp_path: Path) -> str: + """A PATH directory holding every tool this script needs except jq, so + `command -v jq` genuinely fails the way it would in an environment + without jq installed -- rather than mocking that condition.""" + bin_dir = tmp_path / "no-jq-path" + bin_dir.mkdir() + for tool in ("bash", "cat", "git", "python3", "dirname", "mktemp"): + real = shutil.which(tool) + if real: + (bin_dir / tool).symlink_to(real) + return str(bin_dir) + + +def test_denied_when_jq_missing(tmp_path: Path) -> None: + """Live-reproduced before this fix: with jq absent, the very first jq + call (extracting tool_name) crashed under `set -e` with exit 127 + ("command not found") -- before deny() was even defined, and + non-blocking per Claude Code's PreToolUse contract, so a PR carrying no + skill-audit disclosure would have been created unchecked. Must now deny + (exit 2) instead.""" + payload = json.dumps( + {"tool_name": "mcp__github__create_pull_request", "tool_input": {"base": "main", "body": "no evidence"}} + ) + result = subprocess.run( + ["bash", str(REPO_ROOT / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input=payload, + capture_output=True, + text=True, + timeout=60, + env=_hook_env(PATH=_no_jq_path(tmp_path)), + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + assert "jq is not available" in parsed["systemMessage"] + + +def test_denied_on_malformed_json_stdin() -> None: + """Live-reproduced before this fix: jq's own parse-error exit (5) + propagated past deny() under `set -e` -- non-blocking per Claude Code's + PreToolUse contract. Must now deny (exit 2) instead.""" + result = subprocess.run( + ["bash", str(REPO_ROOT / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input="not valid json{{{", + capture_output=True, + text=True, + timeout=60, + env=_hook_env(), + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_denied_when_tool_input_is_not_an_object() -> None: + """A well-formed top-level payload whose tool_input is itself a + non-object would otherwise crash the `.tool_input.body`/`.tool_input.base` + accesses with jq's own "Cannot index" error. Must deny.""" + payload = json.dumps({"tool_name": "mcp__github__create_pull_request", "tool_input": ["not", "an", "object"]}) + result = subprocess.run( + ["bash", str(REPO_ROOT / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input=payload, + capture_output=True, + text=True, + timeout=60, + env=_hook_env(), + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2 + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + def test_outside_a_git_work_tree_the_hook_stays_out_of_the_way(tmp_path: Path) -> None: """No repository means no diff to compute applicability from; CI's skill-audit-gate.yml remains the backstop.""" diff --git a/hooks/test_gitapex_check_template_overwrite.py b/hooks/test_gitapex_check_template_overwrite.py new file mode 100644 index 00000000..780dbba7 --- /dev/null +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -0,0 +1,207 @@ +"""Regression suite for check-template-overwrite.sh's own deny/allow matrix, +plus issue #1208's jq-missing/malformed-payload fail-closed guard. + +No automated coverage existed for this hook before issue #1208 -- the +sibling hooks touched by that issue (check-bash-safety.sh, +check-merge-pull-request-block.sh, check-pr-skill-audit-disclosure.sh) each +already had a suite; this file closes the same gap here rather than leaving +this hook's fix unverified. Same subprocess-driven style as +hooks/test_gitapex_check_bash_safety.py: runs the shipped script with the +PreToolUse JSON shape Claude Code sends on stdin, rather than re-deriving +the shell logic in Python. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).parent / "check-template-overwrite.sh" +REPO_ROOT = Path(__file__).parent.parent + + +def run( + file_path: str, tool_name: str = "Write", extra_env: dict[str, str] | None = None +) -> subprocess.CompletedProcess[str]: + payload = json.dumps({"tool_name": tool_name, "tool_input": {"file_path": file_path}}) + env = dict(os.environ) + if extra_env: + env.update(extra_env) + return subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + + +def assert_denied(file_path: str) -> None: + result = run(file_path) + assert result.returncode == 2, ( + f"expected deny (exit 2) for {file_path!r}, got {result.returncode}: stderr={result.stderr!r}" + ) + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + assert payload["systemMessage"] + + +def assert_allowed(file_path: str) -> None: + result = run(file_path) + assert result.returncode == 0, ( + f"expected allow (exit 0) for {file_path!r}, got {result.returncode}: stderr={result.stderr!r}" + ) + assert result.stdout == "" + assert result.stderr == "" + + +def _find_existing_template() -> str: + """A real, tracked template path in this checkout, so the deny path is + exercised against an actual `-f` hit rather than a synthesized fixture + file this test would need to create and clean up.""" + for candidate in ( + ".github/pull_request_template.md", + ".github/PULL_REQUEST_TEMPLATE.md", + "PULL_REQUEST_TEMPLATE.md", + "docs/PULL_REQUEST_TEMPLATE.md", + "pull_request_template.md", + ): + if (REPO_ROOT / candidate).is_file(): + return candidate + pytest.skip("no existing PR template file found in this checkout to test the overwrite-deny path against") + + +def test_denied_overwriting_the_real_pr_template() -> None: + assert_denied(_find_existing_template()) + + +@pytest.mark.parametrize( + "file_path", + [ + ".github/issue_template/not-there-yet.md", + ".github/pull_request_template/not-there-yet.md", + ".gitlab/issue_templates/not-there-yet.md", + ".gitlab/merge_request_templates/not-there-yet.md", + "pull_request_template.md", + "PULL_REQUEST_TEMPLATE.MD", + ], + ids=[ + "github-issue-template-dir-new-file", + "github-pr-template-dir-new-file", + "gitlab-issue-templates-dir-new-file", + "gitlab-mr-templates-dir-new-file", + "bare-pr-template-basename-not-present-here", + "uppercase-pr-template-basename-not-present-here", + ], +) +def test_allowed_new_template_path_not_yet_on_disk(file_path: str) -> None: + """A template-shaped path that does not yet exist on disk is a genuinely + new template, not an overwrite -- allowed regardless of case or which + template-path rule it matches.""" + assert not (REPO_ROOT / file_path).exists(), f"fixture assumption broken: {file_path} already exists" + assert_allowed(file_path) + + +def test_allowed_non_template_path_even_if_it_exists() -> None: + assert_allowed("hooks/check-template-overwrite.sh") + + +def test_allowed_when_no_file_path() -> None: + assert_allowed("") + + +def test_non_write_tool_name_is_ignored() -> None: + result = run(_find_existing_template(), tool_name="Edit") + assert result.returncode == 0 + assert result.stdout == "" + assert result.stderr == "" + + +# --------------------------------------------------------------------------- +# Issue #1208: fail closed, not open, when jq is missing or the payload is +# malformed. +# --------------------------------------------------------------------------- + + +def _no_jq_path(tmp_path: Path) -> str: + """A PATH directory holding every tool this script needs except jq, so + `command -v jq` genuinely fails the way it would in an environment + without jq installed -- rather than mocking that condition.""" + bin_dir = tmp_path / "no-jq-path" + bin_dir.mkdir() + for tool in ("bash", "cat", "tr", "grep", "sed", "dirname"): + real = shutil.which(tool) + if real: + (bin_dir / tool).symlink_to(real) + return str(bin_dir) + + +def test_denied_when_jq_missing(tmp_path: Path) -> None: + """Live-reproduced before this fix: with jq absent, every jq call under + `set -e` crashed with exit 127 ("command not found") -- non-blocking + per Claude Code's PreToolUse contract, so the overwrite proceeded + unchecked. Must now deny (exit 2) instead.""" + result = run(_find_existing_template(), extra_env={"PATH": _no_jq_path(tmp_path)}) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + assert "jq is not available" in payload["systemMessage"] + + +def test_denied_on_malformed_json_stdin() -> None: + """Live-reproduced before this fix: jq's own parse-error exit (5) + propagated past deny() under `set -e` -- non-blocking per Claude Code's + PreToolUse contract. Must now deny (exit 2) instead.""" + result = subprocess.run( + ["bash", str(SCRIPT)], + input="not valid json{{{", + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_denied_on_valid_json_non_object_stdin() -> None: + """Valid JSON that isn't an object (e.g. a bare array) would otherwise + crash the first field-extraction jq call the same way. Must deny.""" + result = subprocess.run( + ["bash", str(SCRIPT)], + input="[]", + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2 + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_denied_when_tool_input_is_not_an_object() -> None: + """A well-formed top-level payload whose tool_input is itself a + non-object (array/string/number/bool) would otherwise crash the + `.tool_input.file_path` access with jq's own "Cannot index" error. + Must deny.""" + payload = json.dumps({"tool_name": "Write", "tool_input": ["not", "an", "object"]}) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2 + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" From ad67a28b470b495d77d9e0e91b898c16ace84565 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:14:45 +0000 Subject: [PATCH 2/8] test(hooks): make the template-overwrite fixture self-contained The overwrite-deny tests scanned this checkout for a real, existing PR template file, falling back to pytest.skip when none was found. That fallback line was never executed in this repository's own CI (a template always exists here), so Codecov's patch-coverage gate flagged it as an uncovered line and failed the check. Replaces the scan with a tmp_path fixture that creates its own template file at an absolute path -- [ -f "$file_path" ] in the hook works the same for an absolute path regardless of cwd, so the deny path is still exercised against a real -f hit, now without depending on which template file(s) happen to exist in this repository or leaving an unreachable skip branch behind. Refs #1208 --- .../test_gitapex_check_template_overwrite.py | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/hooks/test_gitapex_check_template_overwrite.py b/hooks/test_gitapex_check_template_overwrite.py index 780dbba7..a0f733d5 100644 --- a/hooks/test_gitapex_check_template_overwrite.py +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -62,24 +62,21 @@ def assert_allowed(file_path: str) -> None: assert result.stderr == "" -def _find_existing_template() -> str: - """A real, tracked template path in this checkout, so the deny path is - exercised against an actual `-f` hit rather than a synthesized fixture - file this test would need to create and clean up.""" - for candidate in ( - ".github/pull_request_template.md", - ".github/PULL_REQUEST_TEMPLATE.md", - "PULL_REQUEST_TEMPLATE.md", - "docs/PULL_REQUEST_TEMPLATE.md", - "pull_request_template.md", - ): - if (REPO_ROOT / candidate).is_file(): - return candidate - pytest.skip("no existing PR template file found in this checkout to test the overwrite-deny path against") +@pytest.fixture +def existing_template_file(tmp_path: Path) -> str: + """An absolute, on-disk path matching the single-file PR template + basename rule. A tmp_path fixture rather than a scan over this + checkout's own template file(s): `[ -f "$file_path" ]` in the hook + works the same for an absolute path regardless of cwd, so the deny + path is exercised against a real `-f` hit without the test depending + on which template file(s) happen to exist in this repository.""" + template = tmp_path / "pull_request_template.md" + template.write_text("existing template body\n") + return str(template) -def test_denied_overwriting_the_real_pr_template() -> None: - assert_denied(_find_existing_template()) +def test_denied_overwriting_an_existing_template_file(existing_template_file: str) -> None: + assert_denied(existing_template_file) @pytest.mark.parametrize( @@ -117,8 +114,8 @@ def test_allowed_when_no_file_path() -> None: assert_allowed("") -def test_non_write_tool_name_is_ignored() -> None: - result = run(_find_existing_template(), tool_name="Edit") +def test_non_write_tool_name_is_ignored(existing_template_file: str) -> None: + result = run(existing_template_file, tool_name="Edit") assert result.returncode == 0 assert result.stdout == "" assert result.stderr == "" @@ -143,12 +140,12 @@ def _no_jq_path(tmp_path: Path) -> str: return str(bin_dir) -def test_denied_when_jq_missing(tmp_path: Path) -> None: +def test_denied_when_jq_missing(tmp_path: Path, existing_template_file: str) -> None: """Live-reproduced before this fix: with jq absent, every jq call under `set -e` crashed with exit 127 ("command not found") -- non-blocking per Claude Code's PreToolUse contract, so the overwrite proceeded unchecked. Must now deny (exit 2) instead.""" - result = run(_find_existing_template(), extra_env={"PATH": _no_jq_path(tmp_path)}) + result = run(existing_template_file, extra_env={"PATH": _no_jq_path(tmp_path)}) assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" payload = json.loads(result.stderr) assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" From de7e6bdedebdd77cfd16bca5a3d8614e3b1eaf91 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 15:31:18 +0000 Subject: [PATCH 3/8] fix(hooks): close tool_input:false gap CodeRabbit found in PR #1213 CodeRabbit's review of this PR found that the tool_input-shape guard these three hooks just gained -- (.tool_input // {}) | type == "object" -- accepts the JSON literal false the same way it accepts null or an absent key, since jq's // operator treats both as falsy. A tool_input: false payload therefore slipped past the guard and crashed the next jq field-extraction line with "Cannot index boolean with string ...", exit 5, past deny(), the same fail-open class this whole PR exists to close. Live-confirmed against all three affected hooks before fixing. Tightens the predicate to (.tool_input == null) or (.tool_input | type == "object"), verified correct against the full value matrix (absent, null, false, true, 0, array, string, object). Adds a false/true/zero regression case to each hook's existing non-object tool_input test, plus the [] top-level-array case check-merge-pull-request-block.py and two sibling test files were still missing (CodeRabbit's own nitpick finding). The same gap exists in the two sibling hooks this pattern was originally ported from (check-pr-issue-acm-disclosure.sh, check-pr-title-convention.sh) -- out of scope for this PR since neither is part of its diff; filed as gitapex#1216. Refs #1208 --- hooks/check-bash-safety.sh | 11 ++++- hooks/check-pr-skill-audit-disclosure.sh | 11 ++++- hooks/check-template-overwrite.sh | 11 ++++- hooks/test_gitapex_check_bash_safety.py | 40 ++++++++++++++++--- ..._gitapex_check_merge_pull_request_block.py | 22 ++++++++++ ...x_check_pr_skill_audit_disclosure_shell.py | 35 ++++++++++++++-- .../test_gitapex_check_template_overwrite.py | 20 +++++++--- 7 files changed, 128 insertions(+), 22 deletions(-) diff --git a/hooks/check-bash-safety.sh b/hooks/check-bash-safety.sh index 6cf1348c..3024a7a4 100755 --- a/hooks/check-bash-safety.sh +++ b/hooks/check-bash-safety.sh @@ -77,8 +77,15 @@ fi # in an otherwise well-formed payload, which would crash the # `.tool_input.command` access below with jq's own "Cannot index X with # string" runtime error -- same fail-open class as the top-level check -# above. -if ! printf '%s' "$input" | jq -e '(.tool_input // {}) | type == "object"' >/dev/null 2>&1; then +# above. `(.tool_input // {})` alone is not enough: jq's `//` treats JSON +# `false` the same as `null` (both are falsy), so a `tool_input: false` +# payload slipped past that form and crashed the extraction below anyway +# -- found by code review (PR #1213), live-confirmed with +# `jq -e '(.tool_input // {}) | type == "object"' <<< '{"tool_input":false}'`, +# which wrongly reports true. Checking `.tool_input == null` directly +# (true for both absent and explicit null, never for `false`) closes +# that gap. +if ! printf '%s' "$input" | jq -e '(.tool_input == null) or (.tool_input | type == "object")' >/dev/null 2>&1; then deny "Blocked by hooks/check-bash-safety.sh: tool_input in the payload is not a JSON object. Failing closed." fi diff --git a/hooks/check-pr-skill-audit-disclosure.sh b/hooks/check-pr-skill-audit-disclosure.sh index abe9c2d3..a60f9d39 100755 --- a/hooks/check-pr-skill-audit-disclosure.sh +++ b/hooks/check-pr-skill-audit-disclosure.sh @@ -106,8 +106,15 @@ esac # in an otherwise well-formed payload, which would crash the # `.tool_input.body`/`.tool_input.base` accesses below with jq's own # "Cannot index X with string" runtime error -- same fail-open class as the -# top-level check above. -if ! printf '%s' "$input" | jq -e '(.tool_input // {}) | type == "object"' >/dev/null 2>&1; then +# top-level check above. `(.tool_input // {})` alone is not enough: jq's +# `//` treats JSON `false` the same as `null` (both are falsy), so a +# `tool_input: false` payload slipped past that form and crashed the +# extraction below anyway -- found by code review (PR #1213), +# live-confirmed with `jq -e '(.tool_input // {}) | type == "object"' +# <<< '{"tool_input":false}'`, which wrongly reports true. Checking +# `.tool_input == null` directly (true for both absent and explicit null, +# never for `false`) closes that gap. +if ! printf '%s' "$input" | jq -e '(.tool_input == null) or (.tool_input | type == "object")' >/dev/null 2>&1; then deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: tool_input in the payload is not a JSON object. Failing closed." fi diff --git a/hooks/check-template-overwrite.sh b/hooks/check-template-overwrite.sh index ea8ab417..9df2e34f 100755 --- a/hooks/check-template-overwrite.sh +++ b/hooks/check-template-overwrite.sh @@ -57,8 +57,15 @@ fi # in an otherwise well-formed payload, which would crash the # `.tool_input.file_path` access below with jq's own "Cannot index X with # string" runtime error -- same fail-open class as the top-level check -# above. -if ! printf '%s' "$input" | jq -e '(.tool_input // {}) | type == "object"' >/dev/null 2>&1; then +# above. `(.tool_input // {})` alone is not enough: jq's `//` treats JSON +# `false` the same as `null` (both are falsy), so a `tool_input: false` +# payload slipped past that form and crashed the extraction below anyway +# -- found by code review (PR #1213), live-confirmed with +# `jq -e '(.tool_input // {}) | type == "object"' <<< '{"tool_input":false}'`, +# which wrongly reports true. Checking `.tool_input == null` directly +# (true for both absent and explicit null, never for `false`) closes +# that gap. +if ! printf '%s' "$input" | jq -e '(.tool_input == null) or (.tool_input | type == "object")' >/dev/null 2>&1; then deny "Blocked by hooks/check-template-overwrite.sh: tool_input in the payload is not a JSON object. Failing closed." fi diff --git a/hooks/test_gitapex_check_bash_safety.py b/hooks/test_gitapex_check_bash_safety.py index cbb1ae19..59431f8d 100644 --- a/hooks/test_gitapex_check_bash_safety.py +++ b/hooks/test_gitapex_check_bash_safety.py @@ -247,12 +247,20 @@ def test_denied_on_malformed_json_stdin() -> None: assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" -def test_denied_when_tool_input_is_not_an_object() -> None: +@pytest.mark.parametrize( + "tool_input", [["not", "an", "object"], False, True, 0], ids=["array", "false", "true", "zero"] +) +def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: """A well-formed top-level payload whose tool_input is itself a - non-object (array/string/number/bool) would otherwise crash the - `.tool_input.command` access with jq's own "Cannot index" error. Must - deny.""" - payload = json.dumps({"tool_name": "Bash", "tool_input": ["not", "an", "object"]}) + non-object would otherwise crash the `.tool_input.command` access with + jq's own "Cannot index" error. Must deny. + + `false` is the case that actually escaped the original guard: found by + code review (PR #1213) after the array/string cases above already + passed -- jq's `//` operator treats JSON `false` the same as `null` + (both are falsy), so `(.tool_input // {}) | type == "object"` wrongly + accepted it, and the crash happened one line later, past deny().""" + payload = json.dumps({"tool_name": "Bash", "tool_input": tool_input}) env = dict(os.environ) env.pop("CLAUDE_PROJECT_DIR", None) result = subprocess.run( @@ -264,11 +272,31 @@ def test_denied_when_tool_input_is_not_an_object() -> None: env=env, cwd=str(REPO_ROOT), ) - assert result.returncode == 2 + assert result.returncode == 2, f"expected deny (exit 2) for tool_input={tool_input!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" +def test_denied_on_valid_json_non_object_stdin() -> None: + """Valid JSON that isn't an object at the top level (e.g. a bare array) + would otherwise crash the first field-extraction jq call the same way. + Must deny.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input="[]", + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + payload = json.loads(result.stderr) + assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" + + # --------------------------------------------------------------------------- # Finding 4: git push gated (warn, not deny) on gitapex_scan_provenance.py # --------------------------------------------------------------------------- diff --git a/hooks/test_gitapex_check_merge_pull_request_block.py b/hooks/test_gitapex_check_merge_pull_request_block.py index 70e6ba49..24bbf670 100644 --- a/hooks/test_gitapex_check_merge_pull_request_block.py +++ b/hooks/test_gitapex_check_merge_pull_request_block.py @@ -168,3 +168,25 @@ def test_denied_on_malformed_json_stdin() -> None: assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_denied_on_valid_json_non_object_stdin() -> None: + """Valid JSON that isn't an object at the top level (e.g. a bare array) + would otherwise crash the `.tool_name` extraction the same way. This + hook cannot then tell whether it was a disguised merge_pull_request + call, so it must deny (exit 2) rather than fall through.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + env.pop("CLAUDE_PLUGIN_ROOT", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input="[]", + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" diff --git a/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py index 9d5ebcaa..7eed7eb8 100644 --- a/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py +++ b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py @@ -403,11 +403,20 @@ def test_denied_on_malformed_json_stdin() -> None: assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" -def test_denied_when_tool_input_is_not_an_object() -> None: +@pytest.mark.parametrize( + "tool_input", [["not", "an", "object"], False, True, 0], ids=["array", "false", "true", "zero"] +) +def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: """A well-formed top-level payload whose tool_input is itself a non-object would otherwise crash the `.tool_input.body`/`.tool_input.base` - accesses with jq's own "Cannot index" error. Must deny.""" - payload = json.dumps({"tool_name": "mcp__github__create_pull_request", "tool_input": ["not", "an", "object"]}) + accesses with jq's own "Cannot index" error. Must deny. + + `false` is the case that actually escaped the original guard: found by + code review (PR #1213) after the array/string cases above already + passed -- jq's `//` operator treats JSON `false` the same as `null` + (both are falsy), so `(.tool_input // {}) | type == "object"` wrongly + accepted it, and the crash happened one line later, past deny().""" + payload = json.dumps({"tool_name": "mcp__github__create_pull_request", "tool_input": tool_input}) result = subprocess.run( ["bash", str(REPO_ROOT / "hooks" / "check-pr-skill-audit-disclosure.sh")], input=payload, @@ -417,7 +426,25 @@ def test_denied_when_tool_input_is_not_an_object() -> None: env=_hook_env(), cwd=str(REPO_ROOT), ) - assert result.returncode == 2 + assert result.returncode == 2, f"expected deny (exit 2) for tool_input={tool_input!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_denied_on_valid_json_non_object_stdin() -> None: + """Valid JSON that isn't an object at the top level (e.g. a bare array) + would otherwise crash the first field-extraction jq call the same way. + Must deny.""" + result = subprocess.run( + ["bash", str(REPO_ROOT / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input="[]", + capture_output=True, + text=True, + timeout=60, + env=_hook_env(), + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" diff --git a/hooks/test_gitapex_check_template_overwrite.py b/hooks/test_gitapex_check_template_overwrite.py index a0f733d5..ef592427 100644 --- a/hooks/test_gitapex_check_template_overwrite.py +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -185,12 +185,20 @@ def test_denied_on_valid_json_non_object_stdin() -> None: assert payload["hookSpecificOutput"]["permissionDecision"] == "deny" -def test_denied_when_tool_input_is_not_an_object() -> None: +@pytest.mark.parametrize( + "tool_input", [["not", "an", "object"], False, True, 0], ids=["array", "false", "true", "zero"] +) +def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: """A well-formed top-level payload whose tool_input is itself a - non-object (array/string/number/bool) would otherwise crash the - `.tool_input.file_path` access with jq's own "Cannot index" error. - Must deny.""" - payload = json.dumps({"tool_name": "Write", "tool_input": ["not", "an", "object"]}) + non-object would otherwise crash the `.tool_input.file_path` access + with jq's own "Cannot index" error. Must deny. + + `false` is the case that actually escaped the original guard: found by + code review (PR #1213) after the array/string cases above already + passed -- jq's `//` operator treats JSON `false` the same as `null` + (both are falsy), so `(.tool_input // {}) | type == "object"` wrongly + accepted it, and the crash happened one line later, past deny().""" + payload = json.dumps({"tool_name": "Write", "tool_input": tool_input}) result = subprocess.run( ["bash", str(SCRIPT)], input=payload, @@ -199,6 +207,6 @@ def test_denied_when_tool_input_is_not_an_object() -> None: timeout=10, cwd=str(REPO_ROOT), ) - assert result.returncode == 2 + assert result.returncode == 2, f"expected deny (exit 2) for tool_input={tool_input!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" From 0b0a3afc09f5fff6a8d16753c4cd4acbced9ded2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 16:17:03 +0000 Subject: [PATCH 4/8] fix(hooks): close non-string tool_name bypass and unguarded mktemp An independent adversarial correctness review dispatched against this PR found the most severe gap yet in the ported guard prologue: 1. jq -r never errors on a non-string .tool_name (e.g. an array ["Bash"]) -- it pretty-prints the JSON value across multiple lines instead, which then never equals the plain expected tool-name string (or matches a case pattern) the "defense in depth, don't trust hooks.json's matcher alone" re-check further down relies on. That silently falls through as "not our tool" (exit 0) instead of failing closed. Live-confirmed across all four hooks before fixing, most severely on check-merge-pull-request-block.sh: an array-wrapped tool_name let a real merge_pull_request call straight through this repository's own categorical "no override" deny -- the exact bypass class issue #1208 exists to close, just via a different field than the one it named. Fixed with the same predicate shape already proven for tool_input: (.tool_name == null) or (.tool_name | type == "string"), verified against the full value matrix. 2. Two unguarded `var=$(mktemp)` calls in check-pr-skill-audit- disclosure.sh's tier-1/tier-2 logic crashed the whole script under set -e on an unwritable/full TMPDIR, with mktemp's own exit code (non-2, non-blocking) instead of the intended degrade-to-tier-2- then-CI fallback every other tier-1-incomplete path in this hook already takes. Live-confirmed the crash before fixing; both call sites now catch the failure and fall through with a warning, exactly like the file's own documented fail-open-on-inconclusive-local-state posture already does for every other tier-1 failure mode. Regression tests added for both: a non-string tool_name (array/object/ number/bool) case for all four hooks, and a broken-TMPDIR case for check-pr-skill-audit-disclosure.sh's own fall-through. The identical tool_name-type gap exists in the two sibling hooks this pattern was originally ported from (check-pr-issue-acm-disclosure.sh, check-pr-title-convention.sh) -- confirmed by the same review agent. Out of scope for this PR since neither is part of its diff; filed as gitapex#1217 (alongside gitapex#1216's own tool_input:false finding in the same two files). Refs #1208 --- hooks/check-bash-safety.sh | 27 ++++- hooks/check-merge-pull-request-block.sh | 19 ++++ hooks/check-pr-skill-audit-disclosure.sh | 102 ++++++++++++------ hooks/check-template-overwrite.sh | 14 +++ hooks/test_gitapex_check_bash_safety.py | 91 +++++++++++++++- ..._gitapex_check_merge_pull_request_block.py | 32 ++++++ ...x_check_pr_skill_audit_disclosure_shell.py | 90 +++++++++++++++- .../test_gitapex_check_template_overwrite.py | 49 ++++++++- 8 files changed, 385 insertions(+), 39 deletions(-) diff --git a/hooks/check-bash-safety.sh b/hooks/check-bash-safety.sh index 3024a7a4..1c17d64d 100755 --- a/hooks/check-bash-safety.sh +++ b/hooks/check-bash-safety.sh @@ -46,10 +46,19 @@ deny() { # tool call to proceed (exit 0). Used where the underlying check is # documented as advisory (surfaces candidates, does not decide) rather than # a deterministic write/read classifier -- see the git-push handling below. +# Found by code review (PR #1213): its only call site interpolates +# $scan_output, the provenance scan's own report over the *entire* outgoing +# push's commit messages and patches -- large enough on a big branch to +# blow the OS's ARG_MAX the same way `deny()`'s own pre-hardening form did +# (live-confirmed: `jq -n --arg msg "$BIG"` on a 3MB string exits 126, +# "Argument list too long"). Under `set -euo pipefail` that crash aborts +# the whole script before `exit 0`, past this function's own advisory +# intent -- the push still proceeds either way (any non-2 exit is +# non-blocking), but the warning itself is silently lost instead of +# reaching the operator. Same `jq -Rs` piped-stdin fix as deny() above. warn() { local reason="$1" - jq -n --arg msg "$reason" \ - '{"systemMessage": $msg}' + printf '%s' "$reason" | jq -Rs '{"systemMessage": .}' exit 0 } @@ -65,6 +74,20 @@ if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/ deny "Blocked by hooks/check-bash-safety.sh: the tool-call payload on stdin is not a JSON object. Failing closed." fi +# Found by code review (PR #1213): jq -r never errors on a non-string +# `.tool_name` (e.g. `["Bash"]`) -- it pretty-prints the JSON form across +# multiple lines instead, which then never equals the plain "Bash" string +# the check below compares against. That silently falls through as "not +# our tool" (exit 0) rather than failing closed on a malformed field this +# gate structurally depends on -- live-confirmed: an array-wrapped +# tool_name let a `gh pr merge` command straight through this hook. +# `.tool_name == null` covers both absent and explicit null (an absent +# key indexes as null in jq); only a present non-string, non-null value +# denies. +if ! printf '%s' "$input" | jq -e '(.tool_name == null) or (.tool_name | type == "string")' >/dev/null 2>&1; then + deny "Blocked by hooks/check-bash-safety.sh: tool_name in the payload is not a string. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matcher already restricts this hook to diff --git a/hooks/check-merge-pull-request-block.sh b/hooks/check-merge-pull-request-block.sh index d6f89c2a..98f08c2c 100755 --- a/hooks/check-merge-pull-request-block.sh +++ b/hooks/check-merge-pull-request-block.sh @@ -67,6 +67,25 @@ if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/ deny "Blocked by hooks/check-merge-pull-request-block.sh: the tool-call payload on stdin is not a JSON object, and mcp__github__merge_pull_request is never a valid agent action in this repository regardless. Failing closed." fi +# Found by code review (PR #1213): jq -r never errors on a non-string +# `.tool_name` (e.g. `["mcp__github__merge_pull_request"]`) -- it +# pretty-prints the JSON form across multiple lines instead, which then +# never equals the plain string the check below compares against. That +# silently falls through as "not our tool" (exit 0) rather than failing +# closed on a malformed field this hook's own "no override" categorical +# deny structurally depends on -- live-confirmed: an array-wrapped +# tool_name let a merge_pull_request call straight through this hook, the +# exact bypass class this file exists to close. Same +# fail-closed-on-INDETERMINATE reasoning as the payload-shape check +# above: this hook cannot tell whether a malformed tool_name is a +# disguised merge_pull_request call, so it denies rather than assumes +# not. `.tool_name == null` covers both absent and explicit null (an +# absent key indexes as null in jq); only a present non-string, non-null +# value denies. +if ! printf '%s' "$input" | jq -e '(.tool_name == null) or (.tool_name | type == "string")' >/dev/null 2>&1; then + deny "Blocked by hooks/check-merge-pull-request-block.sh: tool_name in the payload is not a string, and mcp__github__merge_pull_request is never a valid agent action in this repository regardless. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matcher already restricts this hook to diff --git a/hooks/check-pr-skill-audit-disclosure.sh b/hooks/check-pr-skill-audit-disclosure.sh index a60f9d39..ecbfb939 100755 --- a/hooks/check-pr-skill-audit-disclosure.sh +++ b/hooks/check-pr-skill-audit-disclosure.sh @@ -93,6 +93,20 @@ if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/ deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: the tool-call payload on stdin is not a JSON object. Failing closed." fi +# Found by code review (PR #1213): jq -r never errors on a non-string +# `.tool_name` (e.g. `["mcp__github__create_pull_request"]`) -- it +# pretty-prints the JSON form across multiple lines instead, which then +# never matches the `case` pattern below. That silently falls through as +# "not our tool" (exit 0) rather than failing closed on a malformed field +# this gate structurally depends on -- live-confirmed: an array-wrapped +# tool_name let a PR-creation call straight through this hook. +# `.tool_name == null` covers both absent and explicit null (an absent +# key indexes as null in jq); only a present non-string, non-null value +# denies. +if ! printf '%s' "$input" | jq -e '(.tool_name == null) or (.tool_name | type == "string")' >/dev/null 2>&1; then + deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: tool_name in the payload is not a string. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matchers already restrict this hook to @@ -192,50 +206,70 @@ if [ "$base_is_explicit" = "no" ]; then fi if [ "$base_is_explicit" = "yes" ] && [ -n "$repo_root" ] && [ -f "$full_gate" ] && [ -f "$flag_module" ]; then - body_file=$(mktemp) - printf '%s' "$body" >"$body_file" - if full_output=$(cd "$repo_root" && python3 "$full_gate" \ - --check-diff "$merge_base" HEAD --body-file "$body_file" 2>&1); then - full_exit=0 - else - full_exit=$? - fi - rm -f "$body_file" + # Found by code review (PR #1213): an unguarded `body_file=$(mktemp)` + # crashes the whole script under `set -e` (e.g. an unwritable/full + # /tmp), past every deny() and past this block's own fall-through-to- + # tier-2 design, with mktemp's own exit code -- an exit Claude Code's + # PreToolUse contract treats as non-blocking, i.e. an ungated pass + # through the local pre-check instead of the intended degrade-to-tier-2 + # path every OTHER tier-1 failure in this block already takes. + # Live-confirmed: `TMPDIR=/nonexistent-dir bash check-pr-skill-audit- + # disclosure.sh` crashed with mktemp's own exit 1, not falling through. + if body_file=$(mktemp 2>/dev/null); then + printf '%s' "$body" >"$body_file" + if full_output=$(cd "$repo_root" && python3 "$full_gate" \ + --check-diff "$merge_base" HEAD --body-file "$body_file" 2>&1); then + full_exit=0 + else + full_exit=$? + fi + rm -f "$body_file" - if [ "$full_exit" -eq 0 ]; then - exit 0 - fi + if [ "$full_exit" -eq 0 ]; then + exit 0 + fi - # `grep -q` closes stdin on first match, which can SIGPIPE a still-writing - # upstream; under `set -o pipefail` (set above) that upstream's nonzero - # status outranks grep's own zero exit and turns a real match into a false - # "not found" -- i.e. a genuine deny silently downgraded to the warning - # fall-through below. This repository banned the pattern in - # https://github.com/tvna/gitapex/pull/428#discussion_r3654041066 and - # skill-audit-gate.yml's own history records the same fix; `-q` is dropped - # and the output redirected instead, so grep always reads to completion. - if printf '%s' "$full_output" | grep '^FAIL:' >/dev/null; then - # The exact command is in the message on purpose (dimension 17): the - # whole point of issue #874 is that an agent can now iterate on the - # disclosure locally instead of pushing and reading a failed check, and - # a deny that does not say how to re-run the verdict leaves it doing - # the latter anyway. - deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: this PR's diff requires skill-audit disclosure evidence its body does not carry. This is the same verdict .github/workflows/skill-audit-gate.yml will report, computed locally before the push. Fix the '## Skill audit evidence' section, then re-check with: + # `grep -q` closes stdin on first match, which can SIGPIPE a still-writing + # upstream; under `set -o pipefail` (set above) that upstream's nonzero + # status outranks grep's own zero exit and turns a real match into a false + # "not found" -- i.e. a genuine deny silently downgraded to the warning + # fall-through below. This repository banned the pattern in + # https://github.com/tvna/gitapex/pull/428#discussion_r3654041066 and + # skill-audit-gate.yml's own history records the same fix; `-q` is dropped + # and the output redirected instead, so grep always reads to completion. + if printf '%s' "$full_output" | grep '^FAIL:' >/dev/null; then + # The exact command is in the message on purpose (dimension 17): the + # whole point of issue #874 is that an agent can now iterate on the + # disclosure locally instead of pushing and reading a failed check, and + # a deny that does not say how to re-run the verdict leaves it doing + # the latter anyway. + deny "Blocked by hooks/check-pr-skill-audit-disclosure.sh: this PR's diff requires skill-audit disclosure evidence its body does not carry. This is the same verdict .github/workflows/skill-audit-gate.yml will report, computed locally before the push. Fix the '## Skill audit evidence' section, then re-check with: python3 .github/scripts/gitapex_gate_skill_audit_disclosure.py --check-diff ${merge_base} HEAD --body-file $full_output" - fi + fi - # Not a verdict on the body: the local flag computation itself could not - # complete (unreadable gate registry, a ref this checkout cannot resolve, - # a bug in the wrapper). Fall through to the bundled partial check rather - # than denying on an answer that was never computed. - echo "Warning: hooks/check-pr-skill-audit-disclosure.sh could not complete the full local pre-check (exit $full_exit); falling back to the bundled base two-audit check (CI's skill-audit-gate.yml remains authoritative). Output: $full_output" >&2 + # Not a verdict on the body: the local flag computation itself could not + # complete (unreadable gate registry, a ref this checkout cannot resolve, + # a bug in the wrapper). Fall through to the bundled partial check rather + # than denying on an answer that was never computed. + echo "Warning: hooks/check-pr-skill-audit-disclosure.sh could not complete the full local pre-check (exit $full_exit); falling back to the bundled base two-audit check (CI's skill-audit-gate.yml remains authoritative). Output: $full_output" >&2 + else + echo "Warning: hooks/check-pr-skill-audit-disclosure.sh could not create a temp file for the tier-1 body check (mktemp failed); falling back to the bundled base two-audit check (CI's skill-audit-gate.yml remains authoritative)." >&2 + fi fi # --- tier 2: the bundled, SKILL.md-only base check --- -diff_error=$(mktemp) +# Found by code review (PR #1213): same unguarded-mktemp-crashes-under- +# set-e class as the tier-1 body_file above -- an unwritable/full /tmp +# would otherwise crash past this hook's own "skip the local pre-check, +# CI remains authoritative" fallback with mktemp's own exit code, an +# exit Claude Code's PreToolUse contract treats as non-blocking. +if ! diff_error=$(mktemp 2>/dev/null); then + echo "Warning: hooks/check-pr-skill-audit-disclosure.sh could not create a temp file for git-diff error capture (mktemp failed); skipping the local pre-check (CI's skill-audit-gate.yml will still catch this)." >&2 + exit 0 +fi if ! diff_output=$(git diff --name-status "${merge_base}...HEAD" -- 'skills/*/SKILL.md' 2>"$diff_error"); then echo "Warning: hooks/check-pr-skill-audit-disclosure.sh's local git diff failed; skipping the local pre-check (CI's skill-audit-gate.yml will still catch this). $(cat "$diff_error")" >&2 rm -f "$diff_error" diff --git a/hooks/check-template-overwrite.sh b/hooks/check-template-overwrite.sh index 9df2e34f..da9f2776 100755 --- a/hooks/check-template-overwrite.sh +++ b/hooks/check-template-overwrite.sh @@ -45,6 +45,20 @@ if ! printf '%s' "$input" | jq -e 'if type == "object" then . else empty end' >/ deny "Blocked by hooks/check-template-overwrite.sh: the tool-call payload on stdin is not a JSON object. Failing closed." fi +# Found by code review (PR #1213): jq -r never errors on a non-string +# `.tool_name` (e.g. `["Write"]`) -- it pretty-prints the JSON form across +# multiple lines instead, which then never equals the plain "Write" string +# the check below compares against. That silently falls through as "not +# our tool" (exit 0) rather than failing closed on a malformed field this +# gate structurally depends on -- live-confirmed: an array-wrapped +# tool_name let an overwrite of the real .github/PULL_REQUEST_TEMPLATE.md +# straight through this hook. `.tool_name == null` covers both absent and +# explicit null (an absent key indexes as null in jq); only a present +# non-string, non-null value denies. +if ! printf '%s' "$input" | jq -e '(.tool_name == null) or (.tool_name | type == "string")' >/dev/null 2>&1; then + deny "Blocked by hooks/check-template-overwrite.sh: tool_name in the payload is not a string. Failing closed." +fi + tool_name=$(printf '%s' "$input" | jq -r '.tool_name // empty') # Defense in depth: the hooks.json matcher already restricts this hook to diff --git a/hooks/test_gitapex_check_bash_safety.py b/hooks/test_gitapex_check_bash_safety.py index 59431f8d..429dadd3 100644 --- a/hooks/test_gitapex_check_bash_safety.py +++ b/hooks/test_gitapex_check_bash_safety.py @@ -248,7 +248,9 @@ def test_denied_on_malformed_json_stdin() -> None: @pytest.mark.parametrize( - "tool_input", [["not", "an", "object"], False, True, 0], ids=["array", "false", "true", "zero"] + "tool_input", + [["not", "an", "object"], "text", False, True, 0], + ids=["array", "string", "false", "true", "zero"], ) def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: """A well-formed top-level payload whose tool_input is itself a @@ -277,6 +279,57 @@ def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" +def test_allowed_when_tool_input_is_absent_or_null() -> None: + """jq indexes `null`/a missing key as `null`, not a runtime error, so + these fall through the shape guard to the hook's own downstream logic + (an empty `command` here, which is itself allowed) rather than being + wrongly caught by it -- unlike the non-object shapes above.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + for payload in ( + json.dumps({"tool_name": "Bash"}), + json.dumps({"tool_name": "Bash", "tool_input": None}), + ): + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 0, f"payload={payload!r}: expected allow, got {result.returncode}" + assert result.stdout == "" + assert result.stderr == "" + + +@pytest.mark.parametrize("tool_name", [["Bash"], {"x": 1}, 5, True], ids=["array", "object", "number", "bool"]) +def test_denied_when_tool_name_is_not_a_string(tool_name: object) -> None: + """Found by code review (PR #1213): jq -r never errors on a non-string + `.tool_name` -- it pretty-prints the JSON form across multiple lines + instead, which then never equals the plain "Bash" string the matcher + re-check compares against, silently falling through as "not our tool" + (exit 0) instead of failing closed. Live-confirmed before this guard + existed: an array-wrapped tool_name let a `gh pr merge` command + straight through. Must now deny.""" + payload = json.dumps({"tool_name": tool_name, "tool_input": {"command": "gh pr merge 1"}}) + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + def test_denied_on_valid_json_non_object_stdin() -> None: """Valid JSON that isn't an object at the top level (e.g. a bare array) would otherwise crash the first field-extraction jq call the same way. @@ -400,6 +453,42 @@ def test_git_push_warns_when_scan_flags_a_hit(tmp_path: Path) -> None: assert "flagged the outgoing push for review" in payload["systemMessage"] +def _project_with_huge_warning_scan_script(tmp_path: Path, *, size: int = 3_000_000) -> Path: + """A project dir whose scan script is a stand-in, not the real + gitapex_scan_provenance.py: it always exits 1 with `size` bytes of + output, to exercise warn()'s own robustness against a large message in + isolation from the real scanner's detection logic (covered by that + script's own test suite elsewhere).""" + project_dir = tmp_path / "project" + scan_dir = project_dir / "skills" / "outward-artifact-preflight" / "scripts" + scan_dir.mkdir(parents=True) + (scan_dir / "gitapex_scan_provenance.py").write_text(f"import sys\nsys.stdout.write('A' * {size})\nsys.exit(1)\n") + return project_dir + + +def test_git_push_warn_survives_a_huge_scan_message(tmp_path: Path) -> None: + """Found by code review (PR #1213): warn()'s own pre-fix form (`jq -n + --arg`) crashed with exit 126 ("Argument list too long") on a + message this large -- live-confirmed before the fix, via the same + construction used here. Under `set -euo pipefail` that crash would + abort the whole script before `exit 0`; the push still proceeds + either way (any non-2 exit is non-blocking per Claude Code's + PreToolUse contract), but the warning itself would be silently lost + instead of reaching the operator. Must now exit 0 with the full + message intact.""" + project_dir = _project_with_huge_warning_scan_script(tmp_path) + _init_diverged_repo(project_dir, feature_commit_messages=["Fix bug in parser"]) + result = run( + "git push origin HEAD", + extra_env={"CLAUDE_PROJECT_DIR": str(project_dir)}, + ) + assert result.returncode == 0, f"expected allow (exit 0), got {result.returncode}: stderr={result.stderr[:500]!r}" + assert result.stderr == "" + payload = json.loads(result.stdout) + assert "flagged the outgoing push for review" in payload["systemMessage"] + assert len(payload["systemMessage"]) > 3_000_000 + + def test_git_push_silent_when_scan_finds_nothing(tmp_path: Path) -> None: project_dir = _project_with_scan_script(tmp_path) _init_diverged_repo(project_dir, feature_commit_messages=["Fix bug in parser"]) diff --git a/hooks/test_gitapex_check_merge_pull_request_block.py b/hooks/test_gitapex_check_merge_pull_request_block.py index 24bbf670..7f0de807 100644 --- a/hooks/test_gitapex_check_merge_pull_request_block.py +++ b/hooks/test_gitapex_check_merge_pull_request_block.py @@ -23,6 +23,8 @@ import subprocess from pathlib import Path +import pytest + SCRIPT = Path(__file__).parent / "check-merge-pull-request-block.sh" REPO_ROOT = Path(__file__).parent.parent @@ -190,3 +192,33 @@ def test_denied_on_valid_json_non_object_stdin() -> None: assert result.returncode == 2, f"expected deny (exit 2), got {result.returncode}: stderr={result.stderr!r}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +@pytest.mark.parametrize( + "tool_name", [["mcp__github__merge_pull_request"], {"x": 1}, 5, True], ids=["array", "object", "number", "bool"] +) +def test_denied_when_tool_name_is_not_a_string(tool_name: object) -> None: + """Found by code review (PR #1213): jq -r never errors on a non-string + `.tool_name` -- it pretty-prints the JSON form across multiple lines + instead, which then never equals the plain string this hook's own + "no override" categorical deny compares against, silently falling + through as "not our tool" (exit 0) instead of failing closed. + Live-confirmed before this guard existed: an array-wrapped tool_name + let a merge_pull_request call straight through this hook -- the exact + bypass class this file exists to close. Must now deny.""" + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + env.pop("CLAUDE_PLUGIN_ROOT", None) + payload = json.dumps({"tool_name": tool_name, "tool_input": {"owner": "tvna", "repo": "gitapex", "pullNumber": 1}}) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" diff --git a/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py index 7eed7eb8..94dbe8cc 100644 --- a/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py +++ b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py @@ -404,7 +404,9 @@ def test_denied_on_malformed_json_stdin() -> None: @pytest.mark.parametrize( - "tool_input", [["not", "an", "object"], False, True, 0], ids=["array", "false", "true", "zero"] + "tool_input", + [["not", "an", "object"], "text", False, True, 0], + ids=["array", "string", "false", "true", "zero"], ) def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: """A well-formed top-level payload whose tool_input is itself a @@ -431,6 +433,31 @@ def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" +def test_an_update_call_with_absent_or_null_tool_input_is_ignored(repo: Path) -> None: + """jq indexes `null`/a missing key as `null`, not a runtime error, so + absent/explicit-null tool_input falls through the shape guard to the + same empty-body bypass test_an_update_call_with_no_body_is_ignored + exercises for `{}`, rather than being wrongly caught by the guard + itself.""" + payloads = ( + {"tool_name": "mcp__github__update_pull_request"}, + {"tool_name": "mcp__github__update_pull_request", "tool_input": None}, + ) + for payload_dict in payloads: + result = subprocess.run( + ["bash", str(repo / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input=json.dumps(payload_dict), + capture_output=True, + text=True, + timeout=60, + env=_hook_env(), + cwd=str(repo), + ) + assert result.returncode == 0, ( + f"payload={payload_dict!r}: expected allow, got {result.returncode}: {result.stderr!r}" + ) + + def test_denied_on_valid_json_non_object_stdin() -> None: """Valid JSON that isn't an object at the top level (e.g. a bare array) would otherwise crash the first field-extraction jq call the same way. @@ -449,6 +476,67 @@ def test_denied_on_valid_json_non_object_stdin() -> None: assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" +@pytest.mark.parametrize( + "tool_name", + [["mcp__github__create_pull_request"], {"x": 1}, 5, True], + ids=["array", "object", "number", "bool"], +) +def test_denied_when_tool_name_is_not_a_string(tool_name: object) -> None: + """Found by code review (PR #1213): jq -r never errors on a non-string + `.tool_name` -- it pretty-prints the JSON form across multiple lines + instead, which then never matches the `case` pattern below, silently + falling through as "not our tool" (exit 0) instead of failing closed. + Live-confirmed before this guard existed: an array-wrapped tool_name + let a PR-creation call straight through this hook. Must now deny.""" + payload = json.dumps({"tool_name": tool_name, "tool_input": {"base": "main", "body": "no evidence"}}) + result = subprocess.run( + ["bash", str(REPO_ROOT / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input=payload, + capture_output=True, + text=True, + timeout=60, + env=_hook_env(), + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_falls_through_to_exit_0_when_mktemp_is_broken(repo: Path, tmp_path: Path) -> None: + """Found by code review (PR #1213): both mktemp call sites (tier-1's + body_file, tier-2's diff_error) were unguarded, so an unwritable/full + TMPDIR crashed the whole script under `set -e` with mktemp's own exit + code instead of the intended degrade-to-tier-2-then-CI fallback every + OTHER tier-1-incomplete path in this hook already takes. Live- + confirmed before this guard existed: + `TMPDIR=/nonexistent-dir bash check-pr-skill-audit-disclosure.sh` + crashed with mktemp's own exit 1, not falling through. Must now warn + and fall all the way through to exit 0 (CI remains authoritative), + the same outcome a broken TMPDIR should have regardless of what the + PR body discloses -- the local check simply cannot run at all.""" + _with_tier1(repo) + _write(repo, ".github/scripts/gitapex_gate_new.py") + _commit(repo, "new gate") + broken_tmpdir = tmp_path / "does-not-exist" + payload = json.dumps( + {"tool_name": "mcp__github__create_pull_request", "tool_input": {"base": "main", "body": "no evidence"}} + ) + result = subprocess.run( + ["bash", str(repo / "hooks" / "check-pr-skill-audit-disclosure.sh")], + input=payload, + capture_output=True, + text=True, + timeout=60, + env=_hook_env(TMPDIR=str(broken_tmpdir)), + cwd=str(repo), + ) + assert result.returncode == 0, ( + f"expected fall-through to allow (exit 0), got {result.returncode}: {result.stderr!r}" + ) + assert "could not create a temp file" in result.stderr + + def test_outside_a_git_work_tree_the_hook_stays_out_of_the_way(tmp_path: Path) -> None: """No repository means no diff to compute applicability from; CI's skill-audit-gate.yml remains the backstop.""" diff --git a/hooks/test_gitapex_check_template_overwrite.py b/hooks/test_gitapex_check_template_overwrite.py index ef592427..a5c4cf6f 100644 --- a/hooks/test_gitapex_check_template_overwrite.py +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -186,7 +186,9 @@ def test_denied_on_valid_json_non_object_stdin() -> None: @pytest.mark.parametrize( - "tool_input", [["not", "an", "object"], False, True, 0], ids=["array", "false", "true", "zero"] + "tool_input", + [["not", "an", "object"], "text", False, True, 0], + ids=["array", "string", "false", "true", "zero"], ) def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: """A well-formed top-level payload whose tool_input is itself a @@ -210,3 +212,48 @@ def test_denied_when_tool_input_is_not_an_object(tool_input: object) -> None: assert result.returncode == 2, f"expected deny (exit 2) for tool_input={tool_input!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +def test_allowed_when_tool_input_is_absent_or_null() -> None: + """jq indexes `null`/a missing key as `null`, not a runtime error, so + these fall through the shape guard to the hook's own downstream logic + (an empty `file_path` here, which is itself allowed) rather than being + wrongly caught by it -- unlike the non-object shapes above.""" + for payload in ( + json.dumps({"tool_name": "Write"}), + json.dumps({"tool_name": "Write", "tool_input": None}), + ): + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 0, f"payload={payload!r}: expected allow, got {result.returncode}" + assert result.stdout == "" + assert result.stderr == "" + + +@pytest.mark.parametrize("tool_name", [["Write"], {"x": 1}, 5, True], ids=["array", "object", "number", "bool"]) +def test_denied_when_tool_name_is_not_a_string(tool_name: object, existing_template_file: str) -> None: + """Found by code review (PR #1213): jq -r never errors on a non-string + `.tool_name` -- it pretty-prints the JSON form across multiple lines + instead, which then never equals the plain "Write" string the matcher + re-check compares against, silently falling through as "not our tool" + (exit 0) instead of failing closed. Live-confirmed before this guard + existed: an array-wrapped tool_name let an overwrite of the real PR + template straight through. Must now deny.""" + payload = json.dumps({"tool_name": tool_name, "tool_input": {"file_path": existing_template_file}}) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" From 3236ee77d80090b92798b27a90304d5560a6d7c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 16:51:59 +0000 Subject: [PATCH 5/8] fix(hooks): fail closed when command/file_path leaf fields are non-string jq -r never errors on a non-string .tool_input.command or .tool_input.file_path -- it pretty-prints the JSON value across multiple lines, which breaks every whitespace-anchored danger-pattern regex in check-bash-safety.sh and both the basename match and -f test in check-template-overwrite.sh. Live-confirmed: an array-wrapped ["gh","pr","merge","1"] command, and an array-wrapped file_path targeting the real .github/PULL_REQUEST_TEMPLATE.md, both bypassed their respective hook before this fix. Refs #1208 --- hooks/check-bash-safety.sh | 14 ++++++++ hooks/check-template-overwrite.sh | 14 ++++++++ hooks/test_gitapex_check_bash_safety.py | 32 +++++++++++++++++++ .../test_gitapex_check_template_overwrite.py | 30 +++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/hooks/check-bash-safety.sh b/hooks/check-bash-safety.sh index 1c17d64d..a7511a56 100755 --- a/hooks/check-bash-safety.sh +++ b/hooks/check-bash-safety.sh @@ -112,6 +112,20 @@ if ! printf '%s' "$input" | jq -e '(.tool_input == null) or (.tool_input | type deny "Blocked by hooks/check-bash-safety.sh: tool_input in the payload is not a JSON object. Failing closed." fi +# Issue #1208 (round 4): a well-formed, object-shaped tool_input can still +# carry `.tool_input.command` as a JSON array or object (e.g. +# `["gh","pr","merge","1"]`) instead of a string. `jq -r` never errors on +# this -- it pretty-prints the value across multiple lines, which splits +# the dangerous substring across JSON punctuation (quotes, commas, +# brackets) and breaks every `[[:space:]]`-anchored danger-pattern regex +# below, silently letting a genuinely dangerous command through with +# exit 0 instead of exit 2 -- found by code review (PR #1213), +# live-confirmed against `gh pr merge`, `pip install`, and `gh api -X +# POST` payloads wrapped as arrays. Must deny before extraction. +if ! printf '%s' "$input" | jq -e '(.tool_input.command == null) or (.tool_input.command | type == "string")' >/dev/null 2>&1; then + deny "Blocked by hooks/check-bash-safety.sh: tool_input.command in the payload is not a string. Failing closed." +fi + command=$(printf '%s' "$input" | jq -r '.tool_input.command // empty') if [ -z "$command" ]; then diff --git a/hooks/check-template-overwrite.sh b/hooks/check-template-overwrite.sh index da9f2776..1839448e 100755 --- a/hooks/check-template-overwrite.sh +++ b/hooks/check-template-overwrite.sh @@ -83,6 +83,20 @@ if ! printf '%s' "$input" | jq -e '(.tool_input == null) or (.tool_input | type deny "Blocked by hooks/check-template-overwrite.sh: tool_input in the payload is not a JSON object. Failing closed." fi +# Issue #1208 (round 4): a well-formed, object-shaped tool_input can still +# carry `.tool_input.file_path` as a JSON array (e.g. +# `[".github/PULL_REQUEST_TEMPLATE.md"]`) instead of a string. `jq -r` +# never errors on this -- it pretty-prints the value across multiple +# lines, which breaks both `is_template_path()`'s basename matching and +# `[ -f "$file_path" ]`, silently letting an overwrite of a real, +# existing template file through with exit 0 instead of exit 2 -- found +# by code review (PR #1213), live-confirmed against the actual +# `.github/PULL_REQUEST_TEMPLATE.md` in this repository. Must deny +# before extraction. +if ! printf '%s' "$input" | jq -e '(.tool_input.file_path == null) or (.tool_input.file_path | type == "string")' >/dev/null 2>&1; then + deny "Blocked by hooks/check-template-overwrite.sh: tool_input.file_path in the payload is not a string. Failing closed." +fi + file_path=$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty') if [ -z "$file_path" ]; then diff --git a/hooks/test_gitapex_check_bash_safety.py b/hooks/test_gitapex_check_bash_safety.py index 429dadd3..dd331f32 100644 --- a/hooks/test_gitapex_check_bash_safety.py +++ b/hooks/test_gitapex_check_bash_safety.py @@ -330,6 +330,38 @@ def test_denied_when_tool_name_is_not_a_string(tool_name: object) -> None: assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" +@pytest.mark.parametrize( + "command", + [["gh", "pr", "merge", "1"], {"argv": ["gh", "pr", "merge", "1"]}, 5, True], + ids=["array", "object", "number", "bool"], +) +def test_denied_when_tool_input_command_is_not_a_string(command: object) -> None: + """Found by code review (PR #1213, round 4): jq -r never errors on a + non-string `.tool_input.command` -- for an array/object it pretty- + prints the JSON form across multiple lines, which splits a dangerous + substring across JSON punctuation (quotes, commas, brackets) and + breaks every `[[:space:]]`-anchored danger-pattern regex below, + silently letting a genuinely dangerous command through (exit 0) + instead of failing closed. Live-confirmed before this guard existed: + an array-wrapped `["gh","pr","merge","1"]` command let a real merge + call straight through. Must now deny.""" + payload = json.dumps({"tool_name": "Bash", "tool_input": {"command": command}}) + env = dict(os.environ) + env.pop("CLAUDE_PROJECT_DIR", None) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + env=env, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2) for command={command!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + def test_denied_on_valid_json_non_object_stdin() -> None: """Valid JSON that isn't an object at the top level (e.g. a bare array) would otherwise crash the first field-extraction jq call the same way. diff --git a/hooks/test_gitapex_check_template_overwrite.py b/hooks/test_gitapex_check_template_overwrite.py index a5c4cf6f..9fdad6cb 100644 --- a/hooks/test_gitapex_check_template_overwrite.py +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -257,3 +257,33 @@ def test_denied_when_tool_name_is_not_a_string(tool_name: object, existing_templ assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" + + +@pytest.mark.parametrize( + "file_path_value", + [[".github/PULL_REQUEST_TEMPLATE.md"], {"path": ".github/PULL_REQUEST_TEMPLATE.md"}, 5, True], + ids=["array", "object", "number", "bool"], +) +def test_denied_when_tool_input_file_path_is_not_a_string(file_path_value: object) -> None: + """Found by code review (PR #1213, round 4): jq -r never errors on a + non-string `.tool_input.file_path` -- for an array/object it pretty- + prints the JSON form across multiple lines, which breaks both + `is_template_path()`'s basename matching and `[ -f "$file_path" ]`, + silently letting an overwrite of a real, existing template file + through (exit 0) instead of failing closed. Live-confirmed before + this guard existed: an array-wrapped file_path (wrapping the real + `.github/PULL_REQUEST_TEMPLATE.md`) let that overwrite straight + through. The guard denies on type alone, before any path-matching + logic runs, so no on-disk fixture is needed to exercise it here.""" + payload = json.dumps({"tool_name": "Write", "tool_input": {"file_path": file_path_value}}) + result = subprocess.run( + ["bash", str(SCRIPT)], + input=payload, + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + assert result.returncode == 2, f"expected deny (exit 2) for file_path={file_path_value!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" From 2a83666797161051388fd5eb7b962375df178723 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 17:14:50 +0000 Subject: [PATCH 6/8] refactor(hooks): reuse existing run() helper for tool_name-type tests Each of the three test_denied_when_tool_name_is_not_a_string tests hand-built its own ~15-line subprocess.run block instead of reusing the file's own run() helper, which already parameterizes tool_name -- the same helper the sibling hooks this PR's pattern was ported from use for this exact class of case. Widened each run()'s tool_name parameter from str to object so a non-string test value type-checks, then dropped the duplicated block in favor of a single run() call. Refs #1208 --- hooks/test_gitapex_check_bash_safety.py | 15 ++------------- ...est_gitapex_check_merge_pull_request_block.py | 16 ++-------------- hooks/test_gitapex_check_template_overwrite.py | 12 ++---------- 3 files changed, 6 insertions(+), 37 deletions(-) diff --git a/hooks/test_gitapex_check_bash_safety.py b/hooks/test_gitapex_check_bash_safety.py index dd331f32..0fa8c4ad 100644 --- a/hooks/test_gitapex_check_bash_safety.py +++ b/hooks/test_gitapex_check_bash_safety.py @@ -31,7 +31,7 @@ def run( - command: str, tool_name: str = "Bash", extra_env: dict[str, str] | None = None + command: str, tool_name: object = "Bash", extra_env: dict[str, str] | None = None ) -> subprocess.CompletedProcess[str]: payload = json.dumps({"tool_name": tool_name, "tool_input": {"command": command}}) env = dict(os.environ) @@ -313,18 +313,7 @@ def test_denied_when_tool_name_is_not_a_string(tool_name: object) -> None: (exit 0) instead of failing closed. Live-confirmed before this guard existed: an array-wrapped tool_name let a `gh pr merge` command straight through. Must now deny.""" - payload = json.dumps({"tool_name": tool_name, "tool_input": {"command": "gh pr merge 1"}}) - env = dict(os.environ) - env.pop("CLAUDE_PROJECT_DIR", None) - result = subprocess.run( - ["bash", str(SCRIPT)], - input=payload, - capture_output=True, - text=True, - timeout=10, - env=env, - cwd=str(REPO_ROOT), - ) + result = run("gh pr merge 1", tool_name=tool_name) assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" diff --git a/hooks/test_gitapex_check_merge_pull_request_block.py b/hooks/test_gitapex_check_merge_pull_request_block.py index 7f0de807..3405f127 100644 --- a/hooks/test_gitapex_check_merge_pull_request_block.py +++ b/hooks/test_gitapex_check_merge_pull_request_block.py @@ -31,7 +31,7 @@ def run( *, - tool_name: str = "mcp__github__merge_pull_request", + tool_name: object = "mcp__github__merge_pull_request", tool_input: dict[str, object] | None = None, ) -> subprocess.CompletedProcess[str]: payload = json.dumps( @@ -206,19 +206,7 @@ def test_denied_when_tool_name_is_not_a_string(tool_name: object) -> None: Live-confirmed before this guard existed: an array-wrapped tool_name let a merge_pull_request call straight through this hook -- the exact bypass class this file exists to close. Must now deny.""" - env = dict(os.environ) - env.pop("CLAUDE_PROJECT_DIR", None) - env.pop("CLAUDE_PLUGIN_ROOT", None) - payload = json.dumps({"tool_name": tool_name, "tool_input": {"owner": "tvna", "repo": "gitapex", "pullNumber": 1}}) - result = subprocess.run( - ["bash", str(SCRIPT)], - input=payload, - capture_output=True, - text=True, - timeout=10, - env=env, - cwd=str(REPO_ROOT), - ) + result = run(tool_name=tool_name) assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" diff --git a/hooks/test_gitapex_check_template_overwrite.py b/hooks/test_gitapex_check_template_overwrite.py index 9fdad6cb..2a460ccc 100644 --- a/hooks/test_gitapex_check_template_overwrite.py +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -26,7 +26,7 @@ def run( - file_path: str, tool_name: str = "Write", extra_env: dict[str, str] | None = None + file_path: str, tool_name: object = "Write", extra_env: dict[str, str] | None = None ) -> subprocess.CompletedProcess[str]: payload = json.dumps({"tool_name": tool_name, "tool_input": {"file_path": file_path}}) env = dict(os.environ) @@ -245,15 +245,7 @@ def test_denied_when_tool_name_is_not_a_string(tool_name: object, existing_templ (exit 0) instead of failing closed. Live-confirmed before this guard existed: an array-wrapped tool_name let an overwrite of the real PR template straight through. Must now deny.""" - payload = json.dumps({"tool_name": tool_name, "tool_input": {"file_path": existing_template_file}}) - result = subprocess.run( - ["bash", str(SCRIPT)], - input=payload, - capture_output=True, - text=True, - timeout=10, - cwd=str(REPO_ROOT), - ) + result = run(existing_template_file, tool_name=tool_name) assert result.returncode == 2, f"expected deny (exit 2) for tool_name={tool_name!r}, got {result.returncode}" parsed = json.loads(result.stderr) assert parsed["hookSpecificOutput"]["permissionDecision"] == "deny" From a3cd021be3192630f9e4330408c20acaaa98f058 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 10:37:45 +0000 Subject: [PATCH 7/8] chore(ssot): register merge-pull-request-block, refresh fail-closed rule text hooks/check-merge-pull-request-block.sh -- the hook carrying this PR's most severe fix -- had no gates[] entry in .gitapex/ssot.json at all. It was still covered by skill-audit-disclosure's own naming-convention backstop (hooks/(?:check[-_]|...)...), which is why the disclosure requirement fired correctly on this PR regardless, but the registry itself has no reverse-direction check (a gate-shaped file on disk with no registered entry) -- only find_script_drift, which validates that registered entries point to real files, not the other way around. Adds the missing entry, id merge-pull-request-block, tracking_issue 637 per the hook's own header citation. Also refreshes the rule text of the three already-registered gates this PR touches (bash-cli-write-and-install-guard, template-overwrite-guard, skill-audit-disclosure) to mention the fail-closed guards added across this PR's four rounds -- the schema's own rule field description says "grounded in the real script's logic", and the prior text predates all of it. Verified: uv run --frozen python .github/scripts/gitapex_scan_ssot_schema.py reports no drift; tests/test_gitapex_scan_ssot_schema.py (83 passed) and the related wiring/registry suite (191 passed) both clean. Refs #1208 --- .gitapex/ssot.json | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/.gitapex/ssot.json b/.gitapex/ssot.json index cb77c5cf..ba6935cc 100644 --- a/.gitapex/ssot.json +++ b/.gitapex/ssot.json @@ -84,7 +84,7 @@ "id": "bash-cli-write-and-install-guard", "kind": "script", "script": "hooks/check-bash-safety.sh", - "rule": "Denies a Bash call matching a package/plugin-install verb; denies gh issue/gh pr write subcommands and gh api writes (POST/PUT/PATCH/DELETE, a field flag, or 'mutation' in gh api graphql); on git push, runs skills/outward-artifact-preflight/scripts/gitapex_scan_provenance.py against the outgoing commit range and warns (never blocks) if it flags something.", + "rule": "Denies a Bash call matching a package/plugin-install verb; denies gh issue/gh pr write subcommands and gh api writes (POST/PUT/PATCH/DELETE, a field flag, or 'mutation' in gh api graphql); on git push, runs skills/outward-artifact-preflight/scripts/gitapex_scan_provenance.py against the outgoing commit range and warns (never blocks) if it flags something. Fails closed (denies) rather than allowing the call through when jq is missing from PATH, the payload or tool_input is not a JSON object, tool_name is present but not a string, or tool_input.command is present but not a string.", "planes": ["pretooluse"], "local_exclusion": "PreToolUse-only: grades a Claude Code tool-call JSON payload arriving on stdin, which has no working-tree equivalent to reconstruct ahead of a push.", "trigger": "PreToolUse matcher Bash (hooks/hooks.json)", @@ -112,7 +112,7 @@ "id": "template-overwrite-guard", "kind": "script", "script": "hooks/check-template-overwrite.sh", - "rule": "Denies a Write call whose file_path case-insensitively matches an existing GitHub/GitLab issue/PR/MR template location.", + "rule": "Denies a Write call whose file_path case-insensitively matches an existing GitHub/GitLab issue/PR/MR template location. Fails closed (denies) rather than allowing the call through when jq is missing from PATH, the payload or tool_input is not a JSON object, tool_name is present but not a string, or tool_input.file_path is present but not a string.", "planes": ["pretooluse"], "local_exclusion": "PreToolUse-only: grades a Claude Code Write tool-call payload, not repository state.", "trigger": "PreToolUse matcher Write (hooks/hooks.json)", @@ -168,7 +168,7 @@ ".github/workflows/skill-audit-gate.yml", "hooks/hooks.json" ], - "rule": "Requires a PR touching skills/*/SKILL.md to disclose battle-testing-a-skill / evaluating-skill-quality verdicts or waivers under a '## Skill audit evidence' heading; the CI twin additionally requires disclosure for description changes, security-relevant skills, changed design docs, changed checker scripts, changed deterministic gates, and (issue #998) changed checker-or-gate scripts (the union of the checker-script and gate-script scopes). Gate membership is the union of the .github/scripts/gate_*.py / scan_*.py naming convention, every gates[].script path registered in this file, and this file itself; deletions and renames count, and the disclosure must be 'deterministic-gate-quality: RAN' or an explicit waiver with a reason, never 'NOT-RUN'. 'defeat-test-disclosure' shares checker-script-adversarial-review's RAN/NOT-RUN/WAIVED shape instead, disclosing that at least one test was constructed to defeat (not merely exercise the happy path of) the new or changed detection logic. The applicability computation lives in one place (gitapex_compute_skill_audit_flags.py), which the CI diff step and the local pre-push wrapper (gitapex_gate_skill_audit_disclosure.py --check-diff BASE HEAD --body-file PATH, invoked by the PreToolUse hook when .github/ is present) both call, so the full verdict including every conditional extension is reachable before a push.", + "rule": "Requires a PR touching skills/*/SKILL.md to disclose battle-testing-a-skill / evaluating-skill-quality verdicts or waivers under a '## Skill audit evidence' heading; the CI twin additionally requires disclosure for description changes, security-relevant skills, changed design docs, changed checker scripts, changed deterministic gates, and (issue #998) changed checker-or-gate scripts (the union of the checker-script and gate-script scopes). Gate membership is the union of the .github/scripts/gate_*.py / scan_*.py naming convention, every gates[].script path registered in this file, and this file itself; deletions and renames count, and the disclosure must be 'deterministic-gate-quality: RAN' or an explicit waiver with a reason, never 'NOT-RUN'. 'defeat-test-disclosure' shares checker-script-adversarial-review's RAN/NOT-RUN/WAIVED shape instead, disclosing that at least one test was constructed to defeat (not merely exercise the happy path of) the new or changed detection logic. The applicability computation lives in one place (gitapex_compute_skill_audit_flags.py), which the CI diff step and the local pre-push wrapper (gitapex_gate_skill_audit_disclosure.py --check-diff BASE HEAD --body-file PATH, invoked by the PreToolUse hook when .github/ is present) both call, so the full verdict including every conditional extension is reachable before a push. The PreToolUse hook itself fails closed (denies) when jq is missing from PATH, the payload or tool_input is not a JSON object, or tool_name is present but not a string -- an unparseable payload is treated as indeterminate, not as evidence the disclosure requirement does not apply.", "planes": ["pretooluse", "ci"], "local_exclusion": "Grades a PR body against the changed-skill list; the body does not exist until the PR is opened.", "trigger": "PreToolUse matcher mcp__github__create_pull_request and mcp__github__update_pull_request (hooks/hooks.json); .github/workflows/skill-audit-gate.yml on pull_request", @@ -196,6 +196,20 @@ "status": "active", "supersedes": null }, + { + "id": "merge-pull-request-block", + "kind": "script", + "script": "hooks/check-merge-pull-request-block.sh", + "rule": "Denies every mcp__github__merge_pull_request call unconditionally, with no override -- merging a PR is always a separate, explicit human or CI decision. Fails closed (denies) rather than allowing the call through when jq is missing from PATH, the payload is not a JSON object, or tool_name is present but not a string, since none of those states let the hook confirm the call is not a disguised merge_pull_request.", + "planes": ["pretooluse"], + "local_exclusion": "PreToolUse-only: grades a Claude Code tool-call JSON payload arriving on stdin, which has no working-tree equivalent to reconstruct ahead of a push.", + "trigger": "PreToolUse matcher mcp__github__merge_pull_request (hooks/hooks.json)", + "policy_refs": [], + "cluster": "github-operations", + "tracking_issue": 637, + "status": "active", + "supersedes": null + }, { "id": "gitignore-pattern-test-coverage", "kind": "script", From 20c5bfa777a4c73bc2fb81f607d17460a64fa904 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 12:25:35 +0000 Subject: [PATCH 8/8] docs(ssot): disclose ssot-schema-drift's known reverse-direction gap Found while auditing whether PR #1213's own diff was fully reflected in .gitapex/ssot.json: the ssot-schema-drift gate's own rule text accurately describes what it currently checks (registered entries point to real files) but said nothing about the gap that let check-merge-pull-request-block.sh go unregistered until this PR -- the same class of gap issue #1227 now tracks. Discloses it inline, citing #1227, rather than leaving the limitation implicit. Refs #1208 --- .gitapex/ssot.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitapex/ssot.json b/.gitapex/ssot.json index a4b058a7..de3feb70 100644 --- a/.gitapex/ssot.json +++ b/.gitapex/ssot.json @@ -600,7 +600,7 @@ ".github/scripts/gitapex_scan_ssot_schema.py", ".github/scripts/_gitapex_argv_safety.py" ], - "rule": "Validates .gitapex/ssot.json against .gitapex/ssot.schema.json (JSON Schema draft 2020-12); fails if any gates[].script path doesn't exist as a real file, or any policy_refs[] value doesn't resolve to a real policy_sources[].id. The argv-safety predicates this gate applies to local_invocation/local_stdin live in _gitapex_argv_safety.py, registered here (issue #904) so gitapex_detect_changed_gate_scripts.py selects an edit to them as a gate change requiring disclosure.", + "rule": "Validates .gitapex/ssot.json against .gitapex/ssot.schema.json (JSON Schema draft 2020-12); fails if any gates[].script path doesn't exist as a real file, or any policy_refs[] value doesn't resolve to a real policy_sources[].id. The argv-safety predicates this gate applies to local_invocation/local_stdin live in _gitapex_argv_safety.py, registered here (issue #904) so gitapex_detect_changed_gate_scripts.py selects an edit to them as a gate change requiring disclosure. Known gap (issue #1227): checks only that a registered entry's script path is real, not the reverse -- a real gate-shaped file (matching the naming convention gitapex_detect_changed_gate_scripts.py already applies for a different purpose) with no gates[] entry passes this check undetected.", "planes": ["ci", "local"], "local_invocation": ["uv", "run", "--frozen", "python3", ".github/scripts/gitapex_scan_ssot_schema.py"], "trigger": "tests/test_gitapex_scan_ssot_schema.py inside the pytest step of .github/workflows/test.yml",