mirror of
https://github.com/openai/codex.git
synced 2026-10-27 16:02:59 +00:00
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
This commit is contained in:
committed by
copyberry
parent
3d1d26915a
commit
7b38c48da9
@@ -89,6 +89,19 @@ where
|
||||
})
|
||||
}
|
||||
|
||||
fn resolve_sandbox_permissions(
|
||||
sandbox_permissions: Option<SandboxPermissions>,
|
||||
justification: Option<&str>,
|
||||
) -> Result<SandboxPermissions, FunctionCallError> {
|
||||
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")
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 =
|
||||
|
||||
@@ -38,7 +38,7 @@ pub(crate) struct ExecCommandArgs {
|
||||
#[serde(default)]
|
||||
max_output_tokens: Option<usize>,
|
||||
#[serde(default)]
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
sandbox_permissions: Option<SandboxPermissions>,
|
||||
#[serde(default)]
|
||||
additional_permissions: Option<AdditionalPermissionProfile>,
|
||||
#[serde(default)]
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(()));
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user