diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de2b84590..8da6bb62a6 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3345,6 +3345,15 @@ export class Task extends EventEmitter implements TaskLike { // could still build tools and call `createMessage()`. this.abort = true + // Stop post-save diagnostics tails that are still waiting on their delay. A + // disposed task cannot receive their say() emit, and without this the timer (and + // the provider + diagnostics snapshot it holds) survives the teardown. + try { + this.diffViewProvider.cancelPostSaveDiagnosticsTails() + } catch (error) { + console.error("Error cancelling post-save diagnostics tails:", error) + } + // Cancel any in-progress HTTP request try { this.cancelCurrentRequest() diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 57fe347acf..507edbb15a 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3696,12 +3696,17 @@ describe("Project MCP Settings", () => { const expectedRooDir = path.join("/test/workspace", ".roo") const expectedMcpPath = path.join(expectedRooDir, "mcp.json") - // Check that fs.mkdir was called with the correct path - expect(mockedFs.mkdir).toHaveBeenCalledWith(expectedRooDir, { recursive: true }) + // The handler must not create .roo itself: a dangling symlink there resolves + // outside the workspace, and creating that parent is exactly what confinement is + // supposed to prevent. safeWriteJson creates it, but only after its scope check. + expect(mockedFs.mkdir).not.toHaveBeenCalledWith(expectedRooDir, { recursive: true }) expect(pathUtils.getWorkspacePath).toHaveBeenCalled() - // Verify file was created with default content - expect(safeWriteJson).toHaveBeenCalledWith(expectedMcpPath, { mcpServers: {} }, { prettyPrint: true }) + // The project-scoped write carries the workspace root as its confinement scope. + expect(safeWriteJson).toHaveBeenCalledWith(expectedMcpPath, { mcpServers: {} }, { + prettyPrint: true, + confineTo: "/test/workspace", + }) // Check that openFile was called expect(openFileSpy).toHaveBeenCalledWith(expectedMcpPath) @@ -3730,10 +3735,9 @@ describe("Project MCP Settings", () => { const pathUtils = await import("../../../utils/path") vi.mocked(pathUtils.getWorkspacePath).mockReturnValue("/test/workspace") - // Mock fs functions to fail - const fs = await import("fs/promises") - const mockedFs = vi.mocked(fs) - mockedFs.mkdir.mockRejectedValue(new Error("Failed to create directory")) + // The handler no longer creates the directory itself, so the failure has to come + // from the confined write. + vi.mocked(safeWriteJson).mockRejectedValueOnce(new Error("Failed to create directory")) // Trigger openProjectMcpSettings await messageHandler({ diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 4ba94d454c..4a7ba7e35b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1791,11 +1791,17 @@ export const webviewMessageHandler = async ( const mcpPath = path.join(rooDir, "mcp.json") try { - await fs.mkdir(rooDir, { recursive: true }) + // .roo/mcp.json is a path the repository controls, so it must be confined to the + // workspace before anything is created. A dangling symlink chain at .roo makes + // fileExistsAtPath report "absent" and lets a plain write create the parent + // directory OUTSIDE the workspace and drop the placeholder there. safeWriteJson + // resolves the publish target, checks confineTo and only then creates the parent + // directory, so the mkdir that used to run first is deliberately gone: nothing is + // created until the scope check has passed. const exists = await fileExistsAtPath(mcpPath) if (!exists) { - await safeWriteJson(mcpPath, { mcpServers: {} }, { prettyPrint: true }) + await safeWriteJson(mcpPath, { mcpServers: {} }, { prettyPrint: true, confineTo: workspaceFolder }) } await openFile(mcpPath) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 908159f7ab..d4809c50d8 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1166,7 +1166,7 @@ }, "integrations/editor/__tests__/DiffViewProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 310 + "count": 306 } }, "integrations/editor/__tests__/EditorUtils.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..0eb6ae8e73 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -18,6 +18,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" @@ -46,6 +47,10 @@ export class DiffViewProvider { private streamedLines: string[] = [] private preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [] private preEditScrollLine: number | undefined + // One controller per post-save diagnostics tail that is still waiting, so task + // disposal can cancel the wait instead of leaving a timer (and this provider and + // the pre-save diagnostics snapshot it closes over) running past the teardown. + private readonly postSaveTails = new Set() // Tracks whether the user activated the target file's editor tab during the // diff session. When the file was not already open before the edit, we only // keep it open afterward if the user explicitly interacted with it. @@ -1151,12 +1156,15 @@ export class DiffViewProvider { }> { const absolutePath = path.resolve(this.cwd, relPath) - // Get diagnostics before editing the file - this.preDiagnostics = vscode.languages.getDiagnostics() + // Get diagnostics before editing the file. Capture the snapshot locally: + // overlapping saveDirectly calls (multi-file edits) must not let a later + // call overwrite this one's baseline before its diagnostics tail runs. + const preDiagnostics = vscode.languages.getDiagnostics() + this.preDiagnostics = preDiagnostics // Write the content directly to the file await createDirectoriesForFile(absolutePath) - await fs.writeFile(absolutePath, content, "utf-8") + 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 @@ -1175,23 +1183,83 @@ export class DiffViewProvider { await doc.save() } - // Force a small delay to ensure diagnostics are triggered - await new Promise((resolve) => setTimeout(resolve, 100)) + // The 100 ms diagnostics-settle wait is carried by the + // emitPostSaveDiagnostics tail (inMemoryDocument) instead of here: + // blocking the save path delayed every openFile=false save even when + // diagnostics were disabled or the write delay was 0. } - let newProblemsMessage = "" - + // L1 (A2): resolve without awaiting the LSP diagnostics settle. The + // diagnostics check becomes a fire-and-forget tail that emits any new + // problems via the existing "error" ClineSay type; the returned + // newProblemsMessage is therefore always undefined. if (diagnosticsEnabled) { - // Add configurable delay to allow linters time to process - const safeDelayMs = Math.max(0, writeDelayMs) + // The method's outer try/catch guarantees it never rejects, so the + // fire-and-forget call needs no .catch wrapper. + void this.emitPostSaveDiagnostics(relPath, writeDelayMs, preDiagnostics, !openFile) + } + + // Store the results for formatFileWriteResponse + this.newProblemsMessage = undefined + this.userEdits = undefined + this.relPath = relPath + this.newContent = content + + return { + newProblemsMessage: undefined, + userEdits: undefined, + finalContent: content, + } + } + // L1 (A2): fire-and-forget post-save diagnostics. After the write delay, + // collects new Error-severity problems and emits them via the existing + // "error" ClineSay type (only Error-severity diagnostics reach this branch; + // "error" carries no task-failure semantics in core). Abort-safe: say() + // rejects when the task is aborted, so the whole body sits inside a + // try/catch that degrades to a console.warn — the tail can never reject. + // The wait itself is registered in postSaveTails so Task disposal can cancel + // it: an unregistered delay keeps the timer, this provider and the pre-save + // diagnostics snapshot alive past disposal, and the tail then does stale + // diagnostics work against a task that is already gone. + private async emitPostSaveDiagnostics( + relPath: string, + writeDelayMs: number, + preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][], + inMemoryDocument = false, + ): Promise { + const controller = new AbortController() + this.postSaveTails.add(controller) + try { + // Add configurable delay to allow linters time to process. When the + // document was opened in memory (openFile=false), the tail also + // carries the 100 ms diagnostics-settle wait that used to block + // saveDirectly. The signal is the disposal hook: delay() rejects with + // AbortError once the task is gone, which is the tail's expected end. + const safeDelayMs = Math.max(0, writeDelayMs) + (inMemoryDocument ? 100 : 0) try { - await delay(safeDelayMs) + await delay(safeDelayMs, { signal: controller.signal }) } catch (error) { - console.warn(`Failed to apply write delay: ${error}`) + if (controller.signal.aborted) { + return + } + throw error + } + // A cancellation can also land between the wait resolving and the work + // starting; either way nothing is queried or emitted after it. + if (controller.signal.aborted) { + return } - const postDiagnostics = vscode.languages.getDiagnostics() + // Filter to the saved file: saveDirectly resolves before this tail + // completes, so in a multi-file write sequence (e.g. apply_patch) + // a later file's problems must not be attributed to this relPath. + const savedFilePath = path.resolve(this.cwd, relPath) + // arePathsEqual: case-insensitive on Windows, where a relPath whose + // casing differs from the diagnostic URI is still the same file. + const postDiagnostics = vscode.languages + .getDiagnostics() + .filter(([uri]) => arePathsEqual(uri.fsPath, savedFilePath)) // Get diagnostic settings from state const task = this.taskRef.deref() @@ -1199,28 +1267,51 @@ export class DiffViewProvider { const includeDiagnosticMessages = state?.includeDiagnosticMessages ?? true const maxDiagnosticMessages = state?.maxDiagnosticMessages ?? 50 + // The state read above can outlive a cancellation: re-check immediately before the + // emit so a disposed task never gets a persisted error row for a save whose tail + // was cancelled while the state read was still running. + if (controller.signal.aborted) { + return + } + const newProblems = await diagnosticsToProblemsString( - getNewDiagnostics(this.preDiagnostics, postDiagnostics), + getNewDiagnostics(preDiagnostics, postDiagnostics), [vscode.DiagnosticSeverity.Error], this.cwd, includeDiagnosticMessages, maxDiagnosticMessages, ) - newProblemsMessage = - newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : "" - } + // Formatting is awaited too, so a cancellation can land inside it. The emit below + // persists an error row into the task, so it must not start once the caller is gone: + // say() would otherwise be dropped or land in a task the user already left. + if (controller.signal.aborted) { + return + } - // Store the results for formatFileWriteResponse - this.newProblemsMessage = newProblemsMessage - this.userEdits = undefined - this.relPath = relPath - this.newContent = content + if (newProblems.length > 0) { + await task?.say("error", `New problems detected after saving file: ${relPath}\n\n${newProblems}`) + } + } catch (error) { + // Abort-safe: never let a post-save diagnostic emit become an + // unhandled rejection (say() rejects when the task is aborted). + console.warn(`Post-save diagnostics emit failed: ${error}`) + } + finally { + this.postSaveTails.delete(controller) + } + } - return { - newProblemsMessage, - userEdits: undefined, - finalContent: content, + /** + * Cancel post-save diagnostics tails that are still waiting. Called when the + * owning task is disposed. Deliberately NOT called from reset(): reset follows a + * successful write, and the tail that write started still has to report the + * problems it is waiting for. + */ + public cancelPostSaveDiagnosticsTails(): void { + for (const controller of this.postSaveTails) { + controller.abort() } + this.postSaveTails.clear() } } diff --git a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts index 00b3dcaf7a..10c0d3a95f 100644 --- a/src/integrations/editor/__tests__/DiffViewProvider.spec.ts +++ b/src/integrations/editor/__tests__/DiffViewProvider.spec.ts @@ -16,6 +16,14 @@ 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) +vi.mock("../../../services/file-safety/safeWriteText", () => ({ + safeWriteText: vi.fn().mockResolvedValue(undefined), })) // Mock utils @@ -26,7 +34,14 @@ vi.mock("../../../utils/fs", () => ({ // Mock path vi.mock("path", () => ({ resolve: vi.fn((cwd, relPath) => `${cwd}/${relPath}`), + normalize: vi.fn((p: string) => p), basename: vi.fn((path) => path.split("/").pop()), + dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"), + join: (...args: string[]) => args.join("/"), + // diagnosticsToProblemsString formats its output header via + // path.relative(cwd, uri.fsPath).toPosix(); the object-with-toPosix shape + // mirrors the repo's own diagnostics.spec.ts mock. + relative: vi.fn((cwd: string, p: string) => ({ toPosix: () => p.replace(`${cwd}/`, "") })), })) // Mock vscode @@ -165,6 +180,8 @@ describe("DiffViewProvider", () => { }), }), }, + // L1: saveDirectly's fire-and-forget diagnostics tail emits via say(). + say: vi.fn().mockResolvedValue(true), } diffViewProvider = new DiffViewProvider(mockCwd, mockTask) @@ -792,9 +809,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( @@ -802,12 +819,17 @@ describe("DiffViewProvider", () => { { preview: false, preserveFocus: true }, ) - // Verify diagnostics were checked after delay - expect(mockDelay).toHaveBeenCalledWith(2000) + // L1: saveDirectly resolves before the fire-and-forget diagnostics + // tail runs; flush one macrotask tick so the mocked delay (and the + // post-write getDiagnostics) have been reached before asserting. + await new Promise((resolve) => setTimeout(resolve, 0)) + + // Verify the tail applied the configured write delay + expect(mockDelay).toHaveBeenCalledWith(2000, expect.objectContaining({ signal: expect.any(AbortSignal) })) expect(vscode.languages.getDiagnostics).toHaveBeenCalled() - // Verify result - expect(result.newProblemsMessage).toBe("") + // Verify result: L1 no longer returns a problems message + expect(result.newProblemsMessage).toBeUndefined() expect(result.userEdits).toBeUndefined() expect(result.finalContent).toBe("new content") }) @@ -815,9 +837,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,14 +852,18 @@ 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() // getDiagnostics is called once for pre-diagnostics, but not for post-diagnostics expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(1) + + // L1: no diagnostics tail is launched, so nothing is ever emitted + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockTask.say).not.toHaveBeenCalled() }) it("should handle negative delay values", async () => { @@ -846,18 +872,328 @@ describe("DiffViewProvider", () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, true, -500) + // L1: the tail runs after resolve; flush one macrotask tick first. + await new Promise((resolve) => setTimeout(resolve, 0)) + // Verify delay was called with 0 (safe minimum) - expect(mockDelay).toHaveBeenCalledWith(0) + expect(mockDelay).toHaveBeenCalledWith(0, expect.objectContaining({ signal: expect.any(AbortSignal) })) }) it("should store results for formatFileWriteResponse", async () => { await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 1000) - // Verify internal state was updated - expect((diffViewProvider as any).newProblemsMessage).toBe("") - expect((diffViewProvider as any).userEdits).toBeUndefined() - expect((diffViewProvider as any).relPath).toBe("test.ts") - expect((diffViewProvider as any).newContent).toBe("new content") + // Verify internal state was updated (L1: the problems message is no + // longer stored; it is emitted asynchronously via say("error")) + expect(diffViewProvider["newProblemsMessage"]).toBeUndefined() + expect(diffViewProvider["userEdits"]).toBeUndefined() + expect(diffViewProvider["relPath"]).toBe("test.ts") + expect(diffViewProvider["newContent"]).toBe("new content") + }) + + it("resolves immediately and emits new problems via say('error') after the write delay", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // Pre-write diagnostics are empty; the post-write snapshot (read by + // the fire-and-forget tail) reports one new Error-severity problem. + // vscode.workspace.fs.stat is an unimplemented vi.fn() mock, so + // diagnosticsToProblemsString takes its "(unavailable)" fallback + // branch and still formats the line. + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "boom", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + + // L1: saveDirectly resolves before the tail emits anything. + expect(result.newProblemsMessage).toBeUndefined() + expect(mockTask.say).not.toHaveBeenCalled() + + // Flush the fire-and-forget tail (the mocked delay resolves immediately). + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockDelay).toHaveBeenCalledWith(100, expect.objectContaining({ signal: expect.any(AbortSignal) })) + expect(mockTask.say).toHaveBeenCalledTimes(1) + // The existing "error" ClineSay type is used, with the new-problems text. + expect(mockTask.say).toHaveBeenCalledWith( + "error", + expect.stringContaining("New problems detected after saving file: test.ts"), + ) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("boom") + }) + + it("attributes only the saved file's new problems to the saved file", async () => { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // Multi-file write sequence: a later file's new error must not be + // attributed to this tail's relPath by the workspace-wide snapshot. + const ownDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "own-problem", + } + const otherDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "other-file-problem", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [ + [makeUri(`${mockCwd}/test.ts`), [ownDiag]], + [makeUri(`${mockCwd}/other.ts`), [otherDiag]], + ] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("own-problem") + expect(mockTask.say.mock.calls[0]?.[1]).not.toContain("other-file-problem") + + }) + it("gives each post-save tail its own pre-save baseline when two saves overlap", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + const firstProblem: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "first-save-problem", + } + const withProblem: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [firstProblem]]] + + // Save #1's tail waits past save #2, so #2's baseline is captured while #1's + // tail is still waiting. + mockDelay + .mockImplementationOnce(() => new Promise((resolve) => setTimeout(resolve, 20))) + .mockImplementationOnce(() => Promise.resolve()) + + // getDiagnostics order: baseline#1, baseline#2, post#2, post#1. + vi.mocked(vscode.languages.getDiagnostics) + .mockReturnValueOnce([]) + .mockReturnValueOnce(withProblem) + .mockReturnValueOnce(withProblem) + .mockReturnValueOnce(withProblem) + + await diffViewProvider.saveDirectly("test.ts", "a", true, true, 100) + await diffViewProvider.saveDirectly("test.ts", "b", true, true, 100) + await new Promise((resolve) => setTimeout(resolve, 50)) + + // Tail #1 saw the problem appear after ITS baseline and reports it; tail #2 had + // it already in its own baseline and stays silent. A shared (last-captured) + // baseline would silence both, so the count is the assertion that matters. + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("first-save-problem") + }) + + it("stops a waiting post-save tail when the task is disposed", async () => { + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // delay() rejects with AbortError once its signal aborts - the disposal hook. + // Once only: the hanging implementation must not leak into the next test. + mockDelay.mockImplementationOnce((_ms: number, opts?: { signal?: AbortSignal }) => { + return new Promise((_resolve, reject) => { + opts?.signal?.addEventListener("abort", () => + reject(Object.assign(new Error("The operation aborted."), { name: "AbortError" })), + ) + }) + }) + + await diffViewProvider.saveDirectly("test.ts", "content", true, true, 5000) + expect(mockTask.say).not.toHaveBeenCalled() + + diffViewProvider.cancelPostSaveDiagnosticsTails() + await new Promise((resolve) => setTimeout(resolve, 0)) + + // Only the pre-save baseline was read: the tail never reached the diagnostics + // query, so nothing is computed or emitted against a disposed task. + expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(1) + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("stops a post-save tail that is cancelled while the diagnostic settings are read", async () => { + const mockDelay = vi.mocked(delay) + // Two delay() calls exist on this path (the save's own settle wait and the tail's), + // so both must resolve: if the tail's wait never resolves the test would pass + // vacuously instead of proving the re-check. + mockDelay.mockImplementationOnce(() => Promise.resolve()).mockImplementationOnce(() => Promise.resolve()) + + // A real problem is pending, so the only thing that can keep it from being + // persisted is the re-check after the awaited settings read. + const ownDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "cancelled-tail-problem", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [ownDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + // The settings read is the last await before the formatting and the emit. Resolving + // it AFTER the cancellation is how a cancellation lands inside it: the await returns + // normally and only the signal shows that the task is gone. + const provider = mockTask.providerRef.deref() + const originalGetState = provider.getState + const slowGetState = vi.fn( + () => + new Promise((resolve) => + setTimeout(() => resolve({ includeDiagnosticMessages: true, maxDiagnosticMessages: 50 }), 20), + ), + ) + provider.getState = slowGetState + + try { + await diffViewProvider.saveDirectly("test.ts", "content", true, true, 100) + // Let the tail pass the settle delay and reach the settings await. + await new Promise((resolve) => setTimeout(resolve, 5)) + expect(mockTask.say).not.toHaveBeenCalled() + + diffViewProvider.cancelPostSaveDiagnosticsTails() + // The pending settings read resolves after the cancellation. + await new Promise((resolve) => setTimeout(resolve, 80)) + + expect(slowGetState).toHaveBeenCalledTimes(1) + // The tail had already taken its pre-save baseline and read the post-save + // diagnostics; it stopped at the re-check, so the pending problem is never + // emitted into a task that no longer exists. + expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(2) + expect(mockTask.say).not.toHaveBeenCalled() + } finally { + provider.getState = originalGetState + } + }) + + it("attributes diagnostics to the saved file when the URI casing differs (Windows)", async () => { + const platformSpy = vi.spyOn(process, "platform", "get").mockReturnValue("win32") + try { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // The diagnostic URI uses different casing than the saved relPath: + // on Windows this is still the same file (arePathsEqual). + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "case-mismatch-problem", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/Test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100) + + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.say).toHaveBeenCalledTimes(1) + expect(mockTask.say.mock.calls[0]?.[1]).toContain("case-mismatch-problem") + } finally { + // Restore even when an assertion above fails: a leaked process.platform getter would + // make later tests see win32 (arePathsEqual and friends are platform sensitive). + platformSpy.mockRestore() + } + }) + + it("does not block the save on a diagnostics settle delay when diagnostics are disabled", async () => { + // openFile=false used to await a 100 ms settle delay even when + // diagnostics were disabled; that delay now lives in the tail, which + // does not run at all when diagnosticsEnabled is false. + vi.mocked(vscode.languages.getDiagnostics).mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + const result = await diffViewProvider.saveDirectly("test.ts", "new content", false, false) + + expect(result.finalContent).toBe("new content") + expect(mockDelay).not.toHaveBeenCalled() + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("carries the in-memory settle delay in the tail for openFile=false saves", async () => { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + const mockDelay = vi.mocked(delay) + mockDelay.mockClear() + + // Hold the tail delay so the save can be observed resolving while it is still + // pending: the save path must not await the settle delay itself. + let delayPending = true + let releaseDelay: () => void = () => {} + const tailDelay = new Promise((resolve) => { + releaseDelay = resolve + }) + void tailDelay.then(() => { + delayPending = false + }) + mockDelay.mockImplementation(() => tailDelay) + + const save = diffViewProvider.saveDirectly("test.ts", "new content", false, true, 100) + await save + expect(delayPending).toBe(true) + + releaseDelay() + await save + + // writeDelayMs (100) + the 100 ms in-memory diagnostics settle, both + // applied by the tail instead of the save path. + expect(mockDelay).toHaveBeenCalledWith(200, expect.objectContaining({ signal: expect.any(AbortSignal) })) + + // Let the fire-and-forget tail run to completion before the test ends: with no + // diagnostics the tail must stay silent, and an unfinished tail would leak + // into the next test's assertions. + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("never calls say when there are no new problems", async () => { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([]) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 50) + + // Flush the fire-and-forget tail (pre/post snapshots are both empty, + // so diagnosticsToProblemsString returns "" and nothing is emitted). + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockTask.say).not.toHaveBeenCalled() + }) + + it("logs a warning instead of an unhandled rejection when the post-save say() is aborted", async () => { + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + try { + vi.mocked(vscode.languages.getDiagnostics).mockClear() + + // One new Error-severity problem so the tail reaches say(). + const newDiag: vscode.Diagnostic = { + severity: vscode.DiagnosticSeverity.Error, + range: new vscode.Range(0, 0, 0, 1), + message: "boom", + } + const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]] + vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics) + + // The task is aborted while the diagnostics tail is emitting: say() rejects. + mockTask.say.mockRejectedValueOnce(new Error("aborted")) + + await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 0) + // Flush the fire-and-forget tail. + await new Promise((resolve) => setTimeout(resolve, 0)) + + // The method's outer catch swallows the rejection with a warning. + expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining("Post-save diagnostics emit failed:")) + } finally { + // Restore even when an assertion above fails, so the mocked console.warn does not + // leak into later tests. + consoleWarnSpy.mockRestore() + } }) }) 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..c01493ca5b --- /dev/null +++ b/src/services/file-safety/__tests__/safeWriteText.spec.ts @@ -0,0 +1,881 @@ +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 { resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../safeWriteText" + +// 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(), + readlink: 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(), + statSync: vi.fn(), + Stats: class Stats {}, +})) + +// Mock child_process.execFile (callback-based — must invoke callback to resolve) +vi.mock("child_process", () => ({ + execFile: vi.fn((cmd, args, opts, cb) => { + if (typeof cb === "function") cb(null) + }), +})) + +// Minimal stand-in for the ChildProcess that callback-form execFile returns. +const fakeChild = { kill: () => true } as unknown as ChildProcess + +// Helper that mirrors safeWriteText's path resolution exactly +function _resolvedTarget(filePath: string): string { + return path.resolve(filePath) +} +function _dirPath(filePath: string): string { + return path.dirname(_resolvedTarget(filePath)) +} +function _stagingDir(dir: string): string { + return path.join(dir, ".file-safety-staging") +} + +// Minimal Stats stand-in: the SUT only reads `.mode` from it. +function _stats(mode: number): fsSync.Stats { + const s = Object.create(fsSync.Stats.prototype) as fsSync.Stats + Object.assign(s, { mode }) + return s +} + +// ── Test 1: staging file created then cleaned after success ──────────────── + +describe("safeWriteText", () => { + beforeEach(() => { + 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) + // Existing-target default: a regular 0o644 file. + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o644)) + // 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() + }) + }) + + // ── 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).toHaveBeenNthCalledWith(2, 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(2) + + // 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", () => { + it("renames 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.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + + // second rename: temp -> target (realpath mock returns targetPath) + expect(fs.rename).toHaveBeenNthCalledWith(2, expect.stringContaining("safeWriteText_"), targetPath) + + // backup was deleted on success + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.bak_")) + }) + + it("rollback: on failure after rename target->backup, restores backup to target", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // first rename (target->backup) succeeds, second fails + let callCount = 0 + vi.mocked(fs.rename).mockImplementation(async () => { + callCount++ + if (callCount === 1) return // target -> backup + throw new Error("ENOSPC") // temp -> target fails + }) + + await expect(safeWriteText(targetPath, "new data", { backup: true })).rejects.toThrow("ENOSPC") + + // rollback rename is the 3rd call (after target->backup and temp->target failure) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText.bak_"), targetPath) + + // 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("staging directory cleanup", () => { + it("removes the staging directory after a successful commit", 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" }) + + // The staging directory is created inside the user's own directory; leaving + // it behind litters every directory that ever receives a direct save. + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("removes the staging directory after a failed write", 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("EXDEV")) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow("EXDEV") + + expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging")) + }) + + it("leaves no staging directory to remove when the caller supplied the temp file", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "", { tempPath: "/tmp/test-dir/custom.tmp", platform: "linux" }) + + expect(fs.rmdir).not.toHaveBeenCalled() + }) + }) + + describe("win32 DACL", () => { + it.skipIf(process.platform !== "win32")( + "copies target DACL onto staging file via icacls before rename on Windows", + async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // icacls dump + restore were called (execFile is callback-based mock) + expect(execFile).toHaveBeenCalledTimes(2) + }, + ) + + it("non-win32: DACL path is unreachable when platform is not win32", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // icacls was NOT called on non-win32 + expect(execFile).not.toHaveBeenCalled() + }) + + it("win32 DACL failure falls back to plain rename (never fails the write)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // icacls dump fails — the callback-based mock must invoke cb with an error. + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + if (typeof cb === "function") cb(new Error("icacls error"), "", "") + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite icacls failure (fallback to plain rename) + expect(fs.rename).toHaveBeenCalled() + }) + + it("win32 DACL save args are [targetPath, /save, dumpPath, /T] before backup rename", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // icacls was called twice (save + restore) + expect(execFile).toHaveBeenCalledTimes(2) + + // First call: save DACL from target before backup rename + const firstCall = vi.mocked(execFile).mock.calls[0] + expect(firstCall[0]).toBe("icacls") + expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining(".acl.tmp"), "/T"]) + + // Second call: restore DACL onto directory after commit rename + const secondCall = vi.mocked(execFile).mock.calls[1] + expect(secondCall[0]).toBe("icacls") + expect(secondCall[1]).toEqual([ + expect.stringContaining("/tmp/test-dir"), + "/restore", + expect.stringContaining(".acl.tmp"), + ]) + + // dump file was unlinked after restore + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: dump is unlinked even when restore fails", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + // icacls save succeeds, restore fails + let callCount = 0 + vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => { + callCount++ + if (typeof cb === "function") { + cb(callCount === 1 ? null : new Error("icacls restore error"), "", "") + } + return fakeChild + }) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + // write succeeded despite restore failure (best-effort) + expect(fs.rename).toHaveBeenCalled() + + // dump file was still unlinked in finally + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining(".acl.tmp")) + }) + + it("win32 DACL: a failed restore is reported instead of swallowed", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + 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 + }) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + // The write still succeeds (the content is already committed), but the permission + // regression is no longer invisible: a caller can otherwise never learn that the + // target kept the ACL it inherited instead of the one it had. + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fs.rename).toHaveBeenCalled() + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to restore the saved Windows DACL"), + expect.any(Error), + ) + }) + + it("win32 DACL: the dump file name is unique per write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(targetPath, "a", { platform: "win32" }) + const dump1 = String(vi.mocked(execFile).mock.calls[0][1]?.[2]) + vi.mocked(execFile).mockClear() + await safeWriteText(targetPath, "b", { platform: "win32" }) + const dump2 = String(vi.mocked(execFile).mock.calls[0][1]?.[2]) + + // A fixed name would let two overlapping saves of the same target share one + // dump (one unlinking it while the other is still restoring), and would let + // icacls /save overwrite - then delete - a user file at that path. + expect(dump1).toContain(".acl.tmp") + expect(dump1).not.toBe(targetPath + ".acl.tmp") + expect(dump2).not.toBe(dump1) + }) + + 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("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("aborts instead of defaulting the mode when the target stat fails for another reason", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // EACCES means the target is there but unreadable to us, not that it is missing. + // Falling back to 0o644 would let the rename widen an existing file's mode. + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fsSync.statSync).mockImplementation((p) => { + if (String(p) === targetPath) { + throw eacces + } + return _stats(0o644) + }) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + + // Nothing is staged and nothing is published on an unknown target mode. + expect(fsSync.openSync).not.toHaveBeenCalled() + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("opens the temp before applying a read-only target's mode (0o444 does not block the open)", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o444)) + vi.mocked(fsSync.openSync).mockReturnValue(3) + + const customTempPath = "/tmp/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("only ENOENT/EINVAL from readlink may fall back to the link path", async () => { + const linkPath = "/tmp/test-dir/link.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + // The path is a link we cannot read: falling back would publish a regular file + // over the link instead of failing, silently breaking the referent contract. + vi.mocked(fs.readlink).mockRejectedValue(Object.assign(new Error("EACCES"), { code: "EACCES" })) + + await expect(resolvePublishTarget(linkPath)).rejects.toThrow("EACCES") + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("readlink EINVAL (not a link) falls back to the given path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockRejectedValue(Object.assign(new Error("EINVAL"), { code: "EINVAL" })) + + // resolvePublishTarget takes the already-resolved path and returns it unchanged + // when the path is not a link. + await expect(resolvePublishTarget(targetPath)).resolves.toBe(targetPath) + }) + + 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" })) + // Not a link: readlink also reports ENOENT, so the path is genuinely absent. + vi.mocked(fs.readlink).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("publishes through a dangling symlink to its intended referent", async () => { + const linkPath = "/tmp/test-dir/link.txt" + const referentPath = "/tmp/test-dir/real.txt" + // realpath reports ENOENT for a dangling link, so the code must not treat the link path + // as an absent target and replace the link with a regular file. + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockImplementation(async (target) => { + if (String(target).endsWith("link.txt")) return "real.txt" + // The referent is absent and not a link either: the chain ends here. + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "data", { platform: "linux" }) + + const expectedReferent = _resolvedTarget("/tmp/test-dir/real.txt") + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), expectedReferent) + expect(fs.rename).not.toHaveBeenCalledWith(expect.anything(), _resolvedTarget(linkPath)) + }) + + it("follows a two-hop dangling chain to its final referent", async () => { + const linkPath = "/tmp/test-dir/link.txt" + // A -> B -> /elsewhere/final.txt, none of which exists. Stopping at B would let a + // scope check drawn around B pass while the commit landed outside that scope. + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockImplementation(async (target) => { + if (String(target).endsWith("link.txt")) return "mid.txt" + if (String(target).endsWith("mid.txt")) return "/elsewhere/final.txt" + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + vi.mocked(fsSync.openSync).mockReturnValue(1) + + await safeWriteText(linkPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith( + expect.stringContaining("safeWriteText_"), + _resolvedTarget("/elsewhere/final.txt"), + ) + expect(fs.rename).not.toHaveBeenCalledWith( + expect.anything(), + _resolvedTarget("/tmp/test-dir/mid.txt"), + ) + }) + + it("gives up on a symlink loop instead of spinning", async () => { + const linkPath = "/tmp/test-dir/loop.txt" + vi.mocked(fs.realpath).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) + vi.mocked(fs.readlink).mockImplementation(async () => linkPath) + + await expect(safeWriteText(linkPath, "data", { platform: "linux" })).rejects.toThrow( + /symlink hops/, + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) + + // ── Windows commit-rename retry ────────────────────────────────────────── + + describe("commit rename retry on Windows", () => { + it("retries a sharing-violation rename and commits on the next attempt", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // An indexer or antivirus scan that holds the destination without FILE_SHARE_DELETE + // makes MoveFileExW fail with EPERM; the handle is released within milliseconds. + vi.mocked(fs.rename).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + await safeWriteText(targetPath, "data", { platform: "win32" }) + + expect(fs.rename).toHaveBeenCalledTimes(2) + expect(fs.rename).toHaveBeenLastCalledWith( + expect.stringContaining("safeWriteText_"), + targetPath, + ) + }) + + it("gives up after the bounded attempts and surfaces the error", 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"), { code: "EBUSY" })) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow("EBUSY") + + expect(fs.rename).toHaveBeenCalledTimes(4) + expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_")) + }) + + it("does not retry a rename failure that cannot be transient", 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("EXDEV"), { code: "EXDEV" })) + + await expect(safeWriteText(targetPath, "data", { platform: "win32" })).rejects.toThrow("EXDEV") + + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("does not retry on POSIX, where rename does not fail on a held handle", 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("EPERM"), { code: "EPERM" })) + + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toThrow("EPERM") + + expect(fs.rename).toHaveBeenCalledTimes(1) + }) + + it("retries the backup rename too, so a held target does not fail a JSON write", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // With backup:true the first rename is target -> backup, where the held handle is the + // SOURCE. safeWriteJson always writes with backup:true, so without this retry every + // JSON persistence write still fails on Windows when the target is open elsewhere. + vi.mocked(fs.rename).mockRejectedValueOnce(Object.assign(new Error("EPERM"), { code: "EPERM" })) + + await safeWriteText(targetPath, "data", { backup: true, platform: "win32" }) + + // attempt 1 (backup) failed, attempt 2 (backup) committed, then the commit rename. + expect(fs.rename).toHaveBeenCalledTimes(3) + expect(fs.rename).toHaveBeenNthCalledWith(1, targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenNthCalledWith(2, targetPath, expect.stringContaining("safeWriteText.bak_")) + expect(fs.rename).toHaveBeenNthCalledWith(3, expect.stringContaining("safeWriteText_"), targetPath) + }) + }) + + // ── Test 8: review fixes (permissions, partial writes, resolution, durability) ── + + describe("review fixes", () => { + it("preserves the target's restrictive mode and tolerates a failed staging-dir permission repair", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockReturnValue(_stats(0o600)) + // a pre-existing staging dir may fail its best-effort permission repair + vi.mocked(fsSync.chmodSync).mockImplementationOnce(() => { + throw new Error("EACCES") + }) + + await safeWriteText(targetPath, "secret", { platform: "linux" }) + + // the staging file inherits the target's 0o600 mode and the write commits + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o600) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("falls back to the 0o644 default when the target does not exist yet", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + vi.mocked(fsSync.statSync).mockImplementation(() => { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + }) + + await safeWriteText(targetPath, "fresh", { platform: "linux" }) + + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), "w", 0o644) + }) + + it("loops on short writes until the full content is durable before fsync", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const content = "0123456789" // 10 bytes + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + const buffer = Buffer.from(content, "utf8") + // first write (offset 0) reports 4 bytes (short write); the loop continues + vi.mocked(fsSync.writeSync).mockImplementation((...args: unknown[]) => + args[2] === 0 ? 4 : typeof args[3] === "number" ? args[3] : 0, + ) + + await safeWriteText(targetPath, content, { platform: "linux" }) + + // [0,10) reports 4 bytes, then [4,10) writes the remaining 6 + expect(fsSync.writeSync).toHaveBeenCalledTimes(2) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(1, 1, buffer, 0, 10) + expect(fsSync.writeSync).toHaveBeenNthCalledWith(2, 1, buffer, 4, 6) + expect(fsSync.fsyncSync).toHaveBeenCalledWith(1) + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("fsyncs the parent directory after the commit rename on POSIX", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + // temp fd=1 then parent-dir fd=2 - distinct fds prove the ordering + vi.mocked(fsSync.openSync).mockReturnValueOnce(1).mockReturnValue(2) + + await safeWriteText(targetPath, "data", { platform: "linux" }) + + // the directory fsync (fd 2) happens only after the file fsync (fd 1); + // the dir path assertion is path-agnostic (stringContaining) because + // path.dirname renders the same input differently on Windows + expect(fsSync.openSync).toHaveBeenCalledWith(expect.stringContaining("test-dir"), "r") + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(1, 1) + expect(fsSync.fsyncSync).toHaveBeenNthCalledWith(2, 2) + expect(fsSync.closeSync).toHaveBeenCalledWith(2) + }) + + it("treats a failed parent-directory fsync as best-effort", async () => { + const targetPath = "/tmp/test-dir/target.txt" + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync) + .mockReturnValueOnce(1) + .mockImplementationOnce(() => { + throw new Error("EBADF") + }) + + // the content rename already committed; a missing directory fsync is not fatal + await safeWriteText(targetPath, "data", { platform: "linux" }) + + expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath) + }) + + it("propagates realpath errors (EACCES and code-less) instead of the fallback path", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + vi.mocked(fs.realpath).mockRejectedValueOnce(eacces) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(eacces) + expect(fs.rename).not.toHaveBeenCalled() + + const plain = new Error("resolution failed") + vi.mocked(fs.realpath).mockRejectedValueOnce(plain) + await expect(safeWriteText(targetPath, "data", { platform: "linux" })).rejects.toBe(plain) + expect(fs.rename).not.toHaveBeenCalled() + }) + + it("backup:true propagates access errors (EACCES and code-less) instead of skipping the backup", async () => { + const targetPath = "/tmp/test-dir/target.txt" + const eacces = Object.assign(new Error("EACCES"), { code: "EACCES" }) + const plain = new Error("access failed") + vi.mocked(fs.realpath).mockResolvedValue(targetPath) + vi.mocked(fsSync.openSync).mockReturnValue(1) + // each write accesses dirPath then target; only the target access rejects + const rejectTarget = (error: Error) => async (p: unknown) => { + if (typeof p === "string" && p.endsWith("target.txt")) throw error + } + vi.mocked(fs.access) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(eacces)) + .mockImplementationOnce(rejectTarget(plain)) + .mockImplementationOnce(rejectTarget(plain)) + + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toEqual( + expect.objectContaining({ code: "EACCES" }), + ) + await expect(safeWriteText(targetPath, "data", { backup: true, platform: "linux" })).rejects.toThrow( + "access failed", + ) + expect(fs.rename).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/services/file-safety/safeWriteText.ts b/src/services/file-safety/safeWriteText.ts new file mode 100644 index 0000000000..33c4598147 --- /dev/null +++ b/src/services/file-safety/safeWriteText.ts @@ -0,0 +1,486 @@ +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, preserve the old-file semantics: rename target -> backup first, + * after commit rename delete the backup; on failure roll the backup back to + * the target path. When false (default) the atomic rename simply replaces + * the target -- crash-safe window is zero. + */ + 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") +} + +/** Create a private staging sub-directory inside *dir* so that multiple + * concurrent writes never collide on their temp names. */ +function _stagingDir(dir: string): string { + const sd = path.join(dir, ".file-safety-staging") + // mode:0o700 protects a freshly created staging dir; the best-effort chmod + // repairs a pre-existing one (mkdirSync with recursive:true never chmods an + // existing directory), so staged temp files are never group/world readable. + fsSync.mkdirSync(sd, { recursive: true, mode: 0o700 }) + try { + fsSync.chmodSync(sd, 0o700) + } catch { + // best-effort: chmod denied or unavailable; a fresh dir was still + // created with the requested mode + } + return sd +} + +/** Unique name for the Windows DACL dump, kept beside the target. A fixed name + * would let icacls /save overwrite a user file that happens to live at that path + * (and the cleanup below would then delete it), and two overlapping saves of the + * same target would share one dump - one could unlink it while the other is still + * restoring from it. icacls /restore takes the dump path as an argument, so the + * name is free to choose. */ +function _aclDumpName(targetPath: string): string { + return targetPath + "." + Date.now() + "." + Math.random().toString(36).substring(2) + ".acl.tmp" +} + +/** Best-effort removal of the staging directory once the write is finished. The + * directory lives inside the user's own directory, so leaving it behind litters + * every directory that ever receives a write. ENOTEMPTY (a concurrent writer is + * still staging there) and any other error are ignored: removing it is never worth + * failing a write whose content has already committed. */ +async function _removeStagingDir(stagingDir: string): Promise { + try { + await fs.rmdir(stagingDir) + } catch { + // best-effort: not empty, already gone, or not permitted + } +} + +/** + * 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) +} + +/** Save the DACL of *srcPath* to a dump file on Windows. + * Returns true when the dump was written successfully; false otherwise. + * Never throws — callers treat failure as "skip DACL handling". */ +async function _saveDaclWindows(srcPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [srcPath, "/save", dumpPath, "/T"], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch { + return false + } +} + +/** Restore a DACL dump onto *dirPath* on Windows. + * Best-effort: the content is already committed, so a failure is not fatal - but it + * is reported, because a silently skipped restore is how a target ends up keeping + * the ACL it inherited instead of the one it had. */ +async function _restoreDaclWindows(dirPath: string, dumpPath: string, execFileRunner?: typeof execFile): Promise { + const runner = execFileRunner ?? execFile + try { + await new Promise((resolve, reject) => { + runner("icacls", [dirPath, "/restore", dumpPath], { windowsHide: true }, (err) => + err ? reject(err) : resolve(), + ) + }) + return true + } catch (error) { + console.error(`Failed to restore the saved Windows DACL from ${dumpPath} onto ${dirPath}:`, error) + return false + } +} + +// -- 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. + */ + +/** Bound on how far a dangling chain is followed; past that the chain is a loop. */ +const MAX_SYMLINK_HOPS = 40 + +/** + * Resolve the publish target: the symlink referent when the given path is an + * existing symlink, the path itself otherwise. Only ENOENT (target absent yet) + * may fall back to the given path; any other resolution error (EACCES, EIO, ...) + * propagates so a broken or unreadable symlink is never written through its + * link path. Callers that stage a temp file themselves must stage it beside + * the resolved path: the commit is a rename onto the referent, and a rename + * across filesystems fails with EXDEV. + * + * A dangling chain (A -> B -> C where C does not exist) is followed to its end, + * up to MAX_SYMLINK_HOPS, rather than stopping at the first referent: a caller + * that checks a scope against an intermediate hop must not have a later resolution + * of that hop land outside the scope it was checked in. + */ + +export async function resolvePublishTarget(absoluteFilePath: string): Promise { + let current = absoluteFilePath + for (let hop = 0; hop <= MAX_SYMLINK_HOPS; hop++) { + try { + return await fs.realpath(current) + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT") throw error + // realpath reports ENOENT for an absent path and for a dangling symlink. A dangling + // symlink must still publish to its intended referent, otherwise the write replaces the + // link with a regular file and the link itself is lost. + // EINVAL means the path exists but is not a link; ENOENT means it is absent. + // Anything else (EACCES, EIO, ENOTDIR) is a real resolution failure and must not + // be mistaken for "absent" and written through the link path. + const linkTarget = await fs.readlink(current).catch((readlinkError: unknown) => { + const readlinkCode = + typeof readlinkError === "object" && readlinkError !== null && "code" in readlinkError + ? (readlinkError as { code?: string }).code + : undefined + if (readlinkCode !== "ENOENT" && readlinkCode !== "EINVAL") throw readlinkError + return null + }) + if (linkTarget === null) return current // genuinely absent, or not a link: create there + // Follow the WHOLE chain, not one hop. Stopping at the first referent lets a chain + // A -> B -> /outside/x slip past a confinement check drawn around B: the check sees an + // in-scope B, while a later resolution of B lands outside the scope and the commit + // rename writes there. + current = path.resolve(path.dirname(current), linkTarget) + } + } + throw new Error( + `resolvePublishTarget: exceeded ${MAX_SYMLINK_HOPS} symlink hops resolving ${absoluteFilePath}`, + ) +} + +/** + * On Windows fs.rename maps to MoveFileExW(..., MOVEFILE_REPLACE_EXISTING), which fails + * with a sharing error when another process - an indexer, an antivirus scan, an editor + * that opened the file without FILE_SHARE_DELETE - still holds the destination. Those + * handles are released on a millisecond scale, so the commit rename is retried a bounded + * number of times before the failure reaches the caller. POSIX renames do not fail this + * way, so the retry stays off there. + */ +const COMMIT_RENAME_ATTEMPTS = 4 +const COMMIT_RENAME_RETRY_DELAYS_MS = [25, 75, 150] + +function _isTransientRenameError(error: unknown): boolean { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + return code === "EPERM" || code === "EACCES" || code === "EBUSY" +} + +async function _renameWithRetry(source: string, destination: string, platform: string): Promise { + const attempts = platform === "win32" ? COMMIT_RENAME_ATTEMPTS : 1 + for (let attempt = 0; ; attempt++) { + try { + await fs.rename(source, destination) + return + } catch (error) { + if (attempt + 1 >= attempts || !_isTransientRenameError(error)) throw error + await new Promise((resolve) => setTimeout(resolve, COMMIT_RENAME_RETRY_DELAYS_MS[attempt])) + } + } +} + +/** + * Raised when the commit rename failed AND the backup could not be renamed back onto the + * target: the target is missing and the previous content survives only under the + * randomized backup path. The primary failure is preserved as originalError so callers + * keep the reason the publish failed while also learning where the saved state is. + */ +class RollbackFailedError extends Error { + readonly originalError: unknown + + constructor( + public readonly filePath: string, + public readonly backupPath: string, + rollbackError: unknown, + originalError: unknown, + ) { + super(_rollbackFailureMessage(filePath, backupPath, rollbackError, originalError), { + cause: rollbackError, + }) + this.name = "RollbackFailedError" + this.originalError = originalError + } +} + +function _rollbackFailureMessage( + filePath: string, + backupPath: string, + rollbackError: unknown, + originalError: unknown, +): string { + const primary = originalError instanceof Error ? originalError.message : String(originalError) + const rollback = rollbackError instanceof Error ? rollbackError.message : String(rollbackError) + return ( + `Publish to ${filePath} failed (${primary}) and the backup could not be restored (${rollback}). ` + + `The previous content is still at ${backupPath}.` + ) +} + +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. + // Tracked so the directory can be removed again once the write is finished. + const stagingDirPath = options?.tempPath === undefined ? _stagingDir(dirPath) : null + const tempPath = options?.tempPath ?? _tempName(stagingDirPath ?? dirPath, "safeWriteText") + + let backupPath: string | null = null + let releaseBackupOnSuccess = false + let daclDumpPath: string | null = null // tracked for cleanup in finally + + 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 + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch (statError) { + const statCode = + typeof statError === "object" && statError !== null && "code" in statError + ? (statError as { code?: string }).code + : undefined + // Only a genuinely missing target may take the default mode. Any other stat + // failure (EACCES, ELOOP, ENOTDIR) means the target exists but its real + // permissions are unknown from here: staging at 0o644 and renaming over it + // can widen an existing 0o600 file, so surface the error instead of guessing. + if (statCode !== "ENOENT") { + throw statError + } + } + const fd = fsSync.openSync(tempPath, "w", targetMode) + try { + // 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 + try { + targetMode = fsSync.statSync(targetPath).mode & 0o777 + } catch (statError: unknown) { + // Only a genuinely missing target may keep the temp's own mode. Any other stat + // failure (EACCES, ELOOP, ENOTDIR) means the target exists but its real + // permissions are unknown from here, and publishing the caller-staged temp + // as-is can widen a restrictive target (0o600 -> 0o644), so surface the error + // instead of guessing. + const statCode = + typeof statError === "object" && statError !== null && "code" in statError + ? (statError as { code?: string }).code + : undefined + if (statCode !== "ENOENT") { + throw statError + } + } + const fd = fsSync.openSync(tempPath, "r+") + try { + if (targetMode !== null) { + fsSync.fchmodSync(fd, targetMode) + } + _fsyncFile(fd) + } finally { + fsSync.closeSync(fd) + } + } + + // -- Step 2 (win32): save DACL BEFORE backup rename --------------- + const platform = options?.platform ?? process.platform + if (platform === "win32") { + try { + await fs.access(targetPath) // target exists? + daclDumpPath = _aclDumpName(targetPath) + const saved = await _saveDaclWindows(targetPath, daclDumpPath, options?.execFileRunner) + if (!saved) { + daclDumpPath = null // skip DACL handling entirely + } + } catch { + // target does not exist or access failed — no DACL handling + daclDumpPath = null + } + } + + try { + // -- Step 3 (backup:true): rename target -> backup -------------- + if (options?.backup) { + try { + await fs.access(targetPath) + backupPath = _tempName(dirPath, "safeWriteText.bak") + // The backup rename can hit the same sharing violation as the commit rename: the + // file being held open is the SOURCE here, and MoveFileExW fails the same way. + // ENOENT is not transient, so the "no target yet" handling below still applies. + await _renameWithRetry(targetPath, backupPath, platform) + releaseBackupOnSuccess = 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 --------------------- + await _renameWithRetry(tempPath, targetPath, platform) + + // -- 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 { + // best-effort: the content rename already committed + } + } + + // -- Step 5 (win32): restore DACL AFTER commit rename --------- + if (platform === "win32" && daclDumpPath !== null) { + const restoredDir = path.dirname(targetPath) + await _restoreDaclWindows(restoredDir, daclDumpPath, options?.execFileRunner) + } + + // -- Step 6 (backup:true): delete backup on success ----------- + if (releaseBackupOnSuccess && backupPath) { + try { + await fs.unlink(backupPath) + } catch { + // non-fatal — orphaned backup is acceptable + } + } + + // -- Step 7: remove the now-empty staging directory -------------- + if (stagingDirPath !== null) { + await _removeStagingDir(stagingDirPath) + } + } finally { + // Unlink DACL dump regardless of success/failure in this span. + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + } + + // tempPath is now the committed file; no cleanup needed. + } catch (originalError: unknown) { + // -- Rollback / cleanup on failure ---------------------------------- + let rollbackFailure: { backupPath: string; error: unknown } | null = null + if (backupPath && releaseBackupOnSuccess) { + try { + await fs.rename(backupPath, targetPath) + } catch (rollbackError: unknown) { + // The commit failed AND the restore failed: the target is missing and the only + // copy of the previous content is under the randomized backup name. Reporting + // only the primary rename error would leave the caller with no way to find that + // copy, so the rollback failure and the backup path are surfaced too. The + // original failure is kept as originalError (and in the message) so callers that + // branch on it do not lose it. + rollbackFailure = { backupPath, error: rollbackError } + } + } + + // Always clean up the staging temp file on failure. + try { + await fs.unlink(tempPath).catch(() => {}) + } catch { + // cleanup failure is non-fatal + } + + // And the staging directory itself, now that its only entry is gone. + if (stagingDirPath !== null) { + await _removeStagingDir(stagingDirPath) + } + + if (daclDumpPath !== null) { + await fs.unlink(daclDumpPath).catch(() => {}) + } + + if (rollbackFailure !== null) { + throw new RollbackFailedError(targetPath, rollbackFailure.backupPath, rollbackFailure.error, originalError) + } + + throw originalError + } +} diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 42786cfaa5..ccd6ec9d9a 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -39,6 +39,7 @@ import { fileExistsAtPath } from "../../utils/fs" import { TOKEN_EXPIRY_BUFFER_MS, OAUTH_FLOW_TIMEOUT_MS } from "./constants" import { SecretStorageService } from "./SecretStorageService" import { McpOAuthClientProvider } from "./McpOAuthClientProvider" +import { confinedWriteScope } from "./mcpWriteScope" import { arePathsEqual, getWorkspacePath } from "../../utils/path" import { injectVariables } from "../../utils/config" import { safeWriteJson } from "../../utils/safeWriteJson" @@ -621,6 +622,24 @@ export class McpHub { } // Get project-level MCP configuration path + /** + * Scope a settings write to the workspace when the write targets the project + * .roo/mcp.json. safeWriteJson resolves the publish target through symlinks so the + * lock, the merge read and the commit rename all key off the same file - which means + * a repository that plants that file as a link to somewhere else would otherwise + * receive the settings write at the linked path. + * + * The source decides, not path containment: the global settings file lives under the + * extension's global storage and is deliberately left unconfined, and a user who + * opens their home directory as the workspace has that storage inside the workspace + * root, so confining by path alone would reject a legitimate symlinked + * mcp_settings.json. + */ + private confineForSource(source: "global" | "project", configPath: string): string | undefined { + const workspaceRoot = path.resolve(this.providerRef.deref()?.cwd ?? getWorkspacePath()) + return confinedWriteScope(source, configPath, workspaceRoot) + } + private async getProjectMcpPath(): Promise { const workspacePath = this.providerRef.deref()?.cwd ?? getWorkspacePath() const projectMcpDir = path.join(workspacePath, ".roo") @@ -2091,7 +2110,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + confineTo: this.confineForSource(source, configPath), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { @@ -2176,7 +2198,10 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig, { prettyPrint: true }) + await safeWriteJson(configPath, updatedConfig, { + prettyPrint: true, + confineTo: this.confineForSource(serverSource, configPath), + }) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -2385,7 +2410,10 @@ export class McpHub { } this.isProgrammaticUpdate = true try { - await safeWriteJson(normalizedPath, config, { prettyPrint: true }) + await safeWriteJson(normalizedPath, config, { + prettyPrint: true, + confineTo: this.confineForSource(source, normalizedPath), + }) } finally { // Reset flag after watcher debounce period (non-blocking) this.flagResetTimer = setTimeout(() => { diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 441b0310e6..e0dd530616 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -9,6 +9,7 @@ import type { ClineProvider } from "../../../core/webview/ClineProvider" import type { McpHub as McpHubType, McpConnection, ConnectedMcpConnection, DisconnectedMcpConnection } from "../McpHub" import { ServerConfigSchema, McpHub } from "../McpHub" import { OAUTH_FLOW_TIMEOUT_MS } from "../constants" +import { ProjectScopeEscapeError } from "../mcpWriteScope" import { t } from "../../../i18n" type McpHubPrivate = { @@ -229,6 +230,23 @@ describe("McpHub", () => { } }) + it("confines a project settings write to the workspace and leaves a global write unconfined", () => { + // The wiring, not the predicate: mcpWriteScope.spec covers confinedWriteScope itself, + // this pins that McpHub hands safeWriteJson the workspace root for project writes and + // nothing at all for the global settings file (which may legitimately sit inside a + // home-directory workspace and must stay writable through a symlinked target). + const workspaceRoot = path.resolve("/test", "workspace") + const scopedHub = new McpHub({ ...mockProvider, cwd: workspaceRoot } as ClineProvider) + const projectPath = path.join(workspaceRoot, ".roo", "mcp.json") + expect(scopedHub["confineForSource"]("project", projectPath)).toBe(workspaceRoot) + const globalPath = path.join("/test/global-storage", "mcp_settings.json") + expect(scopedHub["confineForSource"]("global", globalPath)).toBeUndefined() + // A project file that resolves outside the workspace is refused, not silently + // unconfined: confineForSource raises instead of handing back undefined (which would + // lift safeWriteJson's confinement and write at the foreign target). + expect(() => scopedHub["confineForSource"]("project", path.resolve("/elsewhere", "mcp.json"))).toThrow(ProjectScopeEscapeError) + }) + it("should log settings watcher startup failures", async () => { const watcherError = new Error("watcher startup failed") const watchSpy = vi diff --git a/src/services/mcp/__tests__/mcpWriteScope.spec.ts b/src/services/mcp/__tests__/mcpWriteScope.spec.ts new file mode 100644 index 0000000000..98e7ca14e2 --- /dev/null +++ b/src/services/mcp/__tests__/mcpWriteScope.spec.ts @@ -0,0 +1,46 @@ +// npx vitest run services/mcp/__tests__/mcpWriteScope.spec.ts + +import * as path from "path" + +import { confinedWriteScope, ProjectScopeEscapeError } from "../mcpWriteScope" +describe("confinedWriteScope", () => { + const workspaceRoot = path.resolve("/home/user/project") + + it("confines a project settings file inside the workspace", () => { + const configPath = path.join(workspaceRoot, ".roo", "mcp.json") + + expect(confinedWriteScope("project", configPath, workspaceRoot)).toBe(workspaceRoot) + }) + + it("leaves a global settings file unconfined even when the workspace contains it", () => { + // The regression: a user who opens their home directory as the workspace has the + // extension's global storage under the workspace root. Deciding by path containment + // alone confined the global write, and a symlinked mcp_settings.json (a dotfiles + // checkout) then failed with ConfinedPathEscapeError on a toggle or a delete. + const globalConfig = path.join("/home/user", ".config", "Code", "User", "globalStorage", "roo", "mcp_settings.json") + + expect(confinedWriteScope("global", globalConfig, path.resolve("/home/user"))).toBeUndefined() + }) + + it("refuses a project settings file that resolves outside the workspace", () => { + const escaped = path.join(workspaceRoot, "..", "elsewhere", "mcp.json") + // Returning undefined here used to lift the confinement, so the merged settings + // landed at the foreign target - the escape this scope exists to close. + expect(() => confinedWriteScope("project", escaped, workspaceRoot)).toThrow(ProjectScopeEscapeError) + try { + confinedWriteScope("project", escaped, workspaceRoot) + } catch (error) { + expect(error).toBeInstanceOf(ProjectScopeEscapeError) + expect((error as ProjectScopeEscapeError).configPath).toBe(path.resolve(escaped)) + expect((error as ProjectScopeEscapeError).workspaceRoot).toBe(workspaceRoot) + } + }) + + it("refuses a sibling named with leading dots but confines a directory named with dots", () => { + // A sibling whose name starts with two dots is outside the workspace, so the write is + // refused; a directory NAMED "..x" INSIDE the workspace is not a traversal and stays + // confined (and writable). + expect(() => confinedWriteScope("project", path.join(workspaceRoot, "..", "..roo", "mcp.json"), workspaceRoot)).toThrow(ProjectScopeEscapeError) + expect(confinedWriteScope("project", path.join(workspaceRoot, "..x", "mcp.json"), workspaceRoot)).toBe(workspaceRoot) + }) +}) diff --git a/src/services/mcp/mcpWriteScope.ts b/src/services/mcp/mcpWriteScope.ts new file mode 100644 index 0000000000..3cf31addce --- /dev/null +++ b/src/services/mcp/mcpWriteScope.ts @@ -0,0 +1,56 @@ +import * as path from "path" + +/** + * Raised when a project-scoped MCP settings file resolves outside the workspace. + * The write is refused rather than allowed: a repository that plants .roo/mcp.json + * as a link to somewhere else must not receive the settings write there. + */ +export class ProjectScopeEscapeError extends Error { + readonly configPath: string + readonly workspaceRoot: string + constructor(configPath: string, workspaceRoot: string) { + super( + `Refusing to write the project MCP settings at ${configPath}: it resolves outside the ` + + `workspace (${workspaceRoot}). Remove the link or point the project settings file inside the workspace.`, + ) + this.name = "ProjectScopeEscapeError" + this.configPath = configPath + this.workspaceRoot = workspaceRoot + } +} + +/** + * Decide whether an MCP settings write is confined to the workspace. + * + * Only a project-scoped file is confined: the project .roo/mcp.json is content the + * repository controls, so a repository that plants it as a symlink elsewhere must not + * receive the write at the linked path. The global settings file lives under the + * extension's global storage and is deliberately left unconfined - and containment is + * not a substitute for the source, because a user who opens their home directory as the + * workspace has their global storage INSIDE the workspace root. Confining the global + * file by path alone would then reject a legitimate symlinked mcp_settings.json. + */ +export function confinedWriteScope( + source: "global" | "project", + configPath: string, + workspaceRoot: string, +): string | undefined { + if (source !== "project") { + return undefined + } + const root = path.resolve(workspaceRoot) + const relative = path.relative(root, path.resolve(configPath)) + // Separator-aware: a sibling entry whose name merely starts with two dots + // ("..roo/mcp.json") is outside the workspace, while a directory named "..x" + // inside it is not. Comparing the prefix without the separator gets that wrong. + const inside = relative !== "" && relative !== ".." && !relative.startsWith(".." + path.sep) && !path.isAbsolute(relative) + // A project write is confined to the workspace whether or not its path is inside it. + // Returning undefined for an escaping path used to LIFT the confinement, so the + // merged settings landed at the foreign target - the exact escape this scope exists + // to close. Refusing here names the offending path instead of relying on a generic + // confinement error from deeper inside the write. + if (!inside) { + throw new ProjectScopeEscapeError(path.resolve(configPath), root) + } + return root +} diff --git a/src/utils/__tests__/fileLock.spec.ts b/src/utils/__tests__/fileLock.spec.ts new file mode 100644 index 0000000000..9fd8a7e80c --- /dev/null +++ b/src/utils/__tests__/fileLock.spec.ts @@ -0,0 +1,84 @@ +// npx vitest run utils/__tests__/fileLock.spec.ts + +import * as path from "path" + +import * as lockfile from "proper-lockfile" + +import { acquireFileLock, withFileLock } from "../fileLock" + +vi.mock("fs/promises", () => ({ + realpath: vi.fn(async (target: unknown) => { + const asString = String(target) + // A path that does not exist yet reports ENOENT, as the real fs does. + if (asString.includes("missing")) { + throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }) + } + return asString.replace("aliasDir", "realDir") + }), +})) + +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(async () => async () => {}), +})) + +/** + * proper-lockfile derives its lock file from the path it is handed, and this module + * turns off the library's own realpath step because the file may not exist yet. Without + * a canonical lock key, a writer that reaches a file through a symlinked directory and a + * deleter that reaches the same file lexically take DIFFERENT locks and silently lose + * updates against each other - the shape a task file under a symlinked task directory + * has against a task-history delete. + */ +const STORE = path.resolve("/tmp/store") + +describe("fileLock - canonical lock keys", () => { + beforeEach(() => { + vi.mocked(lockfile.lock).mockClear() + }) + + test("locks the referent when the path runs through a symlinked directory", async () => { + const releaseLock = await acquireFileLock(path.join(STORE, "aliasDir", "task.json")) + + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "task.json"), + expect.objectContaining({ realpath: false }), + ) + + await releaseLock() + }) + + test("locks the referent but hands the operation the caller's own path", async () => { + // The mutex key is canonical; the path the caller works on is left alone. On Windows + // realpath can answer with the 8.3 short form, and rewriting the path a caller + // unlinks or compares would change behavior for every caller for no mutex gain. + const callerPath = path.join(STORE, "aliasDir", "task.json") + const seen: string[] = [] + await withFileLock(callerPath, async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }) + + expect(seen).toEqual([callerPath]) + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "task.json"), + expect.objectContaining({ realpath: false }), + ) + }) + + test("canonicalizes the nearest existing ancestor when the file does not exist yet", async () => { + // The file and its parent are absent, so realpath reports ENOENT for them; the deepest + // existing ancestor is resolved and the missing components are re-appended. + const seen: string[] = [] + await withFileLock( + path.join(STORE, "aliasDir", "missing", "new.json"), + async (absoluteFilePath) => { + seen.push(absoluteFilePath) + }, + ) + + expect(seen).toEqual([path.join(STORE, "aliasDir", "missing", "new.json")]) + expect(lockfile.lock).toHaveBeenCalledWith( + path.join(STORE, "realDir", "missing", "new.json"), + expect.objectContaining({ realpath: false }), + ) + }) +}) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index 79d08678a0..5cc1393bc6 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -3,7 +3,7 @@ import { Writable } from "stream" import * as path from "path" import * as os from "os" -import { safeWriteJson } from "../safeWriteJson" +import { ConfinedPathEscapeError, safeWriteJson } from "../safeWriteJson" // Capture actual implementations before the vi.mock factory runs, // so they are never wrapped by vi.fn() — avoids infinite recursion when @@ -102,6 +102,253 @@ describe("safeWriteJson", () => { } } + // Staging permissions + 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) + + expect(JSON.parse(await fs.readFile(target, "utf8"))).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("rejects a confined write whose target is outside the confined directory", async () => { + const scope = path.join(tempDir, "project") + await fs.mkdir(scope) + const outside = path.join(tempDir, "elsewhere.json") + + // No symlink needed: the check runs on the resolved publish target, so an + // out-of-scope path is rejected on every platform, and it is rejected before the + // lock is taken and before anything is staged. + await expect(safeWriteJson(outside, { mcpServers: {} }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + const left = await fs.readdir(tempDir) + expect(left).not.toContain("elsewhere.json") + expect(left.filter((entry) => entry.includes(".new_") || entry.endsWith(".lock"))).toEqual([]) + }) + + test.skipIf(process.platform === "win32")( + "rejects a confined write whose symlink resolves outside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project") + await fs.mkdir(projectDir) + const outside = path.join(tempDir, "outside.json") + await fsSyncActual.promises.writeFile(outside, JSON.stringify({ secret: "original" }), "utf8") + // A repository that plants its project settings file as a link to somewhere else + // must not receive the settings write at the linked path. The caller picked + // projectDir/mcp.json from the workspace, so it declares that scope. + const projectConfig = path.join(projectDir, "mcp.json") + await fs.symlink(outside, projectConfig) + + await expect( + safeWriteJson(projectConfig, { mcpServers: {} }, { confineTo: projectDir }), + ).rejects.toThrow(ConfinedPathEscapeError) + + // The linked file is untouched and nothing was staged beside it. + expect(JSON.parse(await fsSyncActual.promises.readFile(outside, "utf8"))).toEqual({ secret: "original" }) + const entries = await fs.readdir(tempDir) + expect(entries).toContain("outside.json") + expect( + entries.filter((entry) => entry.includes(".new_") || entry.includes("safeWriteText") || entry.endsWith(".lock")), + ).toEqual([]) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a write whose symlink referent stays inside the confined directory", + async () => { + const projectDir = path.join(tempDir, "project-in") + await fs.mkdir(projectDir) + const referent = path.join(projectDir, "real-mcp.json") + await fsSyncActual.promises.writeFile(referent, JSON.stringify({ mcpServers: {} }), "utf8") + const alias = path.join(projectDir, "mcp.json") + await fs.symlink(referent, alias) + + // Confining is about the scope, not about forbidding links: a link that stays + // inside the project still publishes to its referent. + await safeWriteJson(alias, { mcpServers: { local: { url: "http://localhost" } } }, { confineTo: projectDir }) + + expect(JSON.parse(await fsSyncActual.promises.readFile(referent, "utf8"))).toEqual({ + mcpServers: { local: { url: "http://localhost" } }, + }) + }, + ) + + test.skipIf(process.platform === "win32")( + "rejects a dangling symlink chain that leaves the confined directory", + async () => { + const scope = path.join(tempDir, "scope-chain") + await fs.mkdir(scope) + // A -> B -> /outside-chain.json. B is inside the scope, the final referent + // is not, and nothing in the chain exists yet, so realpath reports ENOENT for every + // hop. Following only A -> B would confine-check an in-scope path and then publish + // outside it. + const link = path.join(scope, "mcp.json") + const middle = path.join(scope, "mid.json") + const outside = path.join(tempDir, "outside-chain.json") + await fs.symlink("mid.json", link) + await fs.symlink(outside, middle) + + await expect(safeWriteJson(link, { mcpServers: {} }, { confineTo: scope })).rejects.toThrow( + ConfinedPathEscapeError, + ) + + expect(await fileExists(outside)).toBe(false) + expect(await fileExists(middle)).toBe(false) + expect(await fileExists(link)).toBe(false) + }, + ) + + test.skipIf(process.platform === "win32")( + "confines a scope path that itself runs through a symlink and does not exist yet", + async () => { + const real = path.join(tempDir, "real-project") + await fs.mkdir(real) + const alias = path.join(tempDir, "alias-project") + await fs.symlink(real, alias) + // The scope is declared through the alias, and the directory it names does not + // exist yet. Resolving it lexically would compare an unresolved scope against a + // fully resolved target and reject a write that is in fact inside the project - + // the macOS /var -> /private/var shape. The nearest existing ancestor is resolved + // and the remainder re-joined instead. + const nested = path.join(alias, "nested") + const target = path.join(nested, "mcp.json") + + await safeWriteJson(target, { mcpServers: {} }, { confineTo: nested }) + + expect( + JSON.parse(await fsSyncActual.promises.readFile(path.join(real, "nested", "mcp.json"), "utf8")), + ).toEqual({ mcpServers: {} }) + }, + ) + + test.skipIf(process.platform === "win32")( + "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. @@ -312,9 +559,8 @@ 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 + // Test for best-effort backup deletion (the backup lifecycle now lives in safeWriteText) + test("does not fail the write when backup deletion fails (orphaned backup is acceptable)", async () => { const initialData = { message: "Initial" } const newData = { message: "New" } @@ -322,18 +568,23 @@ describe("safeWriteJson", () => { // 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_")) { + if (filePath.toString().includes("safeWriteText.bak_")) { throw new Error("Backup deletion failed") } return fsPromisesActuals.unlink!(filePath) }) + // The write must still succeed: backup cleanup is best-effort inside + // safeWriteText and never masks the committed content. await safeWriteJson(currentTestFilePath, newData) - // Verify console.error was called with the expected message - expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Successfully wrote"), expect.any(Error)) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual(newData) + + // The orphaned backup is still on disk because its deletion failed. + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) - consoleErrorSpy.mockRestore() vi.mocked(fs.unlink).mockRestore() }) @@ -434,9 +685,9 @@ 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" } + // Test for rollback failure scenario (the rollback rename now lives in safeWriteText) + test("re-throws the original error when the rollback rename fails, leaving an orphaned backup", async () => { + const initialData = { message: "Initial, orphaned when rollback fails" } const newData = { message: "New content" } await fsPromisesActuals.writeFile!(currentTestFilePath, JSON.stringify(initialData)) @@ -451,20 +702,34 @@ describe("safeWriteJson", () => { // Second call: tempNewFilePath -> filePath (fail) throw new Error("Primary rename failed") } else if (renameCallCount === 3) { - // Third call: tempBackupFilePath -> filePath (rollback, also fail) + // Third call: backup -> filePath (rollback, also fail) throw new Error("Rollback rename failed") } return fsPromisesActuals.rename!(oldPath, newPath) }) - // Should throw the original error, not the rollback 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" }), - ) + // The original failure must still be the thing the caller can read, even though + // the rollback failure is what gets thrown on top of it. + const rejection = await safeWriteJson(currentTestFilePath, newData).then(() => null, (error) => error) + expect(rejection).toBeInstanceOf(Error) + expect(rejection.name).toBe("RollbackFailedError") + expect(rejection.message).toContain("Primary rename failed") + + // Partial failure has to be actionable: the caller learns the previous content is + // recoverable and where it is, instead of only that a rename failed. + expect(rejection.backupPath).toMatch(/safeWriteText\.bak_/) + expect(String(rejection.message)).toContain("previous content is still at") + expect(rejection.originalError).toBeInstanceOf(Error) + expect((rejection.originalError as Error).message).toBe("Primary rename failed") + expect(rejection.cause).toBeInstanceOf(Error) + expect((rejection.cause as Error).message).toBe("Rollback rename failed") + + // The rollback failed inside safeWriteText, so the target is gone and + // the backup is orphaned on disk - and it is the file the error points at. + expect(await fileExists(currentTestFilePath)).toBe(false) + const entries = await fs.readdir(tempDir) + expect(entries.some((entry) => entry.includes("safeWriteText.bak_"))).toBe(true) + expect(await fileExists(rejection.backupPath)).toBe(true) consoleErrorSpy.mockRestore() }) @@ -542,4 +807,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/fileLock.ts b/src/utils/fileLock.ts index 9f7cad7653..6f63c157a5 100644 --- a/src/utils/fileLock.ts +++ b/src/utils/fileLock.ts @@ -1,5 +1,6 @@ import * as path from "path" import * as lockfile from "proper-lockfile" +import * as fs from "fs/promises" /** * Shared staleness window for per-file advisory locks. This module owns the @@ -8,6 +9,43 @@ import * as lockfile from "proper-lockfile" */ export const LOCK_STALE_MS = 31_000 +/** + * Canonical lock key for a path. + * + * proper-lockfile derives its lock file from the path it is handed, and this module + * turns off the library's own realpath step because the file may not exist yet. Two + * callers that reach the same file by different routes - one lexical, one through a + * symlinked directory - would then take DIFFERENT locks and silently lose updates + * against each other, which is exactly how a task file written through a symlinked + * task directory ends up unlocked against a task-history delete. + * + * The file itself may be absent (a create), so the nearest EXISTING ancestor is + * canonicalized and the missing components are re-appended. If nothing can be + * canonicalized the lexical absolute path is kept: lock keys stay stable and the + * write's own resolution still decides where content lands. + */ +async function canonicalLockPath(filePath: string): Promise { + const absoluteFilePath = path.resolve(filePath) + const missing: string[] = [] + let cursor = absoluteFilePath + for (;;) { + try { + const realPath = await fs.realpath(cursor) + return missing.length > 0 ? path.join(realPath, ...missing.reverse()) : realPath + } catch (error) { + const code = + typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined + if (code !== "ENOENT" && code !== "ENOTDIR") return absoluteFilePath + const parent = path.dirname(cursor) + if (parent === cursor) return absoluteFilePath + missing.push(path.basename(cursor)) + cursor = parent + } + } +} + /** * Acquire the advisory lock for one file path using the exact protocol * `safeWriteJson` uses, so operations that hold this lock serialize with @@ -16,7 +54,7 @@ export const LOCK_STALE_MS = 31_000 * while holding it. */ export async function acquireFileLock(filePath: string): Promise<() => Promise> { - const absoluteFilePath = path.resolve(filePath) + const absoluteFilePath = await canonicalLockPath(filePath) try { return await lockfile.lock(absoluteFilePath, { stale: LOCK_STALE_MS, @@ -50,19 +88,25 @@ export async function withFileLock( filePath: string, operation: (absoluteFilePath: string) => Promise, ): Promise { - const absoluteFilePath = path.resolve(filePath) - const releaseLock = await acquireFileLock(absoluteFilePath) + // The LOCK key is canonical, so a caller that reaches the file through a symlinked + // directory and one that reaches it lexically contend for the same lock file. The + // operation still receives the caller's own absolute path: on Windows realpath can + // answer with the 8.3 short form (C:\Users\RUNNER~1\... for a temp dir under + // C:\Users\runneradmin\...), and rewriting the path a caller unlinks or compares + // would change behavior for every caller while adding nothing to the mutex. + const operationPath = path.resolve(filePath) + const releaseLock = await acquireFileLock(operationPath) let result: T try { - result = await operation(absoluteFilePath) + result = await operation(operationPath) } catch (operationError) { // The operation error is the primary failure. Release without // reporting a secondary release error over it. try { await releaseLock() } catch (releaseError) { - console.error(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } throw operationError } @@ -72,7 +116,7 @@ export async function withFileLock( } catch (releaseError) { // The operation already succeeded, so a release failure is only // logged, matching how `safeWriteJson` handles release failures. - console.error(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + console.error(`Failed to release lock for ${operationPath}:`, releaseError) } return result } diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 7da68b2a7a..ce362e6d44 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -3,6 +3,10 @@ import * as fsSync from "fs" import * as path from "path" import { JsonStreamStringify } from "json-stream-stringify" + +import { resolvePublishTarget, safeWriteText, type SafeWriteTextOptions } from "../services/file-safety/safeWriteText" + + import { acquireFileLock } from "./fileLock" /** @@ -26,13 +30,90 @@ export interface SafeWriteJsonOptions { * cannot be parsed. */ merge?: (existing: unknown, incoming: unknown) => unknown + + /** + * Confine the write to this directory. The publish target is resolved through + * symlinks (so the lock, the merge read and the commit rename all key off the same + * file), which means a caller that picked its path from a project directory can be + * redirected outside it by a link the repository planted. When this option is set, + * a resolved target that leaves the directory is rejected instead of written. + */ + confineTo?: string +} + +/** + * Error raised when a confined write resolves outside the directory it was confined + * to. The requested path is reported alongside the resolved one so the caller can + * tell a planted link from a plain wrong path. + */ +export class ConfinedPathEscapeError extends Error { + readonly requestedPath: string + readonly resolvedPath: string + readonly confineTo: string + + constructor(requestedPath: string, resolvedPath: string, confineTo: string) { + super( + `Refusing to write ${requestedPath}: it resolves to ${resolvedPath}, which is outside the confined directory ${confineTo}`, + ) + this.name = "ConfinedPathEscapeError" + this.requestedPath = requestedPath + this.resolvedPath = resolvedPath + this.confineTo = confineTo + } +} + +/** + * Canonicalize the directory a write is confined to. The publish target is fully + * resolved through symlinks, so the scope has to be resolved the same way or a + * scope path that itself runs through a symlink (macOS /var -> /private/var is the + * common case) would compare lexically against a resolved target and reject every + * legitimate in-scope write. When the scope does not exist yet, the nearest + * existing ancestor is resolved and the remainder re-appended. + */ +async function _resolveScopeRoot(confineTo: string): Promise { + const lexical = path.resolve(confineTo) + try { + return await fs.realpath(lexical) + } catch (error: unknown) { + // Only a missing path means "walk up and re-join". EACCES or ELOOP means the + // scope cannot be canonicalized at all, and continuing would build a partly + // lexical root that can disagree with the canonical target - the failure has to + // surface rather than decide the scope from a guess. + if (_scopeErrorCode(error) !== "ENOENT") { + throw error + } + const missing: string[] = [] + let ancestor = lexical + while (true) { + const parent = path.dirname(ancestor) + if (parent === ancestor) { + return lexical + } + missing.push(path.basename(ancestor)) + ancestor = parent + try { + const real = await fs.realpath(ancestor) + return path.join(real, ...missing.reverse()) + } catch (innerError: unknown) { + if (_scopeErrorCode(innerError) !== "ENOENT") { + throw innerError + } + } + } + } +} + +function _scopeErrorCode(error: unknown): string | undefined { + return typeof error === "object" && error !== null && "code" in error + ? (error as { code?: string }).code + : undefined } /** * 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,26 +123,47 @@ 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) + + // Scope check after resolution and before the lock: this is where a link that + // leaves the caller's directory becomes visible, and rejecting here means no lock, + // no staging file and no publish for an out-of-scope target. + if (options?.confineTo) { + const scopeRoot = await _resolveScopeRoot(options.confineTo) + // Both sides have to be canonicalized the same way. The publish target is resolved + // through a symlink when it exists, but a target that does not exist yet keeps the + // alias components of the path it was given, so comparing it against a resolved + // scope would reject a write that is inside the scope. + const resolvedTarget = await _resolveScopeRoot(canonicalPath) + const relative = path.relative(scopeRoot, resolvedTarget) + if (relative === "" || relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative)) { + throw new ConfinedPathEscapeError(absoluteFilePath, canonicalPath, scopeRoot) + } + } + // 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) throw dirError } + // Acquire the lock before any file operations. `acquireFileLock` owns the // shared advisory lock protocol, so callers that lock the same path with // it (for example task-history deletion) serialize with this write. @@ -69,11 +171,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 +183,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 +194,63 @@ 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 reached through + // a symlink that lives in a world-readable 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 normal 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 } - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) - - // 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 + await _streamDataToFile(actualTempNewFilePath, data, options?.prettyPrint, stagingMode) - // 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, - ) - } + // Step 2: Delegate backup + commit + rollback to safeWriteText with the + // pre-written temp path. backup:true keeps the old safeWriteJson + // semantics (target -> backup before commit, rollback on failure) and + // keeps the target in place until safeWriteText captures its Windows + // DACL (safeWriteText dumps the DACL before its own backup rename and + // restores it onto the directory after the commit rename). + const textOptions: SafeWriteTextOptions = { + tempPath: actualTempNewFilePath, + backup: true, } + + await safeWriteText(canonicalPath, "", textOptions) + + // If we reach here, the new file is successfully in place and any + // backup has already been handled by safeWriteText. + actualTempNewFilePath = null } 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, - ) - } - } - // Cleanup the .new file if it exists + // A failed safeWriteText already rolled the backup (if any) back to + // the target path. Clean up the .new 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 +262,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 +280,16 @@ 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