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 9f67833674..97c786e8f4 100644 --- a/codex-rs/app-server/tests/suite/v2/executor_skills.rs +++ b/codex-rs/app-server/tests/suite/v2/executor_skills.rs @@ -4,9 +4,13 @@ use anyhow::Result; use app_test_support::TestAppServer; use app_test_support::to_response; use codex_app_server_protocol::CapabilityRootLocation; +use codex_app_server_protocol::GrantedPermissionProfile; use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::PermissionGrantScope; +use codex_app_server_protocol::PermissionsRequestApprovalResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::SelectedCapabilityRoot; +use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::ThreadStartParams; use codex_app_server_protocol::ThreadStartResponse; use codex_app_server_protocol::TurnStartParams; @@ -15,6 +19,8 @@ use codex_app_server_protocol::WarningNotification; use codex_exec_server::CreateDirectoryOptions; use codex_utils_path_uri::PathUri; use core_test_support::responses; +use core_test_support::skip_if_remote; +use core_test_support::skip_if_target_windows; use futures::StreamExt; use futures::TryStreamExt; use pretty_assertions::assert_eq; @@ -22,6 +28,9 @@ use serde_json::json; use tempfile::TempDir; use tokio::time::timeout; +#[cfg(target_os = "macos")] +const READ_TIMEOUT: Duration = Duration::from_secs(60); +#[cfg(not(target_os = "macos"))] const READ_TIMEOUT: Duration = Duration::from_secs(20); const SKILL_NAME: &str = "demo-plugin:deploy"; const SKILL_MARKER: &str = "EXECUTOR_SKILL_BODY_MARKER"; @@ -32,6 +41,8 @@ const REFERENCE_MARKER: &str = "EXECUTOR_SKILL_REFERENCE_MARKER"; enum ExecutorSkillScenario { VisibleWithBudgetWarning, ExplicitOnly, + RestrictedPermittedReference, + RestrictedDeniedReference, } #[tokio::test] @@ -44,16 +55,59 @@ async fn explicit_executor_skill_can_read_referenced_file() -> Result<()> { exercise_executor_skill(ExecutorSkillScenario::ExplicitOnly).await } +#[tokio::test] +async fn restricted_executor_skill_can_read_permitted_reference() -> Result<()> { + exercise_executor_skill(ExecutorSkillScenario::RestrictedPermittedReference).await +} + +#[cfg(unix)] +#[tokio::test] +async fn restricted_executor_skill_rejects_reference_until_permission_approved() -> Result<()> { + exercise_executor_skill(ExecutorSkillScenario::RestrictedDeniedReference).await +} + async fn exercise_executor_skill(scenario: ExecutorSkillScenario) -> Result<()> { + let restricted = matches!( + scenario, + ExecutorSkillScenario::RestrictedPermittedReference + | ExecutorSkillScenario::RestrictedDeniedReference + ); + if restricted { + skip_if_target_windows!( + Ok(()), + "the unelevated Windows sandbox cannot enforce restricted filesystem reads" + ); + } + if scenario == ExecutorSkillScenario::RestrictedDeniedReference { + skip_if_remote!(Ok(()), "the external symlink fixture is host-local"); + } + let server = responses::start_mock_server().await; let codex_home = TempDir::new()?; + let (sandbox_config, permission_profile) = if restricted { + ( + "default_permissions = \"workspace\"", + "\n[permissions.workspace.filesystem.\":workspace_roots\"]\n\".\" = \"write\"\n\n[windows]\nsandbox = \"unelevated\"\n", + ) + } else { + ("sandbox_mode = \"read-only\"", "") + }; + let (approval_policy, requested_permission_feature) = + if scenario == ExecutorSkillScenario::RestrictedDeniedReference { + ( + "on-request", + "\n[features]\nrequest_permissions_tool = true\n", + ) + } else { + ("never", "") + }; std::fs::write( codex_home.path().join("config.toml"), format!( r#" model = "mock-model" -approval_policy = "never" -sandbox_mode = "read-only" +approval_policy = "{approval_policy}" +{sandbox_config} model_provider = "mock_provider" [skills] @@ -65,6 +119,8 @@ base_url = "{}/v1" wire_api = "responses" request_max_retries = 0 stream_max_retries = 0 +{permission_profile} +{requested_permission_feature} "#, server.uri() ), @@ -104,7 +160,9 @@ stream_max_retries = 0 let reference_path = reference_dir.join("details.md")?; let reference_size = match scenario { ExecutorSkillScenario::VisibleWithBudgetWarning => 600 * 1024, - ExecutorSkillScenario::ExplicitOnly => 40 * 1024, + ExecutorSkillScenario::ExplicitOnly + | ExecutorSkillScenario::RestrictedPermittedReference + | ExecutorSkillScenario::RestrictedDeniedReference => 40 * 1024, }; let allow_implicit_invocation = scenario == ExecutorSkillScenario::VisibleWithBudgetWarning; let reference_contents = format!("{REFERENCE_MARKER}\n{}", "x".repeat(reference_size)); @@ -136,6 +194,19 @@ stream_max_retries = 0 /*sandbox*/ None, ), )?; + #[cfg(unix)] + if scenario == ExecutorSkillScenario::RestrictedDeniedReference { + let external_reference_dir = codex_home.path().join("external-reference"); + std::fs::create_dir_all(&external_reference_dir)?; + let external_reference = external_reference_dir.join("details.md"); + std::fs::write( + &external_reference, + format!("DENIED_REFERENCE_MARKER\n{REFERENCE_MARKER}"), + )?; + let reference_native_path = reference_path.to_abs_path()?; + std::fs::remove_file(reference_native_path.as_path())?; + std::os::unix::fs::symlink(external_reference, reference_native_path.as_path())?; + } if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { futures::stream::iter(0..200) .map(|index| { @@ -194,21 +265,56 @@ stream_max_retries = 0 responses::ev_completed(&format!("resp-{call_id}")), ]) }; - let response_mock = responses::mount_sse_sequence( - &server, - vec![ - tool_response("list", "list", json!({"authority": {"kind": "executor"}})), + let mut model_responses = vec![ + tool_response("list", "list", json!({"authority": {"kind": "executor"}})), + tool_response( + "main", + "read", + json!({ + "authority": {"kind": "executor", "id": authority_id}, + "package": package.clone(), + "resource": main_resource.clone(), + }), + ), + tool_response( + "reference", + "read", + json!({ + "authority": {"kind": "executor", "id": authority_id}, + "package": package.clone(), + "resource": reference_resource.clone(), + }), + ), + responses::sse(vec![ + responses::ev_response_created("resp-done"), + responses::ev_assistant_message("msg-done", "Done"), + responses::ev_completed("resp-done"), + ]), + ]; + if scenario == ExecutorSkillScenario::RestrictedDeniedReference { + let external_reference_dir = codex_home.path().join("external-reference"); + model_responses.insert( + 3, + responses::sse(vec![ + responses::ev_response_created("resp-permissions"), + responses::ev_function_call( + "permissions", + "request_permissions", + &json!({ + "reason": "Read the approved skill reference", + "permissions": { + "file_system": {"read": [external_reference_dir]} + } + }) + .to_string(), + ), + responses::ev_completed("resp-permissions"), + ]), + ); + model_responses.insert( + 4, tool_response( - "main", - "read", - json!({ - "authority": {"kind": "executor", "id": authority_id}, - "package": package.clone(), - "resource": main_resource.clone(), - }), - ), - tool_response( - "reference", + "approved-reference", "read", json!({ "authority": {"kind": "executor", "id": authority_id}, @@ -216,14 +322,9 @@ stream_max_retries = 0 "resource": reference_resource.clone(), }), ), - responses::sse(vec![ - responses::ev_response_created("resp-done"), - responses::ev_assistant_message("msg-done", "Done"), - responses::ev_completed("resp-done"), - ]), - ], - ) - .await; + ); + } + let response_mock = responses::mount_sse_sequence(&server, model_responses).await; timeout(READ_TIMEOUT, app_server.initialize()).await??; @@ -263,6 +364,26 @@ stream_max_retries = 0 app_server.read_stream_until_response_message(RequestId::Integer(request_id)), ) .await??; + if scenario == ExecutorSkillScenario::RestrictedDeniedReference { + let request = + timeout(READ_TIMEOUT, app_server.read_stream_until_request_message()).await??; + let ServerRequest::PermissionsRequestApproval { request_id, params } = request else { + panic!("expected a skill reference permissions request, got {request:?}"); + }; + app_server + .send_response( + request_id, + serde_json::to_value(PermissionsRequestApprovalResponse { + permissions: GrantedPermissionProfile { + network: None, + file_system: params.permissions.file_system, + }, + scope: PermissionGrantScope::Turn, + strict_auto_review: None, + })?, + ) + .await?; + } if scenario == ExecutorSkillScenario::VisibleWithBudgetWarning { let warning = timeout(READ_TIMEOUT, async { loop { @@ -313,7 +434,9 @@ stream_max_retries = 0 ExecutorSkillScenario::VisibleWithBudgetWarning => { assert!(!skill_fragment.contains("")); } - ExecutorSkillScenario::ExplicitOnly => { + ExecutorSkillScenario::ExplicitOnly + | ExecutorSkillScenario::RestrictedPermittedReference + | ExecutorSkillScenario::RestrictedDeniedReference => { let resource_access = skill_fragment .split_once("") .and_then(|(_, rest)| rest.split_once("")) @@ -353,7 +476,9 @@ stream_max_retries = 0 ); assert!(list_output["next_cursor"].is_string()); } - ExecutorSkillScenario::ExplicitOnly => { + ExecutorSkillScenario::ExplicitOnly + | ExecutorSkillScenario::RestrictedPermittedReference + | ExecutorSkillScenario::RestrictedDeniedReference => { assert_eq!(list_output["skills"], json!([])); } } @@ -363,11 +488,19 @@ stream_max_retries = 0 .expect("main skill output") .contains(SKILL_MARKER) ); - let reference_output = serde_json::from_str::( - &requests[3] - .function_call_output_text("reference") - .expect("referenced skill file output"), - )?; + let reference_output_text = requests[3] + .function_call_output_text("reference") + .expect("referenced skill file output"); + if scenario == ExecutorSkillScenario::RestrictedDeniedReference { + assert!(reference_output_text.contains("failed to read skill resource")); + assert!(!reference_output_text.contains("DENIED_REFERENCE_MARKER")); + let approved_reference_output = requests[5] + .function_call_output_text("approved-reference") + .expect("approved skill reference output"); + assert!(approved_reference_output.contains(REFERENCE_MARKER)); + return Ok(()); + } + let reference_output = serde_json::from_str::(&reference_output_text)?; assert!( reference_output["contents"] .as_str() @@ -377,7 +510,9 @@ stream_max_retries = 0 ExecutorSkillScenario::VisibleWithBudgetWarning => { assert!(reference_output["next_cursor"].is_string()); } - ExecutorSkillScenario::ExplicitOnly => { + ExecutorSkillScenario::ExplicitOnly + | ExecutorSkillScenario::RestrictedPermittedReference + | ExecutorSkillScenario::RestrictedDeniedReference => { assert!(reference_output["next_cursor"].is_null()); } } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 4a22d32fb3..51f40bf24d 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3068,6 +3068,26 @@ impl Session { .await; let extension_data = codex_extension_api::ExtensionData::new(turn_context.sub_id.clone()); extension_data.insert(selected_capability_roots.clone()); + if !turn_context + .config + .permissions + .file_system_sandbox_policy() + .has_full_disk_read_access() + { + let sandbox_contexts = environments + .turn_environments() + .map(|environment| { + ( + environment.environment_id.clone(), + turn_context.file_system_sandbox_context( + /*additional_permissions*/ None, + environment, + ), + ) + }) + .collect::>(); + extension_data.insert(sandbox_contexts); + } let (mcp, prepared_recommendations) = async { tokio::join!( self.mcp_runtime_for_step( diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index ae9d456bef..b314c2ea40 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -1,8 +1,10 @@ +use std::collections::HashMap; 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::FileSystemSandboxContext; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_extension_api::ConfigContributor; @@ -425,7 +427,12 @@ where session_store: &ExtensionData, thread_store: &ExtensionData, ) -> Vec>> { - self.build_skill_tools(session_store, thread_store, /*executor_query*/ None) + self.build_skill_tools( + session_store, + thread_store, + /*executor_query*/ None, + /*sandbox_contexts*/ None, + ) } fn tools_for_step( @@ -452,7 +459,12 @@ where mcp_resources: None, executor_capability_discovery: None, }); - self.build_skill_tools(session_store, thread_store, executor_query) + self.build_skill_tools( + session_store, + thread_store, + executor_query, + step_store.get::>(), + ) } } @@ -679,6 +691,7 @@ impl SkillsExtension { session_store: &ExtensionData, thread_store: &ExtensionData, executor_query: Option, + sandbox_contexts: Option>>, ) -> Vec>> { let Some(thread_state) = thread_store.get::() else { return Vec::new(); @@ -697,6 +710,7 @@ impl SkillsExtension { thread_state, orchestrator_available, executor_query, + sandbox_contexts, Arc::clone(&self.shadow_selection), ) } @@ -742,6 +756,7 @@ impl SkillsExtension { package: entry.id.clone(), resource: entry.main_prompt.clone(), resolved_executor_roots: Vec::new(), + sandbox: None, host_snapshot, mcp_resources, }, diff --git a/codex-rs/ext/skills/src/provider.rs b/codex-rs/ext/skills/src/provider.rs index b92cda87f6..7a5294560a 100644 --- a/codex-rs/ext/skills/src/provider.rs +++ b/codex-rs/ext/skills/src/provider.rs @@ -8,6 +8,7 @@ mod orchestrator; use codex_core_skills::HostSkillsSnapshot; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_mcp::McpResourceClient; use codex_protocol::capabilities::SelectedCapabilityRoot; @@ -46,6 +47,7 @@ pub struct SkillReadRequest { pub package: SkillPackageId, pub resource: SkillResourceId, pub resolved_executor_roots: Vec, + pub sandbox: Option, pub host_snapshot: Option>, pub mcp_resources: Option>, } diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index de64d2f62f..3c9132d619 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use codex_core_skills::loader::load_environment_skills_from_discovery; use codex_core_skills::loader::load_environment_skills_from_root; use codex_exec_server::EnvironmentManager; +use codex_exec_server::FileSystemSandboxContext; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::protocol::Product; use codex_skills::EnvironmentSkillMetadata; @@ -147,6 +148,7 @@ impl SkillProvider for ExecutorSkillProvider { file_system.as_ref(), resource_path, request.resource.as_str(), + request.sandbox.as_ref(), ) .await?; @@ -265,25 +267,58 @@ async fn read_bounded_text( file_system: &dyn codex_exec_server::ExecutorFileSystem, path: &PathUri, resource: &str, + sandbox: Option<&FileSystemSandboxContext>, ) -> Result { let read_error = |err| { SkillProviderError::new(format!( "failed to read executor skill resource {resource}: {err}" )) }; - let mut stream = file_system - .read_file_stream(path, /*sandbox*/ None) - .await - .map_err(&read_error)?; - let mut contents = Vec::new(); - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(&read_error)?; - if contents.len().saturating_add(chunk.len()) > MAX_SKILL_RESOURCE_CONTENT_BYTES { + let contents = if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { + if path.infer_path_convention() == Some(PathConvention::Windows) + && sandbox.is_some_and(|context| { + context.windows_sandbox_level + == codex_protocol::config_types::WindowsSandboxLevel::Disabled + }) + { + return Err(SkillProviderError::new( + "executor skill resource requires an unavailable filesystem sandbox", + )); + } + let metadata = file_system + .get_metadata(path, sandbox) + .await + .map_err(&read_error)?; + if metadata.size > MAX_SKILL_RESOURCE_CONTENT_BYTES as u64 { return Err(SkillProviderError::new(format!( "executor skill resource {resource} exceeds {MAX_SKILL_RESOURCE_CONTENT_BYTES} bytes" ))); } - contents.extend_from_slice(&chunk); + file_system + .read_file(path, sandbox) + .await + .map_err(&read_error)? + } else { + let mut stream = file_system + .read_file_stream(path, sandbox) + .await + .map_err(&read_error)?; + let mut contents = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(&read_error)?; + if contents.len().saturating_add(chunk.len()) > MAX_SKILL_RESOURCE_CONTENT_BYTES { + return Err(SkillProviderError::new(format!( + "executor skill resource {resource} exceeds {MAX_SKILL_RESOURCE_CONTENT_BYTES} bytes" + ))); + } + contents.extend_from_slice(&chunk); + } + contents + }; + if contents.len() > MAX_SKILL_RESOURCE_CONTENT_BYTES { + return Err(SkillProviderError::new(format!( + "executor skill resource {resource} exceeds {MAX_SKILL_RESOURCE_CONTENT_BYTES} bytes" + ))); } String::from_utf8(contents).map_err(|_| { SkillProviderError::new(format!( diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index 0664b7a043..58a7a656ae 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -1,8 +1,10 @@ +use std::collections::HashMap; use std::collections::hash_map::DefaultHasher; use std::hash::Hash; use std::hash::Hasher; use std::sync::Arc; +use codex_exec_server::FileSystemSandboxContext; use codex_extension_api::FunctionCallError; use codex_extension_api::JsonToolOutput; use codex_extension_api::ResponsesApiTool; @@ -44,6 +46,7 @@ pub(crate) fn skill_tools( thread_state: Arc, orchestrator_available: bool, executor_query: Option, + sandbox_contexts: Option>>, shadow_selection: Arc, ) -> Vec>> { let context = SkillToolContext { @@ -52,6 +55,7 @@ pub(crate) fn skill_tools( thread_state, orchestrator_available, executor_query, + sandbox_contexts, executor_catalog: Arc::new(OnceCell::new()), shadow_selection, }; @@ -70,6 +74,7 @@ struct SkillToolContext { thread_state: Arc, orchestrator_available: bool, executor_query: Option, + sandbox_contexts: Option>>, executor_catalog: Arc>, shadow_selection: Arc, } diff --git a/codex-rs/ext/skills/src/tools/read.rs b/codex-rs/ext/skills/src/tools/read.rs index b0c71cb74d..98d5de56b6 100644 --- a/codex-rs/ext/skills/src/tools/read.rs +++ b/codex-rs/ext/skills/src/tools/read.rs @@ -96,6 +96,27 @@ impl ToolExecutor for ReadTool { .as_ref() .map(|query| query.resolved_executor_roots.clone()) .unwrap_or_default(); + let sandbox = requested_resource + .environment_path() + .and_then(|(environment_id, _)| { + self.context.sandbox_contexts.as_ref().and_then(|contexts| { + contexts.get(environment_id).map(|captured| { + call.environments + .iter() + .find(|environment| environment.environment_id == environment_id) + .map(|environment| environment.file_system_sandbox_context.clone()) + .unwrap_or_else(|| captured.clone()) + }) + }) + }); + if self.context.sandbox_contexts.is_some() + && requested_resource.environment_path().is_some() + && sandbox.is_none() + { + return Err(FunctionCallError::RespondToModel( + "failed to read skill resource".to_string(), + )); + } let result = self .context .thread_state @@ -106,6 +127,7 @@ impl ToolExecutor for ReadTool { package, resource: requested_resource.clone(), resolved_executor_roots, + sandbox, host_snapshot: None, mcp_resources: self.context.mcp_resources.clone(), }, 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 24c6a2984c..37a9c16bcf 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -20,8 +20,15 @@ use codex_exec_server::ReadDirectoryEntry; use codex_exec_server::RemoveOptions; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::SkillScope; use codex_skills_extension::ExecutorSkillProvider; +use codex_skills_extension::catalog::SkillAuthority; +use codex_skills_extension::catalog::SkillPackageId; +use codex_skills_extension::catalog::SkillResourceId; +use codex_skills_extension::catalog::SkillSourceKind; use codex_skills_extension::provider::SkillListQuery; use codex_skills_extension::provider::SkillProvider; use codex_skills_extension::provider::SkillReadRequest; @@ -234,6 +241,42 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { ); } +#[tokio::test] +async fn windows_executor_skill_read_rejects_disabled_sandbox_on_any_orchestrator() { + let provider = ExecutorSkillProvider::new_with_restriction_product( + Arc::new(EnvironmentManager::default_for_tests()), + /*restriction_product*/ None, + ); + let sandbox = FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(Vec::new()), + NetworkSandboxPolicy::Restricted, + ), + ); + let resource = SkillResourceId::environment( + "skill://windows-root/C:/skill/SKILL.md", + "local", + PathUri::parse("file:///C:/skill/SKILL.md").expect("Windows resource URI"), + ); + let error = provider + .read(SkillReadRequest { + authority: SkillAuthority::new(SkillSourceKind::Executor, "windows-root"), + package: SkillPackageId("skill://windows-root/C:/skill".into()), + resource, + resolved_executor_roots: Vec::new(), + sandbox: Some(sandbox), + host_snapshot: None, + mcp_resources: None, + }) + .await + .expect_err("disabled Windows sandbox must fail closed"); + + assert_eq!( + error.message, + "executor skill resource requires an unavailable filesystem sandbox" + ); +} + #[tokio::test] async fn selected_root_id_distinguishes_identical_executor_paths() { let root_label = if cfg!(unix) { @@ -346,6 +389,7 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { package: entry.id.clone(), resource: entry.main_prompt.clone(), resolved_executor_roots: Vec::new(), + sandbox: None, host_snapshot: None, mcp_resources: None, };