From 7b38c48da930445eaafee166ed86d9240f490c8d Mon Sep 17 00:00:00 2001 From: Mitsuhiro Kotake Date: Fri, 31 Jul 2026 15:28:49 +0000 Subject: [PATCH] Require explicit sandbox permissions with shell justifications (#36350) ## What changed Reject `shell_command` and unified `exec_command` calls that provide a `justification` without also specifying `sandbox_permissions`. The model-visible error tells callers to explicitly request `require_escalated` execution or omit the justification. ## Testing - Add unit coverage for `shell_command` argument validation. - Add integration coverage for both shell tool paths, including verification that rejected unified exec calls do not begin execution. GitOrigin-RevId: 6af39d2c889af492440615eae12085b99a8b5169 --- codex-rs/core/src/tools/handlers/mod.rs | 13 ++++ .../src/tools/handlers/shell/shell_command.rs | 7 +- .../core/src/tools/handlers/shell_tests.rs | 39 ++++++++++ .../core/src/tools/handlers/unified_exec.rs | 2 +- .../handlers/unified_exec/exec_command.rs | 5 +- codex-rs/core/tests/suite/shell_command.rs | 43 +++++++++++ codex-rs/core/tests/suite/unified_exec.rs | 72 +++++++++++++++++++ 7 files changed, 178 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 3033e7b08c..f64706e868 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -89,6 +89,19 @@ where }) } +fn resolve_sandbox_permissions( + sandbox_permissions: Option, + justification: Option<&str>, +) -> Result { + if justification.is_some() && sandbox_permissions.is_none() { + return Err(FunctionCallError::RespondToModel( + "`justification` requires an explicit `sandbox_permissions`; use `sandbox_permissions: \"require_escalated\"` for unsandboxed execution, or omit `justification`.".to_string(), + )); + } + + Ok(sandbox_permissions.unwrap_or_default()) +} + fn updated_hook_command(updated_input: &Value) -> Result<&str, FunctionCallError> { updated_input .get("command") diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index 62bcee8bdf..2b4cc7e2ed 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -16,6 +16,7 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::handlers::parse_arguments_with_base_path; +use crate::tools::handlers::resolve_sandbox_permissions; use crate::tools::handlers::resolve_workdir_base_path; use crate::tools::handlers::rewrite_function_string_argument; use crate::tools::handlers::updated_hook_command; @@ -106,6 +107,10 @@ impl ShellCommandHandler { ); let active_permission_profile = turn_context.config.permissions.active_permission_profile(); inject_permission_profile_env(&mut env, active_permission_profile.as_ref()); + let sandbox_permissions = resolve_sandbox_permissions( + params.sandbox_permissions, + params.justification.as_deref(), + )?; Ok(ExecParams { command, @@ -115,7 +120,7 @@ impl ShellCommandHandler { env, network: turn_context.network.clone(), network_environment_id: Some(turn_environment.environment_id.clone()), - sandbox_permissions: params.sandbox_permissions.unwrap_or_default(), + sandbox_permissions, windows_sandbox_level: turn_context.windows_sandbox_level, windows_sandbox_private_desktop: turn_context .config diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index 90fd27d878..f71f23ca1b 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -229,6 +229,45 @@ async fn shell_command_handler_defaults_to_non_login_when_disallowed() { ); } +#[tokio::test] +async fn shell_command_handler_rejects_justification_without_sandbox_permissions() { + let (session, turn_context) = make_session_and_context().await; + let turn_environment = turn_context + .environments + .primary() + .expect("primary environment"); + let cwd = turn_environment + .cwd() + .to_abs_path() + .expect("native environment cwd"); + let params = ShellCommandToolCallParams { + command: "echo hello".to_string(), + workdir: None, + login: None, + timeout_ms: None, + sandbox_permissions: None, + additional_permissions: None, + prefix_rule: None, + justification: Some("Allow this command".to_string()), + }; + + let err = ShellCommandHandler::to_exec_params( + ¶ms, + &session, + &turn_context, + turn_environment, + cwd, + /*allow_login_shell*/ false, + ) + .expect_err("justification without sandbox permissions should be rejected"); + + assert!( + err.to_string() + .contains("`justification` requires an explicit `sandbox_permissions`"), + "unexpected error: {err}" + ); +} + #[test] fn shell_command_handler_rejects_login_when_disallowed() { let err = diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 71e776dbf0..f6a5bfa542 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -38,7 +38,7 @@ pub(crate) struct ExecCommandArgs { #[serde(default)] max_output_tokens: Option, #[serde(default)] - sandbox_permissions: SandboxPermissions, + sandbox_permissions: Option, #[serde(default)] additional_permissions: Option, #[serde(default)] diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 3c2f6daf86..16eed42a80 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -13,6 +13,7 @@ use crate::tools::handlers::implicit_granted_permissions; use crate::tools::handlers::normalize_and_validate_additional_permissions; use crate::tools::handlers::parse_arguments; use crate::tools::handlers::parse_arguments_with_base_path; +use crate::tools::handlers::resolve_sandbox_permissions; use crate::tools::handlers::resolve_tool_environment; use crate::tools::handlers::rewrite_function_string_argument; use crate::tools::handlers::updated_hook_command; @@ -188,6 +189,8 @@ impl ExecCommandHandler { parse_arguments(&arguments)? } }; + let sandbox_permissions = + resolve_sandbox_permissions(args.sandbox_permissions, args.justification.as_deref())?; let hook_command = args.cmd.clone(); // TODO(anp) wire PathUri through implicit skills instead of skipping on foreign paths if let Some(native_cwd) = native_cwd.as_ref() { @@ -243,7 +246,7 @@ impl ExecCommandHandler { tty, yield_time_ms, max_output_tokens, - sandbox_permissions, + sandbox_permissions: _, additional_permissions, justification, prefix_rule, diff --git a/codex-rs/core/tests/suite/shell_command.rs b/codex-rs/core/tests/suite/shell_command.rs index fd6befd85c..8fe5f976cd 100644 --- a/codex-rs/core/tests/suite/shell_command.rs +++ b/codex-rs/core/tests/suite/shell_command.rs @@ -1,6 +1,7 @@ use std::time::Duration; use anyhow::Result; +use codex_protocol::models::PermissionProfile; use core_test_support::assert_regex_match; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -13,6 +14,7 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::TestCodexBuilder; use core_test_support::test_codex::TestCodexHarness; use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; use serde_json::json; use test_case::test_case; @@ -124,6 +126,47 @@ async fn shell_command_works() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_command_rejects_justification_without_sandbox_permissions() -> Result<()> { + skip_if_no_network!(Ok(())); + + let harness = shell_command_harness_with(|builder| builder.with_model("gpt-5.4")).await?; + let call_id = "shell-command-missing-sandbox-permissions"; + let args = json!({ + "command": "echo should not run", + "justification": "Allow this command", + }); + mount_sse_sequence( + harness.server(), + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "shell_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + harness + .submit_with_permission_profile( + "run the command with escalation", + PermissionProfile::Disabled, + ) + .await?; + + assert_eq!( + harness.function_call_stdout(call_id).await, + "`justification` requires an explicit `sandbox_permissions`; use `sandbox_permissions: \"require_escalated\"` for unsandboxed execution, or omit `justification`." + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn output_with_login() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 07902d01fd..23f644da57 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -385,6 +385,78 @@ async fn unified_exec_intercepts_apply_patch_exec_command() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_rejects_justification_without_sandbox_permissions() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mut builder = test_codex().with_model("gpt-5.2").with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + let test = builder.build_with_auto_env(&server).await?; + + let call_id = "uexec-missing-sandbox-permissions"; + let args = json!({ + "cmd": "echo should not run", + "justification": "Allow this command", + }); + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "finished"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + submit_unified_exec_turn( + &test, + "run the command with escalation", + PermissionProfile::Disabled, + ) + .await?; + + let mut saw_exec_begin = false; + wait_for_event(&test.codex, |event| match event { + EventMsg::ExecCommandBegin(event) if event.call_id == call_id => { + saw_exec_begin = true; + false + } + EventMsg::TurnComplete(_) => true, + _ => false, + }) + .await; + assert!( + !saw_exec_begin, + "rejected exec_command should not emit ExecCommandBegin" + ); + + let request = mock + .last_request() + .expect("model should receive the rejected tool call output"); + let output_item = request.function_call_output(call_id); + let output = extract_output_text(&output_item) + .expect("rejected tool call should include model-visible text"); + assert_eq!( + output, + "`justification` requires an explicit `sandbox_permissions`; use `sandbox_permissions: \"require_escalated\"` for unsandboxed execution, or omit `justification`." + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { // TODO(anp): Remove after unified-exec fixtures use target-native commands.