diff --git a/codex-rs/core/src/mcp_openai_file.rs b/codex-rs/core/src/mcp_openai_file.rs index b04e012168dc..f86a740a5f3c 100644 --- a/codex-rs/core/src/mcp_openai_file.rs +++ b/codex-rs/core/src/mcp_openai_file.rs @@ -16,8 +16,10 @@ use crate::session::step_context::StepContext; use codex_api::HostedFileUploadContext; use codex_api::OPENAI_FILE_UPLOAD_LIMIT_BYTES; use codex_api::upload_openai_file; -use codex_http_client::RouteAwareClientPool; use codex_login::CodexAuth; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; +use codex_sandboxing::policy_transforms::merge_permission_profiles; use serde_json::Value as JsonValue; use std::collections::HashMap; @@ -51,8 +53,8 @@ pub(crate) async fn rewrite_mcp_tool_arguments_for_openai_files( continue; }; let Some(uploaded_value) = rewrite_argument_value_for_openai_files( + sess, step_context, - &sess.services.openai_file_upload_client_pool, auth.as_ref(), field_name, optional_fields, @@ -74,8 +76,8 @@ pub(crate) async fn rewrite_mcp_tool_arguments_for_openai_files( } async fn rewrite_argument_value_for_openai_files( + sess: &Session, step_context: &StepContext, - client_pool: &RouteAwareClientPool, auth: Option<&CodexAuth>, field_name: &str, optional_fields: &[String], @@ -85,8 +87,8 @@ async fn rewrite_argument_value_for_openai_files( match value { JsonValue::String(file_path) => { let rewritten = build_uploaded_argument_value( + sess, step_context, - client_pool, auth, FileArgumentLocation { field_name, @@ -106,8 +108,8 @@ async fn rewrite_argument_value_for_openai_files( return Ok(None); }; let rewritten = build_uploaded_argument_value( + sess, step_context, - client_pool, auth, FileArgumentLocation { field_name, @@ -127,8 +129,8 @@ async fn rewrite_argument_value_for_openai_files( } async fn build_uploaded_argument_value( + sess: &Session, step_context: &StepContext, - client_pool: &RouteAwareClientPool, auth: Option<&CodexAuth>, argument: FileArgumentLocation<'_>, optional_fields: &[String], @@ -158,9 +160,43 @@ async fn build_uploaded_argument_value( .cwd() .join(file_path) .map_err(|error| contextualize_error(error.to_string()))?; + let additional_permissions = merge_permission_profiles( + sess.granted_session_permissions(&turn_environment.environment_id) + .await + .as_ref(), + sess.granted_turn_permissions(&turn_environment.environment_id) + .await + .as_ref(), + ); + let file_system_policy = effective_file_system_sandbox_policy( + &turn_environment + .permission_profile() + .file_system_sandbox_policy(), + additional_permissions.as_ref(), + ); + let requires_sandbox = !file_system_policy.has_full_disk_read_access() + || file_system_policy + .entries + .iter() + .any(|entry| entry.access == FileSystemAccessMode::Deny); + let sandbox = requires_sandbox.then(|| { + turn_context.file_system_sandbox_context(additional_permissions, turn_environment) + }); + if sandbox.is_some() { + let environment_info = turn_environment + .environment + .info() + .await + .map_err(|error| contextualize_error(error.to_string()))?; + if !environment_info.capabilities.sandboxed_file_streaming { + return Err(contextualize_error( + "selected executor does not support sandboxed file streaming".to_string(), + )); + } + } let fs = turn_environment.environment.get_filesystem(); let metadata = fs - .get_metadata(&path_uri, /*sandbox*/ None) + .get_metadata(&path_uri, sandbox.as_ref()) .await .map_err(|error| contextualize_error(error.to_string()))?; if !metadata.is_file { @@ -178,7 +214,7 @@ async fn build_uploaded_argument_value( ))); } let contents = fs - .read_file_stream(&path_uri, /*sandbox*/ None) + .read_file_stream(&path_uri, sandbox.as_ref()) .await .map_err(|error| contextualize_error(error.to_string()))?; let file_name = path_uri @@ -196,7 +232,7 @@ async fn build_uploaded_argument_value( let uploaded = upload_openai_file( turn_context.config.chatgpt_base_url.trim_end_matches('/'), upload_auth.as_ref(), - client_pool, + &sess.services.openai_file_upload_client_pool, file_name, metadata.size, contents, @@ -368,8 +404,8 @@ mod tests { .environments = step_environments; let rewritten = build_uploaded_argument_value( + &session, &step_context, - &session.services.openai_file_upload_client_pool, Some(&auth), FileArgumentLocation { field_name: "file", @@ -406,8 +442,8 @@ mod tests { let step_context = StepContext::for_test(Arc::new(turn_context)); let error = build_uploaded_argument_value( + &session, &step_context, - &session.services.openai_file_upload_client_pool, Some(&auth), FileArgumentLocation { field_name: "file", @@ -483,8 +519,8 @@ mod tests { turn_context.config = Arc::new(config); let step_context = StepContext::for_test(Arc::new(turn_context)); let rewritten = rewrite_argument_value_for_openai_files( + &session, &step_context, - &session.services.openai_file_upload_client_pool, Some(&auth), "file", &[], @@ -597,8 +633,8 @@ mod tests { turn_context.config = Arc::new(config); let step_context = StepContext::for_test(Arc::new(turn_context)); let rewritten = rewrite_argument_value_for_openai_files( + &session, &step_context, - &session.services.openai_file_upload_client_pool, Some(&auth), "files", &[], diff --git a/codex-rs/core/tests/suite/openai_file_mcp.rs b/codex-rs/core/tests/suite/openai_file_mcp.rs index 057ed132cf88..13bcc24bfc0c 100644 --- a/codex-rs/core/tests/suite/openai_file_mcp.rs +++ b/codex-rs/core/tests/suite/openai_file_mcp.rs @@ -5,9 +5,17 @@ use std::path::Path; use anyhow::Context; use anyhow::Result; +use codex_core::config::Config; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use core_test_support::PathExt; use core_test_support::apps_test_server::AppsTestServer; use core_test_support::apps_test_server::CALENDAR_EXTRACT_TEXT_TOOL_NAME; use core_test_support::apps_test_server::DIRECT_CALENDAR_EXTRACT_TEXT_TOOL as DOCUMENT_EXTRACT_HOOK_MATCHER; @@ -27,6 +35,8 @@ use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::namespace_child_tool; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_sandbox; +use core_test_support::skip_if_target_windows; use core_test_support::test_codex::TestCodex; use pretty_assertions::assert_eq; use serde_json::Value; @@ -42,6 +52,37 @@ use wiremock::matchers::path; const STREAMED_FILE_SIZE: usize = 2 * 1024 * 1024; +fn restrict_apps_upload_reads(config: &mut Config, denied_file_name: &str) { + let denied_path = + AbsolutePathBuf::from_absolute_path(config.cwd.as_path().join(denied_file_name)) + .expect("denied file path should be absolute"); + let mut file_system_policy = FileSystemSandboxPolicy::read_only(); + file_system_policy.entries.push(FileSystemSandboxEntry::new( + FileSystemPath::Path { path: denied_path }, + FileSystemAccessMode::Deny, + )); + config + .permissions + .set_permission_profile(PermissionProfile::from_runtime_permissions( + &file_system_policy, + NetworkSandboxPolicy::Restricted, + )) + .expect("test config should allow a restricted read policy"); + + let user_config_path = config.codex_home.join("config.toml").abs(); + let user_config = toml::from_str( + r#" +[apps.calendar] +default_tools_approval_mode = "approve" +"#, + ) + .expect("Apps approval config should parse"); + config.config_layer_stack = config + .config_layer_stack + .with_user_config(&user_config_path, user_config) + .expect("Apps approval config should be valid"); +} + fn write_post_tool_use_hook(home: &Path) -> Result<()> { let script_path = home.join("post_tool_use_hook.py"); let log_path = home.join("post_tool_use_hook_log.jsonl"); @@ -135,7 +176,11 @@ async fn mount_file_upload_mocks(server: &MockServer, file_size_bytes: u64) { .await; } -async fn run_extract_turn(test: &TestCodex, server: &MockServer) -> Result { +async fn run_extract_turn( + test: &TestCodex, + server: &MockServer, + permission_profile: PermissionProfile, +) -> Result { let mock = mount_sse_sequence( server, vec![ @@ -172,7 +217,7 @@ async fn run_extract_turn(test: &TestCodex, server: &MockServer) -> Result Result< Ok(()) }); let test = builder.build_with_auto_env(&server).await?; - let mock = run_extract_turn(&test, &server).await?; + let mock = run_extract_turn(&test, &server, PermissionProfile::Disabled).await?; let requests = mock.requests(); let search_output = requests[1].tool_search_output("extract-search-1"); @@ -269,7 +314,7 @@ async fn codex_apps_file_params_pass_uploaded_file_to_post_tool_use_hook() -> Re }); let test = builder.build(&server).await?; tokio::fs::write(test.cwd.path().join("report.txt"), b"hello world").await?; - let _responses = run_extract_turn(&test, &server).await?; + let _responses = run_extract_turn(&test, &server, PermissionProfile::Disabled).await?; let hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?; assert_eq!(hook_inputs.len(), 1); @@ -281,3 +326,86 @@ async fn codex_apps_file_params_pass_uploaded_file_to_post_tool_use_hook() -> Re server.verify().await; Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn codex_apps_file_params_stream_allowed_file_under_restricted_read_policy() -> Result<()> { + skip_if_target_windows!( + Ok(()), + "Windows restricted-token sandbox cannot enforce deny-read policies" + ); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + mount_file_upload_mocks(&server, STREAMED_FILE_SIZE as u64).await; + + let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url) + .with_config(|config| restrict_apps_upload_reads(config, "private.txt")) + .with_workspace_setup(|cwd, fs| async move { + let report_path = PathUri::from_abs_path(&cwd.join("report.txt")); + fs.write_file( + &report_path, + vec![b'x'; STREAMED_FILE_SIZE], + /*sandbox*/ None, + ) + .await?; + Ok(()) + }); + let test = builder.build_with_auto_env(&server).await?; + let permission_profile = test.config.permissions.permission_profile().clone(); + + run_extract_turn(&test, &server, permission_profile).await?; + + let apps_tool_call = + recorded_apps_tool_call_by_name(&server, CALENDAR_EXTRACT_TEXT_TOOL_NAME).await; + assert_eq!( + apps_tool_call.pointer("/params/arguments/file"), + Some(&schema_filtered_uploaded_file(&server)) + ); + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn codex_apps_file_params_reject_denied_file_before_upload() -> Result<()> { + skip_if_target_windows!( + Ok(()), + "Windows restricted-token sandbox cannot enforce deny-read policies" + ); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + let mut builder = apps_enabled_builder(apps_server.chatgpt_base_url) + .with_config(|config| restrict_apps_upload_reads(config, "report.txt")) + .with_workspace_setup(|cwd, fs| async move { + let report_path = PathUri::from_abs_path(&cwd.join("report.txt")); + fs.write_file(&report_path, b"private".to_vec(), /*sandbox*/ None) + .await?; + Ok(()) + }); + let test = builder.build_with_auto_env(&server).await?; + let permission_profile = test.config.permissions.permission_profile().clone(); + + let responses = run_extract_turn(&test, &server, permission_profile).await?; + let requests = responses.requests(); + let output = requests[2] + .function_call_output_text("extract-call-1") + .context("denied Apps upload should return a tool error")?; + + assert!( + output.contains("failed to upload `report.txt`") + && (output.contains("Permission denied") || output.contains("Operation not permitted")), + "unexpected upload error: {output}" + ); + assert!( + server + .received_requests() + .await + .context("mock server should expose received requests")? + .iter() + .all(|request| request.url.path() != "/files"), + "denied files must not start an upload" + ); + Ok(()) +}