mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
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.
This commit is contained in:
parent
c8e6b1778b
commit
311a2bdfb4
3 changed files with 61 additions and 49 deletions
|
|
@ -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", () => {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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(", ")}`,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue