mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Enable writable file streaming in exec-server (#50177)
## What changed Advertise `fileWriteStreaming` and support `fs/open` with `mode: "replace"` to create or truncate a file for writing. Implement `fs/writeBlock` with explicit offsets and nonempty chunks of up to 1 MiB, rejecting ranges beyond the signed 64-bit file offset limit. Apply write sandbox permissions when opening writable handles, including when full-disk reads are allowed. Keep `read` as the default open mode. ## Testing Add integration coverage for positional writes, chunk and offset bounds, replacement truncation, read/write handle permissions, and sandbox write restrictions. Verify that duplicate handle IDs and exhausted handle capacity reject replacement opens without creating or truncating files. GitOrigin-RevId: 696ee6437dd5f5b69d5a7667dddea0ff44472094
This commit is contained in:
committed by
copyberry
parent
7135b303d9
commit
c39bfa4c8f
@@ -434,8 +434,8 @@ for invalid or unavailable paths. Native absolute path strings are rejected;
|
||||
callers must convert them to `file:` URIs before sending requests:
|
||||
|
||||
- `fs/readFile`
|
||||
- `fs/open`, `fs/readBlock`, and `fs/close` (internal transport for
|
||||
`ExecutorFileSystem::read_file_stream`)
|
||||
- `fs/open`, `fs/readBlock`, `fs/writeBlock`, and `fs/close` (file handles;
|
||||
reads also back `ExecutorFileSystem::read_file_stream`)
|
||||
- `fs/writeFile`
|
||||
- `fs/createDirectory`
|
||||
- `fs/getMetadata`
|
||||
@@ -444,7 +444,11 @@ callers must convert them to `file:` URIs before sending requests:
|
||||
- `fs/remove`
|
||||
- `fs/copy`
|
||||
|
||||
Each filesystem request accepts an optional `sandbox` object. When `sandbox`
|
||||
`fs/open.mode` is `read` (default) or `replace` (create or truncate for writing).
|
||||
Writable handles require `fileWriteStreaming`; `fs/writeBlock` writes a nonempty
|
||||
base64 `chunk` of up to 1 MiB at an explicit `offset`.
|
||||
|
||||
Path-based filesystem requests accept an optional `sandbox` object. When `sandbox`
|
||||
contains a `ReadOnly` or `WorkspaceWrite` policy, the operation runs in a
|
||||
hidden helper process launched from the top-level `codex` executable and
|
||||
prepared through the shared sandbox transform path. Helper requests and
|
||||
|
||||
@@ -7,6 +7,7 @@ use std::collections::BTreeMap;
|
||||
use std::io;
|
||||
|
||||
use crate::LocalFileSystem;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use codex_config::loader::system_config_toml_file;
|
||||
use codex_file_system::ExecutorFileSystem;
|
||||
use codex_file_system::WalkEntryKind;
|
||||
@@ -182,7 +183,7 @@ async fn discover_cached_plugins(
|
||||
|
||||
async fn read_remote_plugin_id(path: &std::path::Path) -> io::Result<Option<String>> {
|
||||
// Nonblocking open and handle validation reject FIFOs and other non-regular files.
|
||||
let file = match crate::regular_file::open(path).await {
|
||||
let file = match crate::regular_file::open(path, FsOpenMode::Read).await {
|
||||
Ok(file) => file,
|
||||
Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None),
|
||||
Err(error) => return Err(error),
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! Connection-scoped file handles with bounded positional reads.
|
||||
//! Connection-scoped file handles with bounded positional reads and writes.
|
||||
//! Semaphore permits bound open handles and in-flight opens; table locks never cross an await.
|
||||
|
||||
use std::collections::HashMap;
|
||||
@@ -11,6 +11,7 @@ use std::sync::Mutex;
|
||||
use std::sync::MutexGuard;
|
||||
|
||||
use codex_file_system::FILE_READ_CHUNK_SIZE;
|
||||
use codex_file_system::FILE_WRITE_CHUNK_SIZE;
|
||||
use tokio::sync::OwnedSemaphorePermit;
|
||||
use tokio::sync::Semaphore;
|
||||
|
||||
@@ -46,13 +47,13 @@ impl FileHandleManager {
|
||||
if self.lock_handles().contains_key(&handle_id) {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
format!("file read handle `{handle_id}` already exists"),
|
||||
format!("file handle `{handle_id}` already exists"),
|
||||
));
|
||||
}
|
||||
let permit = Arc::clone(&self.slots).try_acquire_owned().map_err(|_| {
|
||||
io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
format!("at most {MAX_OPEN_FILES} file reads may be open per connection"),
|
||||
format!("at most {MAX_OPEN_FILES} file handles may be open per connection"),
|
||||
)
|
||||
})?;
|
||||
let file = Arc::new(open_file.await?.into_std().await);
|
||||
@@ -60,7 +61,7 @@ impl FileHandleManager {
|
||||
let Entry::Vacant(entry) = handles.entry(handle_id.clone()) else {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
format!("file read handle `{handle_id}` already exists"),
|
||||
format!("file handle `{handle_id}` already exists"),
|
||||
));
|
||||
};
|
||||
entry.insert(FileHandleEntry {
|
||||
@@ -91,6 +92,65 @@ impl FileHandleManager {
|
||||
result
|
||||
}
|
||||
|
||||
pub(crate) async fn write_block(
|
||||
&self,
|
||||
handle_id: &str,
|
||||
offset: u64,
|
||||
bytes: Vec<u8>,
|
||||
) -> io::Result<()> {
|
||||
if !(1..=FILE_WRITE_CHUNK_SIZE).contains(&bytes.len()) {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
format!("file write block length must be between 1 and {FILE_WRITE_CHUNK_SIZE}"),
|
||||
));
|
||||
}
|
||||
// Native file offsets are signed; Windows interprets negative offsets as sentinels.
|
||||
let end = offset
|
||||
.checked_add(bytes.len() as u64)
|
||||
.and_then(|end| i64::try_from(end).ok())
|
||||
.ok_or_else(|| {
|
||||
io::Error::new(
|
||||
io::ErrorKind::InvalidInput,
|
||||
"file write range exceeds the signed 64-bit file offset limit",
|
||||
)
|
||||
})? as u64;
|
||||
let file = self.get(handle_id)?;
|
||||
tokio::task::spawn_blocking(move || {
|
||||
let mut position = offset;
|
||||
while position < end {
|
||||
let written = (position - offset) as usize;
|
||||
#[cfg(unix)]
|
||||
let result = std::os::unix::fs::FileExt::write_at(
|
||||
file.as_ref(),
|
||||
&bytes[written..],
|
||||
position,
|
||||
);
|
||||
#[cfg(windows)]
|
||||
let result = std::os::windows::fs::FileExt::seek_write(
|
||||
file.as_ref(),
|
||||
&bytes[written..],
|
||||
position,
|
||||
);
|
||||
match result {
|
||||
Ok(0) => {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::WriteZero,
|
||||
"failed to write file block",
|
||||
));
|
||||
}
|
||||
Ok(count) => position += count as u64,
|
||||
Err(error) if error.kind() == io::ErrorKind::Interrupted => {}
|
||||
Err(error) => return Err(error),
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
})
|
||||
.await
|
||||
.map_err(|error| {
|
||||
io::Error::other(format!("file write task stopped unexpectedly: {error}"))
|
||||
})?
|
||||
}
|
||||
|
||||
fn get(&self, handle_id: &str) -> io::Result<Arc<File>> {
|
||||
self.lock_handles()
|
||||
.get(handle_id)
|
||||
@@ -115,7 +175,7 @@ impl FileHandleManager {
|
||||
|
||||
struct FileHandleEntry {
|
||||
file: Arc<File>,
|
||||
// Closing an entry releases capacity even if a read still holds the file.
|
||||
// Closing an entry releases capacity even if a read or write still holds the file.
|
||||
_permit: OwnedSemaphorePermit,
|
||||
}
|
||||
|
||||
@@ -163,7 +223,7 @@ fn validate_read_block_len(len: usize) -> io::Result<()> {
|
||||
fn unknown_handle_error(handle_id: &str) -> io::Error {
|
||||
io::Error::new(
|
||||
io::ErrorKind::NotFound,
|
||||
format!("unknown file read handle `{handle_id}`"),
|
||||
format!("unknown file handle `{handle_id}`"),
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -39,6 +39,7 @@ use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use crate::protocol::FsReadDirectoryEntry;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadDirectoryResponse;
|
||||
@@ -92,6 +93,8 @@ pub(crate) enum FsHelperRequest {
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub(crate) struct FsHelperOpenParams {
|
||||
pub(crate) path: PathUri,
|
||||
#[serde(default)]
|
||||
pub(crate) mode: FsOpenMode,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
|
||||
@@ -43,7 +43,7 @@ async fn run_main() -> Result<(), Box<dyn Error + Send + Sync>> {
|
||||
FsHelperRequest::Open(params) => {
|
||||
let result: io::Result<_> = async {
|
||||
let path = params.path.to_abs_path()?;
|
||||
let file = regular_file::open(path.as_path()).await?;
|
||||
let file = regular_file::open(path.as_path(), params.mode).await?;
|
||||
// Unix can hand the opened fd directly to the parent.
|
||||
#[cfg(unix)]
|
||||
crate::sandboxed_file_open::transfer_file(&file)?;
|
||||
|
||||
@@ -208,6 +208,7 @@ $handle = $file.SafeFileHandle.DangerousGetHandle().ToInt64()
|
||||
crate::sandboxed_file_open::open(
|
||||
command,
|
||||
PathUri::from_host_native_path(&path).expect("image path URI"),
|
||||
crate::protocol::FsOpenMode::Read,
|
||||
),
|
||||
)
|
||||
.await
|
||||
|
||||
@@ -38,6 +38,7 @@ use crate::WalkOptions;
|
||||
use crate::WalkOutcome;
|
||||
use crate::WriteFileOptions;
|
||||
use crate::no_follow;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use crate::regular_file;
|
||||
use crate::sandboxed_file_system::SandboxedFileSystem;
|
||||
|
||||
@@ -121,15 +122,20 @@ impl LocalFileSystem {
|
||||
pub(crate) async fn open_file(
|
||||
&self,
|
||||
path: &PathUri,
|
||||
mode: FsOpenMode,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<tokio::fs::File> {
|
||||
if let Some(sandbox) = sandbox {
|
||||
sandbox.validate_file_system_paths_for_current_host()?;
|
||||
let needs_sandbox = match mode {
|
||||
FsOpenMode::Read => sandbox.should_read_from_sandbox(),
|
||||
FsOpenMode::Replace => sandbox.should_write_into_sandbox(),
|
||||
};
|
||||
if needs_sandbox {
|
||||
return self.sandboxed()?.open_file(path, mode, Some(sandbox)).await;
|
||||
}
|
||||
}
|
||||
if sandbox.is_some_and(FileSystemSandboxContext::should_read_from_sandbox) {
|
||||
return self.sandboxed()?.open_file(path, sandbox).await;
|
||||
}
|
||||
self.unsandboxed.open_file(path, /*sandbox*/ None).await
|
||||
regular_file::open(path.to_abs_path()?.as_path(), mode).await
|
||||
}
|
||||
|
||||
async fn canonicalize(
|
||||
@@ -348,15 +354,6 @@ impl ExecutorFileSystem for LocalFileSystem {
|
||||
}
|
||||
|
||||
impl UnsandboxedFileSystem {
|
||||
async fn open_file(
|
||||
&self,
|
||||
path: &PathUri,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<tokio::fs::File> {
|
||||
reject_platform_sandbox_context(sandbox)?;
|
||||
self.file_system.open_file(path, /*sandbox*/ None).await
|
||||
}
|
||||
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &PathUri,
|
||||
@@ -584,7 +581,7 @@ impl DirectFileSystem {
|
||||
) -> FileSystemResult<tokio::fs::File> {
|
||||
reject_sandbox_context(sandbox)?;
|
||||
let path = path.to_abs_path()?;
|
||||
regular_file::open(path.as_path()).await
|
||||
regular_file::open(path.as_path(), FsOpenMode::Read).await
|
||||
}
|
||||
|
||||
async fn canonicalize(
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
use crate::FILE_READ_CHUNK_SIZE;
|
||||
use crate::ReadFileOptions;
|
||||
use crate::no_follow;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use crate::regular_file;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::io;
|
||||
@@ -22,7 +23,7 @@ pub(super) async fn read_file(
|
||||
return Err(io::Error::other("file read cancelled"));
|
||||
}
|
||||
let file = if options.follow_symlinks {
|
||||
regular_file::open_sync(path.as_path())?
|
||||
regular_file::open_sync(path.as_path(), FsOpenMode::Read)?
|
||||
} else {
|
||||
no_follow::open_file(path.as_path())?
|
||||
};
|
||||
|
||||
@@ -6,17 +6,26 @@ use std::os::windows::fs::OpenOptionsExt;
|
||||
use std::path::Path;
|
||||
use tokio::io::AsyncReadExt;
|
||||
|
||||
pub(crate) async fn open(path: &Path) -> io::Result<tokio::fs::File> {
|
||||
use crate::protocol::FsOpenMode;
|
||||
|
||||
pub(crate) async fn open(path: &Path, mode: FsOpenMode) -> io::Result<tokio::fs::File> {
|
||||
let path = path.to_path_buf();
|
||||
tokio::task::spawn_blocking(move || open_sync(&path).map(tokio::fs::File::from_std))
|
||||
tokio::task::spawn_blocking(move || open_sync(&path, mode).map(tokio::fs::File::from_std))
|
||||
.await
|
||||
.map_err(|error| io::Error::other(format!("filesystem task failed: {error}")))?
|
||||
}
|
||||
|
||||
/// Opens a regular file without blocking on Unix FIFOs or impersonating Windows pipe servers.
|
||||
pub(crate) fn open_sync(path: &Path) -> io::Result<std::fs::File> {
|
||||
pub(crate) fn open_sync(path: &Path, mode: FsOpenMode) -> io::Result<std::fs::File> {
|
||||
let mut options = std::fs::OpenOptions::new();
|
||||
options.read(true);
|
||||
match mode {
|
||||
FsOpenMode::Read => {
|
||||
options.read(true);
|
||||
}
|
||||
FsOpenMode::Replace => {
|
||||
options.write(true).create(true).truncate(true);
|
||||
}
|
||||
}
|
||||
configure_open(&mut options);
|
||||
|
||||
let file = options.open(path)?;
|
||||
|
||||
@@ -18,14 +18,16 @@ use crate::fs_sandbox::reap_helper_after_response;
|
||||
use crate::fs_sandbox::spawn_command;
|
||||
#[cfg(unix)]
|
||||
use crate::fs_sandbox::wait_for_helper_output;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use crate::rpc::internal_error;
|
||||
use crate::rpc::invalid_request;
|
||||
|
||||
pub(crate) async fn open(
|
||||
command: SandboxExecRequest,
|
||||
path: PathUri,
|
||||
mode: FsOpenMode,
|
||||
) -> Result<tokio::fs::File, JSONRPCErrorError> {
|
||||
let request = serde_json::to_vec(&FsHelperRequest::Open(FsHelperOpenParams { path }))
|
||||
let request = serde_json::to_vec(&FsHelperRequest::Open(FsHelperOpenParams { path, mode }))
|
||||
.map_err(|error| internal_error(format!("invalid fs sandbox helper request: {error}")))?;
|
||||
open_platform(command, request).await
|
||||
}
|
||||
|
||||
@@ -33,6 +33,7 @@ use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
use crate::protocol::FsRemoveParams;
|
||||
@@ -83,6 +84,7 @@ impl SandboxedFileSystem {
|
||||
pub(crate) async fn open_file(
|
||||
&self,
|
||||
path: &PathUri,
|
||||
mode: FsOpenMode,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<tokio::fs::File> {
|
||||
let sandbox = require_platform_sandbox(sandbox)?;
|
||||
@@ -91,7 +93,7 @@ impl SandboxedFileSystem {
|
||||
.sandbox_runner
|
||||
.sandbox_command(sandbox)
|
||||
.map_err(map_sandbox_error)?;
|
||||
crate::sandboxed_file_open::open(command, path.clone())
|
||||
crate::sandboxed_file_open::open(command, path.clone(), mode)
|
||||
.await
|
||||
.map_err(map_sandbox_error)
|
||||
}
|
||||
@@ -367,7 +369,7 @@ impl ExecutorFileSystem for SandboxedFileSystem {
|
||||
sandbox: Option<&'a FileSystemSandboxContext>,
|
||||
) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> {
|
||||
Box::pin(async move {
|
||||
let file = self.open_file(path, sandbox).await?;
|
||||
let file = self.open_file(path, FsOpenMode::Read, sandbox).await?;
|
||||
Ok(FileSystemReadStream::new(ReaderStream::with_capacity(
|
||||
file,
|
||||
FILE_READ_CHUNK_SIZE,
|
||||
|
||||
@@ -28,7 +28,6 @@ use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use crate::protocol::FsOpenParams;
|
||||
use crate::protocol::FsOpenResponse;
|
||||
use crate::protocol::FsReadBlockParams;
|
||||
@@ -125,18 +124,12 @@ impl FileSystemHandler {
|
||||
params: FsOpenParams,
|
||||
) -> Result<FsOpenResponse, JSONRPCErrorError> {
|
||||
validate_file_handle_id(¶ms.handle_id)?;
|
||||
// TODO(anp): Enable replacement opens when writable file streams are implemented.
|
||||
if params.mode == FsOpenMode::Replace {
|
||||
return Err(invalid_request(
|
||||
"exec-server does not support writable file streams".to_string(),
|
||||
));
|
||||
}
|
||||
let handle_id = self
|
||||
.file_handles
|
||||
.open(
|
||||
params.handle_id,
|
||||
self.file_system
|
||||
.open_file(¶ms.path, params.sandbox.as_ref()),
|
||||
.open_file(¶ms.path, params.mode, params.sandbox.as_ref()),
|
||||
)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
@@ -164,10 +157,11 @@ impl FileSystemHandler {
|
||||
params: FsWriteBlockParams,
|
||||
) -> Result<FsWriteBlockResponse, JSONRPCErrorError> {
|
||||
validate_file_handle_id(¶ms.handle_id)?;
|
||||
// TODO(anp): Implement positional writes before advertising writable file streams.
|
||||
Err(invalid_request(
|
||||
"exec-server does not support writable file streams".to_string(),
|
||||
))
|
||||
self.file_handles
|
||||
.write_block(¶ms.handle_id, params.offset, params.chunk.into_inner())
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsWriteBlockResponse {})
|
||||
}
|
||||
|
||||
pub(crate) async fn close(
|
||||
@@ -357,7 +351,7 @@ impl FileSystemHandler {
|
||||
fn validate_file_handle_id(handle_id: &str) -> Result<(), JSONRPCErrorError> {
|
||||
if handle_id.len() > MAX_FILE_HANDLE_ID_BYTES {
|
||||
return Err(invalid_request(format!(
|
||||
"file read handle ID must not exceed {MAX_FILE_HANDLE_ID_BYTES} bytes"
|
||||
"file handle ID must not exceed {MAX_FILE_HANDLE_ID_BYTES} bytes"
|
||||
)));
|
||||
}
|
||||
Ok(())
|
||||
@@ -383,7 +377,6 @@ mod tests {
|
||||
use super::*;
|
||||
use crate::FileSystemSandboxContext;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
|
||||
use crate::protocol::FsWriteFileParams;
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -12,6 +12,7 @@ use codex_exec_server::FsOpenMode;
|
||||
use codex_exec_server::FsOpenParams;
|
||||
use codex_exec_server::FsReadBlockParams;
|
||||
use codex_exec_server::FsReadBlockResponse;
|
||||
use codex_exec_server::FsWriteBlockParams;
|
||||
use codex_exec_server::ReadFileOptions;
|
||||
use codex_exec_server::RemoteExecServerConnectArgs;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
@@ -233,6 +234,15 @@ async fn read_block_supports_non_sequential_offsets_and_lengths() -> Result<()>
|
||||
})
|
||||
.await?;
|
||||
|
||||
client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset: 0,
|
||||
chunk: b"x".to_vec().into(),
|
||||
})
|
||||
.await
|
||||
.expect_err("default read-only handles must reject writes");
|
||||
|
||||
let mut blocks = Vec::new();
|
||||
for (offset, len) in [(6, 3), (1, 2), (8, 4), (0, 2)] {
|
||||
blocks.push(
|
||||
@@ -318,7 +328,7 @@ async fn open_enforces_the_per_connection_limit_and_close_releases_capacity() ->
|
||||
(code, message),
|
||||
(
|
||||
-32600,
|
||||
format!("at most {OPEN_FILE_LIMIT} file reads may be open per connection"),
|
||||
format!("at most {OPEN_FILE_LIMIT} file handles may be open per connection"),
|
||||
)
|
||||
);
|
||||
|
||||
@@ -340,6 +350,56 @@ async fn open_enforces_the_per_connection_limit_and_close_releases_capacity() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Rejected replacement opens must neither truncate existing files nor create missing files.
|
||||
#[test_case::test_case(1, "0", "file handle `0` already exists"; "duplicate_id")]
|
||||
#[test_case::test_case(OPEN_FILE_LIMIT, "overflow", "at most 128 file handles may be open per connection"; "capacity")]
|
||||
#[tokio::test]
|
||||
async fn replace_open_rejects_unavailable_handles_before_touching_files(
|
||||
handle_count: usize,
|
||||
handle_id: &str,
|
||||
expected_message: &str,
|
||||
) -> Result<()> {
|
||||
let server = exec_server().await?;
|
||||
let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new(
|
||||
server.websocket_url().to_string(),
|
||||
"file-admission-test".to_string(),
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
))
|
||||
.await?;
|
||||
let tmp = TempDir::new()?;
|
||||
let existing = tmp.path().join("existing.bin");
|
||||
let missing = tmp.path().join("missing.bin");
|
||||
std::fs::write(&existing, b"original")?;
|
||||
for index in 0..handle_count {
|
||||
client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: index.to_string(),
|
||||
path: PathUri::from_host_native_path(&existing)?,
|
||||
mode: FsOpenMode::Read,
|
||||
sandbox: None,
|
||||
})
|
||||
.await?;
|
||||
}
|
||||
|
||||
for path in [&existing, &missing] {
|
||||
let error = client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: handle_id.to_string(),
|
||||
path: PathUri::from_host_native_path(path)?,
|
||||
mode: FsOpenMode::Replace,
|
||||
sandbox: None,
|
||||
})
|
||||
.await;
|
||||
let Err(ExecServerError::Server { code, message }) = error else {
|
||||
anyhow::bail!("expected server error, got {error:?}");
|
||||
};
|
||||
assert_eq!((code, message), (-32600, expected_message.to_string()));
|
||||
}
|
||||
assert_eq!(std::fs::read(&existing)?, b"original");
|
||||
assert!(!missing.exists());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn open_rejects_handle_ids_longer_than_32_bytes() -> Result<()> {
|
||||
let server = exec_server().await?;
|
||||
@@ -370,12 +430,254 @@ async fn open_rejects_handle_ids_longer_than_32_bytes() -> Result<()> {
|
||||
(code, message),
|
||||
(
|
||||
-32600,
|
||||
"file read handle ID must not exceed 32 bytes".to_string(),
|
||||
"file handle ID must not exceed 32 bytes".to_string(),
|
||||
)
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Writes use explicit offsets and preserve untouched bytes across multiple blocks.
|
||||
#[tokio::test]
|
||||
async fn write_blocks_support_non_sequential_offsets_and_enforce_bounds() -> Result<()> {
|
||||
let server = exec_server().await?;
|
||||
let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new(
|
||||
server.websocket_url().to_string(),
|
||||
"file-write-test".to_string(),
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
))
|
||||
.await?;
|
||||
let tmp = TempDir::new()?;
|
||||
let path = tmp.path().join("blocks.bin");
|
||||
let open = client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: "writer".to_string(),
|
||||
path: PathUri::from_host_native_path(&path)?,
|
||||
mode: FsOpenMode::Replace,
|
||||
sandbox: None,
|
||||
})
|
||||
.await?;
|
||||
for (offset, chunk) in [
|
||||
(BLOCK_SIZE as u64, vec![b'z'; 3]),
|
||||
(0, vec![b'a'; BLOCK_SIZE]),
|
||||
(1, b"bc".to_vec()),
|
||||
] {
|
||||
client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset,
|
||||
chunk: chunk.into(),
|
||||
})
|
||||
.await?;
|
||||
}
|
||||
for (offset, chunk) in [
|
||||
(0, Vec::new()),
|
||||
(0, vec![b'x'; BLOCK_SIZE + 1]),
|
||||
(u64::MAX, vec![b'x']),
|
||||
] {
|
||||
client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset,
|
||||
chunk: chunk.into(),
|
||||
})
|
||||
.await
|
||||
.expect_err("empty, oversized, and overflowing writes must be rejected");
|
||||
}
|
||||
let read = client
|
||||
.fs_read_block(FsReadBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset: 0,
|
||||
len: 4,
|
||||
})
|
||||
.await;
|
||||
read.expect_err("write-only handles must reject reads");
|
||||
let mut expected = vec![b'a'; BLOCK_SIZE];
|
||||
expected[1..3].copy_from_slice(b"bc");
|
||||
expected.extend_from_slice(b"zzz");
|
||||
assert_eq!(std::fs::read(&path)?, expected);
|
||||
client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: "writer".to_string(),
|
||||
offset: 0,
|
||||
chunk: b"x".to_vec().into(),
|
||||
})
|
||||
.await
|
||||
.expect_err("read failures must close the handle");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Invalid signed offsets must not reach Windows' current-position sentinel or mutate the file.
|
||||
#[test_case::test_case(u64::MAX - 1, 1; "windows_current_position_sentinel")]
|
||||
#[test_case::test_case(u64::MAX, 1; "unsigned_overflow")]
|
||||
#[test_case::test_case(i64::MAX as u64, 1; "signed_end_overflow")]
|
||||
#[test_case::test_case(i64::MAX as u64 - 1, 2; "block_crosses_signed_limit")]
|
||||
#[tokio::test]
|
||||
async fn write_blocks_reject_ranges_outside_signed_file_offsets(
|
||||
offset: u64,
|
||||
len: usize,
|
||||
) -> Result<()> {
|
||||
let server = exec_server().await?;
|
||||
let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new(
|
||||
server.websocket_url().to_string(),
|
||||
"file-write-offset-test".to_string(),
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
))
|
||||
.await?;
|
||||
let tmp = TempDir::new()?;
|
||||
let path = tmp.path().join("offsets.bin");
|
||||
let open = client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: "writer".to_string(),
|
||||
path: PathUri::from_host_native_path(&path)?,
|
||||
mode: FsOpenMode::Replace,
|
||||
sandbox: None,
|
||||
})
|
||||
.await?;
|
||||
client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset: 0,
|
||||
chunk: b"original".to_vec().into(),
|
||||
})
|
||||
.await?;
|
||||
|
||||
let error = client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset,
|
||||
chunk: vec![b'x'; len].into(),
|
||||
})
|
||||
.await;
|
||||
let Err(ExecServerError::Server { code, message }) = error else {
|
||||
anyhow::bail!("expected server error, got {error:?}");
|
||||
};
|
||||
assert_eq!(
|
||||
(code, message),
|
||||
(
|
||||
-32600,
|
||||
"file write range exceeds the signed 64-bit file offset limit".to_string(),
|
||||
)
|
||||
);
|
||||
client
|
||||
.fs_close(FsCloseParams {
|
||||
handle_id: open.handle_id,
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(std::fs::read(&path)?, b"original");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Replacement discards the previous contents before any streamed blocks are written.
|
||||
#[tokio::test]
|
||||
async fn replace_open_truncates_existing_file() -> Result<()> {
|
||||
let server = exec_server().await?;
|
||||
let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new(
|
||||
server.websocket_url().to_string(),
|
||||
"file-replace-test".to_string(),
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
))
|
||||
.await?;
|
||||
let tmp = TempDir::new()?;
|
||||
let path = tmp.path().join("replace.bin");
|
||||
std::fs::write(&path, b"original")?;
|
||||
let open = client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: "replacement".to_string(),
|
||||
path: PathUri::from_host_native_path(&path)?,
|
||||
mode: FsOpenMode::Replace,
|
||||
sandbox: None,
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(std::fs::read(&path)?, b"");
|
||||
client
|
||||
.fs_close(FsCloseParams {
|
||||
handle_id: open.handle_id,
|
||||
})
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Both restricted reads and full-disk reads must retain write sandbox enforcement.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn writable_open_obeys_sandbox_write_permissions() -> Result<()> {
|
||||
use codex_exec_server::FileSystemSandboxContext;
|
||||
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::FileSystemSpecialPath;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
let server = exec_server().await?;
|
||||
let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new(
|
||||
server.websocket_url().to_string(),
|
||||
"file-write-sandbox-test".to_string(),
|
||||
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
|
||||
))
|
||||
.await?;
|
||||
let workspace = TempDir::new()?;
|
||||
let outside = TempDir::new()?;
|
||||
let allowed = workspace.path().join("allowed.bin");
|
||||
let denied = outside.path().join("denied.bin");
|
||||
std::fs::write(&denied, b"original")?;
|
||||
for full_disk_read in [false, true] {
|
||||
let mut entries = vec![FileSystemSandboxEntry::new(
|
||||
AbsolutePathBuf::from_absolute_path(workspace.path())?.into(),
|
||||
FileSystemAccessMode::Write,
|
||||
)];
|
||||
if full_disk_read {
|
||||
entries.push(FileSystemSandboxEntry::new(
|
||||
FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Root,
|
||||
},
|
||||
FileSystemAccessMode::Read,
|
||||
));
|
||||
}
|
||||
let sandbox = FileSystemSandboxContext::from_permission_profile(
|
||||
PermissionProfile::from_runtime_permissions(
|
||||
&FileSystemSandboxPolicy::restricted(entries),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
),
|
||||
PathUri::from_host_native_path(workspace.path())?,
|
||||
);
|
||||
client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: "denied".to_string(),
|
||||
path: PathUri::from_host_native_path(&denied)?,
|
||||
mode: FsOpenMode::Replace,
|
||||
sandbox: Some(sandbox.clone()),
|
||||
})
|
||||
.await
|
||||
.expect_err("writable opens must not escape the write sandbox");
|
||||
let open = client
|
||||
.fs_open(FsOpenParams {
|
||||
handle_id: "allowed".to_string(),
|
||||
path: PathUri::from_host_native_path(&allowed)?,
|
||||
mode: FsOpenMode::Replace,
|
||||
sandbox: Some(sandbox.clone()),
|
||||
})
|
||||
.await?;
|
||||
client
|
||||
.fs_write_block(FsWriteBlockParams {
|
||||
handle_id: open.handle_id.clone(),
|
||||
offset: 0,
|
||||
chunk: b"allowed".to_vec().into(),
|
||||
})
|
||||
.await?;
|
||||
client
|
||||
.fs_close(FsCloseParams {
|
||||
handle_id: open.handle_id,
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(std::fs::read(&allowed)?, b"allowed");
|
||||
assert_eq!(std::fs::read(&denied)?, b"original");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn connect_file_system(websocket_url: &str) -> Result<Arc<dyn ExecutorFileSystem>> {
|
||||
let environment = Environment::create_for_tests(Some(websocket_url.to_string()))?;
|
||||
Ok(environment.get_filesystem())
|
||||
|
||||
Reference in New Issue
Block a user