From d0fd4e830a2fe04577ae3098e2738edfd5750133 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Mon, 17 Aug 2026 18:10:35 +0000 Subject: [PATCH] Preserve foreign paths in managed network approvals (#39040) ## Why Unified exec could drop network approval registration when a selected environment used a Windows working directory on a non-Windows host because the trigger path could not be converted to a host absolute path. ## What changed - Keep network approval trigger working directories as `PathUri` values and render them with their inferred native path syntax for Guardian requests. - Fall back to the selected turn environment only when a trigger path can be converted to a host absolute path. - Reject selected environment working directories larger than 8 KiB at the thread-manager validation boundary. ## Testing - Cover managed-network approval registration for a foreign Windows working directory. - Verify that oversized working directories are rejected as invalid `thread/start` requests. GitOrigin-RevId: 646fbf12115a35a7e5cb04b511464797bee6647b --- .../app-server/tests/suite/v2/thread_start.rs | 38 +++++++++++++++++++ .../core/src/guardian/approval_request.rs | 25 +++++++----- codex-rs/core/src/guardian/tests.rs | 10 ++--- codex-rs/core/src/thread_manager.rs | 8 ++++ codex-rs/core/src/tools/network_approval.rs | 7 +++- .../core/src/tools/network_approval_tests.rs | 5 ++- codex-rs/core/src/tools/runtimes/shell.rs | 2 +- .../core/src/tools/runtimes/unified_exec.rs | 2 +- .../suite/unified_exec_process_events.rs | 1 + 9 files changed, 78 insertions(+), 20 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index 5d3135b1a2..eb3e8abedc 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -704,6 +704,44 @@ async fn thread_start_rejects_relative_environment_cwd_as_invalid_request() -> R Ok(()) } +#[tokio::test] +async fn thread_start_rejects_oversized_environment_cwd_as_invalid_request() -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized() + .await?; + let environment_id = mcp.auto_env_params()?.environment_id; + + let request_id = mcp + .send_thread_start_request(ThreadStartParams { + environments: Some(vec![TurnEnvironmentParams { + environment_id, + cwd: serde_json::from_value(json!(format!("C:\\{}", "x".repeat(9_000))))?, + runtime_workspace_roots: None, + }]), + ..Default::default() + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + + assert_eq!(error.id, RequestId::Integer(request_id)); + assert_eq!(error.error.code, INVALID_REQUEST_ERROR_CODE); + assert_eq!( + error.error.message, + "turn environment working directory exceeds the maximum size" + ); + + Ok(()) +} + #[tokio::test] async fn thread_start_response_includes_loaded_instruction_sources() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index bd9a0f7c20..9ccd2426e9 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -7,6 +7,7 @@ use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::request_permissions::RequestPermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; use serde::Serialize; use serde_json::Value; @@ -83,7 +84,7 @@ pub(crate) struct GuardianNetworkAccessTrigger { pub(crate) call_id: String, pub(crate) tool_name: String, pub(crate) command: Vec, - pub(crate) cwd: AbsolutePathBuf, + pub(crate) cwd: PathUri, pub(crate) sandbox_permissions: crate::sandboxing::SandboxPermissions, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) additional_permissions: Option, @@ -330,14 +331,20 @@ pub(crate) fn guardian_approval_request_to_json( protocol, port, trigger, - } => serialize_guardian_action(NetworkAccessApprovalAction { - tool: "network_access", - target, - host, - protocol: *protocol, - port: *port, - trigger: trigger.as_ref(), - }), + } => { + let mut value = serialize_guardian_action(NetworkAccessApprovalAction { + tool: "network_access", + target, + host, + protocol: *protocol, + port: *port, + trigger: trigger.as_ref(), + })?; + if let Some(trigger) = trigger { + value["trigger"]["cwd"] = trigger.cwd.inferred_native_path_string().into(); + } + Ok(value) + } GuardianApprovalRequest::McpToolCall { id: _, server, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index b339e9d4ad..4da79a5eb5 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1207,7 +1207,7 @@ async fn build_guardian_prompt_items_keeps_other_requests_generic() -> anyhow::R #[test] fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_json::Result<()> { - let cwd = test_path_buf("/repo").abs(); + let cwd = PathUri::parse("file:///C:/repo").expect("valid Windows path URI"); let action = GuardianApprovalRequest::NetworkAccess { id: "network-1".to_string(), turn_id: "turn-1".to_string(), @@ -1219,7 +1219,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j call_id: "call-1".to_string(), tool_name: "shell".to_string(), command: vec!["curl".to_string(), "https://example.com".to_string()], - cwd: cwd.clone(), + cwd, sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, justification: Some("Fetch the release metadata.".to_string()), @@ -1239,7 +1239,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j "callId": "call-1", "toolName": "shell", "command": ["curl", "https://example.com"], - "cwd": cwd.to_string_lossy().to_string(), + "cwd": "C:\\repo", "sandboxPermissions": "use_default", "justification": "Fetch the release metadata.", }, @@ -1253,7 +1253,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j async fn build_guardian_prompt_items_explains_network_access_review_scope() -> anyhow::Result<()> { let (session, turn) = guardian_test_session_and_turn_with_base_url("http://localhost").await; seed_guardian_parent_history(&session, &turn).await; - let cwd = test_path_buf("/repo").abs(); + let cwd = PathUri::from_abs_path(&test_path_buf("/repo").abs()); let prompt = build_guardian_prompt_items( session.as_ref(), @@ -1382,7 +1382,7 @@ fn guardian_request_target_item_id_omits_network_access_trigger_call_id() { 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(), + cwd: PathUri::from_abs_path(&test_path_buf("/repo").abs()), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, justification: None, diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index fb8988293a..c01ef5d968 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -98,6 +98,8 @@ use tracing::instrument; use tracing::warn; const THREAD_CREATED_CHANNEL_CAPACITY: usize = 1024; +// Reject pathological selected cwd values at the environment-selection boundary. +const MAX_TURN_ENVIRONMENT_CWD_BYTES: usize = 8 * 1024; /// Test-only override for enabling thread-manager behaviors used by integration /// tests. @@ -724,6 +726,12 @@ impl ThreadManager { ) -> CodexResult<()> { let mut environment_ids = HashSet::with_capacity(environments.len()); for environment in environments { + if environment.cwd.inferred_native_path_string().len() > MAX_TURN_ENVIRONMENT_CWD_BYTES + { + return Err(CodexErr::InvalidRequest( + "turn environment working directory exceeds the maximum size".to_string(), + )); + } if !environment_ids.insert(environment.environment_id.as_str()) { return Err(CodexErr::InvalidRequest(format!( "duplicate turn environment id `{}`", diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index c41f863592..fdb3f6ef0b 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -699,8 +699,11 @@ impl NetworkApprovalService { let command = owner_call .as_ref() .map_or_else(|| prompt_command.join(" "), |call| call.command.clone()); - let cwd = if let Some(owner_call) = owner_call.as_ref() { - owner_call.trigger.cwd.clone() + let cwd = if let Some(cwd) = owner_call + .as_ref() + .and_then(|owner_call| owner_call.trigger.cwd.to_abs_path().ok()) + { + cwd } else { turn_context .environments diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index b83e972485..26ac89712a 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -4,6 +4,7 @@ use codex_network_proxy::BlockedRequestArgs; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_utils_path_uri::PathUri; use core_test_support::PathBufExt; use core_test_support::test_path_buf; use futures::poll; @@ -481,7 +482,7 @@ async fn register_call_with_default_shell_trigger( call_id: "call-1".to_string(), tool_name: "shell_command".to_string(), command: vec!["curl".to_string(), "https://example.com".to_string()], - cwd: test_path_buf("/tmp").abs(), + cwd: PathUri::from_abs_path(&test_path_buf("/tmp").abs()), sandbox_permissions: SandboxPermissions::UseDefault, additional_permissions: None, justification: None, @@ -503,7 +504,7 @@ async fn active_call_preserves_triggering_command_context() { call_id: "call-1".to_string(), tool_name: "shell_command".to_string(), command: vec!["curl".to_string(), "https://example.com".to_string()], - cwd: test_path_buf("/repo").abs(), + cwd: PathUri::parse("file:///C:/repo").expect("valid Windows path URI"), sandbox_permissions: SandboxPermissions::UseDefault, additional_permissions: None, justification: Some("fetch release metadata".to_string()), diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 95c44c7382..58fc9c4021 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -179,7 +179,7 @@ impl ToolRuntime for ShellRuntime { call_id: ctx.call_id.clone(), tool_name: flat_tool_name(&ctx.tool_name).into_owned(), command: req.command.clone(), - cwd: req.cwd.clone(), + cwd: PathUri::from_abs_path(&req.cwd), sandbox_permissions: req.sandbox_permissions, additional_permissions: req.additional_permissions.clone(), justification: req.justification.clone(), diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 099dbd49b5..836142a495 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -230,7 +230,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt call_id: ctx.call_id.clone(), tool_name: flat_tool_name(&ctx.tool_name).into_owned(), command: req.command.clone(), - cwd: req.cwd.to_abs_path().ok()?, + cwd: req.cwd.clone(), sandbox_permissions: req.sandbox_permissions, additional_permissions: req.additional_permissions.clone(), justification: req.justification.clone(), diff --git a/codex-rs/core/tests/suite/unified_exec_process_events.rs b/codex-rs/core/tests/suite/unified_exec_process_events.rs index 952f4c5b36..e91c6ee7f1 100644 --- a/codex-rs/core/tests/suite/unified_exec_process_events.rs +++ b/codex-rs/core/tests/suite/unified_exec_process_events.rs @@ -539,6 +539,7 @@ async fn serve_exec_with_pushed_events( #[test_case(PushedExecScenario::ReplayGap, false, false, false ; "truncated_event_replay")] #[test_case(PushedExecScenario::Complete, true, true, false ; "managed_network_uses_executor_proxy_launch")] #[test_case(PushedExecScenario::Complete, true, false, false ; "strict_managed_allowlist_omits_policy_callbacks")] +#[cfg_attr(not(windows), test_case(PushedExecScenario::Complete, true, true, true ; "foreign_windows_managed_network_preserves_approval_registration"))] #[cfg_attr(not(windows), test_case(PushedExecScenario::Complete, false, false, true ; "foreign_windows_workspace_sandbox"))] #[test_case(PushedExecScenario::ElevatedPowerShell, false, false, true ; "windows_elevated_powershell_disables_profile")] #[cfg_attr(not(windows), test_case(PushedExecScenario::SandboxedInterceptedPatch, false, false, true ; "foreign_windows_intercepted_patch_is_sandboxed"))]