mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Run exec-server fs operations through sandbox helper (#17294)
## Summary - run exec-server filesystem RPCs requiring sandboxing through a `codex-fs` arg0 helper over stdin/stdout - keep direct local filesystem execution for `DangerFullAccess` and external sandbox policies - remove the standalone exec-server binary path in favor of top-level arg0 dispatch/runtime paths - add sandbox escape regression coverage for local and remote filesystem paths ## Validation - `just fmt` - `git diff --check` - remote devbox: `cd codex-rs && bazel test --bes_backend= --bes_results_url= //codex-rs/exec-server:all` (6/6 passed) --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
7c1e41c8b6
commit
d626dc3895
@@ -6,9 +6,11 @@ use codex_app_server_protocol::JSONRPCErrorError;
|
||||
|
||||
use crate::CopyOptions;
|
||||
use crate::CreateDirectoryOptions;
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::ExecutorFileSystem;
|
||||
use crate::RemoveOptions;
|
||||
use crate::local_file_system::LocalFileSystem;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
@@ -28,19 +30,25 @@ use crate::rpc::internal_error;
|
||||
use crate::rpc::invalid_request;
|
||||
use crate::rpc::not_found;
|
||||
|
||||
#[derive(Clone, Default)]
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct FileSystemHandler {
|
||||
file_system: LocalFileSystem,
|
||||
}
|
||||
|
||||
impl FileSystemHandler {
|
||||
pub(crate) fn new(runtime_paths: ExecServerRuntimePaths) -> Self {
|
||||
Self {
|
||||
file_system: LocalFileSystem::with_runtime_paths(runtime_paths),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn read_file(
|
||||
&self,
|
||||
params: FsReadFileParams,
|
||||
) -> Result<FsReadFileResponse, JSONRPCErrorError> {
|
||||
let bytes = self
|
||||
.file_system
|
||||
.read_file_with_sandbox_policy(¶ms.path, params.sandbox_policy.as_ref())
|
||||
.read_file(¶ms.path, params.sandbox.as_ref())
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsReadFileResponse {
|
||||
@@ -54,11 +62,11 @@ impl FileSystemHandler {
|
||||
) -> Result<FsWriteFileResponse, JSONRPCErrorError> {
|
||||
let bytes = STANDARD.decode(params.data_base64).map_err(|err| {
|
||||
invalid_request(format!(
|
||||
"fs/writeFile requires valid base64 dataBase64: {err}"
|
||||
"{FS_WRITE_FILE_METHOD} requires valid base64 dataBase64: {err}"
|
||||
))
|
||||
})?;
|
||||
self.file_system
|
||||
.write_file_with_sandbox_policy(¶ms.path, bytes, params.sandbox_policy.as_ref())
|
||||
.write_file(¶ms.path, bytes, params.sandbox.as_ref())
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsWriteFileResponse {})
|
||||
@@ -68,13 +76,12 @@ impl FileSystemHandler {
|
||||
&self,
|
||||
params: FsCreateDirectoryParams,
|
||||
) -> Result<FsCreateDirectoryResponse, JSONRPCErrorError> {
|
||||
let recursive = params.recursive.unwrap_or(true);
|
||||
self.file_system
|
||||
.create_directory_with_sandbox_policy(
|
||||
.create_directory(
|
||||
¶ms.path,
|
||||
CreateDirectoryOptions {
|
||||
recursive: params.recursive.unwrap_or(true),
|
||||
},
|
||||
params.sandbox_policy.as_ref(),
|
||||
CreateDirectoryOptions { recursive },
|
||||
params.sandbox.as_ref(),
|
||||
)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
@@ -87,7 +94,7 @@ impl FileSystemHandler {
|
||||
) -> Result<FsGetMetadataResponse, JSONRPCErrorError> {
|
||||
let metadata = self
|
||||
.file_system
|
||||
.get_metadata_with_sandbox_policy(¶ms.path, params.sandbox_policy.as_ref())
|
||||
.get_metadata(¶ms.path, params.sandbox.as_ref())
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsGetMetadataResponse {
|
||||
@@ -104,33 +111,30 @@ impl FileSystemHandler {
|
||||
) -> Result<FsReadDirectoryResponse, JSONRPCErrorError> {
|
||||
let entries = self
|
||||
.file_system
|
||||
.read_directory_with_sandbox_policy(¶ms.path, params.sandbox_policy.as_ref())
|
||||
.read_directory(¶ms.path, params.sandbox.as_ref())
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsReadDirectoryResponse {
|
||||
entries: entries
|
||||
.into_iter()
|
||||
.map(|entry| FsReadDirectoryEntry {
|
||||
file_name: entry.file_name,
|
||||
is_directory: entry.is_directory,
|
||||
is_file: entry.is_file,
|
||||
})
|
||||
.collect(),
|
||||
})
|
||||
.map_err(map_fs_error)?
|
||||
.into_iter()
|
||||
.map(|entry| FsReadDirectoryEntry {
|
||||
file_name: entry.file_name,
|
||||
is_directory: entry.is_directory,
|
||||
is_file: entry.is_file,
|
||||
})
|
||||
.collect();
|
||||
Ok(FsReadDirectoryResponse { entries })
|
||||
}
|
||||
|
||||
pub(crate) async fn remove(
|
||||
&self,
|
||||
params: FsRemoveParams,
|
||||
) -> Result<FsRemoveResponse, JSONRPCErrorError> {
|
||||
let recursive = params.recursive.unwrap_or(true);
|
||||
let force = params.force.unwrap_or(true);
|
||||
self.file_system
|
||||
.remove_with_sandbox_policy(
|
||||
.remove(
|
||||
¶ms.path,
|
||||
RemoveOptions {
|
||||
recursive: params.recursive.unwrap_or(true),
|
||||
force: params.force.unwrap_or(true),
|
||||
},
|
||||
params.sandbox_policy.as_ref(),
|
||||
RemoveOptions { recursive, force },
|
||||
params.sandbox.as_ref(),
|
||||
)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
@@ -142,13 +146,13 @@ impl FileSystemHandler {
|
||||
params: FsCopyParams,
|
||||
) -> Result<FsCopyResponse, JSONRPCErrorError> {
|
||||
self.file_system
|
||||
.copy_with_sandbox_policy(
|
||||
.copy(
|
||||
¶ms.source_path,
|
||||
¶ms.destination_path,
|
||||
CopyOptions {
|
||||
recursive: params.recursive,
|
||||
},
|
||||
params.sandbox_policy.as_ref(),
|
||||
params.sandbox.as_ref(),
|
||||
)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
@@ -157,11 +161,68 @@ impl FileSystemHandler {
|
||||
}
|
||||
|
||||
fn map_fs_error(err: io::Error) -> JSONRPCErrorError {
|
||||
if err.kind() == io::ErrorKind::NotFound {
|
||||
not_found(err.to_string())
|
||||
} else if err.kind() == io::ErrorKind::InvalidInput {
|
||||
invalid_request(err.to_string())
|
||||
} else {
|
||||
internal_error(err.to_string())
|
||||
match err.kind() {
|
||||
io::ErrorKind::NotFound => not_found(err.to_string()),
|
||||
io::ErrorKind::InvalidInput | io::ErrorKind::PermissionDenied => {
|
||||
invalid_request(err.to_string())
|
||||
}
|
||||
_ => internal_error(err.to_string()),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use codex_protocol::protocol::NetworkAccess;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::*;
|
||||
use crate::FileSystemSandboxContext;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
use crate::protocol::FsWriteFileParams;
|
||||
|
||||
#[tokio::test]
|
||||
async fn no_platform_sandbox_policies_do_not_require_configured_sandbox_helper() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
let runtime_paths = ExecServerRuntimePaths::new(
|
||||
std::env::current_exe().expect("current exe"),
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)
|
||||
.expect("runtime paths");
|
||||
let handler = FileSystemHandler::new(runtime_paths);
|
||||
|
||||
for (file_name, sandbox_policy) in [
|
||||
("danger.txt", SandboxPolicy::DangerFullAccess),
|
||||
(
|
||||
"external.txt",
|
||||
SandboxPolicy::ExternalSandbox {
|
||||
network_access: NetworkAccess::Restricted,
|
||||
},
|
||||
),
|
||||
] {
|
||||
let path =
|
||||
AbsolutePathBuf::from_absolute_path(temp_dir.path().join(file_name).as_path())
|
||||
.expect("absolute path");
|
||||
|
||||
handler
|
||||
.write_file(FsWriteFileParams {
|
||||
path: path.clone(),
|
||||
data_base64: STANDARD.encode("ok"),
|
||||
sandbox: Some(FileSystemSandboxContext::new(sandbox_policy.clone())),
|
||||
})
|
||||
.await
|
||||
.expect("write file");
|
||||
|
||||
let response = handler
|
||||
.read_file(FsReadFileParams {
|
||||
path,
|
||||
sandbox: Some(FileSystemSandboxContext::new(sandbox_policy)),
|
||||
})
|
||||
.await
|
||||
.expect("read file");
|
||||
|
||||
assert_eq!(response.data_base64, STANDARD.encode("ok"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::sync::atomic::Ordering;
|
||||
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::ExecResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
@@ -48,12 +49,13 @@ impl ExecServerHandler {
|
||||
pub(crate) fn new(
|
||||
session_registry: Arc<SessionRegistry>,
|
||||
notifications: RpcNotificationSender,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
) -> Self {
|
||||
Self {
|
||||
session_registry,
|
||||
notifications,
|
||||
session: StdMutex::new(None),
|
||||
file_system: FileSystemHandler::default(),
|
||||
file_system: FileSystemHandler::new(runtime_paths),
|
||||
initialize_requested: AtomicBool::new(false),
|
||||
initialized: AtomicBool::new(false),
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ use tokio::sync::mpsc;
|
||||
use uuid::Uuid;
|
||||
|
||||
use super::ExecServerHandler;
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::ProcessId;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::InitializeParams;
|
||||
@@ -64,12 +65,21 @@ fn windows_command_processor() -> String {
|
||||
std::env::var("COMSPEC").unwrap_or_else(|_| "cmd.exe".to_string())
|
||||
}
|
||||
|
||||
fn test_runtime_paths() -> ExecServerRuntimePaths {
|
||||
ExecServerRuntimePaths::new(
|
||||
std::env::current_exe().expect("current exe"),
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)
|
||||
.expect("runtime paths")
|
||||
}
|
||||
|
||||
async fn initialized_handler() -> Arc<ExecServerHandler> {
|
||||
let (outgoing_tx, _outgoing_rx) = mpsc::channel(16);
|
||||
let registry = SessionRegistry::new();
|
||||
let handler = Arc::new(ExecServerHandler::new(
|
||||
registry,
|
||||
RpcNotificationSender::new(outgoing_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
let initialize_response = handler
|
||||
.initialize(InitializeParams {
|
||||
@@ -147,6 +157,7 @@ async fn long_poll_read_fails_after_session_resume() {
|
||||
let first_handler = Arc::new(ExecServerHandler::new(
|
||||
Arc::clone(®istry),
|
||||
RpcNotificationSender::new(first_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
let initialize_response = first_handler
|
||||
.initialize(InitializeParams {
|
||||
@@ -187,6 +198,7 @@ async fn long_poll_read_fails_after_session_resume() {
|
||||
let second_handler = Arc::new(ExecServerHandler::new(
|
||||
registry,
|
||||
RpcNotificationSender::new(second_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
second_handler
|
||||
.initialize(InitializeParams {
|
||||
@@ -219,6 +231,7 @@ async fn active_session_resume_is_rejected() {
|
||||
let first_handler = Arc::new(ExecServerHandler::new(
|
||||
Arc::clone(®istry),
|
||||
RpcNotificationSender::new(first_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
let initialize_response = first_handler
|
||||
.initialize(InitializeParams {
|
||||
@@ -232,6 +245,7 @@ async fn active_session_resume_is_rejected() {
|
||||
let second_handler = Arc::new(ExecServerHandler::new(
|
||||
registry,
|
||||
RpcNotificationSender::new(second_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
let err = second_handler
|
||||
.initialize(InitializeParams {
|
||||
@@ -259,6 +273,7 @@ async fn output_and_exit_are_retained_after_notification_receiver_closes() {
|
||||
let handler = Arc::new(ExecServerHandler::new(
|
||||
SessionRegistry::new(),
|
||||
RpcNotificationSender::new(outgoing_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
handler
|
||||
.initialize(InitializeParams {
|
||||
|
||||
@@ -4,6 +4,7 @@ use tokio::sync::mpsc;
|
||||
use tracing::debug;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::connection::CHANNEL_CAPACITY;
|
||||
use crate::connection::JsonRpcConnection;
|
||||
use crate::connection::JsonRpcConnectionEvent;
|
||||
@@ -19,28 +20,43 @@ use crate::server::session_registry::SessionRegistry;
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct ConnectionProcessor {
|
||||
session_registry: Arc<SessionRegistry>,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
}
|
||||
|
||||
impl ConnectionProcessor {
|
||||
pub(crate) fn new() -> Self {
|
||||
pub(crate) fn new(runtime_paths: ExecServerRuntimePaths) -> Self {
|
||||
Self {
|
||||
session_registry: SessionRegistry::new(),
|
||||
runtime_paths,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn run_connection(&self, connection: JsonRpcConnection) {
|
||||
run_connection(connection, Arc::clone(&self.session_registry)).await;
|
||||
run_connection(
|
||||
connection,
|
||||
Arc::clone(&self.session_registry),
|
||||
self.runtime_paths.clone(),
|
||||
)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
async fn run_connection(connection: JsonRpcConnection, session_registry: Arc<SessionRegistry>) {
|
||||
async fn run_connection(
|
||||
connection: JsonRpcConnection,
|
||||
session_registry: Arc<SessionRegistry>,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
) {
|
||||
let router = Arc::new(build_router());
|
||||
let (json_outgoing_tx, mut incoming_rx, mut disconnected_rx, connection_tasks) =
|
||||
connection.into_parts();
|
||||
let (outgoing_tx, mut outgoing_rx) =
|
||||
mpsc::channel::<RpcServerOutboundMessage>(CHANNEL_CAPACITY);
|
||||
let notifications = RpcNotificationSender::new(outgoing_tx.clone());
|
||||
let handler = Arc::new(ExecServerHandler::new(session_registry, notifications));
|
||||
let handler = Arc::new(ExecServerHandler::new(
|
||||
session_registry,
|
||||
notifications,
|
||||
runtime_paths,
|
||||
));
|
||||
|
||||
let outbound_task = tokio::spawn(async move {
|
||||
while let Some(message) = outgoing_rx.recv().await {
|
||||
@@ -184,6 +200,7 @@ mod tests {
|
||||
use tokio::time::timeout;
|
||||
|
||||
use super::run_connection;
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::ProcessId;
|
||||
use crate::connection::JsonRpcConnection;
|
||||
use crate::protocol::EXEC_METHOD;
|
||||
@@ -298,10 +315,18 @@ mod tests {
|
||||
let (server_writer, client_reader) = duplex(1 << 20);
|
||||
let connection =
|
||||
JsonRpcConnection::from_stdio(server_reader, server_writer, label.to_string());
|
||||
let task = tokio::spawn(run_connection(connection, registry));
|
||||
let task = tokio::spawn(run_connection(connection, registry, test_runtime_paths()));
|
||||
(client_writer, BufReader::new(client_reader).lines(), task)
|
||||
}
|
||||
|
||||
fn test_runtime_paths() -> ExecServerRuntimePaths {
|
||||
ExecServerRuntimePaths::new(
|
||||
std::env::current_exe().expect("current exe"),
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)
|
||||
.expect("runtime paths")
|
||||
}
|
||||
|
||||
async fn send_request<P: Serialize>(
|
||||
writer: &mut DuplexStream,
|
||||
id: i64,
|
||||
|
||||
@@ -1,9 +1,10 @@
|
||||
use std::io::Write as _;
|
||||
use std::net::SocketAddr;
|
||||
|
||||
use tokio::net::TcpListener;
|
||||
use tokio_tungstenite::accept_async;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::connection::JsonRpcConnection;
|
||||
use crate::server::processor::ConnectionProcessor;
|
||||
|
||||
@@ -48,19 +49,22 @@ pub(crate) fn parse_listen_url(
|
||||
|
||||
pub(crate) async fn run_transport(
|
||||
listen_url: &str,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
|
||||
let bind_address = parse_listen_url(listen_url)?;
|
||||
run_websocket_listener(bind_address).await
|
||||
run_websocket_listener(bind_address, runtime_paths).await
|
||||
}
|
||||
|
||||
async fn run_websocket_listener(
|
||||
bind_address: SocketAddr,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
|
||||
let listener = TcpListener::bind(bind_address).await?;
|
||||
let local_addr = listener.local_addr()?;
|
||||
let processor = ConnectionProcessor::new();
|
||||
let processor = ConnectionProcessor::new(runtime_paths);
|
||||
tracing::info!("codex-exec-server listening on ws://{local_addr}");
|
||||
println!("ws://{local_addr}");
|
||||
std::io::stdout().flush()?;
|
||||
|
||||
loop {
|
||||
let (stream, peer_addr) = listener.accept().await?;
|
||||
|
||||
Reference in New Issue
Block a user