Harden clone containment traversal and deadline validation

Author: mchwang

agents/contract.ts

@@ -66,8 +66,8 @@ export function captureInvocation(input: InvocationInput, now = Date.now()): Inv
   const context = input.context;
   if (![context.snapshotId, context.planId, context.assignmentId, context.referencedCodeHash].every(nonempty)
     || !integer(context.planRevision) || !integer(context.stateVersion)) throw new Error('Invalid captured context.');
-  if (!Array.isArray(input.approvedArgv) || input.approvedArgv.some(argv => !Array.isArray(argv)
-    || argv.length === 0 || !nonempty(argv[0]) || argv.some(arg => typeof arg !== 'string' || arg.includes('\0'))))
+  if (!Array.isArray(input.approvedArgv) || Array.from(input.approvedArgv).some(argv => !Array.isArray(argv)
+    || argv.length === 0 || !nonempty(argv[0]) || Array.from(argv).some(arg => typeof arg !== 'string' || arg.includes('\0'))))
     throw new Error('Commands must be complete literal argv arrays.');
   if (['planning', 'questions'].includes(input.phase) && input.approvedArgv.length)
     throw new Error('Read-only authoring and questions cannot execute commands.');

git/clone.ts

@@ -1,6 +1,6 @@
 import { execFileSync } from 'node:child_process';
 import { randomUUID } from 'node:crypto';
-import { lstatSync, mkdtempSync, readdirSync, realpathSync, rmSync } from 'node:fs';
+import { lstatSync, mkdtempSync, opendirSync, realpathSync, rmSync } from 'node:fs';
 import { isAbsolute, join, relative, resolve } from 'node:path';
 import type { TaskClone } from '../agents/contract.ts';
 
