From 5decb399aed13261c3c173cb2198a567eb1b22ba Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 30 Jul 2026 09:56:41 +0000 Subject: [PATCH] Respect filesystem permissions during capability discovery (#36124) ## Why Executor capability discovery can traverse plugin and skill roots, including symlinks. Under restricted filesystem permissions, discovery must not expose files outside the permitted paths. ## What changed - Pass each environment's filesystem sandbox context through capability root discovery and apply it to metadata, directory walks, and file reads. - Enable discovery for restricted sessions so permitted executor skills remain available while inaccessible roots and symlink targets are omitted. - Key discovery caches by sandbox context and reject sandboxed discovery on executors that do not advertise support for it. - Split requests with more than 128 roots into supported-size batches. ## Testing - Cover permitted and denied external symlink targets. - Verify restricted skill listing excludes inaccessible skills. - Verify cache separation across permission contexts and discovery of 129 roots. GitOrigin-RevId: 44d16468ca003403bdb8b71a04ae8c9ff94ed494 --- .../tests/suite/v2/executor_skills.rs | 49 ++++++- .../core-skills/tests/environment_loader.rs | 2 + codex-rs/core/src/session/mcp.rs | 77 ++++++++++- codex-rs/core/src/session/mcp_runtime.rs | 3 + codex-rs/core/src/session/mod.rs | 9 +- codex-rs/exec-server-protocol/src/protocol.rs | 14 ++ .../exec-server/src/capability_discovery.rs | 129 +++++++++++++----- .../src/capability_discovery_cache.rs | 67 ++++++--- codex-rs/exec-server/src/environment.rs | 19 ++- .../exec-server/tests/capability_discovery.rs | 109 +++++++++++++++ codex-rs/ext/mcp/tests/executor_plugin_mcp.rs | 2 +- codex-rs/ext/skills/src/extension.rs | 5 +- codex-rs/ext/skills/src/state.rs | 17 ++- .../tests/executor_file_system_authority.rs | 78 ++++++++++- 14 files changed, 510 insertions(+), 70 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/executor_skills.rs b/codex-rs/app-server/tests/suite/v2/executor_skills.rs index 97c786e8f4..a8bdb30523 100644 --- a/codex-rs/app-server/tests/suite/v2/executor_skills.rs +++ b/codex-rs/app-server/tests/suite/v2/executor_skills.rs @@ -36,6 +36,7 @@ const SKILL_NAME: &str = "demo-plugin:deploy"; const SKILL_MARKER: &str = "EXECUTOR_SKILL_BODY_MARKER"; const LOCAL_SKILL_MARKER: &str = "LOCAL_SKILL_BODY_MARKER"; const REFERENCE_MARKER: &str = "EXECUTOR_SKILL_REFERENCE_MARKER"; +const DENIED_SKILL_NAME: &str = "demo-plugin:denied"; #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum ExecutorSkillScenario { @@ -43,6 +44,7 @@ enum ExecutorSkillScenario { ExplicitOnly, RestrictedPermittedReference, RestrictedDeniedReference, + RestrictedVisible, } #[tokio::test] @@ -66,11 +68,17 @@ async fn restricted_executor_skill_rejects_reference_until_permission_approved() exercise_executor_skill(ExecutorSkillScenario::RestrictedDeniedReference).await } +#[tokio::test] +async fn restricted_executor_skill_is_listed_only_when_permitted() -> Result<()> { + exercise_executor_skill(ExecutorSkillScenario::RestrictedVisible).await +} + async fn exercise_executor_skill(scenario: ExecutorSkillScenario) -> Result<()> { let restricted = matches!( scenario, ExecutorSkillScenario::RestrictedPermittedReference | ExecutorSkillScenario::RestrictedDeniedReference + | ExecutorSkillScenario::RestrictedVisible ); if restricted { skip_if_target_windows!( @@ -162,9 +170,13 @@ stream_max_retries = 0 ExecutorSkillScenario::VisibleWithBudgetWarning => 600 * 1024, ExecutorSkillScenario::ExplicitOnly | ExecutorSkillScenario::RestrictedPermittedReference - | ExecutorSkillScenario::RestrictedDeniedReference => 40 * 1024, + | ExecutorSkillScenario::RestrictedDeniedReference + | ExecutorSkillScenario::RestrictedVisible => 40 * 1024, }; - let allow_implicit_invocation = scenario == ExecutorSkillScenario::VisibleWithBudgetWarning; + let allow_implicit_invocation = matches!( + scenario, + ExecutorSkillScenario::VisibleWithBudgetWarning | ExecutorSkillScenario::RestrictedVisible + ); let reference_contents = format!("{REFERENCE_MARKER}\n{}", "x".repeat(reference_size)); tokio::try_join!( file_system.write_file( @@ -207,6 +219,19 @@ stream_max_retries = 0 std::fs::remove_file(reference_native_path.as_path())?; std::os::unix::fs::symlink(external_reference, reference_native_path.as_path())?; } + #[cfg(unix)] + if scenario == ExecutorSkillScenario::RestrictedVisible && !auto_env.environment().is_remote() { + let denied_skill_dir = codex_home.path().join("denied-skill"); + std::fs::create_dir_all(&denied_skill_dir)?; + std::fs::write( + denied_skill_dir.join("SKILL.md"), + "---\nname: denied\ndescription: Skill outside the permitted workspace.\n---\n", + )?; + std::os::unix::fs::symlink( + denied_skill_dir, + plugin_dir.to_abs_path()?.join("skills/denied"), + )?; + } if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { futures::stream::iter(0..200) .map(|index| { @@ -431,7 +456,8 @@ stream_max_retries = 0 assert!(skill_fragment.contains(SKILL_MARKER)); assert!(!skill_fragment.contains(LOCAL_SKILL_MARKER)); match scenario { - ExecutorSkillScenario::VisibleWithBudgetWarning => { + ExecutorSkillScenario::VisibleWithBudgetWarning + | ExecutorSkillScenario::RestrictedVisible => { assert!(!skill_fragment.contains("")); } ExecutorSkillScenario::ExplicitOnly @@ -459,7 +485,8 @@ stream_max_retries = 0 .expect("skills.list output"), )?; match scenario { - ExecutorSkillScenario::VisibleWithBudgetWarning => { + ExecutorSkillScenario::VisibleWithBudgetWarning + | ExecutorSkillScenario::RestrictedVisible => { let deploy_skill = list_output["skills"] .as_array() .and_then(|skills| skills.iter().find(|skill| skill["name"] == SKILL_NAME)) @@ -474,7 +501,16 @@ stream_max_retries = 0 "main_resource": main_resource, }) ); - assert!(list_output["next_cursor"].is_string()); + assert!(list_output["skills"].as_array().is_none_or(|skills| { + skills + .iter() + .all(|skill| skill["name"] != DENIED_SKILL_NAME) + })); + if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { + assert!(list_output["next_cursor"].is_string()); + } else { + assert!(list_output["next_cursor"].is_null()); + } } ExecutorSkillScenario::ExplicitOnly | ExecutorSkillScenario::RestrictedPermittedReference @@ -512,7 +548,8 @@ stream_max_retries = 0 } ExecutorSkillScenario::ExplicitOnly | ExecutorSkillScenario::RestrictedPermittedReference - | ExecutorSkillScenario::RestrictedDeniedReference => { + | ExecutorSkillScenario::RestrictedDeniedReference + | ExecutorSkillScenario::RestrictedVisible => { assert!(reference_output["next_cursor"].is_null()); } } diff --git a/codex-rs/core-skills/tests/environment_loader.rs b/codex-rs/core-skills/tests/environment_loader.rs index e33b010b4d..88b26f2349 100644 --- a/codex-rs/core-skills/tests/environment_loader.rs +++ b/codex-rs/core-skills/tests/environment_loader.rs @@ -589,6 +589,7 @@ async fn executor_bundle_parser_matches_the_existing_environment_loader() { roots: vec![CapabilityRootDiscoverRequest { id: "demo@1".to_string(), path: root_uri, + sandbox: None, }], }, ) @@ -656,6 +657,7 @@ async fn executor_bundle_preserves_parent_namespace_and_manifest_precedence() { roots: vec![CapabilityRootDiscoverRequest { id: "skills-only".to_string(), path: root_uri, + sandbox: None, }], }, ) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 0b90712229..9b56d355ab 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -2,12 +2,14 @@ use super::mcp_refresh::McpRefreshInvalidationGuard; use super::*; use codex_exec_server::ExecutorCapabilityDiscoveryCache; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::MAX_SELECTED_CAPABILITY_ROOTS; use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::ElicitationReviewRequest; use codex_mcp::ElicitationReviewer; use codex_mcp::ElicitationReviewerHandle; +use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_ELICITATION_APPROVAL_KIND_KEY; @@ -90,6 +92,12 @@ impl Session { config: &Config, ) -> (McpConfig, McpRuntimeContext) { let originator = self.originator().await; + let windows_sandbox_level = self + .state + .lock() + .await + .session_configuration + .windows_sandbox_level; let environments = self.services.turn_environments.snapshot().await; let selected_capability_roots = self .resolve_selected_capability_roots_for_step(&environments) @@ -97,7 +105,12 @@ impl Session { let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); let executor_capability_discovery = self - .executor_capability_discovery_for_step(config, &ready_selected_capability_roots) + .executor_capability_discovery_for_step( + config, + &ready_selected_capability_roots, + &environments, + windows_sandbox_level, + ) .await; let mcp_config = self .services @@ -172,6 +185,8 @@ impl Session { .executor_capability_discovery_for_step( &desired.config, &ready_selected_capability_roots, + &desired.environments, + desired.windows_sandbox_level, ) .await; let mcp_projection = self @@ -224,6 +239,8 @@ impl Session { .executor_capability_discovery_for_step( &desired.config, &ready_selected_capability_roots, + &desired.environments, + desired.windows_sandbox_level, ) .await; let mcp_projection = self @@ -294,20 +311,67 @@ impl Session { &self, config: &Config, ready_selected_capability_roots: &[SelectedCapabilityRoot], + environments: &TurnEnvironmentSnapshot, + windows_sandbox_level: WindowsSandboxLevel, ) -> Option> { - if !config - .features - .enabled(Feature::ExecutorCapabilityDiscovery) + let restricted_file_system = !config + .permissions + .file_system_sandbox_policy() + .has_full_disk_read_access(); + if !restricted_file_system + && !config + .features + .enabled(Feature::ExecutorCapabilityDiscovery) { return None; } + let sandbox_contexts = if restricted_file_system { + environments + .turn_environments() + .map(|environment| { + let mut sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd( + config.permissions.permission_profile().clone(), + environment.cwd().clone(), + ); + sandbox.workspace_roots = environment.workspace_roots().to_vec(); + sandbox.windows_sandbox_level = windows_sandbox_level; + sandbox.windows_sandbox_private_desktop = + config.permissions.windows_sandbox_private_desktop; + sandbox.use_legacy_landlock = config.features.use_legacy_landlock(); + (environment.environment_id.clone(), sandbox) + }) + .collect::>() + } else { + HashMap::new() + }; let environment_manager = self.services.turn_environments.environment_manager(); let cache = self .services .thread_extension_data .get_or_init(|| ExecutorCapabilityDiscoveryCache::new(environment_manager)); + let selected_capability_roots = ready_selected_capability_roots + .iter() + .filter(|selected_root| { + if !restricted_file_system { + return true; + } + let CapabilityRootLocation::Environment { environment_id, .. } = + &selected_root.location; + if sandbox_contexts.contains_key(environment_id) { + return true; + } + warn!( + selected_root = selected_root.id, + environment_id, "skipping capability root without a filesystem sandbox context" + ); + false + }) + .cloned() + .collect::>(); Some(Arc::new( - cache.snapshot(ready_selected_capability_roots).await, + cache + .snapshot(&selected_capability_roots, &sandbox_contexts) + .await, )) } @@ -535,10 +599,13 @@ impl Session { .services .mcp_runtime .current_ready_selected_capability_roots(); + let environments = self.services.turn_environments.snapshot().await; let executor_capability_discovery = self .executor_capability_discovery_for_step( refresh_config, &ready_selected_capability_roots, + &environments, + turn_context.windows_sandbox_level, ) .await; let mcp_projection = self diff --git a/codex-rs/core/src/session/mcp_runtime.rs b/codex-rs/core/src/session/mcp_runtime.rs index 01f5ce4a26..765deddca4 100644 --- a/codex-rs/core/src/session/mcp_runtime.rs +++ b/codex-rs/core/src/session/mcp_runtime.rs @@ -16,6 +16,7 @@ pub(super) struct McpDesiredState { pub(super) submit_id: String, pub(super) originator: String, pub(super) environments: TurnEnvironmentSnapshot, + pub(super) windows_sandbox_level: WindowsSandboxLevel, } impl McpDesiredState { @@ -51,6 +52,7 @@ impl Session { submit_id: self.next_internal_sub_id(), originator: session_configuration.originator.clone(), environments, + windows_sandbox_level: session_configuration.windows_sandbox_level, } } @@ -72,6 +74,7 @@ impl Session { submit_id: INITIAL_SUBMIT_ID.to_owned(), originator: session_configuration.originator.clone(), environments: resolved_environments.clone(), + windows_sandbox_level: session_configuration.windows_sandbox_level, }; self.publish_mcp_runtime( &desired, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 51f40bf24d..1e81eecdbd 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3064,11 +3064,18 @@ impl Session { .executor_capability_discovery_for_step( &turn_context.config, &ready_selected_capability_roots, + &environments, + turn_context.windows_sandbox_level, ) .await; let extension_data = codex_extension_api::ExtensionData::new(turn_context.sub_id.clone()); extension_data.insert(selected_capability_roots.clone()); - if !turn_context + if let Some(discovery) = &executor_capability_discovery { + extension_data.insert(discovery.as_ref().clone()); + if !discovery.sandbox_contexts().is_empty() { + extension_data.insert(discovery.sandbox_contexts().clone()); + } + } else if !turn_context .config .permissions .file_system_sandbox_policy() diff --git a/codex-rs/exec-server-protocol/src/protocol.rs b/codex-rs/exec-server-protocol/src/protocol.rs index b87b261d73..ac9a82775f 100644 --- a/codex-rs/exec-server-protocol/src/protocol.rs +++ b/codex-rs/exec-server-protocol/src/protocol.rs @@ -105,6 +105,9 @@ pub struct EnvironmentCapabilities { /// Whether `exec` accepts instructions for launching an executor-local network proxy. #[serde(default)] pub network_proxy_launch: bool, + /// Whether capability discovery applies the filesystem sandbox sent with each root. + #[serde(default)] + pub capability_discovery_sandbox: bool, } /// Status returned by an initialized exec-server connection. @@ -136,6 +139,7 @@ impl EnvironmentInfo { .and_then(|cwd| PathUri::from_host_native_path(cwd).ok()), capabilities: EnvironmentCapabilities { network_proxy_launch: true, + capability_discovery_sandbox: true, }, } } @@ -475,6 +479,9 @@ pub struct CapabilityRootDiscoverRequest { pub id: String, /// Absolute root URI interpreted using the exec-server host's path rules. pub path: PathUri, + /// Filesystem permissions for this root and its symlink targets. + #[serde(default)] + pub sandbox: Option, } /// Executor-local discovery results in request order. @@ -539,6 +546,7 @@ pub struct CapabilityRootDiscovery { #[derive(Clone, Debug)] pub struct ExecutorCapabilityDiscoverySnapshot { roots: Arc<[ExecutorCapabilityDiscoverySnapshotEntry]>, + sandbox_contexts: Arc>, } #[derive(Clone, Debug)] @@ -551,6 +559,7 @@ impl ExecutorCapabilityDiscoverySnapshot { pub fn new( selected_roots: &[SelectedCapabilityRoot], discoveries: Vec, String>>, + sandbox_contexts: HashMap, ) -> Self { debug_assert_eq!(selected_roots.len(), discoveries.len()); Self { @@ -565,12 +574,17 @@ impl ExecutorCapabilityDiscoverySnapshot { }, ) .collect(), + sandbox_contexts: Arc::new(sandbox_contexts), } } pub fn roots(&self) -> &[ExecutorCapabilityDiscoverySnapshotEntry] { &self.roots } + + pub fn sandbox_contexts(&self) -> &HashMap { + self.sandbox_contexts.as_ref() + } } /// HTTP header represented in the executor protocol. diff --git a/codex-rs/exec-server/src/capability_discovery.rs b/codex-rs/exec-server/src/capability_discovery.rs index 5de1e252ec..e935e69698 100644 --- a/codex-rs/exec-server/src/capability_discovery.rs +++ b/codex-rs/exec-server/src/capability_discovery.rs @@ -10,6 +10,7 @@ use codex_exec_server_protocol::DISCOVERABLE_PLUGIN_MANIFEST_PATHS; use codex_exec_server_protocol::DiscoveredPluginFiles; use codex_exec_server_protocol::DiscoveredSkillFiles; use codex_file_system::ExecutorFileSystem; +use codex_file_system::FileSystemSandboxContext; use codex_file_system::WalkEntryKind; use codex_file_system::WalkOptions; use codex_utils_path_uri::PathUri; @@ -17,7 +18,7 @@ use futures::StreamExt; use serde::Deserialize; use serde_json::Value; -const MAX_ROOTS_PER_REQUEST: usize = 128; +pub(crate) const MAX_ROOTS_PER_REQUEST: usize = 128; const MAX_SCAN_DEPTH: usize = 6; const MAX_DIRECTORIES_PER_ROOT: usize = 2_000; const MAX_ENTRIES_PER_ROOT: usize = 20_000; @@ -63,7 +64,8 @@ async fn discover_root( file_system: &dyn ExecutorFileSystem, request: CapabilityRootDiscoverRequest, ) -> CapabilityRootDiscovery { - let CapabilityRootDiscoverRequest { id, path } = request; + let CapabilityRootDiscoverRequest { id, path, sandbox } = request; + let sandbox = sandbox.as_ref(); let mut discovery = CapabilityRootDiscovery { id, path: path.clone(), @@ -74,7 +76,17 @@ async fn discover_root( error: None, }; - match file_system.get_metadata(&path, /*sandbox*/ None).await { + #[cfg(target_os = "windows")] + if sandbox.is_some_and(|context| { + context.should_run_in_sandbox() + && context.windows_sandbox_level + == codex_protocol::config_types::WindowsSandboxLevel::Disabled + }) { + discovery.error = Some("filesystem sandbox is unavailable on this executor".to_string()); + return discovery; + } + + match file_system.get_metadata(&path, sandbox).await { Ok(metadata) if metadata.is_directory => {} Ok(_) => { discovery.error = Some(format!("capability root {path} is not a directory")); @@ -96,7 +108,7 @@ async fn discover_root( follow_directory_symlinks: true, prune_hidden_directories: false, }, - /*sandbox*/ None, + sandbox, ) .await { @@ -143,14 +155,26 @@ async fn discover_root( }); let mut budget = BundleBudget::default(); - let root_manifest = - read_first_plugin_manifest(file_system, &path, &mut budget, &mut discovery.warnings).await; + let root_manifest = read_first_plugin_manifest( + file_system, + &path, + sandbox, + &mut budget, + &mut discovery.warnings, + ) + .await; let inherited_manifest = match root_manifest.as_ref() { Some(manifest) => Some(manifest.clone()), None => { - read_nearest_ancestor_manifest(file_system, &path, &mut budget, &mut discovery.warnings) - .await + read_nearest_ancestor_manifest( + file_system, + &path, + sandbox, + &mut budget, + &mut discovery.warnings, + ) + .await } }; let mut seen_namespace_roots = HashSet::new(); @@ -170,6 +194,7 @@ async fn discover_root( if let Some(manifest) = read_optional_text_file( file_system, manifest_path, + sandbox, &mut budget, &mut discovery.warnings, ) @@ -190,15 +215,27 @@ async fn discover_root( }; let mcp_config = match mcp_path { Some(path) => { - read_optional_text_file(file_system, path, &mut budget, &mut discovery.warnings) - .await + read_optional_text_file( + file_system, + path, + sandbox, + &mut budget, + &mut discovery.warnings, + ) + .await } None => None, }; let apps_config = match declarations.apps_config { Some(path) => { - read_optional_text_file(file_system, path, &mut budget, &mut discovery.warnings) - .await + read_optional_text_file( + file_system, + path, + sandbox, + &mut budget, + &mut discovery.warnings, + ) + .await } None => None, }; @@ -213,6 +250,7 @@ async fn discover_root( let Some(instructions) = read_optional_text_file( file_system, skill_path.clone(), + sandbox, &mut budget, &mut discovery.warnings, ) @@ -228,6 +266,7 @@ async fn discover_root( read_optional_text_file( file_system, metadata_path, + sandbox, &mut budget, &mut discovery.warnings, ) @@ -247,6 +286,7 @@ async fn discover_root( async fn read_first_plugin_manifest( file_system: &dyn ExecutorFileSystem, root: &PathUri, + sandbox: Option<&FileSystemSandboxContext>, budget: &mut BundleBudget, warnings: &mut Vec, ) -> Option { @@ -254,7 +294,9 @@ async fn read_first_plugin_manifest( let Ok(path) = root.join(relative_path) else { continue; }; - if let Some(manifest) = read_optional_text_file(file_system, path, budget, warnings).await { + if let Some(manifest) = + read_optional_text_file(file_system, path, sandbox, budget, warnings).await + { return Some(manifest); } } @@ -264,13 +306,14 @@ async fn read_first_plugin_manifest( async fn read_nearest_ancestor_manifest( file_system: &dyn ExecutorFileSystem, root: &PathUri, + sandbox: Option<&FileSystemSandboxContext>, budget: &mut BundleBudget, warnings: &mut Vec, ) -> Option { let mut ancestor = root.parent(); while let Some(path) = ancestor { if let Some(manifest) = - read_first_plugin_manifest(file_system, &path, budget, warnings).await + read_first_plugin_manifest(file_system, &path, sandbox, budget, warnings).await { return Some(manifest); } @@ -282,10 +325,11 @@ async fn read_nearest_ancestor_manifest( async fn read_optional_text_file( file_system: &dyn ExecutorFileSystem, path: PathUri, + sandbox: Option<&FileSystemSandboxContext>, budget: &mut BundleBudget, warnings: &mut Vec, ) -> Option { - let metadata = match file_system.get_metadata(&path, /*sandbox*/ None).await { + let metadata = match file_system.get_metadata(&path, sandbox).await { Ok(metadata) if metadata.is_file => metadata, Ok(_) => return None, Err(error) if error.kind() == io::ErrorKind::NotFound => return None, @@ -310,32 +354,49 @@ async fn read_optional_text_file( )); return None; } - let mut stream = match file_system.read_file_stream(&path, /*sandbox*/ None).await { - Ok(stream) => stream, - Err(error) => { - warnings.push(format!("failed to read capability file {path}: {error}")); - return None; + let contents = if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { + match file_system.read_file(&path, sandbox).await { + Ok(contents) if contents.len() <= MAX_FILE_BYTES && budget.can_add(contents.len()) => { + contents + } + Ok(_) => { + warnings.push(format!("capability file {path} exceeded its read limit")); + return None; + } + Err(error) => { + warnings.push(format!("failed to read capability file {path}: {error}")); + return None; + } } - }; - let mut contents = Vec::with_capacity(size); - while let Some(chunk) = stream.next().await { - let chunk = match chunk { - Ok(chunk) => chunk, + } else { + let mut stream = match file_system.read_file_stream(&path, sandbox).await { + Ok(stream) => stream, Err(error) => { warnings.push(format!("failed to read capability file {path}: {error}")); return None; } }; - let Some(new_len) = contents.len().checked_add(chunk.len()) else { - warnings.push(format!("capability file {path} exceeded its read limit")); - return None; - }; - if new_len > MAX_FILE_BYTES || !budget.can_add(new_len) { - warnings.push(format!("capability file {path} exceeded its read limit")); - return None; + let mut contents = Vec::with_capacity(size); + while let Some(chunk) = stream.next().await { + let chunk = match chunk { + Ok(chunk) => chunk, + Err(error) => { + warnings.push(format!("failed to read capability file {path}: {error}")); + return None; + } + }; + let Some(new_len) = contents.len().checked_add(chunk.len()) else { + warnings.push(format!("capability file {path} exceeded its read limit")); + return None; + }; + if new_len > MAX_FILE_BYTES || !budget.can_add(new_len) { + warnings.push(format!("capability file {path} exceeded its read limit")); + return None; + } + contents.extend_from_slice(&chunk); } - contents.extend_from_slice(&chunk); - } + contents + }; let contents = match String::from_utf8(contents) { Ok(contents) => contents, Err(error) => { diff --git a/codex-rs/exec-server/src/capability_discovery_cache.rs b/codex-rs/exec-server/src/capability_discovery_cache.rs index dddf87a160..b1e9fb5408 100644 --- a/codex-rs/exec-server/src/capability_discovery_cache.rs +++ b/codex-rs/exec-server/src/capability_discovery_cache.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::collections::HashMap; use std::sync::Arc; use codex_protocol::capabilities::CapabilityRootLocation; @@ -10,6 +11,7 @@ use crate::CapabilityRootDiscovery; use crate::CapabilityRootsDiscoverParams; use crate::EnvironmentManager; use crate::ExecutorCapabilityDiscoverySnapshot; +use crate::FileSystemSandboxContext; /// Thread-scoped cache shared by capability consumers using the high-level executor API. /// @@ -31,6 +33,7 @@ impl std::fmt::Debug for ExecutorCapabilityDiscoveryCache { struct CachedRoot { selected_root: SelectedCapabilityRoot, + sandbox: Option, // Both successes and failures are memoized for the thread. Retrying a transient failure for // the same stable selected root requires explicit invalidation or a new thread. result: Result, String>, @@ -53,36 +56,47 @@ impl ExecutorCapabilityDiscoveryCache { pub async fn discover( &self, selected_roots: &[SelectedCapabilityRoot], + sandbox_contexts: &HashMap, ) -> Vec, String>> { let missing = { let entries = self.entries.lock().await; selected_roots .iter() .filter(|selected_root| { - !entries - .iter() - .any(|cached| cached.selected_root == **selected_root) + let CapabilityRootLocation::Environment { environment_id, .. } = + &selected_root.location; + let sandbox = sandbox_contexts.get(environment_id); + !entries.iter().any(|cached| { + cached.selected_root == **selected_root + && cached.sandbox.as_ref() == sandbox + }) }) .cloned() .collect::>() }; - let discovered = self.discover_missing(missing).await; + let discovered = self.discover_missing(missing, sandbox_contexts).await; let mut entries = self.entries.lock().await; for discovered_root in discovered { - if !entries - .iter() - .any(|cached| cached.selected_root == discovered_root.selected_root) + if let Some(cached) = entries + .iter_mut() + .find(|cached| cached.selected_root == discovered_root.selected_root) { + if cached.sandbox != discovered_root.sandbox { + *cached = discovered_root; + } + } else { entries.push(discovered_root); } } selected_roots .iter() .map(|selected_root| { - match entries - .iter() - .find(|cached| cached.selected_root == *selected_root) - { + let CapabilityRootLocation::Environment { environment_id, .. } = + &selected_root.location; + let sandbox = sandbox_contexts.get(environment_id); + match entries.iter().find(|cached| { + cached.selected_root == *selected_root && cached.sandbox.as_ref() == sandbox + }) { Some(cached) => cached.result.clone(), None => Err(format!( "selected capability root `{}` was not discovered", @@ -97,14 +111,20 @@ impl ExecutorCapabilityDiscoveryCache { pub async fn snapshot( &self, selected_roots: &[SelectedCapabilityRoot], + sandbox_contexts: &HashMap, ) -> ExecutorCapabilityDiscoverySnapshot { ExecutorCapabilityDiscoverySnapshot::new( selected_roots, - self.discover(selected_roots).await, + self.discover(selected_roots, sandbox_contexts).await, + sandbox_contexts.clone(), ) } - async fn discover_missing(&self, missing: Vec) -> Vec { + async fn discover_missing( + &self, + missing: Vec, + sandbox_contexts: &HashMap, + ) -> Vec { let mut grouped = BTreeMap::>::new(); for selected_root in missing { let CapabilityRootLocation::Environment { environment_id, .. } = @@ -115,8 +135,15 @@ impl ExecutorCapabilityDiscoveryCache { .push(selected_root); } - let discoveries = futures::future::join_all(grouped.into_iter().map( - |(environment_id, selected_roots)| async move { + let batches = grouped.into_iter().flat_map(|(environment_id, roots)| { + roots + .chunks(crate::capability_discovery::MAX_ROOTS_PER_REQUEST) + .map(|batch| (environment_id.clone(), batch.to_vec())) + .collect::>() + }); + let discoveries = + futures::future::join_all(batches.map(|(environment_id, selected_roots)| async move { + let sandbox = sandbox_contexts.get(&environment_id).cloned(); let Some(environment) = self.environment_manager.get_environment(&environment_id) else { let error = format!("environment `{environment_id}` is unavailable"); @@ -124,6 +151,7 @@ impl ExecutorCapabilityDiscoveryCache { .into_iter() .map(|selected_root| CachedRoot { selected_root, + sandbox: sandbox.clone(), result: Err(error.clone()), }) .collect::>(); @@ -137,6 +165,7 @@ impl ExecutorCapabilityDiscoveryCache { CapabilityRootDiscoverRequest { id: selected_root.id.clone(), path: path.clone(), + sandbox: sandbox.clone(), } }) .collect(), @@ -149,6 +178,7 @@ impl ExecutorCapabilityDiscoveryCache { .into_iter() .map(|selected_root| CachedRoot { selected_root, + sandbox: sandbox.clone(), result: Err(error.clone()), }) .collect(); @@ -164,6 +194,7 @@ impl ExecutorCapabilityDiscoveryCache { .into_iter() .map(|selected_root| CachedRoot { selected_root, + sandbox: sandbox.clone(), result: Err(error.clone()), }) .collect(); @@ -185,13 +216,13 @@ impl ExecutorCapabilityDiscoveryCache { }; CachedRoot { selected_root, + sandbox: sandbox.clone(), result, } }) .collect() - }, - )) - .await; + })) + .await; discoveries.into_iter().flatten().collect() } } diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 1618a301a7..41185ad3d4 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -814,7 +814,24 @@ impl Environment { params: CapabilityRootsDiscoverParams, ) -> Result { match &self.remote_client { - Some(client) => client.get().await?.discover_capability_roots(params).await, + Some(client) => { + let client = client.get().await?; + if params.roots.iter().any(|root| { + root.sandbox + .as_ref() + .is_some_and(crate::FileSystemSandboxContext::should_run_in_sandbox) + }) && !client + .environment_info() + .await? + .capabilities + .capability_discovery_sandbox + { + return Err(ExecServerError::Protocol( + "exec-server does not support sandboxed capability discovery".to_string(), + )); + } + client.discover_capability_roots(params).await + } None => crate::discover_capability_roots(self.filesystem.as_ref(), params) .await .map_err(|error| ExecServerError::Protocol(error.to_string())), diff --git a/codex-rs/exec-server/tests/capability_discovery.rs b/codex-rs/exec-server/tests/capability_discovery.rs index 988da27b27..840a4727d4 100644 --- a/codex-rs/exec-server/tests/capability_discovery.rs +++ b/codex-rs/exec-server/tests/capability_discovery.rs @@ -4,11 +4,26 @@ use codex_exec_server::CAPABILITY_ROOTS_DISCOVER_METHOD; use codex_exec_server::CapabilityRootDiscovery; use codex_exec_server::CapabilityRootsDiscoverParams; use codex_exec_server::CapabilityRootsDiscoverResponse; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::InitializeParams; use codex_exec_server::InitializeResponse; use codex_exec_server_protocol::CapabilityRootDiscoverRequest; use codex_exec_server_protocol::JSONRPCMessage; use codex_exec_server_protocol::JSONRPCResponse; +#[cfg(unix)] +use codex_protocol::models::PermissionProfile; +#[cfg(unix)] +use codex_protocol::permissions::FileSystemAccessMode; +#[cfg(unix)] +use codex_protocol::permissions::FileSystemPath; +#[cfg(unix)] +use codex_protocol::permissions::FileSystemSandboxEntry; +#[cfg(unix)] +use codex_protocol::permissions::FileSystemSandboxPolicy; +#[cfg(unix)] +use codex_protocol::permissions::NetworkSandboxPolicy; +#[cfg(unix)] +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use common::exec_server::exec_server; use pretty_assertions::assert_eq; @@ -166,10 +181,103 @@ async fn discovers_cursor_plugin_without_reading_default_mcp_for_inline_servers( Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn sandboxed_discovery_follows_only_permitted_external_symlinks() -> anyhow::Result<()> { + let root = tempfile::tempdir()?; + let external = tempfile::tempdir()?; + write_file( + &root.path().join(".codex-plugin/plugin.json"), + r#"{"name":"linked-plugin","mcpServers":"./external-mcp.json"}"#, + )?; + write_file( + &external.path().join("mcp.json"), + r#"{"mcpServers":{"linked":{"command":"linked-server"}}}"#, + )?; + write_file( + &external.path().join("skill/SKILL.md"), + "---\nname: linked\ndescription: Linked external skill.\n---\n", + )?; + std::fs::create_dir_all(root.path().join("skills"))?; + std::os::unix::fs::symlink( + external.path().join("skill"), + root.path().join("skills/linked"), + )?; + std::os::unix::fs::symlink( + external.path().join("mcp.json"), + root.path().join("external-mcp.json"), + )?; + + let mut server = exec_server().await?; + initialize(&mut server).await?; + let root_uri = PathUri::from_host_native_path(root.path())?; + let root_path = AbsolutePathBuf::from_absolute_path(root.path())?; + let external_root = AbsolutePathBuf::from_absolute_path(external.path())?; + let path_entry = + |path, access| FileSystemSandboxEntry::new(FileSystemPath::Path { path }, access); + let read_root = path_entry(root_path, FileSystemAccessMode::Read); + let read_external = path_entry(external_root.clone(), FileSystemAccessMode::Read); + let deny_external_skill = path_entry(external_root.join("skill"), FileSystemAccessMode::Deny); + let cases = [ + ( + "permitted symlinks", + vec![read_root.clone(), read_external.clone()], + true, + true, + ), + ( + "denied external root", + vec![read_root.clone()], + false, + false, + ), + ( + "denied external skill", + vec![read_root, read_external, deny_external_skill], + false, + true, + ), + ]; + + for (scenario, entries, has_skill, has_mcp) in cases { + let policy = FileSystemSandboxPolicy::restricted(entries); + let sandbox = FileSystemSandboxContext::from_permission_profile_with_cwd( + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted), + root_uri.clone(), + ); + let discovery = + discover_root_with_sandbox(&mut server, "linked", root_uri.clone(), Some(sandbox)) + .await?; + + assert_eq!(discovery.error, None, "{scenario}"); + assert_eq!(discovery.skills.len(), usize::from(has_skill), "{scenario}"); + assert_eq!( + discovery + .plugin + .and_then(|plugin| plugin.mcp_config) + .is_some_and(|config| config.contents.contains("linked-server")), + has_mcp, + "{scenario}" + ); + } + + server.shutdown().await?; + Ok(()) +} + async fn discover_root( server: &mut common::exec_server::ExecServerHarness, id: &str, path: PathUri, +) -> anyhow::Result { + discover_root_with_sandbox(server, id, path, /*sandbox*/ None).await +} + +async fn discover_root_with_sandbox( + server: &mut common::exec_server::ExecServerHarness, + id: &str, + path: PathUri, + sandbox: Option, ) -> anyhow::Result { let request_id = server .send_request( @@ -178,6 +286,7 @@ async fn discover_root( roots: vec![CapabilityRootDiscoverRequest { id: id.to_string(), path, + sandbox, }], })?, ) diff --git a/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs b/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs index 5be70c9a4f..c8b34b6b57 100644 --- a/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs +++ b/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs @@ -230,7 +230,7 @@ async fn raw_selected_plugin_contributions( { Some( ExecutorCapabilityDiscoveryCache::new(environment_manager) - .snapshot(&selected_capability_roots) + .snapshot(&selected_capability_roots, &Default::default()) .await, ) } else { diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index b314c2ea40..451ad4f711 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::injection::HostSkillsCatalogInWorldState; use codex_core_skills::injection::InjectedHostSkillPrompts; +use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::ResolvedSelectedCapabilityRoot; @@ -457,7 +458,9 @@ where include_bundled_skills: false, include_orchestrator_skills: false, mcp_resources: None, - executor_capability_discovery: None, + executor_capability_discovery: step_store + .get::() + .map(|discovery| discovery.as_ref().clone()), }); self.build_skill_tools( session_store, diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index 6ba735c158..3f83a0857f 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -4,6 +4,7 @@ use std::future::Future; use std::sync::Arc; use std::sync::Mutex; +use codex_exec_server::FileSystemSandboxContext; use codex_extension_api::ExtensionMetrics; use codex_mcp::McpResourceClient; use codex_mcp::McpResourceClientCacheKey; @@ -138,12 +139,19 @@ impl SkillsThreadState { providers: &SkillProviders, query: SkillListQuery, ) -> SkillCatalog { + let sandbox_contexts = query + .executor_capability_discovery + .as_ref() + .map(|discovery| discovery.sandbox_contexts().clone()) + .unwrap_or_default(); if let Some(cached) = self .executor_discovery_cache .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .as_ref() - .filter(|cached| cached.roots == query.executor_roots) + .filter(|cached| { + cached.roots == query.executor_roots && cached.sandbox_contexts == sandbox_contexts + }) { return cached.catalog.clone(); } @@ -153,11 +161,15 @@ impl SkillsThreadState { .executor_discovery_cache .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - if let Some(cached) = cache.as_ref().filter(|cached| cached.roots == roots) { + if let Some(cached) = cache + .as_ref() + .filter(|cached| cached.roots == roots && cached.sandbox_contexts == sandbox_contexts) + { return cached.catalog.clone(); } *cache = Some(CachedExecutorDiscoveryCatalog { roots, + sandbox_contexts, catalog: discovered.clone(), }); discovered @@ -282,6 +294,7 @@ struct CachedExecutorCatalog { struct CachedExecutorDiscoveryCatalog { roots: Vec, + sandbox_contexts: HashMap, catalog: SkillCatalog, } diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index 37a9c16bcf..1fa0a20989 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::io; use std::sync::Arc; use std::sync::atomic::AtomicUsize; @@ -365,7 +366,7 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { }, }]; let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::new(manager) - .snapshot(&executor_roots) + .snapshot(&executor_roots, &Default::default()) .await; let catalog = provider .list(SkillListQuery { @@ -403,6 +404,81 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { assert_eq!(read.contents, SKILL_CONTENTS); } +#[tokio::test] +async fn high_level_discovery_cache_separates_filesystem_permission_contexts() { + let test_root = create_local_skill_root("permission-context").expect("create local skill root"); + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let cache = ExecutorCapabilityDiscoveryCache::new(manager); + let executor_roots = vec![SelectedCapabilityRoot { + id: "permission-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), + }, + }]; + cache.snapshot(&executor_roots, &HashMap::new()).await; + + let updated_contents = + "---\nname: synthetic\ndescription: Updated executor skill.\n---\n\nUPDATED_BODY\n"; + std::fs::write(test_root.join("skill/SKILL.md"), updated_contents) + .expect("update executor skill"); + let sandbox_contexts = HashMap::from([( + "local".to_string(), + FileSystemSandboxContext::from_permission_profile(PermissionProfile::Disabled), + )]); + let second_snapshot = cache.snapshot(&executor_roots, &sandbox_contexts).await; + let second_discovery = second_snapshot.roots()[0] + .result + .as_ref() + .expect("second discovery"); + + assert_eq!( + second_discovery.skills[0].instructions.contents, + updated_contents + ); + assert_eq!(second_snapshot.sandbox_contexts(), &sandbox_contexts); + + let restored_contents = + "---\nname: synthetic\ndescription: Restored executor skill.\n---\n\nRESTORED_BODY\n"; + std::fs::write(test_root.join("skill/SKILL.md"), restored_contents) + .expect("restore executor skill"); + let restored_snapshot = cache.snapshot(&executor_roots, &HashMap::new()).await; + let restored_discovery = restored_snapshot.roots()[0] + .result + .as_ref() + .expect("restored discovery"); + + assert_eq!( + restored_discovery.skills[0].instructions.contents, + restored_contents + ); + + std::fs::remove_dir_all(test_root).expect("remove skill directory"); +} + +#[tokio::test] +async fn high_level_discovery_batches_more_than_128_roots() { + let test_root = create_local_skill_root("many-roots").expect("create local skill root"); + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let cache = ExecutorCapabilityDiscoveryCache::new(manager); + let executor_roots = (0..129) + .map(|index| SelectedCapabilityRoot { + id: format!("root-{index}"), + location: CapabilityRootLocation::Environment { + environment_id: "local".to_string(), + path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), + }, + }) + .collect::>(); + + let snapshot = cache.snapshot(&executor_roots, &HashMap::new()).await; + + assert_eq!(snapshot.roots().len(), executor_roots.len()); + assert!(snapshot.roots().iter().all(|root| root.result.is_ok())); + + std::fs::remove_dir_all(test_root).expect("remove skill directory"); +} + fn create_local_skill_root(label: &str) -> io::Result { let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed); let test_root = std::env::temp_dir().join(format!(