mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: enforce mode file restrictions for apply_diff tool (#4732)
- Modified isToolAllowedForMode to validate file paths in apply_diff args parameter - Added extraction of paths from XML args for multi-file apply_diff operations - Added comprehensive tests for both single and multi-file scenarios - Ensures mode permissions are properly enforced for all edit tools
This commit is contained in:
Daniel Riccio
parent
2e2f83be60
commit
e0699cdff8
2 changed files with 169 additions and 44 deletions
|
|
@ -94,56 +94,99 @@ describe("isToolAllowedForMode", () => {
|
|||
})
|
||||
|
||||
it("handles partial streaming cases (path only, no content/diff)", () => {
|
||||
// Should allow path-only for matching files (no validation yet since content/diff not provided)
|
||||
expect(
|
||||
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toBe(true)
|
||||
// Should enforce file restrictions even when only path is provided (streaming scenario)
|
||||
// This prevents bypassing restrictions when parameters arrive separately
|
||||
|
||||
expect(
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toBe(true)
|
||||
|
||||
// Should allow path-only for architect mode too
|
||||
expect(
|
||||
isToolAllowedForMode("write_to_file", "architect", [], undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it("applies restrictions to both write_to_file and apply_diff", () => {
|
||||
// Test write_to_file
|
||||
const writeResult = isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
|
||||
path: "test.md",
|
||||
content: "# Test",
|
||||
})
|
||||
expect(writeResult).toBe(true)
|
||||
|
||||
// Test apply_diff
|
||||
const diffResult = isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
path: "test.md",
|
||||
diff: "- old\n+ new",
|
||||
})
|
||||
expect(diffResult).toBe(true)
|
||||
|
||||
// Test both with non-matching file
|
||||
// Non-matching files should be rejected immediately when path is provided
|
||||
expect(() =>
|
||||
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
content: "console.log('test')",
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
expect(() =>
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
// Matching files should be allowed even with path-only
|
||||
expect(
|
||||
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
|
||||
path: "test.md",
|
||||
}),
|
||||
).toBe(true)
|
||||
|
||||
expect(
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
path: "test.md",
|
||||
}),
|
||||
).toBe(true)
|
||||
|
||||
// Architect mode (built-in) allows markdown files
|
||||
expect(
|
||||
isToolAllowedForMode("write_to_file", "architect", [], undefined, {
|
||||
path: "test.md",
|
||||
}),
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it("applies restrictions to all edit tools", () => {
|
||||
// Test all edit tools with matching files
|
||||
const editTools = ["write_to_file", "apply_diff", "insert_content", "search_and_replace"]
|
||||
|
||||
editTools.forEach((tool) => {
|
||||
// Matching file should be allowed
|
||||
const params: any = { path: "test.md" }
|
||||
if (tool === "write_to_file") params.content = "# Test"
|
||||
if (tool === "apply_diff") params.diff = "- old\n+ new"
|
||||
if (tool === "insert_content") params.content = "new line"
|
||||
if (tool === "search_and_replace") {
|
||||
params.search = "old"
|
||||
params.replace = "new"
|
||||
}
|
||||
|
||||
expect(isToolAllowedForMode(tool as any, "markdown-editor", customModes, undefined, params)).toBe(true)
|
||||
|
||||
// Non-matching file should be rejected
|
||||
params.path = "test.js"
|
||||
expect(() =>
|
||||
isToolAllowedForMode(tool as any, "markdown-editor", customModes, undefined, params),
|
||||
).toThrow(FileRestrictionError)
|
||||
})
|
||||
})
|
||||
|
||||
it("prevents bypassing restrictions via parameter streaming", () => {
|
||||
// This test specifically addresses the bug where apply_diff could bypass
|
||||
// file restrictions when path arrives before diff content
|
||||
|
||||
// Step 1: Path arrives first (should be rejected for non-matching files)
|
||||
expect(() =>
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js", // Non-matching file
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
// Step 2: Even when diff arrives later, it should still be rejected
|
||||
expect(() =>
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
diff: "- old\n+ new",
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
// Same for other edit tools
|
||||
expect(() =>
|
||||
isToolAllowedForMode("insert_content", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
expect(() =>
|
||||
isToolAllowedForMode("search_and_replace", "markdown-editor", customModes, undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
})
|
||||
|
||||
it("uses description in file restriction error for custom modes", () => {
|
||||
|
|
@ -203,11 +246,18 @@ describe("isToolAllowedForMode", () => {
|
|||
}),
|
||||
).toBe(true)
|
||||
|
||||
// Test partial streaming cases
|
||||
expect(
|
||||
// Test partial streaming cases - should now reject non-matching files immediately
|
||||
expect(() =>
|
||||
isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
|
||||
path: "test.js",
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
// But should allow matching files even with path-only
|
||||
expect(
|
||||
isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
|
||||
path: "test.md",
|
||||
}),
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
|
|
@ -247,6 +297,72 @@ describe("isToolAllowedForMode", () => {
|
|||
expect(isToolAllowedForMode("browser_action", "architect", [])).toBe(true)
|
||||
expect(isToolAllowedForMode("use_mcp_tool", "architect", [])).toBe(true)
|
||||
})
|
||||
it("validates paths in multi-file apply_diff args parameter", () => {
|
||||
// Test multi-file apply_diff with XML args containing non-matching files
|
||||
const argsWithNonMatchingFile = `<args>
|
||||
<file>
|
||||
<path>test.js</path>
|
||||
<diff>
|
||||
<content>- old
|
||||
+ new</content>
|
||||
</diff>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
expect(() =>
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
args: argsWithNonMatchingFile,
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
// Test with multiple files, one non-matching
|
||||
const argsWithMixedFiles = `<args>
|
||||
<file>
|
||||
<path>doc.md</path>
|
||||
<diff>
|
||||
<content>- old
|
||||
+ new</content>
|
||||
</diff>
|
||||
</file>
|
||||
<file>
|
||||
<path>script.js</path>
|
||||
<diff>
|
||||
<content>- old
|
||||
+ new</content>
|
||||
</diff>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
expect(() =>
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
args: argsWithMixedFiles,
|
||||
}),
|
||||
).toThrow(FileRestrictionError)
|
||||
|
||||
// Test with only matching files (should pass)
|
||||
const argsWithMatchingFiles = `<args>
|
||||
<file>
|
||||
<path>doc1.md</path>
|
||||
<diff>
|
||||
<content>- old
|
||||
+ new</content>
|
||||
</diff>
|
||||
</file>
|
||||
<file>
|
||||
<path>doc2.md</path>
|
||||
<diff>
|
||||
<content>- old
|
||||
+ new</content>
|
||||
</diff>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
expect(
|
||||
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
|
||||
args: argsWithMatchingFiles,
|
||||
}),
|
||||
).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
it("handles non-existent modes", () => {
|
||||
|
|
|
|||
|
|
@ -257,14 +257,23 @@ export function isToolAllowedForMode(
|
|||
|
||||
// For the edit group, check file regex if specified
|
||||
if (groupName === "edit" && options.fileRegex) {
|
||||
// Check direct path parameter
|
||||
const filePath = toolParams?.path
|
||||
if (
|
||||
filePath &&
|
||||
(toolParams.diff || toolParams.content || toolParams.operations) &&
|
||||
!doesFileMatchRegex(filePath, options.fileRegex)
|
||||
) {
|
||||
if (filePath && !doesFileMatchRegex(filePath, options.fileRegex)) {
|
||||
throw new FileRestrictionError(mode.name, options.fileRegex, options.description, filePath)
|
||||
}
|
||||
|
||||
// For apply_diff with multi-file support, also check paths in args parameter
|
||||
if (tool === "apply_diff" && toolParams?.args && typeof toolParams.args === "string") {
|
||||
// Extract all file paths from the XML args
|
||||
const pathMatches = toolParams.args.matchAll(/<path>([^<]+)<\/path>/g)
|
||||
for (const match of pathMatches) {
|
||||
const xmlPath = match[1]
|
||||
if (xmlPath && !doesFileMatchRegex(xmlPath, options.fileRegex)) {
|
||||
throw new FileRestrictionError(mode.name, options.fileRegex, options.description, xmlPath)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return true
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue