diff --git a/src/git-clone.ts b/src/git-clone.ts index f22d3e46..3523156e 100644 --- a/src/git-clone.ts +++ b/src/git-clone.ts @@ -37,15 +37,21 @@ * Every git spawn has a timeout, a hard kill escalation, captured-output caps, * and shares a small global concurrency pool (same reasoning as * `document-conversion-limiter.ts`: N simultaneous clones of large repos is a - * localhost resource-exhaustion vector). Cloning is otherwise unbounded in disk - * and time, which is exactly why the caller must treat the timeout as normal. + * localhost resource-exhaustion vector). The pool's waiter queue is itself + * bounded (overflow answers BUSY immediately), and time spent queued counts + * against the operation's own deadline, so a caller's timeout bounds the whole + * call rather than starting when a slot happens to free up. Cloning is + * otherwise unbounded in disk and time, which is exactly why the caller must + * treat the timeout as normal. * * @module git-clone */ import { spawn, execFileSync } from 'node:child_process'; +import { randomBytes } from 'node:crypto'; import { existsSync } from 'node:fs'; -import { rm } from 'node:fs/promises'; +import { rename, rm } from 'node:fs/promises'; +import { basename, dirname, join } from 'node:path'; import { EXEC_TIMEOUT_MS } from './config/exec-timeout.js'; // ─── Tunables ──────────────────────────────────────────────────────────────── @@ -78,6 +84,18 @@ const MAX_CONCURRENT_GIT_OPERATIONS = (() => { return Number.isFinite(raw) && raw >= 1 ? Math.floor(raw) : 2; })(); +/** + * Waiters allowed BEHIND the pool before new work is refused outright with + * BUSY. Without a bound, every queued request holds its HTTP connection (and + * its closure) open indefinitely, so a burst of clone requests becomes the + * memory/socket exhaustion the pool exists to prevent. Override with + * CODEMAN_MAX_GIT_QUEUE (0 disables queuing entirely). + */ +const MAX_QUEUED_GIT_OPERATIONS = (() => { + const raw = Number(process.env.CODEMAN_MAX_GIT_QUEUE); + return Number.isFinite(raw) && raw >= 0 ? Math.floor(raw) : 16; +})(); + /** Longest accepted repository operand. Real URLs are far shorter; this bounds abuse. */ const MAX_REPOSITORY_LENGTH = 2048; /** Longest accepted branch/tag. git's own limit is much higher; 200 covers every real ref. */ @@ -156,6 +174,7 @@ export type GitFailureCode = | 'REF_NOT_FOUND' | 'HOST_UNREACHABLE' | 'DESTINATION_EXISTS' + | 'BUSY' | 'FAILED'; export interface GitFailure { @@ -314,7 +333,16 @@ export function parseGitRepositoryUrl(input: string): GitUrlParse { } const host = url.host; if (!host || !HOST_RE.test(host)) return reject('BAD_SYNTAX', 'That URL has no usable hostname.'); - const { owner, repo } = splitRepoPath(decodeURIComponent(url.pathname)); + // `new URL` tolerates malformed percent-escapes ("%zz" passes through), but + // decodeURIComponent throws on them: uncaught, that URIError was a 500 for + // what is simply a malformed URL. + let pathname: string; + try { + pathname = decodeURIComponent(url.pathname); + } catch { + return reject('BAD_SYNTAX', 'That URL contains an invalid percent-escape.'); + } + const { owner, repo } = splitRepoPath(pathname); const warnings: string[] = []; if (scheme === 'http') warnings.push('Plain http:// is unencrypted. Prefer https:// when the host offers it.'); @@ -519,6 +547,13 @@ export function classifyGitFailure(stderr: string, timedOut: boolean, spawnError stderr: clean, }; } + if (spawnError && spawnError.startsWith('EBUSY')) { + return { + code: 'BUSY', + message: 'Too many git operations are already running on this server. Try again in a moment.', + stderr: clean, + }; + } if (timedOut) { return { code: 'TIMEOUT', @@ -571,25 +606,57 @@ function firstLine(text: string): string { // ─── IO: bounded git spawns ────────────────────────────────────────────────── let activeGitOperations = 0; -const gitWaiters: Array<() => void> = []; + +type SlotAcquisition = 'acquired' | 'queue-full' | 'timed-out'; +interface GitSlotWaiter { + grant: () => void; +} +const gitWaiters: GitSlotWaiter[] = []; /** Test/diagnostic hook: git operations currently holding a slot. */ export function getActiveGitOperationCount(): number { return activeGitOperations; } -function acquireGitSlot(): Promise { +/** Test/diagnostic hook: git operations currently queued behind the pool. */ +export function getQueuedGitOperationCount(): number { + return gitWaiters.length; +} + +/** + * Acquire a pool slot, waiting at most `maxWaitMs` in a BOUNDED queue. + * + * Both failure modes resolve (never reject): a full queue answers immediately, + * and a queue wait that exhausts the caller's deadline removes itself before + * resolving, so an abandoned waiter can never be granted a slot later and leak + * it. + */ +function acquireGitSlot(maxWaitMs: number): Promise { if (activeGitOperations < MAX_CONCURRENT_GIT_OPERATIONS) { activeGitOperations++; - return Promise.resolve(); + return Promise.resolve('acquired'); } - return new Promise((resolve) => gitWaiters.push(resolve)); + if (gitWaiters.length >= MAX_QUEUED_GIT_OPERATIONS) return Promise.resolve('queue-full'); + return new Promise((resolve) => { + const waiter: GitSlotWaiter = { + grant: () => { + clearTimeout(timer); + resolve('acquired'); + }, + }; + const timer = setTimeout(() => { + const idx = gitWaiters.indexOf(waiter); + if (idx !== -1) gitWaiters.splice(idx, 1); + resolve('timed-out'); + }, maxWaitMs); + gitWaiters.push(waiter); + }); } function releaseGitSlot(): void { const next = gitWaiters.shift(); // Hand the slot straight over so the active count can never exceed the cap. - if (next) next(); + if (next) next.grant(); else activeGitOperations--; } @@ -611,7 +678,18 @@ interface GitRun { * signal is used rather than `child.kill()`. */ async function runGit(args: string[], timeoutMs: number, maxStdoutBytes: number): Promise { - await acquireGitSlot(); + // The queue wait spends the SAME deadline as the operation: `timeoutMs` is a + // promise about the whole call, not about git's runtime after some unbounded + // wait. A full queue is refused outright rather than queued. + const queuedAt = Date.now(); + const slot = await acquireGitSlot(timeoutMs); + if (slot === 'queue-full') { + return { stdout: '', stderr: '', code: null, timedOut: false, spawnError: 'EBUSY: git operation queue is full' }; + } + if (slot === 'timed-out') { + return { stdout: '', stderr: '', code: null, timedOut: true }; + } + const remainingMs = Math.max(1, timeoutMs - (Date.now() - queuedAt)); try { return await new Promise((resolve) => { let child: ReturnType; @@ -649,7 +727,7 @@ async function runGit(args: string[], timeoutMs: number, maxStdoutBytes: number) timedOut = true; killTree('SIGTERM'); killTimer = setTimeout(() => killTree('SIGKILL'), 3_000); - }, timeoutMs); + }, remainingMs); child.stdout?.on('data', (chunk: Buffer) => { stdoutBytes += chunk.length; @@ -735,10 +813,14 @@ export async function probeGitRemote( /** * Clone `repository` into `destination`. * - * On failure the destination is removed — but only when it did not exist before - * the attempt, so a half-written clone is cleaned up while an existing - * directory is never touched. Callers still reject a pre-existing destination - * upfront; this is the second line of that same defence. + * git clones into an ATTEMPT-OWNED temp sibling (`..cloning-`, + * dot-prefixed so an orphan from a crash never shows up as a case), which is + * atomically renamed into place on success. Two concurrent requests for the + * same destination used to both pass the existence check, and the loser's + * failure cleanup then deleted the WINNER's freshly cloned tree; now each + * attempt only ever creates and removes its own directory, the rename decides + * the winner, and the loser reports DESTINATION_EXISTS. The upfront existence + * check stays as the fast path for the common non-racing case. * * Never throws; every outcome is a `CloneResult`. */ @@ -752,19 +834,51 @@ export async function cloneRepository(opts: CloneOptions): Promise failure: { code: 'REF_NOT_FOUND', message: 'Invalid branch or tag name.', stderr: '' }, }; } - const preexisting = existsSync(opts.destination); - if (preexisting) { + if (existsSync(opts.destination)) { return { ok: false, failure: { code: 'DESTINATION_EXISTS', message: 'The destination directory already exists.', stderr: '' }, }; } - const run = await runGit(buildCloneArgs(opts), opts.timeoutMs ?? GIT_CLONE_TIMEOUT_MS, MAX_STDERR_BYTES); - if (run.code === 0 && !run.spawnError) return { ok: true, stderr: sanitizeGitOutput(run.stderr) }; + // Sibling of the destination (same filesystem), so the rename is atomic. + const attemptDir = join( + dirname(opts.destination), + `.${basename(opts.destination)}.cloning-${randomBytes(6).toString('hex')}` + ); + const run = await runGit( + buildCloneArgs({ ...opts, destination: attemptDir }), + opts.timeoutMs ?? GIT_CLONE_TIMEOUT_MS, + MAX_STDERR_BYTES + ); + if (run.code === 0 && !run.spawnError) { + try { + await rename(attemptDir, opts.destination); + return { ok: true, stderr: sanitizeGitOutput(run.stderr) }; + } catch (err) { + // Renaming a directory onto an existing non-empty one fails: someone + // else won the race. Clean up OUR tree only; theirs is never touched. + await rm(attemptDir, { recursive: true, force: true }).catch(() => {}); + const code = (err as NodeJS.ErrnoException).code; + if (code === 'EEXIST' || code === 'ENOTEMPTY' || code === 'ENOTDIR' || code === 'EPERM') { + return { + ok: false, + failure: { code: 'DESTINATION_EXISTS', message: 'The destination directory already exists.', stderr: '' }, + }; + } + return { + ok: false, + failure: { + code: 'FAILED', + message: `Could not move the finished clone into place: ${String(err)}`, + stderr: '', + }, + }; + } + } - // Remove ONLY the directory this attempt created (git may have written a - // partial tree, or nothing at all). - await rm(opts.destination, { recursive: true, force: true }).catch(() => {}); + // Remove ONLY this attempt's temp directory (git may have written a partial + // tree, or nothing at all). The destination is never deleted on failure. + await rm(attemptDir, { recursive: true, force: true }).catch(() => {}); return { ok: false, failure: classifyGitFailure(run.stderr, run.timedOut, run.spawnError) }; } diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 8a2cfc1f..9c0048bd 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -29,7 +29,7 @@ import { randomBytes } from 'node:crypto'; import { existsSync } from 'node:fs'; -import { readFile, writeFile, mkdir, lstat, readdir, rename, unlink, rmdir } from 'node:fs/promises'; +import { readFile, writeFile, mkdir, lstat, readdir, realpath, rename, unlink, rmdir } from 'node:fs/promises'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -526,6 +526,13 @@ export async function updateCaseEnvVars(casePath: string, envVars: Record { + // Same symlink refusal as writeHooksConfig: this writer runs for cloned + // cases too (modelOverride at quick-start), against foreign tree contents. + const blocker = await settingsWriteBlocker(casePath); + if (blocker) { + console.warn(`[hooks-config] Refusing to write model for ${casePath}: ${blocker}`); + return; + } const claudeDir = join(casePath, '.claude'); const settingsPath = join(claudeDir, 'settings.local.json'); await withSettingsLock(settingsPath, async () => { @@ -550,11 +557,47 @@ export async function updateCaseModel(casePath: string, model: string | null): P }); } +/** + * Why writing into `/.claude/settings.local.json` must NOT proceed, + * or null when it is safe. + * + * Case contents can be FOREIGN (a freshly cloned repository, an imported + * tree): `.claude` or the settings file itself can arrive as a symlink + * pointing anywhere on this machine, and `writeFile` follows links, so a + * scaffold write would land outside the case, up to and including replacing + * the user's own `~/.claude/settings.json` (#251 review). Any symlink in the + * chain, or a `.claude` that resolves outside the case, refuses the write. + * A missing `.claude` is fine (the writer creates it). + */ +export async function settingsWriteBlocker(casePath: string): Promise { + const claudeDir = join(casePath, '.claude'); + try { + const dirStat = await lstat(claudeDir).catch(() => null); + if (dirStat?.isSymbolicLink()) return 'its .claude is a symlink'; + if (dirStat && !dirStat.isDirectory()) return 'its .claude is a file, not a directory'; + if (dirStat && (await realpath(claudeDir)) !== join(await realpath(casePath), '.claude')) { + return 'its .claude directory resolves outside the case'; + } + const settingsStat = await lstat(join(claudeDir, 'settings.local.json')).catch(() => null); + if (settingsStat?.isSymbolicLink()) return 'its .claude/settings.local.json is a symlink'; + } catch (err) { + return `its .claude paths could not be verified (${String(err)})`; + } + return null; +} + /** * Writes hooks config to .claude/settings.local.json in the given case path. * Merges with existing file content, only touching the `hooks` key. + * Refuses (with a console.warn, not a throw: hooks degrade to output-based + * idle detection) when `settingsWriteBlocker` reports the target unsafe. */ export async function writeHooksConfig(casePath: string): Promise { + const blocker = await settingsWriteBlocker(casePath); + if (blocker) { + console.warn(`[hooks-config] Refusing to write hooks for ${casePath}: ${blocker}`); + return; + } const claudeDir = join(casePath, '.claude'); const settingsPath = join(claudeDir, 'settings.local.json'); await withSettingsLock(settingsPath, async () => { diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index 224c6d00..978ac941 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -7,7 +7,7 @@ */ import { FastifyInstance } from 'fastify'; -import { existsSync, mkdirSync, writeFileSync, readdirSync, readFileSync, createReadStream } from 'node:fs'; +import { existsSync, lstatSync, mkdirSync, writeFileSync, readdirSync, readFileSync, createReadStream } from 'node:fs'; import { exec } from 'node:child_process'; import fs from 'node:fs/promises'; import { join, resolve, basename } from 'node:path'; @@ -39,7 +39,7 @@ import { } from '../../git-clone.js'; import type { GitRemoteProbe, GitUrlParse } from '../../git-clone.js'; import { generateClaudeMd } from '../../templates/claude-md.js'; -import { writeHooksConfig } from '../../hooks-config.js'; +import { settingsWriteBlocker, writeHooksConfig } from '../../hooks-config.js'; import { canAccessOwned, getAuthUser, @@ -477,17 +477,25 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config ? ApiErrorCode.ALREADY_EXISTS : clone.failure.code === 'REF_NOT_FOUND' ? ApiErrorCode.INVALID_INPUT - : ApiErrorCode.OPERATION_FAILED; + : clone.failure.code === 'BUSY' + ? ApiErrorCode.RATE_LIMITED + : ApiErrorCode.OPERATION_FAILED; const detail = clone.failure.stderr ? `${clone.failure.message} (${gitDiagnosticLine(clone.failure.stderr)})` : clone.failure.message; return createErrorResponse(code, detail); } - // Scaffold WITHOUT overwriting anything the repository shipped. + // Scaffold WITHOUT overwriting anything the repository shipped, and + // WITHOUT writing through anything it shipped as a symlink. const warnings = [...parsed.warnings]; try { - if (!existsSync(join(casePath, 'CLAUDE.md'))) { + // Presence via lstat, not existsSync: a repo-shipped CLAUDE.md SYMLINK + // counts as "the repository ships its own" even when the link is + // broken (existsSync follows links and reports a broken one as + // absent), because writeFileSync would write THROUGH it to a + // repository-chosen path outside the case. + if (!lstatSync(join(casePath, 'CLAUDE.md'), { throwIfNoEntry: false })) { const templatePath = await ctx.getDefaultClaudeMdPath(); const summary = description || `Cloned from ${parsed.repository}`; writeFileSync(join(casePath, 'CLAUDE.md'), generateClaudeMd(name, summary, templatePath)); @@ -499,7 +507,18 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config 'This repository ships its own .claude/settings files. Codeman merged its hooks alongside them without removing anything — review them before starting a session, since repo-supplied hooks run on this machine.' ); } - await writeHooksConfig(casePath); + // A repository can ship `.claude` (or the settings file) as a symlink + // pointing anywhere on this machine; writeHooksConfig itself refuses + // to write through those (settingsWriteBlocker in hooks-config.ts). + // Checking here too turns that refusal into a user-visible warning. + const hooksBlocker = await settingsWriteBlocker(casePath); + if (hooksBlocker) { + warnings.push( + `Codeman hooks were NOT installed: ${hooksBlocker}. Codeman refuses to write through repository-controlled links; replace the link with a real file or directory if you want hooks in this case.` + ); + } else { + await writeHooksConfig(casePath); + } } catch (err) { // The clone itself succeeded: keep the case and report the scaffolding // problem, rather than deleting a tree the user just waited for. diff --git a/test/git-clone.test.ts b/test/git-clone.test.ts index a1c3a731..9c5fb11d 100644 --- a/test/git-clone.test.ts +++ b/test/git-clone.test.ts @@ -14,9 +14,9 @@ * Port: N/A (no server). */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { execFileSync } from 'node:child_process'; -import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { @@ -134,6 +134,13 @@ describe('parseGitRepositoryUrl', () => { it('REFUSES a URL with no repository name', () => { expect(rejected('https://github.com/').code).toBe('NO_REPOSITORY_NAME'); }); + + it('REFUSES a malformed percent-escape as BAD_SYNTAX instead of throwing', () => { + // `new URL` tolerates "%zz" in a path; decodeURIComponent throws on it, + // and uncaught that URIError surfaced as a 500 from the route. + expect(rejected('https://github.com/%zz/repo.git').code).toBe('BAD_SYNTAX'); + expect(rejected('https://github.com/owner/repo%').code).toBe('BAD_SYNTAX'); + }); }); describe('suggestCaseNameFromRepo', () => { @@ -377,8 +384,29 @@ describe.skipIf(!gitPresent)('cloneRepository / probeGitRemote (real git)', () = const result = await cloneRepository({ repository: origin, destination: dest, ref: 'no-such-branch' }); expect(result.ok).toBe(false); if (!result.ok) expect(result.failure.code).toBe('REF_NOT_FOUND'); - // The half-written tree must not survive as a phantom case directory. + // The half-written tree must not survive as a phantom case directory, + // and neither may the attempt-owned temp directory it cloned into. expect(existsSync(dest)).toBe(false); + expect(readdirSync(root).filter((n) => n.includes('.cloning-'))).toEqual([]); + }); + + it('lets two concurrent clones of the SAME destination race safely', async () => { + // Both used to pass the existence check; the loser's cleanup then DELETED + // the winner's finished tree. Now each attempt clones into its own temp + // sibling and an atomic rename decides the winner. + const dest = join(root, 'clone-race'); + const results = await Promise.all([ + cloneRepository({ repository: origin, destination: dest }), + cloneRepository({ repository: origin, destination: dest }), + ]); + expect(results.filter((r) => r.ok)).toHaveLength(1); + const loser = results.find((r) => !r.ok); + if (loser && !loser.ok) expect(loser.failure.code).toBe('DESTINATION_EXISTS'); + // The winner's tree survives the loser's cleanup intact... + expect(existsSync(join(dest, 'README.md'))).toBe(true); + expect(existsSync(join(dest, '.git'))).toBe(true); + // ...and neither attempt leaves its temp directory behind. + expect(readdirSync(root).filter((n) => n.includes('.cloning-'))).toEqual([]); }); it('refuses a destination that already exists instead of cloning into it', async () => { @@ -418,6 +446,70 @@ describe.skipIf(!gitPresent)('cloneRepository / probeGitRemote (real git)', () = expect(result.ok).toBe(false); if (!result.ok) expect(result.failure.code).toBe('TIMEOUT'); expect(existsSync(join(root, 'clone-timeout'))).toBe(false); + expect(readdirSync(root).filter((n) => n.includes('.cloning-'))).toEqual([]); + }); +}); + +// ─── Pool bounds, driven with a fake `git` that sleeps ────────────────────── +// +// A fresh module instance (vi.resetModules + dynamic import) picks up the +// 1-slot/1-waiter env config, and a PATH-shimmed `git` that answers --version +// then sleeps lets one operation HOLD the slot deterministically with no +// network. Placed after the real-git suite so the PATH shim never leaks into it. +describe('git pool queue bounds (fake git)', () => { + let fakeDir: string; + let savedPath: string | undefined; + let mod: typeof import('../src/git-clone.js'); + + beforeAll(async () => { + fakeDir = mkdtempSync(join(tmpdir(), 'codeman-fake-git-')); + writeFileSync( + join(fakeDir, 'git'), + '#!/bin/sh\nif [ "$1" = "--version" ]; then echo "git version 2.43.0"; exit 0; fi\nsleep 30\n', + { mode: 0o755 } + ); + savedPath = process.env.PATH; + process.env.PATH = `${fakeDir}:${savedPath}`; + process.env.CODEMAN_MAX_GIT_OPERATIONS = '1'; + process.env.CODEMAN_MAX_GIT_QUEUE = '1'; + vi.resetModules(); + mod = await import('../src/git-clone.js'); + }); + + afterAll(() => { + process.env.PATH = savedPath; + delete process.env.CODEMAN_MAX_GIT_OPERATIONS; + delete process.env.CODEMAN_MAX_GIT_QUEUE; + rmSync(fakeDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it('bounds the queue with BUSY and counts queue time against the deadline', async () => { + // Occupies the single slot: the fake git sleeps far past its 3s budget. + const holder = mod.probeGitRemote('https://pool.invalid/repo.git', 3_000); + await new Promise((r) => setTimeout(r, 100)); + + // Fills the single queue seat; its 300ms deadline must elapse IN the queue. + const queued = mod.probeGitRemote('https://pool.invalid/repo.git', 300); + await new Promise((r) => setTimeout(r, 50)); + + // Queue full: answered BUSY immediately, without waiting out its own 5s budget. + const before = Date.now(); + const overflow = await mod.probeGitRemote('https://pool.invalid/repo.git', 5_000); + expect(Date.now() - before).toBeLessThan(1_000); + expect(overflow.reachable).toBe(false); + expect(overflow.failure?.code).toBe('BUSY'); + + // The queued waiter timed out WITHOUT ever spawning git (slot never freed). + const queuedResult = await queued; + expect(queuedResult.reachable).toBe(false); + expect(queuedResult.failure?.code).toBe('TIMEOUT'); + + // The slot holder is killed by its own deadline, and nothing leaks. + const holderResult = await holder; + expect(holderResult.failure?.code).toBe('TIMEOUT'); + expect(mod.getActiveGitOperationCount()).toBe(0); + expect(mod.getQueuedGitOperationCount()).toBe(0); }); }); diff --git a/test/hooks-config.test.ts b/test/hooks-config.test.ts index 1f751905..500fa6c9 100644 --- a/test/hooks-config.test.ts +++ b/test/hooks-config.test.ts @@ -6,7 +6,7 @@ */ import { describe, it, expect, beforeAll, beforeEach, afterAll, afterEach } from 'vitest'; -import { closeSync, existsSync, openSync, readFileSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { closeSync, existsSync, openSync, readFileSync, writeFileSync, mkdirSync, rmSync, symlinkSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { spawn } from 'node:child_process'; @@ -16,6 +16,7 @@ import { generateHooksConfig, generateSubagentStopGuardScript, refreshStaleCodemanHooks, + settingsWriteBlocker, writeHooksConfig, } from '../src/hooks-config.js'; @@ -198,6 +199,37 @@ describe('writeHooksConfig', () => { expect(parsed.hooks.Stop).toHaveLength(1); }); + it('refuses to write through a symlinked .claude directory (#251 review)', async () => { + // Case contents can be foreign (a freshly cloned repository): a symlinked + // .claude would redirect the scaffold write outside the case. + const outside = join(testDir, 'outside-target'); + mkdirSync(outside); + const caseDir = join(testDir, 'case'); + mkdirSync(caseDir); + symlinkSync(outside, join(caseDir, '.claude')); + expect(await settingsWriteBlocker(caseDir)).toMatch(/symlink/); + await writeHooksConfig(caseDir); + expect(existsSync(join(outside, 'settings.local.json'))).toBe(false); + }); + + it('refuses to write through a symlinked settings.local.json (#251 review)', async () => { + const outsideFile = join(testDir, 'victim-settings.json'); + writeFileSync(outsideFile, '{"model":"precious"}\n'); + const caseDir = join(testDir, 'case2'); + mkdirSync(join(caseDir, '.claude'), { recursive: true }); + symlinkSync(outsideFile, join(caseDir, '.claude', 'settings.local.json')); + expect(await settingsWriteBlocker(caseDir)).toMatch(/symlink/); + await writeHooksConfig(caseDir); + // The link target is untouched: no hooks were merged into it. + expect(readFileSync(outsideFile, 'utf-8')).toBe('{"model":"precious"}\n'); + }); + + it('reports a real, confined .claude as safe', async () => { + const caseDir = join(testDir, 'case3'); + mkdirSync(join(caseDir, '.claude'), { recursive: true }); + expect(await settingsWriteBlocker(caseDir)).toBeNull(); + }); + it('should merge with existing settings.local.json', async () => { const claudeDir = join(testDir, '.claude'); mkdirSync(claudeDir, { recursive: true }); diff --git a/test/routes/case-clone-routes.test.ts b/test/routes/case-clone-routes.test.ts index 9821d407..6cb7e7a8 100644 --- a/test/routes/case-clone-routes.test.ts +++ b/test/routes/case-clone-routes.test.ts @@ -19,7 +19,16 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach } from import Fastify, { type FastifyInstance } from 'fastify'; import fastifyCookie from '@fastify/cookie'; import { execFileSync } from 'node:child_process'; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js'; @@ -146,6 +155,9 @@ describe('POST /api/cases/clone — input rejection', () => { describe.skipIf(!gitPresent)('POST /api/cases/clone — real clone', () => { let root: string; let origin: string; + let hostileOrigin: string; + let victimDir: string; + let victimFile: string; const created: string[] = []; const git = (args: string[], cwd: string) => @@ -174,6 +186,33 @@ describe.skipIf(!gitPresent)('POST /api/cases/clone — real clone', () => { git(['remote', 'add', 'origin', origin], work); git(['push', '--quiet', 'origin', 'main', '--tags'], work); git(['symbolic-ref', 'HEAD', 'refs/heads/main'], origin); + + // A HOSTILE repository: it ships the scaffold paths as symlinks aimed + // outside the case, so a scaffolder that follows them writes onto this + // machine's own files. victimFile deliberately does NOT exist, because a + // BROKEN CLAUDE.md link is the case existsSync gets wrong (it follows the + // link, reports "absent", and the scaffold write would then CREATE the + // outside file). + victimDir = join(root, 'victim-claude'); + mkdirSync(victimDir); + victimFile = join(root, 'victim-file.md'); + hostileOrigin = join(root, 'hostile.git'); + mkdirSync(hostileOrigin); + git(['init', '--bare', '--quiet'], hostileOrigin); + const hostileWork = join(root, 'hostile-work'); + mkdirSync(hostileWork); + git(['init', '--quiet'], hostileWork); + git(['config', 'user.email', 'test@example.com'], hostileWork); + git(['config', 'user.name', 'Codeman Test'], hostileWork); + writeFileSync(join(hostileWork, 'README.md'), '# hostile fixture\n'); + symlinkSync(victimFile, join(hostileWork, 'CLAUDE.md')); + symlinkSync(victimDir, join(hostileWork, '.claude')); + git(['add', '.'], hostileWork); + git(['commit', '--quiet', '-m', 'hostile'], hostileWork); + git(['branch', '-M', 'main'], hostileWork); + git(['remote', 'add', 'origin', hostileOrigin], hostileWork); + git(['push', '--quiet', 'origin', 'main'], hostileWork); + git(['symbolic-ref', 'HEAD', 'refs/heads/main'], hostileOrigin); }); afterAll(() => { @@ -249,6 +288,24 @@ describe.skipIf(!gitPresent)('POST /api/cases/clone — real clone', () => { expect(error).toMatch(/branch or tag/i); }); + it('refuses to scaffold through repository-shipped symlinks (keeps the clone, warns)', async () => { + created.push('hostile'); + const res = await clone({ name: 'hostile', repository: hostileOrigin }); + expect(res.statusCode).toBe(200); + const body = JSON.parse(res.body); + expect(body.success).toBe(true); + const casePath = join(CASES_DIR, 'hostile'); + // The repo's symlinks are still symlinks: nothing wrote through them. + expect(lstatSync(join(casePath, 'CLAUDE.md')).isSymbolicLink()).toBe(true); + expect(lstatSync(join(casePath, '.claude')).isSymbolicLink()).toBe(true); + // The outside targets were neither created nor written. + expect(existsSync(victimFile)).toBe(false); + expect(existsSync(join(victimDir, 'settings.local.json'))).toBe(false); + // And the response says the hooks scaffold was skipped, and why. + expect(body.data.warnings.join(' ')).toMatch(/hooks were NOT installed/i); + expect(body.data.warnings.join(' ')).toMatch(/symlink/i); + }); + it('preflights the local fixture for its branches and tags', async () => { const res = await preflight(origin); const body = JSON.parse(res.body);