diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 47b574e4b7b..84de3830b78 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -103,7 +103,10 @@ export interface ExtensionMessage { | "branchWorktreeIncludeResult" | "folderSelected" | "skills" + | "fileContent" text?: string + /** For fileContent: { path, content, error? } */ + fileContent?: { path: string; content: string | null; error?: string } payload?: any // eslint-disable-line @typescript-eslint/no-explicit-any checkpointWarning?: { type: "WAIT_TIMEOUT" | "INIT_TIMEOUT" @@ -444,6 +447,7 @@ export interface WebviewMessage { | "openImage" | "saveImage" | "openFile" + | "readFileContent" | "openMention" | "cancelTask" | "cancelAutoApproval" @@ -786,6 +790,8 @@ export interface ClineSayTool { matchCount?: number diff?: string content?: string + // Original file content before first edit (for merged diff display in FileChangesPanel) + originalContent?: string // Unified diff statistics computed by the extension diffStats?: { added: number; removed: number } regex?: string diff --git a/src/core/tools/ApplyDiffTool.ts b/src/core/tools/ApplyDiffTool.ts index 5ca7002ff2d..3b664b3bd22 100644 --- a/src/core/tools/ApplyDiffTool.ts +++ b/src/core/tools/ApplyDiffTool.ts @@ -150,6 +150,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { ...sharedMessageProps, diff: diffContent, content: unifiedPatch, + originalContent, diffStats, isProtected: isWriteProtected, } satisfies ClineSayTool) @@ -194,6 +195,7 @@ export class ApplyDiffTool extends BaseTool<"apply_diff"> { ...sharedMessageProps, diff: diffContent, content: unifiedPatch, + originalContent, diffStats, isProtected: isWriteProtected, } satisfies ClineSayTool) diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts index a9ad591e4a4..3f3295404ba 100644 --- a/src/core/tools/ApplyPatchTool.ts +++ b/src/core/tools/ApplyPatchTool.ts @@ -341,6 +341,7 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> { tool: "appliedDiff", path: getReadablePath(task.cwd, relPath), diff: sanitizedDiff, + originalContent, isOutsideWorkspace, } diff --git a/src/core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts new file mode 100644 index 00000000000..00230c077a6 --- /dev/null +++ b/src/core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts @@ -0,0 +1,210 @@ +// npx vitest core/webview/__tests__/webviewMessageHandler.readFileContent.spec.ts + +import { describe, it, expect, vi, beforeEach } from "vitest" + +vi.mock("../../../api/providers/fetchers/modelCache") + +vi.mock("vscode", () => ({ + window: { + showInformationMessage: vi.fn(), + showErrorMessage: vi.fn(), + showTextDocument: vi.fn(), + }, + workspace: { + workspaceFolders: [{ uri: { fsPath: "/mock/workspace" } }], + openTextDocument: vi.fn().mockResolvedValue({}), + }, +})) + +vi.mock("../../../i18n", () => ({ + t: vi.fn((key: string) => key), +})) + +vi.mock("fs/promises", () => { + const readFile = vi.fn().mockResolvedValue("file content here") + return { + default: { + rm: vi.fn(), + mkdir: vi.fn(), + readFile, + writeFile: vi.fn(), + }, + rm: vi.fn(), + mkdir: vi.fn(), + readFile, + writeFile: vi.fn(), + } +}) + +vi.mock("../../../utils/fs") +vi.mock("../../../utils/path") +vi.mock("../../../utils/globalContext") + +vi.mock("../../../utils/pathUtils", () => ({ + isPathOutsideWorkspace: vi.fn((filePath: string) => { + const nodePath = require("path") + const normalized = nodePath.resolve(filePath) + const workspaceRoot = nodePath.resolve("/mock/workspace") + // Path is inside workspace if it equals or is under workspace root + if (normalized === workspaceRoot) return false + if (normalized.startsWith(workspaceRoot + nodePath.sep)) return false + return true + }), +})) + +vi.mock("../../mentions/resolveImageMentions", () => ({ + resolveImageMentions: vi.fn(async ({ text, images }: { text: string; images?: string[] }) => ({ + text, + images: [...(images ?? [])], + })), +})) + +import { webviewMessageHandler } from "../webviewMessageHandler" +import type { ClineProvider } from "../ClineProvider" +import * as fs from "fs/promises" + +const MOCK_CWD = "/mock/workspace/project" + +const mockProvider = { + getState: vi.fn(), + postMessageToWebview: vi.fn(), + customModesManager: { + getCustomModes: vi.fn(), + deleteCustomMode: vi.fn(), + }, + context: { + extensionPath: "/mock/extension/path", + globalStorageUri: { fsPath: "/mock/global/storage" }, + }, + contextProxy: { + context: { + extensionPath: "/mock/extension/path", + globalStorageUri: { fsPath: "/mock/global/storage" }, + }, + setValue: vi.fn(), + getValue: vi.fn(), + }, + log: vi.fn(), + postStateToWebview: vi.fn(), + getCurrentTask: vi.fn().mockReturnValue({ cwd: MOCK_CWD }), + getTaskWithId: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + cwd: MOCK_CWD, +} as unknown as ClineProvider + +describe("webviewMessageHandler - readFileContent path traversal prevention", () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(fs.readFile).mockResolvedValue("file content here") + vi.mocked(mockProvider.getCurrentTask).mockReturnValue({ cwd: MOCK_CWD } as any) + }) + + it("allows reading a file within the workspace using a relative path", async () => { + await webviewMessageHandler(mockProvider, { + type: "readFileContent", + text: "src/index.ts", + }) + + expect(fs.readFile).toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "fileContent", + fileContent: expect.objectContaining({ + path: "src/index.ts", + content: "file content here", + }), + }), + ) + }) + + it("blocks path traversal with ../", async () => { + await webviewMessageHandler(mockProvider, { + type: "readFileContent", + text: "../../../etc/passwd", + }) + + expect(fs.readFile).not.toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "fileContent", + fileContent: expect.objectContaining({ + path: "../../../etc/passwd", + content: null, + error: "Path is outside workspace", + }), + }), + ) + }) + + it("blocks absolute paths outside the workspace", async () => { + await webviewMessageHandler(mockProvider, { + type: "readFileContent", + text: "/etc/shadow", + }) + + expect(fs.readFile).not.toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "fileContent", + fileContent: expect.objectContaining({ + path: "/etc/shadow", + content: null, + error: "Path is outside workspace", + }), + }), + ) + }) + + it("blocks traversal disguised in the middle of a path", async () => { + await webviewMessageHandler(mockProvider, { + type: "readFileContent", + text: "src/../../../../etc/passwd", + }) + + expect(fs.readFile).not.toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "fileContent", + fileContent: expect.objectContaining({ + content: null, + error: "Path is outside workspace", + }), + }), + ) + }) + + it("returns error when no path is provided", async () => { + await webviewMessageHandler(mockProvider, { + type: "readFileContent", + text: "", + }) + + expect(fs.readFile).not.toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "fileContent", + fileContent: expect.objectContaining({ + content: null, + error: "No path provided", + }), + }), + ) + }) + + it("allows reading a file using an absolute path within the workspace", async () => { + await webviewMessageHandler(mockProvider, { + type: "readFileContent", + text: `${MOCK_CWD}/src/index.ts`, + }) + + expect(fs.readFile).toHaveBeenCalled() + expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "fileContent", + fileContent: expect.objectContaining({ + content: "file content here", + }), + }), + ) + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 4f8d4317245..73ebad67b0b 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -63,6 +63,7 @@ import { openMention } from "../mentions" import { resolveImageMentions } from "../mentions/resolveImageMentions" import { RooIgnoreController } from "../ignore/RooIgnoreController" import { getWorkspacePath } from "../../utils/path" +import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { Mode, defaultModeSlug } from "../../shared/modes" import { getModels, flushModels } from "../../api/providers/fetchers/modelCache" import { GetModelsOptions } from "../../shared/api" @@ -1142,6 +1143,44 @@ export const webviewMessageHandler = async ( } openFile(filePath, message.values as { create?: boolean; content?: string; line?: number }) break + case "readFileContent": { + const relPath = message.text || "" + if (!relPath) { + provider.postMessageToWebview({ + type: "fileContent", + fileContent: { path: relPath, content: null, error: "No path provided" }, + }) + break + } + try { + const cwd = getCurrentCwd() + if (!cwd) { + provider.postMessageToWebview({ + type: "fileContent", + fileContent: { path: relPath, content: null, error: "No workspace path available" }, + }) + break + } + const absPath = path.resolve(cwd, relPath) + // Workspace-boundary validation: prevent path traversal attacks + if (isPathOutsideWorkspace(absPath)) { + provider.postMessageToWebview({ + type: "fileContent", + fileContent: { path: relPath, content: null, error: "Path is outside workspace" }, + }) + break + } + const content = await fs.readFile(absPath, "utf-8") + provider.postMessageToWebview({ type: "fileContent", fileContent: { path: relPath, content } }) + } catch (err) { + const errorMsg = err instanceof Error ? err.message : String(err) + provider.postMessageToWebview({ + type: "fileContent", + fileContent: { path: relPath, content: null, error: errorMsg }, + }) + } + break + } case "openMention": openMention(getCurrentCwd(), message.text) break diff --git a/webview-ui/src/__tests__/FileChangesPanel.spec.tsx b/webview-ui/src/__tests__/FileChangesPanel.spec.tsx index b28102b1fe7..9c5535fb7b3 100644 --- a/webview-ui/src/__tests__/FileChangesPanel.spec.tsx +++ b/webview-ui/src/__tests__/FileChangesPanel.spec.tsx @@ -44,7 +44,11 @@ vi.mock("@src/components/common/CodeAccordian", () => ({ ), })) -function createFileEditMessage(path: string, diff: string): ClineMessage { +function createFileEditMessage( + path: string, + diff: string, + diffStats?: { added: number; removed: number }, +): ClineMessage { return { type: "ask", ask: "tool", @@ -55,6 +59,7 @@ function createFileEditMessage(path: string, diff: string): ClineMessage { tool: "appliedDiff", path, diff, + ...(diffStats && { diffStats }), }), } } @@ -172,4 +177,23 @@ describe("FileChangesPanel", () => { fireEvent.click(accordianToggle) expect(accordianToggle).toHaveTextContent("expanded") }) + + it("hides aggregate stats when no diffStats are present", () => { + const messages = [createFileEditMessage("src/a.ts", "diff a"), createFileEditMessage("src/b.ts", "diff b")] + renderPanel(messages) + + expect(screen.queryByTestId("total-added")).not.toBeInTheDocument() + expect(screen.queryByTestId("total-removed")).not.toBeInTheDocument() + }) + + it("shows aggregated + and - totals in the header when diffStats are present", () => { + const messages = [ + createFileEditMessage("src/a.ts", "diff a", { added: 3, removed: 1 }), + createFileEditMessage("src/b.ts", "diff b", { added: 2, removed: 5 }), + ] + renderPanel(messages) + + expect(screen.getByTestId("total-added")).toHaveTextContent("+5") + expect(screen.getByTestId("total-removed")).toHaveTextContent("-6") + }) }) diff --git a/webview-ui/src/components/chat/FileChangesPanel.tsx b/webview-ui/src/components/chat/FileChangesPanel.tsx index 8a4eb016ccf..7dec194e0c0 100644 --- a/webview-ui/src/components/chat/FileChangesPanel.tsx +++ b/webview-ui/src/components/chat/FileChangesPanel.tsx @@ -1,8 +1,9 @@ -import { memo, useEffect, useMemo, useState, useCallback } from "react" +import { memo, useEffect, useMemo, useState, useCallback, useRef } from "react" import { useTranslation } from "react-i18next" import { ChevronDown, ChevronRight, FileDiff } from "lucide-react" +import { createTwoFilesPatch } from "diff" -import type { ClineMessage } from "@roo-code/types" +import type { ClineMessage, ExtensionMessage } from "@roo-code/types" import { Collapsible, CollapsibleContent, CollapsibleTrigger } from "@/components/ui" import { cn } from "@/lib/utils" @@ -20,10 +21,14 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro const { t } = useTranslation() const [panelExpanded, setPanelExpanded] = useState(false) const [expandedPaths, setExpandedPaths] = useState>(new Set()) + const [finalContentByPath, setFinalContentByPath] = useState>({}) + const pendingPathsRef = useRef>(new Set()) - // Reset expanded file rows when switching to a different task (clineMessages identity change) + // Reset expanded file rows and final content cache when switching to a different task useEffect(() => { setExpandedPaths(new Set()) + setFinalContentByPath({}) + pendingPathsRef.current = new Set() }, [clineMessages]) const fileChanges = useMemo(() => fileChangesFromMessages(clineMessages), [clineMessages]) @@ -40,6 +45,17 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro return map }, [fileChanges]) + // Aggregate total lines added/removed across all files for the panel header + const totalStats = useMemo(() => { + return fileChanges.reduce( + (acc, e) => ({ + added: acc.added + (e.diffStats?.added ?? 0), + removed: acc.removed + (e.diffStats?.removed ?? 0), + }), + { added: 0, removed: 0 }, + ) + }, [fileChanges]) + const togglePath = useCallback((path: string) => { setExpandedPaths((prev) => { const next = new Set(prev) @@ -49,6 +65,38 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro }) }, []) + // Request final file content when a row is expanded and we have originalContent + useEffect(() => { + for (const path of expandedPaths) { + const entries = byPath.get(path) + if (!entries?.length) continue + const originalContent = entries[0].originalContent + const lookupPath = path.startsWith("./") ? path.slice(2) : path + if ( + originalContent !== undefined && + !(lookupPath in finalContentByPath) && + !pendingPathsRef.current.has(lookupPath) + ) { + pendingPathsRef.current.add(lookupPath) + vscode.postMessage({ type: "readFileContent", text: lookupPath }) + } + } + }, [expandedPaths, byPath, finalContentByPath]) + + // Listen for fileContent responses + useEffect(() => { + const handler = (event: MessageEvent) => { + const message: ExtensionMessage = event.data + if (message.type === "fileContent" && message.fileContent?.path != null) { + const fc = message.fileContent + pendingPathsRef.current.delete(fc.path) + setFinalContentByPath((prev) => ({ ...prev, [fc.path]: fc.content ?? null })) + } + } + window.addEventListener("message", handler) + return () => window.removeEventListener("message", handler) + }, []) + if (fileChanges.length === 0) return null const fileCount = byPath.size @@ -69,12 +117,30 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro {t("chat:fileChangesInConversation.header", { count: fileCount })} + {totalStats.added > 0 || totalStats.removed > 0 ? ( +
+ + +{totalStats.added} + + + -{totalStats.removed} + +
+ ) : null}
{Array.from(byPath.entries()).map(([path, entries]) => { - // If multiple edits to same file, concatenate diffs with a separator - const combinedDiff = entries.map((e) => e.diff).join("\n\n") + const originalContent = entries[0].originalContent + const lookupPath = path.startsWith("./") ? path.slice(2) : path + const finalContent = finalContentByPath[lookupPath] + const hasMergedDiff = + originalContent !== undefined && finalContent != null && finalContent !== "" + const displayDiff = hasMergedDiff + ? createTwoFilesPatch(path, path, originalContent, finalContent) + : entries.map((e) => e.diff).join("\n\n") const combinedStats = entries.reduce( (acc, e) => ({ added: acc.added + (e.diffStats?.added ?? 0), @@ -87,7 +153,7 @@ const FileChangesPanel = memo(({ clineMessages, className }: FileChangesPanelPro
togglePath(path)} diff --git a/webview-ui/src/components/chat/utils/fileChangesFromMessages.ts b/webview-ui/src/components/chat/utils/fileChangesFromMessages.ts index 6b77833e9d8..738305ad158 100644 --- a/webview-ui/src/components/chat/utils/fileChangesFromMessages.ts +++ b/webview-ui/src/components/chat/utils/fileChangesFromMessages.ts @@ -8,6 +8,8 @@ export interface FileChangeEntry { path: string diff: string diffStats?: { added: number; removed: number } + /** Original file content before first edit (for merged diff display) */ + originalContent?: string } /** @@ -56,6 +58,7 @@ export function fileChangesFromMessages(messages: ClineMessage[] | undefined): F path: tool.path, diff, diffStats: tool.diffStats, + originalContent: tool.originalContent, }) } }