fix(e2e): scrub git's repo-relocating env before the throwaway workspace repo - #958
Conversation
…ace repo `scripts/lib/tier-live-sources.sh` built its throwaway workspace with a bare `git init` / `git -C "$WS" add -A` / `commit`. A git hook exports GIT_DIR and GIT_INDEX_FILE into every child it spawns, and those OUTRANK `git -C <dir>` — so run from a hook, that sequence does not touch `$WS` at all: it commits the developer's own tree into the developer's own repo, exit 0, no output. That is #893, which landed twice while printing a clean pass, and `scripts/gitenv.py` used to hold the guard until #956 deleted it along with the generator it served. No live trigger today, and the PR body says so: `.githooks/pre-push` runs `just preflight`, `preflight` is `lint clippy hack test`, and `tier-live-sources.sh`'s only caller is the manual, billed `just live-sources`. This keeps that true for the next caller rather than relying on it. `e2e_init_repo` in `e2e-common.sh` scrubs via `git rev-parse --local-env-vars` — git's own answer, 15 vars, fetched not remembered. NOT a `GIT_*` prefix match: that would also strip GIT_SSH_COMMAND and the GIT_COMMITTER_* identity, neither of which relocates a repo, and the selftest pins that distinction. The selftest runs the NAIVE form first and proves it corrupts, so a scrub that stopped scrubbing cannot pass vacuously. Two rounds of mutation testing shaped it, and the second round found a real hole in my own first version: round 1 keyed the safety assertion on the real repo's INDEX. Under the mutation (scrub emptied) it PASSED — because the unscrubbed helper's third command is `commit`, which lands the stolen files as a real commit and leaves `diff --cached` clean again. The index is not the damage; the commit is. round 2 assert on HEAD. Now the load-bearing check reds under the mutation. The index check stays as belt-and-braces, with a comment saying it is NOT the one with teeth and why. GIT_WORK_TREE is exported alongside the other two: without it an unscrubbed `add -A` resolves its work tree from the CWD, so what it stages depends on where the suite was invoked and the assertion passes or fails by accident. Wired into `just lint`'s fan-out AND enumerated in ci-lint.yml's hygiene job — that job lists each selftest as its own step, so the recipe alone would have run locally and never in CI. shellcheck + shfmt clean; `just lint` green (`✓ e2escrub`); actionlint and the 116 Rego CI-contract tests pass on the workflow edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017WdFnoFgpKUZdQtqQY5w5F
Claude Security ReviewHead: Findings: 0 — No findings The diff only touches CI/test tooling (a git-env scrub in scripts/lib/e2e-common.sh, its self-test, and CI wiring) that fixes a known bug where a git hook's exported GIT_DIR/GIT_INDEX_FILE could redirect an e2e helper's git commands into the developer's real repo; the scrub uses git's own authoritative Automated read-only review by Claude Code |
Claude ReviewHead: Findings: 0 — No findings Clean, well-scoped fix for the #893 git-hook env-leak bug: e2e_init_repo() scrubs GIT_DIR/GIT_INDEX_FILE/GIT_WORK_TREE (and other local-env-vars) via git's own --local-env-vars list rather than a hand-rolled GIT_* prefix match, replaces the one existing call site (tier-live-sources.sh), and ships a self-test with a genuine negative control (proves the naive form corrupts, keyed on HEAD not just the index) that is wired into both Automated read-only review by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #958 +/- ##
==========================================
- Coverage 95.70% 95.70% -0.01%
==========================================
Files 196 196
Lines 57587 57587
==========================================
- Hits 55113 55112 -1
- Misses 2474 2475 +1
🚀 New features to boost your workflow:
|
… empty==empty escape All three lenses landed the same HIGH, reproduced end-to-end with no mutation: the selftest built its victim stand-in with the naive form at lines 34-37, which run BEFORE the suite exports its own GIT_DIR. Under an ambient GIT_DIR the fixture created no repo of its own and committed into the developer's — #893, inside the file that exists to pin #893 — and the suite still printed OK. It printed OK because the linchpin assertion compares head_of_real() to head_before, and with no fixture repo both sides are empty: test "" = "" passes in exactly the state the scrub exists to prevent. The trigger is live, not theoretical. A LINKED worktree's pre-push exports GIT_DIR (a plain checkout's does not — measured both ways), and .githooks/pre-push -> just preflight -> lint -> justfile:319 reaches this file. This repo mandates worktrees for concurrent sessions, so the blast radius was every push from one — strictly wider than the hazard the PR fixes, whose only caller is the manual, billed `just live-sources`. CI cannot see it: the runner uses a fresh plain checkout and invokes the recipe directly, never from a hook. Fixes: - Clear git's own list at the top of the selftest, before anything builds a repo. githooks(5) prescribes this idiom verbatim, fetched in-session from `man githooks` on git 2.54.0. - Assert the fixture repo has a HEAD, so the comparison cannot pass vacuously. - e2e_init_repo now fails closed: an empty scrub list was indistinguishable from "nothing to scrub" and left every git call inheriting the caller's GIT_DIR. Verified four ways: clean env green; scrub emptied still reds 3 checks (tooth preserved); leaked GIT_DIR at a throwaway victim leaves it byte-identical while the suite stays green on genuine checks; a stub git makes the new guard return 1. shellcheck -x 0, just shfmt-check 0, just e2e-scrub-selftest 0. plan-miss: the design gate treated the selftest's fixture setup as out of the scrub's scope; the hazard class applies to any git call in the file, not only the helper's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML
Claude Security ReviewHead: Findings: 0 — No findings No security-relevant changes: the diff only adds/edits dev-only e2e test tooling (justfile, CI workflow entry, and bash scripts for git-repo test fixtures) and does not touch the hook shim, socket/pipe transport, config writes, or credential/production-process handling. Automated read-only review by Claude Code |
Two-lens review — round 1 disposition (head
|
Claude ReviewHead: Findings: 0 — No findings Clean, well-scoped bugfix: extracts the naive git-init/add/commit sequence in tier-live-sources.sh into a shared e2e_init_repo helper that scrubs GIT_DIR/GIT_INDEX_FILE/GIT_WORK_TREE (using git's own Automated read-only review by Claude Code |
…'t return Round 1 added a fail-closed check to e2e_init_repo and got the mechanism wrong: `return 1` is only a guard if a caller reads the status, and none does. tier-live-sources.sh:13 is `set -uo pipefail` with no `-e`, and :78 calls `e2e_init_repo "$WS"` bare — so the round-1 version printed a warning and then ran the unscrubbed git calls anyway. A guard that does not guard is the exact class this PR exists to remove, and I shipped one. `exit 2` instead, reusing this file's own authority (`e2e_require_bin`, e2e-common.sh:16) rather than inventing a second convention. Measured with a shadowed git in both failure modes, against a caller shaped like the real one (no -e, status ignored): rev-parse exits 129 -> caller exit=2, aborted before the next line rev-parse exits 0, empty stdout -> caller exit=2, aborted victim repo HEAD unmoved in both Under round 1 both modes returned 0 to the caller, which continued and corrupted the victim. Unchanged and re-verified: selftest 7/7 green, emptying the scrub still reds 3 checks (tooth preserved), leaked GIT_DIR leaves a throwaway victim untouched. shellcheck -x 0, just shfmt-check 0, just e2e-scrub-selftest 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML
Round 2 disposition (head
|
git rev-parse --local-env-vars |
round 1 | round 2 |
|---|---|---|
| exits 129 | returns 0, caller continues, victim corrupted | caller exit 2, aborted before the next line |
| exits 0 with empty stdout | returns 0, caller continues, victim corrupted | caller exit 2, aborted |
Victim HEAD unmoved in both round-2 modes.
Round-1 findings re-verified at this head
Selftest 7/7 green · emptying the scrub still reds 3 checks (tooth preserved) · leaked GIT_DIR leaves a throwaway victim byte-identical · shellcheck -x 0 · just shfmt-check 0 · just e2e-scrub-selftest 0 · full preflight on push, 3174 passed.
Every round-2 change is a one-line hardening with its failure mode measured both before and after. Zero open confirmed HIGH. Within the two-round cap — no round 3.
Claude Security ReviewHead: Findings: 0 — No findings This diff only adds/wires CI self-test tooling for the e2e git-env scrub helper (scripts/lib/e2e-common.sh, e2e-common-selftest.sh, justfile, ci-lint.yml) and does not touch the hook shim, config writes, socket/pipe transport, or transcript/JSONL/pack ingestion, so there are no security-relevant changes. Automated read-only review by Claude Code |
Claude ReviewHead: Findings: 0 — No findings Clean, well-tested fix wiring a git-env-var scrub into e2e_init_repo (fixing the #893 leaked-GIT_DIR class of bug) with a self-test that proves both the corruption and the fix via a negative control; no architecture-invariant violations, bugs, or missing coverage found. Automated read-only review by Claude Code |
…o paths Three lenses. The 10-line change had a defect in the one line that mattered, introduced two regressions, and shipped without the test I argued was impossible. All three of those were my calls. **The guard did not guard.** `unset $(git rev-parse --local-env-vars)` discards the substitution's status — only `unset`'s own counts, and bare `unset` returns 0 — so under `set -euo pipefail` a failing git left the scrub silently not happening and the push proceeded. Measured three ways. This is the same defect as #958 round 1's `return 1`, in a second disguise: I wrote a guard, asserted it was fail-closed, and did not run it. Now an explicit `if ! scrub=$(...) || [ -z "$scrub" ]` with `exit 1`. **Two regressions the change introduced, both found by an adversarial lens:** - `cd "$(git rev-parse --show-toplevel)"` sits after the scrub, which removed the only locator when the hook's CWD is outside the worktree (`git --git-dir=… push` from elsewhere). And `cd "$(...)"` swallows the failure by the same mechanic as `unset $(...)` — nine lines apart, and I fixed one without seeing the other. `toplevel` is now resolved before the scrub. Measured: OLD exit 0, NEW-before-fix exit 1 in /tmp, NEW-after 0. - `SKIP_PREFLIGHT=1` no longer bypassed the new failure, breaking the contract line 8 advertises. The early exit now precedes the scrub, which costs nothing since it exits before anything uses the scrubbed env. **The test I said was impossible.** I argued CI structurally cannot execute a hook test, so shipped none. That conflated testing a push with testing the hook's env contract, which is observable at its `exec`: stub `just`, hand the hook the GIT_DIR a linked worktree exports, read what the child saw. 184ms, no push, no worktree, no toolchain. The failure it now catches is silent by construction — deleting the line reds nothing otherwise. It enumerates `.githooks/*` rather than naming pre-push, so a hook added later fails here instead of inheriting the gap; `pre-commit` is exempt by name while it only runs `cargo fmt`. The control is a literal two-line hook, not one derived by `grep -v` from a real one — deriving it orphans the guard's `if` body, so the mutant exits before `just` and the control silently stops controlling. **The test's own first version was vacuous, caught by running the mutation.** Its `git init` for the fixture ran under the GIT_DIR this suite exports at its top, so `$s/repo/.git` was never created; the hook then died at `toplevel=` and never reached `exec`, `seen` never existed, and `test -z ""` passed against the real hook while the control still fired. The harness was bitten by the bug it tests for. Cleared the list around that init. Both copies now have teeth, verified by mutation: deleting the hook's scrub block reds; a behavioural mutation that keeps the block but unsets nothing also reds; emptying the helper's scrub reds. Also: `LINKED` restored to the hook comment — trimming it on owner feedback turned a true sentence false, since a plain checkout's pre-push exports nothing. And `e2e-common-selftest.sh`'s scrub rationale named the pre-push chain as its cause, which this commit makes false; it now names direct runs and `submodule foreach` (verified in-session: exports GIT_DIR=.git to children). shellcheck 0 · shfmt-check 0 · just lint 0 (19/19) · selftest 9 checks. plan-miss: I scoped this PR as "3 lines, low risk, no test" and was wrong on both halves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML
1 participant
Restores the guard #956 deleted, as a mechanism instead of a script.
The hazard
scripts/lib/tier-live-sources.shbuilt its throwaway workspace with a baregit init/git -C "$WS" add -A/commit. A git hook exportsGIT_DIRandGIT_INDEX_FILEinto every child it spawns, and those outrankgit -C <dir>— so run from a hook, that sequence does not touch$WSat all. It commits the developer's own tree into the developer's own repo. Exit 0, no output.That is #893, which landed twice while printing a clean pass.
scripts/gitenv.pyheld the guard until #956 deleted it along with the generator it served.Measured, not reasoned — with
GIT_DIRexported:No live trigger, and the PR says so
.githooks/pre-pushrunsjust preflight;preflightislint clippy hack test;tier-live-sources.sh's only caller is the manual, billedjust live-sources(justfile:673). Nothing reaches those git calls from a hook today. This keeps that true for the next caller rather than depending on it.The scrub
e2e_init_reposcrubs viagit rev-parse --local-env-vars— git's own answer, 15 vars, fetched in-session rather than remembered. Deliberately not aGIT_*prefix match: that would also stripGIT_SSH_COMMANDand theGIT_COMMITTER_*identity, neither of which relocates a repo. The selftest pins that distinction.The selftest, and the hole mutation testing found in it
It runs the naive form first and proves it corrupts, so a scrub that stopped scrubbing cannot pass vacuously.
Two rounds of mutation testing (empty the scrub, the suite must red). The second round found a real hole in my own first version:
commit, which lands the stolen files as a real commit and leavesdiff --cachedclean again. The index is not the damage; the commit is.GIT_WORK_TREEis exported alongside the other two: without it an unscrubbedadd -Aresolves its work tree from the CWD, so what it stages depends on where the suite was invoked and the assertion passes or fails by accident.Hermetic — one
mktemp -d, no network, the real repo is never a target.Wiring
Both places, because one alone is a no-op:
just lint's fan-out (✓ e2escrub)ci-lint.yml's hygiene job — that job enumerates each selftest as its own step, so the recipe alone would have run locally and never in CI.Gates
preflightgreen (3174) ·shellcheck -xexit 0 ·shfmt-checkclean ·just lintgreen ·actionlintclean · 116 Rego CI-contract tests pass on the workflow edit.Note
Supersedes the stale
fix/e2e-env-scrubbranch (uncommitted work in another worktree). Branched fresh offmain.🤖 Generated with Claude Code
https://claude.ai/code/session_017WdFnoFgpKUZdQtqQY5w5F