mirror of
https://github.com/openai/codex.git
synced 2026-09-20 12:47:38 +00:00
Add permission request hook coverage for exec approvals
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -157,7 +157,7 @@ pub(crate) async fn run_pre_tool_use_hooks(
|
||||
pub(crate) async fn run_permission_request_hooks(
|
||||
sess: &Arc<Session>,
|
||||
turn_context: &Arc<TurnContext>,
|
||||
run_id_suffix: String,
|
||||
request_id: &str,
|
||||
payload: PermissionRequestPayload,
|
||||
approval_attempt: PermissionRequestApprovalAttempt,
|
||||
retry_reason: Option<String>,
|
||||
@@ -171,7 +171,7 @@ pub(crate) async fn run_permission_request_hooks(
|
||||
model: turn_context.model_info.slug.clone(),
|
||||
permission_mode: hook_permission_mode(turn_context),
|
||||
tool_name: payload.tool_name,
|
||||
run_id_suffix,
|
||||
run_id_suffix: format!("{request_id}:{}", approval_attempt.as_wire_value()),
|
||||
command: payload.command,
|
||||
sandbox_permissions: payload.sandbox_permissions,
|
||||
additional_permissions: payload.additional_permissions,
|
||||
|
||||
@@ -314,6 +314,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
.exec_command(
|
||||
ExecCommandRequest {
|
||||
command,
|
||||
hook_command: args.cmd,
|
||||
process_id,
|
||||
yield_time_ms,
|
||||
max_output_tokens,
|
||||
|
||||
@@ -373,11 +373,7 @@ impl ToolOrchestrator {
|
||||
match run_permission_request_hooks(
|
||||
approval_ctx.session,
|
||||
approval_ctx.turn,
|
||||
format!(
|
||||
"{}:{}",
|
||||
approval_ctx.call_id,
|
||||
approval_attempt.as_wire_value()
|
||||
),
|
||||
approval_ctx.call_id,
|
||||
permission_request,
|
||||
approval_attempt,
|
||||
approval_ctx.retry_reason.clone(),
|
||||
|
||||
@@ -8,11 +8,13 @@ use crate::guardian::guardian_timeout_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::hook_runtime::run_permission_request_hooks;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::ExecRequest;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::shell::ShellType;
|
||||
use crate::tools::runtimes::build_sandbox_command;
|
||||
use crate::tools::sandboxing::PermissionRequestPayload;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
@@ -22,6 +24,8 @@ use codex_execpolicy::MatchOptions;
|
||||
use codex_execpolicy::Policy;
|
||||
use codex_execpolicy::RuleMatch;
|
||||
use codex_features::Feature;
|
||||
use codex_hooks::PermissionRequestApprovalAttempt;
|
||||
use codex_hooks::PermissionRequestDecision;
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::SandboxErr;
|
||||
@@ -320,6 +324,7 @@ enum DecisionSource {
|
||||
struct PromptDecision {
|
||||
decision: ReviewDecision,
|
||||
guardian_review_id: Option<String>,
|
||||
rejection_message: Option<String>,
|
||||
}
|
||||
|
||||
fn execve_prompt_is_rejected_by_policy(
|
||||
@@ -394,11 +399,49 @@ impl CoreShellActionProvider {
|
||||
let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id);
|
||||
Ok(stopwatch
|
||||
.pause_for(async move {
|
||||
// 1) Run PermissionRequest hooks
|
||||
let permission_request = PermissionRequestPayload {
|
||||
tool_name: "Bash".to_string(),
|
||||
command: codex_shell_command::parse_command::shlex_join(&command),
|
||||
sandbox_permissions: self.approval_sandbox_permissions,
|
||||
additional_permissions: additional_permissions.clone(),
|
||||
justification: None,
|
||||
};
|
||||
let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone());
|
||||
match run_permission_request_hooks(
|
||||
&session,
|
||||
&turn,
|
||||
&effective_approval_id,
|
||||
permission_request,
|
||||
PermissionRequestApprovalAttempt::Initial,
|
||||
/*retry_reason*/ None,
|
||||
/*network_approval_context*/ None,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Some(PermissionRequestDecision::Allow) => {
|
||||
return PromptDecision {
|
||||
decision: ReviewDecision::Approved,
|
||||
guardian_review_id: None,
|
||||
rejection_message: None,
|
||||
};
|
||||
}
|
||||
Some(PermissionRequestDecision::Deny { message }) => {
|
||||
return PromptDecision {
|
||||
decision: ReviewDecision::Denied,
|
||||
guardian_review_id: None,
|
||||
rejection_message: Some(message),
|
||||
};
|
||||
}
|
||||
None => {}
|
||||
}
|
||||
|
||||
// 2) Route to Guardian if configured
|
||||
if let Some(review_id) = guardian_review_id.clone() {
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
review_id,
|
||||
review_id.clone(),
|
||||
GuardianApprovalRequest::Execve {
|
||||
id: call_id.clone(),
|
||||
source,
|
||||
@@ -413,8 +456,11 @@ impl CoreShellActionProvider {
|
||||
return PromptDecision {
|
||||
decision,
|
||||
guardian_review_id,
|
||||
rejection_message: None,
|
||||
};
|
||||
}
|
||||
|
||||
// 3) Fall back to regular user prompt
|
||||
let decision = session
|
||||
.request_command_approval(
|
||||
&turn,
|
||||
@@ -432,6 +478,7 @@ impl CoreShellActionProvider {
|
||||
PromptDecision {
|
||||
decision,
|
||||
guardian_review_id: None,
|
||||
rejection_message: None,
|
||||
}
|
||||
})
|
||||
.await)
|
||||
@@ -487,7 +534,11 @@ impl CoreShellActionProvider {
|
||||
}
|
||||
},
|
||||
ReviewDecision::Denied => {
|
||||
let message = if let Some(review_id) =
|
||||
let message = if let Some(message) =
|
||||
prompt_decision.rejection_message.clone()
|
||||
{
|
||||
message
|
||||
} else if let Some(review_id) =
|
||||
prompt_decision.guardian_review_id.as_deref()
|
||||
{
|
||||
guardian_rejection_message(self.session.as_ref(), review_id).await
|
||||
|
||||
@@ -6,11 +6,16 @@ use super::evaluate_intercepted_exec_policy;
|
||||
use super::extract_shell_script;
|
||||
use super::join_program_and_argv;
|
||||
use super::map_exec_result;
|
||||
use crate::codex::make_session_and_context;
|
||||
use crate::config::Constrained;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use anyhow::Context;
|
||||
use codex_execpolicy::Decision;
|
||||
use codex_execpolicy::Evaluation;
|
||||
use codex_execpolicy::PolicyParser;
|
||||
use codex_execpolicy::RuleMatch;
|
||||
use codex_hooks::Hooks;
|
||||
use codex_hooks::HooksConfig;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
@@ -21,6 +26,7 @@ use codex_protocol::permissions::FileSystemSpecialPath;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_protocol::protocol::GuardianCommandSource;
|
||||
use codex_protocol::protocol::ReadOnlyAccess;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_sandboxing::SandboxType;
|
||||
@@ -30,8 +36,10 @@ use codex_shell_escalation::ExecResult;
|
||||
use codex_shell_escalation::Permissions as EscalatedPermissions;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
use tokio::sync::RwLock;
|
||||
|
||||
fn host_absolute_path(segments: &[&str]) -> String {
|
||||
let mut path = if cfg!(windows) {
|
||||
@@ -319,6 +327,134 @@ fn shell_request_escalation_execution_is_explicit() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Result<()> {
|
||||
let (mut session, mut turn_context) = make_session_and_context().await;
|
||||
std::fs::create_dir_all(&turn_context.config.codex_home)
|
||||
.context("recreate codex home for hook fixtures")?;
|
||||
let script_path = turn_context
|
||||
.config
|
||||
.codex_home
|
||||
.join("permission_request_hook.py");
|
||||
let log_path = turn_context
|
||||
.config
|
||||
.codex_home
|
||||
.join("permission_request_hook_log.jsonl");
|
||||
std::fs::write(
|
||||
&script_path,
|
||||
format!(
|
||||
"#!/bin/sh\ncat > {log_path}\nprintf '%s\\n' '{response}'\n",
|
||||
log_path = shlex::try_quote(log_path.to_string_lossy().as_ref())?,
|
||||
response = "{\"hookSpecificOutput\":{\"hookEventName\":\"PermissionRequest\",\"decision\":{\"behavior\":\"allow\"}}}",
|
||||
),
|
||||
)
|
||||
.with_context(|| format!("write hook script to {}", script_path.display()))?;
|
||||
#[cfg(unix)]
|
||||
{
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let mut permissions = std::fs::metadata(&script_path)
|
||||
.with_context(|| format!("read hook script metadata from {}", script_path.display()))?
|
||||
.permissions();
|
||||
permissions.set_mode(0o755);
|
||||
std::fs::set_permissions(&script_path, permissions)
|
||||
.with_context(|| format!("set hook script permissions on {}", script_path.display()))?;
|
||||
}
|
||||
std::fs::write(
|
||||
turn_context.config.codex_home.join("hooks.json"),
|
||||
serde_json::json!({
|
||||
"hooks": {
|
||||
"PermissionRequest": [{
|
||||
"hooks": [{
|
||||
"type": "command",
|
||||
"command": script_path.display().to_string(),
|
||||
}]
|
||||
}]
|
||||
}
|
||||
})
|
||||
.to_string(),
|
||||
)
|
||||
.context("write hooks.json")?;
|
||||
|
||||
let mut hook_shell_argv = session
|
||||
.user_shell()
|
||||
.derive_exec_args("", /*use_login_shell*/ false);
|
||||
let hook_shell_program = hook_shell_argv.remove(0);
|
||||
let _ = hook_shell_argv.pop();
|
||||
session.services.hooks = Hooks::new(HooksConfig {
|
||||
feature_enabled: true,
|
||||
config_layer_stack: Some(turn_context.config.config_layer_stack.clone()),
|
||||
shell_program: Some(hook_shell_program),
|
||||
shell_args: hook_shell_argv,
|
||||
..HooksConfig::default()
|
||||
});
|
||||
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
turn_context.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
turn_context.sandbox_policy = Constrained::allow_any(sandbox_policy.clone());
|
||||
turn_context.file_system_sandbox_policy = read_only_file_system_sandbox_policy();
|
||||
turn_context.network_sandbox_policy = NetworkSandboxPolicy::Restricted;
|
||||
|
||||
let workdir = AbsolutePathBuf::try_from(std::env::current_dir()?)?;
|
||||
let target = std::env::temp_dir().join("execve-hook-short-circuit.txt");
|
||||
let target_str = target.display().to_string();
|
||||
let command = vec!["touch".to_string(), target_str.clone()];
|
||||
let expected_hook_command =
|
||||
codex_shell_command::parse_command::shlex_join(&["/usr/bin/touch".to_string(), target_str]);
|
||||
let provider = CoreShellActionProvider {
|
||||
policy: std::sync::Arc::new(RwLock::new(codex_execpolicy::Policy::empty())),
|
||||
session: std::sync::Arc::new(session),
|
||||
turn: std::sync::Arc::new(turn_context),
|
||||
call_id: "execve-hook-call".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy: read_only_file_system_sandbox_policy(),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
approval_sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
prompt_permissions: None,
|
||||
stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)),
|
||||
};
|
||||
|
||||
let action = tokio::time::timeout(
|
||||
Duration::from_secs(5),
|
||||
codex_shell_escalation::EscalationPolicy::determine_action(
|
||||
&provider,
|
||||
&AbsolutePathBuf::from_absolute_path("/usr/bin/touch")
|
||||
.context("build touch absolute path")?,
|
||||
&command,
|
||||
&workdir,
|
||||
),
|
||||
)
|
||||
.await
|
||||
.context("timed out waiting for execve permission hook decision")??;
|
||||
assert!(matches!(
|
||||
action,
|
||||
codex_shell_escalation::EscalationDecision::Escalate(
|
||||
codex_shell_escalation::EscalationExecution::Unsandboxed
|
||||
)
|
||||
));
|
||||
|
||||
let hook_inputs: Vec<Value> = std::fs::read_to_string(&log_path)
|
||||
.with_context(|| format!("read hook log at {}", log_path.display()))?
|
||||
.lines()
|
||||
.map(serde_json::from_str)
|
||||
.collect::<serde_json::Result<_>>()
|
||||
.context("parse hook log")?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(
|
||||
hook_inputs[0]["tool_input"]["command"],
|
||||
expected_hook_command
|
||||
);
|
||||
assert_eq!(
|
||||
hook_inputs[0]["approval_context"]["approval_attempt"],
|
||||
"initial"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn evaluate_intercepted_exec_policy_uses_wrapper_command_when_shell_wrapper_parsing_disabled() {
|
||||
let policy_src = r#"prefix_rule(pattern = ["npm", "publish"], decision = "prompt")"#;
|
||||
|
||||
@@ -88,6 +88,7 @@ impl UnifiedExecContext {
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct ExecCommandRequest {
|
||||
pub command: Vec<String>,
|
||||
pub hook_command: String,
|
||||
pub process_id: i32,
|
||||
pub yield_time_ms: u64,
|
||||
pub max_output_tokens: Option<usize>,
|
||||
|
||||
@@ -677,7 +677,7 @@ impl UnifiedExecProcessManager {
|
||||
.await;
|
||||
let req = UnifiedExecToolRequest {
|
||||
command: request.command.clone(),
|
||||
hook_command: codex_shell_command::parse_command::shlex_join(&request.command),
|
||||
hook_command: request.hook_command.clone(),
|
||||
process_id: request.process_id,
|
||||
cwd,
|
||||
env,
|
||||
|
||||
@@ -1183,6 +1183,100 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn permission_request_hook_sees_raw_exec_command_input() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "permissionrequest-exec-command";
|
||||
let marker = std::env::temp_dir().join("permissionrequest-exec-command-marker");
|
||||
let command = format!("rm -f {}", marker.display());
|
||||
let args = serde_json::json!({
|
||||
"cmd": command,
|
||||
"login": true,
|
||||
});
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
core_test_support::responses::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", "permission request hook allowed exec_command"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) = write_permission_request_hook(
|
||||
home,
|
||||
Some("^Bash$"),
|
||||
"allow",
|
||||
"should not be used for allow",
|
||||
) {
|
||||
panic!("failed to write permission request hook test fixture: {error}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config.use_experimental_unified_exec_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
fs::write(&marker, "seed").context("create exec command permission request marker")?;
|
||||
|
||||
test.submit_turn_with_policies(
|
||||
"run the exec command after hook approval",
|
||||
AskForApproval::OnRequest,
|
||||
codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
requests[1].function_call_output(call_id);
|
||||
assert!(
|
||||
!marker.exists(),
|
||||
"approved exec command should remove marker file"
|
||||
);
|
||||
|
||||
let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(hook_inputs[0]["hook_event_name"], "PermissionRequest");
|
||||
assert_eq!(hook_inputs[0]["tool_name"], "Bash");
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], command);
|
||||
assert_eq!(
|
||||
hook_inputs[0]["approval_context"],
|
||||
serde_json::json!({
|
||||
"sandbox_permissions": "use_default",
|
||||
"additional_permissions": null,
|
||||
"justification": null,
|
||||
"approval_attempt": "initial",
|
||||
"retry_reason": null,
|
||||
"network_approval_context": null,
|
||||
})
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn permission_request_hook_sees_retry_context_after_sandbox_denial() -> Result<()> {
|
||||
|
||||
@@ -211,7 +211,7 @@ fn build_command_input(request: &PermissionRequestRequest) -> PermissionRequestC
|
||||
sandbox_permissions: request.sandbox_permissions,
|
||||
additional_permissions: request.additional_permissions.clone(),
|
||||
justification: request.justification.clone(),
|
||||
approval_attempt: request.approval_attempt.clone(),
|
||||
approval_attempt: request.approval_attempt,
|
||||
retry_reason: request.retry_reason.clone(),
|
||||
network_approval_context: request.network_approval_context.clone(),
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user