mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: preserve approval sandbox decisions in unified exec (#24981)
## Why This PR fixes approval sandbox semantics in the unified-exec path. The zsh-fork runtime exposed the bug because the shell can do meaningful work before any intercepted child `execv(2)` exists: redirections, builtins, globbing, and pipeline setup all happen in the launch process. If the model requested `sandbox_permissions=require_escalated`, or an exec-policy `allow` rule explicitly bypassed the sandbox, that approved sandbox decision needs to be preserved for the launch path and for intercepted execs that use the same approval machinery. The behavior is not only about zsh fork. The production changes are in shared approval/escalation code, so they also affect non-zsh-fork intercepted exec paths that go through the same sandbox decision logic. The narrow intent is to preserve the approval decision while still keeping denied-read profiles and bounded additional-permission requests sandboxed. ## Production Changes - `codex-rs/core/src/tools/runtimes/unified_exec.rs`: derives a `launch_sandbox_permissions` value from the requested sandbox permissions and the runtime filesystem policy, then uses that value for managed-network/env setup and launch sandbox selection. This keeps full approval or policy-bypass decisions visible to the first unified-exec attempt, while still preventing a full sandbox override from discarding denied-read restrictions. Direct unified exec keeps the same decision surface; the important difference is that zsh-fork launch setup no longer accidentally loses the approved parent sandbox decision. - `codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs`: makes intercepted-exec escalation selection explicit for the three sandbox permission modes. `UseDefault` only escalates when an exec-policy decision allows sandbox bypass, `RequireEscalated` escalates when unsandboxed execution is allowed, and `WithAdditionalPermissions` escalates through the bounded additional-permissions path instead of being treated as a full unsandboxed override. Unsandboxed intercepted execs now also rebuild the environment as `RequireEscalated`, which strips managed-network proxy variables consistently with other unsandboxed execution. ## Test Coverage Most of the PR is tests. The new coverage verifies: - unified exec preserves parent approval and exec-policy sandbox decisions for zsh-fork launch selection; - bounded `with_additional_permissions` remains sandboxed and permission-profile based; - denied-read profiles are not weakened by parent approval; - explicit prompt rules still prompt for intercepted execs after the parent command is approved; - unsandboxed intercepted execs strip managed-network env vars. No documentation update is needed; this is an internal approval/sandbox correctness fix. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/24981). * #24982 * __->__ #24981
This commit is contained in:
Michael Bolin
committed by
GitHub
Unverified
GitHub
parent
2ee3358c00
commit
e6c470957d
@@ -630,9 +630,11 @@ impl EscalationPolicy for CoreShellActionProvider {
|
||||
let decision_driven_by_policy =
|
||||
Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision);
|
||||
let unsandboxed_allowed = unsandboxed_execution_allowed(&self.file_system_sandbox_policy);
|
||||
let needs_escalation = unsandboxed_allowed
|
||||
&& (self.sandbox_permissions.requires_escalated_permissions()
|
||||
|| decision_driven_by_policy);
|
||||
let needs_escalation = match self.sandbox_permissions {
|
||||
SandboxPermissions::UseDefault => unsandboxed_allowed && decision_driven_by_policy,
|
||||
SandboxPermissions::RequireEscalated => unsandboxed_allowed,
|
||||
SandboxPermissions::WithAdditionalPermissions => true,
|
||||
};
|
||||
|
||||
let decision_source = if decision_driven_by_policy {
|
||||
DecisionSource::PrefixRule
|
||||
@@ -858,7 +860,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor {
|
||||
EscalationExecution::Unsandboxed => PreparedExec {
|
||||
command,
|
||||
cwd: workdir.to_path_buf(),
|
||||
env,
|
||||
env: exec_env_for_sandbox_permissions(&env, SandboxPermissions::RequireEscalated),
|
||||
arg0: Some(first_arg.clone()),
|
||||
},
|
||||
EscalationExecution::TurnDefault => {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use super::CoreShellActionProvider;
|
||||
use super::CoreShellCommandExecutor;
|
||||
use super::InterceptedExecPolicyContext;
|
||||
use super::ParsedShellCommand;
|
||||
use super::commands_for_intercepted_exec_policy;
|
||||
@@ -16,6 +17,8 @@ use codex_execpolicy::PolicyParser;
|
||||
use codex_execpolicy::RuleMatch;
|
||||
use codex_hooks::Hooks;
|
||||
use codex_hooks::HooksConfig;
|
||||
use codex_network_proxy::PROXY_ACTIVE_ENV_KEY;
|
||||
use codex_network_proxy::PROXY_ENV_KEYS;
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
@@ -30,13 +33,16 @@ use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_protocol::protocol::GuardianCommandSource;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use codex_sandboxing::policy_transforms::effective_permission_profile;
|
||||
use codex_shell_escalation::EscalationExecution;
|
||||
use codex_shell_escalation::EscalationPermissions;
|
||||
use codex_shell_escalation::ExecResult;
|
||||
use codex_shell_escalation::ResolvedPermissionProfile;
|
||||
use codex_shell_escalation::ShellCommandExecutor;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
@@ -349,6 +355,108 @@ fn shell_request_escalation_execution_is_explicit() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unsandboxed_intercepted_exec_strips_managed_network_env() -> anyhow::Result<()> {
|
||||
let workdir = test_sandbox_cwd();
|
||||
let executor = CoreShellCommandExecutor {
|
||||
command: Vec::new(),
|
||||
cwd: workdir.clone(),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
|
||||
sandbox: SandboxType::None,
|
||||
env: HashMap::new(),
|
||||
network: None,
|
||||
windows_sandbox_level: WindowsSandboxLevel::Disabled,
|
||||
arg0: None,
|
||||
sandbox_policy_cwd: workdir.clone(),
|
||||
windows_sandbox_workspace_roots: vec![workdir.clone()],
|
||||
codex_linux_sandbox_exe: None,
|
||||
use_legacy_landlock: false,
|
||||
};
|
||||
let mut env = HashMap::new();
|
||||
env.insert(PROXY_ACTIVE_ENV_KEY.to_string(), "1".to_string());
|
||||
for key in PROXY_ENV_KEYS {
|
||||
env.insert((*key).to_string(), format!("proxy-{key}"));
|
||||
}
|
||||
|
||||
let prepared = executor
|
||||
.prepare_escalated_exec(
|
||||
&AbsolutePathBuf::from_absolute_path("/usr/bin/curl")?,
|
||||
&["curl".to_string(), "example.com".to_string()],
|
||||
&workdir,
|
||||
env,
|
||||
EscalationExecution::Unsandboxed,
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert!(
|
||||
!prepared.env.contains_key(PROXY_ACTIVE_ENV_KEY),
|
||||
"unsandboxed intercepted exec should strip the managed-network active marker"
|
||||
);
|
||||
for key in PROXY_ENV_KEYS {
|
||||
assert!(
|
||||
!prepared.env.contains_key(*key),
|
||||
"unsandboxed intercepted exec should strip managed-network proxy env var {key}"
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyhow::Result<()> {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let requested_permissions = AdditionalPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
/*read*/ None,
|
||||
Some(vec![
|
||||
AbsolutePathBuf::from_absolute_path("/tmp/output").unwrap(),
|
||||
]),
|
||||
)),
|
||||
..Default::default()
|
||||
};
|
||||
let workdir = test_sandbox_cwd();
|
||||
let permission_profile = effective_permission_profile(
|
||||
&PermissionProfile::workspace_write(),
|
||||
Some(&requested_permissions),
|
||||
);
|
||||
let provider = CoreShellActionProvider {
|
||||
policy: Arc::new(RwLock::new(codex_execpolicy::Policy::empty())),
|
||||
session: Arc::new(session),
|
||||
turn: Arc::new(turn_context),
|
||||
call_id: "preapproved-additional-permissions".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
permission_profile: permission_profile.clone(),
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
sandbox_permissions: SandboxPermissions::WithAdditionalPermissions,
|
||||
approval_sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prompt_permissions: Some(requested_permissions),
|
||||
stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)),
|
||||
};
|
||||
|
||||
let action = codex_shell_escalation::EscalationPolicy::determine_action(
|
||||
&provider,
|
||||
&AbsolutePathBuf::from_absolute_path("/usr/bin/printf")?,
|
||||
&["printf".to_string(), "hello".to_string()],
|
||||
&workdir,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let expected = codex_shell_escalation::EscalationDecision::Escalate(
|
||||
EscalationExecution::Permissions(EscalationPermissions::ResolvedPermissionProfile(
|
||||
ResolvedPermissionProfile { permission_profile },
|
||||
)),
|
||||
);
|
||||
assert_eq!(
|
||||
action, expected,
|
||||
"preapproved with_additional_permissions should escalate through the resolved permission profile"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Result<()> {
|
||||
let (session, mut turn_context) = make_session_and_context().await;
|
||||
|
||||
@@ -265,17 +265,15 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecProcess> for UnifiedExecRunt
|
||||
let base_command = &req.command;
|
||||
let session_shell = ctx.session.user_shell();
|
||||
let (file_system_sandbox_policy, _) = attempt.permissions.to_runtime_permissions();
|
||||
let sandbox_permissions = sandbox_permissions_preserving_denied_reads(
|
||||
let launch_sandbox_permissions = sandbox_permissions_preserving_denied_reads(
|
||||
req.sandbox_permissions,
|
||||
&file_system_sandbox_policy,
|
||||
);
|
||||
let req = &UnifiedExecRequest {
|
||||
sandbox_permissions,
|
||||
..req.clone()
|
||||
};
|
||||
let managed_network =
|
||||
managed_network_for_sandbox_permissions(req.network.as_ref(), req.sandbox_permissions);
|
||||
let mut env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions);
|
||||
let managed_network = managed_network_for_sandbox_permissions(
|
||||
req.network.as_ref(),
|
||||
launch_sandbox_permissions,
|
||||
);
|
||||
let mut env = exec_env_for_sandbox_permissions(&req.env, launch_sandbox_permissions);
|
||||
if let Some(network) = managed_network {
|
||||
network.apply_to_env(&mut env);
|
||||
}
|
||||
@@ -412,6 +410,7 @@ mod tests {
|
||||
use crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_tools::ZshForkConfig;
|
||||
use std::time::Duration;
|
||||
use tempfile::tempdir;
|
||||
|
||||
@@ -473,4 +472,107 @@ mod tests {
|
||||
|
||||
assert_eq!(runtime.sandbox_cwd(&request), Some(&sandbox_cwd));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn zsh_fork_first_attempt_preserves_parent_sandbox_override() {
|
||||
let manager = UnifiedExecProcessManager::default();
|
||||
let request = test_request(
|
||||
SandboxPermissions::RequireEscalated,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
);
|
||||
let direct_runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct);
|
||||
let zsh_fork_runtime = UnifiedExecRuntime::new(&manager, zsh_fork_mode());
|
||||
|
||||
assert_eq!(
|
||||
direct_runtime.sandbox_permissions(&request),
|
||||
SandboxPermissions::RequireEscalated,
|
||||
"direct unified exec should preserve a parent require_escalated request"
|
||||
);
|
||||
assert_eq!(
|
||||
zsh_fork_runtime.sandbox_permissions(&request),
|
||||
SandboxPermissions::RequireEscalated,
|
||||
"zsh-fork unified exec should preserve the same parent require_escalated request"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn zsh_fork_first_attempt_preserves_additional_permissions_request() {
|
||||
let manager = UnifiedExecProcessManager::default();
|
||||
let request = test_request(
|
||||
SandboxPermissions::WithAdditionalPermissions,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
);
|
||||
let zsh_fork_runtime = UnifiedExecRuntime::new(&manager, zsh_fork_mode());
|
||||
|
||||
assert_eq!(
|
||||
zsh_fork_runtime.sandbox_permissions(&request),
|
||||
SandboxPermissions::WithAdditionalPermissions,
|
||||
"zsh-fork unified exec should keep bounded additional-permissions requests sandboxed"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn zsh_fork_execpolicy_allow_preserves_parent_sandbox_override() {
|
||||
let manager = UnifiedExecProcessManager::default();
|
||||
let request = test_request(
|
||||
SandboxPermissions::UseDefault,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
);
|
||||
let runtime = UnifiedExecRuntime::new(&manager, zsh_fork_mode());
|
||||
|
||||
assert_eq!(
|
||||
runtime.exec_approval_requirement(&request),
|
||||
Some(ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}),
|
||||
"zsh-fork unified exec should preserve exec-policy allow decisions that bypass the sandbox"
|
||||
);
|
||||
}
|
||||
|
||||
fn test_request(
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
exec_approval_requirement: ExecApprovalRequirement,
|
||||
) -> UnifiedExecRequest {
|
||||
let cwd = AbsolutePathBuf::try_from(std::env::current_dir().unwrap())
|
||||
.expect("current dir is absolute");
|
||||
UnifiedExecRequest {
|
||||
command: vec!["zsh".to_string(), "-c".to_string(), "echo hi".to_string()],
|
||||
shell_type: ShellType::Zsh,
|
||||
hook_command: "echo hi".to_string(),
|
||||
process_id: 1000,
|
||||
cwd: cwd.clone(),
|
||||
sandbox_cwd: cwd,
|
||||
environment: Arc::new(Environment::default_for_tests()),
|
||||
env: HashMap::new(),
|
||||
exec_server_env_config: None,
|
||||
explicit_env_overrides: HashMap::new(),
|
||||
network: None,
|
||||
tty: false,
|
||||
sandbox_permissions,
|
||||
additional_permissions: None,
|
||||
#[cfg(unix)]
|
||||
additional_permissions_preapproved: false,
|
||||
justification: None,
|
||||
exec_approval_requirement,
|
||||
}
|
||||
}
|
||||
|
||||
fn zsh_fork_mode() -> UnifiedExecShellMode {
|
||||
let cwd = std::env::current_dir().expect("read current dir");
|
||||
UnifiedExecShellMode::ZshFork(ZshForkConfig {
|
||||
shell_zsh_path: AbsolutePathBuf::try_from(cwd.join("zsh")).expect("absolute zsh path"),
|
||||
main_execve_wrapper_exe: AbsolutePathBuf::try_from(cwd.join("execve-wrapper"))
|
||||
.expect("absolute wrapper path"),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -94,6 +94,33 @@ where
|
||||
builder.build(server).await
|
||||
}
|
||||
|
||||
pub async fn build_unified_exec_zsh_fork_test<F>(
|
||||
server: &wiremock::MockServer,
|
||||
runtime: ZshForkRuntime,
|
||||
approval_policy: AskForApproval,
|
||||
permission_profile: PermissionProfile,
|
||||
pre_build_hook: F,
|
||||
) -> Result<TestCodex>
|
||||
where
|
||||
F: FnOnce(&Path) + Send + 'static,
|
||||
{
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(pre_build_hook)
|
||||
.with_config(move |config| {
|
||||
runtime.apply_to_config(config, approval_policy, permission_profile);
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExecZshFork)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
builder.build(server).await
|
||||
}
|
||||
|
||||
fn find_test_zsh_path() -> Result<Option<PathBuf>> {
|
||||
let repo_root = codex_utils_cargo_bin::repo_root()?;
|
||||
let dotslash_zsh = repo_root.join("codex-rs/app-server/tests/suite/zsh");
|
||||
|
||||
@@ -115,6 +115,8 @@ mod tools;
|
||||
mod truncation;
|
||||
mod turn_state;
|
||||
mod unified_exec;
|
||||
#[cfg(unix)]
|
||||
mod unified_exec_zsh_fork_approvals;
|
||||
mod unstable_features_warning;
|
||||
mod user_notification;
|
||||
mod user_shell_cmd;
|
||||
|
||||
@@ -0,0 +1,567 @@
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use codex_config::permissions_toml::FilesystemPermissionToml;
|
||||
use codex_config::permissions_toml::PermissionProfileToml;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_core::sandboxing::SandboxPermissions;
|
||||
use codex_protocol::config_types::CollaborationMode;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
use codex_protocol::config_types::Settings;
|
||||
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_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecApprovalRequestEvent;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::ThreadSettingsOverrides;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses::ResponseMock;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::mount_sse_once;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::turn_permission_fields;
|
||||
use core_test_support::wait_for_event;
|
||||
use core_test_support::wait_for_event_with_timeout;
|
||||
use core_test_support::zsh_fork::build_unified_exec_zsh_fork_test;
|
||||
use core_test_support::zsh_fork::restrictive_workspace_write_profile;
|
||||
use core_test_support::zsh_fork::zsh_fork_runtime;
|
||||
use pretty_assertions::assert_eq;
|
||||
use regex_lite::Regex;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::time::Duration;
|
||||
use toml_edit::Key as TomlKey;
|
||||
use wiremock::MockServer;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn unified_exec_zsh_fork_parent_approval_preserves_denied_reads() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let denied_dir = tempfile::tempdir_in(std::env::current_dir()?)?;
|
||||
let denied_path = denied_dir.path().join("secret.env");
|
||||
let secret = "unified-exec-zsh-fork-denied-read-secret";
|
||||
fs::write(&denied_path, format!("{secret}\n"))?;
|
||||
let permission_profile = denied_read_permission_profile(&denied_path)?;
|
||||
assert!(
|
||||
permission_profile
|
||||
.file_system_sandbox_policy()
|
||||
.has_denied_read_restrictions(),
|
||||
"test must exercise a permission profile with denied reads"
|
||||
);
|
||||
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let command = format!("cat {denied_path:?}");
|
||||
let Some((server, test)) = build_unified_exec_zsh_fork_test_or_skip(
|
||||
"unified-exec zsh-fork denied-read approval test",
|
||||
approval_policy,
|
||||
permission_profile,
|
||||
move |_home| {},
|
||||
)
|
||||
.await?
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let call_id = "uexec-zsh-fork-parent-approval-denied-read";
|
||||
let results = mount_unified_exec_command(
|
||||
&server,
|
||||
"uexec-zsh-fork-denied-read",
|
||||
call_id,
|
||||
&command,
|
||||
"attempt a denied read for the test",
|
||||
)
|
||||
.await?;
|
||||
submit_turn_with_session_permissions(
|
||||
&test,
|
||||
"run approved unified exec denied read through zsh fork",
|
||||
approval_policy,
|
||||
)
|
||||
.await?;
|
||||
approve_expected_exec(&test, &command).await?;
|
||||
wait_for_completion_without_approval(&test).await;
|
||||
|
||||
let result = command_result(&results, call_id);
|
||||
assert_ne!(
|
||||
result.exit_code.unwrap_or(0),
|
||||
0,
|
||||
"denied-read command should stay sandboxed after parent approval"
|
||||
);
|
||||
assert!(
|
||||
!result.stdout.contains(secret),
|
||||
"denied-read command unexpectedly printed the secret: {}",
|
||||
result.stdout
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn unified_exec_zsh_fork_parent_approval_escalates_intercepted_exec() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let permission_profile = restrictive_workspace_write_profile();
|
||||
let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?;
|
||||
let outside_path = outside_dir
|
||||
.path()
|
||||
.join("unified-exec-zsh-fork-parent-approval.txt");
|
||||
let command = format!("printf hi > {outside_path:?}");
|
||||
|
||||
let outside_path_for_hook = outside_path.clone();
|
||||
let Some((server, test)) = build_unified_exec_zsh_fork_test_or_skip(
|
||||
"unified-exec zsh-fork parent approval test",
|
||||
approval_policy,
|
||||
permission_profile,
|
||||
move |_home| {
|
||||
let _ = fs::remove_file(&outside_path_for_hook);
|
||||
},
|
||||
)
|
||||
.await?
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let call_id = "uexec-zsh-fork-parent-approval";
|
||||
let results = mount_unified_exec_command(
|
||||
&server,
|
||||
"uexec-zsh-fork-parent-approval",
|
||||
call_id,
|
||||
&command,
|
||||
"write outside the workspace for the test",
|
||||
)
|
||||
.await?;
|
||||
submit_turn_with_session_permissions(
|
||||
&test,
|
||||
"run approved unified exec through zsh fork",
|
||||
approval_policy,
|
||||
)
|
||||
.await?;
|
||||
approve_expected_exec(&test, &command).await?;
|
||||
wait_for_completion_without_approval(&test).await;
|
||||
|
||||
let result = command_result(&results, call_id);
|
||||
assert_eq!(
|
||||
result.exit_code.unwrap_or(0),
|
||||
0,
|
||||
"approved unified exec zsh-fork command should complete: {}",
|
||||
result.stdout
|
||||
);
|
||||
let contents = fs::read_to_string(&outside_path)
|
||||
.with_context(|| format!("read {}", outside_path.display()))?;
|
||||
assert_eq!(
|
||||
contents, "hi",
|
||||
"approved parent sandbox override should allow zsh-fork shell redirection to write outside the workspace"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn unified_exec_zsh_fork_parent_approval_keeps_explicit_prompt_rule() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let permission_profile = restrictive_workspace_write_profile();
|
||||
let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?;
|
||||
let outside_path = outside_dir
|
||||
.path()
|
||||
.join("unified-exec-zsh-fork-explicit-prompt-rule.txt");
|
||||
let command = format!("touch {outside_path:?}");
|
||||
let rules = r#"prefix_rule(pattern=["touch"], decision="prompt")"#.to_string();
|
||||
|
||||
let outside_path_for_hook = outside_path.clone();
|
||||
let Some((server, test)) = build_unified_exec_zsh_fork_test_or_skip(
|
||||
"unified-exec zsh-fork prompt rule approval test",
|
||||
approval_policy,
|
||||
permission_profile,
|
||||
move |home| {
|
||||
let _ = fs::remove_file(&outside_path_for_hook);
|
||||
let rules_dir = home.join("rules");
|
||||
fs::create_dir_all(&rules_dir).unwrap();
|
||||
fs::write(rules_dir.join("default.rules"), &rules).unwrap();
|
||||
},
|
||||
)
|
||||
.await?
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let call_id = "uexec-zsh-fork-parent-approval-explicit-prompt-rule";
|
||||
let results = mount_unified_exec_command(
|
||||
&server,
|
||||
"uexec-zsh-fork-prompt-rule",
|
||||
call_id,
|
||||
&command,
|
||||
"write outside the workspace for the test",
|
||||
)
|
||||
.await?;
|
||||
submit_turn_with_session_permissions(
|
||||
&test,
|
||||
"run approved unified exec prompt rule through zsh fork",
|
||||
approval_policy,
|
||||
)
|
||||
.await?;
|
||||
approve_expected_exec(&test, &command).await?;
|
||||
|
||||
let approval_event = wait_for_event_with_timeout(
|
||||
&test.codex,
|
||||
|event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
},
|
||||
Duration::from_secs(10),
|
||||
)
|
||||
.await;
|
||||
let EventMsg::ExecApprovalRequest(inner_approval) = approval_event else {
|
||||
panic!("expected explicit prompt rule approval before completion");
|
||||
};
|
||||
assert!(
|
||||
inner_approval
|
||||
.command
|
||||
.iter()
|
||||
.any(|arg| arg.ends_with("/touch"))
|
||||
&& inner_approval
|
||||
.command
|
||||
.iter()
|
||||
.any(|arg| arg == outside_path.to_string_lossy().as_ref()),
|
||||
"expected explicit prompt rule approval for intercepted touch, got: {:?}",
|
||||
inner_approval.command
|
||||
);
|
||||
|
||||
approve_exec(&test, inner_approval.effective_approval_id()).await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
let result = command_result(&results, call_id);
|
||||
assert_eq!(
|
||||
result.exit_code.unwrap_or(0),
|
||||
0,
|
||||
"approved unified exec zsh-fork prompt-rule command should complete: {}",
|
||||
result.stdout
|
||||
);
|
||||
assert!(
|
||||
outside_path.exists(),
|
||||
"approved intercepted touch should create the out-of-workspace file"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
struct CommandResult {
|
||||
exit_code: Option<i64>,
|
||||
stdout: String,
|
||||
}
|
||||
|
||||
async fn build_unified_exec_zsh_fork_test_or_skip<F>(
|
||||
test_name: &str,
|
||||
approval_policy: AskForApproval,
|
||||
permission_profile: PermissionProfile,
|
||||
pre_build_hook: F,
|
||||
) -> Result<Option<(MockServer, TestCodex)>>
|
||||
where
|
||||
F: FnOnce(&Path) + Send + 'static,
|
||||
{
|
||||
let Some(runtime) = zsh_fork_runtime(test_name)? else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let test = build_unified_exec_zsh_fork_test(
|
||||
&server,
|
||||
runtime,
|
||||
approval_policy,
|
||||
permission_profile,
|
||||
pre_build_hook,
|
||||
)
|
||||
.await?;
|
||||
Ok(Some((server, test)))
|
||||
}
|
||||
|
||||
fn denied_read_permission_profile(denied_path: &Path) -> Result<PermissionProfile> {
|
||||
let denied_path_key = TomlKey::new(denied_path.to_string_lossy().into_owned());
|
||||
permission_profile_from_toml(&format!(
|
||||
r#"
|
||||
[filesystem]
|
||||
"/" = "read"
|
||||
":project_roots" = "write"
|
||||
{denied_path_key} = "deny"
|
||||
|
||||
[network]
|
||||
enabled = false
|
||||
"#
|
||||
))
|
||||
}
|
||||
|
||||
fn permission_profile_from_toml(profile: &str) -> Result<PermissionProfile> {
|
||||
let profile = toml::from_str::<PermissionProfileToml>(profile)
|
||||
.context("test permission profile should deserialize")?;
|
||||
let filesystem = profile
|
||||
.filesystem
|
||||
.as_ref()
|
||||
.context("test permission profile should include filesystem entries")?;
|
||||
let entries = filesystem
|
||||
.entries
|
||||
.iter()
|
||||
.map(|(path, permission)| {
|
||||
let FilesystemPermissionToml::Access(access) = permission else {
|
||||
anyhow::bail!("unexpected scoped filesystem permission in test profile: {path}");
|
||||
};
|
||||
let path = match path.as_str() {
|
||||
"/" => FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Root,
|
||||
},
|
||||
":project_roots" => FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::project_roots(/*subpath*/ None),
|
||||
},
|
||||
_ if *access == FileSystemAccessMode::Deny => FileSystemPath::GlobPattern {
|
||||
pattern: path.clone(),
|
||||
},
|
||||
_ => anyhow::bail!("unexpected filesystem entry in test profile: {path}"),
|
||||
};
|
||||
Ok(FileSystemSandboxEntry {
|
||||
path,
|
||||
access: *access,
|
||||
})
|
||||
})
|
||||
.collect::<Result<Vec<_>>>()?;
|
||||
let mut file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(entries);
|
||||
file_system_sandbox_policy.glob_scan_max_depth = filesystem.glob_scan_max_depth;
|
||||
let network_sandbox_policy = match profile.network.as_ref().and_then(|network| network.enabled)
|
||||
{
|
||||
Some(true) => NetworkSandboxPolicy::Enabled,
|
||||
Some(false) | None => NetworkSandboxPolicy::Restricted,
|
||||
};
|
||||
|
||||
Ok(PermissionProfile::from_runtime_permissions(
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
))
|
||||
}
|
||||
|
||||
async fn mount_unified_exec_command(
|
||||
server: &MockServer,
|
||||
response_prefix: &str,
|
||||
call_id: &str,
|
||||
command: &str,
|
||||
justification: &str,
|
||||
) -> Result<ResponseMock> {
|
||||
let first_response_id = format!("resp-{response_prefix}-1");
|
||||
let second_response_id = format!("resp-{response_prefix}-2");
|
||||
let message_id = format!("msg-{response_prefix}-1");
|
||||
let event = exec_command_event(
|
||||
call_id,
|
||||
command,
|
||||
Some(30_000),
|
||||
SandboxPermissions::RequireEscalated,
|
||||
justification,
|
||||
)?;
|
||||
let _ = mount_sse_once(
|
||||
server,
|
||||
sse(vec![
|
||||
ev_response_created(&first_response_id),
|
||||
event,
|
||||
ev_completed(&first_response_id),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results = mount_sse_once(
|
||||
server,
|
||||
sse(vec![
|
||||
ev_assistant_message(&message_id, "done"),
|
||||
ev_completed(&second_response_id),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
Ok(results)
|
||||
}
|
||||
|
||||
async fn submit_turn_with_session_permissions(
|
||||
test: &TestCodex,
|
||||
prompt: &str,
|
||||
approval_policy: AskForApproval,
|
||||
) -> Result<()> {
|
||||
let session_model = test.session_configured.model.clone();
|
||||
let (sandbox_policy, permission_profile) = turn_permission_fields(
|
||||
test.session_configured.permission_profile.clone(),
|
||||
test.cwd.path(),
|
||||
);
|
||||
test.codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: prompt.into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
environments: None,
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: ThreadSettingsOverrides {
|
||||
cwd: Some(test.config.cwd.clone()),
|
||||
approval_policy: Some(approval_policy),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
permission_profile,
|
||||
collaboration_mode: Some(CollaborationMode {
|
||||
mode: ModeKind::Default,
|
||||
settings: Settings {
|
||||
model: session_model,
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn approve_expected_exec(test: &TestCodex, expected_command: &str) -> Result<()> {
|
||||
let approval = expect_exec_approval(test, expected_command).await;
|
||||
approve_exec(test, approval.effective_approval_id()).await
|
||||
}
|
||||
|
||||
async fn approve_exec(test: &TestCodex, approval_id: String) -> Result<()> {
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval_id,
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn command_result(results: &ResponseMock, call_id: &str) -> CommandResult {
|
||||
parse_result(&results.single_request().function_call_output(call_id))
|
||||
}
|
||||
|
||||
fn exec_command_event(
|
||||
call_id: &str,
|
||||
cmd: &str,
|
||||
yield_time_ms: Option<u64>,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
justification: &str,
|
||||
) -> Result<Value> {
|
||||
let mut args = json!({
|
||||
"cmd": cmd.to_string(),
|
||||
});
|
||||
if let Some(yield_time_ms) = yield_time_ms {
|
||||
args["yield_time_ms"] = json!(yield_time_ms);
|
||||
}
|
||||
if sandbox_permissions.requests_sandbox_override() {
|
||||
args["sandbox_permissions"] = json!(sandbox_permissions);
|
||||
args["justification"] = json!(justification);
|
||||
}
|
||||
let args_str = serde_json::to_string(&args)?;
|
||||
Ok(ev_function_call(call_id, "exec_command", &args_str))
|
||||
}
|
||||
|
||||
fn parse_result(item: &Value) -> CommandResult {
|
||||
let Some(output_str) = item.get("output").and_then(Value::as_str) else {
|
||||
return CommandResult {
|
||||
exit_code: None,
|
||||
stdout: String::new(),
|
||||
};
|
||||
};
|
||||
match serde_json::from_str::<Value>(output_str) {
|
||||
Ok(parsed) => {
|
||||
let exit_code = parsed["metadata"]["exit_code"].as_i64();
|
||||
let stdout = parsed["output"].as_str().unwrap_or_default().to_string();
|
||||
CommandResult { exit_code, stdout }
|
||||
}
|
||||
Err(_) => parsed_regex_result(r"(?s)^Exit code:\s*(-?\d+).*?Output:\n(.*)$", output_str)
|
||||
.or_else(|| {
|
||||
parsed_regex_result(
|
||||
r"(?s)^.*?Process exited with code (\d+)\n.*?Output:\n(.*)$",
|
||||
output_str,
|
||||
)
|
||||
})
|
||||
.unwrap_or_else(|| CommandResult {
|
||||
exit_code: None,
|
||||
stdout: output_str.to_string(),
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
fn parsed_regex_result(pattern: &str, output_str: &str) -> Option<CommandResult> {
|
||||
let regex = Regex::new(pattern).ok()?;
|
||||
let captures = regex.captures(output_str)?;
|
||||
let exit_code = captures.get(1)?.as_str().parse::<i64>().ok()?;
|
||||
let output = captures.get(2)?.as_str();
|
||||
Some(CommandResult {
|
||||
exit_code: Some(exit_code),
|
||||
stdout: output.to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
async fn expect_exec_approval(
|
||||
test: &TestCodex,
|
||||
expected_command: &str,
|
||||
) -> ExecApprovalRequestEvent {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
|
||||
match event {
|
||||
EventMsg::ExecApprovalRequest(approval) => {
|
||||
let last_arg = approval
|
||||
.command
|
||||
.last()
|
||||
.map(std::string::String::as_str)
|
||||
.unwrap_or_default();
|
||||
assert_eq!(
|
||||
last_arg, expected_command,
|
||||
"approval request should be for the parent unified-exec command"
|
||||
);
|
||||
approval
|
||||
}
|
||||
EventMsg::TurnComplete(_) => panic!("expected approval request before completion"),
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
async fn wait_for_completion_without_approval(test: &TestCodex) {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
|
||||
match event {
|
||||
EventMsg::TurnComplete(_) => {}
|
||||
EventMsg::ExecApprovalRequest(event) => {
|
||||
panic!("unexpected approval request: {:?}", event.command)
|
||||
}
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
async fn wait_for_completion(test: &TestCodex) {
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
}
|
||||
Reference in New Issue
Block a user