Preserve remote read compatibility with older executors (#58195)

## Why

Older executors sandbox reads whenever a policy is supplied, even when the policy allows unrestricted reads. This can interfere with loading skills across executor versions.

## What changed

- Omit sandbox contexts from remote read operations when reads are unrestricted and explicit permission paths are valid for the target path convention. Retain contexts that require executor-side path validation.
- Check remote `windows_mxc` support before sending MXC sandbox contexts for reads or capability discovery, returning an update-executor error when unsupported.

## Testing

Add coverage for permission path validation across read operations, unsupported MXC requests, and skill loading over Noise with mismatched app and executor versions, including selected skill roots with capability discovery disabled.

GitOrigin-RevId: 4a2a7ccaa2ba4daac657bd84aea603404452be45
This commit is contained in:
Dmitriy Kovalenko
2026-10-29 23:56:17 +00:00
committed by copyberry
parent 708e3e2c70
commit 98f79517ab
11 changed files with 351 additions and 55 deletions
@@ -10,6 +10,7 @@ use codex_exec_server::CODEX_EXEC_SERVER_NOISE_AUTH_TOKEN_ENV_VAR;
use codex_exec_server::CODEX_EXEC_SERVER_NOISE_ENVIRONMENT_ID_ENV_VAR;
use codex_exec_server::CODEX_EXEC_SERVER_NOISE_REGISTRY_URL_ENV_VAR;
use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR;
use codex_exec_server::REMOTE_ENVIRONMENT_ID;
use codex_exec_server_test_support::relay::TEST_TIMEOUT as VERSION_SKEW_TIMEOUT;
use codex_exec_server_test_support::relay::accept_websocket;
use codex_exec_server_test_support::relay::assert_relay_data_is_encrypted;
@@ -83,7 +84,28 @@ fn version_skew_binaries() -> Result<Option<(PathBuf, PathBuf)>> {
async fn assert_noise_version_skew(app_binary: &Path, executor_binary: &Path) -> Result<()> {
let codex_home = TempDir::new()?;
let model = mock_model(codex_home.path()).await?;
let workspace = TempDir::new()?;
let skill_dir = workspace.path().join(".agents/skills/version-skew");
std::fs::create_dir_all(&skill_dir)?;
let skill_path = skill_dir.join("SKILL.md");
let skill_body = "Repository instructions loaded across executor versions.";
std::fs::write(
&skill_path,
format!(
"---\nname: version-skew\ndescription: Check executor compatibility.\n---\n{skill_body}\n"
),
)?;
let selected_root = workspace.path().join("selected-skills");
let selected_skill_dir = selected_root.join("selected-version-skew");
std::fs::create_dir_all(&selected_skill_dir)?;
let selected_skill_body = "Selected instructions loaded across executor versions.";
std::fs::write(
selected_skill_dir.join("SKILL.md"),
format!(
"---\nname: selected-version-skew\ndescription: Check selected skill compatibility.\n---\n{selected_skill_body}\n"
),
)?;
let model = mock_model(workspace.path()).await?;
let model_url = model.uri();
std::fs::write(
codex_home.path().join("config.toml"),
@@ -91,9 +113,13 @@ async fn assert_noise_version_skew(app_binary: &Path, executor_binary: &Path) ->
r#"
model = "mock-model"
approval_policy = "never"
sandbox_mode = "danger-full-access"
sandbox_mode = "workspace-write"
model_provider = "mock_provider"
[features]
executor_capability_discovery = false
executor_capability_discovery_v2 = false
[model_providers.mock_provider]
name = "Mock provider"
base_url = "{model_url}/v1"
@@ -228,7 +254,17 @@ stream_max_retries = 0
&mut notifications,
/*id*/ 2,
"thread/start",
json!({"cwd": codex_home.path()}),
json!({
"cwd": workspace.path(),
"selectedCapabilityRoots": [{
"id": "version-skew-selected",
"location": {
"type": "environment",
"environmentId": REMOTE_ENVIRONMENT_ID,
"path": selected_root,
},
}],
}),
)
.await?;
let thread_id = thread["thread"]["id"]
@@ -244,8 +280,12 @@ stream_max_retries = 0
"threadId": thread_id,
"input": [{
"type": "text",
"text": "run the Noise compatibility command",
"text": "Use $selected-version-skew and run the Noise compatibility command.",
"textElements": [],
}, {
"type": "skill",
"name": "version-skew",
"path": skill_path,
}],
}),
)
@@ -263,9 +303,29 @@ stream_max_retries = 0
}
assert_eq!(
std::fs::read_to_string(codex_home.path().join("version-skew-output.txt"))?,
std::fs::read_to_string(workspace.path().join("version-skew-output.txt"))?,
EXPECTED_OUTPUT
);
let requests = model.received_requests().await.context("model requests")?;
let first_request = requests
.iter()
.find(|request| request.url.path() == "/v1/responses")
.context("model response request")?;
let first_request: Value = serde_json::from_slice(&first_request.body)?;
let input = first_request["input"].as_array().context("model input")?;
assert!(input.iter().any(|item| {
item["role"] == "developer"
&& item["content"]
.to_string()
.contains("selected-version-skew")
}));
for body in [skill_body, selected_skill_body] {
assert!(
input
.iter()
.any(|item| item["role"] == "user" && item["content"].to_string().contains(body))
);
}
assert_relay_data_is_encrypted(&captured_frames)?;
registry.verify().await;
model.verify().await;
@@ -825,11 +825,7 @@ async fn check_acquisition(enabled: bool, version: &'static str) -> Result<()> {
"explicit mentions must read through the captured executor"
);
for read in reads {
assert!(read["params"]["sandbox"].is_object());
assert_eq!(
read["params"]["sandbox"]["policyContext"]["cwd"],
json!(cwd)
);
assert!(read["params"]["sandbox"].is_null());
}
}
let discovery = requests
+9
View File
@@ -1071,6 +1071,15 @@ impl Environment {
let connection_state = client.subscribe_connection_state();
let client = client.get().await?;
let discover = || async {
if params.roots.iter().any(|root| {
root.sandbox.as_ref().is_some_and(|sandbox| {
sandbox.windows_sandbox_selection == crate::WindowsSandboxSelection::Mxc
})
}) {
let info = client.environment_info().await?;
crate::sandbox_selection::validate_remote_mxc(&info)
.map_err(|error| ExecServerError::Protocol(error.to_string()))?;
}
if params.roots.iter().any(|root| {
root.sandbox
.as_ref()
@@ -44,6 +44,12 @@ impl Environment {
.to_string(),
));
}
if params.sandbox.as_ref().is_some_and(|sandbox| {
sandbox.windows_sandbox_selection == crate::WindowsSandboxSelection::Mxc
}) {
crate::sandbox_selection::validate_remote_mxc(&info)
.map_err(|error| ExecServerError::Protocol(error.to_string()))?;
}
client.discover_capabilities_v2(params.clone()).await
};
self.discover_with_recovery(connection_state, discover)
@@ -111,6 +111,23 @@ async fn discovery_checks_version_and_retries_disconnect(
return Ok(());
}
assert_eq!(response?, DiscoverV2CapabilitiesResponse::default());
let mut sandbox = crate::FileSystemSandboxContext::from_permission_profile(
codex_protocol::models::PermissionProfile::read_only(),
params.cwd.clone(),
);
sandbox.windows_sandbox_selection = crate::WindowsSandboxSelection::Mxc;
let error = environment
.discover_capabilities_v2(DiscoverV2CapabilitiesRequest {
sandbox: Some(sandbox),
..params.clone()
})
.await
.expect_err("MXC must not be sent to an executor that cannot enforce it");
assert!(
error
.to_string()
.contains("does not support the requested MXC Windows sandbox")
);
assert_eq!(
*attempts.lock().await,
vec![
+48 -5
View File
@@ -7,6 +7,9 @@ use base64::Engine as _;
use base64::engine::general_purpose::STANDARD;
use bytes::Bytes;
use codex_file_system::FILE_WRITE_CHUNK_SIZE;
use codex_protocol::models::ManagedFileSystemPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemPath;
use codex_utils_path_uri::PathUri;
use futures::StreamExt;
use futures::TryFutureExt;
@@ -91,7 +94,7 @@ impl RemoteFileSystem {
let response = client
.fs_canonicalize(FsCanonicalizeParams {
path: path.clone(),
sandbox: sandbox.cloned(),
sandbox: read_sandbox(&client, path, sandbox).await?.cloned(),
})
.await
.map_err(map_remote_error)?;
@@ -110,7 +113,7 @@ impl RemoteFileSystem {
.fs_read_file(FsReadFileParams {
path: path.clone(),
follow_symlinks: (!options.follow_symlinks).then_some(false),
sandbox: sandbox.cloned(),
sandbox: read_sandbox(&client, path, sandbox).await?.cloned(),
})
.await
.map_err(map_remote_error)?;
@@ -129,6 +132,7 @@ impl RemoteFileSystem {
) -> FileSystemResult<FileSystemReadStream> {
trace!("remote fs read_file_stream");
let client = self.client.get().await.map_err(map_remote_error)?;
let sandbox = read_sandbox(&client, path, sandbox).await?;
file_stream::open(client, path.clone(), sandbox).await
}
@@ -227,7 +231,7 @@ impl RemoteFileSystem {
.fs_get_metadata(FsGetMetadataParams {
path: path.clone(),
follow_symlinks: (!options.follow_symlinks).then_some(false),
sandbox: sandbox.cloned(),
sandbox: read_sandbox(&client, path, sandbox).await?.cloned(),
})
.await
.map_err(map_remote_error)?;
@@ -251,7 +255,7 @@ impl RemoteFileSystem {
let response = client
.fs_read_directory(FsReadDirectoryParams {
path: path.clone(),
sandbox: sandbox.cloned(),
sandbox: read_sandbox(&client, path, sandbox).await?.cloned(),
})
.await
.map_err(map_remote_error)?;
@@ -278,7 +282,7 @@ impl RemoteFileSystem {
.fs_walk(FsWalkParams {
path: path.clone(),
options,
sandbox: sandbox.cloned(),
sandbox: read_sandbox(&client, path, sandbox).await?.cloned(),
})
.await
.map_err(map_remote_error)
@@ -567,6 +571,45 @@ impl Drop for FileWriteRegistration {
}
}
// Older executors sandbox any supplied policy. Omit unrestricted read policies only when
// their explicit paths are valid for the read target's convention. The executor rejects foreign
// targets itself; keep uncertain policies intact so it also validates their explicit paths.
async fn read_sandbox<'a>(
client: &ExecServerClient,
path: &PathUri,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> FileSystemResult<Option<&'a FileSystemSandboxContext>> {
let convention = path.infer_path_convention();
let sandbox = sandbox.filter(|sandbox| {
if sandbox.should_read_from_sandbox() {
return true;
}
let PermissionProfile::Managed {
file_system: ManagedFileSystemPermissions::Restricted { entries, .. },
..
} = &sandbox.permissions
else {
return false;
};
entries.iter().any(|entry| {
let FileSystemPath::Path { path } = &entry.path else {
return false;
};
let Some(convention) = convention else {
return true;
};
path.validate_config_path(convention).is_err()
})
});
if sandbox.is_some_and(|sandbox| {
sandbox.windows_sandbox_selection == crate::WindowsSandboxSelection::Mxc
}) {
let info = client.environment_info().await.map_err(map_remote_error)?;
crate::sandbox_selection::validate_remote_mxc(&info)?;
}
Ok(sandbox)
}
pub(crate) fn map_remote_error(error: ExecServerError) -> io::Error {
match error {
ExecServerError::Server { code, message } if code == NOT_FOUND_ERROR_CODE => {
@@ -45,11 +45,11 @@ use crate::protocol::InitializeResponse;
use crate::protocol::WireFsGetMetadataParams;
use crate::protocol::WireFsReadFileParams;
/// Absolute policies preserve foreign selection paths without adding legacy policy directories.
/// Foreign paths retain read restrictions; unrestricted reads stay compatible with old executors.
#[tokio::test]
async fn remote_file_system_sends_path_and_sandbox_cwd_uris_without_native_conversion() {
let (websocket_url, captured_params, server) =
record_read_file_params(/*expected_requests*/ 2).await;
record_read_file_params(/*expected_requests*/ 4).await;
let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new(
ExecServerTransportParams::websocket_url(
websocket_url,
@@ -87,15 +87,38 @@ async fn remote_file_system_sends_path_and_sandbox_cwd_uris_without_native_conve
);
}
for permission_profile in [
PermissionProfile::read_only(),
PermissionProfile::workspace_write(),
] {
let read_access = FileSystemSandboxContext::from_permission_profile(
permission_profile,
sandbox.cwd.clone(),
);
file_system
.read_file(&paths[0], Default::default(), Some(&read_access))
.await
.expect("unrestricted reads should succeed");
}
let captured_params = captured_params.await.expect("captured params");
let expected_params = paths
.into_iter()
let mut expected_params = paths
.iter()
.cloned()
.map(|path| FsReadFileParams {
path,
follow_symlinks: None,
sandbox: Some(sandbox.clone()),
})
.collect::<Vec<_>>();
expected_params.extend(vec![
FsReadFileParams {
path: paths[0].clone(),
follow_symlinks: None,
sandbox: None,
};
2
]);
assert_eq!(
captured_params
.iter()
@@ -107,7 +130,7 @@ async fn remote_file_system_sends_path_and_sandbox_cwd_uris_without_native_conve
"cwd": sandbox.cwd,
"workspaceRoots": sandbox.workspace_roots,
});
for (_, raw_sandbox) in captured_params {
for (_, raw_sandbox) in &captured_params[..2] {
assert_eq!(
(
raw_sandbox.get("cwd"),
@@ -120,6 +143,70 @@ async fn remote_file_system_sends_path_and_sandbox_cwd_uris_without_native_conve
server.await.expect("recording server should succeed");
}
#[tokio::test]
async fn remote_full_disk_reads_preserve_executor_permission_path_validation() {
let cases = [
("file:///workspace", "file:///allowed", false),
("file:///workspace", "file:///C:/allowed", true),
("file:///C:/workspace", "file:///C:/allowed", false),
("file:///C:/workspace", "file://server/share/allowed", false),
("file:///C:/workspace", "file:///allowed", true),
("file:///C:/workspace", "file:///C:/allowed%5Csecret", true),
("file:///workspace", "file:///%00/bad/path/YQ", true),
];
let (websocket_url, captured_params, server) = record_read_file_params(cases.len()).await;
let file_system = RemoteFileSystem::new(LazyRemoteExecServerClient::new(
ExecServerTransportParams::websocket_url(
websocket_url,
DEFAULT_REMOTE_EXEC_SERVER_CONNECT_TIMEOUT,
),
HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault),
));
let mut expected_params = Vec::new();
for (cwd, permission_path, retain_sandbox) in cases {
let cwd = PathUri::parse(cwd).expect("executor cwd");
let path = cwd.join("readable.txt").expect("executor file");
let policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry::new(
FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
FileSystemAccessMode::Read,
),
FileSystemSandboxEntry::new(
PathUri::parse(permission_path)
.expect("permission path")
.into(),
FileSystemAccessMode::Write,
),
]);
let sandbox = FileSystemSandboxContext::from_permission_profile(
PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted),
// Policy cwd is caller-supplied; only the operation path must be native to succeed.
PathUri::parse(permission_path).expect("deliberately mismatched policy cwd"),
);
file_system
.read_file(&path, Default::default(), Some(&sandbox))
.await
.expect("remote read");
expected_params.push(FsReadFileParams {
path,
follow_symlinks: None,
sandbox: retain_sandbox.then_some(sandbox),
});
}
assert_eq!(
captured_params
.await
.expect("captured params")
.into_iter()
.map(|(params, _)| params)
.collect::<Vec<_>>(),
expected_params,
);
server.await.expect("recording server");
}
/// Only dynamic policies retain legacy selection fields; all policies preserve the new context.
#[tokio::test]
async fn remote_file_system_preserves_only_cwd_dependent_policy_directories() {
@@ -745,8 +832,8 @@ async fn record_read_file_params(
let raw_params = request.params.expect("fs/readFile params should exist");
let raw_sandbox = raw_params
.get("sandbox")
.expect("fs/readFile sandbox should exist")
.clone();
.cloned()
.unwrap_or(serde_json::Value::Null);
let params = serde_json::from_value::<WireFsReadFileParams>(raw_params)
.expect("fs/readFile wire params should deserialize")
.try_into_request(|wire| {
+11 -1
View File
@@ -1,4 +1,4 @@
//! Resolves an executor sandbox context to a concrete local sandbox implementation.
//! Resolves local sandbox implementations and checks support for an explicitly selected remote MXC.
use codex_file_system::FileSystemSandboxContext;
use codex_file_system::WindowsSandboxSelection;
@@ -8,6 +8,16 @@ use codex_sandboxing::SandboxManager;
use codex_sandboxing::SandboxType;
use codex_sandboxing::SandboxablePreference;
pub(crate) fn validate_remote_mxc(info: &crate::EnvironmentInfo) -> std::io::Result<()> {
if !info.capabilities.windows_mxc {
return Err(std::io::Error::new(
std::io::ErrorKind::Unsupported,
"exec-server does not support the requested MXC Windows sandbox; update the executor",
));
}
Ok(())
}
pub(crate) fn select_sandbox(
manager: &SandboxManager,
permission_profile: &PermissionProfile,
@@ -134,7 +134,7 @@ async fn sandboxed_file_system_rejects_non_native_uri_as_invalid_input() {
/*codex_linux_sandbox_exe*/ None,
)
.expect("runtime paths");
let file_system = SandboxedFileSystem::new(runtime_paths.clone());
let file_system = SandboxedFileSystem::new(runtime_paths);
let cwd = PathUri::from_host_native_path(std::env::temp_dir()).expect("native temporary cwd");
let sandbox = FileSystemSandboxContext::from_permission_profile(
PermissionProfile::from_runtime_permissions(
@@ -150,34 +150,6 @@ async fn sandboxed_file_system_rejects_non_native_uri_as_invalid_input() {
.expect_err("non-native URI should be rejected");
assert_eq!(error.kind(), io::ErrorKind::InvalidInput);
// A foreign permission path must not select the unsandboxed filesystem, even with a root grant.
let file_system = crate::LocalFileSystem::with_runtime_paths(runtime_paths);
let foreign = non_native_uri();
let policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry::new(
FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
FileSystemAccessMode::Write,
),
FileSystemSandboxEntry::new(foreign.clone().into(), FileSystemAccessMode::Write),
]);
let sandbox = FileSystemSandboxContext::from_permission_profile(
PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted),
cwd,
);
let readable = tempfile::NamedTempFile::new().expect("readable file");
let error = file_system
.read_file(
&PathUri::from_host_native_path(readable.path()).expect("readable file URI"),
Default::default(),
Some(&sandbox),
)
.await
.expect_err("foreign permission path must not allow unsandboxed access");
assert_eq!(error.kind(), io::ErrorKind::InvalidInput);
assert!(error.to_string().contains(&foreign.to_string()));
}
fn non_native_uri() -> PathUri {
+37 -3
View File
@@ -135,7 +135,7 @@ async fn capability_discovery_without_sandbox_support_only_sends_full_disk_reads
if let Message::Text(text) = &message {
let request: serde_json::Value = serde_json::from_str(text.as_ref())?;
if request.get("method").and_then(serde_json::Value::as_str)
== Some(CAPABILITY_ROOTS_DISCOVER_METHOD)
.is_some_and(|method| method == CAPABILITY_ROOTS_DISCOVER_METHOD || method.starts_with("fs/"))
{
requests_tx.send(request)?;
}
@@ -149,6 +149,8 @@ async fn capability_discovery_without_sandbox_support_only_sends_full_disk_reads
for pointer in [
"/result/environmentInfo/capabilities/capabilityDiscoverySandbox",
"/result/capabilities/capabilityDiscoverySandbox",
"/result/environmentInfo/capabilities/windowsMxc",
"/result/capabilities/windowsMxc",
] {
if let Some(capability) = response.pointer_mut(pointer) {
*capability = false.into();
@@ -228,8 +230,8 @@ async fn capability_discovery_without_sandbox_support_only_sends_full_disk_reads
roots: vec![
request.clone(),
CapabilityRootDiscoverRequest {
sandbox: Some(restricted),
..request
sandbox: Some(restricted.clone()),
..request.clone()
},
],
})
@@ -244,6 +246,38 @@ async fn capability_discovery_without_sandbox_support_only_sends_full_disk_reads
requests_rx.try_recv(),
Err(tokio::sync::mpsc::error::TryRecvError::Empty)
);
let mut unsupported = restricted;
unsupported.windows_sandbox_selection = codex_exec_server::WindowsSandboxSelection::Mxc;
let error = environment
.discover_capability_roots(CapabilityRootsDiscoverParams {
roots: vec![CapabilityRootDiscoverRequest {
sandbox: Some(unsupported.clone()),
..request.clone()
}],
})
.await
.expect_err("unsupported MXC must not reach an older decoder");
assert!(
error
.to_string()
.contains("does not support the requested MXC Windows sandbox")
);
let error = environment
.get_filesystem()
.get_metadata(&request.path, Default::default(), Some(&unsupported))
.await
.expect_err("host discovery must fail closed too");
assert_eq!(error.kind(), std::io::ErrorKind::Unsupported);
assert!(
error
.to_string()
.contains("does not support the requested MXC Windows sandbox")
);
assert_eq!(
requests_rx.try_recv(),
Err(tokio::sync::mpsc::error::TryRecvError::Empty)
);
Ok(())
}
@@ -1051,6 +1051,68 @@ async fn file_system_full_disk_read_skips_sandbox_only_for_reads() -> Result<()>
Ok(())
}
#[test_case(FileSystemImplementation::Local ; "local")]
#[test_case(FileSystemImplementation::Remote ; "remote")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn full_disk_reads_reject_foreign_permission_paths(
implementation: FileSystemImplementation,
) -> Result<()> {
let context = create_file_system_context(implementation).await?;
let tmp = TempDir::new()?;
let file = tmp.path().join("note.txt");
std::fs::write(&file, b"note")?;
let file = PathUri::from_host_native_path(file)?;
let root = PathUri::from_host_native_path(tmp.path())?;
let foreign = PathUri::parse(if cfg!(windows) {
"file:///private"
} else {
"file:///C:/private"
})?;
let policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry::new(
FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
FileSystemAccessMode::Read,
),
FileSystemSandboxEntry::new(foreign.clone().into(), FileSystemAccessMode::Write),
]);
let sandbox = FileSystemSandboxContext::from_permission_profile(
PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted),
root.clone(),
);
let fs = context.file_system;
for result in [
fs.canonicalize(&file, Some(&sandbox)).await.map(|_| ()),
fs.read_file(&file, Default::default(), Some(&sandbox))
.await
.map(|_| ()),
fs.read_file_stream(&file, Some(&sandbox)).await.map(|_| ()),
fs.get_metadata(&file, Default::default(), Some(&sandbox))
.await
.map(|_| ()),
fs.read_directory(&root, Some(&sandbox)).await.map(|_| ()),
fs.walk(
&root,
WalkOptions {
max_depth: 1,
max_directories: 1,
max_entries: 1,
follow_directory_symlinks: false,
prune_hidden_directories: false,
},
Some(&sandbox),
)
.await
.map(|_| ()),
] {
let error = result.expect_err("foreign permission paths must not bypass validation");
assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
assert!(error.to_string().contains(&foreign.to_string()));
}
Ok(())
}
/// A stream already opened under one environment remains usable after its accessor is dropped;
/// cache keys still distinguish a new filesystem or changed captured permissions.
#[test_case(FileSystemImplementation::Local ; "local")]