proposing execpolicy amendment when prompting due to sandbox denial (#7653)
Currently, we only show the “don’t ask again for commands that start with…” option when a command is immediately flagged as needing approval. However, there is another case where we ask for approval: When a command is initially auto-approved to run within sandbox, but it fails to run inside sandbox, we would like to attempt to retry running outside of sandbox. This will require a prompt to the user. This PR addresses this latter case
This commit is contained in:
parent
cfda44b98b
commit
c2bdee0946
4 changed files with 121 additions and 35 deletions
|
|
@ -34,6 +34,13 @@ const POLICY_DIR_NAME: &str = "policy";
|
|||
const POLICY_EXTENSION: &str = "codexpolicy";
|
||||
const DEFAULT_POLICY_FILE: &str = "default.codexpolicy";
|
||||
|
||||
fn is_policy_match(rule_match: &RuleMatch) -> bool {
|
||||
match rule_match {
|
||||
RuleMatch::PrefixRuleMatch { .. } => true,
|
||||
RuleMatch::HeuristicsRuleMatch { .. } => false,
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Error)]
|
||||
pub enum ExecPolicyError {
|
||||
#[error("failed to read execpolicy files from {dir}: {source}")]
|
||||
|
|
@ -147,49 +154,62 @@ pub(crate) async fn append_execpolicy_amendment_and_update(
|
|||
Ok(())
|
||||
}
|
||||
|
||||
/// 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:
|
||||
/// Derive a proposed execpolicy amendment when a command requires user approval
|
||||
/// - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement.
|
||||
/// - Otherwise return the first heuristics Prompt.
|
||||
/// - 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 {
|
||||
fn try_derive_execpolicy_amendment_for_prompt_rules(
|
||||
matched_rules: &[RuleMatch],
|
||||
) -> Option<ExecPolicyAmendment> {
|
||||
if matched_rules
|
||||
.iter()
|
||||
.any(|rule_match| is_policy_match(rule_match) && rule_match.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;
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
matched_rules
|
||||
.iter()
|
||||
.find_map(|rule_match| match rule_match {
|
||||
RuleMatch::HeuristicsRuleMatch {
|
||||
command,
|
||||
decision: Decision::Prompt,
|
||||
} => Some(ExecPolicyAmendment::from(command.clone())),
|
||||
_ => None,
|
||||
})
|
||||
}
|
||||
|
||||
/// - Note: we only use this amendment when the command fails to run in sandbox and codex prompts the user to run outside the sandbox
|
||||
/// - The purpose of this amendment is to bypass sandbox for similar commands in the future
|
||||
/// - If any execpolicy rule matches, return None, because we would already be running command outside the sandbox
|
||||
fn try_derive_execpolicy_amendment_for_allow_rules(
|
||||
matched_rules: &[RuleMatch],
|
||||
) -> Option<ExecPolicyAmendment> {
|
||||
if matched_rules.iter().any(is_policy_match) {
|
||||
return None;
|
||||
}
|
||||
|
||||
first_prompt_from_heuristics.map(ExecPolicyAmendment::from)
|
||||
matched_rules
|
||||
.iter()
|
||||
.find_map(|rule_match| match rule_match {
|
||||
RuleMatch::HeuristicsRuleMatch {
|
||||
command,
|
||||
decision: Decision::Allow,
|
||||
} => Some(ExecPolicyAmendment::from(command.clone())),
|
||||
_ => 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
|
||||
{
|
||||
if is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt {
|
||||
Some(PROMPT_REASON.to_string())
|
||||
} else {
|
||||
None
|
||||
|
|
@ -215,10 +235,6 @@ pub(crate) async fn create_exec_approval_requirement_for_command(
|
|||
};
|
||||
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.decision {
|
||||
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
|
||||
|
|
@ -233,7 +249,7 @@ pub(crate) async fn create_exec_approval_requirement_for_command(
|
|||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: derive_prompt_reason(&evaluation),
|
||||
proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) {
|
||||
proposed_execpolicy_amendment(&evaluation)
|
||||
try_derive_execpolicy_amendment_for_prompt_rules(&evaluation.matched_rules)
|
||||
} else {
|
||||
None
|
||||
},
|
||||
|
|
@ -241,7 +257,15 @@ pub(crate) async fn create_exec_approval_requirement_for_command(
|
|||
}
|
||||
}
|
||||
Decision::Allow => ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: has_policy_allow,
|
||||
// Bypass sandbox if execpolicy allows the command
|
||||
bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| {
|
||||
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
|
||||
}),
|
||||
proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) {
|
||||
try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules)
|
||||
} else {
|
||||
None
|
||||
},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
|
@ -730,4 +754,56 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
|||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() {
|
||||
let command = vec!["echo".to_string(), "safe".to_string()];
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
&Arc::new(RwLock::new(Policy::empty())),
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
AskForApproval::OnRequest,
|
||||
&SandboxPolicy::ReadOnly,
|
||||
SandboxPermissions::UseDefault,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn proposed_execpolicy_amendment_is_suppressed_when_policy_matches_allow() {
|
||||
let policy_src = r#"prefix_rule(pattern=["echo"], 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!["echo".to_string(), "safe".to_string()];
|
||||
|
||||
let requirement = create_exec_approval_requirement_for_command(
|
||||
&policy,
|
||||
&Features::with_defaults(),
|
||||
&command,
|
||||
AskForApproval::OnRequest,
|
||||
&SandboxPolicy::ReadOnly,
|
||||
SandboxPermissions::UseDefault,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -133,7 +133,8 @@ impl Approvable<ShellRequest> for ShellRuntime {
|
|||
|| matches!(
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
bypass_sandbox: true,
|
||||
..
|
||||
}
|
||||
)
|
||||
{
|
||||
|
|
|
|||
|
|
@ -154,7 +154,8 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
|
|||
|| matches!(
|
||||
req.exec_approval_requirement,
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true
|
||||
bypass_sandbox: true,
|
||||
..
|
||||
}
|
||||
)
|
||||
{
|
||||
|
|
|
|||
|
|
@ -95,6 +95,9 @@ pub(crate) enum ExecApprovalRequirement {
|
|||
/// The first attempt should skip sandboxing (e.g., when explicitly
|
||||
/// greenlit by policy).
|
||||
bypass_sandbox: bool,
|
||||
/// Proposed execpolicy amendment to skip future approvals for similar commands
|
||||
/// Only applies if the command fails to run in sandbox and codex prompts the user to run outside the sandbox.
|
||||
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
|
||||
},
|
||||
/// Approval required for this tool call.
|
||||
NeedsApproval {
|
||||
|
|
@ -114,6 +117,10 @@ impl ExecApprovalRequirement {
|
|||
proposed_execpolicy_amendment: Some(prefix),
|
||||
..
|
||||
} => Some(prefix),
|
||||
Self::Skip {
|
||||
proposed_execpolicy_amendment: Some(prefix),
|
||||
..
|
||||
} => Some(prefix),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
|
@ -140,6 +147,7 @@ pub(crate) fn default_exec_approval_requirement(
|
|||
} else {
|
||||
ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue