diff --git a/src/utils/git-spawn.ts b/src/utils/git-spawn.ts index 6b699e7d..4426d1a4 100644 --- a/src/utils/git-spawn.ts +++ b/src/utils/git-spawn.ts @@ -12,11 +12,15 @@ * git child does this, not only after a trust decision. An empty * `diff.external` still tries to exec, so a read-only `diff` also passes * `--no-ext-diff` and `--no-textconv` and keeps git's own diff. Repo - * `filter.*` and `diff.*` drivers are blanked the same way, including ones - * pulled in by `include.path`, `includeIf`, or `extensions.worktreeConfig`. + * `filter.*`, `merge.*.driver`, and `diff.*` drivers are blanked for every + * `runGit` child (read-only and mutating), including ones pulled in by + * `include.path`, `includeIf`, or `extensions.worktreeConfig`. * Only `local` and `worktree` scopes are overridden. System and global config * stay in effect, and `command` scope is ignored. Mutating commands - * (worktree add, apply) keep ssh and credential helpers. + * (worktree add, apply, add) get the same discovered executable-config + * overrides so repo `filter.*` and `merge.*.driver` programs do not run; + * they still skip the unconditional read-only blanks for ssh, pager, and + * credential helpers (unless those keys are set locally). * * `GIT_*` on the child comes from the environment captured before a project * `.env`. A project file cannot point git at another repo or config. @@ -72,7 +76,7 @@ function withoutExternalDiff(args: readonly string[]): string[] { /** Repo config keys whose values are programs. */ const EXEC_CONFIG_KEY = - /^(?:filter\..+\.(?:clean|smudge|process)|diff\..+\.(?:textconv|command)|core\.(?:fsmonitor|hooksPath|sshCommand|pager)|credential\.helper)$/; + /^(?:filter\..+\.(?:clean|smudge|process)|merge\..+\.driver|diff\..+\.(?:textconv|command)|core\.(?:fsmonitor|hooksPath|sshCommand|pager)|credential\.helper)$/; /** * Git matches `--get-regexp` against the canonical lowercase key, so @@ -85,7 +89,7 @@ const EXEC_CONFIG_KEY = * own git children (those already force `core.hooksPath` to `/dev/null`). */ const EXEC_CONFIG_REGEXP = - '^(filter\\..*\\.(clean|smudge|process)|diff\\..*\\.(textconv|command)|diff\\.external|core\\.(fsmonitor|hookspath|sshcommand|pager)|credential\\.helper)$'; + '^(filter\\..*\\.(clean|smudge|process)|merge\\..*\\.driver|diff\\..*\\.(textconv|command)|diff\\.external|core\\.(fsmonitor|hookspath|sshcommand|pager)|credential\\.helper)$'; const FILTER_DRIVER = /^filter\.(.+)\.(?:clean|smudge|process)$/i; @@ -455,6 +459,8 @@ export interface RunGitOptions { /** * Default true. Pass false for mutating git (worktree add, apply, add). * `core.fsmonitor` and `core.hooksPath` are overridden either way. + * Discovered local/worktree executable-config overrides (filters, merge + * drivers, …) are injected for both read-only and mutating calls. */ readOnly?: boolean; } @@ -467,10 +473,10 @@ export async function runGit( const env: Record = { ...(opts.env ?? {}) }; if (readOnly) env.GIT_PAGER = 'cat'; let gitArgs = hardenedGitArgs(args, { readOnly }); - if (readOnly) { - const discovered = await executableConfigOverrides(opts.cwd, opts.signal); - gitArgs = insertBeforeSubcommand(gitArgs, discovered); - } + // Mutating git (worktree add, add -A -N, apply --3way) must also blank + // repo filter/merge drivers; those call sites pass readOnly: false. + const discovered = await executableConfigOverrides(opts.cwd, opts.signal); + gitArgs = insertBeforeSubcommand(gitArgs, discovered); return runProcess('git', { args: gitArgs, cwd: opts.cwd, diff --git a/test/git-spawn.spec.mjs b/test/git-spawn.spec.mjs index e6af7c39..840cbd55 100644 --- a/test/git-spawn.spec.mjs +++ b/test/git-spawn.spec.mjs @@ -107,6 +107,7 @@ describe('git spawn', { concurrency: 1 }, () => { 'local\tfilter.x.clean /tmp/clean.sh', 'worktree\tfilter.x.smudge /tmp/smudge.sh', 'local\tfilter.x.process /tmp/process.sh', + 'local\tmerge.x.driver /tmp/merge.sh', 'local\tdiff.x.textconv /tmp/textconv.sh', 'worktree\tdiff.x.command /tmp/command.sh', 'local\tcore.fsmonitor /tmp/fs.sh', @@ -120,6 +121,7 @@ describe('git spawn', { concurrency: 1 }, () => { assert.ok(args.includes('filter.x.smudge=')); assert.ok(args.includes('filter.x.process=')); assert.ok(args.includes('filter.x.required=false')); + assert.ok(args.includes('merge.x.driver=')); assert.ok(args.includes('diff.x.textconv=')); assert.ok(args.includes('diff.x.command=')); assert.ok(args.includes('core.fsmonitor=')); diff --git a/test/worktree-isolation-o79.spec.mjs b/test/worktree-isolation-o79.spec.mjs new file mode 100644 index 00000000..1bd7f6a2 --- /dev/null +++ b/test/worktree-isolation-o79.spec.mjs @@ -0,0 +1,181 @@ +/** + * O79: mutating runGit (worktree add, add -A -N, apply --3way) must not run + * untrusted repo filter/merge drivers. + */ +import test, { describe } from 'node:test'; +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; + +import { + createWorktree, + collectWorktreePatch, + applyWorktreePatch, + cleanupWorktree, +} from '../dist/core/subagent/worktree-isolation.js'; +import { runGit } from '../dist/utils/git-spawn.js'; + +function git(cwd, args, opts = {}) { + return execFileSync('git', args, { cwd, stdio: 'ignore', ...opts }); +} + +async function markerWritten(marker) { + try { + const text = await fs.readFile(marker, 'utf8'); + return text.includes('ran'); + } catch { + return false; + } +} + +describe('O79 worktree isolation git exec', { concurrency: 1 }, () => { + test('createWorktree does not run untrusted filter.smudge', async (t) => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'moss-o79-smudge-')); + t.after(() => fs.rm(root, { recursive: true, force: true })); + const repo = path.join(root, 'repo'); + await fs.mkdir(repo); + const marker = path.join(root, 'SMUDGE_RAN'); + git(repo, ['init', '-q']); + git(repo, ['config', 'user.email', 'o79@example.test']); + git(repo, ['config', 'user.name', 'o79']); + await fs.writeFile(path.join(repo, '.gitattributes'), '*.txt filter=pwn\n'); + await fs.writeFile(path.join(repo, 'a.txt'), 'hello\n'); + git(repo, ['add', '.']); + git(repo, ['commit', '-q', '-m', 'init']); + const cmd = `sh -c 'echo ran >> "${marker.replace(/\\/g, '/')}"; cat'`; + git(repo, ['config', 'filter.pwn.smudge', cmd]); + git(repo, ['config', 'filter.pwn.clean', 'cat']); + + const probe = path.join(root, 'probe'); + git(repo, ['worktree', 'add', '-q', '--detach', probe, 'HEAD']); + assert.equal(await markerWritten(marker), true, 'fixture: plain worktree add runs smudge'); + await fs.rm(marker, { force: true }); + git(repo, ['worktree', 'remove', '--force', probe]); + + const lease = await createWorktree({ parentWorkspace: repo, runId: 'o79-smudge' }); + t.after(() => cleanupWorktree({ parentWorkspace: repo, worktreePath: lease.worktreePath })); + assert.equal(await markerWritten(marker), false, 'createWorktree ran untrusted smudge'); + }); + + test('collectWorktreePatch add -A -N path does not run filter.clean', async (t) => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'moss-o79-clean-')); + t.after(() => fs.rm(root, { recursive: true, force: true })); + const repo = path.join(root, 'repo'); + await fs.mkdir(repo); + const marker = path.join(root, 'CLEAN_RAN'); + const script = path.join(root, 'clean.sh'); + await fs.writeFile(script, `#!/bin/sh\necho ran >> ${JSON.stringify(marker)}\ncat\n`); + await fs.chmod(script, 0o755); + + git(repo, ['init', '-q']); + git(repo, ['config', 'user.email', 'o79@example.test']); + git(repo, ['config', 'user.name', 'o79']); + await fs.writeFile(path.join(repo, '.gitattributes'), '*.txt filter=pwn\n'); + await fs.writeFile(path.join(repo, 'a.txt'), 'hello\n'); + git(repo, ['add', '.']); + git(repo, ['commit', '-q', '-m', 'init']); + git(repo, ['config', 'filter.pwn.clean', script]); + git(repo, ['config', 'filter.pwn.smudge', 'cat']); + git(repo, ['config', 'filter.pwn.required', 'true']); + + await fs.writeFile(path.join(repo, 'a.txt'), 'hello\nchanged\n'); + const future = new Date(Date.now() + 10_000); + await fs.utimes(path.join(repo, 'a.txt'), future, future); + await fs.rm(marker, { force: true }); + git(repo, ['add', '-A']); + assert.equal(await markerWritten(marker), true, 'fixture: raw git add -A runs clean'); + git(repo, ['reset', '-q']); + await fs.rm(marker, { force: true }); + + const lease = await createWorktree({ parentWorkspace: repo, runId: 'o79-clean' }); + t.after(() => cleanupWorktree({ parentWorkspace: repo, worktreePath: lease.worktreePath })); + await fs.writeFile(path.join(lease.worktreePath, 'a.txt'), 'hello\nlease\n'); + await fs.utimes(path.join(lease.worktreePath, 'a.txt'), future, future); + await fs.rm(marker, { force: true }); + + await collectWorktreePatch({ parentWorkspace: repo, lease }); + assert.equal( + await markerWritten(marker), + false, + 'collectWorktreePatch (add -A -N) ran untrusted clean' + ); + + // Same hardening entry point as add -A -N: mutating runGit must blank + // filter.clean. Plain `git add -A` runs clean; prove overrides apply. + await fs.writeFile(path.join(lease.worktreePath, 'a.txt'), 'hello\nlease2\n'); + await fs.utimes(path.join(lease.worktreePath, 'a.txt'), future, future); + await fs.rm(marker, { force: true }); + await runGit(['add', '-A'], { cwd: lease.worktreePath, readOnly: false, timeout: 60_000 }); + assert.equal(await markerWritten(marker), false, 'mutating runGit add -A ran untrusted clean'); + }); + + test('applyWorktreePatch --3way does not run untrusted merge driver', async (t) => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'moss-o79-merge-')); + t.after(() => fs.rm(root, { recursive: true, force: true })); + const repo = path.join(root, 'repo'); + await fs.mkdir(repo); + const marker = path.join(root, 'MERGE_RAN'); + const script = path.join(root, 'merge.sh'); + await fs.writeFile( + script, + `#!/bin/sh\necho ran >> ${JSON.stringify(marker)}\necho driver > "$1"\n` + ); + await fs.chmod(script, 0o755); + + git(repo, ['init', '-q']); + git(repo, ['config', 'user.email', 'o79@example.test']); + git(repo, ['config', 'user.name', 'o79']); + await fs.writeFile(path.join(repo, '.gitattributes'), '*.js merge=pwn\n'); + await fs.writeFile(path.join(repo, 'same.js'), 'value = 0\n'); + git(repo, ['add', '.']); + git(repo, ['commit', '-q', '-m', 'init']); + git(repo, ['config', 'merge.pwn.driver', `${script} %A`]); + git(repo, ['config', 'merge.pwn.name', 'pwn']); + + const leaseA = await createWorktree({ parentWorkspace: repo, runId: 'o79-merge-a' }); + const leaseB = await createWorktree({ parentWorkspace: repo, runId: 'o79-merge-b' }); + t.after(async () => { + await cleanupWorktree({ parentWorkspace: repo, worktreePath: leaseA.worktreePath }); + await cleanupWorktree({ parentWorkspace: repo, worktreePath: leaseB.worktreePath }); + }); + + await fs.writeFile(path.join(leaseA.worktreePath, 'same.js'), 'value = 100\n'); + await fs.writeFile(path.join(leaseB.worktreePath, 'same.js'), 'value = 200\n'); + const patchA = await collectWorktreePatch({ parentWorkspace: repo, lease: leaseA }); + const patchB = await collectWorktreePatch({ parentWorkspace: repo, lease: leaseB }); + + const first = await applyWorktreePatch({ + parentWorkspace: repo, + patchPath: patchA.patchPath, + }); + assert.equal(first.status, 'merged', 'first lease should merge cleanly'); + + // Fixture: raw git apply --3way invokes the merge driver on the clash. + const rawPatch = path.join(root, 'raw-b.patch'); + await fs.copyFile(patchB.patchPath, rawPatch); + await fs.rm(marker, { force: true }); + try { + execFileSync('git', ['apply', '--3way', rawPatch], { cwd: repo, stdio: 'ignore' }); + } catch { + /* conflict or driver rewrite is fine */ + } + assert.equal(await markerWritten(marker), true, 'fixture: raw apply --3way runs merge driver'); + git(repo, ['reset', '--hard', 'HEAD']); + // Re-apply first lease so the second apply has a real clash again. + const reapply = await applyWorktreePatch({ + parentWorkspace: repo, + patchPath: patchA.patchPath, + }); + assert.equal(reapply.status, 'merged'); + + await fs.rm(marker, { force: true }); + await applyWorktreePatch({ parentWorkspace: repo, patchPath: patchB.patchPath }); + assert.equal( + await markerWritten(marker), + false, + 'applyWorktreePatch --3way ran untrusted merge driver' + ); + }); +});