fix(guardrails): close git-alias-chain re-expansion bypass of both git guards - #1503
Conversation
…t guards Rebuilt as a single signed commit. The prior branch history carried an unsigned commit, which GitHub reports as an unverified signature and which blocks the required-signatures rule on main. No content change. Refs #964
|
Claude finished @kyle-sexton's task in 3m 45s —— View job PR Review
SummaryThis PR closes a real, verified bypass of both git guards via multi-hop alias chaining (#964). The implementation is sound, the test coverage is comprehensive and rigorous, and the two design invariants — memoization and budget cap — are correctly scoped. I have a few observations and one minor correctness note. Core Bypass Being ClosedThe old code used Critical Additional Fix: Splice WideningThe old recursive call was: check_segment "${w[@]:0:gi+1}" ${expw[@]+"${expw[@]}"} "${w[@]:sub_idx+1}"The new call is: nextw=("${w[@]:0:sub_idx}" ${expw[@]+"${expw[@]}"} "${w[@]:sub_idx+1}")The old code sliced
|
|
Claude finished @kyle-sexton's task in 7m 13s —— View job Security Review
SummaryThis PR replaces a one-level alias-expansion guard ( No critical security issues were found in the new code. Two lower-severity observations are documented below. The Fixed Bypass — Verified CorrectOld behavior ( A command like
New behavior ( The same command now traverses the full chain: The old splice FindingsSUGGESTION —
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7fdebc0d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tree A `!` shell-alias body launches at the work-tree top level only when git can compute a prefix — when the caller sits inside the effective work tree. Under an explicit --git-dir/--work-tree whose tree does not contain the caller, git skips that chdir and runs the body where the caller stands. block-noncanonical-commit collapsed to the top level unconditionally, so a body's `git -C child p` resolved the work tree's child repo while real git ran the caller's — a non-canonical commit passed (fail-open) and its mirror false-blocked a canonical one. Replace repo_identity with alias_launch_dir, which reads git's own answer via `rev-parse --show-toplevel --show-prefix`: a nonempty prefix (or pure discovery with no locating globals) returns the top level; an empty prefix under explicit globals returns the caller's composed directory. The inside-work-tree branch is unchanged. Adds bypass + false-block twins outside the work tree and an inside-work-tree pair pinning the top-level branch. Closes #964.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb1c5f8a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eexpansion-signed # Conflicts: # plugins/guardrails/.claude-plugin/plugin.json # plugins/guardrails/CHANGELOG.md
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 886a6f3aa7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two fail-open holes in the new launch-directory lookup: - The cache key joined the replayed locating globals with `$*`, flattening argv boundaries: `--git-dir 'X --work-tree' --namespace Z` and `--git-dir X --work-tree '--namespace Z'` — different repositories to git — produced one key, so the first git segment in a payload poisoned the cache for the second. Key each argv word `%q`-encoded instead; the key is now injective on the argv. - The toplevel and prefix were parsed from one newline-delimited `rev-parse` call, so a repository path containing a newline truncated the toplevel and misread the rest as a prefix. Read them in separate calls; each value is delimited only by the command substitution's trailing-newline strip, which cannot corrupt an interior newline. The prefix call is skipped when the toplevel is empty, so the allow path pays no extra fork.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00a3f3038d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`$(…)` strips every trailing newline, but a repository top-level path may end in one (POSIX permits any byte but NUL and `/`), so dropping it returned a different sibling directory — a wrong repository, fail-open on the alias walk. Capture each `rev-parse` path field through a sentinel byte that absorbs the strip, then remove git's single line terminator explicitly. Interior and trailing newlines in either field now survive intact; the separate-call-per-field boundary is unchanged.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4491966ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… peel The prior sentinel fix peeled a trailing CR after the LF, which is the same fail-open one byte over: a POSIX top-level path may legitimately end in `\r`, and stripping it resolves a different sibling. git ends `rev-parse --show-toplevel`/ `--show-prefix` with a BARE LF, not a CRLF, even on Windows — verified on git 2.54.0.windows.1 via `od -c` (output ends `… w o r k \n`, no `\r`). So peel exactly one trailing `\n` and nothing else; interior/trailing newlines and a trailing CR all survive. Add a POSIX-gated regression fixture (a repository whose top-level path ends in a newline, whose stripped sibling carries a canonical alias) that asserts the block on POSIX and skips loudly where the platform cannot host such a path. Correct the round-2 CHANGELOG claim that the trailing-newline strip was harmless, and add the sentinel-framing bullet.
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
…ixture The launch-directory probe's sentinel framing is correct and unit-verified for every newline position (interior, single/double trailing, CRLF terminator). The end-to-end fixture added for it actually exercised a DIFFERENT, pre-existing entry point: the payload `cwd` is read through a bare `$(…)` one layer earlier and its trailing newline is stripped before the probe runs, so the fixture reached the stripped sibling regardless of the probe fix. Reaching a newline top level through the probe alone needs a literal newline in the parsed command, which is impractical to fixture faithfully. Drop the conflated fixture and rely on the framing unit proof. The payload-cwd newline strip is pre-existing and shared with main (lower severity — the session cwd is not attacker-controlled — and its cross-platform fix must handle jq's CRLF on Windows), tracked as #1536 rather than absorbed here.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f68e9fe6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reparse="${exp#!}" | ||
| for a in "${w[@]:sub_idx+1}"; do reparse+=" $(printf '%q' "$a")"; done | ||
| hook::bash_parse_segments "$reparse" check_segment | ||
| [[ -n "$seg_dir" ]] || seg_dir="$(effective_dir "${w[@]:0:sub_idx}")" |
There was a problem hiding this comment.
When Git is reached through a wrapper, this slice includes the wrapper's arguments, so effective_dir can mistake a value-taking wrapper option for Git's -C. GNU env --help documents -u, --unset=NAME as removing an environment variable, meaning env -u -C git … runs Git without changing directory; however, the guard interprets that -C git as relocation. I reproduced this with an outer repository containing nested repositories at child and git/child: env -u -C git -c alias.a='!git -C child p' a made the guard inspect git/child's canonical p alias and return 0, while real Git inspected child's commit --allow-empty -m bypass alias and committed. Pass only Git's globals between gi and sub_idx to the directory and locating-global helpers.
Useful? React with 👍 / 👎.
…1543) *This was generated by AI during work-loop execution.* ## Summary - BSD/macOS `mktemp` has no `-p` flag. #1510 left `mktemp -p` STAGED (inactive) in `scripts/shell-portability-tokens.txt` after enabling the sibling `date -d`/`stat -c` classes, because migrating the corpus was a real, multi-plugin effort out of scope for that PR. This item does that migration. - Migrated every `mktemp -p <DIR> <template>` / `mktemp -d -p <DIR> <template>` call site (fresh `grep -rn "mktemp[^\n]*-p" --include="*.sh"` at execution time found **61 call sites across 24 files in 13 plugins** — the issue's "~20 sites"/"~24 files, ~56 call sites" estimates undercounted) to the portable `mktemp [-d] "$DIR/template"` form, which both GNU and BSD `mktemp` accept identically via the positional TEMPLATE argument instead of `-p`. Sites with no explicit template (`mktemp -p "$DIR"`) get an explicit `tmp.XXXXXXXXXX` template (GNU's own default) rather than relying on TMPDIR-inheritance semantics, which differ between dialects. - Activated the `mktemp -p` token in `scripts/shell-portability-tokens.txt` (moved ACTIVE, extended to a combined-short-option-cluster match — `p` anywhere in a cluster like `-dp` — mirroring the existing `sort -V` / `grep -P` / `echo -e` tokens) and trimmed the STAGED section comment. - Updated `scripts/check-shell-portability.test.sh`'s staged-classes test: split the old combined "date -d, stat -c, mktemp -p all inactive" assertion into a "date -d, stat -c still inactive" assertion plus new assertions that `mktemp -p` (including the `-dp` combined-cluster form) is now flagged and that the portable `mktemp "$DIR/template"` form is not. - **A real, verified scope boundary.** Running `scripts/check-shell-portability.sh --all` against a pristine `origin/main` (before this PR, in a throwaway detached worktree) already exits 1 with 68 pre-existing `PORTABILITY:` findings — none of them `mktemp`. Running the identical scan on this branch produces the byte-for-byte identical finding set (`mktemp` migration nets zero), confirming those 68 are pre-existing corpus debt orthogonal to this issue's scope (the regex-escape family `\b \< \> \s \S \w \W`, deliberately bare/over-flag by design, plus one unrelated unrelated `sed -i` site). Filed separately as #1540 rather than expanding this PR's blast radius into an unrelated ~68-site triage effort. - **5 annotations, in scope.** Touching `block-hook-bypass.test.sh` and `markdown-format.test.sh` (to fix their own `mktemp -p` sites) makes the diff-gated CI check scan those files' FULL content, which surfaced 5 of the 68 pre-existing findings in those two files specifically (PowerShell module-qualified command strings, a Windows path literal, and one unsuffixed `sed -i` in a test fixture — all pre-existing, unrelated to `mktemp -p`, none on lines this PR touches). Since this PR's own act of touching those files is what makes them newly load-bearing for CI, annotated all 5 with `portability-ok: <reason>` rather than letting an unrelated pre-existing gap fail this PR's own CI run. Verified: `check-shell-portability.sh origin/main` (diff-mode, what CI actually runs) is clean before and after — 0 findings. - Bumped `plugin.json` + added a `CHANGELOG.md` entry for all 13 touched plugins (actionlint, autonomy, bash-format, biome-format, claude-ops, desktop-notification, eol-normalizer, go-format, guardrails, markdown-format, powershell-format, ruff-format, typos-format) — test-only changes, so each gets a patch bump under `### Changed`. `guardrails` landed at `0.17.2` (not `0.17.1`, which a concurrently-merged PR (#1503) claimed first) after a rebase conflict. ## Test plan - [x] `bash scripts/check-shell-portability.test.sh` — 71/71 passing, including the new mktemp assertions (flags `mktemp -p`, flags the `-dp` combined cluster, does not flag the portable `mktemp "$DIR/template"` form) and the retained date-d/stat-c-still-staged assertion. - [x] `scripts/check-shell-portability.sh origin/main` (diff-mode, what CI runs on this PR) — clean, 0 findings across the 24 changed shell files. - [x] `scripts/check-shell-portability.sh --all` against the full corpus — 63 pre-existing findings remain (68 minus the 5 annotated in this PR's own touched files), all pre-existing and tracked in #1540; zero `mktemp` findings anywhere. - [x] Every one of the 24 migrated `*.sh` files executed directly and passing: `actionlint-check`, `lane-stop-gate`, `bash-format`, `biome-format`, `desktop-notification`, `eol-normalizer`, `go-format` (skipped — no `goimports` binary on this host, pre-existing/unrelated), `block-dangerous-git` (305/305), `block-hook-bypass` (203/203), `block-no-verify` (112/112), `cli-flag-verify` (48/48), `flag-commit-pr-skill-bypass` (28/28), `hardcoded-path-check` (72/72), `secret-pattern-detection` (42/42), `skill-reference-verify` (68/68), `stale-path-verify` (73/73), `workflow-resilience-check` (15/15), `markdown-format` (92/92), `powershell-format`, `ruff-format` (52/52), `typos-format` (41/41). `claude-ops-test-helpers.sh` and `guardrails-test-helpers.sh` are sourced helpers (never directly executed). - [x] `shellcheck --rcfile=.shellcheckrc` on every changed `*.sh` file — clean. - [x] `typos --config _typos.toml` on every changed file — clean. - [x] `markdownlint-cli2` on every changed `CHANGELOG.md` — clean. - [x] `git diff --stat` reviewed: every `plugin.json` diff is a single version-line change (no unintended reformatting/escaping — an early `json.dump`-based approach mangled em-dashes into `—` escapes across whole files and was caught and redone as a targeted regex substitution before committing). ## Related Closes #1527. Follow-up filed: #1540 (the 68 pre-existing, untriaged `--all` corpus findings this issue's own acceptance criteria surfaced but which are out of scope for a `mktemp -p`-only migration). Follows #1510 (staged-class enable trigger), #1491/#1511 (the gate itself). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…uests (#1781) ## Why A review that lands **after** a merge has nowhere to go: - the ruleset's `required_review_thread_resolution` is a **merge-time predicate** that already passed; - the babysit lane works *open* PRs, and a merged PR leaves that queue; - nothing on a merged PR surfaces its open threads — GitHub shows the merge, not the findings. Six findings — one **P1** — posted **46 seconds after #1720 merged** and sat unread for a day. They surfaced only because a later session happened to audit that merge batch. Nothing was bypassed; the gate was satisfied *because the threads did not yet exist*. The morning brief is the right home: read-only, unattended, and already where attention signals land. ## What it does Compares each unresolved thread's **first-comment timestamp** against the PR's `mergedAt`, and reports only threads the gate could never have seen. A thread that predates the merge was visible to the gate — that is an ordinary unresolved thread, not this failure mode, and it stays out. - **One line per PR, at that PR's worst severity, with a finding count.** Several findings on one PR are one thing to go look at; repeating the title per thread buries every other PR. Collapsing on the *worst* severity means a P0 sitting beside advisory findings can never be softened. - **Severity survives to the operator** — a stranded P1 must not read like a P3. - **`--stranded-days`** (default 3) — wide enough to cover slow bot review *and* an operator-absent weekend. ## It fails loud, not clear A GraphQL error document is well-formed JSON that simply carries no `data`. The extraction would yield an empty list and render **"every merged PR in the window is clear"** — an all-clear asserted from an answer never received, which is the same fail-open shape this section exists to catch. This is not hypothetical: a rate-limit error did exactly that during development. An API error now says explicitly that it is *not* an all-clear, and prints the message. Covered by a regression case. ## This is a standing leak, not a one-off Its **first live run** against this repository immediately surfaced four more stranded findings on other merged PRs — including a **P1 on #1694** (merged `05:04:45Z`, finding posted `05:05:20Z`, 35 seconds later) recording that a shipped `autonomy` cell **never reached installations**. ## Verification - `morning-brief.test.sh`: **30 → 63 cases, 0 failures.** - The **negative** cases carry the weight — a pre-merge thread, an already-resolved post-merge thread, and a merge outside the window must all stay silent, or the section is noise rather than signal. Plus: collapse-does-not-soften-severity, highest-severity-first, window-widening, and the API-error case above. - The fixture mirrors the real #1720 shape, including the 46-second gap. - `shellcheck -x` on script and test — clean. One `SC2016` is declared, not blanket-suppressed: the `$owner`/`$name`/`$endCursor` in the GraphQL query are server-side variables bound by `-F` and **must** reach the server unexpanded. - `node scripts/validate-plugin-contracts.mjs` — 43 setup skills, 2153 files, pass. - `npx markdownlint-cli2` on both changed markdown files — 0 errors. ### Live run — posted in full in the comments below A live run on the current branch found **44 merged PRs carrying post-merge findings in a five-day window: 0 P0, 10 P1, 34 P2.** Among the P1s: **#1503**, a guardrail-bypass fix whose own review landed unread, and **#1322** with 5 findings. Read the **second** comment for the authoritative figures — the first was produced by the pre-review severity logic and reported a false P0, which review then caught. No truncation warning fired, so the read is complete. The five-day window filter was spot-checked against `mergedAt` (a PR numbered #969 in a 5-day window looks wrong until you check: it merged `2026-07-25`, 4.2 days before the run). This is a far larger leak than the six findings that exposed it. ## Related Closes #1777 Refs #1720 Refs #1759 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…1785) ## Why A **reproducible bypass of the commit guard**, reported on #1503 *after that PR merged* — so the thread-resolution gate never saw it and nothing surfaced it. Found by the stranded-findings sweep. The directory and locating-global helpers (`effective_dir`, `collect_locating_globals`, `explicit_git_dir`) were handed the **whole pre-git argv slice**, wrapper arguments included. They parse for git's `-C` / `--git-dir` / `--work-tree`, and they cannot know which *wrapper* options take a value. GNU env's `-u NAME` consumes the next word as the variable to unset. So in `env -u -C git …`, the `-C` is env's operand and `git` is the command — **git receives no `-C` and never changes directory**. The 0-based slice instead saw the bare tokens `-C git` and resolved into `./git`. The guard therefore inspected one repository's aliases while git executed another's. With `child` holding `commit --allow-empty -m bypass` and the decoy `git/child` holding the canonical `commit -F -`: ``` env -u -C git -c alias.a='!git -C child p' a ``` returned **0** — allowed — while real git committed non-canonically. ## What changed The three helpers now receive only the slice from the **resolved git token** (`gi`) to the subcommand. Four call sites. The two sites that build `nextw` still start at index 0, deliberately: they *reconstruct the invocation* rather than parse git's options, and dropping the wrapper there would rewrite the command. The boundary is documented at `effective_dir` so the next caller does not reintroduce it. ## Verification — against the unfixed hook, not merely green A guardrail test that cannot fail proves nothing, so the regression case was run against `main`'s hook first: | | unfixed (`origin/main`) | with this fix | |---|---|---| | `env -u -C git …` reaches the non-canonical commit | **FAIL — exit 0** (bypass reproduced) | **ok — exit 2** | | git's own `-C` still relocates the alias lookup | ok — exit 2 | ok — exit 2 | The second row is the guard against over-correcting: the narrower slice must not stop honouring a relocation git really performs. Full suite: **109 assertions, 0 failures.** `shellcheck -x` clean on both files. ### One correction made during this work I first wrote a second case asserting `env -C git …` was also a bypass. It is not — `hook-utils.sh` already treats `-C` as an env option consuming its operand, so `git` becomes env's *directory* and the resolver correctly finds no git command. That assertion was wrong and was removed rather than papered over. ## Related Closes #1503 Refs #1777 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Replaces #1085, whose branch could not be merged.
Why this PR exists
#1085 carried commit
baa459b4, which had a correct committer email but no signature at all (verification.reason: unsigned).mainrequires verified signatures and GitHub evaluates every PR commit not reachable from the base, so #1085 was permanently BLOCKED with all checks green and zero unresolved threads. Adding further signed commits does not clear it — GitHub still sees the unsigned one.Fixing it in place would need a history rewrite (force-push), which is denied in this environment. So the branch was rebuilt as a single correctly-signed commit on current
main.Content
Identical to #1085 — a clean 3-way
merge --squashof its branch onto currentmain, no conflicts, so nomaincontent was reverted:hooks/block-dangerous-git.shandhooks/block-noncanonical-commit.sh— close the alias-chain re-expansion bypass.test.shsuites for bothRelated
Closes #964. Replaces #1085.