Skip to content

Commit 33e0004

Browse files
committed
Fixes #4732: Enforce mode file restrictions immediately when path is provided
- Fixed bug in isToolAllowedForMode where file regex validation was only triggered when content/diff/operations were present - Mode permissions are now enforced as soon as a file path is provided, preventing bypass during streaming/partial tool calls - Updated tests to reflect correct behavior and added reproduction test - This prevents modes like Architect from editing restricted files like PowerShell scripts when they should only edit markdown/config files
1 parent 2e2f83b commit 33e0004

File tree

2 files changed

+83
-18
lines changed

2 files changed

+83
-18
lines changed

src/shared/__tests__/modes.test.ts

Lines changed: 82 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -93,24 +93,37 @@ describe("isToolAllowedForMode", () => {
9393
).toThrow(/\\.css\$/)
9494
})
9595

96-
it("handles partial streaming cases (path only, no content/diff)", () => {
97-
// Should allow path-only for matching files (no validation yet since content/diff not provided)
98-
expect(
96+
it("enforces file restrictions even when only path is provided", () => {
97+
// File restrictions should be enforced immediately when path is provided, regardless of content/diff
98+
expect(() =>
9999
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
100-
path: "test.js",
100+
path: "test.js", // Should be rejected - doesn't match .md pattern
101101
}),
102-
).toBe(true)
102+
).toThrow(FileRestrictionError)
103103

104-
expect(
104+
expect(() =>
105105
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
106-
path: "test.js",
106+
path: "test.js", // Should be rejected - doesn't match .md pattern
107+
}),
108+
).toThrow(FileRestrictionError)
109+
110+
// Should reject non-markdown files for architect mode too
111+
expect(() =>
112+
isToolAllowedForMode("write_to_file", "architect", [], undefined, {
113+
path: "test.js", // Should be rejected - doesn't match .md pattern
114+
}),
115+
).toThrow(FileRestrictionError)
116+
117+
// But should allow matching files
118+
expect(
119+
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
120+
path: "test.md", // Should be allowed - matches .md pattern
107121
}),
108122
).toBe(true)
109123

110-
// Should allow path-only for architect mode too
111124
expect(
112125
isToolAllowedForMode("write_to_file", "architect", [], undefined, {
113-
path: "test.js",
126+
path: "README.md", // Should be allowed - matches .md pattern
114127
}),
115128
).toBe(true)
116129
})
@@ -203,12 +216,12 @@ describe("isToolAllowedForMode", () => {
203216
}),
204217
).toBe(true)
205218

206-
// Test partial streaming cases
207-
expect(
219+
// Test that non-matching files are rejected even with path only
220+
expect(() =>
208221
isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
209-
path: "test.js",
222+
path: "test.js", // Should be rejected - doesn't match pattern
210223
}),
211-
).toBe(true)
224+
).toThrow(FileRestrictionError)
212225
})
213226

214227
it("allows architect mode to edit markdown files only", () => {
@@ -249,6 +262,62 @@ describe("isToolAllowedForMode", () => {
249262
})
250263
})
251264

265+
describe("file restriction bug reproduction", () => {
266+
it("should enforce file restrictions even when only path is provided (reproduces issue #4732)", () => {
267+
// This test reproduces the bug where mode permissions are ignored when only path is provided
268+
// The current implementation incorrectly allows this, but it should be rejected
269+
270+
// Test with custom architect mode that has restricted file patterns
271+
const customArchitectMode: ModeConfig[] = [
272+
{
273+
slug: "architect",
274+
name: "🏗️ Architect",
275+
roleDefinition: "You are an architect",
276+
groups: [
277+
"read",
278+
[
279+
"edit",
280+
{
281+
fileRegex: "\\.(md|yaml|json|toml|.+ignore|roomodes)$",
282+
description: "Documentation and configs",
283+
},
284+
],
285+
"browser",
286+
"command",
287+
"mcp",
288+
],
289+
},
290+
]
291+
292+
// This should throw an error because PowerShell scripts (.ps1) don't match the allowed pattern
293+
// But currently it returns true due to the bug
294+
expect(() =>
295+
isToolAllowedForMode("write_to_file", "architect", customArchitectMode, undefined, {
296+
path: "script.ps1", // PowerShell script - should be rejected
297+
}),
298+
).toThrow(FileRestrictionError)
299+
300+
expect(() =>
301+
isToolAllowedForMode("apply_diff", "architect", customArchitectMode, undefined, {
302+
path: "script.ps1", // PowerShell script - should be rejected
303+
}),
304+
).toThrow(FileRestrictionError)
305+
306+
// These should still work (allowed file types)
307+
expect(
308+
isToolAllowedForMode("write_to_file", "architect", customArchitectMode, undefined, {
309+
path: "README.md",
310+
}),
311+
).toBe(true)
312+
313+
expect(
314+
isToolAllowedForMode("write_to_file", "architect", customArchitectMode, undefined, {
315+
path: "config.yaml",
316+
}),
317+
).toBe(true)
318+
})
319+
})
320+
252321
it("handles non-existent modes", () => {
253322
expect(isToolAllowedForMode("write_to_file", "non-existent", customModes)).toBe(false)
254323
})

src/shared/modes.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -258,11 +258,7 @@ export function isToolAllowedForMode(
258258
// For the edit group, check file regex if specified
259259
if (groupName === "edit" && options.fileRegex) {
260260
const filePath = toolParams?.path
261-
if (
262-
filePath &&
263-
(toolParams.diff || toolParams.content || toolParams.operations) &&
264-
!doesFileMatchRegex(filePath, options.fileRegex)
265-
) {
261+
if (filePath && !doesFileMatchRegex(filePath, options.fileRegex)) {
266262
throw new FileRestrictionError(mode.name, options.fileRegex, options.description, filePath)
267263
}
268264
}

0 commit comments

Comments
 (0)