From 2fde03b4a040babc9b98c389680cb568b464da85 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 19 Nov 2025 13:59:17 -0800 Subject: [PATCH 1/6] stop over-reporting world-writable directories (#6936) Fix world-writable audit false positives by expanding generic permissions with MapGenericMask and then checking only concrete write bits. The earlier check looked for FILE_GENERIC_WRITE/generic masks directly, which shares bits with read permissions and could flag an Everyone read ACE as writable. --- codex-rs/windows-sandbox-rs/src/audit.rs | 58 +++++++++++++----------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 8873bc2e91..cba8d06335 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -9,15 +9,29 @@ use std::path::PathBuf; use std::time::Duration; use std::time::Instant; use windows_sys::Win32::Foundation::CloseHandle; -use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::Foundation::ERROR_SUCCESS; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; +use windows_sys::Win32::Foundation::LocalFree; +use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; +use windows_sys::Win32::Security::ACE_HEADER; +use windows_sys::Win32::Security::ACL; +use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; +use windows_sys::Win32::Security::AclSizeInformation; use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::GetSecurityInfo; +use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; +use windows_sys::Win32::Security::EqualSid; +use windows_sys::Win32::Security::GetAce; +use windows_sys::Win32::Security::GetAclInformation; +use windows_sys::Win32::Security::MapGenericMask; +use windows_sys::Win32::Security::GENERIC_MAPPING; use windows_sys::Win32::Storage::FileSystem::CreateFileW; +use windows_sys::Win32::Storage::FileSystem::FILE_ALL_ACCESS; use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA; use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; +use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; @@ -26,17 +40,6 @@ use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_ATTRIBUTES; use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA; use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_EA; use windows_sys::Win32::Storage::FileSystem::OPEN_EXISTING; -const GENERIC_ALL_MASK: u32 = 0x1000_0000; -const GENERIC_WRITE_MASK: u32 = 0x4000_0000; -use windows_sys::Win32::Security::AclSizeInformation; -use windows_sys::Win32::Security::EqualSid; -use windows_sys::Win32::Security::GetAce; -use windows_sys::Win32::Security::GetAclInformation; -use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; -use windows_sys::Win32::Security::ACE_HEADER; -use windows_sys::Win32::Security::ACL; -use windows_sys::Win32::Security::ACL_SIZE_INFORMATION; -use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; // Preflight scan limits const MAX_ITEMS_PER_DIR: i32 = 1000; @@ -304,7 +307,7 @@ pub fn world_writable_warning_details( } } // Fast mask-based check: does the DACL contain any ACCESS_ALLOWED ACE for -// Everyone that includes generic or specific write bits? Skips inherit-only +// Everyone that grants write after generic bits are expanded? Skips inherit-only // ACEs (do not apply to the current object). unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut c_void) -> bool { if p_dacl.is_null() { @@ -321,6 +324,12 @@ unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut if ok == 0 { return false; } + let mapping = GENERIC_MAPPING { + GenericRead: FILE_GENERIC_READ, + GenericWrite: FILE_GENERIC_WRITE, + GenericExecute: FILE_GENERIC_EXECUTE, + GenericAll: FILE_ALL_ACCESS, + }; for i in 0..(info.AceCount as usize) { let mut p_ace: *mut c_void = std::ptr::null_mut(); if GetAce(p_dacl as *const ACL, i as u32, &mut p_ace) == 0 { @@ -337,19 +346,16 @@ unsafe fn dacl_quick_world_write_mask_allows(p_dacl: *mut ACL, psid_world: *mut let base = p_ace as usize; let sid_ptr = (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; // skip header + mask - if EqualSid(sid_ptr, psid_world) != 0 { - let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); - let mask = ace.Mask; - let writey = FILE_GENERIC_WRITE - | FILE_WRITE_DATA - | FILE_APPEND_DATA - | FILE_WRITE_EA - | FILE_WRITE_ATTRIBUTES - | GENERIC_WRITE_MASK - | GENERIC_ALL_MASK; - if (mask & writey) != 0 { - return true; - } + if EqualSid(sid_ptr, psid_world) == 0 { + continue; + } + let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); + let mut mask = ace.Mask; + // Expand generic bits to concrete file rights before checking for write. + MapGenericMask(&mut mask, &mapping); + let write_mask = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_EA | FILE_WRITE_ATTRIBUTES; + if (mask & write_mask) != 0 { + return true; } } false From 692989c2770065baafe57d7b6edc89bbd7c61273 Mon Sep 17 00:00:00 2001 From: Beehive Innovations Date: Thu, 20 Nov 2025 02:50:07 +0400 Subject: [PATCH 2/6] fix(context left after review): review footer context after `/review` (#5610) ## Summary - show live review token usage while `/review` runs and restore the main session indicator afterward - add regression coverage for the footer behavior ## Testing - just fmt - cargo test -p codex-tui Fixes #5604 --------- Signed-off-by: Fahad --- codex-rs/tui/src/bottom_pane/mod.rs | 5 ++ codex-rs/tui/src/chatwidget.rs | 47 +++++++++++--- codex-rs/tui/src/chatwidget/tests.rs | 94 ++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index da2efb63c1..6738d7672d 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -114,6 +114,11 @@ impl BottomPane { self.status.as_ref() } + #[cfg(test)] + pub(crate) fn context_window_percent(&self) -> Option { + self.context_window_percent + } + fn active_view(&self) -> Option<&dyn BottomPaneView> { self.view_stack.last().map(std::convert::AsRef::as_ref) } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e832b58902..3a79330b74 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -290,6 +290,8 @@ pub(crate) struct ChatWidget { pending_notification: Option, // Simple review mode flag; used to adjust layout and banners. is_review_mode: bool, + // Snapshot of token usage to restore after review mode exits. + pre_review_token_info: Option>, // Whether to add a final message separator after the last message needs_final_message_separator: bool, @@ -489,16 +491,39 @@ impl ChatWidget { } pub(crate) fn set_token_info(&mut self, info: Option) { - if let Some(info) = info { - let context_window = info - .model_context_window - .or(self.config.model_context_window); - let percent = context_window.map(|window| { + match info { + Some(info) => self.apply_token_info(info), + None => { + self.bottom_pane.set_context_window_percent(None); + self.token_info = None; + } + } + } + + fn apply_token_info(&mut self, info: TokenUsageInfo) { + let percent = self.context_remaining_percent(&info); + self.bottom_pane.set_context_window_percent(percent); + self.token_info = Some(info); + } + + fn context_remaining_percent(&self, info: &TokenUsageInfo) -> Option { + info.model_context_window + .or(self.config.model_context_window) + .map(|window| { info.last_token_usage .percent_of_context_window_remaining(window) - }); - self.bottom_pane.set_context_window_percent(percent); - self.token_info = Some(info); + }) + } + + fn restore_pre_review_token_info(&mut self) { + if let Some(saved) = self.pre_review_token_info.take() { + match saved { + Some(info) => self.apply_token_info(info), + None => { + self.bottom_pane.set_context_window_percent(None); + self.token_info = None; + } + } } } @@ -1150,6 +1175,7 @@ impl ChatWidget { suppress_session_configured_redraw: false, pending_notification: None, is_review_mode: false, + pre_review_token_info: None, needs_final_message_separator: false, last_rendered_width: std::cell::Cell::new(None), feedback, @@ -1223,6 +1249,7 @@ impl ChatWidget { suppress_session_configured_redraw: true, pending_notification: None, is_review_mode: false, + pre_review_token_info: None, needs_final_message_separator: false, last_rendered_width: std::cell::Cell::new(None), feedback, @@ -1693,6 +1720,9 @@ impl ChatWidget { fn on_entered_review_mode(&mut self, review: ReviewRequest) { // Enter review mode and emit a concise banner + if self.pre_review_token_info.is_none() { + self.pre_review_token_info = Some(self.token_info.clone()); + } self.is_review_mode = true; let banner = format!(">> Code review started: {} <<", review.user_facing_hint); self.add_to_history(history_cell::new_review_status_line(banner)); @@ -1733,6 +1763,7 @@ impl ChatWidget { } self.is_review_mode = false; + self.restore_pre_review_token_info(); // Append a finishing banner at the end of this turn. self.add_to_history(history_cell::new_review_status_line( "<< Code review finished >>".to_string(), diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index a6ba546472..a4cef22418 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -38,6 +38,9 @@ use codex_core::protocol::ReviewRequest; use codex_core::protocol::StreamErrorEvent; use codex_core::protocol::TaskCompleteEvent; use codex_core::protocol::TaskStartedEvent; +use codex_core::protocol::TokenCountEvent; +use codex_core::protocol::TokenUsage; +use codex_core::protocol::TokenUsageInfo; use codex_core::protocol::UndoCompletedEvent; use codex_core::protocol::UndoStartedEvent; use codex_core::protocol::ViewImageToolCallEvent; @@ -215,6 +218,81 @@ fn exited_review_mode_emits_results_and_finishes() { assert!(!chat.is_review_mode); } +/// Exiting review restores the pre-review context window indicator. +#[test] +fn review_restores_context_window_indicator() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(); + + let context_window = 13_000; + let pre_review_tokens = 12_700; // ~30% remaining after subtracting baseline. + let review_tokens = 12_030; // ~97% remaining after subtracting baseline. + + chat.handle_codex_event(Event { + id: "token-before".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: Some(make_token_info(pre_review_tokens, context_window)), + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), Some(30)); + + chat.handle_codex_event(Event { + id: "review-start".into(), + msg: EventMsg::EnteredReviewMode(ReviewRequest { + prompt: "Review the latest changes".to_string(), + user_facing_hint: "feature branch".to_string(), + append_to_original_thread: true, + }), + }); + + chat.handle_codex_event(Event { + id: "token-review".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: Some(make_token_info(review_tokens, context_window)), + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), Some(97)); + + chat.handle_codex_event(Event { + id: "review-end".into(), + msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: None, + }), + }); + let _ = drain_insert_history(&mut rx); + + assert_eq!(chat.bottom_pane.context_window_percent(), Some(30)); + assert!(!chat.is_review_mode); +} + +/// Receiving a TokenCount event without usage clears the context indicator. +#[test] +fn token_count_none_resets_context_indicator() { + let (mut chat, _rx, _ops) = make_chatwidget_manual(); + + let context_window = 13_000; + let pre_compact_tokens = 12_700; + + chat.handle_codex_event(Event { + id: "token-before".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: Some(make_token_info(pre_compact_tokens, context_window)), + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), Some(30)); + + chat.handle_codex_event(Event { + id: "token-cleared".into(), + msg: EventMsg::TokenCount(TokenCountEvent { + info: None, + rate_limits: None, + }), + }); + assert_eq!(chat.bottom_pane.context_window_percent(), None); +} + #[cfg_attr( target_os = "macos", ignore = "system configuration APIs are blocked under macOS seatbelt" @@ -292,6 +370,7 @@ fn make_chatwidget_manual() -> ( suppress_session_configured_redraw: false, pending_notification: None, is_review_mode: false, + pre_review_token_info: None, needs_final_message_separator: false, last_rendered_width: std::cell::Cell::new(None), feedback: codex_feedback::CodexFeedback::new(), @@ -338,6 +417,21 @@ fn lines_to_single_string(lines: &[ratatui::text::Line<'static>]) -> String { s } +fn make_token_info(total_tokens: i64, context_window: i64) -> TokenUsageInfo { + fn usage(total_tokens: i64) -> TokenUsage { + TokenUsage { + total_tokens, + ..TokenUsage::default() + } + } + + TokenUsageInfo { + total_token_usage: usage(total_tokens), + last_token_usage: usage(total_tokens), + model_context_window: Some(context_window), + } +} + #[test] fn rate_limit_warnings_emit_thresholds() { let mut state = RateLimitWarningState::default(); From a6597a9958d6ae54e1a3601c653a42cd7d70979a Mon Sep 17 00:00:00 2001 From: Lionel Cheng <60159831+lionelchg@users.noreply.github.com> Date: Wed, 19 Nov 2025 15:52:24 -0800 Subject: [PATCH 3/6] Fix/correct reasoning display (#6749) This closes #6748 by implementing fallback to `model_family.default_reasoning_effort` in `reasoning_effort` display of `/status` when no `model_reasoning_effort` is set in the configuration. ## common/src/config_summary.rs - `create_config_summary_entries` now fills the "reasoning effort" entry with the explicit `config.model_reasoning_effort` when present and falls back to `config.model_family.default_reasoning_effort` when it is `None`, instead of emitting the literal string `none`. - This ensures downstream consumers such as `tui/src/status/helpers.rs` continue to work unchanged while automatically picking up model-family defaults when the user has not selected a reasoning effort. ## tui/src/status/helpers.rs / core/src/model_family.rs `ModelFamily::default_reasoning_effort` metadata is set to `medium` for both `gpt-5*-codex` and `gpt-5` models following the default behaviour of the API and recommendation of the codebase: - per https://platform.openai.com/docs/api-reference/responses/create `gpt-5` defaults to `medium` reasoning when no preset is passed - there is no mention of the preset for `gpt-5.1-codex` in the API docs but `medium` is the default setting for `gpt-5.1-codex` as per `codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__model_reasoning_selection_popup.snap` --------- Signed-off-by: lionelchg Co-authored-by: Eric Traut --- codex-rs/common/src/config_summary.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/codex-rs/common/src/config_summary.rs b/codex-rs/common/src/config_summary.rs index dabc606ce1..8fc1bb26f3 100644 --- a/codex-rs/common/src/config_summary.rs +++ b/codex-rs/common/src/config_summary.rs @@ -15,13 +15,12 @@ pub fn create_config_summary_entries(config: &Config) -> Vec<(&'static str, Stri if config.model_provider.wire_api == WireApi::Responses && config.model_family.supports_reasoning_summaries { - entries.push(( - "reasoning effort", - config - .model_reasoning_effort - .map(|effort| effort.to_string()) - .unwrap_or_else(|| "none".to_string()), - )); + let reasoning_effort = config + .model_reasoning_effort + .or(config.model_family.default_reasoning_effort) + .map(|effort| effort.to_string()) + .unwrap_or_else(|| "none".to_string()); + entries.push(("reasoning effort", reasoning_effort)); entries.push(( "reasoning summaries", config.model_reasoning_summary.to_string(), From 13d378f2ce3a95bdcea912c573c19fc4dca87ca3 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 19 Nov 2025 16:38:14 -0800 Subject: [PATCH 4/6] chore: refactor exec-server to prepare it for standalone MCP use (#6944) This PR reorganizes things slightly so that: - Instead of a single multitool executable, `codex-exec-server`, we now have two executables: - `codex-exec-mcp-server` to launch the MCP server - `codex-execve-wrapper` is the `execve(2)` wrapper to use with the `BASH_EXEC_WRAPPER` environment variable - `BASH_EXEC_WRAPPER` must be a single executable: it cannot be a command string composed of an executable with args (i.e., it no longer adds the `escalate` subcommand, as before) - `codex-exec-mcp-server` takes `--bash` and `--execve` as options. Though if `--execve` is not specified, the MCP server will check the directory containing `std::env::current_exe()` and attempt to use the file named `codex-execve-wrapper` within it. In development, this works out since these executables are side-by-side in the `target/debug` folder. With respect to testing, this also fixes an important bug in `dummy_exec_policy()`, as I was using `ends_with()` as if it applied to a `String`, but in this case, it is used with a `&Path`, so the semantics are slightly different. Putting this all together, I was able to test this by running the following: ``` ~/code/codex/codex-rs$ npx @modelcontextprotocol/inspector \ ./target/debug/codex-exec-mcp-server --bash ~/code/bash/bash ``` If I try to run `git status` in `/Users/mbolin/code/codex` via the `shell` tool from the MCP server: image then I get prompted with the following elicitation, as expected: image Though a current limitation is that the `shell` tool defaults to a timeout of 10s, which means I only have 10s to respond to the elicitation. Ideally, the time spent waiting for a response from a human should not count against the timeout for the command execution. I will address this in a subsequent PR. --- Note `~/code/bash/bash` was created by doing: ``` cd ~/code git clone https://github.com/bminor/bash cd bash git checkout a8a1c2fac029404d3f42cd39f5a20f24b6e4fe4b ./configure make ``` The patch: ``` diff --git a/execute_cmd.c b/execute_cmd.c index 070f5119..d20ad2b9 100644 --- a/execute_cmd.c +++ b/execute_cmd.c @@ -6129,6 +6129,19 @@ shell_execve (char *command, char **args, char **env) char sample[HASH_BANG_BUFSIZ]; size_t larray; + char* exec_wrapper = getenv("BASH_EXEC_WRAPPER"); + if (exec_wrapper && *exec_wrapper && !whitespace (*exec_wrapper)) + { + char *orig_command = command; + + larray = strvec_len (args); + + memmove (args + 2, args, (++larray) * sizeof (char *)); + args[0] = exec_wrapper; + args[1] = orig_command; + command = exec_wrapper; + } + ``` --- codex-rs/exec-server/Cargo.toml | 12 +- .../src/bin/main_execve_wrapper.rs | 8 + .../exec-server/src/bin/main_mcp_server.rs | 8 + codex-rs/exec-server/src/lib.rs | 8 + codex-rs/exec-server/src/main.rs | 11 -- codex-rs/exec-server/src/posix.rs | 163 +++++++----------- .../exec-server/src/posix/escalate_server.rs | 18 +- codex-rs/exec-server/src/posix/mcp.rs | 8 +- 8 files changed, 113 insertions(+), 123 deletions(-) create mode 100644 codex-rs/exec-server/src/bin/main_execve_wrapper.rs create mode 100644 codex-rs/exec-server/src/bin/main_mcp_server.rs create mode 100644 codex-rs/exec-server/src/lib.rs delete mode 100644 codex-rs/exec-server/src/main.rs diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 60e0facff1..54cead4118 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -4,8 +4,16 @@ name = "codex-exec-server" version = { workspace = true } [[bin]] -name = "codex-exec-server" -path = "src/main.rs" +name = "codex-execve-wrapper" +path = "src/bin/main_execve_wrapper.rs" + +[[bin]] +name = "codex-exec-mcp-server" +path = "src/bin/main_mcp_server.rs" + +[lib] +name = "codex_exec_server" +path = "src/lib.rs" [lints] workspace = true diff --git a/codex-rs/exec-server/src/bin/main_execve_wrapper.rs b/codex-rs/exec-server/src/bin/main_execve_wrapper.rs new file mode 100644 index 0000000000..3ab346e8ef --- /dev/null +++ b/codex-rs/exec-server/src/bin/main_execve_wrapper.rs @@ -0,0 +1,8 @@ +#[cfg(not(unix))] +fn main() { + eprintln!("codex-execve-wrapper is only implemented for UNIX"); + std::process::exit(1); +} + +#[cfg(unix)] +pub use codex_exec_server::main_execve_wrapper as main; diff --git a/codex-rs/exec-server/src/bin/main_mcp_server.rs b/codex-rs/exec-server/src/bin/main_mcp_server.rs new file mode 100644 index 0000000000..6c75ae4237 --- /dev/null +++ b/codex-rs/exec-server/src/bin/main_mcp_server.rs @@ -0,0 +1,8 @@ +#[cfg(not(unix))] +fn main() { + eprintln!("codex-exec-mcp-server is only implemented for UNIX"); + std::process::exit(1); +} + +#[cfg(unix)] +pub use codex_exec_server::main_mcp_server as main; diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs new file mode 100644 index 0000000000..adec09d4de --- /dev/null +++ b/codex-rs/exec-server/src/lib.rs @@ -0,0 +1,8 @@ +#[cfg(unix)] +mod posix; + +#[cfg(unix)] +pub use posix::main_execve_wrapper; + +#[cfg(unix)] +pub use posix::main_mcp_server; diff --git a/codex-rs/exec-server/src/main.rs b/codex-rs/exec-server/src/main.rs deleted file mode 100644 index 23a18b2525..0000000000 --- a/codex-rs/exec-server/src/main.rs +++ /dev/null @@ -1,11 +0,0 @@ -#[cfg(target_os = "windows")] -fn main() { - eprintln!("codex-exec-server is not implemented on Windows targets"); - std::process::exit(1); -} - -#[cfg(not(target_os = "windows"))] -mod posix; - -#[cfg(not(target_os = "windows"))] -pub use posix::main; diff --git a/codex-rs/exec-server/src/posix.rs b/codex-rs/exec-server/src/posix.rs index bbb624df09..239eaf61f2 100644 --- a/codex-rs/exec-server/src/posix.rs +++ b/codex-rs/exec-server/src/posix.rs @@ -56,15 +56,12 @@ //! o<-----x //! use std::path::Path; +use std::path::PathBuf; use clap::Parser; -use clap::Subcommand; use tracing_subscriber::EnvFilter; use tracing_subscriber::{self}; -use crate::posix::escalate_protocol::EscalateAction; -use crate::posix::escalate_server::EscalateServer; -use crate::posix::escalation_policy::EscalationPolicy; use crate::posix::mcp_escalation_policy::ExecPolicyOutcome; mod escalate_client; @@ -75,124 +72,84 @@ mod mcp; mod mcp_escalation_policy; mod socket; +/// Default value of --execve option relative to the current executable. +/// Note this must match the name of the binary as specified in Cargo.toml. +const CODEX_EXECVE_WRAPPER_EXE_NAME: &str = "codex-execve-wrapper"; + #[derive(Parser)] -#[command(version)] -pub struct Cli { - #[command(subcommand)] - subcommand: Option, -} +struct McpServerCli { + /// Executable to delegate execve(2) calls to in Bash. + #[arg(long = "execve")] + execve_wrapper: Option, -#[derive(Subcommand)] -enum Commands { - Escalate(EscalateArgs), - ShellExec(ShellExecArgs), -} - -/// Invoked from within the sandbox to (potentially) escalate permissions. -#[derive(Parser, Debug)] -struct EscalateArgs { - file: String, - - #[arg(trailing_var_arg = true)] - argv: Vec, -} - -impl EscalateArgs { - /// This is the escalate client. It talks to the escalate server to determine whether to exec() - /// the command directly or to proxy to the escalate server. - async fn run(self) -> anyhow::Result { - let EscalateArgs { file, argv } = self; - escalate_client::run(file, argv).await - } -} - -/// Debugging command to emulate an MCP "shell" tool call. -#[derive(Parser, Debug)] -struct ShellExecArgs { - command: String, + /// Path to Bash that has been patched to support execve() wrapping. + #[arg(long = "bash")] + bash_path: Option, } #[tokio::main] -pub async fn main() -> anyhow::Result<()> { - let cli = Cli::parse(); +pub async fn main_mcp_server() -> anyhow::Result<()> { tracing_subscriber::fmt() .with_env_filter(EnvFilter::from_default_env()) .with_writer(std::io::stderr) .with_ansi(false) .init(); - match cli.subcommand { - Some(Commands::Escalate(args)) => { - std::process::exit(args.run().await?); - } - Some(Commands::ShellExec(args)) => { - let bash_path = mcp::get_bash_path()?; - let escalate_server = EscalateServer::new(bash_path, DummyEscalationPolicy {}); - let result = escalate_server - .exec( - args.command.clone(), - std::env::vars().collect(), - std::env::current_dir()?, - None, - ) - .await?; - println!("{result:?}"); - std::process::exit(result.exit_code); - } + let cli = McpServerCli::parse(); + let execve_wrapper = match cli.execve_wrapper { + Some(path) => path, None => { - let bash_path = mcp::get_bash_path()?; - - tracing::info!("Starting MCP server"); - let service = mcp::serve(bash_path, dummy_exec_policy) - .await - .inspect_err(|e| { - tracing::error!("serving error: {:?}", e); - })?; - - service.waiting().await?; - Ok(()) + let cwd = std::env::current_exe()?; + cwd.parent() + .map(|p| p.join(CODEX_EXECVE_WRAPPER_EXE_NAME)) + .ok_or_else(|| { + anyhow::anyhow!("failed to determine execve wrapper path from current exe") + })? } - } + }; + let bash_path = match cli.bash_path { + Some(path) => path, + None => mcp::get_bash_path()?, + }; + + tracing::info!("Starting MCP server"); + let service = mcp::serve(bash_path, execve_wrapper, dummy_exec_policy) + .await + .inspect_err(|e| { + tracing::error!("serving error: {:?}", e); + })?; + + service.waiting().await?; + Ok(()) +} + +#[derive(Parser)] +pub struct ExecveWrapperCli { + file: String, + + #[arg(trailing_var_arg = true)] + argv: Vec, +} + +#[tokio::main] +pub async fn main_execve_wrapper() -> anyhow::Result<()> { + tracing_subscriber::fmt() + .with_env_filter(EnvFilter::from_default_env()) + .with_writer(std::io::stderr) + .with_ansi(false) + .init(); + + let ExecveWrapperCli { file, argv } = ExecveWrapperCli::parse(); + let exit_code = escalate_client::run(file, argv).await?; + std::process::exit(exit_code); } // TODO: replace with execpolicy2 -struct DummyEscalationPolicy; - -#[async_trait::async_trait] -impl EscalationPolicy for DummyEscalationPolicy { - async fn determine_action( - &self, - file: &Path, - argv: &[String], - workdir: &Path, - ) -> Result { - let outcome = dummy_exec_policy(file, argv, workdir); - let action = match outcome { - ExecPolicyOutcome::Allow { - run_with_escalated_permissions, - } => { - if run_with_escalated_permissions { - EscalateAction::Escalate - } else { - EscalateAction::Run - } - } - ExecPolicyOutcome::Forbidden => EscalateAction::Deny { - reason: Some("Execution forbidden by policy".to_string()), - }, - ExecPolicyOutcome::Prompt { .. } => EscalateAction::Deny { - reason: Some("Could not prompt user for permission".to_string()), - }, - }; - Ok(action) - } -} - fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> ExecPolicyOutcome { - if file.ends_with("/rm") { + if file.ends_with("rm") { ExecPolicyOutcome::Forbidden - } else if file.ends_with("/git") { + } else if file.ends_with("git") { ExecPolicyOutcome::Prompt { run_with_escalated_permissions: false, } diff --git a/codex-rs/exec-server/src/posix/escalate_server.rs b/codex-rs/exec-server/src/posix/escalate_server.rs index 4f259f0506..1f76687ce0 100644 --- a/codex-rs/exec-server/src/posix/escalate_server.rs +++ b/codex-rs/exec-server/src/posix/escalate_server.rs @@ -27,16 +27,18 @@ use crate::posix::socket::AsyncSocket; pub(crate) struct EscalateServer { bash_path: PathBuf, + execve_wrapper: PathBuf, policy: Arc, } impl EscalateServer { - pub fn new

(bash_path: PathBuf, policy: P) -> Self + pub fn new

(bash_path: PathBuf, execve_wrapper: PathBuf, policy: P) -> Self where P: EscalationPolicy + Send + Sync + 'static, { Self { bash_path, + execve_wrapper, policy: Arc::new(policy), } } @@ -60,8 +62,15 @@ impl EscalateServer { ); env.insert( BASH_EXEC_WRAPPER_ENV_VAR.to_string(), - format!("{} escalate", std::env::current_exe()?.to_string_lossy()), + self.execve_wrapper.to_string_lossy().to_string(), ); + + // TODO: use the sandbox policy and cwd from the calling client. + // Note that sandbox_cwd is ignored for ReadOnly, but needs to be legit + // for `SandboxPolicy::WorkspaceWrite`. + let sandbox_policy = SandboxPolicy::ReadOnly; + let sandbox_cwd = PathBuf::from("/__NONEXISTENT__"); + let result = process_exec_tool_call( codex_core::exec::ExecParams { command: vec![ @@ -77,9 +86,8 @@ impl EscalateServer { arg0: None, }, get_platform_sandbox().unwrap_or(SandboxType::None), - // TODO: use the sandbox policy and cwd from the calling client - &SandboxPolicy::ReadOnly, - &PathBuf::from("/__NONEXISTENT__"), // This is ignored for ReadOnly + &sandbox_policy, + &sandbox_cwd, &None, None, ) diff --git a/codex-rs/exec-server/src/posix/mcp.rs b/codex-rs/exec-server/src/posix/mcp.rs index 2a6e84dd9e..f5785dc5d0 100644 --- a/codex-rs/exec-server/src/posix/mcp.rs +++ b/codex-rs/exec-server/src/posix/mcp.rs @@ -65,15 +65,17 @@ impl From for ExecResult { pub struct ExecTool { tool_router: ToolRouter, bash_path: PathBuf, + execve_wrapper: PathBuf, policy: ExecPolicy, } #[tool_router] impl ExecTool { - pub fn new(bash_path: PathBuf, policy: ExecPolicy) -> Self { + pub fn new(bash_path: PathBuf, execve_wrapper: PathBuf, policy: ExecPolicy) -> Self { Self { tool_router: Self::tool_router(), bash_path, + execve_wrapper, policy, } } @@ -87,6 +89,7 @@ impl ExecTool { ) -> Result { let escalate_server = EscalateServer::new( self.bash_path.clone(), + self.execve_wrapper.clone(), McpEscalationPolicy::new(self.policy, context), ); let result = escalate_server @@ -130,8 +133,9 @@ impl ServerHandler for ExecTool { pub(crate) async fn serve( bash_path: PathBuf, + execve_wrapper: PathBuf, policy: ExecPolicy, ) -> Result, rmcp::service::ServerInitializeError> { - let tool = ExecTool::new(bash_path, policy); + let tool = ExecTool::new(bash_path, execve_wrapper, policy); tool.serve(stdio()).await } From b00a7cf40d7faceab3ed3789c0429d729c91f7b6 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 19 Nov 2025 16:41:38 -0800 Subject: [PATCH 5/6] fix(shell) fallback shells (#6948) ## Summary Add fallbacks when user_shell_path does not resolve to a known shell type ## Testing - [x] Tests still pass --- codex-rs/core/src/shell.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 7bfec089c5..1fb2b97dd7 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -204,10 +204,21 @@ pub async fn default_user_shell() -> Shell { if cfg!(windows) { get_shell(ShellType::PowerShell, None).unwrap_or(Shell::Unknown) } else { - get_user_shell_path() + let user_default_shell = get_user_shell_path() .and_then(|shell| detect_shell_type(&shell)) - .and_then(|shell_type| get_shell(shell_type, None)) - .unwrap_or(Shell::Unknown) + .and_then(|shell_type| get_shell(shell_type, None)); + + let shell_with_fallback = if cfg!(target_os = "macos") { + user_default_shell + .or_else(|| get_shell(ShellType::Zsh, None)) + .or_else(|| get_shell(ShellType::Bash, None)) + } else { + user_default_shell + .or_else(|| get_shell(ShellType::Bash, None)) + .or_else(|| get_shell(ShellType::Zsh, None)) + }; + + shell_with_fallback.unwrap_or(Shell::Unknown) } } From b4f38201fe5206b98f560e994b9a91ad9877f045 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 19 Nov 2025 17:51:39 -0800 Subject: [PATCH 6/6] fix: clean up elicitation used by exec-server --- .../src/posix/mcp_escalation_policy.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs index 24f42fe91b..39958bb4f9 100644 --- a/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs +++ b/codex-rs/exec-server/src/posix/mcp_escalation_policy.rs @@ -6,8 +6,6 @@ use rmcp::model::CreateElicitationRequestParam; use rmcp::model::CreateElicitationResult; use rmcp::model::ElicitationAction; use rmcp::model::ElicitationSchema; -use rmcp::model::PrimitiveSchema; -use rmcp::model::StringSchema; use rmcp::service::RequestContext; use crate::posix::escalate_protocol::EscalateAction; @@ -54,12 +52,19 @@ impl McpEscalationPolicy { context .peer .create_elicitation(CreateElicitationRequestParam { - message: format!("Allow Codex to run `{command:?}` in `{workdir:?}`?"), - #[allow(clippy::expect_used)] + message: format!("Allow agent to run `{command}` in `{}`?", workdir.display()), requested_schema: ElicitationSchema::builder() - .property("dummy", PrimitiveSchema::String(StringSchema::new())) + .title("Execution Permission Request") + .optional_string_with("reason", |schema| { + schema.description("Optional reason for allowing or denying execution") + }) .build() - .expect("failed to build elicitation schema"), + .map_err(|e| { + McpError::internal_error( + format!("failed to build elicitation schema: {e}"), + None, + ) + })?, }) .await .map_err(|e| McpError::internal_error(e.to_string(), None))