diff --git a/CHANGELOG.md b/CHANGELOG.md index 45dae4f..a5906fc 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 + +- Reject drive-relative paths such as `C:secret.txt` with `invalid-path` on every `Root` method and on the `fileStore()` keys that delegate to them, and reject drive-relative segments such as `a/C:b` in the resolving path parser. `path.win32.isAbsolute()` reports these as relative, so on Windows `path.resolve()` consumed the drive prefix and silently aliased them onto the plain in-root path, letting two distinct keys resolve to the same file. Drive-absolute inputs such as `C:\root\file.txt` keep working. + ## 0.5.1 - 2026-08-01 ### Security and Correctness diff --git a/docs/errors.md b/docs/errors.md index 3235ede..822d133 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -92,7 +92,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, or a drive-relative path such as `C:name`. | | `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/root.md b/docs/root.md index b985736..63bb96e 100644 --- a/docs/root.md +++ b/docs/root.md @@ -151,7 +151,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 and drive-relative spellings 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/path.ts b/src/path.ts index 82505ae..698cbfa 100644 --- a/src/path.ts +++ b/src/path.ts @@ -12,6 +12,10 @@ export { type UnsafeDeviceReadPathReason, } from "./device-path.js"; +// Windows treats "C:name" as relative to the drive's own working directory, so +// path.win32.isAbsolute() reports false while path.resolve() still consumes the +// prefix. Reject the spelling outright rather than let it alias a plain segment. +const DRIVE_RELATIVE_PREFIX = /^[A-Za-z]:(?![\\/])/; const NOT_FOUND_CODES = new Set(["ENOENT", "ENOTDIR"]); const SYMLINK_OPEN_CODES = new Set(["ELOOP", "EINVAL", "ENOTSUP"]); const POSIX_SEPARATOR_CHAR_CODE = 0x2f; @@ -135,6 +139,10 @@ export function safeStatSync(targetPath: string): fs.Stats | null { } } +export function isDriveRelativePath(value: string): boolean { + return DRIVE_RELATIVE_PREFIX.test(value); +} + export function splitSafeRelativePath(relativePath: string): string[] { if (relativePath.length === 0 || relativePath === ".") { return []; @@ -156,6 +164,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..d8c357c 100644 --- a/src/root-context.ts +++ b/src/root-context.ts @@ -3,7 +3,12 @@ import os from "node:os"; import path from "node:path"; import { FsSafeError } from "./errors.js"; import { expandHomePrefix } from "./home-dir.js"; -import { assertNoNulPathInput, isNotFoundPathError, isPathInside } from "./path.js"; +import { + assertNoNulPathInput, + isDriveRelativePath, + isNotFoundPathError, + isPathInside, +} from "./path.js"; import { ROOT_PATH_ALIAS_POLICIES, resolveRootPath } from "./root-path.js"; export type RootContext = { @@ -17,6 +22,16 @@ export const ensureTrailingSep = (value: string) => export function assertValidRootRelativePath(relativePath: string): void { assertNoNulPathInput(relativePath, "relative path contains a NUL byte"); + if (isDriveRelativePath(relativePath)) { + throw new FsSafeError("invalid-path", "relative path must not start with a drive letter"); + } +} + +export function assertValidRootPathInput(filePath: string): void { + assertNoNulPathInput(filePath, "file path contains a NUL byte"); + if (isDriveRelativePath(filePath)) { + throw new FsSafeError("invalid-path", "file path must not start with a drive letter"); + } } let cachedHomePath: { raw: string; real: string } | undefined; diff --git a/src/root-impl.ts b/src/root-impl.ts index 87665fc..c77b885 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 { + assertValidRootPathInput, assertValidRootRelativePath, ensureTrailingSep, expandRelativePathWithHome, @@ -683,6 +684,7 @@ async function readPathInRoot( symlinks?: SymlinkPolicy; }, ): Promise { + assertValidRootPathInput(params.filePath); const rootDir = root.rootDir; const candidatePath = path.isAbsolute(params.filePath) ? path.resolve(params.filePath) 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..ce60e93 100644 --- a/test/windows-path.test.ts +++ b/test/windows-path.test.ts @@ -1,6 +1,16 @@ +import { mkdtemp, readFile, realpath, rm } 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 { + isDriveRelativePath, + isPathInside, + normalizeWindowsPathForComparison, + splitSafeRelativePath, +} from "../src/path.js"; +import { root as openRoot } from "../src/root.js"; describe("Windows path classification", () => { it("distinguishes extended local paths from UNC paths", () => { @@ -33,6 +43,104 @@ describe("Windows comparison normalization", () => { }); }); +describe("drive-relative relative paths", () => { + it("rejects drive-letter segments that Windows would resolve away", () => { + const inputs = ["C:evil", "c:evil", "C:", "C:..", "C:evil/sub", "a/C:b", "./C:evil", "a/D:b"]; + for (const input of inputs) { + 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 a drive-relative key on every root and store entry point", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "fs-safe-drive-relative-delegates-")); + try { + const store = fileStore({ rootDir }); + await store.writeText("secret.txt", "real"); + const root = await openRoot(rootDir); + const key = "C:secret.txt"; + + const calls: Array<[string, () => Promise]> = [ + ["root.read", () => root.read(key)], + ["root.readText", () => root.readText(key)], + ["root.open", () => root.open(key)], + ["root.stat", () => root.stat(key)], + ["root.exists", () => root.exists(key)], + ["root.list", () => root.list(key)], + ["root.remove", () => root.remove(key)], + ["root.write", () => root.write(key, "aliased")], + ["root.move", () => root.move(key, "moved.txt")], + ["root.readAbsolute", () => root.readAbsolute(key)], + ["root.reader", () => root.reader()(key)], + ["store.readText", () => store.readText(key)], + ["store.open", () => store.open(key)], + ["store.exists", () => store.exists(key)], + ["store.remove", () => store.remove(key)], + ]; + for (const [label, call] of calls) { + await expect(call(), label).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 }); + } + }); + + 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);