From cd727ad97659a765a0c9ec7831ddc8dd56a5f289 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 20 Aug 2026 17:14:39 +0800 Subject: [PATCH 1/9] fix(review): pin the verified git identity across the residue probe (#9557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `worktreeResidue` verified a tree's identity once and then ran five commands that each re-discovered the repository from `cwd`, through the same `.git` file the check had read — a file writable by anything running as this user. Two halves, and they close different windows: - **The gate now requires the gitfile's admin entry to point BACK at this tree.** A repository whose `core.worktree` names this path answers `--show-toplevel` with this path, so the old gate saw itself while every command after it measured the plant's index — which already holds the contamination as committed content, so `status` comes back clean for a tree with a mutant in it. A planted standalone repo has no admin entry to round-trip; `scratch-tree` gates its own reset on exactly this. - **The verified `--git-dir`/`--work-tree` are pinned onto every later spawn**, so a swap that lands AFTER the gate cannot redirect them either. The reason recorded against doing this — that an explicit pin changes what `ls-files` resolves, so it needs its own round — was measured and is wrong. Across a standalone checkout, a linked worktree, a superproject with an initialised submodule, and a worktree reached through a symlinked ancestor, all five commands return **byte-identical** output pinned and unpinned. What was right about it is that it needed measuring, which is what this is. The backpointer half is pinned by a test that goes red without it. The pin half is not: it closes a check-then-use window the suite cannot open deterministically (the swap has to land between two spawns). Its evidence is a shell repro — with the identity captured, then the gitfile swapped, discovery reports a clean tree while the pinned commands still report ` M a.ts` and the untracked probe file. Saying that rather than implying a pin that is not there. --- .../src/commands/review/lib/worktree.test.ts | 50 ++++++ .../cli/src/commands/review/lib/worktree.ts | 167 ++++++++++++++++-- 2 files changed, 204 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts index db899feba6e..4e741e42010 100644 --- a/packages/cli/src/commands/review/lib/worktree.test.ts +++ b/packages/cli/src/commands/review/lib/worktree.test.ts @@ -185,6 +185,56 @@ describe('worktreeResidue', () => { expect(worktreeResidue(tree).unmeasured).toBeUndefined(); }); + it('says UNMEASURED for a gitfile swapped at a repo that answers for this path', () => { + // The identity gate reads `--show-toplevel`, which prints the directory the + // `.git` FILE sits in — whatever that file points at. A repository whose + // `core.worktree` names this tree answers with this path, so the gate saw + // itself while every command after it measured the plant's index, which + // already holds the contamination as committed content. Measured: through + // discovery the swap reports a clean tree with the mutant on disk. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + // Genuine first, so the fixture is known to be measurable at all. + expect(worktreeResidue(tree).paths.sort()).toEqual([ + '__probe__.test.ts', + 'a.ts', + ]); + + const forge = join(repo, 'forge'); + mkdirSync(forge); + const fgit = (...args: string[]) => + execFileSync( + 'git', + [ + '-c', + 'user.email=t@t.t', + '-c', + 'user.name=t', + '-c', + 'commit.gpgsign=false', + ...args, + ], + { cwd: forge, encoding: 'utf8' }, + ); + fgit('init', '-q', '-b', 'main', '--template=', '.'); + writeFileSync(join(forge, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(forge, '__probe__.test.ts'), 'probe'); + fgit('add', '-A'); + fgit( + 'commit', + '-qm', + 'the mutant, as if it were the commit', + '--no-verify', + ); + fgit('config', 'core.worktree', tree); + writeFileSync(join(tree, '.git'), `gitdir: ${join(forge, '.git')}\n`); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + expect(got.unmeasured).toContain('does not point back'); + }); + it('says UNMEASURED, not clean, when a repository is planted at the path', () => { // The concealment: `rm .git && git init && git add -A && git commit` over // the contamination answers a clean `git status` for a dirty tree, and no diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts index d57edd0016f..c1ae610468d 100644 --- a/packages/cli/src/commands/review/lib/worktree.ts +++ b/packages/cli/src/commands/review/lib/worktree.ts @@ -474,10 +474,18 @@ interface IgnoreRule { function ignoreSourcesOf( cwd: string, paths: string[], + anchor: readonly string[] = [], ): Map | null { const r = spawnSync( 'git', - [...pipelineExcludeArgs(), 'check-ignore', '-z', '-v', '--stdin'], + [ + ...anchor, + ...pipelineExcludeArgs(), + 'check-ignore', + '-z', + '-v', + '--stdin', + ], { cwd, input: `${paths.join('\0')}\0`, @@ -539,13 +547,17 @@ function hidesEverything(pattern: string): boolean { * out of the worktree is not asked about at all — `ls-files` would reject the * pathspec, and unknown provenance is untrusted provenance. */ -function trackedIgnoreSources(cwd: string, sources: Set): Set { +function trackedIgnoreSources( + cwd: string, + sources: Set, + anchor: readonly string[] = [], +): Set { const inside = [...sources].filter( (s) => s.length > 0 && !isAbsolute(s) && !s.split('/').some((p) => p === '..'), ); if (inside.length === 0) return new Set(); - const r = spawnSync('git', ['ls-files', '-z', '--', ...inside], { + const r = spawnSync('git', [...anchor, 'ls-files', '-z', '--', ...inside], { cwd, encoding: 'utf8', maxBuffer: 64 * 1024 * 1024, @@ -557,6 +569,80 @@ function trackedIgnoreSources(cwd: string, sources: Set): Set { return new Set(r.stdout.split('\0').filter((p) => p.length > 0)); } +/** + * The repo-local `filter.` COMMANDS, when any are defined. + * + * Every checkout in this pipeline EXECUTES these — the scratch tree's reset and + * rebuild, and the probe tree's per-run restore — hooks being disabled covers + * hooks and not filters. The planting surface is two plain writes a probe can + * make into the COMMON dir this command's report calls shared: + * `git config filter.evil.smudge CMD` and one line appended to + * `$(git rev-parse --git-path info/attributes)`. discard and cleanup never + * wipe the common dir, so a filter planted while reviewing one PR fires on + * every later matching checkout of the user's OWN repository — persistence + * planted by reviewing a malicious PR, measured live. The two local config + * files are checked with `--file` rather than merged config because filters + * in the user's global config (git-lfs is the common one) are the user's own + * contract, exactly like any git command they run — while a probe's planting + * surface is the repo-local files. The state cannot be told apart from a + * filter the user set deliberately, and cannot be safely wiped, so a hit is a + * refusal upstream, not a cleanup here. + */ +export function localFilterCommands(worktree: string): string[] { + const files = spawnSync( + 'git', + ['rev-parse', '--git-common-dir', '--git-dir'], + { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() }, + ); + if (files.error || files.status !== 0 || typeof files.stdout !== 'string') { + return []; + } + const [commonDir, gitDir] = files.stdout.trim().split('\n'); + const common = resolve(worktree, commonDir); + const candidates = [ + join(common, 'config'), + join(resolve(worktree, gitDir), 'config.worktree'), + ]; + // Every OTHER worktree's per-worktree config too. This screen runs against + // the review worktree, but the checkout it authorises runs in the SCRATCH + // tree, whose own `/worktrees/