diff --git a/git/clone.ts b/git/clone.ts index c1b23814..09428f97 100644 --- a/git/clone.ts +++ b/git/clone.ts @@ -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'; @@ -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. @@ -62,11 +75,7 @@ function* cloneSteps(options: CloneOptions): Generator { - 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 { const outcome = yield { cwd, args, timeoutMs: remaining() }; if (!outcome || outcome.status !== 0) throw outcome?.error ?? new Error(`git ${args[0] ?? ''} failed.`); @@ -74,7 +83,7 @@ function* cloneSteps(options: CloneOptions): Generator { 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}`); diff --git a/test/agent-clone.test.ts b/test/agent-clone.test.ts index 065f25f0..da7cd9c0 100644 --- a/test/agent-clone.test.ts +++ b/test/agent-clone.test.ts @@ -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(); @@ -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(); @@ -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', () => {