mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: enforce denied commands regardless of auto-approval settings
Previously, the denied commands list was only checked when alwaysAllowExecute was true. This meant agents could bypass the deny list by wrapping denied commands inside chains (&&, ||, ;) or multi-line scripts like heredocs. Now the deny list check runs before the autoApprovalEnabled guard, so denied commands are always blocked regardless of whether auto-approval or alwaysAllowExecute is enabled. Fixes #12292
This commit is contained in:
parent
ad25634905
commit
0f9198e181
3 changed files with 177 additions and 0 deletions
126
src/core/auto-approval/__tests__/checkAutoApproval.spec.ts
Normal file
126
src/core/auto-approval/__tests__/checkAutoApproval.spec.ts
Normal file
|
|
@ -0,0 +1,126 @@
|
|||
import { checkAutoApproval } from "../index"
|
||||
import type { ExtensionState } from "@roo-code/types"
|
||||
|
||||
function makeState(overrides: Record<string, unknown> = {}) {
|
||||
return {
|
||||
autoApprovalEnabled: false,
|
||||
alwaysAllowReadOnly: false,
|
||||
alwaysAllowWrite: false,
|
||||
alwaysAllowExecute: false,
|
||||
alwaysAllowMcp: false,
|
||||
alwaysAllowModeSwitch: false,
|
||||
alwaysAllowSubtasks: false,
|
||||
alwaysAllowFollowupQuestions: false,
|
||||
alwaysAllowReadOnlyOutsideWorkspace: false,
|
||||
alwaysAllowWriteOutsideWorkspace: false,
|
||||
alwaysAllowWriteProtected: false,
|
||||
followupAutoApproveTimeoutMs: 0,
|
||||
mcpServers: {},
|
||||
deniedCommands: [] as string[],
|
||||
allowedCommands: [] as string[],
|
||||
...overrides,
|
||||
} as Pick<ExtensionState, any>
|
||||
}
|
||||
|
||||
describe("checkAutoApproval — denied commands enforcement", () => {
|
||||
const deniedCommands = ["rm", "sudo"]
|
||||
|
||||
it("should deny a command matching the deny list even when autoApprovalEnabled is false", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState({ deniedCommands }),
|
||||
ask: "command",
|
||||
text: "rm -rf /tmp/test",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("deny")
|
||||
})
|
||||
|
||||
it("should deny a command matching the deny list even when alwaysAllowExecute is false", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState({ autoApprovalEnabled: true, deniedCommands }),
|
||||
ask: "command",
|
||||
text: "sudo apt install something",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("deny")
|
||||
})
|
||||
|
||||
it("should deny a chained command where denied command appears after &&", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState({ deniedCommands }),
|
||||
ask: "command",
|
||||
text: "cat file.txt && rm file.txt",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("deny")
|
||||
})
|
||||
|
||||
it("should deny the exact heredoc bypass scenario from the issue", async () => {
|
||||
const command = [
|
||||
"cat > verify-hook-install.nu << 'HEREDOC'",
|
||||
"use scripts/development/modules/nu/install_hooks.nu [install-git-hooks]",
|
||||
"let project_root = ($env | get -o FILE_PWD | default (pwd))",
|
||||
"install-git-hooks $project_root",
|
||||
"HEREDOC",
|
||||
"nu verify-hook-install.nu && rm verify-hook-install.nu",
|
||||
].join("\n")
|
||||
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState({
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowExecute: true,
|
||||
deniedCommands: ["rm"],
|
||||
allowedCommands: ["cat", "nu"],
|
||||
}),
|
||||
ask: "command",
|
||||
text: command,
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("deny")
|
||||
})
|
||||
|
||||
it("should return 'ask' for a non-denied command when autoApprovalEnabled is false", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState({ deniedCommands }),
|
||||
ask: "command",
|
||||
text: "git status",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
|
||||
it("should not deny when deny list is empty", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState(),
|
||||
ask: "command",
|
||||
text: "rm -rf /tmp/test",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
|
||||
it("should respect longest prefix match: allowed 'rm -i' overrides denied 'rm'", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: makeState({
|
||||
autoApprovalEnabled: true,
|
||||
alwaysAllowExecute: true,
|
||||
deniedCommands: ["rm"],
|
||||
allowedCommands: ["rm -i"],
|
||||
}),
|
||||
ask: "command",
|
||||
text: "rm -i file.txt",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("approve")
|
||||
})
|
||||
|
||||
it("should return 'ask' when state is undefined", async () => {
|
||||
const result = await checkAutoApproval({
|
||||
state: undefined,
|
||||
ask: "command",
|
||||
text: "rm file",
|
||||
})
|
||||
|
||||
expect(result.decision).toBe("ask")
|
||||
})
|
||||
})
|
||||
|
|
@ -82,6 +82,46 @@ describe("containsDangerousSubstitution — true positives still caught", () =>
|
|||
})
|
||||
})
|
||||
|
||||
describe("getCommandDecision — denied commands in chained/wrapped commands", () => {
|
||||
it("should auto_deny when denied command appears after && in a chain", () => {
|
||||
expect(getCommandDecision("cat file.txt && rm file.txt", [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should auto_deny when denied command appears after || in a chain", () => {
|
||||
expect(getCommandDecision("test -f file || rm file", [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should auto_deny when denied command appears after ; in a chain", () => {
|
||||
expect(getCommandDecision("echo done; rm -rf /tmp/test", [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should auto_deny when denied command appears after pipe", () => {
|
||||
expect(getCommandDecision("ls | rm file", [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should auto_deny for heredoc-style bypass with rm at end (multi-line)", () => {
|
||||
const command = `cat > script.sh << 'HEREDOC'\necho hello\nHEREDOC\nnu script.sh && rm script.sh`
|
||||
expect(getCommandDecision(command, [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should auto_deny when denied command is the first in a chain", () => {
|
||||
expect(getCommandDecision("rm file && echo done", [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should auto_deny for the exact issue scenario", () => {
|
||||
const command = `cat > verify-hook-install.nu << 'HEREDOC'\nuse scripts/development/modules/nu/install_hooks.nu [install-git-hooks]\nlet project_root = ($env | get -o FILE_PWD | default (pwd))\ninstall-git-hooks $project_root\nHEREDOC\nnu verify-hook-install.nu && rm verify-hook-install.nu`
|
||||
expect(getCommandDecision(command, [], ["rm"])).toBe("auto_deny")
|
||||
})
|
||||
|
||||
it("should not deny when denied command is not present", () => {
|
||||
expect(getCommandDecision("git status && echo done", [], ["rm"])).toBe("ask_user")
|
||||
})
|
||||
|
||||
it("should respect longest prefix match: allowed 'rm -i' overrides denied 'rm'", () => {
|
||||
expect(getCommandDecision("rm -i file.txt", ["rm -i"], ["rm"])).toBe("auto_approve")
|
||||
})
|
||||
})
|
||||
|
||||
describe("getCommandDecision — integration with dangerous substitution checks", () => {
|
||||
const allowedCommands = ["node", "echo"]
|
||||
|
||||
|
|
|
|||
|
|
@ -59,6 +59,17 @@ export async function checkAutoApproval({
|
|||
return { decision: "approve" }
|
||||
}
|
||||
|
||||
// Always enforce denied commands regardless of auto-approval settings.
|
||||
// This prevents agents from bypassing the deny list by wrapping denied
|
||||
// commands inside chains (&&, ||, ;) or multi-line scripts.
|
||||
if (ask === "command" && text && state?.deniedCommands?.length) {
|
||||
const decision = getCommandDecision(text, state.allowedCommands || [], state.deniedCommands)
|
||||
|
||||
if (decision === "auto_deny") {
|
||||
return { decision: "deny" }
|
||||
}
|
||||
}
|
||||
|
||||
if (!state || !state.autoApprovalEnabled) {
|
||||
return { decision: "ask" }
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue