diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts index 875c7dcb9a..a8baab438b 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts @@ -54,6 +54,40 @@ function historyFilePath(storagePath: string, taskId: string): string { return path.join(storagePath, "tasks", taskId, GlobalFileNames.historyItem) } +// The delete locks the file it unlinks, and the spelling of that path is whatever the +// store was given: `delete` hands `deleteJsonFile` a joined path, `deleteJsonFile` hands it +// to `withFileLock`, and `withFileLock` only `path.resolve`s it -- the canonicalisation +// through fs/promises realpath happens deeper, when the lock file itself is placed, so it +// never reaches this spy. On the Windows runner the two spellings of one temp file differ +// (C:/Users/runneradmin/... and its NTFS 8.3 alias C:/Users/RUNNER~1/...), so comparing the +// key by text holds on a developer box and misses there. File identity is the comparison +// that holds on both spellings, which is how the landed +// src/utils/__tests__/safeWriteJson.interruption.test.ts handles the same hazard. +async function fileIdentity(candidate: string): Promise { + try { + const stats = await actualFs.stat(candidate) + return `${stats.dev}:${stats.ino}` + } catch { + return null + } +} + +async function lockKeyNamesTheSameFile(lockKey: unknown, expectedPath: string): Promise { + if (typeof lockKey !== "string") { + return false + } + // The unlink removes the file and leaves the per-task directory in place, so both sides + // still carry an identity here. Case is folded because it is not significant on Windows, + // where the alias spelling appears. + const expectedDirectory = await fileIdentity(path.dirname(expectedPath)) + const keyDirectory = await fileIdentity(path.dirname(lockKey)) + return ( + expectedDirectory !== null && + keyDirectory === expectedDirectory && + path.basename(lockKey).toLowerCase() === path.basename(expectedPath).toLowerCase() + ) +} + function storeInternals(store: TaskHistoryStore): { cache: Map taskFileMtimes: Map @@ -152,14 +186,23 @@ describe("TaskHistoryStore best-effort deletion semantics", () => { const store = createStore() await store.initialize() await store.upsert(makeHistoryItem({ id: "locked-delete" })) + // The lock spy is created once for the whole file, so clear it to make the + // assertion below about this delete and not about every case before it. + vi.mocked(withFileLock).mockClear() onWrite.mockClear() await expect(store.delete("locked-delete")).resolves.toBeUndefined() - expect(vi.mocked(withFileLock)).toHaveBeenCalledWith( - historyFilePath(storagePath, "locked-delete"), - expect.any(Function), + const lockCalls = vi.mocked(withFileLock).mock.calls + expect(lockCalls).toHaveLength(1) + // The lock is taken for the file the delete unlinked, under a function body: the key + // is pinned by identity rather than by text, and the arity and the body are pinned + // alongside it so an argument dropped or swapped cannot slip through. + expect(lockCalls[0]).toHaveLength(2) + expect(await lockKeyNamesTheSameFile(lockCalls[0][0], historyFilePath(storagePath, "locked-delete"))).toBe( + true, ) + expect(lockCalls[0][1]).toBeTypeOf("function") await expect(fs.access(historyFilePath(storagePath, "locked-delete"))).rejects.toMatchObject({ code: "ENOENT", }) diff --git a/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts new file mode 100644 index 0000000000..b965765ac9 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -0,0 +1,51 @@ +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" + +import { safeWriteText } from "../safeWriteText" + +// No fs mocks in this file: the point is to assert what a real filesystem ends up +// holding after a publish attempt, which the mocked spec cannot show. The failure is +// provoked with real filesystem semantics rather than with a stubbed call. +describe("safeWriteText against a real filesystem", () => { + let dir: string + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-text-int-")) + }) + + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }) + }) + + it("publishes the new bytes and leaves no staging or backup residue", async () => { + const targetPath = path.join(dir, "target.txt") + await fs.writeFile(targetPath, "old bytes") + + // No platform override: the real platform's own durability and ACL steps run. + // A failed icacls restore in a throwaway temp directory is reported, not thrown, + // so the publish still lands. + await safeWriteText(targetPath, "new bytes", { backup: true }) + + expect(await fs.readFile(targetPath, "utf8")).toBe("new bytes") + expect(await fs.readdir(dir)).toEqual(["target.txt"]) + }) + + it("leaves the target bytes untouched when the commit cannot replace it", async () => { + // A regular file cannot be renamed over a directory, so the commit rename itself fails on a + // real filesystem with no mocking at all. No backup is asked for: with backup:true the run + // stopped one step earlier, at the backup copy hitting the same EISDIR, and the commit rename + // this test is named for never ran. + const targetPath = path.join(dir, "target-dir") + await fs.mkdir(targetPath) + const inside = path.join(targetPath, "payload.txt") + await fs.writeFile(inside, "original bytes") + + await expect(safeWriteText(targetPath, "new data")).rejects.toThrow() + + // The directory and its content are exactly as they were, and no backup copy + // or staging directory was left behind next to them. + expect(await fs.readFile(inside, "utf8")).toBe("original bytes") + expect(await fs.readdir(dir)).toEqual(["target-dir"]) + }) +}) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts new file mode 100644 index 0000000000..5e1af96cd7 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,2082 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess, ExecFileException } from "child_process" +import * as path from "path" +import { userInfo } from "os" + +import { PostCommitDurabilityError, resolveLockKey, safeWriteText, StagingPathError } from "../safeWriteText" + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + copyFile: vi.fn(), + chmod: vi.fn(), + mkdir: vi.fn(), + access: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + rmdir: vi.fn(), + realpath: vi.fn(), + lstat: vi.fn(), + readlink: vi.fn(), +})) + +// Full mock for fs — all sync methods are vi.fn() stubs. Stats is a bare +// class stub so tests can build minimal Stats stand-ins via its prototype. +vi.mock("fs", () => ({ + openSync: vi.fn(), + writeSync: vi.fn(), + closeSync: vi.fn(), + mkdirSync: vi.fn(), + fsyncSync: vi.fn(), + chmodSync: vi.fn(), + fchmodSync: vi.fn(), + statSync: vi.fn(), + // readSync answers EOF by default: a row that does not stage backup bytes still + // finishes the copy loop instead of spinning on an undefined read. + readSync: vi.fn(() => 0), + Stats: class Stats {}, +})) + +// Mock os.userInfo so the staging grant asks the host for an account name on a fixed +// schedule rather than on whatever the host happens to report (same shape as the shell +// spec, which already mocks os this way). +vi.mock("os", () => ({ + userInfo: vi.fn(() => ({ username: "unit-test-user", uid: 0, gid: 0, homedir: "", shell: null })), +})) + +// Mock child_process.execFile (callback-based — must invoke callback to resolve) +vi.mock("child_process", () => ({ + execFile: vi.fn((cmd, args, opts, cb) => { + if (typeof cb === "function") cb(null) + }), +})) + +// Minimal stand-in for the ChildProcess that callback-form execFile returns. +const fakeChild = { kill: () => true } as unknown as ChildProcess + +// Helper that mirrors safeWriteText's path resolution exactly +function _resolvedTarget(filePath: string): string { + return path.resolve(filePath) +} +function _dirPath(filePath: string): string { + return path.dirname(_resolvedTarget(filePath)) +} +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +// Async lstat stand-in: the SUT only asks whether the path is a link or a file. +// Built on the Stats prototype so the mock value still satisfies fsSync.Stats. +function _fileStats(isLink: boolean): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + s.isSymbolicLink = () => isLink + s.isFile = () => !isLink + return s +} + +// fs.BigIntStats is a type-only export (fs.BigIntStats is undefined at runtime), so the stand-in is +// a Stats object carrying bigint ino/dev - exactly what fs.lstat(path, { bigint: true }) hands +// back at runtime. +function _fileStatsWithIdentity(ino: bigint, dev: bigint): fsSync.BigIntStats { + // Double assertion: the runtime value is the Stats stand-in, the type is the bigint variant. + const s = _fileStats(false) as unknown as fsSync.BigIntStats + s.ino = ino + s.dev = dev + return s +} + +function mockDefaults(): void { + vi.resetAllMocks() + // After resetAllMocks, vi.fn() returns undefined — restore promise defaults. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.rename).mockResolvedValue(undefined) + vi.mocked(fs.unlink).mockResolvedValue(undefined) + vi.mocked(fs.rmdir).mockResolvedValue(undefined) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Staged-file default: a regular file, not a link, so a caller-supplied + // tempPath passes the location and file-type check by default. + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + // The staging-directory comparison and canonicalDirKey both go through realpath. + // Identity is the honest default - it says "no component of this path is a symlink", + // which is what every row that does not stage an alias assumes. A row that models a + // symlinked ancestor overrides it. + vi.mocked(fs.realpath).mockImplementation(async (target) => String(target)) + // resetAllMocks above drops the factory implementation above, so restore the host account + // name this code may fall back to: every row that does not pin USERNAME gets the same + // answer on every host. + vi.mocked(userInfo).mockReturnValue({ username: "unit-test-user", uid: 0, gid: 0, homedir: "", shell: null }) +} +function _stats(mode: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, { mode }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + mockDefaults() + // Default sync-write behaviour: report that all requested bytes were + // written. The Buffer overload passes (fd, buffer, offset, length), + // so the fourth argument is the requested length. + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + }) + + describe("staging and cleanup", () => { + it("creates a temp file in the staging dir, fsyncs it, renames to target, and cleans up on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) // fd=1 + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(targetPath, "hello world", { platform: "linux" }) + + // staging dir was created with private permissions — use + // stringContaining to handle Windows path resolution + expect(fsSync.mkdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), { + recursive: true, + mode: 0o700, + }) + // a pre-existing staging dir is repaired to private permissions too + expect(fsSync.chmodSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), 0o700) + + // temp file was opened for writing with the existing target's mode + // (default 0o644 from the statSync default mock) + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + + // content was written as a buffer (partial-write loop, full write) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("hello world", "utf8"), 0, 11) + + // fsync (sync form) was called on the fd + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // file was closed + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + + // atomic rename happened — realpath mock returns targetPath, so that's the dest + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink of temp (it's now the committed file; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("removes the now-empty staging directory after a successful self-staged commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { platform: "linux" }) + + // the staging subdir is removed best-effort after the commit rename + // (stringContaining: the SUT and the test helper resolve Windows + // drive-relative paths differently, as in the existing staging tests) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + // the win32 DACL restore gate must stay closed on other platforms: + // no icacls save or restore is attempted + expect(execFile).not.toHaveBeenCalled() + }) + + it("still removes the staging directory when no options are supplied at all", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The staging-directory grant names the account it grants from the process + // environment, so this row pins the account rather than inheriting whatever the host + // happens to carry: with neither USERNAME nor USER, _grantPrivateDaclWindows takes its + // skip path and one of the three icacls calls never happens, which is a state a runner + // can reach and this row cannot. + vi.stubEnv("USERNAME", "staging-acl-user") + vi.stubEnv("USER", "") + + // options is undefined: the self-staged check and the optional-chained + // DACL runner lookup must not dereference it + await expect(safeWriteText(targetPath, "hello")).resolves.toBeUndefined() + + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + if (process.platform === "win32") { + // default platform is win32: the staging hardening grant, the target DACL save + // and the post-commit restore all ran through the default icacls path + // (options?.execFileRunner must not throw when options is undefined). Named by + // argument rather than by a bare count, so a missing step reports which step. + const icaclsCalls = vi.mocked(execFile).mock.calls.map(function (call) { + return (call[1] ?? []).map(String) + }) + const grants = icaclsCalls.filter(function (args) { + return args.includes("/inheritance:r") + }) + const saves = icaclsCalls.filter(function (args) { + return args.includes("/save") + }) + const restores = icaclsCalls.filter(function (args) { + return args.includes("/restore") + }) + expect(grants).toHaveLength(1) + expect(saves).toHaveLength(1) + expect(restores).toHaveLength(1) + expect(icaclsCalls).toHaveLength(3) + expect(grants[0]?.[3]).toBe("staging-acl-user:(OI)(CI)F") + expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + } + vi.unstubAllEnvs() + }) + it.skipIf(process.platform !== "win32")( + "skips the staging-directory grant and reports the leftover exposure when neither the environment nor the host resolves an account", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + const warnings: string[] = [] + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Both sources cleared: the environment carries neither USERNAME nor USER and the host + // cannot report an account either, which is what userInfo throwing means. The grant never + // runs, so the two calls left are the target DACL save and the post-commit restore. + vi.stubEnv("USERNAME", "") + vi.stubEnv("USER", "") + vi.mocked(userInfo).mockImplementation(() => { + throw new Error("EACCES: account entry unread") + }) + + await expect( + safeWriteText(targetPath, "hello", { + platform: "win32", + onWarning: (message: string) => warnings.push(message), + }), + ).resolves.toBeUndefined() + + const icaclsCalls = vi.mocked(execFile).mock.calls.map(function (call) { + return (call[1] ?? []).map(String) + }) + expect(icaclsCalls).toHaveLength(2) + expect( + icaclsCalls.filter(function (args) { + return args.includes("/inheritance:r") + }), + ).toEqual([]) + expect( + icaclsCalls.filter(function (args) { + return args.includes("/save") + }), + ).toHaveLength(1) + expect( + icaclsCalls.filter(function (args) { + return args.includes("/restore") + }), + ).toHaveLength(1) + expect( + warnings.some(function (message) { + return message.includes("may be readable by other accounts") + }), + ).toBe(true) + vi.unstubAllEnvs() + }, + ) + + it.skipIf(process.platform !== "win32")( + "names the account the host reports when the environment carries neither USERNAME nor USER", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.stubEnv("USERNAME", "") + vi.stubEnv("USER", "") + vi.mocked(userInfo).mockReturnValue({ + username: "host-reported-account", + uid: 0, + gid: 0, + homedir: "", + shell: null, + }) + + await expect(safeWriteText(targetPath, "hello", { platform: "win32" })).resolves.toBeUndefined() + + const grants = vi + .mocked(execFile) + .mock.calls.map(function (call) { + return (call[1] ?? []).map(String) + }) + .filter(function (args) { + return args.includes("/inheritance:r") + }) + expect(grants).toHaveLength(1) + expect(grants[0]?.[3]).toBe("host-reported-account:(OI)(CI)F") + vi.unstubAllEnvs() + }, + ) + + it("does not remove the staging directory when the caller supplies its own tempPath", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const callerTemp = "/tmp/test-dir/caller-staged.txt" + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "hello", { platform: "linux", tempPath: callerTemp }) + + // the caller owns its temp file's directory; safeWriteText must not + // rmdir a directory it did not create + expect(fs.rmdir).not.toHaveBeenCalled() + }) + + it("a failed staging-dir removal never fails the committed write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) + + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).resolves.toBeUndefined() + + // the commit rename still happened and the rmdir error was swallowed + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(fs.rmdir).toHaveBeenCalledTimes(1) + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("gives each self-staged write its own staging directory so a concurrent write cannot remove it", async () => { + const targetA = "/tmp/test-dir/target-a.txt" + const targetB = "/tmp/test-dir/target-b.txt" + vi.mocked(fs.realpath).mockImplementation((p) => Promise.resolve(p as string)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetA, "a", { platform: "linux" }) + await safeWriteText(targetB, "b", { platform: "linux" }) + + // Two self-staged writes in the same directory must not share one staging + // directory: the first write's best-effort rmdir would otherwise delete the + // directory the second write had created but not yet opened (ENOENT on openSync). + const created = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + const staging = created.filter((p) => p.includes(".file-safety-staging_")) + expect(staging).toHaveLength(2) + expect(staging[0]).not.toBe(staging[1]) + // Uniqueness comes from the documented name shape + // /.file-safety-staging__: pinning the shape + // keeps the separator and the random suffix meaningful, not just the prefix. + for (const dir of staging) { + expect(dir).toMatch(/\.file-safety-staging_\d+_[a-z0-9]+$/) + } + const removed = vi.mocked(fs.rmdir).mock.calls.map((c) => String(c[0])) + expect(removed).toEqual([staging[0], staging[1]]) + }) + + it("removes its own staging directory when a self-staged write fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "hello", { platform: "linux" })).rejects.toThrow("EACCES") + + // The failed write's temp file is unlinked, then the directory it + // created is removed — a failed write must not leave an empty + // .file-safety-staging directory behind. + // mkdirSync created this write's staging directory; the temp file lives + // inside it, so the unlink targets a path under that directory. + const staging = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + expect(staging).toHaveLength(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(staging[0])) + expect(fs.rmdir).toHaveBeenCalledWith(staging[0]) + // The directory is only empty after its temp file is gone, so the + // unlink must happen before the rmdir. + expect(vi.mocked(fs.unlink).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fs.rmdir).mock.invocationCallOrder[0], + ) + }) + + it("does not remove a staging directory it did not create when a caller-staged write fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const callerTemp = "/tmp/test-dir/caller-staged.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect( + safeWriteText(targetPath, "hello", { platform: "linux", tempPath: callerTemp }), + ).rejects.toThrow("EACCES") + + // The caller owns that directory: only the caller's temp file is cleaned, + // never a rmdir of a directory safeWriteText never created. + expect(fs.unlink).toHaveBeenCalledWith(callerTemp) + expect(fs.rmdir).not.toHaveBeenCalled() + }) + }) + + it("win32: a rejecting async onWarning does not abort the write or leak an unhandled rejection", async () => { + // TypeScript accepts an async sink where a void callback is expected, so the + // wrapper has to attach a handler to the returned promise: an unhandled + // rejection can end the process under Node's default mode, after a write that + // already succeeded. + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + const consoleWarn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await expect( + safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: async () => { + throw new Error("async sink down") + }, + }), + ).resolves.toBeUndefined() + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + // The rejection is reported through the fallback sink rather than surfacing as an + // unhandled rejection. + expect(consoleWarn).toHaveBeenCalledWith(expect.stringContaining("onWarning callback rejected")) + consoleWarn.mockRestore() + }) + + // ── Test 2: fsync ordering ─────────────────────────────────────────────── + + describe("fsync ordering", () => { + it("calls fsync on the fd before close, and rename after close", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Verify call order: openSync(temp) → writeSync → fsyncSync(temp) + // → closeSync(temp) → rename. On POSIX the parent directory is then + // opened and fsynced after the commit rename, so openSync/fsyncSync/ + // closeSync each have a second (directory) call. + expect(vi.mocked(fsSync.openSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.writeSync).mock.calls.length).toBe(1) + expect(vi.mocked(fsSync.fsyncSync).mock.calls.length).toBe(2) + expect(vi.mocked(fsSync.closeSync).mock.calls.length).toBe(2) + + // the temp file was fully closed before the commit rename + expect(vi.mocked(fsSync.closeSync).mock.calls[0][0]).toBe(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // The title promises the order, so compare the invocations rather than + // only count them: a rename before closeSync, or a close before fsync, + // would not be a durable commit. + const fsyncOrder = vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0] + const closeOrder = vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0] + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] + expect(fsyncOrder).toBeLessThan(closeOrder) + expect(closeOrder).toBeLessThan(renameOrder) + }) + }) + + // ── Test 3: simulated failure between write and rename leaves target intact ── + + describe("crash/torn-write safety", () => { + it("simulated failure between fsync and rename leaves the target byte-identical and no temp left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { platform: "linux" })).rejects.toThrow("ENOSPC") + + // rename was attempted (the failure point) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // temp file was cleaned up on failure + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + + // backup was NOT created (backup:false by default), so target is untouched + // The only rename call was temp→target, not a rollback rename + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("a post-commit backup cleanup failure is non-fatal: the target stays committed and no temp is left behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The post-commit backup unlink (SUT step 6) fails — the write must + // still succeed; an orphaned backup is the documented acceptable + // outcome, so the failure is swallowed instead of rolling back. + vi.mocked(fs.unlink).mockRejectedValueOnce(new Error("EPERM")) + + await safeWriteText(targetPath, "data", { backup: true, platform: "linux" }) + + // the commit rename (temp -> target) still happened; it is the only rename + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // the failing cleanup was the post-commit backup unlink + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + + // the staging temp was already committed by the rename; nothing + // temp-shaped is unlinked afterwards + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("flushes the directory entry naming the backup before the commit rename so a later directory failure cannot strand the recovery copy", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Distinct fds so the recorded order can say which flush belongs to which file: + // 1 for the staged temp and the target read, 8 for the seeded backup, 7 for the + // parent directory. + vi.mocked(fsSync.openSync).mockImplementation((target) => { + const name = String(target) + if (name === dirPath) { + return 7 + } + if (name.includes("safeWriteText.bak_")) { + return 8 + } + return 1 + }) + const order: string[] = [] + vi.mocked(fsSync.fsyncSync).mockImplementation((fd) => { + order.push("fsync:" + String(fd)) + }) + vi.mocked(fs.rename).mockImplementation(async () => { + order.push("rename") + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // The backup is an entry inside dirPath. Before this change the only directory flush + // in the function ran after the commit rename, so a crash between the two could + // leave the rename durable while the name of the recovery copy was not - the one + // case where the backupPath handed to PostCommitDurabilityError points at a copy + // that no longer exists. The directory is now flushed before the rename, where a + // failure is still a pre-commit failure, and the flush after the rename stays + // because that rename changed a different entry in the same directory. + const backupFlush = order.indexOf("fsync:8") + const firstDirFlush = order.indexOf("fsync:7") + const renameAt = order.indexOf("rename") + expect(backupFlush).toBeGreaterThanOrEqual(0) + expect(firstDirFlush).toBeGreaterThan(backupFlush) + expect(firstDirFlush).toBeLessThan(renameAt) + expect(renameAt).toBeGreaterThanOrEqual(0) + expect(order.filter((entry) => entry === "fsync:7")).toHaveLength(2) + expect(order.lastIndexOf("fsync:7")).toBeGreaterThan(renameAt) + }) + + it("a failed directory flush for the backup entry is a pre-commit failure that removes the copy and never runs the rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockImplementation((target) => (String(target) === dirPath ? 7 : 1)) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd) => { + if (fd === 7) { + throw new Error("EIO: directory flush failed") + } + }) + + const error = await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + }).catch((caught: unknown) => caught) + + // The failure lands before the commit rename: the target still holds its previous + // content, the write is not reported as a post-commit durability failure, and the + // half-useful copy is removed instead of left beside a target whose writer was told + // nothing had failed. + expect(error).toBeInstanceOf(Error) + expect(String((error as Error).message)).toContain("EIO: directory flush failed") + expect(error).not.toBeInstanceOf(PostCommitDurabilityError) + expect(fs.rename).not.toHaveBeenCalled() + const backupUnlinks = vi + .mocked(fs.unlink) + .mock.calls.filter((call: unknown[]) => String(call[0]).includes("safeWriteText.bak_")) + expect(backupUnlinks).toHaveLength(1) + }) + + it("a failed post-commit directory fsync does not roll the backup back over the published content", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // The file fd opens normally. A backup:true write now opens the parent directory + // twice: once before the commit rename, to flush the entry that names the recovery + // copy, and once after it, to flush the entry the rename changed. This case is about + // the post-rename open, so the first directory open succeeds and the second fails, + // which is the post-commit durability failure. + let dirOpens = 0 + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) { + dirOpens += 1 + if (dirOpens > 1) { + throw new Error("EBADF") + } + return 1 + } + return 1 + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) + + // The commit rename already published the new content, and the backup was only + // ever a copy: the target was never moved, so there is nothing to rename back. + // The copy went through descriptors this code opened: the destination seeded + // private, the target opened read-only as the source, then the copy reopened for + // its flush - and never through fs.copyFile, which takes its destination mode + // from the source. + const backupOpens = vi.mocked(fsSync.openSync).mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.bak_") + }) + expect( + backupOpens.map(function (call) { + return call[1] + }), + ).toEqual(["wx", "r+"]) + expect(fsSync.openSync).toHaveBeenCalledWith(targetPath, "r") + expect(fs.copyFile).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // The durability failure is reported, not swallowed. The backup is NOT deleted: + // after the commit the target holds the NEW content, and this copy is the only + // one known to hold the previous content - the recovery copy the contract + // promises for exactly this path. + const backupUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call: unknown[]) { + return String(call[0]).includes("safeWriteText.bak_") + }) + expect(backupUnlinks).toEqual([]) + // The staged temp is still cleaned up. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("reports the retained backup path on a post-commit durability failure", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Two directory opens happen on this path now: one before the commit rename for the + // entry naming the backup, one after it for the entry the rename changed. This case + // wants the post-rename one to fail. + let dirOpens = 0 + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) { + dirOpens += 1 + if (dirOpens > 1) { + throw new Error("EBADF") + } + return 1 + } + return 1 + }) + + const error = await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + }).catch(function (caught: unknown) { + return caught + }) + + // The backup is kept on this path, and its name is a hidden random string. Without + // the field the caller is told a recovery copy exists and given no way to find it. + expect(error).toBeInstanceOf(PostCommitDurabilityError) + const retained = (error as PostCommitDurabilityError).backupPath + expect(typeof retained).toBe("string") + expect(String(retained)).toContain("safeWriteText.bak_") + // The reported path is exactly the copy that was made - the private descriptor + // this code opened and filled - so a caller can open it. + const seededBackups = vi + .mocked(fsSync.openSync) + .mock.calls.filter(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + .map(function (call) { + return String(call[0]) + }) + expect(seededBackups).toContain(retained) + }) + + it("reports a null backup path when no backup was taken", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) { + throw new Error("EBADF") + } + return 1 + }) + + const error = await safeWriteText(targetPath, "new data", { + platform: "linux", + }).catch(function (caught: unknown) { + return caught + }) + + expect(error).toBeInstanceOf(PostCommitDurabilityError) + expect((error as PostCommitDurabilityError).backupPath).toBeNull() + }) + + it("swallows an ENOENT from the backup-directory open as if the target itself were absent, and commits the rename anyway", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const dirPath = path.dirname(targetPath) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // A backup:true write opens dirPath twice: once before the commit rename, to flush + // the entry that names the recovery copy, and once after it, to flush the entry the + // rename changed. This row fails the FIRST of them with an error that carries + // code ENOENT, which is the shape a missing directory reports, and lets the + // post-rename open through so the row can tell the two apart afterwards. + let dirOpens = 0 + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) { + dirOpens += 1 + if (dirOpens === 1) { + throw Object.assign(new Error("ENOENT: no such file or directory, open '" + dirPath + "'"), { + code: "ENOENT", + }) + } + return 1 + } + return 1 + }) + + const error = await safeWriteText(targetPath, "new data", { + backup: true, + platform: "linux", + }).catch(function (caught: unknown) { + return caught + }) + + // What the shipped code actually does, measured rather than intended. The backup + // block's own catch removes the half-useful copy and rethrows, and the catch around + // that block tests only the errno - it cannot tell an ENOENT from opening dirPath + // apart from the ENOENT that means the target is not there yet. So the failure is + // swallowed, no recovery copy is made, and the write goes on to publish. + expect(error).toBeUndefined() + expect(dirOpens).toBe(2) + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // The partial copy is removed by the backup block's own catch, so the swallowing does + // not leave a hidden copy beside the published target. The staged temp is not + // unlinked here because on this path the commit rename above is what makes it the + // target: a successful publish leaves exactly one unlink behind, and it is the + // copy's. + const backupUnlinks = vi + .mocked(fs.unlink) + .mock.calls.filter((call: unknown[]) => String(call[0]).includes("safeWriteText.bak_")) + expect(backupUnlinks).toHaveLength(1) + expect(vi.mocked(fs.unlink).mock.calls).toHaveLength(1) + + // Whether this is the expectation the reviewer wants is a ruling this row does not + // make. The follow-up asked that the ENOENT exception be limited to the target + // existence check, which would turn the swallow above into a failure of the write. + // If that swallowing is itself the defect, then a green row here pins the shipped + // behaviour and not the asked-for one. + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics, copy-based ── + + describe("backup:true", () => { + it("copies target -> backup before commit without moving the target, deletes the copy on success", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // target was accessed (exists check) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // The backup is a copy: the canonical target is never moved away, so readers + // never see a missing file and no later step can clobber a concurrent publish. + // It is copied into a descriptor this code opened privately and read out of the + // target, so the copy never carries the target's own permissions. + expect(fsSync.openSync).toHaveBeenCalledWith(targetPath, "r") + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), "wx", 0o600) + expect(fs.copyFile).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + + // the only rename is the atomic commit temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // backup copy was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("a failed commit does not move the target, so nothing has to be rolled back", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The commit rename is the only rename in the flow and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // The target never left its path, so there is no restore rename and the + // pre-write content is still what a reader sees at targetPath - it was only ever + // read, through the descriptor the backup copy filled. + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fsSync.openSync).toHaveBeenCalledWith(targetPath, "r") + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), "wx", 0o600) + expect(fs.copyFile).not.toHaveBeenCalled() + + // Both the backup copy and the staging temp are cleaned up on failure. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("creates the backup privately before its content exists, then fsyncs it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Distinct descriptors: 7 is this write's backup copy, 9 is the target it is + // copied from, so each assertion can say which file the call touched. + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 9, + ) + // One 6-byte chunk, then EOF: the copy must drain the source and hand every byte + // to the descriptor it opened. + vi.mocked(fsSync.readSync).mockReturnValueOnce(6).mockReturnValue(0) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // The destination must exist with the private mode fixed at open time BEFORE any + // byte of the copy exists at it. "wx" also means a pre-existing path is never + // silently reused. + const seedOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + expect(seedOpen).toBeDefined() + expect(seedOpen?.[2]).toBe(0o600) + const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!) + ] + expect(seedOrder).toBeLessThan(vi.mocked(fsSync.readSync).mock.invocationCallOrder[0]) + + // Every byte went into the descriptor opened above, read out of the target. + expect(fsSync.readSync).toHaveBeenCalledWith(9, expect.any(Buffer), 0, expect.any(Number), 0) + expect(fsSync.writeSync).toHaveBeenCalledWith(7, expect.any(Buffer), 0, 6) + + // fs.copyFile picks its own destination mode and, on POSIX, re-applies the source's + // mode to the file it writes, so a copy through it would widen this private seed to + // the target's permissions for the whole copy window. Nothing has to be chmodded + // afterwards, because the mode this code chose never moved. + expect(fs.copyFile).not.toHaveBeenCalled() + expect(fs.chmod).not.toHaveBeenCalled() + + // The copy is then reopened for fsync with the writable flag, on the private mode + // it has held since the first byte. + const backupOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" + }) + expect(backupOpen).toBeDefined() + expect(fsSync.fsyncSync).toHaveBeenCalledWith(7) + }) + + it("a failed backup flush is reported and leaves no partial backup behind", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The copy lands, but the fsync of the copy fails: the retained content is not + // known to be durable, so the write must not proceed on a half-written backup. + // The staged temp is fsynced earlier with a different handle, so target the + // backup's fd specifically. + vi.mocked(fsSync.openSync).mockImplementation((p: unknown) => + String(p).includes("safeWriteText.bak_") ? 7 : 1, + ) + vi.mocked(fsSync.fsyncSync).mockImplementation((fd: unknown) => { + if (fd === 7) { + throw new Error("EIO") + } + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the incomplete copy is removed rather than left + // next to the target looking like a usable backup. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("a backup copy that fails part-way is removed, not left as a usable-looking backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The destination is seeded with openSync("wx") BEFORE any content exists at it, + // and the copy then fails part-way: the name is there, holding a partial copy of + // nothing usable. Cleanup has to key off the attempt, not off a copy that + // succeeded, or this file outlives the failed write beside the target. + vi.mocked(fsSync.readSync).mockImplementation(() => { + throw Object.assign(new Error("EIO"), { code: "EIO" }) + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true, platform: "linux" })).rejects.toThrow( + "EIO", + ) + + // Nothing was published, and the half-written copy is removed rather than left + // next to the target looking like a backup someone could restore. + expect(fs.rename).not.toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("backup:true when target does not exist: no backup created, just commit", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.access resolves for dirPath check, but rejects for target check (backup path) + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + }) + + await safeWriteText(targetPath, "new data", { backup: true, platform: "linux" }) + + // no backup rename (target didn't exist) + expect(fs.access).toHaveBeenCalledWith(targetPath) + + // only one rename: temp -> target + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + + // no unlink (no backup to delete; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + }) + it("keeps a multi-chunk backup complete when the destination write comes up short", async () => { + const targetPath = "/tmp/test-dir/target.txt" + // More than one 64 KiB chunk of distinct bytes, so a wrong source position or a + // wrong destination offset lands bytes that differ from the right ones. The period + // 251 does not divide the chunk size, which is what makes an offset error visible. + const source = Buffer.alloc(70 * 1024) + for (let i = 0; i < source.length; i++) { + source[i] = i % 251 + } + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.copyFile).mockResolvedValue(undefined) + // The descriptor the copy writes into is the seeded backup (7); the target is read + // through 9 and the staged temp through 1, so an assertion can name who got which + // byte instead of trusting the call order alone. + vi.mocked(fsSync.openSync).mockImplementation((...args: unknown[]) => { + const openedPath = String(args[0]) + if (openedPath.includes("safeWriteText.bak_")) { + return 7 + } + if (openedPath === targetPath) { + return 9 + } + return 1 + }) + // readSync answers from the recorded source instead of the default immediate EOF, + // so the production copy loop runs for two chunks rather than being short-circuited. + vi.mocked(fsSync.readSync).mockImplementation((...args: unknown[]) => { + const buffer = args[1] + if (!Buffer.isBuffer(buffer)) { + return 0 + } + const position = typeof args[4] === "number" ? args[4] : 0 + const offset = typeof args[2] === "number" ? args[2] : 0 + const length = typeof args[3] === "number" ? args[3] : 0 + const bytes = Math.min(length, Math.max(0, source.length - position)) + if (bytes === 0) { + return 0 + } + source.copy(buffer, offset, position, position + bytes) + return bytes + }) + const copiedChunks: Buffer[] = [] + let shortWriteLeft = 1 + // The seeded backup descriptor reports one short write, 40000 of the 65536 bytes + // asked for, and every other call reports the full request. + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => { + const fd = args[0] + const buffer = args[1] + const offset = typeof args[2] === "number" ? args[2] : 0 + const length = typeof args[3] === "number" ? args[3] : 0 + if (!Buffer.isBuffer(buffer) || fd !== 7) { + return length + } + const granted = shortWriteLeft > 0 && length > 40000 ? 40000 : length + if (granted < length) { + shortWriteLeft = 0 + } + copiedChunks.push(Buffer.from(buffer.subarray(offset, offset + granted))) + return granted + }) + + await expect(safeWriteText(targetPath, "new content", { backup: true })).resolves.toBeUndefined() + + const seedOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + expect(seedOpen?.[2]).toBe(0o600) + // The first chunk went in at offset 0 and the short write forced a second call at + // the offset the destination actually reached, with only the remainder asked for. + expect(fsSync.writeSync).toHaveBeenCalledWith(7, expect.any(Buffer), 0, 65536) + expect(fsSync.writeSync).toHaveBeenCalledWith(7, expect.any(Buffer), 40000, 65536 - 40000) + // The tail chunk landed in one call at offset 0 of a fresh buffer. + expect(fsSync.writeSync).toHaveBeenCalledWith(7, expect.any(Buffer), 0, 70 * 1024 - 65536) + const copied = Buffer.concat(copiedChunks) + expect(copied.length).toBe(source.length) + expect(copied.equals(source)).toBe(true) + expect(fsSync.closeSync).toHaveBeenCalledWith(7) + expect(fsSync.closeSync).toHaveBeenCalledWith(9) + // The loop under test is this code's own, not fs.copyFile's. + expect(fs.copyFile).not.toHaveBeenCalled() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + beforeEach(() => { + // The staging-directory hardening grants the private DACL to the account named in + // the process environment. Pin it so these rows ask for the same grant on a runner + // that reaches the win32 branch through the platform override as on a Windows host. + vi.stubEnv("USERNAME", "staging-acl-user") + }) + afterEach(() => { + vi.unstubAllEnvs() + }) + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // staging grant + DACL dump + DACL restore (execFile is the callback mock) + expect(execFile).toHaveBeenCalledTimes(3) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails — the callback-based mock must invoke cb with an error. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite icacls failure (fallback to plain rename) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + // Only the save was attempted: a failed DACL apply must not try to restore. The + // staging-directory grant is a separate call and is not part of this claim. + const attempts = vi + .mocked(execFile) + .mock.calls.filter(function (call) { + const args = (call[1] ?? []).map(String) + return args.includes("/save") || args.includes("/restore") + }) + .map(function (call) { + return String((call[1] ?? [])[1]) + }) + expect(attempts).toEqual(["/save"]) + }) + + it("win32: reports that access rights may change when the DACL cannot be saved", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + const warnings: string[] = [] + + await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) + + // The write still commits - a failing icacls must not leave the user unable to save - + // but the caller is told the replacement may not carry the old ACL. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(warnings.filter((m) => m.includes("different access rights"))).toHaveLength(1) + }) + + it("win32: reports when the target cannot be checked for DACL preservation", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The target exists but is not readable: that is not "absent", and skipping DACL + // preservation has to be visible. + vi.mocked(fs.access).mockImplementation(async (p) => { + if (String(p) === targetPath) { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }) + } + }) + const warnings: string[] = [] + + await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) + + // No save and no restore: the target could not be checked, so there was no DACL to + // carry over. (The staging-directory grant answers a different question.) + const daclCalls = vi.mocked(execFile).mock.calls.filter(function (call) { + const args = (call[1] ?? []).map(String) + return args.includes("/save") || args.includes("/restore") + }) + expect(daclCalls).toEqual([]) + expect(warnings.filter((m) => m.includes("Could not check"))).toHaveLength(1) + }) + + // Warning delivery is advisory: it must not be able to fail the save it is reporting on. + it("win32: a throwing onWarning does not abort the write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await expect( + safeWriteText(targetPath, "data", { + platform: "win32", + onWarning: () => { + throw new Error("callback down") + }, + }), + ).resolves.toBeUndefined() + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + }) + + it("win32 DACL: a partial dump left by a failed save is removed and never restored", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls save fails — a real icacls may have written a partial dump + // before erroring, so the dump path must be cleaned up and must never + // be used for a restore. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls save error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write committed; only the save was attempted (no restore from a failed dump) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + const saveCalls = vi.mocked(execFile).mock.calls.filter(function (call) { + return (call[1] ?? []).map(String).includes("/save") + }) + const restoreCalls = vi.mocked(execFile).mock.calls.filter(function (call) { + return (call[1] ?? []).map(String).includes("/restore") + }) + expect(saveCalls).toHaveLength(1) + // Nothing is restored from a dump whose save failed. + expect(restoreCalls).toEqual([]) + // the dump path (possibly partially created by icacls) was unlinked + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // three icacls runs: the staging-directory grant, then the save, then the restore + expect(execFile).toHaveBeenCalledTimes(3) + + // The save: DACL taken from the target before the backup copy + const firstCall = vi.mocked(execFile).mock.calls.find(function (call) { + return (call[1] ?? []).map(String).includes("/save") + })! + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining("safeWriteText.acl"), "/T"]) + + // The restore: DACL put back onto the directory after the commit rename + const secondCall = vi.mocked(execFile).mock.calls.find(function (call) { + return (call[1] ?? []).map(String).includes("/restore") + })! + expect(secondCall[0]).toBe("icacls") + expect(secondCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining("safeWriteText.acl"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + it("win32 DACL save runs before the backup copy, not after it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // The title is about order, so assert the order the mocks were actually + // called in. If the save ran after the backup copy the dump could describe a + // file that a concurrent publish had already replaced. + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + const execOrders = vi.mocked(execFile).mock.invocationCallOrder + const orderOfDacl = function (operation: string): number { + return execOrders[ + vi.mocked(execFile).mock.calls.findIndex(function (call) { + return (call[1] ?? []).map(String).includes(operation) + }) + ] + } + const saveCall = orderOfDacl("/save") + const restoreCall = orderOfDacl("/restore") + // The backup copy is the private descriptor this code opened and filled. + const backupOpens = vi.mocked(fsSync.openSync).mock.calls + const backupCopy = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ + backupOpens.findIndex(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "wx" + }) + ] + const commitRename = vi.mocked(fs.rename).mock.invocationCallOrder[0] + + expect(saveCall).toBeLessThan(backupCopy) + expect(backupCopy).toBeLessThan(commitRename) + expect(commitRename).toBeLessThan(restoreCall) + }) + + it("win32 DACL: a failed restore is reported and the dump is still unlinked", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + // Every icacls succeeds except the restore. + vi.mocked(execFile).mockImplementation((_cmd, args, _opts, cb) => { + const restoring = Array.isArray(args) && args.map(String).includes("/restore") + if (typeof cb === "function") { + cb(restoring ? new Error("icacls restore error") : null, "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // The content did commit: failing here would break every publish on a machine + // where icacls cannot reapply the saved ACEs. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // The changed access rights are reported instead of being swallowed. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("could not be restored")) + warnSpy.mockRestore() + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + }) + + it("win32 DACL: when target does not exist, no save/restore/dump", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // fs.access rejects for targetPath (ENOENT), but resolves for dirPath + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw { code: "ENOENT" } + return undefined + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // no save, no restore, no dump: the target was absent, so there was no DACL to + // carry over. The staging-directory grant answers a different question. + const daclCalls = vi.mocked(execFile).mock.calls.filter(function (call) { + const args = (call[1] ?? []).map(String) + return args.includes("/save") || args.includes("/restore") + }) + expect(daclCalls).toEqual([]) + + // no dump file created or unlinked + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("win32: holds the staging write until the staging-directory grant has returned", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Held as an array entry rather than a bare function value, because expect() on a + // function value in this suite only offers call matchers. + const held: Array<(error: ExecFileException | null, stdout: string, stderr: string) => void> = [] + vi.mocked(execFile).mockImplementation((_cmd, args, _opts, cb) => { + // Only the grant is held, so this row is about the privacy barrier rather than + // about icacls in general: the dump and the restore complete as soon as asked. + if ((args ?? []).map(String).includes("/inheritance:r")) { + if (typeof cb === "function") { + held.push(cb) + } + return fakeChild + } + if (typeof cb === "function") cb(null, "", "") + return fakeChild + }) + + const write = safeWriteText(targetPath, "data", { platform: "win32" }) + await new Promise((resolve) => setTimeout(resolve, 0)) + + // The grant is the only icacls call attempted and no descriptor has been opened + // for staged bytes yet: the hardening is awaited, not merely ordered before them. + expect(vi.mocked(execFile).mock.calls).toHaveLength(1) + expect(vi.mocked(fsSync.openSync).mock.calls).toHaveLength(0) + expect(held.length).toBe(1) + + held[0]?.(null, "", "") + await expect(write).resolves.toBeUndefined() + + expect(vi.mocked(fs.rename)).toHaveBeenCalledTimes(1) + const calls = vi.mocked(execFile).mock.calls.map(function (call) { + return (call[1] ?? []).map(String) + }) + expect( + calls.filter(function (args) { + return args.includes("/save") + }), + ).toHaveLength(1) + expect( + calls.filter(function (args) { + return args.includes("/restore") + }), + ).toHaveLength(1) + const grantOrder = + vi.mocked(execFile).mock.invocationCallOrder[ + calls.findIndex(function (args) { + return args.includes("/inheritance:r") + }) + ] + const firstOpenOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + expect(grantOrder).toBeLessThan(firstOpenOrder!) + }) + + it("win32: strips inherited ACEs from the staging directory before staging content", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(null, "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // mkdirSync's mode is ignored by Windows directory creation and chmodSync does not + // establish a DACL, so the privacy of the staged bytes has to come from an explicit + // grant. This is the incantation a measured icacls run on a Windows host showed to + // leave a single explicit ACE, which the files created inside then inherit. + const grants = vi.mocked(execFile).mock.calls.filter(function (call) { + return (call[1] ?? []).map(String).includes("/inheritance:r") + }) + expect(grants).toHaveLength(1) + const grantArgs = (grants[0]?.[1] ?? []).map(String) + expect(grantArgs[0]).toContain(".file-safety-staging") + expect(grantArgs[1]).toBe("/inheritance:r") + expect(grantArgs[2]).toBe("/grant:r") + expect(grantArgs[3]).toBe("staging-acl-user:(OI)(CI)F") + + // Order is the finding: the grant has to precede the first open of the staging + // file, or the content sits in the inherited directory readable to whoever the + // parent admitted until the step-5 restore. + expect(vi.mocked(execFile).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.openSync).mock.invocationCallOrder[0], + ) + }) + + it("win32: a staging directory that cannot be made private is reported and the write still saves", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls unavailable"), "", "") + return fakeChild + }) + const warnings: string[] = [] + + // A host where icacls is missing or refuses the grant must not lose the ability to + // save: the write proceeds and the leftover exposure is reported. + await expect( + safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }), + ).resolves.toBeUndefined() + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) + expect(warnings.filter((m) => m.includes("readable by other accounts"))).toHaveLength(1) + }) + }) + + // ── Test 6: pre-written temp path (tempPath option) ────────────────────── + + describe("pre-written temp path", () => { + it("uses the provided tempPath, fsyncs it, and renames to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + const customTempPath = "/tmp/test-dir/custom-temp.tmp" + + // platform:linux skips DACL entirely so this test focuses on tempPath only + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // openSync was called on the custom temp path (r+ mode for fsync) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + + // fsync was called + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + + // rename happened — realpath mock returns targetPath + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + + // no unlink of custom temp (caller's concern; DACL skipped via platform:linux) + expect(fs.unlink).not.toHaveBeenCalled() + + // a caller-supplied tempPath must not create the staging directory + expect(fsSync.mkdirSync).not.toHaveBeenCalled() + }) + + it("applies the existing target's mode to a caller-supplied tempPath before publishing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/test-dir/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // the caller-staged temp is fchmod'd to the restrictive target mode so + // the atomic rename cannot widen a 0o600 target (CWE-732 regression) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(2, 0o600) + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("keeps the temp's default mode when the target does not exist yet (ENOENT)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/test-dir/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // no existing target, so nothing to preserve and no fchmod on the temp + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("propagates a non-ENOENT stat failure rather than defaulting the mode (caller-staged)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eacces + }) + vi.mocked(fsSync.openSync).mockReturnValue(2) + + const customTempPath = "/tmp/test-dir/custom-temp.tmp" + + // A target that cannot be stat'd is not a fresh target: publishing with + // the default mode would widen a restrictive target through the rename. + await expect( + safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }), + ).rejects.toThrow("EACCES") + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("propagates a non-ENOENT stat failure rather than defaulting the mode (self-staged)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const eio = Object.assign(new Error("EIO: i/o error"), { code: "EIO" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eio + }) + + // The mode is read before the temp is opened, so a real I/O failure stops + // the write before anything is staged. + await expect(safeWriteText(targetPath, "hello world", { platform: "linux" })).rejects.toThrow("EIO") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/test-dir/custom-temp.tmp" + + await safeWriteText(targetPath, "", { tempPath: customTempPath, platform: "linux" }) + + // a 0o444 target must not make openSync(tempPath, "r+") fail: the mode + // is applied with fchmodSync on the already-open fd, after the open + expect(fsSync.openSync).toHaveBeenCalledWith(customTempPath, "r+") + expect(fsSync.fchmodSync).toHaveBeenCalledWith(3, 0o444) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(customTempPath, targetPath) + }) + + it("applies the existing target's exact mode to the self-staged temp (umask must not narrow it)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // openSync's creation mode is narrowed by the process umask (0o664 -> 0o644 with + // the common 0o022), and the rename publishes the temp's mode onto the target, + // so the existing target's mode must be applied on the fd before the commit. + expect(fsSync.fchmodSync).toHaveBeenCalledWith(1, 0o664) + const openIdx = vi.mocked(fsSync.openSync).mock.invocationCallOrder[0] + const fchmodIdx = vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0] + expect(openIdx).toBeLessThan(fchmodIdx) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("does not fchmod the self-staged temp for a fresh target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Nothing exists to preserve: the default creation mode is the intended one. + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + }) + }) + + // ── Test 7: symlink handling (Finding 4 regression test) ───────────────── + + describe("symlink handling", () => { + it("a write through a symlink commits onto the resolved referent, never the link path", async () => { + const linkPath = "/tmp/links/link.txt" + const referentPath = "/tmp/targets/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(referentPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "new-content", { platform: "linux" }) + + // The commit rename must target the realpath result (the referent), never the link itself — + // that is what guarantees a write through a symlink replaces the referent's content + // and preserves the link. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), referentPath) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), linkPath) + }) + + it("when realpath reports ENOENT (target absent), uses the given path as-is", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + // lstat reports the path itself as absent, so this is a new target and + // the fallback is allowed. + vi.mocked(fs.lstat).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // rename still happened with the fallback path (path.resolve on /tmp → C:\tmp) + const resolvedFallback = _resolvedTarget(targetPath) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), resolvedFallback) + }) + + it("propagates a dangling symlink instead of writing through the link path", async () => { + // realpath resolves the referent, so a link whose target is missing reports + // ENOENT. Falling back to the link path would replace the symlink with a + // regular file, so the error must propagate and nothing may be committed. + const linkPath = "/tmp/test-dir/dangling-link.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + const linkStats = Object.create(fsSync.Stats.prototype) as fsSync.Stats + linkStats.isSymbolicLink = () => true + vi.mocked(fs.lstat).mockResolvedValue(linkStats) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await expect(safeWriteText(linkPath, "data", { platform: "linux" })).rejects.toThrow("ENOENT") + + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("preserves the target's restrictive mode and tolerates a failed staging-dir permission repair", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + // a pre-existing staging dir may fail its best-effort permission repair + vi.mocked(fsSync.chmodSync).mockImplementationOnce(() => { + throw new Error("EACCES") + }) + + await safeWriteText(targetPath, "secret", { platform: "linux" }) + + // the staging file inherits the target's 0o600 mode and the write commits + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o600) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("falls back to the 0o644 default when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + await safeWriteText(targetPath, "fresh", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + }) + + it("loops on short writes until the full content is durable before fsync", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const content = "0123456789" // 10 bytes + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const buffer = Buffer.from(content, "utf8") + // first write (offset 0) reports 4 bytes (short write); the loop continues + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + args[2] === 0 ? 4 : typeof args[3] === "number" ? args[3] : 0, + ) + + await safeWriteText(targetPath, content, { platform: "linux" }) + + // [0,10) reports 4 bytes, then [4,10) writes the remaining 6 + expect(fsSync.writeSync).toHaveBeenCalledTimes(2) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(1, 1, buffer, 0, 10) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(2, 1, buffer, 4, 6) + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("fsyncs the parent directory after the commit rename on POSIX", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // temp fd=1 then parent-dir fd=2 - distinct fds prove the ordering + vi.mocked(fsSync.openSync).mockReturnValueOnce(1).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // the directory fsync (fd 2) happens only after the file fsync (fd 1); + // the dir path assertion is path-agnostic (stringContaining) because + // path.dirname renders the same input differently on Windows + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("test-dir"), "r") + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(1, 1) + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(2, 2) + expect(fsSync.closeSync).toHaveBeenCalledWith(2) + }) + + it("fsyncs the entry naming each parent directory it created, deepest first", async () => { + // path.resolve keeps the mock's spellings and the SUT's identical: on Windows the + // SUT prefixes a drive letter where a bare path.join does not. + const createdFrom = path.resolve("/tmp/test-dir", "a") + const dirPath = path.join(createdFrom, "b") + const targetPath = path.join(dirPath, "target.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + // fs.mkdir reports the shallowest directory it had to create: here that is "a", so + // both "a" and "b" are new entries living inside directories that were there. + vi.mocked(fs.mkdir).mockResolvedValue(createdFrom) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + const syncedDirs = vi + .mocked(fsSync.openSync) + .mock.calls.filter(function (call) { + return call[1] === "r" + }) + .map(function (call) { + return String(call[0]) + }) + // The directory the commit rename changed an entry in, then each containing + // directory up to the nearest one that already existed. Synchronizing a created + // directory's own inode does not synchronize the entry that names it. + expect(syncedDirs).toEqual([dirPath, createdFrom, path.dirname(createdFrom)]) + expect(fsSync.fsyncSync).toHaveBeenCalledTimes(syncedDirs.length + 1) + }) + + it("does not walk ancestor directories when mkdir had nothing to create", async () => { + const dirPath = path.resolve("/tmp/test-dir", "deep") + const targetPath = path.join(dirPath, "target.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + // An undefined return says every component already existed, so no ancestor entry + // changed and there is nothing beyond the commit rename's own directory to sync. + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + const dirOpens = vi.mocked(fsSync.openSync).mock.calls.filter(function (call) { + return call[1] === "r" + }) + expect( + dirOpens.map(function (call) { + return String(call[0]) + }), + ).toEqual([dirPath]) + }) + + it("reports a failed parent-directory fsync instead of claiming a durable write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // The content rename committed, so the caller can still find the data at + // the target; what the write cannot claim is that the directory entry + // reached the disk. Returning success here would claim durability the + // filesystem did not grant. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow( + PostCommitDurabilityError, + ) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("propagates realpath errors (EACCES and code-less) instead of the fallback path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(eacces) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("backup:true propagates access errors (EACCES and code-less) instead of skipping the backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES"), { code: "EACCES" }) + const plain = new Error("access failed") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // each write accesses dirPath then target; only the target access rejects + const rejectTarget = (error: Error) => async (p: unknown) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw error + } + vi.mocked(fs.access) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(plain)) + .mockImplementationOnce(rejectTarget(plain)) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toEqual( + expect.objectContaining({ code: "EACCES" }), + ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "access failed", + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + describe("content bytes", () => { + const targetPath = "/tmp/enc-dir/target.txt" + + beforeEach(() => { + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + }) + + it("stages UTF-8 bytes for string content", async () => { + await safeWriteText(targetPath, "héllo", { platform: "linux" }) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, Buffer.from("héllo", "utf8"), 0, 6) + }) + + it("publishes caller-supplied bytes unchanged instead of re-encoding them", async () => { + // The extension host encodes a document with VS Code's own codec, which + // covers the legacy code pages and BOMs Node cannot represent, and hands + // the result over: those bytes must reach the commit rename exactly as + // they were given. + const bytes = Buffer.from([0x00, 0x68, 0x00, 0x69]) + await safeWriteText(targetPath, bytes, { platform: "linux" }) + expect(fsSync.writeSync).toHaveBeenCalledWith(1, bytes, 0, 4) + }) + }) +}) + +// ── Test 12: lock key, staging path, and post-commit durability ───────────── + +describe("resolveLockKey", () => { + beforeEach(() => mockDefaults()) + + it("canonicalizes the parent directory, not just the file", async () => { + vi.mocked(fs.realpath).mockImplementation(async (target) => { + const key = String(target) + if (key === "/tmp/linkdir/file.json") return "/real/dir/file.json" + if (key === "/real/dir") return "/real/dir" + return key + }) + + // The key is the canonical directory plus the basename, so a symlinked + // ancestor and its referent share one lock. + await expect(resolveLockKey("/tmp/linkdir/file.json")).resolves.toBe(path.join("/real/dir", "file.json")) + }) + + it("computes a key for a dangling link, which resolvePublishTarget refuses", async () => { + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + // Only the link path is read, so a single answer is enough and keeps the mock's + // return type matching fs.promises.readlink. + vi.mocked(fs.readlink).mockResolvedValue("referent.json") + + // Mid-commit a peer writer renames the referent away and back, so the key + // must still be computable while the link dangles. + await expect(resolveLockKey("/tmp/linkdir/file.json")).resolves.toBe( + path.resolve(path.join("/tmp/linkdir", "referent.json")), + ) + }) + + it("terminates on a two-link cycle instead of walking forever", async () => { + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + // Every readlink answers with the same link, so an unbounded walk would + // never end; the bounded walk returns the key it actually reached. + vi.mocked(fs.readlink).mockImplementation(async () => "a.json") + + await expect(resolveLockKey("/tmp/linkdir/a.json")).resolves.toBe( + path.resolve(path.join("/tmp/linkdir", "a.json")), + ) + // The bound is MAX_SYMLINK_HOPS - 40 links, the longest chain Node resolves at all - + // so a cycle terminates at the bound and a chain the operating system could follow is + // never cut off short of its referent. + expect(fs.readlink).toHaveBeenCalledTimes(40) + }) +}) + +describe("caller-supplied staging path", () => { + beforeEach(() => { + mockDefaults() + // The location check compares canonical directories. A stub that answers every path + // with the target spells both sides from the same string and cannot tell an outside + // directory from the target's own, so these rows resolve identity: no component is a + // link, which is what each of them assumes unless it says otherwise. + vi.mocked(fs.realpath).mockImplementation(async (target) => String(target)) + }) + + it("rejects a staging file outside the target's directory before writing anything", async () => { + const targetPath = "/tmp/test-dir/target.txt" + + // A rename across filesystems fails with EXDEV, and a path elsewhere lets + // a caller publish an unrelated file onto the target. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/other-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(StagingPathError) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("accepts a staging file whose directory is an alias of the target's directory", async () => { + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Both spellings reach one directory through a symlinked ancestor: realpath resolves + // each alias to the canonical directory, and that is the only comparison that says + // whether the commit rename would cross a filesystem. + vi.mocked(fs.realpath).mockImplementation(async (p: unknown) => + String(p).includes("alias-dir") ? String(p).replace("alias-dir", "canonical-dir") : String(p), + ) + + await safeWriteText(path.join("/tmp/alias-dir", "target.txt"), "data", { + tempPath: path.join("/tmp/alias-dir", "caller-staged.txt"), + platform: "linux", + }) + + expect(fs.rename).toHaveBeenCalledWith( + expect.stringContaining("caller-staged.txt"), + expect.stringContaining(path.join("canonical-dir", "target.txt")), + ) + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("caller-staged.txt"), "r+") + }) + + it("rejects a staging path that is a symlink", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) + + // Renaming a link over the target publishes whatever the link points at. + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/x.tmp", platform: "linux" }), + ).rejects.toThrow(StagingPathError) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("rejects a staging path that is the target itself", async () => { + const targetPath = "/tmp/test-dir/target.txt" + // Same inode and device for the supplied staging path and the target: the + // failure handler would unlink the only copy of the content, so a failed + // write would delete the file it was meant to protect. + const stats = _fileStatsWithIdentity(42n, 7n) + vi.mocked(fs.lstat).mockResolvedValue(stats) + + await expect(safeWriteText(targetPath, "data", { tempPath: targetPath, platform: "linux" })).rejects.toThrow( + StagingPathError, + ) + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // The comparison is only sound when both stats are read as bigint: on NTFS/ReFS the file + // identifiers exceed Number.MAX_SAFE_INTEGER. + // Filter on the options, not the spelling: path.resolve prefixes a drive letter on Windows, + // so the two identity reads are the calls that asked for options at all. + const identityLookups = vi.mocked(fs.lstat).mock.calls.filter((c) => c[1] !== undefined) + expect(identityLookups.length).toBeGreaterThanOrEqual(2) + for (const c of identityLookups) { + expect(c[1]).toEqual({ bigint: true }) + } + expect(fs.unlink).not.toHaveBeenCalled() + }) + + it("rejects when the target identity cannot be compared for a reason other than a missing target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + // A hard-linked staging file shares the target's inode, so the identity comparison is the only thing + // between this write and a rename onto the very file the guard protects. An EACCES from the target + // lstat must not be mistaken for "there is no target". + const stagingStats = _fileStatsWithIdentity(42n, 7n) + // The guard reads the target under its resolved spelling, which on Windows carries a + // drive letter the literal here does not. + const resolvedTarget = path.resolve(targetPath) + vi.mocked(fs.lstat).mockImplementation(async (p) => { + if (String(p) === targetPath || String(p) === resolvedTarget) { + throw Object.assign(new Error("EACCES"), { code: "EACCES" }) + } + return stagingStats + }) + + await expect( + safeWriteText(targetPath, "data", { tempPath: "/tmp/test-dir/hardlink.txt", platform: "linux" }), + ).rejects.toThrow("Staging file could not be compared with the target") + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + // Both identity reads must ask for bigint stats, or the comparison silently falls + // back to rounded numbers on NTFS/ReFS. + const identityLookups = vi.mocked(fs.lstat).mock.calls.filter((c) => c[1] !== undefined) + expect(identityLookups).toHaveLength(2) + for (const c of identityLookups) { + expect(c[1]).toEqual({ bigint: true }) + } + }) +}) + +describe("cleanup when a backed-up write fails before commit", () => { + beforeEach(() => mockDefaults()) + + it("releases the staged file, its copy and its own staging directory before throwing", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The commit rename is the only rename in this flow and it fails. + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow("ENOSPC") + + // The staging file and this write's own directory must not leak, and neither may + // the backup copy: the target still holds the pre-write content on disk. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + const stagingDirs = vi.mocked(fsSync.mkdirSync).mock.calls.map((call) => String(call[0])) + expect(stagingDirs.length).toBe(1) + expect(fs.rmdir).toHaveBeenCalledWith(stagingDirs[0]) + + const failingRenameOrder = vi.mocked(fs.rename).mock.invocationCallOrder[0] + const unlinkOrder = vi.mocked(fs.unlink).mock.invocationCallOrder[0] + const rmdirOrder = vi.mocked(fs.rmdir).mock.invocationCallOrder[0] + expect(unlinkOrder).toBeGreaterThan(failingRenameOrder) + expect(rmdirOrder).toBeGreaterThan(failingRenameOrder) + }) +}) + +describe("resolvePublishTarget", () => { + beforeEach(() => mockDefaults()) + + it("propagates an lstat failure that is not ENOENT instead of falling back to the link path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockRejectedValue(eacces) + + // A failed lstat says nothing about whether the path is a link, so the + // fallback would publish through a link we were not allowed to inspect. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("still falls back to the given path when lstat also reports the path as absent", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fs.lstat).mockRejectedValue(enoent) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The fallback is the resolved path, not the string that was handed in. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), path.resolve(targetPath)) + }) +}) + +// ── Lock key with a missing parent: one file, one lock ────────────────────── + +describe("resolveLockKey when the parent directory does not exist yet", () => { + beforeEach(() => mockDefaults()) + + it("canonicalizes through the nearest existing ancestor instead of the literal parent", async () => { + // Two writers must take ONE lock: the one whose parent directory is already there, + // and the one racing to create it. Resolving only the immediate parent and falling + // back to its literal spelling on ENOENT gave them different keys whenever an + // ancestor was a symlink or a Windows short name, so a read-modify-write under the + // advisory lock lost one side. + const aliasDir = path.resolve("/tmp/alias-parent") + const canonicalDir = path.resolve("/tmp/real-parent") + const nested = path.join(aliasDir, "nested") + const target = path.join(nested, "history_item.json") + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + const s = String(p) + if (s === target || s === nested) { + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + } + if (s === aliasDir) { + return canonicalDir + } + return s + }) + + expect(await resolveLockKey(target)).toBe(path.join(canonicalDir, "nested", "history_item.json")) + }) + + it("propagates a realpath failure that is not ENOENT instead of guessing a key", async () => { + // A realpath that fails for another reason says nothing about the canonical form; + // returning a literal key would silently put this writer on a different lock. + const target = path.join(path.resolve("/tmp/test-dir"), "history_item.json") + vi.mocked(fs.lstat).mockResolvedValue(_fileStats(false)) + // Not a link: the walk must reach the canonicalization step, which is where the + // non-ENOENT failure has to surface. + vi.mocked(fs.readlink).mockRejectedValue( + Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }), + ) + vi.mocked(fs.realpath).mockImplementation(async (p) => { + if (String(p) === target) return target + throw Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + }) + + await expect(resolveLockKey(target)).rejects.toThrow("EACCES") + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..a908459d19 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,817 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" +import { userInfo } from "os" + +export interface SafeWriteTextOptions { + /** + * When true, keep the old-file semantics without ever removing the target: the + * previous content is copied to a hidden backup path and flushed before the + * commit rename, the commit rename atomically replaces the target, and on success + * the backup copy is deleted. A failure before the commit leaves the target + * untouched (there is nothing to roll back) and removes the backup copy. When + * false (default) the atomic rename simply replaces the target. + */ + backup?: boolean + + /** + * Platform override for testing. When omitted the real process.platform + * value is used. Set to "win32" or "linux" / "darwin" from tests so that + * both branches are reachable without needing a real Windows runner. + */ + platform?: string + + /** + * Custom execFile runner for testing (e.g. vi.fn). When omitted the real + * child_process.execFile is used. + */ + execFileRunner?: typeof execFile + + /** + * Sink for non-fatal safety notices. A Windows DACL that could not be captured means the + * committed file may inherit different access rights: the write still proceeds (a missing or + * failing icacls must not block saving), but the caller is told instead of the change being + * silent. Defaults to console.warn. + */ + onWarning?: (message: string) => void + + /** + * Pre-written temp path to use for the commit phase. When provided, + * safeWriteText skips creating its own staging file and uses this path + * instead (it still fsyncs before rename). Useful when a caller has + * already written data to a temp file via a custom stream. + */ + tempPath?: string +} + +/** + * A caller-supplied staging path that is not a file this write may publish: it + * sits outside the target's directory (so the commit rename would cross + * filesystems) or is not a regular file. Rejecting it before any write keeps the + * target from being replaced by whatever the path points at. + */ +export class StagingPathError extends Error { + readonly stagingPath: string + + constructor(message: string, stagingPath: string) { + super(message) + this.name = "StagingPathError" + this.stagingPath = stagingPath + } +} + +/** + * The commit rename succeeded but the parent-directory fsync did not, so the + * directory entry is not known to be durable. The content is at the target; the + * caller cannot assume it survives a crash. Reported as its own error so a + * successful return never claims durability the filesystem did not grant. + */ +export class PostCommitDurabilityError extends Error { + readonly targetPath: string + /** + * The old-content copy retained for recovery on this path, or null when backup + * mode was off or the target did not exist before the write. The name is a hidden + * random path, so without this field no caller could find the copy the catch block + * deliberately keeps. + */ + readonly backupPath: string | null + + constructor(targetPath: string, cause: unknown, backupPath: string | null = null) { + super( + "The rename committed but the parent directory could not be fsynced -- the content is at the target path reported on this error, and the directory entry may not be durable.", + { cause }, + ) + this.name = "PostCommitDurabilityError" + this.targetPath = targetPath + this.backupPath = backupPath + } +} +// -- helpers --------------------------------------------------------------- + +/** + * How far resolveLockKey follows a chain of links. 40 is where Node's own resolver + * gives up with ELOOP, so every chain the operating system is willing to resolve is a + * chain this walk resolves too. A smaller bound stops at an intermediate link of a + * long dangling chain, and the writer that stopped there queues on a different lock + * from the writer standing at the referent - the two-locks-for-one-file case + * canonicalDirKey exists to prevent. + */ +const MAX_SYMLINK_HOPS = 40 + +/** Generate a unique temp file name in the given directory. */ +function _tempName(dir: string, prefix: string): string { + return path.join(dir, "." + prefix + "_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp") +} + +/** Create a private per-write staging sub-directory inside *dir*. The name is + * unique per write, so concurrent writes never collide on their temp names and + * never remove a staging directory another write is still using: with one shared + * name, one write's best-effort rmdir could delete the directory another write + * had just created but not yet opened, failing its openSync with ENOENT. */ +function _stagingDir(dir: string): string { + const sd = path.join(dir, ".file-safety-staging_" + Date.now() + "_" + Math.random().toString(36).substring(2)) + // mode:0o700 protects a freshly created staging dir; the best-effort chmod + // repairs a pre-existing one (mkdirSync with recursive:true never chmods an + // existing directory), so staged temp files are never group/world readable. + // That is the POSIX story only: Windows directory creation ignores this mode argument + // and chmodSync establishes no DACL, so on win32 what makes the staging directory + // private is the caller's _grantPrivateDaclWindows step, which runs before the first + // staged byte and reports rather than fails when icacls is unavailable. + fsSync.mkdirSync(sd, { recursive: true, mode: 0o700 }) + try { + fsSync.chmodSync(sd, 0o700) + } catch { + // best-effort: chmod denied or unavailable; a fresh dir was still + // created with the requested mode + } + return sd +} + +function _fsyncFile(fd: number): void { + fsSync.fsyncSync(fd) +} + +/** Save the DACL of *srcPath* to a dump file on Windows. + * Returns true when the dump was written successfully; false otherwise. + * Never throws — callers treat failure as "skip DACL handling". */ +async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [srcPath, "/save", dumpPath, "/T"], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch { + return false + } +} + +/** Restore a DACL dump onto *dirPath* on Windows. + * Returns whether icacls succeeded; the caller reports a failure. */ +async function _restoreDaclWindows( + dirPath: string, + dumpPath: string, + execFileRunner?: typeof execFile, +): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [dirPath, "/restore", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch { + return false + } +} + +/** Apply a restrictive Windows DACL to a freshly created staging directory. + * `icacls /inheritance:r /grant:r ":(OI)(CI)F"` drops every inherited + * ACE and leaves one explicit ACE for the account running the write, so files created + * inside inherit only that account. Measured on a Windows host: the directory went + * from three inherited ACEs (SYSTEM, Administrators and the user) to the single + * explicit one, and a file written into it inherited only the user. + * Returns false instead of throwing: a machine without icacls, or with no account name + * readable either from the environment or from the host itself, still saves the file, and + * the caller reports the leftover exposure through onWarning. */ +/** + * The account name the staging grant names. The environment comes first because that is + * the source a caller can control and the source the tests pin. It is empty on a process + * that inherits neither USERNAME nor USER, which is how the hosted windows jobs run, so + * the account the host itself reports is asked next: userInfo is synchronous, and it throws + * when no account entry can be read, which is the same no-account state the caller already + * reports through onWarning. A name icacls will not resolve makes the grant fail, and a + * failed grant already takes this same warning path rather than rejecting the write. */ +function _resolveStagingAccountName(): string | null { + const fromEnvironment = process.env.USERNAME ?? process.env.USER + if (typeof fromEnvironment === "string" && fromEnvironment.length > 0) { + return fromEnvironment + } + try { + const reported = userInfo().username + return typeof reported === "string" && reported.length > 0 ? reported : null + } catch { + return null + } +} + +async function _grantPrivateDaclWindows(stagingDirPath: string, execFileRunner?: typeof execFile): Promise { + const account = _resolveStagingAccountName() + if (!account) { + return false + } + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner( + "icacls", + [stagingDirPath, "/inheritance:r", "/grant:r", `${account}:(OI)(CI)F`], + { windowsHide: true }, + (err) => (err ? reject(err) : resolve()), + ) + }) + return true + } catch { + return false + } +} + +// -- public API ------------------------------------------------------------ + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) + * may fall back to the given path; any other resolution error (EACCES, EIO, ...) + * propagates so a broken or unreadable symlink is never written through its + * link path. Callers that stage a temp file themselves must stage it beside + * the resolved path: the commit is a rename onto the referent, and a rename + * across filesystems fails with EXDEV. + */ +export async function resolvePublishTarget(absoluteFilePath: string): Promise { + return fs.realpath(absoluteFilePath).catch(async (error: unknown) => { + if (errorCode(error) !== "ENOENT") throw error + // ENOENT also covers a dangling symlink, which must never be written through. + // Only a lstat that also reports the path as absent may fall back to the + // given path; a real lstat failure (EACCES, EIO) says nothing about whether + // the path is a link, so falling back would write through a link we were + // simply not allowed to inspect. + const linkStat = await fs.lstat(absoluteFilePath).catch((lstatError: unknown) => { + if (errorCode(lstatError) === "ENOENT") return undefined + throw lstatError + }) + if (linkStat?.isSymbolicLink()) throw error + return absoluteFilePath + }) +} +/** + * Distinguish "the target does not exist" from a real I/O failure (EACCES, + * EIO, ...). The mode-preservation path may only fall back to the fresh-file + * default on ENOENT; any other failure is propagated, otherwise a restrictive + * target (0o600) would be published with the default 0o644 through the rename. + */ +/** + * The errno-style code of a filesystem error, or undefined for anything that is not one. Exported + * because safeWriteJson asks the same question of the same errors; a second copy of this test drifts. + */ +export function errorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? String((error as { code: unknown }).code) + : undefined +} + +/** + * Canonicalize the parent directory and re-join the basename. fs.realpath + * canonicalizes every component, including a symlinked ancestor directory or a + * Windows 8.3 short name, so a lock key must be canonical even when the file + * itself is not there yet -- otherwise the key for one file depends on whether + * the file exists when the key is computed, and two writers take two locks. + */ + +async function canonicalDirKey(absoluteFilePath: string): Promise { + const dirPath = path.dirname(absoluteFilePath) + // Walk up to the nearest ancestor that EXISTS, canonicalize that, and re-join the + // components that are not there yet. Falling back to the unresolved spelling of the + // whole parent - what a single realpath(...).catch(() => dirPath) used to do - makes + // the key depend on whether the directory happens to exist: a writer whose parent is + // already there canonicalizes through a symlinked ancestor (or a short name) while a + // writer racing to create the same directory gets the literal spelling, so the two + // take different locks for one file and a read-modify-write loses one side. + // A realpath failure that is not "not there yet" says nothing about the canonical + // form, so it is propagated rather than papered over with a key that may be wrong. + let cursor = dirPath + const missing: string[] = [] + for (;;) { + const canonical = await fs.realpath(cursor).catch((error: unknown) => { + if (errorCode(error) === "ENOENT") return undefined + throw error + }) + if (canonical !== undefined) { + return path.join(canonical, ...missing.reverse(), path.basename(absoluteFilePath)) + } + missing.push(path.basename(cursor)) + const parent = path.dirname(cursor) + if (parent === cursor) { + // Every component up to the root is missing: there is nothing to canonicalize + // against, and the literal path is the only key left. + return path.join(dirPath, path.basename(absoluteFilePath)) + } + cursor = parent + } +} + +/** + * Lock key for a publish target: the symlink referent when the path is an + * existing symlink, the path itself otherwise. Unlike resolvePublishTarget this + * tolerates a dangling link, because the lock key has to be computable while a + * peer writer is mid-commit (a publish renames the staged file onto the referent, + * and backup mode keeps a copy beside it). + * The walk is bounded so a link cycle terminates, and every key it returns is + * canonicalized through canonicalDirKey. The bound is MAX_SYMLINK_HOPS: a dangling + * chain longer than the bound ends the walk at an intermediate link, and a writer + * named through that chain then queues on a different lock from a writer named at + * the referent. 40 links is the longest chain Node resolves at all, so no chain the + * operating system itself can follow is cut off here. + */ +export async function resolveLockKey(absoluteFilePath: string): Promise { + try { + return await canonicalDirKey(await resolvePublishTarget(absoluteFilePath)) + } catch { + // A real readlink throws for anything that is not a link, so a normal chain + // ends the walk. Two links that point at each other never would, so the + // walk is bounded and callers use the key they actually reached. + let key = absoluteFilePath + for (let depth = 0; depth < MAX_SYMLINK_HOPS; depth++) { + const target = await fs.readlink(key).catch(() => undefined) + if (target === undefined) return await canonicalDirKey(key) + key = await canonicalDirKey(path.resolve(path.dirname(key), target)) + } + return await canonicalDirKey(key) + } +} + +/** + * @internal Intentional direct-test seam: no production file calls this write path + * yet, and this unit's own specs exercise it directly, which is the case + * `docs/development-quality-checks.md` line 36 tells authors to declare here rather + * than in configuration. Knip still follows the test imports in default mode. + */ +export async function safeWriteText( + filePath: string, + content: string | Uint8Array, + options?: SafeWriteTextOptions, +): Promise { + const absoluteFilePath = path.resolve(filePath) + + // Resolve the symlink referent (see resolvePublishTarget). + const targetPath = await resolvePublishTarget(absoluteFilePath) + const dirPath = path.dirname(targetPath) + + // Ensure parent directory exists (mirrors safeWriteJson behaviour). + // fs.mkdir reports the shallowest directory it had to create and returns undefined + // when every component was already there. The return value is kept because step 4b + // uses it to synchronize the directory ENTRIES this write created: fsyncing a + // created directory syncs that directory's own inode, not the entry in its parent + // that names it, so a crash could lose the saved file together with the new + // directory holding it. + const createdFrom = await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + // Create the staging directory only when we generate the temp file there; + // callers supplying their own tempPath (e.g. safeWriteJson) must not be left + // with an empty .file-safety-staging directory behind. Track the directory this + // write created so its cleanup removes its own directory, not a shared one. + let stagingDir: string | null = null + let tempPath: string + if (options?.tempPath) { + // A caller-supplied staging file is only safe when it is the file this + // write is staging, not an arbitrary path. Two properties are checked: + // it must sit beside the resolved target (a rename across filesystems + // fails with EXDEV, and a path elsewhere lets a caller publish an + // unrelated file onto the target), and it must be a regular file rather + // than a link — renaming a link over the target publishes whatever the + // link points at, which is the same trust problem as writing through a + // dangling symlink in resolvePublishTarget. + const supplied = path.resolve(options.tempPath) + // Compare canonical directories, not spellings. fs.realpath resolves a symlinked + // ancestor directory and a Windows 8.3 short name, so a caller that staged through + // an alias of the target's own directory staged beside the target and the commit + // rename would not cross a filesystem: rejecting it on spelling alone failed a + // write that was always legal. canonicalDirKey propagates any realpath failure that + // is not "not there yet", so a directory whose canonical form cannot be established + // is rejected rather than guessed at. + const suppliedDirKey = path.dirname(await canonicalDirKey(supplied)) + if (suppliedDirKey !== path.dirname(await canonicalDirKey(targetPath))) { + throw new StagingPathError( + `Staging file must sit in the target's directory (${dirPath}), got ${supplied}`, + supplied, + ) + } + // BigInt stats: on NTFS/ReFS the file identity can exceed Number.MAX_SAFE_INTEGER, and + // a rounded number makes two different files look identical (rejecting a valid staging + // file) or hides a real alias. + const stagingStat = await fs.lstat(supplied, { bigint: true }) + if (stagingStat.isSymbolicLink() || !stagingStat.isFile()) { + throw new StagingPathError( + `Staging file must be a regular file, not ${stagingStat.isSymbolicLink() ? "a symlink" : "another file type"}`, + supplied, + ) + } + // A staging path that is the target would be unlinked by the failure handler + // while it still holds the only copy of the content, so a failed write would + // delete the file it was meant to protect. Compare identities, not spellings: + // an alias of the target is the same hazard. + // Only a missing target may be skipped: an EACCES/ELOOP/ENOTDIR here means the + // identity comparison could not be made, and treating that as "no target" would let a + // staging alias reach the commit and let cleanup delete the file it was meant to + // protect. + const targetStat = await fs.lstat(targetPath, { bigint: true }).catch((error: unknown) => { + if (errorCode(error) !== "ENOENT") { + throw new StagingPathError("Staging file could not be compared with the target", supplied) + } + return null + }) + if ( + targetStat && + typeof stagingStat.ino === "bigint" && + typeof targetStat.ino === "bigint" && + stagingStat.ino === targetStat.ino && + stagingStat.dev === targetStat.dev + ) { + throw new StagingPathError("Staging file must not be the target itself", supplied) + } + // The caller's own path is used as given; only the check is canonical. + tempPath = options.tempPath + } else { + stagingDir = _stagingDir(dirPath) + tempPath = _tempName(stagingDir, "safeWriteText") + } + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + // Whether the commit rename ran. A failure after it is a durability problem + // with the published file, not a pre-commit failure, and the backup is then + // the only known-good copy of the previous content. + let committed = false + // Non-null only when the win32 step-2 block saved a successful DACL dump: + // it gates the step-5 restore and is tracked for the cleanup unlinks. + let daclDumpPath: string | null = null + // The step-2 DACL handling and the staging-directory hardening in step 1 both key + // off this value, and the hardening has to report through the same sink, so both are + // resolved before the staging directory is created. + const platform = options?.platform ?? process.platform + // Warning delivery must never abort the write: the notices below describe a + // committed-but-imperfect publish, and a caller whose callback throws (a UI sink, + // a logger that is mid-restart) must not turn that into a failed save. + const warn = (message: string) => { + const report = (label: string, error: unknown) => { + console.warn( + `safeWriteText: onWarning callback ${label}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + try { + const sink = options?.onWarning ?? ((m: string) => console.warn(m)) + const result: unknown = sink(message) + // A sink may be async - TypeScript accepts a value-returning callback where + // a void one is expected. Awaiting it would let warning delivery delay a + // write that has already committed (and hang it if the sink never settles), + // while leaving the promise unhandled turns a rejection into an unhandled + // rejection, which under Node's default mode can end the process after a + // successful write. Attach a handler without awaiting. + if (result instanceof Promise) { + result.catch((error: unknown) => report("rejected", error)) + } + } catch (error: unknown) { + report("failed", error) + } + } + + try { + // -- Step 1: write content to staging temp file ------------------- + if (!options?.tempPath) { + // Preserve the existing target's permissions: the staging file must + // not be published wider than the file it replaces (a 0o600 target + // must not become 0o644 through the atomic rename). + // Encode before opening the staging file: an encoding Node cannot + // represent must not leave a half-written temp file behind. + // A string is encoded as UTF-8; bytes handed in by the caller (the + // extension host encodes a document with VS Code's own codec, which + // covers the legacy code pages Node cannot represent) are published + // unchanged. + // Windows: the platform's directory creation ignores mkdirSync's mode argument and + // chmodSync does not establish a private DACL, so a staging directory created + // inside a parent whose DACL admits other accounts holds the staged content readable + // by those accounts until the step-5 restore. Make the directory private before the + // first byte is written; stagingDir is non-null exactly on this branch. + if (platform === "win32" && stagingDir !== null) { + if (!(await _grantPrivateDaclWindows(stagingDir, options?.execFileRunner))) { + warn( + `Staged content in ${stagingDir} may be readable by other accounts: the staging directory's Windows DACL could not be made restrictive before the content was written.`, + ) + } + } + const buffer = Buffer.from(content) + let targetMode = 0o644 // default for a fresh target + let targetExists = false + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + targetExists = true + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") throw error + // target does not exist yet - keep the default + } + // openSync's creation mode is narrowed by the process umask, so an + // existing 0o664 target would be published as 0o644 through the + // rename. Apply the existing target's exact mode on the fd, as the + // caller-staged branch does; a fresh target keeps the default mode. + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + if (targetExists) { + fsSync.fchmodSync(fd, targetMode) + } + // Loop until every byte is written: writeSync can report a short + // (partial) write, and publishing a truncated staging file would + // commit corrupt content. + let offset = 0 + while (offset < buffer.length) { + offset += fsSync.writeSync(fd, buffer, offset, buffer.length - offset) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } else { + // Preserve the existing target's mode (CWE-732): the caller-staged + // temp carries its own creation mode, and publishing it as-is would + // widen a restrictive target (e.g. 0o600 -> 0o644) through rename. + // The mode is applied with fchmodSync on the open fd (AFTER openSync): + // chmodSync on the path before the open would make a read-only target + // (0o400/0o444) fail openSync(tempPath, "r+") with EACCES. + let targetMode: number | null = null + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch (error: unknown) { + if (errorCode(error) !== "ENOENT") throw error + // target does not exist yet - keep the temp's default mode + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- + if (platform === "win32") { + let accessError: unknown = null + try { + await fs.access(targetPath) // target exists? + } catch (error: unknown) { + accessError = error + } + if (accessError === null) { + const dumpPath = _tempName(dirPath, "safeWriteText.acl") + const saved = await _saveDaclWindows(targetPath, dumpPath, options?.execFileRunner) + if (saved) { + // Only a successfully saved dump may be restored onto the + // committed file (step 5). + daclDumpPath = dumpPath + } else { + // A failed icacls may have left a partial dump behind; + // remove it now (best-effort) so no partial dump survives and + // no later step can restore from it. + await fs.unlink(dumpPath).catch(() => {}) + // The target exists and its DACL could not be captured, so the commit rename + // replaces it with a file that inherits different access rights. The write still + // proceeds - a missing or failing icacls must not leave the user unable to save - + // but the replacement is no longer ACL-identical and that has to be visible + // instead of silent. + warn( + `Could not save the DACL of ${targetPath}; the replacement may inherit different access rights.`, + ) + } + } else if (errorCode(accessError) !== "ENOENT") { + // Not "absent": the target is there but could not be checked (EACCES, ...), so + // DACL preservation was skipped for a reason the caller cannot infer from the + // successful write alone. + warn( + `Could not check ${targetPath} for DACL preservation (${errorCode(accessError) ?? "unknown error"}); the replacement may inherit different access rights.`, + ) + } + } + try { + // -- Step 3 (backup:true): durable copy target -> backup ---- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + // Copy, never move. Renaming the target away leaves the canonical path absent for + // the whole commit window: readers see a missing file, and a concurrent + // writer can create a new target that a later rollback would destroy. A copy + // keeps the target present, so the step 4 rename is the only change to the + // canonical path. The copy is flushed so the retained content survives a crash. + try { + // Create the destination BEFORE any content exists at it, with the mode fixed at + // open time, and copy the bytes through descriptors this code opened. fs.copyFile + // picks the destination's mode itself (the platform creation mask subject to umask + // on some platforms) and, on POSIX, re-applies the SOURCE's mode to the file it + // writes: a copy through it would widen this private 0o600 seed to the target's + // permissions for the whole copy window - the previous content readable to every + // account the target never admitted - and on Windows it carries the source's + // read-only attribute onto the copy. Writing into the descriptor opened below keeps + // the mode this code chose from the first byte to the last, so nothing has to be + // chmodded afterwards. + const seedFd = fsSync.openSync(backupPath, "wx", 0o600) + try { + const sourceFd = fsSync.openSync(targetPath, "r") + try { + const chunk = Buffer.allocUnsafe(64 * 1024) + let readOffset = 0 + for (;;) { + const read = fsSync.readSync(sourceFd, chunk, 0, chunk.length, readOffset) + if (read === 0) { + break + } + // writeSync can report a short write: publishing a truncated backup would + // hand recovery a copy that is not a complete previous state. + let written = 0 + while (written < read) { + written += fsSync.writeSync(seedFd, chunk, written, read - written) + } + readOffset += read + } + } finally { + fsSync.closeSync(sourceFd) + } + } finally { + fsSync.closeSync(seedFd) + } + // "r+" not "r": fsync on a read-only handle is EPERM on Windows, and the same + // flag the staged temp file uses above. + const backupFd = fsSync.openSync(backupPath, "r+") + try { + _fsyncFile(backupFd) + } finally { + fsSync.closeSync(backupFd) + } + // The recovery copy is an entry inside dirPath, and the only directory fsync in + // this function runs after the commit rename below. A crash between that rename + // and that fsync can leave the rename on disk while the entry naming the recovery + // copy has not reached it, which is the one case where PostCommitDurabilityError + // hands the caller a backupPath for a copy that no longer exists: the content + // survived into the renamed-to directory but the name for the copy did not. + // Synchronizing dirPath here, while a failure is still a pre-commit failure, means + // a directory-entry failure leaves the target at its previous content and the catch + // below removes the copy, instead of the caller having to recover from a write that + // already committed. The cost is one extra fsync on the backup path; the fsync after + // the rename stays, because that rename changes a different entry in this directory. + if (platform !== "win32") { + const backupDirFd = fsSync.openSync(dirPath, "r") + try { + _fsyncFile(backupDirFd) + } finally { + fsSync.closeSync(backupDirFd) + } + } + } catch (backupError: unknown) { + // A partial backup must not outlive this attempt: it is not a complete copy + // of anything, and once the write fails nothing else removes it. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null + throw backupError + } + releaseBackupOnSuccess = true + } catch (err: unknown) { + if (errorCode(err) !== "ENOENT") throw err + } + } + + // -- Step 4: atomic rename temp -> target --------------------- + await fs.rename(tempPath, targetPath) + committed = true + + // -- Step 4b (POSIX): fsync the parent directory so the directory entry + // changed by the commit rename is durable, not just the file content. + if (platform !== "win32") { + try { + const dirFd = fsSync.openSync(dirPath, "r") + try { + _fsyncFile(dirFd) + } finally { + fsSync.closeSync(dirFd) + } + } catch (error: unknown) { + // The content rename committed, but the directory entry that + // points at it is not known to be durable. Reporting success + // here would let a caller believe the write survives a crash, + // so the failure is surfaced as its own error: the caller can + // still find the content at the target, it just cannot rely on + // the directory entry having reached the disk. + // The catch block keeps the backup on this path, so hand the caller the + // path of the copy it is keeping - it is a hidden random name and is + // otherwise unrecoverable. + throw new PostCommitDurabilityError(targetPath, error, releaseBackupOnSuccess ? backupPath : null) + } + // The rename above changed one entry inside dirPath. When this write also + // created dirPath or any of its ancestors, each of those directories is itself + // an entry in its own parent, and the fsync above synchronized dirPath's inode, + // not the entries naming it. Walk up from the first directory this write created + // and synchronize each containing directory, deepest first: the same failure is + // reported the same way, because the content is at the target either way and only + // its durability is in question. + if (createdFrom !== undefined) { + const stopAt = path.dirname(createdFrom) + let ancestorCursor = path.dirname(dirPath) + try { + for (;;) { + const ancestorFd = fsSync.openSync(ancestorCursor, "r") + try { + _fsyncFile(ancestorFd) + } finally { + fsSync.closeSync(ancestorFd) + } + if (ancestorCursor === stopAt) { + break + } + const ancestorParent = path.dirname(ancestorCursor) + if (ancestorParent === ancestorCursor) { + break + } + ancestorCursor = ancestorParent + } + } catch (ancestorError: unknown) { + throw new PostCommitDurabilityError( + targetPath, + ancestorError, + releaseBackupOnSuccess ? backupPath : null, + ) + } + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + // daclDumpPath is non-null only when the win32 step-2 block saved a + // successful dump, so this gate is closed on every other platform + // and on every failed save. + if (daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + const restored = await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + if (!restored) { + // The content is committed, but the published file may carry a different DACL + // from the one that was saved. Failing the write here would break every + // publish on machines where icacls cannot reapply the saved ACEs (a plain + // temp directory restore fails with "Not all privileges or groups referenced + // are assigned to the caller"), so the change of access rights is reported + // rather than thrown. + warn( + `Content committed at ${targetPath}, but the saved DACL could not be restored from ${daclDumpPath}; the file may carry different access rights than the one it replaced.`, + ) + } + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. + + // Best-effort: remove the now-empty staging directory. Self-staged + // writes only, and only this write's own directory: a per-write directory + // cannot be the one another concurrent write is still using. A failure must + // never un-commit a published file, so the removal swallows all errors. + if (stagingDir) { + await fs.rmdir(stagingDir).catch(() => {}) + } + } catch (originalError: unknown) { + // The backup is never restored: it is a copy, and the target already holds + // either the pre-write content (before the commit) or the published content. + // Only a pre-commit failure may discard the backup. After the commit the target + // holds the NEW content, and when the failure is a post-commit durability error the + // backup is the only copy known to hold the previous content - deleting it here + // would destroy the recovery copy the contract promises. + if (backupPath && releaseBackupOnSuccess && !committed) { + // Nothing to restore: the backup is a copy, so the target still holds whatever + // the commit left there. Before the commit that is the pre-write content, and + // the copy beside it is redundant; it must not be left where no caller can + // find it. + await fs.unlink(backupPath).catch(() => {}) + backupPath = null + } + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // A failed self-staged write must not leave its staging directory behind. + // Only the directory this write created, and only after its temp file is + // gone, so the directory is empty and the removal stays best-effort. + if (stagingDir) { + await fs.rmdir(stagingDir).catch(() => {}) + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + throw originalError + } +} diff --git a/src/utils/__tests__/safeWriteJson.interruption.test.ts b/src/utils/__tests__/safeWriteJson.interruption.test.ts index c9c5993f84..8b5d68c81f 100644 --- a/src/utils/__tests__/safeWriteJson.interruption.test.ts +++ b/src/utils/__tests__/safeWriteJson.interruption.test.ts @@ -37,7 +37,44 @@ const actualRename = fsPromises.rename // replacement protocol the writer uses; exit before performing it. let commitAttempts = 0 fsPromises.rename = async function interruptionRename(oldPath, newPath) { - if (path.resolve(String(newPath)) === targetPath) { + // Identity is the only comparison that holds on both spellings of one file. The + // writer publishes at the path fs.realpath returns, and that call expands an NTFS 8.3 + // short name and applies the casing stored on disk, while targetPath above holds the + // spelling the parent process handed to the writer. On a Windows runner the temp + // directory is reported as an alias (C:/Users/RUNNER~1/...), so text equality misses + // the commit rename there and hits it on a developer box — which is the exact shape + // of the 12 failures this harness produced at 5a8e64a8b275, every one of them + // "expected WRITE_COMPLETED to contain EXIT_AT_REPLACEMENT_RENAME". + function fileIdentity(candidate) { + try { + const stats = fs.statSync(candidate) + return stats.dev + ":" + stats.ino + } catch { + return null + } + } + const targetBaseName = path.basename(targetPath).toLowerCase() + const targetDirectoryIdentity = fileIdentity(path.dirname(targetPath)) + function isCommitRename(newPath) { + const candidate = path.resolve(String(newPath)) + if (candidate === targetPath) { + return true + } + const candidateIdentity = fileIdentity(candidate) + if (candidateIdentity !== null) { + return candidateIdentity === fileIdentity(targetPath) + } + // The destination is not on disk yet, which is the commit of a first write. Same + // directory identity plus the same basename then names the same file. Case is folded + // because it is not significant on Windows; on POSIX the fold could only merge two + // names this protocol never renames onto each other, and the staging rename carries a + // different basename anyway. + return ( + fileIdentity(path.dirname(candidate)) === targetDirectoryIdentity && + path.basename(candidate).toLowerCase() === targetBaseName + ) + } + if (isCommitRename(newPath)) { commitAttempts++ if (process.env.INTERRUPTION_MODE === "fallback" && commitAttempts === 1) { return Promise.reject(Object.assign(new Error("Injected EPERM"), { code: "EPERM" })) diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts new file mode 100644 index 0000000000..e0ffcf8a35 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -0,0 +1,500 @@ +// npx vitest run utils/__tests__/safeWriteJson.lockKey.spec.ts + +import { existsSync } from "fs" +import * as os from "os" +import path from "path" +import type { BigIntStats } from "fs" +import * as fs from "fs/promises" +import { acquireFileLock } from "../fileLock" +import { ConfinedPathEscapeError, LockIdentityChangedError, safeWriteJson } from "../safeWriteJson" +import { resolveLockKey } from "../../services/file-safety/safeWriteText" + +vi.mock("../fileLock", () => ({ + acquireFileLock: vi.fn(async () => async () => {}), +})) + +vi.mock("fs/promises", async () => { + const actual = await vi.importActual("fs/promises") + return { ...actual, realpath: vi.fn(), lstat: vi.fn(), readlink: vi.fn() } +}) + +const mockedRealpath = vi.mocked(fs.realpath) +const mockedLstat = vi.mocked(fs.lstat) +const mockedReadlink = vi.mocked(fs.readlink) +const mockedAcquireFileLock = vi.mocked(acquireFileLock) + +const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + +// Each test creates a real temp directory so the real fs calls still work. +// doubles between tests so an implementation from one test cannot carry over. +const createdDirs: string[] = [] +async function makeDir(prefix: string): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)) + createdDirs.push(dir) + return dir +} + +beforeEach(() => { + mockedRealpath.mockReset() + mockedLstat.mockReset() + mockedReadlink.mockReset() + mockedAcquireFileLock.mockReset() +}) + +afterEach(async () => { + for (const dir of createdDirs) { + await fs.rm(dir, { recursive: true, force: true }).catch(() => undefined) + } + createdDirs.length = 0 +}) + +// Only isSymbolicLink() is consulted by the guard, so the double carries just +// that method. The mocks reject asynchronously: a synchronous throw would bypass +// resolvePublishTarget's catch and skip the ENOENT/symlink branch under test. +const symlinkStat = (target: unknown) => + ({ + isSymbolicLink: () => target === currentLink, + // The staging-path check in safeWriteText also asks whether the path is a + // regular file, so the double carries that predicate as well. + isFile: () => target !== currentLink, + }) as unknown as BigIntStats +let currentLink = "" + +describe("safeWriteJson lock key under a peer commit", () => { + it("waits for the peer instead of rejecting, and locks the referent", async () => { + const order: string[] = [] + const dir = await makeDir("lockkey-") + const referent = path.join(dir, "history_item.json") + currentLink = path.join(dir, "link.json") + + // The peer writer has renamed the referent away and has not committed yet, + // so the first resolution fails with ENOENT while lstat still reports a + // symbolic link. A strict resolve here rejects the caller before it can ever + // queue behind the peer, and the caller's delta write is lost. + mockedRealpath + .mockImplementationOnce(async () => { + order.push("resolve-failed") + throw enoent + }) + .mockImplementation(async (target) => { + order.push("resolve") + // The second call happens under the lock, where the peer has committed. + return target === currentLink ? referent : String(target) + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async () => { + order.push("lock") + return async () => {} + }) + + await safeWriteJson(currentLink, { id: "task-1" }) + + // The lock key is the key every other writer to this file uses, so the caller + // queued behind the peer instead of failing before the lock. + expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent) + // The last resolve is this module's own publish-target resolution under the lock: + // the commit rename needs the referent, and the staging path is derived from it. + // Nothing here consults a delegated writer any more - this module performs the + // commit rename itself - so the operation list is shorter than it was under + // delegation. Every entry still runs after the lock key was resolved and after + // the lock was taken, so none of them changes which lock the caller queued behind. + // The recovery pass resolves the publish target before it scans, so one more resolution + // runs inside the lock: it is what makes the scan look beside the referent instead of + // beside the link. The pinned entries still say nothing ran before the lock key was + // resolved and nothing ran while the lock was unheld, which is the claim of this case. + expect(order).toEqual(["resolve-failed", "lstat", "resolve", "resolve", "lock", "resolve", "resolve"]) + expect(JSON.parse(await fs.readFile(referent, "utf8"))).toEqual({ id: "task-1" }) + }) + + it("releases the lock when the resolution under the lock rejects", async () => { + const order: string[] = [] + let released = false + const dir = await makeDir("lockkey-") + const referent = path.join(dir, "history_item.json") + currentLink = path.join(dir, "link.json") + + // A real dangling link: the walk tolerates it so the caller can queue behind + // the peer, but once the lock is held the strict rejection still applies. A + // rejection outside the protected block would leave the lock held until the + // stale timeout for every other writer to the same file. + mockedRealpath.mockImplementation(async () => { + throw enoent + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockImplementation(async () => { + order.push("lock") + return async () => { + order.push("release") + released = true + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" })).rejects.toThrow(enoent) + expect(released).toBe(true) + // The strict rejection is reached through the ENOENT + symlink branch, not + // through a synchronous throw that skips it. + // Same extra in-lock resolution as the case above: the recovery scan resolves the publish + // target before scanning for leftovers. The claim this case pins - the strict rejection + // happens inside the protected block, so the lock is released - is unchanged: the entry + // after "lock" is still a resolution that rejects, and "release" still follows it. + expect(order).toEqual(["lstat", "lock", "lstat", "lstat", "release"]) + }) + + it("canonicalizes the parent directory when the file itself is not there yet", async () => { + // fs.realpath canonicalizes every component, including a symlinked ancestor + // directory or a Windows 8.3 short name. If the fallback returns the alias + // directory, the key depends on whether the file exists at the moment the key + // is computed, and a writer that resolved the canonical directory takes a + // different lock for the same file. + const aliasDir = path.join(os.tmpdir(), "alias-dir") + const canonicalDir = path.join(os.tmpdir(), "canonical-dir") + const file = path.join(aliasDir, "history_item.json") + mockedRealpath.mockImplementation(async (target) => { + if (target === file) throw enoent + return canonicalDir + }) + mockedLstat.mockImplementation( + async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats, + ) + + expect(await resolveLockKey(file)).toBe(path.join(canonicalDir, "history_item.json")) + }) + + it("follows a chain of nine links to the file it ends at", async () => { + // Nine links, dangling at the far end. A walk that stops at eight leaves the key naming + // an intermediate link, so a writer that named the file through this chain queued on a + // different lock from a writer standing at the file itself - two locks for one file, + // reached by chain length instead of by spelling. + const chain = [ + "a0.json", + "a1.json", + "a2.json", + "a3.json", + "a4.json", + "a5.json", + "a6.json", + "a7.json", + "a8.json", + ] + const referent = path.resolve("/tmp/chain-dir", "history_item.json") + const targets = new Map() + chain.forEach((name, index) => { + targets.set( + path.resolve("/tmp/chain-dir", name), + index + 1 < chain.length ? path.resolve("/tmp/chain-dir", chain[index + 1]) : referent, + ) + }) + mockedRealpath.mockRejectedValue(enoent) + mockedLstat.mockImplementation( + async (target) => + ({ + isSymbolicLink: () => targets.has(String(target)), + isFile: () => !targets.has(String(target)), + }) as unknown as BigIntStats, + ) + mockedReadlink.mockImplementation(async (target) => { + const next = targets.get(String(target)) + return next === undefined ? Promise.reject(new Error("not a link")) : next + }) + + const viaChain = await resolveLockKey(path.resolve("/tmp/chain-dir", chain[0])) + + // One readlink per link, all nine, plus the probe that finds a non-link at the far end: + // the key names the file, not a link part-way down the chain. + expect(mockedReadlink).toHaveBeenCalledTimes(chain.length + 1) + expect(viaChain).toBe(referent) + }) + + it("heals the interrupted publish beside the referent while the alias dangles", async () => { + const linkDir = await makeDir("lockkey-link-") + const referentDir = await makeDir("lockkey-referent-") + const referent = path.join(referentDir, "history_item.json") + currentLink = path.join(linkDir, "link.json") + // An interrupted two-step replacement: the previous write renamed the committed file to + // its backup and never landed the replacement, so the committed content sits in a + // backup beside the referent while the referent itself is absent. The link was renamed + // away too, so the alias resolves nowhere. + const backupPath = path.join(referentDir, ".history_item.json.bak_previous.tmp") + await fs.writeFile(backupPath, JSON.stringify({ version: "committed" })) + // Neither spelling resolves while the referent is missing, which is what a dangling + // link looks like to realpath. Recovery restores the referent, and from then on both + // spellings resolve again. + mockedRealpath.mockImplementation(async (target) => { + if (!existsSync(referent)) { + throw enoent + } + return String(target) === currentLink ? referent : String(target) + }) + mockedLstat.mockImplementation(async (target) => symlinkStat(target)) + mockedReadlink.mockImplementation(async (target) => + target === currentLink ? referent : Promise.reject(new Error("not a link")), + ) + mockedAcquireFileLock.mockResolvedValue(async () => {}) + + await safeWriteJson(currentLink, { id: "task-1" }) + + // The lock key is the referent's key, and recovery looked beside the referent as well, + // so the interrupted publish's committed state was restored before this write decided + // what the file holds. Named at the dangling alias, this write queued on the referent's + // lock and healed the referent's leftovers instead of failing every retry. + expect(mockedAcquireFileLock).toHaveBeenCalledWith(referent) + expect(JSON.parse(await fs.readFile(referent, "utf8"))).toEqual({ id: "task-1" }) + expect(await fs.readdir(referentDir)).toEqual(["history_item.json"]) + expect(await fs.readdir(linkDir)).toEqual([]) + }) +}) + +it("does not log a cleanup error when the safety net finds the temp file already gone", async () => { + // safeWriteText removes its own temp file on failure, so the safety net in + // safeWriteJson normally finds it gone. That is the expected outcome, not a + // second failure, and it must not be logged as one. + const dir = await makeDir("cleanup-") + const target = path.join(dir, "history_item.json") + currentLink = "" + mockedRealpath.mockImplementation(async (t) => String(t)) + mockedLstat.mockImplementation(async (t) => symlinkStat(t)) + + const renameSpy = vi.spyOn(fs, "rename").mockRejectedValue(new Error("commit rename failed")) + const unlinkSpy = vi.spyOn(fs, "unlink").mockRejectedValue(enoent) + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow("commit rename failed") + + // Only the original failure is reported. + expect(consoleError).toHaveBeenCalledTimes(1) + + renameSpy.mockRestore() + unlinkSpy.mockRestore() + consoleError.mockRestore() +}) + +describe("safeWriteJson confinement when the alias moves under the lock", () => { + it("refuses a confined write whose alias moved outside the declared scope while the lock was held, without scanning or editing the directory it would have recovered from", async () => { + const order: string[] = [] + const scope = await makeDir("lockkey-scope-") + const outside = await makeDir("lockkey-outside-") + const insideReferent = path.join(scope, "item.json") + const outsideReferent = path.join(outside, "item.json") + currentLink = path.join(scope, "link.json") + + // A leftover beside the outside referent, named the way jsonFileRecovery names a + // staged copy, so the unchanged-listing assertion below is not vacuous: had recovery + // run beside this directory it would have scanned this entry and renamed or deleted it. + const leftover = path.join(outside, ".item.json.new_planted.tmp") + await fs.writeFile(leftover, "staged") + const outsideBefore = (await fs.readdir(outside)).sort() + + // The alias moves inside the lock acquisition below, so the resolution that runs after + // the lock is taken names a directory outside the declared scope. + let flipped = false + + // Identity resolutions: each helper sees exactly the path it was handed, so the scope + // comparison is between the two directories this row built. + // realpath follows the alias the way a real one would: it names the referent the link + // currently points at, which is the answer every helper below gets. The alias is the only + // path whose answer changes during the row. + mockedRealpath.mockImplementation(async (target) => { + order.push("resolve") + if (String(target) === currentLink) { + return flipped ? outsideReferent : insideReferent + } + return String(target) + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + // The alias points inside the declared scope while the lock key is resolved and while + // the lock is taken. It moves while this writer holds the lock, which is the window + // the confinement contract has to cover: recovery scans beside whatever the alias + // names afterwards, so a decision taken before the mutation is the only one that can + // still refuse without having already edited a foreign directory. + mockedReadlink.mockImplementation(async (target) => { + if (target !== currentLink) { + return Promise.reject(new Error("not a link")) + } + return flipped ? outsideReferent : insideReferent + }) + let released = false + mockedAcquireFileLock.mockImplementation(async (key) => { + order.push("lock:" + path.basename(String(key))) + flipped = true + return async () => { + order.push("release") + released = true + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + // Refused before recovery mutated anything: the outside directory is the directory it + // was, planted staged copy included, and the lock this writer took is released rather + // than left for the stale timeout. + expect((await fs.readdir(outside)).sort()).toEqual(outsideBefore) + expect(existsSync(leftover)).toBe(true) + expect(released).toBe(true) + expect(order.filter((entry) => entry === "release")).toHaveLength(1) + expect(order.filter((entry) => entry === "lock:item.json")).toHaveLength(1) + }) +}) + +describe("safeWriteJson refuses a lock whose identity moved out from under it", () => { + it("refuses an alias that moved to another in-scope target while this writer waited for the lock", async () => { + const order: string[] = [] + const scope = await makeDir("lockkey-identity-") + const lockedTarget = path.join(scope, "item.json") + const movedTarget = path.join(scope, "moved.json") + currentLink = path.join(scope, "link.json") + + // The second target lies INSIDE the declared scope, so every confinement check this + // write runs passes for it, and the only thing that can tell the file the lock was + // taken on apart from the file the alias now names is the identity comparison. A + // staged copy is planted beside it, named the way jsonFileRecovery names one, so the + // assertions below are not vacuous: had this write gone on, the recovery scan would + // have found this entry and the commit rename would have landed on the file it + // belongs to. + await fs.writeFile(lockedTarget, JSON.stringify({ which: "locked" })) + await fs.writeFile(movedTarget, JSON.stringify({ which: "moved" })) + const movedLeftover = path.join(scope, ".moved.json.new_planted.tmp") + await fs.writeFile(movedLeftover, "staged") + const scopeBefore = (await fs.readdir(scope)).sort() + + // The alias moves inside the lock acquisition below: the key this writer took names + // item.json, and every resolution that runs after the lock is held names moved.json + // instead. + let flipped = false + mockedRealpath.mockImplementation(async (target) => { + order.push("resolve") + if (String(target) === currentLink) { + return flipped ? movedTarget : lockedTarget + } + return String(target) + }) + mockedLstat.mockImplementation(async (target) => { + order.push("lstat") + return symlinkStat(target) + }) + mockedReadlink.mockImplementation(async (target) => { + if (target !== currentLink) { + return Promise.reject(new Error("not a link")) + } + return flipped ? movedTarget : lockedTarget + }) + let released = false + let lockTakes = 0 + mockedAcquireFileLock.mockImplementation(async (key) => { + lockTakes += 1 + order.push("lock:" + path.basename(String(key))) + flipped = true + return async () => { + order.push("release") + released = true + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" }, { confineTo: scope })).rejects.toThrow( + LockIdentityChangedError, + ) + + // Refused outright rather than retried under a new key, and refused before the + // recovery pass ran: the scope directory is what it was, the staged copy beside the + // second target is neither removed nor overwritten, and neither target carries this + // write's payload. + expect((await fs.readdir(scope)).sort()).toEqual(scopeBefore) + expect(existsSync(movedLeftover)).toBe(true) + expect(JSON.parse(await fs.readFile(movedTarget, "utf8"))).toEqual({ which: "moved" }) + expect(JSON.parse(await fs.readFile(lockedTarget, "utf8"))).toEqual({ which: "locked" }) + + // The lock this writer took is released exactly once, by the finally block, rather + // than left for the stale timeout, and it was taken on the identity it chose. + expect(released).toBe(true) + expect(lockTakes).toBe(1) + expect(order.filter((entry) => entry === "release")).toHaveLength(1) + expect(order.filter((entry) => entry === "lock:item.json")).toHaveLength(1) + }) + + it("refuses the publication when the alias is retargeted onto another in-scope target after the recovery pass", async () => { + const scope = await makeDir("lockkey-recovery-") + const lockedTarget = path.join(scope, "item.json") + const movedTarget = path.join(scope, "moved.json") + currentLink = path.join(scope, "link.json") + + await fs.writeFile(lockedTarget, JSON.stringify({ which: "locked" })) + await fs.writeFile(movedTarget, JSON.stringify({ which: "moved" })) + // A staged copy that belongs to the SECOND target. The recovery pass scans beside the + // file the alias named when the key was computed, and jsonFileRecovery treats a + // leftover as belonging only to the target its name was derived from, so this entry is + // still here - and still active - when the re-resolution that names the publish target + // happens. + const movedLeftover = path.join(scope, ".moved.json.new_planted.tmp") + await fs.writeFile(movedLeftover, "staged") + const scopeBefore = (await fs.readdir(scope)).sort() + + // This row declares confineTo, so the alias is resolved four times before this write + // could publish, and the fourth resolution is the one the ask names: the first answers + // the confinement check that runs before the lock is taken, the second computes the + // lock key, the third chooses the directory the recovery pass scans, and the fourth is + // the re-resolution that names the merge target and the commit-rename target. Flipping + // as that fourth answer is produced puts the move after the recovery call and before + // the merge read, the staging, and the rename. The count is asserted below so that a + // change in this order fails the row loudly instead of moving the flip into a + // different window unnoticed. + let aliasAnswers = 0 + let flipped = false + mockedRealpath.mockImplementation(async (target) => { + if (String(target) === currentLink) { + aliasAnswers += 1 + if (aliasAnswers === 4) { + flipped = true + } + return flipped ? movedTarget : lockedTarget + } + return String(target) + }) + mockedLstat.mockImplementation(async (target) => symlinkStat(target)) + mockedReadlink.mockImplementation(async (target) => { + if (target !== currentLink) { + return Promise.reject(new Error("not a link")) + } + return flipped ? movedTarget : lockedTarget + }) + let released = false + let lockTakes = 0 + mockedAcquireFileLock.mockImplementation(async () => { + lockTakes += 1 + return async () => { + released = true + } + }) + + await expect(safeWriteJson(currentLink, { id: "task-1" }, { confineTo: scope })).rejects.toThrow( + LockIdentityChangedError, + ) + + // The recovery pass had already run beside the locked file, and this write still + // refused before reading, staging, or renaming anything: the staged copy beside the + // second target survives untouched, neither target carries this write's payload, and + // the scope listing is what it was. + expect((await fs.readdir(scope)).sort()).toEqual(scopeBefore) + expect(existsSync(movedLeftover)).toBe(true) + expect(JSON.parse(await fs.readFile(movedTarget, "utf8"))).toEqual({ which: "moved" }) + expect(JSON.parse(await fs.readFile(lockedTarget, "utf8"))).toEqual({ which: "locked" }) + expect(released).toBe(true) + expect(lockTakes).toBe(1) + expect(aliasAnswers).toBe(4) + }) +}) diff --git a/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts b/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts new file mode 100644 index 0000000000..32cfb31eb2 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts @@ -0,0 +1,186 @@ +// npx vitest run utils/__tests__/safeWriteJson.postCommitDurability.spec.ts + +import * as fsSync from "fs" +import type { WriteStream as FsWriteStream } from "fs" +import * as os from "os" +import path from "path" + +import * as fs from "fs/promises" + +import { safeWriteJson } from "../safeWriteJson" + +// Real filesystem throughout: what this spec has to prove is what the target's directory +// actually holds after a durability failure, and a fully mocked fs cannot show that. +// Only the sync primitives this module reaches for are wrapped - openSync for the +// post-commit directory fsync and createWriteStream for the staged write - and each +// wrapper delegates to the real implementation for every path except the one a test +// deliberately fails. The real implementations are captured inside the mock factory, +// before any vi.fn() wrapper exists, so a test's implementation callback can delegate +// without recursing into itself. +type OpenSyncRest = (...args: unknown[]) => number +// The options type carries the fs option as well: the staged write below fails at its own +// open by handing Node's WriteStream an fs whose open never succeeds. +// The hooks Node's WriteStream calls on a caller-supplied fs object, spelled out in the +// callback form Node itself uses. They are not named out of @types/node: this tree's +// TypeScript reports TS2694 for both CreateWriteStreamFSImplementation and the +// WriteStreamOptions type that carries it under the "fs" spelling, and the +// promise-flavoured typeof of an fs function carries a __promisify__ member a plain stand-in +// does not have (TS2741), so the bag is written from the call shapes instead. +type WriteStreamFsHooks = { + fs?: { + open?: ( + path: string | Buffer | URL, + flags: string, + mode: number | null | undefined, + callback: (err: Error | null, fd: number) => void, + ) => void + close?: (fd: number, callback: (err?: Error | null) => void) => void + write: ( + fd: number, + buffer: Uint8Array | string, + offset: number | null | undefined, + length: number, + position: number | null | undefined, + callback: (err: Error | null, bytesWritten: number) => void, + ) => void + } | null +} +type WriteStreamRest = (filePath: string, options: { encoding: string } & WriteStreamFsHooks) => FsWriteStream +const actuals = vi.hoisted( + (): { + openSync: OpenSyncRest | null + writeStream: WriteStreamRest | null + } => ({ openSync: null, writeStream: null }), +) + +vi.mock("fs", async () => { + const actual = await vi.importActual("fs") + // Node declares both functions as sets of overloads; capturing each as a rest-args or + // narrowed function is what lets this spec forward the arguments unchanged instead of + // re-declaring them. + actuals.openSync = actual.openSync as OpenSyncRest + actuals.writeStream = actual.createWriteStream as unknown as WriteStreamRest + return { + ...actual, + openSync: vi.fn(actual.openSync), + createWriteStream: vi.fn(actual.createWriteStream as unknown as WriteStreamRest), + } +}) + +// The advisory lock is not under test here; the real proper-lockfile would add a lock +// directory to the very directory this spec inspects for residue. +vi.mock("../fileLock", () => ({ + acquireFileLock: vi.fn(async () => async () => {}), +})) + +describe("safeWriteJson post-commit durability", () => { + let dir: string + // safeWriteJson derives the directory it fsyncs from the RESOLVED target, so the + // injected failure has to be keyed to the same spelling realpath reports. + let dirKey: string + let targetPath: string + let originalPlatform: NodeJS.Platform + + beforeEach(async () => { + originalPlatform = process.platform + // The parent-directory fsync (_flushDirectoryToDisk) is POSIX-only: Windows cannot + // open a directory for this purpose, so the branch is only reachable when the + // process reports a POSIX platform. + Object.defineProperty(process, "platform", { value: "linux", configurable: true }) + + dir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-json-postcommit-")) + dirKey = await fs.realpath(dir) + targetPath = path.join(dirKey, "history_item.json") + await fs.writeFile(targetPath, JSON.stringify({ version: "old" })) + }) + + afterEach(async () => { + Object.defineProperty(process, "platform", { value: originalPlatform, configurable: true }) + vi.restoreAllMocks() + await fs.rm(dir, { recursive: true, force: true }) + }) + + it("releases every staging artifact when the post-commit directory fsync fails, and reports the write as committed", async () => { + // The commit rename already published the new JSON; only the durability of the + // directory entry that records the rename is in doubt. This module owns that fsync, + // and it swallows the failure rather than reporting a completed write as a failed + // one: the content is at the target either way, so a rejection would leave in-memory + // state diverging from disk. The staging file (and the backup copy, when the write + // took one) is still removed - leaving them behind would mean every affected write + // leaks another hidden copy next to the target. + let dirOpenAttempted = false + const openSyncActual = actuals.openSync + if (openSyncActual === null) { + throw new Error("fs mock did not capture the real openSync") + } + vi.mocked(fsSync.openSync).mockImplementation((...args: Parameters) => { + if (String(args[0]) === dirKey) { + // Pin the injection: without this flag the test would also pass on a run where + // the directory sync never fired at all, which proves nothing about the path + // that the failure handler takes. + dirOpenAttempted = true + throw Object.assign(new Error("EINVAL: invalid argument, open '" + dirKey + "'"), { code: "EINVAL" }) + } + return openSyncActual(...args) + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await safeWriteJson(targetPath, { version: "new" }) + + expect(dirOpenAttempted).toBe(true) + expect(JSON.parse(await fs.readFile(targetPath, "utf8"))).toEqual({ version: "new" }) + // The swallowed error is still reported to the log: the directory entry is not known + // to be durable, and a caller that cannot see that would over-claim the write. + expect(warnSpy).toHaveBeenCalledTimes(1) + expect(errorSpy).not.toHaveBeenCalled() + // No backup copy, no staging temp, no staging directory: only the published file. + expect(await fs.readdir(dir)).toEqual(["history_item.json"]) + }) + + it("still rejects a failure before the commit rename, and leaves no residue", async () => { + // The post-commit leniency must stay narrow: a failure that happens before the commit + // has not published anything, and swallowing it would report a write that never ran. + // This module writes the staged file through a stream, so the pre-commit failure is + // injected on that stream. + let stagedOpenAttempted = false + const writeStreamActual = actuals.writeStream + if (writeStreamActual === null) { + throw new Error("fs mock did not capture the real createWriteStream") + } + // The open that never succeeds, in the callback form Node's WriteStream calls it in. + const failOpen = (...args: unknown[]): void => { + const callback = args.at(-1) + if (typeof callback === "function") { + process.nextTick(() => + callback(Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })), + ) + } + } + vi.mocked(fsSync.createWriteStream).mockImplementation( + (...args: Parameters) => { + const stagedPath = String(args[0]) + if (stagedPath.includes(".new_")) { + // Pin the injection: without this flag the test would also pass on a run where + // the write never reached a staged file at all. + stagedOpenAttempted = true + // Fail the staged file at its open rather than emitting a synthetic error after a + // successful one. A real open creates the name before anything can fail, so the + // residue assertion below would then depend on when the cleanup happened to run; + // an open that never succeeds means the name is never created at all. + return writeStreamActual(stagedPath, { encoding: "utf8", fs: { ...fsSync, open: failOpen } }) + } + return writeStreamActual(stagedPath, { encoding: "utf8" }) + }, + ) + + await expect(safeWriteJson(targetPath, { version: "new" })).rejects.toThrow("EACCES: permission denied") + + expect(stagedOpenAttempted).toBe(true) + // Nothing was published and nothing was staged: the target still holds the previous + // content, and the staged file's name never came into existence because its open is + // what failed. + expect(JSON.parse(await fs.readFile(targetPath, "utf8"))).toEqual({ version: "old" }) + expect(await fs.readdir(dir)).toEqual(["history_item.json"]) + }) +}) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 30ac500212..a2eea61dd4 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -3,6 +3,7 @@ import { Writable } from "stream" import * as path from "path" import * as os from "os" +import { ConfinedPathEscapeError } from "../safeWriteJson" import { safeWriteJson } from "../safeWriteJson" // Capture actual implementations before the vi.mock factory runs, @@ -12,6 +13,8 @@ const fsPromisesActuals = vi.hoisted(() => ({ rename: undefined as (typeof import("fs/promises"))["rename"] | undefined, unlink: undefined as (typeof import("fs/promises"))["unlink"] | undefined, writeFile: undefined as (typeof import("fs/promises"))["writeFile"] | undefined, + chmod: undefined as (typeof import("fs/promises"))["chmod"] | undefined, + stat: undefined as (typeof import("fs/promises"))["stat"] | undefined, })) vi.mock("fs/promises", async () => { @@ -19,6 +22,8 @@ vi.mock("fs/promises", async () => { fsPromisesActuals.rename = actual.rename fsPromisesActuals.unlink = actual.unlink fsPromisesActuals.writeFile = actual.writeFile + fsPromisesActuals.chmod = actual.chmod + fsPromisesActuals.stat = actual.stat // Start with all actual implementations. const mockedFs = { ...actual, @@ -28,6 +33,11 @@ vi.mock("fs/promises", async () => { unlink: vi.fn(actual.unlink), access: vi.fn(actual.access), stat: vi.fn(actual.stat), + // chmod defaults to the actual chmod, so a test that does not override it really + // copies the mode and cannot have its chmod silently swallowed; a test that wants a + // refused copy must override it to throw, which is what the mode-copy rows at the end + // of this file do. + chmod: vi.fn(actual.chmod), open: vi.fn(actual.open), mkdtemp: vi.fn(actual.mkdtemp), rm: vi.fn(actual.rm), @@ -818,4 +828,504 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual(newData) }) + + // The commit rename targets the symlink referent. The staged temp file must + // therefore be created beside the RESOLVED target — staging beside the link + // would make the commit rename fail with EXDEV when the referent is on + // another filesystem. (Real symlinks are unavailable in this CI lane, so the + // resolution is simulated by mocking fs.realpath the same way.) + test("stages the temp file beside the symlink referent and commits onto it", async () => { + const referentDir = path.join(tempDir, "referent") + const linkDir = path.join(tempDir, "link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "test-file.json") + const referentPath = path.join(referentDir, "test-file.json") + // Seed the RESOLVED referent with real content (via the actual fs) so the + // write exercises replacement of an EXISTING referent: the lock, the + // backup, and the commit all target the resolved referent. + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: true })) + + // Only the file resolves through the link; the directory is already canonical, + // so the lock key is the referent rather than the alias directory + basename. + vi.spyOn(fs, "realpath").mockImplementation(async (target) => + target === callerPath ? referentPath : String(target), + ) + + await safeWriteJson(callerPath, { after: true }) + + // the temp file was created next to the resolved referent, NOT beside the link + const tempPaths = vi.mocked(fsSyncActual.createWriteStream).mock.calls.map((call) => String(call[0])) + expect(tempPaths.some((p) => p.startsWith(referentDir + path.sep) && p.includes(".new_"))).toBe(true) + expect(tempPaths.some((p) => p.startsWith(linkDir + path.sep))).toBe(false) + + // the content was committed onto the referent + expect(await readFileContent(referentPath)).toEqual({ after: true }) + }) + + // proper-lockfile with realpath:false keys the lock by the given path, so a + // symlink alias and its referent must coordinate through ONE lock on the + // resolved referent — otherwise a concurrent merge through both aliases + // reads the same JSON and overwrites one update. (Real symlinks are + // unavailable in this CI lane, so the resolution is simulated by mocking + // fs.realpath, the same way as the staging test above.) + test("acquires the lock on the resolved referent, not the caller alias", async () => { + vi.resetModules() // fresh module instances so the doMock below is picked up + + const referentDir = path.join(tempDir, "lock-referent") + const linkDir = path.join(tempDir, "lock-link") + await fs.mkdir(referentDir, { recursive: true }) + await fs.mkdir(linkDir, { recursive: true }) + // caller-visible path (the link) vs the resolved referent path + const callerPath = path.join(linkDir, "locked.json") + const referentPath = path.join(referentDir, "locked.json") + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: 1 })) + + // Only the file resolves through the link; the directory is already canonical, + // so the lock key is the referent rather than the alias directory + basename. + const realpathSpy = vi + .spyOn(fs, "realpath") + .mockImplementation(async (target) => (target === callerPath ? referentPath : String(target))) + + // Wrap the real lock in a capturing mock, and drive the two rare error paths + // (the onCompromised callback and a failing release) so they stay covered + // without real lockfile staleness. The callback rethrows by design, so + // the mock swallows that throw and lets the real lock proceed. + const realLockfile = await vi.importActual("proper-lockfile") + const lockMockFn = vi.fn( + async ( + file: Parameters[0], + options?: Parameters[1], + ) => { + try { + options?.onCompromised?.(new Error("lock compromised (test)")) + } catch { + // onCompromised rethrows by design; swallow so the real lock proceeds. + } + const release = await realLockfile.lock(file, options) + return async () => { + await release() + throw new Error("release failed (test)") + } + }, + ) + const lockMock = lockMockFn as unknown as typeof realLockfile.lock + vi.doMock("proper-lockfile", () => ({ + ...realLockfile, + lock: lockMock, + })) + + // Re-import safeWriteJson so it picks up the mocked proper-lockfile. + const { safeWriteJson: mockedSafeWriteJson } = await import("../safeWriteJson") + + const mergeFn = vi.fn((existing: unknown, incoming: unknown) => ({ + ...(existing as Record), + ...(incoming as Record), + })) + + // Capture the compromise + release-failure logs. + const consoleErrorSpy = vi.spyOn(console, "error") + try { + await mockedSafeWriteJson(callerPath, { added: true }, { merge: mergeFn }) + + // The lock was keyed by the resolved referent — every alias shares it. + expect(lockMock).toHaveBeenCalledTimes(1) + expect(String(lockMockFn.mock.calls[0][0])).toBe(referentPath) + // The merge read the referent's content through that single lock. + expect(mergeFn).toHaveBeenCalledWith({ seed: 1 }, { added: true }) + expect(await readFileContent(referentPath)).toEqual({ seed: 1, added: true }) + // The compromise callback and the failed release were logged, not thrown. + expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("was compromised"), expect.any(Error)) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to release lock"), + expect.any(Error), + ) + } finally { + // Cleanup must run even when an assertion fails: a leaked mock + // registration or console spy changes later tests, and vi.unmock + // alone does not reset a module that already imported the mock. + realpathSpy.mockRestore() + vi.unmock("proper-lockfile") + vi.resetModules() + consoleErrorSpy.mockRestore() + } + }) + + // CWE-732 regression: safeWriteJson stages the temp itself and passes it + // via tempPath, so safeWriteText must apply the existing target's mode to + // the staged temp before the atomic rename — otherwise a 0o600 target is + // published as 0o644. POSIX-only assertion (Windows ignores POSIX modes). + test("rejects a confined write whose target is outside the confined directory", async () => { + const scope = path.join(tempDir, "project") + await fs.mkdir(scope) + const outside = path.join(tempDir, "elsewhere.json") + + // No symlink needed: the check runs on the resolved publish target, so an + // out-of-scope path is rejected on every platform, and it is rejected before the + // lock is taken and before anything is staged. + await expect(safeWriteJson(outside, { mcpServers: {} }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + const left = await fs.readdir(tempDir) + expect(left).not.toContain("elsewhere.json") + expect(left.filter((entry) => entry.includes(".new_") || entry.endsWith(".lock"))).toEqual([]) + }) + + test("rejects an out-of-scope target before the advisory lock is taken", async () => { + const projectDir = path.join(tempDir, "scope") + await fs.mkdir(projectDir) + const outside = path.join(tempDir, "elsewhere.json") + + // Capture every lock attempt without changing how locking works. The lock file + // lives NEXT TO the key, so taking the lock on an out-of-scope referent already + // writes outside the scope the caller declared - and an unwritable directory + // there would surface a lock error instead of the confinement verdict. + const realLockfile = await vi.importActual("proper-lockfile") + const lockCalls: string[] = [] + const lockMock = vi.fn(async (file: unknown, options?: unknown) => { + lockCalls.push(String(file)) + return realLockfile.lock(file as never, options as never) + }) + vi.doMock("proper-lockfile", () => ({ + ...realLockfile, + lock: lockMock as unknown as typeof realLockfile.lock, + })) + + // Re-import the whole graph so the SUT AND its fileLock dependency pick up the + // wrapped lock; without the reset, fileLock keeps the already-loaded real + // proper-lockfile and the capture never sees a call. The error class is taken from + // the same module instance the SUT came from. + vi.resetModules() + const { safeWriteJson: reimported, ConfinedPathEscapeError: ReimportedError } = await import("../safeWriteJson") + + try { + await expect(reimported(outside, { mcpServers: {} }, { confineTo: projectDir })).rejects.toThrow( + ReimportedError, + ) + + // The confinement verdict came first: no lock was ever attempted. + expect(lockCalls).toEqual([]) + expect( + (await fs.readdir(tempDir)).filter(function (entry: string) { + return entry.endsWith(".lock") + }), + ).toEqual([]) + } finally { + vi.doUnmock("proper-lockfile") + vi.resetModules() + } + }) + + test("rejects an out-of-scope target before creating any directory", async () => { + const projectDir = path.join(tempDir, "scope") + await fs.mkdir(projectDir) + // The target lives in a directory that does not exist. A confinement check that runs + // after the mkdir -p would create that directory OUTSIDE the declared scope first - + // the same side effect the pre-lock check exists to prevent, reached through a + // symlinked ancestor in the reported case - and an unwritable referent would surface + // the mkdir error instead of the confinement verdict. + const missingParent = path.join(tempDir, "no-such-dir") + const outside = path.join(missingParent, "elsewhere.json") + + await expect(safeWriteJson(outside, { mcpServers: {} }, { confineTo: projectDir })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + // Nothing was created outside the scope, and no lock was attempted there. + expect(fsSyncActual.existsSync(missingParent)).toBe(false) + expect( + (await fs.readdir(tempDir)).filter(function (entry: string) { + return entry.endsWith(".lock") + }), + ).toEqual([]) + }) + + test.skipIf(process.platform === "win32")( + "rejects a confined write whose symlink resolves outside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project") + await fs.mkdir(projectDir) + const outside = path.join(tempDir, "outside.json") + await fsSyncActual.promises.writeFile(outside, JSON.stringify({ secret: "original" }), "utf8") + // A repository that plants its project settings file as a link to somewhere else + // must not receive the settings write at the linked path. The caller picked + // projectDir/mcp.json from the workspace, so it declares that scope. + const projectConfig = path.join(projectDir, "mcp.json") + await fs.symlink(outside, projectConfig) + + await expect(safeWriteJson(projectConfig, { mcpServers: {} }, { confineTo: projectDir })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + // The linked file is untouched and nothing was staged beside it. + expect(JSON.parse(await fsSyncActual.promises.readFile(outside, "utf8"))).toEqual({ + secret: "original", + }) + const entries = await fs.readdir(tempDir) + expect(entries).toContain("outside.json") + expect( + entries.filter( + (entry) => entry.includes(".new_") || entry.includes("safeWriteText") || entry.endsWith(".lock"), + ), + ).toEqual([]) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a write whose symlink referent stays inside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project-in") + await fs.mkdir(projectDir) + const referent = path.join(projectDir, "real-mcp.json") + await fsSyncActual.promises.writeFile(referent, JSON.stringify({ mcpServers: {} }), "utf8") + const alias = path.join(projectDir, "mcp.json") + await fs.symlink(referent, alias) + + // Confining is about the scope, not about forbidding links: a link that stays + // inside the project still publishes to its referent. + await safeWriteJson( + alias, + { mcpServers: { local: { url: "http://localhost" } } }, + { confineTo: projectDir }, + ) + + expect(JSON.parse(await fsSyncActual.promises.readFile(referent, "utf8"))).toEqual({ + mcpServers: { local: { url: "http://localhost" } }, + }) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a scope path that itself runs through a symlink and does not exist yet", + async () => { + const real = path.join(tempDir, "real-project") + await fs.mkdir(real) + const alias = path.join(tempDir, "alias-project") + await fs.symlink(real, alias) + // The scope is declared through the alias, and the directory it names does not + // exist yet. Resolving it lexically would compare an unresolved scope against a + // fully resolved target and reject a write that is in fact inside the project - + // the macOS /var -> /private/var shape. The nearest existing ancestor is resolved + // and the remainder re-joined instead. + const nested = path.join(alias, "nested") + const target = path.join(nested, "mcp.json") + + await safeWriteJson(target, { mcpServers: {} }, { confineTo: nested }) + + expect( + JSON.parse(await fsSyncActual.promises.readFile(path.join(real, "nested", "mcp.json"), "utf8")), + ).toEqual({ mcpServers: {} }) + }, + ) + + test.skipIf(process.platform === "win32")( + "preserves a restrictive 0o600 target mode through the atomic publish", + async () => { + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify({ before: true })) + fsSyncActual.chmodSync(currentTestFilePath, 0o600) + + await safeWriteJson(currentTestFilePath, { after: true }) + + expect(fsSyncActual.statSync(currentTestFilePath).mode & 0o777).toBe(0o600) + expect(await readFileContent(currentTestFilePath)).toEqual({ after: true }) + }, + ) + + // A commit failure that is not EPERM never reaches the fallback, so no backup rename + // has happened: the target keeps its previous content, the staged file is cleaned up, + // and the original error is what the caller sees. + test("a failed commit keeps the previous content at the target and leaves no backup behind", async () => { + const initialData = { message: "Initial, must survive a failed commit" } + const newData = { message: "New content" } + + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error + + // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn + let renameCallCount = 0 + vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { + renameCallCount++ + if (renameCallCount === 1) { + // The commit rename fails with a non-EPERM error, so no fallback runs. + throw new Error("Primary rename failed") + } + return fsPromisesActuals.rename!(oldPath, newPath) + }) + + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") + + // Exactly one rename was attempted, and it was the commit. + expect(renameCallCount).toBe(1) + + // The target never left its path, so the previous content is still what a + // reader sees, and no orphaned backup copy is left behind either. + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(initialData) + const leftovers = (await fs.readdir(tempDir)).filter( + (entry) => entry.includes(".bak_") || entry.includes(".new_") || entry.includes("safeWriteText.bak_"), + ) + expect(leftovers).toEqual([]) + + consoleErrorSpy.mockRestore() + }) + + // CWE-732 follow-up on the publish-time mode copy, placed at the layer the failure lives + // in: this is _copyTargetMode's own reaction, not an integration or e2e wrapper. The staged + // mode in the first two rows is what those rows report through the double, not what a host + // measured, so they prove how the code reacts to a refused or unreadable mode read and prove + // nothing about how a real filesystem would report a mode-copy failure. They run on every + // platform, Windows included, because the mode numbers they compare are supplied here rather + // than asked of the host - which is why the 0o600-preservation row above is + // test.skipIf(process.platform === "win32") and these are not. + + test("refuses the publish when the staged file will not take the target's mode", async () => { + const initialData = { message: "Initial, must survive a refused mode copy" } + const newData = { message: "New content" } + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + + const targetStats = await fsPromisesActuals.stat!(currentTestFilePath) + const targetMode = targetStats.mode & 0o777 + // A mode the publish cannot reproduce, picked so that it differs from whatever the host + // reports for the target on any platform. + const otherMode = targetMode ^ 0o077 + const refusal = Object.assign(new Error("EPERM: operation not permitted, chmod"), { code: "EPERM" }) + // The publish reads only .mode off the stats object, so a copy of the target's real stats + // carrying a different mode is enough to drive the branch. + const statsWithMode = (mode: number): Awaited> => ({ ...targetStats, mode }) + + let stagedPath = "" + // The module-level chmod double keeps its call history across rows in this file, so the + // count below is measured from here and not from the start of the run. + const chmodSpy = vi + .mocked(fs.chmod) + .mockClear() + .mockImplementation(async (filePath) => { + stagedPath = String(filePath) + throw refusal + }) + const statSpy = vi.mocked(fs.stat).mockImplementation(async (filePath) => { + // The target is stat'd first, so the mode the publish tries to copy is the real one; + // the staged file is stat'd only inside the refused-copy branch, where this answers. + if (stagedPath !== "" && String(filePath) === stagedPath) { + return statsWithMode(otherMode) + } + return fsPromisesActuals.stat!(filePath) + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow(refusal) + + // One attempt, on the staged file, asking for the mode the target carries. + expect(chmodSpy).toHaveBeenCalledTimes(1) + expect(chmodSpy.mock.calls[0]?.[1]).toBe(targetMode) + expect(stagedPath).not.toBe(currentTestFilePath) + // The refusal is logged with the wording the publish emits at safeWriteJson.ts:491. + expect(consoleErrorSpy.mock.calls.some((call) => String(call[0]).includes("Failed to copy mode"))).toBe(true) + // The target never left its path, so a reader still sees the previous content, and the + // staged file went away through the caller's failure path rather than being left beside it. + expect(await readFileContent(currentTestFilePath)).toEqual(initialData) + const leftovers = (await fs.readdir(tempDir)).filter( + (entry) => entry.includes(".new_") || entry.includes(".bak_"), + ) + expect(leftovers).toEqual([]) + + chmodSpy.mockRestore() + statSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) + + test("publishes when the copy is refused but the staged file already carries the target's mode", async () => { + const initialData = { message: "Initial, must survive a refused copy that changed nothing" } + const newData = { message: "New content" } + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + + const targetStats = await fsPromisesActuals.stat!(currentTestFilePath) + const targetMode = targetStats.mode & 0o777 + const refusal = Object.assign(new Error("EPERM: operation not permitted, chmod"), { code: "EPERM" }) + const statsWithMode = (mode: number): Awaited> => ({ ...targetStats, mode }) + + let stagedPath = "" + // The module-level chmod double keeps its call history across rows in this file, so the + // count below is measured from here and not from the start of the run. + const chmodSpy = vi + .mocked(fs.chmod) + .mockClear() + .mockImplementation(async (filePath) => { + stagedPath = String(filePath) + throw refusal + }) + const statSpy = vi.mocked(fs.stat).mockImplementation(async (filePath) => { + // The staged file reports the target's own mode, as a umask that already produced it + // or a filesystem that ignores POSIX modes would. + if (stagedPath !== "" && String(filePath) === stagedPath) { + return statsWithMode(targetMode) + } + return fsPromisesActuals.stat!(filePath) + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(currentTestFilePath, newData)).resolves.toBeUndefined() + + expect(chmodSpy).toHaveBeenCalledTimes(1) + // Both lines the publish emits on this path, in the order it emits them: the + // already-matches line at safeWriteJson.ts:494-496, then the failed-copy line at 497. + const messages = consoleErrorSpy.mock.calls.map((call) => String(call[0])) + const copiedNoMode = messages.findIndex((message) => message.includes("Copied no mode onto")) + const failedCopy = messages.findIndex((message) => message.includes("Failed to copy mode")) + expect(copiedNoMode).toBeGreaterThanOrEqual(0) + expect(failedCopy).toBeGreaterThanOrEqual(0) + expect(copiedNoMode).toBeLessThan(failedCopy) + // The publish still landed, and the staged file is not left beside the target. + expect(await readFileContent(currentTestFilePath)).toEqual(newData) + const leftovers = (await fs.readdir(tempDir)).filter( + (entry) => entry.includes(".new_") || entry.includes(".bak_"), + ) + expect(leftovers).toEqual([]) + + chmodSpy.mockRestore() + statSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) + + test("publishes with the staged file's own mode when the target's mode cannot be read", async () => { + const initialData = { message: "Initial, must survive an unreadable target mode" } + const newData = { message: "New content" } + await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) + + const unreadable = Object.assign(new Error("EACCES: permission denied, stat"), { code: "EACCES" }) + const statSpy = vi.mocked(fs.stat).mockImplementation(async (filePath) => { + // The publish stats the target before it stats anything else, and stats the staged + // file only inside the copy branch, so the target's basename separates the two calls + // without depending on how this host spells the temporary file's name. + if (path.basename(String(filePath)) === path.basename(currentTestFilePath)) { + throw unreadable + } + return fsPromisesActuals.stat!(filePath) + }) + // Same reason as the two rows above: this row asserts that no chmod happens at all, so + // its count starts here. + const chmodSpy = vi.mocked(fs.chmod).mockClear() + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(safeWriteJson(currentTestFilePath, newData)).resolves.toBeUndefined() + + // Logged with the wording emitted at safeWriteJson.ts:475, and the write went on without + // a mode copy: the function returns at 477 with the mode left unset, so 480 is never + // reached and no chmod of any kind happens. + expect(consoleErrorSpy.mock.calls.some((call) => String(call[0]).includes("Could not read the mode of"))).toBe( + true, + ) + expect(chmodSpy).not.toHaveBeenCalled() + expect(await readFileContent(currentTestFilePath)).toEqual(newData) + const leftovers = (await fs.readdir(tempDir)).filter( + (entry) => entry.includes(".new_") || entry.includes(".bak_"), + ) + expect(leftovers).toEqual([]) + + statSpy.mockRestore() + consoleErrorSpy.mockRestore() + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e54b8c51d3..43d6df789d 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -6,6 +6,7 @@ import { JsonStreamStringify } from "json-stream-stringify" import { getErrorCode } from "./errorCode" import { acquireFileLock } from "./fileLock" import { recoverInterruptedReplacement } from "./jsonFileRecovery" +import { resolveLockKey, resolvePublishTarget } from "../services/file-safety/safeWriteText" /** * Stages reported through the optional `trace` callback. They mark the @@ -46,6 +47,103 @@ export interface SafeWriteJsonOptions { * to emit diagnostic trace lines; it never influences the write itself. */ trace?: SafeWriteJsonTrace + + /** + * Restrict the write to a directory. The publish target is resolved through + * symlinks before this check runs, so a caller that picked the path from a + * known scope (a workspace, a project settings directory) can refuse a write + * that a planted symlink would land somewhere else. The check runs before the + * advisory lock is taken and before anything is staged. + */ + confineTo?: string +} + +/** + * Thrown when a write declared with `confineTo` resolves outside that directory. + */ +export class ConfinedPathEscapeError extends Error { + constructor( + readonly requestedPath: string, + readonly resolvedPath: string, + readonly confineTo: string, + ) { + super( + `Refusing to write ${resolvedPath}: it resolves outside the confined directory ${confineTo} (requested ${requestedPath}).`, + ) + this.name = "ConfinedPathEscapeError" + } +} + +/** + * Thrown when the identity this writer took the advisory lock on no longer names the file + * the write is about to edit. The key is the referent the alias named when the key was + * computed; a third party that retargets the alias - or renames another file onto it - + * while this writer holds the lock would otherwise have this writer heal, merge, stage + * and rename onto a file no lock names, and report durable success for it. Refused + * rather than retried: choosing a different key means choosing which writer waits, which + * this writer cannot decide on another writer's behalf. + */ +export class LockIdentityChangedError extends Error { + constructor( + readonly requestedPath: string, + readonly lockedKey: string, + readonly resolvedKey: string, + ) { + super( + `Refusing to write ${resolvedKey}: ${requestedPath} no longer names the file this writer locked (${lockedKey}).`, + ) + this.name = "LockIdentityChangedError" + } +} + +/** + * Canonicalize the directory a write is confined to. The publish target is fully + * resolved through symlinks, so the scope has to be resolved the same way or a + * scope path that itself runs through a symlink (macOS /var -> /private/var is the + * common case) would compare lexically against a resolved target and reject every + * legitimate in-scope write. When the scope does not exist yet, the nearest + * existing ancestor is resolved and the remainder re-appended. + */ +async function _resolveScopeRoot(confineTo: string): Promise { + const lexical = path.resolve(confineTo) + try { + return await fs.realpath(lexical) + } catch (error: unknown) { + // Only a missing path means "walk up and re-join". EACCES or ELOOP means the + // scope cannot be canonicalized at all, and continuing would build a partly + // lexical root that can disagree with the canonical target - the failure has to + // surface rather than decide the scope from a guess. + if (getErrorCode(error) !== "ENOENT") { + throw error + } + const missing: string[] = [] + let ancestor = lexical + while (true) { + const parent = path.dirname(ancestor) + if (parent === ancestor) { + return lexical + } + missing.push(path.basename(ancestor)) + ancestor = parent + try { + const real = await fs.realpath(ancestor) + return path.join(real, ...missing.reverse()) + } catch (innerError: unknown) { + if (getErrorCode(innerError) !== "ENOENT") { + throw innerError + } + } + } + } +} + +/** + * Whether `candidate` sits outside `scopeRoot`. The scope root itself counts as + * outside: a file write cannot land on the directory that declares the scope. + */ +function _escapesScope(scopeRoot: string, candidate: string): boolean { + const relative = path.relative(scopeRoot, candidate) + return relative === "" || relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative) } function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): string { @@ -62,6 +160,9 @@ function createTempFilePath(absoluteFilePath: string, kind: "new" | "bak"): stri * - Recovers an interrupted previous write before touching the file: restores an orphaned backup when the canonical path is absent, and removes leftover temp files. * - Writes to a temporary file first, then commits it with a single atomic rename, so the committed target stays readable through an interrupted write. * - When a single rename over the target fails with EPERM (a reader holds the file open on Windows), falls back to backing the target up before replacing it. + * - Stages the replacement beside the resolved publish target, resolves the advisory + * lock key to the same referent, and refuses a write declared with confineTo when + * either the target or the lock key resolves outside that scope. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. * @@ -78,6 +179,24 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // For directory creation const dirPath = path.dirname(absoluteFilePath) + // Declared outside the protected block so the catch and finally can still name + // the target when the resolution itself rejects. + let resolvedTargetPath: string | undefined + + // Confinement is checked before ANY filesystem side effect. mkdir -p through a + // planted symlinked ancestor (projectDir/.roo -> /elsewhere) would create + // directories outside the declared scope, and an unwritable referent would surface + // the mkdir error instead of the ConfinedPathEscapeError the caller is entitled to. + // _resolveScopeRoot resolves a not-yet-existing path through its nearest existing + // ancestor, so this works for the target's missing parents too. + if (options?.confineTo) { + const scopeRoot = await _resolveScopeRoot(options.confineTo) + const requested = await _resolveScopeRoot(absoluteFilePath) + if (_escapesScope(scopeRoot, requested)) { + throw new ConfinedPathEscapeError(absoluteFilePath, requested, scopeRoot) + } + } + // Ensure directory structure exists with improved reliability try { // Create directory with recursive option @@ -90,14 +209,34 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit throw dirError } - // Acquire the lock before any file operations. `acquireFileLock` owns the - // shared advisory lock protocol, so callers that lock the same path with - // it (for example task-history deletion) serialize with this write. - // If lock acquisition fails, it throws immediately. The releaseLock - // remains a no-op, so the finally block in the main file operations - // try-catch-finally won't try to release an unacquired lock if this - // path is taken. - releaseLock = await acquireFileLock(absoluteFilePath) + // Lock key: the symlink referent when the path is an existing symlink, so a + // symlink alias and its referent share one lock. The key must stay computable while + // a peer writer is mid-commit - the commit rename briefly leaves the path without + // the referent it had when the walk started - so the walk tolerates a dangling link + // instead of rejecting it here. (The backup itself is a copy: the target is never + // renamed away, so a dangling link here means a peer's commit or an external + // unlink, not a backup rename.) + const lockKey = await resolveLockKey(absoluteFilePath) + + // Confinement is checked here as well as under the lock. The lock file lives NEXT + // TO the key, so a planted symlink that points outside the declared scope would + // first make this write create ".lock" outside that scope - a side effect + // the caller declared it would not have - and if that directory is not writable the + // caller would get a lock-acquisition error instead of the ConfinedPathEscapeError + // it is entitled to. The key is the symlink referent, so this check sees the same + // destination the lock would be taken on. The under-lock check stays: it covers the + // target re-resolved after a peer commits. + if (options?.confineTo) { + const scopeRoot = await _resolveScopeRoot(options.confineTo) + if (_escapesScope(scopeRoot, await _resolveScopeRoot(lockKey))) { + throw new ConfinedPathEscapeError(absoluteFilePath, lockKey, scopeRoot) + } + } + + // Acquire the lock before any file operations. If acquisition fails it throws + // immediately, and releaseLock stays a no-op so the finally block does not try + // to release an unacquired lock. + releaseLock = await acquireFileLock(lockKey) // Variables to hold the actual paths of temp files if they are created. let actualTempNewFilePath: string | null = null @@ -106,8 +245,60 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit try { // Heal any state an interrupted previous write left behind: an absent // canonical path with an orphaned backup, or leftover replacement - // files. Runs first so a merge callback reads the restored content. - const recovery = await recoverInterruptedReplacement(absoluteFilePath).catch((error: unknown) => { + // Recovery has to look beside the file an interrupted publish actually renamed onto. The + // leftover names are derived from the publish target, so when the caller named a symlink + // whose referent lives in another directory, the backup and the staged copy sit beside the + // referent while this scan - which used the lexical spelling - searched the link's own + // directory, found nothing, and let the write below publish straight over the orphaned + // newer content. Resolve the publish target first so the scan looks where the bytes are. + // Best-effort on purpose: resolvePublishTarget rejects a link with no referent, and in + // that state the strict resolution below still decides whether this write may proceed at + // all. The fallback is the lock key rather than the spelling the caller named: + // resolveLockKey already followed the links as far as they went and canonicalized what it + // reached, so while the link dangles it still names the file an interrupted publish was + // renaming onto, whereas the named spelling sits in a different directory whenever the + // referent lives elsewhere. Falling back to the named path made this scan look into the + // link's own directory, find nothing, and let the write publish straight over the orphaned + // newer copy this scan exists to heal. + let recoveryTarget = lockKey + try { + recoveryTarget = await resolvePublishTarget(absoluteFilePath) + } catch { + recoveryTarget = lockKey + } + // Confinement has to be decided before recovery touches anything, not afterwards. + // The scan below deletes and renames leftovers beside whatever recoveryTarget names, + // and when the alias moved while this writer waited for the lock that name can sit + // outside the scope the caller declared: the check further down would then refuse the + // write after a foreign directory had already been edited by it. Same predicates as + // the other two confinement checks and it does not replace the one below, which is + // what rejects a target that moves again after recovery and what keeps the strict + // dangling-link rejection inside the protected block. A write whose alias never moved + // reaches the same refusal through an earlier check and with the same error type. + if (options?.confineTo) { + const scopeRoot = await _resolveScopeRoot(options.confineTo) + if (_escapesScope(scopeRoot, await _resolveScopeRoot(recoveryTarget))) { + throw new ConfinedPathEscapeError(absoluteFilePath, recoveryTarget, scopeRoot) + } + } + + // The identity the lock was taken on has to still be the identity this write is about + // to edit. recoveryTarget is the alias as it reads now, so when the alias was + // retargeted while this writer waited for the lock, the file this write would heal, + // merge and publish is a file no lock names. The comparison is skipped in the two + // shapes where the two spellings are known to differ for a reason that is not an + // alias moving: a resolution that handed back the path it was given is the shape of a + // target that does not exist yet, and resolveLockKey canonicalized the parent + // directory of that same path when it computed the key, so a first write into a + // symlinked directory differs in spelling by construction; and the dangling-link + // fallback above chose the lock key itself, which is the same identity. This runs + // after the confinement check above on purpose: a target that escaped the declared + // scope is the caller's own error to receive, and it must not be re-labelled as an + // identity change. + if (recoveryTarget !== absoluteFilePath && recoveryTarget !== lockKey) { + throw new LockIdentityChangedError(absoluteFilePath, lockKey, recoveryTarget) + } + const recovery = await recoverInterruptedReplacement(recoveryTarget).catch((error: unknown) => { const code = getErrorCode(error) ?? "unknown" options?.trace?.({ stage: "recover", detail: `failed:${code}` }) throw error @@ -116,13 +307,43 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit options?.trace?.({ stage: "recover", detail: recovery.restoredFrom ?? "cleaned-leftovers" }) } + // Resolve the publish target under the lock: the peer has committed by now, so + // the strict dangling-link rejection still applies to a real dangling link. It + // must stay inside the protected block, otherwise a rejection here leaves the + // advisory lock held until the stale timeout for every other writer. + resolvedTargetPath = await resolvePublishTarget(absoluteFilePath) + + // Confinement, if the caller declared a scope. Both sides are canonicalized the + // same way: the publish target is resolved through symlinks, and a target that + // does not exist yet still carries the alias components of the path it was + // given. This runs before the merge read and before anything is staged, so a + // rejected write leaves nothing behind. + if (options?.confineTo) { + const scopeRoot = await _resolveScopeRoot(options.confineTo) + if (_escapesScope(scopeRoot, await _resolveScopeRoot(resolvedTargetPath))) { + throw new ConfinedPathEscapeError(absoluteFilePath, resolvedTargetPath, scopeRoot) + } + } + + // The same question again after the recovery pass and before anything is read, + // staged, or renamed: recovery may have just renamed a leftover onto the file this + // write locked, and the alias can be retargeted between that scan and this + // re-resolution, which is the window a rename beside the target - not only a rename + // out of the scope - leaves open. The skipped shapes are the two described above. + // What this comparison does not distinguish is an alias that was replaced by a + // regular file under the same spelling: that resolves back to the path it was given + // and is indistinguishable, from here, from a target that is not there yet. + if (resolvedTargetPath !== absoluteFilePath && resolvedTargetPath !== lockKey) { + throw new LockIdentityChangedError(absoluteFilePath, lockKey, resolvedTargetPath) + } + // If a merge callback was provided, read the current file under the lock // and let the caller merge before we write. Must be inside try/finally // so a throwing merge still releases the lock. if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(resolvedTargetPath, "utf8")) } catch (error: unknown) { const code = getErrorCode(error) if (!(error instanceof SyntaxError) && code !== "ENOENT") { @@ -135,9 +356,19 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // Step 1: Write data to a new temporary file and flush it to storage, // making a best-effort flush before committing the replacement. options?.trace?.({ stage: "write-start" }) - actualTempNewFilePath = createTempFilePath(absoluteFilePath, "new") + // Stage it beside the *resolved* publish target (the symlink referent when the + // path is a symlink): the commit rename below lands on that referent, and a rename + // across filesystems would fail with EXDEV. + actualTempNewFilePath = createTempFilePath(resolvedTargetPath, "new") await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) + + // CWE-732: the commit renames the staged file onto the target, so the file that + // lands carries the STAGED file's mode, not the target's. A 0o600 target would + // otherwise be published as 0o644 with the umask-derived default. Copy the target's + // mode onto the staged file before the rename; a target that is not there yet has no + // mode to copy, and POSIX modes are ignored on Windows. + await _copyTargetMode(resolvedTargetPath, actualTempNewFilePath) await _flushFileToDisk(actualTempNewFilePath) // Step 2: Commit the replacement with a single atomic rename. The @@ -146,7 +377,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // canonical path absent. try { options?.trace?.({ stage: "replacement-rename" }) - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await fs.rename(actualTempNewFilePath, resolvedTargetPath) } catch (renameError: unknown) { if (getErrorCode(renameError) !== "EPERM") { throw renameError @@ -159,15 +390,19 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // second rename fails, and the recovery pass at the start of the // next write repairs a crash between the two renames. options?.trace?.({ stage: "backup-rename", detail: "eperm-fallback" }) - actualTempBackupFilePath = createTempFilePath(absoluteFilePath, "bak") - await fs.rename(absoluteFilePath, actualTempBackupFilePath) + actualTempBackupFilePath = createTempFilePath(resolvedTargetPath, "bak") + await fs.rename(resolvedTargetPath, actualTempBackupFilePath) options?.trace?.({ stage: "replacement-rename", detail: "eperm-fallback" }) - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await fs.rename(actualTempNewFilePath, resolvedTargetPath) } // If we reach here, the new file is successfully in place. // The original actualTempNewFilePath is now the main file, so we shouldn't try to clean it up as "temp". // Mark as "used" or "committed" + + // The rename is durable only once the directory entry that records it reaches disk + // too. Best effort and POSIX-only - see _flushDirectoryToDisk. + _flushDirectoryToDisk(path.dirname(resolvedTargetPath)) actualTempNewFilePath = null options?.trace?.({ stage: "write-complete" }) @@ -181,13 +416,16 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // Log this error, but do not re-throw. The main operation was successful. // actualTempBackupFilePath remains set, indicating an orphaned backup. console.error( - `Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, + `Successfully wrote ${resolvedTargetPath ?? absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, unlinkBackupError, ) } } } catch (originalError) { - console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) + console.error( + `Operation failed for ${resolvedTargetPath ?? absoluteFilePath}: [Original Error Caught]`, + originalError, + ) const newFileToCleanupWithinCatch = actualTempNewFilePath const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath @@ -195,13 +433,15 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // Attempt rollback if a backup was made if (backupFileToRollbackOrCleanupWithinCatch) { try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) + await fs.rename(backupFileToRollbackOrCleanupWithinCatch, resolvedTargetPath ?? absoluteFilePath) // Mark as handled, prevent later unlink of this path actualTempBackupFilePath = null } catch (rollbackError) { // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch console.error( - `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, + `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${ + resolvedTargetPath ?? absoluteFilePath + }:`, rollbackError, ) } @@ -211,11 +451,16 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit if (newFileToCleanupWithinCatch) { try { await fs.unlink(newFileToCleanupWithinCatch) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, - cleanupError, - ) + } catch (cleanupError: unknown) { + // An already-gone staging file is the normal outcome when the failure was the + // commit rename itself, so it is not a second failure and must not be logged as + // one; the caller would otherwise see two errors for one write. + if (getErrorCode(cleanupError) !== "ENOENT") { + console.error( + `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, + cleanupError, + ) + } } } @@ -230,7 +475,7 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit await releaseLock() } catch (unlockError) { // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + console.error(`Failed to release lock for ${lockKey}:`, unlockError) } } } @@ -263,6 +508,76 @@ async function _streamDataToFile(targetPath: string, data: unknown, prettyPrint }) } +/** + * Copies the existing target's permission mode onto the staged file so the publish + * does not silently widen it. A missing target has no mode to copy; a stat or chmod + * that fails for another reason is logged and the write proceeds with the staged + * file's own mode, because the replacement protocol - not the mode - is what makes the + * write atomic. + */ +async function _copyTargetMode(targetPath: string, stagedPath: string): Promise { + let mode: number + try { + const stats = await fs.stat(targetPath) + mode = stats.mode & 0o777 + } catch (statError: unknown) { + const code = getErrorCode(statError) + if (code && code !== "ENOENT" && code !== "ENOTDIR") { + console.error(`Could not read the mode of ${targetPath} before publishing onto it:`, statError) + } + return + } + try { + await fs.chmod(stagedPath, mode) + } catch (chmodError: unknown) { + // A mode this write could not reproduce is not cosmetic. Publishing over a 0o600 + // target with the staged file's own 0o644 widens who can read the file, and the + // rename would then report success. Re-read what the staged file actually ended up + // with: refuse the publish unless the mode matches anyway, because a filesystem + // that ignores POSIX modes, or a umask that already produced the target mode, is + // not a widening. The staged file is cleaned by the caller's failure path, so the + // target is left exactly as it was. + const stagedMode = await fs.stat(stagedPath).catch(() => undefined) + if (stagedMode === undefined || (Number(stagedMode.mode) & 0o777) !== mode) { + console.error(`Failed to copy mode ${mode.toString(8)} from ${targetPath} onto ${stagedPath}:`, chmodError) + throw chmodError + } + console.error( + `Copied no mode onto ${stagedPath}; its own mode ${String((Number(stagedMode.mode) & 0o777).toString(8))} already matches the target ${targetPath}.`, + ) + console.error(`Failed to copy mode ${mode.toString(8)} from ${targetPath} onto ${stagedPath}:`, chmodError) + } +} + +/** + * Syncs the directory that holds a freshly committed file. A rename only survives a + * crash once the directory entry recording it is on disk as well. Best effort by + * design: the write already published, and a filesystem that refuses to open or fsync + * a directory must not turn a completed write into a reported failure - the content is + * at the target either way, so the caller is told through the log, not through a + * rejection. POSIX only: Windows cannot open a directory for this purpose. + */ +function _flushDirectoryToDisk(directoryPath: string): void { + if (process.platform === "win32") { + return + } + let fd: number | undefined + try { + fd = fsSync.openSync(directoryPath, "r") + fsSync.fsyncSync(fd) + } catch (dirSyncError: unknown) { + console.warn(`Could not fsync ${directoryPath} after publishing a file into it:`, dirSyncError) + } finally { + if (fd !== undefined) { + try { + fsSync.closeSync(fd) + } catch (closeError: unknown) { + void closeError + } + } + } +} + /** * Flushes a fully written temp file to storage before it is committed. * Best effort: durability against power loss is a bonus here, process-crash