From 311a2bdfb4f404a760f1ec0467689fb5ddc384d1 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 06:04:41 +0000 Subject: [PATCH] fix: harden task permissions - anchor regex patterns, validate at schema level, simplify validation code 1. Anchor regex patterns in matchesAnyPattern with ^(?:...)$ wrapping so patterns like "src/.*" require full-path matching instead of substring matching. Prevents "evil/src/foo" from matching a "src/.*" permission. 2. Add regex validation at schema level (regexString refinement) so invalid patterns are rejected at parse time rather than silently failing at runtime. 3. Simplify duplicate file/command pattern validation in validateToolUse by unifying layered and flat code paths into a single branch that falls back to wrapping flat patterns as a single layer. 4. Remove unused matchesAnyPattern import from validateToolUse.ts. 5. Add tests for anchoring behavior, pre-anchored patterns, and invalid regex rejection at schema level. --- .../src/__tests__/task-permissions.spec.ts | 27 +++++++++ packages/types/src/task-permissions.ts | 27 +++++++-- src/core/tools/validateToolUse.ts | 56 ++++--------------- 3 files changed, 61 insertions(+), 49 deletions(-) diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts index 1fae674145..cea98cff38 100644 --- a/packages/types/src/__tests__/task-permissions.spec.ts +++ b/packages/types/src/__tests__/task-permissions.spec.ts @@ -38,6 +38,20 @@ describe("TaskPermissions", () => { }) expect(result.success).toBe(false) }) + + it("rejects invalid regex patterns in filePatterns", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["[invalid"], + }) + expect(result.success).toBe(false) + }) + + it("rejects invalid regex patterns in commandPatterns", () => { + const result = taskPermissionsSchema.safeParse({ + commandPatterns: ["(unclosed"], + }) + expect(result.success).toBe(false) + }) }) describe("toTaskPermissions", () => { @@ -191,6 +205,19 @@ describe("TaskPermissions", () => { it("does not match restricted commands", () => { expect(matchesAnyPattern("rm -rf /", ["npm.*", "yarn.*"])).toBe(false) }) + + it("anchors patterns so substrings do not match", () => { + // "src/.*" should NOT match a path that merely contains "src/" as a substring + expect(matchesAnyPattern("evil/src/components/foo.ts", ["src/.*"])).toBe(false) + // But should still match paths that start with src/ + expect(matchesAnyPattern("src/components/foo.ts", ["src/.*"])).toBe(true) + }) + + it("respects pre-anchored patterns (starting with ^)", () => { + // A pattern already starting with ^ should not be double-wrapped + expect(matchesAnyPattern("src/foo.ts", ["^src/.*$"])).toBe(true) + expect(matchesAnyPattern("evil/src/foo.ts", ["^src/.*$"])).toBe(false) + }) }) describe("matchesAllPatternLayers", () => { diff --git a/packages/types/src/task-permissions.ts b/packages/types/src/task-permissions.ts index a7e1daebc6..61f5001ffe 100644 --- a/packages/types/src/task-permissions.ts +++ b/packages/types/src/task-permissions.ts @@ -9,20 +9,34 @@ import { z } from "zod" * more access than its parent. */ +/** Zod refinement that rejects strings which are not valid regular expressions. */ +const regexString = z.string().refine( + (val) => { + try { + new RegExp(val) + return true + } catch { + return false + } + }, + { message: "Invalid regular expression" }, +) + export const taskPermissionsSchema = z.object({ /** * Regex patterns for allowed file paths. * When set, file operations (read/write) are restricted to paths matching - * at least one of these patterns. + * at least one of these patterns. Patterns are automatically anchored + * (wrapped in `^(?:...)$`) at runtime so they match the full path. */ - filePatterns: z.array(z.string()).optional(), + filePatterns: z.array(regexString).optional(), /** * Regex patterns for allowed shell commands. * When set, command execution is restricted to commands matching - * at least one of these patterns. + * at least one of these patterns. Patterns are automatically anchored. */ - commandPatterns: z.array(z.string()).optional(), + commandPatterns: z.array(regexString).optional(), /** * Explicit tool allowlist. When set, only these tools may be used @@ -174,7 +188,10 @@ function collectPatternLayers( export function matchesAnyPattern(value: string, patterns: string[]): boolean { return patterns.some((pattern) => { try { - return new RegExp(pattern).test(value) + // Anchor patterns so they must match the entire value, not a substring. + // This prevents "src/.*" from matching "evil/src/foo". + const anchored = pattern.startsWith("^") ? pattern : `^(?:${pattern})$` + return new RegExp(anchored).test(value) } catch { // Invalid regex -- treat as non-match return false diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index d8459c9d01..6dd9597f0a 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -1,5 +1,5 @@ import type { ToolName, ModeConfig, ExperimentId, GroupOptions, GroupEntry, TaskPermissions } from "@roo-code/types" -import { toolNames as validToolNames, matchesAnyPattern, matchesAllPatternLayers } from "@roo-code/types" +import { toolNames as validToolNames, matchesAllPatternLayers } from "@roo-code/types" import { customToolRegistry } from "@roo-code/core" import { type Mode, FileRestrictionError, getModeBySlug, getGroupName } from "../../shared/modes" @@ -170,9 +170,12 @@ export function isToolAllowedForMode( } } - // Check filePatterns using layered enforcement (AND between layers, OR within each layer). - // Falls back to flat filePatterns if no layers are present. - const filePatternLayers = taskPermissions._filePatternLayers + // Check filePatterns -- use layered enforcement when available (AND between + // layers, OR within each layer), fall back to flat filePatterns as a single layer. + const filePatternLayers = + taskPermissions._filePatternLayers ?? + (taskPermissions.filePatterns?.length ? [taskPermissions.filePatterns] : undefined) + if (filePatternLayers && filePatternLayers.length > 0) { const filePath = toolParams?.path || toolParams?.file_path if (filePath && typeof filePath === "string") { @@ -193,33 +196,13 @@ export function isToolAllowedForMode( } } } - } else if (taskPermissions.filePatterns && taskPermissions.filePatterns.length > 0) { - // Fallback for non-merged permissions (single layer) - const filePath = toolParams?.path || toolParams?.file_path - if (filePath && typeof filePath === "string") { - if (!matchesAnyPattern(filePath, taskPermissions.filePatterns)) { - throw new TaskPermissionError( - tool, - `File "${filePath}" is outside the allowed file patterns: ${taskPermissions.filePatterns.join(", ")}`, - ) - } - } - - if (tool === "apply_patch" && typeof toolParams?.patch === "string") { - const patchFilePaths = extractFilePathsFromPatch(toolParams.patch) - for (const patchFilePath of patchFilePaths) { - if (!matchesAnyPattern(patchFilePath, taskPermissions.filePatterns)) { - throw new TaskPermissionError( - tool, - `File "${patchFilePath}" in patch is outside the allowed file patterns: ${taskPermissions.filePatterns.join(", ")}`, - ) - } - } - } } - // Check commandPatterns using layered enforcement - const commandPatternLayers = taskPermissions._commandPatternLayers + // Check commandPatterns -- same layered approach as filePatterns. + const commandPatternLayers = + taskPermissions._commandPatternLayers ?? + (taskPermissions.commandPatterns?.length ? [taskPermissions.commandPatterns] : undefined) + if (commandPatternLayers && commandPatternLayers.length > 0 && resolvedTool === "execute_command") { const command = toolParams?.command if (command && typeof command === "string") { @@ -227,21 +210,6 @@ export function isToolAllowedForMode( throw new TaskPermissionError(tool, `Command "${command}" is outside the allowed command patterns.`) } } - } else if ( - taskPermissions.commandPatterns && - taskPermissions.commandPatterns.length > 0 && - resolvedTool === "execute_command" - ) { - // Fallback for non-merged permissions (single layer) - const command = toolParams?.command - if (command && typeof command === "string") { - if (!matchesAnyPattern(command, taskPermissions.commandPatterns)) { - throw new TaskPermissionError( - tool, - `Command "${command}" is outside the allowed command patterns: ${taskPermissions.commandPatterns.join(", ")}`, - ) - } - } } }