From c0f7e1b99fce210e1e0dfc97160d79454c04a987 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 20 May 2026 10:00:33 -0700 Subject: [PATCH 1/7] [2 of 2] Start fresh TUI thread in background (#23176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why After the terminal-probe work in #23175, fresh-session startup still waits for `thread/start` before the chat input can become usable. The chat widget already has the machinery to hold early submissions until a session is configured, so fresh `thread/start` does not need to stay on the input-ready hot path. Refs #16335. ## What This PR starts fresh app-server threads in a background task, reports completion through a startup app event, and attaches the primary session once `thread/start` returns. Resume and fork startup paths remain synchronous. ## Benchmark In the local pty startup benchmark, this PR's pre-optimization base branch, #23175, measured about 152ms median from launch to accepted chat input. The stacked result measured about 66ms median, for an approximate additional savings of 85-95ms. For broader context, the original `main` baseline before either startup optimization was about 250.5ms median. We also measured Codex 0.117.0 on the same machine at about 64.6ms median, so the stacked branch is back in the old-startup-time range. ## Stack 1. [#23175: [1 of 2] Optimize TUI startup terminal probes](https://github.com/openai/codex/pull/23175) — base PR 2. [#23176: [2 of 2] Start fresh TUI thread in background](https://github.com/openai/codex/pull/23176) — this PR ## Verification - `cargo test -p codex-tui` --- codex-rs/tui/src/app.rs | 35 +++++- codex-rs/tui/src/app/event_dispatch.rs | 4 + codex-rs/tui/src/app/session_lifecycle.rs | 38 ++++++ codex-rs/tui/src/app/side.rs | 6 +- codex-rs/tui/src/app/test_support.rs | 1 + codex-rs/tui/src/app/tests.rs | 2 + codex-rs/tui/src/app/tests/startup.rs | 118 ++++++++++++++++++ codex-rs/tui/src/app_event.rs | 6 + codex-rs/tui/src/app_server_session.rs | 26 +++- codex-rs/tui/src/bottom_pane/chat_composer.rs | 92 +++++++++++++- codex-rs/tui/src/bottom_pane/mod.rs | 4 + codex-rs/tui/src/chatwidget/input_flow.rs | 5 + codex-rs/tui/src/chatwidget/session_flow.rs | 2 + 13 files changed, 329 insertions(+), 10 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 3164f16acb..0288c9c593 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -543,6 +543,7 @@ pub(crate) struct App { primary_session_configured: Option, pending_primary_events: VecDeque, pending_app_server_requests: PendingAppServerRequests, + pending_startup_thread_start: bool, // Serialize plugin enablement writes per plugin so stale completions cannot // overwrite a newer toggle, even if the plugin is toggled from different // cwd contexts. @@ -575,6 +576,27 @@ async fn resolve_runtime_model_provider_base_url(provider: &ModelProviderInfo) - } } +fn spawn_startup_thread_start( + app_server: &AppServerSession, + config: Config, + app_event_tx: AppEventSender, +) { + let request_handle = app_server.request_handle(); + let thread_params_mode = app_server.thread_params_mode(); + let remote_cwd_override = app_server.remote_cwd_override().map(Path::to_path_buf); + tokio::spawn(async move { + let result = crate::app_server_session::start_thread_with_request_handle( + request_handle, + config, + thread_params_mode, + remote_cwd_override, + ) + .await + .map_err(|err| format!("{err:#}")); + app_event_tx.send(AppEvent::StartupThreadStarted { result }); + }); +} + #[derive(Debug, Clone, PartialEq, Eq)] enum ActiveTurnSteerRace { Missing, @@ -778,10 +800,14 @@ impl App { &initial_images, ); let thread_and_widget_started_at = Instant::now(); + let pending_startup_thread_start = matches!( + &session_selection, + SessionSelection::StartFresh | SessionSelection::Exit + ); let (mut chat_widget, initial_started_thread) = match session_selection { SessionSelection::StartFresh | SessionSelection::Exit => { - let started = app_server.start_thread(&config).await?; - // Only count a startup tooltip once the fresh thread can actually render it. + spawn_startup_thread_start(&app_server, config.clone(), app_event_tx.clone()); + // Count a startup tooltip once the initial chat widget can render it. let startup_tooltip_override = prepare_startup_tooltip_override(&mut config, &available_models, is_first_run) .await; @@ -811,7 +837,9 @@ impl App { .clone(), session_telemetry: session_telemetry.clone(), }; - (ChatWidget::new_with_app_event(init), Some(started)) + let mut chat_widget = ChatWidget::new_with_app_event(init); + chat_widget.set_queue_submissions_until_session_configured(/*queue*/ true); + (chat_widget, None) } SessionSelection::Resume(target_session) => { let resumed = app_server @@ -956,6 +984,7 @@ See the Codex keymap documentation for supported actions and examples." primary_session_configured: None, pending_primary_events: VecDeque::new(), pending_app_server_requests: PendingAppServerRequests::default(), + pending_startup_thread_start, pending_plugin_enabled_writes: HashMap::new(), pending_hook_enabled_writes: HashMap::new(), }; diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 68bd759e2f..c1cd816b0d 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -23,6 +23,10 @@ impl App { ) .await; } + AppEvent::StartupThreadStarted { result } => { + self.handle_startup_thread_started(app_server, result) + .await?; + } AppEvent::ClearUi => { self.clear_terminal_ui(tui, /*redraw_header*/ false)?; self.reset_app_ui_state_after_clear(); diff --git a/codex-rs/tui/src/app/session_lifecycle.rs b/codex-rs/tui/src/app/session_lifecycle.rs index 6ea53d8b3c..6c87894434 100644 --- a/codex-rs/tui/src/app/session_lifecycle.rs +++ b/codex-rs/tui/src/app/session_lifecycle.rs @@ -418,10 +418,48 @@ impl App { self.primary_session_configured = None; self.pending_primary_events.clear(); self.pending_app_server_requests.clear(); + self.pending_startup_thread_start = false; self.chat_widget.set_pending_thread_approvals(Vec::new()); self.sync_active_agent_label(); } + pub(super) async fn handle_startup_thread_started( + &mut self, + app_server: &mut AppServerSession, + result: Result, + ) -> Result<()> { + if !self.pending_startup_thread_start { + if let Ok(started) = result { + let thread_id = started.session.thread_id; + if let Err(err) = app_server.thread_unsubscribe(thread_id).await { + tracing::warn!( + thread_id = %thread_id, + "failed to unsubscribe stale startup thread: {err}" + ); + } + self.discard_thread_local_state(thread_id).await; + } + return Ok(()); + } + + self.pending_startup_thread_start = false; + self.chat_widget + .set_queue_submissions_until_session_configured(/*queue*/ false); + match result { + Ok(started) => { + self.enqueue_primary_thread_session(started.session, started.turns) + .await?; + self.chat_widget.maybe_send_next_queued_input(); + } + Err(err) => { + return Err(color_eyre::eyre::eyre!( + "Failed to start a fresh session through the app server: {err}" + )); + } + } + Ok(()) + } + pub(super) async fn start_fresh_session_with_summary_hint( &mut self, tui: &mut tui::Tui, diff --git a/codex-rs/tui/src/app/side.rs b/codex-rs/tui/src/app/side.rs index 044cb9b391..492639f139 100644 --- a/codex-rs/tui/src/app/side.rs +++ b/codex-rs/tui/src/app/side.rs @@ -369,15 +369,15 @@ impl App { self.chat_widget.add_error_message(message); return false; } - self.discard_side_thread_local(thread_id).await; + self.discard_thread_local_state(thread_id).await; true } pub(super) async fn discard_closed_side_thread(&mut self, thread_id: ThreadId) { - self.discard_side_thread_local(thread_id).await; + self.discard_thread_local_state(thread_id).await; } - async fn discard_side_thread_local(&mut self, thread_id: ThreadId) { + pub(super) async fn discard_thread_local_state(&mut self, thread_id: ThreadId) { self.abort_thread_event_listener(thread_id); self.thread_event_channels.remove(&thread_id); self.side_threads.remove(&thread_id); diff --git a/codex-rs/tui/src/app/test_support.rs b/codex-rs/tui/src/app/test_support.rs index 6b6c424210..10c234a903 100644 --- a/codex-rs/tui/src/app/test_support.rs +++ b/codex-rs/tui/src/app/test_support.rs @@ -60,6 +60,7 @@ pub(super) async fn make_test_app() -> App { primary_session_configured: None, pending_primary_events: VecDeque::new(), pending_app_server_requests: PendingAppServerRequests::default(), + pending_startup_thread_start: false, pending_plugin_enabled_writes: HashMap::new(), pending_hook_enabled_writes: HashMap::new(), } diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index bddf5e544c..8d52500af6 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -3882,6 +3882,7 @@ async fn make_test_app() -> App { primary_session_configured: None, pending_primary_events: VecDeque::new(), pending_app_server_requests: PendingAppServerRequests::default(), + pending_startup_thread_start: false, pending_plugin_enabled_writes: HashMap::new(), pending_hook_enabled_writes: HashMap::new(), } @@ -3945,6 +3946,7 @@ async fn make_test_app_with_channels() -> ( primary_session_configured: None, pending_primary_events: VecDeque::new(), pending_app_server_requests: PendingAppServerRequests::default(), + pending_startup_thread_start: false, pending_plugin_enabled_writes: HashMap::new(), pending_hook_enabled_writes: HashMap::new(), }, diff --git a/codex-rs/tui/src/app/tests/startup.rs b/codex-rs/tui/src/app/tests/startup.rs index c0aa1b61b7..51d6622e16 100644 --- a/codex-rs/tui/src/app/tests/startup.rs +++ b/codex-rs/tui/src/app/tests/startup.rs @@ -1,4 +1,7 @@ use super::*; +use crossterm::event::KeyCode; +use crossterm::event::KeyEvent; +use crossterm::event::KeyModifiers; use pretty_assertions::assert_eq; #[test] @@ -132,6 +135,121 @@ fn startup_waiting_gate_not_applied_for_resume_or_fork_session_selection() { ); } +#[tokio::test] +async fn startup_thread_started_submits_queued_startup_input() { + let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; + app.pending_startup_thread_start = true; + app.chat_widget + .set_queue_submissions_until_session_configured(/*queue*/ true); + app.chat_widget + .apply_external_edit("queued before startup completes".to_string()); + app.chat_widget + .handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!( + app.chat_widget.queued_user_message_texts(), + vec!["queued before startup completes".to_string()] + ); + + let mut app_server = Box::pin(crate::start_embedded_app_server_for_picker( + app.chat_widget.config_ref(), + )) + .await + .expect("embedded app server"); + let thread_id = ThreadId::new(); + app.handle_startup_thread_started( + &mut app_server, + Ok(AppServerStartedThread { + session: test_thread_session(thread_id, test_path_buf("/tmp/project")), + turns: Vec::new(), + }), + ) + .await + .expect("startup thread should attach"); + + match next_user_turn_op(&mut op_rx) { + Op::UserTurn { items, .. } => assert_eq!( + items, + vec![UserInput::Text { + text: "queued before startup completes".to_string(), + text_elements: Vec::new(), + }] + ), + other => panic!("expected queued startup input submission, got {other:?}"), + } +} + +#[tokio::test] +async fn startup_thread_start_failure_returns_error() { + let (mut app, _app_event_rx, _op_rx) = make_test_app_with_channels().await; + app.pending_startup_thread_start = true; + + let mut app_server = Box::pin(crate::start_embedded_app_server_for_picker( + app.chat_widget.config_ref(), + )) + .await + .expect("embedded app server"); + let err = app + .handle_startup_thread_started(&mut app_server, Err("boom".to_string())) + .await + .expect_err("startup thread failure should exit instead of leaving chat unconfigured"); + + assert!( + err.to_string() + .contains("Failed to start a fresh session through the app server: boom") + ); + assert!(!app.pending_startup_thread_start); + assert_eq!(app.primary_thread_id, None); +} + +#[test] +fn stale_startup_thread_started_removes_local_routing_state() -> Result<()> { + tokio::runtime::Builder::new_multi_thread() + .worker_threads(1) + .thread_stack_size(8 * 1024 * 1024) + .enable_all() + .build()? + .block_on(async { + let mut app = make_test_app().await; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + let primary_thread_id = ThreadId::new(); + let stale_thread_id = ThreadId::new(); + app.primary_thread_id = Some(primary_thread_id); + app.thread_event_channels.insert( + primary_thread_id, + ThreadEventChannel::new(THREAD_EVENT_CHANNEL_CAPACITY), + ); + app.activate_thread_channel(primary_thread_id).await; + app.thread_event_channels.insert( + stale_thread_id, + ThreadEventChannel::new(THREAD_EVENT_CHANNEL_CAPACITY), + ); + app.agent_navigation.upsert( + stale_thread_id, + /*agent_nickname*/ None, + /*agent_role*/ None, + /*is_closed*/ false, + ); + assert!(app.thread_event_channels.contains_key(&stale_thread_id)); + assert!(app.agent_navigation.get(&stale_thread_id).is_some()); + + app.handle_startup_thread_started( + &mut app_server, + Ok(AppServerStartedThread { + session: test_thread_session(stale_thread_id, test_path_buf("/tmp/project")), + turns: Vec::new(), + }), + ) + .await?; + + assert!(!app.thread_event_channels.contains_key(&stale_thread_id)); + assert_eq!(app.agent_navigation.get(&stale_thread_id), None); + assert_eq!(app.active_thread_id, Some(primary_thread_id)); + Ok(()) + }) +} + #[tokio::test] async fn ignore_same_thread_resume_reports_noop_for_current_thread() { let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 07a5b51175..a85ee5a70c 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -33,6 +33,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_approval_presets::ApprovalPreset; use crate::app_command::AppCommand; +use crate::app_server_session::AppServerStartedThread; use crate::bottom_pane::ApprovalRequest; use crate::bottom_pane::StatusLineItem; use crate::bottom_pane::TerminalTitleItem; @@ -180,6 +181,11 @@ pub(crate) enum AppEvent { /// Start a new session. NewSession, + /// Result of the fresh startup thread that is attached after the input UI is live. + StartupThreadStarted { + result: Result, + }, + /// Clear the terminal UI (screen + scrollback), start a fresh session, and keep the /// previous chat resumable. ClearUi, diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 1bcb4a5597..51029a6dee 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -119,6 +119,7 @@ use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; use std::collections::HashMap; use std::path::PathBuf; +use uuid::Uuid; fn bootstrap_request_error(context: &'static str, err: TypedRequestError) -> color_eyre::Report { color_eyre::eyre::eyre!("{context}: {err}") @@ -167,6 +168,7 @@ impl ThreadParamsMode { } } +#[derive(Debug)] pub(crate) struct AppServerStartedThread { pub(crate) session: ThreadSessionState, pub(crate) turns: Vec, @@ -337,6 +339,7 @@ impl AppServerSession { self.client.next_event().await } + #[cfg(test)] pub(crate) async fn start_thread(&mut self, config: &Config) -> Result { self.start_thread_with_session_start_source(config, /*session_start_source*/ None) .await @@ -427,7 +430,7 @@ impl AppServerSession { Ok(started) } - fn thread_params_mode(&self) -> ThreadParamsMode { + pub(crate) fn thread_params_mode(&self) -> ThreadParamsMode { self.thread_params_mode } @@ -1002,6 +1005,27 @@ impl AppServerSession { } } +pub(crate) async fn start_thread_with_request_handle( + request_handle: AppServerRequestHandle, + config: Config, + thread_params_mode: ThreadParamsMode, + remote_cwd_override: Option, +) -> Result { + let response: ThreadStartResponse = request_handle + .request_typed(ClientRequest::ThreadStart { + request_id: RequestId::String(format!("startup-thread-start-{}", Uuid::new_v4())), + params: thread_start_params_from_config( + &config, + thread_params_mode, + remote_cwd_override.as_deref(), + /*session_start_source*/ None, + ), + }) + .await + .map_err(|err| bootstrap_request_error("thread/start failed during TUI bootstrap", err))?; + started_thread_from_start_response(response, &config, thread_params_mode).await +} + fn thread_realtime_start_params( thread_id: ThreadId, transport: Option, diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index ef870c6776..a3413dd3be 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -355,6 +355,7 @@ pub(crate) struct ChatComposer { attachments: AttachmentState, placeholder_text: String, is_task_running: bool, + queue_submissions: bool, /// Slash-command draft staged for local recall after application-level dispatch. /// /// This slot is intentionally separate from `ChatComposerHistory` so inline slash commands can @@ -524,6 +525,7 @@ impl ChatComposer { attachments: AttachmentState::default(), placeholder_text, is_task_running: false, + queue_submissions: false, pending_slash_command_history: None, #[cfg(not(target_os = "linux"))] next_element_id: 0, @@ -2817,6 +2819,9 @@ impl ChatComposer { now: Instant, ) -> (InputResult, bool) { if should_queue { + if let Some(pasted) = self.draft.paste_burst.flush_before_modified_input() { + self.handle_paste(pasted); + } let raw_text = self.draft.textarea.text(); let defer_slash_validation = self.should_parse_as_slash_on_dequeue_from_raw_text(raw_text); @@ -3227,13 +3232,13 @@ impl ChatComposer { self.footer.mode = reset_mode_after_activity(self.footer.mode); } if self.queue_keys.is_pressed(key_event) - && (self.is_task_running || !self.is_bang_shell_command()) + && (self.is_task_running || self.queue_submissions || !self.is_bang_shell_command()) { - return self.handle_submission(self.is_task_running); + return self.handle_submission(self.is_task_running || self.queue_submissions); } if self.submit_keys.is_pressed(key_event) { - return self.handle_submission(/*should_queue*/ false); + return self.handle_submission(self.queue_submissions); } if let KeyEvent { @@ -4143,6 +4148,10 @@ impl ChatComposer { self.is_task_running = running; } + pub(crate) fn set_queue_submissions(&mut self, queue_submissions: bool) { + self.queue_submissions = queue_submissions; + } + pub(crate) fn set_context_window(&mut self, percent: Option, used_tokens: Option) { if self.footer.context_window_percent == percent && self.footer.context_window_used_tokens == used_tokens @@ -7064,6 +7073,49 @@ mod tests { assert_eq!(composer.draft.textarea.text(), "hi\nthere"); } + /// Behavior: startup-pending submissions are queued immediately, so Enter should flush any + /// buffered burst text into that queued message instead of turning into a draft newline. + #[test] + fn queued_submission_flushes_ascii_burst_instead_of_inserting_newline() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + + let mut now = Instant::now(); + let step = Duration::from_millis(1); + for ch in ['h', 'i'] { + let _ = composer.handle_input_basic_with_time( + KeyEvent::new(KeyCode::Char(ch), KeyModifiers::NONE), + now, + ); + now += step; + } + assert!(composer.is_in_paste_burst()); + + let (result, _) = composer.handle_submission_with_time(/*should_queue*/ true, now); + + assert_eq!( + result, + InputResult::Queued { + text: "hi".to_string(), + text_elements: Vec::new(), + action: QueuedInputAction::Plain, + } + ); + assert!(composer.draft.textarea.text().is_empty()); + assert!(!composer.is_in_paste_burst()); + } + /// Behavior: even if Enter suppression would normally be active for a burst, Enter should /// still dispatch a built-in slash command when the first line begins with `/`. #[test] @@ -8029,6 +8081,40 @@ mod tests { assert!(found_error, "expected error history cell to be sent"); } + #[test] + fn enter_queues_when_queue_submissions_is_enabled() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + /*has_input_focus*/ true, + sender, + /*enhanced_keys_supported*/ false, + "Ask Codex to do anything".to_string(), + /*disable_paste_burst*/ false, + ); + composer.set_queue_submissions(/*queue_submissions*/ true); + composer + .draft + .textarea + .set_text_clearing_elements("queued before session"); + + let (result, _needs_redraw) = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!( + result, + InputResult::Queued { + text: "queued before session".to_string(), + text_elements: Vec::new(), + action: QueuedInputAction::Plain, + } + ); + } + #[test] fn tab_queues_slash_led_prompts_while_task_running_without_validation() { use crossterm::event::KeyCode; diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 5aeb2a4cfd..65770b61ae 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -999,6 +999,10 @@ impl BottomPane { } } + pub(crate) fn set_queue_submissions(&mut self, queue_submissions: bool) { + self.composer.set_queue_submissions(queue_submissions); + } + /// Hide the status indicator while leaving task-running state untouched. pub(crate) fn hide_status_indicator(&mut self) { if self.status.take().is_some() { diff --git a/codex-rs/tui/src/chatwidget/input_flow.rs b/codex-rs/tui/src/chatwidget/input_flow.rs index 704d5dcac6..dbc52269a3 100644 --- a/codex-rs/tui/src/chatwidget/input_flow.rs +++ b/codex-rs/tui/src/chatwidget/input_flow.rs @@ -72,6 +72,11 @@ impl ChatWidget { self.queue_user_message_with_options(user_message, QueuedInputAction::Plain); } + pub(crate) fn set_queue_submissions_until_session_configured(&mut self, queue: bool) { + self.bottom_pane + .set_queue_submissions(queue && !self.is_session_configured()); + } + pub(super) fn queue_user_message_with_options( &mut self, user_message: UserMessage, diff --git a/codex-rs/tui/src/chatwidget/session_flow.rs b/codex-rs/tui/src/chatwidget/session_flow.rs index 2b8beefdf8..9837eaa6ea 100644 --- a/codex-rs/tui/src/chatwidget/session_flow.rs +++ b/codex-rs/tui/src/chatwidget/session_flow.rs @@ -20,6 +20,8 @@ impl ChatWidget { self.session_network_proxy = session.network_proxy.clone(); let previous_thread_id = self.thread_id; self.thread_id = Some(session.thread_id); + self.bottom_pane + .set_queue_submissions(/*queue_submissions*/ false); if previous_thread_id != self.thread_id { self.review.recent_auto_review_denials = RecentAutoReviewDenials::default(); } From 242d886cfe6845a3d77527b0161d2da4727f1c3e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:01:58 -0700 Subject: [PATCH 2/7] windows-sandbox: add resolved permissions helper --- codex-rs/windows-sandbox-rs/src/allow.rs | 131 ++++++++++-------- codex-rs/windows-sandbox-rs/src/lib.rs | 2 + .../src/resolved_permissions.rs | 116 ++++++++++++++++ codex-rs/windows-sandbox-rs/src/setup.rs | 7 +- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 3 +- 5 files changed, 194 insertions(+), 65 deletions(-) create mode 100644 codex-rs/windows-sandbox-rs/src/resolved_permissions.rs diff --git a/codex-rs/windows-sandbox-rs/src/allow.rs b/codex-rs/windows-sandbox-rs/src/allow.rs index 5b6d0d617a..e82cf8cd62 100644 --- a/codex-rs/windows-sandbox-rs/src/allow.rs +++ b/codex-rs/windows-sandbox-rs/src/allow.rs @@ -1,4 +1,5 @@ use crate::policy::SandboxPolicy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use dunce::canonicalize; use std::collections::HashMap; use std::collections::HashSet; @@ -11,9 +12,19 @@ pub struct AllowDenyPaths { pub deny: HashSet, } -pub fn compute_allow_paths( +pub(crate) fn compute_allow_paths( policy: &SandboxPolicy, - policy_cwd: &Path, + _policy_cwd: &Path, + command_cwd: &Path, + env_map: &HashMap, +) -> AllowDenyPaths { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, command_cwd); + compute_allow_paths_for_permissions(&permissions, command_cwd, env_map) +} + +pub(crate) fn compute_allow_paths_for_permissions( + permissions: &ResolvedWindowsSandboxPermissions, command_cwd: &Path, env_map: &HashMap, ) -> AllowDenyPaths { @@ -30,65 +41,15 @@ pub fn compute_allow_paths( deny.insert(p); } }; - let include_tmp_env_vars = matches!( - policy, - SandboxPolicy::WorkspaceWrite { - exclude_tmpdir_env_var: false, - .. - } - ); - if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { - let add_writable_root = - |root: PathBuf, - policy_cwd: &Path, - add_allow: &mut dyn FnMut(PathBuf), - add_deny: &mut dyn FnMut(PathBuf)| { - let candidate = if root.is_absolute() { - root - } else { - policy_cwd.join(root) - }; - let canonical = canonicalize(&candidate).unwrap_or(candidate); - add_allow(canonical.clone()); - - for protected_subdir in [".git", ".codex", ".agents"] { - let protected_entry = canonical.join(protected_subdir); - if protected_entry.exists() { - add_deny(protected_entry); - } - } - }; - - add_writable_root( - command_cwd.to_path_buf(), - policy_cwd, - &mut add_allow_path, - &mut add_deny_path, - ); - - if let SandboxPolicy::WorkspaceWrite { writable_roots, .. } = policy { - for root in writable_roots { - add_writable_root( - root.clone().into(), - policy_cwd, - &mut add_allow_path, - &mut add_deny_path, - ); - } - } - } - if include_tmp_env_vars { - for key in ["TEMP", "TMP"] { - if let Some(v) = env_map.get(key) { - let abs = PathBuf::from(v); - add_allow_path(abs); - } else if let Ok(v) = std::env::var(key) { - let abs = PathBuf::from(v); - add_allow_path(abs); - } + for writable_root in permissions.writable_roots_for_cwd(command_cwd, env_map) { + let canonical = canonicalize(&writable_root.root).unwrap_or(writable_root.root); + add_allow_path(canonical); + for read_only_subpath in writable_root.read_only_subpaths { + add_deny_path(read_only_subpath); } } + AllowDenyPaths { allow, deny } } @@ -146,6 +107,7 @@ mod tests { }; let mut env_map = HashMap::new(); env_map.insert("TEMP".into(), temp_dir.to_string_lossy().to_string()); + env_map.insert("TMP".into(), temp_dir.to_string_lossy().to_string()); let paths = compute_allow_paths(&policy, &command_cwd, &command_cwd, &env_map); @@ -162,6 +124,59 @@ mod tests { assert!(paths.deny.is_empty(), "no deny paths expected"); } + #[test] + fn includes_tmp_env_vars_when_requested() { + let tmp = TempDir::new().expect("tempdir"); + let command_cwd = tmp.path().join("workspace"); + let temp_dir = tmp.path().join("temp"); + let _ = fs::create_dir_all(&command_cwd); + let _ = fs::create_dir_all(&temp_dir); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }; + let mut env_map = HashMap::new(); + env_map.insert("TEMP".into(), temp_dir.to_string_lossy().to_string()); + env_map.insert("TMP".into(), temp_dir.to_string_lossy().to_string()); + + let paths = compute_allow_paths(&policy, &command_cwd, &command_cwd, &env_map); + + let expected_allow: HashSet = [ + dunce::canonicalize(&command_cwd).unwrap(), + dunce::canonicalize(&temp_dir).unwrap(), + ] + .into_iter() + .collect(); + + assert_eq!(expected_allow, paths.allow); + assert!(paths.deny.is_empty(), "no deny paths expected"); + } + + #[test] + fn ignores_unix_slash_tmp_for_windows_allow_roots() { + let tmp = TempDir::new().expect("tempdir"); + let command_cwd = tmp.path().join("workspace"); + let _ = fs::create_dir_all(&command_cwd); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: false, + }; + + let paths = compute_allow_paths(&policy, &command_cwd, &command_cwd, &HashMap::new()); + let expected_allow: HashSet = [dunce::canonicalize(&command_cwd).unwrap()] + .into_iter() + .collect(); + + assert_eq!(expected_allow, paths.allow); + assert!(paths.deny.is_empty(), "no deny paths expected"); + } + #[test] fn denies_git_dir_inside_writable_root() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index da2a2d3634..5accd0be36 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -38,6 +38,8 @@ mod policy; #[cfg(target_os = "windows")] mod process; #[cfg(target_os = "windows")] +mod resolved_permissions; +#[cfg(target_os = "windows")] mod token; #[cfg(target_os = "windows")] mod wfp; diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs new file mode 100644 index 0000000000..6b2daa56e0 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -0,0 +1,116 @@ +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +/// Windows-local view of the runtime permission profile. +/// +/// Most Windows sandbox code needs resolved runtime permissions plus a few +/// Windows-specific path conventions, not the user/config-facing +/// `PermissionProfile` enum itself. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct ResolvedWindowsSandboxPermissions { + file_system: FileSystemSandboxPolicy, + network: NetworkSandboxPolicy, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct WindowsWritableRoot { + pub(crate) root: PathBuf, + pub(crate) read_only_subpaths: Vec, +} + +impl ResolvedWindowsSandboxPermissions { + pub(crate) fn from_legacy_policy(policy: &SandboxPolicy) -> Self { + Self { + file_system: FileSystemSandboxPolicy::from(policy), + network: NetworkSandboxPolicy::from(policy), + } + } + + pub(crate) fn from_legacy_policy_for_cwd(policy: &SandboxPolicy, cwd: &Path) -> Self { + Self { + file_system: FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(policy, cwd), + network: NetworkSandboxPolicy::from(policy), + } + } + + pub(crate) fn should_apply_network_block(&self) -> bool { + !self.network.is_enabled() + } + + pub(crate) fn writable_roots_for_cwd( + &self, + cwd: &Path, + env_map: &HashMap, + ) -> Vec { + let mut file_system = self.file_system.clone(); + file_system + .entries + .retain(|FileSystemSandboxEntry { path, .. }| { + !matches!( + path, + FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir + | codex_protocol::permissions::FileSystemSpecialPath::SlashTmp, + } + ) + }); + + let mut roots = file_system + .get_writable_roots_with_cwd(cwd) + .into_iter() + .map(|root| WindowsWritableRoot { + root: root.root.into_path_buf(), + read_only_subpaths: root + .read_only_subpaths + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(), + }) + .collect::>(); + + if self.has_writable_tmpdir_entry() { + roots.extend(windows_temp_env_roots(env_map).into_iter().map(|root| { + WindowsWritableRoot { + root, + read_only_subpaths: Vec::new(), + } + })); + } + + roots + } + + fn has_writable_tmpdir_entry(&self) -> bool { + self.file_system + .entries + .iter() + .any(|FileSystemSandboxEntry { path, access }| { + matches!( + path, + FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir, + } + ) && access.can_write() + }) + } +} + +fn windows_temp_env_roots(env_map: &HashMap) -> Vec { + ["TEMP", "TMP"] + .into_iter() + .filter_map(|key| { + env_map + .get(key) + .map(|value| PathBuf::from(value.as_str())) + .or_else(|| std::env::var_os(key).map(PathBuf::from)) + }) + .filter(|path| path.is_absolute()) + .collect() +} diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 0de2c6e029..6f4dd86019 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -395,14 +395,9 @@ pub(crate) fn gather_write_roots( command_cwd: &Path, env_map: &HashMap, ) -> Vec { - let mut roots: Vec = Vec::new(); - // Always include the command CWD for workspace-write. - if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { - roots.push(command_cwd.to_path_buf()); - } let AllowDenyPaths { allow, .. } = compute_allow_paths(policy, policy_cwd, command_cwd, env_map); - roots.extend(allow); + let roots: Vec = allow.into_iter().collect(); let mut dedup: HashSet = HashSet::new(); let mut out: Vec = Vec::new(); for r in canonical_existing(&roots) { diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index b27fa822fa..8df1ba804f 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -19,6 +19,7 @@ use crate::logging::log_start; use crate::path_normalization::canonicalize_path; use crate::policy::SandboxPolicy; use crate::policy::parse_policy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::sandbox_utils::ensure_codex_home_exists; use crate::sandbox_utils::inject_git_safe_directory; use crate::setup::effective_write_roots_for_setup; @@ -73,7 +74,7 @@ pub(crate) struct LegacyAclSids<'a> { } pub(crate) fn should_apply_network_block(policy: &SandboxPolicy) -> bool { - !policy.has_full_network_access() + ResolvedWindowsSandboxPermissions::from_legacy_policy(policy).should_apply_network_block() } fn prepare_spawn_context_common( From 6dcb3ba00e229b6f59c329b7ca9ae8c77df97326 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:01:58 -0700 Subject: [PATCH 3/7] windows-sandbox: send permission profiles to elevated runner --- .../src/bin/command_runner/win.rs | 20 +- .../src/elevated/ipc_framed.rs | 45 ++++- .../windows-sandbox-rs/src/elevated_impl.rs | 7 +- codex-rs/windows-sandbox-rs/src/lib.rs | 4 + .../src/resolved_permissions.rs | 182 ++++++++++++++++++ .../src/unified_exec/backends/elevated.rs | 9 +- 6 files changed, 252 insertions(+), 15 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs b/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs index b161dd57b8..49814789f6 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs @@ -22,11 +22,11 @@ use codex_windows_sandbox::OutputPayload; use codex_windows_sandbox::OutputStream; use codex_windows_sandbox::PipeSpawnHandles; use codex_windows_sandbox::ResizePayload; -use codex_windows_sandbox::SandboxPolicy; use codex_windows_sandbox::SpawnReady; use codex_windows_sandbox::SpawnRequest; use codex_windows_sandbox::StderrMode; use codex_windows_sandbox::StdinMode; +use codex_windows_sandbox::WindowsSandboxTokenMode; use codex_windows_sandbox::allow_null_device; use codex_windows_sandbox::create_readonly_token_with_caps_and_user_from; use codex_windows_sandbox::create_workspace_write_token_with_caps_and_user_from; @@ -35,11 +35,11 @@ use codex_windows_sandbox::encode_bytes; use codex_windows_sandbox::get_current_token_for_restriction; use codex_windows_sandbox::hide_current_user_profile_dir; use codex_windows_sandbox::log_note; -use codex_windows_sandbox::parse_policy; use codex_windows_sandbox::read_frame; use codex_windows_sandbox::read_handle_loop; use codex_windows_sandbox::spawn_process_with_pipes; use codex_windows_sandbox::to_wide; +use codex_windows_sandbox::token_mode_for_permission_profile; use codex_windows_sandbox::write_frame; use std::ffi::OsStr; use std::fs::File; @@ -235,7 +235,12 @@ fn effective_cwd(req_cwd: &Path, log_dir: Option<&Path>) -> PathBuf { fn spawn_ipc_process(req: &SpawnRequest) -> Result { let log_dir = req.codex_home.clone(); hide_current_user_profile_dir(req.codex_home.as_path()); - let policy = parse_policy(&req.policy_json_or_preset).context("parse policy_json_or_preset")?; + let token_mode = token_mode_for_permission_profile( + &req.permission_profile, + &req.permission_profile_cwd, + &req.env, + ) + .context("resolve permission profile token mode")?; let mut cap_psids: Vec = Vec::new(); for sid in &req.cap_sids { cap_psids.push( @@ -253,16 +258,13 @@ fn spawn_ipc_process(req: &SpawnRequest) -> Result { let cap_psid_ptrs: Vec<*mut _> = cap_psids.iter().map(LocalSid::as_ptr).collect(); let base = OwnedWinHandle::new(unsafe { get_current_token_for_restriction()? }); let h_token = OwnedWinHandle::new(unsafe { - match &policy { - SandboxPolicy::ReadOnly { .. } => { + match token_mode { + WindowsSandboxTokenMode::ReadOnlyCapability => { create_readonly_token_with_caps_and_user_from(base.raw(), &cap_psid_ptrs) } - SandboxPolicy::WorkspaceWrite { .. } => { + WindowsSandboxTokenMode::WriteCapabilityRoots => { create_workspace_write_token_with_caps_and_user_from(base.raw(), &cap_psid_ptrs) } - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - unreachable!() - } } }?); unsafe { diff --git a/codex-rs/windows-sandbox-rs/src/elevated/ipc_framed.rs b/codex-rs/windows-sandbox-rs/src/elevated/ipc_framed.rs index 56e33e57ca..0c98bf3bab 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/ipc_framed.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/ipc_framed.rs @@ -10,6 +10,7 @@ use anyhow::Result; use base64::Engine as _; use base64::engine::general_purpose::STANDARD; +use codex_protocol::models::PermissionProfile; use serde::Deserialize; use serde::Serialize; use std::collections::HashMap; @@ -55,8 +56,8 @@ pub struct SpawnRequest { pub command: Vec, pub cwd: PathBuf, pub env: HashMap, - pub policy_json_or_preset: String, - pub sandbox_policy_cwd: PathBuf, + pub permission_profile: PermissionProfile, + pub permission_profile_cwd: PathBuf, pub codex_home: PathBuf, pub real_codex_home: PathBuf, pub cap_sids: Vec, @@ -164,6 +165,7 @@ pub fn read_frame(mut reader: R) -> Result> { #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; #[test] fn framed_round_trip() { @@ -189,4 +191,43 @@ mod tests { other => panic!("unexpected message: {other:?}"), } } + + #[test] + fn spawn_request_serializes_permission_profile() { + let msg = FramedMessage { + version: 1, + message: Message::SpawnRequest { + payload: Box::new(SpawnRequest { + command: vec!["cmd.exe".to_string(), "/c".to_string(), "ver".to_string()], + cwd: PathBuf::from(r"C:\workspace"), + env: HashMap::new(), + permission_profile: PermissionProfile::read_only(), + permission_profile_cwd: PathBuf::from(r"C:\workspace"), + codex_home: PathBuf::from(r"C:\codex"), + real_codex_home: PathBuf::from(r"C:\Users\codex"), + cap_sids: vec!["S-1-15-3-1024-1".to_string()], + timeout_ms: Some(1000), + tty: false, + stdin_open: false, + use_private_desktop: false, + }), + }, + }; + + let encoded = serde_json::to_value(&msg).expect("serialize"); + assert_eq!("spawn_request", encoded["type"]); + assert_eq!("managed", encoded["payload"]["permission_profile"]["type"]); + assert_eq!(None, encoded["payload"].get("policy_json_or_preset")); + assert_eq!(None, encoded["payload"].get("sandbox_policy_cwd")); + + let decoded: FramedMessage = serde_json::from_value(encoded).expect("deserialize"); + let Message::SpawnRequest { payload } = decoded.message else { + panic!("unexpected message"); + }; + assert_eq!(PermissionProfile::read_only(), payload.permission_profile); + assert_eq!( + PathBuf::from(r"C:\workspace"), + payload.permission_profile_cwd + ); + } } diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index f43715c122..5861317376 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -45,6 +45,7 @@ mod windows_impl { use crate::setup::effective_write_roots_for_setup; use crate::token::LocalSid; use anyhow::Result; + use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use std::path::Path; @@ -144,12 +145,14 @@ mod windows_impl { } (|| -> Result { + let permission_profile = + PermissionProfile::from_legacy_sandbox_policy_for_cwd(&policy, sandbox_policy_cwd); let spawn_request = SpawnRequest { command: command.clone(), cwd: cwd.to_path_buf(), env: env_map.clone(), - policy_json_or_preset: policy_json_or_preset.to_string(), - sandbox_policy_cwd: sandbox_policy_cwd.to_path_buf(), + permission_profile, + permission_profile_cwd: sandbox_policy_cwd.to_path_buf(), codex_home: sandbox_base.clone(), real_codex_home: codex_home.to_path_buf(), cap_sids, diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 5accd0be36..481531cf8e 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -197,6 +197,10 @@ pub use process::read_handle_loop; #[cfg(target_os = "windows")] pub use process::spawn_process_with_pipes; #[cfg(target_os = "windows")] +pub use resolved_permissions::WindowsSandboxTokenMode; +#[cfg(target_os = "windows")] +pub use resolved_permissions::token_mode_for_permission_profile; +#[cfg(target_os = "windows")] pub use setup::SETUP_VERSION; #[cfg(target_os = "windows")] pub use setup::SandboxSetupRequest; diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs index 6b2daa56e0..cbf4e22949 100644 --- a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -1,5 +1,8 @@ +use anyhow::Result; +use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; @@ -25,6 +28,33 @@ pub(crate) struct WindowsWritableRoot { pub(crate) read_only_subpaths: Vec, } +/// Restricted-token family needed to enforce a Windows permission profile. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum WindowsSandboxTokenMode { + ReadOnlyCapability, + WriteCapabilityRoots, +} + +/// Chooses the restricted-token family needed for a managed permission profile. +pub fn token_mode_for_permission_profile( + permission_profile: &PermissionProfile, + cwd: &Path, + env_map: &HashMap, +) -> Result { + let permissions = + ResolvedWindowsSandboxPermissions::try_from_permission_profile(permission_profile)?; + if permissions.file_system.has_full_disk_write_access() { + anyhow::bail!( + "permission profile requests full-disk filesystem writes, which cannot be enforced by the Windows sandbox" + ); + } + if permissions.writable_roots_for_cwd(cwd, env_map).is_empty() { + Ok(WindowsSandboxTokenMode::ReadOnlyCapability) + } else { + Ok(WindowsSandboxTokenMode::WriteCapabilityRoots) + } +} + impl ResolvedWindowsSandboxPermissions { pub(crate) fn from_legacy_policy(policy: &SandboxPolicy) -> Self { Self { @@ -40,6 +70,26 @@ impl ResolvedWindowsSandboxPermissions { } } + pub(crate) fn try_from_permission_profile( + permission_profile: &PermissionProfile, + ) -> Result { + if !matches!(permission_profile, PermissionProfile::Managed { .. }) { + anyhow::bail!( + "only managed permission profiles can be enforced by the Windows sandbox" + ); + } + let (file_system, network) = permission_profile.to_runtime_permissions(); + if !matches!(file_system.kind, FileSystemSandboxKind::Restricted) { + anyhow::bail!( + "only restricted managed filesystem permissions can be enforced by the Windows sandbox" + ); + } + Ok(Self { + file_system, + network, + }) + } + pub(crate) fn should_apply_network_block(&self) -> bool { !self.network.is_enabled() } @@ -114,3 +164,135 @@ fn windows_temp_env_roots(env_map: &HashMap) -> Vec { .filter(|path| path.is_absolute()) .collect() } + +#[cfg(test)] +mod tests { + use super::*; + use codex_protocol::models::ManagedFileSystemPermissions; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSpecialPath; + use pretty_assertions::assert_eq; + use tempfile::TempDir; + + #[test] + fn permission_profile_workspace_write_uses_windows_temp_env_vars() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = tmp.path().join("workspace"); + let temp_dir = tmp.path().join("temp"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + std::fs::create_dir_all(&temp_dir).expect("create temp dir"); + + let mut env_map = HashMap::new(); + env_map.insert("TEMP".to_string(), temp_dir.to_string_lossy().to_string()); + env_map.insert("TMP".to_string(), temp_dir.to_string_lossy().to_string()); + + let permissions = ResolvedWindowsSandboxPermissions::try_from_permission_profile( + &PermissionProfile::workspace_write(), + ) + .expect("managed permission profile"); + let roots = permissions + .writable_roots_for_cwd(&cwd, &env_map) + .into_iter() + .map(|root| root.root) + .collect::>(); + + let expected_roots = [ + temp_dir, + dunce::canonicalize(&cwd).expect("canonicalize cwd"), + ] + .into_iter() + .collect::>(); + + assert_eq!(expected_roots, roots); + } + + #[test] + fn token_mode_for_profile_without_writable_roots_uses_readonly_capability() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = tmp.path().join("workspace"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + + let token_mode = token_mode_for_permission_profile( + &PermissionProfile::read_only(), + &cwd, + &HashMap::new(), + ) + .expect("token mode"); + + assert_eq!(WindowsSandboxTokenMode::ReadOnlyCapability, token_mode); + } + + #[test] + fn token_mode_for_profile_with_writable_roots_uses_write_capabilities() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = tmp.path().join("workspace"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + + let token_mode = token_mode_for_permission_profile( + &PermissionProfile::workspace_write(), + &cwd, + &HashMap::new(), + ) + .expect("token mode"); + + assert_eq!(WindowsSandboxTokenMode::WriteCapabilityRoots, token_mode); + } + + #[test] + fn permission_profile_rejects_disabled_profiles() { + let err = ResolvedWindowsSandboxPermissions::try_from_permission_profile( + &PermissionProfile::Disabled, + ) + .expect_err("disabled profile should not resolve for sandbox enforcement"); + + assert!( + err.to_string() + .contains("only managed permission profiles can be enforced") + ); + } + + #[test] + fn permission_profile_rejects_unrestricted_managed_filesystem() { + let permission_profile = PermissionProfile::Managed { + file_system: ManagedFileSystemPermissions::Unrestricted, + network: NetworkSandboxPolicy::Restricted, + }; + + let err = + ResolvedWindowsSandboxPermissions::try_from_permission_profile(&permission_profile) + .expect_err("unrestricted profile should not resolve for sandbox enforcement"); + + assert!( + err.to_string() + .contains("only restricted managed filesystem permissions can be enforced") + ); + } + + #[test] + fn token_mode_rejects_full_disk_write_entries() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = tmp.path().join("workspace"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + let permission_profile = PermissionProfile::Managed { + file_system: ManagedFileSystemPermissions::Restricted { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }], + glob_scan_max_depth: None, + }, + network: NetworkSandboxPolicy::Restricted, + }; + + let err = token_mode_for_permission_profile(&permission_profile, &cwd, &HashMap::new()) + .expect_err("full disk writes should not resolve to a token mode"); + + assert!( + err.to_string() + .contains("full-disk filesystem writes, which cannot be enforced") + ); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs index 0d7e94a1ff..5a756b6413 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs @@ -10,6 +10,7 @@ use crate::ipc_framed::SpawnRequest; use crate::runner_client::spawn_runner_transport; use crate::spawn_prep::prepare_elevated_spawn_context; use anyhow::Result; +use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; use codex_utils_pty::SpawnedProcess; @@ -60,12 +61,16 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( &deny_write_paths_override, )?; + let permission_profile = PermissionProfile::from_legacy_sandbox_policy_for_cwd( + &elevated.common.policy, + sandbox_policy_cwd, + ); let spawn_request = SpawnRequest { command: command.clone(), cwd: cwd.to_path_buf(), env: env_map.clone(), - policy_json_or_preset: policy_json_or_preset.to_string(), - sandbox_policy_cwd: sandbox_policy_cwd.to_path_buf(), + permission_profile, + permission_profile_cwd: sandbox_policy_cwd.to_path_buf(), codex_home: elevated.common.sandbox_base.clone(), real_codex_home: codex_home.to_path_buf(), cap_sids: elevated.cap_sids.clone(), From 11b6688a138df6d737b05816c0f763861f390092 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:06:53 -0700 Subject: [PATCH 4/7] windows-sandbox: drive write roots from resolved permissions --- codex-rs/windows-sandbox-rs/src/allow.rs | 33 ++- codex-rs/windows-sandbox-rs/src/audit.rs | 29 ++- .../windows-sandbox-rs/src/elevated_impl.rs | 51 ++-- codex-rs/windows-sandbox-rs/src/identity.rs | 9 +- codex-rs/windows-sandbox-rs/src/lib.rs | 46 +++- .../src/resolved_permissions.rs | 50 +++- codex-rs/windows-sandbox-rs/src/setup.rs | 84 ++++++- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 231 ++++++++++++------ .../src/unified_exec/backends/legacy.rs | 35 ++- 9 files changed, 420 insertions(+), 148 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/allow.rs b/codex-rs/windows-sandbox-rs/src/allow.rs index e82cf8cd62..b13081cdb5 100644 --- a/codex-rs/windows-sandbox-rs/src/allow.rs +++ b/codex-rs/windows-sandbox-rs/src/allow.rs @@ -14,12 +14,12 @@ pub struct AllowDenyPaths { pub(crate) fn compute_allow_paths( policy: &SandboxPolicy, - _policy_cwd: &Path, + policy_cwd: &Path, command_cwd: &Path, env_map: &HashMap, ) -> AllowDenyPaths { let permissions = - ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, command_cwd); + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); compute_allow_paths_for_permissions(&permissions, command_cwd, env_map) } @@ -91,6 +91,35 @@ mod tests { assert!(paths.deny.is_empty(), "no deny paths expected"); } + #[test] + fn uses_policy_cwd_for_legacy_workspace_root() { + let tmp = TempDir::new().expect("tempdir"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let paths = compute_allow_paths(&policy, &policy_cwd, &command_cwd, &HashMap::new()); + + assert!( + paths + .allow + .contains(&dunce::canonicalize(&policy_cwd).unwrap()) + ); + assert!( + !paths + .allow + .contains(&dunce::canonicalize(&command_cwd).unwrap()) + ); + assert!(paths.deny.is_empty(), "no deny paths expected"); + } + #[test] fn excludes_tmp_env_vars_when_requested() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index c7e7fc0b11..cab0a06adf 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -8,7 +8,8 @@ use crate::logging::debug_log; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; use crate::policy::SandboxPolicy; -use crate::setup::effective_write_roots_for_setup; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; +use crate::setup::effective_write_roots_for_permissions; use crate::token::LocalSid; use crate::token::world_sid; use anyhow::Result; @@ -259,11 +260,18 @@ pub fn apply_capability_denies_for_world_writable( let cap_path = cap_sid_file(codex_home); let caps = load_or_create_cap_sids(codex_home)?; std::fs::write(&cap_path, serde_json::to_string(&caps)?)?; - let (active_sids, workspace_roots): (Vec, Vec) = match sandbox_policy { - SandboxPolicy::WorkspaceWrite { .. } => { - let roots = effective_write_roots_for_setup( - sandbox_policy, - cwd, + if matches!( + sandbox_policy, + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } + ) { + return Ok(()); + } + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(sandbox_policy, cwd); + let (active_sids, workspace_roots): (Vec, Vec) = + if permissions.uses_write_capabilities_for_cwd(cwd, env_map) { + let roots = effective_write_roots_for_permissions( + &permissions, cwd, env_map, codex_home, @@ -277,14 +285,9 @@ pub fn apply_capability_denies_for_world_writable( }) .collect::>>()?; (active_sids, roots) - } - SandboxPolicy::ReadOnly { .. } => { + } else { (vec![LocalSid::from_string(&caps.readonly)?], Vec::new()) - } - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - return Ok(()); - } - }; + }; for path in flagged { if workspace_roots .iter() diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 5861317376..b012fd8af5 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -39,10 +39,11 @@ mod windows_impl { use crate::logging::log_success; use crate::policy::SandboxPolicy; use crate::policy::parse_policy; + use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::runner_client::spawn_runner_transport; use crate::sandbox_utils::ensure_codex_home_exists; use crate::sandbox_utils::inject_git_safe_directory; - use crate::setup::effective_write_roots_for_setup; + use crate::setup::effective_write_roots_for_permissions; use crate::token::LocalSid; use anyhow::Result; use codex_protocol::models::PermissionProfile; @@ -81,6 +82,10 @@ mod windows_impl { .map(AbsolutePathBuf::to_path_buf) .collect::>(); let policy = parse_policy(policy_json_or_preset)?; + let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + &policy, + sandbox_policy_cwd, + ); normalize_null_device_env(&mut env_map); ensure_non_interactive_pager(&mut env_map); inherit_path_env(&mut env_map); @@ -112,32 +117,26 @@ mod windows_impl { anyhow::bail!("DangerFullAccess and ExternalSandbox are not supported for sandboxing") } let caps = load_or_create_cap_sids(codex_home)?; - let (sid_for_null, cap_sids) = match &policy { - SandboxPolicy::ReadOnly { .. } => { - let sid = LocalSid::from_string(&caps.readonly)?; - (sid, vec![caps.readonly]) - } - SandboxPolicy::WorkspaceWrite { .. } => { - let write_roots = effective_write_roots_for_setup( - &policy, - sandbox_policy_cwd, - cwd, - &env_map, - codex_home, - write_roots_override, - ); - let cap_sids = write_roots - .iter() - .map(|root| workspace_write_cap_sid_for_root(codex_home, cwd, root)) - .collect::>>()?; - if cap_sids.is_empty() { - anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); - } - (LocalSid::from_string(&cap_sids[0])?, cap_sids) - } - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - unreachable!("DangerFullAccess handled above") + let (sid_for_null, cap_sids) = if permissions.uses_write_capabilities_for_cwd(cwd, &env_map) + { + let write_roots = effective_write_roots_for_permissions( + &permissions, + cwd, + &env_map, + codex_home, + write_roots_override, + ); + let cap_sids = write_roots + .iter() + .map(|root| workspace_write_cap_sid_for_root(codex_home, cwd, root)) + .collect::>>()?; + if cap_sids.is_empty() { + anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); } + (LocalSid::from_string(&cap_sids[0])?, cap_sids) + } else { + let sid = LocalSid::from_string(&caps.readonly)?; + (sid, vec![caps.readonly]) }; unsafe { diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index e49af3017b..30bf7ef493 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -1,12 +1,13 @@ use crate::dpapi; use crate::logging::debug_log; use crate::policy::SandboxPolicy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup::SandboxNetworkIdentity; use crate::setup::SandboxUserRecord; use crate::setup::SandboxUsersFile; use crate::setup::SetupMarker; use crate::setup::gather_read_roots; -use crate::setup::gather_write_roots; +use crate::setup::gather_write_roots_for_permissions; use crate::setup::offline_proxy_settings_from_env; use crate::setup::run_elevated_setup; use crate::setup::run_setup_refresh_with_overrides; @@ -143,14 +144,16 @@ pub fn require_logon_sandbox_creds( deny_write_paths_override: &[PathBuf], proxy_enforced: bool, ) -> Result { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); let sandbox_dir = crate::setup::sandbox_dir(codex_home); let needed_read = read_roots_override .map(<[PathBuf]>::to_vec) .unwrap_or_else(|| gather_read_roots(command_cwd, policy, codex_home)); let needed_write = write_roots_override .map(<[PathBuf]>::to_vec) - .unwrap_or_else(|| gather_write_roots(policy, policy_cwd, command_cwd, env_map)); - let network_identity = SandboxNetworkIdentity::from_policy(policy, proxy_enforced); + .unwrap_or_else(|| gather_write_roots_for_permissions(&permissions, command_cwd, env_map)); + let network_identity = SandboxNetworkIdentity::from_permissions(&permissions, proxy_enforced); let desired_offline_proxy_settings = offline_proxy_settings_from_env(env_map, network_identity); // NOTE: Do not add CODEX_HOME/.sandbox to `needed_write`; it must remain non-writable by the // restricted capability token. The setup helper's `lock_sandbox_dir` is responsible for diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 481531cf8e..a8926ad2a5 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -285,18 +285,21 @@ pub use stub::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] mod windows_impl { + use super::acl::revoke_ace; use super::logging::log_failure; use super::logging::log_success; use super::policy::SandboxPolicy; use super::process::create_process_as_user; use super::sandbox_utils::ensure_codex_home_exists; use super::spawn_prep::LegacyAclSids; + use super::spawn_prep::SpawnPrepOptions; use super::spawn_prep::allow_null_device_for_workspace_write; use super::spawn_prep::apply_legacy_session_acl_rules; use super::spawn_prep::legacy_session_capability_roots; use super::spawn_prep::prepare_legacy_session_security; use super::spawn_prep::prepare_legacy_spawn_context; use super::spawn_prep::root_capability_sids; + use super::token::LocalSid; use anyhow::Result; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; @@ -398,17 +401,20 @@ mod windows_impl { .collect::>(); let common = prepare_legacy_spawn_context( policy_json_or_preset, + sandbox_policy_cwd, codex_home, cwd, &mut env_map, &command, - /*inherit_path*/ false, - /*add_git_safe_directory*/ false, + SpawnPrepOptions { + inherit_path: false, + add_git_safe_directory: false, + }, )?; let policy = common.policy; let current_dir = common.current_dir; let logs_base_dir = common.logs_base_dir.as_deref(); - let is_workspace_write = common.is_workspace_write; + let uses_write_capabilities = common.uses_write_capabilities; if !policy.has_full_disk_read_access() { anyhow::bail!( "Restricted read-only access requires the elevated Windows sandbox backend" @@ -427,8 +433,9 @@ mod windows_impl { codex_home, ); let security = prepare_legacy_session_security(&policy, codex_home, cwd, capability_roots)?; - allow_null_device_for_workspace_write(is_workspace_write); - apply_legacy_session_acl_rules( + allow_null_device_for_workspace_write(uses_write_capabilities); + let persist_aces = uses_write_capabilities; + let guards = apply_legacy_session_acl_rules( &policy, sandbox_policy_cwd, codex_home, @@ -441,6 +448,7 @@ mod windows_impl { readonly_sid_str: security.readonly_sid_str.as_deref(), write_root_sids: &security.write_root_sids, }, + persist_aces, )?; let (stdin_pair, stdout_pair, stderr_pair) = unsafe { setup_stdio_pipes()? }; let ((in_r, in_w), (out_r, out_w), (err_r, err_w)) = (stdin_pair, stdout_pair, stderr_pair); @@ -465,6 +473,13 @@ mod windows_impl { CloseHandle(out_w); CloseHandle(err_r); CloseHandle(err_w); + if !persist_aces { + for (p, sid_str) in &guards { + if let Ok(sid) = LocalSid::from_string(sid_str) { + revoke_ace(p, sid.as_ptr()); + } + } + } CloseHandle(security.h_token); } return Err(err); @@ -565,6 +580,16 @@ mod windows_impl { log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } + if !persist_aces { + unsafe { + for (p, sid_str) in guards { + if let Ok(sid) = LocalSid::from_string(&sid_str) { + revoke_ace(&p, sid.as_ptr()); + } + } + } + } + Ok(CaptureResult { exit_code, stdout, @@ -595,7 +620,7 @@ mod windows_impl { codex_home, ); let write_root_sids = root_capability_sids(codex_home, cwd, capability_roots)?; - apply_legacy_session_acl_rules( + let _guards = apply_legacy_session_acl_rules( sandbox_policy, sandbox_policy_cwd, codex_home, @@ -608,6 +633,7 @@ mod windows_impl { readonly_sid_str: None, write_root_sids: &write_root_sids, }, + /*persist_aces*/ true, )?; Ok(()) @@ -616,7 +642,8 @@ mod windows_impl { #[cfg(test)] mod tests { use crate::policy::SandboxPolicy; - use crate::spawn_prep::should_apply_network_block; + use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; + use std::path::Path; fn workspace_policy(network_access: bool) -> SandboxPolicy { SandboxPolicy::WorkspaceWrite { @@ -627,6 +654,11 @@ mod windows_impl { } } + fn should_apply_network_block(policy: &SandboxPolicy) -> bool { + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, Path::new(".")) + .should_apply_network_block() + } + #[test] fn applies_network_block_when_access_is_disabled() { assert!(should_apply_network_block(&workspace_policy( diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs index cbf4e22949..2a1cc8d3eb 100644 --- a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -56,16 +56,10 @@ pub fn token_mode_for_permission_profile( } impl ResolvedWindowsSandboxPermissions { - pub(crate) fn from_legacy_policy(policy: &SandboxPolicy) -> Self { - Self { - file_system: FileSystemSandboxPolicy::from(policy), - network: NetworkSandboxPolicy::from(policy), - } - } - pub(crate) fn from_legacy_policy_for_cwd(policy: &SandboxPolicy, cwd: &Path) -> Self { Self { - file_system: FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(policy, cwd), + file_system: FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(policy, cwd) + .materialize_project_roots_with_cwd(cwd), network: NetworkSandboxPolicy::from(policy), } } @@ -94,6 +88,18 @@ impl ResolvedWindowsSandboxPermissions { !self.network.is_enabled() } + pub(crate) fn network_policy(&self) -> NetworkSandboxPolicy { + self.network + } + + pub(crate) fn uses_write_capabilities_for_cwd( + &self, + cwd: &Path, + env_map: &HashMap, + ) -> bool { + !self.writable_roots_for_cwd(cwd, env_map).is_empty() + } + pub(crate) fn writable_roots_for_cwd( &self, cwd: &Path, @@ -207,6 +213,34 @@ mod tests { assert_eq!(expected_roots, roots); } + #[test] + fn legacy_workspace_root_stays_bound_to_policy_cwd() { + let tmp = TempDir::new().expect("tempdir"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + std::fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(&policy, &policy_cwd); + + let roots = permissions + .writable_roots_for_cwd(&command_cwd, &HashMap::new()) + .into_iter() + .map(|root| root.root) + .collect::>(); + + assert_eq!( + roots, + vec![dunce::canonicalize(&policy_cwd).expect("canonical policy cwd")] + ); + } + #[test] fn token_mode_for_profile_without_writable_roots_uses_readonly_capability() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 6f4dd86019..353b9efeb5 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -17,6 +17,7 @@ use crate::logging::log_note; use crate::path_normalization::canonical_path_key; use crate::path_normalization::canonicalize_path; use crate::policy::SandboxPolicy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup_error::SetupErrorCode; use crate::setup_error::SetupFailure; use crate::setup_error::clear_setup_error_report; @@ -173,8 +174,12 @@ fn run_setup_refresh_inner( let (read_roots, write_roots) = build_payload_roots(&request, &overrides); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); + let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + request.policy, + request.policy_cwd, + ); let network_identity = - SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); + SandboxNetworkIdentity::from_permissions(&permissions, request.proxy_enforced); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); let payload = ElevationPayload { version: SETUP_VERSION, @@ -389,15 +394,16 @@ pub(crate) fn gather_read_roots( gather_legacy_full_read_roots(command_cwd, policy, codex_home) } -pub(crate) fn gather_write_roots( - policy: &SandboxPolicy, - policy_cwd: &Path, +pub(crate) fn gather_write_roots_for_permissions( + permissions: &ResolvedWindowsSandboxPermissions, command_cwd: &Path, env_map: &HashMap, ) -> Vec { - let AllowDenyPaths { allow, .. } = - compute_allow_paths(policy, policy_cwd, command_cwd, env_map); - let roots: Vec = allow.into_iter().collect(); + let roots = permissions + .writable_roots_for_cwd(command_cwd, env_map) + .into_iter() + .map(|root| root.root) + .collect::>(); let mut dedup: HashSet = HashSet::new(); let mut out: Vec = Vec::new(); for r in canonical_existing(&roots) { @@ -415,11 +421,29 @@ pub(crate) fn effective_write_roots_for_setup( env_map: &HashMap, codex_home: &Path, write_roots_override: Option<&[PathBuf]>, +) -> Vec { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); + effective_write_roots_for_permissions( + &permissions, + command_cwd, + env_map, + codex_home, + write_roots_override, + ) +} + +pub(crate) fn effective_write_roots_for_permissions( + permissions: &ResolvedWindowsSandboxPermissions, + command_cwd: &Path, + env_map: &HashMap, + codex_home: &Path, + write_roots_override: Option<&[PathBuf]>, ) -> Vec { let write_roots = if let Some(roots) = write_roots_override { canonical_existing(roots) } else { - gather_write_roots(policy, policy_cwd, command_cwd, env_map) + gather_write_roots_for_permissions(permissions, command_cwd, env_map) }; let write_roots = expand_user_profile_root(write_roots); let write_roots = filter_user_profile_root(write_roots); @@ -463,8 +487,11 @@ pub(crate) enum SandboxNetworkIdentity { } impl SandboxNetworkIdentity { - pub(crate) fn from_policy(policy: &SandboxPolicy, proxy_enforced: bool) -> Self { - if proxy_enforced || !policy.has_full_network_access() { + pub(crate) fn from_permissions( + permissions: &ResolvedWindowsSandboxPermissions, + proxy_enforced: bool, + ) -> Self { + if proxy_enforced || !permissions.network_policy().is_enabled() { Self::Offline } else { Self::Online @@ -744,8 +771,12 @@ pub fn run_elevated_setup( let (read_roots, write_roots) = build_payload_roots(&request, &overrides); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); + let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + request.policy, + request.policy_cwd, + ); let network_identity = - SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); + SandboxNetworkIdentity::from_permissions(&permissions, request.proxy_enforced); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); let payload = ElevationPayload { version: SETUP_VERSION, @@ -1473,6 +1504,37 @@ mod tests { assert!(!effective_write_roots.contains(&forbidden_sandbox)); } + #[test] + fn effective_write_roots_use_policy_cwd_for_legacy_workspace_root() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let effective_write_roots = super::effective_write_roots_for_setup( + &policy, + &policy_cwd, + &command_cwd, + &HashMap::new(), + &codex_home, + /*write_roots_override*/ None, + ); + + assert_eq!( + effective_write_roots, + vec![dunce::canonicalize(&policy_cwd).expect("canonical policy cwd")] + ); + } + #[test] fn payload_deny_write_paths_merge_explicit_and_protected_children() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 8df1ba804f..cf73ae3e7c 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -3,11 +3,13 @@ use crate::acl::add_deny_write_ace; use crate::acl::allow_null_device; use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths; +use crate::allow::compute_allow_paths_for_permissions; use crate::cap::load_or_create_cap_sids; use crate::cap::workspace_write_cap_sid_for_root; use crate::cap::workspace_write_root_contains_path; use crate::cap::workspace_write_root_overlaps_path; use crate::cap::workspace_write_root_specificity; +use crate::deny_read_acl::apply_deny_read_acls; use crate::deny_read_state::sync_persistent_deny_read_acls; use crate::env::apply_no_network_to_env; use crate::env::ensure_non_interactive_pager; @@ -22,7 +24,7 @@ use crate::policy::parse_policy; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::sandbox_utils::ensure_codex_home_exists; use crate::sandbox_utils::inject_git_safe_directory; -use crate::setup::effective_write_roots_for_setup; +use crate::setup::effective_write_roots_for_permissions; use crate::token::LocalSid; use crate::token::create_readonly_token_with_cap; use crate::token::create_workspace_write_token_with_caps_from; @@ -42,10 +44,11 @@ use windows_sys::Win32::Foundation::HANDLE; pub(crate) struct SpawnContext { pub(crate) policy: SandboxPolicy, + pub(crate) permissions: ResolvedWindowsSandboxPermissions, pub(crate) current_dir: PathBuf, pub(crate) sandbox_base: PathBuf, pub(crate) logs_base_dir: Option, - pub(crate) is_workspace_write: bool, + pub(crate) uses_write_capabilities: bool, } pub(crate) struct ElevatedSpawnContext { @@ -54,6 +57,12 @@ pub(crate) struct ElevatedSpawnContext { pub(crate) cap_sids: Vec, } +#[derive(Debug, Clone, Copy)] +pub(crate) struct SpawnPrepOptions { + pub(crate) inherit_path: bool, + pub(crate) add_git_safe_directory: bool, +} + pub(crate) struct LegacySessionSecurity { pub(crate) h_token: HANDLE, pub(crate) readonly_sid: Option, @@ -73,18 +82,14 @@ pub(crate) struct LegacyAclSids<'a> { pub(crate) write_root_sids: &'a [RootCapabilitySid], } -pub(crate) fn should_apply_network_block(policy: &SandboxPolicy) -> bool { - ResolvedWindowsSandboxPermissions::from_legacy_policy(policy).should_apply_network_block() -} - fn prepare_spawn_context_common( policy_json_or_preset: &str, + policy_cwd: &Path, codex_home: &Path, cwd: &Path, env_map: &mut HashMap, command: &[String], - inherit_path: bool, - add_git_safe_directory: bool, + options: SpawnPrepOptions, ) -> Result { let policy = parse_policy(policy_json_or_preset)?; if matches!( @@ -96,10 +101,10 @@ fn prepare_spawn_context_common( normalize_null_device_env(env_map); ensure_non_interactive_pager(env_map); - if inherit_path { + if options.inherit_path { inherit_path_env(env_map); } - if add_git_safe_directory { + if options.add_git_safe_directory { inject_git_safe_directory(env_map, cwd); } @@ -109,36 +114,39 @@ fn prepare_spawn_context_common( let logs_base_dir = Some(sandbox_base.clone()); log_start(command, logs_base_dir.as_deref()); - let is_workspace_write = matches!(&policy, SandboxPolicy::WorkspaceWrite { .. }); + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(&policy, policy_cwd); + let uses_write_capabilities = permissions.uses_write_capabilities_for_cwd(cwd, env_map); Ok(SpawnContext { policy, + permissions, current_dir: cwd.to_path_buf(), sandbox_base, logs_base_dir, - is_workspace_write, + uses_write_capabilities, }) } pub(crate) fn prepare_legacy_spawn_context( policy_json_or_preset: &str, + policy_cwd: &Path, codex_home: &Path, cwd: &Path, env_map: &mut HashMap, command: &[String], - inherit_path: bool, - add_git_safe_directory: bool, + options: SpawnPrepOptions, ) -> Result { let common = prepare_spawn_context_common( policy_json_or_preset, + policy_cwd, codex_home, cwd, env_map, command, - inherit_path, - add_git_safe_directory, + options, )?; - if should_apply_network_block(&common.policy) { + if common.permissions.should_apply_network_block() { apply_no_network_to_env(env_map)?; } Ok(common) @@ -195,14 +203,15 @@ pub(crate) fn legacy_session_capability_roots( env_map: &HashMap, codex_home: &Path, ) -> Vec { - let allow_paths = compute_allow_paths(policy, policy_cwd, current_dir, env_map) + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); + let allow_paths = compute_allow_paths_for_permissions(&permissions, current_dir, env_map) .allow .into_iter() .collect::>(); - if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { - effective_write_roots_for_setup( - policy, - policy_cwd, + if permissions.uses_write_capabilities_for_cwd(current_dir, env_map) { + effective_write_roots_for_permissions( + &permissions, current_dir, env_map, codex_home, @@ -283,9 +292,11 @@ pub(crate) fn apply_legacy_session_acl_rules( additional_deny_read_paths: &[PathBuf], additional_deny_write_paths: &[PathBuf], acl_sids: LegacyAclSids<'_>, -) -> Result<()> { + persist_aces: bool, +) -> Result> { let AllowDenyPaths { allow, mut deny } = compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map); + let mut guards: Vec<(PathBuf, String)> = Vec::new(); unsafe { for path in additional_deny_write_paths { // Explicit carveouts must exist before the command starts so the @@ -298,19 +309,31 @@ pub(crate) fn apply_legacy_session_acl_rules( } if let Some(readonly_sid) = acl_sids.readonly_sid { for p in &allow { - let _ = add_allow_ace(p, readonly_sid.as_ptr()); + if matches!(add_allow_ace(p, readonly_sid.as_ptr()), Ok(true)) + && !persist_aces + && let Some(readonly_sid_str) = acl_sids.readonly_sid_str + { + guards.push((p.clone(), readonly_sid_str.to_string())); + } } } else { for p in &allow { let Some(root_sid) = matching_root_capability(p, acl_sids.write_root_sids) else { continue; }; - let _ = add_allow_ace(p, root_sid.sid.as_ptr()); + if matches!(add_allow_ace(p, root_sid.sid.as_ptr()), Ok(true)) && !persist_aces { + guards.push((p.clone(), root_sid.sid_str.clone())); + } } } for p in &deny { for root_sid in deny_root_capabilities_for_path(p, acl_sids.write_root_sids) { - let _ = add_deny_write_ace(p, root_sid.sid.as_ptr()); + if let Ok(added) = add_deny_write_ace(p, root_sid.sid.as_ptr()) + && added + && !persist_aces + { + guards.push((p.clone(), root_sid.sid_str.clone())); + } } } if !additional_deny_read_paths.is_empty() { @@ -318,20 +341,42 @@ pub(crate) fn apply_legacy_session_acl_rules( let Some(readonly_sid_str) = acl_sids.readonly_sid_str else { anyhow::bail!("readonly capability SID string missing"); }; - sync_persistent_deny_read_acls( - codex_home, - readonly_sid_str, - additional_deny_read_paths, - readonly_sid.as_ptr(), - )?; - } else { - for root_sid in acl_sids.write_root_sids { + let applied_deny_read_paths = if persist_aces { sync_persistent_deny_read_acls( codex_home, - &root_sid.sid_str, + readonly_sid_str, additional_deny_read_paths, - root_sid.sid.as_ptr(), - )?; + readonly_sid.as_ptr(), + )? + } else { + apply_deny_read_acls(additional_deny_read_paths, readonly_sid.as_ptr())? + }; + if !persist_aces { + guards.extend( + applied_deny_read_paths + .into_iter() + .map(|path| (path, readonly_sid_str.to_string())), + ); + } + } else { + for root_sid in acl_sids.write_root_sids { + let applied_deny_read_paths = if persist_aces { + sync_persistent_deny_read_acls( + codex_home, + &root_sid.sid_str, + additional_deny_read_paths, + root_sid.sid.as_ptr(), + )? + } else { + apply_deny_read_acls(additional_deny_read_paths, root_sid.sid.as_ptr())? + }; + if !persist_aces { + guards.extend( + applied_deny_read_paths + .into_iter() + .map(|path| (path, root_sid.sid_str.clone())), + ); + } } } } @@ -341,7 +386,8 @@ pub(crate) fn apply_legacy_session_acl_rules( if let Some(readonly_sid) = acl_sids.readonly_sid { allow_null_device(readonly_sid.as_ptr()); } - if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) + if persist_aces + && !acl_sids.write_root_sids.is_empty() && let Some(workspace_sid) = matching_root_capability(current_dir, acl_sids.write_root_sids) { @@ -352,7 +398,7 @@ pub(crate) fn apply_legacy_session_acl_rules( } } } - Ok(()) + Ok(guards) } #[allow(clippy::too_many_arguments)] @@ -371,32 +417,30 @@ pub(crate) fn prepare_elevated_spawn_context( ) -> Result { let common = prepare_spawn_context_common( policy_json_or_preset, + sandbox_policy_cwd, codex_home, cwd, env_map, command, - /*inherit_path*/ true, - /*add_git_safe_directory*/ true, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: true, + }, )?; - let AllowDenyPaths { allow, deny } = compute_allow_paths( - &common.policy, - sandbox_policy_cwd, - &common.current_dir, - env_map, - ); + let AllowDenyPaths { allow, deny } = + compute_allow_paths_for_permissions(&common.permissions, &common.current_dir, env_map); let write_roots: Vec = allow.into_iter().collect(); let deny_write_paths: Vec = deny.into_iter().collect(); - let computed_write_roots_override = if common.is_workspace_write { + let computed_write_roots_override = if common.uses_write_capabilities { Some(write_roots.as_slice()) } else { None }; let write_roots_for_setup = write_roots_override.or(computed_write_roots_override); - let effective_write_roots = if common.is_workspace_write { - effective_write_roots_for_setup( - &common.policy, - sandbox_policy_cwd, + let effective_write_roots = if common.uses_write_capabilities { + effective_write_roots_for_permissions( + &common.permissions, &common.current_dir, env_map, codex_home, @@ -405,7 +449,7 @@ pub(crate) fn prepare_elevated_spawn_context( } else { Vec::new() }; - let setup_write_roots_override = if common.is_workspace_write { + let setup_write_roots_override = if common.uses_write_capabilities { Some(effective_write_roots.as_slice()) } else { write_roots_override @@ -428,24 +472,20 @@ pub(crate) fn prepare_elevated_spawn_context( /*proxy_enforced*/ false, )?; let caps = load_or_create_cap_sids(codex_home)?; - let (psid_to_use, cap_sids) = match &common.policy { - SandboxPolicy::ReadOnly { .. } => ( + let (psid_to_use, cap_sids) = if common.uses_write_capabilities { + let cap_sids = root_capability_sids(codex_home, cwd, effective_write_roots)? + .into_iter() + .map(|root_sid| root_sid.sid_str) + .collect::>(); + if cap_sids.is_empty() { + anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); + } + (LocalSid::from_string(&cap_sids[0])?, cap_sids) + } else { + ( LocalSid::from_string(&caps.readonly)?, vec![caps.readonly.clone()], - ), - SandboxPolicy::WorkspaceWrite { .. } => { - let cap_sids = root_capability_sids(codex_home, cwd, effective_write_roots)? - .into_iter() - .map(|root_sid| root_sid.sid_str) - .collect::>(); - if cap_sids.is_empty() { - anyhow::bail!("workspace-write sandbox has no writable root capability SIDs"); - } - (LocalSid::from_string(&cap_sids[0])?, cap_sids) - } - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { - unreachable!("dangerous policies rejected before elevated session prep") - } + ) }; unsafe { @@ -462,19 +502,26 @@ pub(crate) fn prepare_elevated_spawn_context( #[cfg(test)] mod tests { use super::SandboxPolicy; + use super::SpawnPrepOptions; use super::deny_root_capabilities_for_path; use super::legacy_session_capability_roots; use super::prepare_legacy_spawn_context; use super::prepare_spawn_context_common; use super::root_capability_sids; - use super::should_apply_network_block; use crate::cap::load_or_create_cap_sids; use crate::cap::workspace_write_cap_sid_for_root; + use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; + use std::path::Path; use tempfile::TempDir; + fn should_apply_network_block(policy: &SandboxPolicy) -> bool { + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, Path::new(".")) + .should_apply_network_block() + } + #[test] fn no_network_env_rewrite_applies_for_workspace_write() { assert!(should_apply_network_block( @@ -502,12 +549,15 @@ mod tests { let _context = prepare_legacy_spawn_context( "workspace-write", + cwd.path(), codex_home.path(), cwd.path(), &mut env_map, &["cmd.exe".to_string()], - /*inherit_path*/ true, - /*add_git_safe_directory*/ false, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: false, + }, ) .expect("legacy env prep"); @@ -529,12 +579,15 @@ mod tests { let context = prepare_spawn_context_common( "workspace-write", + cwd.path(), codex_home.path(), cwd.path(), &mut env_map, &["cmd.exe".to_string()], - /*inherit_path*/ true, - /*add_git_safe_directory*/ true, + SpawnPrepOptions { + inherit_path: true, + add_git_safe_directory: true, + }, ) .expect("preserve existing env prep"); assert_eq!(context.policy, SandboxPolicy::new_workspace_write_policy()); @@ -546,6 +599,36 @@ mod tests { ); } + #[test] + fn legacy_session_capability_roots_use_policy_cwd_for_workspace_root() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + let policy_cwd = tmp.path().join("workspace"); + let command_cwd = policy_cwd.join("subdir"); + std::fs::create_dir_all(&codex_home).expect("create codex home"); + std::fs::create_dir_all(&command_cwd).expect("create command cwd"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let roots = legacy_session_capability_roots( + &policy, + &policy_cwd, + &command_cwd, + &HashMap::new(), + &codex_home, + ); + + assert_eq!( + roots, + vec![dunce::canonicalize(&policy_cwd).expect("canonical policy cwd")] + ); + } + #[test] fn root_capability_sids_only_include_active_roots() { let temp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs index 2cb12bfd71..64ec2ced6e 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs @@ -1,5 +1,6 @@ use super::windows_common::finish_driver_spawn; use super::windows_common::normalize_windows_tty_input; +use crate::acl::revoke_ace; use crate::conpty::ConptyInstance; use crate::conpty::spawn_conpty_process_as_user; use crate::desktop::LaunchDesktop; @@ -10,11 +11,13 @@ use crate::process::StdinMode; use crate::process::read_handle_loop; use crate::process::spawn_process_with_pipes; use crate::spawn_prep::LegacyAclSids; +use crate::spawn_prep::SpawnPrepOptions; use crate::spawn_prep::allow_null_device_for_workspace_write; use crate::spawn_prep::apply_legacy_session_acl_rules; use crate::spawn_prep::legacy_session_capability_roots; use crate::spawn_prep::prepare_legacy_session_security; use crate::spawn_prep::prepare_legacy_spawn_context; +use crate::token::LocalSid; use anyhow::Result; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::ProcessDriver; @@ -22,6 +25,7 @@ use codex_utils_pty::SpawnedProcess; use codex_utils_pty::TerminalSize; use std::collections::HashMap; use std::path::Path; +use std::path::PathBuf; use std::ptr; use std::sync::Arc; use std::sync::Mutex as StdMutex; @@ -203,6 +207,7 @@ fn finalize_exit( process_handle: Arc>>, thread_handle: HANDLE, output_join: std::thread::JoinHandle<()>, + guards: Vec<(PathBuf, String)>, logs_base_dir: Option<&Path>, command: Vec, ) { @@ -238,6 +243,14 @@ fn finalize_exit( } else { log_failure(&command, &format!("exit code {exit_code}"), logs_base_dir); } + + unsafe { + for (path, cap_sid) in guards { + if let Ok(sid) = LocalSid::from_string(&cap_sid) { + revoke_ace(&path, sid.as_ptr()); + } + } + } } fn resize_conpty_handle(hpc: &Arc>>, size: TerminalSize) -> Result<()> { @@ -283,12 +296,15 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( ) -> Result { let common = prepare_legacy_spawn_context( policy_json_or_preset, + sandbox_policy_cwd, codex_home, cwd, &mut env_map, &command, - /*inherit_path*/ false, - /*add_git_safe_directory*/ false, + SpawnPrepOptions { + inherit_path: false, + add_git_safe_directory: false, + }, )?; if !common.policy.has_full_disk_read_access() { anyhow::bail!("Restricted read-only access requires the elevated Windows sandbox backend"); @@ -311,9 +327,10 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( ); let security = prepare_legacy_session_security(&common.policy, codex_home, cwd, capability_roots)?; - allow_null_device_for_workspace_write(common.is_workspace_write); + allow_null_device_for_workspace_write(common.uses_write_capabilities); - apply_legacy_session_acl_rules( + let persist_aces = common.uses_write_capabilities; + let guards = apply_legacy_session_acl_rules( &common.policy, sandbox_policy_cwd, codex_home, @@ -326,6 +343,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( readonly_sid_str: security.readonly_sid_str.as_deref(), write_root_sids: &security.write_root_sids, }, + persist_aces, )?; let (writer_tx, writer_rx) = mpsc::channel::>(128); @@ -361,6 +379,13 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( Ok(handles) => handles, Err(err) => { unsafe { + if !persist_aces { + for (path, cap_sid) in &guards { + if let Ok(sid) = LocalSid::from_string(cap_sid) { + revoke_ace(path, sid.as_ptr()); + } + } + } CloseHandle(security.h_token); } return Err(err); @@ -371,6 +396,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let process_handle = Arc::new(StdMutex::new(Some(pi.hProcess))); let wait_handle = Arc::clone(&process_handle); let command_for_wait = command.clone(); + let guards_for_wait = if persist_aces { Vec::new() } else { guards }; let hpc_for_wait = hpc_handle.clone(); std::thread::spawn(move || { let _desktop = desktop; @@ -401,6 +427,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( wait_handle, pi.hThread, output_join, + guards_for_wait, common.logs_base_dir.as_deref(), command_for_wait, ); From 26dc133ae85ec533aee6a5ee011c758b634affb2 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:06:53 -0700 Subject: [PATCH 5/7] windows-sandbox: feed setup from resolved permissions --- codex-rs/core/src/windows_sandbox.rs | 7 +- codex-rs/core/tests/suite/windows_sandbox.rs | 17 +- codex-rs/windows-sandbox-rs/src/allow.rs | 23 ++- codex-rs/windows-sandbox-rs/src/audit.rs | 35 ++-- .../windows-sandbox-rs/src/elevated_impl.rs | 3 +- codex-rs/windows-sandbox-rs/src/identity.rs | 18 +- codex-rs/windows-sandbox-rs/src/lib.rs | 15 +- .../src/resolved_permissions.rs | 28 ++- codex-rs/windows-sandbox-rs/src/setup.rs | 160 +++++++++++------- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 9 +- .../src/unified_exec/backends/legacy.rs | 3 +- 11 files changed, 201 insertions(+), 117 deletions(-) diff --git a/codex-rs/core/src/windows_sandbox.rs b/codex-rs/core/src/windows_sandbox.rs index a094545bab..2c87c885ad 100644 --- a/codex-rs/core/src/windows_sandbox.rs +++ b/codex-rs/core/src/windows_sandbox.rs @@ -179,10 +179,13 @@ pub fn run_elevated_setup( env_map: &HashMap, codex_home: &Path, ) -> anyhow::Result<()> { + let permissions = + codex_windows_sandbox::ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + policy, policy_cwd, + ); codex_windows_sandbox::run_elevated_setup( codex_windows_sandbox::SandboxSetupRequest { - policy, - policy_cwd, + permissions: &permissions, command_cwd, env_map, codex_home, diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs index 189248be3a..0be8be3bbb 100644 --- a/codex-rs/core/tests/suite/windows_sandbox.rs +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -56,7 +56,22 @@ fn stage_windows_sandbox_helpers() -> anyhow::Result<()> { for helper_name in ["codex-windows-sandbox-setup", "codex-command-runner"] { let helper = codex_utils_cargo_bin::cargo_bin(helper_name)?; let file_name = Path::new(helper_name).with_extension("exe"); - std::fs::copy(helper, resources_dir.join(file_name))?; + let destination = resources_dir.join(file_name); + if let Err(err) = std::fs::copy(&helper, &destination) { + // A sandbox helper can briefly remain alive after the sandboxed + // command exits. Bazel may retry the test while that process still + // has the staged executable open, so keep the already-staged copy. + if err.kind() == std::io::ErrorKind::PermissionDenied && destination.exists() { + continue; + } + return Err(err).with_context(|| { + format!( + "stage Windows sandbox helper {} at {}", + helper.display(), + destination.display() + ) + }); + } } Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/allow.rs b/codex-rs/windows-sandbox-rs/src/allow.rs index b13081cdb5..541a86fc1c 100644 --- a/codex-rs/windows-sandbox-rs/src/allow.rs +++ b/codex-rs/windows-sandbox-rs/src/allow.rs @@ -1,4 +1,3 @@ -use crate::policy::SandboxPolicy; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use dunce::canonicalize; use std::collections::HashMap; @@ -12,17 +11,6 @@ pub struct AllowDenyPaths { pub deny: HashSet, } -pub(crate) fn compute_allow_paths( - policy: &SandboxPolicy, - policy_cwd: &Path, - command_cwd: &Path, - env_map: &HashMap, -) -> AllowDenyPaths { - let permissions = - ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); - compute_allow_paths_for_permissions(&permissions, command_cwd, env_map) -} - pub(crate) fn compute_allow_paths_for_permissions( permissions: &ResolvedWindowsSandboxPermissions, command_cwd: &Path, @@ -61,6 +49,17 @@ mod tests { use std::fs; use tempfile::TempDir; + fn compute_allow_paths( + policy: &SandboxPolicy, + policy_cwd: &Path, + command_cwd: &Path, + env_map: &HashMap, + ) -> AllowDenyPaths { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); + compute_allow_paths_for_permissions(&permissions, command_cwd, env_map) + } + #[test] fn includes_additional_writable_roots() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index cab0a06adf..0d6f4b5744 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -224,15 +224,33 @@ pub fn apply_world_writable_scan_and_denies( env_map: &std::collections::HashMap, sandbox_policy: &SandboxPolicy, logs_base_dir: Option<&Path>, +) -> Result<()> { + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(sandbox_policy, cwd); + apply_world_writable_scan_and_denies_for_permissions( + codex_home, + cwd, + env_map, + &permissions, + logs_base_dir, + ) +} + +pub fn apply_world_writable_scan_and_denies_for_permissions( + codex_home: &Path, + cwd: &Path, + env_map: &std::collections::HashMap, + permissions: &ResolvedWindowsSandboxPermissions, + logs_base_dir: Option<&Path>, ) -> Result<()> { let flagged = audit_everyone_writable(cwd, env_map, logs_base_dir)?; if flagged.is_empty() { return Ok(()); } - if let Err(err) = apply_capability_denies_for_world_writable( + if let Err(err) = apply_capability_denies_for_world_writable_for_permissions( codex_home, &flagged, - sandbox_policy, + permissions, cwd, env_map, logs_base_dir, @@ -245,10 +263,10 @@ pub fn apply_world_writable_scan_and_denies( Ok(()) } -pub fn apply_capability_denies_for_world_writable( +fn apply_capability_denies_for_world_writable_for_permissions( codex_home: &Path, flagged: &[PathBuf], - sandbox_policy: &SandboxPolicy, + permissions: &ResolvedWindowsSandboxPermissions, cwd: &Path, env_map: &std::collections::HashMap, logs_base_dir: Option<&Path>, @@ -260,18 +278,13 @@ pub fn apply_capability_denies_for_world_writable( let cap_path = cap_sid_file(codex_home); let caps = load_or_create_cap_sids(codex_home)?; std::fs::write(&cap_path, serde_json::to_string(&caps)?)?; - if matches!( - sandbox_policy, - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } - ) { + if !permissions.is_enforceable_by_windows_sandbox() { return Ok(()); } - let permissions = - ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(sandbox_policy, cwd); let (active_sids, workspace_roots): (Vec, Vec) = if permissions.uses_write_capabilities_for_cwd(cwd, env_map) { let roots = effective_write_roots_for_permissions( - &permissions, + permissions, cwd, env_map, codex_home, diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index b012fd8af5..dd343a22ce 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -97,8 +97,7 @@ mod windows_impl { let logs_base_dir: Option<&Path> = Some(sandbox_base.as_path()); log_start(&command, logs_base_dir); let sandbox_creds = require_logon_sandbox_creds( - &policy, - sandbox_policy_cwd, + &permissions, cwd, &env_map, codex_home, diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 30bf7ef493..37238190d0 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -1,6 +1,5 @@ use crate::dpapi; use crate::logging::debug_log; -use crate::policy::SandboxPolicy; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup::SandboxNetworkIdentity; use crate::setup::SandboxUserRecord; @@ -132,8 +131,7 @@ fn select_identity( #[allow(clippy::too_many_arguments)] pub fn require_logon_sandbox_creds( - policy: &SandboxPolicy, - policy_cwd: &Path, + permissions: &ResolvedWindowsSandboxPermissions, command_cwd: &Path, env_map: &HashMap, codex_home: &Path, @@ -144,16 +142,14 @@ pub fn require_logon_sandbox_creds( deny_write_paths_override: &[PathBuf], proxy_enforced: bool, ) -> Result { - let permissions = - ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); let sandbox_dir = crate::setup::sandbox_dir(codex_home); let needed_read = read_roots_override .map(<[PathBuf]>::to_vec) - .unwrap_or_else(|| gather_read_roots(command_cwd, policy, codex_home)); + .unwrap_or_else(|| gather_read_roots(command_cwd, permissions, env_map, codex_home)); let needed_write = write_roots_override .map(<[PathBuf]>::to_vec) - .unwrap_or_else(|| gather_write_roots_for_permissions(&permissions, command_cwd, env_map)); - let network_identity = SandboxNetworkIdentity::from_permissions(&permissions, proxy_enforced); + .unwrap_or_else(|| gather_write_roots_for_permissions(permissions, command_cwd, env_map)); + let network_identity = SandboxNetworkIdentity::from_permissions(permissions, proxy_enforced); let desired_offline_proxy_settings = offline_proxy_settings_from_env(env_map, network_identity); // NOTE: Do not add CODEX_HOME/.sandbox to `needed_write`; it must remain non-writable by the // restricted capability token. The setup helper's `lock_sandbox_dir` is responsible for @@ -194,8 +190,7 @@ pub fn require_logon_sandbox_creds( } run_elevated_setup( crate::setup::SandboxSetupRequest { - policy, - policy_cwd, + permissions, command_cwd, env_map, codex_home, @@ -214,8 +209,7 @@ pub fn require_logon_sandbox_creds( // Always refresh ACLs (non-elevated) for current roots via the setup binary. run_setup_refresh_with_overrides( crate::setup::SandboxSetupRequest { - policy, - policy_cwd, + permissions, command_cwd, env_map, codex_home, diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index a8926ad2a5..a363652476 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -108,6 +108,8 @@ pub use acl::path_mask_allows; #[cfg(target_os = "windows")] pub use audit::apply_world_writable_scan_and_denies; #[cfg(target_os = "windows")] +pub use audit::apply_world_writable_scan_and_denies_for_permissions; +#[cfg(target_os = "windows")] pub use cap::load_or_create_cap_sids; #[cfg(target_os = "windows")] pub use cap::workspace_cap_sid_for_cwd; @@ -197,6 +199,8 @@ pub use process::read_handle_loop; #[cfg(target_os = "windows")] pub use process::spawn_process_with_pipes; #[cfg(target_os = "windows")] +pub use resolved_permissions::ResolvedWindowsSandboxPermissions; +#[cfg(target_os = "windows")] pub use resolved_permissions::WindowsSandboxTokenMode; #[cfg(target_os = "windows")] pub use resolved_permissions::token_mode_for_permission_profile; @@ -290,6 +294,7 @@ mod windows_impl { use super::logging::log_success; use super::policy::SandboxPolicy; use super::process::create_process_as_user; + use super::resolved_permissions::ResolvedWindowsSandboxPermissions; use super::sandbox_utils::ensure_codex_home_exists; use super::spawn_prep::LegacyAclSids; use super::spawn_prep::SpawnPrepOptions; @@ -412,6 +417,7 @@ mod windows_impl { }, )?; let policy = common.policy; + let permissions = common.permissions; let current_dir = common.current_dir; let logs_base_dir = common.logs_base_dir.as_deref(); let uses_write_capabilities = common.uses_write_capabilities; @@ -436,8 +442,7 @@ mod windows_impl { allow_null_device_for_workspace_write(uses_write_capabilities); let persist_aces = uses_write_capabilities; let guards = apply_legacy_session_acl_rules( - &policy, - sandbox_policy_cwd, + &permissions, codex_home, ¤t_dir, &env_map, @@ -621,8 +626,10 @@ mod windows_impl { ); let write_root_sids = root_capability_sids(codex_home, cwd, capability_roots)?; let _guards = apply_legacy_session_acl_rules( - sandbox_policy, - sandbox_policy_cwd, + &ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( + sandbox_policy, + sandbox_policy_cwd, + ), codex_home, ¤t_dir, env_map, diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs index 2a1cc8d3eb..496f4d1108 100644 --- a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -17,7 +17,7 @@ use std::path::PathBuf; /// Windows-specific path conventions, not the user/config-facing /// `PermissionProfile` enum itself. #[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct ResolvedWindowsSandboxPermissions { +pub struct ResolvedWindowsSandboxPermissions { file_system: FileSystemSandboxPolicy, network: NetworkSandboxPolicy, } @@ -56,7 +56,7 @@ pub fn token_mode_for_permission_profile( } impl ResolvedWindowsSandboxPermissions { - pub(crate) fn from_legacy_policy_for_cwd(policy: &SandboxPolicy, cwd: &Path) -> Self { + pub fn from_legacy_policy_for_cwd(policy: &SandboxPolicy, cwd: &Path) -> Self { Self { file_system: FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(policy, cwd) .materialize_project_roots_with_cwd(cwd), @@ -64,9 +64,7 @@ impl ResolvedWindowsSandboxPermissions { } } - pub(crate) fn try_from_permission_profile( - permission_profile: &PermissionProfile, - ) -> Result { + pub fn try_from_permission_profile(permission_profile: &PermissionProfile) -> Result { if !matches!(permission_profile, PermissionProfile::Managed { .. }) { anyhow::bail!( "only managed permission profiles can be enforced by the Windows sandbox" @@ -92,6 +90,26 @@ impl ResolvedWindowsSandboxPermissions { self.network } + pub(crate) fn is_enforceable_by_windows_sandbox(&self) -> bool { + matches!(self.file_system.kind, FileSystemSandboxKind::Restricted) + } + + pub(crate) fn has_full_disk_read_access(&self) -> bool { + self.file_system.has_full_disk_read_access() + } + + pub(crate) fn include_platform_defaults(&self) -> bool { + self.file_system.include_platform_defaults() + } + + pub(crate) fn readable_roots_for_cwd(&self, cwd: &Path) -> Vec { + self.file_system + .get_readable_roots_with_cwd(cwd) + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect() + } + pub(crate) fn uses_write_capabilities_for_cwd( &self, cwd: &Path, diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 353b9efeb5..e2bdd486cf 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -11,7 +11,7 @@ use std::process::Command; use std::process::Stdio; use crate::allow::AllowDenyPaths; -use crate::allow::compute_allow_paths; +use crate::allow::compute_allow_paths_for_permissions; use crate::helper_materialization::helper_bin_dir; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; @@ -85,8 +85,7 @@ pub fn sandbox_users_path(codex_home: &Path) -> PathBuf { } pub struct SandboxSetupRequest<'a> { - pub policy: &'a SandboxPolicy, - pub policy_cwd: &'a Path, + pub permissions: &'a ResolvedWindowsSandboxPermissions, pub command_cwd: &'a Path, pub env_map: &'a HashMap, pub codex_home: &'a Path, @@ -110,10 +109,17 @@ pub fn run_setup_refresh( codex_home: &Path, proxy_enforced: bool, ) -> Result<()> { + if matches!( + policy, + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } + ) { + return Ok(()); + } + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); run_setup_refresh_inner( SandboxSetupRequest { - policy, - policy_cwd, + permissions: &permissions, command_cwd, env_map, codex_home, @@ -139,12 +145,19 @@ pub fn run_setup_refresh_with_extra_read_roots( extra_read_roots: Vec, proxy_enforced: bool, ) -> Result<()> { - let mut read_roots = gather_read_roots(command_cwd, policy, codex_home); + if matches!( + policy, + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } + ) { + return Ok(()); + } + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); + let mut read_roots = gather_read_roots(command_cwd, &permissions, env_map, codex_home); read_roots.extend(extra_read_roots); run_setup_refresh_inner( SandboxSetupRequest { - policy, - policy_cwd, + permissions: &permissions, command_cwd, env_map, codex_home, @@ -164,22 +177,14 @@ fn run_setup_refresh_inner( request: SandboxSetupRequest<'_>, overrides: SetupRootOverrides, ) -> Result<()> { - // Skip in danger-full-access. - if matches!( - request.policy, - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } - ) { - return Ok(()); + if !request.permissions.is_enforceable_by_windows_sandbox() { + anyhow::bail!("unsupported filesystem permissions for Windows sandbox setup"); } let (read_roots, write_roots) = build_payload_roots(&request, &overrides); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); - let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( - request.policy, - request.policy_cwd, - ); let network_identity = - SandboxNetworkIdentity::from_permissions(&permissions, request.proxy_enforced); + SandboxNetworkIdentity::from_permissions(request.permissions, request.proxy_enforced); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); let payload = ElevationPayload { version: SETUP_VERSION, @@ -363,9 +368,10 @@ fn gather_helper_read_roots(codex_home: &Path) -> Vec { vec![helper_dir] } -fn gather_legacy_full_read_roots( +fn gather_full_read_roots_for_permissions( command_cwd: &Path, - policy: &SandboxPolicy, + permissions: &ResolvedWindowsSandboxPermissions, + env_map: &HashMap, codex_home: &Path, ) -> Vec { let mut roots = gather_helper_read_roots(codex_home); @@ -378,20 +384,40 @@ fn gather_legacy_full_read_roots( roots.extend(profile_read_roots(Path::new(&up))); } roots.push(command_cwd.to_path_buf()); - if let SandboxPolicy::WorkspaceWrite { writable_roots, .. } = policy { - for root in writable_roots { - roots.push(root.to_path_buf()); - } - } + roots.extend( + permissions + .writable_roots_for_cwd(command_cwd, env_map) + .into_iter() + .map(|root| root.root), + ); canonical_existing(&roots) } pub(crate) fn gather_read_roots( command_cwd: &Path, - policy: &SandboxPolicy, + permissions: &ResolvedWindowsSandboxPermissions, + env_map: &HashMap, codex_home: &Path, ) -> Vec { - gather_legacy_full_read_roots(command_cwd, policy, codex_home) + if permissions.has_full_disk_read_access() { + return gather_full_read_roots_for_permissions( + command_cwd, + permissions, + env_map, + codex_home, + ); + } + + let mut roots = gather_helper_read_roots(codex_home); + if permissions.include_platform_defaults() { + roots.extend( + WINDOWS_PLATFORM_DEFAULT_READ_ROOTS + .iter() + .map(PathBuf::from), + ); + } + roots.extend(permissions.readable_roots_for_cwd(command_cwd)); + canonical_existing(&roots) } pub(crate) fn gather_write_roots_for_permissions( @@ -415,17 +441,14 @@ pub(crate) fn gather_write_roots_for_permissions( } pub(crate) fn effective_write_roots_for_setup( - policy: &SandboxPolicy, - policy_cwd: &Path, + permissions: &ResolvedWindowsSandboxPermissions, command_cwd: &Path, env_map: &HashMap, codex_home: &Path, write_roots_override: Option<&[PathBuf]>, ) -> Vec { - let permissions = - ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd); effective_write_roots_for_permissions( - &permissions, + permissions, command_cwd, env_map, codex_home, @@ -760,6 +783,9 @@ pub fn run_elevated_setup( request: SandboxSetupRequest<'_>, overrides: SetupRootOverrides, ) -> Result<()> { + if !request.permissions.is_enforceable_by_windows_sandbox() { + anyhow::bail!("unsupported filesystem permissions for Windows sandbox setup"); + } // Ensure the shared sandbox directory exists before we send it to the elevated helper. let sbx_dir = sandbox_dir(request.codex_home); std::fs::create_dir_all(&sbx_dir).map_err(|err| { @@ -771,12 +797,8 @@ pub fn run_elevated_setup( let (read_roots, write_roots) = build_payload_roots(&request, &overrides); let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); - let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( - request.policy, - request.policy_cwd, - ); let network_identity = - SandboxNetworkIdentity::from_permissions(&permissions, request.proxy_enforced); + SandboxNetworkIdentity::from_permissions(request.permissions, request.proxy_enforced); let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity); let payload = ElevationPayload { version: SETUP_VERSION, @@ -808,8 +830,7 @@ fn build_payload_roots( overrides: &SetupRootOverrides, ) -> (Vec, Vec) { let write_roots = effective_write_roots_for_setup( - request.policy, - request.policy_cwd, + request.permissions, request.command_cwd, request.env_map, request.codex_home, @@ -829,7 +850,12 @@ fn build_payload_roots( read_roots.extend(roots.iter().cloned()); canonical_existing(&read_roots) } else { - gather_read_roots(request.command_cwd, request.policy, request.codex_home) + gather_read_roots( + request.command_cwd, + request.permissions, + request.env_map, + request.codex_home, + ) }; read_roots = expand_user_profile_root(read_roots); read_roots = filter_user_profile_root(read_roots); @@ -844,9 +870,8 @@ fn build_payload_deny_write_paths( request: &SandboxSetupRequest<'_>, explicit_deny_write_paths: Option>, ) -> Vec { - let allow_deny_paths: AllowDenyPaths = compute_allow_paths( - request.policy, - request.policy_cwd, + let allow_deny_paths: AllowDenyPaths = compute_allow_paths_for_permissions( + request.permissions, request.command_cwd, request.env_map, ); @@ -992,7 +1017,7 @@ fn filter_sensitive_write_roots(mut roots: Vec, codex_home: &Path) -> V mod tests { use super::WINDOWS_PLATFORM_DEFAULT_READ_ROOTS; use super::build_payload_roots; - use super::gather_legacy_full_read_roots; + use super::gather_full_read_roots_for_permissions; use super::gather_read_roots; use super::loopback_proxy_port_from_url; use super::offline_proxy_settings_from_env; @@ -1000,6 +1025,7 @@ mod tests { use super::proxy_ports_from_env; use crate::helper_materialization::helper_bin_dir; use crate::policy::SandboxPolicy; + use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -1015,6 +1041,13 @@ mod tests { .collect() } + fn permissions_for( + policy: &SandboxPolicy, + policy_cwd: &std::path::Path, + ) -> ResolvedWindowsSandboxPermissions { + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(policy, policy_cwd) + } + #[test] fn loopback_proxy_url_parsing_supports_common_forms() { assert_eq!( @@ -1315,8 +1348,9 @@ mod tests { let command_cwd = tmp.path().join("workspace"); fs::create_dir_all(&command_cwd).expect("create workspace"); let policy = SandboxPolicy::new_read_only_policy(); + let permissions = permissions_for(&policy, &command_cwd); - let roots = gather_read_roots(&command_cwd, &policy, &codex_home); + let roots = gather_read_roots(&command_cwd, &permissions, &HashMap::new(), &codex_home); let expected = dunce::canonicalize(helper_bin_dir(&codex_home)).expect("canonical helper dir"); @@ -1340,8 +1374,9 @@ mod tests { exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }; + let permissions = permissions_for(&policy, &command_cwd); - let roots = gather_read_roots(&command_cwd, &policy, &codex_home); + let roots = gather_read_roots(&command_cwd, &permissions, &HashMap::new(), &codex_home); let expected_writable = dunce::canonicalize(&writable_root).expect("canonical writable root"); @@ -1361,11 +1396,11 @@ mod tests { let policy = SandboxPolicy::ReadOnly { network_access: false, }; + let permissions = permissions_for(&policy, &policy_cwd); let (read_roots, write_roots) = build_payload_roots( &super::SandboxSetupRequest { - policy: &policy, - policy_cwd: &policy_cwd, + permissions: &permissions, command_cwd: &command_cwd, env_map: &HashMap::new(), codex_home: &codex_home, @@ -1409,11 +1444,11 @@ mod tests { let policy = SandboxPolicy::ReadOnly { network_access: false, }; + let permissions = permissions_for(&policy, &policy_cwd); let (read_roots, write_roots) = build_payload_roots( &super::SandboxSetupRequest { - policy: &policy, - policy_cwd: &policy_cwd, + permissions: &permissions, command_cwd: &command_cwd, env_map: &HashMap::new(), codex_home: &codex_home, @@ -1461,6 +1496,7 @@ mod tests { exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }; + let permissions = permissions_for(&policy, &command_cwd); let override_roots = vec![ command_cwd.clone(), extra_root.clone(), @@ -1468,8 +1504,7 @@ mod tests { sandbox_root.clone(), ]; let request = super::SandboxSetupRequest { - policy: &policy, - policy_cwd: &command_cwd, + permissions: &permissions, command_cwd: &command_cwd, env_map: &HashMap::new(), codex_home: &codex_home, @@ -1484,8 +1519,7 @@ mod tests { }; let effective_write_roots = super::effective_write_roots_for_setup( - &policy, - &command_cwd, + &permissions, &command_cwd, &HashMap::new(), &codex_home, @@ -1519,10 +1553,10 @@ mod tests { exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }; + let permissions = permissions_for(&policy, &policy_cwd); let effective_write_roots = super::effective_write_roots_for_setup( - &policy, - &policy_cwd, + &permissions, &command_cwd, &HashMap::new(), &codex_home, @@ -1555,9 +1589,9 @@ mod tests { exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }; + let permissions = permissions_for(&policy, &command_cwd); let request = super::SandboxSetupRequest { - policy: &policy, - policy_cwd: &command_cwd, + permissions: &permissions, command_cwd: &command_cwd, env_map: &HashMap::new(), codex_home: &codex_home, @@ -1586,8 +1620,14 @@ mod tests { let command_cwd = tmp.path().join("workspace"); fs::create_dir_all(&command_cwd).expect("create workspace"); let policy = SandboxPolicy::new_read_only_policy(); + let permissions = permissions_for(&policy, &command_cwd); - let roots = gather_legacy_full_read_roots(&command_cwd, &policy, &codex_home); + let roots = gather_full_read_roots_for_permissions( + &command_cwd, + &permissions, + &HashMap::new(), + &codex_home, + ); assert!( canonical_windows_platform_default_roots() diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index cf73ae3e7c..bff54a140b 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -2,7 +2,6 @@ use crate::acl::add_allow_ace; use crate::acl::add_deny_write_ace; use crate::acl::allow_null_device; use crate::allow::AllowDenyPaths; -use crate::allow::compute_allow_paths; use crate::allow::compute_allow_paths_for_permissions; use crate::cap::load_or_create_cap_sids; use crate::cap::workspace_write_cap_sid_for_root; @@ -284,8 +283,7 @@ pub(crate) fn allow_null_device_for_workspace_write(is_workspace_write: bool) { #[allow(clippy::too_many_arguments)] pub(crate) fn apply_legacy_session_acl_rules( - policy: &SandboxPolicy, - sandbox_policy_cwd: &Path, + permissions: &ResolvedWindowsSandboxPermissions, codex_home: &Path, current_dir: &Path, env_map: &HashMap, @@ -295,7 +293,7 @@ pub(crate) fn apply_legacy_session_acl_rules( persist_aces: bool, ) -> Result> { let AllowDenyPaths { allow, mut deny } = - compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map); + compute_allow_paths_for_permissions(permissions, current_dir, env_map); let mut guards: Vec<(PathBuf, String)> = Vec::new(); unsafe { for path in additional_deny_write_paths { @@ -455,8 +453,7 @@ pub(crate) fn prepare_elevated_spawn_context( write_roots_override }; let sandbox_creds = require_logon_sandbox_creds( - &common.policy, - sandbox_policy_cwd, + &common.permissions, cwd, env_map, codex_home, diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs index 64ec2ced6e..a382518822 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs @@ -331,8 +331,7 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let persist_aces = common.uses_write_capabilities; let guards = apply_legacy_session_acl_rules( - &common.policy, - sandbox_policy_cwd, + &common.permissions, codex_home, &common.current_dir, &env_map, From 7d0a05b01e60bffb0847d08e0b4425976d540f13 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:06:53 -0700 Subject: [PATCH 6/7] windows-sandbox: add profile-native elevated APIs --- .../windows-sandbox-rs/src/elevated_impl.rs | 104 ++++++++++++++---- codex-rs/windows-sandbox-rs/src/lib.rs | 6 + codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 82 +++++++++----- .../src/unified_exec/backends/elevated.rs | 70 +++++++++--- .../src/unified_exec/mod.rs | 39 +++++++ 5 files changed, 240 insertions(+), 61 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index dd343a22ce..ead1934750 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -1,3 +1,4 @@ +use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::path::Path; @@ -20,8 +21,26 @@ pub struct ElevatedSandboxCaptureRequest<'a> { pub deny_write_paths_override: &'a [AbsolutePathBuf], } +pub struct ElevatedSandboxProfileCaptureRequest<'a> { + pub permission_profile: &'a PermissionProfile, + pub permission_profile_cwd: &'a Path, + pub codex_home: &'a Path, + pub command: Vec, + pub cwd: &'a Path, + pub env_map: HashMap, + pub timeout_ms: Option, + pub use_private_desktop: bool, + pub proxy_enforced: bool, + pub read_roots_override: Option<&'a [PathBuf]>, + pub read_roots_include_platform_defaults: bool, + pub write_roots_override: Option<&'a [PathBuf]>, + pub deny_read_paths_override: &'a [AbsolutePathBuf], + pub deny_write_paths_override: &'a [AbsolutePathBuf], +} + mod windows_impl { use super::ElevatedSandboxCaptureRequest; + use super::ElevatedSandboxProfileCaptureRequest; use crate::acl::allow_null_device; use crate::cap::load_or_create_cap_sids; use crate::cap::workspace_write_cap_sid_for_root; @@ -37,7 +56,6 @@ mod windows_impl { use crate::logging::log_failure; use crate::logging::log_start; use crate::logging::log_success; - use crate::policy::SandboxPolicy; use crate::policy::parse_policy; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::runner_client::spawn_runner_transport; @@ -54,12 +72,12 @@ mod windows_impl { /// Launches the command runner under the sandbox user and captures its output. #[allow(clippy::too_many_arguments)] - pub fn run_windows_sandbox_capture( - request: ElevatedSandboxCaptureRequest<'_>, + pub fn run_windows_sandbox_capture_for_permission_profile( + request: ElevatedSandboxProfileCaptureRequest<'_>, ) -> Result { - let ElevatedSandboxCaptureRequest { - policy_json_or_preset, - sandbox_policy_cwd, + let ElevatedSandboxProfileCaptureRequest { + permission_profile, + permission_profile_cwd, codex_home, command, cwd, @@ -73,6 +91,8 @@ mod windows_impl { deny_read_paths_override, deny_write_paths_override, } = request; + let permissions = + ResolvedWindowsSandboxPermissions::try_from_permission_profile(permission_profile)?; let deny_read_paths_override = deny_read_paths_override .iter() .map(AbsolutePathBuf::to_path_buf) @@ -81,11 +101,6 @@ mod windows_impl { .iter() .map(AbsolutePathBuf::to_path_buf) .collect::>(); - let policy = parse_policy(policy_json_or_preset)?; - let permissions = ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd( - &policy, - sandbox_policy_cwd, - ); normalize_null_device_env(&mut env_map); ensure_non_interactive_pager(&mut env_map); inherit_path_env(&mut env_map); @@ -109,12 +124,6 @@ mod windows_impl { proxy_enforced, )?; // Build capability SID for ACL grants. - if matches!( - &policy, - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } - ) { - anyhow::bail!("DangerFullAccess and ExternalSandbox are not supported for sandboxing") - } let caps = load_or_create_cap_sids(codex_home)?; let (sid_for_null, cap_sids) = if permissions.uses_write_capabilities_for_cwd(cwd, &env_map) { @@ -143,14 +152,12 @@ mod windows_impl { } (|| -> Result { - let permission_profile = - PermissionProfile::from_legacy_sandbox_policy_for_cwd(&policy, sandbox_policy_cwd); let spawn_request = SpawnRequest { command: command.clone(), cwd: cwd.to_path_buf(), env: env_map.clone(), - permission_profile, - permission_profile_cwd: sandbox_policy_cwd.to_path_buf(), + permission_profile: permission_profile.clone(), + permission_profile_cwd: permission_profile_cwd.to_path_buf(), codex_home: sandbox_base.clone(), real_codex_home: codex_home.to_path_buf(), cap_sids, @@ -210,6 +217,48 @@ mod windows_impl { })() } + /// Legacy policy-string adapter for callers that have not moved to permission profiles yet. + #[allow(clippy::too_many_arguments)] + pub fn run_windows_sandbox_capture( + request: ElevatedSandboxCaptureRequest<'_>, + ) -> Result { + let ElevatedSandboxCaptureRequest { + policy_json_or_preset, + sandbox_policy_cwd, + codex_home, + command, + cwd, + env_map, + timeout_ms, + use_private_desktop, + proxy_enforced, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + deny_read_paths_override, + deny_write_paths_override, + } = request; + let policy = parse_policy(policy_json_or_preset)?; + let permission_profile = + PermissionProfile::from_legacy_sandbox_policy_for_cwd(&policy, sandbox_policy_cwd); + run_windows_sandbox_capture_for_permission_profile(ElevatedSandboxProfileCaptureRequest { + permission_profile: &permission_profile, + permission_profile_cwd: sandbox_policy_cwd, + codex_home, + command, + cwd, + env_map, + timeout_ms, + use_private_desktop, + proxy_enforced, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + deny_read_paths_override, + deny_write_paths_override, + }) + } + #[cfg(test)] mod tests { use crate::policy::SandboxPolicy; @@ -242,10 +291,13 @@ mod windows_impl { #[cfg(target_os = "windows")] pub use windows_impl::run_windows_sandbox_capture; +#[cfg(target_os = "windows")] +pub use windows_impl::run_windows_sandbox_capture_for_permission_profile; #[cfg(not(target_os = "windows"))] mod stub { use super::ElevatedSandboxCaptureRequest; + use super::ElevatedSandboxProfileCaptureRequest; use anyhow::Result; use anyhow::bail; @@ -264,7 +316,17 @@ mod stub { ) -> Result { bail!("Windows sandbox is only available on Windows") } + + /// Stub implementation for non-Windows targets; sandboxing only works on Windows. + #[allow(clippy::too_many_arguments)] + pub fn run_windows_sandbox_capture_for_permission_profile( + _request: ElevatedSandboxProfileCaptureRequest<'_>, + ) -> Result { + bail!("Windows sandbox is only available on Windows") + } } #[cfg(not(target_os = "windows"))] pub use stub::run_windows_sandbox_capture; +#[cfg(not(target_os = "windows"))] +pub use stub::run_windows_sandbox_capture_for_permission_profile; diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index a363652476..6bd91f3180 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -139,8 +139,12 @@ pub use dpapi::unprotect as dpapi_unprotect; #[cfg(target_os = "windows")] pub use elevated_impl::ElevatedSandboxCaptureRequest; #[cfg(target_os = "windows")] +pub use elevated_impl::ElevatedSandboxProfileCaptureRequest; +#[cfg(target_os = "windows")] pub use elevated_impl::run_windows_sandbox_capture as run_windows_sandbox_capture_elevated; #[cfg(target_os = "windows")] +pub use elevated_impl::run_windows_sandbox_capture_for_permission_profile as run_windows_sandbox_capture_for_permission_profile_elevated; +#[cfg(target_os = "windows")] pub use helper_materialization::resolve_current_exe_for_launch; #[cfg(target_os = "windows")] pub use hide_users::hide_current_user_profile_dir; @@ -256,6 +260,8 @@ pub use token::get_current_token_for_restriction; #[cfg(target_os = "windows")] pub use unified_exec::spawn_windows_sandbox_session_elevated; #[cfg(target_os = "windows")] +pub use unified_exec::spawn_windows_sandbox_session_elevated_for_permission_profile; +#[cfg(target_os = "windows")] pub use unified_exec::spawn_windows_sandbox_session_legacy; #[cfg(target_os = "windows")] pub use wfp::install_wfp_filters_for_account; diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index bff54a140b..65b5c38d4a 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -51,7 +51,8 @@ pub(crate) struct SpawnContext { } pub(crate) struct ElevatedSpawnContext { - pub(crate) common: SpawnContext, + pub(crate) sandbox_base: PathBuf, + pub(crate) logs_base_dir: Option, pub(crate) sandbox_creds: SandboxCreds, pub(crate) cap_sids: Vec, } @@ -400,9 +401,8 @@ pub(crate) fn apply_legacy_session_acl_rules( } #[allow(clippy::too_many_arguments)] -pub(crate) fn prepare_elevated_spawn_context( - policy_json_or_preset: &str, - sandbox_policy_cwd: &Path, +pub(crate) fn prepare_elevated_spawn_context_for_permissions( + permissions: ResolvedWindowsSandboxPermissions, codex_home: &Path, cwd: &Path, env_map: &mut HashMap, @@ -413,33 +413,33 @@ pub(crate) fn prepare_elevated_spawn_context( deny_read_paths_override: &[PathBuf], deny_write_paths_override: &[PathBuf], ) -> Result { - let common = prepare_spawn_context_common( - policy_json_or_preset, - sandbox_policy_cwd, - codex_home, - cwd, - env_map, - command, - SpawnPrepOptions { - inherit_path: true, - add_git_safe_directory: true, - }, - )?; + normalize_null_device_env(env_map); + ensure_non_interactive_pager(env_map); + inherit_path_env(env_map); + inject_git_safe_directory(env_map, cwd); + + // Use a temp-based log dir that the sandbox user can write. + let sandbox_base = codex_home.join(".sandbox"); + ensure_codex_home_exists(&sandbox_base)?; + let logs_base_dir = Some(sandbox_base.clone()); + log_start(command, logs_base_dir.as_deref()); + + let uses_write_capabilities = permissions.uses_write_capabilities_for_cwd(cwd, env_map); let AllowDenyPaths { allow, deny } = - compute_allow_paths_for_permissions(&common.permissions, &common.current_dir, env_map); + compute_allow_paths_for_permissions(&permissions, cwd, env_map); let write_roots: Vec = allow.into_iter().collect(); let deny_write_paths: Vec = deny.into_iter().collect(); - let computed_write_roots_override = if common.uses_write_capabilities { + let computed_write_roots_override = if uses_write_capabilities { Some(write_roots.as_slice()) } else { None }; let write_roots_for_setup = write_roots_override.or(computed_write_roots_override); - let effective_write_roots = if common.uses_write_capabilities { + let effective_write_roots = if uses_write_capabilities { effective_write_roots_for_permissions( - &common.permissions, - &common.current_dir, + &permissions, + cwd, env_map, codex_home, write_roots_for_setup, @@ -447,13 +447,13 @@ pub(crate) fn prepare_elevated_spawn_context( } else { Vec::new() }; - let setup_write_roots_override = if common.uses_write_capabilities { + let setup_write_roots_override = if uses_write_capabilities { Some(effective_write_roots.as_slice()) } else { write_roots_override }; let sandbox_creds = require_logon_sandbox_creds( - &common.permissions, + &permissions, cwd, env_map, codex_home, @@ -469,7 +469,7 @@ pub(crate) fn prepare_elevated_spawn_context( /*proxy_enforced*/ false, )?; let caps = load_or_create_cap_sids(codex_home)?; - let (psid_to_use, cap_sids) = if common.uses_write_capabilities { + let (psid_to_use, cap_sids) = if uses_write_capabilities { let cap_sids = root_capability_sids(codex_home, cwd, effective_write_roots)? .into_iter() .map(|root_sid| root_sid.sid_str) @@ -490,12 +490,44 @@ pub(crate) fn prepare_elevated_spawn_context( } Ok(ElevatedSpawnContext { - common, + sandbox_base, + logs_base_dir, sandbox_creds, cap_sids, }) } +#[allow(clippy::too_many_arguments)] +pub(crate) fn prepare_elevated_spawn_context( + policy_json_or_preset: &str, + sandbox_policy_cwd: &Path, + codex_home: &Path, + cwd: &Path, + env_map: &mut HashMap, + command: &[String], + read_roots_override: Option<&[PathBuf]>, + read_roots_include_platform_defaults: bool, + write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[PathBuf], + deny_write_paths_override: &[PathBuf], +) -> Result { + let policy = parse_policy(policy_json_or_preset)?; + let permissions = + ResolvedWindowsSandboxPermissions::from_legacy_policy_for_cwd(&policy, sandbox_policy_cwd); + prepare_elevated_spawn_context_for_permissions( + permissions, + codex_home, + cwd, + env_map, + command, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + deny_read_paths_override, + deny_write_paths_override, + ) +} + #[cfg(test)] mod tests { use super::SandboxPolicy; diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs index 5a756b6413..67cb532df7 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs @@ -7,8 +7,10 @@ use crate::ipc_framed::EmptyPayload; use crate::ipc_framed::FramedMessage; use crate::ipc_framed::Message; use crate::ipc_framed::SpawnRequest; +use crate::policy::parse_policy; +use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::runner_client::spawn_runner_transport; -use crate::spawn_prep::prepare_elevated_spawn_context; +use crate::spawn_prep::prepare_elevated_spawn_context_for_permissions; use anyhow::Result; use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; @@ -22,9 +24,9 @@ use tokio::sync::mpsc; use tokio::sync::oneshot; #[allow(clippy::too_many_arguments)] -pub(crate) async fn spawn_windows_sandbox_session_elevated( - policy_json_or_preset: &str, - sandbox_policy_cwd: &Path, +pub(crate) async fn spawn_windows_sandbox_session_elevated_for_permission_profile( + permission_profile: &PermissionProfile, + permission_profile_cwd: &Path, codex_home: &Path, command: Vec, cwd: &Path, @@ -47,9 +49,10 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( .iter() .map(AbsolutePathBuf::to_path_buf) .collect::>(); - let elevated = prepare_elevated_spawn_context( - policy_json_or_preset, - sandbox_policy_cwd, + let permissions = + ResolvedWindowsSandboxPermissions::try_from_permission_profile(permission_profile)?; + let elevated = prepare_elevated_spawn_context_for_permissions( + permissions, codex_home, cwd, &mut env_map, @@ -61,17 +64,13 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( &deny_write_paths_override, )?; - let permission_profile = PermissionProfile::from_legacy_sandbox_policy_for_cwd( - &elevated.common.policy, - sandbox_policy_cwd, - ); let spawn_request = SpawnRequest { command: command.clone(), cwd: cwd.to_path_buf(), env: env_map.clone(), - permission_profile, - permission_profile_cwd: sandbox_policy_cwd.to_path_buf(), - codex_home: elevated.common.sandbox_base.clone(), + permission_profile: permission_profile.clone(), + permission_profile_cwd: permission_profile_cwd.to_path_buf(), + codex_home: elevated.sandbox_base.clone(), real_codex_home: codex_home.to_path_buf(), cap_sids: elevated.cap_sids.clone(), timeout_ms, @@ -82,7 +81,7 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( let codex_home = codex_home.to_path_buf(); let cwd = cwd.to_path_buf(); let sandbox_creds = elevated.sandbox_creds.clone(); - let logs_base_dir = elevated.common.logs_base_dir.clone(); + let logs_base_dir = elevated.logs_base_dir.clone(); let transport = tokio::task::spawn_blocking(move || -> Result<_> { spawn_runner_transport( &codex_home, @@ -143,3 +142,44 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( stdin_open, )) } + +#[allow(clippy::too_many_arguments)] +pub(crate) async fn spawn_windows_sandbox_session_elevated( + policy_json_or_preset: &str, + sandbox_policy_cwd: &Path, + codex_home: &Path, + command: Vec, + cwd: &Path, + env_map: HashMap, + timeout_ms: Option, + read_roots_override: Option<&[PathBuf]>, + read_roots_include_platform_defaults: bool, + write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[AbsolutePathBuf], + deny_write_paths_override: &[AbsolutePathBuf], + tty: bool, + stdin_open: bool, + use_private_desktop: bool, +) -> Result { + let policy = parse_policy(policy_json_or_preset)?; + let permission_profile = + PermissionProfile::from_legacy_sandbox_policy_for_cwd(&policy, sandbox_policy_cwd); + spawn_windows_sandbox_session_elevated_for_permission_profile( + &permission_profile, + sandbox_policy_cwd, + codex_home, + command, + cwd, + env_map, + timeout_ms, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + deny_read_paths_override, + deny_write_paths_override, + tty, + stdin_open, + use_private_desktop, + ) + .await +} diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs index adb653b6fe..ceb4bd03b6 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/mod.rs @@ -10,6 +10,7 @@ mod backends; use anyhow::Result; +use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_pty::SpawnedProcess; use std::collections::HashMap; @@ -48,6 +49,44 @@ pub async fn spawn_windows_sandbox_session_legacy( .await } +#[allow(clippy::too_many_arguments)] +pub async fn spawn_windows_sandbox_session_elevated_for_permission_profile( + permission_profile: &PermissionProfile, + permission_profile_cwd: &Path, + codex_home: &Path, + command: Vec, + cwd: &Path, + env_map: HashMap, + timeout_ms: Option, + read_roots_override: Option<&[PathBuf]>, + read_roots_include_platform_defaults: bool, + write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[AbsolutePathBuf], + deny_write_paths_override: &[AbsolutePathBuf], + tty: bool, + stdin_open: bool, + use_private_desktop: bool, +) -> Result { + backends::elevated::spawn_windows_sandbox_session_elevated_for_permission_profile( + permission_profile, + permission_profile_cwd, + codex_home, + command, + cwd, + env_map, + timeout_ms, + read_roots_override, + read_roots_include_platform_defaults, + write_roots_override, + deny_read_paths_override, + deny_write_paths_override, + tty, + stdin_open, + use_private_desktop, + ) + .await +} + #[allow(clippy::too_many_arguments)] pub async fn spawn_windows_sandbox_session_elevated( policy_json_or_preset: &str, From f1987f519cbb696a3524285afdff5758281ae3a6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 20 May 2026 10:06:53 -0700 Subject: [PATCH 7/7] core: pass permission profiles to Windows runner --- codex-rs/core/src/exec.rs | 34 +++++++++++++------ .../core/src/unified_exec/process_manager.rs | 4 +-- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index a6d958b107..84ebe337c6 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -433,10 +433,10 @@ pub(crate) async fn execute_exec_request( expiration, capture_policy, sandbox, - windows_sandbox_policy_cwd: _, + windows_sandbox_policy_cwd, windows_sandbox_level, windows_sandbox_private_desktop, - permission_profile: _, + permission_profile, file_system_sandbox_policy: _, network_sandbox_policy, windows_sandbox_filesystem_overrides, @@ -465,6 +465,8 @@ pub(crate) async fn execute_exec_request( after_spawn, sandbox, &sandbox_policy, + &permission_profile, + &windows_sandbox_policy_cwd, windows_sandbox_filesystem_overrides.as_ref(), ) .await; @@ -472,6 +474,7 @@ pub(crate) async fn execute_exec_request( finalize_exec_result(raw_output_result, sandbox, duration) } +#[allow(clippy::too_many_arguments)] async fn get_raw_output_result( params: ExecParams, network_sandbox_policy: NetworkSandboxPolicy, @@ -479,14 +482,22 @@ async fn get_raw_output_result( after_spawn: Option>, #[cfg_attr(not(windows), allow(unused_variables))] sandbox: SandboxType, #[cfg_attr(not(windows), allow(unused_variables))] sandbox_policy: &SandboxPolicy, + #[cfg_attr(not(windows), allow(unused_variables))] permission_profile: &PermissionProfile, + #[cfg_attr(not(windows), allow(unused_variables))] windows_sandbox_policy_cwd: &AbsolutePathBuf, #[cfg_attr(not(windows), allow(unused_variables))] windows_sandbox_filesystem_overrides: Option< &WindowsSandboxFilesystemOverrides, >, ) -> Result { #[cfg(target_os = "windows")] if sandbox == SandboxType::WindowsRestrictedToken { - return exec_windows_sandbox(params, sandbox_policy, windows_sandbox_filesystem_overrides) - .await; + return exec_windows_sandbox( + params, + sandbox_policy, + permission_profile, + windows_sandbox_policy_cwd, + windows_sandbox_filesystem_overrides, + ) + .await; } exec(params, network_sandbox_policy, stdout_stream, after_spawn).await @@ -562,10 +573,12 @@ fn record_windows_sandbox_spawn_failure( async fn exec_windows_sandbox( params: ExecParams, sandbox_policy: &SandboxPolicy, + permission_profile: &PermissionProfile, + windows_sandbox_policy_cwd: &AbsolutePathBuf, windows_sandbox_filesystem_overrides: Option<&WindowsSandboxFilesystemOverrides>, ) -> Result { use crate::config::find_codex_home; - use codex_windows_sandbox::run_windows_sandbox_capture_elevated; + use codex_windows_sandbox::run_windows_sandbox_capture_for_permission_profile_elevated; use codex_windows_sandbox::run_windows_sandbox_capture_with_filesystem_overrides; let ExecParams { @@ -596,7 +609,8 @@ async fn exec_windows_sandbox( "failed to serialize Windows sandbox policy: {err}" ))) })?; - let sandbox_cwd = cwd.clone(); + let sandbox_cwd = windows_sandbox_policy_cwd.clone(); + let permission_profile = permission_profile.clone(); let codex_home = find_codex_home().map_err(|err| { CodexErr::Io(io::Error::other(format!( "windows sandbox: failed to resolve codex_home: {err}" @@ -620,10 +634,10 @@ async fn exec_windows_sandbox( .and_then(|overrides| overrides.write_roots_override.clone()); let spawn_res = tokio::task::spawn_blocking(move || { if use_elevated { - run_windows_sandbox_capture_elevated( - codex_windows_sandbox::ElevatedSandboxCaptureRequest { - policy_json_or_preset: policy_str.as_str(), - sandbox_policy_cwd: &sandbox_cwd, + run_windows_sandbox_capture_for_permission_profile_elevated( + codex_windows_sandbox::ElevatedSandboxProfileCaptureRequest { + permission_profile: &permission_profile, + permission_profile_cwd: &sandbox_cwd, codex_home: codex_home.as_ref(), command, cwd: &cwd, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 2f5de77319..fb99500755 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -901,8 +901,8 @@ impl UnifiedExecProcessManager { .and_then(|overrides| overrides.write_roots_override.clone()); let spawned = match request.windows_sandbox_level { codex_protocol::config_types::WindowsSandboxLevel::Elevated => { - codex_windows_sandbox::spawn_windows_sandbox_session_elevated( - policy_json.as_str(), + codex_windows_sandbox::spawn_windows_sandbox_session_elevated_for_permission_profile( + &request.permission_profile, request.windows_sandbox_policy_cwd.as_path(), codex_home.as_ref(), request.command.clone(),