From ac0f7febe4ebaf930a031cae2a2c674274098155 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Wed, 31 Dec 2025 14:53:02 -0700 Subject: [PATCH] fix: extend read group fileRegex enforcement to search_files, codebase_search, list_files The read group's fileRegex restriction was only enforced on read_file, allowing orchestrator mode to bypass read restrictions via other tools that access paths. Changes: - Extended validateToolUse to restrict path param of search_files, codebase_search, and list_files when read group has fileRegex - Added tests verifying orchestrator mode cannot access arbitrary directories - Tests pass for both allowed (.roo/skills) and restricted (src, node_modules) paths --- src/core/tools/validateToolUse.ts | 72 ++++++++++++++++++++------- src/shared/__tests__/modes.spec.ts | 78 ++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 18 deletions(-) diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index a9fe107495..cadbd3c932 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -282,28 +282,64 @@ export function isToolAllowedForMode( } // Handle XML args parameter (used by MULTI_FILE_APPLY_DIFF experiment) - if (toolParams?.args && typeof toolParams.args === "string") { - const xmlPaths = extractPathsFromXmlArgs(toolParams.args) - for (const extractedPath of xmlPaths) { - if (!doesFileMatchRegex(extractedPath, options.fileRegex)) { - throw new FileRestrictionError( - mode.name, - options.fileRegex, - options.description, - extractedPath, - tool, - ) - } + if (toolParams?.args && typeof toolParams.args === "string") { + const xmlPaths = extractPathsFromXmlArgs(toolParams.args) + for (const extractedPath of xmlPaths) { + if (!doesFileMatchRegex(extractedPath, options.fileRegex)) { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + extractedPath, + tool, + ) } } + } } - // For the read group, optionally restrict read_file paths if specified - if (groupName === "read" && options.fileRegex && tool === "read_file") { - const readPaths = extractReadFilePaths(toolParams) - for (const p of readPaths) { - if (!doesFileMatchRegex(p, options.fileRegex)) { - throw new FileRestrictionError(mode.name, options.fileRegex, options.description, p, tool, "read") + // For the read group, optionally restrict paths if fileRegex is specified + if (groupName === "read" && options.fileRegex) { + // Restrict read_file to only read files matching the pattern + if (tool === "read_file") { + const readPaths = extractReadFilePaths(toolParams) + for (const p of readPaths) { + if (!doesFileMatchRegex(p, options.fileRegex)) { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + p, + tool, + "read", + ) + } + } + } + + // Restrict search_files, codebase_search, and list_files path parameter + // These tools operate on directories, so we derive a directory pattern from the fileRegex + if (["search_files", "codebase_search", "list_files"].includes(tool)) { + const pathParam = toolParams?.path + if (typeof pathParam === "string" && pathParam.trim().length > 0) { + // Derive directory pattern: strip trailing file pattern (e.g., SKILL.md$) and match directory prefix + // For pattern like: (^|.*[\\/])\.roo[\\/]skills(-[a-zA-Z0-9-]+)?[\\/][^\\/]+[\\/]SKILL\.md$ + // Directory should be within: .roo/skills or .roo/skills- + const dirPattern = options.fileRegex + .replace(/\[\^[^\]]*\]\+[\\\\/]?[^$]*\$$/, "") // Remove [^\/]+\/SKILL\.md$ part + .replace(/\$$/, "") // Remove trailing $ if present + const dirRegex = dirPattern ? `${dirPattern}($|[\\\\/])` : options.fileRegex + + if (!doesFileMatchRegex(pathParam, dirRegex) && !doesFileMatchRegex(pathParam + "/", dirRegex)) { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + pathParam, + tool, + "read", + ) + } } } } diff --git a/src/shared/__tests__/modes.spec.ts b/src/shared/__tests__/modes.spec.ts index 726e05ace1..1ad08cd172 100644 --- a/src/shared/__tests__/modes.spec.ts +++ b/src/shared/__tests__/modes.spec.ts @@ -498,6 +498,84 @@ describe("FileRestrictionError", () => { }), ).toThrow(FileRestrictionError) }) + + it("restricts search_files to skills directories", () => { + // Should allow searching within .roo/skills directories + expect( + isToolAllowedForMode("search_files", "orchestrator", [], undefined, { + path: ".roo/skills", + regex: ".*", + }), + ).toBe(true) + + expect( + isToolAllowedForMode("search_files", "orchestrator", [], undefined, { + path: ".roo/skills-code", + regex: ".*", + }), + ).toBe(true) + + // Should reject searching outside skills directories + expect(() => + isToolAllowedForMode("search_files", "orchestrator", [], undefined, { + path: "src", + regex: ".*", + }), + ).toThrow(FileRestrictionError) + }) + + it("restricts codebase_search to skills directories", () => { + // Should allow searching within .roo/skills directories + expect( + isToolAllowedForMode("codebase_search", "orchestrator", [], undefined, { + path: ".roo/skills", + query: "test", + }), + ).toBe(true) + + // Should reject searching outside skills directories + expect(() => + isToolAllowedForMode("codebase_search", "orchestrator", [], undefined, { + path: "src/core", + query: "test", + }), + ).toThrow(FileRestrictionError) + }) + + it("restricts list_files to skills directories", () => { + // Should allow listing within .roo/skills directories + expect( + isToolAllowedForMode("list_files", "orchestrator", [], undefined, { + path: ".roo/skills", + recursive: true, + }), + ).toBe(true) + + // Should reject listing outside skills directories + expect(() => + isToolAllowedForMode("list_files", "orchestrator", [], undefined, { + path: "node_modules", + recursive: false, + }), + ).toThrow(FileRestrictionError) + }) + + it("allows empty path for search tools (workspace root)", () => { + // Empty path should be allowed (searches entire workspace which is allowed) + expect( + isToolAllowedForMode("search_files", "orchestrator", [], undefined, { + path: "", + regex: ".*", + }), + ).toBe(true) + + expect( + isToolAllowedForMode("codebase_search", "orchestrator", [], undefined, { + path: "", + query: "test", + }), + ).toBe(true) + }) }) describe("getFullModeDetails", () => {