test(#3090): stop paying for the guard on every teardown
The Windows node-24 scoped-test job hit its 15-minute ceiling twice on this
branch and GitHub reported both as cancelled, which is how a job timeout
surfaces. The same job finishes in about two minutes twenty on next, across
four consecutive runs. cleanup() is the only hot-path change here.
It was doing up to seven filesystem calls per invocation: three probes of the
temp root, two more for each conventional temp dir, then an existence check and
a realpath of the target. On Windows fs.realpathSync.native opens a file handle
and Defender charges for each one, and this runs in the teardown of effectively
every test.
The root candidates are now memoized on the live os.tmpdir() value. The key
matters: two files in the suite override TMPDIR mid-run and restore it, so a
plain module-level hoist would go stale for them, while re-reading os.tmpdir()
costs an env lookup. Measured over 20,000 calls: 546ms unmemoized, 10ms
memoized.
The symlink-escape check is removed rather than optimized, because it was
guarding something that cannot happen. Verified directly on node v26.5.1:
fs.rmSync(link, {recursive: true, force: true})
link exists false
victim exists true
file exists true
rmSync unlinks a top-level symlink and leaves its target alone, and a symlink
nested inside a tree being recursively removed is also unlinked rather than
followed. The check cost two filesystem calls per teardown on the slowest
platform in the matrix and bought nothing. Its test asserted the victim
survived, which was true with or without the guard.
What closed the original defect is untouched: a target outside the known temp
roots is still refused, before the chdir and before the rmSync, with the roots
named in the message.
Refs #3057
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
sim
@@ -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)'
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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));
|
||||
|
||||
Reference in New Issue
Block a user