mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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:
parent
311a2bdfb4
commit
caf552194f
5 changed files with 165 additions and 15 deletions
|
|
@ -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"])
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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<typeof historyItemSchema>
|
||||
|
|
|
|||
|
|
@ -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({
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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 }
|
||||
|
|
|
|||
|
|
@ -41,6 +41,7 @@ import {
|
|||
getApiProtocol,
|
||||
type TaskPermissions,
|
||||
mergeTaskPermissions,
|
||||
toTaskPermissions,
|
||||
getModelId,
|
||||
isRetiredProvider,
|
||||
isIdleAsk,
|
||||
|
|
@ -480,8 +481,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,
|
||||
|
|
@ -1211,6 +1217,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,
|
||||
|
|
@ -1222,6 +1241,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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue