Skip to content

Commit 1b9cb7d

Browse files
kanchinadd98ooadcsulb22cursoragent
authored
[Fix] apply_diff auto-approves edits outside the workspace (#2343)
* 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 <cursoragent@cursor.com> * 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 <cursoragent@cursor.com> --------- Co-authored-by: ooadcsulb22 <kanchinadd98@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 5319fe9 commit 1b9cb7d

3 files changed

Lines changed: 247 additions & 0 deletions

File tree

‎src/core/tools/ApplyDiffTool.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
55
import { TelemetryService } from "@roo-code/telemetry"
66

77
import { getReadablePath } from "../../utils/path"
8+
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
89
import { Task } from "../task/Task"
910
import { formatResponse } from "../prompts/responses"
1011
import { fileExistsAtPath } from "../../utils/fs"
@@ -138,11 +139,13 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
138139

139140
// Check if file is write-protected
140141
const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath) || false
142+
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
141143

142144
const sharedMessageProps: ClineSayTool = {
143145
tool: "appliedDiff",
144146
path: getReadablePath(task.cwd, relPath),
145147
diff: diffContent,
148+
isOutsideWorkspace,
146149
}
147150

148151
if (isPreventFocusDisruptionEnabled) {
@@ -332,10 +335,14 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> {
332335
return
333336
}
334337

338+
// relPath is guaranteed non-null after hasPathStabilized
339+
const absolutePath = path.resolve(task.cwd, relPath!)
340+
335341
const sharedMessageProps: ClineSayTool = {
336342
tool: "appliedDiff",
337343
path: getReadablePath(task.cwd, relPath),
338344
diff: diffContent,
345+
isOutsideWorkspace: isPathOutsideWorkspace(absolutePath),
339346
}
340347

341348
let toolProgressStatus

‎src/core/tools/__tests__/applyDiffTool.failureReporting.spec.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ vi.mock("../../../utils/path", () => ({
2626
getReadablePath: vi.fn().mockReturnValue("test/file.txt"),
2727
}))
2828

29+
vi.mock("../../../utils/pathUtils", () => ({
30+
isPathOutsideWorkspace: vi.fn().mockReturnValue(false),
31+
}))
32+
2933
vi.mock("../../prompts/responses", () => ({
3034
formatResponse: {
3135
toolError: vi.fn((message: string) => `Error: ${message}`),
Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
// npx vitest run core/tools/__tests__/applyDiffTool.outsideWorkspace.spec.ts
2+
3+
import fs from "fs/promises"
4+
import path from "path"
5+
6+
import type { ClineSayTool } from "@roo-code/types"
7+
8+
import type { ToolUse } from "../../../shared/tools"
9+
import { isPathOutsideWorkspace } from "../../../utils/pathUtils"
10+
import { checkAutoApproval } from "../../auto-approval"
11+
import { baseState } from "../../auto-approval/__tests__/fixtures"
12+
import type { Task } from "../../task/Task"
13+
import type { ToolCallbacks } from "../BaseTool"
14+
import { applyDiffTool } from "../ApplyDiffTool"
15+
16+
vi.mock("@roo-code/telemetry", () => ({
17+
TelemetryService: { instance: { captureDiffApplicationError: vi.fn() } },
18+
}))
19+
20+
vi.mock("fs/promises", () => ({
21+
default: {
22+
readFile: vi.fn(),
23+
},
24+
}))
25+
26+
vi.mock("../../../utils/fs", () => ({
27+
fileExistsAtPath: vi.fn().mockResolvedValue(true),
28+
}))
29+
30+
vi.mock("../../../utils/path", () => ({
31+
getReadablePath: vi.fn((_cwd: string, relPath?: string) => relPath ?? ""),
32+
}))
33+
34+
vi.mock("../../../utils/pathUtils", () => ({
35+
isPathOutsideWorkspace: vi.fn(),
36+
}))
37+
38+
vi.mock("../../prompts/responses", () => ({
39+
formatResponse: {
40+
toolError: vi.fn((message: string) => `Error: ${message}`),
41+
rooIgnoreError: vi.fn((filePath: string) => `Access denied: ${filePath}`),
42+
createPrettyPatch: vi.fn(() => "mock-diff"),
43+
},
44+
}))
45+
46+
vi.mock("../../diff/stats", () => ({
47+
sanitizeUnifiedDiff: vi.fn((diff: string) => diff),
48+
computeDiffStats: vi.fn(() => ({ additions: 1, deletions: 1 })),
49+
}))
50+
51+
vi.mock("vscode", () => ({
52+
window: { showWarningMessage: vi.fn() },
53+
env: { openExternal: vi.fn() },
54+
Uri: { parse: vi.fn() },
55+
workspace: {
56+
get workspaceFolders() {
57+
return [{ uri: { fsPath: path.resolve(CWD) } }]
58+
},
59+
},
60+
}))
61+
62+
const CWD = "/workspace"
63+
const INSIDE_PATH = "src/index.ts"
64+
const OUTSIDE_PATH = "/home/me/.zshrc"
65+
const DIFF = "<<<<<<< SEARCH\nold\n=======\nnew\n>>>>>>> REPLACE"
66+
67+
const cases = [
68+
{ name: "a file outside the workspace", relPath: OUTSIDE_PATH, isOutside: true },
69+
{ name: "a file inside the workspace", relPath: INSIDE_PATH, isOutside: false },
70+
]
71+
72+
const approvalPaths = [
73+
{ name: "diff view", preventFocusDisruption: false },
74+
{ name: "preventFocusDisruption", preventFocusDisruption: true },
75+
]
76+
77+
describe("ApplyDiffTool isOutsideWorkspace", () => {
78+
let askApproval: ReturnType<typeof vi.fn<ToolCallbacks["askApproval"]>>
79+
80+
function createTask(preventFocusDisruption = false): Task {
81+
const task = {
82+
cwd: CWD,
83+
consecutiveMistakeCount: 0,
84+
consecutiveMistakeCountForApplyDiff: new Map<string, number>(),
85+
didEditFile: false,
86+
api: { getModel: () => ({ id: "claude-test" }) },
87+
rooIgnoreController: { validateAccess: vi.fn().mockReturnValue(true) },
88+
rooProtectedController: { isWriteProtected: vi.fn().mockReturnValue(false) },
89+
providerRef: {
90+
deref: vi.fn().mockReturnValue({
91+
getState: vi.fn().mockResolvedValue({
92+
diagnosticsEnabled: false,
93+
writeDelayMs: 0,
94+
experiments: { preventFocusDisruption },
95+
}),
96+
}),
97+
},
98+
diffStrategy: {
99+
getName: () => "test",
100+
applyDiff: vi.fn().mockResolvedValue({ success: true, content: "new\n" }),
101+
},
102+
diffViewProvider: {
103+
editType: undefined,
104+
originalContent: "",
105+
open: vi.fn().mockResolvedValue(undefined),
106+
update: vi.fn().mockResolvedValue(undefined),
107+
reset: vi.fn().mockResolvedValue(undefined),
108+
revertChanges: vi.fn().mockResolvedValue(undefined),
109+
saveChanges: vi.fn().mockResolvedValue(undefined),
110+
saveDirectly: vi.fn().mockResolvedValue(undefined),
111+
scrollToFirstDiff: vi.fn(),
112+
pushToolWriteResult: vi.fn().mockResolvedValue("Changes successfully applied."),
113+
},
114+
fileContextTracker: { trackFileContext: vi.fn().mockResolvedValue(undefined) },
115+
say: vi.fn().mockResolvedValue(undefined),
116+
ask: vi.fn().mockResolvedValue(undefined),
117+
recordToolError: vi.fn(),
118+
processQueuedMessages: vi.fn(),
119+
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing param error"),
120+
}
121+
// The double provides only the Task members that ApplyDiffTool.execute and handlePartial read.
122+
return task as unknown as Task
123+
}
124+
125+
async function runExecute(relPath: string, isOutside: boolean, preventFocusDisruption = false): Promise<string> {
126+
vi.mocked(isPathOutsideWorkspace).mockReturnValue(isOutside)
127+
128+
await applyDiffTool.execute({ path: relPath, diff: DIFF }, createTask(preventFocusDisruption), {
129+
askApproval,
130+
handleError: vi.fn().mockResolvedValue(undefined),
131+
pushToolResult: vi.fn(),
132+
toolCallId: "tool-call-1",
133+
})
134+
135+
expect(askApproval).toHaveBeenCalledTimes(1)
136+
return String(askApproval.mock.calls[0][1])
137+
}
138+
139+
beforeEach(() => {
140+
vi.clearAllMocks()
141+
vi.mocked(isPathOutsideWorkspace).mockReset()
142+
applyDiffTool.resetPartialState()
143+
vi.mocked(fs.readFile).mockResolvedValue("old\n")
144+
askApproval = vi.fn<ToolCallbacks["askApproval"]>().mockResolvedValue(true)
145+
})
146+
147+
describe.each(approvalPaths)("approval payload ($name)", ({ preventFocusDisruption }) => {
148+
it.each(cases)("reports isOutsideWorkspace for $name", async ({ relPath, isOutside }) => {
149+
const payload = JSON.parse(await runExecute(relPath, isOutside, preventFocusDisruption)) as ClineSayTool
150+
151+
expect(isPathOutsideWorkspace).toHaveBeenCalledWith(path.resolve(CWD, relPath))
152+
expect(payload.tool).toBe("appliedDiff")
153+
expect(payload.isOutsideWorkspace).toBe(isOutside)
154+
})
155+
})
156+
157+
it.each(cases)("reports isOutsideWorkspace in the partial preview for $name", async ({ relPath, isOutside }) => {
158+
vi.mocked(isPathOutsideWorkspace).mockReturnValue(isOutside)
159+
const task = createTask()
160+
const block: ToolUse<"apply_diff"> = {
161+
type: "tool_use",
162+
name: "apply_diff",
163+
params: { path: relPath, diff: DIFF },
164+
partial: true,
165+
}
166+
167+
// The first call records the path; the second call sees it stable and asks.
168+
await applyDiffTool.handlePartial(task, block)
169+
await applyDiffTool.handlePartial(task, block)
170+
171+
expect(task.ask).toHaveBeenCalledTimes(1)
172+
const payload = JSON.parse(String(vi.mocked(task.ask).mock.calls[0][1])) as ClineSayTool
173+
expect(isPathOutsideWorkspace).toHaveBeenCalledWith(path.resolve(CWD, relPath))
174+
expect(payload.isOutsideWorkspace).toBe(isOutside)
175+
})
176+
177+
describe("with the real workspace check", () => {
178+
it.each([
179+
{
180+
name: "a ../ path that resolves outside",
181+
relPath: "../outside/config.json",
182+
isOutside: true,
183+
expected: "ask",
184+
},
185+
{ name: "a path inside", relPath: "src/index.ts", isOutside: false, expected: "approve" },
186+
])("marks $name the workspace and returns $expected", async ({ relPath, isOutside, expected }) => {
187+
const actual = await vi.importActual<typeof import("../../../utils/pathUtils")>("../../../utils/pathUtils")
188+
vi.mocked(isPathOutsideWorkspace).mockImplementation(actual.isPathOutsideWorkspace)
189+
190+
await applyDiffTool.execute({ path: relPath, diff: DIFF }, createTask(), {
191+
askApproval,
192+
handleError: vi.fn().mockResolvedValue(undefined),
193+
pushToolResult: vi.fn(),
194+
toolCallId: "tool-call-1",
195+
})
196+
197+
const text = String(askApproval.mock.calls[0][1])
198+
const payload = JSON.parse(text) as ClineSayTool
199+
expect(payload.isOutsideWorkspace).toBe(isOutside)
200+
201+
const result = await checkAutoApproval({
202+
state: { ...baseState, alwaysAllowWrite: true },
203+
cwd: CWD,
204+
ask: "tool",
205+
text,
206+
})
207+
expect(result).toEqual({ decision: expected })
208+
})
209+
})
210+
211+
describe("auto-approval decision with alwaysAllowWrite on", () => {
212+
it.each([
213+
{ ...cases[0], allowOutside: false, expected: "ask" },
214+
{ ...cases[0], allowOutside: true, expected: "approve" },
215+
{ ...cases[1], allowOutside: false, expected: "approve" },
216+
])(
217+
"returns $expected for $name when alwaysAllowWriteOutsideWorkspace is $allowOutside",
218+
async ({ relPath, isOutside, allowOutside, expected }) => {
219+
const text = await runExecute(relPath, isOutside)
220+
221+
const result = await checkAutoApproval({
222+
state: {
223+
...baseState,
224+
alwaysAllowWrite: true,
225+
alwaysAllowWriteOutsideWorkspace: allowOutside,
226+
},
227+
cwd: CWD,
228+
ask: "tool",
229+
text,
230+
})
231+
232+
expect(result).toEqual({ decision: expected })
233+
},
234+
)
235+
})
236+
})

0 commit comments

Comments
 (0)