diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index e4b15aa27e..ad2a2cddb9 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1686,7 +1686,7 @@ }, "utils/__tests__/safeWriteJson.test.ts": { "@typescript-eslint/no-explicit-any": { - "count": 27 + "count": 26 } }, "utils/__tests__/shell.spec.ts": { @@ -1716,7 +1716,7 @@ }, "utils/safeWriteJson.ts": { "@typescript-eslint/no-explicit-any": { - "count": 4 + "count": 3 } }, "utils/tts.ts": { diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bb3368f063..b43ce12cf0 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -1,6 +1,7 @@ import * as vscode from "vscode" import * as path from "path" import * as fs from "fs/promises" +import { constants as fsConstants } from "fs" import * as diff from "diff" import stripBom from "strip-bom" import delay from "delay" @@ -18,6 +19,7 @@ import { arePathsEqual, getReadablePath } from "../../utils/path" import { formatResponse } from "../../core/prompts/responses" import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics" import { Task } from "../../core/task/Task" +import { safeWriteText } from "../../services/file-safety/safeWriteText" import { DecorationController } from "./DecorationController" @@ -1156,7 +1158,20 @@ export class DiffViewProvider { // Write the content directly to the file await createDirectoriesForFile(absolutePath) - await fs.writeFile(absolutePath, content, "utf-8") + // safeWriteText publishes with rename, which only needs write permission on the + // DIRECTORY. The previous fs.writeFile required it on the file itself, so a target + // the user marked read-only used to fail with EACCES; keep that contract instead of + // silently renaming over it. A not-yet-existing target stays creatable. + try { + await fs.access(absolutePath, fsConstants.W_OK) + } catch (error: unknown) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT") throw error + } + await safeWriteText(absolutePath, content) // Open the document to ensure diagnostics are loaded // When openFile is false (PREVENT_FOCUS_DISRUPTION enabled), we only open in memory diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..756511f3af 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -16,8 +16,22 @@ vi.mock("fs/promises", () => ({ readFile: vi.fn().mockResolvedValue("file content"), writeFile: vi.fn().mockResolvedValue(undefined), access: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + rename: vi.fn().mockResolvedValue(undefined), + unlink: vi.fn().mockResolvedValue(undefined), })) +// Mock safeWriteText (used by saveDirectly). The real module's error classes stay +// available through importOriginal, so a test can hand the caller an actual +// DaclRestoreError instead of a look-alike. +vi.mock("../../../services/file-safety/safeWriteText", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + safeWriteText: vi.fn().mockResolvedValue(undefined), + } +}) + // Mock utils vi.mock("../../../utils/fs", () => ({ createDirectoriesForFile: vi.fn().mockResolvedValue([]), @@ -27,6 +41,8 @@ vi.mock("../../../utils/fs", () => ({ vi.mock("path", () => ({ resolve: vi.fn((cwd, relPath) => `${cwd}/${relPath}`), basename: vi.fn((path) => path.split("/").pop()), + dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"), + join: (...args: string[]) => args.join("/"), })) // Mock vscode @@ -780,6 +796,86 @@ describe("DiffViewProvider", () => { }) describe("saveDirectly method", () => { + it("rejects a save over a read-only target instead of renaming onto it", async () => { + const fs = await import("fs/promises") + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + vi.mocked(safeWriteText).mockClear() + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.access).mockRejectedValueOnce(eacces) + + await expect(diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200)).rejects.toThrow( + "EACCES", + ) + + // The old fs.writeFile path failed the same way; safeWriteText must not run. + expect(safeWriteText).not.toHaveBeenCalled() + vi.mocked(fs.access).mockResolvedValue(undefined) + }) + + it("still writes when the target does not exist yet", async () => { + const fs = await import("fs/promises") + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + vi.mocked(safeWriteText).mockClear() + vi.mocked(fs.access).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200) + + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content") + // Call counts alone would still pass if the publish ran but the caller reported + // nothing about the content it wrote. + expect(result.finalContent).toBe("new content") + expect(result.userEdits).toBeUndefined() + vi.mocked(fs.access).mockResolvedValue(undefined) + }) + + it("checks write permission on the absolute target before publishing", async () => { + const fs = await import("fs/promises") + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + vi.mocked(safeWriteText).mockClear() + vi.mocked(fs.access).mockClear() + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200) + + // safeWriteText publishes with rename, which only needs write permission on the + // DIRECTORY. The contract this unit preserves requires it on the FILE, so the guard + // must be handed the absolute target - not the relative path the caller passed. + const calls = vi.mocked(fs.access).mock.calls + const accessIndex = calls.findIndex((call) => String(call[0]).endsWith("test.ts")) + expect(accessIndex).toBeGreaterThanOrEqual(0) + expect(calls[accessIndex][0]).toBe(`${mockCwd}/test.ts`) + + // An existence-only check would not catch a read-only target: the guard has to be handed + // the write bit, otherwise the read-only contract this unit preserves is silently gone. + const { constants } = await import("fs") + expect(calls[accessIndex][1]).toBe(constants.W_OK) + + // The check is only meaningful before the publish; after it, an EACCES target would + // already have been renamed over. + expect(vi.mocked(fs.access).mock.invocationCallOrder[accessIndex]).toBeLessThan( + vi.mocked(safeWriteText).mock.invocationCallOrder[0], + ) + }) + + it("surfaces a failed DACL restore as a rejection instead of a completed save", async () => { + const { DaclRestoreError, safeWriteText } = await import("../../../services/file-safety/safeWriteText") + vi.mocked(safeWriteText).mockClear() + // The primitive reports a lost DACL after the commit rename as DaclRestoreError. + // The direct-save path must not swallow it: a caller that sees saveDirectly + // resolve would report a save whose access rights silently changed. + vi.mocked(safeWriteText).mockRejectedValueOnce(new DaclRestoreError(`${mockCwd}/test.ts`, null)) + + await expect( + diffViewProvider.saveDirectly("test.ts", "new content", true, true, 200), + ).rejects.toBeInstanceOf(DaclRestoreError) + + // The rejection is the primitive's own - the publish was attempted exactly once, + // not retried into a success the caller could observe. + const publishCalls = vi.mocked(safeWriteText).mock.calls.filter(function (call) { + return call[0] === `${mockCwd}/test.ts` + }) + expect(publishCalls).toHaveLength(1) + }) + beforeEach(() => { // Mock vscode functions vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any) @@ -792,9 +888,9 @@ describe("DiffViewProvider", () => { const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 2000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content") // Verify file was opened without focus expect(vscode.window.showTextDocument).toHaveBeenCalledWith( @@ -815,9 +911,9 @@ describe("DiffViewProvider", () => { it("should not open file when openWithoutFocus is false", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", false, true, 1000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content") // Verify file was NOT opened expect(vscode.window.showTextDocument).not.toHaveBeenCalled() @@ -830,9 +926,9 @@ describe("DiffViewProvider", () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, false, 1000) - // Verify file was written - const fs = await import("fs/promises") - expect(fs.writeFile).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content", "utf-8") + // Verify file was written via safeWriteText + const { safeWriteText } = await import("../../../services/file-safety/safeWriteText") + expect(safeWriteText).toHaveBeenCalledWith(`${mockCwd}/test.ts`, "new content") // Verify delay was NOT called expect(mockDelay).not.toHaveBeenCalled() 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..77bdd9e633 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.integration.spec.ts @@ -0,0 +1,60 @@ +import fsSync from "node:fs" +import os from "node:os" +import path from "node:path" + +import { safeWriteText } from "../safeWriteText" + +/** + * Every other suite in this directory mocks fs/promises and fs, so nothing there proves the real + * open/write/fsync/rename sequence produces a file on disk. This suite uses a real temporary + * directory and no mocks at all: it asserts the outcome (bytes on disk) and the absence of residue + * (staging directory, temp file, backup). + */ +describe("safeWriteText against a real filesystem", () => { + let dir: string + + beforeEach(() => { + dir = fsSync.mkdtempSync(path.join(os.tmpdir(), "safe-write-it-")) + }) + + afterEach(() => { + fsSync.rmSync(dir, { recursive: true, force: true }) + }) + + function _leftovers(): string[] { + return ( + fsSync + .readdirSync(dir) + // The Windows DACL dump is written next to the target and only ever shows up on the + // windows-latest run, so a filter that misses it lets a leaked dump pass a suite + // whose claim is the absence of residue. + .filter(function (name) { + return ( + name.includes(".file-safety-staging") || + name.includes(".new_") || + name.includes(".bak") || + name.includes(".acl.tmp") + ) + }) + ) + } + + it("creates a new file with the requested bytes and leaves no residue", async () => { + const target = path.join(dir, "brand-new.txt") + + await safeWriteText(target, "first content") + + expect(fsSync.readFileSync(target, "utf8")).toBe("first content") + expect(_leftovers()).toEqual([]) + }) + + it("replaces an existing file and leaves no residue", async () => { + const target = path.join(dir, "existing.txt") + fsSync.writeFileSync(target, "old") + + await safeWriteText(target, "replacement") + + expect(fsSync.readFileSync(target, "utf8")).toBe("replacement") + expect(_leftovers()).toEqual([]) + }) +}) 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..6fe7cf3491 --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,2099 @@ +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 { DaclRestoreError, resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" + +// The two failure classes are module-private (knip ignores __tests__, so a +// test-only export would be reported as unused), so tests match them by name. +async function _rejectionName(promise: Promise): Promise { + try { + await promise + return "resolved" + } catch (error) { + return error instanceof Error ? error.name : String(error) + } +} + +/** + * Runs a publish exactly once and reports both halves of what a caller observes: the + * rejection's class name and its wording. Needed where the report is the fact under test - + * which condition a post-commit error names is what separates "this host cannot restore" + * from "the restore failed this time" - and a second run would not be the same publish. + */ +async function _rejectionReport(promise: Promise): Promise<{ name: string; message: string }> { + try { + await promise + return { name: "resolved", message: "" } + } catch (error: unknown) { + return { + name: error instanceof Error ? error.name : String(error), + message: error instanceof Error ? error.message : String(error), + } + } +} + +// Full mock for fs/promises — all methods are vi.fn() stubs +vi.mock("fs/promises", () => ({ + mkdir: vi.fn(), + access: vi.fn(), + rename: vi.fn(), + unlink: vi.fn(), + realpath: vi.fn(), + lstat: vi.fn(), + readlink: vi.fn(), + copyFile: vi.fn(), + rmdir: 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(), + fchownSync: vi.fn(), + statSync: vi.fn(), + lstatSync: vi.fn(), + rmdirSync: 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)) +} +/** + * The one shared fixed name this layer used to stage under. A workspace can still ship an entry + * with that name, and the per-write name this layer generates now STARTS with it, so a substring + * probe cannot tell the two apart - every comparison below is against the full name. + */ +function _sharedStagingDir(dir: string): string { + return path.join(dir, ".file-safety-staging") +} + +/** + * Exact shape of a per-write staging name, anchored at both ends: the separator, the timestamp and + * the random suffix all have to be there. Matching the prefix alone would also accept the shared + * name a workspace shipped. + */ +function _isPerWriteStagingName(name: string): boolean { + return /^\.file-safety-staging_\d+_[a-z0-9]+$/.test(name) +} + +/** ENOENT stand-in, as the fs mocks report it for a path that does not exist yet. */ +function _enoent(): Error { + return Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) +} + +/** + * lstat stand-in that answers per path: the shared name reports whatever the workspace shipped at + * it, and every other path behaves like a fresh entry - absent until this call creates it, then + * a real directory owned by this process. + */ +function _lstatPerPath(shared: string, sharedStats: fsSync.Stats): (p: unknown) => fsSync.Stats { + const seen = new Set() + return (p: unknown) => { + const name = String(p) + if (name === shared) { + return sharedStats + } + if (!seen.has(name)) { + seen.add(name) + throw _enoent() + } + return _dirStats() + } +} + +// Stats stand-in for the staging directory: a real directory owned by this +// process, which is what _stagingDir requires before it will stage there. +function _dirStats(): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats & { + uid?: number + isDirectory?: () => boolean + } + s.uid = typeof process.getuid === "function" ? process.getuid() : 0 + s.isDirectory = () => true + return s +} + +// Stats stand-in for a symlink planted at the staging path. +function _linkStats(): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats & { + isDirectory?: () => boolean + } + s.isDirectory = () => false + return s +} + +// Minimal Stats stand-in: the SUT reads `.mode` and, on POSIX, `.gid` from it. +function _stats(mode: number, gid?: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, gid === undefined ? { mode } : { mode, gid }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + 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) + // Link lookups default to 'not a link' so an ENOENT realpath falls back to the path. + vi.mocked(fs.lstat).mockResolvedValue(null as unknown as fsSync.Stats) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // Staging path default: a real directory owned by this process. + vi.mocked(fsSync.lstatSync).mockReturnValue(_dirStats()) + // 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() + }) + + // ── the staging directory's NAME ────────────────────────────────────────── + // One shared fixed name made the staging directory a path the workspace controls: + // mkdirSync with recursive:true accepts an existing entry of the requested name + // without creating anything, and the chmod below follows a link. The chain's shape + // is a name generated per write, so the entry a write creates, chmods and writes + // into is one it made. Every assertion here compares FULL names: the generated name + // starts with the shared name, so a substring probe cannot tell them apart. + + it("gives each self-staged write its own staging directory, of the generated shape", async () => { + const targetA = "/tmp/test-dir/target-a.txt" + const targetB = "/tmp/test-dir/target-b.txt" + vi.mocked(fs.realpath).mockImplementation(async (p) => String(p)) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetA, "a", { platform: "linux" }) + await safeWriteText(targetB, "b", { platform: "linux" }) + + const created = vi.mocked(fsSync.mkdirSync).mock.calls.map((c) => String(c[0])) + expect(created).toHaveLength(2) + expect(created[0]).not.toBe(created[1]) + // Uniqueness has to come from the name itself, so the shape is pinned end to end. + for (const staging of created) { + expect(_isPerWriteStagingName(path.basename(staging))).toBe(true) + } + // Each write's permission repair names the directory it created, ... + expect(vi.mocked(fsSync.chmodSync).mock.calls.map((c) => String(c[0]))).toEqual(created) + // ...each staged file is opened inside its own directory, ... + const staged = vi + .mocked(fsSync.openSync) + .mock.calls.filter((c) => c[1] === "w") + .map((c) => String(c[0])) + expect(staged).toHaveLength(2) + expect(staged.map((p) => path.dirname(p))).toEqual(created) + // ...and each write releases its own directory, never the other one's. + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => String(c[0]))).toEqual(created) + }) + + it("a symlink at the shared staging name cannot redirect the permission repair or the staged file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const shared = _sharedStagingDir(path.dirname(targetPath)) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The workspace ships .file-safety-staging as a symlink to a directory the user owns + // outside the target directory. Only that exact name answers as a link. + vi.mocked(fsSync.lstatSync).mockImplementation(_lstatPerPath(shared, _linkStats())) + + // The link is not this write's directory, so the write does not stop at it. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).resolves.toBeUndefined() + + // The permission repair never reaches the shared name: every chmod names a directory + // of the generated shape, which is one this call created. + const repaired = vi.mocked(fsSync.chmodSync).mock.calls.map((c) => String(c[0])) + expect(repaired.length).toBeGreaterThan(0) + for (const staging of repaired) { + expect(staging).not.toBe(shared) + expect(_isPerWriteStagingName(path.basename(staging))).toBe(true) + } + // The staged file is created inside that directory and committed from it, so nothing + // lands in the outside directory the link points at. + const staged = String(vi.mocked(fsSync.openSync).mock.calls[0]?.[0]) + expect(path.dirname(staged)).toBe(repaired[0]) + expect(fs.rename).toHaveBeenCalledWith(staged, targetPath) + // And the release removes this write's directory, never the planted link. + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => String(c[0]))).toEqual([repaired[0]]) + expect(fsSync.rmdirSync).not.toHaveBeenCalled() + }) + + it("a plain directory at the shared staging name is never chmod-ed or written into", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const shared = _sharedStagingDir(path.dirname(targetPath)) + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // A committed plain directory owned by this process: nothing here rejects it on + // inspection, so only the generated name keeps this write out of it. + vi.mocked(fsSync.lstatSync).mockImplementation(_lstatPerPath(shared, _dirStats())) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).resolves.toBeUndefined() + + // The repair never touches a directory this write did not create ... + for (const call of vi.mocked(fsSync.chmodSync).mock.calls) { + expect(String(call[0])).not.toBe(shared) + } + // ...nothing is staged inside it ... + for (const call of vi.mocked(fsSync.openSync).mock.calls) { + expect(path.dirname(String(call[0]))).not.toBe(shared) + } + // ...and the release does not remove a directory the workspace shipped. + expect(fsSync.rmdirSync).not.toHaveBeenCalledWith(shared) + expect(vi.mocked(fs.rmdir).mock.calls.map((c) => String(c[0]))).not.toContain(shared) + }) + }) + + // ── 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).toHaveBeenCalled() + + // Cross-mock invocation ORDER, not just call counts: a count-only assertion still + // passes if the implementation fsyncs after the commit rename, which is the exact + // durability regression this suite exists to catch. + const firstCallOf = (mock: { mock: { invocationCallOrder: number[] } }) => mock.mock.invocationCallOrder[0] + const renameOrder = firstCallOf(vi.mocked(fs.rename)) + expect(firstCallOf(vi.mocked(fsSync.openSync))).toBeLessThan(renameOrder) + expect(firstCallOf(vi.mocked(fsSync.writeSync))).toBeLessThan(renameOrder) + expect(firstCallOf(vi.mocked(fsSync.fsyncSync))).toBeLessThan(renameOrder) + expect(firstCallOf(vi.mocked(fsSync.closeSync))).toBeLessThan(renameOrder) + // the staged file is fsynced before it is closed + expect(vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.closeSync).mock.invocationCallOrder[0], + ) + // the parent-directory fsync is the second fsync and lands AFTER the rename + expect(vi.mocked(fsSync.fsyncSync).mock.invocationCallOrder[1]).toBeGreaterThan(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 + 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_")) + + // no rollback rename: the committed target is not restored from the backup + expect(fs.rename).toHaveBeenCalledTimes(1) + + // the staging temp was already committed by the rename; nothing + // temp-shaped is unlinked afterwards + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + }) + + // ── Test 4: backup:true keeps old safeWriteJson semantics incl. rollback ── + + describe("backup:true", () => { + // These tests exercise the win32 publish path, whose DACL capture is judged by its + // artifact, so the mock world has to produce a usable dump. + beforeEach(() => { + // Path-aware: this code stats two different paths - the target, for its mode, and the + // dump, to judge the capture. One stub for both paths would let a regression that + // stats the target instead of the dump pass, and would silently stage with mode 0 + // because the stub carries no mode. + vi.mocked(fsSync.statSync).mockImplementation(((p: unknown) => + typeof p === "string" && p.includes(".acl.tmp") + ? { isFile: () => true, size: 256 } + : _stats(0o644)) as never) + }) + it("copies target -> backup before commit, deletes backup 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) + + // first rename: target -> backup + expect(fs.copyFile).toHaveBeenCalledWith(targetPath, expect.stringContaining("safeWriteText.bak_")) + + // second rename: temp -> target (realpath mock returns targetPath) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // backup was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("a failed publish leaves the target in place and removes the backup copy", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // the only rename is the publish, and it fails + vi.mocked(fs.rename).mockRejectedValue(new Error("ENOSPC")) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // no rollback rename exists: the target was never moved, so cleanup removes the copy + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenCalledTimes(1) + + // temp was cleaned up on failure + 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", () => { + // The DACL capture is judged by its artifact, so the mock world has to produce one: + // a regular, non-empty dump. Tests that want an unusable artifact override this. + beforeEach(() => { + // Path-aware: this code stats two different paths - the target, for its mode, and the + // dump, to judge the capture. One stub for both paths would let a regression that + // stats the target instead of the dump pass, and would silently stage with mode 0 + // because the stub carries no mode. + vi.mocked(fsSync.statSync).mockImplementation(((p: unknown) => + typeof p === "string" && p.includes(".acl.tmp") + ? { isFile: () => true, size: 256 } + : _stats(0o644)) as never) + }) + 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) + vi.mocked(fsSync.openSync).mockReturnValue(1) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // The neighbouring argument test passes backup:true, so this no-backup case is + // kept - but it has to check the commands, not just their count. + expect(execFile).toHaveBeenCalledTimes(2) + + const saveCall = vi.mocked(execFile).mock.calls[0] + expect(saveCall[0]).toBe("icacls") + // No /T: with a file path it makes icacls walk the whole tree and save every file of + // that name, and the restore then rewrites all of their DACLs. + expect(saveCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp")]) + + const restoreCall = vi.mocked(execFile).mock.calls[1] + expect(restoreCall[0]).toBe("icacls") + expect(restoreCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining(".acl.tmp"), + ]) + }, + ) + + 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("fails closed when icacls reports success but wrote no dump", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls exits 0 without producing the dump (measured when the target's owner differs + // from the caller). The exit code alone is not evidence that a descriptor was captured. + vi.mocked(execFile).mockImplementation((( + _c: string, + _a: string[], + _o: unknown, + cb?: (e?: Error | null) => void, + ) => { + cb?.(null) + return undefined + }) as never) + vi.mocked(fsSync.statSync).mockImplementation((() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) as never) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow( + /refusing to publish/, + ) + }) + + it("fails closed when the dump is a regular file but empty", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // A zero-byte dump is not a captured security descriptor: icacls can create the file and + // write nothing into it. Accepting it would publish with a restore that carries no data. + vi.mocked(execFile).mockImplementation((( + _c: string, + _a: string[], + _o: unknown, + cb?: (e?: Error | null) => void, + ) => { + cb?.(null) + return undefined + }) as never) + vi.mocked(fsSync.statSync).mockImplementation((() => ({ isFile: () => true, size: 0 })) as never) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow(/refus/) + }) + + it.each([ + ["EACCES", "EACCES"], + ["a code-less probe error", undefined], + ])("propagates a %s target-existence probe instead of treating the target as absent", async (_label, code) => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // An unreadable target is not an absent target: publishing over it would replace a file + // whose security descriptor was never captured. + // Call-order aware: only the DACL step's probe fails. A later fs.access in the publish + // path succeeds, so a swallowed probe error would let the write continue - which is what + // makes this assertion load-bearing rather than accidentally satisfied by a later failure. + // Path-aware, not call-order: fs.access is also called for the parent directory earlier in + // the flow, so a first-call mock fails the WRONG probe and the assertion is satisfied by + // accident. Only the probe of the target itself may fail here. + vi.mocked(fs.access).mockImplementation((async (p: string) => { + if (p === targetPath) { + throw Object.assign(new Error("probe failed"), code ? { code } : {}) + } + return undefined + }) as never) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow("probe failed") + expect(execFile).not.toHaveBeenCalled() + // Nothing downstream of the probe may run: a swallowed probe error would keep going and + // copy the target to a backup, which is what makes this assertion load-bearing. + expect(fs.copyFile).not.toHaveBeenCalled() + }) + + it("removes the staged file and the dump when the DACL capture aborts", 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" }) + }) as never) + const unlinkMock = vi.mocked(fs.unlink) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow( + /refusing to publish/, + ) + + // The abort leaves the try/finally that owns cleanup, so the artifacts are released on the + // abort path itself: neither the dump nor the staged file may be orphaned. + const unlinked = unlinkMock.mock.calls.map(function (call) { + return String(call[0]) + }) + expect( + unlinked.some(function (p) { + return p.includes(".acl.tmp") + }), + ).toBe(true) + expect( + unlinked.some(function (p) { + return p.includes("safeWriteText_") + }), + ).toBe(true) + }) + + it("reports the retained artifact when the abort cleanup cannot remove it", 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" }) + }) as never) + vi.mocked(fs.unlink).mockRejectedValue(new Error("EBUSY")) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow( + /refusing to publish/, + ) + + const warned = warnSpy.mock.calls + .map(function (call) { + return String(call[0]) + }) + .join("\n") + expect(warned).toContain("could not remove") + }) + + it("win32 DACL capture failure aborts before publishing", async () => { + // The capture is judged by the artifact: make the dump absent so the capture fails. + vi.mocked(fsSync.statSync).mockImplementation((() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) as never) + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails and no usable dump exists. + 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" })).rejects.toThrow( + /refusing to publish/, + ) + + // Fail closed: the target is never published, because publishing would replace its security + // descriptor with inherited permissions and nothing could put the original back. + // The refusal lands before the commit rename - the target keeps its content. + expect(fs.rename).not.toHaveBeenCalled() + // Only the save ran: a failed capture must not be followed by a restore attempt. + expect(execFile).toHaveBeenCalledTimes(1) + // The staging file and any partial dump are still cleaned up by the rollback. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl.tmp")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("win32 DACL: a non-zero icacls exit that still wrote a dump counts as a capture", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Measured on a normal host: /save can exit non-zero while leaving a usable dump behind. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls exit 1"), "", "") + return fakeChild + }) + vi.mocked(fsSync.statSync).mockReturnValue({ isFile: () => true, size: 256 } as never) + + // The capture is judged by the artifact, so the publish proceeds and the restore is + // attempted. This mocked icacls fails the restore too, and a failed restore reaches + // the caller as DaclRestoreError rather than a warning beside a resolved write. + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf( + DaclRestoreError, + ) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // Save, then the restore attempt (icacls is retried once if it fails, so >= 2). + const restoreCalls = vi.mocked(execFile).mock.calls.filter(function (call) { + return Array.isArray(call[1]) && String(call[1]).includes("/restore") + }) + expect(restoreCalls.length).toBeGreaterThan(0) + }) + + it("skips the DACL save and restore for the rest of the process once a restore reports the missing privilege", async () => { + // The memo is process-wide, so this test runs against a fresh module instance: + // otherwise the write below would disable DACL handling for every later test in + // the file. vi.mock factories still apply to the re-imported module. + vi.resetModules() + // The re-imported module has its own class identity, so the rejection is matched + // against the class this instance throws, not the one imported at the top. + const { safeWriteText: freshWriteText, DaclRestoreError: FreshDaclRestoreError } = + await import("../safeWriteText") + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + const privilegeFailure = Object.assign(new Error("Access is denied."), { code: 1300 }) + let calls = 0 + const runner = vi.fn((...args: unknown[]) => { + calls++ + const cb = args[args.length - 1] as (err: Error | null) => void + // The save succeeds; every restore reports the missing privilege. + cb(args[1] && String((args[1] as string[])[1]) === "/restore" ? privilegeFailure : null) + }) as unknown as typeof execFile + const options = { platform: "win32", execFileRunner: runner } + + // The first publish still loses its DACL, so it is reported as the typed error - + // the memo only changes what LATER writes in this process pay for. + await expect(freshWriteText(targetPath, "data", options)).rejects.toBeInstanceOf(FreshDaclRestoreError) + // Exactly one save and one restore: the 1300 exit tells the restore it is + // the missing privilege, so the transient retry must not run. + expect(calls).toBe(2) + expect(warn).toHaveBeenCalledWith(expect.stringContaining("cannot restore DACLs")) + + calls = 0 + warn.mockClear() + // The memo buys the zero-spawn path; it does not buy a silent success. This write + // still replaced the target's descriptor, so it is still reported. + await expect(freshWriteText(targetPath, "data", options)).rejects.toBeInstanceOf(FreshDaclRestoreError) + + expect(calls).toBe(0) + expect(warn).not.toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + warn.mockRestore() + }) + + it("win32 DACL: a later write that ran with restoration disabled reports the lost DACL", async () => { + // The memo skips the save/restore pair to save the process spawns. It must not also + // change the answer: the commit is a rename, so a later write over an existing target + // still hands the path a file created with a different descriptor, and with the pair + // skipped nothing saved the old one and nothing can put it back. Resolving quietly + // would tell the caller the access rights are unchanged when they are not. + vi.resetModules() + const { safeWriteText: freshWriteText, DaclRestoreError: FreshDaclRestoreError } = + await import("../safeWriteText") + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + const privilegeFailure = Object.assign(new Error("Access is denied."), { code: 1300 }) + let calls = 0 + const runner = vi.fn((...args: unknown[]) => { + calls++ + const cb = args[args.length - 1] as (err: Error | null) => void + cb(args[1] && String((args[1] as string[])[1]) === "/restore" ? privilegeFailure : null) + }) as unknown as typeof execFile + const options = { platform: "win32", execFileRunner: runner, backup: true } + + // First write: capture ran, restore reported 1300, memo set, own loss reported. + await expect(freshWriteText(targetPath, "data", options)).rejects.toBeInstanceOf(FreshDaclRestoreError) + + calls = 0 + vi.mocked(fs.copyFile).mockClear() + vi.mocked(fs.unlink).mockClear() + const second = await _rejectionReport(freshWriteText(targetPath, "data", options)) + + // The condition reaches the caller as the same post-commit error a failed restore + // uses, instead of a resolved write, and the report names the condition it is + // reporting: a reader must be able to tell 'this host cannot restore' apart from + // 'the restore command failed this time'. + expect(second.name).toBe("DaclRestoreError") + expect(second.message).toContain("restoration is disabled") + // Restoration stayed disabled: the memo still buys zero process spawns. + expect(calls).toBe(0) + // The content is still committed - a landed publish whose access rights changed, + // not a refused write. + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + // And the backup copy, now the only artifact carrying the original descriptor, was + // really created and really retained. + const copies = vi.mocked(fs.copyFile).mock.calls + expect(copies).toHaveLength(1) + expect(String(copies[0]?.[0])).toBe(targetPath) + const backupDestination = String(copies[0]?.[1]) + expect( + vi.mocked(fs.unlink).mock.calls.filter((call) => String(call[0]) === backupDestination), + ).toHaveLength(0) + warn.mockRestore() + }) + + it("win32 DACL: restoration disabled does not reject a write to a brand-new target", async () => { + // A file that did not exist has no descriptor to lose - it takes a default one at + // creation either way. Reporting the condition there would fail ordinary saves on + // every un-elevated Windows host, so the report is scoped to replaced targets. + vi.resetModules() + const { safeWriteText: freshWriteText, DaclRestoreError: FreshDaclRestoreError } = + await import("../safeWriteText") + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + const privilegeFailure = Object.assign(new Error("Access is denied."), { code: 1300 }) + const runner = vi.fn((...args: unknown[]) => { + const cb = args[args.length - 1] as (err: Error | null) => void + cb(args[1] && String((args[1] as string[])[1]) === "/restore" ? privilegeFailure : null) + }) as unknown as typeof execFile + const options = { platform: "win32", execFileRunner: runner } + + await expect(freshWriteText(targetPath, "data", options)).rejects.toBeInstanceOf(FreshDaclRestoreError) + + // From here the target is gone: the step 2b probe must read ENOENT as "nothing to + // preserve" and let the publish resolve. + vi.mocked(fs.access).mockImplementation(async (p) => { + if (typeof p === "string" && p.endsWith("target.txt")) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + return undefined + }) + + await expect(freshWriteText(targetPath, "data", options)).resolves.toBeUndefined() + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + warn.mockRestore() + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath] 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(".acl.tmp")]) + + // 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(".acl.tmp"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("two concurrent writes to the same target use different DACL dumps", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await Promise.all([ + safeWriteText(targetPath, "a", { platform: "win32" }), + safeWriteText(targetPath, "b", { platform: "win32" }), + ]) + + // A shared dump name lets one call unlink or overwrite the file the other is still using, + // which silently loses the DACL restore. Each call must own its own dump. + const saves = vi.mocked(execFile).mock.calls.filter((call) => (call[1] as string[]).indexOf("/save") >= 0) + expect(saves.length).toBe(2) + const dumps = saves.map((call) => String((call[1] as string[])[2])) + expect(dumps[0]).not.toBe(dumps[1]) + expect(dumps[0]).toContain("safeWriteText.acl.tmp") + expect(dumps[1]).toContain("safeWriteText.acl.tmp") + }) + + it("win32 DACL: a failed restore is a DaclRestoreError 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, both restore attempts fail + 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 + }) + + // A publish whose DACL was not restored reaches the caller as its own error: + // no caller can observe success for it. + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBeInstanceOf( + DaclRestoreError, + ) + + // The content did commit before the restore failed: the rename happened exactly + // once even though the publish reports an error. + expect(fs.rename).toHaveBeenCalledTimes(1) + + // Contract change: the changed access rights are an error the caller receives, + // not a warning beside a write that resolved. + expect(warnSpy.mock.calls.flat().join(" ")).not.toContain("could not be restored") + warnSpy.mockRestore() + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("removes the DACL dump on the second attempt when the first unlink fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + let dumpUnlinks = 0 + const unlinkMock = vi.mocked(fs.unlink) + unlinkMock.mockImplementation(async (p) => { + if (typeof p === "string" && p.includes(".acl.tmp")) { + dumpUnlinks++ + if (dumpUnlinks === 1) { + throw new Error("EBUSY") + } + } + return undefined + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // One bounded retry is enough here, and nothing is surfaced because the retry succeeded. + expect(dumpUnlinks).toBe(2) + expect(warnSpy).not.toHaveBeenCalled() + }) + + it("warns with the retained dump path when both dump unlinks fail", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const unlinkMock = vi.mocked(fs.unlink) + unlinkMock.mockImplementation(async (p) => { + if (typeof p === "string" && p.includes(".acl.tmp")) { + throw new Error("EBUSY") + } + return undefined + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // An icacls dump is a readable copy of the target's ACL: leaving it behind is acceptable, + // leaving it behind invisibly is not. + const dumpCalls = unlinkMock.mock.calls.filter(function (call) { + return typeof call[0] === "string" && call[0].includes(".acl.tmp") + }) + expect(dumpCalls).toHaveLength(2) + const warned = warnSpy.mock.calls + .map(function (call) { + return String(call[0]) + }) + .join("\n") + expect(warned).toContain(".acl.tmp") + }) + + it("keeps the publish and warns when both backup-removal attempts fail", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Only the backup unlink fails, twice; every other unlink works. + const unlinkMock = vi.mocked(fs.unlink) + unlinkMock.mockImplementation(async (p) => { + if (typeof p === "string" && p.includes(".bak")) { + throw new Error("EBUSY") + } + return undefined + }) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await safeWriteText(targetPath, "new data", { backup: true }) + + // The write itself is unaffected - the backup is cleanup, not part of the commit. + expect(fs.rename).toHaveBeenCalled() + const bakCalls = unlinkMock.mock.calls.filter(function (call) { + return typeof call[0] === "string" && call[0].includes(".bak") + }) + expect(bakCalls).toHaveLength(2) + const warned = warnSpy.mock.calls + .map(function (call) { + return String(call[0]) + }) + .join("\n") + expect(warned).toContain("could not be removed") + expect(warned).toContain(".bak") + }) + + it("removes a partially copied backup when the copy itself fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // copyFile can create the destination and then fail part way through. + vi.mocked(fs.copyFile).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("EACCES") + + // The half-written backup must not be left next to the target, and the original error + // is still the one that surfaces. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".bak")) + }) + + it("closes the staging descriptor when applying the target mode fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(7) + vi.mocked(fsSync.fchmodSync).mockImplementation(() => { + throw Object.assign(new Error("EPERM"), { code: "EPERM" }) + }) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EPERM") + + // The descriptor is opened before the mode is applied, so the close has to live in the + // finally that also covers the mode call - otherwise a mode failure leaks the fd. + expect(fsSync.closeSync).toHaveBeenCalledWith(7) + expect(fsSync.writeSync).not.toHaveBeenCalled() + }) + + it("win32 DACL: when both restore attempts fail, the backup is retained", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // /save succeeds (call 1); both /restore attempts fail (calls 2 and 3). + 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 + }) + + // The publish reports the lost DACL as the typed error... + await expect(safeWriteText(targetPath, "data", { platform: "win32", backup: true })).rejects.toBeInstanceOf( + DaclRestoreError, + ) + + // ...but the content is still committed - the restore failure lands after the rename. + expect(fs.rename).toHaveBeenCalled() + // The dump is still cleaned up in the finally block. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + // And the backup survives the rejection: it is the only artifact that still carries + // the target's original security descriptor, so deleting it would destroy the + // recovery path. The copy is asserted first: counting the absence of a .bak unlink + // also passes when no backup was ever created, which is not the same fact. + const copies = vi.mocked(fs.copyFile).mock.calls + expect(copies).toHaveLength(1) + const [copiedSource, copiedDestination] = copies[0] ?? [] + expect(String(copiedSource)).toBe(targetPath) + const backupDestination = String(copiedDestination) + expect(backupDestination).toContain("safeWriteText.bak_") + const bakUnlinks = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]) === backupDestination + }) + expect(bakUnlinks).toHaveLength(0) + }) + + it("staging temp release: a transient unlink failure on the rollback path is retried", 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("EBUSY rename"), { code: "EBUSY" })) + let stagingUnlinks = 0 + vi.mocked(fs.unlink).mockImplementation((async (path: string) => { + if (String(path).includes(".file-safety-staging")) { + stagingUnlinks++ + if (stagingUnlinks === 1) { + throw Object.assign(new Error("EBUSY unlink"), { code: "EBUSY" }) + } + } + }) as never) + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EBUSY rename") + expect(stagingUnlinks).toBe(2) + expect( + warn.mock.calls.filter(function (call) { + return String(call[0]).includes("staging temp release failed") + }).length, + ).toBe(0) + warn.mockRestore() + }) + + it("staging temp release: a persistent failure names the retained path exactly once", 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("EBUSY rename"), { code: "EBUSY" })) + let stagingUnlinks = 0 + vi.mocked(fs.unlink).mockImplementation((async (path: string) => { + if (String(path).includes(".file-safety-staging")) { + stagingUnlinks++ + throw Object.assign(new Error("EBUSY unlink"), { code: "EBUSY" }) + } + }) as never) + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + + await expect(safeWriteText(targetPath, "data")).rejects.toThrow("EBUSY rename") + expect(stagingUnlinks).toBe(2) + // The retry exists so the reader learns WHICH path was retained, so the assertions + // have to name it: counting attempts and matching a prefix would still pass if the + // warning reported a different path than the one being unlinked. + const stagedPath = String(vi.mocked(fs.rename).mock.calls[0]?.[0]) + expect(stagedPath).toContain(".file-safety-staging") + const unlinkAttempts = vi.mocked(fs.unlink).mock.calls.filter(function (call) { + return String(call[0]) === stagedPath + }) + expect(unlinkAttempts.length).toBe(2) + const releaseWarnings = warn.mock.calls.filter(function (call) { + return String(call[0]).includes("staging temp release failed") + }) + expect(releaseWarnings.length).toBe(1) + expect(String(releaseWarnings[0]?.[0])).toContain(stagedPath) + warn.mockRestore() + }) + + it("win32 DACL: a non-regular dump file fails closed even when it is non-empty", 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 + }) + // The dump exists with bytes in it, but it is not a regular file (a directory, a FIFO or a + // device left behind by another process). icacls /restore against it is not a restore of our + // descriptor, so the write has to fail closed instead of publishing. + vi.mocked(fsSync.statSync).mockReturnValue({ isFile: () => false, size: 4096 } as never) + + await expect(safeWriteText(targetPath, "data", { platform: "win32", backup: true })).rejects.toThrow( + /DaclCaptureError|refus/i, + ) + + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("win32 DACL: a failed first restore is retried and the backup is then removed", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // /save succeeds (call 1), the first /restore fails (call 2), the retry succeeds (call 3). + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 2 ? new Error("icacls transient error") : null, "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32", backup: true }) + + const saves = vi.mocked(execFile).mock.calls.filter(function (call) { + return String(call[1]).includes("/save") + }) + const restores = vi.mocked(execFile).mock.calls.filter(function (call) { + return String(call[1]).includes("/restore") + }) + expect(saves.length).toBe(1) + expect(restores.length).toBe(2) + expect(fs.rename).toHaveBeenCalled() + // The descriptor is back, so the backup is no longer the recovery artifact and is removed. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".bak")) + }) + + 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/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/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("refuses to stage when the target mode is unknown (non-ENOENT stat error)", 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 + }) + + // Falling back to 0o644 here could publish a 0o600 target with a wider mode. + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + }) + + it("refuses to publish a caller-supplied temp when the target mode is unknown", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const customTempPath = "/tmp/custom-temp.tmp" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(2) + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw eacces + }) + + // The temp carries its own creation mode, which may be wider than the target's. + await expect( + safeWriteText(targetPath, "data", { tempPath: customTempPath, platform: "linux" }), + ).rejects.toBe(eacces) + + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + }) + + 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/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("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/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) + }) + }) + + // ── 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" })) + 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("follows a dangling symlink to its referent instead of replacing the link", async () => { + const linkPath = path.resolve("/tmp/dangling-dir/link.txt") + const referent = path.resolve("/tmp/dangling-dir/real.txt") + // realpath reports ENOENT both for a missing path and for a dangling link; only the + // link lookup is a symlink, the referent resolves as an ordinary absent target. + vi.mocked(fs.realpath).mockRejectedValueOnce(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.realpath).mockResolvedValue(referent) + const linkStats = Object.create(fsSync.Stats.prototype) as fsSync.Stats & { + isSymbolicLink?: () => boolean + isDirectory?: () => boolean + } + linkStats.isSymbolicLink = () => true + linkStats.isDirectory = () => false + vi.mocked(fs.lstat).mockResolvedValueOnce(linkStats) + vi.mocked(fs.readlink).mockResolvedValueOnce("real.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(linkPath, "hello", { platform: "linux" }) + + // The commit rename must publish at the referent, never over the link path. + expect(fs.lstat).toHaveBeenCalledWith(linkPath) + const renames = vi.mocked(fs.rename).mock.calls + expect(renames.at(-1)?.[1]).toBe(referent) + expect(renames.some((call) => call[1] === linkPath)).toBe(false) + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("applies an existing target's mode with fchmodSync, not the umask-masked creation mode", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + // A group-writable target: openSync(path, "w", 0o664) would create 0o644 under a + // 0o022 umask, and the rename would publish the narrowed mode. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664)) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fsSync.fchmodSync).toHaveBeenCalledWith(1, 0o664) + }) + + it("leaves a fresh target to its creation mode", async () => { + const targetPath = "/tmp/test-dir/new-target.txt" + const enoent = Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + vi.mocked(fs.realpath).mockRejectedValue(enoent) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw enoent + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.closeSync).mockReturnValue(undefined) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fsSync.fchmodSync).not.toHaveBeenCalled() + }) + + 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("keeps the target's group on the staged file before publishing over it", async () => { + // The commit is a rename, so the new inode would otherwise take this process's + // primary group and a shared 0o664 group-owned file would lose group write access + // even with the mode restored. Order matters: chown can clear setgid bits, so it + // has to run before the chmod. + 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(0o664, 4321)) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fsSync.fchownSync).toHaveBeenCalledWith(1, -1, 4321) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(1, 0o664) + expect(vi.mocked(fsSync.fchownSync).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0], + ) + }) + + it("keeps the target's group when the caller staged the temp file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(7) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o664, 4321)) + + await safeWriteText(targetPath, "data", { platform: "linux", tempPath: "/tmp/test-dir/caller.tmp" }) + + expect(fsSync.fchownSync).toHaveBeenCalledWith(7, -1, 4321) + expect(fsSync.fchmodSync).toHaveBeenCalledWith(7, 0o664) + expect(vi.mocked(fsSync.fchownSync).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(fsSync.fchmodSync).mock.invocationCallOrder[0], + ) + }) + + it("does not attempt a group change on a platform whose identity is the DACL", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Windows carries no POSIX gid to preserve - its identity is the DACL, which the + // save/restore steps own - so the DACL path has to stay reachable here. + vi.mocked(fsSync.statSync).mockImplementation(((p: unknown) => + typeof p === "string" && p.includes(".acl.tmp") + ? { isFile: () => true, size: 256 } + : _stats(0o664, 4321)) as never) + vi.mocked(execFile).mockImplementation(((...args: unknown[]) => { + const cb = args[args.length - 1] as (err: Error | null) => void + cb(null) + }) as never) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fsSync.fchownSync).not.toHaveBeenCalled() + expect(fsSync.fchmodSync).toHaveBeenCalledWith(1, 0o664) + }) + + 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 resolving as durable", 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 rename committed, so the content is in place, but the caller must not be + // told the publish is durable when the directory entry could not be fsynced. + expect(await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux" }))).toBe( + "PublishNotDurableError", + ) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("keeps the backup when the commit is not confirmed durable", 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") + }) + + expect(await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux", backup: true }))).toBe( + "PublishNotDurableError", + ) + + // The previous content stays recoverable while durability is unconfirmed. + expect(fs.unlink).not.toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("refuses to stage through a planted .file-safety-staging symlink", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // lstat reports a non-directory at the staging path. mkdirSync(recursive) + // would follow it, staging - and then publishing - outside the target directory. + vi.mocked(fsSync.lstatSync).mockReturnValue(_linkStats()) + + expect(await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux" }))).toBe( + "UnsafeStagingDirectoryError", + ) + + expect(fsSync.mkdirSync).not.toHaveBeenCalled() + }) + + it.skipIf(process.platform === "win32")( + "refuses to stage in a .file-safety-staging directory owned by another uid", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // A staging directory that exists but belongs to a different uid: the writer must + // not place temp files where another user could observe or pre-create them. + const foreign = _dirStats() as fsSync.Stats & { uid?: number } + foreign.uid = (process.getuid?.() ?? 0) + 1 + vi.mocked(fsSync.lstatSync).mockReturnValue(foreign) + + expect(await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux" }))).toBe( + "UnsafeStagingDirectoryError", + ) + + // Nothing is staged and nothing is published from the foreign directory. + }, + ) + + it("removes the staging directory once the write is over", 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" }) + + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("re-creates the staging directory when a concurrent write removes it mid-write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockImplementationOnce(() => { + // Another writer committed and rmdir'd the shared staging directory + // between _stagingDir() and this open. + throw Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + }) + .mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // Once for the original staging call, once for the recovery. + expect(fsSync.mkdirSync).toHaveBeenCalledTimes(2) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("gives up after one recovery when the staging open keeps failing with 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.openSync).mockImplementation(() => { + throw enoent + }) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(enoent) + + // One staging create plus one recovery attempt, then the error surfaces. + expect(fsSync.mkdirSync).toHaveBeenCalledTimes(2) + expect(fsSync.openSync).toHaveBeenCalledTimes(2) + }) + + it("retries a transient Windows sharing failure during the commit rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // No target yet, so the Windows DACL steps are skipped and only the rename is exercised. + vi.mocked(fs.access).mockImplementation(async (p: fsSync.PathLike) => { + if (p === targetPath) throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const eperm = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + vi.mocked(fs.rename).mockRejectedValueOnce(eperm) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fs.rename).toHaveBeenCalledTimes(2) + }) + + it.each(["EPERM", "EACCES", "EBUSY"] as const)( + "retries the commit rename for the transient Windows sharing code %s", + async (code) => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.access).mockImplementation(async (p: fsSync.PathLike) => { + if (p === targetPath) throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const failure = Object.assign(new Error(code + ": transient sharing violation"), { code }) + vi.mocked(fs.rename).mockRejectedValueOnce(failure) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fs.rename).toHaveBeenCalledTimes(2) + }, + ) + + it("gives up after five retries and propagates the original rename error", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fs.access).mockImplementation(async (p: fsSync.PathLike) => { + if (p === targetPath) throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const ebusy = Object.assign(new Error("EBUSY: resource busy or locked"), { code: "EBUSY" }) + vi.mocked(fs.rename).mockRejectedValue(ebusy) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toBe(ebusy) + + // The initial attempt plus _RENAME_RETRY_ATTEMPTS (5), then the original error. + expect(fs.rename).toHaveBeenCalledTimes(6) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("creates the staging dir when lstatSync reports it absent on first use", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const enoent = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + // First lstat (the guard) sees no staging dir at all; the post-create re-check does. + vi.mocked(fsSync.lstatSync) + .mockImplementationOnce(() => { + throw enoent + }) + .mockImplementation(() => _dirStats()) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // The name is generated per write, so it is read back from the create rather than + // restated here: what this test pins is that the guard's ENOENT path still creates + // exactly one private staging directory, and that this write stages inside it. + const creates = vi.mocked(fsSync.mkdirSync).mock.calls + expect(creates).toHaveLength(1) + expect(creates[0]?.[1]).toEqual({ recursive: true, mode: 0o700 }) + const staged = String(vi.mocked(fsSync.openSync).mock.calls[0]?.[0]) + expect(path.dirname(staged)).toBe(String(creates[0]?.[0])) + expect(fs.rename).toHaveBeenCalled() + }) + + it("rejects when the staging path is replaced between the guard and the re-check", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Guard: a real directory. Re-check after mkdirSync: a symlink planted in between. + vi.mocked(fsSync.lstatSync).mockReturnValueOnce(_dirStats()).mockReturnValueOnce(_linkStats()) + + const name = await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux" })) + expect(name).toBe("UnsafeStagingDirectoryError") + }) + + it("removes a staging directory it created when the post-create re-check rejects it", 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" }) + // Guard: nothing there, so this call is the one that creates it. Re-check: swapped for a + // symlink. The caller never receives the path, so this call is also the only one that can + // clean it up - otherwise a stray .file-safety-staging sits in the user's directory. + vi.mocked(fsSync.lstatSync) + .mockImplementationOnce(() => { + throw enoent + }) + .mockReturnValueOnce(_linkStats()) + + const name = await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux" })) + expect(name).toBe("UnsafeStagingDirectoryError") + // The directory removed is exactly the one this call created - read back rather than + // restated, since the name is generated per write. + expect(fsSync.rmdirSync).toHaveBeenCalledWith(String(vi.mocked(fsSync.mkdirSync).mock.calls[0]?.[0])) + }) + + it("leaves a pre-existing staging directory when the post-create re-check rejects it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // Guard: the directory was already there - it may belong to a concurrent write, so this + // call must not remove it even though the re-check rejects the path. + vi.mocked(fsSync.lstatSync).mockReturnValueOnce(_dirStats()).mockReturnValueOnce(_linkStats()) + + const name = await _rejectionName(safeWriteText(targetPath, "data", { platform: "linux" })) + expect(name).toBe("UnsafeStagingDirectoryError") + expect(fsSync.rmdirSync).not.toHaveBeenCalled() + }) + + it("does not retry a rename failure on a platform without sharing violations", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const eperm = Object.assign(new Error("EPERM: operation not permitted"), { code: "EPERM" }) + vi.mocked(fs.rename).mockRejectedValue(eperm) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eperm) + + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + 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) + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + }) + + 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", + ) + }) + }) + + describe("failure paths inside the staging write", () => { + it("propagates a writeSync failure, closes the descriptor, and never publishes the staged file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const writeError = new Error("ENOSPC: no space left on device") + vi.mocked(fsSync.writeSync).mockImplementationOnce(() => { + throw writeError + }) + + await expect(safeWriteText(targetPath, "hello world", { platform: "linux" })).rejects.toBe(writeError) + + // The descriptor opened for the staging file must still be closed. + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + // Nothing may be renamed into the target position after a partial write. + // The truncated staging file is removed on failure. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("propagates an fsync failure, closes the descriptor, and never publishes the staged file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const fsyncError = new Error("EBADF: fsync failed") + vi.mocked(fsSync.fsyncSync).mockImplementationOnce(() => { + throw fsyncError + }) + + await expect(safeWriteText(targetPath, "hello world", { platform: "linux" })).rejects.toBe(fsyncError) + + expect(fsSync.closeSync).toHaveBeenCalledWith(1) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("backup:true copies the target instead of moving it, so a failed publish leaves the target in place", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const publishError = new Error("EXDEV: cross-device rename not permitted") + vi.mocked(fs.rename).mockRejectedValueOnce(publishError) + + await expect(safeWriteText(targetPath, "hello world", { platform: "linux", backup: true })).rejects.toBe( + publishError, + ) + + // The backup is a copy, so the target was never renamed away and there is nothing to roll back. + expect(fs.copyFile).toHaveBeenCalled() + expect(fs.rename).toHaveBeenCalledTimes(1) + // Both the backup copy and the staging temp are removed on failure. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("removes the DACL dump and aborts when the save fails after creating it", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // execFile callback form: report a failure after the dump file was created. + vi.mocked(execFile).mockImplementationOnce((( + _cmd: string, + _args: string[], + _opts: unknown, + cb: (err: Error) => void, + ) => { + cb(new Error("icacl dump failed")) + return fakeChild + }) as unknown as typeof execFile) + + await expect(safeWriteText(targetPath, "hello world", { platform: "win32" })).rejects.toThrow( + /refusing to publish/, + ) + + // The dump path stays tracked so the rollback removes the file the failed save created. + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl.tmp")) + }) + }) +}) + +describe("resolvePublishTarget symlink cycle (S1)", () => { + beforeEach(() => { + // This describe sits outside the file's main describe, so it clears the shared + // fs stubs itself: the assertions below are about 'never called at all'. + vi.mocked(fs.rename).mockClear() + vi.mocked(fs.copyFile).mockClear() + vi.mocked(fs.unlink).mockClear() + vi.mocked(fsSync.openSync).mockClear() + vi.mocked(fsSync.writeSync).mockClear() + }) + + it("raises ELOOP for two dangling links that point at each other, before any staging", async () => { + const linkA = path.resolve("/work/a.link") + const linkB = path.resolve("/work/b.link") + + // realpath reports ENOENT for a dangling link, so the resolver follows the link text. + // Two links that point at each other are both dangling: the visited set is what bounds + // the recursion, and the cycle must surface as ELOOP. + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.lstat).mockResolvedValue({ isSymbolicLink: () => true } as unknown as fsSync.Stats) + vi.mocked(fs.readlink).mockImplementation(async (target) => (String(target) === linkA ? linkB : linkA)) + + await expect(resolvePublishTarget(linkA)).rejects.toMatchObject({ code: "ELOOP" }) + + // Nothing may be staged or published for a path whose referent cannot be resolved. + expect(vi.mocked(fs.rename)).not.toHaveBeenCalled() + expect(vi.mocked(fsSync.openSync)).not.toHaveBeenCalled() + expect(vi.mocked(fs.copyFile)).not.toHaveBeenCalled() + }) + + it("surfaces ELOOP through safeWriteText without touching the target", async () => { + const linkA = path.resolve("/work/a.link") + const linkB = path.resolve("/work/b.link") + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.lstat).mockResolvedValue({ isSymbolicLink: () => true } as unknown as fsSync.Stats) + vi.mocked(fs.readlink).mockImplementation(async (target) => (String(target) === linkA ? linkB : linkA)) + + await expect(safeWriteText(linkA, "payload")).rejects.toMatchObject({ code: "ELOOP" }) + + expect(vi.mocked(fs.rename)).not.toHaveBeenCalled() + expect(vi.mocked(fsSync.openSync)).not.toHaveBeenCalled() + }) + + it("propagates an lstat failure that follows a realpath ENOENT", async () => { + const linkPath = "/tmp/test-dir/dangling-link" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.lstat).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + // readlink is stubbed by earlier tests in this file and this describe does not reset it, + // so clear it here to make "never called" mean what it says. + vi.mocked(fs.readlink).mockClear() + + // "Could not inspect" is not "not a link": falling back to the link path here would let the + // publish replace the link, which is what the resolver exists to prevent. + await expect(resolvePublishTarget(linkPath)).rejects.toThrow("EACCES") + expect(fs.readlink).not.toHaveBeenCalled() + }) + + it("propagates a readlink failure for a dangling symlink", async () => { + const linkPath = "/tmp/test-dir/dangling-link" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.lstat).mockResolvedValue({ isSymbolicLink: () => true } as never) + vi.mocked(fs.readlink).mockRejectedValue(Object.assign(new Error("EIO"), { code: "EIO" })) + + // The link is known to exist but its target cannot be read: publishing onto the link path + // would replace the link with a regular file, so the error has to surface. + await expect(resolvePublishTarget(linkPath)).rejects.toThrow("EIO") + }) +}) + +describe("parent directory creation (safeWriteText.ts:288-290)", () => { + beforeEach(() => { + // This block sits outside describe("safeWriteText"), so the resetAllMocks there + // never runs for it: without its own reset it inherits whatever the previous block + // left in the mocks, and a run with -t that selects only these tests gets bare + // vi.fn() stubs (undefined stats) and fails for unrelated reasons. + vi.resetAllMocks() + 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(fsSync.statSync).mockReturnValue(_stats(0o644)) + vi.mocked(fsSync.lstatSync).mockReturnValue(_dirStats()) + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + vi.mocked(fs.realpath).mockResolvedValue("/tmp/test-dir/target.txt") + vi.mocked(fs.mkdir).mockResolvedValue(undefined) + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fsSync.openSync).mockReturnValue(1) + }) + + it("propagates an mkdir failure before any staging, open or publish", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const err = Object.assign(new Error("EACCES: cannot create"), { code: "EACCES" }) + vi.mocked(fs.mkdir).mockRejectedValue(err) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(err) + }) + + it("propagates an access failure on the parent directory before any staging, open or publish", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const err = Object.assign(new Error("EACCES: cannot verify"), { code: "EACCES" }) + vi.mocked(fs.access).mockRejectedValue(err) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(err) + }) + + it("creates and verifies the parent directory on the successful path", async () => { + const targetPath = "/tmp/test-dir/nested/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fs.mkdir).toHaveBeenCalledWith("/tmp/test-dir/nested", { recursive: true }) + expect(fs.access).toHaveBeenCalledWith("/tmp/test-dir/nested") + expect(fs.rename).toHaveBeenCalled() + }) +}) + +describe("partial backup after a failed copy (backup:true)", () => { + beforeEach(() => { + // Same isolation reason as the block above: outside the main describe, so nothing + // resets the mocks or restores the defaults for it. + vi.resetAllMocks() + 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(fsSync.statSync).mockReturnValue(_stats(0o644)) + vi.mocked(fsSync.lstatSync).mockReturnValue(_dirStats()) + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + typeof args[3] === "number" ? args[3] : 0, + ) + }) + + it("removes the partial backup when copyFile fails with ENOENT and the publish succeeds", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // The target exists for the backup pre-check, then the copy itself fails: a partial file + // may already sit at the .bak path while backupCreated stays false. The publish still + // succeeds, and nothing else in the flow would ever remove that half-written backup. + vi.mocked(fs.access).mockResolvedValue(undefined) + vi.mocked(fs.copyFile).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + + await safeWriteText(targetPath, "data", { platform: "linux", backup: true }) + + expect(fs.rename).toHaveBeenCalled() + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak")) + }) +}) + +describe("staging directory release", () => { + it("retries a transient release failure once and then surfaces it", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + vi.mocked(fs.realpath).mockResolvedValue("/tmp/test-dir/target.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("EBUSY"), { code: "EBUSY" })) + + await safeWriteText("/tmp/test-dir/target.txt", "data", { platform: "linux" }) + + const messages = warn.mock.calls.map(function (c) { + return String(c[0]) + }) + expect( + messages.filter(function (m) { + return m.indexOf("staging directory release failed") >= 0 + }).length, + ).toBe(1) + warn.mockRestore() + }) + + it("treats ENOTEMPTY as benign: no warning at all", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + vi.mocked(fs.realpath).mockResolvedValue("/tmp/test-dir/target.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fs.rmdir).mockRejectedValue(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) + + await safeWriteText("/tmp/test-dir/target.txt", "data", { platform: "linux" }) + + expect(warn).not.toHaveBeenCalled() + warn.mockRestore() + }) +}) + +describe("post-create staging probe failure", () => { + it("removes a staging directory it created when the post-create lstat fails", async () => { + const ioError = Object.assign(new Error("EIO"), { code: "EIO" }) + vi.mocked(fs.realpath).mockResolvedValue("/tmp/test-dir/target.txt") + vi.mocked(fsSync.openSync).mockReturnValue(1) + // Path-aware, not positional: the ancestor walk also calls lstatSync, so a once-chain + // would be consumed by the wrong site. Only the staging directory's own probes are + // scripted here - absent, then an I/O error on the post-create re-check. + let stagingProbes = 0 + vi.mocked(fsSync.lstatSync).mockImplementation(((target: string) => { + if (String(target).includes(".file-safety-staging")) { + stagingProbes++ + if (stagingProbes === 1) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + throw ioError + } + return _dirStats() + }) as never) + + await expect(safeWriteText("/tmp/test-dir/target.txt", "data", { platform: "linux" })).rejects.toBe(ioError) + + expect(stagingProbes).toBe(2) + expect(fsSync.rmdirSync).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..c162a579f2 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,821 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import { execFile } from "child_process" + +/** + * Options for safeWriteText atomic text publish primitive. + */ +export interface SafeWriteTextOptions { + /** + * When true, copy the target to a backup file before the commit rename. The + * commit is a single atomic rename, so the target is never removed first and + * nothing restores from the copy: it is deleted after a confirmed commit, and + * kept on disk when the commit could not be confirmed durable or the saved + * DACL could not be restored, so the previous content and its security + * descriptor stay recoverable. Costs a full read+write of the old file, so + * callers that do not need that recovery copy should leave this off. + */ + 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 + + /** + * 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 +} + +// -- 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") +} + +/** Sharing errors that clear on their own when the target is held open. */ +const _TRANSIENT_RENAME_CODES = ["EPERM", "EACCES", "EBUSY"] +// Bounded so a permanently locked target still fails fast: 5 retries back off to +// 320ms, for a worst case of ~620ms added to a save. +const _RENAME_RETRY_ATTEMPTS = 5 +const _RENAME_RETRY_BASE_MS = 20 + +/** Error raised when the staging path cannot be trusted. */ +class UnsafeStagingDirectoryError extends Error { + constructor(stagingPath: string, reason: string) { + super(`Refusing to stage in ${stagingPath}: ${reason}`) + this.name = "UnsafeStagingDirectoryError" + } +} + +/** + * Raised when the commit rename landed but the parent-directory fsync failed. + * The new content is in place; what is unconfirmed is that the directory entry + * survives a crash, so callers must not treat the publish as durable. + */ +/** + * The target exists but its DACL could not be captured, so publishing would replace its security + * descriptor with inherited permissions and no record would exist to restore it from. Thrown + * before the commit rename; the target is left untouched. + */ +export class DaclCaptureError extends Error { + constructor( + public readonly targetPath: string, + public readonly dumpPath: string, + ) { + super( + `safeWriteText: refusing to publish ${targetPath}: its DACL could not be captured (no icacls dump was written at ${dumpPath}). No change was made to the target.`, + ) + this.name = "DaclCaptureError" + } +} + +export class PublishNotDurableError extends Error { + constructor(targetPath: string, reason: string) { + super(`Published ${targetPath} but could not confirm it is durable: ${reason}`) + this.name = "PublishNotDurableError" + } +} + +/** + * The content is committed but the saved DACL could not be put back on it, so the file at the + * target answers to different access rights than the one it replaced. Reported as an error rather + * than a warning: the caller has to know that the publish changed who can read the file. Raised + * after the commit rename, so the content is at the target and the backup copy is retained. + */ +export class DaclRestoreError extends Error { + constructor( + public readonly targetPath: string, + public readonly causeError: unknown, + reason: string = "its saved access rights could not be restored", + ) { + super(`safeWriteText: content committed at ${targetPath}, but ${reason}`) + this.name = "DaclRestoreError" + } +} + +function _errorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined +} + +/** Create a private per-write staging sub-directory inside *dir*. The name is + * generated here, the way the temp file inside it is named, so concurrent writes + * never collide on their temp names and never release a directory another write + * is still using. */ +function _stagingDir(dir: string): string { + // A shared fixed name is a path the workspace controls: mkdirSync with + // recursive:true accepts an existing entry of that name without creating + // anything, so a committed .file-safety-staging symlink would have the chmod + // below repair the permissions of a directory outside the workspace and put + // the staged file in it. A name this call generated cannot be pre-planted. + const sd = path.join(dir, ".file-safety-staging_" + Date.now() + "_" + Math.random().toString(36).substring(2)) + // The name is this call's, the entry at it still has to be checked: a colliding + // or swapped-in path would stage - and then publish - outside the target + // directory. Anything that is not a real directory owned by this process is + // rejected instead of used. + let preExisted = false + try { + const existing = fsSync.lstatSync(sd) + preExisted = true + if (!existing.isDirectory()) { + throw new UnsafeStagingDirectoryError(sd, "it exists and is not a directory") + } + } catch (error: unknown) { + if (error instanceof UnsafeStagingDirectoryError) throw error + if (_errorCode(error) !== "ENOENT") throw error + } + // A directory this call created but never hands to the caller would sit in the user's + // target directory forever - the caller never receives the path, so nothing else can + // clean it up. A pre-existing staging directory is left alone: it may belong to a + // concurrent write. + function _abandonIfCreated(): void { + if (!preExisted) { + try { + fsSync.rmdirSync(sd) + } catch { + // not empty or already gone: there is nothing else this call can do + } + } + } + // 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 }) + // Re-check after the create: the path can be swapped for a symlink between the + // check above and mkdirSync. + let created: fsSync.Stats + try { + created = fsSync.lstatSync(sd) + } catch (error: unknown) { + // The directory exists at this point and this call may have just created it. Letting the + // probe error propagate would strand it: the caller never receives the path, so nothing + // else can remove it. Abandon first, then rethrow the original error. + _abandonIfCreated() + throw error + } + if (!created.isDirectory()) { + _abandonIfCreated() + throw new UnsafeStagingDirectoryError(sd, "it was replaced by a non-directory") + } + if (typeof process.getuid === "function" && created.uid !== process.getuid()) { + _abandonIfCreated() + throw new UnsafeStagingDirectoryError(sd, "it is not owned by this process") + } + try { + fsSync.chmodSync(sd, 0o700) + } catch { + // best-effort: chmod denied or unavailable; a fresh dir was still + // created with the requested mode + } + return sd +} + +/** + * fsync a file descriptor so its data is durable before the atomic rename. + * Uses the sync form because this repo's @types/node does not declare + * fs.promises.fsync; the staging file is small, so the blocking window is bounded. + */ +function _fsyncFile(fd: number): void { + fsSync.fsyncSync(fd) +} + +/** + * Keep the existing target's group on the staged file before it is published. + * + * The commit is a rename, so the published inode is a new one: it takes the process's + * primary group (or the directory's gid when the directory is setgid). A shared 0o664 + * file owned by group `devs` would therefore lose group write access for every member + * whose primary group is not `devs`, even though the fchmod below restores the mode - the + * mode fix exists precisely to stop that regression, so the group has to survive too. + * + * Best-effort: fchown only succeeds when this process belongs to that group, which is the + * case that matters; anywhere else the mode fix still stands. It runs BEFORE fchmod + * because chown can clear the setgid bits that chmod has just set. Windows carries no + * POSIX gid to preserve - its identity is the DACL, handled by the save/restore steps. + */ +function _applyTargetOwnership(fd: number, gid: number | undefined, platform: string): void { + if (platform === "win32" || typeof gid !== "number") { + return + } + try { + fsSync.fchownSync(fd, -1, gid) + } catch { + // Not a member of the target's group: keep the mode preservation and move on. + } +} + +/** Save the DACL of *srcPath* to a dump file on Windows. + * Returns true when a usable dump exists; false otherwise. + * Never throws - a false return means no usable dump was produced, which the caller treats + * as "the security descriptor cannot be preserved" and fails closed before publishing. */ +async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + // No /T: with a file path, /T makes icacls walk the whole directory tree and save every + // file that shares the name (a save of \package.json also walks node_modules), + // and the matching /restore then rewrites the DACL of every one of those files rather + // than only the target's. An inaccessible subdirectory can also make /save exit + // non-zero, leaving the fail-closed check to depend on a dump that may not contain the + // target's own entry. + runner("icacls", [srcPath, "/save", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return _dumpIsUsable(dumpPath) + } catch { + // icacls can exit non-zero while still writing a usable dump (measured on a normal host), so + // judge the capture by the artifact, not only by the exit code: a non-empty dump is a capture. + try { + return _dumpIsUsable(dumpPath) + } catch { + return false + } + } +} + +// A DACL dump is only usable if it is actually there, is a regular file, and has bytes in it. +// Both the success and the failure callback have to be judged by this: icacls can exit 0 without +// writing anything (measured on a host where the target's owner differs from the caller), and a +// zero-byte dump restores nothing - so trusting the exit code alone would let the commit rename +// destroy a security descriptor that was never captured. +function _dumpIsUsable(dumpPath: string): boolean { + try { + const st = fsSync.statSync(dumpPath) + return st.isFile() && st.size > 0 + } catch { + return false + } +} + +/** Restore a DACL dump onto *dirPath* on Windows. + * `restored` reports whether icacls succeeded; the caller decides whether a lost DACL is + * tolerable. `privilegeUnavailable` separates the one failure that will never fix itself + * from a transient one: on a non-elevated host /restore always exits 1300 (measured), so + * retrying it on every later write costs three process spawns per write for a DACL that + * cannot be restored. A transient failure must not disable preservation for the process. */ +async function _restoreDaclWindows( + dirPath: string, + dumpPath: string, + execFileRunner?: typeof execFile, +): Promise<{ restored: boolean; privilegeUnavailable: boolean }> { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [dirPath, "/restore", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return { restored: true, privilegeUnavailable: false } + } catch (error: unknown) { + // execFile reports a non-zero exit as an error carrying that exit code. + const code = + error && typeof error === "object" && "code" in error ? (error as { code?: unknown }).code : undefined + return { restored: false, privilegeUnavailable: code === _ICACLS_PRIVILEGE_EXIT_CODE } + } +} + +// Set once a restore has failed with the missing-privilege exit code: from then on the +// process skips the save/restore pair instead of paying for it on every write and warning +// about the same host limitation each time. Skipping the pair is a cost decision only: it +// must not change what a publish reports. A later write that replaces an existing target +// still loses that target's descriptor, so step 2b records it and the call reports the +// condition instead of resolving quietly. The reason is logged once, when the memo is set. +let daclRestorePrivilegeUnavailable = false + +// icacls exit code for "a required privilege is not held by the client". +const _ICACLS_PRIVILEGE_EXIT_CODE = 1300 + +// -- public API ------------------------------------------------------------ + +/** + * Atomic text publish primitive. + * + * 1. Write content to a temp file in a private per-write staging subdir + * (same volume -> atomic rename guaranteed). + * 2. fsync the temp file, then close it. + * 3. win32 only: if target exists save its DACL dump BEFORE backup rename. + * 4. Optionally rename target -> backup (when backup:true). + * 5. Atomic rename temp -> target. + * 6. win32 only: restore DACL onto the directory AFTER commit rename. + * 7. On success: delete backup (if any) and unlink DACL dump. + * 8. On failure: rollback backup to target path; clean up temp + dump. + */ + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. A dangling symlink is followed to + * its referent too (renaming onto the link path would replace the link), and only + * a path that neither exists nor is a link falls back to the given path. Any other + * resolution error (EACCES, EIO, ELOOP, ...) 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, + visitedLinks: Set = new Set(), +): Promise { + try { + return await fs.realpath(absoluteFilePath) + } catch (error: unknown) { + if (_errorCode(error) !== "ENOENT") throw error + // realpath reports ENOENT for a dangling symlink as well as for a missing path. + // Renaming onto the link path would replace the link with a regular file, so follow + // the link text to its referent instead; a link cycle surfaces as ELOOP and still + // propagates, so the recursion is bounded. + // Only ENOENT means "there is nothing here that could be a link". Any other lstat error + // (EACCES, EIO, ...) must propagate: treating "could not inspect" as "not a link" would let + // a write land on the link path and replace the link, which is exactly what this resolver + // exists to prevent - and the doc comment above already promises that it propagates. + let link: Awaited> | null = null + try { + link = await fs.lstat(absoluteFilePath) + } catch (statError: unknown) { + if (_errorCode(statError) !== "ENOENT") { + throw statError + } + } + if (link?.isSymbolicLink()) { + const linkText = await fs.readlink(absoluteFilePath) + if (visitedLinks.has(absoluteFilePath)) { + const loopError = new Error(`Symlink loop while resolving publish target ${absoluteFilePath}`) + ;(loopError as NodeJS.ErrnoException).code = "ELOOP" + throw loopError + } + visitedLinks.add(absoluteFilePath) + return resolvePublishTarget(path.resolve(path.dirname(absoluteFilePath), linkText), visitedLinks) + } + return absoluteFilePath + } +} + +export async function safeWriteText(filePath: string, content: string, 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. + const stagingDir = options?.tempPath ? null : _stagingDir(dirPath) + const tempPath = options?.tempPath ?? _tempName(stagingDir ?? dirPath, "safeWriteText") + + // The staging sub-directory only exists to hold this write's temp file, so it is + // removed once the write is over. rmdir fails with ENOTEMPTY while another write + // still stages here, which is exactly the concurrency guard wanted. + async function _releaseStagingDir(): Promise { + if (stagingDir === null) return + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.rmdir(stagingDir) + return + } catch (error: unknown) { + const code = _errorCode(error) + // ENOENT: another writer already removed it. ENOTEMPTY: a concurrent write is still + // staging here, which is the concurrency guard this directory exists for. Both are + // benign - no retry, no warning. + if (code === "ENOENT" || code === "ENOTEMPTY") return + // Anything else (EBUSY, EPERM, EACCES, ...) is transient here: retry once, then + // surface the retained directory. A leftover staging directory is acceptable, an + // invisible one is not. + if (attempt === 1) { + console.warn( + `safeWriteText: staging directory release failed: ${stagingDir} (${code ?? String(error)})`, + ) + } + } + } + } + + let backupPath: string | null = null + let backupCreated = false + // Set as soon as the backup destination is chosen, before the copy runs: a copyFile that + // fails part way can still leave a partial file at that path, and it has to be cleaned up + // like a completed backup would be. + let backupAttempted = false + // Set when the commit landed but its durability could not be confirmed. The + // content is in place, so there is nothing to roll back, but the caller must + // not be told the publish is durable. + let durabilityError: unknown = null + // Set when the commit landed but the saved DACL could not be put back on the published file. + // The content is in place, so there is nothing to roll back, but the caller must not observe a + // success whose access rights changed, and the backup is the only artifact still carrying the + // original security descriptor, so it has to survive for recovery. + let daclRestoreError: DaclRestoreError | null = null + let daclDumpPath: string | null = null // tracked for cleanup in finally + let daclSaved = false // restore step runs only when the save succeeded + // Set when restoration is disabled for this process and this write replaces an existing + // target. The commit is a rename, and a rename does not hand the target the descriptor of + // the file it replaced, so the published file answers to inherited access rights that + // nothing saved and nothing can put back. Reported after the commit like a failed restore: + // the content is committed, so this is not a failure to roll back. + let daclPreservationSkipped = false + + const platform = options?.platform ?? process.platform + + 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). + let targetMode = 0o644 // default for a fresh target + let targetGid: number | undefined + let targetExists = false + try { + const targetStats = fsSync.statSync(targetPath) + targetMode = targetStats.mode & 0o777 + targetGid = targetStats.gid + targetExists = true + } catch (error: unknown) { + // Only a genuinely absent target may take the default mode. Any other + // stat failure leaves the real mode unknown, and publishing with the + // default could widen a restrictive file, so the write does not proceed. + if (_errorCode(error) !== "ENOENT") throw error + } + let fd: number + try { + fd = fsSync.openSync(tempPath, "w", targetMode) + } catch (error: unknown) { + // Every write to one directory shares the staging dir, and a write that finishes + // removes it when empty. A concurrent writer can therefore remove it between + // _stagingDir() and this open. Re-create it and try once; any other errno, or + // a second ENOENT, is a real failure. + if (_errorCode(error) !== "ENOENT" || stagingDir === null) throw error + _stagingDir(dirPath) + fd = fsSync.openSync(tempPath, "w", targetMode) + } + // The creation mode handed to openSync is masked by the process umask: an existing + // 0o664 target would be published as 0o644 and a shared repository would lose group + // write access on every save. Apply the exact target mode on the descriptor, which is + // also what the caller-staged branch below does. + try { + if (targetExists) { + _applyTargetOwnership(fd, targetGid, platform) + 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. + const buffer = Buffer.from(content, "utf8") + 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 + let targetGid: number | undefined + try { + const targetStats = fsSync.statSync(targetPath) + targetMode = targetStats.mode & 0o777 + targetGid = targetStats.gid + } catch (error: unknown) { + // As above: an unknown target mode must not be replaced by the temp's + // own creation mode, which can be wider than the target's. + if (_errorCode(error) !== "ENOENT") throw error + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + _applyTargetOwnership(fd, targetGid, platform) + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + // Once a restore has proven this host cannot restore at all, saving a dump it cannot + // apply is three process spawns per write for nothing, so the pair is skipped. The + // skip is decided here, not in step 5, because the save is the expensive half. + if (platform === "win32" && !daclRestorePrivilegeUnavailable) { + try { + await fs.access(targetPath) // target exists? + // Unique per call: two concurrent writes to the same target must not share one dump, + // where one call can unlink or overwrite the file the other is still using. Kept in the + // target directory, not the staging dir, so a write that never stages a temp file still + // leaves no extra directory behind. + daclDumpPath = _tempName(dirPath, "safeWriteText.acl.tmp") + const saved = await _saveDaclWindows(targetPath, daclDumpPath, options?.execFileRunner) + if (!saved) { + // Fail closed. Publishing without a captured DACL replaces the target's security + // descriptor with whatever the parent directory inherits, and no later step can put the + // original back. icacls /save does not need the backup/restore privileges that /restore + // does (measured: /restore exits 1300 un-elevated), so refusing here does not break + // ordinary hosts - it removes the exposure instead of accepting it. The staging file, + // any partial backup and the dump are cleaned by the rollback below. + daclSaved = false + // This throw leaves the try/finally that owns cleanup, so release the artifacts here: + // the staged file and the (unusable) dump must not be orphaned. Removal is retried once + // and a retained path is reported rather than suppressed. + const abortArtifacts: Array = [ + daclDumpPath, + options?.tempPath ? null : tempPath, // a caller-supplied temp belongs to the caller + ] + for (const artifact of abortArtifacts) { + if (!artifact) { + continue + } + let removed = false + for (let attempt = 0; attempt < 2 && !removed; attempt++) { + try { + await fs.unlink(artifact) + removed = true + } catch { + // retry once, then report + } + } + if (!removed) { + console.warn( + `safeWriteText: aborted before publishing and could not remove ${artifact}; it is retained and must be cleaned up out of band.`, + ) + } + } + throw new DaclCaptureError(targetPath, daclDumpPath) + } else { + daclSaved = true + } + } catch (err: unknown) { + if (err instanceof DaclCaptureError) { + // Not an access probe failure: the target exists and its DACL could not be captured. + throw err + } + + // Only ENOENT means "there is no target, so there is no DACL to preserve". EACCES or any + // other probe error says nothing about the target's security descriptor, and publishing + // over a target we could not inspect would replace it without a captured DACL. + const probeCode = + err && typeof err === "object" && "code" in err ? (err as { code?: string }).code : undefined + if (probeCode !== "ENOENT") { + throw err + } + daclDumpPath = null + } + } else if (platform === "win32") { + // -- Step 2b (win32): restoration is disabled for this process ---------------- + // The memo skips the save/restore pair to save the process spawns, but the commit is + // still a rename over whatever sits at the target, and a rename does not hand the + // published file the descriptor of the file it replaced. A brand-new file takes a + // default descriptor and loses nothing, so only an existing target is a real loss - + // and that loss has to reach the caller instead of resolving quietly. One probe, no + // process spawn: the memo keeps its cost saving. + try { + await fs.access(targetPath) + daclPreservationSkipped = true + } catch (probeError: unknown) { + // Same rule as the capture probe: only ENOENT proves there is no descriptor to + // preserve. An unreadable target is not evidence that a plain success is safe. + if (_errorCode(probeError) !== "ENOENT") throw probeError + } + } + + try { + // -- Step 3 (backup:true): copy the target to a backup ---------------- + // The target must stay at its canonical path until the single atomic rename + // below publishes the staged file. Renaming the target away first would leave + // the path missing if that publish rename failed, so the backup is a copy and + // there is nothing to roll back. + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + backupAttempted = true + await fs.copyFile(targetPath, backupPath) + backupCreated = true + } catch (err: unknown) { + const code = + typeof err === "object" && err !== null && "code" in err + ? (err as { code?: string }).code + : undefined + if (code !== "ENOENT") throw err + } + } + // -- Step 4: atomic rename temp -> target --------------------- + // On Windows the rename can be rejected while another process holds the + // target without FILE_SHARE_DELETE. The previous fs.writeFile path tolerated + // that, so retry a bounded number of times before failing. + for (let attempt = 0; ; attempt++) { + try { + await fs.rename(tempPath, targetPath) + break + } catch (error: unknown) { + const transient = + platform === "win32" && + attempt < _RENAME_RETRY_ATTEMPTS && + _TRANSIENT_RENAME_CODES.includes(_errorCode(error) ?? "") + if (!transient) throw error + await new Promise((resolve) => setTimeout(resolve, _RENAME_RETRY_BASE_MS * 2 ** attempt)) + } + } + + // -- 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 rename committed, so the content is in place and there is nothing to + // roll back, but the directory entry is not confirmed durable. Record it so + // the call fails and the backup is kept for recovery. + durabilityError = error + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + if (platform === "win32" && daclSaved && daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + let attempt = await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + if (!attempt.restored && !attempt.privilegeUnavailable) { + // One retry, and only for a failure that could be transient: icacls can fail + // while another process still holds the just-renamed file open. A host without + // the privilege fails identically every time, so retrying it is pure cost. + attempt = await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + if (!attempt.restored) { + if (attempt.privilegeUnavailable && !daclRestorePrivilegeUnavailable) { + daclRestorePrivilegeUnavailable = true + console.warn( + "safeWriteText: this host cannot restore DACLs (icacls /restore reports a missing privilege), so later writes in this process will skip saving and restoring them.", + ) + } + // The content is committed, but the published file answers to a different DACL + // than the one that was saved, and nothing here verified an equivalent + // restrictive ACL on the replacement. Reported as an error rather than a + // warning: a publish that changed who can read the file must not look like an + // ordinary successful save. Thrown after the rollback handler below - the + // content is committed, so this is not a failure to roll back, and the backup + // has to survive for recovery. + daclRestoreError = new DaclRestoreError(targetPath, null) + } + } + + // -- Step 5b (win32): restoration disabled for this process ------------------ + // Nothing was captured, so nothing could be restored: the published file answers to + // the descriptor it was created with, not the one the file it replaced had. Reported + // as the same post-commit error as a failed restore, because from the caller's side + // it is the same fact - the content is committed and the access rights changed - and + // it keeps the promise that no caller can observe success for a publish whose DACL + // was not preserved. It also has to run before step 6: with no dump in play, the + // retained backup copy is the only artifact still carrying the original descriptor + // (CopyFileW hands the destination the source's security attributes). + if (daclPreservationSkipped && daclRestoreError === null) { + daclRestoreError = new DaclRestoreError( + targetPath, + null, + "DACL restoration is disabled on this host, so the access rights of the file it replaced were not preserved", + ) + } + + // -- Step 6 (backup:true): delete backup only on a fully successful publish -- + // Kept when durability is unconfirmed (the rename may be lost on power loss) AND when + // the DACL restore failed: that backup is the only artifact still carrying the target's + // original security descriptor, so deleting it would destroy the recovery path. + // A copy that failed part way leaves a partial file at backupPath with backupCreated + // never set, so the condition has to cover backupAttempted too: after a successful + // publish nothing else would ever remove that half-written backup. + if ( + (backupCreated || backupAttempted) && + backupPath && + durabilityError === null && + daclRestoreError === null + ) { + try { + await fs.unlink(backupPath) + } catch { + // One bounded retry: the backup can still be held open by another process right + // after the commit rename. If that fails too the path is surfaced instead of + // silently abandoned - an orphaned backup is acceptable, an invisible one is not. + try { + await fs.unlink(backupPath) + } catch (cleanupError: unknown) { + console.warn( + `safeWriteText: the write to ${targetPath} committed but its backup copy could not be removed at ${backupPath} (${String(cleanupError)}); the copy is left in place.`, + ) + } + } + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + try { + await fs.unlink(daclDumpPath) + } catch { + // One bounded retry, then surface the path: an icacls dump left next to the + // settings file is a readable copy of its ACL, so an invisible orphan is worse + // than a visible one. + try { + await fs.unlink(daclDumpPath) + } catch (cleanupError: unknown) { + console.warn( + `safeWriteText: the DACL dump at ${daclDumpPath} could not be removed (${String(cleanupError)}); it is left in place.`, + ) + } + } + } + } + + // tempPath is now the committed file; no cleanup needed. + } catch (originalError: unknown) { + // -- Rollback / cleanup on failure ---------------------------------- + if ((backupCreated || backupAttempted) && backupPath) { + // Covers a copyFile that failed after creating the destination: the partial file + // would otherwise be orphaned next to the target. + // The target was never moved, so rollback is just removing the backup copy. + try { + await fs.unlink(backupPath) + } catch (cleanupError: unknown) { + // Cleanup failure is non-fatal, but an invisible orphan is worse than a visible one: + // the original error is still the one that gets thrown. + console.warn( + `safeWriteText: the write to ${targetPath} failed and its backup copy at ${backupPath} could not be removed (${String(cleanupError)}).`, + ) + } + } + + // Always clean up the staging temp file on failure. A single swallowed unlink is not enough: + // a transient EBUSY/EPERM would silently leave the temp inside the staging directory, and nothing + // would ever report the retained path. Bounded retry, same shape as the DACL and backup cleanup. + for (let attempt = 0; attempt < 2; attempt++) { + try { + await fs.unlink(tempPath) + break + } catch (cleanupError: unknown) { + const code = _errorCode(cleanupError) + if (code === "ENOENT") { + break + } + if (attempt === 1) { + console.warn( + `safeWriteText: staging temp release failed: ${tempPath} (${code ?? String(cleanupError)})`, + ) + } + } + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + await _releaseStagingDir() + + throw originalError + } + + await _releaseStagingDir() + + // Raised outside the rollback handler on purpose: the content is committed, so + // this is not a failure to roll back, and the backup must survive for recovery. + if (durabilityError !== null) { + const reason = durabilityError instanceof Error ? durabilityError.message : String(durabilityError) + throw new PublishNotDurableError(targetPath, reason) + } + + // Raised outside the rollback handler for the same reason as PublishNotDurableError: the + // content is committed, so this is not a failure to roll back, and the backup retained above + // is the only artifact still carrying the target's original security descriptor. A restore + // failure therefore reaches the caller as its own error: no caller can observe success for a + // publish whose DACL was not restored. + if (daclRestoreError !== null) { + throw daclRestoreError + } +} diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..1b024205d6 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -5,6 +5,14 @@ import * as os from "os" import { safeWriteJson } from "../safeWriteJson" +// Pass-through spy over the real publish primitive: every test keeps the real +// behaviour, and one test can hand safeWriteJson the post-commit DaclRestoreError +// without a real Windows DACL failure. +vi.mock("../../services/file-safety/safeWriteText", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, safeWriteText: vi.fn(actual.safeWriteText) } +}) + // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when // test mockImplementation callbacks delegate to the real implementation. @@ -29,6 +37,7 @@ vi.mock("fs/promises", async () => { mockedFs.readFile = vi.fn(actual.readFile) as any mockedFs.rename = vi.fn(actual.rename) as any mockedFs.unlink = vi.fn(actual.unlink) as any + mockedFs.copyFile = vi.fn(actual.copyFile) mockedFs.access = vi.fn(actual.access) as any mockedFs.mkdtemp = vi.fn(actual.mkdtemp) as any mockedFs.rm = vi.fn(actual.rm) as any @@ -45,6 +54,8 @@ vi.mock("fs", async () => { return { ...actualFs, // Spread actual implementations createWriteStream: vi.fn(actualFs.createWriteStream) as any, // Default to actual, but mockable + // Wrapped so a test can fail the parent-directory fsync that follows the commit rename. + fsyncSync: vi.fn(actualFs.fsyncSync), } }) @@ -67,6 +78,9 @@ describe("safeWriteJson", () => { let currentTestFilePath: string beforeEach(async () => { + // Reset implementations between tests: a mockImplementation set by one test keeps its + // closure counter into the next test, so a call-count based mock leaks across tests. + vi.resetAllMocks() // Create a temporary directory for each test tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safeWriteJson-test-")) @@ -102,6 +116,219 @@ describe("safeWriteJson", () => { } } + // Durability of the commit (Persistence Integrity) + test.skipIf(process.platform === "win32")( + "keeps the published target when the commit landed but the directory fsync failed", + async () => { + const target = path.join(tempDir, "durable.json") + // The first fsync is the staged file, the second is the parent directory AFTER the + // commit rename. Failing only the second one is the PublishNotDurableError case: + // the bytes are in place, only their durability is unconfirmed. + let fsyncCalls = 0 + vi.mocked(fsSyncActual.fsyncSync).mockImplementation(() => { + fsyncCalls++ + if (fsyncCalls === 2) { + throw Object.assign(new Error("EIO: i/o error"), { code: "EIO" }) + } + }) + + const payload = { committed: "value" } + await expect(safeWriteJson(target, payload)).rejects.toThrow(/could not confirm it is durable/) + expect(fsyncCalls).toBe(2) + + // The staged file was renamed onto the target, so the catch path must not treat that + // path as a leftover temp file: the new content has to survive the rejection. + expect(await readFileContent(target)).toEqual(payload) + }, + ) + + // Security Boundaries: a landed commit whose DACL was lost + test("does not treat the staged path as a leftover when the commit landed but the DACL restore failed", async () => { + const { safeWriteText, DaclRestoreError } = await import("../../services/file-safety/safeWriteText") + const target = path.join(tempDir, "restore-fail.json") + + // The commit rename landed; only putting the saved DACL back failed. The consumer + // must classify this as a landed commit - not a failed publish whose staged file + // is a leftover to remove - and it must still rethrow, so no caller observes + // success for a publish whose DACL was not restored. + // The mock has to reproduce the state a real DaclRestoreError arrives with: the + // commit rename already landed, and only the DACL restore afterwards failed. Throwing + // without the rename would let the no-unlink assertion pass on a write that never + // committed, which is a different situation with a different correct answer. + vi.mocked(safeWriteText).mockImplementationOnce(async (_target, _content, textOptions) => { + // fs.rename is a pass-through spy over the real rename, so this is the real commit. + await fs.rename(String(textOptions?.tempPath), target) + throw new DaclRestoreError(target, null) + }) + + await expect(safeWriteJson(target, { committed: "value" })).rejects.toBeInstanceOf(DaclRestoreError) + + const streamCall = vi + .mocked(fsSyncActual.createWriteStream) + .mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(streamCall).toBeDefined() + const stagedPath = String(streamCall![0]) + + // The commit really landed: the new bytes are at the target and the staged name is + // gone, so this is the post-commit state the catch path has to classify. + expect(await readFileContent(target)).toEqual({ committed: "value" }) + expect(await fileExists(stagedPath)).toBe(false) + + // The staged path was consumed by the commit rename, so the catch path must not + // unlink it as a leftover temp file. + const stagedUnlinks = vi.mocked(fs.unlink).mock.calls.filter((call) => String(call[0]) === stagedPath) + expect(stagedUnlinks).toHaveLength(0) + }) + + // Staging permissions + test.skipIf(process.platform === "win32")( + "omits an explicit mode for an absent target so the file takes the umask default", + async () => { + const target = path.join(tempDir, "absent-target.json") + await safeWriteJson(target, { fresh: 1 }) + + // Mirroring a mode only makes sense when there IS a target to mirror. Inheriting a + // mode here would either pin 0o600 for a brand new file or, worse, mask the umask. + const streamCall = vi.mocked(fsSyncActual.createWriteStream).mock.calls.at(-1) + expect((streamCall?.[1] as { mode?: number } | undefined)?.mode).toBeUndefined() + + const mode = (await fs.stat(target)).mode & 0o777 + expect(mode).toBe(0o666 & ~process.umask()) + }, + ) + + test.skipIf(process.platform === "win32")( + "stages the temp file with the existing target's mode instead of the process default", + async () => { + const target = path.join(tempDir, "private.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 }), { mode: 0o600 }) + await fs.chmod(target, 0o600) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + + await safeWriteJson(target, { updated: 2 }) + + const staged = streamCalls.mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(staged).toBeDefined() + // createWriteStream defaults to 0o666 (& ~umask = 0o644). The staged file holds + // the whole payload until the commit rename, so beside a 0o600 target it would + // be readable by other local users for the duration of the write. + expect(Number((staged![1] as { mode?: number } | undefined)?.mode)).toBe(0o600) + }, + ) + + test.skipIf(process.platform === "win32")( + "stages with the target's own mode when it is the ordinary 0o644", + async () => { + const target = path.join(tempDir, "public.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 }), { mode: 0o644 }) + await fs.chmod(target, 0o644) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + + await safeWriteJson(target, { updated: 2 }) + + const staged = streamCalls.mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(staged).toBeDefined() + // No widening and no narrowing: the staged file mirrors the target it replaces. + expect(Number((staged![1] as { mode?: number } | undefined)?.mode)).toBe(0o644) + }, + ) + + test.skipIf(process.platform === "win32")( + "keeps the staged file owner-writable when the target is read-only", + async () => { + const target = path.join(tempDir, "readonly.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 })) + await fs.chmod(target, 0o400) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + + // Mirroring the target mode verbatim would stage a 0o400 file, and safeWriteText + // reopens the staged file with "r+" (before applying the target mode with + // fchmod), so the write would fail with EACCES for an ordinary user. + await safeWriteJson(target, { updated: 2 }) + + const staged = streamCalls.mock.calls.find((call) => String(call[0]).includes(".new_")) + expect(staged).toBeDefined() + expect(Number((staged![1] as { mode?: number } | undefined)?.mode)).toBe(0o600) + + const written = JSON.parse(await fs.readFile(target, "utf8")) + expect(written).toEqual({ updated: 2 }) + await fs.chmod(target, 0o600) + }, + ) + + test("surfaces a stat failure other than ENOENT instead of staging with the default mode", async () => { + const target = path.join(tempDir, "stat-fails.json") + await fs.writeFile(target, JSON.stringify({ initial: 1 })) + + const streamCalls = vi.mocked(fsSyncActual.createWriteStream) + streamCalls.mockClear() + const statSpy = vi.spyOn(fsSyncActual, "statSync").mockImplementation(() => { + throw Object.assign(new Error("EIO"), { code: "EIO" }) + }) + + try { + await expect(safeWriteJson(target, { updated: 2 })).rejects.toThrow(/EIO/) + // The stat failure has to surface BEFORE anything is staged: a staged file + // created with the wide default mode would sit beside a restrictive target + // for the duration of the write. Asserting no stream call (not just no + // leftover) is what pins the order - safeWriteText would also reject this + // EIO later, which alone would pass without any staging-mode check. + expect(streamCalls).not.toHaveBeenCalled() + expect((await fs.readdir(tempDir)).filter((entry) => entry.includes(".new_"))).toEqual([]) + } finally { + statSpy.mockRestore() + } + }) + + test.skipIf(process.platform === "win32")( + "serializes a writer that reaches the file through a symlink with one that uses the referent", + async () => { + const referent = path.join(tempDir, "state.json") + await fsSyncActual.promises.writeFile(referent, JSON.stringify({ a: 1 }), "utf8") + const alias = path.join(tempDir, "alias.json") + await fs.symlink(referent, alias) + + const merge = (existing: unknown, incoming: unknown) => ({ + ...((existing ?? {}) as Record), + ...((incoming ?? {}) as Record), + }) + + // Hold the first writer's commit rename until the second writer has taken its + // lock and read the target. Keyed on the caller's alias the two writers take + // different lock files (.alias.json.lock vs state.json.lock), so the second + // read-modify-write starts from the pre-write state and its update is lost. + let proceed: () => void = () => {} + const proceedGate = new Promise((resolve) => { + proceed = resolve + }) + let reachedRename: () => void = () => {} + const reachedFirstRename = new Promise((resolve) => { + reachedRename = resolve + }) + vi.mocked(fs.rename).mockImplementationOnce(async (oldPath, newPath) => { + reachedRename() + await proceedGate + return fsPromisesActuals.rename!(oldPath, newPath) + }) + + const throughAlias = safeWriteJson(alias, { b: 2 }, { merge }) + await reachedFirstRename + const throughReferent = safeWriteJson(referent, { c: 3 }, { merge }) + // Let the second writer reach its lock attempt / read before the first commits. + await new Promise((resolve) => setTimeout(resolve, 150)) + proceed() + await Promise.all([throughAlias, throughReferent]) + + expect(JSON.parse(await fsSyncActual.promises.readFile(referent, "utf8"))).toEqual({ a: 1, b: 2, c: 3 }) + }, + ) + // Success Scenarios // Note: Since we pre-create the file in beforeEach, this test will overwrite it. // If "creation from non-existence" is critical and locking prevents it, safeWriteJson or locking strategy needs review. @@ -158,55 +385,37 @@ describe("safeWriteJson", () => { expect(content).toEqual({ initial: "content" }) }) - test("should handle failure when renaming filePath to tempBackupFilePath (filePath exists)", async () => { - const initialData = { message: "Initial content, should remain" } - const newData = { message: "New content, should not be written" } + test("does not copy the target before the commit: the rename is already atomic", async () => { + const initialData = { message: "Initial content" } + const newData = { message: "New content" } - // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn - vi.mocked(fs.rename).mockImplementationOnce(async () => { - throw new Error("Rename to backup failed") - }) + await safeWriteJson(currentTestFilePath, newData) - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename to backup failed") + // A backup copy would be a full extra read+write of the old file on every + // persistence step, and nothing restores from it: the commit is one rename, + // so the target is intact until it lands. + expect(fs.copyFile).not.toHaveBeenCalled() - // Verify the original file still exists with initial content const content = await readFileContent(currentTestFilePath) - expect(content).toEqual(initialData) + expect(content).toEqual(newData) }) - test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)", async () => { + test("a failed publish leaves the target in place because the backup is a copy", async () => { const initialData = { message: "Initial content, should be restored" } const newData = { message: "New content" } // Overwrite the pre-created file with specific initial data await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - // Track rename calls - let renameCallCount = 0 - - // fs.rename is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn - vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { - renameCallCount++ - if (renameCallCount === 1) { - // First call: filePath -> tempBackupFilePath (should succeed) - return fsPromisesActuals.rename!(oldPath, newPath) - } else if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) - throw new Error("Rename from temp to final failed") - } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, should succeed) - return fsPromisesActuals.rename!(oldPath, newPath) - } - // Default: use original implementation - return fsPromisesActuals.rename!(oldPath, newPath) + // The backup is a copy, so the only rename is the publish. + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Rename from temp to final failed") }) - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Rename from temp to final failed") - // Verify the file was restored to initial content + // The target was never moved, so it still holds the initial content. const content = await readFileContent(currentTestFilePath) expect(content).toEqual(initialData) }) @@ -312,30 +521,9 @@ describe("safeWriteJson", () => { expect(content).toEqual(newData) }) - // Test for console error suppression during backup deletion - test("should suppress console.error when backup deletion fails", async () => { - const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error - const initialData = { message: "Initial" } - const newData = { message: "New" } - - await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) - - // fs.unlink is already vi.fn() — use vi.mocked to avoid double-wrapping via vi.spyOn - vi.mocked(fs.unlink).mockImplementation(async (filePath: any) => { - if (filePath.toString().includes(".bak_")) { - throw new Error("Backup deletion failed") - } - return fsPromisesActuals.unlink!(filePath) - }) - - await safeWriteJson(currentTestFilePath, newData) - - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) - - consoleErrorSpy.mockRestore() - vi.mocked(fs.unlink).mockRestore() - }) + // Removed with the backup option: safeWriteJson no longer takes a backup copy, + // so there is no orphaned backup to tolerate here. The equivalent coverage of + // best-effort backup cleanup lives in safeWriteText.spec.ts. // The expected error message might need to change if the mock behaves differently. test("should handle failure when renaming tempNewFilePath to filePath (filePath initially exists)", async () => { @@ -349,8 +537,8 @@ describe("safeWriteJson", () => { let renameCallCount = 0 vi.mocked(fs.rename).mockImplementation(async (oldPath, newPath) => { renameCallCount++ - if (renameCallCount === 2) { - // Second call: tempNewFilePath -> filePath (should fail) + if (renameCallCount === 1) { + // The only rename is the publish (temp -> filePath). throw new Error("Rename failed") } // For all other calls, use the original implementation @@ -434,9 +622,10 @@ describe("safeWriteJson", () => { expect(vi.mocked(fs.access)).toHaveBeenCalled() }) - // Test for rollback failure scenario - test("should log error and re-throw original if rollback fails", async () => { - const initialData = { message: "Initial, should be lost if rollback fails" } + // The publish is a single rename: a failed commit must leave the target intact + // and must not leak the staged file. + test("propagates the publish failure and removes the staged .new_ file", async () => { + const initialData = { message: "Initial, must survive a failed publish" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) @@ -444,27 +633,21 @@ describe("safeWriteJson", () => { 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 === 2) { - // Second call: tempNewFilePath -> filePath (fail) - throw new Error("Primary rename failed") - } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) - throw new Error("Rollback rename failed") - } - return fsPromisesActuals.rename!(oldPath, newPath) + // The only rename is the publish, so failing it is the failed-publish case. + vi.mocked(fs.rename).mockImplementationOnce(async () => { + throw new Error("Primary rename failed") }) - // Should throw the original error, not the rollback error + // The original error must propagate, not the cleanup error await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Primary rename failed") - // Verify console.error was called for the rollback failure - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("Failed to restore backup"), - expect.objectContaining({ message: "Rollback rename failed" }), - ) + expect(await fileExists(currentTestFilePath)).toBe(true) + + // This is the caller-supplied staged path, so it is the file that leaks when the + // caller's cleanup misses it; safeWriteText's own failure test only covers the + // temp path it generates itself. + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes(".new_"))).toBe(false) consoleErrorSpy.mockRestore() }) @@ -542,4 +725,53 @@ describe("safeWriteJson", () => { const content = await readFileContent(currentTestFilePath) expect(content).toEqual({ c: 3 }) }) + + // 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 is + // acquired on the caller path (realpath:false, which may be absent) while + // the backup + commit happen on the referent. + await fsPromisesActuals.writeFile!(referentPath, JSON.stringify({ seed: true })) + + vi.spyOn(fs, "realpath").mockResolvedValue(referentPath) + + 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 }) + }) + + // 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.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 }) + }, + ) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 7da68b2a7a..2c978824ba 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,14 @@ import * as fsSync from "fs" import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" +import { + resolvePublishTarget, + safeWriteText, + DaclRestoreError, + PublishNotDurableError, + type SafeWriteTextOptions, +} from "../services/file-safety/safeWriteText" + import { acquireFileLock } from "./fileLock" /** @@ -32,7 +40,7 @@ export interface SafeWriteJsonOptions { * Safely writes JSON data to a file. * - Creates parent directories if they don't exist * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path. - * - Writes to a temporary file first. + * - Writes to a temporary file first via JsonStreamStringify streaming. * - If the target file exists, it's backed up before being replaced. * - Attempts to roll back and clean up in case of errors. * - Supports pretty-printing with indentation while maintaining streaming efficiency. @@ -42,20 +50,24 @@ export interface SafeWriteJsonOptions { * @param {SafeWriteJsonOptions} options - Optional configuration for JSON formatting. * @returns {Promise} */ - async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op + // Resolve the publish target (the symlink referent when the path is a symlink) + // BEFORE the lock is taken. The lock, the merge read, the staged file and the + // commit rename must all key off this one canonical path: if the lock is keyed on + // the caller's alias while the publish lands on the referent, two writers reaching + // the same file through different names (the link and its referent) serialize on + // different locks and silently lose each other's merged updates. + const canonicalPath = await resolvePublishTarget(absoluteFilePath) + // For directory creation - const dirPath = path.dirname(absoluteFilePath) + const dirPath = path.dirname(canonicalPath) // Ensure directory structure exists with improved reliability try { - // Create directory with recursive option await fs.mkdir(dirPath, { recursive: true }) - - // Verify directory exists after creation attempt await fs.access(dirPath) } catch (dirError: any) { console.error(`Failed to create or access directory for ${absoluteFilePath}:`, dirError) @@ -69,11 +81,10 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // 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) + releaseLock = await acquireFileLock(canonicalPath) - // Variables to hold the actual paths of temp files if they are created. + // Variables to hold the actual path of the temp file if it is created. let actualTempNewFilePath: string | null = null - let actualTempBackupFilePath: string | null = null try { // If a merge callback was provided, read the current file under the lock @@ -82,7 +93,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso if (options?.merge) { let existing: unknown = null try { - existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + existing = JSON.parse(await fs.readFile(canonicalPath, "utf8")) } catch (error: unknown) { const code = error && typeof error === "object" && "code" in error ? (error as { code: string }).code : undefined @@ -93,79 +104,70 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso data = options.merge(existing, data) } - // Step 1: Write data to a new temporary file. + // Stage it beside the canonical target: safeWriteText commits by renaming onto + // that referent, and a rename across filesystems would fail with EXDEV. actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, + path.dirname(canonicalPath), + ".new_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp", ) - await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint) - - // Step 2: Check if the target file exists. If so, rename it to a backup path. + // The staged file holds the entire new content while it exists, so it must not + // be created with the process default (0o666 & ~umask, i.e. 0o644) next to a + // target that is deliberately narrower - a 0o600 settings file in a shared + // directory, for example. Mirror the existing target's mode, but always keep the + // owner read/write bits: safeWriteText reopens the staged file with "r+" before it + // applies the target mode with fchmod, so a read-only mirror (0o400/0o444) would + // fail that open with EACCES. A target that does not exist yet keeps the ordinary + // default; any other stat failure is surfaced instead of silently widening the + // creation mode. + let stagingMode: number | undefined try { - // Check for target file existence - await fs.access(absoluteFilePath) - // Target exists, create a backup path and rename. - actualTempBackupFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, - ) - await fs.rename(absoluteFilePath, actualTempBackupFilePath) - } catch (accessError: any) { - // Explicitly type accessError - if (accessError.code !== "ENOENT") { - // An error other than "file not found" occurred during access check. - throw accessError + stagingMode = (fsSync.statSync(canonicalPath).mode & 0o777) | 0o600 + } catch (statError: unknown) { + const statCode = + typeof statError === "object" && statError !== null && "code" in statError + ? (statError as { code?: string }).code + : undefined + if (statCode !== "ENOENT") { + throw statError } - // Target file does not exist, so no backup is made. actualTempBackupFilePath remains null. + stagingMode = undefined + } + + await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint, stagingMode) + + // Step 2: Delegate the atomic commit to safeWriteText with the pre-written + // temp path. The publish is a single rename, so the target stays intact on + // failure and no backup copy is needed. safeWriteText still captures the + // Windows DACL before the commit rename and restores it afterwards. + const textOptions: SafeWriteTextOptions = { + tempPath: actualTempNewFilePath, } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) + await safeWriteText(canonicalPath, "", textOptions) // 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" actualTempNewFilePath = null - - // Step 4: If a backup was created, attempt to delete it. - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - // Mark backup as handled - actualTempBackupFilePath = null - } catch (unlinkBackupError) { - // 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}:`, - unlinkBackupError, - ) - } - } } catch (originalError) { console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) - const newFileToCleanupWithinCatch = actualTempNewFilePath - const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath - - // Attempt rollback if a backup was made - if (backupFileToRollbackOrCleanupWithinCatch) { - try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, 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}:`, - rollbackError, - ) - } + // PublishNotDurableError and DaclRestoreError are the failures where the commit + // rename DID land: the staged path was renamed onto the target, so it is no + // longer a leftover temp file and must never be treated as one below. For + // PublishNotDurableError only the durability of the directory entry is + // unconfirmed; for DaclRestoreError the content is committed and durable but the + // saved access rights could not be put back. Both rethrow, so neither can be + // observed as a successful save. + if (originalError instanceof PublishNotDurableError || originalError instanceof DaclRestoreError) { + actualTempNewFilePath = null } - // Cleanup the .new file if it exists + const newFileToCleanupWithinCatch = actualTempNewFilePath + + // Any other failure means the commit rename never landed, so the target still + // holds the previous bytes. Clean up the staged file if it still exists + // (safeWriteText also cleans up its tempPath on failure; this is a safety net + // in case its cleanup missed it). if (newFileToCleanupWithinCatch) { try { await fs.unlink(newFileToCleanupWithinCatch) @@ -177,26 +179,12 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso } } - // Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored) - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, - cleanupError, - ) - } - } throw originalError // This MUST be the error that rejects the promise. } finally { // Release the lock in the main finally block. try { - // releaseLock will be the actual unlock function if lock was acquired, - // or the initial no-op if acquisition failed. 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) } } @@ -209,9 +197,14 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso * @param prettyPrint Whether to format the JSON with indentation. * @returns Promise */ -async function _streamDataToFile(targetPath: string, data: any, prettyPrint = false): Promise { +async function _streamDataToFile(targetPath: string, data: any, prettyPrint = false, mode?: number): Promise { // Stream data to avoid high memory usage for large JSON objects. - const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) + // mode is explicit because createWriteStream defaults to 0o666 (& ~umask): the + // staged file is readable by others until the commit renames it onto the target. + const fileWriteStream = fsSync.createWriteStream(targetPath, { + encoding: "utf8", + ...(mode !== undefined ? { mode } : {}), + }) // JsonStreamStringify traverses the object and streams tokens directly // The 'spaces' parameter adds indentation during streaming, not via a separate pass