agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

Refactor execpolicy fallback evaluation (#7544)

Browse Source 
## Refactor of the `execpolicy` crate

To illustrate why we need this refactor, consider an agent attempting to
run `apple | rm -rf ./`. Suppose `apple` is allowed by `execpolicy`.
Before this PR, `execpolicy` would consider `apple` and `pear` and only
render one rule match: `Allow`. We would skip any heuristics checks on
`rm -rf ./` and immediately approve `apple | rm -rf ./` to run.

To fix this, we now thread a `fallback` evaluation function into
`execpolicy` that runs when no `execpolicy` rules match a given command.
In our example, we would run `fallback` on `rm -rf ./` and prevent
`apple | rm -rf ./` from being run without approval.
This commit is contained in:
zhao-oai
2025-12-03 23:39:48 -08:00
committed by GitHub
parent e925a380dc
commit 3d35cb4619
27 changed files with 538 additions and 257 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/apply_patch.rs
+1 -1
View File
@@ -71,7 +71,7 @@ pub(crate) async fn apply_patch(
.await;
match rx_approve.await.unwrap_or_default() {
ReviewDecision::Approved
| ReviewDecision::ApprovedAllowPrefix { .. }
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {
InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
action,
codex-rs/core/src/codex.rs
+18 -15
View File
@@ -25,6 +25,7 @@ use crate::util::error_or_panic;
use async_channel::Receiver;
use async_channel::Sender;
use codex_protocol::ConversationId;
use codex_protocol::approvals::ExecPolicyAmendment;
use codex_protocol::items::TurnItem;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::HasLegacyEvent;
@@ -871,13 +872,11 @@ impl Session {
.await
}
/// Adds a prefix rule to the exec policy
///
/// This mutates the in-memory execpolicy so the current conversation can use the new
/// prefix and persists the change in default.execpolicy so new conversations will also allow the new prefix.
pub(crate) async fn persist_command_allow_prefix(
/// Adds an execpolicy amendment to both the in-memory and on-disk policies so future
/// commands can use the newly approved prefix.
pub(crate) async fn persist_execpolicy_amendment(
&self,
prefix: &[String],
amendment: &ExecPolicyAmendment,
) -> Result<(), ExecPolicyUpdateError> {
let features = self.features.clone();
let (codex_home, current_policy) = {
@@ -897,10 +896,10 @@ impl Session {
return Err(ExecPolicyUpdateError::FeatureDisabled);
}
crate::exec_policy::append_allow_prefix_rule_and_update(
crate::exec_policy::append_execpolicy_amendment_and_update(
&codex_home,
&current_policy,
prefix,
&amendment.command,
)
.await?;
@@ -921,7 +920,7 @@ impl Session {
cwd: PathBuf,
reason: Option<String>,
risk: Option<SandboxCommandAssessment>,
allow_prefix: Option<Vec<String>>,
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
) -> ReviewDecision {
let sub_id = turn_context.sub_id.clone();
// Add the tx_approve callback to the map before sending the request.
@@ -949,7 +948,7 @@ impl Session {
cwd,
reason,
risk,
allow_prefix,
proposed_execpolicy_amendment,
parsed_cmd,
});
self.send_event(turn_context, event).await;
@@ -1672,13 +1671,17 @@ mod handlers {
}
}
/// Propagate a user's exec approval decision to the session
/// Also optionally whitelists command in execpolicy
/// Propagate a user's exec approval decision to the session.
/// Also optionally applies an execpolicy amendment.
pub async fn exec_approval(sess: &Arc<Session>, id: String, decision: ReviewDecision) {
if let ReviewDecision::ApprovedAllowPrefix { allow_prefix } = &decision
&& let Err(err) = sess.persist_command_allow_prefix(allow_prefix).await
if let ReviewDecision::ApprovedExecpolicyAmendment {
proposed_execpolicy_amendment,
} = &decision
&& let Err(err) = sess
.persist_execpolicy_amendment(proposed_execpolicy_amendment)
.await
{
let message = format!("Failed to update execpolicy allow list: {err}");
let message = format!("Failed to apply execpolicy amendment: {err}");
tracing::warn!("{message}");
let warning = EventMsg::Warning(WarningEvent { message });
sess.send_event_raw(Event {
codex-rs/core/src/codex_delegate.rs
+1 -1
View File
@@ -281,7 +281,7 @@ async fn handle_exec_approval(
event.cwd,
event.reason,
event.risk,
event.allow_prefix,
event.proposed_execpolicy_amendment,
);
let decision = await_approval_with_cancel(
approval_fut,
codex-rs/core/src/exec_policy.rs
+207 -82
View File
@@ -10,7 +10,9 @@ use codex_execpolicy::Error as ExecPolicyRuleError;
use codex_execpolicy::Evaluation;
use codex_execpolicy::Policy;
use codex_execpolicy::PolicyParser;
use codex_execpolicy::RuleMatch;
use codex_execpolicy::blocking_append_allow_prefix_rule;
use codex_protocol::approvals::ExecPolicyAmendment;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::SandboxPolicy;
use thiserror::Error;
@@ -25,6 +27,8 @@ use crate::sandboxing::SandboxPermissions;
use crate::tools::sandboxing::ExecApprovalRequirement;
const FORBIDDEN_REASON: &str = "execpolicy forbids this command";
const PROMPT_CONFLICT_REASON: &str =
"execpolicy requires approval for this command, but AskForApproval is set to Never";
const PROMPT_REASON: &str = "execpolicy requires approval for this command";
const POLICY_DIR_NAME: &str = "policy";
const POLICY_EXTENSION: &str = "codexpolicy";
@@ -112,7 +116,7 @@ pub(crate) fn default_policy_path(codex_home: &Path) -> PathBuf {
codex_home.join(POLICY_DIR_NAME).join(DEFAULT_POLICY_FILE)
}
pub(crate) async fn append_allow_prefix_rule_and_update(
pub(crate) async fn append_execpolicy_amendment_and_update(
codex_home: &Path,
current_policy: &Arc<RwLock<Policy>>,
prefix: &[String],
@@ -139,45 +143,54 @@ pub(crate) async fn append_allow_prefix_rule_and_update(
Ok(())
}
fn requirement_from_decision(
decision: Decision,
approval_policy: AskForApproval,
) -> ExecApprovalRequirement {
match decision {
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
reason: FORBIDDEN_REASON.to_string(),
},
Decision::Prompt => {
let reason = PROMPT_REASON.to_string();
if matches!(approval_policy, AskForApproval::Never) {
ExecApprovalRequirement::Forbidden { reason }
} else {
ExecApprovalRequirement::NeedsApproval {
reason: Some(reason),
allow_prefix: None,
/// Returns a proposed execpolicy amendment only when heuristics caused
/// the prompt decision, so we can offer to apply that amendment for future runs.
///
/// The amendment uses the first command heuristics marked as `Prompt`. If any explicit
/// execpolicy rule also prompts, we return `None` because applying the amendment would not
/// skip that policy requirement.
///
/// Examples:
/// - execpolicy: empty. Command: `["python"]`. Heuristics prompt -> `Some(vec!["python"])`.
/// - execpolicy: empty. Command: `["bash", "-c", "cd /some/folder && prog1 --option1 arg1 && prog2 --option2 arg2"]`.
/// Parsed commands include `cd /some/folder`, `prog1 --option1 arg1`, and `prog2 --option2 arg2`. If heuristics allow `cd` but prompt
/// on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`.
/// - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above,
/// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"].
fn proposed_execpolicy_amendment(evaluation: &Evaluation) -> Option<ExecPolicyAmendment> {
if evaluation.decision != Decision::Prompt {
return None;
}
let mut first_prompt_from_heuristics: Option<Vec<String>> = None;
for rule_match in &evaluation.matched_rules {
match rule_match {
RuleMatch::HeuristicsRuleMatch { command, decision } => {
if *decision == Decision::Prompt && first_prompt_from_heuristics.is_none() {
first_prompt_from_heuristics = Some(command.clone());
}
}
_ if rule_match.decision() == Decision::Prompt => {
return None;
}
_ => {}
}
Decision::Allow => ExecApprovalRequirement::Skip {
bypass_sandbox: true,
},
}
first_prompt_from_heuristics.map(ExecPolicyAmendment::from)
}
/// Return an allow-prefix option when a single plain command needs approval without
/// any matching policy rule. We only surface the prefix opt-in when execpolicy did
/// not already drive the decision (NoMatch) and when the command is a single
/// unrolled command (multi-part scripts shouldn’t be whitelisted via prefix) and
/// when execpolicy feature is enabled.
fn allow_prefix_if_applicable(
commands: &[Vec<String>],
features: &Features,
) -> Option<Vec<String>> {
if features.enabled(Feature::ExecPolicy) && commands.len() == 1 {
Some(commands[0].clone())
} else {
None
}
/// Only return PROMPT_REASON when an execpolicy rule drove the prompt decision.
fn derive_prompt_reason(evaluation: &Evaluation) -> Option<String> {
evaluation.matched_rules.iter().find_map(|rule_match| {
if !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })
&& rule_match.decision() == Decision::Prompt
{
Some(PROMPT_REASON.to_string())
} else {
None
}
})
}
pub(crate) async fn create_exec_approval_requirement_for_command(
@@ -189,27 +202,43 @@ pub(crate) async fn create_exec_approval_requirement_for_command(
sandbox_permissions: SandboxPermissions,
) -> ExecApprovalRequirement {
let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]);
let evaluation = exec_policy.read().await.check_multiple(commands.iter());
let heuristics_fallback = |cmd: &[String]| {
if requires_initial_appoval(approval_policy, sandbox_policy, cmd, sandbox_permissions) {
Decision::Prompt
} else {
Decision::Allow
}
};
let policy = exec_policy.read().await;
let evaluation = policy.check_multiple(commands.iter(), &heuristics_fallback);
let has_policy_allow = evaluation.matched_rules.iter().any(|rule_match| {
!matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })
&& rule_match.decision() == Decision::Allow
});
match evaluation {
Evaluation::Match { decision, .. } => requirement_from_decision(decision, approval_policy),
Evaluation::NoMatch { .. } => {
if requires_initial_appoval(
approval_policy,
sandbox_policy,
command,
sandbox_permissions,
) {
ExecApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: allow_prefix_if_applicable(&commands, features),
match evaluation.decision {
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
reason: FORBIDDEN_REASON.to_string(),
},
Decision::Prompt => {
if matches!(approval_policy, AskForApproval::Never) {
ExecApprovalRequirement::Forbidden {
reason: PROMPT_CONFLICT_REASON.to_string(),
}
} else {
ExecApprovalRequirement::Skip {
bypass_sandbox: false,
ExecApprovalRequirement::NeedsApproval {
reason: derive_prompt_reason(&evaluation),
proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) {
proposed_execpolicy_amendment(&evaluation)
} else {
None
},
}
}
}
Decision::Allow => ExecApprovalRequirement::Skip {
bypass_sandbox: has_policy_allow,
},
}
}
@@ -282,10 +311,19 @@ mod tests {
.expect("policy result");
let commands = [vec!["rm".to_string()]];
assert!(matches!(
policy.read().await.check_multiple(commands.iter()),
Evaluation::NoMatch { .. }
));
assert_eq!(
Evaluation {
decision: Decision::Allow,
matched_rules: vec![RuleMatch::HeuristicsRuleMatch {
command: vec!["rm".to_string()],
decision: Decision::Allow
}],
},
policy
.read()
.await
.check_multiple(commands.iter(), &|_| Decision::Allow)
);
assert!(!temp_dir.path().join(POLICY_DIR_NAME).exists());
}
@@ -316,10 +354,19 @@ mod tests {
.await
.expect("policy result");
let command = [vec!["rm".to_string()]];
assert!(matches!(
policy.read().await.check_multiple(command.iter()),
Evaluation::Match { .. }
));
assert_eq!(
Evaluation {
decision: Decision::Forbidden,
matched_rules: vec![RuleMatch::PrefixRuleMatch {
matched_prefix: vec!["rm".to_string()],
decision: Decision::Forbidden
}],
},
policy
.read()
.await
.check_multiple(command.iter(), &|_| Decision::Allow)
);
}
#[tokio::test]
@@ -335,10 +382,19 @@ mod tests {
.await
.expect("policy result");
let command = [vec!["ls".to_string()]];
assert!(matches!(
policy.read().await.check_multiple(command.iter()),
Evaluation::NoMatch { .. }
));
assert_eq!(
Evaluation {
decision: Decision::Allow,
matched_rules: vec![RuleMatch::HeuristicsRuleMatch {
command: vec!["ls".to_string()],
decision: Decision::Allow
}],
},
policy
.read()
.await
.check_multiple(command.iter(), &|_| Decision::Allow)
);
}
#[tokio::test]
@@ -400,7 +456,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: Some(PROMPT_REASON.to_string()),
allow_prefix: None,
proposed_execpolicy_amendment: None,
}
);
}
@@ -428,7 +484,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
assert_eq!(
requirement,
ExecApprovalRequirement::Forbidden {
reason: PROMPT_REASON.to_string()
reason: PROMPT_CONFLICT_REASON.to_string()
}
);
}
@@ -452,29 +508,61 @@ prefix_rule(pattern=["rm"], decision="forbidden")
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: Some(command)
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command))
}
);
}
#[tokio::test]
async fn append_allow_prefix_rule_updates_policy_and_file() {
async fn heuristics_apply_when_other_commands_match_policy() {
let policy_src = r#"prefix_rule(pattern=["apple"], decision="allow")"#;
let mut parser = PolicyParser::new();
parser
.parse("test.codexpolicy", policy_src)
.expect("parse policy");
let policy = Arc::new(RwLock::new(parser.build()));
let command = vec![
"bash".to_string(),
"-lc".to_string(),
"apple | orange".to_string(),
];
assert_eq!(
create_exec_approval_requirement_for_command(
&policy,
&Features::with_defaults(),
&command,
AskForApproval::UnlessTrusted,
&SandboxPolicy::DangerFullAccess,
SandboxPermissions::UseDefault,
)
.await,
ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"orange".to_string()
]))
}
);
}
#[tokio::test]
async fn append_execpolicy_amendment_updates_policy_and_file() {
let codex_home = tempdir().expect("create temp dir");
let current_policy = Arc::new(RwLock::new(Policy::empty()));
let prefix = vec!["echo".to_string(), "hello".to_string()];
append_allow_prefix_rule_and_update(codex_home.path(), &current_policy, &prefix)
append_execpolicy_amendment_and_update(codex_home.path(), &current_policy, &prefix)
.await
.expect("update policy");
let evaluation = current_policy.read().await.check(&[
"echo".to_string(),
"hello".to_string(),
"world".to_string(),
]);
let evaluation = current_policy.read().await.check(
&["echo".to_string(), "hello".to_string(), "world".to_string()],
&|_| Decision::Allow,
);
assert!(matches!(
evaluation,
Evaluation::Match {
Evaluation {
decision: Decision::Allow,
..
}
@@ -490,12 +578,12 @@ prefix_rule(pattern=["rm"], decision="forbidden")
}
#[tokio::test]
async fn append_allow_prefix_rule_rejects_empty_prefix() {
async fn append_execpolicy_amendment_rejects_empty_prefix() {
let codex_home = tempdir().expect("create temp dir");
let current_policy = Arc::new(RwLock::new(Policy::empty()));
let result =
append_allow_prefix_rule_and_update(codex_home.path(), &current_policy, &[]).await;
append_execpolicy_amendment_and_update(codex_home.path(), &current_policy, &[]).await;
assert!(matches!(
result,
@@ -507,7 +595,7 @@ prefix_rule(pattern=["rm"], decision="forbidden")
}
#[tokio::test]
async fn allow_prefix_is_present_for_single_command_without_policy_match() {
async fn proposed_execpolicy_amendment_is_present_for_single_command_without_policy_match() {
let command = vec!["cargo".to_string(), "build".to_string()];
let empty_policy = Arc::new(RwLock::new(Policy::empty()));
@@ -525,13 +613,13 @@ prefix_rule(pattern=["rm"], decision="forbidden")
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: Some(command)
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command))
}
);
}
#[tokio::test]
async fn allow_prefix_is_disabled_when_execpolicy_feature_disabled() {
async fn proposed_execpolicy_amendment_is_disabled_when_execpolicy_feature_disabled() {
let command = vec!["cargo".to_string(), "build".to_string()];
let mut features = Features::with_defaults();
@@ -551,13 +639,13 @@ prefix_rule(pattern=["rm"], decision="forbidden")
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
proposed_execpolicy_amendment: None,
}
);
}
#[tokio::test]
async fn allow_prefix_is_omitted_when_policy_prompts() {
async fn proposed_execpolicy_amendment_is_omitted_when_policy_prompts() {
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
let mut parser = PolicyParser::new();
parser
@@ -580,13 +668,13 @@ prefix_rule(pattern=["rm"], decision="forbidden")
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: Some(PROMPT_REASON.to_string()),
allow_prefix: None,
proposed_execpolicy_amendment: None,
}
);
}
#[tokio::test]
async fn allow_prefix_is_omitted_for_multi_command_scripts() {
async fn proposed_execpolicy_amendment_is_present_for_multi_command_scripts() {
let command = vec![
"bash".to_string(),
"-lc".to_string(),
@@ -606,7 +694,44 @@ prefix_rule(pattern=["rm"], decision="forbidden")
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"cargo".to_string(),
"build".to_string()
])),
}
);
}
#[tokio::test]
async fn proposed_execpolicy_amendment_uses_first_no_match_in_multi_command_scripts() {
let policy_src = r#"prefix_rule(pattern=["cat"], decision="allow")"#;
let mut parser = PolicyParser::new();
parser
.parse("test.codexpolicy", policy_src)
.expect("parse policy");
let policy = Arc::new(RwLock::new(parser.build()));
let command = vec![
"bash".to_string(),
"-lc".to_string(),
"cat && apple".to_string(),
];
assert_eq!(
create_exec_approval_requirement_for_command(
&policy,
&Features::with_defaults(),
&command,
AskForApproval::UnlessTrusted,
&SandboxPolicy::ReadOnly,
SandboxPermissions::UseDefault,
)
.await,
ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
"apple".to_string()
])),
}
);
}
codex-rs/core/src/tools/orchestrator.rs
+2 -2
View File
@@ -95,7 +95,7 @@ impl ToolOrchestrator {
return Err(ToolError::Rejected("rejected by user".to_string()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedAllowPrefix { .. }
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {}
}
already_approved = true;
@@ -178,7 +178,7 @@ impl ToolOrchestrator {
return Err(ToolError::Rejected("rejected by user".to_string()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedAllowPrefix { .. }
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {}
}
}
codex-rs/core/src/tools/runtimes/shell.rs
+3 -1
View File
@@ -114,7 +114,9 @@ impl Approvable<ShellRequest> for ShellRuntime {
cwd,
reason,
risk,
req.exec_approval_requirement.allow_prefix().cloned(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
)
.await
})
codex-rs/core/src/tools/runtimes/unified_exec.rs
+3 -1
View File
@@ -132,7 +132,9 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
cwd,
reason,
risk,
req.exec_approval_requirement.allow_prefix().cloned(),
req.exec_approval_requirement
.proposed_execpolicy_amendment()
.cloned(),
)
.await
})
codex-rs/core/src/tools/sandboxing.rs
+7 -5
View File
@@ -13,6 +13,7 @@ use crate::sandboxing::CommandSpec;
use crate::sandboxing::SandboxManager;
use crate::sandboxing::SandboxTransformError;
use crate::state::SessionServices;
use codex_protocol::approvals::ExecPolicyAmendment;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::ReviewDecision;
use std::collections::HashMap;
@@ -98,18 +99,19 @@ pub(crate) enum ExecApprovalRequirement {
/// 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>>,
/// Proposed execpolicy amendment to skip future approvals for similar commands
/// See core/src/exec_policy.rs for more details on how proposed_execpolicy_amendment is determined.
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
},
/// Execution forbidden for this tool call.
Forbidden { reason: String },
}
impl ExecApprovalRequirement {
pub fn allow_prefix(&self) -> Option<&Vec<String>> {
pub fn proposed_execpolicy_amendment(&self) -> Option<&ExecPolicyAmendment> {
match self {
Self::NeedsApproval {
allow_prefix: Some(prefix),
proposed_execpolicy_amendment: Some(prefix),
..
} => Some(prefix),
_ => None,
@@ -133,7 +135,7 @@ pub(crate) fn default_exec_approval_requirement(
if needs_approval {
ExecApprovalRequirement::NeedsApproval {
reason: None,
allow_prefix: None,
proposed_execpolicy_amendment: None,
}
} else {
ExecApprovalRequirement::Skip {