mirror of
https://github.com/openai/codex.git
synced 2026-10-21 17:48:44 +00:00
Harden model-selected shell approval boundaries
This commit is contained in:
@@ -1045,7 +1045,7 @@ fn scenarios() -> Vec<ScenarioSpec> {
|
||||
outcome: Outcome::ExecApprovalWithAmendment {
|
||||
decision: ReviewDecision::Denied,
|
||||
expected_reason: None,
|
||||
expected_execpolicy_amendment: Some(&["echo", "known-safe-escalation"]),
|
||||
expected_execpolicy_amendment: None,
|
||||
},
|
||||
expectation: Expectation::CommandFailure {
|
||||
output_contains: "rejected by user",
|
||||
@@ -1792,6 +1792,164 @@ fn scenarios() -> Vec<ScenarioSpec> {
|
||||
]
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test_case("./ls", "ls" ; "direct_relative_executable")]
|
||||
#[test_case("./zsh -c ls", "zsh" ; "configured_shell_with_relative_nested_shell")]
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn fake_shell_sandbox_override_prompts_before_execution(
|
||||
authored_command: &str,
|
||||
executable_name: &str,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
};
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
let configured_shell =
|
||||
codex_core::shell::get_shell_by_model_provided_path(&PathBuf::from("/bin/sh"));
|
||||
let expected_command =
|
||||
configured_shell.derive_exec_args(authored_command, /*use_login_shell*/ false);
|
||||
let mut builder = test_codex()
|
||||
.with_user_shell(configured_shell)
|
||||
.with_config(move |config| {
|
||||
config.permissions.allow_login_shell = false;
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config
|
||||
.set_legacy_sandbox_policy(sandbox_policy_for_config)
|
||||
.expect("set workspace-write sandbox policy");
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let fake_executable = test.cwd.path().join(executable_name);
|
||||
let sentinel = test.cwd.path().join(format!("{executable_name}.started"));
|
||||
fs::write(
|
||||
&fake_executable,
|
||||
"#!/bin/sh\nprintf '%s\\n' started > \"$0.started\"\n",
|
||||
)?;
|
||||
let mut permissions = fs::metadata(&fake_executable)?.permissions();
|
||||
permissions.set_mode(0o755);
|
||||
fs::set_permissions(&fake_executable, permissions)?;
|
||||
assert!(!sentinel.exists(), "sentinel must start absent");
|
||||
|
||||
let call_id = format!("fake-shell-sandbox-override-{executable_name}");
|
||||
let event = shell_event(
|
||||
&call_id,
|
||||
authored_command,
|
||||
/*timeout_ms*/ 1_000,
|
||||
SandboxPermissions::RequireEscalated,
|
||||
)?;
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-fake-shell-1"),
|
||||
event,
|
||||
ev_completed("resp-fake-shell-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-fake-shell-1", "done"),
|
||||
ev_completed("resp-fake-shell-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"run the fake shell with an explicit sandbox override",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
)
|
||||
.await?;
|
||||
|
||||
// Shell-command begin/end events describe the logical tool call, not the
|
||||
// child-process lifetime: the begin event intentionally precedes policy
|
||||
// evaluation. The executable sentinel is the authority boundary here.
|
||||
let approval = loop {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_)
|
||||
| EventMsg::ExecCommandBegin(_)
|
||||
| EventMsg::ExecCommandEnd(_)
|
||||
| EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
assert!(
|
||||
!sentinel.exists(),
|
||||
"fake executable must not run before approval"
|
||||
);
|
||||
match event {
|
||||
EventMsg::ExecApprovalRequest(approval) => break approval,
|
||||
EventMsg::ExecCommandBegin(_) => continue,
|
||||
EventMsg::ExecCommandEnd(end) => {
|
||||
panic!("logical shell call ended before approval: {end:?}")
|
||||
}
|
||||
EventMsg::TurnComplete(_) => panic!("expected approval before turn completion"),
|
||||
_ => unreachable!(),
|
||||
}
|
||||
};
|
||||
assert_eq!(approval.call_id, call_id);
|
||||
assert_eq!(approval.command, expected_command);
|
||||
assert_eq!(approval.proposed_execpolicy_amendment, None);
|
||||
assert!(
|
||||
!sentinel.exists(),
|
||||
"fake executable must remain stopped while approval is pending"
|
||||
);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
.await?;
|
||||
|
||||
loop {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecCommandEnd(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
assert!(
|
||||
!sentinel.exists(),
|
||||
"denying approval must not run the fake executable"
|
||||
);
|
||||
if matches!(event, EventMsg::TurnComplete(_)) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
!sentinel.exists(),
|
||||
"denying approval must not run the fake executable"
|
||||
);
|
||||
|
||||
let output = parse_result(
|
||||
&results_mock
|
||||
.single_request()
|
||||
.function_call_output(call_id.as_str()),
|
||||
);
|
||||
assert!(
|
||||
output.stdout.contains("rejected by user"),
|
||||
"tool result should report the normal user rejection: {}",
|
||||
output.stdout
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(ScenarioGroup::DangerFullAccess ; "danger_full_access")]
|
||||
#[test_case(ScenarioGroup::ReadOnly ; "read_only")]
|
||||
#[test_case(ScenarioGroup::WorkspaceWrite ; "workspace_write")]
|
||||
@@ -3211,7 +3369,7 @@ exec {remote_bash_exec} "$@"
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(unix)]
|
||||
async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Result<()> {
|
||||
async fn permission_expansion_does_not_offer_fallback_rule_for_compound_command() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
@@ -3230,7 +3388,7 @@ async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Resu
|
||||
let event = shell_event_with_prefix_rule(
|
||||
call_id,
|
||||
command,
|
||||
/*timeout_ms*/ 1_000,
|
||||
/*timeout_ms*/ 10_000,
|
||||
SandboxPermissions::RequireEscalated,
|
||||
Some(vec!["touch".to_string()]),
|
||||
)?;
|
||||
@@ -3244,6 +3402,14 @@ async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Resu
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let _results = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-invalid-prefix-no-amendment", "done"),
|
||||
ev_completed("resp-invalid-prefix-no-amendment-results"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
@@ -3254,17 +3420,22 @@ async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Resu
|
||||
.await?;
|
||||
|
||||
let approval = expect_exec_approval(&test, command).await;
|
||||
let amendment = approval
|
||||
.proposed_execpolicy_amendment
|
||||
.expect("should have a proposed execpolicy amendment");
|
||||
assert!(amendment.command.contains(&command.to_string()));
|
||||
assert_eq!(approval.proposed_execpolicy_amendment, None);
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(unix)]
|
||||
async fn approving_fallback_rule_for_compound_command_works() -> Result<()> {
|
||||
async fn approving_permission_expansion_does_not_persist_compound_command() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
@@ -3283,7 +3454,7 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> {
|
||||
let event = shell_event_with_prefix_rule(
|
||||
call_id,
|
||||
command,
|
||||
/*timeout_ms*/ 1_000,
|
||||
/*timeout_ms*/ 10_000,
|
||||
SandboxPermissions::RequireEscalated,
|
||||
Some(vec!["touch".to_string()]),
|
||||
)?;
|
||||
@@ -3297,6 +3468,14 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> {
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let first_results = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-invalid-prefix-first", "done"),
|
||||
ev_completed("resp-invalid-prefix-first-results"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
@@ -3308,29 +3487,27 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> {
|
||||
|
||||
let approval = expect_exec_approval(&test, command).await;
|
||||
let approval_id = approval.effective_approval_id();
|
||||
let amendment = approval
|
||||
.proposed_execpolicy_amendment
|
||||
.expect("should have a proposed execpolicy amendment");
|
||||
assert!(amendment.command.contains(&command.to_string()));
|
||||
assert_eq!(approval.proposed_execpolicy_amendment, None);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval_id,
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: amendment.clone(),
|
||||
},
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
let first_output = parse_result(&first_results.single_request().function_call_output(call_id));
|
||||
assert_eq!(first_output.exit_code.unwrap_or(0), 0);
|
||||
|
||||
let call_id = "invalid-prefix-rule-again";
|
||||
let command =
|
||||
"touch /tmp/codex-fallback-rule-test.txt && echo hello > /tmp/codex-fallback-rule-test.txt";
|
||||
let event = shell_event_with_prefix_rule(
|
||||
call_id,
|
||||
command,
|
||||
/*timeout_ms*/ 1_000,
|
||||
/*timeout_ms*/ 10_000,
|
||||
SandboxPermissions::RequireEscalated,
|
||||
Some(vec!["touch".to_string()]),
|
||||
)?;
|
||||
@@ -3361,16 +3538,24 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> {
|
||||
)
|
||||
.await?;
|
||||
|
||||
wait_for_completion_without_approval(&test).await;
|
||||
let approval = expect_exec_approval(&test, command).await;
|
||||
assert_eq!(approval.proposed_execpolicy_amendment, None);
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Denied,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
let second_output = parse_result(
|
||||
&second_results
|
||||
.single_request()
|
||||
.function_call_output(call_id),
|
||||
);
|
||||
assert_eq!(second_output.exit_code.unwrap_or(0), 0);
|
||||
assert!(
|
||||
second_output.stdout.is_empty(),
|
||||
second_output.stdout.contains("rejected by user"),
|
||||
"unexpected stdout: {}",
|
||||
second_output.stdout
|
||||
);
|
||||
|
||||
@@ -8,12 +8,12 @@ use codex_protocol::config_types::Settings;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
#[cfg(windows)]
|
||||
#[cfg(any(unix, windows))]
|
||||
use codex_protocol::protocol::ExecApprovalPurpose;
|
||||
use codex_protocol::protocol::Op;
|
||||
#[cfg(windows)]
|
||||
#[cfg(any(unix, windows))]
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
#[cfg(windows)]
|
||||
#[cfg(any(unix, windows))]
|
||||
use codex_protocol::protocol::TurnAbortReason;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
@@ -30,6 +30,8 @@ use core_test_support::wait_for_event;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::fs;
|
||||
#[cfg(unix)]
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
#[cfg(windows)]
|
||||
use std::path::PathBuf;
|
||||
|
||||
@@ -103,7 +105,6 @@ fn installed_windows_powershell() -> PathBuf {
|
||||
.into_path_buf()
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn enable_unified_exec(config: &mut codex_core::config::Config) {
|
||||
config
|
||||
.features
|
||||
@@ -111,6 +112,164 @@ fn enable_unified_exec(config: &mut codex_core::config::Config) {
|
||||
.expect("test config should allow feature update");
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn unified_exec_model_selected_posix_shell_requires_one_shot_approval_before_execution()
|
||||
-> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let mut builder = test_codex().with_config(enable_unified_exec);
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let model_shell_dir = test.config.cwd.join("model-shell");
|
||||
fs::create_dir_all(&model_shell_dir)?;
|
||||
let model_shell = model_shell_dir.join("sh");
|
||||
let sentinel = test.config.cwd.join("model-shell-started.txt");
|
||||
let sentinel_arg = sentinel
|
||||
.to_str()
|
||||
.expect("the sentinel path must be valid UTF-8")
|
||||
.replace('\'', "'\"'\"'");
|
||||
fs::write(
|
||||
&model_shell,
|
||||
format!("#!/bin/sh\nprintf started > '{sentinel_arg}'\nexec /bin/sh \"$@\"\n"),
|
||||
)?;
|
||||
fs::set_permissions(&model_shell, fs::Permissions::from_mode(0o755))?;
|
||||
let requested_shell = "./model-shell/sh";
|
||||
let resolved_model_shell = model_shell
|
||||
.to_str()
|
||||
.expect("the model-selected shell path must be valid UTF-8")
|
||||
.to_string();
|
||||
|
||||
let expected_command = vec![resolved_model_shell, "-c".to_string(), "ls".to_string()];
|
||||
let call_id = "unified-exec-model-selected-posix-one-shot";
|
||||
let args = json!({
|
||||
"shell": requested_shell,
|
||||
"cmd": "ls",
|
||||
"login": false,
|
||||
"yield_time_ms": 1_000,
|
||||
});
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-model-selected-posix-1"),
|
||||
ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?),
|
||||
ev_completed("resp-model-selected-posix-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-model-selected-posix-1", "done"),
|
||||
ev_completed("resp-model-selected-posix-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_user_turn(
|
||||
&test,
|
||||
"run a read-only command with a model-selected POSIX shell",
|
||||
AskForApproval::UnlessTrusted,
|
||||
PermissionProfile::Disabled,
|
||||
/*collaboration_mode*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_)
|
||||
| EventMsg::ExecCommandBegin(_)
|
||||
| EventMsg::ExecCommandEnd(_)
|
||||
| EventMsg::TurnAborted(_)
|
||||
| EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
assert!(
|
||||
!sentinel.exists(),
|
||||
"the model-selected POSIX shell must not start before approval"
|
||||
);
|
||||
let approval = match event {
|
||||
EventMsg::ExecApprovalRequest(approval) => approval,
|
||||
EventMsg::ExecCommandBegin(begin) => {
|
||||
panic!("model-selected POSIX shell began before approval: {begin:?}")
|
||||
}
|
||||
EventMsg::ExecCommandEnd(end) => {
|
||||
panic!("model-selected POSIX shell completed before approval: {end:?}")
|
||||
}
|
||||
EventMsg::TurnAborted(aborted) => {
|
||||
panic!("turn aborted before one-shot approval: {aborted:?}")
|
||||
}
|
||||
EventMsg::TurnComplete(_) => panic!("expected one-shot approval before completion"),
|
||||
_ => unreachable!(),
|
||||
};
|
||||
assert_eq!(approval.call_id, call_id);
|
||||
assert_eq!(approval.command, expected_command);
|
||||
assert_eq!(approval.cwd, test.config.cwd);
|
||||
let approval_id = approval
|
||||
.approval_id
|
||||
.clone()
|
||||
.expect("one-shot approval must carry a callback ID");
|
||||
assert_ne!(approval_id, call_id);
|
||||
assert_eq!(
|
||||
approval.approval_purpose,
|
||||
Some(ExecApprovalPurpose::Initial)
|
||||
);
|
||||
assert_eq!(
|
||||
approval.effective_approval_purpose(),
|
||||
ExecApprovalPurpose::Initial
|
||||
);
|
||||
assert_eq!(
|
||||
approval.effective_available_decisions(),
|
||||
vec![ReviewDecision::Approved, ReviewDecision::Abort]
|
||||
);
|
||||
assert_eq!(approval.proposed_execpolicy_amendment, None);
|
||||
assert!(!sentinel.exists());
|
||||
|
||||
let approval_turn_id = approval.turn_id.clone();
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval_id,
|
||||
turn_id: Some(approval_turn_id.clone()),
|
||||
decision: ReviewDecision::Abort,
|
||||
})
|
||||
.await?;
|
||||
|
||||
let aborted = match wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecCommandBegin(_)
|
||||
| EventMsg::ExecCommandEnd(_)
|
||||
| EventMsg::TurnAborted(_)
|
||||
| EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await
|
||||
{
|
||||
EventMsg::TurnAborted(aborted) => aborted,
|
||||
EventMsg::ExecCommandBegin(begin) => {
|
||||
panic!("model-selected POSIX shell began after abort: {begin:?}")
|
||||
}
|
||||
EventMsg::ExecCommandEnd(end) => {
|
||||
panic!("model-selected POSIX shell completed after abort: {end:?}")
|
||||
}
|
||||
EventMsg::TurnComplete(_) => panic!("aborted approval unexpectedly completed the turn"),
|
||||
_ => unreachable!(),
|
||||
};
|
||||
assert_eq!(aborted.turn_id.as_deref(), Some(approval_turn_id.as_str()));
|
||||
assert_eq!(aborted.reason, TurnAbortReason::Interrupted);
|
||||
assert!(
|
||||
!sentinel.exists(),
|
||||
"aborting one-shot approval must never start the model-selected POSIX shell"
|
||||
);
|
||||
assert!(
|
||||
results_mock.requests().is_empty(),
|
||||
"aborting one-shot approval must not continue the turn"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn unified_exec_workspace_powershell_path_requires_one_shot_approval_before_execution()
|
||||
|
||||
@@ -248,32 +248,27 @@ async fn remote_test_env_exposes_target_shell_to_model() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn explicit_remote_shell_runs_in_remote_cwd() -> Result<()> {
|
||||
async fn compatible_remote_path_hint_uses_environment_shell_in_remote_cwd() -> Result<()> {
|
||||
const CALL_ID: &str = "remote-explicit-shell";
|
||||
|
||||
skip_if_no_remote_env!(Ok(()));
|
||||
|
||||
let (shell, command) = match test_target_os() {
|
||||
TestTargetOs::Linux => (
|
||||
"bash",
|
||||
r#"case "$PWD" in /tmp/codex-core-test-cwd-*) ;; *) echo "unexpected cwd: $PWD" >&2; exit 1 ;; esac"#,
|
||||
),
|
||||
TestTargetOs::Windows => (
|
||||
"powershell",
|
||||
r#"$cwd = (Get-Location).Path; if ($cwd -notlike 'C:\codex-core-test-cwd-*') { Write-Error "unexpected cwd: $cwd"; exit 1 }"#,
|
||||
),
|
||||
let (shell, expected_cwd_prefix) = match test_target_os() {
|
||||
TestTargetOs::Linux => ("/attacker/bash", "/tmp/codex-core-test-cwd-"),
|
||||
TestTargetOs::Windows => (r"C:\attacker\PowerShell.ExE", r"C:\codex-core-test-cwd-"),
|
||||
TestTargetOs::MacOs => unreachable!("remote test targets do not run macOS"),
|
||||
};
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let arguments = serde_json::to_string(&json!({
|
||||
"cmd": command,
|
||||
"cmd": "pwd",
|
||||
"shell": shell,
|
||||
"login": false,
|
||||
"yield_time_ms": 10_000,
|
||||
}))?;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted);
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
@@ -312,9 +307,14 @@ async fn explicit_remote_shell_runs_in_remote_cwd() -> Result<()> {
|
||||
.function_call_output_content_and_success(CALL_ID)
|
||||
.context("remote shell tool result should be present")?;
|
||||
assert_ne!(success, Some(false));
|
||||
let output = output.context("remote shell command should return output")?;
|
||||
assert!(
|
||||
output.is_some_and(|output| output.contains("Process exited with code 0")),
|
||||
"remote shell command should exit successfully",
|
||||
output.contains("Process exited with code 0"),
|
||||
"remote shell command should exit successfully: {output}",
|
||||
);
|
||||
assert!(
|
||||
output.contains(expected_cwd_prefix),
|
||||
"remote shell command should run in the remote cwd: {output}",
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
@@ -2024,6 +2024,162 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn model_shell_resolution_failure_precedes_process_allocation_and_events() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
let invalid_call_id = "uexec-missing-model-shell";
|
||||
let invalid_args = serde_json::json!({
|
||||
"shell": "./missing/sh",
|
||||
"cmd": "true",
|
||||
"login": false,
|
||||
});
|
||||
let start_call_id = "uexec-after-missing-model-shell";
|
||||
let start_args = serde_json::json!({
|
||||
"cmd": "tail -f /dev/null",
|
||||
"yield_time_ms": 10,
|
||||
"tty": false,
|
||||
});
|
||||
let interrupt_call_id = "uexec-after-missing-model-shell-interrupt";
|
||||
let interrupt_args = serde_json::json!({
|
||||
"chars": "\u{3}",
|
||||
"session_id": 1000,
|
||||
"yield_time_ms": 1000,
|
||||
});
|
||||
|
||||
let responses = vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-missing-shell-1"),
|
||||
ev_function_call(
|
||||
invalid_call_id,
|
||||
"exec_command",
|
||||
&serde_json::to_string(&invalid_args)?,
|
||||
),
|
||||
ev_completed("resp-missing-shell-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-missing-shell-2"),
|
||||
ev_function_call(
|
||||
start_call_id,
|
||||
"exec_command",
|
||||
&serde_json::to_string(&start_args)?,
|
||||
),
|
||||
ev_completed("resp-missing-shell-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-missing-shell-3"),
|
||||
ev_function_call(
|
||||
interrupt_call_id,
|
||||
"write_stdin",
|
||||
&serde_json::to_string(&interrupt_args)?,
|
||||
),
|
||||
ev_completed("resp-missing-shell-3"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-missing-shell-4"),
|
||||
ev_assistant_message("msg-missing-shell", "done"),
|
||||
ev_completed("resp-missing-shell-4"),
|
||||
]),
|
||||
];
|
||||
let request_log = mount_sse_sequence(&server, responses).await;
|
||||
|
||||
submit_unified_exec_turn(
|
||||
&test,
|
||||
"resolve a model-selected shell before allocating a process",
|
||||
PermissionProfile::Disabled,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let mut valid_begin = None;
|
||||
loop {
|
||||
let event = wait_for_event(&test.codex, |_| true).await;
|
||||
match event {
|
||||
EventMsg::ExecApprovalRequest(approval) if approval.call_id == invalid_call_id => {
|
||||
panic!("shell resolution failure must precede approval: {approval:?}")
|
||||
}
|
||||
EventMsg::ExecCommandBegin(begin) if begin.call_id == invalid_call_id => {
|
||||
panic!("shell resolution failure must precede begin events: {begin:?}")
|
||||
}
|
||||
EventMsg::ExecCommandEnd(end) if end.call_id == invalid_call_id => {
|
||||
panic!("shell resolution failure must precede end events: {end:?}")
|
||||
}
|
||||
EventMsg::ExecCommandBegin(begin) if begin.call_id == start_call_id => {
|
||||
assert!(
|
||||
valid_begin.is_none(),
|
||||
"expected one begin event for the valid follow-up command"
|
||||
);
|
||||
valid_begin = Some(begin);
|
||||
}
|
||||
EventMsg::TurnComplete(_) => break,
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
|
||||
let valid_begin = valid_begin.expect("the valid follow-up command should begin normally");
|
||||
assert_eq!(
|
||||
valid_begin.process_id.as_deref(),
|
||||
Some("1000"),
|
||||
"failed shell resolution must not consume the first process ID"
|
||||
);
|
||||
|
||||
let invalid_output = request_log
|
||||
.function_call_output_text(invalid_call_id)
|
||||
.expect("missing typed shell-resolution failure output");
|
||||
assert!(
|
||||
invalid_output.contains("model-provided shell")
|
||||
&& invalid_output.contains("does not exist"),
|
||||
"unexpected shell-resolution failure: {invalid_output:?}"
|
||||
);
|
||||
assert!(
|
||||
!invalid_output.contains("Process running with session ID"),
|
||||
"failed shell resolution must not claim a running process: {invalid_output:?}"
|
||||
);
|
||||
|
||||
let start_output = parse_unified_exec_output(
|
||||
&request_log
|
||||
.function_call_output_text(start_call_id)
|
||||
.expect("missing valid follow-up exec_command output"),
|
||||
)?;
|
||||
assert_eq!(
|
||||
start_output.process_id.as_deref(),
|
||||
Some("1000"),
|
||||
"the first successfully allocated process should retain deterministic ID 1000"
|
||||
);
|
||||
assert!(
|
||||
start_output.exit_code.is_none(),
|
||||
"the valid process should still be running before write_stdin interrupts it"
|
||||
);
|
||||
|
||||
let interrupt_output = parse_unified_exec_output(
|
||||
&request_log
|
||||
.function_call_output_text(interrupt_call_id)
|
||||
.expect("missing write_stdin interrupt output"),
|
||||
)?;
|
||||
assert!(
|
||||
interrupt_output.process_id.is_none(),
|
||||
"write_stdin should clear the interrupted process from the session map"
|
||||
);
|
||||
assert_eq!(
|
||||
interrupt_output.exit_code,
|
||||
Some(130),
|
||||
"write_stdin should preserve the normal Unix SIGINT exit status"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn write_stdin_ctrl_c_interrupts_non_tty_session() -> Result<()> {
|
||||
// TODO(anp): Add a target-Windows test for explicit interrupt handling.
|
||||
|
||||
@@ -46,6 +46,12 @@ enum PushedExecScenario {
|
||||
ReplayGap,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
struct ExecServerObservation {
|
||||
process_read_requests: usize,
|
||||
process_start_argv: Vec<String>,
|
||||
}
|
||||
|
||||
async fn read_exec_server_json(websocket: &mut WebSocketStream<TcpStream>) -> Value {
|
||||
loop {
|
||||
match timeout(Duration::from_secs(5), websocket.next())
|
||||
@@ -109,7 +115,7 @@ async fn send_environment_info(websocket: &mut WebSocketStream<TcpStream>) {
|
||||
async fn serve_exec_with_pushed_events(
|
||||
listener: TcpListener,
|
||||
scenario: PushedExecScenario,
|
||||
) -> usize {
|
||||
) -> ExecServerObservation {
|
||||
let mut websocket = accept_initialized_exec_server(listener).await;
|
||||
send_environment_info(&mut websocket).await;
|
||||
|
||||
@@ -144,6 +150,16 @@ async fn serve_exec_with_pushed_events(
|
||||
.as_str()
|
||||
.expect("process/start should include processId")
|
||||
.to_string();
|
||||
let process_start_argv = process_start["params"]["argv"]
|
||||
.as_array()
|
||||
.expect("process/start should include argv")
|
||||
.iter()
|
||||
.map(|arg| {
|
||||
arg.as_str()
|
||||
.expect("process/start argv entries should be strings")
|
||||
.to_string()
|
||||
})
|
||||
.collect();
|
||||
|
||||
let replay_output = |seq| -> &'static [u8] {
|
||||
match seq {
|
||||
@@ -350,7 +366,10 @@ async fn serve_exec_with_pushed_events(
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
return process_read_requests;
|
||||
return ExecServerObservation {
|
||||
process_read_requests,
|
||||
process_start_argv,
|
||||
};
|
||||
}
|
||||
method => panic!("unexpected exec-server request: {method:?}"),
|
||||
}
|
||||
@@ -448,7 +467,7 @@ async fn exec_command_consumes_pushed_remote_process_events(
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
let process_read_requests = timeout(Duration::from_secs(5), exec_server)
|
||||
let observation = timeout(Duration::from_secs(5), exec_server)
|
||||
.await
|
||||
.context("fake exec-server should observe process cleanup")??;
|
||||
let request = response_mock
|
||||
@@ -464,26 +483,161 @@ async fn exec_command_consumes_pushed_remote_process_events(
|
||||
assert!(saw_exec_command_begin);
|
||||
assert!(output.contains("Process exited with code 0"));
|
||||
assert!(output.contains(COMPLETE_OUTPUT));
|
||||
assert_eq!(process_read_requests, 0, "unexpected compatibility read");
|
||||
assert_eq!(
|
||||
observation.process_read_requests, 0,
|
||||
"unexpected compatibility read"
|
||||
);
|
||||
}
|
||||
PushedExecScenario::DirectDenied => {
|
||||
assert!(!saw_exec_command_begin);
|
||||
assert!(output.contains("Process exited with code 1"));
|
||||
assert_eq!(process_read_requests, 0, "unexpected compatibility read");
|
||||
assert_eq!(
|
||||
observation.process_read_requests, 0,
|
||||
"unexpected compatibility read"
|
||||
);
|
||||
}
|
||||
PushedExecScenario::LegacyExit => {
|
||||
assert!(!saw_exec_command_begin);
|
||||
assert!(output.contains("Process exited with code 1"));
|
||||
assert_eq!(process_read_requests, 1, "expected compatibility read");
|
||||
assert_eq!(
|
||||
observation.process_read_requests, 1,
|
||||
"expected compatibility read"
|
||||
);
|
||||
}
|
||||
PushedExecScenario::ReplayGap => {
|
||||
assert_ne!(success, Some(false));
|
||||
assert!(saw_exec_command_begin);
|
||||
assert_eq!(output.matches(RECOVERED_OUTPUT).count(), 1);
|
||||
assert_eq!(output.matches(RETAINED_OUTPUT).count(), 1);
|
||||
assert_eq!(process_read_requests, 1, "expected replay recovery read");
|
||||
assert_eq!(
|
||||
observation.process_read_requests, 1,
|
||||
"expected replay recovery read"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn remote_explicit_shell_hint_uses_environment_reported_executable() -> Result<()> {
|
||||
const MODEL_SHELL_HINT: &str = r"C:\attacker\ZSH.ExE";
|
||||
const COMMAND: &str = "echo remote-hint-ok";
|
||||
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
let server = start_mock_server().await;
|
||||
let response_mock = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(
|
||||
CALL_ID,
|
||||
"exec_command",
|
||||
&json!({
|
||||
"shell": MODEL_SHELL_HINT,
|
||||
"cmd": COMMAND,
|
||||
"login": false,
|
||||
"yield_time_ms": 1_000,
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-2", "done"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
let exec_server_url = format!("ws://{}", listener.local_addr()?);
|
||||
let exec_server = tokio::spawn(serve_exec_with_pushed_events(
|
||||
listener,
|
||||
PushedExecScenario::Complete,
|
||||
));
|
||||
let mut builder = test_codex()
|
||||
.with_exec_server_url(exec_server_url)
|
||||
.with_config(|config| {
|
||||
config.project_doc_max_bytes = 0;
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = timeout(Duration::from_secs(5), builder.build(&server))
|
||||
.await
|
||||
.context("thread startup should connect to the fake exec-server")??;
|
||||
|
||||
let (sandbox_policy, permission_profile) =
|
||||
turn_permission_fields(PermissionProfile::Disabled, test.config.cwd.as_path());
|
||||
test.codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "run a remote command with an explicit shell hint".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: codex_protocol::protocol::ThreadSettingsOverrides {
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
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: test.session_configured.model.clone(),
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
loop {
|
||||
let event = timeout(Duration::from_secs(5), test.codex.next_event())
|
||||
.await
|
||||
.context("turn should complete")??
|
||||
.msg;
|
||||
if matches!(event, EventMsg::TurnComplete(_)) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
let observation = timeout(Duration::from_secs(5), exec_server)
|
||||
.await
|
||||
.context("fake exec-server should observe process cleanup")??;
|
||||
assert_eq!(
|
||||
observation.process_start_argv,
|
||||
["/bin/zsh", "-c", COMMAND],
|
||||
"the model hint must not replace the environment-reported shell"
|
||||
);
|
||||
assert!(
|
||||
observation
|
||||
.process_start_argv
|
||||
.iter()
|
||||
.all(|arg| !arg.to_ascii_lowercase().contains("attacker")),
|
||||
"process/start must not contain the model-supplied path"
|
||||
);
|
||||
assert_eq!(
|
||||
observation.process_read_requests, 0,
|
||||
"unexpected compatibility read"
|
||||
);
|
||||
|
||||
let request = response_mock
|
||||
.last_request()
|
||||
.context("model should receive the exec_command output")?;
|
||||
let (output, success) = request
|
||||
.function_call_output_content_and_success(CALL_ID)
|
||||
.context("exec_command output should be model visible")?;
|
||||
let output = output.context("exec_command output should contain text")?;
|
||||
assert_ne!(success, Some(false));
|
||||
assert!(output.contains("Process exited with code 0"));
|
||||
assert!(output.contains(COMPLETE_OUTPUT));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user