diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a20e9c..9e25a92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Security and Correctness +- Reject drive-relative segments such as `C:secret.txt` and `a/C:b` in portable relative-path parsing and every `FileStore` key, and reject leading drive-relative spellings on `Root` destinations and `resolve()` with `invalid-path`. Existing-object Root operations, including reads, inspection, removal, and the source of `move()`, continue to accept legal POSIX filenames such as `c:notes.txt`; thanks @Yigtwxx for the fix. + - Apply atomic-replacement parent-directory modes through verified no-follow descriptors on POSIX, so a directory-entry swap cannot redirect `chmod` through a symlink to an unrelated directory. Windows keeps its explicit `mkdir(mode)`-only behavior because Node does not enforce POSIX directory modes there. - Retry a contended file-lock acquisition when Windows denies access to a lock file whose directory entry is still being torn down, including when the native binding performs the exclusive create. Both the exclusive create and the holder's snapshot read reported that transient `EPERM` as a hard failure, so concurrent `acquireFileLock()` calls failed intermittently on Windows even though the very next attempt would have succeeded. Retries stay bounded, so a genuine permission denial still surfaces as `EPERM` rather than a lock timeout; thanks @Yigtwxx for the fix. - Apply `replaceFileAtomic()` and `replaceFileAtomicSync()` modes through pinned temp-file descriptors before rename, and through pinned copy-fallback descriptors, so a post-rename symlink swap cannot redirect `chmod` to an unrelated file while exact modes remain independent of umask; thanks @yetval for reporting this (#86). diff --git a/docs/errors.md b/docs/errors.md index a21c47d..5796899 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -96,7 +96,7 @@ type FsSafeErrorCode = | `helper-failed` | A native mechanism or multi-step operational helper failed. | Inspect `cause` and any operation-specific `details`; retrying may be unsafe if the operation partially completed. | | `helper-unavailable` | Required native binding could not be loaded. | Unsupported platform, missing/incompatible bundled binary, or `FS_SAFE_NATIVE_MODE=require`. `auto` falls back where possible; `require` fails closed. | | `insecure-permissions` | A secure file or path permission check found a mode/ACL that allows broader access than requested. | File or directory is group/world writable/readable; Windows ACL grants broad read. | -| `invalid-path` | Input was empty, contained NUL, was an unparseable URL, or otherwise unusable. | Caller didn't validate input; input was a network path on Windows. | +| `invalid-path` | Input was empty, contained NUL, was an unparseable URL, or otherwise unusable. | Caller didn't validate input; input was a network path on Windows, a drive-relative segment in a portable relative path or store key, or a leading drive-relative spelling such as `C:name` used as a Root destination. Existing-object Root lookups retain legal POSIX drive-like names. | | `not-empty` | `remove()` on a non-empty directory. | Use `replaceDirectoryAtomic` or remove children first. | | `not-file` | Read or copy targeted a non-regular file. | Target was a directory, FIFO, socket, device. | | `not-found` | The target does not exist (or its parent does not, with `mkdir: false`). | Typical missing-file case. | diff --git a/docs/file-store.md b/docs/file-store.md index 319d333..afa4540 100644 --- a/docs/file-store.md +++ b/docs/file-store.md @@ -64,6 +64,13 @@ type FileStore = { `path()` returns the absolute path the store would use, after asserting it stays inside `rootDir`. Useful for logging or for handing to other libraries. +Every `relativePath` is a portable store key, including keys passed to reads, +`exists`, and `remove`. A segment with a Windows drive-relative spelling such +as `C:name` (including an embedded segment such as `a/C:name`) throws +`invalid-path` on every platform. This prevents a key created on POSIX from +aliasing a different file when the store is moved to Windows. Colons elsewhere, +such as the timestamp in `logs/2026-08-02T10:30:00Z.log`, remain valid. + `root()` returns a [`Root`](root.md) handle for the same directory when you need the full surface (move, list, mkdir). It's a fresh handle per call and is safe to call frequently. ## Writes diff --git a/docs/root.md b/docs/root.md index b985736..8d4f7aa 100644 --- a/docs/root.md +++ b/docs/root.md @@ -103,6 +103,14 @@ fs.ensureRoot(options?) // accepts "" / "." as the root itself `copyIn` is a one-shot ingest from a trusted absolute source path: it streams the source through the boundary, atomically renames into the root, and respects `maxBytes`. +Root operations that choose a new destination reject a leading Windows +drive-relative spelling such as `C:name` on every platform. This applies to +`write`, `create`, `append`, `openWritable`, `mkdir`, `copyIn`, and the +destination argument of `move`. In particular, `copyIn(path.basename(source), +source)` can reject a legal POSIX basename such as `c:photo.png`; callers that +derive portable destination names from host files must sanitize or map that +basename first. + `openWritable` opens a writable file with options `mode?: number` and `writeMode?: "replace" | "append" | "update"`. `replace` truncates existing files and is the default; `update` keeps existing contents. Use it for streaming output. Prefer `await using` for cleanup. All mutation methods accept `denyMutations?: { paths?: string[]; prefixes?: string[] }`. Entries must be absolute paths. `paths` blocks those exact paths; `prefixes` blocks those paths and their descendants. fs-safe preserves path strings exactly and canonicalizes through existing ancestors before comparing, so a symlinked ancestor to a denied location is still denied. Denied mutations throw `FsSafeError` with code `denied-path`. Use this for caller-specific sensitive paths, not as a replacement for the root boundary, symlink, or hardlink checks. @@ -119,6 +127,12 @@ fs.resolve(rel) // absolute path inside the root, after canonic These do not pin a later operation. They are safe to expose to UIs and decision points; for the actual read or write, use the verb methods so the operation pins identity at the point of use. +`resolve()` is the exception to the existing-object rule: because it selects a +location for later use, it rejects a leading drive-relative spelling. Reads, +`stat`, `exists`, `list`, `walk`, `remove`, and the source argument of `move` +accept an existing POSIX filename such as `c:notes.txt`. For `move`, only the +new destination name is subject to the portable guard. + ## Native helper mode Create-only writes prefer the bundled native helper for fd-relative opens and @@ -151,7 +165,7 @@ Every method throws `FsSafeError` with a `code`. Branch on `err.code`, not messa | Code | When it fires | |---|---| -| `invalid-path` | The input path is malformed, including embedded NUL bytes. | +| `invalid-path` | The input path is malformed, including embedded NUL bytes. Portable relative-path helpers and `FileStore` keys reject drive-relative segments; Root destination and resolution operations reject a leading drive-relative spelling such as `C:name`. | | `outside-workspace` | The input resolves outside the root, or contains a `..` segment that would escape it. | | `not-found` | The target does not exist (or its parent does not, with `mkdir: false`). | | `not-file` | A read or copy targeted a non-regular file (directory, FIFO, socket, …). | diff --git a/src/file-store.ts b/src/file-store.ts index 2b30f2f..fed810b 100644 --- a/src/file-store.ts +++ b/src/file-store.ts @@ -6,10 +6,7 @@ import type { Readable } from "node:stream"; import { readFileDescriptorBoundedSync } from "./bounded-read.js"; import { createSyncDirectoryGuard } from "./directory-guard.js"; import { FsSafeError } from "./errors.js"; -import { - pruneExpiredStoreEntries, - type FileStorePruneOptions, -} from "./file-store-prune.js"; +import { pruneExpiredStoreEntries, type FileStorePruneOptions } from "./file-store-prune.js"; export type { FileStorePruneOptions } from "./file-store-prune.js"; import { assertSyncDirectoryGuard, @@ -26,6 +23,7 @@ import { isPathInside, resolveSafeRelativePath } from "./path.js"; import { root, type OpenResult, type ReadResult, type Root, type RootReadOptions } from "./root.js"; import { DEFAULT_ROOT_MAX_BYTES } from "./root-impl.js"; import { matchRootFileOpenFailure, openRootFileSync, type RootFileOpenFailure } from "./root-file.js"; +import { assertNoDriveRelativePathSegments } from "./safe-path-segment.js"; import { writeSecretFileAtomic } from "./secret-file.js"; import { getFsSafeTestHooks } from "./test-hooks.js"; @@ -113,7 +111,7 @@ function assertRelativePath(relativePath: string): string { if (!raw) { throw new FsSafeError("invalid-path", "relative path must be non-empty"); } - return raw.replaceAll("\\", "/"); + return assertNoDriveRelativePathSegments(raw.replaceAll("\\", "/"), "store key"); } function resolveStorePath(rootDir: string, relativePath: string): string { diff --git a/src/path.ts b/src/path.ts index 82505ae..17296ed 100644 --- a/src/path.ts +++ b/src/path.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { FsSafeError } from "./errors.js"; +import { isDriveRelativePath } from "./safe-path-segment.js"; export { assertNoUnsafeDeviceReadPath, @@ -156,6 +157,9 @@ export function splitSafeRelativePath(relativePath: string): string[] { if (segment === "..") { throw new FsSafeError("invalid-path", "relative path must not contain '..'"); } + if (isDriveRelativePath(segment)) { + throw new FsSafeError("invalid-path", "relative path must not contain a drive letter"); + } } return segments; } diff --git a/src/root-context.ts b/src/root-context.ts index 14a07c9..ac1ba42 100644 --- a/src/root-context.ts +++ b/src/root-context.ts @@ -5,6 +5,7 @@ import { FsSafeError } from "./errors.js"; import { expandHomePrefix } from "./home-dir.js"; import { assertNoNulPathInput, isNotFoundPathError, isPathInside } from "./path.js"; import { ROOT_PATH_ALIAS_POLICIES, resolveRootPath } from "./root-path.js"; +import { isDriveRelativePath } from "./safe-path-segment.js"; export type RootContext = { rootDir: string; @@ -19,6 +20,13 @@ export function assertValidRootRelativePath(relativePath: string): void { assertNoNulPathInput(relativePath, "relative path contains a NUL byte"); } +export function assertValidRootDestinationPath(relativePath: string): void { + assertValidRootRelativePath(relativePath); + if (isDriveRelativePath(relativePath)) { + throw new FsSafeError("invalid-path", "relative path must not start with a drive letter"); + } +} + let cachedHomePath: { raw: string; real: string } | undefined; export async function expandRelativePathWithHome(relativePath: string): Promise { diff --git a/src/root-impl.ts b/src/root-impl.ts index 5aa2b97..48f4f2c 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -36,6 +36,7 @@ import { readOpenedFileSafely, type ReadResult } from "./read-opened-file.js"; import { pathStatFromStats } from "./path-stat.js"; import { resolveRootPath } from "./root-path.js"; import { + assertValidRootDestinationPath, assertValidRootRelativePath, ensureTrailingSep, expandRelativePathWithHome, @@ -364,6 +365,7 @@ class RootHandle implements Root { } async resolve(relativePath: string): Promise { + assertValidRootDestinationPath(relativePath); return ( await resolvePathInRoot(this.context, relativePath, { allowFinalSymlink: true }) ).resolved; @@ -428,6 +430,7 @@ class RootHandle implements Root { relativePath: string, options: RootOpenWritableOptions = {}, ): Promise { + assertValidRootDestinationPath(relativePath); const writeMode = options.writeMode ?? "replace"; return await openWritableFileInRoot(this.context, { relativePath, @@ -441,6 +444,7 @@ class RootHandle implements Root { } async append(relativePath: string, data: string | Buffer, options: RootAppendOptions = {}): Promise { + assertValidRootDestinationPath(relativePath); await appendFileInRoot(this.context, { relativePath, data, @@ -460,7 +464,7 @@ class RootHandle implements Root { } async mkdir(relativePath: string, options: RootMkdirOptions = {}): Promise { - assertValidRootRelativePath(relativePath); + assertValidRootDestinationPath(relativePath); await mkdirPathInRoot(this.context, { relativePath, denyMutations: mergeDenyMutationPolicies(this.defaults.denyMutations, options.denyMutations), @@ -480,6 +484,7 @@ class RootHandle implements Root { data: string | Buffer, options: RootWriteOptions = {}, ): Promise { + assertValidRootDestinationPath(relativePath); await writeFileInRoot(this.context, { relativePath, data, @@ -496,6 +501,7 @@ class RootHandle implements Root { data: string | Buffer, options: RootCreateOptions = {}, ): Promise { + assertValidRootDestinationPath(relativePath); await writeFileInRoot(this.context, { relativePath, data, @@ -532,7 +538,7 @@ class RootHandle implements Root { sourcePath: string, options: RootCopyOptions = {}, ): Promise { - assertValidRootRelativePath(relativePath); + assertValidRootDestinationPath(relativePath); await copyFileInRoot(this.context, { sourcePath, relativePath, @@ -579,7 +585,7 @@ class RootHandle implements Root { options: RootMoveOptions = {}, ): Promise { assertValidRootRelativePath(fromRelative); - assertValidRootRelativePath(toRelative); + assertValidRootDestinationPath(toRelative); validatePinnedOperationPayload({ from: fromRelative, to: toRelative }); const denyMutations = mergeDenyMutationPolicies( this.defaults.denyMutations, diff --git a/src/safe-path-segment.ts b/src/safe-path-segment.ts index 8c1f2a5..d0fb853 100644 --- a/src/safe-path-segment.ts +++ b/src/safe-path-segment.ts @@ -2,6 +2,9 @@ import { FsSafeError } from "./errors.js"; const SAFE_PATH_SEGMENT_PATTERN = /^[A-Za-z0-9_-][A-Za-z0-9._-]*$/; const SAFE_DOT_PREFIX_PATH_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/; +// Windows treats "C:name" as relative to the drive's current directory even +// though path.win32.isAbsolute() reports false. +const DRIVE_RELATIVE_PREFIX = /^[A-Za-z]:(?![\\/])/; const HYPHEN_CHAR_CODE = 0x2d; export type SafePathSegmentOptions = { @@ -9,6 +12,17 @@ export type SafePathSegmentOptions = { label?: string; }; +export function isDriveRelativePath(value: string): boolean { + return DRIVE_RELATIVE_PREFIX.test(value); +} + +export function assertNoDriveRelativePathSegments(value: string, label: string): string { + if (value.split("/").some(isDriveRelativePath)) { + throw new FsSafeError("invalid-path", `${label} must not contain a drive letter`); + } + return value; +} + function trimHyphenEdges(value: string): string { let start = 0; let end = value.length; diff --git a/test/api-coverage.test.ts b/test/api-coverage.test.ts index 4425d73..9f041bf 100644 --- a/test/api-coverage.test.ts +++ b/test/api-coverage.test.ts @@ -257,7 +257,7 @@ describe("path helpers", () => { expect(safeStatSync(path.join(root, "missing"))).toBeNull(); expect(() => assertNoNulPathInput("a\0b")).toThrow("NUL"); expect(splitSafeRelativePath("./a//b")).toEqual(["a", "b"]); - for (const bad of ["../x", "/x", "C:\\x", "a\\b", "a\0b"]) { + for (const bad of ["../x", "/x", "C:\\x", "C:evil", "C:..", "a/C:b", "a\\b", "a\0b"]) { expect(() => splitSafeRelativePath(bad)).toThrow(); } expect(resolveSafeRelativePath(root, "a/b")).toBe(path.join(root, "a", "b")); diff --git a/test/windows-path.test.ts b/test/windows-path.test.ts index cf5d627..117af58 100644 --- a/test/windows-path.test.ts +++ b/test/windows-path.test.ts @@ -1,6 +1,16 @@ +import { mkdir, mkdtemp, readFile, realpath, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { describe, expect, it } from "vitest"; +import { fileStore } from "../src/file-store.js"; import { isWindowsNetworkPath } from "../src/local-file-access.js"; -import { isPathInside, normalizeWindowsPathForComparison } from "../src/path.js"; +import { + isPathInside, + normalizeWindowsPathForComparison, + splitSafeRelativePath, +} from "../src/path.js"; +import { root as openRoot } from "../src/root.js"; +import { isDriveRelativePath } from "../src/safe-path-segment.js"; describe("Windows path classification", () => { it("distinguishes extended local paths from UNC paths", () => { @@ -33,6 +43,176 @@ describe("Windows comparison normalization", () => { }); }); +describe("drive-relative relative paths", () => { + const aliasingInputs = [ + "C:evil", + "c:evil", + "C:", + "C:..", + "C:evil/sub", + "a/C:b", + "./C:evil", + "a/D:b", + ]; + + it("rejects drive-letter segments that Windows would resolve away", () => { + for (const input of aliasingInputs) { + expect(() => splitSafeRelativePath(input)).toThrow("drive letter"); + } + }); + + it("separates drive-relative spellings from drive-absolute ones", () => { + for (const value of ["C:evil", "c:", "Z:.."]) { + expect(isDriveRelativePath(value), value).toBe(true); + } + for (const value of ["C:\\root\\file.txt", "C:/root/file.txt", "notes/c:file"]) { + expect(isDriveRelativePath(value), value).toBe(false); + } + }); + + it("keeps accepting ordinary segments that merely contain a colon", () => { + expect(splitSafeRelativePath("logs/2026-08-02T10:30:00Z.log")).toEqual([ + "logs", + "2026-08-02T10:30:00Z.log", + ]); + }); + + it("rejects drive-relative Root destinations without rejecting existing-object sources", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "fs-safe-drive-relative-root-")); + try { + const root = await openRoot(rootDir); + await root.write("source.txt", "source"); + const destination = "C:destination.txt"; + + const calls: Array<[string, () => Promise]> = [ + ["root.resolve", () => root.resolve(destination)], + ["root.openWritable", () => root.openWritable(destination)], + ["root.append", () => root.append(destination, "aliased")], + ["root.mkdir", () => root.mkdir(destination)], + ["root.write", () => root.write(destination, "aliased")], + ["root.create", () => root.create(destination, "aliased")], + ["root.copyIn", () => root.copyIn(destination, path.join(rootDir, "source.txt"))], + ["root.move destination", () => root.move("source.txt", destination)], + ]; + for (const [label, call] of calls) { + await expect(call(), label).rejects.toMatchObject({ code: "invalid-path" }); + } + + await expect(readFile(path.join(rootDir, "source.txt"), "utf8")).resolves.toBe("source"); + } finally { + await rm(rootDir, { force: true, recursive: true }); + } + }); + + it.skipIf(process.platform === "win32")( + "reads and inspects POSIX-legal drive-like filenames", + async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "fs-safe-drive-like-existing-")); + try { + await writeFile(path.join(rootDir, "c:notes.txt"), "notes"); + await writeFile(path.join(rootDir, "c:move.txt"), "move"); + await writeFile(path.join(rootDir, "c:remove.txt"), "remove"); + await mkdir(path.join(rootDir, "c:folder")); + await writeFile(path.join(rootDir, "c:folder", "entry.txt"), "entry"); + const root = await openRoot(rootDir); + + const opened = await root.open("c:notes.txt"); + await opened.handle.close(); + await expect(root.readText("c:notes.txt")).resolves.toBe("notes"); + await expect(root.readAbsolute("c:notes.txt")).resolves.toMatchObject({ + buffer: Buffer.from("notes"), + }); + await expect(root.reader()("c:notes.txt")).resolves.toEqual(Buffer.from("notes")); + await expect(root.stat("c:notes.txt")).resolves.toMatchObject({ isFile: true }); + await expect(root.exists("c:notes.txt")).resolves.toBe(true); + await expect(root.list("c:folder")).resolves.toEqual(["entry.txt"]); + + const walked: string[] = []; + for await (const entry of root.walk("c:folder", { symlinkPolicy: "skip" })) { + walked.push(entry.relativePath); + } + expect(walked).toEqual(["c:folder/entry.txt"]); + + await root.move("c:move.txt", "moved.txt"); + await expect(readFile(path.join(rootDir, "moved.txt"), "utf8")).resolves.toBe("move"); + await root.remove("c:remove.txt"); + await expect(root.exists("c:remove.txt")).resolves.toBe(false); + } finally { + await rm(rootDir, { force: true, recursive: true }); + } + }, + ); + + it("rejects drive-relative spellings on every FileStore key path", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "fs-safe-drive-relative-store-")); + try { + const store = fileStore({ rootDir }); + await store.writeText("source.txt", "source"); + for (const input of aliasingInputs) { + expect(() => store.path(input), input).toThrow("drive letter"); + } + expect(store.path("logs/2026-08-02T10:30:00Z.log")).toBe( + path.join(rootDir, "logs", "2026-08-02T10:30:00Z.log"), + ); + + const key = "C:source.txt"; + const calls: Array<[string, () => Promise]> = [ + ["store.open", () => store.open(key)], + ["store.readText", () => store.readText(key)], + ["store.exists", () => store.exists(key)], + ["store.remove", () => store.remove(key)], + ["store.writeText", () => store.writeText(key, "aliased")], + ["store.copyIn", () => store.copyIn(key, path.join(rootDir, "source.txt"))], + ]; + for (const [label, call] of calls) { + await expect(call(), label).rejects.toMatchObject({ code: "invalid-path" }); + } + expect(() => store.json(key)).toThrow("drive letter"); + await expect(readFile(path.join(rootDir, "source.txt"), "utf8")).resolves.toBe("source"); + } finally { + await rm(rootDir, { force: true, recursive: true }); + } + }); + + it("keeps accepting an absolute path that stays inside the root", async () => { + // The temp dir must be resolved: macOS hands back /var/... for a /private/var/... + // root and Windows CI hands back an 8.3 short name, either of which reads as + // outside the root once openRoot() resolves it. + const rootDir = await realpath( + await mkdtemp(path.join(os.tmpdir(), "fs-safe-drive-relative-absolute-")), + ); + try { + const root = await openRoot(rootDir); + await root.write("note.txt", "kept"); + + await expect(root.readText(path.join(rootDir, "note.txt"))).resolves.toBe("kept"); + await expect( + root.readAbsolute(path.join(rootDir, "note.txt")), + ).resolves.toMatchObject({ buffer: Buffer.from("kept") }); + await expect(root.reader()(path.join(rootDir, "note.txt"))).resolves.toEqual( + Buffer.from("kept"), + ); + } finally { + await rm(rootDir, { force: true, recursive: true }); + } + }); + + it("stops a drive-relative store key from aliasing a plain key", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "fs-safe-drive-relative-")); + try { + const store = fileStore({ rootDir }); + await store.writeText("secret.txt", "real"); + + await expect(store.writeText("C:secret.txt", "aliased")).rejects.toMatchObject({ + code: "invalid-path", + }); + await expect(readFile(path.join(rootDir, "secret.txt"), "utf8")).resolves.toBe("real"); + } finally { + await rm(rootDir, { force: true, recursive: true }); + } + }); +}); + describe.skipIf(process.platform !== "win32")("Windows containment with padded roots", () => { it("does not report a sibling directory as inside a space-padded root", () => { expect(isPathInside("C:\\root ", "C:\\root\\secret.txt")).toBe(false);