diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 41d9462f1a..81553fae2e 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -817,7 +817,7 @@ impl McpConnectionManager { let params = serde_json::to_value(params).context("failed to serialize file request")?; let result = client .client - .send_custom_request(method, Some(params), client.tool_timeout) + .send_custom_request_with_timeout(method, Some(params), client.tool_timeout) .await .with_context(|| format!("MCP file request `{method}` failed for `{server}`"))?; let ServerResult::CustomResult(result) = result else { diff --git a/codex-rs/core/src/mcp_file_transfer.rs b/codex-rs/core/src/mcp_file_transfer.rs index 91fc0713db..6869502a59 100644 --- a/codex-rs/core/src/mcp_file_transfer.rs +++ b/codex-rs/core/src/mcp_file_transfer.rs @@ -2,32 +2,30 @@ use std::collections::HashMap; use std::path::PathBuf; -use std::time::Duration; use base64::Engine; use base64::engine::general_purpose::STANDARD; use codex_mcp::FileInputSpec; use codex_mcp::PrepareUploadParams; use codex_protocol::mcp::CallToolResult; -use futures::StreamExt; -use reqwest::Method; -use reqwest::header::ACCEPT; -use reqwest::header::CONTENT_LENGTH; -use reqwest::header::HeaderMap; -use reqwest::header::HeaderValue; -use reqwest::header::USER_AGENT; -use reqwest::redirect::Policy; use serde_json::Value as JsonValue; -use tokio::io::AsyncReadExt; -use tokio::io::AsyncWriteExt; use url::Url; use crate::session::session::Session; use crate::session::turn_context::TurnContext; const DEFAULT_MAX_FILE_BYTES: u64 = 50 * 1024 * 1024; -const TRANSFER_CONNECT_TIMEOUT: Duration = Duration::from_secs(10); -const TRANSFER_TIMEOUT: Duration = Duration::from_secs(5 * 60); + +use self::http::download_transfer_file; +#[cfg(test)] +use self::http::is_disallowed_transfer_address; +#[cfg(test)] +use self::http::is_trusted_direct_transfer_host; +use self::http::put_transfer_file; +#[cfg(test)] +use self::http::validated_transfer_descriptor; + +mod http; #[tracing::instrument( name = "mcp_file_transfer.adapt_input", @@ -114,6 +112,7 @@ pub(crate) async fn materialize_mcp_file_outputs( ); let output_path = unique_output_path(&output_dir, &name).await; let size = download_transfer_file( + sess, &download.transfer, &output_path, download @@ -243,62 +242,6 @@ async fn unique_output_path(output_dir: &std::path::Path, name: &str) -> PathBuf output_dir.join(format!("{}-{name}", uuid::Uuid::new_v4())) } -async fn download_transfer_file( - transfer: &codex_mcp::FileTransferDescriptor, - output_path: &std::path::Path, - max_size: u64, -) -> Result { - let url = validated_transfer_descriptor(transfer, "GET")?; - let response = transfer_client()? - .get(url) - .send() - .await - .map_err(|_| "MCP download transfer request failed".to_string())?; - let status = response.status(); - let response = response - .error_for_status() - .map_err(|_| format!("MCP download transfer returned HTTP {status}"))?; - if response - .content_length() - .is_some_and(|size| size > max_size) - { - return Err(format!("MCP download exceeds the {max_size}-byte limit")); - } - let temporary_path = output_path.with_extension("part"); - let result = async { - let mut output = tokio::fs::File::create(&temporary_path) - .await - .map_err(|error| format!("failed to create MCP download: {error}"))?; - let mut size = 0_u64; - let mut stream = response.bytes_stream(); - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(|error| format!("failed to read MCP download: {error}"))?; - size = size.saturating_add(chunk.len() as u64); - if size > max_size { - return Err(format!("MCP download exceeds the {max_size}-byte limit")); - } - output - .write_all(&chunk) - .await - .map_err(|error| format!("failed to write MCP download: {error}"))?; - } - output - .flush() - .await - .map_err(|error| format!("failed to flush MCP download: {error}"))?; - drop(output); - tokio::fs::rename(&temporary_path, output_path) - .await - .map_err(|error| format!("failed to finalize MCP download: {error}"))?; - Ok(size) - } - .await; - if result.is_err() { - let _ = tokio::fs::remove_file(&temporary_path).await; - } - result -} - async fn rewrite_file_value( sess: &Session, turn_context: &TurnContext, @@ -335,25 +278,45 @@ async fn rewrite_single_file( spec.path )); } - let path = resolve_file_path(turn_context, file_ref)?; - let metadata = tokio::fs::metadata(&path) + let Some(turn_environment) = turn_context.environments.primary() else { + return Err("no primary turn environment is available".to_string()); + }; + let path = resolve_file_path(turn_environment.cwd_uri(), file_ref)?; + let path_display = path.to_url().to_string(); + let sandbox = turn_context.file_system_sandbox_context( + /*additional_permissions*/ None, + turn_environment.cwd_uri(), + ); + let fs = turn_environment.environment.get_filesystem(); + let metadata = fs + .get_metadata(&path, Some(&sandbox)) .await - .map_err(|error| format!("failed to read `{}`: {error}", path.display()))?; - if !metadata.is_file() { - return Err(format!("`{}` is not a regular file", path.display())); + .map_err(|error| format!("failed to read `{path_display}`: {error}"))?; + if !metadata.is_file { + return Err(format!("`{path_display}` is not a regular file")); } let max_size = spec .max_size .unwrap_or(DEFAULT_MAX_FILE_BYTES) .min(DEFAULT_MAX_FILE_BYTES); - if metadata.len() > max_size { + if metadata.size > max_size { return Err(format!( - "file `{}` is {} bytes, exceeding the {max_size}-byte limit", - path.display(), - metadata.len() + "file `{path_display}` is {} bytes, exceeding the {max_size}-byte limit", + metadata.size )); } - let mime_type = mime_guess::from_path(&path) + let bytes = fs + .read_file(&path, Some(&sandbox)) + .await + .map_err(|error| format!("failed to read `{path_display}`: {error}"))?; + let size = bytes.len() as u64; + if size > max_size { + return Err(format!( + "file `{path_display}` exceeds the {max_size}-byte limit" + )); + } + let name = path.basename().unwrap_or_else(|| "upload".to_string()); + let mime_type = mime_guess::from_path(&name) .first_raw() .unwrap_or("application/octet-stream"); if !spec.accepts.is_empty() @@ -363,8 +326,7 @@ async fn rewrite_single_file( .any(|accept| mime_matches(accept, mime_type)) { return Err(format!( - "file `{}` has MIME type `{mime_type}`, which is not accepted by `{}`", - path.display(), + "file `{path_display}` has MIME type `{mime_type}`, which is not accepted by `{}`", spec.path )); } @@ -376,27 +338,22 @@ async fn rewrite_single_file( if spec.accepts_upload() && capabilities.prepare_upload && capabilities.complete_upload { tracing::debug!( transfer_mode = "upload", - size_bucket = file_size_bucket(metadata.len()), + size_bucket = file_size_bucket(size), "adapting MCP file input" ); - let name = path - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("upload") - .to_string(); let prepared = manager .prepare_file_upload( server, PrepareUploadParams { name, mime_type: mime_type.to_string(), - size: metadata.len(), + size, }, ) .await .map_err(|error| format!("failed to prepare MCP file upload: {error:#}"))?; validate_mcp_file_uri(&prepared.file.uri)?; - put_transfer_file(&prepared.transfer, &path, max_size).await?; + put_transfer_file(sess, &prepared.transfer, bytes, max_size).await?; let completed = manager .complete_file_upload(server, prepared.file.uri) .await @@ -407,23 +364,9 @@ async fn rewrite_single_file( if spec.accepts_inline() { tracing::debug!( transfer_mode = "inline", - size_bucket = file_size_bucket(metadata.len()), + size_bucket = file_size_bucket(size), "adapting MCP file input" ); - let file = tokio::fs::File::open(&path) - .await - .map_err(|error| format!("failed to open `{}`: {error}", path.display()))?; - let mut bytes = Vec::with_capacity(metadata.len() as usize); - file.take(max_size.saturating_add(1)) - .read_to_end(&mut bytes) - .await - .map_err(|error| format!("failed to read `{}`: {error}", path.display()))?; - if bytes.len() as u64 > max_size { - return Err(format!( - "file `{}` exceeds the {max_size}-byte limit", - path.display() - )); - } return Ok(JsonValue::String(format!( "data:{mime_type};base64,{}", STANDARD.encode(bytes) @@ -469,153 +412,20 @@ fn file_size_bucket(size: u64) -> &'static str { } } -fn resolve_file_path(turn_context: &TurnContext, file_ref: &str) -> Result { +fn resolve_file_path( + cwd: &codex_utils_path_uri::PathUri, + file_ref: &str, +) -> Result { if file_ref.starts_with("file:") { - return Url::parse(file_ref) - .map_err(|error| format!("invalid file URI: {error}"))? - .to_file_path() - .map_err(|()| "file URI does not identify a local path".to_string()); + return codex_utils_path_uri::PathUri::parse(file_ref) + .map_err(|error| format!("invalid file URI: {error}")); } - #[allow(deprecated)] - Ok(turn_context - .resolve_path(Some(file_ref.to_string())) - .to_path_buf()) -} - -async fn put_transfer_file( - transfer: &codex_mcp::FileTransferDescriptor, - path: &std::path::Path, - max_size: u64, -) -> Result<(), String> { - let url = validated_upload_transfer_descriptor(transfer)?; - let method = Method::from_bytes(transfer.method.as_bytes()) - .map_err(|error| format!("invalid MCP transfer method: {error}"))?; - let size = tokio::fs::metadata(path) - .await - .map_err(|error| format!("failed to inspect MCP upload: {error}"))? - .len(); - if size > max_size { - return Err(format!("MCP upload exceeds the {max_size}-byte limit")); + if std::path::Path::new(file_ref).is_absolute() { + return codex_utils_path_uri::PathUri::from_path(file_ref) + .map_err(|error| format!("invalid absolute file path: {error}")); } - let file = tokio::fs::File::open(path) - .await - .map_err(|error| format!("failed to open MCP upload: {error}"))?; - let stream = futures::stream::try_unfold((file, 0_u64), move |(mut file, sent)| async move { - let mut buffer = vec![0_u8; 64 * 1024]; - let read = file.read(&mut buffer).await?; - if read == 0 { - return Ok(None); - } - buffer.truncate(read); - let sent = sent.saturating_add(read as u64); - if sent > max_size { - return Err(std::io::Error::other("MCP upload exceeded its size limit")); - } - Ok(Some((buffer, (file, sent)))) - }); - let azure_blob_upload = url.host_str().is_some_and(|host| { - host.ends_with(".blob.core.windows.net") || host.ends_with(".oaiusercontent.com") - }); - let mut request = transfer_client()? - .request(method, url) - .header(CONTENT_LENGTH, size) - .body(reqwest::Body::wrap_stream(stream)); - if azure_blob_upload { - request = request.header("x-ms-blob-type", "BlockBlob"); - } - let response = request - .send() - .await - .map_err(|_| "MCP upload transfer request failed".to_string())?; - let status = response.status(); - response - .error_for_status() - .map_err(|_| format!("MCP upload transfer returned HTTP {status}"))?; - Ok(()) -} - -fn validated_transfer_url(url: &str) -> Result { - let url = Url::parse(url).map_err(|error| format!("invalid MCP transfer URL: {error}"))?; - let local_http = cfg!(test) - && url.scheme() == "http" - && url - .host_str() - .is_some_and(|host| host == "localhost" || host == "127.0.0.1" || host == "::1"); - if url.scheme() != "https" && !local_http { - return Err("MCP transfer URL must use HTTPS".to_string()); - } - if !url.username().is_empty() || url.password().is_some() { - return Err("MCP transfer URL must not contain credentials".to_string()); - } - if let Some(url::Host::Ipv4(address)) = url.host() - && (address.is_private() - || address.is_link_local() - || address.is_loopback() - || address.is_unspecified()) - && !local_http - { - return Err("MCP transfer URL must not target a private address".to_string()); - } - if let Some(url::Host::Ipv6(address)) = url.host() - && (address.is_loopback() - || address.is_unspecified() - || (address.segments()[0] & 0xffc0) == 0xfe80) - && !local_http - { - return Err("MCP transfer URL must not target a private address".to_string()); - } - Ok(url) -} - -fn validated_transfer_descriptor( - transfer: &codex_mcp::FileTransferDescriptor, - expected_method: &str, -) -> Result { - if transfer - .transport - .as_deref() - .is_some_and(|value| value != "https") - { - return Err("MCP transfer transport must be HTTPS".to_string()); - } - if transfer.method != expected_method { - return Err(format!("MCP transfer method must be {expected_method}")); - } - if let Some(expires_at) = transfer.expires_at.as_deref() { - let expires_at = chrono::DateTime::parse_from_rfc3339(expires_at) - .map_err(|error| format!("invalid MCP transfer expiry: {error}"))?; - if expires_at <= chrono::Utc::now() { - return Err("MCP transfer descriptor has expired".to_string()); - } - } - validated_transfer_url(&transfer.url) -} - -fn validated_upload_transfer_descriptor( - transfer: &codex_mcp::FileTransferDescriptor, -) -> Result { - if !matches!(transfer.method.as_str(), "PUT" | "POST") { - return Err("MCP upload transfer method must be PUT or POST".to_string()); - } - validated_transfer_descriptor(transfer, &transfer.method) -} - -fn transfer_client() -> Result { - let mut headers = HeaderMap::new(); - headers.insert(ACCEPT, HeaderValue::from_static("*/*")); - headers.insert( - USER_AGENT, - HeaderValue::from_static("Mozilla/5.0 Codex MCP File Transfer"), - ); - let client = reqwest::Client::builder() - .default_headers(headers) - .connect_timeout(TRANSFER_CONNECT_TIMEOUT) - .timeout(TRANSFER_TIMEOUT) - .no_proxy() - .redirect(Policy::none()) - .build() - .map_err(|error| format!("failed to build MCP transfer client: {error}"))?; - Ok(client) + cwd.join(file_ref) + .map_err(|error| format!("invalid file path `{file_ref}`: {error}")) } #[cfg(test)] diff --git a/codex-rs/core/src/mcp_file_transfer/http.rs b/codex-rs/core/src/mcp_file_transfer/http.rs new file mode 100644 index 0000000000..c255786ac6 --- /dev/null +++ b/codex-rs/core/src/mcp_file_transfer/http.rs @@ -0,0 +1,301 @@ +use std::net::IpAddr; +use std::time::Duration; + +use codex_network_proxy::NetworkProxy; +use futures::StreamExt; +use reqwest::Method; +use reqwest::header::ACCEPT; +use reqwest::header::CONTENT_LENGTH; +use reqwest::header::HeaderMap; +use reqwest::header::HeaderValue; +use reqwest::header::USER_AGENT; +use reqwest::redirect::Policy; +use tokio::io::AsyncWriteExt; +use url::Url; + +use crate::session::session::Session; + +const TRANSFER_CONNECT_TIMEOUT: Duration = Duration::from_secs(10); +const TRANSFER_TIMEOUT: Duration = Duration::from_secs(5 * 60); + +pub(super) async fn download_transfer_file( + sess: &Session, + transfer: &codex_mcp::FileTransferDescriptor, + output_path: &std::path::Path, + max_size: u64, +) -> Result { + let url = validated_transfer_descriptor(transfer, "GET")?; + let response = transfer_client(sess, &url) + .await? + .get(url) + .send() + .await + .map_err(|_| "MCP download transfer request failed".to_string())?; + let status = response.status(); + let response = response + .error_for_status() + .map_err(|_| format!("MCP download transfer returned HTTP {status}"))?; + if response + .content_length() + .is_some_and(|size| size > max_size) + { + return Err(format!("MCP download exceeds the {max_size}-byte limit")); + } + let temporary_path = output_path.with_extension("part"); + let result = async { + let mut output = tokio::fs::File::create(&temporary_path) + .await + .map_err(|error| format!("failed to create MCP download: {error}"))?; + let mut size = 0_u64; + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|error| format!("failed to read MCP download: {error}"))?; + size = size.saturating_add(chunk.len() as u64); + if size > max_size { + return Err(format!("MCP download exceeds the {max_size}-byte limit")); + } + output + .write_all(&chunk) + .await + .map_err(|error| format!("failed to write MCP download: {error}"))?; + } + output + .flush() + .await + .map_err(|error| format!("failed to flush MCP download: {error}"))?; + drop(output); + tokio::fs::rename(&temporary_path, output_path) + .await + .map_err(|error| format!("failed to finalize MCP download: {error}"))?; + Ok(size) + } + .await; + if result.is_err() { + let _ = tokio::fs::remove_file(&temporary_path).await; + } + result +} + +pub(super) async fn put_transfer_file( + sess: &Session, + transfer: &codex_mcp::FileTransferDescriptor, + bytes: Vec, + max_size: u64, +) -> Result<(), String> { + let url = validated_upload_transfer_descriptor(transfer)?; + let method = Method::from_bytes(transfer.method.as_bytes()) + .map_err(|error| format!("invalid MCP transfer method: {error}"))?; + let size = bytes.len() as u64; + if size > max_size { + return Err(format!("MCP upload exceeds the {max_size}-byte limit")); + } + let stream = futures::stream::once(async move { Ok::<_, std::io::Error>(bytes) }); + let azure_blob_upload = url.host_str().is_some_and(|host| { + host.ends_with(".blob.core.windows.net") || host.ends_with(".oaiusercontent.com") + }); + let mut request = transfer_client(sess, &url) + .await? + .request(method, url) + .header(CONTENT_LENGTH, size) + .body(reqwest::Body::wrap_stream(stream)); + if azure_blob_upload { + request = request.header("x-ms-blob-type", "BlockBlob"); + } + let response = request + .send() + .await + .map_err(|_| "MCP upload transfer request failed".to_string())?; + let status = response.status(); + response + .error_for_status() + .map_err(|_| format!("MCP upload transfer returned HTTP {status}"))?; + Ok(()) +} + +pub(super) fn validated_transfer_descriptor( + transfer: &codex_mcp::FileTransferDescriptor, + expected_method: &str, +) -> Result { + if transfer + .transport + .as_deref() + .is_some_and(|value| value != "https") + { + return Err("MCP transfer transport must be HTTPS".to_string()); + } + if transfer.method != expected_method { + return Err(format!("MCP transfer method must be {expected_method}")); + } + if let Some(expires_at) = transfer.expires_at.as_deref() { + let expires_at = chrono::DateTime::parse_from_rfc3339(expires_at) + .map_err(|error| format!("invalid MCP transfer expiry: {error}"))?; + if expires_at <= chrono::Utc::now() { + return Err("MCP transfer descriptor has expired".to_string()); + } + } + validated_transfer_url(&transfer.url) +} + +fn validated_upload_transfer_descriptor( + transfer: &codex_mcp::FileTransferDescriptor, +) -> Result { + if !matches!(transfer.method.as_str(), "PUT" | "POST") { + return Err("MCP upload transfer method must be PUT or POST".to_string()); + } + validated_transfer_descriptor(transfer, &transfer.method) +} + +fn validated_transfer_url(url: &str) -> Result { + let url = Url::parse(url).map_err(|error| format!("invalid MCP transfer URL: {error}"))?; + let local_http = is_local_test_url(&url); + if url.scheme() != "https" && !local_http { + return Err("MCP transfer URL must use HTTPS".to_string()); + } + if !url.username().is_empty() || url.password().is_some() { + return Err("MCP transfer URL must not contain credentials".to_string()); + } + if url + .host() + .and_then(|host| match host { + url::Host::Ipv4(address) => Some(IpAddr::V4(address)), + url::Host::Ipv6(address) => Some(IpAddr::V6(address)), + url::Host::Domain(_) => None, + }) + .is_some_and(is_disallowed_transfer_address) + && !local_http + { + return Err("MCP transfer URL must not target a private address".to_string()); + } + Ok(url) +} + +async fn transfer_client(sess: &Session, url: &Url) -> Result { + let network = sess + .services + .network_proxy + .load_full() + .map_or(TransferNetwork::Direct, |started| { + TransferNetwork::Managed(started.proxy()) + }); + build_transfer_client(url, network).await +} + +enum TransferNetwork { + Managed(NetworkProxy), + Direct, +} + +async fn build_transfer_client( + url: &Url, + network: TransferNetwork, +) -> Result { + let mut headers = HeaderMap::new(); + headers.insert(ACCEPT, HeaderValue::from_static("*/*")); + headers.insert( + USER_AGENT, + HeaderValue::from_static("Mozilla/5.0 Codex MCP File Transfer"), + ); + let mut builder = reqwest::Client::builder() + .default_headers(headers) + .connect_timeout(TRANSFER_CONNECT_TIMEOUT) + .timeout(TRANSFER_TIMEOUT) + .no_proxy() + .redirect(Policy::none()); + match network { + TransferNetwork::Managed(proxy) => { + let proxy_url = format!("http://{}", proxy.http_addr()); + let proxy = reqwest::Proxy::all(&proxy_url) + .map_err(|error| format!("failed to configure MCP transfer proxy: {error}"))?; + builder = builder.proxy(proxy); + } + TransferNetwork::Direct => { + builder = pin_public_transfer_addresses(builder, url).await?; + } + } + builder + .build() + .map_err(|error| format!("failed to build MCP transfer client: {error}")) +} + +async fn pin_public_transfer_addresses( + mut builder: reqwest::ClientBuilder, + url: &Url, +) -> Result { + let Some(host) = url.host_str() else { + return Err("MCP transfer URL must contain a host".to_string()); + }; + if is_local_test_url(url) { + return Ok(builder); + } + if !is_trusted_direct_transfer_host(host) { + return Err( + "MCP transfer URL requires the managed network proxy or a trusted transfer host" + .to_string(), + ); + } + let port = url + .port_or_known_default() + .ok_or_else(|| "MCP transfer URL must contain a valid port".to_string())?; + let addresses = tokio::net::lookup_host((host, port)) + .await + .map_err(|_| "failed to resolve MCP transfer host".to_string())? + .collect::>(); + if addresses.is_empty() { + return Err("MCP transfer host did not resolve".to_string()); + } + if addresses + .iter() + .any(|address| is_disallowed_transfer_address(address.ip())) + { + return Err("MCP transfer URL must not target a private address".to_string()); + } + if matches!(url.host(), Some(url::Host::Domain(_))) { + builder = builder.resolve_to_addrs(host, &addresses); + } + Ok(builder) +} + +pub(super) fn is_trusted_direct_transfer_host(host: &str) -> bool { + let host = host.trim_end_matches('.').to_ascii_lowercase(); + host.ends_with(".blob.core.windows.net") || host.ends_with(".oaiusercontent.com") +} + +fn is_local_test_url(url: &Url) -> bool { + cfg!(test) + && url.scheme() == "http" + && url + .host_str() + .is_some_and(|host| host == "localhost" || host == "127.0.0.1" || host == "::1") +} + +pub(super) fn is_disallowed_transfer_address(address: IpAddr) -> bool { + match address { + IpAddr::V4(address) => { + let octets = address.octets(); + address.is_private() + || address.is_link_local() + || address.is_loopback() + || address.is_unspecified() + || address.is_multicast() + || address.is_broadcast() + || address.is_documentation() + || octets[0] == 0 + || (octets[0] == 100 && (64..=127).contains(&octets[1])) + || (octets[0] == 192 && octets[1] == 0 && octets[2] == 0) + || (octets[0] == 198 && (18..=19).contains(&octets[1])) + || octets[0] >= 240 + } + IpAddr::V6(address) => { + let segments = address.segments(); + address.is_loopback() + || address.is_unspecified() + || address.is_multicast() + || (segments[0] & 0xfe00) == 0xfc00 + || (segments[0] & 0xffc0) == 0xfe80 + || (segments[0] == 0x2001 && segments[1] == 0x0db8) + || address + .to_ipv4_mapped() + .is_some_and(|address| is_disallowed_transfer_address(IpAddr::V4(address))) + } + } +} diff --git a/codex-rs/core/src/mcp_file_transfer_tests.rs b/codex-rs/core/src/mcp_file_transfer_tests.rs index d6238325c2..69394b7818 100644 --- a/codex-rs/core/src/mcp_file_transfer_tests.rs +++ b/codex-rs/core/src/mcp_file_transfer_tests.rs @@ -30,16 +30,18 @@ fn model_file_ref_accepts_string_and_blob_like_values() { #[tokio::test] async fn transfer_rejects_non_https_remote_urls() { + let (session, _) = crate::session::tests::make_session_and_context().await; assert_eq!( put_transfer_file( + &session, &codex_mcp::FileTransferDescriptor { transport: Some("https".to_string()), method: "PUT".to_string(), url: "http://example.com/upload".to_string(), expires_at: None, }, - std::path::Path::new("unused"), - 0, + Vec::new(), + /*max_size*/ 0, ) .await .expect_err("remote HTTP must be rejected"), @@ -122,9 +124,51 @@ fn validates_opaque_mcp_file_uris() { ); } +#[test] +fn rejects_non_public_transfer_addresses() { + for address in [ + "127.0.0.1", + "10.0.0.1", + "100.64.0.1", + "169.254.169.254", + "192.168.0.1", + "198.18.0.1", + "224.0.0.1", + "::1", + "fc00::1", + "fe80::1", + "ff02::1", + "::ffff:127.0.0.1", + ] { + assert!(is_disallowed_transfer_address( + address.parse().expect("IP address") + )); + } + for address in ["1.1.1.1", "8.8.8.8", "2606:4700:4700::1111"] { + assert!(!is_disallowed_transfer_address( + address.parse().expect("IP address") + )); + } +} + +#[test] +fn direct_transfers_require_trusted_storage_hosts() { + assert!(is_trusted_direct_transfer_host( + "account.blob.core.windows.net" + )); + assert!(is_trusted_direct_transfer_host("files.oaiusercontent.com")); + assert!(is_trusted_direct_transfer_host("FILES.OAIUSERCONTENT.COM.")); + assert!(!is_trusted_direct_transfer_host("oaiusercontent.com")); + assert!(!is_trusted_direct_transfer_host( + "files.oaiusercontent.com.attacker.example" + )); + assert!(!is_trusted_direct_transfer_host("example.com")); +} + #[tokio::test] async fn inline_rewrite_turns_a_local_path_into_a_data_uri() { - let (session, turn_context) = crate::session::tests::make_session_and_context().await; + let (session, mut turn_context) = crate::session::tests::make_session_and_context().await; + turn_context.permission_profile = codex_protocol::models::PermissionProfile::Disabled; let directory = tempfile::tempdir().expect("temp dir"); let path = directory.path().join("report.txt"); tokio::fs::write(&path, b"hello") @@ -159,6 +203,7 @@ async fn inline_rewrite_turns_a_local_path_into_a_data_uri() { #[tokio::test] async fn upload_transfer_streams_the_exact_file_bytes() { + let (session, _) = crate::session::tests::make_session_and_context().await; let server = MockServer::start().await; Mock::given(method("PUT")) .and(path("/upload")) @@ -168,21 +213,16 @@ async fn upload_transfer_streams_the_exact_file_bytes() { .expect(1) .mount(&server) .await; - let directory = tempdir().expect("temp dir"); - let file = directory.path().join("upload.txt"); - tokio::fs::write(&file, b"stream me") - .await - .expect("write upload"); - put_transfer_file( + &session, &codex_mcp::FileTransferDescriptor { transport: Some("https".to_string()), method: "PUT".to_string(), url: format!("{}/upload", server.uri()), expires_at: None, }, - &file, - 32, + b"stream me".to_vec(), + /*max_size*/ 32, ) .await .expect("upload succeeds"); @@ -190,6 +230,7 @@ async fn upload_transfer_streams_the_exact_file_bytes() { #[tokio::test] async fn upload_transfer_accepts_post_descriptors() { + let (session, _) = crate::session::tests::make_session_and_context().await; let server = MockServer::start().await; Mock::given(method("POST")) .and(path("/upload")) @@ -199,21 +240,16 @@ async fn upload_transfer_accepts_post_descriptors() { .expect(1) .mount(&server) .await; - let directory = tempdir().expect("temp dir"); - let file = directory.path().join("upload.txt"); - tokio::fs::write(&file, b"stream me") - .await - .expect("write upload"); - put_transfer_file( + &session, &codex_mcp::FileTransferDescriptor { transport: None, method: "POST".to_string(), url: format!("{}/upload", server.uri()), expires_at: None, }, - &file, - 32, + b"stream me".to_vec(), + /*max_size*/ 32, ) .await .expect("upload succeeds"); @@ -221,6 +257,7 @@ async fn upload_transfer_accepts_post_descriptors() { #[tokio::test] async fn download_transfer_materializes_exact_bytes() { + let (session, _) = crate::session::tests::make_session_and_context().await; let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/download")) @@ -232,6 +269,7 @@ async fn download_transfer_materializes_exact_bytes() { let output = directory.path().join("download.txt"); let size = download_transfer_file( + &session, &codex_mcp::FileTransferDescriptor { transport: Some("https".to_string()), method: "GET".to_string(), @@ -239,7 +277,7 @@ async fn download_transfer_materializes_exact_bytes() { expires_at: None, }, &output, - 32, + /*max_size*/ 32, ) .await .expect("download succeeds"); @@ -253,6 +291,7 @@ async fn download_transfer_materializes_exact_bytes() { #[tokio::test] async fn transfer_errors_do_not_expose_signed_urls() { + let (session, _) = crate::session::tests::make_session_and_context().await; let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/download")) @@ -264,6 +303,7 @@ async fn transfer_errors_do_not_expose_signed_urls() { let secret = "sensitive-signature"; let error = download_transfer_file( + &session, &codex_mcp::FileTransferDescriptor { transport: None, method: "GET".to_string(), @@ -271,7 +311,7 @@ async fn transfer_errors_do_not_expose_signed_urls() { expires_at: None, }, &output, - 32, + /*max_size*/ 32, ) .await .expect_err("failed transfer"); @@ -285,6 +325,7 @@ async fn transfer_errors_do_not_expose_signed_urls() { #[tokio::test] async fn failed_download_removes_partial_file() { + let (session, _) = crate::session::tests::make_session_and_context().await; let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/download")) @@ -295,6 +336,7 @@ async fn failed_download_removes_partial_file() { let output = directory.path().join("download.txt"); let error = download_transfer_file( + &session, &codex_mcp::FileTransferDescriptor { transport: None, method: "GET".to_string(), @@ -302,7 +344,7 @@ async fn failed_download_removes_partial_file() { expires_at: None, }, &output, - 32, + /*max_size*/ 32, ) .await .expect_err("oversized transfer"); diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index 7d466fb53d..2b879e75ca 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -26,7 +26,6 @@ use codex_sandboxing::SandboxType; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use core_test_support::PathBufExt; -use core_test_support::PathExt; use pretty_assertions::assert_eq; use std::path::PathBuf; use std::process::Command; diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 4336b4335b..e734c41a0b 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -32,6 +32,7 @@ use codex_mcp::MCP_SANDBOX_STATE_META_CAPABILITY; use codex_models_manager::manager::RefreshStrategy; use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::models::ManagedFileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::InputModality; @@ -40,6 +41,10 @@ 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::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::McpInvocation; @@ -675,6 +680,120 @@ async fn stdio_server_adapts_inline_mcp_file_input() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn stdio_server_rejects_mcp_file_input_denied_by_turn_sandbox() -> anyhow::Result<()> { + let model_server = responses::start_mock_server().await; + let files = tempdir()?; + let denied_path = files.path().join("denied.txt"); + let call_marker = files.path().join("tool-called"); + fs::write(&denied_path, b"must not leave the sandbox")?; + let call_id = "file-call-denied"; + let server_name = "rmcp_files_denied"; + let namespace = format!("mcp__{server_name}"); + let arguments = serde_json::to_string(&json!({"file": denied_path}))?; + responses::mount_sse_sequence( + &model_server, + vec![ + responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call_with_namespace( + call_id, + &namespace, + "file_round_trip", + &arguments, + ), + responses::ev_completed("resp-1"), + ]), + responses::sse(vec![ + responses::ev_assistant_message("msg-1", "The file read was denied."), + responses::ev_completed("resp-2"), + ]), + ], + ) + .await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let call_marker_for_config = call_marker.clone(); + let fixture = test_codex() + .with_config(move |config| { + config + .features + .enable(Feature::McpFileTransfer) + .expect("test feature should enable"); + insert_mcp_server( + config, + server_name, + stdio_transport( + rmcp_test_server_bin, + Some(HashMap::from([ + ("MCP_TEST_FILE_INLINE".to_string(), "1".to_string()), + ( + "MCP_TEST_FILE_CALL_MARKER".to_string(), + call_marker_for_config.display().to_string(), + ), + ])), + Vec::new(), + ), + TestMcpServerOptions::default(), + ); + }) + .build(&model_server) + .await?; + wait_for_mcp_server(&fixture.codex, server_name).await?; + + let permission_profile = PermissionProfile::Managed { + file_system: ManagedFileSystemPermissions::Restricted { + entries: vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_path.as_path().try_into()?, + }, + access: FileSystemAccessMode::Deny, + }, + ], + glob_scan_max_depth: None, + }, + network: NetworkSandboxPolicy::Restricted, + }; + fixture + .codex + .submit(user_turn_with_permission_profile( + &fixture, + "send the denied local file", + fixture.session_configured.model.clone(), + permission_profile, + )) + .await?; + + let end_event = wait_for_event(&fixture.codex, |event| { + matches!(event, EventMsg::McpToolCallEnd(_)) + }) + .await; + let EventMsg::McpToolCallEnd(end) = end_event else { + unreachable!("event guard guarantees McpToolCallEnd"); + }; + let error = end.result.expect_err("sandbox-denied file should fail"); + assert!( + error.contains("failed to read"), + "unexpected error: {error}" + ); + assert!( + !call_marker.exists(), + "MCP server must not receive file input" + ); + wait_for_event(&fixture.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shutdown_cancels_startup_prewarm_waiting_for_mcp_startup() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/rmcp-client/src/bin/test_stdio_server.rs b/codex-rs/rmcp-client/src/bin/test_stdio_server.rs index cfe184d6f6..a5359ea2a6 100644 --- a/codex-rs/rmcp-client/src/bin/test_stdio_server.rs +++ b/codex-rs/rmcp-client/src/bin/test_stdio_server.rs @@ -45,6 +45,7 @@ const MEMO_CONTENT: &str = "This is a sample MCP resource served by the rmcp tes const SANDBOX_STATE_META_CAPABILITY: &str = "codex/sandbox-state-meta"; const SMALL_PNG_BASE64: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg=="; const FILE_INLINE_ENV: &str = "MCP_TEST_FILE_INLINE"; +const FILE_CALL_MARKER_ENV: &str = "MCP_TEST_FILE_CALL_MARKER"; pub fn stdio() -> (tokio::io::Stdin, tokio::io::Stdout) { (tokio::io::stdin(), tokio::io::stdout()) @@ -189,11 +190,13 @@ impl TestToolServer { "additionalProperties": false })) .expect("file round-trip tool schema should deserialize"); - Tool::new( + let mut tool = Tool::new( Cow::Borrowed("file_round_trip"), Cow::Borrowed("Accept an uploaded file and return a downloadable file."), Arc::new(schema), - ) + ); + tool.annotations = Some(ToolAnnotations::new().read_only(true)); + tool } fn sync_tool() -> Tool { @@ -517,6 +520,10 @@ impl ServerHandler for TestToolServer { ) -> Result { match request.name.as_ref() { "file_round_trip" => { + if let Ok(marker) = std::env::var(FILE_CALL_MARKER_ENV) { + std::fs::write(marker, b"called") + .map_err(|err| McpError::internal_error(err.to_string(), None))?; + } let args = Self::parse_call_args::(&request, "file_round_trip")?; if args.file != "data:text/plain;base64,dXBsb2FkIG1l" { return Err(McpError::invalid_params( diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index bde88dea7c..35b269aef9 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -669,6 +669,15 @@ impl RmcpClient { &self, method: &str, params: Option, + ) -> Result { + self.send_custom_request_with_timeout(method, params, /*timeout*/ None) + .await + } + + pub async fn send_custom_request_with_timeout( + &self, + method: &str, + params: Option, timeout: Option, ) -> Result { self.refresh_oauth_if_needed().await;