fix: harden access checks for apply_patch move operations and fix flaky test

hannesrudolph · hannesrudolph · commit 1bf1f1f0e74e · 2025-11-28T00:42:57.000-07:00
- Add validation for destination path (movePath) in ApplyPatchTool:
  - Check rooignore access permissions for destination
  - Check write protection for destination path
  - Validate destination is within workspace boundaries
- Fix flaky ChatView focus test on Windows by waiting for component
  to settle before clearing mock focus calls
diff --git a/src/core/tools/ApplyPatchTool.ts b/src/core/tools/ApplyPatchTool.ts
@@ -350,6 +350,39 @@ export class ApplyPatchTool extends BaseTool<"apply_patch"> {
 		if (change.movePath) {
 			const moveAbsolutePath = path.resolve(task.cwd, change.movePath)
 
+			// Validate destination path access permissions
+			const moveAccessAllowed = task.rooIgnoreController?.validateAccess(change.movePath)
+			if (!moveAccessAllowed) {
+				await task.say("rooignore_error", change.movePath)
+				pushToolResult(formatResponse.rooIgnoreError(change.movePath))
+				await task.diffViewProvider.reset()
+				return
+			}
+
+			// Check if destination path is write-protected
+			const isMovePathWriteProtected = task.rooProtectedController?.isWriteProtected(change.movePath) || false
+			if (isMovePathWriteProtected) {
+				task.consecutiveMistakeCount++
+				task.recordToolError("apply_patch")
+				const errorMessage = `Cannot move file to write-protected path: ${change.movePath}`
+				await task.say("error", errorMessage)
+				pushToolResult(formatResponse.toolError(errorMessage))
+				await task.diffViewProvider.reset()
+				return
+			}
+
+			// Check if destination path is outside workspace
+			const isMoveOutsideWorkspace = isPathOutsideWorkspace(moveAbsolutePath)
+			if (isMoveOutsideWorkspace) {
+				task.consecutiveMistakeCount++
+				task.recordToolError("apply_patch")
+				const errorMessage = `Cannot move file to path outside workspace: ${change.movePath}`
+				await task.say("error", errorMessage)
+				pushToolResult(formatResponse.toolError(errorMessage))
+				await task.diffViewProvider.reset()
+				return
+			}
+
 			// Save new content to the new path
 			if (isPreventFocusDisruptionEnabled) {
 				await task.diffViewProvider.saveDirectly(
diff --git a/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx
@@ -463,7 +463,12 @@ describe("ChatView - Focus Grabbing Tests", () => {
 			],
 		})
 
-		// Clear any initial calls
+		// Wait for the component to fully render and settle before clearing mocks
+		await waitFor(() => {
+			expect(getByTestId("chat-textarea")).toBeInTheDocument()
+		})
+
+		// Clear any initial calls after state has settled
 		mockFocus.mockClear()
 
 		// Add follow-up question
@@ -484,7 +489,7 @@ describe("ChatView - Focus Grabbing Tests", () => {
 			],
 		})
 
-		// Wait a bit to ensure any focus operations would have occurred
+		// Wait for state update to complete
 		await waitFor(() => {
 			expect(getByTestId("chat-textarea")).toBeInTheDocument()
 		})
