mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Propagate temporary directories into sandbox policy contexts (#40660)
## Why Filesystem policies containing `:tmpdir` need executor-local directory bindings, including when execution occurs in a remote environment. ## What changed - Cache temporary directories reported by remote executors and discover the equivalent directories for local environments. - Preserve the cached directories across inherited environment selection and include them in `FileSystemSandboxContext`. - Add `FileSystemSandboxPolicyContext` and a context accessor so filesystem policy entries can be resolved with the executor-owned current directory, workspace roots, and temporary directories. ## Testing Extend environment-selection tests to cover local, remote, and inherited temporary-directory policy context. GitOrigin-RevId: 36335af3465c529f024bf69293af288803dd582d
This commit is contained in:
@@ -8,6 +8,7 @@ use arc_swap::ArcSwap;
|
||||
use async_channel::Sender;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_exec_server::EnvironmentConnectionState;
|
||||
use codex_exec_server::EnvironmentInfo;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_exec_server::ExecServerError;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
@@ -136,6 +137,7 @@ type PendingConfigurationResult = Result<EnvironmentConfig, String>;
|
||||
struct ResolvedEnvironment {
|
||||
environment: Arc<Environment>,
|
||||
shell: Option<Shell>,
|
||||
temporary_directories: Option<Vec<PathUri>>,
|
||||
shell_snapshot: ShellSnapshotTask,
|
||||
shell_snapshot_v2_supported: bool,
|
||||
installed_config: Option<EnvironmentConfig>,
|
||||
@@ -239,6 +241,7 @@ impl ThreadEnvironments {
|
||||
futures::future::ready(Ok(ResolvedEnvironment {
|
||||
environment: environment.environment,
|
||||
shell: environment.shell,
|
||||
temporary_directories: environment.temporary_directories,
|
||||
shell_snapshot: environment.shell_snapshot,
|
||||
shell_snapshot_v2_supported: environment.shell_snapshot_v2_supported,
|
||||
installed_config: None,
|
||||
@@ -604,10 +607,13 @@ impl ThreadEnvironments {
|
||||
};
|
||||
// Resolve the attachment only after both prerequisites are ready.
|
||||
let ((), installed_config) = tokio::try_join!(connection_ready, configuration_ready)?;
|
||||
let (shell, shell_snapshot_v2_supported) = if environment.is_remote() {
|
||||
let (shell, temporary_directories, shell_snapshot_v2_supported) = if environment.is_remote()
|
||||
{
|
||||
match environment.info().await {
|
||||
Ok(info) => (
|
||||
match Shell::from_environment_shell_info(info.shell) {
|
||||
Ok(info) => {
|
||||
let temporary_directories = info.temporary_directories;
|
||||
let shell_snapshot_v2_supported = info.capabilities.shell_snapshot_v2;
|
||||
let shell = match Shell::from_environment_shell_info(info.shell) {
|
||||
Ok(shell) => Some(shell),
|
||||
Err(err) => {
|
||||
tracing::warn!(
|
||||
@@ -615,16 +621,20 @@ impl ThreadEnvironments {
|
||||
);
|
||||
None
|
||||
}
|
||||
},
|
||||
info.capabilities.shell_snapshot_v2,
|
||||
),
|
||||
};
|
||||
(shell, temporary_directories, shell_snapshot_v2_supported)
|
||||
}
|
||||
Err(err) => {
|
||||
tracing::warn!("failed to get info for environment `{environment_id}`: {err}");
|
||||
(None, false)
|
||||
(None, None, false)
|
||||
}
|
||||
}
|
||||
} else {
|
||||
(Some(local_shell), cfg!(unix))
|
||||
(
|
||||
Some(local_shell),
|
||||
Some(EnvironmentInfo::local_temporary_directories()),
|
||||
cfg!(unix),
|
||||
)
|
||||
};
|
||||
let task = shell_snapshot
|
||||
.build(Arc::clone(&environment), selection.cwd, shell.clone())
|
||||
@@ -636,6 +646,7 @@ impl ThreadEnvironments {
|
||||
Ok(ResolvedEnvironment {
|
||||
environment,
|
||||
shell,
|
||||
temporary_directories,
|
||||
shell_snapshot: task,
|
||||
shell_snapshot_v2_supported,
|
||||
installed_config,
|
||||
@@ -715,6 +726,7 @@ impl TurnEnvironmentState {
|
||||
turn_environment.shell_snapshot = environment.shell_snapshot;
|
||||
turn_environment.shell_snapshot_v2_supported =
|
||||
environment.shell_snapshot_v2_supported;
|
||||
turn_environment.temporary_directories = environment.temporary_directories;
|
||||
Some(Self::Ready(turn_environment))
|
||||
}
|
||||
Some(Err(err)) => {
|
||||
@@ -881,6 +893,7 @@ mod tests {
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::models::ActivePermissionProfile;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -986,7 +999,10 @@ mod tests {
|
||||
.send(Message::Text(
|
||||
serde_json::json!({
|
||||
"id": info["id"],
|
||||
"result": { "shell": { "name": "zsh", "path": "/bin/zsh" } }
|
||||
"result": {
|
||||
"shell": { "name": "zsh", "path": "/bin/zsh" },
|
||||
"temporaryDirectories": ["file:///tmp/remote"],
|
||||
}
|
||||
})
|
||||
.to_string()
|
||||
.into(),
|
||||
@@ -1111,9 +1127,21 @@ url = "ws://127.0.0.1:8765"
|
||||
|
||||
let snapshot = turn_environments.snapshot().await;
|
||||
let environment = snapshot.primary().expect("local environment");
|
||||
let expected_temporary_directories = EnvironmentInfo::local_temporary_directories();
|
||||
|
||||
assert_eq!(environment.shell.as_ref(), Some(&local_shell));
|
||||
assert_eq!(environment.config(), &expected_config);
|
||||
assert_eq!(
|
||||
environment
|
||||
.sandbox_context(/*additional_permissions*/ None)
|
||||
.policy_context()
|
||||
.expect("selected environment sandbox context has cwd"),
|
||||
FileSystemSandboxPolicyContext {
|
||||
cwd: environment.cwd(),
|
||||
workspace_roots: &[],
|
||||
temporary_directories: Some(expected_temporary_directories.as_slice()),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1424,6 +1452,20 @@ url = "ws://127.0.0.1:8765"
|
||||
vec![expected_config.clone(), expected_config]
|
||||
);
|
||||
assert_eq!(attached.to_selections(), vec![remote, local]);
|
||||
let environment = attached.primary().expect("remote environment");
|
||||
let expected_temporary_directories =
|
||||
[PathUri::parse("file:///tmp/remote").expect("remote temporary directory")];
|
||||
assert_eq!(
|
||||
environment
|
||||
.sandbox_context(/*additional_permissions*/ None)
|
||||
.policy_context()
|
||||
.expect("selected environment sandbox context has cwd"),
|
||||
FileSystemSandboxPolicyContext {
|
||||
cwd: environment.cwd(),
|
||||
workspace_roots: &[],
|
||||
temporary_directories: Some(expected_temporary_directories.as_slice()),
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
next_starting
|
||||
.refresh_readiness()
|
||||
@@ -1637,7 +1679,7 @@ url = "ws://127.0.0.1:8765"
|
||||
Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string()))
|
||||
.expect("inherited environment"),
|
||||
);
|
||||
let inherited = TurnEnvironment::new(
|
||||
let mut inherited = TurnEnvironment::new(
|
||||
TurnEnvironmentSelection {
|
||||
config: EnvironmentConfigState::Ready(test_environment_config()),
|
||||
..selection.clone()
|
||||
@@ -1646,6 +1688,9 @@ url = "ws://127.0.0.1:8765"
|
||||
Arc::clone(&inherited_environment),
|
||||
/*shell*/ None,
|
||||
);
|
||||
inherited.temporary_directories = Some(vec![
|
||||
PathUri::parse("file:///tmp/inherited").expect("temporary directory"),
|
||||
]);
|
||||
let manager = Arc::new(environment_manager_without_environments());
|
||||
manager
|
||||
.upsert_environment(
|
||||
@@ -1687,6 +1732,15 @@ url = "ws://127.0.0.1:8765"
|
||||
let inherited = snapshot.primary().expect("inherited environment");
|
||||
assert!(Arc::ptr_eq(&inherited.environment, &inherited_environment));
|
||||
assert_eq!(inherited.config(), &child_config);
|
||||
assert_eq!(
|
||||
inherited
|
||||
.sandbox_context(/*additional_permissions*/ None)
|
||||
.temporary_directories
|
||||
.as_deref(),
|
||||
Some(
|
||||
[PathUri::parse("file:///tmp/inherited").expect("temporary directory")].as_slice()
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -9103,6 +9103,7 @@ async fn capability_discovery_uses_environment_permission_profile() {
|
||||
permissions: environment.permission_profile().clone().into(),
|
||||
cwd: Some(environment.cwd().clone()),
|
||||
workspace_roots: environment.workspace_roots().to_vec(),
|
||||
temporary_directories: environment.temporary_directories.clone(),
|
||||
windows_sandbox_level: WindowsSandboxLevel::Elevated,
|
||||
windows_sandbox_private_desktop: false,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
|
||||
@@ -43,6 +43,8 @@ pub(crate) struct TurnEnvironment {
|
||||
pub(crate) selection: TurnEnvironmentSelection,
|
||||
pub(crate) config_origin: EnvironmentConfigOrigin,
|
||||
pub(crate) environment: Arc<Environment>,
|
||||
/// Cached from the selected executor; `None` means it did not report them.
|
||||
pub(crate) temporary_directories: Option<Vec<PathUri>>,
|
||||
pub(crate) shell: Option<shell::Shell>,
|
||||
pub(crate) shell_snapshot: ShellSnapshotTask,
|
||||
pub(crate) shell_snapshot_v2_supported: bool,
|
||||
@@ -60,6 +62,7 @@ impl TurnEnvironment {
|
||||
selection,
|
||||
config_origin,
|
||||
environment,
|
||||
temporary_directories: None,
|
||||
shell,
|
||||
shell_snapshot: futures::future::ready(None).boxed().shared(),
|
||||
shell_snapshot_v2_supported: false,
|
||||
@@ -123,6 +126,7 @@ impl TurnEnvironment {
|
||||
permissions: permissions.into(),
|
||||
cwd: Some(self.cwd().clone()),
|
||||
workspace_roots: self.workspace_roots().to_vec(),
|
||||
temporary_directories: self.temporary_directories.clone(),
|
||||
windows_sandbox_level: executor_windows_sandbox_level(
|
||||
config.windows_sandbox_level,
|
||||
self.cwd(),
|
||||
@@ -161,6 +165,7 @@ impl std::fmt::Debug for TurnEnvironment {
|
||||
.field("environment", &self.environment)
|
||||
.field("cwd", &self.selection.cwd)
|
||||
.field("workspace_roots", &self.selection.workspace_roots)
|
||||
.field("temporary_directories", &self.temporary_directories)
|
||||
.field("shell", &self.shell)
|
||||
.field("config", self.config())
|
||||
.field("config_origin", &self.config_origin)
|
||||
|
||||
@@ -100,6 +100,7 @@ impl ApplyPatchRuntime {
|
||||
permissions: permissions.into(),
|
||||
cwd: Some(attempt.sandbox_cwd.clone()),
|
||||
workspace_roots: attempt.workspace_roots.to_vec(),
|
||||
temporary_directories: None,
|
||||
windows_sandbox_level: executor_windows_sandbox_level(
|
||||
attempt.windows_sandbox_level,
|
||||
attempt.sandbox_cwd,
|
||||
|
||||
@@ -344,6 +344,7 @@ async fn file_system_sandbox_context_respects_sandbox_request() {
|
||||
permissions: permissions.into(),
|
||||
cwd: Some(cwd.clone()),
|
||||
workspace_roots: vec![cwd],
|
||||
temporary_directories: None,
|
||||
windows_sandbox_level: WindowsSandboxLevel::RestrictedToken,
|
||||
windows_sandbox_private_desktop: false,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
|
||||
@@ -502,6 +502,7 @@ impl<'a> SandboxAttempt<'a> {
|
||||
permissions: exec_server_permissions.into(),
|
||||
cwd: Some(exec_request.windows_sandbox_policy_cwd.clone()),
|
||||
workspace_roots: self.workspace_roots.to_vec(),
|
||||
temporary_directories: None,
|
||||
windows_sandbox_level: executor_windows_sandbox_level(
|
||||
self.windows_sandbox_level,
|
||||
self.sandbox_cwd,
|
||||
|
||||
@@ -369,6 +369,7 @@ fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
permissions: exec_server_permissions.clone().into(),
|
||||
cwd: Some(cwd_uri.clone()),
|
||||
workspace_roots: vec![cwd_uri.clone()],
|
||||
temporary_directories: None,
|
||||
windows_sandbox_level: if cfg!(windows) {
|
||||
codex_protocol::config_types::WindowsSandboxLevel::RestrictedToken
|
||||
} else {
|
||||
|
||||
@@ -153,9 +153,16 @@ pub enum EnvironmentStatusKind {
|
||||
}
|
||||
|
||||
impl EnvironmentInfo {
|
||||
/// Returns information about the current local exec-server process.
|
||||
pub fn local() -> Self {
|
||||
/// Returns executor-local default directories used to resolve `:tmpdir`.
|
||||
///
|
||||
/// This is separate from `local` so orchestrator startup can cache the
|
||||
/// directories without repeating local shell detection.
|
||||
pub fn local_temporary_directories() -> Vec<PathUri> {
|
||||
let cwd = std::env::current_dir().ok();
|
||||
Self::local_temporary_directories_with_cwd(cwd.as_deref())
|
||||
}
|
||||
|
||||
fn local_temporary_directories_with_cwd(cwd: Option<&std::path::Path>) -> Vec<PathUri> {
|
||||
let temporary_directory_env_vars: &[&str] = if cfg!(windows) {
|
||||
&["TEMP", "TMP"]
|
||||
} else {
|
||||
@@ -181,6 +188,22 @@ impl EnvironmentInfo {
|
||||
temporary_directories.push(path);
|
||||
}
|
||||
}
|
||||
temporary_directories
|
||||
}
|
||||
|
||||
/// Returns information about the current local exec-server process.
|
||||
pub fn local() -> Self {
|
||||
let cwd = std::env::current_dir().ok();
|
||||
let temporary_directories = Self::local_temporary_directories_with_cwd(cwd.as_deref());
|
||||
let normalize_temp_path = |path: std::ffi::OsString| {
|
||||
PathUri::from_host_native_path(&path).ok().or_else(|| {
|
||||
if cfg!(unix) {
|
||||
PathUri::from_host_native_path(cwd.as_ref()?.join(path)).ok()
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
};
|
||||
let temp_dir = normalize_temp_path(std::env::temp_dir().into_os_string());
|
||||
|
||||
Self {
|
||||
|
||||
@@ -12,6 +12,7 @@ use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntryMissingPathBehavior;
|
||||
use codex_protocol::permissions::FileSystemSandboxKind;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicyContext;
|
||||
use codex_protocol::permissions::FileSystemSpecialPath;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
@@ -332,6 +333,9 @@ pub struct FileSystemSandboxContext {
|
||||
pub cwd: Option<PathUri>,
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub workspace_roots: Vec<PathUri>,
|
||||
/// Executor-local default directories used to resolve `:tmpdir` policy entries.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub temporary_directories: Option<Vec<PathUri>>,
|
||||
pub windows_sandbox_level: WindowsSandboxLevel,
|
||||
#[serde(default)]
|
||||
pub windows_sandbox_private_desktop: bool,
|
||||
@@ -376,6 +380,7 @@ impl FileSystemSandboxContext {
|
||||
permissions: permissions.into(),
|
||||
cwd,
|
||||
workspace_roots,
|
||||
temporary_directories: None,
|
||||
windows_sandbox_level: WindowsSandboxLevel::Disabled,
|
||||
windows_sandbox_private_desktop: false,
|
||||
windows_sandbox_proxy_settings_mode: None,
|
||||
@@ -393,6 +398,17 @@ impl FileSystemSandboxContext {
|
||||
&& !file_system_policy.has_full_disk_write_access()
|
||||
}
|
||||
|
||||
/// Borrows the executor-owned paths needed to interpret filesystem policy entries.
|
||||
///
|
||||
/// Returns `None` after the cwd has been omitted from a transport context.
|
||||
pub fn policy_context(&self) -> Option<FileSystemSandboxPolicyContext<'_>> {
|
||||
Some(FileSystemSandboxPolicyContext {
|
||||
cwd: self.cwd.as_ref()?,
|
||||
workspace_roots: &self.workspace_roots,
|
||||
temporary_directories: self.temporary_directories.as_deref(),
|
||||
})
|
||||
}
|
||||
|
||||
pub fn has_cwd_dependent_permissions(&self) -> bool {
|
||||
match &self.permissions {
|
||||
ExecPermissionProfile::Managed {
|
||||
|
||||
@@ -266,6 +266,17 @@ pub struct RawFileSystemSandboxPolicy {
|
||||
pub entries: Vec<RawFileSystemSandboxEntry>,
|
||||
}
|
||||
|
||||
/// Executor-owned paths needed to interpret filesystem sandbox policy entries.
|
||||
///
|
||||
/// Orchestrator callers keep these as `PathUri` values until execution crosses
|
||||
/// into the executor that owns them.
|
||||
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
|
||||
pub struct FileSystemSandboxPolicyContext<'a> {
|
||||
pub cwd: &'a PathUri,
|
||||
pub workspace_roots: &'a [PathUri],
|
||||
pub temporary_directories: Option<&'a [PathUri]>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
struct ResolvedFileSystemEntry {
|
||||
path: AbsolutePathBuf,
|
||||
|
||||
Reference in New Issue
Block a user