From 8a8483e811aeafe19920b24a866248122a79c760 Mon Sep 17 00:00:00 2001 From: ooadcsulb22 Date: Fri, 6 Nov 2026 23:14:00 -0800 Subject: [PATCH 1/2] fix(tools): report isOutsideWorkspace in apply_diff approvals apply_diff did not set isOutsideWorkspace in its approval payload. Auto-approval read the missing value as inside the workspace, so with alwaysAllowWrite on it approved diffs to files outside the workspace even when alwaysAllowWriteOutsideWorkspace was off. Compute the flag in execute and handlePartial, as write_to_file and edit already do. Closes #2249 Co-authored-by: Cursor --- src/core/tools/ApplyDiffTool.ts | 7 + .../applyDiffTool.failureReporting.spec.ts | 4 + .../applyDiffTool.outsideWorkspace.spec.ts | 196 ++++++++++++++++++ 3 files changed, 207 insertions(+) create mode 100644 src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 4011f802e9..b08ef9b6e6 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -5,6 +5,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { getReadablePath } from "../../utils/path" +import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Task } from "../task/Task" import { formatResponse } from "../prompts/responses" import { fileExistsAtPath } from "../../utils/fs" @@ -138,11 +139,13 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { // Check if file is write-protected const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false + const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath) const sharedMessageProps: ClineSayTool = { tool: "appliedDiff", path: getReadablePath(task.cwd, relPath), diff: diffContent, + isOutsideWorkspace, } if (isPreventFocusDisruptionEnabled) { @@ -332,10 +335,14 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { return } + // relPath is guaranteed non-null after hasPathStabilized + const absolutePath = path.resolve(task.cwd, relPath!) + const sharedMessageProps: ClineSayTool = { tool: "appliedDiff", path: getReadablePath(task.cwd, relPath), diff: diffContent, + isOutsideWorkspace: isPathOutsideWorkspace(absolutePath), } let toolProgressStatus diff --git a/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts b/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts index bf7b3f5351..7c372645d6 100644 --- a/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts +++ b/src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts @@ -26,6 +26,10 @@ vi.mock("../../../utils/path", () => ({ getReadablePath: vi.fn().mockReturnValue("test/file.txt"), })) +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn().mockReturnValue(false), +})) + vi.mock("../../prompts/responses", () => ({ formatResponse: { toolError: vi.fn((message: string) => `Error: ${message}`), diff --git a/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts b/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts new file mode 100644 index 0000000000..edea6986a3 --- /dev/null +++ b/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts @@ -0,0 +1,196 @@ +// npx vitest run core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts + +import fs from "fs/promises" +import path from "path" + +import type { ClineSayTool } from "@roo-code/types" + +import type { ToolUse } from "../../../shared/tools" +import { isPathOutsideWorkspace } from "../../../utils/pathUtils" +import { checkAutoApproval } from "../../auto-approval" +import { baseState } from "../../auto-approval/__tests__/fixtures" +import type { Task } from "../../task/Task" +import type { ToolCallbacks } from "../BaseTool" +import { applyDiffTool } from "../ApplyDiffTool" + +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { instance: { captureDiffApplicationError: vi.fn() } }, +})) + +vi.mock("fs/promises", () => ({ + default: { + readFile: vi.fn(), + }, +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockResolvedValue(true), +})) + +vi.mock("../../../utils/path", () => ({ + getReadablePath: vi.fn((_cwd: string, relPath?: string) => relPath ?? ""), +})) + +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn(), +})) + +vi.mock("../../prompts/responses", () => ({ + formatResponse: { + toolError: vi.fn((message: string) => `Error: ${message}`), + rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`), + createPrettyPatch: vi.fn(() => "mock-diff"), + }, +})) + +vi.mock("../../diff/stats", () => ({ + sanitizeUnifiedDiff: vi.fn((diff: string) => diff), + computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })), +})) + +vi.mock("vscode", () => ({ + window: { showWarningMessage: vi.fn() }, + env: { openExternal: vi.fn() }, + Uri: { parse: vi.fn() }, +})) + +const CWD = "/workspace" +const INSIDE_PATH = "src/index.ts" +const OUTSIDE_PATH = "/home/me/.zshrc" +const DIFF = "<<<<<<< SEARCH\nold\n=======\nnew\n>>>>>>> REPLACE" + +const cases = [ + { name: "a file outside the workspace", relPath: OUTSIDE_PATH, isOutside: true }, + { name: "a file inside the workspace", relPath: INSIDE_PATH, isOutside: false }, +] + +const approvalPaths = [ + { name: "diff view", preventFocusDisruption: false }, + { name: "preventFocusDisruption", preventFocusDisruption: true }, +] + +describe("ApplyDiffTool isOutsideWorkspace", () => { + let askApproval: ReturnType> + + function createTask(preventFocusDisruption = false): Task { + const task = { + cwd: CWD, + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + didEditFile: false, + api: { getModel: () => ({ id: "claude-test" }) }, + rooIgnoreController: { validateAccess: vi.fn().mockReturnValue(true) }, + rooProtectedController: { isWriteProtected: vi.fn().mockReturnValue(false) }, + providerRef: { + deref: vi.fn().mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + diagnosticsEnabled: false, + writeDelayMs: 0, + experiments: { preventFocusDisruption }, + }), + }), + }, + diffStrategy: { + getName: () => "test", + applyDiff: vi.fn().mockResolvedValue({ success: true, content: "new\n" }), + }, + diffViewProvider: { + editType: undefined, + originalContent: "", + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + reset: vi.fn().mockResolvedValue(undefined), + revertChanges: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue(undefined), + saveDirectly: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + pushToolWriteResult: vi.fn().mockResolvedValue("Changes successfully applied."), + }, + fileContextTracker: { trackFileContext: vi.fn().mockResolvedValue(undefined) }, + say: vi.fn().mockResolvedValue(undefined), + ask: vi.fn().mockResolvedValue(undefined), + recordToolError: vi.fn(), + processQueuedMessages: vi.fn(), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"), + } + // The double provides only the Task members that ApplyDiffTool.execute and handlePartial read. + return task as unknown as Task + } + + async function runExecute(relPath: string, isOutside: boolean, preventFocusDisruption = false): Promise { + vi.mocked(isPathOutsideWorkspace).mockReturnValue(isOutside) + + await applyDiffTool.execute({ path: relPath, diff: DIFF }, createTask(preventFocusDisruption), { + askApproval, + handleError: vi.fn().mockResolvedValue(undefined), + pushToolResult: vi.fn(), + toolCallId: "tool-call-1", + }) + + expect(askApproval).toHaveBeenCalledTimes(1) + return String(askApproval.mock.calls[0][1]) + } + + beforeEach(() => { + vi.clearAllMocks() + applyDiffTool.resetPartialState() + vi.mocked(fs.readFile).mockResolvedValue("old\n") + askApproval = vi.fn().mockResolvedValue(true) + }) + + describe.each(approvalPaths)("approval payload ($name)", ({ preventFocusDisruption }) => { + it.each(cases)("reports isOutsideWorkspace for $name", async ({ relPath, isOutside }) => { + const payload = JSON.parse(await runExecute(relPath, isOutside, preventFocusDisruption)) as ClineSayTool + + expect(isPathOutsideWorkspace).toHaveBeenCalledWith(path.resolve(CWD, relPath)) + expect(payload.tool).toBe("appliedDiff") + expect(payload.isOutsideWorkspace).toBe(isOutside) + }) + }) + + it.each(cases)("reports isOutsideWorkspace in the partial preview for $name", async ({ relPath, isOutside }) => { + vi.mocked(isPathOutsideWorkspace).mockReturnValue(isOutside) + const task = createTask() + const block: ToolUse<"apply_diff"> = { + type: "tool_use", + name: "apply_diff", + params: { path: relPath, diff: DIFF }, + partial: true, + } + + // The first call records the path; the second call sees it stable and asks. + await applyDiffTool.handlePartial(task, block) + await applyDiffTool.handlePartial(task, block) + + expect(task.ask).toHaveBeenCalledTimes(1) + const payload = JSON.parse(String(vi.mocked(task.ask).mock.calls[0][1])) as ClineSayTool + expect(isPathOutsideWorkspace).toHaveBeenCalledWith(path.resolve(CWD, relPath)) + expect(payload.isOutsideWorkspace).toBe(isOutside) + }) + + describe("auto-approval decision with alwaysAllowWrite on", () => { + it.each([ + { ...cases[0], allowOutside: false, expected: "ask" }, + { ...cases[0], allowOutside: true, expected: "approve" }, + { ...cases[1], allowOutside: false, expected: "approve" }, + ])( + "returns $expected for $name when alwaysAllowWriteOutsideWorkspace is $allowOutside", + async ({ relPath, isOutside, allowOutside, expected }) => { + const text = await runExecute(relPath, isOutside) + + const result = await checkAutoApproval({ + state: { + ...baseState, + alwaysAllowWrite: true, + alwaysAllowWriteOutsideWorkspace: allowOutside, + }, + cwd: CWD, + ask: "tool", + text, + }) + + expect(result).toEqual({ decision: expected }) + }, + ) + }) +}) From a4b201489bdd02c013ffbed5d82c5a7fd9b1779f Mon Sep 17 00:00:00 2001 From: ooadcsulb22 Date: Fri, 6 Nov 2026 23:32:45 -0800 Subject: [PATCH 2/2] test(tools): cover apply_diff ../ traversal with the real workspace check Run isPathOutsideWorkspace against a mocked workspace folder. A ../ path that resolves outside the workspace sets isOutsideWorkspace and asks for approval. A path inside the workspace is still auto-approved. Co-authored-by: Cursor --- .../applyDiffTool.outsideWorkspace.spec.ts | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts b/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts index edea6986a3..cbaa2433ca 100644 --- a/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts +++ b/src/core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts @@ -52,6 +52,11 @@ vi.mock("vscode", () => ({ window: { showWarningMessage: vi.fn() }, env: { openExternal: vi.fn() }, Uri: { parse: vi.fn() }, + workspace: { + get workspaceFolders() { + return [{ uri: { fsPath: path.resolve(CWD) } }] + }, + }, })) const CWD = "/workspace" @@ -133,6 +138,7 @@ describe("ApplyDiffTool isOutsideWorkspace", () => { beforeEach(() => { vi.clearAllMocks() + vi.mocked(isPathOutsideWorkspace).mockReset() applyDiffTool.resetPartialState() vi.mocked(fs.readFile).mockResolvedValue("old\n") askApproval = vi.fn().mockResolvedValue(true) @@ -168,6 +174,40 @@ describe("ApplyDiffTool isOutsideWorkspace", () => { expect(payload.isOutsideWorkspace).toBe(isOutside) }) + describe("with the real workspace check", () => { + it.each([ + { + name: "a ../ path that resolves outside", + relPath: "../outside/config.json", + isOutside: true, + expected: "ask", + }, + { name: "a path inside", relPath: "src/index.ts", isOutside: false, expected: "approve" }, + ])("marks $name the workspace and returns $expected", async ({ relPath, isOutside, expected }) => { + const actual = await vi.importActual("../../../utils/pathUtils") + vi.mocked(isPathOutsideWorkspace).mockImplementation(actual.isPathOutsideWorkspace) + + await applyDiffTool.execute({ path: relPath, diff: DIFF }, createTask(), { + askApproval, + handleError: vi.fn().mockResolvedValue(undefined), + pushToolResult: vi.fn(), + toolCallId: "tool-call-1", + }) + + const text = String(askApproval.mock.calls[0][1]) + const payload = JSON.parse(text) as ClineSayTool + expect(payload.isOutsideWorkspace).toBe(isOutside) + + const result = await checkAutoApproval({ + state: { ...baseState, alwaysAllowWrite: true }, + cwd: CWD, + ask: "tool", + text, + }) + expect(result).toEqual({ decision: expected }) + }) + }) + describe("auto-approval decision with alwaysAllowWrite on", () => { it.each([ { ...cases[0], allowOutside: false, expected: "ask" },