mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Scope command approvals by execution environment (#28738)
## Why Command approval cache keys included the command and working directory, but not the execution environment. An approval for `/workspace` locally could therefore be reused for the same command and path on an executor. ## What changed - Include the selected environment ID in shell and unified-exec approval cache keys. - Carry that ID through the normal command approval request so clients can show which environment is being approved. - Expose the environment through app-server as a required nullable `environmentId` and show it in the inline TUI approval prompt. - Keep older recorded approval events compatible when the environment is absent. For example, `echo ok` in local `/workspace` and `echo ok` in executor `/workspace` now produce different approval keys and separate prompts. ## Scope This PR does not change network approvals, Guardian review actions, MCP elicitation, full-screen TUI rendering, or environment-ID validation. Remote `shell_command` execution itself remains in #28722; this PR only makes its approval key environment-aware.
This commit is contained in:
@@ -524,6 +524,7 @@ impl NetworkApprovalService {
|
||||
turn_context.as_ref(),
|
||||
guardian_approval_id,
|
||||
/*approval_id*/ None,
|
||||
/*environment_id*/ None,
|
||||
prompt_command,
|
||||
#[allow(deprecated)]
|
||||
turn_context.cwd.clone(),
|
||||
|
||||
@@ -93,6 +93,7 @@ pub struct ShellRuntime {
|
||||
|
||||
#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)]
|
||||
pub(crate) struct ApprovalKey {
|
||||
environment_id: String,
|
||||
command: Vec<String>,
|
||||
cwd: AbsolutePathBuf,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
@@ -127,6 +128,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
|
||||
fn approval_keys(&self, req: &ShellRequest) -> Vec<Self::ApprovalKey> {
|
||||
vec![ApprovalKey {
|
||||
environment_id: req.turn_environment.environment_id.clone(),
|
||||
command: canonicalize_command_for_approval(&req.command),
|
||||
cwd: req.cwd.clone(),
|
||||
sandbox_permissions: req.sandbox_permissions,
|
||||
@@ -142,6 +144,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
let keys = self.approval_keys(req);
|
||||
let command = req.command.clone();
|
||||
let cwd = req.cwd.clone();
|
||||
let environment_id = Some(req.turn_environment.environment_id.clone());
|
||||
let retry_reason = ctx.retry_reason.clone();
|
||||
let reason = retry_reason.clone().or_else(|| req.justification.clone());
|
||||
let session = ctx.session;
|
||||
@@ -173,6 +176,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
turn,
|
||||
call_id,
|
||||
/*approval_id*/ None,
|
||||
environment_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
@@ -325,3 +329,7 @@ impl ToolRuntime<ShellRequest, ExecToolCallOutput> for ShellRuntime {
|
||||
Ok(out)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "shell_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -222,6 +222,7 @@ pub(super) async fn try_run_zsh_fork(
|
||||
session: Arc::clone(&ctx.session),
|
||||
turn: Arc::clone(&ctx.turn),
|
||||
call_id: ctx.call_id.clone(),
|
||||
environment_id: req.turn_environment.environment_id.clone(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: ctx.turn.approval_policy.value(),
|
||||
permission_profile: command_executor.permission_profile.clone(),
|
||||
@@ -294,6 +295,7 @@ pub(crate) async fn prepare_unified_exec_zsh_fork(
|
||||
session: Arc::clone(&ctx.session),
|
||||
turn: Arc::clone(&ctx.turn),
|
||||
call_id: ctx.call_id.clone(),
|
||||
environment_id: req.turn_environment.environment_id.clone(),
|
||||
tool_name: GuardianCommandSource::UnifiedExec,
|
||||
approval_policy: ctx.turn.approval_policy.value(),
|
||||
permission_profile: exec_request.permission_profile.clone(),
|
||||
@@ -328,6 +330,7 @@ struct CoreShellActionProvider {
|
||||
session: Arc<crate::session::session::Session>,
|
||||
turn: Arc<crate::session::turn_context::TurnContext>,
|
||||
call_id: String,
|
||||
environment_id: String,
|
||||
tool_name: GuardianCommandSource,
|
||||
approval_policy: AskForApproval,
|
||||
permission_profile: PermissionProfile,
|
||||
@@ -424,6 +427,7 @@ impl CoreShellActionProvider {
|
||||
let turn = self.turn.clone();
|
||||
let call_id = self.call_id.clone();
|
||||
let approval_id = Some(Uuid::new_v4().to_string());
|
||||
let environment_id = Some(self.environment_id.clone());
|
||||
let source = self.tool_name;
|
||||
let guardian_review_id = routes_approval_to_guardian(&turn).then(new_guardian_review_id);
|
||||
Ok(stopwatch
|
||||
@@ -489,6 +493,7 @@ impl CoreShellActionProvider {
|
||||
&turn,
|
||||
call_id,
|
||||
approval_id,
|
||||
environment_id,
|
||||
command,
|
||||
workdir.clone(),
|
||||
/*reason*/ None,
|
||||
|
||||
@@ -425,6 +425,7 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho
|
||||
session: Arc::new(session),
|
||||
turn: Arc::new(turn_context),
|
||||
call_id: "preapproved-additional-permissions".to_string(),
|
||||
environment_id: "local".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
permission_profile: permission_profile.clone(),
|
||||
@@ -560,6 +561,7 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul
|
||||
session: std::sync::Arc::new(session),
|
||||
turn: std::sync::Arc::new(turn_context),
|
||||
call_id: "execve-hook-call".to_string(),
|
||||
environment_id: "local".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
@@ -770,6 +772,7 @@ prefix_rule(pattern = ["{cat_path_literal}"], decision = "allow")
|
||||
session: Arc::new(session),
|
||||
turn: Arc::new(turn_context),
|
||||
call_id: "deny-read-prefix-allow".to_string(),
|
||||
environment_id: "local".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
permission_profile,
|
||||
@@ -806,6 +809,7 @@ async fn denied_reads_keep_granular_sandbox_rejection_for_escalation() -> anyhow
|
||||
session: Arc::new(session),
|
||||
turn: Arc::new(turn_context),
|
||||
call_id: "deny-read-granular-sandbox-reject".to_string(),
|
||||
environment_id: "local".to_string(),
|
||||
tool_name: GuardianCommandSource::Shell,
|
||||
approval_policy: AskForApproval::Granular(GranularApprovalConfig {
|
||||
sandbox_approval: false,
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
use super::*;
|
||||
use codex_exec_server::Environment;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::sync::Arc;
|
||||
|
||||
#[tokio::test]
|
||||
async fn approval_key_includes_environment_id() {
|
||||
let cwd = AbsolutePathBuf::try_from(std::env::current_dir().expect("read current dir"))
|
||||
.expect("current dir is absolute");
|
||||
let mut request = ShellRequest {
|
||||
command: vec!["echo".to_string(), "hello".to_string()],
|
||||
turn_environment: TurnEnvironment::new(
|
||||
"remote".to_string(),
|
||||
Arc::new(Environment::default_for_tests()),
|
||||
PathUri::from_abs_path(&cwd),
|
||||
/*shell*/ None,
|
||||
),
|
||||
shell_type: None,
|
||||
hook_command: "echo hello".to_string(),
|
||||
cwd: cwd.clone(),
|
||||
timeout_ms: None,
|
||||
cancellation_token: CancellationToken::new(),
|
||||
env: HashMap::new(),
|
||||
explicit_env_overrides: HashMap::new(),
|
||||
network: None,
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
#[cfg(unix)]
|
||||
additional_permissions_preapproved: false,
|
||||
justification: None,
|
||||
exec_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
};
|
||||
let runtime = ShellRuntime::for_shell_command(ShellRuntimeBackend::ShellCommandClassic);
|
||||
let original_key = runtime.approval_keys(&request);
|
||||
request.turn_environment.environment_id = "other".to_string();
|
||||
let other_key = runtime.approval_keys(&request);
|
||||
|
||||
assert_ne!(original_key, other_key);
|
||||
}
|
||||
@@ -83,6 +83,7 @@ pub struct UnifiedExecRequest {
|
||||
/// unified-exec launches.
|
||||
#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)]
|
||||
pub struct UnifiedExecApprovalKey {
|
||||
pub environment_id: String,
|
||||
pub command: Vec<String>,
|
||||
pub cwd: AbsolutePathBuf,
|
||||
pub tty: bool,
|
||||
@@ -135,6 +136,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
|
||||
fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec<Self::ApprovalKey> {
|
||||
vec![UnifiedExecApprovalKey {
|
||||
environment_id: req.turn_environment.environment_id.clone(),
|
||||
command: canonicalize_command_for_approval(&req.command),
|
||||
cwd: req.cwd.clone(),
|
||||
tty: req.tty,
|
||||
@@ -154,6 +156,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
let call_id = ctx.call_id.to_string();
|
||||
let command = req.command.clone();
|
||||
let cwd = req.cwd.clone();
|
||||
let environment_id = Some(req.turn_environment.environment_id.clone());
|
||||
let retry_reason = ctx.retry_reason.clone();
|
||||
let reason = retry_reason.clone().or_else(|| req.justification.clone());
|
||||
let guardian_review_id = ctx.guardian_review_id.clone();
|
||||
@@ -183,6 +186,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
turn,
|
||||
call_id,
|
||||
/*approval_id*/ None,
|
||||
environment_id,
|
||||
command,
|
||||
cwd.clone(),
|
||||
reason,
|
||||
@@ -463,6 +467,25 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approval_key_includes_environment_id() {
|
||||
let manager = UnifiedExecProcessManager::default();
|
||||
let runtime = UnifiedExecRuntime::new(&manager, UnifiedExecShellMode::Direct);
|
||||
let mut request = test_request(
|
||||
SandboxPermissions::UseDefault,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
);
|
||||
request.turn_environment.environment_id = "remote".to_string();
|
||||
let original_key = runtime.approval_keys(&request);
|
||||
request.turn_environment.environment_id = "other".to_string();
|
||||
let other_key = runtime.approval_keys(&request);
|
||||
|
||||
assert_ne!(original_key, other_key);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn unified_exec_uses_the_trusted_sandbox_cwd() {
|
||||
let cwd_dir = tempdir().expect("create process temp dir");
|
||||
|
||||
Reference in New Issue
Block a user