mirror of
https://github.com/openai/codex.git
synced 2026-09-05 15:18:41 +00:00
fix: honor parent approvals for intercepted execs
This commit is contained in:
@@ -17,6 +17,7 @@ use crate::shell::ShellType;
|
||||
use crate::tools::runtimes::build_sandbox_command;
|
||||
use crate::tools::runtimes::exec_env_for_sandbox_permissions;
|
||||
use crate::tools::runtimes::prepend_zsh_fork_bin_to_path;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::PermissionRequestPayload;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
@@ -100,6 +101,19 @@ fn approval_sandbox_permissions(
|
||||
}
|
||||
}
|
||||
|
||||
fn parent_approved_sandbox_override(
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions_preapproved: bool,
|
||||
exec_approval_requirement: &ExecApprovalRequirement,
|
||||
) -> bool {
|
||||
sandbox_permissions.requests_sandbox_override()
|
||||
&& (additional_permissions_preapproved
|
||||
|| matches!(
|
||||
exec_approval_requirement,
|
||||
ExecApprovalRequirement::NeedsApproval { .. }
|
||||
))
|
||||
}
|
||||
|
||||
pub(super) async fn try_run_zsh_fork(
|
||||
req: &ShellRequest,
|
||||
attempt: &SandboxAttempt<'_>,
|
||||
@@ -228,6 +242,11 @@ pub(super) async fn try_run_zsh_fork(
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
approval_sandbox_permissions,
|
||||
prompt_permissions: req.additional_permissions.clone(),
|
||||
parent_sandbox_override_approved: parent_approved_sandbox_override(
|
||||
req.sandbox_permissions,
|
||||
req.additional_permissions_preapproved,
|
||||
&req.exec_approval_requirement,
|
||||
),
|
||||
stopwatch: stopwatch.clone(),
|
||||
};
|
||||
|
||||
@@ -304,6 +323,11 @@ pub(crate) async fn prepare_unified_exec_zsh_fork(
|
||||
req.additional_permissions_preapproved,
|
||||
),
|
||||
prompt_permissions: req.additional_permissions.clone(),
|
||||
parent_sandbox_override_approved: parent_approved_sandbox_override(
|
||||
req.sandbox_permissions,
|
||||
req.additional_permissions_preapproved,
|
||||
&req.exec_approval_requirement,
|
||||
),
|
||||
stopwatch: Stopwatch::unlimited(),
|
||||
};
|
||||
|
||||
@@ -336,6 +360,7 @@ struct CoreShellActionProvider {
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
approval_sandbox_permissions: SandboxPermissions,
|
||||
prompt_permissions: Option<AdditionalPermissionProfile>,
|
||||
parent_sandbox_override_approved: bool,
|
||||
stopwatch: Stopwatch,
|
||||
}
|
||||
|
||||
@@ -637,6 +662,17 @@ impl EscalationPolicy for CoreShellActionProvider {
|
||||
let needs_unsandboxed_escalation = unsandboxed_allowed
|
||||
&& (self.sandbox_permissions.requires_escalated_permissions()
|
||||
|| decision_driven_by_policy);
|
||||
// The parent shell/unified-exec approval already covered unmatched
|
||||
// prompts caused by its sandbox override. Explicit exec-policy rules
|
||||
// still keep their own decisions.
|
||||
let decision = if self.parent_sandbox_override_approved
|
||||
&& evaluation.decision == Decision::Prompt
|
||||
&& !decision_driven_by_policy
|
||||
{
|
||||
Decision::Allow
|
||||
} else {
|
||||
evaluation.decision
|
||||
};
|
||||
let needs_escalation =
|
||||
self.sandbox_permissions.uses_additional_permissions() || needs_unsandboxed_escalation;
|
||||
|
||||
@@ -656,7 +692,7 @@ impl EscalationPolicy for CoreShellActionProvider {
|
||||
),
|
||||
};
|
||||
self.process_decision(
|
||||
evaluation.decision,
|
||||
decision,
|
||||
needs_escalation,
|
||||
program,
|
||||
argv,
|
||||
|
||||
@@ -153,6 +153,42 @@ fn approval_sandbox_permissions_only_downgrades_preapproved_additional_permissio
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parent_approved_sandbox_override_tracks_parent_approval_sources() {
|
||||
assert!(super::parent_approved_sandbox_override(
|
||||
SandboxPermissions::RequireEscalated,
|
||||
/*additional_permissions_preapproved*/ false,
|
||||
&crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
));
|
||||
assert!(super::parent_approved_sandbox_override(
|
||||
SandboxPermissions::WithAdditionalPermissions,
|
||||
/*additional_permissions_preapproved*/ true,
|
||||
&crate::tools::sandboxing::ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
));
|
||||
assert!(!super::parent_approved_sandbox_override(
|
||||
SandboxPermissions::UseDefault,
|
||||
/*additional_permissions_preapproved*/ false,
|
||||
&crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
));
|
||||
assert!(!super::parent_approved_sandbox_override(
|
||||
SandboxPermissions::RequireEscalated,
|
||||
/*additional_permissions_preapproved*/ false,
|
||||
&crate::tools::sandboxing::ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_shell_script_preserves_login_flag() {
|
||||
assert_eq!(
|
||||
@@ -428,6 +464,7 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho
|
||||
sandbox_permissions: SandboxPermissions::WithAdditionalPermissions,
|
||||
approval_sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prompt_permissions: Some(requested_permissions),
|
||||
parent_sandbox_override_approved: true,
|
||||
stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)),
|
||||
};
|
||||
|
||||
@@ -449,6 +486,93 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn parent_require_escalated_approval_escalates_intercepted_exec() -> anyhow::Result<()> {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let workdir = test_sandbox_cwd();
|
||||
let provider = CoreShellActionProvider {
|
||||
policy: Arc::new(RwLock::new(codex_execpolicy::Policy::empty())),
|
||||
session: Arc::new(session),
|
||||
turn: Arc::new(turn_context),
|
||||
call_id: "parent-require-escalated".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
sandbox_policy_cwd: workdir.clone(),
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
approval_sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prompt_permissions: None,
|
||||
parent_sandbox_override_approved: true,
|
||||
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/curl")?,
|
||||
&["curl".to_string(), "example.com".to_string()],
|
||||
&workdir,
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
action,
|
||||
codex_shell_escalation::EscalationDecision::Escalate(EscalationExecution::Unsandboxed)
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn parent_approval_does_not_override_intercepted_exec_policy_prompt() -> anyhow::Result<()> {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let mut parser = PolicyParser::new();
|
||||
parser.parse(
|
||||
"test.rules",
|
||||
r#"prefix_rule(pattern = ["curl"], decision = "prompt")"#,
|
||||
)?;
|
||||
let workdir = test_sandbox_cwd();
|
||||
let provider = CoreShellActionProvider {
|
||||
policy: Arc::new(RwLock::new(parser.build())),
|
||||
session: Arc::new(session),
|
||||
turn: Arc::new(turn_context),
|
||||
call_id: "parent-policy-prompt".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::Granular(GranularApprovalConfig {
|
||||
sandbox_approval: true,
|
||||
rules: false,
|
||||
skill_approval: true,
|
||||
request_permissions: true,
|
||||
mcp_elicitations: true,
|
||||
}),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
sandbox_policy_cwd: workdir.clone(),
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
approval_sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prompt_permissions: None,
|
||||
parent_sandbox_override_approved: true,
|
||||
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/curl")?,
|
||||
&["curl".to_string(), "example.com".to_string()],
|
||||
&workdir,
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
action,
|
||||
codex_shell_escalation::EscalationDecision::Deny {
|
||||
reason: Some("Execution forbidden by policy".to_string()),
|
||||
}
|
||||
);
|
||||
|
||||
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;
|
||||
@@ -561,6 +685,7 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
approval_sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prompt_permissions: None,
|
||||
parent_sandbox_override_approved: false,
|
||||
stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)),
|
||||
};
|
||||
|
||||
@@ -775,6 +900,7 @@ prefix_rule(pattern = ["{cat_path_literal}"], decision = "allow")
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
approval_sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prompt_permissions: None,
|
||||
parent_sandbox_override_approved: false,
|
||||
stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)),
|
||||
};
|
||||
|
||||
@@ -818,6 +944,7 @@ async fn denied_reads_keep_granular_sandbox_rejection_for_escalation() -> anyhow
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
approval_sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prompt_permissions: None,
|
||||
parent_sandbox_override_approved: false,
|
||||
stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)),
|
||||
};
|
||||
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -43,6 +43,7 @@ use core_test_support::test_codex::test_codex;
|
||||
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::build_zsh_fork_test;
|
||||
use core_test_support::zsh_fork::restrictive_workspace_write_profile;
|
||||
use core_test_support::zsh_fork::zsh_fork_runtime;
|
||||
@@ -2800,6 +2801,122 @@ async fn matched_prefix_rule_runs_unsandboxed_under_zsh_fork() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[cfg(unix)]
|
||||
async fn unified_exec_zsh_fork_parent_approval_escalates_intercepted_exec() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let Some(runtime) = zsh_fork_runtime("unified-exec zsh-fork parent approval test")? else {
|
||||
return 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!("touch {outside_path:?}");
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let outside_path_for_hook = outside_path.clone();
|
||||
let test = build_unified_exec_zsh_fork_test(
|
||||
&server,
|
||||
runtime,
|
||||
approval_policy,
|
||||
permission_profile,
|
||||
move |_home| {
|
||||
let _ = fs::remove_file(&outside_path_for_hook);
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
let call_id = "uexec-zsh-fork-parent-approval";
|
||||
let event = exec_command_event(
|
||||
call_id,
|
||||
&command,
|
||||
Some(30_000),
|
||||
SandboxPermissions::RequireEscalated,
|
||||
Some("write outside the workspace for the test"),
|
||||
)?;
|
||||
let _ = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-uexec-zsh-fork-parent-approval-1"),
|
||||
event,
|
||||
ev_completed("resp-uexec-zsh-fork-parent-approval-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-uexec-zsh-fork-parent-approval-1", "done"),
|
||||
ev_completed("resp-uexec-zsh-fork-parent-approval-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
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: "run approved unified exec through zsh fork".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
environments: None,
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: codex_protocol::protocol::ThreadSettingsOverrides {
|
||||
cwd: Some(test.cwd.path().to_path_buf()),
|
||||
approval_policy: Some(approval_policy),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
permission_profile,
|
||||
collaboration_mode: Some(codex_protocol::config_types::CollaborationMode {
|
||||
mode: codex_protocol::config_types::ModeKind::Default,
|
||||
settings: codex_protocol::config_types::Settings {
|
||||
model: session_model,
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let approval = expect_exec_approval(&test, &command).await;
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion_without_approval(&test).await;
|
||||
|
||||
let result = parse_result(&results.single_request().function_call_output(call_id));
|
||||
assert_eq!(
|
||||
result.exit_code.unwrap_or(0),
|
||||
0,
|
||||
"approved unified exec zsh-fork command should complete: {}",
|
||||
result.stdout
|
||||
);
|
||||
assert!(
|
||||
outside_path.exists(),
|
||||
"expected parent approval to let the intercepted touch run unsandboxed; output: {}",
|
||||
result.stdout
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(unix)]
|
||||
async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Result<()> {
|
||||
|
||||
Reference in New Issue
Block a user