Skip to content

Document and gate worktree seed provenance, 0700 default path - #1037

Open
DROOdotFOO wants to merge 11 commits into
masterfrom
fix/worktree-provenance
Open

DROOdotFOO wants to merge 11 commits into
masterfrom
fix/worktree-provenance

Conversation

@DROOdotFOO

@DROOdotFOO DROOdotFOO commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Summary

Hardens scripts/worktree.sh so cache seeding cannot silently cross trust or filesystem boundaries.

  • Reject option-like --from values and resolve revisions to commit OIDs before directory creation.
  • Atomically claim explicit destinations at mode 0700; default paths remain private mktemp directories.
  • Fall back to /tmp when TMPDIR is empty, invalid, or /.
  • Compare root/package mix.exs and mix.lock provenance.
  • Reject symlinked manifests, package paths, and cache destinations.
  • Serialize each target's seed operation with a private git-dir lock.
  • Treat the complete seed as one transaction; rollback all replaced caches in reverse order on errors or signals.
  • Verify directory identity and repository provenance before recursive failed-add cleanup; preserve and report residue when cleanup cannot be proved safe.
  • Keep usage errors concise while preserving full explicit help.
  • Update tracked docs to match the implemented guarantees.

Manual testing

  • bash -n scripts/worktree.sh
  • Actual add --fresh / rm smoke completed and cleaned its branch/path
  • Leading-dash --from was rejected before path creation
  • Failed-add residue check confirmed no requested path remained
  • Transaction, signal rollback, lock contention, symlink, manifest-drift, and TMPDIR=/ smokes were exercised during implementation

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

Unified Regression Test Results

Workflow: Unified Regression Testing
Event: pull_request
SHA: e47836a

Performance Results

Found 1 performance result(s)

Memory Results

Found 3 memory result(s)

Targets

  • Parser: <3μs average
  • Render: <1ms average
  • Memory: <3MB per session

@DROOdotFOO

Copy link
Copy Markdown
Owner Author

Adversarial Review: PR #1037 (worktree-provenance)

Three personas ran independently (Saboteur, New Hire, Security Auditor), all three driving the branch's script against scratch repos in /tmp.

CRITICAL

  • A symlinked packages/<pkg> in the branch under review makes the seed delete and overwrite arbitrary directories. [Security Auditor + Saboteur -> promoted from HIGH] seed_caches globs packages/*/deps and packages/*/_build in the TARGET worktree and replace_dirs through them. Reproduced in a scratch repo: a branch committing packages/foo as a symlink to /tmp/wtaudit/victim -> worktree.sh add evil ... exits 0, prints "seeded ... (root + 1 package(s))", and victim/_build/canary.txt is GONE, with victim/_build and victim/deps replaced by the source's caches. Aimed at the source checkout itself (packages/foo -> /tmp/wtaudit2/src) it deleted the SOURCE's _build/ROOTMARKER. The exact use case this PR's own header calls out — "a worktree created to review someone else's branch" — is the attack. The fix is cheap: [[ -L "$target/packages/$p" ]] && skip, or resolve each package dir with -P and refuse anything outside $target.

HIGH

  • The lock gate is defeated by the branch it gates. [Security Auditor] cmp -s and -f follow symlinks, so a branch committing mix.lock as a symlink to the source checkout's own lock compares the file to itself and passes (worktree.sh:209). Verified, exit 0. Same class as the finding above: branch-controlled filesystem shapes are not defended anywhere in the seed path.
  • git worktree add failing AFTER checkout leaves a populated, registered, unseeded worktree plus a stray branch. [Saboteur] The new unwind is rmdir with failures swallowed (:349-352). Reproduced with a core.hooksPath post-checkout that exits 1: git materialises and registers the worktree, the hook fails, rmdir fails silently, the script prints nothing and exits 1, leaving deps=NO _build=NO — precisely the unseeded worktree the adjacent comment says must never exist. Separately, git worktree add -b creates the branch before materialising, so even a pre-checkout failure (read-only parent, exit 128) leaks the branch, and re-running then fails with the misleading "already exists; --from would be ignored".
  • sync inherits the exit-3 gate with no escape hatch. [Saboteur + New Hire -> promoted from MEDIUM] assert_locks_match lives inside seed_caches, which cmd_sync also calls — so the one command whose whole purpose is "re-seed after a dependency bump in this checkout" now refuses exactly that case, and the remedy it prints (add --fresh) means destroy and recreate the worktree. Worse, cmd_sync parses no flags: sync <path> --fresh silently seeds and exits 0, the opposite of what was asked, while add rejects unknown options.

MEDIUM

  • The comment justifying the gate says "Mix will not notice" a lock mismatch; a scratch mix compile with a mismatched lock shows Mix does notice [Saboteur].
  • The refusal message names a target directory that is deleted milliseconds later, does not say which side is ahead, and gives backwards advice for its most likely trigger (an uncommitted mix.lock in the source checkout) [New Hire].
  • The explicit-path [[ -e || -L ]] check is check-then-use; the header's "still refused if anything is already there" reads as a guarantee and is best-effort [Security Auditor].

LOW

  • The unwind claims it deleted a branch it did not delete when the branch pre-existed [Saboteur + New Hire -> promoted to MEDIUM].
  • --help mentions neither the trust boundary nor --fresh's cold-build cost [New Hire]; scripts/README.md still describes the old behaviour [New Hire]; a branch name starting with - is not rejected [Saboteur]; the lock-path dedupe uses a string scan where an associative array is clearer [New Hire]; CLAUDE.md:143 omits the "staleness, not integrity" caveat the script header states correctly [Security Auditor].

