diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts index 4fc2d6211db..809ea99f403 100644 --- a/packages/cli/src/commands/review/base-tree.test.ts +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -15,9 +15,11 @@ // actually succeeded, since an A/B against a half-built tree measures the build, // not the diff. -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { execFileSync } from 'node:child_process'; import { + appendFileSync, + chmodSync, utimesSync, mkdtempSync, mkdirSync, @@ -29,8 +31,28 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { runBaseTree, type BaseTreeReport } from './base-tree.js'; import { baseWorktreePath } from './lib/paths.js'; +import { gitConfigPath } from './lib/test-utils.js'; import type { BuildTestReport } from './build-test.js'; +// The race witness needs a writer landing BETWEEN the opening screen and the +// creation add; the sweep is the code that runs between them, so it is the +// seam. Defaults to the real implementation — every other test runs it. +const seam = vi.hoisted(() => ({ + realDiscard: undefined as ((...args: unknown[]) => unknown) | undefined, + beforeDiscard: undefined as (() => void) | undefined, +})); +vi.mock('./lib/worktree.js', async (importOriginal) => { + const actual = await importOriginal(); + seam.realDiscard = actual.discardWorktree as (...a: unknown[]) => unknown; + return { + ...actual, + discardWorktree: (...args: unknown[]) => { + seam.beforeDiscard?.(); + return seam.realDiscard!(...args); + }, + }; +}); + const okBuild = { ok: true, toolchain: 'npm', @@ -94,7 +116,10 @@ describe('runBaseTree', () => { git(repo, 'worktree', 'add', '--detach', '-q', worktree, headSha); }); - afterEach(() => rmSync(repo, { recursive: true, force: true })); + afterEach(() => { + seam.beforeDiscard = undefined; + rmSync(repo, { recursive: true, force: true }); + }); it('creates a sibling worktree holding the BASE commit, not the head', () => { const r = run(); @@ -113,6 +138,100 @@ describe('runBaseTree', () => { expect(r.path).toBe(`${worktree}-base`); }); + it('REFUSES the creation checkout when the common config plants a content filter', () => { + // The creation checkout rewrites every file the base commit carries, + // which EXECUTES a planted filter, and nothing in the pipeline wipes the + // common dir the plant persists in. The screen must refuse the build the + // way the probe-creation screen refuses its checkout — an unavailable A/B + // (infrastructure), never an executed filter. + const pwned = join(repo, 'PWNED-base-create'); + appendFileSync( + join(repo, '.git', 'config'), + `[filter "evil"]\n\tsmudge = touch ${gitConfigPath(pwned)}\n`, + ); + mkdirSync(join(repo, '.git', 'info'), { recursive: true }); + writeFileSync(join(repo, '.git', 'info', 'attributes'), '* filter=evil\n'); + + const builds: string[] = []; + const r = run({}, (w) => { + builds.push(w); + return okBuild; + }); + + expect(r.available).toBe(false); + expect(r.note).toContain('content filter'); + expect(builds).toHaveLength(0); + expect(existsSync(pwned)).toBe(false); + expect(existsSync(baseWorktreePath(worktree))).toBe(false); + }); + + it('the creation checkout is INERT — a planted post-checkout hook never fires', () => { + // The screen certifies FILTERS only; `worktree add` still fires + // `post-checkout` from the shared common hooks dir (measured live, git + // 2.39 and 2.43, on this exact `--detach` shape). A probe writes the + // hook with the same facility as the filter plant — one write + chmod — + // so the creation spawn must carry the inert overrides too. + const pwned = join(repo, 'PWNED-base-hook'); + mkdirSync(join(repo, '.git', 'hooks'), { recursive: true }); + writeFileSync( + join(repo, '.git', 'hooks', 'post-checkout'), + `#!/bin/sh\ntouch ${pwned}\n`, + ); + chmodSync(join(repo, '.git', 'hooks', 'post-checkout'), 0o755); + + const r = run(); + + // Inert is not "the build failed": the tree came up and the build ran. + expect(r.available).toBe(true); + expect(existsSync(pwned)).toBe(false); + }); + + it('refuses a planted core.fsmonitor at the screen — and never executes it', () => { + // fsmonitor rides the refusal regex on the include posture: a planted + // command is execution the screen cannot certify. The creation refuses + // naming the key — and, refused or not, the planted command never + // fires. (The creation spawn ALSO carries the empty `-c` override, as + // defense against a config swapped in the screen-to-spawn window.) + const pwned = join(repo, 'PWNED-base-fsmonitor'); + appendFileSync( + join(repo, '.git', 'config'), + '[core]\n\tfsmonitor = touch ' + gitConfigPath(pwned) + '\n', + ); + + const r = run(); + + expect(r.available).toBe(false); + expect(r.note).toContain('core.fsmonitor'); + expect(existsSync(pwned)).toBe(false); + }); + + it('re-screens after the sweep — a plant between the screen and the add is refused', () => { + // The lock excludes other base-tree builders, not shards running attacker + // code: a concurrent probe can land the two-write plant the instant the + // stale tree's sweep finishes — after the opening screen read, before + // the creation add re-parses the config (measured live: 6/6 race + // iterations executed the plant). The seam stands in for that writer. + const pwned = join(repo, 'PWNED-base-race'); + seam.beforeDiscard = () => { + appendFileSync( + join(repo, '.git', 'config'), + `[filter "evil"]\n\tsmudge = touch ${gitConfigPath(pwned)}\n`, + ); + mkdirSync(join(repo, '.git', 'info'), { recursive: true }); + writeFileSync( + join(repo, '.git', 'info', 'attributes'), + '* filter=evil\n', + ); + }; + + const r = run(); + + expect(r.available).toBe(false); + expect(r.note).toContain('content filter'); + expect(existsSync(pwned)).toBe(false); + expect(existsSync(baseWorktreePath(worktree))).toBe(false); + }); + it('builds in the base tree, and only there', () => { const seen: string[] = []; const r = run({}, (w) => { diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index dd42529e33c..5ac1fb94272 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -56,6 +56,8 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { baseWorktreePath } from './lib/paths.js'; import { discardWorktree, + INERT_GIT_ARGS, + localFilterRefusal, sanitizedGitEnv, worktreeCreateFailureDetail, type SweepResult, @@ -260,12 +262,44 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // The parameter re-narrows: TS narrowing does not cross function scopes. function buildBaseTree(baseSha: string): BaseTreeReport { + // The creation checkout rewrites every file the base commit carries, which + // EXECUTES a planted filter — screen it the way the probe tree's creation + // is screened. The refusal reads as an unavailable A/B (infrastructure), + // never as a finding against the PR. + const filterRefusal = localFilterRefusal( + worktree, + "the base tree's creation checkout", + ); + if (filterRefusal) return unavailable(filterRefusal); let sweep: SweepResult | undefined; try { // Clear a stale base tree left by a crashed run — it would fail `add`. Its // stderr is kept, because it is usually what explains that failure. sweep = discardWorktree(worktree, tree); - git(worktree, 'worktree', 'add', '--detach', tree, baseSha); + // Re-screen beside the add, after the sweep: this is the only screened + // checkout that overlaps LIVE probes — the lock excludes other base-tree + // builders, not shards running attacker code, and the sweep's completion + // is the public signal a watcher plants on. A filter landing between the + // screen above and the add below executed in the creation checkout the + // screen certified clean (measured: 6/6 race iterations pwned); the + // re-screen narrows that window to the gap between itself and the add. + const rescreenRefusal = localFilterRefusal( + worktree, + "the base tree's creation checkout", + ); + if (rescreenRefusal) return unavailable(rescreenRefusal); + // INERT_GIT_ARGS: the screen reads filters only, while `worktree add` + // also fires `post-checkout` from the shared common hooks dir and runs + // a repo-local `core.fsmonitor` — both plantable, both measured live. + git( + worktree, + ...INERT_GIT_ARGS, + 'worktree', + 'add', + '--detach', + tree, + baseSha, + ); } catch (e) { return unavailable( worktreeCreateFailureDetail('base', e, String(sweep?.stderr ?? '')), diff --git a/packages/cli/src/commands/review/fetch-pr.test.ts b/packages/cli/src/commands/review/fetch-pr.test.ts index 0745cead4e0..8ae7eb0ee8a 100644 --- a/packages/cli/src/commands/review/fetch-pr.test.ts +++ b/packages/cli/src/commands/review/fetch-pr.test.ts @@ -30,6 +30,7 @@ import { DEADLINE_ENV, hasReviewDeadline } from './lib/deadline.js'; import type { MergeBaseResult } from './lib/merge-base.js'; import { buildRoleBrief } from './agent-prompt.js'; import { PARSE_ARGS_REPORT, tmpFile, worktreePath } from './lib/paths.js'; +import { INERT_GIT_ARGS, SCREEN_SPAWN_TIMEOUT_MS } from './lib/worktree.js'; import { buildDiffPlan } from './lib/diff-plan.js'; import { buildPlanReport } from './lib/report.js'; import { operatorReviewSettings } from './lib/review-settings.js'; @@ -253,6 +254,10 @@ const producerMocks = vi.hoisted(() => ({ execFileSync: vi.fn(), refExists: vi.fn((..._refs: unknown[]): boolean => false), releaseWorktree: vi.fn(() => ({ existed: false, freed: true })), + // The screen itself is owned by lib/worktree's own suite; here it is a seam + // — the contract under test is that fetch-pr ASKS before the creation + // checkout and refuses the fetch on a hit. Default: a clean repository. + localFilterRefusal: vi.fn((..._args: unknown[]): string | null => null), gitOpt: vi.fn((..._args: string[]): string | null => null), // The exit-status-aware probe as its own vi.fn: the default mapping (set // in beforeEach) can only produce exit 0 and the DEFINITIVE no (exit 1), @@ -354,6 +359,13 @@ vi.mock('./lib/git.js', () => ({ releaseWorktree: producerMocks.releaseWorktree, })); +// Partial mock: only the filter screen is steered from the tests; everything +// else (sanitizedGitEnv included) stays the real module. +vi.mock('./lib/worktree.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, localFilterRefusal: producerMocks.localFilterRefusal }; +}); + vi.mock('./lib/merge-base.js', () => ({ resolveMergeBase: producerMocks.resolveMergeBase, })); @@ -423,6 +435,7 @@ describe('fetch-pr report assembly', () => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); }); producerMocks.refExists.mockReturnValue(false); + producerMocks.localFilterRefusal.mockReturnValue(null); producerMocks.git.mockImplementation((...args: string[]) => args[0] === 'rev-parse' ? 'f00df00df00d' : '', ); @@ -572,6 +585,175 @@ describe('fetch-pr report assembly', () => { expect(reportCall).toBeUndefined(); }); + it('refuses the creation worktree when the local filter screen refuses', async () => { + // The review worktree's own creation checkout is the pipeline's actual + // FIRST checkout — every later screen runs inside the tree it creates — + // and it rewrites every fetched file, executing a planted filter. A + // screen hit must refuse the fetch before the checkout, roll the fetched + // ref back, release the lease, and write no report. + producerMocks.localFilterRefusal.mockImplementation( + (_cwd: unknown, checkout: unknown) => + checkout === "the review worktree's creation checkout" + ? "the repository's local config defines content filter(s) " + + 'filter.evil.smudge (in /repo/.git/config) — the review worktree’s ' + + 'creation checkout would EXECUTE them' + : null, + ); + + await expect(reportFor({})).rejects.toThrow( + /Failed to create worktree .*content filter/, + ); + expect(producerMocks.localFilterRefusal).toHaveBeenCalledWith( + process.cwd(), + "the review worktree's creation checkout", + ); + // The creation checkout itself never ran. `includes`, not positional: + // the spawn carries INERT_GIT_ARGS ahead of the subcommand, so a + // positional match never sees the add call and the ordering this test + // pins would go untested. + expect( + producerMocks.git.mock.calls.some( + (args: unknown[]) => args.includes('worktree') && args.includes('add'), + ), + ).toBe(false); + // No report: the refusal is a hard stop, not a degraded review. + const reportCall = producerMocks.writeFileSync.mock.calls.find( + ([path]) => path === '/tmp/fetch-report.json', + ); + expect(reportCall).toBeUndefined(); + // The lease this run created is rolled back with every other failure. + expect(clearReviewWorktreeLeaseIfOwned).toHaveBeenCalled(); + }); + + it('refuses the head fetch when the local filter screen refuses — before any fetch runs', async () => { + // The step-2 fetch is the pipeline's first network operation and + // EXECUTES the command-valued config keys the screen names (measured: + // a planted core.sshCommand ran during a pipeline-shaped fetch) — a key + // an earlier malicious PR's probe planted in the never-wiped common dir + // runs during it. The screen reads only repo-local config and needs + // nothing the fetch produces, so it runs FIRST: the refusal precedes + // every fetch spawn, the way step 4's screen precedes the creation + // checkout. + producerMocks.localFilterRefusal.mockImplementation( + (_cwd: unknown, checkout: unknown) => + checkout === "the review worktree's head fetch" + ? "the repository's local config names command-execution key(s) " + + 'core.sshcommand (in /repo/.git/config) — a checkout that ' + + 'lazy-fetches EXECUTES the commands they name' + : null, + ); + + await expect(reportFor({})).rejects.toThrow(/command-execution key/); + expect(producerMocks.localFilterRefusal).toHaveBeenCalledWith( + process.cwd(), + "the review worktree's head fetch", + ); + // The fetch itself never ran — and nothing downstream of it: no + // worktree add, no report. The refusal is a hard stop; the outer catch + // releases the lease. + expect( + producerMocks.git.mock.calls.some((args: unknown[]) => + args.includes('fetch'), + ), + ).toBe(false); + const reportCall = producerMocks.writeFileSync.mock.calls.find( + ([path]) => path === '/tmp/fetch-report.json', + ); + expect(reportCall).toBeUndefined(); + expect(clearReviewWorktreeLeaseIfOwned).toHaveBeenCalled(); + }); + + it('runs the creation checkout INERT — hooks and fsmonitor emptied at the spawn', async () => { + // The screen certifies FILTERS only; `worktree add` still fires + // `post-checkout` from the shared common hooks dir and runs a repo-local + // `core.fsmonitor` (measured live, both surfaces, on this exact shape). + // This checkout is the pipeline's FIRST — it must carry the same + // overrides the probe-tree spawns carry. + await reportFor({}); + const addCall = producerMocks.git.mock.calls.find( + (args: unknown[]) => args.includes('worktree') && args.includes('add'), + ); + expect(addCall).toBeDefined(); + expect(addCall!.slice(0, 4)).toEqual([ + '-c', + 'core.hooksPath=/dev/null/no-hooks', + '-c', + 'core.fsmonitor=', + ]); + }); + + it('runs the head fetch INERT and never recurses into submodules', async () => { + // The fetch EXECUTES what the creation checkout executes: a probe-planted + // `submodule.recurse = true` beside a transport key in an ABSORBED + // submodule config — `/modules/*/config`, never a screen + // candidate — made the certified fetch recurse and run the plant + // (measured live). INERT_GIT_ARGS + an explicit `--no-recurse-submodules` + // — the override immune to git-version precedence fights. + await reportFor({}); + const fetchCall = producerMocks.git.mock.calls.find((args: unknown[]) => + args.includes('fetch'), + ); + expect(fetchCall).toBeDefined(); + expect(fetchCall!.slice(0, INERT_GIT_ARGS.length)).toEqual([ + ...INERT_GIT_ARGS, + ]); + expect(fetchCall).toContain('--no-recurse-submodules'); + }); + + it('runs the base branch fetch INERT and never recurses into submodules', async () => { + // Same spawn class as the head fetch: resolveMergeBase's fetch closure + // is a bare `git fetch` in the user's clone, seconds to minutes after + // the head screen, in a tree a detached writer can swap between the + // two. Drive the closure through the mocked resolveMergeBase and read + // the recorded argv. + producerMocks.resolveMergeBase.mockImplementation( + (_remote: unknown, _base: unknown, _head: unknown, probe: unknown) => { + (probe as { fetch: (r: string, ref: string) => boolean }).fetch( + 'origin', + 'main', + ); + return { sha: null, baseFetchFailed: true }; + }, + ); + await reportFor({}); + const baseFetchCall = producerMocks.gitExit.mock.calls.find( + (args: unknown[]) => args.includes('fetch'), + ); + expect(baseFetchCall).toBeDefined(); + expect(baseFetchCall!.slice(0, INERT_GIT_ARGS.length)).toEqual([ + ...INERT_GIT_ARGS, + ]); + expect(baseFetchCall).toContain('--no-recurse-submodules'); + }); + + it('refuses the base branch fetch when the screen refuses — before resolveMergeBase', async () => { + // The base fetch runs later than the head screen and EXECUTES the same + // command-valued repo-local keys (measured live: a planted + // core.sshCommand ran during a pipeline-shaped fetch); a config swapped + // between the two must not certify it. The refusal is a hard stop, the + // way steps 2 and 4 refuse — merge-base never resolves. + producerMocks.localFilterRefusal.mockImplementation( + (_cwd: unknown, checkout: unknown) => + checkout === 'the base branch fetch' + ? "the repository's local config names command-execution key(s) " + + 'core.sshcommand (in /repo/.git/config) — a checkout that ' + + 'lazy-fetches EXECUTES the commands they name' + : null, + ); + + await expect(reportFor({})).rejects.toThrow(/command-execution key/); + expect(producerMocks.localFilterRefusal).toHaveBeenCalledWith( + process.cwd(), + 'the base branch fetch', + ); + expect(producerMocks.resolveMergeBase).not.toHaveBeenCalled(); + const reportCall = producerMocks.writeFileSync.mock.calls.find( + ([path]) => path === '/tmp/fetch-report.json', + ); + expect(reportCall).toBeUndefined(); + expect(clearReviewWorktreeLeaseIfOwned).toHaveBeenCalled(); + }); + it('refuses the refspec channel on baseRefName too (+ and colon)', async () => { // `--` ends option parsing, but a leading `+` or `src:dst` shape still // parses as a (force) refspec after it — same channels as @@ -670,9 +852,10 @@ describe('fetch-pr report assembly', () => { }), ); // The fetch itself exits 0 (tag shape), but no `origin/v1.0` tracking - // ref exists afterwards. + // ref exists afterwards. Match the SUBCOMMAND: the base fetch carries + // INERT_GIT_ARGS ahead of it. producerMocks.gitOpt.mockImplementation((...args: string[]) => - args[0] === 'fetch' ? '' : null, + args.includes('fetch') ? '' : null, ); producerMocks.refExists.mockReturnValue(false); // Drive the seam the way the real resolveMergeBase does: the probe the @@ -705,8 +888,10 @@ describe('fetch-pr report assembly', () => { body: '', }), ); + // Match the SUBCOMMAND: the base fetch carries INERT_GIT_ARGS ahead + // of it. producerMocks.gitOpt.mockImplementation((...args: string[]) => - args[0] === 'fetch' ? '' : null, + args.includes('fetch') ? '' : null, ); const checked: string[] = []; producerMocks.refExists.mockImplementation((...refs: unknown[]) => { @@ -742,8 +927,11 @@ describe('fetch-pr report assembly', () => { ); const fetched: string[][] = []; producerMocks.gitOpt.mockImplementation((...args: string[]) => { - if (args[0] === 'fetch') fetched.push(args.slice(1)); - return args[0] === 'fetch' ? '' : null; + // Match the SUBCOMMAND: the base fetch carries INERT_GIT_ARGS ahead + // of it. Record what follows it. + const i = args.indexOf('fetch'); + if (i !== -1) fetched.push(args.slice(i + 1)); + return i !== -1 ? '' : null; }); producerMocks.refExists.mockImplementation((...refs: unknown[]) => { void refs; @@ -756,7 +944,12 @@ describe('fetch-pr report assembly', () => { }); await reportFor({}); expect(fetched).toEqual([ - ['origin', '--', '+refs/heads/v1.0:refs/remotes/origin/v1.0'], + [ + '--no-recurse-submodules', + 'origin', + '--', + '+refs/heads/v1.0:refs/remotes/origin/v1.0', + ], ]); }); @@ -955,6 +1148,18 @@ describe('fetch-pr report assembly', () => { expect(leaseOrder).toBeLessThan( producerMocks.execFileSync.mock.invocationCallOrder[0]!, ); + // The stale-clean's `branch -D` opens the repo config: a FIFO planted + // there holds an unbounded delete in open(), so the spawn carries the + // screen's bound — and it fires the reference-transaction hook from + // the never-wiped common hooks dir, so it carries INERT_GIT_ARGS too. + expect(producerMocks.execFileSync).toHaveBeenCalledWith( + 'git', + [...INERT_GIT_ARGS, 'branch', '-D', 'qwen-review/pr-42'], + expect.objectContaining({ + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', + }), + ); }); }); @@ -1013,10 +1218,18 @@ describe('fetch-pr report assembly', () => { ); expect(producerMocks.execFileSync).toHaveBeenCalledWith( 'git', - ['branch', '-D', 'qwen-review/pr-42'], + [...INERT_GIT_ARGS, 'branch', '-D', 'qwen-review/pr-42'], // Sanitized env: a delete must land in the repository the caller - // named, not the one an exported `GIT_DIR` points at. - expect.objectContaining({ stdio: 'pipe', env: expect.any(Object) }), + // named, not the one an exported `GIT_DIR` points at. Bounded: + // `git branch` opens the repo config, and a FIFO planted there + // holds an unbounded delete in open(). INERT_GIT_ARGS: the delete + // fires the reference-transaction hook from the common hooks dir. + expect.objectContaining({ + stdio: 'pipe', + env: expect.any(Object), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', + }), ); expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( process.cwd(), @@ -1035,15 +1248,112 @@ describe('fetch-pr report assembly', () => { ); }); + it('refuses when the fetched SHA is not the head the platform advertised', async () => { + // A redirected head fetch lands content the platform never named: a + // swapped `remote..url` is outside the screen's candidate set, + // and an attacker mirror serves its own commit under the PR's + // refspec. SHAs are content-addressed, so comparing the fetched ref + // against the platform's head catches ANY redirect mechanism — the + // refusal precedes the worktree and every later step, and the ref is + // rolled back the way a metadata failure rolls it back. + producerMocks.git.mockImplementation((...args: string[]) => + args[0] === 'rev-parse' ? 'deadbeefdead' : '', + ); + + await expect(reportFor({})).rejects.toThrow( + /the fetched content is not the PR under review/, + ); + // INERT_GIT_ARGS: `branch -D` fires the reference-transaction hook + // from the never-wiped common hooks dir, so a correctly REFUSED run + // must not execute a planted hook on its way out. Bounded like every + // rollback: the spawn opens the repo config. + expect(producerMocks.execFileSync).toHaveBeenCalledWith( + 'git', + [...INERT_GIT_ARGS, 'branch', '-D', 'qwen-review/pr-42'], + expect.objectContaining({ + stdio: 'pipe', + env: expect.any(Object), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', + }), + ); + // No worktree, no report; the outer catch releases the lease. + expect( + producerMocks.git.mock.calls.some( + (args: unknown[]) => + args.includes('worktree') && args.includes('add'), + ), + ).toBe(false); + const reportCall = producerMocks.writeFileSync.mock.calls.find( + ([path]) => path === '/tmp/fetch-report.json', + ); + expect(reportCall).toBeUndefined(); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalled(); + }); + + it('accepts the platform head whatever case the object ID arrives in', async () => { + // Object IDs compare case-insensitively: a platform that advertises + // the head in capitals is the SAME commit git prints in lowercase, + // and refusing it would refuse a legitimate fetch. + producerMocks.git.mockImplementation((...args: string[]) => + args[0] === 'rev-parse' ? 'F00DF00DF00D' : '', + ); + + const report = await reportFor({}); + + expect(report.fetchedSha).toBe('F00DF00DF00D'); + }); + + it('proceeds (with a disclosed warning) when the advertised head is not an object ID', async () => { + // Aone fills headRefOid from `sourceBranch`, which is a branch NAME on + // non-AGit-Flow MRs. Comparing a name against the fetched SHA would + // refuse every such MR as a redirected fetch — the comparison runs + // only when the advertised head has an object ID's shape, and the + // redirect defense being off is disclosed on stderr. + producerMocks.gh.mockReturnValue( + JSON.stringify({ + headRefName: 'feature/login', + headRefOid: 'feature/login', + baseRefName: 'main', + additions: 1, + deletions: 0, + changedFiles: 1, + isCrossRepository: false, + body: '', + }), + ); + + const report = await reportFor({}); + + expect(report.fetchedSha).toBe('f00df00df00d'); + const warned = producerMocks.writeStderrLine.mock.calls + .map((c) => String(c[0])) + .some((l) => l.includes('defense is off')); + expect(warned).toBe(true); + }); + it('clears the lease when the worktree add fails', async () => { producerMocks.git.mockImplementation((...args: string[]) => { - if (args[0] === 'worktree') throw new Error('disk full'); - return args[0] === 'rev-parse' ? 'f00df00d' : ''; + // The creation add carries the inert `-c` overrides ahead of the + // subcommand, so match the pair anywhere in the argv. + if (args.includes('worktree')) throw new Error('disk full'); + return args[0] === 'rev-parse' ? 'f00df00df00d' : ''; }); await expect(reportFor({})).rejects.toThrow( 'Failed to create worktree at', ); + // The rollback the failure triggers is bounded like its siblings and + // inert like the SHA-mismatch one: `branch -D` fires the + // reference-transaction hook from the common hooks dir. + expect(producerMocks.execFileSync).toHaveBeenCalledWith( + 'git', + [...INERT_GIT_ARGS, 'branch', '-D', 'qwen-review/pr-42'], + expect.objectContaining({ + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', + }), + ); expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( process.cwd(), 'pr-42', @@ -1802,12 +2112,15 @@ describe('fetch-pr report assembly', () => { out: string | null; status: number | null; }) => { + // Match SUBCOMMANDS, not argv[0] — the base fetch carries + // INERT_GIT_ARGS ahead of it, the way the merge-base probe carries + // its `-c` pin. producerMocks.gitOpt.mockImplementation((...args: string[]) => - args[0] === 'fetch' || - args[0] === 'cat-file' || - args[0] === 'merge-base' + args.includes('fetch') || + args.includes('cat-file') || + args.includes('merge-base') ? '' - : args[0] === 'rev-parse' + : args.includes('rev-parse') ? ANCHOR : null, ); @@ -2750,11 +3063,15 @@ describe('fetch-pr report assembly', () => { // incremental path narrows. expect(writtenDiff()).toBe(NARROWED); // ...and the diff came from the Aone ref namespace, not GitHub's. - expect(producerMocks.git.mock.calls).toContainEqual([ - 'fetch', - 'origin', - 'refs/merge-requests/42/head:qwen-review/pr-42', - ]); + // The head fetch carries INERT_GIT_ARGS ahead of the subcommand, so + // match the refspec it fetched, not the exact argv. + const headFetchCall = producerMocks.git.mock.calls.find( + (args: unknown[]) => + args.includes('refs/merge-requests/42/head:qwen-review/pr-42'), + ); + expect(headFetchCall).toBeDefined(); + expect(headFetchCall).toContain('fetch'); + expect(headFetchCall).toContain('origin'); }); it('never asks an ancestry question on the Aone platform', async () => { @@ -3861,6 +4178,12 @@ describe('fetch-pr --resume', () => { }); it('falls through to a fresh fetch when the head moved, and says so', async () => { + // The fresh fetch lands the NEW head the platform now advertises, so + // the fetchedSha read answers it (a stale answer is the very shape the + // head-cross-check refuses). + producerMocks.git.mockImplementation((...args: string[]) => + args[0] === 'rev-parse' ? 'aaaa1111bbbb' : '', + ); producerMocks.gh.mockImplementation((...args: string[]) => { if (args.includes('headRefOid') && !args.includes('headRefName')) { return JSON.stringify({ headRefOid: 'aaaa1111bbbb' }); @@ -4030,6 +4353,12 @@ describe('fetch-pr --resume', () => { .mocked(gitOpt) .mock.calls.find((c) => c.includes('status') && c.includes('-C')); expect(statusCall).toContain('--untracked-files=normal'); + // INERT_GIT_ARGS, not the fsmonitor pin alone: `status` refreshes the + // index, which fires `post-index-change` from the never-wiped common + // hooks dir, beside running a planted `core.fsmonitor` (both measured + // live) — and this probe runs AHEAD of the first screen. + expect(statusCall).toContain('core.fsmonitor='); + expect(statusCall).toContain('core.hooksPath=/dev/null/no-hooks'); }); it('refuses on an explicit effort different from the recorded run', async () => { diff --git a/packages/cli/src/commands/review/fetch-pr.ts b/packages/cli/src/commands/review/fetch-pr.ts index 799b6662c97..b9da00a5832 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -38,7 +38,12 @@ import { reviewLeaseHeldByAnotherSession, reviewLeasePath, } from '../../services/review-worktree-lease.js'; -import { sanitizedGitEnv } from './lib/worktree.js'; +import { + INERT_GIT_ARGS, + localFilterRefusal, + sanitizedGitEnv, + SCREEN_SPAWN_TIMEOUT_MS, +} from './lib/worktree.js'; import { setGhHost } from './lib/gh.js'; import { getPlatformReader } from './lib/platform/registry.js'; import type { ReviewPlatformReader } from './lib/platform/types.js'; @@ -557,7 +562,9 @@ const gitProbe: GitProbe = { // arm below and SKILL.md's once-cap — never on the status. fetch: (remote, ref) => gitExit( + ...INERT_GIT_ARGS, 'fetch', + '--no-recurse-submodules', remote, '--', `+refs/heads/${ref}:refs/remotes/${remote}/${ref}`, @@ -615,12 +622,18 @@ function cleanStale(prNumber: string): void { const ref = reviewBranch(prNumber); if (refExists(ref)) { tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { + execFileSync('git', [...INERT_GIT_ARGS, 'branch', '-D', ref], { stdio: 'pipe', // Same reason as every other git spawn in this pipeline: a delete must // land in the repository the caller named, not the one the shell's - // `GIT_DIR` points at. + // `GIT_DIR` points at. INERT_GIT_ARGS beside the timeout: `branch -D` + // fires the reference-transaction hook from the never-wiped common + // hooks dir — the plantable surface every other spawn in this diff + // neutralizes. Bounded because `git branch` opens the repo config and + // a FIFO planted there holds an unbounded delete in open(). env: sanitizedGitEnv(), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', }), ); } @@ -690,6 +703,11 @@ function tryResume( const status = gitOpt( '-C', wt, + // INERT_GIT_ARGS, not the fsmonitor pin alone: `status` refreshes the + // index, which fires `post-index-change` from the never-wiped common + // hooks dir, beside running a planted `core.fsmonitor` (both measured + // live) — and this probe runs AHEAD of the first screen. + ...INERT_GIT_ARGS, 'status', '--porcelain', '--untracked-files=normal', @@ -901,10 +919,32 @@ async function runFetchPr(args: FetchPrArgs): Promise { // 2. Fetch PR HEAD into a unique local ref. The refspec source is // platform-specific: GitHub `pull//head`, Aone - // `refs/merge-requests//head`. + // `refs/merge-requests//head`. Screen BEFORE the fetch: + // this is the pipeline's first network operation, and it EXECUTES the + // command-valued config keys the screen names (`core.sshCommand`, + // `credential.*.helper`, an `ext::` remote under + // `protocol.ext.allow`) — a key an earlier malicious PR's probe + // planted in the never-wiped common dir runs during THIS fetch + // (measured live). The screen reads only repo-local config and needs + // nothing the fetch produces, so running it first loses nothing; a + // hit throws through the outer catch, which releases the lease. + const headFetchRefusal = localFilterRefusal( + process.cwd(), + "the review worktree's head fetch", + ); + if (headFetchRefusal) throw new Error(headFetchRefusal); + // INERT_GIT_ARGS + an explicit `--no-recurse-submodules` for the same + // reason the creation checkout carries them: `submodule.recurse = true` + // beside a transport key planted in an ABSORBED submodule config — + // `/modules/*/config`, a probe write the screen's candidate set + // never reads — makes this fetch recurse and EXECUTE the plant, and the + // absorbed config persists in the never-wiped common dir (measured + // live: the certified fetch ran the planted command). try { git( + ...INERT_GIT_ARGS, 'fetch', + '--no-recurse-submodules', remote, `${platform.fetchHeadRefSpec(Number(prNumber))}:${ref}`, ); @@ -954,34 +994,112 @@ async function runFetchPr(args: FetchPrArgs): Promise { } catch (err) { // Roll back the fetched ref so the next run starts clean. tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { + execFileSync('git', [...INERT_GIT_ARGS, 'branch', '-D', ref], { stdio: 'pipe', // Same reason as every other git spawn in this pipeline: a delete must // land in the repository the caller named, not the one the shell's - // `GIT_DIR` points at. + // `GIT_DIR` points at. INERT_GIT_ARGS beside the timeout: `branch -D` + // fires the reference-transaction hook from the never-wiped common + // hooks dir — the plantable surface every other spawn in this diff + // neutralizes. Bounded because `git branch` opens the repo config and + // a FIFO planted there holds an unbounded delete in open(). env: sanitizedGitEnv(), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', }), ); throw new Error( `Failed to fetch PR #${prNumber} metadata: ${(err as Error).message}`, ); } + // The fetched ref must BE the head the platform advertised. This is the + // only comparison that catches a head fetch redirected away from the PR — + // a `remote..url` swap is outside the screen's candidate set, and + // an attacker mirror serves its own commit under the PR's refspec: the + // review would then run, and post findings, over content the platform + // never named. SHAs are content-addressed, so a mismatch proves the + // fetch landed something else whatever the redirect's mechanism; the + // comparison folds case because object IDs are case-insensitive. The + // ref is rolled back the way a metadata failure rolls it back — plus + // the inert overrides the new rollback needs (see its comment) — and + // the outer catch releases the lease. The comparison runs only when + // the advertised head has the shape of an object ID: a platform can + // advertise a branch name there (Aone fills it from `sourceBranch` on + // non-AGit-Flow MRs), and comparing a name against the fetched SHA + // would refuse every such MR as a redirected fetch. A non-OID head + // leaves the redirect defense off — disclose that on stderr. + if (meta.headRefOid && !SHA_RE.test(meta.headRefOid.toLowerCase())) { + writeStderrLine( + `WARNING: the platform advertised ${JSON.stringify(meta.headRefOid)} ` + + `as the PR head, which is not an object ID — the fetched head ` + + `could not be cross-checked against it, so the redirected-fetch ` + + `defense is off for this run.`, + ); + } + if ( + meta.headRefOid && + SHA_RE.test(meta.headRefOid.toLowerCase()) && + meta.headRefOid.toLowerCase() !== fetchedSha.toLowerCase() + ) { + tryRemove(() => + // INERT_GIT_ARGS beside the timeout: `branch -D` fires the + // reference-transaction hook from the never-wiped common hooks dir — + // the plantable surface every other spawn in this diff neutralizes — + // and it opens the repo config, which a FIFO holds in open() past any + // bound (the screen-to-spawn window). A timed-out rollback stays + // best-effort: tryRemove swallows the kill, the real error throws. + execFileSync('git', [...INERT_GIT_ARGS, 'branch', '-D', ref], { + stdio: 'pipe', + env: sanitizedGitEnv(), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', + }), + ); + throw new Error( + `refusing to review PR #${prNumber}: the fetch landed ` + + `${fetchedSha}, but the platform's head is ${meta.headRefOid} — ` + + `the fetched content is not the PR under review (a repo-local ` + + `config rewriting the fetch's remote URL is one cause), so no ` + + `worktree is created and no review runs`, + ); + } // Aone does not advertise diff stats; compute them from the captured diff // once it exists. Tracked so the recomputed numbers replace the zeros. const needsLocalStats = platform.kind !== 'github'; - // 4. Create the ephemeral worktree. + // 4. Create the ephemeral worktree. Its creation checkout rewrites every + // file the fetched head carries — which EXECUTES a planted content + // filter — and it is the pipeline's actual FIRST checkout: every other + // screen runs inside the tree this call creates, so none exists yet in + // this run. A screen hit refuses the creation the way step 2's screen + // refuses the head fetch; the rollback below then removes the fetched + // ref and the outer catch releases the lease. The spawn + // carries INERT_GIT_ARGS because the screen reads filters only, while + // `worktree add` ALSO fires `post-checkout` from the shared common + // hooks dir and runs a repo-local `core.fsmonitor` — both plantable + // with one write each, both measured live on this exact shape. try { + const filterRefusal = localFilterRefusal( + process.cwd(), + "the review worktree's creation checkout", + ); + if (filterRefusal) throw new Error(filterRefusal); mkdirSync(dirname(wt), { recursive: true }); - git('worktree', 'add', wt, ref); + git(...INERT_GIT_ARGS, 'worktree', 'add', wt, ref); } catch (err) { tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { + execFileSync('git', [...INERT_GIT_ARGS, 'branch', '-D', ref], { stdio: 'pipe', // Same reason as every other git spawn in this pipeline: a delete must // land in the repository the caller named, not the one the shell's - // `GIT_DIR` points at. + // `GIT_DIR` points at. INERT_GIT_ARGS beside the timeout: `branch -D` + // fires the reference-transaction hook from the never-wiped common + // hooks dir — the plantable surface every other spawn in this diff + // neutralizes. Bounded because `git branch` opens the repo config and + // a FIFO planted there holds an unbounded delete in open(). env: sanitizedGitEnv(), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', }), ); throw new Error( @@ -997,6 +1115,19 @@ async function runFetchPr(args: FetchPrArgs): Promise { // chunk agents read ranges out of it and `diffHashOf` hashes it. What // the round trip does not do is normalise CRLF (that would rewrite // every hunk of a CRLF file) or drop the trailing newline. + // Screen the base-branch fetch the way step 2 screens the head fetch: + // it runs later — the worktree-add phase sits between them — and + // EXECUTES the same command-valued repo-local keys (measured live: a + // planted core.sshCommand ran during a pipeline-shaped fetch). The + // spawn it drives carries INERT_GIT_ARGS and `--no-recurse-submodules` + // beside it — the absorbed-submodule plant the screen's candidate set + // cannot read — but a config swapped between step 2's read and this + // fetch must not certify. + const baseFetchRefusal = localFilterRefusal( + process.cwd(), + 'the base branch fetch', + ); + if (baseFetchRefusal) throw new Error(baseFetchRefusal); let mergeBaseSha: string | null; let baseFetchFailed: boolean; /** The merge-base probe threw: the surface, not the history. */ diff --git a/packages/cli/src/commands/review/lib/platform/aone.test.ts b/packages/cli/src/commands/review/lib/platform/aone.test.ts index 616aec1b255..d682105f8fd 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.test.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.test.ts @@ -13,6 +13,7 @@ const { ensureAuthMock, gitMock, gitRawMock, + localFilterRefusalMock, } = vi.hoisted(() => ({ a1JsonMock: vi.fn(), a1JsonOnceMock: vi.fn(), @@ -20,6 +21,9 @@ const { ensureAuthMock: vi.fn(), gitMock: vi.fn(), gitRawMock: vi.fn(), + // The screen is owned by lib/worktree's own suite; here it is a seam, + // defaulting to a clean repository the way fetch-pr.test.ts steers it. + localFilterRefusalMock: vi.fn((..._args: unknown[]): string | null => null), })); vi.mock('./aone-client.js', async (importOriginal) => { @@ -42,6 +46,13 @@ vi.mock('../git.js', () => ({ gitRaw: gitRawMock, })); +// Partial mock: only the filter screen is steered from the tests; the rest +// of the module (INERT_GIT_ARGS included) stays real. +vi.mock('../worktree.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, localFilterRefusal: localFilterRefusalMock }; +}); + import { AonePartialPostError, aoneAccountName, @@ -54,6 +65,7 @@ import { submitAoneReview, } from './aone.js'; import { PINNED_DIFF_CONFIG, PINNED_DIFF_FLAGS } from '../diff-flags.js'; +import { INERT_GIT_ARGS } from '../worktree.js'; describe('parseRemoteUrl hardening', () => { it('discards an explicit port instead of folding it into the path', () => { @@ -1014,6 +1026,10 @@ describe("getMrAuthorAndHead (the presubmit gate's Aone seam)", () => { describe('aoneReader.fetchDiff', () => { beforeEach(() => { vi.clearAllMocks(); + // clearAllMocks leaves implementations set by an earlier test standing; + // re-assert the default (a clean repository) the way fetch-pr.test.ts + // does for its own screen seam. + localFilterRefusalMock.mockReturnValue(null); }); it('fetches the MR ref, merge-bases, and diffs via gitRaw (byte-faithful)', () => { @@ -1046,7 +1062,9 @@ describe('aoneReader.fetchDiff', () => { // run must not fail the fetch when the head was rewritten), and the target // branch is fetched so the merge-base is current. expect(gitMock).toHaveBeenCalledWith( + ...INERT_GIT_ARGS, 'fetch', + '--no-recurse-submodules', 'origin', expect.stringMatching( /^\+refs\/merge-requests\/7\/head:__qwen-review-diff-7-\d+$/, @@ -1057,18 +1075,55 @@ describe('aoneReader.fetchDiff', () => { // silent stale-base state). The head side of every read is qualified // (refs/heads/…) for the same shadow class. expect(gitMock).toHaveBeenCalledWith( + ...INERT_GIT_ARGS, 'fetch', + '--no-recurse-submodules', 'origin', '+refs/heads/master:refs/remotes/origin/master', ); - // The throwaway ref is cleaned up. + // The throwaway ref is cleaned up — inert, because `branch -D` fires + // the reference-transaction hook from the never-wiped common hooks dir. expect(gitMock).toHaveBeenCalledWith( + ...INERT_GIT_ARGS, 'branch', '-D', expect.stringMatching(refRe), ); }); + it('screens the clone before either fetch and refuses on a hit — no fetch runs', () => { + // Both fetches are network spawns against the user's clone and EXECUTE + // the command-valued repo-local config keys the screen names — a key an + // earlier review's probe planted in the never-wiped common dir runs + // during them. The screen runs before EITHER fetch, the way fetch-pr + // screens ahead of its head/base fetches; a hit refuses before any + // fetch spawn. + a1JsonMock.mockReturnValue({ + mergeRequest: { sourceBranch: 'sha', targetBranch: 'master' }, + }); + gitMock.mockImplementation((...args: string[]) => { + if (args[0] === 'remote') return 'git@gitlab.alibaba-inc.com:g/p.git'; + return ''; + }); + localFilterRefusalMock.mockReturnValue( + "the repository's local config names command-execution key(s) " + + 'core.sshcommand (in /repo/.git/config) — a checkout that ' + + 'lazy-fetches EXECUTES the commands they name', + ); + + expect(() => aoneReader.fetchDiff(7, 'g/p')).toThrow( + /command-execution key/, + ); + expect(localFilterRefusalMock).toHaveBeenCalledWith( + process.cwd(), + 'the Aone MR diff fetches', + ); + expect( + gitMock.mock.calls.some((args: unknown[]) => args.includes('fetch')), + ).toBe(false); + expect(gitRawMock).not.toHaveBeenCalled(); + }); + it('refuses a dash-leading target branch from the MR metadata', () => { a1JsonMock.mockReturnValue({ mergeRequest: { @@ -1326,7 +1381,9 @@ describe('aoneReader.fetchDiff', () => { // The target fetch never dwims onto a same-named tag: explicit branch // refspec, both sides fully qualified. expect(gitMock).toHaveBeenCalledWith( + ...INERT_GIT_ARGS, 'fetch', + '--no-recurse-submodules', 'origin', '+refs/heads/master:refs/remotes/origin/master', ); diff --git a/packages/cli/src/commands/review/lib/platform/aone.ts b/packages/cli/src/commands/review/lib/platform/aone.ts index 27ad8126e4f..b22a1d2385c 100644 --- a/packages/cli/src/commands/review/lib/platform/aone.ts +++ b/packages/cli/src/commands/review/lib/platform/aone.ts @@ -11,6 +11,7 @@ // docs/design/2026-08-15-review-aone-provider.md. import { git, gitRaw } from '../git.js'; +import { INERT_GIT_ARGS, localFilterRefusal } from '../worktree.js'; import { isOwnerRepo } from '../gh.js'; import { PINNED_DIFF_CONFIG, PINNED_DIFF_FLAGS } from '../diff-flags.js'; import { isAoneHostFamily } from '../remote-match.js'; @@ -574,11 +575,30 @@ export const aoneReader: ReviewPlatformReader = { // local branch of the reserved name is force-moved by the `+` fetch, // then deleted, reflog and all (fsck dangling-commit recovery only). const ref = `__qwen-review-diff-${prNumber}-${process.pid}`; + // Screen BEFORE the fetches, the way fetch-pr screens its head and base + // fetches: these are network spawns against the user's clone and EXECUTE + // the command-valued repo-local config keys the screen names (a key an + // earlier review's probe planted in the never-wiped common dir runs + // during them). The spawns carry INERT_GIT_ARGS and an explicit + // `--no-recurse-submodules` for the absorbed-submodule plant the + // screen's candidate set cannot read — the same shape fetch-pr's + // fetches close. + const fetchRefusal = localFilterRefusal( + process.cwd(), + 'the Aone MR diff fetches', + ); + if (fetchRefusal) throw new Error(fetchRefusal); try { // Force-fetch (`+`): a stale throwaway ref left by an interrupted // earlier run would otherwise make this fetch fail whenever the MR head // was rewritten — the normal AGit-Flow iteration shape. - git('fetch', 'origin', `+${mrHeadRefSpec(prNumber)}:${ref}`); + git( + ...INERT_GIT_ARGS, + 'fetch', + '--no-recurse-submodules', + 'origin', + `+${mrHeadRefSpec(prNumber)}:${ref}`, + ); // Fetch the target branch so the merge-base is current. If the fetch // fails (transient network, expired credential), DISCLOSE it: merge-base // then resolves against a possibly-stale local ref, and the diff may @@ -590,7 +610,9 @@ export const aoneReader: ReviewPlatformReader = { // succeeded"). try { git( + ...INERT_GIT_ARGS, 'fetch', + '--no-recurse-submodules', 'origin', `+refs/heads/${target}:refs/remotes/origin/${target}`, ); @@ -649,7 +671,11 @@ export const aoneReader: ReviewPlatformReader = { ).toString('latin1'); } finally { try { - git('branch', '-D', ref); + // INERT_GIT_ARGS: `branch -D` fires the reference-transaction hook + // from the never-wiped common hooks dir — the same hazard the + // fetch-pr rollbacks neutralize — and this delete runs on every + // fetchDiff exit, the certified success path included. + git(...INERT_GIT_ARGS, 'branch', '-D', ref); } catch { // The ref may not exist if the fetch failed; nothing to clean. } diff --git a/packages/cli/src/commands/review/lib/test-utils.test.ts b/packages/cli/src/commands/review/lib/test-utils.test.ts new file mode 100644 index 00000000000..15bcf90583f --- /dev/null +++ b/packages/cli/src/commands/review/lib/test-utils.test.ts @@ -0,0 +1,21 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { gitConfigPath } from './test-utils.js'; + +describe('gitConfigPath', () => { + // `gitConfigPath` exists for Windows: `join()` builds backslash paths + // there, and git's config lexer rejects them inside hand-written config + // text (`fatal: bad config line`, measured on git 2.47.3). On POSIX the + // separator IS the forward slash, so the transform is pinned here with + // the Windows separator named explicitly. + it('forward-slashes backslash paths — the spelling git config parses on every platform', () => { + expect(gitConfigPath('C:\\a\\_temp\\behind.config', '\\')).toBe( + 'C:/a/_temp/behind.config', + ); + }); +}); diff --git a/packages/cli/src/commands/review/lib/test-utils.ts b/packages/cli/src/commands/review/lib/test-utils.ts index 051e9d51e70..07119e9e9fd 100644 --- a/packages/cli/src/commands/review/lib/test-utils.ts +++ b/packages/cli/src/commands/review/lib/test-utils.ts @@ -11,7 +11,7 @@ import { rmSync, writeFileSync, } from 'node:fs'; -import { dirname, join } from 'node:path'; +import { dirname, join, sep } from 'node:path'; import { tmpdir } from 'node:os'; import { PARSE_ARGS_REPORT } from './paths.js'; import { DIGEST_FILE } from './stale-bundle.js'; @@ -47,6 +47,20 @@ export function isolateHostGitConfig(): { }; } +/** + * The spelling a path needs inside hand-written git CONFIG text: forward + * slashes. On Windows `join()` builds backslash paths that git's config + * lexer rejects as invalid escape sequences (`fatal: bad config line`, + * exit 128) — the plant the fixture proves becomes an unreadable-config + * refusal, and a test asserting the CERTIFIED shape hard-fails (measured + * on git 2.47.3). Forward slashes parse and resolve on every platform. + * Plants written THROUGH `git config` need no help — git escapes its own + * output. + */ +export function gitConfigPath(p: string, separator: string = sep): string { + return p.split(separator).join('/'); +} + /** * Redirect the review settings the phase gates read away from the operator's * own — the same shape as `isolateHostGitConfig`, for the same reason. diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts index 946020fc8f9..35a6fd76cea 100644 --- a/packages/cli/src/commands/review/lib/worktree.test.ts +++ b/packages/cli/src/commands/review/lib/worktree.test.ts @@ -11,7 +11,7 @@ // every review produces are gitignored. import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { execFileSync } from 'node:child_process'; +import { execFileSync, spawn } from 'node:child_process'; import { appendFileSync, chmodSync, @@ -30,10 +30,11 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, dirname, join } from 'node:path'; -import { isolateHostGitConfig } from './test-utils.js'; +import { gitConfigPath, isolateHostGitConfig } from './test-utils.js'; import { discardWorktree, exposeDependencies, + localFilterRefusal, sanitizedGitEnv, worktreeCreateFailureDetail, worktreeResidue, @@ -138,6 +139,44 @@ describe('worktreeResidue', () => { expect(got.total).toBe(2); }); + it('never executes a planted core.fsmonitor while measuring — the spawn pins hold', () => { + // A probe plants `core.fsmonitor` in the shared common dir with the + // same one-write facility as the filter plant, and `ls-files`, + // `check-ignore` and `status` all RUN it (measured live on every + // shape). Own fixture because the leg that exercises the pin is + // ignore-attribution: an untracked file hidden by a COMMITTED ignore + // rule that is not pipeline footprint (node_modules/dist are dropped + // before any attribution runs). The measurement must come back clean + // without executing the plant: every spawn carries the empty-value + // pin. + const host = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-residue-fsm-'))); + const hgit = (...args: string[]) => + execFileSync('git', args, { cwd: host, encoding: 'utf8' }).trim(); + try { + hgit('init', '-q', '-b', 'main'); + hgit('config', 'user.email', 't@t.t'); + hgit('config', 'user.name', 't'); + writeFileSync(join(host, '.gitignore'), 'coverage\n'); + writeFileSync(join(host, 'a.ts'), 'export const x = 1;\n'); + hgit('add', '-A'); + hgit('commit', '-qm', 'head'); + const wt = join(host, '.qwen', 'tmp', 'review-wt'); + mkdirSync(dirname(wt), { recursive: true }); + hgit('worktree', 'add', '--detach', '-q', wt, 'HEAD'); + const marker = join(host, 'FSMONITOR-PWNED'); + hgit('config', 'core.fsmonitor', 'touch ' + marker); + mkdirSync(join(wt, 'coverage'), { recursive: true }); + writeFileSync(join(wt, 'coverage', 'x'), 'x'); + + const got = worktreeResidue(wt, 12, hgit('rev-parse', 'HEAD')); + + expect(got.paths).toEqual([]); + expect(existsSync(marker)).toBe(false); + } finally { + rmSync(host, { recursive: true, force: true }); + } + }); + it('ignores what every review leaves behind', () => { // Agent 7 installs and builds in this tree. If that read as residue, every // reader of every review would be told to distrust its own worktree — the @@ -1648,7 +1687,7 @@ describe('discardWorktree', () => { beforeEach(() => { gitIsolation = isolateHostGitConfig(); - repo = mkdtempSync(join(tmpdir(), 'qwen-discard-')); + repo = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-discard-'))); git(repo, 'init', '-q', '-b', 'main'); git(repo, 'config', 'user.email', 't@t.t'); git(repo, 'config', 'user.name', 't'); @@ -1885,3 +1924,841 @@ describe('worktreeCreateFailureDetail', () => { ); }); }); + +describe('localFilterRefusal', () => { + // Real repo + linked worktree: the production shape fetch-pr screens + // against. The screen's whole job is what a REAL `git config --file` sees, + // so a mocked spawn could not measure it. + let repo: string; + let tree: string; + let gitIsolation: ReturnType; + + const gitRepo = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + + beforeEach(() => { + gitIsolation = isolateHostGitConfig(); + // realpathSync because macOS's tmpdir is a symlink (/var -> /private/var) + // while git reports resolved paths — the sibling real-git suites' convention, + // which the FIFO test's absolute-path assertion depends on. + repo = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-filter-screen-'))); + gitRepo('init', '-q', '-b', 'main'); + gitRepo('config', 'user.email', 't@t.t'); + gitRepo('config', 'user.name', 't'); + writeFileSync(join(repo, 'a.ts'), 'export const x = 1;\n'); + gitRepo('add', '-A'); + gitRepo('commit', '-qm', 'head'); + tree = join(repo, '.qwen', 'tmp', 'review-wt'); + mkdirSync(dirname(tree), { recursive: true }); + gitRepo('worktree', 'add', '--detach', '-q', tree, 'HEAD'); + }); + + afterEach(() => { + gitIsolation.dispose(); + rmSync(repo, { recursive: true, force: true }); + }); + + it('answers null on a clean repository', () => { + expect(localFilterRefusal(tree, 'the probe checkout')).toBeNull(); + }); + + it("refuses a filter in the MAIN worktree's per-worktree config — /config.worktree", () => { + // With `extensions.worktreeConfig` on, a checkout run in ANY worktree + // of the repository also reads the main worktree's per-worktree config + // (measured live: the planted smudge EXECUTED on a checkout in a + // linked tree). Screened from the linked worktree the way the pipeline + // screens the probe/scratch/base trees, the candidates used to name + // only the common config, the screened tree's own per-worktree config + // and the sibling admin entries — never this file. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'extensions.worktreeConfig', 'true'); + writeFileSync( + join(repo, '.git', 'config.worktree'), + `[filter "evil"]\n\tsmudge = touch ${gitConfigPath(join(repo, 'PWNED'))}\n`, + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('filter.evil.smudge'); + expect(r).toContain(join(repo, '.git', 'config.worktree')); + expect(r).toContain('the probe checkout'); + }); + + it.skipIf(process.platform === 'win32')( + 'fails CLOSED on a non-regular candidate file — the FIFO shape', + () => { + // The weaponized shape is a FIFO: it passes existsSync and + // accessSync(R_OK), and the `--file` spawn carries no timeout a writer + // would answer, so one `mkfifo`+rename into the never-wiped common dir + // blocks every later review before its first checkout. The gate that + // closes it refuses EVERY non-regular file; a symlink to /dev/null + // exercises that same gate without hanging this test the way a real + // FIFO hangs the mutant. Planted in a fake worktree admin entry, the + // way a probe plants it: the screen readdirs every registered + // worktree's admin dir (git honours those files only with + // extensions.worktreeConfig on; the screen reads them always). + const admin = join(repo, '.git', 'worktrees', 'planted'); + mkdirSync(admin, { recursive: true }); + symlinkSync('/dev/null', join(admin, 'config.worktree')); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('not a regular file'); + expect(r).toContain(join(admin, 'config.worktree')); + expect(r).toContain('the probe checkout'); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'caps the unreadable list — a planted entry per fake worktree cannot bury the refusal', + () => { + // The unreadable list is assembled from the `/worktrees` + // readdir, which is unbounded, and test-efficacy re-embeds the refusal + // verbatim in every probe result: an unreadable entry per planted fake + // worktree would bury the actionable part under megabytes of message. + // The join is capped like the key lists. Symlinks exercise the + // not-a-regular-file shape without hanging the test the way FIFOs + // would. + for (let i = 0; i < 10; i++) { + const admin = join(repo, '.git', 'worktrees', `planted-${i}`); + mkdirSync(admin, { recursive: true }); + symlinkSync('/dev/null', join(admin, 'config.worktree')); + } + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('… and 2 more unreadable files'); + // Only the first 8 entries are named, whatever the readdir order. + expect((r ?? '').match(/planted-/g) ?? []).toHaveLength(8); + }, + ); + + it('refuses an include directive — the screen cannot see what it expands', () => { + // `--file` does not expand `include.path`/`includeIf.*.path` while the + // checkout's merged read DOES: a filter planted behind the include + // below is invisible to the screen and EXECUTES in the certified + // checkout (measured live). Until the origin-scoped follow-up lands, + // any include directive in the candidates refuses fail-closed. + const behind = join(repo, 'behind-include.config'); + writeFileSync( + behind, + `[filter "evil"]\n\tsmudge = touch ${gitConfigPath(join(repo, 'PWNED'))}\n`, + ); + appendFileSync( + join(repo, '.git', 'config'), + `[include]\n\tpath = ${gitConfigPath(behind)}\n`, + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('include directive'); + expect(r).toContain('include.path'); + expect(r).toContain('the probe checkout'); + }); + + it('refuses an includeIf directive too — the other spelling of the same hole', () => { + // The regex's `includeif\..+\.path` alternative had no witness: dropping + // it left the suite green while a `[includeIf "gitdir:…"]` plant passed + // the screen and EXECUTED behind the checkout's merged read. The screen + // reads the directive's key whatever its condition, so the plant shape is + // the include test's with the conditional spelling. + const behind = join(repo, 'behind-includeif.config'); + writeFileSync( + behind, + `[filter "evil"]\n\tsmudge = touch ${gitConfigPath(join(repo, 'PWNED'))}\n`, + ); + appendFileSync( + join(repo, '.git', 'config'), + `[includeIf "gitdir:${gitConfigPath(repo)}/"]\n\tpath = ${gitConfigPath(behind)}\n`, + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('include directive'); + expect(r).toContain('includeif.'); + }); + + it('names a key whose subsection carries whitespace — never a truncated fragment', () => { + // `--get-regexp` prints `key value` per line, and taking the first + // whitespace token truncated such a key to a fragment that exists nowhere + // verbatim and deduped distinct directives together; `--name-only` prints + // one bare key per line, so the whole line is the key. + appendFileSync( + join(repo, '.git', 'config'), + `[filter "my lfs"]\n\tsmudge = touch ${gitConfigPath(join(repo, 'PWNED'))}\n`, + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('filter.my lfs.smudge'); + }); + + it('flattens control characters a planted key carries — the catch is inert', () => { + // A caught attacker still controls the bytes naming the catch: a config + // subsection may hold any byte but NUL and newline, so an ESC sequence + // rides `--get-regexp` intact. `inertPath` must flatten it before the + // refusal reaches the terminal and the report. + appendFileSync( + join(repo, '.git', 'config'), + `[filter "evil\u001b[31m"]\n\tsmudge = touch ${gitConfigPath(join(repo, 'PWNED'))}\n`, + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).not.toContain('\u001b'); + expect(r).toContain('filter.evil [31m.smudge'); + }); + + it('caps the enumeration a padded config hands it — the refusal stays actable', () => { + // test-efficacy re-embeds the full refusal in every probe's detail, so an + // unbounded key list multiplies into megabytes of report; the message + // names the first few pairs and counts the rest. + const padding = Array.from( + { length: 12 }, + (_, i) => `[filter "pad${i}"]\n\tsmudge = x`, + ).join('\n'); + appendFileSync(join(repo, '.git', 'config'), `${padding}\n`); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('filter.pad0.smudge'); + expect(r).toContain('\u2026 and 4 more filter keys'); + expect(r).not.toContain('filter.pad8.smudge'); + }); + + it.skipIf(process.platform === 'win32')( + 'fails CLOSED when the repository path contains a newline — never certifies a misparse', + () => { + // `rev-parse` prints the two paths as two stdout lines; a newline in a + // directory component splits one answer across two lines, every + // candidate misresolves, and the old parse certified a checkout it had + // read nothing of. The trigger is the user's own filesystem layout — + // newlines are legal in directory names on POSIX, only Windows forbids + // them, hence the skip — and the screen's contract is never to fail + // open. Separate fixture: the shared one cannot carry the newline. + const base = mkdtempSync(join(tmpdir(), 'qwen-filter-nl-')); + const nlRepo = join(base, 'x\ny'); + mkdirSync(nlRepo, { recursive: true }); + const g = (...args: string[]) => + execFileSync('git', args, { cwd: nlRepo, encoding: 'utf8' }).trim(); + g('init', '-q', '-b', 'main'); + g('config', 'user.email', 't@t.t'); + g('config', 'user.name', 't'); + writeFileSync(join(nlRepo, 'a.ts'), 'export const x = 1;\n'); + g('add', '-A'); + g('commit', '-qm', 'head'); + g('config', 'filter.evil.smudge', `touch ${join(base, 'PWNED')}`); + const nlTree = join(nlRepo, 'wt'); + g('worktree', 'add', '--detach', '-q', nlTree, 'HEAD'); + try { + const r = localFilterRefusal(nlTree, 'the probe checkout'); + expect(r).not.toBeNull(); + expect(r).toContain('could not be parsed'); + expect(r).toContain('the probe checkout'); + } finally { + rmSync(base, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'fails CLOSED when the worktrees admin dir cannot be listed — EACCES is not ENOENT', + () => { + // A `chmod 0100` on the admin directory drops every sibling + // config.worktree candidate — readdir EACCES — while lookup by exact + // path still works: the certified checkout then reads a config the + // screen never saw (measured live: the reset EXECUTED the plant + // through the x-only dir). Only ENOENT means "no linked worktrees". + // Root bypasses mode bits, hence the skip. + const admin = join(repo, '.git', 'worktrees'); + const sibling = join(admin, 'planted'); + mkdirSync(sibling, { recursive: true }); + writeFileSync( + join(sibling, 'config.worktree'), + `[filter "evil"]\n\tsmudge = touch ${gitConfigPath(join(repo, 'PWNED'))}\n`, + ); + chmodSync(admin, 0o100); + try { + const r = localFilterRefusal(tree, 'the probe checkout'); + expect(r).not.toBeNull(); + expect(r).toContain('linked worktrees could not be enumerated'); + expect(r).toContain('the probe checkout'); + } finally { + chmodSync(admin, 0o755); + } + }, + ); + + it('refuses the transport-command keys a lazy-fetch EXECUTES — all five', () => { + // `extensions.partialClone` + a promisor remote + one deleted loose + // object makes a certified checkout lazy-fetch, and the four + // command-valued keys name what that fetch EXECUTES (measured live on + // every pipeline spawn shape). `INERT_GIT_ARGS` cannot neutralize them + // — two are list-valued or fall back when emptied — so repo-local hits + // refuse here, each key named in the refusal. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'core.repositoryformatversion', '1'); + g('config', 'extensions.partialClone', 'evil'); + g('config', 'core.sshCommand', 'evil-ssh'); + g('config', 'core.gitProxy', 'evil-proxy'); + g('config', 'credential.helper', 'evil-helper'); + g('config', 'protocol.ext.allow', 'always'); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('command-execution key(s)'); + expect(r).toContain('extensions.partialclone'); + expect(r).toContain('core.sshcommand'); + expect(r).toContain('core.gitproxy'); + expect(r).toContain('credential.helper'); + expect(r).toContain('protocol.ext.allow'); + expect(r).toContain('the probe checkout'); + // They are not misfiled as filters or includes — the remediation + // routing downstream keys on these exact part prefixes. + expect(r).not.toContain('defines content filter(s)'); + expect(r).not.toContain('names include directive(s)'); + }); + + it('refuses the fetch trigger and the keys the first cut missed — promisor, URL-scoped helper, askpass, uploadpack, bare protocol.allow', () => { + // The trigger itself was never screened: a promisor remote — + // `remote..promisor`, with or without `extensions.partialClone` + // — makes a certified checkout that hits a missing object lazy-fetch + // (measured: promisor alone suffices), at which point the whole + // transport surface is live. The URL-scoped credential helper, + // `core.askpass` (the config twin of the GIT_ASKPASS env var + // `sanitizedGitEnv` strips), `remote..uploadpack`, and the bare + // `protocol.allow` that lifts ext's default-deny join it — each + // planted alone certified the checkout before this closure (measured). + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.promisor', 'true'); + g('config', 'credential.http://127.0.0.1:9/.helper', 'evil'); + g('config', 'core.askpass', 'evil-askpass'); + g('config', 'remote.origin.uploadpack', 'evil-uploadpack'); + g('config', 'protocol.allow', 'always'); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('command-execution key(s)'); + expect(r).toContain('remote.origin.promisor'); + expect(r).toContain('credential.http://127.0.0.1:9/.helper'); + expect(r).toContain('core.askpass'); + expect(r).toContain('remote.origin.uploadpack'); + expect(r).toContain('protocol.allow'); + expect(r).not.toContain('defines content filter(s)'); + expect(r).not.toContain('names include directive(s)'); + }); + + it('certifies a repository whose only remote..fetch is the clone-default refspec', () => { + // The clone-default `+refs/heads/*:refs/remotes/origin/*` lives under + // this key in EVERY clone's local config and maps only into + // refs/remotes/, so the key itself can never refuse — refusing it would + // put every repository into permanent refusal. Only a destination + // outside refs/remotes/ fails the screen (the next test). + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + + expect(localFilterRefusal(tree, 'the probe checkout')).toBeNull(); + }); + + it('refuses a colon-less literal source — the ref is remote state the screen cannot read', () => { + // Used to certify because FETCH_HEAD alone rewrites no ref. The + // judgment now fails CLOSED on literal (non-wildcard) sources: when + // the named ref is absent from the remote, every later bare + // fetch-by-remote-name dies `fatal: couldn't find remote ref` — + // measured live on git 2.39.5, exit 128, a permanent wedge planted by + // one config write. That is remote state the screen cannot read, so + // it fails the way the GRAMMAR class does; literal `HEAD` stays + // excepted because every remote resolves it. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.origin.fetch', '+refs/heads/main'); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('fetch refspec'); + expect(r).toContain('remote.origin.fetch'); + }); + + it('certifies an EMPTY fetch refspec value — git ignores it at fetch', () => { + // `git config remote.origin.fetch ''` is legal and git skips the empty + // value when it fetches (measured live), so refusing it would refuse a + // shape a user's own repository can legitimately carry. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.origin.fetch', ''); + + expect(localFilterRefusal(tree, 'the probe checkout')).toBeNull(); + }); + + it('refuses a planted fetch refspec whose destination writes into refs/heads/', () => { + // A destination under refs/heads/ rewrites the user's own branches on + // any fetch that applies the configured refspec: a planted + // `+refs/heads/*:refs/heads/*` force-updated local branches and + // orphaned unpushed work, and a glob aimed at the checked-out branch + // fails every fetch forever (both measured live). Judged by VALUE, + // never by presence — the key itself is in every clone. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.origin.fetch', '+refs/heads/*:refs/heads/*'); + g('config', 'remote.evil.url', 'https://example.invalid/y'); + g('config', 'remote.evil.fetch', 'refs/heads/main:refs/heads/main'); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('fetch refspec'); + expect(r).toContain('remote.origin.fetch'); + expect(r).toContain('remote.evil.fetch'); + expect(r).toContain('the probe checkout'); + // Not misfiled into the transport part — the remediation routing + // downstream keys on these exact part prefixes. + expect(r).not.toContain('command-execution key(s)'); + expect(r).not.toContain('defines content filter(s)'); + }); + + it('certifies a PR-mirror fetch refspec — refs/pull/ is not refs/heads/', () => { + // `refs/pull/*/head:refs/pull/*` is an ordinary clone-local config for + // checking out PR heads; it writes no branch, so the screen must not + // touch it — the rule keys on refs/heads/ for exactly this reason. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.origin.fetch', '+refs/pull/*/head:refs/pull/*'); + + expect(localFilterRefusal(tree, 'the probe checkout')).toBeNull(); + }); + + it('certifies a HEAD source — every remote resolves it, so it cannot wedge', () => { + // The SOURCE class excepts literal `HEAD`: whatever the remote is, + // `HEAD` resolves on it, so the value carries no + // couldn't-find-remote-ref wedge a literal branch name carries. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.origin.fetch', '+HEAD:refs/remotes/origin/live-head'); + + expect(localFilterRefusal(tree, 'the probe checkout')).toBeNull(); + }); + + it.each([ + // The destination rows ride a `HEAD` source — exempted from the + // literal-src class because every remote resolves it — so each pins + // the DESTINATION policy alone; a wildcard pair isolates it the same + // way. The literal-source rows pin the SOURCE class alone, their + // destinations allowlisted. + ['a bare namespace destination', '+HEAD:refs/tags'], + ['a bare refs/heads destination', '+HEAD:refs/heads'], + ['a custom-namespace destination', '+HEAD:refs/pr/1'], + [ + 'a collision shape inside the allowed prefixes', + '+HEAD:refs/remotes/origin', + ], + [ + 'a literal refs/remotes//HEAD destination', + '+HEAD:refs/remotes/origin/HEAD', + ], + [ + 'a wildcard path under refs/remotes//HEAD', + '+refs/heads/*:refs/remotes/origin/HEAD/*', + ], + [ + 'a literal source — the single-branch clone shape', + '+refs/heads/main:refs/remotes/origin/main', + ], + [ + 'a literal source naming a ref the remote may not carry', + '+refs/heads/z-does-not-exist:refs/remotes/origin/z', + ], + ])( + 'refuses a planted fetch refspec with %s — default-deny destinations, wildcard sources', + (_shape, refspec) => { + // The destination policy is default-DENY now — only + // refs/remotes// and refs/pull/ certify — and a literal + // source fails closed beside GRAMMAR and DESTINATION. Each shape + // measured live certifying on the old enumeration: a bare namespace + // wrote a ref named for the namespace itself; the HEAD destination + // wrote THROUGH the origin/HEAD symref and relocated the branch it + // pointed at; the literal source wedged every bare fetch when the + // ref is absent from the remote. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.evil.url', 'https://example.invalid/y'); + g('config', 'remote.evil.fetch', refspec); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('fetch refspec'); + expect(r).toContain('remote.evil.fetch'); + }, + ); + + it('refuses core.alternateRefsCommand — every fetch runs it against the alternates', () => { + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'core.alternateRefsCommand', 'evil-alternates'); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('command-execution key(s)'); + expect(r).toContain('core.alternaterefscommand'); + expect(r).toContain('the probe checkout'); + }); + + it('fails CLOSED when rev-parse cannot read the repository — never certifies an unreadable layout', () => { + // A `.git` file whose gitdir target is gone makes `rev-parse` exit 128. + // The old branch certified that state (returned null); a spawn the + // screen cannot run reads no config, and the checkout it guards opens + // the same layout, so the answer is a refusal. + const broken = mkdtempSync(join(tmpdir(), 'qwen-filter-broken-')); + try { + writeFileSync(join(broken, '.git'), 'gitdir: /nonexistent-qwen-gitdir\n'); + const r = localFilterRefusal(broken, 'the probe checkout'); + expect(r).not.toBeNull(); + expect(r).toContain('git directory could not be read'); + expect(r).toContain('the probe checkout'); + } finally { + rmSync(broken, { recursive: true, force: true }); + } + }); + + it.skipIf(process.platform === 'win32')( + 'fails CLOSED when the common config is a FIFO — the spawn is bounded, not hung', + { timeout: 30_000 }, + () => { + // The first spawn reads `/config` itself, BEFORE the lstat + // gate ever sees a candidate: one mkfifo+rename over the common + // config blocks it in open(). The bounded spawn SIGKILLs at the + // screen timeout and the error branch refuses. The writer below + // proves the bound: an UNBOUNDED spawn would wait for its pulses, + // read the empty FIFO, and only then meet the lstat gate — a + // different refusal, arriving at the pulses, not at the bound. + const fifo = join(repo, '.git', 'config.fifo'); + execFileSync('mkfifo', [fifo]); + renameSync(fifo, join(repo, '.git', 'config')); + // The writer PULSES — git opens the config several times in one + // command, and each open blocks until a writer arrives — starting + // only AFTER the screen's 5s bound: the bounded spawn refuses before + // the first pulse, while an unbounded one would wait for the pulses, + // read empty config, and certify past the rev-parse branch. + const writer = spawn( + process.execPath, + [ + '-e', + "setTimeout(() => { const iv = setInterval(() => { try { const fd = require('fs').openSync(process.argv[1], 'r+'); require('fs').closeSync(fd); } catch {} if (Date.now() - t0 > 20000) { clearInterval(iv); process.exit(0); } }, 250); }, 6000); const t0 = Date.now();", + join(repo, '.git', 'config'), + ], + { stdio: 'ignore', detached: true }, + ); + writer.unref(); + + const started = Date.now(); + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('git directory could not be read'); + expect(r).toContain('the probe checkout'); + // The bound, not the writer: the refusal arrives before the + // pulses start, which would unblock an unbounded spawn. + expect(Date.now() - started).toBeLessThan(7_500); + }, + ); + + it('refuses a planted refspec under a remote name carrying whitespace and colons — judged on its real value', () => { + // The value read used to re-split `key value` lines at the FIRST + // space: a remote subsection legally carries whitespace and colons, + // the split landed inside the key, and the judgment ran on a + // fabricated value — certifying the very refspec it exists to refuse + // (measured live: `git fetch 'a b:c'` applied the planted refspec and + // force-updated a local branch). `--null` records end the key at the + // first newline, which a config key cannot carry. + appendFileSync( + join(repo, '.git', 'config'), + '[remote "a b:c"]\n\tfetch = +refs/heads/*:refs/heads/*\n', + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('fetch refspec'); + expect(r).toContain('remote.a b:c.fetch'); + }); + + it.each([ + ['a refs/stash destination', '+refs/heads/main:refs/stash'], + ['a refs/tags destination', '+refs/heads/main:refs/tags/v1.0'], + [ + 'an unqualified destination (resolves into refs/heads/)', + '+refs/heads/main:main', + ], + ['a wildcard destination outside remotes/pull', '+refs/*:refs/*'], + ['a case-folded refs/heads destination', '+refs/heads/*:refs/Heads/*'], + ])('refuses a planted fetch refspec with %s', (_shape, refspec) => { + // Bypass shapes of the old `startsWith('refs/heads/')` predicate, + // each measured live certifying a refspec that moved a user ref: a + // tag update, a forced branch update, the permanent fetch wedge, and + // the case-fold git resolves case-insensitively. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.evil.url', 'https://example.invalid/y'); + g('config', 'remote.evil.fetch', refspec); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('fetch refspec'); + expect(r).toContain('remote.evil.fetch'); + }); + + it.each([ + [ + 'a colon-less wildcard (git rejects: a wildcard needs a destination)', + '+refs/heads/*', + ], + ['a value that is not a ref name (whitespace)', 'tag evil'], + [ + 'a dotdot escape through the refs/remotes/ allow-prefix', + '+refs/heads/main:refs/remotes/../heads/pwned', + ], + [ + 'an invalid source beside a clean destination', + 'bad src:refs/remotes/origin/x', + ], + [ + 'mismatched wildcards (source only)', + 'refs/heads/*:refs/remotes/origin/main', + ], + [ + 'mismatched wildcards (destination only)', + 'refs/heads/main:refs/remotes/origin/*', + ], + ])( + 'refuses a planted fetch refspec git cannot fetch with — %s', + (_shape, refspec) => { + // A value git rejects with `fatal: invalid refspec` dies on every + // later fetch-by-remote-name — persistence planted by one config + // write into the never-wiped common dir, each shape measured live + // wedging before this closure. The screen fails CLOSED on the + // grammar the way it does on a destination that rewrites a user ref; + // the dotdot shape is the one the refs/remotes/ allow-prefix alone + // would certify. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', 'remote.origin.url', 'https://example.invalid/x'); + g('config', 'remote.evil.url', 'https://example.invalid/y'); + g('config', 'remote.evil.fetch', refspec); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('fetch refspec'); + expect(r).toContain('remote.evil.fetch'); + }, + ); + + it.each([ + ['core.sparsecheckout', 'core.sparseCheckout', 'true'], + ['core.attributesfile', 'core.attributesFile', '/tmp/evil-attributes'], + ['core.excludesfile', 'core.excludesFile', '/tmp/evil-excludes'], + ['core.fsmonitor', 'core.fsmonitor', 'evil-monitor'], + ['http.proxy', 'http.proxy', 'http://127.0.0.1:9/'], + ['https.proxy', 'https.proxy', 'http://127.0.0.1:9/'], + [ + 'a URL-scoped proxy', + 'http.https://example.invalid/.proxy', + 'http://127.0.0.1:9/', + ], + ['a remote-scoped proxy', 'remote.origin.proxy', 'http://127.0.0.1:9/'], + ])( + 'refuses a planted %s — the redirect and proxy families fail closed', + (_shown, key, value) => { + // One write into the never-wiped common dir arms each of these: + // sparseCheckout empties every certified creation checkout; + // attributesFile/excludesFile redirect the attribute/exclude reads + // away from the info/ files this screen covers; the proxy keys + // route every certified fetch through an attacker proxy the SHA + // cross-check cannot see. A fresh pipeline clone carries none of + // them, so the refusal breaks nothing legitimate — the include + // posture. + const g = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); + g('config', key, value); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('command-execution key(s)'); + // Git prints the key lowercased in the refusal. + expect(r).toContain(key.toLowerCase()); + expect(r).not.toContain('defines content filter(s)'); + expect(r).not.toContain('names include directive(s)'); + }, + ); + + it('refuses url-rewrite keys — an insteadOf plant redirects the guarded fetches', () => { + // `url..insteadOf` rewrote the URL the pipeline's fetches run + // against: one plant in the never-wiped common dir sent the guarded + // head fetch to an attacker-controlled repository while the screen + // certified (measured live). A fresh pipeline clone never carries + // these keys, so refusal breaks nothing legitimate — the same posture + // as include directives. pushInsteadOf is the push-side twin. + appendFileSync( + join(repo, '.git', 'config'), + '[url "/tmp/qwen-evil/"]\n\tinsteadOf = https://github.com/x/y\n' + + '\tpushInsteadOf = https://github.com/x/z\n', + ); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('command-execution key(s)'); + expect(r).toContain('url./tmp/qwen-evil/.insteadof'); + expect(r).toContain('url./tmp/qwen-evil/.pushinsteadof'); + }); + + it.skipIf(process.platform === 'win32')( + 'fails CLOSED when the git-dir path ends in whitespace — never a truncated parse', + () => { + // `stdout.trim()` silently cut the trailing whitespace off the final + // rev-parse answer: the config.worktree candidate misresolved, + // missed existsSync, and the screen certified a checkout it never + // read that file — the fail-open twin of the newline case (measured + // live: a planted uploadpack executed on the authorised fetch). + // `git init --separate-git-dir` legally creates such a repository; + // Windows forbids trailing spaces in directory names, hence the + // skip. Separate fixture: the shared one cannot carry the space. + const base = mkdtempSync(join(tmpdir(), 'qwen-filter-ws-')); + const wc = join(base, 'wc'); + mkdirSync(wc, { recursive: true }); + try { + execFileSync( + 'git', + [ + 'init', + '-q', + '-b', + 'main', + '--template=', + '--separate-git-dir', + join(base, 'sep '), + wc, + ], + { encoding: 'utf8' }, + ); + + const r = localFilterRefusal(wc, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('could not be parsed'); + expect(r).toContain('the probe checkout'); + } finally { + rmSync(base, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(process.platform === 'win32')( + 'fails CLOSED on a non-regular info/attributes or info/exclude — the checkout opens them too', + () => { + // The authorised checkout reads the common dir's info/attributes and + // info/exclude: a FIFO planted at either held it in open() while the + // screen certified — neither was a config candidate (measured live: + // the restore checkout blocked until externally killed). The + // regular-file gate the config candidates carry now covers both; a + // symlink to /dev/null exercises that gate without hanging the suite + // the way a real FIFO hangs the checkout. + const info = join(repo, '.git', 'info'); + mkdirSync(info, { recursive: true }); + // git init ships a regular info/exclude; the plant replaces it. + rmSync(join(info, 'exclude'), { force: true }); + symlinkSync('/dev/null', join(info, 'attributes')); + symlinkSync('/dev/null', join(info, 'exclude')); + + const r = localFilterRefusal(tree, 'the probe checkout'); + + expect(r).not.toBeNull(); + expect(r).toContain('not a regular file'); + expect(r).toContain(join(info, 'attributes')); + expect(r).toContain(join(info, 'exclude')); + expect(r).toContain('the probe checkout'); + }, + ); + + it('fails CLOSED when the config swaps between the screen\u2019s two reads', () => { + // Pass 1 enumerates `remote..fetch` and queues the file for the + // value read; pass 2 judges the value. A config swapped between the two + // reads used to CERTIFY: exit 1 ("no remote.*.fetch in the file right + // now") skipped judgment while the classification lists still reflected + // the OLD content, and the certified checkout executed whatever the + // swap brought in. Exit 1 without a race is impossible — pass 2 matches + // the identical key pass 1 just saw in the same file — so failing + // closed on it adds no false refusals. The swapper alternates the + // common config between a benign clone shape and a filter plant on a + // tight copy+rename loop; every screen answer must be null (a benign + // read) or a refusal, and the between-reads refusal must land within + // the loop budget. + const cfg = join(repo, '.git', 'config'); + const benignSrc = join(repo, 'benign.config'); + const plantSrc = join(repo, 'plant.config'); + writeFileSync( + benignSrc, + '[remote "origin"]\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n', + ); + writeFileSync( + plantSrc, + '[filter "evil"]\n\tsmudge = touch ' + + gitConfigPath(join(repo, 'PWNED')) + + '\n', + ); + copyFileSync(benignSrc, cfg); + const swapper = spawn( + process.execPath, + [ + '-e', + "const fs=require('fs');const [b,p,c,t1,t2]=process.argv.slice(1);const t0=Date.now();while(Date.now()-t0<50000){try{fs.copyFileSync(b,t1);fs.renameSync(t1,c);fs.copyFileSync(p,t2);fs.renameSync(t2,c);}catch{}}", + benignSrc, + plantSrc, + cfg, + join(repo, 'swap-t1'), + join(repo, 'swap-t2'), + ], + { stdio: 'ignore', detached: true }, + ); + swapper.unref(); + try { + let sawSwapRefusal = false; + for (let i = 0; i < 400 && !sawSwapRefusal; i++) { + const r = localFilterRefusal(tree, 'the probe checkout'); + if (r !== null) { + expect(r).toContain('the probe checkout'); + if (r.includes('changed between the screen reads')) { + sawSwapRefusal = true; + } + } + } + expect(sawSwapRefusal).toBe(true); + } finally { + if (swapper.pid !== undefined) process.kill(swapper.pid, 'SIGKILL'); + } + }); +}); diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts index 13a122da70f..36b927b81b6 100644 --- a/packages/cli/src/commands/review/lib/worktree.ts +++ b/packages/cli/src/commands/review/lib/worktree.ts @@ -18,6 +18,8 @@ import { spawnSync } from 'node:child_process'; import { + accessSync, + constants, existsSync, lstatSync, mkdtempSync, @@ -34,6 +36,7 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, dirname, isAbsolute, join, resolve, sep } from 'node:path'; +import { inertPath } from './paths.js'; import { readWorkspacePackages } from './workspaces.js'; export type SweepResult = ReturnType; @@ -483,6 +486,10 @@ function ignoreSourcesOf( [ ...anchor, ...pipelineExcludeArgs(), + // `check-ignore` runs a planted `core.fsmonitor` too (measured live) + // — the same pin the sibling spawns carry. + '-c', + 'core.fsmonitor=', 'check-ignore', '-z', '-v', @@ -559,18 +566,635 @@ function trackedIgnoreSources( s.length > 0 && !isAbsolute(s) && !s.split('/').some((p) => p === '..'), ); if (inside.length === 0) return new Set(); - const r = spawnSync('git', [...anchor, 'ls-files', '-z', '--', ...inside], { - cwd, - encoding: 'utf8', - maxBuffer: 64 * 1024 * 1024, - env: sanitizedGitEnv(), - }); + const r = spawnSync( + 'git', + // The `-c core.fsmonitor=` pin every sibling spawn in this file carries: + // `ls-files` runs a planted `core.fsmonitor` (measured live), and this + // spawn has no screen beside it. + [...anchor, '-c', 'core.fsmonitor=', 'ls-files', '-z', '--', ...inside], + { + cwd, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + env: sanitizedGitEnv(), + }, + ); if (r.error || r.status !== 0 || typeof r.stdout !== 'string') { return new Set(); } return new Set(r.stdout.split('\0').filter((p) => p.length > 0)); } +/** + * The `-c` overrides every checkout and fetch spawn in this pipeline + * carries. The screen below reads repo-local config for filters and for + * the transport-command keys a fetch EXECUTES; the surfaces the screen + * cannot read, the spawn neutralizes — each carries the half the other + * cannot. A probe plants every one of these into the never-wiped common + * dir with the same facility as the filter plant — one write — so a spawn + * without these overrides simply moves the persistence channel this PR + * closes from a config key to another executable surface: + * + * - hooks: `worktree add` and a pathspec checkout both fire + * `post-checkout` from the shared common hooks dir (measured live, on + * the branch and the `--detach` forms). + * - fsmonitor: both shapes also run a repo-local `core.fsmonitor` + * (measured live); the empty value disables it. + * - submodule recursion: `submodule.recurse=true` makes a certified + * checkout recurse into initialized submodules and EXECUTE filters + * planted in their absorbed configs — files the screen's candidates + * never include (measured live). The pipeline never initializes a + * submodule itself — `worktree add` does not recurse — so nothing + * legitimate depends on the recursion this override turns off. + * + * The transport-command keys ride the screen instead of this list: they + * are list-valued or have fallback semantics an empty `-c` override does + * not reliably neutralize, so repo-local hits refuse fail-closed there + * (see `localFilterRefusal`). + */ +export const INERT_GIT_ARGS = [ + '-c', + 'core.hooksPath=/dev/null/no-hooks', + '-c', + 'core.fsmonitor=', + '-c', + 'submodule.recurse=false', +]; + +// A padded config can hand this refusal tens of thousands of keys, and +// test-efficacy re-embeds the full string in every probe's detail (its loops +// continue past a refusal), so an unbounded enumeration buries the actionable +// part of the report under megabytes of it. Name the first few pairs and +// count the rest — an oncall's first move is `git config --local +// --get-regexp`, which surfaces the whole set — the way the residue note +// bounds its path list. +const MAX_NAMED_SCREEN_KEYS = 8; +function nameScreenKeys( + list: Array<{ key: string; file: string }>, + noun: string, +): string { + const named = list + .slice(0, MAX_NAMED_SCREEN_KEYS) + .map((f) => `${inertPath(f.key)} (in ${inertPath(f.file)})`) + .join(', '); + const rest = list.length - MAX_NAMED_SCREEN_KEYS; + return rest > 0 + ? `${named}, … and ${rest} more ${noun}${rest === 1 ? '' : 's'}` + : named; +} + +// One part (src or dst) of a fetch refspec, at the check-ref-format grade +// git's own refspec parser applies — false for anything git rejects with +// `fatal: invalid refspec`, a shape that wedges every later fetch-by- +// remote-name (measured live for each rule: whitespace, `..`, a leading or +// trailing slash, doubled slashes, a trailing dot, a dotted or `.lock` +// component, a backslash). A single `*` is the refspec wildcard and rides; +// a bare `@` is admitted because git dwims it to HEAD before validating +// (measured), so refusing it would refuse a value git fetches with. +function refspecPartValid(part: string): boolean { + if (part === '@') return true; + if (part.length === 0) return false; + if (part.split('*').length > 2) return false; + // eslint-disable-next-line no-control-regex + if (/[\s~^:?[\]\\\x00-\x1f\x7f]/.test(part)) return false; + if (part.startsWith('/') || part.endsWith('/') || part.endsWith('.')) { + return false; + } + if (part.includes('..') || part.includes('@{')) return false; + return part + .split('/') + .every((c) => c.length > 0 && !c.startsWith('.') && !c.endsWith('.lock')); +} + +// Whether that destination fails the screen. Default-DENY: the only +// destinations that certify are the two namespaces a fetch can write without +// moving a ref the user owns — `refs/remotes//`, the namespace +// the clone-default refspec maps into (wildcard destinations included), and +// `refs/pull/`, which PR-mirror clones carry. Everything else fails closed, +// each shape measured live certifying before this inversion: a bare namespace +// name (`refs/heads`, `refs/tags`) writes a ref named for the namespace +// itself, colliding with the refs every clone already has; custom namespaces +// (`refs/pr/1`), `refs/heads/`, `refs/tags/`, `refs/notes/`, +// `refs/replace/`, exact `refs/stash` and an unqualified name name the +// user's own refs or rewrite them outright; and a literal +// `refs/remotes//HEAD` destination writes THROUGH the remote-tracking +// HEAD symref, silently relocating the branch it points at — so any path under +// the remote name whose first component is `head` refuses too. Git resolves +// all of them case-insensitively, so the compare is too. +function fetchRefspecDstRefuses(dst: string): boolean { + const d = dst.toLowerCase(); + if (d.startsWith('refs/pull/')) return false; + if (d.startsWith('refs/remotes/')) { + const rest = d.slice('refs/remotes/'.length); + const slash = rest.indexOf('/'); + // A bare `refs/remotes/` (no path) is the collision shape: it + // names the namespace every tracking ref of that remote lives under. + if (slash === -1 || slash === rest.length - 1) return true; + return rest.slice(slash + 1).split('/')[0] === 'head'; + } + return true; +} + +// Whether a configured `remote..fetch` value fails the screen. Three +// independent classes, each measured live: +// +// GRAMMAR — a value git rejects with `fatal: invalid refspec` (`tag evil`, +// a wildcard without a destination, mismatched wildcards, any part that +// fails `refspecPartValid`) wedges every later fetch-by-remote-name: one +// config write, persistence in the never-wiped common dir, and no checkout +// this screen guards can run. The screen fails CLOSED on them the way it +// does on every state it cannot certify. An EMPTY value certifies: git +// ignores it (measured live), so refusing it would refuse a legal clone. +// +// SOURCE — a literal (non-wildcard) source names remote state the screen +// cannot read: when the ref is absent from the remote, every later bare +// fetch-by-remote-name dies `fatal: couldn't find remote ref` (measured +// live) — the same permanent wedge GRAMMAR fails closed on, so it fails the +// same way. Literal `HEAD` is excepted: every remote resolves it, so it +// cannot wedge. +// +// DESTINATION — a value git accepts still writes whatever its destination +// names on any fetch that applies it, so a non-empty destination also has +// to pass the default-deny allowlist above. +function fetchRefspecRefuses(value: string): boolean { + if (value.length === 0) return false; + const srcDst = value.startsWith('+') ? value.slice(1) : value; + const sep = srcDst.indexOf(':'); + const src = sep === -1 ? srcDst : srcDst.slice(0, sep); + const dst = sep === -1 ? '' : srcDst.slice(sep + 1); + if (!refspecPartValid(src)) return true; + if (dst !== '' && !refspecPartValid(dst)) return true; + // A wildcard needs a destination to expand into (a colon-less wildcard + // is the invalid-refspec shape), and a coloned refspec pairs them — git + // rejects either mismatch (measured live). + const srcWild = src.includes('*'); + const dstWild = dst.includes('*'); + if (sep === -1 ? srcWild : srcWild !== dstWild) return true; + if (!srcWild && src.toLowerCase() !== 'head') return true; + return dst !== '' && fetchRefspecDstRefuses(dst); +} + +/** + * The refusal for a checkout about to run through a repo-local content filter — + * or through a repo-local config file the screen could not read or expand — + * and null when the screen came back clean. `checkout` is the noun phrase + * naming the checkout being authorised ("this tree's restore", "the revert + * checkout"); it lands in the message verbatim. + * + * Every checkout in this pipeline EXECUTES these filters — the scratch tree's + * reset and rebuild, the probe tree's creation, per-run restore and revert + * checkouts, the base tree's creation checkout, and the review worktree's own + * creation checkout — each is screened before it runs, and each spawn carries + * `INERT_GIT_ARGS` beside the screen: hooks and fsmonitor are config-driven + * surfaces too, and no screen here reads them. 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 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. `--file` likewise does not expand + * `include.path`/`includeIf.*.path` while the checkout's merged read DOES, so + * a filter planted behind an include would EXECUTE unseen: any include + * directive in the candidates refuses the checkout fail-closed until the + * origin-scoped follow-up tracked on this PR lands — a merged read with + * `--show-origin` refusing only repo-local hits. (`--includes` or plain merged + * config now would follow an include into the user's global config and + * re-import `filter.lfs.clean`, the permanent-refusal failure.) 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. + * + * The transport-command keys a lazy-fetch EXECUTES ride this screen for the + * same reason filters do: a promisor remote — `remote..promisor`, + * with or without `extensions.partialClone` — makes a checkout that hits a + * missing object fetch (measured: promisor alone suffices), and + * `core.sshCommand`, `core.gitProxy`, `core.askpass`, bare and URL-scoped + * `credential.*.helper`, `remote..uploadpack`, `ext::` remote + * URLs — `protocol.allow` lifts the default-deny beside the + * `protocol.ext.allow` that names the protocol — and + * `core.alternateRefsCommand`, which every fetch runs once per registered + * alternate during ref negotiation, are commands `INERT_GIT_ARGS` cannot + * neutralize: two are list-valued or fall back when emptied. Repo-local + * hits refuse fail-closed, the trigger keys included (measured live through + * all three pipeline spawn shapes). Two keys ride the screen on their own + * terms: a planted `remote..fetch` refspec is applied beside the + * command-line one on every fetch and a destination that rewrites a ref + * the user owns moves it (measured: unpushed work orphaned; a glob aimed + * at the checked-out branch wedges every fetch), but the clone-default + * refspec has exactly that key in every clone's local config — so it is + * judged by VALUE (see `fetchRefspecRefuses`), never by the key + * itself. The `url..insteadOf` / `pushInsteadOf` rewrites ride the + * transport refusal: one plant redirected the guarded fetches to an + * attacker-controlled repository while the screen certified (measured + * live), and a fresh pipeline clone never carries them — refusal cannot + * break a legitimate one, the same posture as include directives. + */ +// A screen spawn must still END against a config that blocks in open(): a +// FIFO planted at a candidate path — one mkfifo+rename into the never-wiped +// common dir, the same write class as the filter plant — hangs an unbounded +// spawnSync before any gate below runs, and every screen after it. SIGKILL, +// because a child blocked in a syscall cannot be asked. Local reads only; +// seconds are generous for them. +export const SCREEN_SPAWN_TIMEOUT_MS = 5_000; + +export function localFilterRefusal( + worktree: string, + checkout: string, +): string | null { + const files = spawnSync( + 'git', + ['rev-parse', '--git-common-dir', '--git-dir'], + { + cwd: worktree, + encoding: 'utf8', + env: sanitizedGitEnv(), + timeout: SCREEN_SPAWN_TIMEOUT_MS, + killSignal: 'SIGKILL', + }, + ); + if (files.error || files.status !== 0 || typeof files.stdout !== 'string') { + // Fail CLOSED — this branch used to return null and certify, but a spawn + // a FIFO holds in open() read no config, and the checkout this screen + // guards opens the same file. A repository rev-parse cannot read is one + // the checkout cannot run in either, so refusing costs nothing. + return `the repository's git directory could not be read (${inertPath( + files.error + ? files.error.message + : (files.stderr ?? '').toString().trim() || + `git rev-parse exited ${files.status}`, + )}), so the screen cannot certify that ${checkout} would not EXECUTE a content filter`; + } + // Parse the raw stdout UNTRIMMED: a directory path legally ENDS IN + // WHITESPACE (`git clone --separate-git-dir '/x/sep '`), and trimming + // silently truncated the final answer — the config.worktree candidate + // misresolved, missed existsSync, and the screen certified a checkout it + // never read that file (measured live: a planted uploadpack executed on + // the authorised fetch). The shape is exactly two answers and one + // trailing newline; anything else — a newline inside a path component + // splitting one answer across lines, an answer that differs from its + // trimmed self — fails closed like every other ambiguous state below. + const lines = files.stdout.split('\n'); + if ( + lines.length !== 3 || + lines[2] !== '' || + lines[0] !== lines[0].trim() || + lines[1] !== lines[1].trim() + ) { + return `the repository's git directory layout could not be parsed (a repository path containing a newline or leading/trailing whitespace), so the screen cannot certify that ${checkout} would not EXECUTE a content filter`; + } + const [commonDir, gitDir] = lines; + const common = resolve(worktree, commonDir); + // `/config.worktree` is the MAIN worktree's per-worktree config. + // It joins the screened set because a checkout run in ANY worktree of the + // repository reads it once `extensions.worktreeConfig` is on — including + // the probe/scratch/base trees this screen authorises checkouts in — and + // neither the common `config` nor a linked worktree's own + // `config.worktree` names it. A filter planted there executed during a + // certified probe checkout while this function reported the repository + // clean. When the screened tree IS the main worktree this duplicates the + // entry below; the Set dedups it. + const candidates = [ + join(common, 'config'), + join(common, 'config.worktree'), + 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 can run in ANOTHER + // tree — the SCRATCH tree, the probe tree — whose own + // `/worktrees/