From c480143f974e13cf6073dfa3bdcdbcdc043e8579 Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sat, 14 Nov 2026 14:44:23 +0800 Subject: [PATCH 1/7] u917 recut rung 1: the safe-write primitives with the tests that describe them and the inherited delete-semantics expectation Runner-only test fix: the delete-lock assertion compared the lock key by text against a path this file canonicalized with fs/promises realpath. delete() hands deleteJsonFile a joined path and deleteJsonFile hands it to withFileLock, which only path.resolve-s it, so the spied key carries the spelling the store was built from, while the expectation carried a realpath spelling. On the Windows runner those are two spellings of one file (C:/Users/runneradmin/... and its NTFS 8.3 alias C:/Users/RUNNER~1/...), so the case passed here and failed there at TaskHistoryStore.deleteSemantics.spec.ts:174. The expectation is now pinned by file identity plus case-folded basename, which is how the landed src/utils/__tests__/safeWriteJson.interruption.test.ts compares one file across both spellings, with the call count, arity and function body pinned alongside it. --- .../TaskHistoryStore.deleteSemantics.spec.ts | 49 +- .../safeWriteText.integration.spec.ts | 51 + .../__tests__/safeWriteText.spec.ts | 1454 +++++++++++++++++ src/services/file-safety/safeWriteText.ts | 641 ++++++++ .../safeWriteJson.interruption.test.ts | 39 +- .../__tests__/safeWriteJson.lockKey.spec.ts | 197 +++ ...safeWriteJson.postCommitDurability.spec.ts | 149 ++ src/utils/__tests__/safeWriteJson.test.ts | 344 ++++ src/utils/safeWriteJson.ts | 296 +++- 9 files changed, 3190 insertions(+), 30 deletions(-) create mode 100644 src/services/file-safety/__tests__/safeWriteText.integration.spec.ts create mode 100644 src/services/file-safety/__tests__/safeWriteText.spec.ts create mode 100644 src/services/file-safety/safeWriteText.ts create mode 100644 src/utils/__tests__/safeWriteJson.lockKey.spec.ts create mode 100644 src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts 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..3035b4552a --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,1454 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import { execFile } from "child_process" +import type { ChildProcess } from "child_process" +import * as path from "path" + +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(), + Stats: class Stats {}, +})) + +// 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)) +} +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) + + // 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 DACL save + restore still ran + // through the default icacls path (options?.execFileRunner must + // not throw when options is undefined) + expect(vi.mocked(execFile)).toHaveBeenCalledTimes(2) + expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) + } + }) + + it("does not remove the staging directory when the caller supplies its own tempPath", 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) + + 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("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; the parent-directory open after the commit + // rename fails, which is the post-commit durability failure. + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) throw new Error("EBADF") + 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. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + 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) + vi.mocked(fsSync.openSync).mockImplementation((target) => { + if (String(target) === dirPath) { + throw new Error("EBADF") + } + 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, so a caller can open it. + const copiedBackups = vi + .mocked(fs.copyFile) + .mock.calls.map(function (call: unknown[]) { + return String(call[1]) + }) + .filter(function (p: string) { + return p.includes("safeWriteText.bak_") + }) + expect(copiedBackups).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() + }) + }) + + // ── 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. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + 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. + expect(fs.rename).toHaveBeenCalledTimes(1) + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + + // 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) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // The destination must exist with a private mode before copyFile writes anything + // into it: copyFile chooses the destination mode itself, so a restrictive target + // could otherwise leave a group/world-readable copy that a later chmod cannot + // undo. "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(fs.copyFile).mock.invocationCallOrder[0]) + + // The chmod keeps a copied read-only attribute (Windows) from breaking the fsync + // open, and keeps a backup of a permissive file private. + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.chmod).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), 0o600) + expect(vi.mocked(fs.chmod).mock.invocationCallOrder[0]).toBeGreaterThan( + vi.mocked(fs.copyFile).mock.invocationCallOrder[0], + ) + + // The copy is then opened for fsync with the writable flag. + const backupOpen = vi.mocked(fsSync.openSync).mock.calls.find(function (call) { + return String(call[0]).includes("safeWriteText.bak_") && call[1] === "r+" + }) + expect(backupOpen).toBeDefined() + }) + + 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(fs.copyFile).mockRejectedValue(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() + }) + }) + + // ── Test 5: win32 DACL path ────────────────────────────────────────────── + + describe("win32 DACL", () => { + 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" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + 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) + // one icacls attempt only: a failed DACL apply must not try to restore + expect(execFile).toHaveBeenCalledTimes(1) + }) + + 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) }) + + expect(execFile).not.toHaveBeenCalled() + 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) + expect(execFile).toHaveBeenCalledTimes(1) + const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1] + expect(saveArgs?.[1]).toBe("/save") + // 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" }) + + // icacls was called twice (save + restore) + expect(execFile).toHaveBeenCalledTimes(2) + + // First call: save DACL from target before backup rename + const firstCall = vi.mocked(execFile).mock.calls[0] + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining("safeWriteText.acl"), "/T"]) + + // Second call: restore DACL onto directory after commit rename + const secondCall = vi.mocked(execFile).mock.calls[1] + 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 callOrder = vi.mocked(execFile).mock.invocationCallOrder + const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder + const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder + const saveCall = callOrder[0] + const restoreCall = callOrder[1] + const backupCopy = copyOrder[0] + const commitRename = renameOrder[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(() => {}) + + // icacls save succeeds, restore fails + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + } + 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" }) + + // icacls was NOT called (target absent → skip DACL entirely) + expect(execFile).not.toHaveBeenCalled() + + // no dump file created or unlinked + expect(fs.unlink).not.toHaveBeenCalled() + }) + }) + + // ── 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("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")), + ) + expect(fs.readlink).toHaveBeenCalledTimes(8) + }) +}) + +describe("caller-supplied staging path", () => { + beforeEach(() => mockDefaults()) + + it("rejects a staging file outside the target's directory before writing anything", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + + // 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("rejects a staging path that is a symlink", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + 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" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // 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" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // 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) + vi.mocked(fs.lstat).mockImplementation(async (p) => { + if (String(p) === targetPath) { + 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..7baf10c919 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,641 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +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 --------------------------------------------------------------- + +/** 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. + 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 + } +} + +// -- 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 two-link cycle terminates, and every key it returns is + * canonicalized through canonicalDirKey. + */ +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 < 8; 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). + 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) + if (path.dirname(supplied) !== path.resolve(dirPath)) { + 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 + 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. + 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 ----------- + 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) + } + } + 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. fs.copyFile picks the destination mode itself (the platform + // creation mask subject to umask on some platforms, the source's mode - or its + // read-only attribute - on others), so letting it create the file would either + // leave a restrictive target's bytes briefly readable to others, or leave the + // copy unwritable so the fsync open below fails with EACCES. open() ignores its + // mode argument for an existing file, so this 0o600 survives the copy on POSIX; + // the chmod afterwards is what clears a copied read-only attribute on Windows + // and keeps a backup of a permissive file private. + const seedFd = fsSync.openSync(backupPath, "wx", 0o600) + fsSync.closeSync(seedFd) + await fs.copyFile(targetPath, backupPath) + await fs.chmod(backupPath, 0o600) + // "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) + } + } 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) + } + } + + // -- 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..0dc50e1609 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -0,0 +1,197 @@ +// npx vitest run utils/__tests__/safeWriteJson.lockKey.spec.ts + +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 { 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("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() +}) diff --git a/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts b/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts new file mode 100644 index 0000000000..c9a8cdab31 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts @@ -0,0 +1,149 @@ +// 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 +type WriteStreamRest = (filePath: string, options: { encoding: string }) => 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 tempWriteAttempted = false + const writeStreamActual = actuals.writeStream + if (writeStreamActual === null) { + throw new Error("fs mock did not capture the real createWriteStream") + } + 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. + tempWriteAttempted = true + const stream = writeStreamActual(stagedPath, { encoding: "utf8" }) + process.nextTick(() => { + stream.emit("error", Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })) + }) + return stream + } + return writeStreamActual(stagedPath, { encoding: "utf8" }) + }, + ) + + await expect(safeWriteJson(targetPath, { version: "new" })).rejects.toThrow("EACCES: permission denied") + + expect(tempWriteAttempted).toBe(true) + // Nothing was published: the target still holds the previous content, and the staged + // file this write created is gone again - the failure handler removed it. + 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..435ca63b6a 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, @@ -818,4 +819,347 @@ 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() + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e54b8c51d3..fbe8b07563 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,81 @@ 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" + } +} + +/** + * 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 +138,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 +157,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 +187,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 +223,23 @@ 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 nothing can be attributed to a referent, so recovery falls back to the named path - + // exactly what it always scanned - and the strict resolution below still decides whether + // this write may proceed at all. + let recoveryTarget = absoluteFilePath + try { + recoveryTarget = await resolvePublishTarget(absoluteFilePath) + } catch { + recoveryTarget = absoluteFilePath + } + const recovery = await recoverInterruptedReplacement(recoveryTarget).catch((error: unknown) => { const code = getErrorCode(error) ?? "unknown" options?.trace?.({ stage: "recover", detail: `failed:${code}` }) throw error @@ -116,13 +248,31 @@ 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) + } + } + // 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 +285,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 +306,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 +319,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 +345,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 +362,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 +380,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 +404,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 +437,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 From 8368f149f6673499ac512fb834a7f5cba33ae412 Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 15 Nov 2026 01:52:25 +0800 Subject: [PATCH 2/7] u917 recut rung 1: answer the seven review threads on safe write Answers the changes-requested verdict coderabbitai[bot] submitted at 16:37:33Z against c480143f974e, cluster by cluster, each with the case that would have failed before it. - safeWriteText.ts:106 (staging privacy on Windows): make the staging directory private before the first staged byte, with icacls /inheritance:r /grant:r ":(OI)(CI)F". Measured on a Windows host the directory went from three inherited ACEs (SYSTEM, Administrators, the user) to that one explicit ACE, and a file written inside inherited only the user. mkdirSync's mode is ignored by Windows directory creation and chmodSync establishes no DACL, so the notices below step 5 could not be enough. A host where icacls fails still saves and reports the leftover exposure. - safeWriteText.ts:257 (link-walk bound): MAX_SYMLINK_HOPS = 40, the longest chain Node resolves at all, so a chain the operating system itself can follow is never cut off short of its referent and two writers cannot take two locks for one file. This changes a value this file declares by design, not ported content, and says so. - safeWriteText.ts:284 (created-directory entries): fs.mkdir reports the shallowest directory it had to create; fsync each containing directory up to the nearest one that already existed, deepest first, because syncing a created directory's inode does not sync the entry in its parent that names it. - safeWriteText.ts:303 (staging location compare): compare canonical directories through canonicalDirKey instead of path spellings, so a caller that staged through an alias of the target's own directory - the same directory, a legal same-directory rename - is no longer failed for how the path was spelled. - safeWriteText.ts:512 (backup copy mode): copy the backup through descriptors this code opened - seed with "wx" at 0o600, read the target, write the seed, including the short write loop - instead of fs.copyFile, which takes its destination mode from the source on POSIX and carries the read-only attribute on Windows. No chmod follows, because the mode this code chose never moved. The exposure window itself is only directly measurable on POSIX; the row proves no copyFile and no chmod on the backup plus content integrity. - postCommitDurability.spec.ts (test-only): fail the staged write at its own open, through the fs option Node's WriteStream calls, so the staged name is never created and the residue assertion is not a timing race. This cluster is a test change, so its control is the assertion inside the row rather than a run against the old source. - safeWriteJson.ts:240 (dangling-alias recovery): recover beside the lock key the walk already resolved instead of the spelling the caller named, so an interrupted publish is healed where its backup actually sits and the write stops rejecting on every retry. Proof, both directions, whole suites of every touched file: - with these changes: 7 files passed, 148 passed | 4 skipped (152). - with the two source files reverted to c480143f974e and the new rows left in: 15 failed | 133 passed | 4 skipped (152). Six of those are rows this commit adds and nine are rows it re-pins to the new contract. At the unit's own parent f98fcf7d8653 the file these threads name does not exist at all, so that base has no run to compare against. - eslint --max-warnings=0 over the nine touched paths: 0 problems, with the shared prune switch left off; src/eslint-suppressions.json is byte identical to its head blob ae3d504003d7e616b129986c252cc97a35b7b568 and no count moved. - prettier 3.9.10 --check over the same nine paths: all matched files use Prettier style. No changeset, no changelog. --- .../__tests__/safeWriteText.spec.ts | 358 ++++++++++++++---- src/services/file-safety/safeWriteText.ts | 221 ++++++++--- .../__tests__/safeWriteJson.lockKey.spec.ts | 84 ++++ ...safeWriteJson.postCommitDurability.spec.ts | 59 ++- src/utils/safeWriteJson.ts | 17 +- 5 files changed, 611 insertions(+), 128 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 3035b4552a..ccfc006ec0 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -31,6 +31,9 @@ vi.mock("fs", () => ({ 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 {}, })) @@ -85,6 +88,11 @@ function mockDefaults(): void { // 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)) } function _stats(mode: number): fsSync.Stats { const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats @@ -172,18 +180,18 @@ describe("safeWriteText", () => { expect(fs.rmdir).toHaveBeenCalledTimes(1) expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) if (process.platform === "win32") { - // default platform is win32: the DACL save + restore still ran - // through the default icacls path (options?.execFileRunner must - // not throw when options is undefined) - expect(vi.mocked(execFile)).toHaveBeenCalledTimes(2) + // default platform is win32: the staging DACL hardening, the DACL save + // and the DACL restore all ran through the default icacls path + // (options?.execFileRunner must not throw when options is undefined) + expect(vi.mocked(execFile)).toHaveBeenCalledTimes(3) expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl")) } }) it("does not remove the staging directory when the caller supplies its own tempPath", async () => { const targetPath = "/tmp/test-dir/target.txt" - const callerTemp = "/tmp/test-dir/caller-staged.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 }) @@ -400,7 +408,20 @@ describe("safeWriteText", () => { // 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. - expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + // 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) @@ -440,16 +461,17 @@ describe("safeWriteText", () => { 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, so a caller can open it. - const copiedBackups = vi - .mocked(fs.copyFile) - .mock.calls.map(function (call: unknown[]) { - return String(call[1]) + // 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" }) - .filter(function (p: string) { - return p.includes("safeWriteText.bak_") + .map(function (call) { + return String(call[0]) }) - expect(copiedBackups).toContain(retained) + expect(seededBackups).toContain(retained) }) it("reports a null backup path when no backup was taken", async () => { @@ -489,7 +511,11 @@ describe("safeWriteText", () => { // 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. - expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + // 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 @@ -510,9 +536,12 @@ describe("safeWriteText", () => { 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. + // 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(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + 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_")) @@ -522,14 +551,20 @@ describe("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) - vi.mocked(fsSync.openSync).mockReturnValue(1) + // 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 a private mode before copyFile writes anything - // into it: copyFile chooses the destination mode itself, so a restrictive target - // could otherwise leave a group/world-readable copy that a later chmod cannot - // undo. "wx" also means a pre-existing path is never silently reused. + // 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" }) @@ -538,21 +573,26 @@ describe("safeWriteText", () => { const seedOrder = vi.mocked(fsSync.openSync).mock.invocationCallOrder[ vi.mocked(fsSync.openSync).mock.calls.indexOf(seedOpen!) ] - expect(seedOrder).toBeLessThan(vi.mocked(fs.copyFile).mock.invocationCallOrder[0]) - - // The chmod keeps a copied read-only attribute (Windows) from breaking the fsync - // open, and keeps a backup of a permissive file private. - expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) - expect(fs.chmod).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_"), 0o600) - expect(vi.mocked(fs.chmod).mock.invocationCallOrder[0]).toBeGreaterThan( - vi.mocked(fs.copyFile).mock.invocationCallOrder[0], - ) + 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 opened for fsync with the writable flag. + // 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 () => { @@ -591,7 +631,9 @@ describe("safeWriteText", () => { // 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(fs.copyFile).mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" })) + 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", @@ -630,6 +672,15 @@ describe("safeWriteText", () => { // ── 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 () => { @@ -637,8 +688,8 @@ describe("safeWriteText", () => { vi.mocked(fs.realpath).mockResolvedValue(targetPath) await safeWriteText(targetPath, "data", { platform: "win32" }) - // icacls dump + restore were called (execFile is callback-based mock) - expect(execFile).toHaveBeenCalledTimes(2) + // staging grant + DACL dump + DACL restore (execFile is the callback mock) + expect(execFile).toHaveBeenCalledTimes(3) }, ) @@ -667,8 +718,18 @@ describe("safeWriteText", () => { // write succeeded despite icacls failure (fallback to plain rename) expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath) - // one icacls attempt only: a failed DACL apply must not try to restore - expect(execFile).toHaveBeenCalledTimes(1) + // 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 () => { @@ -704,7 +765,13 @@ describe("safeWriteText", () => { await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }) - expect(execFile).not.toHaveBeenCalled() + // 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) }) @@ -747,9 +814,15 @@ describe("safeWriteText", () => { // 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) - expect(execFile).toHaveBeenCalledTimes(1) - const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1] - expect(saveArgs?.[1]).toBe("/save") + 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")) }) @@ -761,16 +834,20 @@ describe("safeWriteText", () => { await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) - // icacls was called twice (save + restore) - expect(execFile).toHaveBeenCalledTimes(2) + // three icacls runs: the staging-directory grant, then the save, then the restore + expect(execFile).toHaveBeenCalledTimes(3) - // First call: save DACL from target before backup rename - const firstCall = vi.mocked(execFile).mock.calls[0] + // 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"]) - // Second call: restore DACL onto directory after commit rename - const secondCall = vi.mocked(execFile).mock.calls[1] + // 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"), @@ -792,13 +869,24 @@ describe("safeWriteText", () => { // file that a concurrent publish had already replaced. await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) - const callOrder = vi.mocked(execFile).mock.invocationCallOrder - const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder - const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder - const saveCall = callOrder[0] - const restoreCall = callOrder[1] - const backupCopy = copyOrder[0] - const commitRename = renameOrder[0] + 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) @@ -811,12 +899,11 @@ describe("safeWriteText", () => { vi.mocked(fsSync.openSync).mockReturnValue(1) const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) - // icacls save succeeds, restore fails - let callCount = 0 - vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { - callCount++ + // 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(callCount === 1 ? null : new Error("icacls restore error"), "", "") + cb(restoring ? new Error("icacls restore error") : null, "", "") } return fakeChild }) @@ -849,12 +936,69 @@ describe("safeWriteText", () => { await safeWriteText(targetPath, "data", { platform: "win32" }) - // icacls was NOT called (target absent → skip DACL entirely) - expect(execFile).not.toHaveBeenCalled() + // 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: 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) ────────────────────── @@ -1131,6 +1275,54 @@ describe("safeWriteText", () => { 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) @@ -1258,16 +1450,25 @@ describe("resolveLockKey", () => { await expect(resolveLockKey("/tmp/linkdir/a.json")).resolves.toBe( path.resolve(path.join("/tmp/linkdir", "a.json")), ) - expect(fs.readlink).toHaveBeenCalledTimes(8) + // 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()) + 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" - vi.mocked(fs.realpath).mockResolvedValue(targetPath) // A rename across filesystems fails with EXDEV, and a path elsewhere lets // a caller publish an unrelated file onto the target. @@ -1278,9 +1479,29 @@ describe("caller-supplied staging path", () => { 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.realpath).mockResolvedValue(targetPath) vi.mocked(fs.lstat).mockResolvedValue(_fileStats(true)) // Renaming a link over the target publishes whatever the link points at. @@ -1293,7 +1514,6 @@ describe("caller-supplied staging path", () => { it("rejects a staging path that is the target itself", async () => { const targetPath = "/tmp/test-dir/target.txt" - vi.mocked(fs.realpath).mockResolvedValue(targetPath) // 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. @@ -1319,13 +1539,15 @@ describe("caller-supplied staging path", () => { 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" - vi.mocked(fs.realpath).mockResolvedValue(targetPath) // 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) { + if (String(p) === targetPath || String(p) === resolvedTarget) { throw Object.assign(new Error("EACCES"), { code: "EACCES" }) } return stagingStats diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 7baf10c919..9a9d6ec884 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -88,6 +88,16 @@ export class PostCommitDurabilityError extends Error { } // -- 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") @@ -103,6 +113,10 @@ function _stagingDir(dir: string): string { // 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) @@ -154,6 +168,36 @@ async function _restoreDaclWindows( } } +/** 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 without a resolvable + * account name in the environment, still saves the file, and the caller reports the + * leftover exposure through onWarning. */ +async function _grantPrivateDaclWindows(stagingDirPath: string, execFileRunner?: typeof execFile): Promise { + const account = process.env.USERNAME ?? process.env.USER + 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 ------------------------------------------------------------ /** @@ -243,8 +287,12 @@ async function canonicalDirKey(absoluteFilePath: string): Promise { * 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 two-link cycle terminates, and every key it returns is - * canonicalized through canonicalDirKey. + * 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 { @@ -254,7 +302,7 @@ export async function resolveLockKey(absoluteFilePath: string): Promise // 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 < 8; depth++) { + 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)) @@ -281,7 +329,13 @@ export async function safeWriteText( const dirPath = path.dirname(targetPath) // Ensure parent directory exists (mirrors safeWriteJson behaviour). - await fs.mkdir(dirPath, { recursive: true }) + // 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; @@ -300,7 +354,15 @@ export async function safeWriteText( // link points at, which is the same trust problem as writing through a // dangling symlink in resolvePublishTarget. const supplied = path.resolve(options.tempPath) - if (path.dirname(supplied) !== path.resolve(dirPath)) { + // 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, @@ -355,6 +417,36 @@ export async function safeWriteText( // 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) { @@ -367,6 +459,18 @@ export async function safeWriteText( // 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 @@ -423,32 +527,6 @@ export async function safeWriteText( } // -- Step 2 (win32): save DACL BEFORE the backup copy ----------- - 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) - } - } if (platform === "win32") { let accessError: unknown = null try { @@ -498,19 +576,41 @@ export async function safeWriteText( // 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. fs.copyFile picks the destination mode itself (the platform - // creation mask subject to umask on some platforms, the source's mode - or its - // read-only attribute - on others), so letting it create the file would either - // leave a restrictive target's bytes briefly readable to others, or leave the - // copy unwritable so the fsync open below fails with EACCES. open() ignores its - // mode argument for an existing file, so this 0o600 survives the copy on POSIX; - // the chmod afterwards is what clears a copied read-only attribute on Windows - // and keeps a backup of a permissive file private. + // 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) - fsSync.closeSync(seedFd) - await fs.copyFile(targetPath, backupPath) - await fs.chmod(backupPath, 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+") @@ -558,6 +658,41 @@ export async function safeWriteText( // 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 --------- diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 0dc50e1609..951f46588c 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -1,5 +1,6 @@ // 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" @@ -170,6 +171,89 @@ describe("safeWriteJson lock key under a peer commit", () => { 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 () => { diff --git a/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts b/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts index c9a8cdab31..32cfb31eb2 100644 --- a/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts +++ b/src/utils/__tests__/safeWriteJson.postCommitDurability.spec.ts @@ -18,7 +18,34 @@ import { safeWriteJson } from "../safeWriteJson" // before any vi.fn() wrapper exists, so a test's implementation callback can delegate // without recursing into itself. type OpenSyncRest = (...args: unknown[]) => number -type WriteStreamRest = (filePath: string, options: { encoding: string }) => FsWriteStream +// 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 @@ -116,23 +143,32 @@ describe("safeWriteJson post-commit durability", () => { // 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 tempWriteAttempted = false + 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. - tempWriteAttempted = true - const stream = writeStreamActual(stagedPath, { encoding: "utf8" }) - process.nextTick(() => { - stream.emit("error", Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" })) - }) - return stream + 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" }) }, @@ -140,9 +176,10 @@ describe("safeWriteJson post-commit durability", () => { await expect(safeWriteJson(targetPath, { version: "new" })).rejects.toThrow("EACCES: permission denied") - expect(tempWriteAttempted).toBe(true) - // Nothing was published: the target still holds the previous content, and the staged - // file this write created is gone again - the failure handler removed it. + 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/safeWriteJson.ts b/src/utils/safeWriteJson.ts index fbe8b07563..6c5623e04e 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -229,15 +229,20 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit // 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 nothing can be attributed to a referent, so recovery falls back to the named path - - // exactly what it always scanned - and the strict resolution below still decides whether - // this write may proceed at all. - let recoveryTarget = absoluteFilePath + // 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 = absoluteFilePath + recoveryTarget = lockKey } const recovery = await recoverInterruptedReplacement(recoveryTarget).catch((error: unknown) => { const code = getErrorCode(error) ?? "unknown" From 1f56e8089f0f178ec0718ec098171b3a362033f8 Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 15 Nov 2026 07:17:49 +0800 Subject: [PATCH 3/7] u917 leg 186 port: name each icacls step the staging-cleanup row counts, pin the account it grants from, and add the no-account skip, the multi-chunk short-write backup and the held-grant barrier --- .../__tests__/safeWriteText.spec.ts | 219 +++++++++++++++++- 1 file changed, 214 insertions(+), 5 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index ccfc006ec0..e2465d6e80 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -1,7 +1,7 @@ import * as fs from "fs/promises" import * as fsSync from "fs" import { execFile } from "child_process" -import type { ChildProcess } from "child_process" +import type { ChildProcess, ExecFileException } from "child_process" import * as path from "path" import { PostCommitDurabilityError, resolveLockKey, safeWriteText, StagingPathError } from "../safeWriteText" @@ -172,6 +172,13 @@ describe("safeWriteText", () => { 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 @@ -180,13 +187,78 @@ describe("safeWriteText", () => { expect(fs.rmdir).toHaveBeenCalledTimes(1) expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) if (process.platform === "win32") { - // default platform is win32: the staging DACL hardening, the DACL save - // and the DACL restore all ran through the default icacls path - // (options?.execFileRunner must not throw when options is undefined) - expect(vi.mocked(execFile)).toHaveBeenCalledTimes(3) + // 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 no account name resolves", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + const warnings: string[] = [] + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Both account sources cleared: the state a host reaches when its environment + // carries neither USERNAME nor USER. The grant never runs, so the two calls left + // are the target DACL save and the post-commit restore, named by argument. + vi.stubEnv("USERNAME", "") + vi.stubEnv("USER", "") + + 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("does not remove the staging directory when the caller supplies its own tempPath", async () => { const targetPath = "/tmp/test-dir/target.txt" @@ -667,6 +739,87 @@ describe("safeWriteText", () => { // 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 ────────────────────────────────────────────── @@ -948,6 +1101,62 @@ describe("safeWriteText", () => { 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) From fff1cc3b31b82ace183e158232b52d08baf7839d Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 15 Nov 2026 07:36:08 +0800 Subject: [PATCH 4/7] u917 leg 187: resolve the staging account from the host when USERNAME and USER are both unset, so the privacy grant runs on a process that inherits neither --- .../__tests__/safeWriteText.spec.ts | 55 +++++++++++++++++-- src/services/file-safety/safeWriteText.ts | 30 ++++++++-- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index e2465d6e80..c540d93057 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -3,6 +3,7 @@ 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" @@ -37,6 +38,13 @@ vi.mock("fs", () => ({ 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) => { @@ -93,6 +101,10 @@ function mockDefaults(): void { // 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 @@ -213,17 +225,20 @@ describe("safeWriteText", () => { vi.unstubAllEnvs() }) it.skipIf(process.platform !== "win32")( - "skips the staging-directory grant and reports the leftover exposure when no account name resolves", + "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 account sources cleared: the state a host reaches when its environment - // carries neither USERNAME nor USER. The grant never runs, so the two calls left - // are the target DACL save and the post-commit restore, named by argument. + // 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", { @@ -260,6 +275,38 @@ describe("safeWriteText", () => { }, ) + 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) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index 9a9d6ec884..ec89854921 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -2,6 +2,7 @@ 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 { /** @@ -174,11 +175,32 @@ async function _restoreDaclWindows( * 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 without a resolvable - * account name in the environment, still saves the file, and the caller reports the - * leftover exposure through onWarning. */ + * 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 = process.env.USERNAME ?? process.env.USER + const account = _resolveStagingAccountName() if (!account) { return false } From a9342567043e443f31203fe5940b7f6d2baf46f4 Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 15 Nov 2026 08:15:13 +0800 Subject: [PATCH 5/7] file-safety: flush the backup directory entry before the commit rename, decide confinement before recovery mutates Two of the asks on the current head were still live against this code rather than already answered by it. The recovery copy on a backup:true write is an entry inside the target directory, and the only directory flush in safeWriteText ran after the commit rename: a crash in between could leave the rename durable while the name of the copy was not, which is 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 that removes the copy and leaves the target alone, and the flush after the rename stays because that rename changed a different entry. In safeWriteJson the confinement decision was taken after recoverInterruptedReplacement had already deleted and renamed leftovers beside the re-resolved target, so when the alias moved while the writer waited for the lock a refusal arrived after a foreign directory had been edited. The same predicates now also run against that recovery target before the scan, so the refusal lands before the mutation. The under-lock check stays where the design wants it. Both directions are covered by rows in the two specs named in the review; no suppression changed and no changeset was added. --- .../__tests__/safeWriteText.spec.ts | 101 +++++++++++++++++- src/services/file-safety/safeWriteText.ts | 19 ++++ .../__tests__/safeWriteJson.lockKey.spec.ts | 74 ++++++++++++- src/utils/safeWriteJson.ts | 15 +++ 4 files changed, 204 insertions(+), 5 deletions(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index c540d93057..89bbfb14fc 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -510,14 +510,99 @@ describe("safeWriteText", () => { 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; the parent-directory open after the commit - // rename fails, which is the post-commit durability failure. + // 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) throw new Error("EBADF") + if (String(target) === dirPath) { + dirOpens += 1 + if (dirOpens > 1) { + throw new Error("EBADF") + } + return 1 + } return 1 }) @@ -560,9 +645,17 @@ describe("safeWriteText", () => { 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) { - throw new Error("EBADF") + dirOpens += 1 + if (dirOpens > 1) { + throw new Error("EBADF") + } + return 1 } return 1 }) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts index ec89854921..a908459d19 100644 --- a/src/services/file-safety/safeWriteText.ts +++ b/src/services/file-safety/safeWriteText.ts @@ -641,6 +641,25 @@ export async function safeWriteText( } 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. diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 951f46588c..73718334a2 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -6,7 +6,7 @@ import path from "path" import type { BigIntStats } from "fs" import * as fs from "fs/promises" import { acquireFileLock } from "../fileLock" -import { safeWriteJson } from "../safeWriteJson" +import { ConfinedPathEscapeError, safeWriteJson } from "../safeWriteJson" import { resolveLockKey } from "../../services/file-safety/safeWriteText" vi.mock("../fileLock", () => ({ @@ -279,3 +279,75 @@ it("does not log a cleanup error when the safety net finds the temp file already 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) + }) +}) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 6c5623e04e..e564d1cee1 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -244,6 +244,21 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit } 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) + } + } const recovery = await recoverInterruptedReplacement(recoveryTarget).catch((error: unknown) => { const code = getErrorCode(error) ?? "unknown" options?.trace?.({ stage: "recover", detail: `failed:${code}` }) From 9a6ebef7bd97c1f32ab44acb85e7d5eac4a6ec43 Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 15 Nov 2026 09:13:48 +0800 Subject: [PATCH 6/7] test(safeWriteJson): cover the publish-time mode copy when chmod is refused or the target mode is unreadable The mutation gate's regression-evidence check named three gaps in the coverage of _copyTargetMode. This adds chmod to the fs/promises mock as a passthrough double and three rows at the layer the failure lives in, with no production change: - a refused copy onto a staged file that reports a different mode refuses the publish, logs the failed-copy line, keeps the previous content at the target and leaves no staged file behind; - a refused copy where the staged file already carries the target's mode logs the already-matches line and then the failed-copy line and still publishes; - a target mode that cannot be read logs and publishes with the staged file's own mode, reaching no chmod at all. The mode numbers the first two rows compare are reported through the double rather than measured from the host, so the rows run on every platform, unlike the existing 0o600-preservation row which is skipped on win32. No changeset, no changelog row, no suppression and no Stryker directive. --- src/utils/__tests__/safeWriteJson.test.ts | 166 ++++++++++++++++++++++ 1 file changed, 166 insertions(+) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 435ca63b6a..a2eea61dd4 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -13,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 () => { @@ -20,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, @@ -29,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), @@ -1162,4 +1171,161 @@ describe("safeWriteJson", () => { 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() + }) }) From f84f078f65224afc4220f21923d54acfe00c6ced Mon Sep 17 00:00:00 2001 From: "user.email" <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Sun, 15 Nov 2026 10:40:00 +0800 Subject: [PATCH 7/7] safeWriteJson: refuse a publish whose lock identity moved under it The follow-up on 4576377536 recommends (b) - reject a changed lock identity without an internal retry - and asks for the identity to be checked before recovery and again after recovery, before the merge and the publication. The lock is taken on the referent resolveLockKey reached, and nothing compared the later resolutions with that key, so a third party that retargets the alias while this writer holds the lock had this writer heal, merge, stage and rename onto a file no lock names, and report durable success for it. Adds LockIdentityChangedError and two comparisons: one after recoveryTarget is settled and before the recovery call, one after the publish re-resolution and before the merge read, the staging and the rename. Both sit after the confinement checks on purpose - a target that left the declared scope is the caller's own ConfinedPathEscapeError and must not be re-labelled as an identity change. Each comparison skips the two shapes where the two spellings are known to differ for some other reason: a resolution that handed back the path it was given, which is the shape of a target that does not exist yet, and the dangling-link fallback, which is the lock key itself. What it does not distinguish is an alias replaced by a regular file under the same spelling: that resolves back to the path it was given. The finally release is untouched. Two rows in safeWriteJson.lockKey.spec.ts put the second target inside the declared scope and plant an active staged copy beside it. One moves the alias while lock acquisition waits, the other moves it after the recovery pass and before the publication. Both assert the refusal, that the planted copy and both targets are untouched, and one release of one lock. Both are red with the two comparisons taken out. The row 4576378276 asks for is added to safeWriteText.spec.ts: the backup directory open fails carrying code ENOENT, and the row asserts what the shipped code does - the copy is removed, the failure is swallowed by the errno-only catch around the backup block, and the commit rename runs. The row and this message both say that a green row there pins the shipped behaviour rather than the narrowed exception the follow-up asks for, which is a ruling that is not the row's to make. The interleaving ask on 4576392741 got no row and this says why: from the commit rename to the last ancestor fsync the path holds no await point, so no second in-process writer can land inside that window with the doubles the neighbouring rows already use, and per-process doubles cannot put a required ancestor entry into the state of being unsynced at all. The asks on 4576393091 and 4576395175 are Windows asks and are left, with the human and platform rulings they carry. No changeset, no changelog row, no suppression and no Stryker directive. --- .../__tests__/safeWriteText.spec.ts | 57 +++++++ .../__tests__/safeWriteJson.lockKey.spec.ts | 149 +++++++++++++++++- src/utils/safeWriteJson.ts | 51 ++++++ 3 files changed, 256 insertions(+), 1 deletion(-) diff --git a/src/services/file-safety/__tests__/safeWriteText.spec.ts b/src/services/file-safety/__tests__/safeWriteText.spec.ts index 89bbfb14fc..5e1af96cd7 100644 --- a/src/services/file-safety/__tests__/safeWriteText.spec.ts +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -706,6 +706,63 @@ describe("safeWriteText", () => { 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 ── diff --git a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts index 73718334a2..e0ffcf8a35 100644 --- a/src/utils/__tests__/safeWriteJson.lockKey.spec.ts +++ b/src/utils/__tests__/safeWriteJson.lockKey.spec.ts @@ -6,7 +6,7 @@ import path from "path" import type { BigIntStats } from "fs" import * as fs from "fs/promises" import { acquireFileLock } from "../fileLock" -import { ConfinedPathEscapeError, safeWriteJson } from "../safeWriteJson" +import { ConfinedPathEscapeError, LockIdentityChangedError, safeWriteJson } from "../safeWriteJson" import { resolveLockKey } from "../../services/file-safety/safeWriteText" vi.mock("../fileLock", () => ({ @@ -351,3 +351,150 @@ describe("safeWriteJson confinement when the alias moves under the lock", () => 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/safeWriteJson.ts b/src/utils/safeWriteJson.ts index e564d1cee1..43d6df789d 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -74,6 +74,28 @@ export class ConfinedPathEscapeError extends Error { } } +/** + * 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 @@ -259,6 +281,23 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit 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}` }) @@ -286,6 +325,18 @@ async function safeWriteJson(filePath: string, data: unknown, options?: SafeWrit } } + // 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.