Skip to content

Commit 7d0b22f

Browse files
mrubensmrubens
authored
Add logic to prevent auto-approving edits of configuration files (#4667)
* Add logic to prevent auto-approving edits of configuration files * Fix tests * Update patterns
1 parent 10b2fb3 commit 7d0b22f

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

55 files changed

+488
-32
lines changed

‎packages/types/src/global-settings.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ export const globalSettingsSchema = z.object({
3636
alwaysAllowReadOnlyOutsideWorkspace: z.boolean().optional(),
3737
alwaysAllowWrite: z.boolean().optional(),
3838
alwaysAllowWriteOutsideWorkspace: z.boolean().optional(),
39+
alwaysAllowWriteProtected: z.boolean().optional(),
3940
writeDelayMs: z.number().optional(),
4041
alwaysAllowBrowser: z.boolean().optional(),
4142
alwaysApproveResubmit: z.boolean().optional(),
@@ -177,6 +178,7 @@ export const EVALS_SETTINGS: RooCodeSettings = {
177178
alwaysAllowReadOnlyOutsideWorkspace: false,
178179
alwaysAllowWrite: true,
179180
alwaysAllowWriteOutsideWorkspace: false,
181+
alwaysAllowWriteProtected: false,
180182
writeDelayMs: 1000,
181183
alwaysAllowBrowser: true,
182184
alwaysApproveResubmit: true,

‎packages/types/src/message.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ export const clineMessageSchema = z.object({
153153
checkpoint: z.record(z.string(), z.unknown()).optional(),
154154
progressStatus: toolProgressStatusSchema.optional(),
155155
contextCondense: contextCondenseSchema.optional(),
156+
isProtected: z.boolean().optional(),
156157
})
157158

158159
export type ClineMessage = z.infer<typeof clineMessageSchema>

‎src/core/assistant-message/presentAssistantMessage.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,15 @@ export async function presentAssistantMessage(cline: Task) {
261261
type: ClineAsk,
262262
partialMessage?: string,
263263
progressStatus?: ToolProgressStatus,
264+
isProtected?: boolean,
264265
) => {
265-
const { response, text, images } = await cline.ask(type, partialMessage, false, progressStatus)
266+
const { response, text, images } = await cline.ask(
267+
type,
268+
partialMessage,
269+
false,
270+
progressStatus,
271+
isProtected || false,
272+
)
266273

267274
if (response !== "yesButtonClicked") {
268275
// Handle both messageResponse and noButtonClicked with text.

‎src/core/prompts/responses.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { Anthropic } from "@anthropic-ai/sdk"
22
import * as path from "path"
33
import * as diff from "diff"
44
import { RooIgnoreController, LOCK_TEXT_SYMBOL } from "../ignore/RooIgnoreController"
5+
import { RooProtectedController } from "../protect/RooProtectedController"
56

67
export const formatResponse = {
78
toolDenied: () => `The user denied this operation.`,
@@ -95,6 +96,7 @@ Otherwise, if you have not completed the task and do not need additional informa
9596
didHitLimit: boolean,
9697
rooIgnoreController: RooIgnoreController | undefined,
9798
showRooIgnoredFiles: boolean,
99+
rooProtectedController?: RooProtectedController,
98100
): string => {
99101
const sorted = files
100102
.map((file) => {
@@ -143,7 +145,13 @@ Otherwise, if you have not completed the task and do not need additional informa
143145
// Otherwise, mark it with a lock symbol
144146
rooIgnoreParsed.push(LOCK_TEXT_SYMBOL + " " + filePath)
145147
} else {
146-
rooIgnoreParsed.push(filePath)
148+
// Check if file is write-protected (only for non-ignored files)
149+
const isWriteProtected = rooProtectedController?.isWriteProtected(absoluteFilePath) || false
150+
if (isWriteProtected) {
151+
rooIgnoreParsed.push("🛡️ " + filePath)
152+
} else {
153+
rooIgnoreParsed.push(filePath)
154+
}
147155
}
148156
}
149157
}

‎src/core/protect/RooProtectedController.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import path from "path"
2+
import ignore, { Ignore } from "ignore"
3+
4+
export const SHIELD_SYMBOL = "\u{1F6E1}"
5+
6+
/**
7+
* Controls write access to Roo configuration files by enforcing protection patterns.
8+
* Prevents auto-approved modifications to sensitive Roo configuration files.
9+
*/
10+
export class RooProtectedController {
11+
private cwd: string
12+
private ignoreInstance: Ignore
13+
14+
// Predefined list of protected Roo configuration patterns
15+
private static readonly PROTECTED_PATTERNS = [
16+
".rooignore",
17+
".roomodes",
18+
".roorules*",
19+
".clinerules*",
20+
".roo/**",
21+
".rooprotected", // For future use
22+
]
23+
24+
constructor(cwd: string) {
25+
this.cwd = cwd
26+
// Initialize ignore instance with protected patterns
27+
this.ignoreInstance = ignore()
28+
this.ignoreInstance.add(RooProtectedController.PROTECTED_PATTERNS)
29+
}
30+
31+
/**
32+
* Check if a file is write-protected
33+
* @param filePath - Path to check (relative to cwd)
34+
* @returns true if file is write-protected, false otherwise
35+
*/
36+
isWriteProtected(filePath: string): boolean {
37+
try {
38+
// Normalize path to be relative to cwd and use forward slashes
39+
const absolutePath = path.resolve(this.cwd, filePath)
40+
const relativePath = path.relative(this.cwd, absolutePath).toPosix()
41+
42+
// Use ignore library to check if file matches any protected pattern
43+
return this.ignoreInstance.ignores(relativePath)
44+
} catch (error) {
45+
// If there's an error processing the path, err on the side of caution
46+
// Ignore is designed to work with relative file paths, so will throw error for paths outside cwd
47+
console.error(`Error checking protection for ${filePath}:`, error)
48+
return false
49+
}
50+
}
51+
52+
/**
53+
* Get set of write-protected files from a list
54+
* @param paths - Array of paths to filter (relative to cwd)
55+
* @returns Set of protected file paths
56+
*/
57+
getProtectedFiles(paths: string[]): Set<string> {
58+
const protectedFiles = new Set<string>()
59+
60+
for (const filePath of paths) {
61+
if (this.isWriteProtected(filePath)) {
62+
protectedFiles.add(filePath)
63+
}
64+
}
65+
66+
return protectedFiles
67+
}
68+
69+
/**
70+
* Filter an array of paths, marking which ones are protected
71+
* @param paths - Array of paths to check (relative to cwd)
72+
* @returns Array of objects with path and protection status
73+
*/
74+
annotatePathsWithProtection(paths: string[]): Array<{ path: string; isProtected: boolean }> {
75+
return paths.map((filePath) => ({
76+
path: filePath,
77+
isProtected: this.isWriteProtected(filePath),
78+
}))
79+
}
80+
81+
/**
82+
* Get display message for protected file operations
83+
*/
84+
getProtectionMessage(): string {
85+
return "This is a Roo configuration file and requires approval for modifications"
86+
}
87+
88+
/**
89+
* Get formatted instructions about protected files for the LLM
90+
* @returns Formatted instructions about file protection
91+
*/
92+
getInstructions(): string {
93+
const patterns = RooProtectedController.PROTECTED_PATTERNS.join(", ")
94+
return `# Protected Files\n\n(The following Roo configuration file patterns are write-protected and always require approval for modifications, regardless of autoapproval settings. When using list_files, you'll notice a ${SHIELD_SYMBOL} next to files that are write-protected.)\n\nProtected patterns: ${patterns}`
95+
}
96+
97+
/**
98+
* Get the list of protected patterns (for testing/debugging)
99+
*/
100+
static getProtectedPatterns(): readonly string[] {
101+
return RooProtectedController.PROTECTED_PATTERNS
102+
}
103+
}

‎src/core/protect/__tests__/RooProtectedController.spec.ts‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
import path from "path"
2+
import { RooProtectedController } from "../RooProtectedController"
3+
4+
describe("RooProtectedController", () => {
5+
const TEST_CWD = "/test/workspace"
6+
let controller: RooProtectedController
7+
8+
beforeEach(() => {
9+
controller = new RooProtectedController(TEST_CWD)
10+
})
11+
12+
describe("isWriteProtected", () => {
13+
it("should protect .rooignore file", () => {
14+
expect(controller.isWriteProtected(".rooignore")).toBe(true)
15+
})
16+
17+
it("should protect files in .roo directory", () => {
18+
expect(controller.isWriteProtected(".roo/config.json")).toBe(true)
19+
expect(controller.isWriteProtected(".roo/settings/user.json")).toBe(true)
20+
expect(controller.isWriteProtected(".roo/modes/custom.json")).toBe(true)
21+
})
22+
23+
it("should protect .rooprotected file", () => {
24+
expect(controller.isWriteProtected(".rooprotected")).toBe(true)
25+
})
26+
27+
it("should protect .roomodes files", () => {
28+
expect(controller.isWriteProtected(".roomodes")).toBe(true)
29+
})
30+
31+
it("should protect .roorules* files", () => {
32+
expect(controller.isWriteProtected(".roorules")).toBe(true)
33+
expect(controller.isWriteProtected(".roorules.md")).toBe(true)
34+
})
35+
36+
it("should protect .clinerules* files", () => {
37+
expect(controller.isWriteProtected(".clinerules")).toBe(true)
38+
expect(controller.isWriteProtected(".clinerules.md")).toBe(true)
39+
})
40+
41+
it("should not protect other files starting with .roo", () => {
42+
expect(controller.isWriteProtected(".roosettings")).toBe(false)
43+
expect(controller.isWriteProtected(".rooconfig")).toBe(false)
44+
})
45+
46+
it("should not protect regular files", () => {
47+
expect(controller.isWriteProtected("src/index.ts")).toBe(false)
48+
expect(controller.isWriteProtected("package.json")).toBe(false)
49+
expect(controller.isWriteProtected("README.md")).toBe(false)
50+
})
51+
52+
it("should not protect files that contain 'roo' but don't start with .roo", () => {
53+
expect(controller.isWriteProtected("src/roo-utils.ts")).toBe(false)
54+
expect(controller.isWriteProtected("config/roo.config.js")).toBe(false)
55+
})
56+
57+
it("should handle nested paths correctly", () => {
58+
expect(controller.isWriteProtected(".roo/config.json")).toBe(true) // .roo/** matches at root
59+
expect(controller.isWriteProtected("nested/.rooignore")).toBe(true) // .rooignore matches anywhere by default
60+
expect(controller.isWriteProtected("nested/.roomodes")).toBe(true) // .roomodes matches anywhere by default
61+
expect(controller.isWriteProtected("nested/.roorules.md")).toBe(true) // .roorules* matches anywhere by default
62+
})
63+
64+
it("should handle absolute paths by converting to relative", () => {
65+
const absolutePath = path.join(TEST_CWD, ".rooignore")
66+
expect(controller.isWriteProtected(absolutePath)).toBe(true)
67+
})
68+
69+
it("should handle paths with different separators", () => {
70+
expect(controller.isWriteProtected(".roo\\config.json")).toBe(true)
71+
expect(controller.isWriteProtected(".roo/config.json")).toBe(true)
72+
})
73+
})
74+
75+
describe("getProtectedFiles", () => {
76+
it("should return set of protected files from a list", () => {
77+
const files = ["src/index.ts", ".rooignore", "package.json", ".roo/config.json", "README.md"]
78+
79+
const protectedFiles = controller.getProtectedFiles(files)
80+
81+
expect(protectedFiles).toEqual(new Set([".rooignore", ".roo/config.json"]))
82+
})
83+
84+
it("should return empty set when no files are protected", () => {
85+
const files = ["src/index.ts", "package.json", "README.md"]
86+
87+
const protectedFiles = controller.getProtectedFiles(files)
88+
89+
expect(protectedFiles).toEqual(new Set())
90+
})
91+
})
92+
93+
describe("annotatePathsWithProtection", () => {
94+
it("should annotate paths with protection status", () => {
95+
const files = ["src/index.ts", ".rooignore", ".roo/config.json", "package.json"]
96+
97+
const annotated = controller.annotatePathsWithProtection(files)
98+
99+
expect(annotated).toEqual([
100+
{ path: "src/index.ts", isProtected: false },
101+
{ path: ".rooignore", isProtected: true },
102+
{ path: ".roo/config.json", isProtected: true },
103+
{ path: "package.json", isProtected: false },
104+
])
105+
})
106+
})
107+
108+
describe("getProtectionMessage", () => {
109+
it("should return appropriate protection message", () => {
110+
const message = controller.getProtectionMessage()
111+
expect(message).toBe("This is a Roo configuration file and requires approval for modifications")
112+
})
113+
})
114+
115+
describe("getInstructions", () => {
116+
it("should return formatted instructions about protected files", () => {
117+
const instructions = controller.getInstructions()
118+
119+
expect(instructions).toContain("# Protected Files")
120+
expect(instructions).toContain("write-protected")
121+
expect(instructions).toContain(".rooignore")
122+
expect(instructions).toContain(".roo/**")
123+
expect(instructions).toContain("\u{1F6E1}") // Shield symbol
124+
})
125+
})
126+
127+
describe("getProtectedPatterns", () => {
128+
it("should return the list of protected patterns", () => {
129+
const patterns = RooProtectedController.getProtectedPatterns()
130+
131+
expect(patterns).toEqual([
132+
".rooignore",
133+
".roomodes",
134+
".roorules*",
135+
".clinerules*",
136+
".roo/**",
137+
".rooprotected",
138+
])
139+
})
140+
})
141+
})

‎src/core/task/Task.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ import { SYSTEM_PROMPT } from "../prompts/system"
6565
import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector"
6666
import { FileContextTracker } from "../context-tracking/FileContextTracker"
6767
import { RooIgnoreController } from "../ignore/RooIgnoreController"
68+
import { RooProtectedController } from "../protect/RooProtectedController"
6869
import { type AssistantMessageContent, parseAssistantMessage, presentAssistantMessage } from "../assistant-message"
6970
import { truncateConversationIfNeeded } from "../sliding-window"
7071
import { ClineProvider } from "../webview/ClineProvider"
@@ -144,6 +145,7 @@ export class Task extends EventEmitter<ClineEvents> {
144145

145146
toolRepetitionDetector: ToolRepetitionDetector
146147
rooIgnoreController?: RooIgnoreController
148+
rooProtectedController?: RooProtectedController
147149
fileContextTracker: FileContextTracker
148150
urlContentFetcher: UrlContentFetcher
149151
terminalProcess?: RooTerminalProcess
@@ -223,6 +225,7 @@ export class Task extends EventEmitter<ClineEvents> {
223225
this.taskNumber = -1
224226

225227
this.rooIgnoreController = new RooIgnoreController(this.cwd)
228+
this.rooProtectedController = new RooProtectedController(this.cwd)
226229
this.fileContextTracker = new FileContextTracker(provider, this.taskId)
227230

228231
this.rooIgnoreController.initialize().catch((error) => {
@@ -406,6 +409,7 @@ export class Task extends EventEmitter<ClineEvents> {
406409
text?: string,
407410
partial?: boolean,
408411
progressStatus?: ToolProgressStatus,
412+
isProtected?: boolean,
409413
): Promise<{ response: ClineAskResponse; text?: string; images?: string[] }> {
410414
// If this Cline instance was aborted by the provider, then the only
411415
// thing keeping us alive is a promise still running in the background,
@@ -433,6 +437,7 @@ export class Task extends EventEmitter<ClineEvents> {
433437
lastMessage.text = text
434438
lastMessage.partial = partial
435439
lastMessage.progressStatus = progressStatus
440+
lastMessage.isProtected = isProtected
436441
// TODO: Be more efficient about saving and posting only new
437442
// data or one whole message at a time so ignore partial for
438443
// saves, and only post parts of partial message instead of
@@ -444,7 +449,7 @@ export class Task extends EventEmitter<ClineEvents> {
444449
// state.
445450
askTs = Date.now()
446451
this.lastMessageTs = askTs
447-
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, partial })
452+
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, partial, isProtected })
448453
throw new Error("Current ask promise was ignored (#2)")
449454
}
450455
} else {
@@ -471,6 +476,7 @@ export class Task extends EventEmitter<ClineEvents> {
471476
lastMessage.text = text
472477
lastMessage.partial = false
473478
lastMessage.progressStatus = progressStatus
479+
lastMessage.isProtected = isProtected
474480
await this.saveClineMessages()
475481
this.updateClineMessage(lastMessage)
476482
} else {
@@ -480,7 +486,7 @@ export class Task extends EventEmitter<ClineEvents> {
480486
this.askResponseImages = undefined
481487
askTs = Date.now()
482488
this.lastMessageTs = askTs
483-
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text })
489+
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, isProtected })
484490
}
485491
}
486492
} else {
@@ -490,7 +496,7 @@ export class Task extends EventEmitter<ClineEvents> {
490496
this.askResponseImages = undefined
491497
askTs = Date.now()
492498
this.lastMessageTs = askTs
493-
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text })
499+
await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, isProtected })
494500
}
495501

496502
await pWaitFor(() => this.askResponse !== undefined || this.lastMessageTs !== askTs, { interval: 100 })

0 commit comments

Comments
 (0)