mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
exec-server: add bounded file reads
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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<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 {
|
||||
@@ -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(
|
||||
@@ -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(
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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::*;
|
||||
|
||||
@@ -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)]
|
||||
@@ -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);
|
||||
|
||||
@@ -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)?;
|
||||
|
||||
@@ -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!(
|
||||
|
||||
@@ -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)?;
|
||||
@@ -86,6 +96,7 @@ impl SandboxedFileSystem {
|
||||
FsHelperRequest::ReadFile(FsReadFileParams {
|
||||
path: path.clone(),
|
||||
sandbox: None,
|
||||
max_bytes: Some(max_bytes),
|
||||
}),
|
||||
)
|
||||
.await?
|
||||
|
||||
@@ -108,7 +108,11 @@ impl FileSystemHandler {
|
||||
) -> Result<FsReadFileResponse, JSONRPCErrorError> {
|
||||
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");
|
||||
|
||||
@@ -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,
|
||||
),
|
||||
))
|
||||
}
|
||||
@@ -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!(
|
||||
|
||||
Reference in New Issue
Block a user