From caf552194f8b33bc3ca8f9f2d0b7dbd5aceaa7ba Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 12 May 2026 06:17:03 +0000 Subject: [PATCH] fix: persist permissions in HistoryItem and add ReDoS mitigation 1. Persist taskPermissions in HistoryItem so permissions survive task restarts. Added taskPermissions field to historyItemSchema, included it in taskMetadata output, and restored it in the Task constructor when loading from history. 2. Add ReDoS mitigation for model-provided regex patterns: - isSafeRegex() heuristic rejects nested quantifiers like (a+)+ and overlapping alternations in repeated groups like (a|a)+ - Max pattern length capped at 200 characters - Both checks enforced at schema validation time via Zod refinements - 11 new tests covering ReDoS detection and persistence round-trips --- .../src/__tests__/task-permissions.spec.ts | 76 +++++++++++++++++++ packages/types/src/history.ts | 3 + packages/types/src/task-permissions.ts | 68 ++++++++++++++--- src/core/task-persistence/taskMetadata.ts | 6 +- src/core/task/Task.ts | 27 ++++++- 5 files changed, 165 insertions(+), 15 deletions(-) diff --git a/packages/types/src/__tests__/task-permissions.spec.ts b/packages/types/src/__tests__/task-permissions.spec.ts index cea98cff38..48990c9875 100644 --- a/packages/types/src/__tests__/task-permissions.spec.ts +++ b/packages/types/src/__tests__/task-permissions.spec.ts @@ -5,6 +5,7 @@ import { matchesAllPatternLayers, taskPermissionsSchema, toTaskPermissions, + isSafeRegex, } from "../task-permissions.js" import type { TaskPermissions } from "../task-permissions.js" @@ -246,4 +247,79 @@ describe("TaskPermissions", () => { expect(matchesAllPatternLayers("docs/readme.md", layers)).toBe(false) }) }) + + describe("isSafeRegex", () => { + it("accepts simple file path patterns", () => { + expect(isSafeRegex("src/.*")).toBe(true) + expect(isSafeRegex("src/components/.*\\.tsx")).toBe(true) + expect(isSafeRegex("npm test.*")).toBe(true) + }) + + it("rejects nested quantifiers (classic ReDoS)", () => { + expect(isSafeRegex("(a+)+")).toBe(false) + expect(isSafeRegex("(a*)+")).toBe(false) + expect(isSafeRegex("(a+)*")).toBe(false) + expect(isSafeRegex("(a+){2,}")).toBe(false) + }) + + it("rejects overlapping alternations in repeated groups", () => { + expect(isSafeRegex("(a|a)+")).toBe(false) + expect(isSafeRegex("(.|a)*")).toBe(false) + }) + + it("rejects patterns exceeding maximum length", () => { + const longPattern = "a".repeat(201) + expect(isSafeRegex(longPattern)).toBe(false) + }) + + it("accepts patterns at maximum length", () => { + const maxPattern = "a".repeat(200) + expect(isSafeRegex(maxPattern)).toBe(true) + }) + }) + + describe("schema ReDoS rejection", () => { + it("rejects ReDoS-vulnerable patterns in filePatterns", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["(a+)+"], + }) + expect(result.success).toBe(false) + }) + + it("rejects ReDoS-vulnerable patterns in commandPatterns", () => { + const result = taskPermissionsSchema.safeParse({ + commandPatterns: ["(cmd|cmd)*"], + }) + expect(result.success).toBe(false) + }) + + it("rejects overly long patterns at schema level", () => { + const result = taskPermissionsSchema.safeParse({ + filePatterns: ["a".repeat(201)], + }) + expect(result.success).toBe(false) + }) + }) + + describe("persistence round-trip", () => { + it("taskPermissionsSchema can parse persisted permissions (without internal fields)", () => { + // Simulate what gets persisted: only the input-level fields + const persisted = { + filePatterns: ["src/.*"], + commandPatterns: ["npm test.*"], + allowedTools: ["read_file"], + deniedTools: ["execute_command"], + } + const result = taskPermissionsSchema.safeParse(persisted) + expect(result.success).toBe(true) + if (result.success) { + // Can be converted back to internal representation + const restored = toTaskPermissions(result.data) + expect(restored._filePatternLayers).toEqual([["src/.*"]]) + expect(restored._commandPatternLayers).toEqual([["npm test.*"]]) + expect(restored.allowedTools).toEqual(["read_file"]) + expect(restored.deniedTools).toEqual(["execute_command"]) + } + }) + }) }) diff --git a/packages/types/src/history.ts b/packages/types/src/history.ts index 2b0a681f48..74fef5c53e 100644 --- a/packages/types/src/history.ts +++ b/packages/types/src/history.ts @@ -1,5 +1,7 @@ import { z } from "zod" +import { taskPermissionsSchema } from "./task-permissions.js" + /** * SubtaskQueueItem — a single queued subtask definition for sequential fan-out. * Used by the orchestrator to define a pipeline of subtasks that execute one after another. @@ -52,6 +54,7 @@ export const historyItemSchema = z.object({ subtaskQueue: z.array(subtaskQueueItemSchema).optional(), // Remaining subtasks to execute subtaskQueueIndex: z.number().optional(), // Current position in the original queue (0-based) subtaskResults: z.array(subtaskResultSchema).optional(), // Results from completed queue subtasks + taskPermissions: taskPermissionsSchema.optional(), // Permission boundaries set by parent task }) export type HistoryItem = z.infer diff --git a/packages/types/src/task-permissions.ts b/packages/types/src/task-permissions.ts index 61f5001ffe..938b321ec7 100644 --- a/packages/types/src/task-permissions.ts +++ b/packages/types/src/task-permissions.ts @@ -9,18 +9,62 @@ 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" }, -) +/** Maximum allowed length for a regex pattern to limit complexity. */ +const MAX_REGEX_PATTERN_LENGTH = 200 + +/** + * Heuristic check for ReDoS-vulnerable patterns. + * Detects common dangerous constructs like nested quantifiers: + * (a+)+, (a*)+, (a+)*, (a*){2,}, etc. + * These can cause catastrophic backtracking on crafted input. + */ +export function isSafeRegex(pattern: string): boolean { + if (pattern.length > MAX_REGEX_PATTERN_LENGTH) { + return false + } + + // Detect nested quantifiers: a group with a quantifier inside, followed by an outer quantifier. + // Examples: (a+)+, (a+)*, (a*){2,}, (?:a+)+ + // This regex looks for: group containing a quantifier, followed by another quantifier + const nestedQuantifierPattern = /\([^)]*[+*][^)]*\)[+*{]/ + if (nestedQuantifierPattern.test(pattern)) { + return false + } + + // Detect overlapping alternations inside repeated groups: (a|a)+, (.|a)+ + // where both alternatives can match the same input + const overlappingAlternationInGroup = /\([^)]*\|[^)]*\)[+*{]/ + if (overlappingAlternationInGroup.test(pattern)) { + return false + } + + return true +} + +/** + * Zod refinement that rejects strings which are not valid regular expressions, + * and also rejects patterns that are vulnerable to ReDoS (catastrophic backtracking). + */ +const regexString = z + .string() + .max(MAX_REGEX_PATTERN_LENGTH, { + message: `Regex pattern must be at most ${MAX_REGEX_PATTERN_LENGTH} characters`, + }) + .refine( + (val) => { + try { + new RegExp(val) + return true + } catch { + return false + } + }, + { message: "Invalid regular expression" }, + ) + .refine((val) => isSafeRegex(val), { + message: + "Regex pattern rejected: potentially vulnerable to ReDoS (catastrophic backtracking). Avoid nested quantifiers like (a+)+ or overlapping alternations in repeated groups.", + }) export const taskPermissionsSchema = z.object({ /** diff --git a/src/core/task-persistence/taskMetadata.ts b/src/core/task-persistence/taskMetadata.ts index 4b77126971..83dee5fb47 100644 --- a/src/core/task-persistence/taskMetadata.ts +++ b/src/core/task-persistence/taskMetadata.ts @@ -1,7 +1,7 @@ import NodeCache from "node-cache" import getFolderSize from "get-folder-size" -import type { ClineMessage, HistoryItem } from "@roo-code/types" +import type { ClineMessage, HistoryItem, TaskPermissionsInput } from "@roo-code/types" import { combineApiRequests } from "../../shared/combineApiRequests" import { combineCommandSequences } from "../../shared/combineCommandSequences" @@ -25,6 +25,8 @@ export type TaskMetadataOptions = { apiConfigName?: string /** Initial status for the task (e.g., "active" for child tasks) */ initialStatus?: "active" | "delegated" | "completed" + /** Permission boundaries for the task, set by the parent via new_task tool */ + taskPermissions?: TaskPermissionsInput } export async function taskMetadata({ @@ -38,6 +40,7 @@ export async function taskMetadata({ mode, apiConfigName, initialStatus, + taskPermissions, }: TaskMetadataOptions) { const taskDir = await getTaskDirectoryPath(globalStoragePath, id) @@ -112,6 +115,7 @@ export async function taskMetadata({ mode, ...(typeof apiConfigName === "string" && apiConfigName.length > 0 ? { apiConfigName } : {}), ...(initialStatus && { status: initialStatus }), + ...(taskPermissions && { taskPermissions }), } return { historyItem, tokenUsage } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index df7acbdcb7..a4d836af8b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -41,6 +41,7 @@ import { getApiProtocol, type TaskPermissions, mergeTaskPermissions, + toTaskPermissions, getModelId, isRetiredProvider, isIdleAsk, @@ -480,8 +481,13 @@ export class Task extends EventEmitter implements TaskLike { this.parentTaskId = historyItem ? historyItem.parentTaskId : parentTask?.taskId this.childTaskId = undefined - // Merge task permissions with parent (most-restrictive-wins) - this.taskPermissions = mergeTaskPermissions(parentTask?.taskPermissions, taskPermissions) + // Merge task permissions with parent (most-restrictive-wins). + // When restoring from history, use the persisted permissions as the base; + // when creating fresh, use the permissions passed via new_task tool. + const effectivePermissions = historyItem?.taskPermissions + ? toTaskPermissions(historyItem.taskPermissions) + : taskPermissions + this.taskPermissions = mergeTaskPermissions(parentTask?.taskPermissions, effectivePermissions) this.metadata = { task: historyItem ? historyItem.task : task, @@ -1211,6 +1217,19 @@ export class Task extends EventEmitter implements TaskLike { await this.taskApiConfigReady } + // Serialize only the input-level permission fields for persistence + // (exclude internal _*PatternLayers fields which are runtime-only) + const persistablePermissions = this.taskPermissions + ? { + ...(this.taskPermissions.filePatterns && { filePatterns: this.taskPermissions.filePatterns }), + ...(this.taskPermissions.commandPatterns && { + commandPatterns: this.taskPermissions.commandPatterns, + }), + ...(this.taskPermissions.allowedTools && { allowedTools: this.taskPermissions.allowedTools }), + ...(this.taskPermissions.deniedTools && { deniedTools: this.taskPermissions.deniedTools }), + } + : undefined + const { historyItem, tokenUsage } = await taskMetadata({ taskId: this.taskId, rootTaskId: this.rootTaskId, @@ -1222,6 +1241,10 @@ export class Task extends EventEmitter implements TaskLike { mode: this._taskMode || defaultModeSlug, // Use the task's own mode, not the current provider mode. apiConfigName: this._taskApiConfigName, // Use the task's own provider profile, not the current provider profile. initialStatus: this.initialStatus, + taskPermissions: + persistablePermissions && Object.keys(persistablePermissions).length > 0 + ? persistablePermissions + : undefined, }) // Emit token/tool usage updates using debounced function