Verified negatives

shellcheck 0.11.0 and bash -n clean. mktemp default path: 0700, unguessable, correct under TMPDIR unset / trailing slash / spaces / nonexistent / read-only. Four concurrent add invocations and a same-branch race unwound correctly. cp -R does not dereference tree symlinks on either BSD or GNU, and the repo's 109 _build symlinks are all relative — the seed plants no links back into the source. --fresh really does avoid the caches, and the Hex tarball cache it falls back on IS checksum-verified (sha256 of the cached jason-1.4.5.tar equals the mix.lock entry). The tamper probe confirms the header's "staleness check, not an integrity one" paragraph is exactly right. Nothing in the script executes branch-controlled content (core.hooksPath is source-side; no post-checkout hook present).

Cross-persona overlaps

Issue Personas Promotion
Branch-controlled symlink in packages/* Security Auditor, Saboteur HIGH -> CRITICAL
sync + exit 3 + ignored --fresh Saboteur, New Hire MEDIUM -> HIGH
Unwind message lies about the branch Saboteur, New Hire LOW -> MEDIUM

Verdict: BLOCK

Before merge: refuse symlinked packages/* entries and a symlinked mix.lock in the target (both are one-line guards and both defeat the feature this PR advertises); complete the unwind with git worktree remove --force plus a tried_branch marker; either exempt sync from the lock gate or give it a real flag parser and a sync-shaped remedy.

`add` takes the branch from $1 before the option loop runs, so
`add --fresh feature/x` binds branch=--fresh and path=feature/x. A
boolean flag is naturally written first, so `--fresh` widens a footgun
that git then reports as `git branch` usage text naming neither
argument.

Reject a leading `-` in the branch position with a message that shows
where options go, and route -h/--help to usage instead, on `add`,
`sync` and the top-level dispatch.
`rmdir "$path" 2>/dev/null || true` discarded its own failure, so a
`git worktree add` that failed after materialising or registering
anything exited with no `worktree:` line and left a 0700 temp
directory and a stale registration behind, one set per failure. The
repo rule is to log or rethrow, so collect what could not be undone
and say so.

`created_branch` was also only set once git returned success, but
`git worktree add -b` creates refs/heads/<branch> BEFORE the checkout:
a later failure leaked the ref, and the next run then took the
"already exists" path and died about --from being ignored, describing
the previous failure rather than the current command. Decide the ref
by show-ref during the unwind instead.

Asking show-ref for the branch before the path is created also keeps
the "--from would be ignored" refusal from leaving a fresh mktemp
directory behind.
The gate lived inside seed_caches, which `sync` also calls, so `sync`
always exited 3 -- and its documented job is to re-seed after a
dependency bump in this checkout, which IS a lock difference. The die
message told the caller to re-run with `add --fresh`, a flag `sync`
does not have (cmd_sync reads only $1). Split the check out and give
it a policy: `add` refuses, since no worktree exists yet and --fresh
is a correct answer, while `sync` warns and proceeds, which is what it
exists to do.

Refuse symlinks rather than comparing through them. `-f`, `cmp` and
the packages glob all follow links, so a target mix.lock symlinked at
this checkout's own lock compares byte-identical while Mix reads
something else, and a symlinked packages/<pkg> sends replace_dir
outside the worktree. The branch under review controls both, which is
exactly the threat the header names.

Say which side moved. The comparison reads working-tree files, so an
uncommitted mix.lock here -- mid `mix deps.update`, or a bump not yet
committed -- differs from every worktree at once, and "bring this
checkout to that lock first" is then advice to discard the newer lock.
Ask git which copy is the edited one and advise per direction.

Check the warm cache before the locks. A source with no _build/deps
cannot seed whatever the locks say, and reporting exit 3 there sent
the caller after the second blocker while the first still stood.
usage() extracts help with sed between `# Usage:` and `# ---8<---`,
and the TRUST BOUNDARY block sat above `# Usage:`, so the one contract
the printed text tells the caller to go read -- "the trust boundary in
this file's header" -- was the one thing it never printed. Move the
block inside the extracted range and point at it there.

While the header is open, state the default path's actual guarantee.
"Unpredictable" and "derivable" are not properties this script can
promise: how much entropy mktemp spends on eight characters is libc's
business. What it does promise is an atomic exclusive create at 0700,
so the path is never known-and-unowned, which is the part that answers
CWE-377. Lead with that, and document the per-subcommand gate, the
symlink refusal and the check ordering.
CLAUDE.md attributed the gate to `add` alone while it also fired for
`sync`, and omitted the header's own caveat that the lock comparison
is a staleness check and not an integrity one, so a reader who never
opens the script concluded the seed was verified. State both, plus the
symlink refusal, and that only --fresh buys checksum verification.

The default-path paragraph sold predictability as the security
property; the guarantee is mktemp's atomic 0700 create. Reword.

scripts/README.md listed only the speed claim, with none of the
caveats: add a pointer to `add --help`.
@DROOdotFOO DROOdotFOO changed the title Document and gate worktree seed provenance, unpredictable default path Document and gate worktree seed provenance, 0700 default path Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant