Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions git/clone.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { execFileSync } from 'node:child_process';
import { randomUUID } from 'node:crypto';
import { lstatSync, mkdtempSync, opendirSync, realpathSync, rmSync } from 'node:fs';
import { isAbsolute, join, relative, resolve } from 'node:path';
import { isAbsolute, join, relative, resolve, sep } from 'node:path';
import type { TaskClone } from '../agents/contract.ts';
import { runInProcessGroup, type ProcessGroup } from '../agents/process-group.ts';
import { runTrackedProcess, type ProcessGroupLifecycle } from '../agents/tracked-docker.ts';
Expand Down Expand Up @@ -46,6 +46,19 @@ export interface CloneOptions {
readonly timeoutMs?: number;
}

interface PathSemantics {
readonly sep: string;
isAbsolute(path: string): boolean;
relative(from: string, to: string): string;
}

/** Whether `candidate` is `base` itself or one of its descendants under the supplied platform's path rules. */
export function pathIsWithin(base: string, candidate: string,
paths: PathSemantics = { sep, isAbsolute, relative }): boolean {
const rel = paths.relative(base, candidate);
return rel === '' || (!paths.isAbsolute(rel) && rel !== '..' && !rel.startsWith(`..${paths.sep}`));
}

/**
* The clone, as a sequence of Git calls. Both variants run these same steps; only how each call runs differs. Every
* failure after the clone directory exists, an abort included, removes it before the error leaves.
Expand All @@ -62,19 +75,15 @@ function* cloneSteps(options: CloneOptions): Generator<CloneStep, TaskClone, Git
return value;
};
const source = realpathSync(options.source), parent = realpathSync(options.parent);
const within = (base: string, path: string) => {
const rel = relative(base, path);
return rel === '' || (!isAbsolute(rel) && rel !== '..' && !rel.startsWith('../'));
};
if (within(source, parent)) throw new Error('Task storage must be outside the source repository.');
if (pathIsWithin(source, parent)) throw new Error('Task storage must be outside the source repository.');
function* run(cwd: string, ...args: string[]): Generator<CloneStep, string, GitOutcome | undefined> {
const outcome = yield { cwd, args, timeoutMs: remaining() };
if (!outcome || outcome.status !== 0) throw outcome?.error ?? new Error(`git ${args[0] ?? ''} failed.`);
remaining();
return outcome.stdout.trim();
}
const common = realpathSync(resolve(source, yield* run(source, 'rev-parse', '--git-common-dir')));
if (within(common, parent)) throw new Error('Task storage must be outside source metadata.');
if (pathIsWithin(common, parent)) throw new Error('Task storage must be outside source metadata.');
function* audit(metadata: string, independent: boolean): Generator<CloneStep, void, GitOutcome | undefined> {
for (const name of ['shallow', 'info/grafts', 'objects/info/alternates', 'objects/info/http-alternates']) {
if (lstatSync(join(metadata, name), { throwIfNoEntry: false })) throw new Error(`Unsupported Git storage: ${name}`);
Expand Down
24 changes: 20 additions & 4 deletions test/agent-clone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import { execFileSync } from 'node:child_process';
import { fixtureGit } from './fixtures/git.ts';
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 { join, win32 } from 'node:path';
import { afterEach, describe, expect, it, vi } from 'vitest';
import { createTaskClone } from '../git/clone.ts';
import { createTaskClone, pathIsWithin } from '../git/clone.ts';

vi.mock('node:fs', async importOriginal => {
const actual = await importOriginal<typeof import('node:fs')>();
Expand All @@ -26,6 +26,22 @@ function fixture() {
return { source, parent, head: git(source, 'rev-parse', 'HEAD'), taskId: 'task-1' };
}
afterEach(() => { vi.restoreAllMocks(); vi.resetAllMocks(); for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); });
describe('path containment', () => {
it.each([
['same directory despite case', 'C:\\repo', 'c:\\REPO', true],
['nested directory', 'C:\\repo', 'C:\\repo\\tasks', true],
['similarly named sibling', 'C:\\repo', 'C:\\repository', false],
['same-drive sibling', 'C:\\repo', 'C:\\tasks', false],
['parent directory', 'C:\\repo\\source', 'C:\\repo', false],
['different drive', 'C:\\repo', 'D:\\tasks', false],
['nested UNC directory', '\\\\server\\share\\repo', '\\\\server\\share\\repo\\tasks', true],
['same-share UNC sibling', '\\\\server\\share\\repo', '\\\\server\\share\\tasks', false],
['different UNC share', '\\\\server\\share\\repo', '\\\\server\\other\\tasks', false],
['different UNC server', '\\\\server\\share\\repo', '\\\\other\\share\\tasks', false],
])('uses Windows boundaries for a %s', (_name, base, candidate, expected) => {
expect(pathIsWithin(base, candidate, win32)).toBe(expected);
});
});
describe('isolated staging clone', () => {
it('copies objects, ignores dirty source changes, and has no origin or shared metadata', () => {
const input = fixture();
Expand Down Expand Up @@ -66,9 +82,9 @@ describe('isolated staging clone', () => {
git(input.source, 'update-ref', `refs/replace/${input.head}`, input.head);
expect(() => createTaskClone(input)).toThrow('Replacement');
});
it('rejects nested storage, including paths through symlinks', () => {
it('rejects nested storage after resolving a directory alias, including a Windows junction', () => {
const input = fixture(), nested = join(input.source, 'tasks'), alias = join(input.parent, 'alias');
mkdirSync(nested); symlinkSync(nested, alias);
mkdirSync(nested); symlinkSync(nested, alias, process.platform === 'win32' ? 'junction' : 'dir');
expect(() => createTaskClone({ ...input, parent: alias })).toThrow('outside');
});
it('requires a full existing commit and finite time budget', () => {
Expand Down
Loading