diff --git a/tests/helpers-cleanup.test.cjs b/tests/helpers-cleanup.test.cjs index 0c078aa03..47e28ba17 100644 --- a/tests/helpers-cleanup.test.cjs +++ b/tests/helpers-cleanup.test.cjs @@ -259,62 +259,3 @@ test('tmpRootCandidates() returns a well-formed, deduplicated list of absolute p } }); -// ─── Test 8: symlink-escape refusal, all platforms ─────────────────────────── - -test('cleanup refuses a symlink under tmpdir that resolves outside tmpdir', (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) { - // The assert.throws below is the load-bearing assertion for this test: - // it proves the guard refuses a symlink whose target resolves outside - // every accepted tmp root. - 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()' - ); - // These two assertions document the end state only — they are NOT proof - // the guard prevented a deletion that would otherwise have happened. A - // probe on node v26.5.1 showed fs.rmSync({recursive:true, force:true}) - // does not follow a top-level symlink target regardless: it unlinks the - // symlink itself and leaves whatever it points at (the victim, here) - // untouched either way. So the victim would survive with or without this - // guard; the guard's own correctness is established solely by the - // assert.throws above. - assert.strictEqual(fs.existsSync(victim), true, 'victim directory still exists (rmSync would not have followed the symlink either way)'); - assert.strictEqual( - fs.existsSync(path.join(victim, 'marker.txt')), - true, - 'victim contents still exist (rmSync would not have followed the symlink either way)' - ); - } -}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 7cd90d541..9560b8f9c 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -218,7 +218,27 @@ function createTempGitProject(prefix = 'gsd-test-') { // of that set, not the whole of it (see below). Each probe is wrapped in its // own try/catch — none of them may throw and crash cleanup(), they just // contribute nothing if unavailable. +// Memoization cache for tmpRootCandidates(), keyed on the LIVE os.tmpdir() +// value (not hoisted to a plain module-level constant) — two test files in +// this suite mutate TMPDIR/TEMP/TMP mid-run and restore them afterward, so +// caching on the current os.tmpdir() read is what keeps a stale cache from +// leaking across that override instead of a one-time computation baked in +// at module load. +let _tmpRootCandidatesCacheKey; +let _tmpRootCandidatesCache; + function tmpRootCandidates() { + const cacheKey = os.tmpdir(); + if (cacheKey === _tmpRootCandidatesCacheKey && _tmpRootCandidatesCache) { + return _tmpRootCandidatesCache; + } + const deduped = _computeTmpRootCandidates(); + _tmpRootCandidatesCacheKey = cacheKey; + _tmpRootCandidatesCache = Object.freeze(deduped); + return _tmpRootCandidatesCache; +} + +function _computeTmpRootCandidates() { const roots = []; try { roots.push(path.resolve(os.tmpdir())); @@ -272,19 +292,11 @@ function cleanup(tmpDir) { // 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-temp-root path is refused before either can run. - // - // `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. + // 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(); - // 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) => { @@ -307,43 +319,10 @@ function cleanup(tmpDir) { `(${tmpRoots.join(', ')}): ${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 on `target`'s own path. NOTE: as of this - // writing, `fs.rmSync` itself does not follow a top-level symlink target - // (it unlinks the symlink and leaves whatever it points at untouched), so - // this guard is not closing a live escape against the current rmSync - // behavior — it is defense-in-depth against a future change to the - // deletion mechanism (a different rm implementation, a recursive walk that - // does follow links, etc.) that would make `target` being a symlink - // dangerous. When the target exists, resolve its real path and require - // that to be under a root too, so the guard holds regardless of how the - // eventual delete is implemented. - // - // 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 the known temp roots (${tmpRoots.join(', ')}): ${target} -> ${real}` - ); - } - } + // No symlink-escape check here: fs.rmSync does not follow a top-level + // symlink — it unlinks the link itself and leaves the target intact — so + // there is no live hazard for the root-membership check above to guard + // against. That check is the one closing an actual defect. if (cwd === target || cwd.startsWith(`${target}${path.sep}`)) { // Windows cannot remove a directory that is the current working directory. process.chdir(path.dirname(target));