valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(security): Shell commands auto-executing in 'suggest' mode without permission (#197)

Browse Source 
## Problem

There's a security vulnerability in the current implementation where
shell commands are being executed without requesting user permission
even when in 'suggest' mode. According to our documentation:

> In **Suggest** mode (default): All file writes/patches and **ALL
shell/Bash commands** should require approval.

However, the current implementation in `approvals.ts` was auto-approving
commands deemed "safe" by the `isSafeCommand` function, bypassing the
user permission requirement. This is a security risk as users expect all
shell commands to require explicit approval in 'suggest' mode.

## Solution

This PR fixes the issue by modifying the `canAutoApprove` function in
`approvals.ts` to respect the 'suggest' mode policy for all shell
commands:

1. Added an early check at the beginning of `canAutoApprove` to
immediately return `{ type: "ask-user" }` when the policy is `suggest`,
regardless of whether the command is considered "safe" or not.

2. Added a similar check in the bash command handling section to ensure
bash commands also respect the 'suggest' mode.

3. Updated tests to verify the new behavior, ensuring that all shell
commands require approval in 'suggest' mode, while still being
auto-approved in 'auto-edit' and 'full-auto' modes when appropriate.

## Testing

All tests pass, confirming that the fix works as expected. The updated
tests verify that:
- All commands (even "safe" ones) require approval in 'suggest' mode
- Safe commands are still auto-approved in 'auto-edit' mode
- Bash commands with redirects still require approval in all modes

This change ensures that the behavior matches what's documented and what
users expect, improving security by requiring explicit permission for
all shell commands in the default 'suggest' mode.
This commit is contained in:
Brayden Moon
2025-04-18 00:15:02 +10:00
committed by GitHub
parent af69e793e7
commit b62ef70d2a
2 changed files with 83 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
codex-cli/src/approvals.ts
View File

@@ -84,6 +84,11 @@ export function canAutoApprove(
};
}
// In 'suggest' mode, all shell commands should require user permission
if (policy === "suggest") {
return { type: "ask-user" };
}
const isSafe = isSafeCommand(command);
if (isSafe != null) {
const { reason, group } = isSafe;
@@ -112,23 +117,23 @@ export function canAutoApprove(
} catch (e) {
// In practice, there seem to be syntactically valid shell commands that
// shell-quote cannot parse, so we should not reject, but ask the user.
switch (policy) {
case "full-auto":
// In full-auto, we still run the command automatically, but must
// restrict it to the sandbox.
return {
type: "auto-approve",
reason: "Full auto mode",
group: "Running commands",
runInSandbox: true,
};
case "suggest":
case "auto-edit":
// In all other modes, since we cannot reason about the command, we
// should ask the user.
return {
type: "ask-user",
};
// We already checked for 'suggest' mode at the beginning of the function,
// so at this point we know policy is either 'auto-edit' or 'full-auto'
if (policy === "full-auto") {
// In full-auto, we still run the command automatically, but must
// restrict it to the sandbox.
return {
type: "auto-approve",
reason: "Full auto mode",
group: "Running commands",
runInSandbox: true,
};
} else {
// In auto-edit mode, since we cannot reason about the command, we
// should ask the user.
return {
type: "ask-user",
};
}
}
@@ -138,6 +143,9 @@ export function canAutoApprove(
// all operators belong to an allow‑list. If so, the entire expression is
// considered auto‑approvable.
// We already checked for 'suggest' mode at the beginning of the function,
// so at this point we know policy is either 'auto-edit' or 'full-auto'
const shellSafe = isEntireShellExpressionSafe(bashCmd);
if (shellSafe != null) {
const { reason, group } = shellSafe;

82
codex-cli/tests/approvals.test.ts
View File

@@ -10,23 +10,33 @@ describe("canAutoApprove()", () => {
};
const writeablePaths: Array<string> = [];
const check = (command: ReadonlyArray<string>): SafetyAssessment =>
canAutoApprove(command, "suggest", writeablePaths, env);
const check = (
command: ReadonlyArray<string>,
policy: "suggest" | "auto-edit" | "full-auto" = "suggest",
): SafetyAssessment => canAutoApprove(command, policy, writeablePaths, env);
test("simple safe commands", () => {
expect(check(["ls"])).toEqual({
test("simple commands in suggest mode should require approval", () => {
// In suggest mode, all commands should require approval
expect(check(["ls"])).toEqual({ type: "ask-user" });
expect(check(["cat", "file.txt"])).toEqual({ type: "ask-user" });
expect(check(["pwd"])).toEqual({ type: "ask-user" });
});
test("simple safe commands in auto-edit mode", () => {
// In auto-edit mode, safe commands should be auto-approved
expect(check(["ls"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "List directory",
group: "Searching",
runInSandbox: false,
});
expect(check(["cat", "file.txt"])).toEqual({
expect(check(["cat", "file.txt"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "View file contents",
group: "Reading files",
runInSandbox: false,
});
expect(check(["pwd"])).toEqual({
expect(check(["pwd"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "Print working directory",
group: "Navigating",
@@ -34,20 +44,30 @@ describe("canAutoApprove()", () => {
});
});
test("simple safe commands within a `bash -lc` call", () => {
expect(check(["bash", "-lc", "ls"])).toEqual({
type: "auto-approve",
reason: "List directory",
group: "Searching",
runInSandbox: false,
});
expect(check(["bash", "-lc", "ls $HOME"])).toEqual({
type: "auto-approve",
reason: "List directory",
group: "Searching",
runInSandbox: false,
});
test("bash commands in suggest mode should require approval", () => {
// In suggest mode, all bash commands should require approval
expect(check(["bash", "-lc", "ls"])).toEqual({ type: "ask-user" });
expect(check(["bash", "-lc", "ls $HOME"])).toEqual({ type: "ask-user" });
expect(check(["bash", "-lc", "git show ab9811cb90"])).toEqual({
type: "ask-user",
});
});
test("bash commands in auto-edit mode", () => {
// In auto-edit mode, safe bash commands should be auto-approved
expect(check(["bash", "-lc", "ls"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "List directory",
group: "Searching",
runInSandbox: false,
});
expect(check(["bash", "-lc", "ls $HOME"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "List directory",
group: "Searching",
runInSandbox: false,
});
expect(check(["bash", "-lc", "git show ab9811cb90"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "Git show",
group: "Using git",
@@ -56,13 +76,23 @@ describe("canAutoApprove()", () => {
});
test("bash -lc commands with unsafe redirects", () => {
// In suggest mode, all commands should require approval
expect(check(["bash", "-lc", "echo hello > file.txt"])).toEqual({
type: "ask-user",
});
// In theory, we could make our checker more sophisticated to auto-approve
// This previously required approval, but now that we consider safe
// operators like "&&" the entire expression can be auto‑approved.
expect(check(["bash", "-lc", "ls && pwd"])).toEqual({
type: "ask-user",
});
// In auto-edit mode, commands with redirects should still require approval
expect(
check(["bash", "-lc", "echo hello > file.txt"], "auto-edit"),
).toEqual({
type: "ask-user",
});
// In auto-edit mode, safe commands with safe operators should be auto-approved
expect(check(["bash", "-lc", "ls && pwd"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "List directory",
group: "Searching",
@@ -70,8 +100,12 @@ describe("canAutoApprove()", () => {
});
});
test("true command is considered safe", () => {
expect(check(["true"])).toEqual({
test("true command in suggest mode requires approval", () => {
expect(check(["true"])).toEqual({ type: "ask-user" });
});
test("true command in auto-edit mode is auto-approved", () => {
expect(check(["true"], "auto-edit")).toEqual({
type: "auto-approve",
reason: "No‑op (true)",
group: "Utility",