From aba0e89cc96d6a60132b82ab172044622188ce35 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 22 Jan 2026 16:35:01 +0000 Subject: [PATCH 1/2] fix: enforce file restrictions for all editing tools Previously, apply_patch, search_replace, and edit_file bypassed mode file restrictions because: 1. EDIT_OPERATION_PARAMS was missing their parameter names (patch, old_string, new_string) 2. File path extraction only checked toolParams?.path, not toolParams?.file_path This allowed these tools to edit non-matching files in restricted modes like Architect. Changes: - Added missing parameters to EDIT_OPERATION_PARAMS array - Updated file path extraction to check both path and file_path - Added comprehensive tests for all editing tools with file restrictions Fixes issue where Architect mode could edit non-.md files using certain tools. --- src/core/tools/validateToolUse.ts | 15 +- src/shared/__tests__/modes.spec.ts | 246 +++++++++++++++++++++++++++++ 2 files changed, 259 insertions(+), 2 deletions(-) diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index 0e56bb6e65f..19e306b1e51 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -62,7 +62,18 @@ export function validateToolUse( } } -const EDIT_OPERATION_PARAMS = ["diff", "content", "operations", "search", "replace", "args", "line"] as const +const EDIT_OPERATION_PARAMS = [ + "diff", + "content", + "operations", + "search", + "replace", + "args", + "line", + "patch", // Used by apply_patch + "old_string", // Used by search_replace and edit_file + "new_string", // Used by search_replace and edit_file +] as const function getGroupOptions(group: GroupEntry): GroupOptions | undefined { return Array.isArray(group) ? group[1] : undefined @@ -155,7 +166,7 @@ export function isToolAllowedForMode( // For the edit group, check file regex if specified if (groupName === "edit" && options.fileRegex) { - const filePath = toolParams?.path + const filePath = toolParams?.path || toolParams?.file_path // Check if this is an actual edit operation (not just path-only for streaming) const isEditOperation = EDIT_OPERATION_PARAMS.some((param) => toolParams?.[param]) diff --git a/src/shared/__tests__/modes.spec.ts b/src/shared/__tests__/modes.spec.ts index 1c74c25e13e..a8c04c627c2 100644 --- a/src/shared/__tests__/modes.spec.ts +++ b/src/shared/__tests__/modes.spec.ts @@ -273,6 +273,252 @@ describe("isToolAllowedForMode", () => { }), ).toThrow(/Markdown files only/) }) + + it("applies restrictions to apply_patch (custom tool)", () => { + // Test that apply_patch respects file restrictions when included + const patchResult = isToolAllowedForMode( + "apply_patch", + "markdown-editor", + customModes, + undefined, + { + path: "test.md", + patch: "diff --git a/test.md b/test.md\n--- a/test.md\n+++ b/test.md\n@@ -1 +1 @@\n-old\n+new", + }, + undefined, + ["apply_patch"], // Include custom tool + ) + expect(patchResult).toBe(true) + + // Test apply_patch with non-matching file + expect(() => + isToolAllowedForMode( + "apply_patch", + "markdown-editor", + customModes, + undefined, + { + path: "test.js", + patch: "diff --git a/test.js b/test.js\n--- a/test.js\n+++ b/test.js\n@@ -1 +1 @@\n-old\n+new", + }, + undefined, + ["apply_patch"], // Include custom tool + ), + ).toThrow(FileRestrictionError) + expect(() => + isToolAllowedForMode( + "apply_patch", + "markdown-editor", + customModes, + undefined, + { + path: "test.js", + patch: "diff --git a/test.js b/test.js\n--- a/test.js\n+++ b/test.js\n@@ -1 +1 @@\n-old\n+new", + }, + undefined, + ["apply_patch"], // Include custom tool + ), + ).toThrow(/\\.md\$/) + }) + + it("applies restrictions to search_replace (custom tool)", () => { + // Test that search_replace respects file restrictions when included + const searchReplaceResult = isToolAllowedForMode( + "search_replace", + "markdown-editor", + customModes, + undefined, + { + file_path: "test.md", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["search_replace"], // Include custom tool + ) + expect(searchReplaceResult).toBe(true) + + // Test search_replace with non-matching file + expect(() => + isToolAllowedForMode( + "search_replace", + "markdown-editor", + customModes, + undefined, + { + file_path: "test.js", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["search_replace"], // Include custom tool + ), + ).toThrow(FileRestrictionError) + expect(() => + isToolAllowedForMode( + "search_replace", + "markdown-editor", + customModes, + undefined, + { + file_path: "test.js", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["search_replace"], // Include custom tool + ), + ).toThrow(/\\.md\$/) + }) + + it("applies restrictions to edit_file (custom tool)", () => { + // Test that edit_file respects file restrictions when included + const editFileResult = isToolAllowedForMode( + "edit_file", + "markdown-editor", + customModes, + undefined, + { + file_path: "test.md", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["edit_file"], // Include custom tool + ) + expect(editFileResult).toBe(true) + + // Test edit_file with non-matching file + expect(() => + isToolAllowedForMode( + "edit_file", + "markdown-editor", + customModes, + undefined, + { + file_path: "test.js", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["edit_file"], // Include custom tool + ), + ).toThrow(FileRestrictionError) + expect(() => + isToolAllowedForMode( + "edit_file", + "markdown-editor", + customModes, + undefined, + { + file_path: "test.js", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["edit_file"], // Include custom tool + ), + ).toThrow(/\\.md\$/) + }) + + it("applies restrictions to all editing tools in architect mode (custom tools)", () => { + // Test apply_patch in architect mode + expect( + isToolAllowedForMode( + "apply_patch", + "architect", + [], + undefined, + { + path: "test.md", + patch: "diff --git a/test.md b/test.md\n--- a/test.md\n+++ b/test.md\n@@ -1 +1 @@\n-old\n+new", + }, + undefined, + ["apply_patch"], // Include custom tool + ), + ).toBe(true) + + expect(() => + isToolAllowedForMode( + "apply_patch", + "architect", + [], + undefined, + { + path: "test.js", + patch: "diff --git a/test.js b/test.js\n--- a/test.js\n+++ b/test.js\n@@ -1 +1 @@\n-old\n+new", + }, + undefined, + ["apply_patch"], // Include custom tool + ), + ).toThrow(FileRestrictionError) + + // Test search_replace in architect mode + expect( + isToolAllowedForMode( + "search_replace", + "architect", + [], + undefined, + { + file_path: "test.md", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["search_replace"], // Include custom tool + ), + ).toBe(true) + + expect(() => + isToolAllowedForMode( + "search_replace", + "architect", + [], + undefined, + { + file_path: "test.js", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["search_replace"], // Include custom tool + ), + ).toThrow(FileRestrictionError) + + // Test edit_file in architect mode + expect( + isToolAllowedForMode( + "edit_file", + "architect", + [], + undefined, + { + file_path: "test.md", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["edit_file"], // Include custom tool + ), + ).toBe(true) + + expect(() => + isToolAllowedForMode( + "edit_file", + "architect", + [], + undefined, + { + file_path: "test.js", + old_string: "old text", + new_string: "new text", + }, + undefined, + ["edit_file"], // Include custom tool + ), + ).toThrow(FileRestrictionError) + }) }) it("handles non-existent modes", () => { From a3c18ffe02c7f3594bf1f72e8d5912d71e123602 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 22 Jan 2026 16:58:27 +0000 Subject: [PATCH 2/2] fix: parse apply_patch content to extract file paths for validation The apply_patch tool only accepts { patch: string } with no path parameter. File paths are embedded in the patch content using markers like: - "*** Add File: path" - "*** Delete File: path" - "*** Update File: path" This commit: - Adds extractFilePathsFromPatch helper function to parse patch content - Updates validation logic to extract and validate file paths from patch content - Fixes tests to use realistic apply_patch parameters (only patch, no path) - Uses proper patch format with *** Begin Patch, *** Update File: markers --- src/core/tools/validateToolUse.ts | 44 ++++++++++++++++++++++++++++++ src/shared/__tests__/modes.spec.ts | 19 ++++++------- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index 19e306b1e51..3579fde32cf 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -75,6 +75,34 @@ const EDIT_OPERATION_PARAMS = [ "new_string", // Used by search_replace and edit_file ] as const +// Markers used in apply_patch format to identify file operations +const PATCH_FILE_MARKERS = ["*** Add File: ", "*** Delete File: ", "*** Update File: "] as const + +/** + * Extract file paths from apply_patch content. + * The patch format uses markers like "*** Add File: path", "*** Delete File: path", "*** Update File: path" + * @param patchContent The patch content string + * @returns Array of file paths found in the patch + */ +function extractFilePathsFromPatch(patchContent: string): string[] { + const filePaths: string[] = [] + const lines = patchContent.split("\n") + + for (const line of lines) { + for (const marker of PATCH_FILE_MARKERS) { + if (line.startsWith(marker)) { + const path = line.substring(marker.length).trim() + if (path) { + filePaths.push(path) + } + break + } + } + } + + return filePaths +} + function getGroupOptions(group: GroupEntry): GroupOptions | undefined { return Array.isArray(group) ? group[1] : undefined } @@ -175,6 +203,22 @@ export function isToolAllowedForMode( throw new FileRestrictionError(mode.name, options.fileRegex, options.description, filePath, tool) } + // Handle apply_patch: extract file paths from patch content and validate each + if (tool === "apply_patch" && typeof toolParams?.patch === "string") { + const patchFilePaths = extractFilePathsFromPatch(toolParams.patch) + for (const patchFilePath of patchFilePaths) { + if (!doesFileMatchRegex(patchFilePath, options.fileRegex)) { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + patchFilePath, + tool, + ) + } + } + } + // Native-only: multi-file edits provide structured params; no legacy XML args parsing. } diff --git a/src/shared/__tests__/modes.spec.ts b/src/shared/__tests__/modes.spec.ts index a8c04c627c2..0282bd9515c 100644 --- a/src/shared/__tests__/modes.spec.ts +++ b/src/shared/__tests__/modes.spec.ts @@ -276,21 +276,21 @@ describe("isToolAllowedForMode", () => { it("applies restrictions to apply_patch (custom tool)", () => { // Test that apply_patch respects file restrictions when included + // Note: apply_patch only accepts { patch: string } - file paths are embedded in patch content const patchResult = isToolAllowedForMode( "apply_patch", "markdown-editor", customModes, undefined, { - path: "test.md", - patch: "diff --git a/test.md b/test.md\n--- a/test.md\n+++ b/test.md\n@@ -1 +1 @@\n-old\n+new", + patch: "*** Begin Patch\n*** Update File: test.md\n@@ \n-old\n+new\n*** End Patch", }, undefined, ["apply_patch"], // Include custom tool ) expect(patchResult).toBe(true) - // Test apply_patch with non-matching file + // Test apply_patch with non-matching file (file path embedded in patch content) expect(() => isToolAllowedForMode( "apply_patch", @@ -298,8 +298,7 @@ describe("isToolAllowedForMode", () => { customModes, undefined, { - path: "test.js", - patch: "diff --git a/test.js b/test.js\n--- a/test.js\n+++ b/test.js\n@@ -1 +1 @@\n-old\n+new", + patch: "*** Begin Patch\n*** Update File: test.js\n@@ \n-old\n+new\n*** End Patch", }, undefined, ["apply_patch"], // Include custom tool @@ -312,8 +311,7 @@ describe("isToolAllowedForMode", () => { customModes, undefined, { - path: "test.js", - patch: "diff --git a/test.js b/test.js\n--- a/test.js\n+++ b/test.js\n@@ -1 +1 @@\n-old\n+new", + patch: "*** Begin Patch\n*** Update File: test.js\n@@ \n-old\n+new\n*** End Patch", }, undefined, ["apply_patch"], // Include custom tool @@ -423,6 +421,7 @@ describe("isToolAllowedForMode", () => { it("applies restrictions to all editing tools in architect mode (custom tools)", () => { // Test apply_patch in architect mode + // Note: apply_patch only accepts { patch: string } - file paths are embedded in patch content expect( isToolAllowedForMode( "apply_patch", @@ -430,8 +429,7 @@ describe("isToolAllowedForMode", () => { [], undefined, { - path: "test.md", - patch: "diff --git a/test.md b/test.md\n--- a/test.md\n+++ b/test.md\n@@ -1 +1 @@\n-old\n+new", + patch: "*** Begin Patch\n*** Update File: test.md\n@@ \n-old\n+new\n*** End Patch", }, undefined, ["apply_patch"], // Include custom tool @@ -445,8 +443,7 @@ describe("isToolAllowedForMode", () => { [], undefined, { - path: "test.js", - patch: "diff --git a/test.js b/test.js\n--- a/test.js\n+++ b/test.js\n@@ -1 +1 @@\n-old\n+new", + patch: "*** Begin Patch\n*** Update File: test.js\n@@ \n-old\n+new\n*** End Patch", }, undefined, ["apply_patch"], // Include custom tool