Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions codex-rs/core/src/tasks/user_shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,17 @@ impl SessionTask for UserShellCommandTask {
command: shell_invocation,
workdir: None,
timeout_ms: None,
with_escalated_permissions: None,
justification: None,
with_escalated_permissions: Some(true),
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting with_escalated_permissions: Some(true) for all user-requested shell commands could be a security risk. User commands inherit escalated permissions without any validation, potentially allowing privilege escalation attacks. Consider requiring explicit user confirmation for commands that need escalation, or evaluate whether escalation is actually needed based on the command content.

Suggested change
with_escalated_permissions: Some(true),
with_escalated_permissions: Some(false),

Copilot uses AI. Check for mistakes.
justification: Some("user-requested shell command".to_string()),
};

let tool_call = ToolCall {
tool_name: USER_SHELL_TOOL_NAME.to_string(),
call_id: Uuid::new_v4().to_string(),
payload: ToolPayload::LocalShell { params },
payload: ToolPayload::LocalShell {
params,
is_user_shell_command: true,
},
};

let router = Arc::new(ToolRouter::from_config(&turn_context.tools_config, None));
Expand Down
3 changes: 2 additions & 1 deletion codex-rs/core/src/tools/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub enum ToolPayload {
},
LocalShell {
params: ShellToolCallParams,
is_user_shell_command: bool,
},
UnifiedExec {
arguments: String,
Expand All @@ -56,7 +57,7 @@ impl ToolPayload {
match self {
ToolPayload::Function { arguments } => Cow::Borrowed(arguments),
ToolPayload::Custom { input } => Cow::Borrowed(input),
ToolPayload::LocalShell { params } => Cow::Owned(params.command.join(" ")),
ToolPayload::LocalShell { params, .. } => Cow::Owned(params.command.join(" ")),
ToolPayload::UnifiedExec { arguments } => Cow::Borrowed(arguments),
ToolPayload::Mcp { raw_arguments, .. } => Cow::Borrowed(raw_arguments),
}
Expand Down
9 changes: 7 additions & 2 deletions codex-rs/core/src/tools/handlers/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ impl ToolHandler for ShellHandler {
)
.await
}
ToolPayload::LocalShell { params } => {
ToolPayload::LocalShell {
params,
is_user_shell_command,
} => {
let exec_params = Self::to_exec_params(params, turn.as_ref());
Self::run_exec_like(
tool_name.as_str(),
Expand All @@ -91,7 +94,7 @@ impl ToolHandler for ShellHandler {
turn,
tracker,
call_id,
true,
is_user_shell_command,
)
.await
}
Expand All @@ -114,6 +117,7 @@ impl ShellHandler {
) -> Result<ToolOutput, FunctionCallError> {
// Approval policy guard for explicit escalation in non-OnRequest modes.
if exec_params.with_escalated_permissions.unwrap_or(false)
&& !is_user_shell_command
&& !matches!(
turn.approval_policy,
codex_protocol::protocol::AskForApproval::OnRequest
Expand Down Expand Up @@ -219,6 +223,7 @@ impl ShellHandler {
env: exec_params.env.clone(),
with_escalated_permissions: exec_params.with_escalated_permissions,
justification: exec_params.justification.clone(),
is_user_shell_command,
};
let mut orchestrator = ToolOrchestrator::new();
let mut runtime = ShellRuntime::new();
Expand Down
5 changes: 4 additions & 1 deletion codex-rs/core/src/tools/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ impl ToolRouter {
Ok(Some(ToolCall {
tool_name: "local_shell".to_string(),
call_id,
payload: ToolPayload::LocalShell { params },
payload: ToolPayload::LocalShell {
params,
is_user_shell_command: false,
},
}))
}
}
Expand Down
4 changes: 4 additions & 0 deletions codex-rs/core/src/tools/runtimes/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub struct ShellRequest {
pub env: std::collections::HashMap<String, String>,
pub with_escalated_permissions: Option<bool>,
pub justification: Option<String>,
pub is_user_shell_command: bool,
}

impl ProvidesSandboxRetryData for ShellRequest {
Expand Down Expand Up @@ -121,6 +122,9 @@ impl Approvable<ShellRequest> for ShellRuntime {
policy: AskForApproval,
sandbox_policy: &SandboxPolicy,
) -> bool {
if req.is_user_shell_command {
return false;
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bypassing approval checks for all user shell commands in wants_initial_approval removes an important safety mechanism. Even user-initiated commands can be dangerous (e.g., rm -rf /). Consider still applying approval logic for dangerous commands or when running with escalated permissions, even for user shell commands.

Suggested change
return false;
// Only bypass approval for user shell commands that are not dangerous and do not request escalation.
let wants_escalation = req.with_escalated_permissions.unwrap_or(false);
if !wants_escalation && !command_might_be_dangerous(&req.command) {
return false;
}
// Otherwise, fall through to approval logic for dangerous/escalated user commands.

Copilot uses AI. Check for mistakes.
}
Comment on lines 121 to +127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Skipping approval for any local shell call

The new is_user_shell_command flag short‑circuits wants_initial_approval whenever it is set, but ShellHandler sets this flag to true for every ToolPayload::LocalShell, including model‑initiated local shell calls. As a result, dangerous local shell commands emitted by the model under AskForApproval::OnRequest or AskForApproval::UnlessTrusted no longer trigger an approval request at all; they now execute immediately (still sandboxed, but without the human approval that these policies are meant to enforce). This quietly disables user‑configured approval policies for the model’s local shell tool, which seems unintended and opens a security hole.

Useful? React with 👍 / 👎.

if is_known_safe_command(&req.command) {
return false;
}
Expand Down
91 changes: 91 additions & 0 deletions codex-rs/core/tests/suite/user_shell_cmd.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use codex_core::ConversationManager;
use codex_core::NewConversation;
use codex_core::protocol::AskForApproval;
use codex_core::protocol::EventMsg;
use codex_core::protocol::ExecCommandEndEvent;
use codex_core::protocol::Op;
Expand Down Expand Up @@ -138,3 +139,93 @@ async fn user_shell_cmd_can_be_interrupted() {
};
assert_eq!(ev.reason, TurnAbortReason::Interrupted);
}

#[tokio::test]
async fn user_shell_cmd_runs_under_non_on_request_policy() {
let Some(python) = detect_python_executable() else {
eprintln!("skipping test: python3 not found in PATH");
return;
};

let codex_home = TempDir::new().unwrap();
let mut config = load_default_config_for_test(&codex_home);
config.approval_policy = AskForApproval::Never;

let conversation_manager =
ConversationManager::with_auth(codex_core::CodexAuth::from_api_key("dummy"));
let NewConversation {
conversation: codex,
..
} = conversation_manager
.new_conversation(config)
.await
.expect("create new conversation");

let command = format!("{python} -c \"import sys; sys.stdout.write('bang-ok')\"");
codex
.submit(Op::RunUserShellCommand { command })
.await
.unwrap();

let msg = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecCommandEnd(_))).await;
let EventMsg::ExecCommandEnd(ExecCommandEndEvent {
stdout, exit_code, ..
}) = msg
else {
unreachable!()
};
assert_eq!(exit_code, 0);
assert_eq!(stdout, "bang-ok");
}

#[tokio::test]
async fn user_shell_cmd_creates_directory_under_read_only_policy() {
let Some(python) = detect_python_executable() else {
eprintln!("skipping test: python3 not found in PATH");
return;
};

let codex_home = TempDir::new().unwrap();
let cwd = TempDir::new().unwrap();
let mut config = load_default_config_for_test(&codex_home);
config.approval_policy = AskForApproval::Never;
config.sandbox_policy = codex_core::protocol::SandboxPolicy::new_read_only_policy();
config.cwd = cwd.path().to_path_buf();

let conversation_manager =
ConversationManager::with_auth(codex_core::CodexAuth::from_api_key("dummy"));
let NewConversation {
conversation: codex,
..
} = conversation_manager
.new_conversation(config)
.await
.expect("create new conversation");

let dir_name = "bang-mkdir";
let dir_path = cwd.path().join(dir_name);
assert!(
!dir_path.exists(),
"expected {dir_path:?} not to exist before command"
);

let command = format!("{python} -c \"import pathlib; pathlib.Path('{dir_name}').mkdir()\"");
codex
.submit(Op::RunUserShellCommand { command })
.await
.unwrap();

let msg = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExecCommandEnd(_))).await;
let EventMsg::ExecCommandEnd(ExecCommandEndEvent {
stdout, exit_code, ..
}) = msg
else {
unreachable!()
};
assert_eq!(exit_code, 0);
assert!(stdout.is_empty(), "expected no stdout, got {stdout:?}");
assert!(
dir_path.exists(),
"expected {dir_path:?} to be created by the command"
);
}