diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index b16d931e57..68a1862a50 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -119,7 +119,6 @@ use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequest use codex_protocol::request_permissions::RequestPermissionsResponse as CoreRequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputAnswer as CoreRequestUserInputAnswer; use codex_protocol::request_user_input::RequestUserInputResponse as CoreRequestUserInputResponse; -use codex_sandboxing::policy_transforms::intersect_permission_profiles; use codex_shell_command::parse_command::shlex_join; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::LegacyAppPathString; @@ -930,8 +929,7 @@ pub(crate) async fn apply_bespoke_event_handling( let permission_guard = thread_watch_manager .note_permission_requested(&conversation_id.to_string()) .await; - let requested_permissions = request.permissions.clone(); - let request_cwd = match request.cwd.clone() { + let request_cwd = match request.cwd { Some(cwd) => cwd, None => conversation.config_snapshot().await.cwd().clone(), }; @@ -941,7 +939,7 @@ pub(crate) async fn apply_bespoke_event_handling( item_id: request.call_id.clone(), environment_id: request.environment_id.clone(), started_at_ms: request.started_at_ms, - cwd: request_cwd.clone(), + cwd: request_cwd, reason: request.reason, permissions: request.permissions.into(), }; @@ -952,8 +950,6 @@ pub(crate) async fn apply_bespoke_event_handling( call_id: request.call_id, conversation_id, turn_id: request.turn_id, - requested_permissions, - request_cwd, pending_request_id, outgoing, receiver: rx, @@ -1887,8 +1883,6 @@ async fn on_request_permissions_response( call_id, conversation_id, turn_id, - requested_permissions, - request_cwd, pending_request_id, outgoing, receiver, @@ -1897,11 +1891,7 @@ async fn on_request_permissions_response( let response = receiver.await; resolve_server_request_on_thread_listener(&thread_state, pending_request_id.clone()).await; drop(request_permissions_guard); - let response = match request_permissions_response_from_client_result( - requested_permissions, - response, - request_cwd.as_path(), - ) { + let response = match request_permissions_response_from_client_result(response) { Ok(Some(response)) => response, Ok(None) => return, // TODO(anp): Remove this native-path localization error path once core permission paths @@ -1944,8 +1934,6 @@ struct PendingRequestPermissionsResponse { call_id: String, conversation_id: ThreadId, turn_id: String, - requested_permissions: CoreRequestPermissionProfile, - request_cwd: AbsolutePathBuf, pending_request_id: RequestId, outgoing: ThreadScopedOutgoingMessageSender, receiver: oneshot::Receiver, @@ -1953,9 +1941,7 @@ struct PendingRequestPermissionsResponse { } fn request_permissions_response_from_client_result( - requested_permissions: CoreRequestPermissionProfile, response: std::result::Result, - cwd: &std::path::Path, ) -> std::io::Result> { let value = match response { Ok(Ok(value)) => value, @@ -2002,13 +1988,9 @@ fn request_permissions_response_from_client_result( })); } let granted_permissions: CoreAdditionalPermissionProfile = response.permissions.try_into()?; - let permissions = if granted_permissions.is_empty() { - CoreRequestPermissionProfile::default() - } else { - intersect_permission_profiles(requested_permissions.into(), granted_permissions, cwd).into() - }; + // Core intersects with the request using the originating environment's policy context. Ok(Some(CoreRequestPermissionsResponse { - permissions, + permissions: CoreRequestPermissionProfile::from(granted_permissions), scope: response.scope.to_core(), strict_auto_review, })) @@ -2229,10 +2211,6 @@ mod tests { use codex_protocol::models::FileSystemPermissions as CoreFileSystemPermissions; use codex_protocol::models::NetworkPermissions as CoreNetworkPermissions; use codex_protocol::models::PermissionProfile; - use codex_protocol::permissions::FileSystemAccessMode; - use codex_protocol::permissions::FileSystemPath; - use codex_protocol::permissions::FileSystemSandboxEntry; - use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::protocol::AgentMessageEvent; @@ -3058,18 +3036,14 @@ mod tests { data: Some(serde_json::json!({ "reason": "turnTransition" })), }; - let response = request_permissions_response_from_client_result( - CoreRequestPermissionProfile::default(), - Ok(Err(error)), - std::env::current_dir().expect("current dir").as_path(), - ) - .expect("paths should localize"); + let response = request_permissions_response_from_client_result(Ok(Err(error))) + .expect("paths should localize"); assert_eq!(response, None); } #[test] - fn request_permissions_response_accepts_partial_network_and_file_system_grants() { + fn request_permissions_response_deserializes_network_and_file_system_grants() { let input_path = if cfg!(target_os = "windows") { r"C:\tmp\input" } else { @@ -3080,23 +3054,14 @@ mod tests { } else { "/tmp/output" }; - let ignored_path = if cfg!(target_os = "windows") { - r"C:\tmp\ignored" + let extra_path = if cfg!(target_os = "windows") { + r"C:\tmp\extra" } else { - "/tmp/ignored" + "/tmp/extra" }; let absolute_path = |path: &str| { AbsolutePathBuf::try_from(std::path::PathBuf::from(path)).expect("absolute path") }; - let requested_permissions = CoreRequestPermissionProfile { - network: Some(CoreNetworkPermissions { - enabled: Some(true), - }), - file_system: Some(CoreFileSystemPermissions::from_read_write_roots( - Some(vec![absolute_path(input_path)]), - Some(vec![absolute_path(output_path)]), - )), - }; let cases = vec![ ( serde_json::json!({}), @@ -3133,7 +3098,7 @@ mod tests { serde_json::json!({ "fileSystem": { "read": [input_path], - "write": [output_path, ignored_path], + "write": [output_path, extra_path], }, "macos": { "calendar": true, @@ -3142,24 +3107,20 @@ mod tests { CoreRequestPermissionProfile { file_system: Some(CoreFileSystemPermissions::from_read_write_roots( Some(vec![absolute_path(input_path)]), - Some(vec![absolute_path(output_path)]), + Some(vec![absolute_path(output_path), absolute_path(extra_path)]), )), ..CoreRequestPermissionProfile::default() }, ), ]; - let cwd = std::env::current_dir().expect("current dir"); for (granted_permissions, expected_permissions) in cases { - let response = request_permissions_response_from_client_result( - requested_permissions.clone(), - Ok(Ok(serde_json::json!({ + let response = + request_permissions_response_from_client_result(Ok(Ok(serde_json::json!({ "permissions": granted_permissions, - }))), - cwd.as_path(), - ) - .expect("paths should localize") - .expect("response should be accepted"); + })))) + .expect("paths should localize") + .expect("response should be accepted"); assert_eq!( response, @@ -3174,14 +3135,10 @@ mod tests { #[test] fn request_permissions_response_preserves_session_scope() { - let response = request_permissions_response_from_client_result( - CoreRequestPermissionProfile::default(), - Ok(Ok(serde_json::json!({ - "scope": "session", - "permissions": {}, - }))), - std::env::current_dir().expect("current dir").as_path(), - ) + let response = request_permissions_response_from_client_result(Ok(Ok(serde_json::json!({ + "scope": "session", + "permissions": {}, + })))) .expect("paths should localize") .expect("response should be accepted"); @@ -3197,19 +3154,15 @@ mod tests { #[test] fn request_permissions_response_rejects_session_scoped_strict_auto_review() { - let response = request_permissions_response_from_client_result( - CoreRequestPermissionProfile::default(), - Ok(Ok(serde_json::json!({ - "scope": "session", - "strictAutoReview": true, - "permissions": { - "network": { - "enabled": true, - }, + let response = request_permissions_response_from_client_result(Ok(Ok(serde_json::json!({ + "scope": "session", + "strictAutoReview": true, + "permissions": { + "network": { + "enabled": true, }, - }))), - std::env::current_dir().expect("current dir").as_path(), - ) + }, + })))) .expect("paths should localize") .expect("response should be accepted"); @@ -3225,23 +3178,14 @@ mod tests { #[test] fn request_permissions_response_preserves_turn_scoped_strict_auto_review() { - let response = request_permissions_response_from_client_result( - CoreRequestPermissionProfile { - network: Some(codex_protocol::models::NetworkPermissions { - enabled: Some(true), - }), - ..Default::default() - }, - Ok(Ok(serde_json::json!({ - "strictAutoReview": true, - "permissions": { - "network": { - "enabled": true, - }, + let response = request_permissions_response_from_client_result(Ok(Ok(serde_json::json!({ + "strictAutoReview": true, + "permissions": { + "network": { + "enabled": true, }, - }))), - std::env::current_dir().expect("current dir").as_path(), - ) + }, + })))) .expect("paths should localize") .expect("response should be accepted"); @@ -3249,135 +3193,6 @@ mod tests { assert!(response.strict_auto_review); } - #[test] - fn request_permissions_response_accepts_explicit_child_grant_for_requested_cwd_scope() { - let temp_dir = TempDir::new().expect("temp dir"); - let cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute cwd"); - let child = cwd.join("child"); - let requested_permissions = CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions { - entries: vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::project_roots(/*subpath*/ None), - }, - access: FileSystemAccessMode::Write, - missing_path_behavior: None, - }], - glob_scan_max_depth: None, - }), - ..Default::default() - }; - - let response = request_permissions_response_from_client_result( - requested_permissions, - Ok(Ok(serde_json::json!({ - "permissions": { - "fileSystem": { - "write": [child], - }, - }, - }))), - cwd.as_path(), - ) - .expect("paths should localize") - .expect("response should be accepted"); - - assert_eq!( - response.permissions, - CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions::from_read_write_roots( - /*read*/ None, - Some(vec![child]), - )), - ..Default::default() - } - ); - } - - #[test] - fn request_permissions_response_rejects_child_grant_outside_requested_cwd_scope() { - let temp_dir = TempDir::new().expect("temp dir"); - let request_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("request-cwd")) - .expect("absolute request cwd"); - let later_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("later-cwd")) - .expect("absolute later cwd"); - let later_child = later_cwd.join("child"); - let requested_permissions = CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions { - entries: vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::project_roots(/*subpath*/ None), - }, - access: FileSystemAccessMode::Write, - missing_path_behavior: None, - }], - glob_scan_max_depth: None, - }), - ..Default::default() - }; - - let response = request_permissions_response_from_client_result( - requested_permissions, - Ok(Ok(serde_json::json!({ - "permissions": { - "fileSystem": { - "write": [later_child], - }, - }, - }))), - request_cwd.as_path(), - ) - .expect("paths should localize") - .expect("response should be accepted"); - - assert_eq!( - response.permissions, - CoreRequestPermissionProfile::default() - ); - } - - #[test] - fn request_permissions_response_ignores_broader_cwd_grant_for_requested_child_path() { - let temp_dir = TempDir::new().expect("temp dir"); - let cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute cwd"); - let child = cwd.join("child"); - let requested_permissions = CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions::from_read_write_roots( - /*read*/ None, - Some(vec![child]), - )), - ..Default::default() - }; - - let response = request_permissions_response_from_client_result( - requested_permissions, - Ok(Ok(serde_json::json!({ - "permissions": { - "fileSystem": { - "entries": [{ - "path": { - "type": "special", - "value": { - "kind": "project_roots", - "subpath": null - } - }, - "access": "write" - }], - }, - }, - }))), - cwd.as_path(), - ) - .expect("paths should localize") - .expect("response should be accepted"); - - assert_eq!( - response.permissions, - CoreRequestPermissionProfile::default() - ); - } - #[tokio::test] async fn test_handle_error_records_message() -> Result<()> { let conversation_id = ThreadId::new(); diff --git a/codex-rs/app-server/tests/suite/v2/request_permissions.rs b/codex-rs/app-server/tests/suite/v2/request_permissions.rs index 93cb18cae7..ab5d874671 100644 --- a/codex-rs/app-server/tests/suite/v2/request_permissions.rs +++ b/codex-rs/app-server/tests/suite/v2/request_permissions.rs @@ -2,7 +2,6 @@ use anyhow::Result; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; use app_test_support::create_final_assistant_message_sse_response; -use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_request_permissions_sse_response; use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::JSONRPCMessage; @@ -16,7 +15,15 @@ use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::UserInput as V2UserInput; use codex_features::Feature; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::request_permissions::PermissionGrantScope as CorePermissionGrantScope; +use codex_protocol::request_permissions::RequestPermissionProfile; +use codex_protocol::request_permissions::RequestPermissionsResponse; +use codex_utils_path_uri::LegacyAppPathString; +use core_test_support::responses; use core_test_support::skip_if_wine_exec; +use pretty_assertions::assert_eq; +use serde_json::json; use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); @@ -30,11 +37,31 @@ async fn request_permissions_round_trip() -> Result<()> { ); let codex_home = tempfile::TempDir::new()?; + let project_root_entry = json!({ + "path": { + "type": "special", + "value": {"kind": "project_roots", "subpath": "output"} + }, + "access": "write" + }); let responses = vec![ create_request_permissions_sse_response("call1")?, + responses::sse(vec![ + responses::ev_response_created("resp-2"), + responses::ev_function_call( + "call2", + "request_permissions", + &json!({ + "permissions": {"file_system": {"entries": [project_root_entry]}} + }) + .to_string(), + ), + responses::ev_completed("resp-2"), + ]), create_final_assistant_message_sse_response("done")?, ]; - let server = create_mock_responses_server_sequence(responses).await; + let server = responses::start_mock_server().await; + let mock = responses::mount_sse_sequence(&server, responses).await; MockResponsesConfig::new(&server.uri()) .with_approval_policy("on-request") .enable_feature(Feature::RequestPermissionsTool) @@ -44,6 +71,10 @@ async fn request_permissions_round_trip() -> Result<()> { .with_codex_home(codex_home.path()) .build_initialized() .await?; + let cwd = mcp.auto_env()?.selection().cwd.clone(); + let workspace_root = cwd.parent().expect("test cwd has a parent"); + let mut environment = mcp.auto_env_params()?; + environment.runtime_workspace_roots = Some(vec![workspace_root.clone().into()]); let ThreadStartResponse { thread, .. } = mcp .start_thread(ThreadStartParams { @@ -58,6 +89,7 @@ async fn request_permissions_round_trip() -> Result<()> { params: TurnStartParams { thread_id: thread.id.clone(), client_user_message_id: None, + environments: Some(vec![environment]), input: vec![V2UserInput::Text { text: "pick a directory".to_string(), text_elements: Vec::new(), @@ -108,8 +140,6 @@ async fn request_permissions_round_trip() -> Result<()> { }, ]) ); - let resolved_request_id = request_id.clone(); - mcp.send_response( request_id, serde_json::to_value(PermissionsRequestApprovalResponse { @@ -128,6 +158,28 @@ async fn request_permissions_round_trip() -> Result<()> { ) .await?; + let server_req = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::PermissionsRequestApproval { request_id, params } = server_req else { + panic!("expected PermissionsRequestApproval request, got: {server_req:?}"); + }; + assert_eq!(params.item_id, "call2"); + let resolved_request_id = request_id.clone(); + let outside_request: LegacyAppPathString = cwd.join("output")?.into(); + mcp.send_response( + request_id, + json!({ + "permissions": {"fileSystem": {"entries": [ + project_root_entry, + {"path": {"type": "path", "path": outside_request}, "access": "write"} + ]}} + }), + ) + .await?; + let mut saw_resolved = false; loop { let message = timeout(DEFAULT_READ_TIMEOUT, mcp.read_next_message()).await??; @@ -143,8 +195,9 @@ async fn request_permissions_round_trip() -> Result<()> { .expect("serverRequest/resolved params"), )?; assert_eq!(resolved.thread_id, thread.id); - assert_eq!(resolved.request_id, resolved_request_id); - saw_resolved = true; + if resolved.request_id == resolved_request_id { + saw_resolved = true; + } } "turn/completed" => { assert!(saw_resolved, "serverRequest/resolved should arrive first"); @@ -154,5 +207,25 @@ async fn request_permissions_round_trip() -> Result<()> { } } + let (output, _) = mock.requests()[2] + .function_call_output_content_and_success("call2") + .expect("permission tool output"); + let response: RequestPermissionsResponse = + serde_json::from_str(&output.expect("permission response text"))?; + assert_eq!( + response, + RequestPermissionsResponse { + permissions: RequestPermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_path_uris( + /*read*/ None, + Some(vec![workspace_root.join("output")?]), + )), + ..Default::default() + }, + scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, + } + ); + Ok(()) } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index a6603e99f0..15be7e485d 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2,7 +2,6 @@ use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; use std::fmt::Debug; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; @@ -121,6 +120,7 @@ use codex_protocol::models::SandboxEnforcement; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::HasLegacyEvent; @@ -156,7 +156,7 @@ use codex_rollout::state_db; use codex_rollout_trace::AgentResultTracePayload; use codex_rollout_trace::ThreadStartedTraceMetadata; use codex_rollout_trace::ThreadTraceContext; -use codex_sandboxing::policy_transforms::intersect_permission_profiles; +use codex_sandboxing::policy_transforms::intersect_permission_profiles_with_context; use codex_shell_command::parse_command::parse_command; use codex_terminal_detection::user_agent; use codex_thread_store::CreateThreadParams; @@ -2757,12 +2757,27 @@ impl Session { step_context: &Arc, call_id: String, args: RequestPermissionsArgs, - environment: TurnEnvironmentSelection, + environment_selection: TurnEnvironmentSelection, cancellation_token: CancellationToken, ) -> Option { let turn_context = &step_context.turn; let approval_policy = step_context.settings.approval_policy(); let approvals_reviewer = step_context.settings.approvals_reviewer(); + let Some(environment) = step_context + .environments + .turn_environments() + .find(|candidate| { + candidate.selection.environment_id == environment_selection.environment_id + }) + .cloned() + else { + return Some(RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }); + }; + let cwd = environment.cwd(); match approval_policy { AskForApproval::Never => { return Some(RequestPermissionsResponse { @@ -2786,19 +2801,14 @@ impl Session { } let requested_permissions = args.permissions; - // TODO(anp): Migrate request_permissions to support paths from foreign environments. - let Ok(native_environment_cwd) = environment.cwd.to_abs_path() else { - warn!( - cwd = %environment.cwd, - "request_permissions requires a cwd native to the Codex host" - ); + let sandbox_context = environment.sandbox_context(/*additional_permissions*/ None); + let Some(context) = sandbox_context.policy_context() else { return Some(RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, strict_auto_review: false, }); }; - if crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) { let originating_turn_state = { @@ -2868,17 +2878,30 @@ impl Session { let response = Self::normalize_request_permissions_response( requested_permissions, response, - native_environment_cwd.as_path(), + &context, ); self.record_granted_request_permissions_for_turn( &response, - &environment.environment_id, + &environment.selection.environment_id, originating_turn_state.as_ref(), ) .await; return Some(response); } + // The interactive approval event remains host-native until its public + // app-server/TUI boundary migrates in the next stack stage. + let Ok(native_cwd) = cwd.to_abs_path() else { + warn!( + cwd = %cwd, + "request_permissions interactive approval requires a cwd native to the Codex host" + ); + return Some(RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }); + }; let _elicitation = self.services.elicitations.register(); let (tx_response, rx_response) = oneshot::channel(); let prev_entry = { @@ -2905,11 +2928,11 @@ impl Session { let event = EventMsg::RequestPermissions(RequestPermissionsEvent { call_id: call_id.clone(), turn_id: turn_context.sub_id.clone(), - environment_id: Some(environment.environment_id.clone()), + environment_id: Some(environment.selection.environment_id.clone()), started_at_ms: now_unix_timestamp_ms(), reason: args.reason, permissions: requested_permissions, - cwd: Some(native_environment_cwd), + cwd: Some(native_cwd), }); self.send_event(turn_context.as_ref(), event).await; tokio::select! { @@ -3020,29 +3043,25 @@ impl Session { }; match entry { Some(entry) => { - // TODO(anp): Migrate request_permissions to support paths from foreign environments. - let response = match entry.environment.cwd.to_abs_path() { - Ok(native_environment_cwd) => Self::normalize_request_permissions_response( + let sandbox_context = entry + .environment + .sandbox_context(/*additional_permissions*/ None); + let response = if let Some(context) = sandbox_context.policy_context() { + Self::normalize_request_permissions_response( entry.requested_permissions, response, - native_environment_cwd.as_path(), - ), - Err(err) => { - warn!( - cwd = %entry.environment.cwd, - %err, - "request_permissions requires a cwd native to the Codex host" - ); - RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - } + &context, + ) + } else { + RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, } }; self.record_granted_request_permissions_for_turn( &response, - &entry.environment.environment_id, + &entry.environment.selection.environment_id, originating_turn_state.as_ref(), ) .await; @@ -3057,7 +3076,7 @@ impl Session { fn normalize_request_permissions_response( requested_permissions: RequestPermissionProfile, response: RequestPermissionsResponse, - cwd: &Path, + context: &FileSystemSandboxPolicyContext<'_>, ) -> RequestPermissionsResponse { if response.strict_auto_review && matches!(response.scope, PermissionGrantScope::Session) { return RequestPermissionsResponse { @@ -3072,10 +3091,10 @@ impl Session { } RequestPermissionsResponse { - permissions: intersect_permission_profiles( + permissions: intersect_permission_profiles_with_context( requested_permissions.into(), response.permissions.into(), - cwd, + context, ) .into(), scope: response.scope, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 4113d14aec..b5931b330e 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -79,6 +79,7 @@ use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::protocol::EnvironmentConfigState; use codex_protocol::protocol::ErrorEvent; @@ -7244,6 +7245,13 @@ fn strict_auto_review_session_scope_grants_no_permissions() { ..RequestPermissionProfile::default() }; + let cwd = PathUri::parse("file:///tmp").expect("test cwd should be valid"); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: &[], + user_home_dir: None, + temporary_directories: None, + }; let response = Session::normalize_request_permissions_response( requested_permissions.clone(), codex_protocol::request_permissions::RequestPermissionsResponse { @@ -7251,7 +7259,7 @@ fn strict_auto_review_session_scope_grants_no_permissions() { scope: PermissionGrantScope::Session, strict_auto_review: true, }, - std::path::Path::new("/tmp"), + &context, ); assert_eq!( @@ -7355,7 +7363,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() } #[tokio::test] -async fn request_permissions_tool_resolves_relative_paths_against_selected_environment() { +async fn request_permissions_tool_resolves_legacy_paths_against_selected_environment() { let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; *session.active_turn.lock().await = Some(ActiveTurn::default()); let environment_cwd = { @@ -7363,6 +7371,7 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir let legacy_cwd = turn_context.cwd.clone(); legacy_cwd.join("request-permissions-environment") }; + let environment_home = environment_cwd.join("home"); std::fs::create_dir_all(environment_cwd.as_path()).expect("create environment cwd"); let turn_context_mut = Arc::get_mut(&mut turn_context).expect("single thread settings ref"); Arc::make_mut(&mut turn_context_mut.config) @@ -7382,8 +7391,9 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir .expect("primary environment") .clone(); let current_environment_config = current_environment.config().clone(); - turn_context_mut.environments.environments[0] = - TurnEnvironmentState::Ready(TurnEnvironment::new( + let environment = TurnEnvironment { + user_home_dir: Some(PathUri::from_abs_path(&environment_home)), + ..TurnEnvironment::new( TurnEnvironmentSelection { environment_id: "remote".to_string(), cwd: PathUri::from_abs_path(&environment_cwd), @@ -7393,7 +7403,9 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir current_environment.config_origin, current_environment.environment, current_environment.shell, - )); + ) + }; + turn_context_mut.environments.environments[0] = TurnEnvironmentState::Ready(environment); let call_id = "call-1".to_string(); let handler = RequestPermissionsHandler; @@ -7422,13 +7434,8 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir "reason": "need write", "permissions": { "file_system": { - "entries": [{ - "path": { - "type": "path", - "path": "relative.txt", - }, - "access": "write", - }], + "read": null, + "write": ["relative.txt", "~/home-relative.txt"], }, }, }) @@ -7447,16 +7454,13 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir panic!("expected request_permissions event"); }; let expected_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - entries: vec![FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: environment_cwd.join("relative.txt").into(), - }, - access: FileSystemAccessMode::Write, - missing_path_behavior: None, - }], - glob_scan_max_depth: None, - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + /*read*/ None, + Some(vec![ + environment_cwd.join("relative.txt"), + environment_home.join("home-relative.txt"), + ]), + )), ..Default::default() }; assert_eq!(request.environment_id.as_deref(), Some("remote")); @@ -7479,9 +7483,22 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir .expect("request_permissions handler should succeed"); } +#[test_case("missing", "unknown turn environment id `missing`"; "unknown environment")] +#[test_case("local", "permission path cannot be represented losslessly"; "lossy path")] #[tokio::test] -async fn request_permissions_tool_rejects_unknown_environment_id() { - let (session, turn_context) = make_session_and_context().await; +async fn request_permissions_tool_rejects_invalid_requests( + environment_id: &str, + expected_error: &str, +) { + let (session, mut turn_context) = make_session_and_context().await; + Arc::make_mut(&mut turn_context.config) + .permissions + .approval_policy = codex_config::Constrained::allow_any(AskForApproval::Never); + let TurnEnvironmentState::Ready(environment) = &mut turn_context.environments.environments[0] + else { + panic!("turn environment should be ready"); + }; + environment.selection.cwd = PathUri::parse("file:///workspace/%FF").expect("non-UTF8 cwd"); let turn_context = Arc::new(turn_context); let step_context = StepContext::for_test(Arc::clone(&turn_context)); let result = RequestPermissionsHandler @@ -7496,10 +7513,10 @@ async fn request_permissions_tool_rejects_unknown_environment_id() { source: ToolCallSource::Direct, payload: ToolPayload::Function { arguments: json!({ - "environment_id": "missing", + "environment_id": environment_id, "permissions": { - "network": { - "enabled": true, + "file_system": { + "write": ["relative"], }, }, }) @@ -7509,9 +7526,9 @@ async fn request_permissions_tool_rejects_unknown_environment_id() { .await; let Err(FunctionCallError::RespondToModel(output)) = result else { - panic!("expected unknown environment id to be rejected"); + panic!("expected invalid request to be rejected"); }; - assert_eq!(output, "unknown turn environment id `missing`"); + assert_eq!(output, expected_error); } #[tokio::test] diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index fdd1612015..c61e6431e9 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -9,7 +9,6 @@ use tokio_util::task::AbortOnDropHandle; use codex_diagnostics::GaugeGuard; use codex_protocol::dynamic_tools::DynamicToolResponse; -use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputResponse; @@ -23,6 +22,7 @@ use crate::mcp_tool_call::McpToolApprovalMetadata; use crate::session::TurnInputQueue; use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; +use crate::session::turn_context::TurnEnvironment; use crate::tasks::AnySessionTask; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::McpInvocation; @@ -109,7 +109,7 @@ pub(crate) struct TurnState { pub(crate) struct PendingRequestPermissions { pub(crate) tx_response: oneshot::Sender, pub(crate) requested_permissions: RequestPermissionProfile, - pub(crate) environment: TurnEnvironmentSelection, + pub(crate) environment: TurnEnvironment, } impl TurnState { diff --git a/codex-rs/core/src/tools/handlers/request_permissions.rs b/codex-rs/core/src/tools/handlers/request_permissions.rs index fc28c80d81..bb3a7de75d 100644 --- a/codex-rs/core/src/tools/handlers/request_permissions.rs +++ b/codex-rs/core/src/tools/handlers/request_permissions.rs @@ -1,5 +1,8 @@ +use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::request_permissions::RequestPermissionsArgs; -use codex_sandboxing::policy_transforms::normalize_additional_permissions; +use codex_sandboxing::policy_transforms::normalize_additional_permissions_with_context; +use codex_utils_path_uri::LegacyAppPathString; +use serde_json::Value; use crate::function_tool::FunctionCallError; use crate::tools::context::FunctionToolOutput; @@ -7,7 +10,6 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::handlers::parse_arguments; -use crate::tools::handlers::parse_arguments_with_base_path; use crate::tools::handlers::resolve_tool_environment; use crate::tools::handlers::shell_spec::create_request_permissions_tool; use crate::tools::handlers::shell_spec::request_permissions_tool_description; @@ -75,19 +77,25 @@ impl RequestPermissionsHandler { "request_permissions requires a primary environment".to_string(), )); }; - // TODO(anp): Migrate request_permissions parsing and permission profiles to PathUri so - // environment-native foreign paths do not require host conversion. - let native_cwd = turn_environment.cwd().to_abs_path().map_err(|err| { - FunctionCallError::RespondToModel(format!( - "request_permissions cwd `{}` is not native to the Codex host: {err}", - turn_environment.cwd() - )) + let sandbox_context = + turn_environment.sandbox_context(/*additional_permissions*/ None); + let context = sandbox_context.policy_context().ok_or_else(|| { + FunctionCallError::RespondToModel( + "request_permissions requires an executor cwd".to_string(), + ) })?; + let mut arguments: Value = parse_arguments(&arguments)?; + resolve_permission_path_strings(&mut arguments, &context)?; let mut args: RequestPermissionsArgs = - parse_arguments_with_base_path(&arguments, &native_cwd)?; - args.permissions = normalize_additional_permissions(args.permissions.into()) - .map(codex_protocol::request_permissions::RequestPermissionProfile::from) - .map_err(FunctionCallError::RespondToModel)?; + serde_json::from_value(arguments).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to parse function arguments: {err}" + )) + })?; + args.permissions = + normalize_additional_permissions_with_context(args.permissions.into(), &context) + .map(codex_protocol::request_permissions::RequestPermissionProfile::from) + .map_err(FunctionCallError::RespondToModel)?; if args.permissions.is_empty() { return Err(FunctionCallError::RespondToModel( "request_permissions requires at least one permission".to_string(), @@ -123,3 +131,83 @@ impl RequestPermissionsHandler { } impl CoreToolRuntime for RequestPermissionsHandler {} + +fn resolve_permission_path_strings( + arguments: &mut Value, + context: &FileSystemSandboxPolicyContext<'_>, +) -> Result<(), FunctionCallError> { + let Some(file_system) = arguments + .pointer_mut("/permissions/file_system") + .and_then(Value::as_object_mut) + else { + return Ok(()); + }; + let mut legacy_entries = Vec::new(); + for (field, access) in [("read", "read"), ("write", "write")] { + if let Some(paths) = file_system.remove(field) { + if paths.is_null() { + continue; + } + let Value::Array(paths) = paths else { + file_system.insert(field.to_string(), paths); + continue; + }; + for mut path in paths { + resolve_permission_path_string(&mut path, context)?; + legacy_entries.push(serde_json::json!({ + "path": {"type": "path", "path": path}, + "access": access, + })); + } + } + } + if !legacy_entries.is_empty() { + file_system + .entry("entries".to_string()) + .or_insert_with(|| Value::Array(Vec::new())) + .as_array_mut() + .ok_or_else(|| { + FunctionCallError::RespondToModel( + "request_permissions file_system.entries must be an array".to_string(), + ) + })? + .extend(legacy_entries); + } + if let Some(entries) = file_system.get_mut("entries").and_then(Value::as_array_mut) { + for entry in entries { + if entry.pointer("/path/type").and_then(Value::as_str) == Some("path") + && let Some(path) = entry.pointer_mut("/path/path") + { + resolve_permission_path_string(path, context)?; + } + } + } + Ok(()) +} + +fn resolve_permission_path_string( + value: &mut Value, + context: &FileSystemSandboxPolicyContext<'_>, +) -> Result<(), FunctionCallError> { + let Some(path) = value.as_str() else { + return Ok(()); + }; + let cwd = context.cwd; + let convention = cwd.infer_path_convention().ok_or_else(|| { + FunctionCallError::RespondToModel(format!( + "request_permissions cwd `{cwd}` has no path convention" + )) + })?; + let path = LegacyAppPathString::from_string(path) + .resolve_against(cwd, context.user_home_dir) + .map_err(|err| FunctionCallError::RespondToModel(err.to_string()))?; + if path.is_opaque() || std::str::from_utf8(&path.decoded_path_bytes()).is_err() { + return Err(FunctionCallError::RespondToModel( + "permission path cannot be represented losslessly".to_string(), + )); + } + let path = LegacyAppPathString::from_path_uri(&path, convention) + .map_err(|err| FunctionCallError::RespondToModel(err.to_string()))?; + *value = Value::String(path.into_string()); + Ok(()) +} diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index 0f8918df5c..927ccc06fc 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -411,7 +411,7 @@ fn deny_entry_constrains_accepted_grant( return true; } let Some(grant_path) = resolve_permission_path(&entry.path, context) else { - return false; + return true; }; match &deny_entry.path { FileSystemPath::GlobPattern { pattern } => { diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index 2a13575cdf..c84f0185dc 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -562,6 +562,47 @@ fn intersect_permission_profiles_preserves_deny_across_case_variant_grant() { ); } +#[test] +fn intersect_permission_profiles_preserves_denies_for_unresolved_tmpdir() { + let cwd = PathUri::parse("file:///workspace").expect("cwd URI"); + let context = FileSystemSandboxPolicyContext { + cwd: &cwd, + workspace_roots: std::slice::from_ref(&cwd), + user_home_dir: None, + temporary_directories: None, + }; + let write = FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir, + }, + FileSystemAccessMode::Write, + ); + let deny = FileSystemSandboxEntry::new( + PathUri::parse("file:///tmp/private").expect("deny").into(), + FileSystemAccessMode::Deny, + ); + let profile = |entries| PermissionProfile { + file_system: Some(FileSystemPermissions { + entries, + glob_scan_max_depth: None, + }), + ..Default::default() + }; + let grant = profile(vec![write.clone()]); + let grant_with_deny = profile(vec![write, deny]); + + for (requested, granted) in [ + (grant_with_deny.clone(), grant.clone()), + (grant, grant_with_deny.clone()), + (grant_with_deny.clone(), grant_with_deny.clone()), + ] { + assert_eq!( + intersect_permission_profiles_with_context(requested, granted, &context), + grant_with_deny + ); + } +} + #[test] fn intersect_permission_profiles_preserves_rooted_first_segment_glob_deny() { use FileSystemAccessMode::Deny; diff --git a/codex-rs/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index 3a10c64055..f4a42afbbc 100644 --- a/codex-rs/utils/path-uri/src/api_path_string.rs +++ b/codex-rs/utils/path-uri/src/api_path_string.rs @@ -1,5 +1,6 @@ use crate::PathConvention; use crate::PathUri; +use crate::PathUriParseError; use crate::is_windows_separator_byte; use codex_utils_absolute_path::AbsolutePathBuf; use schemars::JsonSchema; @@ -93,6 +94,54 @@ impl LegacyAppPathString { }) } + /// Resolves this raw API path spelling against an executor cwd. + /// + /// Relative paths use the cwd's inferred convention. Home-relative paths + /// use the supplied executor home, and clearly foreign absolute paths are + /// rejected rather than reinterpreted as relative path text. + pub fn resolve_against( + &self, + cwd: &PathUri, + user_home_dir: Option<&PathUri>, + ) -> Result { + let convention = cwd.infer_path_convention().ok_or_else(|| { + LegacyAppPathStringError::MissingBaseConvention { + cwd: cwd.to_string(), + } + })?; + let is_windows = convention == PathConvention::Windows; + let path = self.as_str(); + let home_relative = path + .strip_prefix("~/") + .or_else(|| (path == "~").then_some("")) + .or_else(|| is_windows.then(|| path.strip_prefix(r"~\")).flatten()); + if let Some(suffix) = home_relative { + let home = + user_home_dir.ok_or_else(|| LegacyAppPathStringError::MissingHomeDirectory { + path: path.to_string(), + })?; + return Ok(home.join(suffix.trim_start_matches(|separator| { + separator == '/' || is_windows && separator == '\\' + }))?); + } + + if is_windows && (path.starts_with("//") || path.starts_with(r"\\")) { + return self.to_path_uri(PathConvention::Windows); + } + + match self.infer_absolute_path_convention() { + Some(path_convention) if path_convention == convention => self.to_path_uri(convention), + Some(PathConvention::Posix) if is_windows => Ok(cwd.join(path)?), + Some(path_convention) => Err(LegacyAppPathStringError::MismatchedConvention { + path: path.to_string(), + path_convention, + cwd: cwd.to_string(), + convention, + }), + None => Ok(cwd.join(path)?), + } + } + /// Parses this API string as an absolute path using the convention inferred from its spelling. pub fn to_inferred_path_uri(&self) -> Option { PathUri::try_from(self.clone()).ok() @@ -364,6 +413,21 @@ pub enum LegacyAppPathStringError { path: String, convention: Option, }, + #[error("path URI `{cwd}` has no path convention")] + MissingBaseConvention { cwd: String }, + #[error("cannot resolve home-relative path `{path}` without an executor home")] + MissingHomeDirectory { path: String }, + #[error( + "path {path} uses {path_convention} paths, but executor cwd {cwd} uses {convention} paths" + )] + MismatchedConvention { + path: String, + path_convention: PathConvention, + cwd: String, + convention: PathConvention, + }, + #[error(transparent)] + PathUri(#[from] PathUriParseError), } #[cfg(test)] diff --git a/codex-rs/utils/path-uri/src/api_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs index aecbad8edb..d5a621d290 100644 --- a/codex-rs/utils/path-uri/src/api_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/api_path_string_tests.rs @@ -466,6 +466,74 @@ fn converts_absolute_api_paths_using_the_inferred_convention() { } } +#[test] +fn resolves_legacy_paths_against_executor_context() { + for (cwd, path, user_home_dir, expected) in [ + ( + "file:///workspace", + "relative.txt", + None, + "file:///workspace/relative.txt", + ), + ( + "file:///C:/workspace", + "/Windows", + None, + "file:///C:/Windows", + ), + ( + "file:///C:/workspace", + "//server/share/file.txt", + None, + "file://server/share/file.txt", + ), + ( + "file:///workspace", + "~//notes", + Some("file:///home/executor"), + "file:///home/executor/notes", + ), + ( + "file:///C:/workspace", + r"~\\notes", + Some("file:///C:/Users/executor"), + "file:///C:/Users/executor/notes", + ), + ] { + let cwd = PathUri::parse(cwd).expect("valid cwd"); + let user_home_dir = user_home_dir.map(|home| PathUri::parse(home).expect("valid home")); + + assert_eq!( + LegacyAppPathString::from_string(path).resolve_against(&cwd, user_home_dir.as_ref()), + Ok(PathUri::parse(expected).expect("valid expected path")), + "resolving {path:?} against {cwd}" + ); + } +} + +#[test] +fn rejects_legacy_paths_without_executor_context() { + let cwd = PathUri::parse("file:///workspace").expect("valid cwd"); + + assert_eq!( + LegacyAppPathString::from_string(r"C:\\tmp") + .resolve_against(&cwd, /*user_home_dir*/ None), + Err(LegacyAppPathStringError::MismatchedConvention { + path: r"C:\\tmp".to_string(), + path_convention: PathConvention::Windows, + cwd: cwd.to_string(), + convention: PathConvention::Posix, + }) + ); + assert_eq!( + LegacyAppPathString::from_string("~/secret") + .resolve_against(&cwd, /*user_home_dir*/ None), + Err(LegacyAppPathStringError::MissingHomeDirectory { + path: "~/secret".to_string(), + }) + ); +} + #[test] fn ambiguous_absolute_api_paths_preserve_their_inferred_convention() { for (raw_path, convention) in [