diff --git a/.gitapex/ssot.json b/.gitapex/ssot.json index 897f78e2..4d5b84ca 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)", @@ -114,7 +114,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)", @@ -163,7 +163,7 @@ "id": "merge-pull-request-block", "kind": "script", "script": "hooks/check-merge-pull-request-block.sh", - "rule": "Denies mcp__github__merge_pull_request unconditionally, no override -- per planning-a-branch-from-an-issue/SKILL.md (\"Do not merge or enable auto-merge; that is a separate, explicit human or CI decision\"), drafting-a-pr-to-merge/SKILL.md's own step 9 (a PR's terminal action is converting to DRAFT, never merging it), and the ranking-the-open-queue Routine specs' \"100% human review of any pull request merge\" policy; closes the gap where check-bash-safety.sh's shell-only 'gh pr merge' block had no equivalent for the platform-integrated MCP tool call.", + "rule": "Denies mcp__github__merge_pull_request unconditionally, no override -- per planning-a-branch-from-an-issue/SKILL.md (\"Do not merge or enable auto-merge; that is a separate, explicit human or CI decision\"), drafting-a-pr-to-merge/SKILL.md's own step 9 (a PR's terminal action is converting to DRAFT, never merging it), and the ranking-the-open-queue Routine specs' \"100% human review of any pull request merge\" policy; closes the gap where check-bash-safety.sh's shell-only 'gh pr merge' block had no equivalent for the platform-integrated MCP tool call. 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": "Grades a Claude Code tool-call matcher only (mcp__github__merge_pull_request), an unconditional deny with no payload inspection; nothing in the working tree to reconstruct this decision against.", "trigger": "PreToolUse matcher mcp__github__merge_pull_request (hooks/hooks.json)", @@ -188,7 +188,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", @@ -634,7 +634,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", diff --git a/hooks/check-bash-safety.sh b/hooks/check-bash-safety.sh index 2672a6f6..a7511a56 100755 --- a/hooks/check-bash-safety.sh +++ b/hooks/check-bash-safety.sh @@ -19,8 +19,75 @@ set -euo pipefail +# 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 + +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 +} + +# Non-blocking counterpart to deny(): surfaces a systemMessage but allows the +# 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" + printf '%s' "$reason" | jq -Rs '{"systemMessage": .}' + 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 + +# 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 @@ -29,6 +96,36 @@ 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. `(.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 + +# 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 @@ -37,24 +134,6 @@ fi lc_command=$(printf '%s' "$command" | tr '[:upper:]' '[:lower:]') -deny() { - local reason="$1" - jq -n --arg msg "$reason" \ - '{"hookSpecificOutput": {"permissionDecision": "deny"}, "systemMessage": $msg}' >&2 - exit 2 -} - -# Non-blocking counterpart to deny(): surfaces a systemMessage but allows the -# 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. -warn() { - local reason="$1" - jq -n --arg msg "$reason" \ - '{"systemMessage": $msg}' - exit 0 -} - # --- 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..98f08c2c 100755 --- a/hooks/check-merge-pull-request-block.sh +++ b/hooks/check-merge-pull-request-block.sh @@ -25,8 +25,67 @@ 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 + +# 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 @@ -35,8 +94,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..ecbfb939 100755 --- a/hooks/check-pr-skill-audit-disclosure.sh +++ b/hooks/check-pr-skill-audit-disclosure.sh @@ -59,8 +59,54 @@ 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 + +# 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 @@ -70,6 +116,22 @@ 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. `(.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 + 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 +144,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" @@ -151,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 7dddd6e4..1839448e 100755 --- a/hooks/check-template-overwrite.sh +++ b/hooks/check-template-overwrite.sh @@ -11,8 +11,54 @@ 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 + +# 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 @@ -21,6 +67,36 @@ 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. `(.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 + +# 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 @@ -57,9 +133,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..0fa8c4ad 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 @@ -30,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) @@ -192,6 +193,184 @@ 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" + + +@pytest.mark.parametrize( + "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 + 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( + ["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_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 `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.""" + 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" + + +@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. + 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 # --------------------------------------------------------------------------- @@ -295,6 +474,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 18b610ba..3405f127 100644 --- a/hooks/test_gitapex_check_merge_pull_request_block.py +++ b/hooks/test_gitapex_check_merge_pull_request_block.py @@ -19,16 +19,19 @@ import json import os +import shutil import subprocess from pathlib import Path +import pytest + SCRIPT = Path(__file__).parent / "check-merge-pull-request-block.sh" REPO_ROOT = Path(__file__).parent.parent 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( @@ -91,3 +94,119 @@ 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" + + +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" + + +@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.""" + 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_pr_skill_audit_disclosure_shell.py b/hooks/test_gitapex_check_pr_skill_audit_disclosure_shell.py index 6781a61a..94dbe8cc 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,202 @@ 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" + + +@pytest.mark.parametrize( + "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 + non-object would otherwise crash the `.tool_input.body`/`.tool_input.base` + 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, + 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_input={tool_input!r}, got {result.returncode}" + parsed = json.loads(result.stderr) + 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. + 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" + + +@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 new file mode 100644 index 00000000..2a460ccc --- /dev/null +++ b/hooks/test_gitapex_check_template_overwrite.py @@ -0,0 +1,281 @@ +"""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: 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) + 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 == "" + + +@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_an_existing_template_file(existing_template_file: str) -> None: + assert_denied(existing_template_file) + + +@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(existing_template_file: str) -> None: + result = run(existing_template_file, 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, 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(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" + 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" + + +@pytest.mark.parametrize( + "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 + 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, + capture_output=True, + text=True, + timeout=10, + cwd=str(REPO_ROOT), + ) + 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.""" + 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" + + +@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"