From cc0a4685f14b3b7d9c7076383509b031e148935d Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 5 Aug 2026 19:51:11 -0400 Subject: [PATCH] test(#3090): close the symlink escape, and stop the test from mirroring the guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An isolated review found the safety precondition was not safe. Test 4 uses the repo's own tests/ directory as a path that must never be deleted, and asserted beforehand that it sits outside the temp root — but it computed that root as path.resolve(os.tmpdir()) alone, while cleanup() accepts a target under any of several root spellings. For a checkout under the realpath'd temp root the precondition reads "outside, safe to proceed" while the guard reads "inside, delete it", and rmSync runs on tests/ before the assertion fails. The commit that added it claimed it would fail loudly instead of destructively; it would have done the opposite. The fix is not a second root in the test. tmpRootCandidates() is exported and the precondition calls it, so there is one source of truth and nothing left to drift. A mirror was the defect, not its contents. The same review bounded what the refusal actually guarantees: the check is a string prefix test, so a symlink living under tmpdir but pointing outside it passes while rmSync follows the link and deletes the real directory. When the target exists its real path is now checked too, against the same predicate — factored into one function so the two comparisons cannot diverge the way the test's copy did. A realpath failure refuses rather than proceeds; a safety check that cannot verify must not report safe, which is the whole subject of this branch. Missing targets are skipped, since rmSync with force no-ops on them and realpathSync would only throw ENOENT. `const isTmpPath = true` is gone. It survived the previous commit as a way to keep the catch's `&& isTmpPath` reading as a real condition, but a constant dressed as a test states nothing; the guard clauses above throw, so the catch comment now says the invariant in words instead. The new coverage does not depend on the platform the bug lives on. The root list is asserted directly — non-empty, absolute, deduped, and containing a freshly created temp dir — and the symlink refusal runs everywhere, skipping only where symlink creation is unavailable. The previous realpath test was coverage-identical to the control on Linux, so the only lanes the matrix runs could not have verified the fix it was written for. Refs #3057 Co-Authored-By: Claude Opus 5 --- tests/helpers-cleanup.test.cjs | 123 ++++++++++++++++++++++++++++++--- tests/helpers.cjs | 74 +++++++++++++++----- 2 files changed, 171 insertions(+), 26 deletions(-) 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 };