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
This commit is contained in:
Roo Code 2026-05-12 06:17:03 +00:00
parent d9a172d2ed
commit eb256220e7
5 changed files with 165 additions and 15 deletions

View file

@ -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"])
}
})
})
})

View file

@ -1,5 +1,7 @@
import { z } from "zod"
import { taskPermissionsSchema } from "./task-permissions.js"
/**
* HistoryItem
*/
@ -26,6 +28,7 @@ export const historyItemSchema = z.object({
awaitingChildId: z.string().optional(), // Child currently awaited (set when delegated)
completedByChildId: z.string().optional(), // Child that completed and resumed this parent
completionResultSummary: z.string().optional(), // Summary from completed child
taskPermissions: taskPermissionsSchema.optional(), // Permission boundaries set by parent task
})
export type HistoryItem = z.infer<typeof historyItemSchema>

View file

@ -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({
/**

View file

@ -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 }

View file

@ -41,6 +41,7 @@ import {
getApiProtocol,
type TaskPermissions,
mergeTaskPermissions,
toTaskPermissions,
getModelId,
isRetiredProvider,
isIdleAsk,
@ -460,8 +461,13 @@ export class Task extends EventEmitter<TaskEvents> 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,
@ -1182,6 +1188,19 @@ export class Task extends EventEmitter<TaskEvents> 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,
@ -1193,6 +1212,10 @@ export class Task extends EventEmitter<TaskEvents> 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