@@ -32,12 +32,16 @@ export function createTaskClone(options: {
   // Deliberately do not inherit Git variables or credential/config environment.
   const env = { PATH: process.env.PATH, GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null',
     GIT_TERMINAL_PROMPT: '0', GIT_NO_LAZY_FETCH: '1', GIT_GRAFT_FILE: '/dev/null' };
-  const run = (cwd: string, ...args: string[]) => execFileSync('git', [
-    '--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'init.templateDir=',
-    '-c', 'protocol.allow=never', '-c', 'submodule.recurse=false', ...args,
-  ], { cwd, env, timeout: remaining(), killSignal: 'SIGKILL', maxBuffer: 1024 * 1024,
-    stdio: ['ignore', 'pipe', 'pipe'] }).toString().trim();
-  const common = resolve(source, run(source, 'rev-parse', '--git-common-dir'));
+  const run = (cwd: string, ...args: string[]) => {
+    const result = execFileSync('git', [
+      '--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'init.templateDir=',
+      '-c', 'protocol.allow=never', '-c', 'submodule.recurse=false', ...args,
+    ], { cwd, env, timeout: remaining(), killSignal: 'SIGKILL', maxBuffer: 1024 * 1024,
+      stdio: ['ignore', 'pipe', 'pipe'] });
+    remaining();
+    return result.toString().trim();
+  };
+  const common = realpathSync(resolve(source, run(source, 'rev-parse', '--git-common-dir')));
   if (within(common, parent)) throw new Error('Task storage must be outside source metadata.');
   function audit(metadata: string, independent: boolean) {
     for (const name of ['shallow', 'info/grafts', 'objects/info/alternates', 'objects/info/http-alternates']) {
@@ -52,9 +56,14 @@ export function createTaskClone(options: {
       if (stat.isSymbolicLink() || (!stat.isDirectory() && !stat.isFile())) throw new Error('Unsupported object entry.');
       if (independent && stat.isFile() && stat.nlink !== 1) throw new Error('Task objects must not be hard-linked.');
       if (stat.isDirectory()) {
-        const children = readdirSync(path);
-        if (count + pending.length + children.length > 100_000) throw new Error('Object storage exceeds inspection limit.');
-        pending.push(...children.map(child => join(path, child)));
+        const directory = opendirSync(path, { bufferSize: 1 });
+        try {
+          for (let entry = directory.readSync(); entry; entry = directory.readSync()) {
+            remaining();
+            if (count + pending.length >= 100_000) throw new Error('Object storage exceeds inspection limit.');
+            pending.push(join(path, entry.name));
+          }
+        } finally { directory.closeSync(); }
       }
     }
   }

test/agent-clone.test.ts

@@ -1,10 +1,19 @@
 import { execFileSync } from 'node:child_process';
-import { mkdtempSync, mkdirSync, readFileSync, readdirSync, rmSync, lstatSync, symlinkSync, writeFileSync } from 'node:fs';
+import { mkdtempSync, mkdirSync, readFileSync, readdirSync, rmSync, lstatSync, symlinkSync, writeFileSync, renameSync, opendirSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
-import { afterEach, describe, expect, it } from 'vitest';
+import { afterEach, describe, expect, it, vi } from 'vitest';
 import { createTaskClone } from '../git/clone.ts';
 
+vi.mock('node:fs', async importOriginal => {
+  const actual = await importOriginal<typeof import('node:fs')>();
+  return { ...actual, readdirSync: vi.fn(actual.readdirSync), opendirSync: vi.fn(actual.opendirSync) };
+});
+vi.mock('node:child_process', async importOriginal => {
+  const actual = await importOriginal<typeof import('node:child_process')>();
+  return { ...actual, execFileSync: vi.fn(actual.execFileSync) };
+});
+
 const roots: string[] = [];
 const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core.hooksPath=/dev/null', ...args],
   { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
@@ -16,7 +25,7 @@ function fixture() {
   writeFileSync(join(source, 'file.txt'), 'trusted\n'); git(source, 'add', '.'); git(source, 'commit', '-m', 'baseline');
   return { source, parent, head: git(source, 'rev-parse', 'HEAD'), taskId: 'task-1' };
 }
-afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); });
+afterEach(() => { vi.restoreAllMocks(); vi.resetAllMocks(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); });
 describe('isolated staging clone', () => {
   it('copies objects, ignores dirty source changes, and has no origin or shared metadata', () => {
     const input = fixture();
@@ -69,6 +78,43 @@ describe('isolated staging clone', () => {
     expect(() => createTaskClone({ ...input, timeoutMs: Infinity })).toThrow('deadline');
     expect(readdirSync(input.parent)).toEqual([]);
   });
+  it('canonicalizes symlinked common metadata before checking storage containment', () => {
+    const input = fixture(), metadata = join(input.parent, 'metadata');
+    renameSync(join(input.source, '.git'), metadata);
+    symlinkSync(metadata, join(input.source, '.git'));
+    const parent = join(metadata, 'tasks'); mkdirSync(parent);
+    // Disputed intermediate state: Git reports a lexical path through the link.
+    expect(git(input.source, 'rev-parse', '--git-common-dir')).toBe('.git');
+    expect(lstatSync(join(input.source, '.git')).isSymbolicLink()).toBe(true);
+    expect(() => createTaskClone({ ...input, parent })).toThrow('outside source metadata');
+    expect(readdirSync(parent)).toEqual([]);
+  });
+  it('never materializes an entire object directory before checking its entry budget', () => {
+    const input = fixture();
+    // A bulk enumeration is forbidden even for a small fixture; this asserts
+    // the disputed intermediate representation, not only a later limit error.
+    vi.mocked(readdirSync).mockClear();
+    createTaskClone(input);
+    expect(readdirSync).not.toHaveBeenCalled();
+    expect(opendirSync).toHaveBeenCalled();
+  });
+  it('rejects a successful final Git call that returns after the overall deadline', async () => {
+    const input = fixture();
+    const actual = await vi.importActual<typeof import('node:child_process')>('node:child_process');
+    let elapsed = 0, lateResult = false;
+    vi.spyOn(performance, 'now').mockImplementation(() => elapsed);
+    vi.mocked(execFileSync).mockImplementation(((file: string, args: string[], options: object) => {
+      const result = actual.execFileSync(file, args, options);
+      if (args.at(-2) === 'rev-parse' && args.at(-1) === 'HEAD') {
+        elapsed = 1001; lateResult = true;
+      }
+      return result;
+    }) as typeof execFileSync);
+    expect(() => createTaskClone({ ...input, timeoutMs: 1000 })).toThrow('deadline');
+    expect(lateResult).toBe(true);
+    expect(readdirSync(input.parent)).toEqual([]);
+    vi.mocked(execFileSync).mockImplementation(actual.execFileSync);
+  });
   it('does not inherit Git directory, index, configuration or object overrides', () => {
     const input = fixture();
     const keys = ['GIT_DIR', 'GIT_INDEX_FILE', 'GIT_CONFIG_COUNT', 'GIT_CONFIG_KEY_0', 'GIT_CONFIG_VALUE_0'];

test/agent-contract.test.ts

@@ -39,4 +39,10 @@ describe('invocation boundary', () => {
     expect(() => captureInvocation({ ...request(), phase: 'shell' } as unknown as InvocationInput, 1000)).toThrow('profile');
     expect(() => captureInvocation({ ...request(), attemptId: '' }, 1000)).toThrow('identity');
   });
+  it('rejects sparse allowlists with missing arguments or commands', () => {
+    const argv = ['npm', 'test']; delete argv[1];
+    expect(1 in argv).toBe(false);
+    expect(() => captureInvocation({ ...request(), approvedArgv: [argv] }, 1000)).toThrow('argv');
+    expect(() => captureInvocation({ ...request(), approvedArgv: new Array(1) }, 1000)).toThrow('argv');
+  });
 });