-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Execute user-issued bang commands without sandboxing #6013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Execute user-issued bang commands without sandboxing #6013
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modifies user-initiated shell commands to bypass approval requirements and sandbox restrictions. The changes introduce an is_user_shell_command flag throughout the shell execution pipeline and set escalated permissions for all user shell commands.
- Added
is_user_shell_commandflag to track user-initiated shell commands - User shell commands now run with escalated permissions and bypass approval checks
- Added tests verifying user commands execute under various approval and sandbox policies
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| codex-rs/core/tests/suite/user_shell_cmd.rs | Added tests for user shell commands under different approval and sandbox policies |
| codex-rs/core/src/tools/runtimes/shell.rs | Modified approval logic to skip approval for user shell commands |
| codex-rs/core/src/tools/handlers/shell.rs | Extended escalation guard to exempt user shell commands |
| codex-rs/core/src/tasks/user_shell.rs | Set escalated permissions and justification for user shell commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| timeout_ms: None, | ||
| with_escalated_permissions: None, | ||
| justification: None, | ||
| with_escalated_permissions: Some(true), |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
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.
| with_escalated_permissions: Some(true), | |
| with_escalated_permissions: Some(false), |
| sandbox_policy: &SandboxPolicy, | ||
| ) -> bool { | ||
| if req.is_user_shell_command { | ||
| return false; |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| policy: AskForApproval, | ||
| sandbox_policy: &SandboxPolicy, | ||
| ) -> bool { | ||
| if req.is_user_shell_command { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 / 👎.
…iate user shell commands
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Thanks for the proof-of-concept PR. We're discussing this behavioral change internally. |
Current version of CLI fails to execute shell commands issued by user in a sandboxed environment (partially redacted example here). This makes the experience not as streamlined as doing many of those requires switching to a different terminal session to execute
Proposed change elevates bang-commands to skip sandboxing, so commands issued by user can be executed with full privileges
Used CLI itself while working on this, have basic knowledge of Rust fundamentals