From 261ca3cbc0672d92bb64e819fd6b5fd9deaaa147 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 6 May 2026 23:05:13 +0100 Subject: [PATCH] fix(fs): preserve prune and trash fallback behavior --- CHANGELOG.md | 1 + src/file-store-prune.ts | 2 ++ src/root-impl.ts | 2 +- src/trash.ts | 44 +++++++++++++++++-------- test/edge-coverage.test.ts | 17 ++++++++++ test/platform-fallback-coverage.test.ts | 17 ++++++++++ 6 files changed, 69 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aaa7e9e..c1e6883 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - Centralize safe path segment validation, directory identity guards, and guarded mutation wrappers so future filesystem helpers reuse the same race-resistant checks. - Route archive ZIP staging, temp workspace sync reads, secret-file commits, and atomic move/replace fallbacks through shared pinned-read or guarded-write primitives without applying private-directory modes to public paths. - Close guarded fallback write handles without following path names if post-write directory verification fails, avoiding descriptor leaks and unsafe cleanup in symlink-swap races. +- Preserve empty-directory pruning and broken-symlink trash moves across guarded fallback paths. ### Tests diff --git a/src/file-store-prune.ts b/src/file-store-prune.ts index ddc51c4..3f43cf9 100644 --- a/src/file-store-prune.ts +++ b/src/file-store-prune.ts @@ -84,6 +84,8 @@ export async function pruneExpiredStoreEntries(params: { } if (shouldDescend && (await pruneDir(fullPath, relativePath, depth + 1))) { await assertRootGuard(); + // Keep empty-dir pruning on the same root-bounded remove path as files; + // the Root fallback handles empty directories without recursive delete. await scopedRoot.remove(relativePath).catch(() => undefined); } continue; diff --git a/src/root-impl.ts b/src/root-impl.ts index 27ce272..e820913 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -1379,7 +1379,7 @@ async function removePathFallback(resolved: { resolved: string }): Promise const guard = await createAsyncDirectoryGuard(path.dirname(resolved.resolved)); await getFsSafeTestHooks()?.beforeRootFallbackMutation?.("remove", resolved.resolved); await assertAsyncDirectoryGuard(guard); - await fs.rm(resolved.resolved); + await ((await fs.lstat(resolved.resolved)).isDirectory() ? fs.rmdir(resolved.resolved) : fs.rm(resolved.resolved)); await assertAsyncDirectoryGuard(guard).catch(() => undefined); } diff --git a/src/trash.ts b/src/trash.ts index 4dca17a..ff0dade 100644 --- a/src/trash.ts +++ b/src/trash.ts @@ -30,11 +30,14 @@ function isSameOrChildPath(candidate: string, parent: string): boolean { } function resolveAllowedTrashRoots(allowedRoots?: Iterable): string[] { - const roots = [...(allowedRoots ?? [os.homedir(), os.tmpdir()])].map((root) => { + const roots = [...(allowedRoots ?? [os.homedir(), os.tmpdir()])].flatMap((root) => { + const lexicalRoot = path.resolve(root); try { - return path.resolve(fs.realpathSync.native(root)); + // Keep both spellings: broken symlink targets cannot be realpathed and + // may only compare equal to the caller's lexical allowed root. + return [path.resolve(fs.realpathSync.native(root)), lexicalRoot]; } catch { - return path.resolve(root); + return [lexicalRoot]; } }); return [...new Set(roots)]; @@ -43,27 +46,39 @@ function resolveAllowedTrashRoots(allowedRoots?: Iterable): string[] { type TrashTargetGuard = { path: string; realPath: string; + realPathResolved: boolean; stat: fs.Stats; }; +function resolveTrashTargetPath(targetPath: string): { path: string; resolved: boolean } { + try { + return { path: path.resolve(fs.realpathSync.native(targetPath)), resolved: true }; + } catch { + // Broken symlinks are valid trash targets. Fall back to the lexical path, + // then rely on lstat identity so the move renames the symlink itself. + return { path: path.resolve(targetPath), resolved: false }; + } +} + function assertAllowedTrashTarget( targetPath: string, allowedRoots?: Iterable, ): TrashTargetGuard { - let resolvedTargetPath = path.resolve(targetPath); - const stat = fs.lstatSync(resolvedTargetPath); - try { - resolvedTargetPath = path.resolve(fs.realpathSync.native(targetPath)); - } catch { - // The subsequent move will surface missing or inaccessible targets. - } + const stat = fs.lstatSync(path.resolve(targetPath)); + const resolvedTarget = resolveTrashTargetPath(targetPath); + const resolvedTargetPath = resolvedTarget.path; const isAllowed = resolveAllowedTrashRoots(allowedRoots).some( (root) => resolvedTargetPath !== root && isSameOrChildPath(resolvedTargetPath, root), ); if (!isAllowed) { throw new Error(`Refusing to trash path outside allowed roots: ${targetPath}`); } - return { path: path.resolve(targetPath), realPath: resolvedTargetPath, stat }; + return { + path: path.resolve(targetPath), + realPath: resolvedTargetPath, + realPathResolved: resolvedTarget.resolved, + stat, + }; } function assertTrashTargetGuard(guard: TrashTargetGuard): void { @@ -71,8 +86,11 @@ function assertTrashTargetGuard(guard: TrashTargetGuard): void { if (!sameFileIdentity(stat, guard.stat)) { throw new Error(`Refusing to trash path after it changed: ${guard.path}`); } - const realPath = path.resolve(fs.realpathSync.native(guard.path)); - if (realPath !== guard.realPath) { + const current = resolveTrashTargetPath(guard.path); + if (guard.realPathResolved && (!current.resolved || current.path !== guard.realPath)) { + throw new Error(`Refusing to trash path after it changed: ${guard.path}`); + } + if (!guard.realPathResolved && current.resolved) { throw new Error(`Refusing to trash path after it changed: ${guard.path}`); } } diff --git a/test/edge-coverage.test.ts b/test/edge-coverage.test.ts index 537718b..a411563 100644 --- a/test/edge-coverage.test.ts +++ b/test/edge-coverage.test.ts @@ -229,4 +229,21 @@ describe("trash edge paths", () => { await fs.rm(path.dirname(copiedDest), { recursive: true, force: true }); } }); + + it.runIf(process.platform !== "win32")("moves broken symlinks to trash", async () => { + const root = await tempRoot("fs-safe-trash-broken-link-"); + const linkPath = path.join(root, "broken-link"); + const missingTarget = path.join(root, "missing-target"); + await fs.symlink(missingTarget, linkPath); + + const dest = await movePathToTrash(linkPath, { allowedRoots: [root] }); + try { + // Broken links cannot be realpathed; the guard keeps lstat identity and + // renames the link itself instead of requiring the target to exist. + await expect(fs.readlink(dest)).resolves.toBe(missingTarget); + await expect(fs.lstat(linkPath)).rejects.toMatchObject({ code: "ENOENT" }); + } finally { + await fs.rm(path.dirname(dest), { recursive: true, force: true }); + } + }); }); diff --git a/test/platform-fallback-coverage.test.ts b/test/platform-fallback-coverage.test.ts index ec7b302..c4cf2c9 100644 --- a/test/platform-fallback-coverage.test.ts +++ b/test/platform-fallback-coverage.test.ts @@ -72,4 +72,21 @@ describe("platform fallback coverage", () => { code: "ENOENT", }); }); + + it("prunes empty directories through the Windows remove fallback", async () => { + await importRootForPlatform("win32"); + const { fileStore } = await import("../src/file-store.js"); + const rootDir = await tempRoot("fs-safe-win-prune-"); + const store = fileStore({ rootDir }); + const stalePath = path.join(rootDir, "old", "stale.txt"); + await fs.mkdir(path.dirname(stalePath), { recursive: true }); + await fs.writeFile(stalePath, "stale", "utf8"); + await fs.utimes(stalePath, new Date(0), new Date(0)); + + await store.pruneExpired({ ttlMs: 1, recursive: true, pruneEmptyDirs: true }); + + // Root.remove's Node fallback must use rmdir for empty directories; fs.rm + // without recursive rejects dirs and would silently leave pruneEmptyDirs work. + await expect(fs.stat(path.join(rootDir, "old"))).rejects.toMatchObject({ code: "ENOENT" }); + }); });