Skip to content

Commit ea9d3df

Browse files
fix(tools): resolve symlinks in remaining workspace-boundary checks (#169)
Route the workspace-boundary check through the symlink-aware resolveIsOutsideWorkspace helper in the tools that still called isPathOutsideWorkspace() directly, closing the read-outside-workspace bypass from #169: - ReadFileTool (batch + single approval, handlePartial, legacy read) - WriteToFileTool, ApplyPatchTool, EditTool, SearchFilesTool, GenerateImageTool - webviewMessageHandler readFileContent now fetches allowSymlinksOutsideWorkspace from the provider and passes it through Add regression coverage for the symlink-resolved read paths and for the allowSymlinksOutsideWorkspace setting in the webview settings UI.
1 parent f71b6d6 commit ea9d3df

11 files changed

Lines changed: 224 additions & 35 deletions

File tree

src/core/tools/ApplyPatchTool.ts

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

66
import { getReadablePath } from "../../utils/path"
7-
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
87
import { Task } from "../task/Task"
98
import { formatResponse } from "../prompts/responses"
109
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
@@ -160,7 +159,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
160159
}
161160

162161
const newContent = change.newContent || ""
163-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
162+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
164163

165164
// Initialize diff view for new file
166165
task.diffViewProvider.editType = "create"
@@ -250,7 +249,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
250249
return
251250
}
252251

253-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
252+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
254253

255254
const sharedMessageProps: ClineSayTool = {
256255
tool: "appliedDiff",
@@ -310,7 +309,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
310309

311310
const originalContent = change.originalContent || ""
312311
const newContent = change.newContent || ""
313-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
312+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
314313

315314
// Initialize diff view
316315
task.diffViewProvider.editType = "modify"
@@ -396,7 +395,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
396395
}
397396

398397
// Check if destination path is outside workspace
399-
const isMoveOutsideWorkspace = isPathOutsideWorkspace(moveAbsolutePath)
398+
const isMoveOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, moveAbsolutePath)
400399
if (isMoveOutsideWorkspace) {
401400
task.consecutiveMistakeCount++
402401
task.recordToolError("apply_patch")
@@ -469,7 +468,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
469468
tool: "appliedDiff",
470469
path: displayPath || path.basename(task.cwd) || "workspace",
471470
diff: patchPreview || "Parsing patch...",
472-
isOutsideWorkspace: isPathOutsideWorkspace(absolutePath),
471+
isOutsideWorkspace: await this.resolveIsOutsideWorkspace(task, absolutePath),
473472
}
474473

475474
await task.ask("tool", JSON.stringify(sharedMessageProps), block.partial).catch(() => {})

src/core/tools/EditTool.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import path from "path"
44
import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
55

66
import { getReadablePath } from "../../utils/path"
7-
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
87
import { Task } from "../task/Task"
98
import { formatResponse } from "../prompts/responses"
109
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
@@ -174,7 +173,7 @@ export class EditTool extends BaseTool<"edit"> {
174173

175174
const sanitizedDiff = sanitizeUnifiedDiff(diff)
176175
const diffStats = computeDiffStats(sanitizedDiff) || undefined
177-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
176+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
178177

179178
const sharedMessageProps: ClineSayTool = {
180179
tool: "appliedDiff",
@@ -253,7 +252,7 @@ export class EditTool extends BaseTool<"edit"> {
253252

254253
// relPath is guaranteed non-null after hasPathStabilized
255254
const absolutePath = path.resolve(task.cwd, relPath!)
256-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
255+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
257256

258257
const sharedMessageProps: ClineSayTool = {
259258
tool: "appliedDiff",

src/core/tools/GenerateImageTool.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { Task } from "../task/Task"
1111
import { formatResponse } from "../prompts/responses"
1212
import { fileExistsAtPath } from "../../utils/fs"
1313
import { getReadablePath } from "../../utils/path"
14-
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
1514
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1615
import { OpenRouterHandler } from "../../api/providers/openrouter"
1716
import { BaseTool, ToolCallbacks } from "./BaseTool"
@@ -163,7 +162,7 @@ export class GenerateImageTool extends BaseTool<"generate_image"> {
163162
}
164163

165164
const fullPath = path.resolve(task.cwd, relPath)
166-
const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
165+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, fullPath)
167166

168167
const sharedMessageProps = {
169168
tool: "generateImage" as const,

src/core/tools/ReadFileTool.ts

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import { isLegacyReadFileParams, type ClineSayTool } from "@roo-code/types"
1818
import { Task } from "../task/Task"
1919
import { formatResponse } from "../prompts/responses"
2020
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
21-
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
2221
import { getReadablePath } from "../../utils/path"
2322
import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../integrations/misc/extract-text"
2423
import { readWithIndentation, readWithSlice } from "../../integrations/misc/indentation-reader"
@@ -433,17 +432,19 @@ export class ReadFileTool extends BaseTool<"read_file"> {
433432

434433
if (filesToApprove.length > 1) {
435434
// Batch approval
436-
const batchFiles = filesToApprove.map((fileResult) => {
437-
const relPath = fileResult.path
438-
const fullPath = path.resolve(task.cwd, relPath)
439-
const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
440-
const readablePath = getReadablePath(task.cwd, relPath)
441-
442-
const lineSnippet = this.getLineSnippet(fileResult.entry!)
443-
const key = `${readablePath}${lineSnippet ? ` (${lineSnippet})` : ""}`
444-
445-
return { path: readablePath, lineSnippet, isOutsideWorkspace, key, content: fullPath }
446-
})
435+
const batchFiles = await Promise.all(
436+
filesToApprove.map(async (fileResult) => {
437+
const relPath = fileResult.path
438+
const fullPath = path.resolve(task.cwd, relPath)
439+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, fullPath)
440+
const readablePath = getReadablePath(task.cwd, relPath)
441+
442+
const lineSnippet = this.getLineSnippet(fileResult.entry!)
443+
const key = `${readablePath}${lineSnippet ? ` (${lineSnippet})` : ""}`
444+
445+
return { path: readablePath, lineSnippet, isOutsideWorkspace, key, content: fullPath }
446+
}),
447+
)
447448

448449
const completeMessage = JSON.stringify({ tool: "readFile", batchFiles } satisfies ClineSayTool)
449450
const { response, text, images } = await task.ask("tool", completeMessage, false)
@@ -501,7 +502,7 @@ export class ReadFileTool extends BaseTool<"read_file"> {
501502
const fileResult = filesToApprove[0]
502503
const relPath = fileResult.path
503504
const fullPath = path.resolve(task.cwd, relPath)
504-
const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
505+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, fullPath)
505506
const lineSnippet = this.getLineSnippet(fileResult.entry!)
506507

507508
const startLine = this.getStartLine(fileResult.entry!)
@@ -651,7 +652,7 @@ export class ReadFileTool extends BaseTool<"read_file"> {
651652
const sharedMessageProps: ClineSayTool = {
652653
tool: "readFile",
653654
path: getReadablePath(task.cwd, filePath),
654-
isOutsideWorkspace: filePath ? isPathOutsideWorkspace(fullPath) : false,
655+
isOutsideWorkspace: filePath ? await this.resolveIsOutsideWorkspace(task, fullPath) : false,
655656
}
656657
const partialMessage = JSON.stringify({
657658
...sharedMessageProps,
@@ -698,7 +699,7 @@ export class ReadFileTool extends BaseTool<"read_file"> {
698699
}
699700

700701
// Request approval for single file
701-
const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
702+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, fullPath)
702703
let lineSnippet = ""
703704
if (entry.lineRanges && entry.lineRanges.length > 0) {
704705
const ranges = entry.lineRanges.map((range: LineRange) => `(lines ${range.start}-${range.end})`)

src/core/tools/SearchFilesTool.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { type ClineSayTool } from "@roo-code/types"
44

55
import { Task } from "../task/Task"
66
import { getReadablePath } from "../../utils/path"
7-
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
87
import { regexSearchFiles } from "../../services/ripgrep"
98
import type { ToolUse } from "../../shared/tools"
109

@@ -45,7 +44,7 @@ export class SearchFilesTool extends BaseTool<"search_files"> {
4544
task.consecutiveMistakeCount = 0
4645

4746
const absolutePath = path.resolve(task.cwd, relDirPath)
48-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
47+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
4948

5049
const sharedMessageProps: ClineSayTool = {
5150
tool: "searchFiles",
@@ -77,7 +76,7 @@ export class SearchFilesTool extends BaseTool<"search_files"> {
7776
const filePattern = block.params.file_pattern
7877

7978
const absolutePath = relDirPath ? path.resolve(task.cwd, relDirPath) : task.cwd
80-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
79+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
8180

8281
const sharedMessageProps: ClineSayTool = {
8382
tool: "searchFiles",

src/core/tools/WriteToFileTool.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
1010
import { fileExistsAtPath, createDirectoriesForFile } from "../../utils/fs"
1111
import { stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text"
1212
import { getReadablePath } from "../../utils/path"
13-
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
1413
import { unescapeHtmlEntities } from "../../utils/text-normalization"
1514
import { EXPERIMENT_IDS, experiments } from "../../shared/experiments"
1615
import { convertNewFileToUnifiedDiff, computeDiffStats, sanitizeUnifiedDiff } from "../diff/stats"
@@ -86,7 +85,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
8685
}
8786

8887
const fullPath = relPath ? path.resolve(task.cwd, relPath) : ""
89-
const isOutsideWorkspace = isPathOutsideWorkspace(fullPath)
88+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, fullPath)
9089

9190
const sharedMessageProps: ClineSayTool = {
9291
tool: fileExists ? "editedExistingFile" : "newFileCreated",
@@ -231,7 +230,7 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
231230
}
232231

233232
const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false
234-
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
233+
const isOutsideWorkspace = await this.resolveIsOutsideWorkspace(task, absolutePath)
235234

236235
const sharedMessageProps: ClineSayTool = {
237236
tool: fileExists ? "editedExistingFile" : "newFileCreated",

src/core/tools/__tests__/readFileTool.spec.ts

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import {
2626
} from "../helpers/imageHelpers"
2727
import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../../integrations/misc/extract-text"
2828
import { readWithIndentation, readWithSlice } from "../../../integrations/misc/indentation-reader"
29+
import { isPathOutsideWorkspace } from "../../../utils/pathUtils"
2930

3031
// ─── Mocks ────────────────────────────────────────────────────────────────────
3132

@@ -60,6 +61,12 @@ vi.mock("../../../integrations/misc/indentation-reader", () => ({
6061
readWithSlice: vi.fn(),
6162
}))
6263

64+
// Spy on the workspace-boundary check so we can assert symlink resolution is
65+
// honored for reads (#169 / #241). Default to "inside workspace" (false).
66+
vi.mock("../../../utils/pathUtils", () => ({
67+
isPathOutsideWorkspace: vi.fn(() => false),
68+
}))
69+
6370
vi.mock("../helpers/imageHelpers", () => ({
6471
DEFAULT_MAX_IMAGE_FILE_SIZE_MB: 5,
6572
DEFAULT_MAX_TOTAL_IMAGE_SIZE_MB: 20,
@@ -132,10 +139,17 @@ interface MockTaskOptions {
132139
rooIgnoreAllowed?: boolean
133140
maxImageFileSize?: number
134141
maxTotalImageSize?: number
142+
allowSymlinksOutsideWorkspace?: boolean
135143
}
136144

137145
function createMockTask(options: MockTaskOptions = {}) {
138-
const { supportsImages = false, rooIgnoreAllowed = true, maxImageFileSize = 5, maxTotalImageSize = 20 } = options
146+
const {
147+
supportsImages = false,
148+
rooIgnoreAllowed = true,
149+
maxImageFileSize = 5,
150+
maxTotalImageSize = 20,
151+
allowSymlinksOutsideWorkspace = false,
152+
} = options
139153

140154
return {
141155
cwd: "/test/workspace",
@@ -162,6 +176,7 @@ function createMockTask(options: MockTaskOptions = {}) {
162176
getState: vi.fn().mockResolvedValue({
163177
maxImageFileSize,
164178
maxTotalImageSize,
179+
allowSymlinksOutsideWorkspace,
165180
}),
166181
}),
167182
},
@@ -195,6 +210,57 @@ describe("ReadFileTool", () => {
195210
})
196211
})
197212

213+
describe("workspace boundary (symlink resolution, #169 / #241)", () => {
214+
it("routes the read boundary check through symlink resolution with the setting disabled", async () => {
215+
vi.mocked(isPathOutsideWorkspace).mockReturnValue(false)
216+
const mockTask = createMockTask({ allowSymlinksOutsideWorkspace: false })
217+
const callbacks = createMockCallbacks()
218+
219+
await readFileTool.execute({ path: "test.txt" }, mockTask as any, callbacks)
220+
221+
// The boundary check must be invoked with the resolved option, not bypassed.
222+
expect(isPathOutsideWorkspace).toHaveBeenCalledWith(
223+
expect.any(String),
224+
expect.objectContaining({ allowSymlinksOutsideWorkspace: false }),
225+
)
226+
227+
// The approval payload must reflect the boundary decision.
228+
const toolAsk = mockTask.ask.mock.calls.find(([type]: [string]) => type === "tool")
229+
expect(toolAsk).toBeDefined()
230+
expect(JSON.parse(toolAsk![1] as string)).toEqual(
231+
expect.objectContaining({ tool: "readFile", isOutsideWorkspace: false }),
232+
)
233+
})
234+
235+
it("forwards allowSymlinksOutsideWorkspace=true from provider state to the boundary check", async () => {
236+
vi.mocked(isPathOutsideWorkspace).mockReturnValue(false)
237+
const mockTask = createMockTask({ allowSymlinksOutsideWorkspace: true })
238+
const callbacks = createMockCallbacks()
239+
240+
await readFileTool.execute({ path: "test.txt" }, mockTask as any, callbacks)
241+
242+
expect(isPathOutsideWorkspace).toHaveBeenCalledWith(
243+
expect.any(String),
244+
expect.objectContaining({ allowSymlinksOutsideWorkspace: true }),
245+
)
246+
})
247+
248+
it("surfaces a symlink resolving outside the workspace as isOutsideWorkspace=true", async () => {
249+
// Simulate a symlink whose real target is outside the workspace.
250+
vi.mocked(isPathOutsideWorkspace).mockReturnValue(true)
251+
const mockTask = createMockTask({ allowSymlinksOutsideWorkspace: false })
252+
const callbacks = createMockCallbacks()
253+
254+
await readFileTool.execute({ path: "link-to-outside.txt" }, mockTask as any, callbacks)
255+
256+
const toolAsk = mockTask.ask.mock.calls.find(([type]: [string]) => type === "tool")
257+
expect(toolAsk).toBeDefined()
258+
expect(JSON.parse(toolAsk![1] as string)).toEqual(
259+
expect.objectContaining({ tool: "readFile", isOutsideWorkspace: true }),
260+
)
261+
})
262+
})
263+
198264
describe("input validation", () => {
199265
it("should return error when path is missing", async () => {
200266
const mockTask = createMockTask()

src/core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,8 @@ vi.mock("../../../utils/pathUtils", () => ({
5252
}),
5353
}))
5454

55+
import { isPathOutsideWorkspace } from "../../../utils/pathUtils"
56+
5557
vi.mock("../../mentions/resolveImageMentions", () => ({
5658
resolveImageMentions: vi.fn(async ({ text, images }: { text: string; images?: string[] }) => ({
5759
text,
@@ -97,6 +99,7 @@ describe("webviewMessageHandler - readFileContent path traversal prevention", ()
9799
vi.clearAllMocks()
98100
vi.mocked(fs.readFile).mockResolvedValue("file content here")
99101
vi.mocked(mockProvider.getCurrentTask).mockReturnValue({ cwd: MOCK_CWD } as any)
102+
vi.mocked(mockProvider.getState).mockResolvedValue({ allowSymlinksOutsideWorkspace: false } as any)
100103
})
101104

102105
it("allows reading a file within the workspace using a relative path", async () => {
@@ -207,4 +210,32 @@ describe("webviewMessageHandler - readFileContent path traversal prevention", ()
207210
}),
208211
)
209212
})
213+
214+
it("forwards the allowSymlinksOutsideWorkspace setting to the boundary check (default false)", async () => {
215+
vi.mocked(mockProvider.getState).mockResolvedValue({ allowSymlinksOutsideWorkspace: false } as any)
216+
217+
await webviewMessageHandler(mockProvider, {
218+
type: "readFileContent",
219+
text: "src/index.ts",
220+
})
221+
222+
expect(isPathOutsideWorkspace).toHaveBeenCalledWith(
223+
expect.any(String),
224+
expect.objectContaining({ allowSymlinksOutsideWorkspace: false }),
225+
)
226+
})
227+
228+
it("passes allowSymlinksOutsideWorkspace=true through when the user opted in", async () => {
229+
vi.mocked(mockProvider.getState).mockResolvedValue({ allowSymlinksOutsideWorkspace: true } as any)
230+
231+
await webviewMessageHandler(mockProvider, {
232+
type: "readFileContent",
233+
text: "src/index.ts",
234+
})
235+
236+
expect(isPathOutsideWorkspace).toHaveBeenCalledWith(
237+
expect.any(String),
238+
expect.objectContaining({ allowSymlinksOutsideWorkspace: true }),
239+
)
240+
})
210241
})

src/core/webview/webviewMessageHandler.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,8 +1205,11 @@ export const webviewMessageHandler = async (
12051205
break
12061206
}
12071207
const absPath = path.resolve(cwd, relPath)
1208-
// Workspace-boundary validation: prevent path traversal attacks
1209-
if (isPathOutsideWorkspace(absPath)) {
1208+
// Workspace-boundary validation: prevent path traversal attacks.
1209+
// Honor the `allowSymlinksOutsideWorkspace` setting (#169 / #241) so symlink
1210+
// targets are resolved (fail-closed) unless the user opted in.
1211+
const { allowSymlinksOutsideWorkspace } = await provider.getState()
1212+
if (isPathOutsideWorkspace(absPath, { allowSymlinksOutsideWorkspace })) {
12101213
provider.postMessageToWebview({
12111214
type: "fileContent",
12121215
fileContent: { path: relPath, content: null, error: "Path is outside workspace" },

0 commit comments

Comments
 (0)