Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion codex-rs/exec-server/src/fs_helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,11 @@ pub(crate) async fn run_direct_request(
match request {
FsHelperRequest::ReadFile(params) => {
let data = file_system
.read_file(&params.path, /*sandbox*/ None)
.read_file_with_limit(
&params.path,
/*sandbox*/ None,
params.max_bytes.unwrap_or(512 * 1024 * 1024),
)
.await
.map_err(map_fs_error)?;
Ok(FsHelperPayload::ReadFile(FsReadFileResponse {
Expand Down
90 changes: 78 additions & 12 deletions codex-rs/exec-server/src/local_file_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
)
}

Expand Down Expand Up @@ -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<Vec<u8>> {
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 {
Expand Down Expand Up @@ -119,8 +137,8 @@ impl LocalFileSystem {
path: &PathUri,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
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(
Expand Down Expand Up @@ -306,9 +324,21 @@ impl UnsandboxedFileSystem {
&self,
path: &PathUri,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
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<Vec<u8>> {
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(
Expand Down Expand Up @@ -514,17 +544,26 @@ impl DirectFileSystem {
path: &PathUri,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
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<Vec<u8>> {
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)
}
Expand Down Expand Up @@ -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::*;
Expand Down
3 changes: 3 additions & 0 deletions codex-rs/exec-server/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ pub struct TerminateResponse {
pub struct FsReadFileParams {
pub path: PathUri,
pub sandbox: Option<FileSystemSandboxContext>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub max_bytes: Option<u64>,
}

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions codex-rs/exec-server/src/remote_file_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();
assert_eq!(
Expand Down
11 changes: 11 additions & 0 deletions codex-rs/exec-server/src/sandboxed_file_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,16 @@ impl SandboxedFileSystem {
&self,
path: &PathUri,
sandbox: Option<&FileSystemSandboxContext>,
) -> FileSystemResult<Vec<u8>> {
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<Vec<u8>> {
let sandbox = require_platform_sandbox(sandbox)?;
validate_native_path(path)?;
Expand All @@ -86,6 +96,7 @@ impl SandboxedFileSystem {
FsHelperRequest::ReadFile(FsReadFileParams {
path: path.clone(),
sandbox: None,
max_bytes: Some(max_bytes),
}),
)
.await?
Expand Down
7 changes: 6 additions & 1 deletion codex-rs/exec-server/src/server/file_system_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,11 @@ impl FileSystemHandler {
) -> Result<FsReadFileResponse, JSONRPCErrorError> {
let bytes = self
.file_system
.read_file(&params.path, params.sandbox.as_ref())
.read_file_with_limit(
&params.path,
params.sandbox.as_ref(),
params.max_bytes.unwrap_or(512 * 1024 * 1024),
)
.await
.map_err(map_fs_error)?;
Ok(FsReadFileResponse {
Expand Down Expand Up @@ -322,6 +326,7 @@ mod tests {
.read_file(FsReadFileParams {
path,
sandbox: Some(sandbox_context(sandbox_policy)),
max_bytes: None,
})
.await
.expect("read file");
Expand Down
95 changes: 95 additions & 0 deletions codex-rs/exec-server/tests/bounded_read.rs
Original file line number Diff line number Diff line change
@@ -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<FileSystemSandboxContext> {
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,
),
))
}
1 change: 1 addition & 0 deletions codex-rs/exec-server/tests/relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
Loading