test(#3090): compare both canonical forms of the temp root, not just the unresolved one
The guard added in the previous commit refused legitimate temp directories on macOS. os.tmpdir() returns a path under /var/folders/..., /var is a symlink to /private/var, and path.resolve() does not resolve symlinks. So a caller that passed the realpath'd form — via fs.realpathSync(), or via process.cwd() after chdir-ing into a temp dir, which returns the resolved path — produced /private/var/folders/... and failed a check written against /var/folders/... Measured on this machine before the fix: mkdtemp path /var/folders/.../probe-XXX allowed fs.realpathSync of the same dir /private/var/folders/.../probe-XXX REFUSED process.cwd() after chdir to it /private/var/folders/.../probe-XXX REFUSED The accepted-roots set is now built from both the resolved and realpath'd forms of os.tmpdir(), deduped — on Linux they are identical, so the set collapses to one entry and nothing changes there. realpathSync is wrapped because it throws if the root is momentarily missing, and a safety check must not become a new crash. The target itself is deliberately NOT realpath'd: cleanup() is called on already-deleted directories, where realpathSync raises ENOENT. The roots are computed per call rather than hoisted to module scope, because two test files override TMPDIR and a hoisted value would go stale for them. Worth naming: the remote matrix runs linux-node22 and linux-node24 only, and it passed 30544/30544 on the broken commit. os.tmpdir() is /tmp on Linux with no symlink indirection, so that matrix could not have caught this at any sample size. The regression test added here branches on whether realpath differs from the original path, so it exercises the real case on macOS and stays meaningful rather than vacuous on Linux. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -159,3 +159,29 @@ test('cleanup still removes a real os.tmpdir()-rooted directory (control)', () =
|
||||
|
||||
assert.strictEqual(fs.existsSync(dir), false, 'os.tmpdir()-rooted directory should be removed');
|
||||
});
|
||||
|
||||
// ─── Test 6: realpath'd os.tmpdir() form is not refused (regression) ────────
|
||||
|
||||
test('cleanup accepts a realpath()d temp dir even when it differs from the raw path (macOS /var -> /private/var)', (t) => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cleanup-realpath-'));
|
||||
const realPath = fs.realpathSync(dir);
|
||||
|
||||
if (realPath !== dir) {
|
||||
// macOS (and any other symlinked-tmpdir platform): the realpath'd form
|
||||
// diverges from the raw mkdtempSync() path. This is exactly the shape a
|
||||
// caller gets from fs.realpathSync() or from process.cwd() after
|
||||
// chdir-ing into a realpath'd dir — assert the guard does NOT refuse it.
|
||||
cleanup(realPath);
|
||||
assert.strictEqual(fs.existsSync(realPath), false, 'realpath()d form should be removed, not refused');
|
||||
assert.strictEqual(fs.existsSync(dir), false, 'raw path should also be gone (same directory)');
|
||||
} else {
|
||||
// Linux and any platform with no tmpdir symlink indirection: realpath()
|
||||
// equals the raw path, so this branch exercises the ordinary path and
|
||||
// keeps the test meaningful (non-vacuous) on both platforms.
|
||||
t.after(() => {
|
||||
if (fs.existsSync(dir)) cleanup(dir);
|
||||
});
|
||||
cleanup(dir);
|
||||
assert.strictEqual(fs.existsSync(dir), false, 'temp dir should be removed');
|
||||
}
|
||||
});
|
||||
|
||||
@@ -210,13 +210,32 @@ function cleanup(tmpDir) {
|
||||
if (typeof tmpDir !== 'string' || tmpDir.length === 0) return;
|
||||
const target = path.resolve(tmpDir);
|
||||
const cwd = path.resolve(process.cwd());
|
||||
const tmpRoot = path.resolve(os.tmpdir());
|
||||
// isTmpPath was previously computed only inside the catch block below, 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.
|
||||
const isTmpPath = target === tmpRoot || target.startsWith(`${tmpRoot}${path.sep}`);
|
||||
//
|
||||
// Two acceptable roots, not one: on macOS os.tmpdir() returns a path under
|
||||
// /var/folders/... but /var is a symlink to /private/var, and path.resolve()
|
||||
// does not resolve symlinks. A caller that passed the REALPATH'd form of a
|
||||
// temp dir (e.g. via fs.realpathSync(), or via process.cwd() after chdir-ing
|
||||
// into a realpath'd dir) would resolve to /private/var/folders/... and get
|
||||
// wrongly refused by a check against only path.resolve(os.tmpdir()). Build
|
||||
// the accepted-roots set from both the raw and realpath'd forms of
|
||||
// os.tmpdir() — realpathSync is wrapped in try/catch because it throws if
|
||||
// the temp root is momentarily missing, and this guard must never crash
|
||||
// cleanup() over that. Do NOT realpath `target` itself: cleanup() is
|
||||
// legitimately called on already-deleted or never-created dirs, and
|
||||
// realpathSync throws ENOENT on a missing path.
|
||||
const tmpRoots = [path.resolve(os.tmpdir())];
|
||||
try {
|
||||
const realTmpRoot = fs.realpathSync(os.tmpdir());
|
||||
if (!tmpRoots.includes(realTmpRoot)) tmpRoots.push(realTmpRoot);
|
||||
} catch (_) { /* temp root unreadable — fall back to the resolved form only */ }
|
||||
const isTmpPath = tmpRoots.some(
|
||||
(root) => target === root || target.startsWith(`${root}${path.sep}`)
|
||||
);
|
||||
if (!isTmpPath) {
|
||||
throw new Error(`cleanup() refused to remove a path outside os.tmpdir(): ${target}`);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user