diff --git a/codex-rs/app-server/src/request_processors/apps_processor/installed.rs b/codex-rs/app-server/src/request_processors/apps_processor/installed.rs index a7ce8dd66a..37c650448b 100644 --- a/codex-rs/app-server/src/request_processors/apps_processor/installed.rs +++ b/codex-rs/app-server/src/request_processors/apps_processor/installed.rs @@ -14,7 +14,6 @@ use codex_mcp::effective_mcp_servers; use codex_mcp::host_owned_codex_apps_enabled; use codex_mcp::tool_is_model_visible; use codex_protocol::mcp::ClientMcpExtensions; -use codex_protocol::models::PermissionProfile; #[cfg(test)] #[path = "installed_tests.rs"] @@ -57,12 +56,10 @@ impl AppsRequestProcessor { .apps_enabled_for_auth(auth.as_ref().is_some_and(CodexAuth::uses_codex_backend)); let mcp_manager = self.thread_manager.mcp_manager(); - let mut mcp_config = mcp_manager.runtime_config(&config).await; - // Installed-app discovery has no active turn or reviewer. - mcp_config.permission_profile = PermissionProfile::default(); - let mcp_config = Arc::new(mcp_config); + let mcp_config = mcp_manager.runtime_config(&config).await; let mut mcp_servers = effective_mcp_servers(&mcp_config, auth.as_ref()); mcp_servers.retain(|name, _| name == CODEX_APPS_MCP_SERVER_NAME); + let mcp_config = Arc::new(mcp_config.for_threadless_operations(&mcp_servers)); let cache_key = connector_runtime_context_key(auth.as_ref()); let previous_snapshot = mcp_manager .codex_apps_tools_cache() diff --git a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs index 2ccc6ddbee..f4427207b5 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_resource.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_resource.rs @@ -36,7 +36,6 @@ use codex_protocol::protocol::SessionSource; use core_test_support::responses; use pretty_assertions::assert_eq; use rmcp::handler::server::ServerHandler; -use rmcp::model::BooleanSchema; use rmcp::model::CallToolRequestParams; use rmcp::model::CallToolResult; use rmcp::model::ElicitRequestParams; @@ -47,7 +46,6 @@ use rmcp::model::ListResourcesResult; use rmcp::model::ListToolsResult; use rmcp::model::MetaObject; use rmcp::model::PaginatedRequestParams; -use rmcp::model::PrimitiveSchemaDefinition; use rmcp::model::ProtocolVersion; use rmcp::model::ReadResourceRequestParams; use rmcp::model::ReadResourceResult; @@ -569,7 +567,8 @@ async fn mcp_resource_read_returns_contents_and_declines_elicitation_without_thr r#" chatgpt_base_url = "{apps_server_url}" mcp_oauth_credentials_store = "file" -approval_policy = "on-request" +approval_policy = "never" +sandbox_mode = "danger-full-access" [features] apps = true @@ -979,10 +978,6 @@ impl ServerHandler for ResourceAppsMcpServer { } if uri == TEST_ELICITATION_RESOURCE_URI { let requested_schema = ElicitationSchema::builder() - .required_property( - "confirmed", - PrimitiveSchemaDefinition::Boolean(BooleanSchema::new()), - ) .build() .map_err(|err| rmcp::ErrorData::internal_error(err.to_string(), None))?; let result = context diff --git a/codex-rs/codex-mcp/src/binding.rs b/codex-rs/codex-mcp/src/binding.rs index 862558de5b..79e1451c08 100644 --- a/codex-rs/codex-mcp/src/binding.rs +++ b/codex-rs/codex-mcp/src/binding.rs @@ -10,6 +10,7 @@ use anyhow::Context; use anyhow::Result; use codex_config::AppToolApproval; use codex_protocol::mcp::CallToolResult; +use codex_protocol::models::PermissionProfile; use rmcp::model::ListResourceTemplatesResult; use rmcp::model::ListResourcesResult; use rmcp::model::PaginatedRequestParams; @@ -196,9 +197,10 @@ impl PreparedMcpCall { server_metadata: McpServerMetadata, plugin_id: Option, selected_plugin_server: bool, - ) -> Self { + ) -> Option { let server_name = tool_info.server_name.clone(); - Self { + config.permission_profile_for_server(&server_name)?; + Some(Self { _connections: connections, client, config, @@ -209,7 +211,7 @@ impl PreparedMcpCall { server_metadata, plugin_id, selected_plugin_server, - } + }) } pub fn tool_info(&self) -> &ToolInfo { @@ -221,6 +223,15 @@ impl PreparedMcpCall { &self.config } + /// Returns the owner permissions validated when this immutable call was prepared. + pub fn permission_profile(&self) -> &PermissionProfile { + let Some(permission_profile) = self.config.permission_profile_for_server(&self.server_name) + else { + unreachable!("prepared MCP calls retain their immutable permission authority"); + }; + permission_profile + } + pub fn server_name(&self) -> &str { &self.server_name } diff --git a/codex-rs/codex-mcp/src/binding_tests.rs b/codex-rs/codex-mcp/src/binding_tests.rs index 2d04479492..b42f75c53a 100644 --- a/codex-rs/codex-mcp/src/binding_tests.rs +++ b/codex-rs/codex-mcp/src/binding_tests.rs @@ -105,6 +105,9 @@ async fn test_step( } else { config.approvals_reviewer = ApprovalsReviewer::AutoReview; } + config + .server_permission_profiles + .insert(SERVER_NAME.to_string(), config.permission_profile.clone()); let config = Arc::new(config); let prepared = PreparedMcpCall::new( Arc::clone(&connections), @@ -125,7 +128,8 @@ async fn test_step( }, Some(format!("{label}-plugin")), label == "old", - ); + ) + .expect("test call should retain its thread-owned permission profile"); let calls = HashMap::from([((SERVER_NAME.to_string(), TOOL_NAME.to_string()), prepared)]); TestStep { @@ -213,7 +217,7 @@ async fn prepared_call_keeps_captured_connection_and_authority_after_refresh() - assert_eq!( ( old_call.config().approval_policy.value(), - &old_call.config().permission_profile, + old_call.permission_profile(), old_call.config().approvals_reviewer, ), ( diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 0128c3dac4..d0c50bfa56 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -61,8 +61,6 @@ use codex_diagnostics::Gauge; use codex_diagnostics::GaugeGuard; use codex_protocol::mcp::CallToolResult; use codex_protocol::mcp::McpServerInfo; -use codex_protocol::models::PermissionProfile; -use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::McpStartupCompleteEvent; @@ -226,8 +224,6 @@ impl McpConnectionSet { } = input; let store_mode = config.mcp_oauth_credentials_store_mode; let keyring_backend_kind = config.auth_keyring_backend_kind; - let approval_policy = &config.approval_policy; - let initial_permission_profile = config.permission_profile.clone(); let codex_home = config.codex_home.clone(); let prefix_mcp_tool_names = config.prefix_mcp_tool_names; let non_prefixed_mcp_tool_servers = config.non_prefixed_mcp_tool_servers.clone(); @@ -255,8 +251,7 @@ impl McpConnectionSet { let reusable_previous = previous.filter(|previous| { !previous.servers.is_empty() && previous.elicitation_requests.update( - approval_policy.value(), - initial_permission_profile.clone(), + Arc::clone(&config), elicitation_reviewer.clone(), elicitation_lifecycle.clone(), ) @@ -265,8 +260,7 @@ impl McpConnectionSet { previous.elicitation_requests.clone() } else { ElicitationRequestManager::new( - approval_policy.value(), - initial_permission_profile, + Arc::clone(&config), elicitation_reviewer, elicitation_lifecycle, elicitation_router, @@ -747,13 +741,7 @@ impl McpConnectionSet { tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), prefix_mcp_tool_names, non_prefixed_mcp_tool_servers: Vec::new(), - elicitation_requests: ElicitationRequestManager::new( - AskForApproval::Never, - PermissionProfile::default(), - /*reviewer*/ None, - /*lifecycle*/ None, - ElicitationRequestRouter::default(), - ), + elicitation_requests: ElicitationRequestManager::default(), } } diff --git a/codex-rs/codex-mcp/src/connection_manager/tool_catalog.rs b/codex-rs/codex-mcp/src/connection_manager/tool_catalog.rs index 4075bd616a..ca92e0eeda 100644 --- a/codex-rs/codex-mcp/src/connection_manager/tool_catalog.rs +++ b/codex-rs/codex-mcp/src/connection_manager/tool_catalog.rs @@ -334,7 +334,7 @@ impl McpConnectionSet { ) -> Option { let server_name = &tool_info.server_name; let view = self.servers.get(server_name)?; - Some(PreparedMcpCall::new( + PreparedMcpCall::new( Arc::clone(self), client, config, @@ -345,7 +345,7 @@ impl McpConnectionSet { self.plugin_id_for_mcp_server_name(server_name) .map(str::to_string), self.is_selected_plugin_mcp_server(server_name), - )) + ) } /// Force-refresh Codex Apps tools and publish one new exact catalog revision. diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 2347c9120e..e168840c15 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -6,6 +6,7 @@ use crate::elicitation::ElicitationRequestRouter; use crate::elicitation::ElicitationReviewRequest; use crate::elicitation::ElicitationReviewer; use crate::elicitation::elicitation_is_rejected_by_policy; +use crate::mcp::tests::test_elicitation_config; use crate::rmcp_client::AsyncManagedClient; use crate::rmcp_client::CODEX_APPS_RECONNECT_INITIAL_BACKOFF; use crate::rmcp_client::CodexAppsStartupReconnect; @@ -43,6 +44,7 @@ use codex_protocol::approvals::ElicitationRequest; use codex_protocol::mcp::ClientMcpExtensions; use codex_protocol::mcp::McpServerInfo; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; use codex_protocol::protocol::McpStartupFailureReason; use codex_rmcp_client::ElicitationResponse; @@ -104,8 +106,11 @@ impl McpConnectionSet { prefix_mcp_tool_names, non_prefixed_mcp_tool_servers: Vec::new(), elicitation_requests: ElicitationRequestManager::new( - approval_policy.value(), - permission_profile.get().clone(), + test_elicitation_config( + "server", + approval_policy.value(), + permission_profile.get().clone(), + ), /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -205,9 +210,15 @@ fn store_current_tools(cache_context: &ConnectorRuntimeContext, tools: } async fn capture_binding(manager: &Arc) -> McpBinding { + let mut config = crate::mcp::tests::test_mcp_config(std::env::temp_dir()); + config.server_permission_profiles = manager + .servers + .keys() + .map(|name| (name.clone(), PermissionProfile::default())) + .collect(); manager .capture_binding_with_metadata( - Arc::new(crate::mcp::tests::test_mcp_config(std::env::temp_dir())), + Arc::new(config), /*plugins_available*/ false, /*required_servers*/ &[], ) @@ -787,8 +798,7 @@ fn elicitation_granular_policy_respects_never_and_config() { #[tokio::test] async fn disabled_permissions_auto_accept_elicitation_with_empty_form_schema() { let manager = ElicitationRequestManager::new( - AskForApproval::Never, - PermissionProfile::Disabled, + test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -822,8 +832,7 @@ async fn disabled_permissions_auto_accept_elicitation_with_empty_form_schema() { #[tokio::test] async fn disabled_permissions_do_not_auto_accept_elicitation_with_requested_fields() { let manager = ElicitationRequestManager::new( - AskForApproval::Never, - PermissionProfile::Disabled, + test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -914,8 +923,7 @@ async fn assert_elicitation_declined_with_reviewer_calls( ) { let reviewer = Arc::new(DecliningElicitationReviewer::default()); let manager = ElicitationRequestManager::new( - approval_policy, - PermissionProfile::Disabled, + test_elicitation_config(server_name, approval_policy, PermissionProfile::Disabled), Some(reviewer.clone()), /*lifecycle*/ None, full_access_form_input_enabled_router(), @@ -955,8 +963,7 @@ async fn assert_requested_user_input_is_declined( router: ElicitationRequestRouter, ) { let manager = ElicitationRequestManager::new( - approval_policy, - permission_profile, + test_elicitation_config("server", approval_policy, permission_profile), /*reviewer*/ None, /*lifecycle*/ None, router, @@ -1043,8 +1050,7 @@ async fn assert_disabled_permissions_surface_requested_user_input( let router = full_access_form_input_enabled_router(); let reviewer = Arc::new(DecliningElicitationReviewer::default()); let manager = ElicitationRequestManager::new( - AskForApproval::Never, - PermissionProfile::Disabled, + test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), Some(reviewer.clone()), /*lifecycle*/ None, router.clone(), @@ -1190,8 +1196,7 @@ async fn on_request_approval_forms_remain_with_the_reviewer() { #[tokio::test] async fn disabled_permissions_decline_user_input_without_an_event_channel() { let manager = ElicitationRequestManager::new( - AskForApproval::Never, - PermissionProfile::Disabled, + test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), /*reviewer*/ None, /*lifecycle*/ None, full_access_form_input_enabled_router(), @@ -1222,8 +1227,11 @@ async fn disabled_permissions_decline_user_input_without_an_event_channel() { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { let manager = ElicitationRequestManager::new( - AskForApproval::Never, - PermissionProfile::default(), + test_elicitation_config( + "server", + AskForApproval::Never, + PermissionProfile::default(), + ), /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -1232,14 +1240,20 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { let updater = tokio::spawn(async move { for _ in 0..1_000 { assert!(updating_manager.update( - AskForApproval::OnRequest, - PermissionProfile::Disabled, + test_elicitation_config( + "server", + AskForApproval::OnRequest, + PermissionProfile::Disabled + ), /*reviewer*/ None, /*lifecycle*/ None, )); assert!(updating_manager.update( - AskForApproval::Never, - PermissionProfile::default(), + test_elicitation_config( + "server", + AskForApproval::Never, + PermissionProfile::default() + ), /*reviewer*/ None, /*lifecycle*/ None, )); @@ -1292,15 +1306,21 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() { } }); let manager_a = ElicitationRequestManager::new( - AskForApproval::OnRequest, - PermissionProfile::default(), + test_elicitation_config( + "server", + AskForApproval::OnRequest, + PermissionProfile::default(), + ), /*reviewer*/ None, Some(lifecycle.clone()), router.clone(), ); let manager_b = ElicitationRequestManager::new( - AskForApproval::OnRequest, - PermissionProfile::default(), + test_elicitation_config( + "server", + AskForApproval::OnRequest, + PermissionProfile::default(), + ), /*reviewer*/ None, Some(lifecycle), router.clone(), @@ -1392,8 +1412,11 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() { async fn cancelled_elicitation_is_removed_without_affecting_other_pending_requests() { let router = ElicitationRequestRouter::default(); let manager = ElicitationRequestManager::new( - AskForApproval::OnRequest, - PermissionProfile::default(), + test_elicitation_config( + "server", + AskForApproval::OnRequest, + PermissionProfile::default(), + ), /*reviewer*/ None, /*lifecycle*/ None, router.clone(), @@ -4849,8 +4872,14 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv .authority .lock() .expect("elicitation authority lock"); - authority.approval_policy = AskForApproval::Never; - authority.permission_profile = PermissionProfile::Disabled; + let config = Arc::make_mut( + &mut authority + .as_mut() + .expect("test manager should have permission authority") + .config, + ); + config.approval_policy = Constrained::allow_any(AskForApproval::Never); + config.permission_profile = PermissionProfile::Disabled; } let reconciled = reconcile_reusable_server(&previous, config, runtime_context).await; @@ -4861,8 +4890,12 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv .authority .lock() .expect("elicitation authority lock"); - assert_eq!(authority.approval_policy, AskForApproval::OnRequest); - assert_eq!(authority.permission_profile, PermissionProfile::default()); + let config = &authority + .as_ref() + .expect("reconciled manager should have permission authority") + .config; + assert_eq!(config.approval_policy.value(), AskForApproval::OnRequest); + assert_eq!(config.permission_profile, PermissionProfile::default()); } #[tokio::test] diff --git a/codex-rs/codex-mcp/src/elicitation.rs b/codex-rs/codex-mcp/src/elicitation.rs index 6218419449..7b934e5d63 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -12,6 +12,7 @@ use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; +use crate::McpConfig; use crate::mcp::McpPermissionPromptAutoApproveContext; use crate::mcp::mcp_permission_prompt_is_auto_approved; use anyhow::Context; @@ -25,7 +26,6 @@ use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_TOOL_SUGGESTION; use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY; use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY; -use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -153,53 +153,48 @@ impl ElicitationRequestRouter { #[derive(Clone)] pub(crate) struct ElicitationAuthority { - pub(crate) approval_policy: AskForApproval, - pub(crate) permission_profile: PermissionProfile, + pub(crate) config: Arc, reviewer: Option, lifecycle: Option, } -#[derive(Clone)] +#[derive(Clone, Default)] pub(crate) struct ElicitationRequestManager { router: ElicitationRequestRouter, - pub(crate) authority: Arc>, + pub(crate) authority: Arc>>, } impl ElicitationRequestManager { pub(crate) fn new( - approval_policy: AskForApproval, - permission_profile: PermissionProfile, + config: Arc, reviewer: Option, lifecycle: Option, router: ElicitationRequestRouter, ) -> Self { Self { router, - authority: Arc::new(StdMutex::new(ElicitationAuthority { - approval_policy, - permission_profile, + authority: Arc::new(StdMutex::new(Some(ElicitationAuthority { + config, reviewer, lifecycle, - })), + }))), } } pub(crate) fn update( &self, - approval_policy: AskForApproval, - permission_profile: PermissionProfile, + config: Arc, reviewer: Option, lifecycle: Option, ) -> bool { let Ok(mut authority) = self.authority.lock() else { return false; }; - *authority = ElicitationAuthority { - approval_policy, - permission_profile, + *authority = Some(ElicitationAuthority { + config, reviewer, lifecycle, - }; + }); true } @@ -237,7 +232,8 @@ impl ElicitationRequestManager { }); } - let Ok(authority) = authority.lock().map(|authority| authority.clone()) else { + let Ok(Some(authority)) = authority.lock().map(|authority| authority.clone()) + else { return Ok(ElicitationResponse { action: ElicitationAction::Decline, content: None, @@ -245,11 +241,19 @@ impl ElicitationRequestManager { }); }; let ElicitationAuthority { - approval_policy, - permission_profile, + config, reviewer, lifecycle, } = authority; + let approval_policy = config.approval_policy.value(); + let Some(permission_profile) = config.permission_profile_for_server(&server_name) + else { + return Ok(ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: None, + }); + }; match elicitation .meta() @@ -303,7 +307,7 @@ impl ElicitationRequestManager { let permission_prompt_is_auto_approved = mcp_permission_prompt_is_auto_approved( approval_policy, - &permission_profile, + permission_profile, McpPermissionPromptAutoApproveContext::default(), ); if permission_prompt_is_auto_approved && can_auto_accept_elicitation(&elicitation) { diff --git a/codex-rs/codex-mcp/src/elicitation_tests.rs b/codex-rs/codex-mcp/src/elicitation_tests.rs index 1c206a0a4e..9bd780ccaf 100644 --- a/codex-rs/codex-mcp/src/elicitation_tests.rs +++ b/codex-rs/codex-mcp/src/elicitation_tests.rs @@ -1,5 +1,7 @@ use super::*; +use crate::mcp::tests::test_elicitation_config; use async_channel::Receiver; +use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::GranularApprovalConfig; use pretty_assertions::assert_eq; use rmcp::model::ElicitRequestParams; @@ -75,9 +77,16 @@ fn elicitation_fixture( LifecycleRegistration(active_elicitations.clone()) }) }); - let manager = ElicitationRequestManager::new( + let mut config = test_elicitation_config( + "independent-mcp", approval_policy, - permission_profile, + permission_profile.clone(), + ); + Arc::make_mut(&mut config) + .server_permission_profiles + .insert("another-independent-mcp".to_string(), permission_profile); + let manager = ElicitationRequestManager::new( + config, reviewer.map(|reviewer| reviewer as Arc), lifecycle, ElicitationRequestRouter::default(), @@ -132,8 +141,11 @@ fn closed_event_channel_immediately_cleans_up_pending_elicitation() { /*reviewer*/ None, ); assert!(manager.update( - AskForApproval::OnRequest, - PermissionProfile::Disabled, + test_elicitation_config( + "independent-mcp", + AskForApproval::OnRequest, + PermissionProfile::Disabled + ), /*reviewer*/ None, Some(lifecycle), )); @@ -283,3 +295,106 @@ async fn strict_auto_review_fails_closed_without_a_canonical_decision() { } assert_declined(json!(true), /*response*/ None).await; } + +#[tokio::test] +async fn reused_elicitation_senders_follow_each_servers_latest_permission_authority() { + let mut config = crate::mcp::tests::test_mcp_config(std::env::temp_dir()); + config.approval_policy = codex_config::Constrained::allow_any(AskForApproval::Never); + config.permission_profile = PermissionProfile::Disabled; + config.apps_enabled = true; + let auth = codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(); + + let hosted_server = crate::codex_apps_mcp_server_config( + "https://example.com", + /*apps_mcp_product_sku*/ None, + /*originator*/ None, + ); + let mut attached_server = hosted_server.clone(); + attached_server.environment_id = "attached".to_string(); + let mut catalog = crate::ResolvedMcpCatalog::builder(); + catalog.register(crate::McpServerRegistration::from_config( + "attached".to_string(), + attached_server, + )); + catalog.register(crate::McpServerRegistration::from_hosted_apps( + "host", + /*contribution_order*/ 0, + hosted_server, + )); + config.mcp_server_catalog = catalog.build(); + let servers = crate::effective_mcp_servers(&config, Some(&auth)); + config.set_server_permission_profiles( + &servers, + [("attached".to_string(), PermissionProfile::read_only())], + ); + + let manager = ElicitationRequestManager::new( + Arc::new(config.clone()), + /*reviewer*/ None, + /*lifecycle*/ None, + ElicitationRequestRouter::default(), + ); + let attached = manager.make_sender("attached".to_string(), /*tx_event*/ None); + let hosted = manager.make_sender( + crate::CODEX_APPS_MCP_SERVER_NAME.to_string(), + /*tx_event*/ None, + ); + + assert_eq!( + send_elicitation(&attached, /*marker*/ None).await.action, + ElicitationAction::Decline + ); + assert_eq!( + send_elicitation(&hosted, /*marker*/ None).await.action, + ElicitationAction::Accept + ); + + config.set_server_permission_profiles( + &servers, + [("attached".to_string(), PermissionProfile::Disabled)], + ); + assert!(manager.update( + Arc::new(config.clone()), + /*reviewer*/ None, + /*lifecycle*/ None, + )); + assert_eq!( + send_elicitation(&attached, /*marker*/ None).await.action, + ElicitationAction::Accept + ); + + let mut configured_servers = config.mcp_server_catalog.configured_servers(); + configured_servers + .get_mut("attached") + .expect("attached server should be registered") + .enabled = false; + config.mcp_server_catalog = config + .mcp_server_catalog + .with_materialized_servers(configured_servers); + let servers = crate::effective_mcp_servers(&config, Some(&auth)); + config.set_server_permission_profiles( + &servers, + [("attached".to_string(), PermissionProfile::Disabled)], + ); + assert!(manager.update( + Arc::new(config.clone()), + /*reviewer*/ None, + /*lifecycle*/ None, + )); + assert_eq!( + send_elicitation(&attached, /*marker*/ None).await.action, + ElicitationAction::Decline + ); + + let servers = crate::effective_mcp_servers(&config, /*auth*/ None); + config.set_server_permission_profiles(&servers, std::iter::empty()); + assert!(manager.update( + Arc::new(config.clone()), + /*reviewer*/ None, + /*lifecycle*/ None, + )); + assert_eq!( + send_elicitation(&hosted, /*marker*/ None).await.action, + ElicitationAction::Decline + ); +} diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 3f3e0195ef..05d8a8de68 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -49,6 +49,7 @@ use serde_json::Value; use tokio_util::sync::CancellationToken; use crate::McpProtocolMode; +use crate::McpServerSource; use crate::ResolvedMcpCatalog; use crate::connection_manager::McpConnectionSet; use crate::runtime::McpPublicationGate; @@ -145,6 +146,8 @@ pub struct McpConfig { pub approvals_reviewer: ApprovalsReviewer, /// Working directories for the exact environment handles used by this runtime. pub environment_cwds: HashMap, + /// Explicit server permissions; unresolved or unavailable servers have no entry. + pub server_permission_profiles: HashMap, /// Optional path to `codex-linux-sandbox` for sandboxed MCP tool execution. pub codex_linux_sandbox_exe: Option, /// Whether to use legacy Landlock behavior in the MCP sandbox state. @@ -172,6 +175,58 @@ pub struct McpConfig { pub connector_snapshot: ConnectorSnapshot, } +impl McpConfig { + /// Resolves enabled runtime servers against the exact attachment permissions being published. + pub fn set_server_permission_profiles( + &mut self, + servers: &HashMap, + environment_profiles: impl IntoIterator, + ) { + let environment_profiles = environment_profiles.into_iter().collect::>(); + self.server_permission_profiles = servers + .iter() + .filter(|(_, server)| server.enabled()) + .filter_map(|(server_name, _)| { + let server = self.mcp_server_catalog.server(server_name)?; + let permission_profile = if server + .source() + .is_host_owned_apps(server_name, server.config()) + { + &self.permission_profile + } else if let Some(permission_profile) = + environment_profiles.get(&server.config().environment_id) + { + permission_profile + } else if server.config().is_local_environment() + || matches!(server.source(), McpServerSource::SelectedPlugin(_)) + { + &self.permission_profile + } else { + return None; + }; + Some((server_name.clone(), permission_profile.clone())) + }) + .collect(); + } + + /// Returns this server's published permission authority. + pub fn permission_profile_for_server(&self, server_name: &str) -> Option<&PermissionProfile> { + self.server_permission_profiles.get(server_name) + } + + /// Standalone discovery and resource reads must not inherit thread execution authority. + pub fn for_threadless_operations(&self, servers: &HashMap) -> Self { + let mut config = self.clone(); + config.permission_profile = PermissionProfile::default(); + config.server_permission_profiles = servers + .iter() + .filter(|(_, server)| server.enabled()) + .map(|(name, _)| (name.clone(), PermissionProfile::default())) + .collect(); + config + } +} + #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct ToolPluginProvenance { plugin_display_names_by_connector_id: HashMap>, @@ -357,8 +412,7 @@ pub async fn read_mcp_resource( let mut mcp_servers = effective_mcp_servers(config, auth); mcp_servers.retain(|name, _| name == server); let cancel_token = CancellationToken::new(); - let mut runtime_config = config.clone(); - runtime_config.permission_profile = PermissionProfile::default(); + let runtime_config = config.for_threadless_operations(&mcp_servers); let manager = McpConnectionSet::new( /*previous*/ None, McpPublicationGate::already_published(), @@ -433,8 +487,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( let server_names = mcp_servers.keys().cloned().collect(); let cancel_token = CancellationToken::new(); - let mut runtime_config = config.clone(); - runtime_config.permission_profile = PermissionProfile::default(); + let runtime_config = config.for_threadless_operations(&mcp_servers); let mcp_connection_manager = McpConnectionSet::new( /*previous*/ None, McpPublicationGate::already_published(), diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 79193c6086..350e8f8878 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -16,6 +16,7 @@ use pretty_assertions::assert_eq; use std::collections::HashMap; use std::collections::HashSet; use std::path::PathBuf; +use std::sync::Arc; pub(crate) fn test_mcp_config(codex_home: PathBuf) -> McpConfig { McpConfig { @@ -32,6 +33,7 @@ pub(crate) fn test_mcp_config(codex_home: PathBuf) -> McpConfig { config_layer_stack: codex_config::ConfigLayerStack::default(), approvals_reviewer: codex_config::types::ApprovalsReviewer::default(), environment_cwds: HashMap::new(), + server_permission_profiles: HashMap::new(), codex_linux_sandbox_exe: None, use_legacy_landlock: false, apps_enabled: false, @@ -44,6 +46,20 @@ pub(crate) fn test_mcp_config(codex_home: PathBuf) -> McpConfig { } } +pub(crate) fn test_elicitation_config( + server_name: &str, + approval_policy: AskForApproval, + permission_profile: PermissionProfile, +) -> Arc { + let mut config = test_mcp_config(PathBuf::new()); + config.approval_policy = Constrained::allow_any(approval_policy); + config.permission_profile = permission_profile.clone(); + config + .server_permission_profiles + .insert(server_name.to_string(), permission_profile); + Arc::new(config) +} + #[test] fn qualified_mcp_tool_name_prefix_sanitizes_server_names_without_lowercasing() { assert_eq!( @@ -52,6 +68,47 @@ fn qualified_mcp_tool_name_prefix_sanitizes_server_names_without_lowercasing() { ); } +#[test] +fn mcp_server_permissions_handle_unattached_and_threadless_servers() { + let mut config = test_mcp_config(PathBuf::new()); + config.permission_profile = PermissionProfile::Disabled; + let mut missing_server = codex_apps_mcp_server_config( + "https://example.com", + /*apps_mcp_product_sku*/ None, + /*originator*/ None, + ); + missing_server.environment_id = "missing".to_string(); + let mut selected_server = missing_server.clone(); + selected_server.environment_id = "unattached".to_string(); + let mut catalog = ResolvedMcpCatalog::builder(); + catalog.register(McpServerRegistration::from_config( + "missing".to_string(), + missing_server, + )); + catalog.register(McpServerRegistration::from_selected_plugin( + "selected".to_string(), + McpPluginAttribution::new("selected@test".to_string(), "Selected".to_string()), + /*selection_order*/ 0, + selected_server, + )); + config.mcp_server_catalog = catalog.build(); + let servers = effective_mcp_servers(&config, /*auth*/ None); + assert_eq!(config.permission_profile_for_server("selected"), None); + config.set_server_permission_profiles(&servers, std::iter::empty()); + + assert_eq!( + config.permission_profile_for_server("selected"), + Some(&PermissionProfile::Disabled) + ); + assert_eq!(config.permission_profile_for_server("missing"), None); + + let config = config.for_threadless_operations(&servers); + assert_eq!( + config.permission_profile_for_server("selected"), + Some(&PermissionProfile::default()) + ); +} + #[test] fn mcp_prompt_auto_approval_honors_unrestricted_managed_profiles() { assert!(mcp_permission_prompt_is_auto_approved( diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index af97ffd5dd..57d5299177 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -104,6 +104,7 @@ struct PublishedMcpRuntime { auth_token: Option, plugins_available: bool, ready_selected_capability_roots: Vec, + selected_environments: HashMap>, cached_binding: Mutex>, } @@ -174,6 +175,7 @@ impl McpRuntime { auth_token: None, plugins_available: false, ready_selected_capability_roots: Vec::new(), + selected_environments: HashMap::new(), cached_binding: Mutex::new(None), }), hosted_event_server_removals: watch::channel(()).0, @@ -270,6 +272,7 @@ impl McpRuntime { let auth_token = auth.as_ref().and_then(|auth| auth.get_token().ok()); let plugins_available = input.plugins_available; let ready_selected_capability_roots = input.ready_selected_capability_roots.clone(); + let selected_environments = input.runtime_context.selected_environments.clone(); let connections = Arc::new( McpConnectionSet::new( previous, @@ -295,6 +298,7 @@ impl McpRuntime { auth_token, plugins_available, ready_selected_capability_roots, + selected_environments, cached_binding: Mutex::new(None), })); let _ = publish.send(true); @@ -432,6 +436,22 @@ impl McpRuntime { self.current.load().ready_selected_capability_roots.clone() } + /// Whether this publication uses the currently ready environment handles. + pub fn current_environments_match( + &self, + environments: &HashMap>, + ) -> bool { + let current = self.current.load(); + current.config.is_some() + && current.selected_environments.len() == environments.len() + && environments.iter().all(|(id, environment)| { + current + .selected_environments + .get(id) + .is_some_and(|published| Arc::ptr_eq(published, environment)) + }) + } + pub fn elicitations_auto_deny(&self) -> bool { self.elicitation_router.auto_deny() } @@ -789,6 +809,7 @@ mod tests { auth_token: None, plugins_available: false, ready_selected_capability_roots: Vec::new(), + selected_environments: HashMap::new(), cached_binding: Mutex::new(None), }); let first = McpRuntime::binding_from_published_runtime( diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index dffcfd3f40..252e9ffa22 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1725,6 +1725,7 @@ impl Config { config_layer_stack: self.config_layer_stack.clone(), approvals_reviewer: self.approvals_reviewer, environment_cwds: HashMap::new(), + server_permission_profiles: HashMap::new(), codex_linux_sandbox_exe: self.codex_linux_sandbox_exe.clone(), use_legacy_landlock: self.features.use_legacy_landlock(), apps_enabled: self.features.enabled(Feature::Apps), diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index cea1a81719..ef3b7839fe 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -41,7 +41,6 @@ use codex_mcp::ToolPluginProvenance; use codex_mcp::effective_mcp_servers; use codex_mcp::tool_plugin_provenance; use codex_protocol::mcp::ClientMcpExtensions; -use codex_protocol::models::PermissionProfile; const CONNECTORS_READY_TIMEOUT_ON_EMPTY_TOOLS: Duration = Duration::from_secs(30); @@ -224,10 +223,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( }); } let cache_key = accessible_connectors_cache_key(config, auth.as_ref()); - let mut mcp_config = mcp_manager.runtime_config(config).await; - // Discovery has no active turn or reviewer and must never inherit execution authority. - mcp_config.permission_profile = PermissionProfile::default(); - let mcp_config = Arc::new(mcp_config); + let mcp_config = mcp_manager.runtime_config(config).await; let tool_plugin_provenance = tool_plugin_provenance(&mcp_config); if !force_refetch && let Some(cached_connectors) = read_cached_accessible_connectors(&cache_key) { @@ -240,6 +236,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( let mut mcp_servers = effective_mcp_servers(&mcp_config, auth.as_ref()); mcp_servers.retain(|name, _| name == CODEX_APPS_MCP_SERVER_NAME); + let mcp_config = Arc::new(mcp_config.for_threadless_operations(&mcp_servers)); if mcp_servers.is_empty() { return Ok(AccessibleConnectorsStatus { connectors: Vec::new(), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 1b9057478c..038a9a24c4 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -59,6 +59,7 @@ use codex_protocol::mcp_approval_meta::TOOL_DESCRIPTION_KEY as MCP_TOOL_APPROVAL use codex_protocol::mcp_approval_meta::TOOL_PARAMS_DISPLAY_KEY as MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY; use codex_protocol::mcp_approval_meta::TOOL_PARAMS_KEY as MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY; use codex_protocol::mcp_approval_meta::TOOL_TITLE_KEY as MCP_TOOL_APPROVAL_TOOL_TITLE_KEY; +use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::InputModality; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::McpInvocation; @@ -243,6 +244,7 @@ pub(crate) async fn handle_mcp_tool_call( &hook_tool_name, &metadata, prepared_call.config(), + prepared_call.permission_profile(), approval_policy, ) .await @@ -775,9 +777,8 @@ async fn augment_mcp_tool_request_meta_with_sandbox_state( }; // TODO(anp): Build this metadata from the server's captured // TurnEnvironment::sandbox_context instead of the runtime-wide Landlock value. - let permission_profile = prepared_call.config().permission_profile.clone(); let sandbox_state = serde_json::to_value(SandboxState { - permission_profile, + permission_profile: prepared_call.permission_profile().clone(), codex_linux_sandbox_exe: prepared_call.config().codex_linux_sandbox_exe.clone(), sandbox_cwd, use_legacy_landlock: prepared_call.config().use_legacy_landlock, @@ -1301,6 +1302,7 @@ async fn maybe_request_mcp_tool_approval( hook_tool_name: &HookToolName, metadata: &McpToolApprovalMetadata, config: &codex_mcp::McpConfig, + permission_profile: &PermissionProfile, policy: McpToolApprovalPolicy, ) -> Option { let turn_context = &step_context.turn; @@ -1324,7 +1326,7 @@ async fn maybe_request_mcp_tool_approval( if !strict_auto_review && mcp_permission_prompt_is_auto_approved( config.approval_policy.value(), - &config.permission_profile, + permission_profile, McpPermissionPromptAutoApproveContext { tool_approval_mode: Some(policy.mode), }, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index e8fd9f9f46..51b983339f 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2335,6 +2335,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) .await; @@ -2415,6 +2416,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) .await; @@ -2477,6 +2479,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) .await; @@ -2547,6 +2550,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) .await; @@ -2632,6 +2636,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) .await; @@ -2731,6 +2736,7 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { &HookToolName::new("mcp__test__tool"), &metadata, &captured_mcp_config, + &captured_mcp_config.permission_profile, McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) .await; @@ -2808,6 +2814,7 @@ async fn assert_mcp_user_approval_persistence( &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) .await @@ -2893,6 +2900,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Prompt), ) .await @@ -2958,6 +2966,7 @@ async fn full_access_mode_skips_mcp_tool_approval_for_all_approval_modes() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(approval_mode), ) .await; @@ -3051,6 +3060,7 @@ async fn approve_mode_skips_guardian_in_every_permission_mode() { &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), + turn_context.config.permissions.permission_profile(), McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) .await; diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 847f6e21da..026330b541 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -178,6 +178,24 @@ impl Session { return; }; loop { + let environments = self.services.turn_environments.snapshot().await; + let ready_environments = environments + .turn_environments() + .map(|environment| { + ( + environment.selection.environment_id.clone(), + Arc::clone(&environment.environment), + ) + }) + .collect(); + // Attachment resolution can finish after the owner's configuration callback. + if !self + .services + .mcp_runtime + .current_environments_match(&ready_environments) + { + self.mark_mcp_runtime_dirty(); + } let auth = self.services.auth_manager.auth_cached(); if !self .services @@ -232,9 +250,6 @@ impl Session { ) .await; refresh_invalidation.published = true; - if !self.mcp_refresh.is_pending() { - return; - } } } @@ -813,9 +828,14 @@ async fn review_guardian_mcp_elicitation( let approval_policy = mcp_config.approval_policy.value(); match approval_policy { AskForApproval::Never => { + let Some(permission_profile) = + mcp_config.permission_profile_for_server(&request.server_name) + else { + return Ok(Some(mcp_elicitation_decline_without_message())); + }; if codex_mcp::mcp_permission_prompt_is_auto_approved( approval_policy, - &mcp_config.permission_profile, + permission_profile, codex_mcp::McpPermissionPromptAutoApproveContext::default(), ) && matches!( &request.elicitation, diff --git a/codex-rs/core/src/session/mcp_refresh.rs b/codex-rs/core/src/session/mcp_refresh.rs index 19cf2f8b9b..9afd414bf6 100644 --- a/codex-rs/core/src/session/mcp_refresh.rs +++ b/codex-rs/core/src/session/mcp_refresh.rs @@ -22,6 +22,7 @@ impl McpRefresh { self.pending.store(true, Ordering::Release); } + #[cfg(test)] pub(super) fn is_pending(&self) -> bool { self.pending.load(Ordering::Acquire) } diff --git a/codex-rs/core/src/session/mcp_runtime.rs b/codex-rs/core/src/session/mcp_runtime.rs index 10029b8a8f..010769ca22 100644 --- a/codex-rs/core/src/session/mcp_runtime.rs +++ b/codex-rs/core/src/session/mcp_runtime.rs @@ -333,8 +333,17 @@ impl Session { .environment_cwds .entry(codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()) .or_insert_with(|| PathUri::from_abs_path(&desired.config.cwd)); + let mcp_servers = effective_mcp_servers(&config, auth.as_ref()); + config.set_server_permission_profiles( + &mcp_servers, + desired.environments.turn_environments().map(|environment| { + ( + environment.selection.environment_id.clone(), + environment.permission_profile_with_workspace_roots(), + ) + }), + ); let mcp_config = Arc::new(config); - let mcp_servers = effective_mcp_servers(&mcp_config, auth.as_ref()); let runtime_context = McpRuntimeContext::new( self.services.turn_environments.environment_manager(), desired.local_process_cwd.clone(), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 211cd48c90..77709e494b 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -169,6 +169,7 @@ use core_test_support::PathExt; use core_test_support::context_snapshot; use core_test_support::context_snapshot::ContextSnapshotOptions; use core_test_support::context_snapshot::ContextSnapshotRenderMode; +use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; @@ -8777,7 +8778,33 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() { #[tokio::test] async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() { - let (session, old_turn, rx) = make_session_and_context_with_rx().await; + let guardian_server = start_mock_server().await; + mount_sse_once( + &guardian_server, + sse(vec![ + ev_response_created("guardian-review"), + ev_assistant_message("guardian-review", r#"{"outcome":"allow"}"#), + ev_completed("guardian-review"), + ]), + ) + .await; + let (session, old_turn, rx) = make_session_and_context_with_auth_and_config_and_rx( + CodexAuth::from_api_key("Test API Key"), + Vec::new(), + |config| { + config.model_provider.base_url = Some(format!("{}/v1", guardian_server.uri())); + config + .mcp_servers + .set( + serde_json::from_value(json!({ + "browser-use": { "command": "missing-test-mcp-server" } + })) + .expect("test MCP server configuration should deserialize"), + ) + .expect("test MCP server should be configurable"); + }, + ) + .await; assert_eq!(old_turn.config.approvals_reviewer, ApprovalsReviewer::User); session .spawn_task( @@ -8870,7 +8897,7 @@ async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() { assert_eq!( session .mcp_elicitation_reviewer() - .review(request) + .review(request.clone()) .await .expect("elicitation review should succeed"), Some(ElicitationResponse { @@ -8880,6 +8907,39 @@ async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() { }) ); + let selection = session + .services + .turn_environments + .selections() + .into_iter() + .next() + .expect("session should select its executor environment"); + let mut owner_config = old_turn + .environments + .primary() + .expect("ready environment") + .config() + .clone(); + owner_config.permission_profile = + PermissionProfileSnapshot::legacy(PermissionProfile::read_only()); + session + .environment_ready(&selection, owner_config) + .await + .expect("attachment owner should install its restricted permissions"); + session.refresh_mcp_if_dirty().await; + assert_eq!( + session + .mcp_elicitation_reviewer() + .review(request) + .await + .expect("elicitation review should succeed"), + Some(ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: Some(json!({ "approvals_reviewer": "auto_review" })), + }) + ); + session.abort_all_tasks(TurnAbortReason::Interrupted).await; } diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index bab8d92c51..c1f667f1d9 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -143,14 +143,9 @@ impl TurnEnvironment { } pub(crate) fn permission_profile_with_workspace_roots(&self) -> PermissionProfile { - let workspace_roots = self - .workspace_roots() - .iter() - .filter_map(|workspace_root| workspace_root.to_abs_path().ok()) - .collect::>(); self.permission_profile() .clone() - .materialize_project_roots_with_workspace_roots(&workspace_roots) + .materialize_project_roots_with_path_uris(self.workspace_roots()) } pub(crate) fn selection(&self) -> TurnEnvironmentSelection { diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 73d243b631..d4f011254a 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -871,6 +871,8 @@ async fn settings_update_does_not_retarget_active_turn_environment() -> Result<( async fn deferred_executor_promotes_primary_environment_when_startup_completes() -> Result<()> { let listener = TcpListener::bind("127.0.0.1:0").await?; let server = start_mock_server().await; + let apps = core_test_support::apps_test_server::AppsTestServer::mount(&server).await?; + let mcp_url = format!("{}/api/codex/ps/mcp", apps.chatgpt_base_url); let response_mock = mount_sse_sequence( &server, vec![ @@ -912,7 +914,20 @@ async fn deferred_executor_promotes_primary_environment_when_startup_completes() .await; let mut builder = test_codex() .with_exec_server_url(format!("ws://{}", listener.local_addr()?)) - .with_config(|config| { + .with_model_info_override("gpt-5.4", |model| model.supports_search_tool = false) + .with_config(move |config| { + config + .mcp_servers + .set(HashMap::from([( + "deferred".to_string(), + serde_json::from_value(json!({ + "url": mcp_url, + "environment_id": REMOTE_ENVIRONMENT_ID, + "startup_timeout_sec": 60, + })) + .expect("MCP server config"), + )])) + .expect("set MCP server"); config.project_doc_max_bytes = 0; assert!(config.features.enable(Feature::DeferredExecutor).is_ok()); assert!( @@ -966,8 +981,56 @@ async fn deferred_executor_promotes_primary_environment_when_startup_completes() assert!(initial_context.contains("")); assert!(initial_context.contains("")); assert!(initial_context.contains("starting")); + assert!( + requests[1] + .tool_by_name("mcp__deferred", "calendar_list_events") + .is_none() + ); - serve_environment_info(listener).await; + let mut websocket = accept_initialized_exec_server(listener).await; + // Forward MCP HTTP through the fake executor while keeping startup under test control. + let http_client = + codex_exec_server::Environment::create_for_tests(/*exec_server_url*/ None)? + .get_http_client(); + let executor = tokio::spawn(async move { + loop { + let request = read_exec_server_json(&mut websocket).await; + let Some(id) = request.get("id") else { + continue; + }; + let mut messages = Vec::new(); + if request["method"] == "http/request" { + let params = serde_json::from_value(request["params"].clone()).unwrap(); + let mut response = + serde_json::to_value(http_client.http_request(params).await.unwrap()).unwrap(); + if request["params"]["streamResponse"] == true { + let body = response["bodyBase64"].take(); + response["bodyBase64"] = json!(""); + messages.push(json!({ + "method": "http/request/bodyDelta", + "params": { "requestId": request["params"]["requestId"], "seq": 1, + "deltaBase64": body, "done": true } + })); + } + messages.insert(0, json!({ "id": id, "result": response })); + } else if request["method"] == "environment/info" { + messages.push(json!({ "id": id, + "result": { "shell": { "name": "zsh", "path": "/bin/zsh" } } + })); + } else { + messages.push( + json!({ "id": id, "error": { "code": -32601, "message": "unsupported" } }), + ); + } + for message in messages { + websocket + .send(Message::Text(message.to_string().into())) + .await + .unwrap(); + } + } + }); + core_test_support::wait_for_mcp_server(&test.codex, "deferred").await?; test.codex .submit(Op::UserInputAnswer { id: request.turn_id, @@ -987,6 +1050,11 @@ async fn deferred_executor_promotes_primary_environment_when_startup_completes() .await; let requests = response_mock.requests(); + assert!( + requests[2] + .tool_by_name("mcp__deferred", "calendar_list_events") + .is_some() + ); let updated_context = requests[2] .message_input_texts("user") .into_iter() @@ -1017,6 +1085,8 @@ async fn deferred_executor_promotes_primary_environment_when_startup_completes() Some(&Value::Null) ); + executor.abort(); + let _ = executor.await; Ok(()) } diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index a9def0d0fc..ec6be70b83 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -61,6 +61,12 @@ use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::openai_models::TruncationPolicyConfig; +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 codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EnvironmentConfigState; use codex_protocol::protocol::EventMsg; @@ -1541,14 +1547,16 @@ async fn local_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> Ok(()) } -#[test_case(false, false; "both disabled")] -#[test_case(true, false; "auto review required")] -#[test_case(false, true; "disabled")] -#[test_case(true, true; "both enabled")] +#[test_case(false, false, false; "both disabled")] +#[test_case(true, false, false; "auto review required")] +#[test_case(false, true, false; "disabled")] +#[test_case(true, true, false; "both enabled")] +#[test_case(false, false, true; "attachment-owned permissions preserve foreign workspace roots")] #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn stdio_mcp_tool_call_includes_sandbox_state_meta( node_repl_auto_review_required: bool, node_repl_disabled: bool, + attachment_owned_permissions: bool, ) -> anyhow::Result<()> { // TODO(anp): Remove after packaging a Windows stdio test server for Wine exec. skip_if_wine_exec!( @@ -1560,6 +1568,7 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta( let server = responses::start_mock_server().await; let call_id = "sandbox-meta-call"; + let restricted_call_id = "owner-restricted-call"; let server_name = "rmcp"; let namespace = format!("mcp__{server_name}"); let mut models = codex_models_manager::bundled_models_response()?; @@ -1572,15 +1581,20 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta( model.node_repl_disabled = node_repl_disabled; let models_mock = mount_models_once(&server, models).await; - let initial_mock = mount_sse_once( - &server, - responses::sse(vec![ - responses::ev_response_created("resp-1"), - responses::ev_function_call_with_namespace(call_id, &namespace, "sandbox_meta", "{}"), - responses::ev_completed("resp-1"), - ]), - ) - .await; + let mut response_events = vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call_with_namespace(call_id, &namespace, "sandbox_meta", "{}"), + ]; + if attachment_owned_permissions { + response_events.push(responses::ev_function_call_with_namespace( + restricted_call_id, + &namespace, + "sync", + "{}", + )); + } + response_events.push(responses::ev_completed("resp-1")); + let initial_mock = mount_sse_once(&server, responses::sse(response_events)).await; let final_mock = mount_sse_once( &server, responses::sse(vec![ @@ -1609,6 +1623,91 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta( .await?; wait_for_mcp_server(&fixture.codex, server_name).await?; + let owner_permission_profile = if attachment_owned_permissions { + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some(".git".to_string())), + }, + FileSystemAccessMode::Read, + ), + ]), + NetworkSandboxPolicy::Restricted, + ) + } else { + PermissionProfile::read_only() + }; + let owner_workspace_roots = if attachment_owned_permissions { + let selection = fixture + .codex + .environment_selections() + .await + .into_iter() + .find(|selection| selection.environment_id == remote_aware_environment_id()) + .context("thread should select the MCP server's executor environment")?; + let selection = TurnEnvironmentSelection { + workspace_roots: vec![PathUri::parse(if cfg!(windows) { + "file:///foreign/workspace" + } else { + "file:///C:/workspace" + })?], + ..selection + }; + submit_thread_settings( + &fixture.codex, + ThreadSettingsOverrides { + environments: Some(TurnEnvironmentSelections::new( + fixture.config.cwd.clone(), + vec![TurnEnvironmentSelection { + config: EnvironmentConfigState::Pending, + ..selection.clone() + }], + )), + ..Default::default() + }, + ) + .await?; + let workspace_roots = selection.workspace_roots.clone(); + fixture + .codex + .environment_ready( + &selection, + EnvironmentConfig { + allow_login_shell: fixture.config.permissions.allow_login_shell, + permission_profile: PermissionProfileSnapshot::legacy( + owner_permission_profile.clone(), + ), + shell_environment_policy: Default::default(), + windows_sandbox_level: WindowsSandboxLevel::from_config(&fixture.config), + windows_sandbox_private_desktop: fixture + .config + .permissions + .windows_sandbox_private_desktop, + use_legacy_landlock: fixture.config.features.use_legacy_landlock(), + exec_policy: None, + mcp_policy: None, + network_policy: None, + selected_capability_roots: Vec::new(), + }, + ) + .await?; + workspace_roots + } else { + Vec::new() + }; fixture .thread_manager .get_models_manager() @@ -1622,7 +1721,11 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta( fixture .submit_turn_with_permission_profile( "call the rmcp sandbox_meta tool", - PermissionProfile::read_only(), + if attachment_owned_permissions { + PermissionProfile::Disabled + } else { + PermissionProfile::read_only() + }, ) .await?; @@ -1641,7 +1744,19 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta( json!(node_repl_disabled) ); - let output_item = final_mock.single_request().function_call_output(call_id); + let final_request = final_mock.single_request(); + if attachment_owned_permissions { + let restricted_output = final_request + .function_call_output_text(restricted_call_id) + .expect("restricted MCP tool should produce a denied call output"); + assert!( + restricted_output + .contains("MCP tool call requires approval, but approval policy is never"), + "attachment-owned permissions should deny the mutable MCP tool: {restricted_output}" + ); + } + + let output_item = final_request.function_call_output(call_id); let output_text = output_item .get("output") .and_then(Value::as_str) @@ -1665,10 +1780,26 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta( .get(MCP_SANDBOX_STATE_META_CAPABILITY) .expect("sandbox state metadata should be present"); let sandbox_state: SandboxState = serde_json::from_value(sandbox_meta.clone())?; + if attachment_owned_permissions { + let workspace_root = &owner_workspace_roots[0]; + for path in [workspace_root.clone(), workspace_root.join(".git")?] { + let expected_entry = + FileSystemSandboxEntry::new(path.into(), FileSystemAccessMode::Read); + assert!( + sandbox_state + .permission_profile + .file_system_sandbox_policy() + .entries + .contains(&expected_entry), + "foreign workspace root should retain its owner restriction: {expected_entry:?}" + ); + } + } assert_eq!( sandbox_state, SandboxState { - permission_profile: PermissionProfile::read_only(), + permission_profile: owner_permission_profile + .materialize_project_roots_with_path_uris(&owner_workspace_roots), codex_linux_sandbox_exe: fixture.config.codex_linux_sandbox_exe.clone(), sandbox_cwd: PathUri::from_abs_path(&fixture.config.cwd), use_legacy_landlock: false, diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index e8bcab9c98..4d02ef8d2b 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -548,15 +548,28 @@ impl PermissionProfile { pub fn materialize_project_roots_with_workspace_roots( self, workspace_roots: &[AbsolutePathBuf], + ) -> Self { + self.materialize_project_roots_with(|policy| { + policy.materialize_project_roots_with_workspace_roots(workspace_roots) + }) + } + + pub fn materialize_project_roots_with_path_uris(self, workspace_roots: &[PathUri]) -> Self { + self.materialize_project_roots_with(|policy| { + policy.materialize_project_roots_with_path_uris(workspace_roots) + }) + } + + fn materialize_project_roots_with( + self, + materialize: impl FnOnce(FileSystemSandboxPolicy) -> FileSystemSandboxPolicy, ) -> Self { match self { Self::Managed { file_system, network, } => { - let file_system = file_system - .to_sandbox_policy() - .materialize_project_roots_with_workspace_roots(workspace_roots); + let file_system = materialize(file_system.to_sandbox_policy()); Self::Managed { file_system: ManagedFileSystemPermissions::from_sandbox_policy(&file_system), network, diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index e94ed5c60b..38d9c4b78f 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -7,6 +7,7 @@ use std::path::PathBuf; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_preserving_symlinks; use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; use globset::GlobBuilder; use globset::GlobMatcher; @@ -1030,6 +1031,65 @@ impl FileSystemSandboxPolicy { self } + /// Materializes workspace-root entries without projecting executor paths onto the host. + pub fn materialize_project_roots_with_path_uris(mut self, workspace_roots: &[PathUri]) -> Self { + if let Ok(native_workspace_roots) = workspace_roots + .iter() + .map(PathUri::to_abs_path) + .collect::, _>>() + { + return self.materialize_project_roots_with_workspace_roots(&native_workspace_roots); + } + + let mut entries = Vec::with_capacity(self.entries.len()); + for entry in self.entries { + let (subpath, is_glob) = match &entry.path { + FileSystemPath::Special { + value: FileSystemSpecialPath::ProjectRoots { subpath }, + } => (subpath.as_deref(), false), + FileSystemPath::GlobPattern { pattern } + if pattern.starts_with(PROJECT_ROOTS_GLOB_PATTERN_PREFIX) => + { + ( + Some(&pattern[PROJECT_ROOTS_GLOB_PATTERN_PREFIX.len()..]), + true, + ) + } + _ => { + entries.push(entry); + continue; + } + }; + entries.extend(workspace_roots.iter().filter_map(|root| { + let path = subpath.map_or_else( + || Some(root.clone()), + |subpath| resolve_scoped_workspace_path(root, subpath), + ); + let (path, access) = match path { + Some(path) if is_glob => ( + FileSystemPath::GlobPattern { + pattern: path.inferred_native_path_string(), + }, + entry.access, + ), + Some(path) => (FileSystemPath::Path { path }, entry.access), + None if !entry.access.can_write() => ( + FileSystemPath::Path { path: root.clone() }, + FileSystemAccessMode::Deny, + ), + None => return None, + }; + Some(FileSystemSandboxEntry { + path, + access, + missing_path_behavior: entry.missing_path_behavior, + }) + })); + } + self.entries = entries; + self + } + /// Preserves symbolic `:workspace_roots` entries while also adding concrete /// entries for each provided workspace root. pub fn with_materialized_project_roots_for_workspace_roots( @@ -1600,6 +1660,28 @@ fn resolve_candidate_path(path: &Path, cwd: &Path) -> Option { } } +/// Resolves a workspace-relative path using the root's own path convention. +/// +/// Rejects absolute paths, traversal, Windows drive changes, and any result +/// outside the root so foreign-platform permission rules cannot escape scope. +fn resolve_scoped_workspace_path(root: &PathUri, subpath: &str) -> Option { + let convention = root.infer_path_convention()?; + if subpath.starts_with('/') + || convention == PathConvention::Windows && subpath.starts_with('\\') + || convention + .path_segments(subpath) + .any(|segment| segment == "." || segment == "..") + || convention == PathConvention::Windows + && convention + .path_segments(subpath) + .any(|segment| segment.contains(':')) + { + return None; + } + let path = root.join(subpath).ok()?; + path.starts_with(root).then_some(path) +} + /// Returns true when two config paths refer to the same exact target before /// any prefix matching is applied. ///