openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Honor filesystem permissions for app file uploads (#38416)

Browse Source 
## Why

App tool file arguments could read and upload files without applying the active
filesystem sandbox policy.

## What changed

- Apply the effective filesystem policy, including session and turn grants, when
  reading metadata and streaming files for upload.
- Reject uploads before contacting the files endpoint when the selected path is
  denied.
- Report an error when the executor cannot provide sandboxed file streaming for
  a restricted policy.

## Testing

Added integration coverage showing that restricted policies allow permitted
files and reject denied files before upload.

GitOrigin-RevId: 63b239c5066dcb9d18b5050089c659fb2a784909
This commit is contained in:
jif
2026-08-13 18:36:12 +00:00
committed by copyberry
parent 2aba3219e6
commit 4f7032173e
2 changed files with 181 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/mcp_openai_file.rs
+49 -13
View File
@@ -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",
&[],
codex-rs/core/tests/suite/openai_file_mcp.rs
+132 -4
View File
@@ -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<ResponseMock> {
async fn run_extract_turn(
test: &TestCodex,
server: &MockServer,
permission_profile: PermissionProfile,
) -> Result<ResponseMock> {
let mock = mount_sse_sequence(
server,
vec![
@@ -172,7 +217,7 @@ async fn run_extract_turn(test: &TestCodex, server: &MockServer) -> Result<Respo
test.submit_turn_with_approval_and_permission_profile(
"Extract the report text with the app tool.",
AskForApproval::Never,
PermissionProfile::Disabled,
permission_profile,
)
.await?;
@@ -197,7 +242,7 @@ async fn codex_apps_file_params_omit_fields_absent_from_tool_schema() -> 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(())
}