fix(review): screen content filters before the probe tree's restore too (#9558)

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<typeof import('./lib/worktree.js')>();
+  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) => {

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 ?? '')),