90 lines
8.6 KiB
Markdown
90 lines
8.6 KiB
Markdown
# #1174 — Wrapper guard rounds 10–11
|
||
|
||
## Objective
|
||
|
||
Make checkout enforcement judge Git placement operands rather than every HOME-shaped word in the command, without reopening `--separate-git-dir` placement under HOME.
|
||
|
||
## Plan
|
||
|
||
1. Reproduce the four over-blocks and the placement-option control at head `20d86e39`.
|
||
2. Add RED fixtures before production changes.
|
||
3. Extract clone/worktree placement operands from the existing shell-aware normalized stream.
|
||
4. Run the full guard corpus, historical-head discrimination, syntax/static checks, probes, review, and CI.
|
||
|
||
## Progress and evidence
|
||
|
||
- Reproduced: `NOTE=$HOME`, `--reference=$HOME`, `GIT_DIR=$HOME/x`, and `--template=$HOME/t` all blocked despite explicit `/src/wt` destinations.
|
||
- RED at `20d86e39`: expanded suite had 8 failures, all HOME-valued non-placement cases.
|
||
- GREEN: expanded suite passes 242/242.
|
||
- Round-10 probes: 7/7 placement expectations and 4/4 placement-option controls pass.
|
||
- Earlier path probes remain green: 60/60, 24/24, and 17/17.
|
||
- Historical discrimination with the 242-fixture suite:
|
||
- `3d0a882a`: 216 pass / 26 fail.
|
||
- `4b8eba95`: 222 pass / 20 fail.
|
||
- `20d86e39`: 234 pass / 8 fail.
|
||
- `bash -n`, ShellCheck warning-or-higher, and `git diff --check`: pass.
|
||
|
||
## Residual / risk
|
||
|
||
- Relative destinations whose effective path depends on cwd are tracked separately by #1197 and remain out of scope.
|
||
- Unknown future Git options with a separate following value fail closed when that value is HOME-shaped. This may require classification when Git adds an unrelated path-taking option, but prevents a new placement option from silently bypassing the guard.
|
||
|
||
## Round 11 objective and intake
|
||
|
||
- **Issue / PR:** #1174.
|
||
- **Objective:** Remove the finite boolean-flag allowlists that turn accepted clone/worktree flags into fake placement operands, while preserving all real HOME placement blocks.
|
||
- **Scope:** `wrapper-guard.sh`, its hermetic fixtures, and task documentation. Relative cwd-dependent destinations remain in #1197.
|
||
- **Surfaces:** security-sensitive Bash hook behavior and shell/Git option grammar; no API, DB, UI, auth, deploy, or dependency changes.
|
||
- **Budget assumption:** 25K working tokens; reduce exploratory matrices before reducing acceptance coverage.
|
||
|
||
### Round 11 plan
|
||
|
||
1. Use Git itself to classify accepted/rejected clone and worktree options, and Bash itself to resolve path-word expectations.
|
||
2. Add RED fixtures for all six reported clone flags, generated negations, and equivalent worktree grammar.
|
||
3. Replace the open-ended unknown-option fail-closed fallback with a parser based on the closed value-taking option surface; keep explicit placement options special.
|
||
4. Run the full corpus, historical discrimination, shell/static checks, targeted probes, independent code/security review, one push, and exact-head CI.
|
||
|
||
### Root-cause evidence
|
||
|
||
- Git 2.39.5 accepts all six reported clone flags and the broader generated family measured in the brief: `--bare`, `--mirror`, `--ipv4`, `--ipv6`, `-4`, `-6`, `--no-local`, `--no-reject-shallow`, `--no-bare`, `--no-sparse`, `--no-dissociate`, `--no-shallow-submodules`, `--no-quiet`, `--no-progress`, and `--no-recurse-submodules`; it rejects `--relative-paths` as unknown.
|
||
- Git 2.39.5 accepts worktree negations including `--no-force`, `--no-detach`, `--no-lock`, `--no-guess-remote`, and `--no-track`; the current finite worktree flag list does not describe that generated family.
|
||
- `bash -c "printf '%s' <word>"` resolves `$HOME/source`, `${HOME}/source`, and `"$HOME"/source` under HOME while `/src/wt` remains outside it.
|
||
- **Hypothesis:** only separate-value options need positive classification. Treat every other option token as a no-value flag unless it is the explicit placement option; this matches Git's non-enumerable boolean family and confines the residual to genuinely new future value-taking options.
|
||
|
||
### TDD and verification checkpoints
|
||
|
||
- RED against the unmodified `91cc37bc` guard: 253 pass / 22 fail in the initial expanded 275-fixture suite. Failures include all 15 accepted clone flags, accepted long abbreviations, short value-taking bundles, abbreviated placement, worktree metadata abbreviation, and both directions of bundled worktree branch parsing.
|
||
- An exploratory fail-closed residual test drove emission of every worktree positional. Re-review correctly showed that this over-blocked HOME-shaped commit-ish metadata; a new commit-ish fixture failed RED against that intermediate implementation (278 pass / 2 fail, including one transient message assertion) and the parser was restored to emit only the actual path.
|
||
- GREEN after remediation: 280/280.
|
||
- Ultron's 13-shape option probe: 13/13 correct, including the six reported over-blocks, HOME destinations, end-of-options, worktree controls, and a later-command placement.
|
||
- Round-10 probes remain green: 7/7 subject-placement expectations and 4/4 `--separate-git-dir` controls.
|
||
- Earlier shell/path probes remain green: 60/60, 24/24, and 17/17.
|
||
- `bash -n`, ShellCheck warning-or-higher, and `git diff --check`: pass.
|
||
|
||
### Deliberate residual
|
||
|
||
A future Git release could add a new separate-value option absent from the closed value grammar. It defaults to no-value flag parsing, which leaves the following word positional. For clone, this can fail open if that future option itself creates repository state at its value. For worktree, it can shift which word is read as the path. This hypothetical future ambiguity is accepted deliberately because failing closed on every unclassified option is proven to over-block Git's open-ended present-day boolean/`--no-*` family. Every value-taking and placement option Git currently supports is classified, including accepted abbreviations of `--separate-git-dir`. Relative cwd-dependent targets remain in #1197.
|
||
|
||
### Independent review checkpoint
|
||
|
||
- Initial Codex code/security review raised `--orphan` as value-taking. Upstream Git `master` contradicts that premise: the synopsis is `[--orphan] [(-b | -B) <new-branch>] <path> [<commit-ish>]`, and the prose derives the branch from the path when `-b`/`-B` is absent. `--orphan` is therefore correctly handled as a boolean flag.
|
||
- The security review separately identified the generic future worktree shift residual. An attempted fail-closed remediation emitted every positional, but code re-review correctly rejected it because valid grammar has only one placement positional and an optional commit-ish. Final behavior checks only the path and documents the hypothetical future option shift deliberately; paired actual-grammar `--orphan` fixtures cover safe/HOME paths and `-b` metadata.
|
||
- Security re-review initially had no findings. Code re-review's commit-ish blocker was remediated with a RED fixture and path-only restoration; final code re-review approved with no findings.
|
||
- Final security review then found non-canonical absolute and symlink aliases. Eight lexical fixtures failed RED against the prior implementation, followed by three symlink fixtures failing RED. Remediation expands only shell-visible HOME tokens, resolves the longest existing directory prefix physically, and lexically normalizes the nonexistent suffix. The suite is now 292/292.
|
||
- Inherent residual: a symlink can be replaced between pre-tool inspection and Git execution. Existing aliases are resolved; eliminating the race requires enforcement inside the filesystem mutation path rather than a text pre-hook. Security review classified this medium, and architectural closure is tracked in #1199.
|
||
- Final independent code review: APPROVE, 0 findings. Final security review: no critical/high findings; the single medium TOCTOU residual is explicitly tracked in #1199.
|
||
|
||
### Final local evidence
|
||
|
||
- Final hermetic suite: 292/292; the same suite against `91cc37bc` discriminates at 256 pass / 36 fail.
|
||
- Ultron option probe: 13/13; round-10 probes: 7/7 plus 4/4 controls; earlier shell/path probes: 60/60, 24/24, and 17/17.
|
||
- `bash -n`, ShellCheck warning-or-higher, `git diff --check`, sanitization gate, and test-enumeration gate (population 55; 38 enumerated; 18 signed exclusions): pass.
|
||
- Independent code review: APPROVE, 0 findings. Security review's remaining medium TOCTOU architecture residual is tracked in #1199; no critical/high findings remain.
|
||
- Repository-wide TypeScript gates require dependencies absent from this worktree; the canonical Woodpecker pipeline will run them against the pushed exact head.
|
||
|
||
### Documentation checklist
|
||
|
||
- `docs/PRD.md` updated with WPG requirements, acceptance, canonicalization, and residual risk.
|
||
- Task scratchpad updated in the same logical change set; `docs/TASKS.md` remains orchestrator-only.
|
||
- No API, auth, UI, navigation, deployment, user-guide, or admin-guide surface changed; OpenAPI, endpoint index, sitemap, and publishing are not applicable.
|