mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: persist task permissions in HistoryItem and add ReDoS mitigation
- Add taskPermissions field to historyItemSchema so permissions survive VS Code restarts - Pass taskPermissions through taskMetadata when saving task history - Restore permissions via toTaskPermissions() in createTaskWithHistoryItem - Add isSafeRegex() heuristic that detects nested quantifiers and excessive pattern length (ReDoS vectors) - Apply safety check in both Zod schema validation (parse time) and matchesAnyPattern() (runtime belt-and-suspenders) - Add 32 new tests covering isSafeRegex, schema rejection of unsafe patterns, runtime skipping, HistoryItem round-trip, and edge cases Addresses remaining considerations from PR #12337 review before merge.
This commit is contained in:
parent
d9a172d2ed
commit
c2711b687e
6 changed files with 264 additions and 14 deletions
|
|
@ -5,7 +5,9 @@ import {
|
|||
matchesAllPatternLayers,
|
||||
taskPermissionsSchema,
|
||||
toTaskPermissions,
|
||||
isSafeRegex,
|
||||
} from "../task-permissions.js"
|
||||
import { historyItemSchema } from "../history.js"
|
||||
import type { TaskPermissions } from "../task-permissions.js"
|
||||
|
||||
describe("TaskPermissions", () => {
|
||||
|
|
@ -246,4 +248,174 @@ describe("TaskPermissions", () => {
|
|||
expect(matchesAllPatternLayers("docs/readme.md", layers)).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe("isSafeRegex", () => {
|
||||
it("allows simple patterns", () => {
|
||||
expect(isSafeRegex("src/.*")).toBe(true)
|
||||
expect(isSafeRegex("^foo$")).toBe(true)
|
||||
expect(isSafeRegex("[a-z]+")).toBe(true)
|
||||
expect(isSafeRegex("npm\\s+test.*")).toBe(true)
|
||||
expect(isSafeRegex("src/components/.*\\.tsx?")).toBe(true)
|
||||
})
|
||||
|
||||
it("rejects nested quantifiers (a+)+", () => {
|
||||
expect(isSafeRegex("(a+)+")).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects nested quantifiers (a*)*", () => {
|
||||
expect(isSafeRegex("(a*)*")).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects nested quantifiers (.*)+", () => {
|
||||
expect(isSafeRegex("(.*)+")).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects nested quantifiers with braces (a{1,})+", () => {
|
||||
expect(isSafeRegex("(a{1,})+")).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects star-height > 1 patterns like (.+)+", () => {
|
||||
expect(isSafeRegex("(.+)+")).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects star-height > 1 patterns like (\\w+)*", () => {
|
||||
expect(isSafeRegex("(\\w+)*")).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects patterns that exceed max length", () => {
|
||||
const longPattern = "a".repeat(501)
|
||||
expect(isSafeRegex(longPattern)).toBe(false)
|
||||
})
|
||||
|
||||
it("allows patterns at max length", () => {
|
||||
const maxPattern = "a".repeat(500)
|
||||
expect(isSafeRegex(maxPattern)).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("schema rejects unsafe regex", () => {
|
||||
it("rejects nested quantifiers in filePatterns", () => {
|
||||
const result = taskPermissionsSchema.safeParse({
|
||||
filePatterns: ["(a+)+"],
|
||||
})
|
||||
expect(result.success).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects nested quantifiers in commandPatterns", () => {
|
||||
const result = taskPermissionsSchema.safeParse({
|
||||
commandPatterns: ["(.*)+"],
|
||||
})
|
||||
expect(result.success).toBe(false)
|
||||
})
|
||||
|
||||
it("allows safe regex in schema", () => {
|
||||
const result = taskPermissionsSchema.safeParse({
|
||||
filePatterns: ["src/components/.*\\.tsx?"],
|
||||
commandPatterns: ["npm\\s+test.*"],
|
||||
})
|
||||
expect(result.success).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("matchesAnyPattern skips unsafe patterns at runtime", () => {
|
||||
it("skips unsafe pattern and does not match", () => {
|
||||
// (a+)+ is a ReDoS-prone pattern -- should be skipped
|
||||
expect(matchesAnyPattern("aaa", ["(a+)+"])).toBe(false)
|
||||
})
|
||||
|
||||
it("still matches safe patterns alongside unsafe ones", () => {
|
||||
// The safe pattern "aaa" should still work
|
||||
expect(matchesAnyPattern("aaa", ["(a+)+", "aaa"])).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("HistoryItem taskPermissions persistence", () => {
|
||||
it("accepts a HistoryItem with taskPermissions", () => {
|
||||
const result = historyItemSchema.safeParse({
|
||||
id: "test-task",
|
||||
number: 1,
|
||||
ts: Date.now(),
|
||||
task: "Test task",
|
||||
tokensIn: 100,
|
||||
tokensOut: 50,
|
||||
totalCost: 0.01,
|
||||
taskPermissions: {
|
||||
filePatterns: ["src/.*"],
|
||||
commandPatterns: ["npm test.*"],
|
||||
allowedTools: ["read_file"],
|
||||
deniedTools: ["execute_command"],
|
||||
},
|
||||
})
|
||||
expect(result.success).toBe(true)
|
||||
if (result.success) {
|
||||
expect(result.data.taskPermissions).toBeDefined()
|
||||
expect(result.data.taskPermissions?.filePatterns).toEqual(["src/.*"])
|
||||
}
|
||||
})
|
||||
|
||||
it("accepts a HistoryItem without taskPermissions", () => {
|
||||
const result = historyItemSchema.safeParse({
|
||||
id: "test-task",
|
||||
number: 1,
|
||||
ts: Date.now(),
|
||||
task: "Test task",
|
||||
tokensIn: 100,
|
||||
tokensOut: 50,
|
||||
totalCost: 0.01,
|
||||
})
|
||||
expect(result.success).toBe(true)
|
||||
if (result.success) {
|
||||
expect(result.data.taskPermissions).toBeUndefined()
|
||||
}
|
||||
})
|
||||
|
||||
it("rejects HistoryItem with invalid taskPermissions regex", () => {
|
||||
const result = historyItemSchema.safeParse({
|
||||
id: "test-task",
|
||||
number: 1,
|
||||
ts: Date.now(),
|
||||
task: "Test task",
|
||||
tokensIn: 100,
|
||||
tokensOut: 50,
|
||||
totalCost: 0.01,
|
||||
taskPermissions: {
|
||||
filePatterns: ["[invalid"],
|
||||
},
|
||||
})
|
||||
expect(result.success).toBe(false)
|
||||
})
|
||||
|
||||
it("rejects HistoryItem with unsafe ReDoS patterns", () => {
|
||||
const result = historyItemSchema.safeParse({
|
||||
id: "test-task",
|
||||
number: 1,
|
||||
ts: Date.now(),
|
||||
task: "Test task",
|
||||
tokensIn: 100,
|
||||
tokensOut: 50,
|
||||
totalCost: 0.01,
|
||||
taskPermissions: {
|
||||
filePatterns: ["(a+)+"],
|
||||
},
|
||||
})
|
||||
expect(result.success).toBe(false)
|
||||
})
|
||||
|
||||
it("round-trips permissions through toTaskPermissions", () => {
|
||||
const input = {
|
||||
filePatterns: ["src/.*"],
|
||||
commandPatterns: ["npm.*"],
|
||||
allowedTools: ["read_file"],
|
||||
deniedTools: ["execute_command"],
|
||||
}
|
||||
const internal = toTaskPermissions(input)
|
||||
expect(internal._filePatternLayers).toEqual([["src/.*"]])
|
||||
expect(internal._commandPatternLayers).toEqual([["npm.*"]])
|
||||
// Serializable fields survive round-trip
|
||||
expect(internal.filePatterns).toEqual(input.filePatterns)
|
||||
expect(internal.commandPatterns).toEqual(input.commandPatterns)
|
||||
expect(internal.allowedTools).toEqual(input.allowedTools)
|
||||
expect(internal.deniedTools).toEqual(input.deniedTools)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,5 +1,7 @@
|
|||
import { z } from "zod"
|
||||
|
||||
import { taskPermissionsSchema } from "./task-permissions.js"
|
||||
|
||||
/**
|
||||
* HistoryItem
|
||||
*/
|
||||
|
|
@ -26,6 +28,9 @@ 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
|
||||
/** Persisted task permissions set by parent via new_task tool.
|
||||
* Allows permissions to survive VS Code restarts for subtasks. */
|
||||
taskPermissions: taskPermissionsSchema.optional(),
|
||||
})
|
||||
|
||||
export type HistoryItem = z.infer<typeof historyItemSchema>
|
||||
|
|
|
|||
|
|
@ -9,18 +9,67 @@ 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 string.
|
||||
* Keeps patterns short to limit the surface area for ReDoS.
|
||||
*/
|
||||
const MAX_REGEX_LENGTH = 500
|
||||
|
||||
/**
|
||||
* Heuristic check for ReDoS-prone regex patterns.
|
||||
*
|
||||
* Detects common dangerous constructs:
|
||||
* - Nested quantifiers: `(a+)+`, `(a*)*`, `(a+)*`, `(a{1,})+` etc.
|
||||
* - Overlapping alternation with quantifiers that can cause exponential backtracking
|
||||
*
|
||||
* This is a conservative heuristic -- it may reject some safe patterns,
|
||||
* but it will catch the most common ReDoS vectors.
|
||||
*
|
||||
* @returns `true` if the pattern appears safe, `false` if it looks dangerous.
|
||||
*/
|
||||
export function isSafeRegex(pattern: string): boolean {
|
||||
if (pattern.length > MAX_REGEX_LENGTH) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Detect nested quantifiers: a group/char-class followed by a quantifier,
|
||||
// itself followed by another quantifier.
|
||||
// e.g., (a+)+ (\w+)* [a-z]+* (.+){2,}+
|
||||
// Pattern: something quantified inside a group, then the group is quantified again.
|
||||
// We look for `)` followed by a quantifier, preceded by content that also has a quantifier.
|
||||
const nestedQuantifierRe = /(\+|\*|\?|\{[0-9,]+\})\s*\)(\+|\*|\?|\{[0-9,]+\})/
|
||||
if (nestedQuantifierRe.test(pattern)) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Detect star-height > 1 patterns like (.+)+ (.*)* (.+)*
|
||||
const starHeightRe = /\(([^)]*(\+|\*|\{[0-9,]+\})[^)]*)\)(\+|\*|\{[0-9,]+\})/
|
||||
if (starHeightRe.test(pattern)) {
|
||||
return false
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
/** Zod refinement that rejects strings which are not valid regular expressions
|
||||
* or that contain potentially dangerous ReDoS patterns. */
|
||||
const regexString = z
|
||||
.string()
|
||||
.refine(
|
||||
(val) => {
|
||||
try {
|
||||
new RegExp(val)
|
||||
return true
|
||||
} catch {
|
||||
return false
|
||||
}
|
||||
},
|
||||
{ message: "Invalid regular expression" },
|
||||
)
|
||||
.refine((val) => isSafeRegex(val), {
|
||||
message:
|
||||
"Regex pattern is potentially unsafe (nested quantifiers or excessive length). Simplify the pattern to avoid ReDoS risk.",
|
||||
})
|
||||
|
||||
export const taskPermissionsSchema = z.object({
|
||||
/**
|
||||
|
|
@ -188,6 +237,11 @@ function collectPatternLayers(
|
|||
export function matchesAnyPattern(value: string, patterns: string[]): boolean {
|
||||
return patterns.some((pattern) => {
|
||||
try {
|
||||
// Skip patterns that fail the safety heuristic at runtime
|
||||
// (belt-and-suspenders: Zod schema also checks at parse time)
|
||||
if (!isSafeRegex(pattern)) {
|
||||
return false
|
||||
}
|
||||
// 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})$`
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
/** Task permissions set by parent via new_task tool, persisted for restart survival */
|
||||
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 }
|
||||
|
|
|
|||
|
|
@ -1193,6 +1193,17 @@ 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,
|
||||
// Persist the serializable (input) permission fields so they survive restarts.
|
||||
// Internal layer fields (_filePatternLayers, _commandPatternLayers) are
|
||||
// recomputed from the flat fields via toTaskPermissions() on restore.
|
||||
taskPermissions: this.taskPermissions
|
||||
? {
|
||||
filePatterns: this.taskPermissions.filePatterns,
|
||||
commandPatterns: this.taskPermissions.commandPatterns,
|
||||
allowedTools: this.taskPermissions.allowedTools,
|
||||
deniedTools: this.taskPermissions.deniedTools,
|
||||
}
|
||||
: undefined,
|
||||
})
|
||||
|
||||
// Emit token/tool usage updates using debounced function
|
||||
|
|
|
|||
|
|
@ -82,7 +82,7 @@ import { CustomModesManager } from "../config/CustomModesManager"
|
|||
import { Task } from "../task/Task"
|
||||
|
||||
import { webviewMessageHandler } from "./webviewMessageHandler"
|
||||
import type { ClineMessage, TodoItem, TaskPermissions } from "@roo-code/types"
|
||||
import { toTaskPermissions, type ClineMessage, type TodoItem, type TaskPermissions } from "@roo-code/types"
|
||||
import { readApiMessages, saveApiMessages, saveTaskMessages, TaskHistoryStore } from "../task-persistence"
|
||||
import { readTaskMessages } from "../task-persistence/taskMessages"
|
||||
import { getNonce } from "./getNonce"
|
||||
|
|
@ -953,6 +953,10 @@ export class ClineProvider
|
|||
startTask: options?.startTask ?? true,
|
||||
// Preserve the status from the history item to avoid overwriting it when the task saves messages
|
||||
initialStatus: historyItem.status,
|
||||
// Restore persisted task permissions from history so they survive VS Code restarts.
|
||||
// toTaskPermissions() converts the flat input fields back into the internal
|
||||
// representation with pattern layers for runtime enforcement.
|
||||
taskPermissions: historyItem.taskPermissions ? toTaskPermissions(historyItem.taskPermissions) : undefined,
|
||||
})
|
||||
|
||||
if (isRehydratingCurrentTask) {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue