diff --git a/codex-rs/exec-server/src/fs_helper.rs b/codex-rs/exec-server/src/fs_helper.rs index f4db844b5b15..6cffbbcdf4eb 100644 --- a/codex-rs/exec-server/src/fs_helper.rs +++ b/codex-rs/exec-server/src/fs_helper.rs @@ -190,7 +190,11 @@ pub(crate) async fn run_direct_request( match request { FsHelperRequest::ReadFile(params) => { let data = file_system - .read_file(¶ms.path, /*sandbox*/ None) + .read_file_with_limit( + ¶ms.path, + /*sandbox*/ None, + params.max_bytes.unwrap_or(512 * 1024 * 1024), + ) .await .map_err(map_fs_error)?; Ok(FsHelperPayload::ReadFile(FsReadFileResponse { diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index 3129606d6b27..3342c6398160 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -27,10 +27,10 @@ use crate::sandboxed_file_system::SandboxedFileSystem; const MAX_READ_FILE_BYTES: u64 = 512 * 1024 * 1024; -fn file_too_large_error() -> io::Error { +fn file_too_large_error(max_bytes: u64) -> io::Error { io::Error::new( io::ErrorKind::InvalidInput, - format!("file is too large to read: limit is {MAX_READ_FILE_BYTES} bytes"), + format!("file is too large to read: limit is {max_bytes} bytes"), ) } @@ -88,6 +88,24 @@ impl LocalFileSystem { Ok((&self.unsandboxed, sandbox)) } } + + pub(crate) async fn read_file_with_limit( + &self, + path: &PathUri, + sandbox: Option<&FileSystemSandboxContext>, + max_bytes: u64, + ) -> FileSystemResult> { + let max_bytes = max_bytes.min(MAX_READ_FILE_BYTES); + if sandbox.is_some_and(FileSystemSandboxContext::should_run_in_sandbox) { + self.sandboxed()? + .read_file_with_limit(path, sandbox, max_bytes) + .await + } else { + self.unsandboxed + .read_file_with_limit(path, sandbox, max_bytes) + .await + } + } } impl LocalFileSystem { @@ -119,8 +137,8 @@ impl LocalFileSystem { path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { - let (file_system, sandbox) = self.file_system_for(sandbox)?; - file_system.read_file(path, sandbox).await + self.read_file_with_limit(path, sandbox, MAX_READ_FILE_BYTES) + .await } async fn read_file_stream( @@ -306,9 +324,21 @@ impl UnsandboxedFileSystem { &self, path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.read_file_with_limit(path, sandbox, MAX_READ_FILE_BYTES) + .await + } + + async fn read_file_with_limit( + &self, + path: &PathUri, + sandbox: Option<&FileSystemSandboxContext>, + max_bytes: u64, ) -> FileSystemResult> { reject_platform_sandbox_context(sandbox)?; - self.file_system.read_file(path, /*sandbox*/ None).await + self.file_system + .read_file_with_limit(path, /*sandbox*/ None, max_bytes) + .await } async fn read_file_stream( @@ -514,17 +544,26 @@ impl DirectFileSystem { path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult> { + self.read_file_with_limit(path, sandbox, MAX_READ_FILE_BYTES) + .await + } + + pub(crate) async fn read_file_with_limit( + &self, + path: &PathUri, + sandbox: Option<&FileSystemSandboxContext>, + max_bytes: u64, + ) -> FileSystemResult> { + let max_bytes = max_bytes.min(MAX_READ_FILE_BYTES); let file = self.open_file_for_read(path, sandbox).await?; let metadata = file.metadata().await?; - if metadata.len() > MAX_READ_FILE_BYTES { - return Err(file_too_large_error()); + if metadata.len() > max_bytes { + return Err(file_too_large_error(max_bytes)); } let mut bytes = Vec::with_capacity(metadata.len() as usize); - file.take(MAX_READ_FILE_BYTES + 1) - .read_to_end(&mut bytes) - .await?; - if bytes.len() as u64 > MAX_READ_FILE_BYTES { - return Err(file_too_large_error()); + file.take(max_bytes + 1).read_to_end(&mut bytes).await?; + if bytes.len() as u64 > max_bytes { + return Err(file_too_large_error(max_bytes)); } Ok(bytes) } @@ -897,6 +936,33 @@ fn system_time_to_unix_ms(time: SystemTime) -> i64 { #[path = "local_file_system_path_uri_tests.rs"] mod path_uri_tests; +#[cfg(test)] +mod bounded_read_tests { + use super::*; + + #[tokio::test] + async fn direct_read_file_with_limit_rejects_bytes_past_the_boundary() -> io::Result<()> { + let temp_dir = tempfile::TempDir::new()?; + let path = temp_dir.path().join("bounded.txt"); + tokio::fs::write(&path, b"four").await?; + let path = PathUri::from_path(&path).expect("temporary path should be absolute"); + let file_system = DirectFileSystem; + + let error = file_system + .read_file_with_limit(&path, /*sandbox*/ None, /*max_bytes*/ 3) + .await + .expect_err("file should exceed the requested byte boundary"); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + assert_eq!( + file_system + .read_file_with_limit(&path, /*sandbox*/ None, /*max_bytes*/ 4) + .await?, + b"four" + ); + Ok(()) + } +} + #[cfg(all(test, unix))] mod tests { use super::*; diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index e05595f273f6..9c00f8f465b7 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -212,6 +212,8 @@ pub struct TerminateResponse { pub struct FsReadFileParams { pub path: PathUri, pub sandbox: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub max_bytes: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -530,6 +532,7 @@ mod tests { let expected = FsReadFileParams { path: PathUri::from_path(legacy_path).expect("path URI"), sandbox: Some(expected_sandbox.clone()), + max_bytes: None, }; assert_eq!(params, expected); diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index f9136e146108..9b99417de70c 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -69,6 +69,7 @@ impl RemoteFileSystem { .fs_read_file(FsReadFileParams { path: path.clone(), sandbox: remote_sandbox_context(sandbox), + max_bytes: None, }) .await .map_err(map_remote_error)?; diff --git a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs index 1205c6d7e436..6adcf6788082 100644 --- a/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs +++ b/codex-rs/exec-server/src/remote_file_system_path_uri_tests.rs @@ -73,6 +73,7 @@ async fn remote_file_system_sends_path_and_sandbox_cwd_uris_without_native_conve .map(|path| FsReadFileParams { path, sandbox: Some(sandbox.clone()), + max_bytes: None, }) .collect::>(); assert_eq!( diff --git a/codex-rs/exec-server/src/sandboxed_file_system.rs b/codex-rs/exec-server/src/sandboxed_file_system.rs index d0beb57a43a8..49e3d315e1ed 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -77,6 +77,16 @@ impl SandboxedFileSystem { &self, path: &PathUri, sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.read_file_with_limit(path, sandbox, 512 * 1024 * 1024) + .await + } + + pub(crate) async fn read_file_with_limit( + &self, + path: &PathUri, + sandbox: Option<&FileSystemSandboxContext>, + max_bytes: u64, ) -> FileSystemResult> { let sandbox = require_platform_sandbox(sandbox)?; validate_native_path(path)?; @@ -86,6 +96,7 @@ impl SandboxedFileSystem { FsHelperRequest::ReadFile(FsReadFileParams { path: path.clone(), sandbox: None, + max_bytes: Some(max_bytes), }), ) .await? diff --git a/codex-rs/exec-server/src/server/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index ddf17f21cd93..ade2900c1902 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -108,7 +108,11 @@ impl FileSystemHandler { ) -> Result { let bytes = self .file_system - .read_file(¶ms.path, params.sandbox.as_ref()) + .read_file_with_limit( + ¶ms.path, + params.sandbox.as_ref(), + params.max_bytes.unwrap_or(512 * 1024 * 1024), + ) .await .map_err(map_fs_error)?; Ok(FsReadFileResponse { @@ -322,6 +326,7 @@ mod tests { .read_file(FsReadFileParams { path, sandbox: Some(sandbox_context(sandbox_policy)), + max_bytes: None, }) .await .expect("read file"); diff --git a/codex-rs/exec-server/tests/bounded_read.rs b/codex-rs/exec-server/tests/bounded_read.rs new file mode 100644 index 000000000000..0860933f968a --- /dev/null +++ b/codex-rs/exec-server/tests/bounded_read.rs @@ -0,0 +1,95 @@ +#![cfg(unix)] + +mod common; + +use codex_app_server_protocol::JSONRPCMessage; +use codex_app_server_protocol::JSONRPCResponse; +use codex_exec_server::FileSystemSandboxContext; +use codex_exec_server::FsReadFileParams; +use codex_exec_server::InitializeParams; +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_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; +use common::exec_server::ExecServerHarness; +use common::exec_server::exec_server; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn sandbox_helper_enforces_fs_read_file_max_bytes() -> anyhow::Result<()> { + let source_dir = tempfile::tempdir()?; + let source = source_dir.path().join("bounded.txt"); + tokio::fs::write(&source, b"four").await?; + let mut server = exec_server().await?; + initialize_exec_server(&mut server).await?; + + let request_id = server + .send_request( + "fs/readFile", + serde_json::to_value(FsReadFileParams { + path: PathUri::from_path(&source)?, + sandbox: Some(read_only_sandbox(source_dir.path().to_path_buf())?), + max_bytes: Some(3), + })?, + ) + .await?; + let error = server.next_event().await?; + let JSONRPCMessage::Error(error) = error else { + anyhow::bail!("expected JSON-RPC error response, got {error:?}") + }; + assert_eq!(error.id, request_id); + assert_eq!( + (error.error.code, error.error.message), + ( + -32600, + "file is too large to read: limit is 3 bytes".to_string() + ) + ); + + server.shutdown().await?; + Ok(()) +} + +async fn initialize_exec_server(server: &mut ExecServerHarness) -> anyhow::Result<()> { + let initialize_id = server + .send_request( + "initialize", + serde_json::to_value(InitializeParams { + client_name: "bounded-read-test".to_string(), + resume_session_id: None, + })?, + ) + .await?; + let response = server + .wait_for_event(|event| { + matches!( + event, + JSONRPCMessage::Response(JSONRPCResponse { id, .. }) if id == &initialize_id + ) + }) + .await?; + let JSONRPCMessage::Response(JSONRPCResponse { id, .. }) = response else { + anyhow::bail!("expected initialize response") + }; + assert_eq!(id, initialize_id); + server + .send_notification("initialized", serde_json::json!({})) + .await?; + Ok(()) +} + +fn read_only_sandbox(path: std::path::PathBuf) -> anyhow::Result { + let path = AbsolutePathBuf::from_absolute_path(&path)?; + Ok(FileSystemSandboxContext::from_permission_profile( + PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { path }, + access: FileSystemAccessMode::Read, + }]), + NetworkSandboxPolicy::Restricted, + ), + )) +} diff --git a/codex-rs/exec-server/tests/relay.rs b/codex-rs/exec-server/tests/relay.rs index 5f49655e3939..25fb1c0cd3e5 100644 --- a/codex-rs/exec-server/tests/relay.rs +++ b/codex-rs/exec-server/tests/relay.rs @@ -169,6 +169,7 @@ async fn remote_environment_routes_encrypted_exec_server_rpc() -> Result<()> { .fs_read_file(FsReadFileParams { path: PathUri::from_path(large_file_path)?, sandbox: None, + max_bytes: None, }) .await?; assert_eq!(