From 44cc2064931e8e3e72b825f2b73f2dcee2632663 Mon Sep 17 00:00:00 2001 From: Yigtwxx Date: Sun, 2 Aug 2026 10:29:17 +0300 Subject: [PATCH 1/2] fix(root): report documented remove() failure codes Root.remove() routed every failure through normalizePinnedPathError, which rewrites any non-FsSafeError into path-alias "path is not under root". A missing target and a non-empty directory were both reported as boundary violations, and the documented not-empty and not-removable codes were never constructed anywhere in the package. Map the syscall failures at the remove call site instead: ENOENT/ENOTDIR to not-found, ENOTEMPTY/EEXIST to not-empty, and any other errno to not-removable. Directory guard failures are already FsSafeError instances, so path-mismatch and not-file pass through untouched. --- CHANGELOG.md | 4 ++++ src/root-errors.ts | 21 ++++++++++++++++++++- src/root-impl.ts | 3 ++- test/edge-coverage.test.ts | 22 ++++++++++++++++++++++ test/fs-safe.test.ts | 16 ++++++++++++++++ 5 files changed, 64 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45dae4f..8b972b3 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`. + ## 0.5.1 - 2026-08-01 ### Security and Correctness diff --git a/src/root-errors.ts b/src/root-errors.ts index 355efb6..4c9302c 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,20 @@ export function normalizePinnedPathError(error: unknown): Error { cause: error instanceof Error ? error : undefined, }); } + +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..340c4be 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -47,6 +47,7 @@ import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemovePathError, } from "./root-errors.js"; import { getFsSafeTestHooks } from "./test-hooks.js"; import { stringifyJsonDocument } from "./json-stringify.js"; @@ -988,7 +989,7 @@ async function removePathInRoot( try { await removePathFallback(resolved); } catch (error) { - throw normalizePinnedPathError(error); + throw normalizeRemovePathError(error); } } diff --git a/test/edge-coverage.test.ts b/test/edge-coverage.test.ts index a411563..204270d 100644 --- a/test/edge-coverage.test.ts +++ b/test/edge-coverage.test.ts @@ -20,6 +20,7 @@ import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemovePathError, } from "../src/root-errors.js"; import { movePathToTrash } from "../src/trash.js"; @@ -60,6 +61,27 @@ 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" }); + }); }); 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"); From e96eb9c4b267f391c79855f0fec68b56517bfe45 Mon Sep 17 00:00:00 2001 From: Yigtwxx Date: Sun, 2 Aug 2026 20:56:00 +0300 Subject: [PATCH 2/2] fix(root): keep remove guard failures out of the errno mapping removePathFallback() creates and asserts the parent-directory guard before it touches the target, so wrapping the whole call in the remove normalizer turned a raw guard error such as ELOOP into not-removable even though no deletion was attempted. Scope the errno mapping to the deletion syscalls and give the guard stage its own normalizer: FsSafeError passes through, ENOENT/ENOTDIR still report not-found because the target is definitionally absent and nothing mutated, and every other raw error keeps the fail-closed path-alias contract. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ah1kwRo32A6sGU1tbMFEFk --- CHANGELOG.md | 2 +- src/root-errors.ts | 12 ++++++++++++ src/root-impl.ts | 24 +++++++++++++++++++----- test/edge-coverage.test.ts | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b972b3..4fe6163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ ### 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`. +- 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 diff --git a/src/root-errors.ts b/src/root-errors.ts index 4c9302c..e352a13 100644 --- a/src/root-errors.ts +++ b/src/root-errors.ts @@ -25,6 +25,18 @@ export function normalizePinnedPathError(error: unknown): Error { }); } +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; diff --git a/src/root-impl.ts b/src/root-impl.ts index 340c4be..5aa2b97 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -47,6 +47,7 @@ import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemoveGuardError, normalizeRemovePathError, } from "./root-errors.js"; import { getFsSafeTestHooks } from "./test-hooks.js"; @@ -989,7 +990,7 @@ async function removePathInRoot( try { await removePathFallback(resolved); } catch (error) { - throw normalizeRemovePathError(error); + throw normalizePinnedPathError(error); } } @@ -1325,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 204270d..a6c455b 100644 --- a/test/edge-coverage.test.ts +++ b/test/edge-coverage.test.ts @@ -16,12 +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(); @@ -82,6 +85,39 @@ describe("root error helpers", () => { 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", () => {