diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 267a425a6..a337ddee1 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -12,7 +12,7 @@ const path = require('path'); const os = require('os'); -const { cleanup, createTempDir } = require('./helpers.cjs'); +const { cleanup, createTempDir, tmpRootCandidates } = require('./helpers.cjs'); // ─── Test 1: Real-FS happy path ────────────────────────────────────────────── @@ -116,17 +116,31 @@ test('cleanup throws and does not chdir or delete when target is outside os.tmpd const outsideDir = __dirname; const knownFile = path.join(outsideDir, 'helpers-cleanup.test.cjs'); - // Mirror cleanup()'s own out-of-tmpdir predicate (tests/helpers.cjs) so - // this test cannot run on a target it does not actually control. - const tmpRoot = path.resolve(os.tmpdir()); + // Use cleanup()'s own tmpRootCandidates() as the single source of truth + // for "is this path inside tmpdir" instead of a hand-mirrored predicate. + // The old copy here checked only path.resolve(os.tmpdir()), one of + // SEVERAL root spellings cleanup() actually accepts (it also accepts the + // realpath'd and native-realpath'd forms) — a repo checked out under a + // root this precondition didn't know about would compute "outside, safe" + // while cleanup() computed "inside, delete", turning this refusal test + // into the very destructive scenario it exists to prevent. Importing the + // real function removes that drift class entirely. const resolvedOutsideDir = path.resolve(outsideDir); - const isInsideTmpdir = - resolvedOutsideDir === tmpRoot || resolvedOutsideDir.startsWith(`${tmpRoot}${path.sep}`); + const roots = tmpRootCandidates(); + const isWindows = process.platform === 'win32'; + const outsideDirForCompare = isWindows ? resolvedOutsideDir.toLowerCase() : resolvedOutsideDir; + const isInsideTmpdir = roots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + outsideDirForCompare === rootForCompare || + outsideDirForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); assert.strictEqual( isInsideTmpdir, false, - `this test cannot run safely when the repo lives under os.tmpdir(): ` + - `outsideDir (${resolvedOutsideDir}) is inside os.tmpdir() (${tmpRoot})` + `this test cannot run safely when the repo lives under any tmpdir root ` + + `cleanup() accepts: outsideDir (${resolvedOutsideDir}) is inside one of ${JSON.stringify(roots)}` ); const cwdBefore = process.cwd(); @@ -200,3 +214,96 @@ test('cleanup accepts a realpath()d temp dir even when it differs from the raw p assert.strictEqual(fs.existsSync(dir), false, 'temp dir should be removed'); } }); + +// ─── Test 7: tmpRootCandidates() shape invariants (platform-independent) ──── +// +// Test 6 above is coverage-identical to Test 5's control on any platform +// where fs.realpathSync(tmpdir) does not diverge from the raw path (Linux, +// notably — the platform the remote matrix actually runs on), so it cannot +// verify the macOS-specific fix there. These assertions hold the exported +// tmpRootCandidates() to a contract that is meaningful on every platform. + +test('tmpRootCandidates() returns a well-formed, deduplicated list of absolute paths', () => { + const roots = tmpRootCandidates(); + + assert.ok(Array.isArray(roots) && roots.length > 0, 'must return a non-empty array'); + + for (const root of roots) { + assert.strictEqual(path.isAbsolute(root), true, `root must be absolute: ${root}`); + } + + const isWindows = process.platform === 'win32'; + const keys = roots.map((r) => (isWindows ? r.toLowerCase() : r)); + const uniqueKeys = new Set(keys); + assert.strictEqual(uniqueKeys.size, keys.length, `roots must be deduplicated: ${JSON.stringify(roots)}`); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-roots-contract-')); + try { + const resolvedDir = path.resolve(dir); + const dirForCompare = isWindows ? resolvedDir.toLowerCase() : resolvedDir; + const isUnderSomeRoot = roots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + dirForCompare === rootForCompare || + dirForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); + assert.strictEqual( + isUnderSomeRoot, + true, + `a freshly mkdtempSync'd dir under os.tmpdir() must be under at least one returned root: ` + + `${resolvedDir} vs ${JSON.stringify(roots)}` + ); + } finally { + cleanup(dir); + } +}); + +// ─── Test 8: symlink-escape refusal, all platforms ─────────────────────────── + +test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir, and the victim survives', (t) => { + const symlinkParent = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-symlink-')); + t.after(() => { + if (fs.existsSync(symlinkParent)) cleanup(symlinkParent); + }); + + // The victim must live OUTSIDE tmpdir (that's the whole point of the + // scenario) — put it under this worktree's gitignored .gsd/ directory + // rather than os.homedir() so the test has no dependency on the runner's + // home layout. cleanup() will (correctly, by design) refuse a path there, + // so teardown cannot route through cleanup() either; the raw rmSync below + // is narrowly scoped and justified inline. + const victimParent = path.join(__dirname, '..', '.gsd'); + fs.mkdirSync(victimParent, { recursive: true }); + const victim = fs.mkdtempSync(path.join(victimParent, 'cleanup-symlink-victim-')); + t.after(() => { + if (fs.existsSync(victim)) { + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- victim is deliberately outside os.tmpdir() (that's the scenario under test), so cleanup() refuses it by design and cannot be used for its own teardown. + fs.rmSync(victim, { recursive: true, force: true }); + } + }); + fs.writeFileSync(path.join(victim, 'marker.txt'), 'victim'); + + const symlinkPath = path.join(symlinkParent, 'escape-link'); + let symlinkCreated = true; + try { + fs.symlinkSync(victim, symlinkPath, 'dir'); + } catch (error) { + symlinkCreated = false; + t.skip(`symlink creation not permitted in this environment (${error.code || error.message}) — likely Windows without developer mode`); + } + + if (symlinkCreated) { + assert.throws( + () => cleanup(symlinkPath), + (err) => err instanceof Error && err.message.includes(symlinkPath), + 'cleanup must throw and name the symlink path when it resolves outside os.tmpdir()' + ); + assert.strictEqual(fs.existsSync(victim), true, 'victim directory must survive the refusal'); + assert.strictEqual( + fs.existsSync(path.join(victim, 'marker.txt')), + true, + 'victim contents must survive the refusal' + ); + } +}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index cbe70d7e6..4d16f8cd0 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -244,30 +244,67 @@ function cleanup(tmpDir) { if (typeof tmpDir !== 'string' || tmpDir.length === 0) return; const target = path.resolve(tmpDir); const cwd = path.resolve(process.cwd()); - // isTmpPath was previously computed only inside the catch block below, so it - // classified a transient Windows error but was never consulted by the + // The temp-root check below was previously done only inside the catch block, + // so it classified a transient Windows error but was never consulted by the // destructive rmSync call itself — a wrong `target` would still chdir out of // its own tree and get force-deleted. Hoisted above both the chdir and the // rmSync so an out-of-tmpdir path is refused before either can run. // - // Do NOT realpath `target` itself: cleanup() is legitimately called on - // already-deleted or never-created dirs, and realpathSync throws ENOENT on - // a missing path. Comparison is case-insensitive on Windows (drive-letter - // and path casing vary there) and case-sensitive everywhere else; the - // error message below always prints the original-case target. + // `target` itself is only realpath'd below when it exists (fs.existsSync + // guard on the symlink-escape check) — realpathSync throws ENOENT on a + // missing path, and cleanup() is legitimately called on already-deleted or + // never-created dirs. Comparison is case-insensitive on Windows + // (drive-letter and path casing vary there) and case-sensitive everywhere + // else; the error message below always prints the original-case target. const isWindows = process.platform === 'win32'; const tmpRoots = tmpRootCandidates(); - const targetForCompare = isWindows ? target.toLowerCase() : target; - const isTmpPath = tmpRoots.some((root) => { - const rootForCompare = isWindows ? root.toLowerCase() : root; - return ( - targetForCompare === rootForCompare || - targetForCompare.startsWith(`${rootForCompare}${path.sep}`) - ); - }); - if (!isTmpPath) { + // Shared by the literal-target check and the symlink-realpath check below + // so the two cannot drift apart — a second, independently written copy of + // this comparison is how the matching precondition in the tests came to + // disagree with the guard it was meant to mirror. + function isUnderRoots(p) { + const pForCompare = isWindows ? p.toLowerCase() : p; + return tmpRoots.some((root) => { + const rootForCompare = isWindows ? root.toLowerCase() : root; + return ( + pForCompare === rootForCompare || + pForCompare.startsWith(`${rootForCompare}${path.sep}`) + ); + }); + } + if (!isUnderRoots(target)) { throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`); } + // Symlink-escape guard: the check above is a string prefix test on `target` + // alone, so a symlink physically located under tmpdir but pointing OUTSIDE + // it would pass that check while rmSync follows the link and deletes the + // real, out-of-tree directory. When the target exists, also resolve its + // real path and require that to be under a root too. + // + // Only when it exists: cleanup() is legitimately called on already-deleted + // or never-created dirs, and fs.realpathSync throws ENOENT on those — a + // missing target is harmless anyway since rmSync({force: true}) no-ops on it. + // + // Any realpath error (including a non-ENOENT failure) fails CLOSED: refuse + // rather than silently proceed. This is a safety check, not a best-effort + // probe — an unreadable/looping symlink must not be treated as safe. + if (fs.existsSync(target)) { + let real; + try { + real = fs.realpathSync(target); + } catch (error) { + throw new Error( + `cleanup() refused to remove a path it could not verify via realpath ` + + `(${(error && error.code) || (error && error.message) || 'unknown error'}): ${target}` + ); + } + if (!isUnderRoots(real)) { + throw new Error( + `cleanup() refused to remove a path that resolves via symlink to a ` + + `location outside os.tmpdir(): ${target} -> ${real}` + ); + } + } if (cwd === target || cwd.startsWith(`${target}${path.sep}`)) { // Windows cannot remove a directory that is the current working directory. process.chdir(path.dirname(target)); @@ -282,8 +319,9 @@ function cleanup(tmpDir) { } catch (error) { // After retries, Windows can still briefly hold temp dirs open after a timed-out // child exits. Ignore that teardown-only flake for temp roots, but rethrow everything else. + // By this point target is guaranteed under a temp root: the guard clauses above throw + // for any other path, so this swallow doesn't need to re-test that. const isTransientWinErr = process.platform === 'win32' - && isTmpPath && ['EBUSY', 'ENOTEMPTY', 'EPERM'].includes(error && error.code); if (!isTransientWinErr) throw error; } @@ -693,4 +731,4 @@ function clearSessionEnv() { for (const k of SESSION_ENV_KEYS) delete process.env[k]; } -module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, readFileNormalized, readWorkflowCombined, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, absPlanningPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, SESSION_ENV_KEYS, saveSessionEnv, restoreSessionEnv, clearSessionEnv, TOOLS_PATH }; +module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, tmpRootCandidates, readFileNormalized, readWorkflowCombined, parseFrontmatter, isUsageOutput, captureConsole, toPosixPath, absPlanningPath, runNpm, isolatedNpmEnv, withIsolatedProcessState, delay, waitFor, resetRuntimeWarningCaches, SESSION_ENV_KEYS, saveSessionEnv, restoreSessionEnv, clearSessionEnv, TOOLS_PATH };