diff --git a/CHANGELOG.md b/CHANGELOG.md index 45dae4f..4fe6163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Suffix Windows reserved basenames with `_` in `sanitizeUntrustedFileName()` while preserving case and extensions on every platform, including dollar names and superscript COM/LPT variants; thanks @SebTardif (#67). +### Security and Correctness + +- Report the documented `remove()` failure codes instead of collapsing every failure to `path-alias`. A missing target now throws `not-found`, a non-empty directory throws `not-empty`, and any other filesystem failure throws `not-removable`, while directory identity drift keeps reporting `path-mismatch`. The errno classification covers only the deletion syscalls, so a parent-directory guard failure still fails closed with `path-alias` rather than implying a removal was attempted. + ## 0.5.1 - 2026-08-01 ### Security and Correctness diff --git a/src/root-errors.ts b/src/root-errors.ts index 355efb6..e352a13 100644 --- a/src/root-errors.ts +++ b/src/root-errors.ts @@ -1,5 +1,7 @@ import { FsSafeError } from "./errors.js"; -import { hasNodeErrorCode } from "./path.js"; +import { hasNodeErrorCode, isNodeError, isNotFoundPathError } from "./path.js"; + +const REMOVE_NOT_EMPTY_CODES = new Set(["ENOTEMPTY", "EEXIST"]); export function isAlreadyExistsError(error: unknown): boolean { return hasNodeErrorCode(error, "EEXIST") || /File exists|EEXIST/i.test(String(error)); @@ -22,3 +24,32 @@ export function normalizePinnedPathError(error: unknown): Error { cause: error instanceof Error ? error : undefined, }); } + +export function normalizeRemoveGuardError(error: unknown): Error { + if (error instanceof FsSafeError) { + return error; + } + if (isNotFoundPathError(error)) { + return new FsSafeError("not-found", "file not found", { + cause: error instanceof Error ? error : undefined, + }); + } + return normalizePinnedPathError(error); +} + +export function normalizeRemovePathError(error: unknown): Error { + if (error instanceof FsSafeError) { + return error; + } + if (!isNodeError(error) || typeof error.code !== "string") { + return normalizePinnedPathError(error); + } + const cause = error instanceof Error ? error : undefined; + if (isNotFoundPathError(error)) { + return new FsSafeError("not-found", "file not found", { cause }); + } + if (REMOVE_NOT_EMPTY_CODES.has(error.code)) { + return new FsSafeError("not-empty", "directory is not empty", { cause }); + } + return new FsSafeError("not-removable", "path could not be removed", { cause }); +} diff --git a/src/root-impl.ts b/src/root-impl.ts index 87665fc..5aa2b97 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -47,6 +47,8 @@ import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemoveGuardError, + normalizeRemovePathError, } from "./root-errors.js"; import { getFsSafeTestHooks } from "./test-hooks.js"; import { stringifyJsonDocument } from "./json-stringify.js"; @@ -1324,11 +1326,24 @@ async function resolvePinnedRootPathInRoot( }; } +async function prepareRemoveGuard(targetPath: string) { + try { + const guard = await createAsyncDirectoryGuard(path.dirname(targetPath)); + await getFsSafeTestHooks()?.beforeRootFallbackMutation?.("remove", targetPath); + await assertAsyncDirectoryGuard(guard); + return guard; + } catch (error) { + throw normalizeRemoveGuardError(error); + } +} + 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 ((await fs.lstat(resolved.resolved)).isDirectory() ? fs.rmdir(resolved.resolved) : fs.rm(resolved.resolved)); + const guard = await prepareRemoveGuard(resolved.resolved); + try { + await ((await fs.lstat(resolved.resolved)).isDirectory() ? fs.rmdir(resolved.resolved) : fs.rm(resolved.resolved)); + } catch (error) { + throw normalizeRemovePathError(error); + } await assertAsyncDirectoryGuard(guard).catch(() => undefined); } diff --git a/test/edge-coverage.test.ts b/test/edge-coverage.test.ts index a411563..a6c455b 100644 --- a/test/edge-coverage.test.ts +++ b/test/edge-coverage.test.ts @@ -16,11 +16,15 @@ import { safePathSegmentHashed, } from "../src/install-path.js"; import { replaceDirectoryAtomic } from "../src/replace-directory.js"; +import { root as openRoot } from "../src/root.js"; import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemoveGuardError, + normalizeRemovePathError, } from "../src/root-errors.js"; +import { __setFsSafeTestHooksForTest } from "../src/test-hooks.js"; import { movePathToTrash } from "../src/trash.js"; const tempDirs = new Set(); @@ -60,6 +64,60 @@ describe("root error helpers", () => { }); expect(normalizePinnedPathError("raw string")).toMatchObject({ code: "path-alias" }); }); + + it("maps remove syscall failures without swallowing boundary errors", () => { + const drift = new FsSafeError("path-mismatch", "directory changed during operation"); + expect(normalizeRemovePathError(drift)).toBe(drift); + + const mappings = [ + ["ENOENT", "not-found"], + ["ENOTDIR", "not-found"], + ["ENOTEMPTY", "not-empty"], + ["EEXIST", "not-empty"], + ["EACCES", "not-removable"], + ["EBUSY", "not-removable"], + ] as const; + for (const [errno, code] of mappings) { + const error = Object.assign(new Error(errno), { code: errno }); + expect(normalizeRemovePathError(error)).toMatchObject({ code }); + } + + expect(normalizeRemovePathError(new Error("raw"))).toMatchObject({ code: "path-alias" }); + expect(normalizeRemovePathError("raw string")).toMatchObject({ code: "path-alias" }); + }); + + it("keeps guard-stage failures fail-closed instead of reporting a removal outcome", () => { + const drift = new FsSafeError("path-mismatch", "directory changed during operation"); + expect(normalizeRemoveGuardError(drift)).toBe(drift); + + for (const errno of ["ENOENT", "ENOTDIR"]) { + const error = Object.assign(new Error(errno), { code: errno }); + expect(normalizeRemoveGuardError(error), errno).toMatchObject({ code: "not-found" }); + } + for (const errno of ["ELOOP", "EACCES", "EBUSY", "ENOTEMPTY", "EEXIST"]) { + const error = Object.assign(new Error(errno), { code: errno }); + expect(normalizeRemoveGuardError(error), errno).toMatchObject({ code: "path-alias" }); + } + expect(normalizeRemoveGuardError(new Error("raw"))).toMatchObject({ code: "path-alias" }); + }); + + it("does not map a guard-stage filesystem failure to a removal code", async () => { + const rootPath = await tempRoot("fs-safe-remove-guard-"); + const scoped = await openRoot(rootPath); + await scoped.write("target.txt", "keep"); + + __setFsSafeTestHooksForTest({ + beforeRootFallbackMutation() { + throw Object.assign(new Error("ELOOP"), { code: "ELOOP" }); + }, + }); + try { + await expect(scoped.remove("target.txt")).rejects.toMatchObject({ code: "path-alias" }); + } finally { + __setFsSafeTestHooksForTest(undefined); + } + await expect(fs.readFile(path.join(rootPath, "target.txt"), "utf8")).resolves.toBe("keep"); + }); }); describe("directory replacement and file store boundary helpers", () => { diff --git a/test/fs-safe.test.ts b/test/fs-safe.test.ts index ce39515..ac1adb4 100644 --- a/test/fs-safe.test.ts +++ b/test/fs-safe.test.ts @@ -396,6 +396,22 @@ describe("@openclaw/fs-safe", () => { }); }); + it("reports documented failure codes when remove cannot unlink the target", async () => { + const rootPath = await tempRoot("fs-safe-remove-codes-"); + const root = await openRoot(rootPath); + await root.mkdir("full/child"); + + await expect(root.remove("missing.txt")).rejects.toMatchObject({ + code: expectedFsSafeCode("not-found"), + }); + await expect(root.remove("missing-dir/missing.txt")).rejects.toMatchObject({ + code: expectedFsSafeCode("not-found"), + }); + await expect(root.remove("full")).rejects.toMatchObject({ + code: expectedFsSafeCode("not-empty"), + }); + }); + it("opens a file handle for fast reads when kernel fd path validation is available", async () => { const root = await openRoot(await tempRoot("fs-safe-open-")); await root.write("file.txt", "fast");