mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
whitelist command prefix integration in core and tui (#7033)
this PR enables TUI to approve commands and add their prefixes to an allowlist: <img width="708" height="605" alt="Screenshot 2025-11-21 at 4 18 07 PM" src="https://github.com/user-attachments/assets/56a19893-4553-4770-a881-becf79eeda32" /> note: we only show the option to whitelist the command when 1) command is not multi-part (e.g `git add -A && git commit -m 'hello world'`) 2) command is not already matched by an existing rule
This commit is contained in:
committed by
GitHub
Unverified
parent
ccdeb9d9c4
commit
e925a380dc
@@ -6,7 +6,7 @@ use std::sync::Arc;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::exec::ExecParams;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::exec_policy::create_approval_requirement_for_command;
|
||||
use crate::exec_policy::create_exec_approval_requirement_for_command;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::is_safe_command::is_known_safe_command;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
@@ -231,8 +231,10 @@ impl ShellHandler {
|
||||
let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None);
|
||||
emitter.begin(event_ctx).await;
|
||||
|
||||
let approval_requirement = create_approval_requirement_for_command(
|
||||
let features = session.features();
|
||||
let exec_approval_requirement = create_exec_approval_requirement_for_command(
|
||||
&turn.exec_policy,
|
||||
&features,
|
||||
&exec_params.command,
|
||||
turn.approval_policy,
|
||||
&turn.sandbox_policy,
|
||||
@@ -247,7 +249,7 @@ impl ShellHandler {
|
||||
env: exec_params.env.clone(),
|
||||
with_escalated_permissions: exec_params.with_escalated_permissions,
|
||||
justification: exec_params.justification.clone(),
|
||||
approval_requirement,
|
||||
exec_approval_requirement,
|
||||
};
|
||||
let mut orchestrator = ToolOrchestrator::new();
|
||||
let mut runtime = ShellRuntime::new();
|
||||
|
||||
@@ -11,14 +11,14 @@ use crate::error::get_error_message_ui;
|
||||
use crate::exec::ExecToolCallOutput;
|
||||
use crate::sandboxing::SandboxManager;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ProvidesSandboxRetryData;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
use crate::tools::sandboxing::ToolCtx;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use crate::tools::sandboxing::ToolRuntime;
|
||||
use crate::tools::sandboxing::default_approval_requirement;
|
||||
use crate::tools::sandboxing::default_exec_approval_requirement;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
|
||||
@@ -54,17 +54,17 @@ impl ToolOrchestrator {
|
||||
// 1) Approval
|
||||
let mut already_approved = false;
|
||||
|
||||
let requirement = tool.approval_requirement(req).unwrap_or_else(|| {
|
||||
default_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
|
||||
let requirement = tool.exec_approval_requirement(req).unwrap_or_else(|| {
|
||||
default_exec_approval_requirement(approval_policy, &turn_ctx.sandbox_policy)
|
||||
});
|
||||
match requirement {
|
||||
ApprovalRequirement::Skip { .. } => {
|
||||
otel.tool_decision(otel_tn, otel_ci, ReviewDecision::Approved, otel_cfg);
|
||||
ExecApprovalRequirement::Skip { .. } => {
|
||||
otel.tool_decision(otel_tn, otel_ci, &ReviewDecision::Approved, otel_cfg);
|
||||
}
|
||||
ApprovalRequirement::Forbidden { reason } => {
|
||||
ExecApprovalRequirement::Forbidden { reason } => {
|
||||
return Err(ToolError::Rejected(reason));
|
||||
}
|
||||
ApprovalRequirement::NeedsApproval { reason } => {
|
||||
ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
let mut risk = None;
|
||||
|
||||
if let Some(metadata) = req.sandbox_retry_data() {
|
||||
@@ -88,13 +88,15 @@ impl ToolOrchestrator {
|
||||
};
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
|
||||
otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone());
|
||||
otel.tool_decision(otel_tn, otel_ci, &decision, otel_user.clone());
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedAllowPrefix { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
}
|
||||
already_approved = true;
|
||||
}
|
||||
@@ -169,13 +171,15 @@ impl ToolOrchestrator {
|
||||
};
|
||||
|
||||
let decision = tool.start_approval_async(req, approval_ctx).await;
|
||||
otel.tool_decision(otel_tn, otel_ci, decision, otel_user);
|
||||
otel.tool_decision(otel_tn, otel_ci, &decision, otel_user);
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
return Err(ToolError::Rejected("rejected by user".to_string()));
|
||||
}
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedAllowPrefix { .. }
|
||||
| ReviewDecision::ApprovedForSession => {}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -127,6 +127,7 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
|
||||
cwd,
|
||||
Some(reason),
|
||||
risk,
|
||||
None,
|
||||
)
|
||||
.await
|
||||
} else if user_explicitly_approved {
|
||||
|
||||
@@ -9,7 +9,7 @@ use crate::sandboxing::execute_env;
|
||||
use crate::tools::runtimes::build_command_spec;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ProvidesSandboxRetryData;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
@@ -32,7 +32,7 @@ pub struct ShellRequest {
|
||||
pub env: std::collections::HashMap<String, String>,
|
||||
pub with_escalated_permissions: Option<bool>,
|
||||
pub justification: Option<String>,
|
||||
pub approval_requirement: ApprovalRequirement,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
}
|
||||
|
||||
impl ProvidesSandboxRetryData for ShellRequest {
|
||||
@@ -107,22 +107,30 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
||||
Box::pin(async move {
|
||||
with_cached_approval(&session.services, key, move || async move {
|
||||
session
|
||||
.request_command_approval(turn, call_id, command, cwd, reason, risk)
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
req.exec_approval_requirement.allow_prefix().cloned(),
|
||||
)
|
||||
.await
|
||||
})
|
||||
.await
|
||||
})
|
||||
}
|
||||
|
||||
fn approval_requirement(&self, req: &ShellRequest) -> Option<ApprovalRequirement> {
|
||||
Some(req.approval_requirement.clone())
|
||||
fn exec_approval_requirement(&self, req: &ShellRequest) -> Option<ExecApprovalRequirement> {
|
||||
Some(req.exec_approval_requirement.clone())
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride {
|
||||
if req.with_escalated_permissions.unwrap_or(false)
|
||||
|| matches!(
|
||||
req.approval_requirement,
|
||||
ApprovalRequirement::Skip {
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
}
|
||||
)
|
||||
|
||||
@@ -10,7 +10,7 @@ use crate::exec::ExecExpiration;
|
||||
use crate::tools::runtimes::build_command_spec;
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::ApprovalCtx;
|
||||
use crate::tools::sandboxing::ApprovalRequirement;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use crate::tools::sandboxing::ProvidesSandboxRetryData;
|
||||
use crate::tools::sandboxing::SandboxAttempt;
|
||||
use crate::tools::sandboxing::SandboxOverride;
|
||||
@@ -36,7 +36,7 @@ pub struct UnifiedExecRequest {
|
||||
pub env: HashMap<String, String>,
|
||||
pub with_escalated_permissions: Option<bool>,
|
||||
pub justification: Option<String>,
|
||||
pub approval_requirement: ApprovalRequirement,
|
||||
pub exec_approval_requirement: ExecApprovalRequirement,
|
||||
}
|
||||
|
||||
impl ProvidesSandboxRetryData for UnifiedExecRequest {
|
||||
@@ -66,7 +66,7 @@ impl UnifiedExecRequest {
|
||||
env: HashMap<String, String>,
|
||||
with_escalated_permissions: Option<bool>,
|
||||
justification: Option<String>,
|
||||
approval_requirement: ApprovalRequirement,
|
||||
exec_approval_requirement: ExecApprovalRequirement,
|
||||
) -> Self {
|
||||
Self {
|
||||
command,
|
||||
@@ -74,7 +74,7 @@ impl UnifiedExecRequest {
|
||||
env,
|
||||
with_escalated_permissions,
|
||||
justification,
|
||||
approval_requirement,
|
||||
exec_approval_requirement,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -125,22 +125,33 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
||||
Box::pin(async move {
|
||||
with_cached_approval(&session.services, key, || async move {
|
||||
session
|
||||
.request_command_approval(turn, call_id, command, cwd, reason, risk)
|
||||
.request_command_approval(
|
||||
turn,
|
||||
call_id,
|
||||
command,
|
||||
cwd,
|
||||
reason,
|
||||
risk,
|
||||
req.exec_approval_requirement.allow_prefix().cloned(),
|
||||
)
|
||||
.await
|
||||
})
|
||||
.await
|
||||
})
|
||||
}
|
||||
|
||||
fn approval_requirement(&self, req: &UnifiedExecRequest) -> Option<ApprovalRequirement> {
|
||||
Some(req.approval_requirement.clone())
|
||||
fn exec_approval_requirement(
|
||||
&self,
|
||||
req: &UnifiedExecRequest,
|
||||
) -> Option<ExecApprovalRequirement> {
|
||||
Some(req.exec_approval_requirement.clone())
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride {
|
||||
if req.with_escalated_permissions.unwrap_or(false)
|
||||
|| matches!(
|
||||
req.approval_requirement,
|
||||
ApprovalRequirement::Skip {
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
}
|
||||
)
|
||||
|
||||
@@ -88,26 +88,42 @@ pub(crate) struct ApprovalCtx<'a> {
|
||||
|
||||
// Specifies what tool orchestrator should do with a given tool call.
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub(crate) enum ApprovalRequirement {
|
||||
pub(crate) enum ExecApprovalRequirement {
|
||||
/// No approval required for this tool call.
|
||||
Skip {
|
||||
/// The first attempt should skip sandboxing (e.g., when explicitly
|
||||
/// greenlit by policy).
|
||||
bypass_sandbox: bool,
|
||||
},
|
||||
/// Approval required for this tool call
|
||||
NeedsApproval { reason: Option<String> },
|
||||
/// Execution forbidden for this tool call
|
||||
/// Approval required for this tool call.
|
||||
NeedsApproval {
|
||||
reason: Option<String>,
|
||||
/// Prefix that can be whitelisted via execpolicy to skip future approvals for similar commands
|
||||
allow_prefix: Option<Vec<String>>,
|
||||
},
|
||||
/// Execution forbidden for this tool call.
|
||||
Forbidden { reason: String },
|
||||
}
|
||||
|
||||
impl ExecApprovalRequirement {
|
||||
pub fn allow_prefix(&self) -> Option<&Vec<String>> {
|
||||
match self {
|
||||
Self::NeedsApproval {
|
||||
allow_prefix: Some(prefix),
|
||||
..
|
||||
} => Some(prefix),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// - Never, OnFailure: do not ask
|
||||
/// - OnRequest: ask unless sandbox policy is DangerFullAccess
|
||||
/// - UnlessTrusted: always ask
|
||||
pub(crate) fn default_approval_requirement(
|
||||
pub(crate) fn default_exec_approval_requirement(
|
||||
policy: AskForApproval,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
) -> ApprovalRequirement {
|
||||
) -> ExecApprovalRequirement {
|
||||
let needs_approval = match policy {
|
||||
AskForApproval::Never | AskForApproval::OnFailure => false,
|
||||
AskForApproval::OnRequest => !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess),
|
||||
@@ -115,9 +131,12 @@ pub(crate) fn default_approval_requirement(
|
||||
};
|
||||
|
||||
if needs_approval {
|
||||
ApprovalRequirement::NeedsApproval { reason: None }
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
allow_prefix: None,
|
||||
}
|
||||
} else {
|
||||
ApprovalRequirement::Skip {
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
}
|
||||
}
|
||||
@@ -149,10 +168,9 @@ pub(crate) trait Approvable<Req> {
|
||||
matches!(policy, AskForApproval::Never)
|
||||
}
|
||||
|
||||
/// Override the default approval requirement. Return `Some(_)` to specify
|
||||
/// a custom requirement, or `None` to fall back to
|
||||
/// policy-based default.
|
||||
fn approval_requirement(&self, _req: &Req) -> Option<ApprovalRequirement> {
|
||||
/// Return `Some(_)` to specify a custom exec approval requirement, or `None`
|
||||
/// to fall back to policy-based default.
|
||||
fn exec_approval_requirement(&self, _req: &Req) -> Option<ExecApprovalRequirement> {
|
||||
None
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user