From 2d73bac45f9b7a5a3304c17a2e67b9a6fc6ff10c Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Wed, 22 Apr 2026 14:00:53 -0700 Subject: [PATCH 1/2] feat: add guardian network approval trigger context (#18197) ## Summary Give guardian network-access reviews the command context that triggered a managed-network approval. The prompt JSON now includes the originating tool call id, tool name, command argv, cwd, sandbox permissions, additional permissions, justification, and tty state when a single active tool call can be attributed. The implementation keeps the trigger shape canonical by serializing `GuardianNetworkAccessTrigger` directly and lets each runtime build that trigger from its `ToolCtx`. Non-guardian approval prompts avoid cloning the full trigger payload. ## UX changes Guardian network-access reviews now include a `trigger` object that explains what command caused the network approval. Instead of seeing only the requested host, the guardian reviewer can also see the originating tool call, argv, working directory, sandbox mode, justification, and tty state. Example payload the guardian reviewer can see: ```json { "tool": "network_access", "target": "https://api.github.com:443", "host": "api.github.com", "protocol": "https", "port": 443, "trigger": { "callId": "call_abc123", "toolName": "shell", "command": ["gh", "api", "/repos/openai/codex/pulls/18197"], "cwd": "/workspace/codex", "sandboxPermissions": "require_escalated", "justification": "Fetch PR metadata from GitHub.", "tty": false } } ``` The network review itself remains scoped to the network decision: `target_item_id` stays `null`. `trigger.callId` is attribution context only, so clients can still distinguish network reviews from item-targeted command reviews. ## Verification - Added coverage for serializing network trigger context in guardian approval JSON. - Added regression coverage that network guardian reviews do not reuse `trigger.callId` as `target_item_id`. --------- Co-authored-by: Codex --- .../core/src/guardian/approval_request.rs | 50 +++++++++--- codex-rs/core/src/guardian/mod.rs | 1 + codex-rs/core/src/guardian/tests.rs | 70 ++++++++++++++++ codex-rs/core/src/tools/network_approval.rs | 33 ++++++-- .../core/src/tools/network_approval_tests.rs | 80 +++++++++++++------ codex-rs/core/src/tools/runtimes/shell.rs | 13 ++- .../core/src/tools/runtimes/unified_exec.rs | 13 ++- 7 files changed, 220 insertions(+), 40 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 989c6ee119..2afc5e0805 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -54,6 +54,7 @@ pub(crate) enum GuardianApprovalRequest { host: String, protocol: NetworkApprovalProtocol, port: u16, + trigger: Option, }, McpToolCall { id: String, @@ -75,6 +76,22 @@ pub(crate) enum GuardianApprovalRequest { }, } +#[derive(Debug, Clone, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct GuardianNetworkAccessTrigger { + pub(crate) call_id: String, + pub(crate) tool_name: String, + pub(crate) command: Vec, + pub(crate) cwd: AbsolutePathBuf, + pub(crate) sandbox_permissions: crate::sandboxing::SandboxPermissions, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) additional_permissions: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) justification: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) tty: Option, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub(crate) struct GuardianMcpAnnotations { #[serde(skip_serializing_if = "Option::is_none")] @@ -131,6 +148,18 @@ struct McpToolCallApprovalAction<'a> { annotations: Option<&'a GuardianMcpAnnotations>, } +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct NetworkAccessApprovalAction<'a> { + tool: &'static str, + target: &'a str, + host: &'a str, + protocol: NetworkApprovalProtocol, + port: u16, + #[serde(skip_serializing_if = "Option::is_none")] + trigger: Option<&'a GuardianNetworkAccessTrigger>, +} + #[derive(Serialize)] struct RequestPermissionsApprovalAction<'a> { tool: &'static str, @@ -297,13 +326,15 @@ pub(crate) fn guardian_approval_request_to_json( host, protocol, port, - } => Ok(serde_json::json!({ - "tool": "network_access", - "target": target, - "host": host, - "protocol": protocol, - "port": port, - })), + trigger, + } => serialize_guardian_action(NetworkAccessApprovalAction { + tool: "network_access", + target, + host, + protocol: *protocol, + port: *port, + trigger: trigger.as_ref(), + }), GuardianApprovalRequest::McpToolCall { id: _, server, @@ -371,12 +402,13 @@ pub(crate) fn guardian_assessment_action( } } GuardianApprovalRequest::NetworkAccess { - id: _, - turn_id: _, + id: _id, + turn_id: _turn_id, target, host, protocol, port, + trigger: _trigger, } => GuardianAssessmentAction::NetworkAccess { target: target.clone(), host: host.clone(), diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 155db6f1f4..531815ed7b 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -25,6 +25,7 @@ use serde::Serialize; pub(crate) use approval_request::GuardianApprovalRequest; pub(crate) use approval_request::GuardianMcpAnnotations; +pub(crate) use approval_request::GuardianNetworkAccessTrigger; pub(crate) use approval_request::guardian_approval_request_to_json; pub(crate) use review::guardian_rejection_message; pub(crate) use review::guardian_timeout_message; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index cf29c54112..41e33accf4 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -12,6 +12,7 @@ use crate::config_loader::NetworkDomainPermissionToml; use crate::config_loader::NetworkDomainPermissionsToml; use crate::config_loader::RequirementSource; use crate::config_loader::Sourced; +use crate::guardian::approval_request::guardian_request_target_item_id; use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::test_support; @@ -727,6 +728,50 @@ fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json Ok(()) } +#[test] +fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_json::Result<()> { + let cwd = test_path_buf("/repo").abs(); + let action = GuardianApprovalRequest::NetworkAccess { + id: "network-1".to_string(), + turn_id: "turn-1".to_string(), + target: "https://example.com:443".to_string(), + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Https, + port: 443, + trigger: Some(GuardianNetworkAccessTrigger { + call_id: "call-1".to_string(), + tool_name: "shell".to_string(), + command: vec!["curl".to_string(), "https://example.com".to_string()], + cwd: cwd.clone(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: Some("Fetch the release metadata.".to_string()), + tty: None, + }), + }; + + assert_eq!( + guardian_approval_request_to_json(&action)?, + serde_json::json!({ + "tool": "network_access", + "target": "https://example.com:443", + "host": "example.com", + "protocol": "https", + "port": 443, + "trigger": { + "callId": "call-1", + "toolName": "shell", + "command": ["curl", "https://example.com"], + "cwd": cwd.to_string_lossy().to_string(), + "sandboxPermissions": "use_default", + "justification": "Fetch the release metadata.", + }, + }) + ); + + Ok(()) +} + #[test] fn guardian_assessment_action_redacts_apply_patch_patch_text() { let cwd = test_path_buf("/tmp").abs(); @@ -758,6 +803,7 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() { host: "example.com".to_string(), protocol: NetworkApprovalProtocol::Https, port: 443, + trigger: None, }; let apply_patch = GuardianApprovalRequest::ApplyPatch { id: "patch-1".to_string(), @@ -777,6 +823,30 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() { ); } +#[test] +fn guardian_request_target_item_id_omits_network_access_trigger_call_id() { + let network_access = GuardianApprovalRequest::NetworkAccess { + id: "network-1".to_string(), + turn_id: "owner-turn".to_string(), + target: "https://example.com:443".to_string(), + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Https, + port: 443, + trigger: Some(GuardianNetworkAccessTrigger { + call_id: "call-1".to_string(), + tool_name: "shell".to_string(), + command: vec!["curl".to_string(), "https://example.com".to_string()], + cwd: test_path_buf("/repo").abs(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + tty: None, + }), + }; + + assert_eq!(guardian_request_target_item_id(&network_access), None); +} + #[tokio::test] async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { let (session, turn, rx) = crate::session::tests::make_session_and_context_with_rx().await; diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 4be18bad84..d43859e626 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -1,4 +1,5 @@ use crate::guardian::GuardianApprovalRequest; +use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; @@ -47,6 +48,7 @@ pub(crate) enum NetworkApprovalMode { pub(crate) struct NetworkApprovalSpec { pub network: Option, pub mode: NetworkApprovalMode, + pub trigger: GuardianNetworkAccessTrigger, pub command: String, } @@ -177,6 +179,7 @@ impl PendingHostApproval { struct ActiveNetworkApprovalCall { registration_id: String, turn_id: String, + trigger: GuardianNetworkAccessTrigger, command: String, } @@ -210,7 +213,13 @@ impl NetworkApprovalService { other_approved_hosts.extend(approved_hosts.iter().cloned()); } - async fn register_call(&self, registration_id: String, turn_id: String, command: String) { + async fn register_call( + &self, + registration_id: String, + turn_id: String, + trigger: GuardianNetworkAccessTrigger, + command: String, + ) { let mut active_calls = self.active_calls.lock().await; let key = registration_id.clone(); active_calls.insert( @@ -218,6 +227,7 @@ impl NetworkApprovalService { Arc::new(ActiveNetworkApprovalCall { registration_id, turn_id, + trigger, command, }), ); @@ -369,11 +379,11 @@ impl NetworkApprovalService { return NetworkDecision::deny(REASON_NOT_ALLOWED); } + let owner_call = self.resolve_single_active_call().await; let network_approval_context = NetworkApprovalContext { host: request.host.clone(), protocol, }; - let owner_call = self.resolve_single_active_call().await; let guardian_approval_id = Self::approval_id_for_key(&key); let prompt_command = vec!["network-access".to_string(), target.clone()]; let command = owner_call @@ -431,6 +441,7 @@ impl NetworkApprovalService { host: request.host, protocol, port: key.port, + trigger: owner_call.as_ref().map(|call| call.trigger.clone()), }, Some(policy_denial_message.clone()), ) @@ -623,8 +634,13 @@ pub(crate) async fn begin_network_approval( managed_network_active: bool, spec: Option, ) -> Option { - let spec = spec?; - if !managed_network_active || spec.network.is_none() { + let NetworkApprovalSpec { + network, + mode, + trigger, + command, + } = spec?; + if !managed_network_active || network.is_none() { return None; } @@ -632,12 +648,17 @@ pub(crate) async fn begin_network_approval( session .services .network_approval - .register_call(registration_id.clone(), turn_id.to_string(), spec.command) + .register_call( + registration_id.clone(), + turn_id.to_string(), + trigger, + command, + ) .await; Some(ActiveNetworkApproval { registration_id: Some(registration_id), - mode: spec.mode, + mode, }) } diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index 888d0bc353..ac0046228b 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -1,7 +1,10 @@ use super::*; +use crate::sandboxing::SandboxPermissions; use codex_network_proxy::BlockedRequestArgs; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; +use core_test_support::PathBufExt; +use core_test_support::test_path_buf; use pretty_assertions::assert_eq; #[tokio::test] @@ -207,17 +210,66 @@ fn denied_blocked_request(host: &str) -> BlockedRequest { }) } +async fn register_call_with_default_shell_trigger( + service: &NetworkApprovalService, + registration_id: &str, +) { + service + .register_call( + registration_id.to_string(), + "turn-1".to_string(), + GuardianNetworkAccessTrigger { + call_id: "call-1".to_string(), + tool_name: "shell".to_string(), + command: vec!["curl".to_string(), "https://example.com".to_string()], + cwd: test_path_buf("/tmp").abs(), + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + justification: None, + tty: None, + }, + "curl https://example.com".to_string(), + ) + .await; +} + #[tokio::test] -async fn record_blocked_request_sets_policy_outcome_for_owner_call() { +async fn active_call_preserves_triggering_command_context() { let service = NetworkApprovalService::default(); + let expected = GuardianNetworkAccessTrigger { + call_id: "call-1".to_string(), + tool_name: "shell".to_string(), + command: vec!["curl".to_string(), "https://example.com".to_string()], + cwd: test_path_buf("/repo").abs(), + sandbox_permissions: SandboxPermissions::UseDefault, + additional_permissions: None, + justification: Some("fetch release metadata".to_string()), + tty: None, + }; + service .register_call( "registration-1".to_string(), "turn-1".to_string(), - "curl http://example.com".to_string(), + expected.clone(), + "curl https://example.com".to_string(), ) .await; + let call = service + .resolve_single_active_call() + .await + .expect("single active call should resolve"); + + assert_eq!(&call.trigger, &expected); + assert_eq!(call.command, "curl https://example.com"); +} + +#[tokio::test] +async fn record_blocked_request_sets_policy_outcome_for_owner_call() { + let service = NetworkApprovalService::default(); + register_call_with_default_shell_trigger(&service, "registration-1").await; + service .record_blocked_request(denied_blocked_request("example.com")) .await; @@ -233,13 +285,7 @@ async fn record_blocked_request_sets_policy_outcome_for_owner_call() { #[tokio::test] async fn blocked_request_policy_does_not_override_user_denial_outcome() { let service = NetworkApprovalService::default(); - service - .register_call( - "registration-1".to_string(), - "turn-1".to_string(), - "curl http://example.com".to_string(), - ) - .await; + register_call_with_default_shell_trigger(&service, "registration-1").await; service .record_call_outcome("registration-1", NetworkApprovalOutcome::DeniedByUser) @@ -257,20 +303,8 @@ async fn blocked_request_policy_does_not_override_user_denial_outcome() { #[tokio::test] async fn record_blocked_request_ignores_ambiguous_unattributed_blocked_requests() { let service = NetworkApprovalService::default(); - service - .register_call( - "registration-1".to_string(), - "turn-1".to_string(), - "curl http://example.com".to_string(), - ) - .await; - service - .register_call( - "registration-2".to_string(), - "turn-1".to_string(), - "gh api /foo".to_string(), - ) - .await; + register_call_with_default_shell_trigger(&service, "registration-1").await; + register_call_with_default_shell_trigger(&service, "registration-2").await; service .record_blocked_request(denied_blocked_request("example.com")) diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 34292421a0..4bbbe774a7 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -11,6 +11,7 @@ pub(crate) mod zsh_fork_backend; use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::guardian::GuardianApprovalRequest; +use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; @@ -217,12 +218,22 @@ impl ToolRuntime for ShellRuntime { fn network_approval_spec( &self, req: &ShellRequest, - _ctx: &ToolCtx, + ctx: &ToolCtx, ) -> Option { req.network.as_ref()?; Some(NetworkApprovalSpec { network: req.network.clone(), mode: NetworkApprovalMode::Immediate, + trigger: GuardianNetworkAccessTrigger { + call_id: ctx.call_id.clone(), + tool_name: ctx.tool_name.clone(), + command: req.command.clone(), + cwd: req.cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: None, + }, command: req.hook_command.clone(), }) } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 955f7d830c..1a4132da66 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -8,6 +8,7 @@ use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::guardian::GuardianApprovalRequest; +use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::review_approval_request; use crate::sandboxing::ExecOptions; use crate::sandboxing::ExecServerEnvConfig; @@ -202,12 +203,22 @@ impl<'a> ToolRuntime for UnifiedExecRunt fn network_approval_spec( &self, req: &UnifiedExecRequest, - _ctx: &ToolCtx, + ctx: &ToolCtx, ) -> Option { req.network.as_ref()?; Some(NetworkApprovalSpec { network: req.network.clone(), mode: NetworkApprovalMode::Deferred, + trigger: GuardianNetworkAccessTrigger { + call_id: ctx.call_id.clone(), + tool_name: ctx.tool_name.clone(), + command: req.command.clone(), + cwd: req.cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: Some(req.tty), + }, command: req.hook_command.clone(), }) } From a96e08da33a6d9d461762187d0c4fc1f70d662ef Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 22 Apr 2026 14:06:36 -0700 Subject: [PATCH 2/2] exec-server: expose arg0 alias root to fs sandbox ## Why The post-merge `rust-ci-full` run for #18999 still failed the Ubuntu remote `suite::remote_env` sandboxed filesystem tests. That run checked out merge commit `ddde50c611e4800cb805f243ed3c50bbafe7d011`, so the arg0 guard lifetime fix was present. The remaining gap is that the remote exec-server can pass an arg0 alias path such as `codex-linux-sandbox` as a runtime helper, but the sandboxed filesystem helper only added the real Codex binary parent as a readable runtime root. When bubblewrap re-enters Codex through the alias path, the alias directory also has to be visible inside the sandbox. ## What Changed - Track all helper runtime read roots instead of a single root. - Add both the real Codex executable parent and the `codex-linux-sandbox` alias parent to sandbox readable roots. - Add unit coverage for the alias-parent root. ## Verification - `cargo test -p codex-exec-server` - `just argument-comment-lint -p codex-exec-server` - `just fix -p codex-exec-server` --- codex-rs/core/tests/suite/remote_env.rs | 42 ++++--- codex-rs/exec-server/src/file_system.rs | 28 +++++ codex-rs/exec-server/src/fs_sandbox.rs | 113 +++++++++++------- .../exec-server/src/remote_file_system.rs | 75 ++++++++++-- .../exec-server/src/server/handler/tests.rs | 4 +- 5 files changed, 194 insertions(+), 68 deletions(-) diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 36c9b35c30..1e781a669b 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -59,26 +59,38 @@ fn absolute_path(path: PathBuf) -> AbsolutePathBuf { } fn read_only_sandbox(readable_root: PathBuf) -> FileSystemSandboxContext { - FileSystemSandboxContext::new(SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: vec![absolute_path(readable_root)], + let readable_root = absolute_path(readable_root); + // The policy is evaluated in the remote container, so use a container path + // for cwd instead of capturing the local test runner cwd. + FileSystemSandboxContext::from_legacy_sandbox_policy( + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![readable_root.clone()], + }, + network_access: false, }, - network_access: false, - }) + readable_root, + ) } fn workspace_write_sandbox(writable_root: PathBuf) -> FileSystemSandboxContext { - FileSystemSandboxContext::new(SandboxPolicy::WorkspaceWrite { - writable_roots: vec![absolute_path(writable_root)], - read_only_access: ReadOnlyAccess::Restricted { - include_platform_defaults: false, - readable_roots: vec![], + let writable_root = absolute_path(writable_root); + // The policy is evaluated in the remote container, so use a container path + // for cwd instead of capturing the local test runner cwd. + FileSystemSandboxContext::from_legacy_sandbox_policy( + SandboxPolicy::WorkspaceWrite { + writable_roots: vec![writable_root.clone()], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: vec![], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, }, - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }) + writable_root, + ) } fn assert_normalized_path_rejected(error: &std::io::Error) { diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index 77676548ff..001c500f1b 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -1,11 +1,14 @@ use async_trait::async_trait; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; +use std::path::Path; use tokio::io; #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -103,6 +106,31 @@ impl FileSystemSandboxContext { matches!(file_system_policy.kind, FileSystemSandboxKind::Restricted) && !file_system_policy.has_full_disk_write_access() } + + pub(crate) fn drop_cwd_if_unused(mut self) -> Self { + let file_system_policy = self.permissions.file_system_sandbox_policy(); + if !file_system_policy_has_cwd_dependent_entries(&file_system_policy) { + self.cwd = None; + } + self + } +} + +pub(crate) fn file_system_policy_has_cwd_dependent_entries( + file_system_policy: &FileSystemSandboxPolicy, +) -> bool { + file_system_policy + .entries + .iter() + .any(|entry| match &entry.path { + FileSystemPath::GlobPattern { pattern } => !Path::new(pattern).is_absolute(), + FileSystemPath::Special { + value: + FileSystemSpecialPath::CurrentWorkingDirectory + | FileSystemSpecialPath::ProjectRoots { .. }, + } => true, + FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => false, + }) } pub type FileSystemResult = io::Result; diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 5f347e6e1c..d8261fae14 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -21,6 +21,7 @@ use tokio::process::Command; use crate::ExecServerRuntimePaths; use crate::FileSystemSandboxContext; +use crate::file_system::file_system_policy_has_cwd_dependent_entries; use crate::fs_helper::CODEX_FS_HELPER_ARG1; use crate::fs_helper::FsHelperPayload; use crate::fs_helper::FsHelperRequest; @@ -52,12 +53,12 @@ impl FileSystemSandboxRunner { ) -> Result { let cwd = sandbox_cwd(sandbox)?; let mut file_system_policy = sandbox.permissions.file_system_sandbox_policy(); - let helper_read_root = if sandbox.use_legacy_landlock { - None + let helper_read_roots = if sandbox.use_legacy_landlock { + Vec::new() } else { - helper_read_root(&self.runtime_paths) + helper_read_roots(&self.runtime_paths) }; - add_helper_runtime_permissions(&mut file_system_policy, helper_read_root, cwd.as_path()); + add_helper_runtime_permissions(&mut file_system_policy, &helper_read_roots, cwd.as_path()); normalize_file_system_policy_root_aliases(&mut file_system_policy); let network_policy = NetworkSandboxPolicy::Restricted; let sandbox_policy = @@ -133,33 +134,24 @@ fn sandbox_cwd(sandbox: &FileSystemSandboxContext) -> Result bool { - file_system_policy - .entries - .iter() - .any(|entry| match &entry.path { - FileSystemPath::GlobPattern { pattern } => !std::path::Path::new(pattern).is_absolute(), - FileSystemPath::Special { - value: - FileSystemSpecialPath::CurrentWorkingDirectory - | FileSystemSpecialPath::ProjectRoots { .. }, - } => true, - FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => false, - }) -} - -fn helper_read_root(runtime_paths: &ExecServerRuntimePaths) -> Option { - runtime_paths - .codex_self_exe - .parent() - .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) +fn helper_read_roots(runtime_paths: &ExecServerRuntimePaths) -> Vec { + let mut roots = Vec::new(); + for path in std::iter::once(runtime_paths.codex_self_exe.as_path()) + .chain(runtime_paths.codex_linux_sandbox_exe.as_deref().into_iter()) + { + if let Some(parent) = path.parent() + && let Ok(root) = AbsolutePathBuf::from_absolute_path(parent) + && !roots.contains(&root) + { + roots.push(root); + } + } + roots } fn add_helper_runtime_permissions( file_system_policy: &mut FileSystemSandboxPolicy, - helper_read_root: Option, + helper_read_roots: &[AbsolutePathBuf], cwd: &std::path::Path, ) { if !file_system_policy.has_full_disk_read_access() { @@ -174,19 +166,18 @@ fn add_helper_runtime_permissions( } } - let Some(helper_read_root) = helper_read_root else { - return; - }; - if file_system_policy.can_read_path_with_cwd(helper_read_root.as_path(), cwd) { - return; - } + for helper_read_root in helper_read_roots { + if file_system_policy.can_read_path_with_cwd(helper_read_root.as_path(), cwd) { + continue; + } - file_system_policy.entries.push(FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: helper_read_root, - }, - access: FileSystemAccessMode::Read, - }); + file_system_policy.entries.push(FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: helper_read_root.clone(), + }, + access: FileSystemAccessMode::Read, + }); + } } fn compatibility_sandbox_policy( @@ -371,7 +362,7 @@ mod tests { use super::helper_env; use super::helper_env_from_vars; use super::helper_env_key_is_allowed; - use super::helper_read_root; + use super::helper_read_roots; use super::sandbox_cwd; #[test] @@ -388,7 +379,7 @@ mod tests { let mut policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); - add_helper_runtime_permissions(&mut policy, /*helper_read_root*/ None, cwd.as_path()); + add_helper_runtime_permissions(&mut policy, /*helper_read_roots*/ &[], cwd.as_path()); assert!(policy.include_platform_defaults()); } @@ -410,7 +401,7 @@ mod tests { let mut policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); - add_helper_runtime_permissions(&mut policy, /*helper_read_root*/ None, cwd.as_path()); + add_helper_runtime_permissions(&mut policy, /*helper_read_roots*/ &[], cwd.as_path()); assert!(policy.include_platform_defaults()); } @@ -449,7 +440,7 @@ mod tests { add_helper_runtime_permissions( &mut policy, - helper_read_root(&runtime_paths), + &helper_read_roots(&runtime_paths), cwd.as_path(), ); @@ -611,10 +602,44 @@ mod tests { add_helper_runtime_permissions( &mut policy, - helper_read_root(&runtime_paths), + &helper_read_roots(&runtime_paths), cwd.as_path(), ); assert!(policy.can_read_path_with_cwd(readable.as_path(), cwd.as_path())); } + + #[test] + fn helper_permissions_include_linux_sandbox_alias_parent() { + let root = tempfile::tempdir().expect("temp dir"); + let codex_self_exe = root.path().join("bin").join("codex"); + let codex_linux_sandbox_exe = root.path().join("aliases").join("codex-linux-sandbox"); + let runtime_paths = + ExecServerRuntimePaths::new(codex_self_exe, Some(codex_linux_sandbox_exe)) + .expect("runtime paths"); + let cwd = AbsolutePathBuf::from_absolute_path(std::env::temp_dir().as_path()) + .expect("absolute cwd"); + let sandbox_policy = SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: false, + readable_roots: Vec::new(), + }, + network_access: false, + }; + let mut policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); + let codex_parent = AbsolutePathBuf::from_absolute_path(root.path().join("bin")) + .expect("absolute codex parent"); + let alias_parent = AbsolutePathBuf::from_absolute_path(root.path().join("aliases")) + .expect("absolute alias parent"); + + add_helper_runtime_permissions( + &mut policy, + &helper_read_roots(&runtime_paths), + cwd.as_path(), + ); + + assert!(policy.can_read_path_with_cwd(codex_parent.as_path(), cwd.as_path())); + assert!(policy.can_read_path_with_cwd(alias_parent.as_path(), cwd.as_path())); + } } diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index dc269505a1..fc5cdb01d2 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -50,7 +50,7 @@ impl ExecutorFileSystem for RemoteFileSystem { let response = client .fs_read_file(FsReadFileParams { path: path.clone(), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -74,7 +74,7 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_write_file(FsWriteFileParams { path: path.clone(), data_base64: STANDARD.encode(contents), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -93,7 +93,7 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_create_directory(FsCreateDirectoryParams { path: path.clone(), recursive: Some(options.recursive), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -110,7 +110,7 @@ impl ExecutorFileSystem for RemoteFileSystem { let response = client .fs_get_metadata(FsGetMetadataParams { path: path.clone(), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -133,7 +133,7 @@ impl ExecutorFileSystem for RemoteFileSystem { let response = client .fs_read_directory(FsReadDirectoryParams { path: path.clone(), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -161,7 +161,7 @@ impl ExecutorFileSystem for RemoteFileSystem { path: path.clone(), recursive: Some(options.recursive), force: Some(options.force), - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -182,7 +182,7 @@ impl ExecutorFileSystem for RemoteFileSystem { source_path: source_path.clone(), destination_path: destination_path.clone(), recursive: options.recursive, - sandbox: sandbox.cloned(), + sandbox: remote_sandbox_context(sandbox), }) .await .map_err(map_remote_error)?; @@ -190,6 +190,14 @@ impl ExecutorFileSystem for RemoteFileSystem { } } +fn remote_sandbox_context( + sandbox: Option<&FileSystemSandboxContext>, +) -> Option { + sandbox + .cloned() + .map(FileSystemSandboxContext::drop_cwd_if_unused) +} + fn map_remote_error(error: ExecServerError) -> io::Error { match error { ExecServerError::Server { code, message } if code == NOT_FOUND_ERROR_CODE => { @@ -208,10 +216,58 @@ fn map_remote_error(error: ExecServerError) -> io::Error { #[cfg(test)] mod tests { + use codex_protocol::models::PermissionProfile; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::permissions::FileSystemSpecialPath; + use codex_protocol::permissions::NetworkSandboxPolicy; use pretty_assertions::assert_eq; use super::*; + #[test] + fn remote_sandbox_context_drops_unused_cwd() { + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: absolute_test_path("remote-root"), + }, + access: FileSystemAccessMode::Read, + }]); + let permissions = + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); + let sandbox_context = FileSystemSandboxContext::from_permission_profile_with_cwd( + permissions, + absolute_test_path("host-checkout"), + ); + + let remote_context = + remote_sandbox_context(Some(&sandbox_context)).expect("remote sandbox context"); + + assert_eq!(remote_context.cwd, None); + } + + #[test] + fn remote_sandbox_context_preserves_required_cwd() { + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }]); + let permissions = + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted); + let cwd = absolute_test_path("host-checkout"); + let sandbox_context = + FileSystemSandboxContext::from_permission_profile_with_cwd(permissions, cwd.clone()); + + let remote_context = + remote_sandbox_context(Some(&sandbox_context)).expect("remote sandbox context"); + + assert_eq!(remote_context.cwd, Some(cwd)); + } + #[test] fn transport_errors_map_to_broken_pipe() { let errors = [ @@ -241,4 +297,9 @@ mod tests { ] ); } + + fn absolute_test_path(name: &str) -> AbsolutePathBuf { + let path = std::env::temp_dir().join(name); + AbsolutePathBuf::from_absolute_path(&path).expect("absolute path") + } } diff --git a/codex-rs/exec-server/src/server/handler/tests.rs b/codex-rs/exec-server/src/server/handler/tests.rs index 9d1e4c470c..e4c743fa2b 100644 --- a/codex-rs/exec-server/src/server/handler/tests.rs +++ b/codex-rs/exec-server/src/server/handler/tests.rs @@ -317,7 +317,7 @@ async fn read_process_until_closed( handler: &ExecServerHandler, process_id: ProcessId, ) -> (String, Option) { - let deadline = tokio::time::Instant::now() + Duration::from_secs(2); + let deadline = tokio::time::Instant::now() + Duration::from_secs(5); let mut output = String::new(); let mut exit_code = None; let mut after_seq = None; @@ -346,7 +346,7 @@ async fn read_process_until_closed( after_seq = response.next_seq.checked_sub(1).or(after_seq); assert!( tokio::time::Instant::now() < deadline, - "process should close within 2s" + "process should close within 5s" ); } }