fix(source-control): close the merge wrapper's argparse prefix-abbreviation bypass#1382
fix(source-control): close the merge wrapper's argparse prefix-abbreviation bypass#1382kyle-sexton wants to merge 3 commits into
Conversation
…iation bypass `source-control-babysit-merge` exists to add exactly one refusal on top of `babysit_merge.py` -- it rejects `--allow-unpinned-head` so that no unattended, allow-rule-covered invocation can merge an unvetted head. It filtered by exact string equality while the CLI behind it built its parser with argparse's `allow_abbrev` left at the default `True`, so any unambiguous prefix (`--allow-unpinned-hea`, `--allow-unpinned`, `--allow-unpi`) was a valid spelling of the flag to the CLI and an unrecognized string to the wrapper. Dropping one character therefore reached `--merge` without a pinned head -- precisely the outcome the wrapper's existence is justified by. Fix both halves, because either alone leaves a gap: - The wrapper now refuses any `--`-prefixed argument that is a prefix of the flag, after stripping a `--flag=value` tail, so it holds independently of how the CLI's parser happens to be configured. Sibling flags are unaffected -- `--allowed-owners`, `--allow-dependency`, and `--allow-unprotected` are not prefixes of the flag. - All nine babysit-prs lane CLIs now build their parsers with `allow_abbrev=False`. This closes the class rather than the one flag: `--mer` was an accepted spelling of `--merge`, so a consumer permission rule granting the wrapper *but not with `--merge`* was bypassable by the same mechanism. Regression coverage asserts the wrapper's own refusal text rather than exit 2 (overloaded three ways on this path) across nine spellings, asserts argparse reports `unrecognized arguments` at the CLI, and holds the whole scripts/ directory to `allow_abbrev=False` so a new lane script cannot reopen the class. Closes #1371 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 58s —— View job Code Review: fix(source-control) — merge wrapper argparse prefix-abbreviation bypass
SummaryThis is a well-executed, defense-in-depth security fix. The root cause (argparse A few observations below, ranging from a genuine correctness note to style. Findings1. Redirect order in
|
|
Claude finished @kyle-sexton's task in 4m 27s —— View job Security Review — PR #1382
FindingsNo CRITICAL or IMPORTANT vulnerabilities found. One SUGGESTION-level gap is noted below. Bash wrapper — prefix comparison logic
The core comparison: if [[ "${unpinned_flag:0:${#candidate}}" == "$candidate" ]]; thenThe RHS is Printf — no format-string injection
|
…rden the new rows Addresses the two automated review passes on the PR: - The prefix test now runs in both directions, so a future flag that STARTS with `--allow-unpinned-head` is refused too. `allow_abbrev=False` cannot catch that case -- such a flag would be a recognized, distinct option -- so only the wrapper half can, and the PR's claim that this half holds on its own is now true for superstrings as well as abbreviations. New row covers it. - Documented the load-bearing `2>&1 >/dev/null` redirect order in `check_wrapper_refusal`: the familiar `>/dev/null 2>&1` spelling would discard stderr and make every row vacuously pass. - The AST scan now uses `rglob` (excluding `tests/`), so a parser added in a subdirectory of `scripts/` cannot escape the `allow_abbrev=False` check. - Rewrapped the overlong SKILL.md line the doc edit introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both automated review passes triaged. Summary of dispositions: Applied (commit
Noted, no change (code review, finding 3): Re-verified after the changes: This was generated by AI during work-loop execution. |
|
Claude finished @kyle-sexton's task in 2m 39s —— View job PR Review — follow-up pass after
|
|
Claude finished @kyle-sexton's task in 2m 32s —— View job Security Review — PR #1382 (after
|
…e-wrapper-abbrev-bypass # Conflicts: # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md # plugins/source-control/bin/source-control-babysit-merge # plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py # plugins/source-control/skills/babysit-prs/scripts/babysit_resolve_thread.py
|
Closed in favour of #1428, which carries This PR was opened before #1354 landed and is now a strict superset of it: main already has #1428 is the residual on a fresh base — the seven parsers #1354 did not reach, plus the directory-wide gate this PR proposed, restated as a guard-contract check now that #1285 gave it a home. Nothing from here is dropped: #1371 stays open until #1428 merges, and the branch remains on the remote if any of this diff is wanted back. |
…point (#1428) Closes #1371. Replaces #1382, which is closed in favour of this. #1382 was opened before #1354 landed and is now a strict superset of it: re-scoped here to the residual only, on a fresh base, with the property bound to the directory rather than to a list of files. ## The gap A permission grant states its condition as the literal presence or absence of a flag in the command text — above all *"no `--merge` means check-only"*. Argparse's default prefix abbreviation lets `--mer` resolve to `--merge` while the command text contains no such flag, so the written command and the resolved behavior diverge. That is exactly what such a condition has to be able to rule out. #1354 closed this on `babysit_merge.py` and `babysit_resolve_thread.py`. Seven entry points still inherited the default: `babysit_findings.py` · `manage_babysit_lease.py` · `manage_feedback_ledger.py` · `pr_queue_snapshot.py` · `prune_babysit_worktrees.py` · `refresh_pr_branch.py` · `request_review.py` All nine now set `allow_abbrev=False`. ## Why a gate, not seven more edits Hardening entry points one at a time is what let the gap survive #1354 for seven files. The guard contract gains a check over the whole catalogue: every catalogued Python entry point is invoked with `--hel` and must not exit 0. `--help` is registered on every parser and short-circuits parsing, so an abbreviation that *resolves* exits 0 before required-argument validation ever runs, while one that does not is a usage error. That makes the exit code a sufficient discriminator without a per-CLI argument shape — which is what makes this a gate over the catalogue rather than a hand-maintained list of cases. The message is deliberately not asserted: several of these parsers have a required mutually exclusive group that errors before any unrecognized argument is reported. A companion test asserts that discrimination against argparse itself rather than assuming it. ## Verification - `python -m unittest discover -s tests` — 387 tests, OK. - Detector verified rather than assumed: reverting `request_review.py`'s `allow_abbrev=False` fails the gate naming that file (`resolved the abbreviation --hel to --help and exited 0`), and it passes again on restore. ## Compatibility Abbreviated invocations that previously worked are now usage errors. That is the intent, and the version takes a minor bump for it — `source-control` 0.29.0 → 0.31.0 (0.30.0 is claimed by #1264, open). ## Related - #1354 — closed the same defect on the first two entry points - #1382 — the superset PR this replaces - #1285 — the guard contract this gate is added to --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
plugins/source-control/bin/source-control-babysit-mergeexists to add exactly one refusal ontop of
babysit_merge.py: it rejects--allow-unpinned-headso that no unattended,allow-rule-covered invocation can merge an unvetted head. That refusal was bypassable by dropping
one character.
The two halves disagreed. The wrapper filtered by exact string equality; the CLI it wraps built
its parser with argparse's
allow_abbrevleft at the defaultTrue(CPython docs —
allow_abbrev:"Normally, when you pass an argument list to the
parse_args()method of anArgumentParser, itrecognizes abbreviations of long
options"). So any unambiguous prefix was a valid spelling of the flag to the CLI and an unrecognized
string to the wrapper.
Reproduced on
origin/mainbefore touching anything (RC=3is the owner-allowlist refusaldownstream of parsing — proof the argument was consumed as a recognized option;
RC=2from thewrapper would be the guard firing):
Both halves are fixed, because either alone leaves a gap
bin/source-control-babysit-mergenow refuses any---prefixed argument that is a prefixof the flag, after stripping a
--flag=valuetail. It therefore holds independently of how theCLI's parser happens to be configured — a guard that is correct only because the thing it guards
is configured a particular way is not a guard. The comparison is a length-sliced literal compare
with the RHS quoted (never a glob match). It deliberately over-refuses a prefix appearing
after a
--end-of-options separator or as another option's value: no legitimate invocationcarries one, and a merge guard fails closed. Sibling flags are untouched —
--allowed-owners,--allow-dependency, and--allow-unprotectedare not prefixes of--allow-unpinned-head, andthere are explicit no-over-refusal test rows for all three.
All nine
babysit-prslane parsers now passallow_abbrev=False(babysit_merge.py,babysit_resolve_thread.py,babysit_findings.py,manage_babysit_lease.py,manage_feedback_ledger.py,prune_babysit_worktrees.py,request_review.py,pr_queue_snapshot.py,refresh_pr_branch.py). This is the root cause, and it closes theclass rather than the one flag.
The systemic finding worth stating outright
The same root cause defeats any control anchored on a flag's exact text, not just this wrapper's
refusal. Verified empirically on
origin/main:A consumer permission rule that grants the wrapper but not with
--merge— which is how theissue describes
melodic-software/dotfiles'autoMode.allow, i.e. the configuration that boundedthe reported severity — was bypassable by the same mechanism. That exposure is closed by
allow_abbrev=False, not by the wrapper change; it is why this PR fixes the parser class ratherthan the one flag.
Blast radius
No exact spelling changed behavior. A repo-wide scan of every
.md/.sh/.py/.json/.ymllinementioning these nine CLIs or the two wrappers found no invocation using an abbreviated flag
(two hits were prose lines naming multiple scripts, not invocations).
SKILL.mdandreference/safety.mdhad published the refusal as unconditional; both now state that it coversevery spelling including every prefix, and that the CLI's parser is
allow_abbrev=False.Test plan
origin/mainand confirmed they fail:8 Python failures and 9 of the new bash rows failing (
FAIL: merge wrapper rejects --allow-unpinned-hea (want exit 2 + wrapper refusal text, got exit 3: )).ways on this path — the wrapper's refusal, argparse's usage/ambiguity errors, and
babysit_merge.py's own usage errors — so a code-only assertion cannot tell a held guard froman accidental parse failure.
--allow-unpis exactly that trap: it returns 2 on unfixedmainvia argparse ambiguity, so a code-only row would have been green before and after. New
check_wrapper_refusalhelper inengine.test.shgreps stderr for the refusal text.--allow-unpinned-hea,--allow-unpinned-h,--allow-unpinned,--allow-unpi,--allow-unp,--allow-u,--allow, plus--allow-unpinned-head=1and--allow-unpi=1);Python rows in
test_guards.pyinvokebabysit_merge.pydirectly and assert argparse'sunrecognized argumentson stderr (the existingrun()helper discarded stderr and parsedstdout as JSON — an argparse rejection writes nothing to stdout, so any assertion on the JSON
payload would have tested nothing; added a
run_raw()that returns stderr).test_every_lane_parser_disables_abbreviationAST-scans every
ArgumentParser(...)underbabysit-prs/scripts/and fails if any lacksallow_abbrev=False, so a new lane script cannot silently reopen this.--allow-dependency,--allow-unprotected, and--allowed-ownersstill reach the CLI (exit 3), and that the exact--merge --allow-unpinned-head --allow-dependencycombination still parses to the owner-scope refusalrather than argparse's exit 2.
engine.test.sh→ 356 unittests OK, ruff clean, all 16 wrapper rowsPASS, exit 0. Every
plugins/source-control/**/*.test.sh(9 suites) passes.shellcheck -xon the wrapper andengine.test.shclean;scripts/validate-plugins.sh✔;scripts/check-changelog-parity.sh --checkand--check-bump origin/main✔;scripts/check-changed-skills.sh→CHECK-SKILL babysit-prs: PASS; markdownlint clean on the three touched docs.0.26.4→0.26.5with the matching CHANGELOG entry.Related
Closes #1371.
--allow-unpinned-head's bash-vs-Python splitis one of the four fact families it names.
merge.unpinned-head-refused-by-wrapperas a proven contract row. Itsguard_contract.pyexists only onfeat/1265-guard-contract-suite, not onmain, so theabbreviated-spelling contract row belongs to that branch, not this PR. The regression rows here
cover the behavior in the shipped suite meanwhile; feat(source-control): bind babysit guard semantics to an executable contract #1285 should add a row asserting every prefix of
the flag is refused so the published claim is bound by the contract suite too. Found by the Codex
reviewer on feat(source-control): bind babysit guard semantics to an executable contract #1285.
This was generated by AI during work-loop execution.
🤖 Generated with Claude Code