mirror of
https://github.com/openai/codex.git
synced 2026-11-07 20:30:33 +00:00
Allow dangerous-command review when sandbox approvals are disabled (#59120)
## Why Setting granular `sandbox_approval` to `false` also blocked approval requests for dangerous commands such as `rm -f`, even when they requested no additional permissions. Reviewing a dangerous command should remain possible within the existing permission profile. ## What changed Allow heuristic dangerous-command approval requests with `sandbox_approval = false` when execution stays within current permissions. Do not propose reusable execution-policy amendments for these approvals, since reusable allow rules can bypass the sandbox on later calls. Keep escalation requests, explicit policy rules, and Windows execution without required sandbox protection subject to their existing approval restrictions. Clarify the meaning of `sandbox_approval` in the protocol and generated schemas. ## Testing Add coverage for POSIX and Windows approval decisions, approval and denial of deletion commands, preservation of read-only restrictions, and separate dangerous-command approval after turn-scoped or session-scoped write grants. GitOrigin-RevId: 90adc137c7b7d3f3ee68efb1f6bea531229e7834
This commit is contained in:
jif
@@ -429,20 +429,35 @@ impl ExecPolicyManager {
|
||||
let prompt_is_rule = evaluation.matched_rules.iter().any(|rule_match| {
|
||||
is_policy_match(rule_match) && rule_match.decision() == evaluation.decision
|
||||
});
|
||||
match prompt_is_rejected_by_policy(approval_policy, prompt_is_rule) {
|
||||
let dangerous_command = dangerous_command_match_for_heuristics(
|
||||
&evaluation,
|
||||
Decision::Prompt,
|
||||
command_origin,
|
||||
command_platform,
|
||||
);
|
||||
// Reviewing a dangerous command does not grant additional permissions.
|
||||
// Keep explicit rules, escalation requests, and unavailable Windows
|
||||
// sandbox protection on their existing approval-policy paths.
|
||||
let allow_dangerous_command_prompt = matches!(
|
||||
approval_policy,
|
||||
AskForApproval::Granular(config) if !config.sandbox_approval
|
||||
) && !prompt_is_rule
|
||||
&& dangerous_command.is_some()
|
||||
&& !sandbox_permissions.requests_sandbox_override()
|
||||
&& !(command_platform == DangerousCommandPlatform::Windows
|
||||
&& windows_sandbox_level == WindowsSandboxLevel::Disabled
|
||||
&& profile_has_managed_filesystem_restrictions(&permission_profile));
|
||||
let rejection = if allow_dangerous_command_prompt {
|
||||
None
|
||||
} else {
|
||||
prompt_is_rejected_by_policy(approval_policy, prompt_is_rule)
|
||||
};
|
||||
match rejection {
|
||||
Some(reason) if prompt_is_rule => ExecApprovalRequirement::Forbidden {
|
||||
reason: reason.to_string(),
|
||||
},
|
||||
Some(reason) => ExecApprovalRequirement::Forbidden {
|
||||
reason: derive_rejected_prompt_reason(
|
||||
reason,
|
||||
dangerous_command_match_for_heuristics(
|
||||
&evaluation,
|
||||
Decision::Prompt,
|
||||
command_origin,
|
||||
command_platform,
|
||||
),
|
||||
),
|
||||
reason: derive_rejected_prompt_reason(reason, dangerous_command),
|
||||
},
|
||||
None if evaluation.decision == Decision::PromptUser => {
|
||||
ExecApprovalRequirement::NeedsHumanApproval {
|
||||
@@ -451,15 +466,21 @@ impl ExecPolicyManager {
|
||||
}
|
||||
None => ExecApprovalRequirement::NeedsApproval {
|
||||
reason: derive_prompt_reason(command, &evaluation),
|
||||
proposed_execpolicy_amendment: requested_amendment.or_else(|| {
|
||||
if auto_amendment_allowed {
|
||||
try_derive_execpolicy_amendment_for_prompt_rules(
|
||||
&evaluation.matched_rules,
|
||||
)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}),
|
||||
// A reusable allow rule can bypass the sandbox on later calls.
|
||||
// This approval only authorizes execution with current permissions.
|
||||
proposed_execpolicy_amendment: if allow_dangerous_command_prompt {
|
||||
None
|
||||
} else {
|
||||
requested_amendment.or_else(|| {
|
||||
if auto_amendment_allowed {
|
||||
try_derive_execpolicy_amendment_for_prompt_rules(
|
||||
&evaluation.matched_rules,
|
||||
)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1418,6 +1418,75 @@ async fn exec_approval_requirement_rejects_unmatched_sandbox_escalation_when_gra
|
||||
.await;
|
||||
}
|
||||
|
||||
#[test_case::test_case(DangerousCommandPlatform::Posix, WindowsSandboxLevel::RestrictedToken; "posix")]
|
||||
#[test_case::test_case(DangerousCommandPlatform::Windows, WindowsSandboxLevel::RestrictedToken; "windows")]
|
||||
#[test_case::test_case(DangerousCommandPlatform::Windows, WindowsSandboxLevel::Disabled; "windows_without_sandbox")]
|
||||
#[tokio::test]
|
||||
async fn granular_dangerous_command_approval_does_not_authorize_escalation(
|
||||
platform: DangerousCommandPlatform,
|
||||
windows_sandbox_level: WindowsSandboxLevel,
|
||||
) {
|
||||
let command = match platform {
|
||||
DangerousCommandPlatform::Posix => vec!["rm", "-f", "recording.mp4"],
|
||||
DangerousCommandPlatform::Windows => vec![
|
||||
"powershell.exe",
|
||||
"-Command",
|
||||
"Remove-Item -Force recording.mp4",
|
||||
],
|
||||
}
|
||||
.into_iter()
|
||||
.map(str::to_owned)
|
||||
.collect::<Vec<_>>();
|
||||
for sandbox_permissions in [
|
||||
SandboxPermissions::UseDefault,
|
||||
SandboxPermissions::WithAdditionalPermissions,
|
||||
SandboxPermissions::RequireEscalated,
|
||||
] {
|
||||
let requirement = ExecPolicyManager::default()
|
||||
.create_exec_approval_requirement_for_command_platform(
|
||||
ExecApprovalRequest {
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::Granular(GranularApprovalConfig {
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
skill_approval: false,
|
||||
request_permissions: true,
|
||||
mcp_elicitations: true,
|
||||
}),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
environment_policy: None,
|
||||
windows_sandbox_level,
|
||||
sandbox_permissions,
|
||||
prefix_rule: None,
|
||||
allow_prefix_rules: AllowPrefixRules::Honor,
|
||||
},
|
||||
platform,
|
||||
)
|
||||
.await;
|
||||
let expected = if sandbox_permissions == SandboxPermissions::UseDefault
|
||||
&& windows_sandbox_level == WindowsSandboxLevel::RestrictedToken
|
||||
{
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
} else {
|
||||
ExecApprovalRequirement::Forbidden {
|
||||
reason: match platform {
|
||||
DangerousCommandPlatform::Posix => {
|
||||
"rm -f style commands are not permitted. Use a safer approach".to_string()
|
||||
}
|
||||
DangerousCommandPlatform::Windows => REJECT_SANDBOX_APPROVAL_REASON.to_string(),
|
||||
},
|
||||
}
|
||||
};
|
||||
assert_eq!(
|
||||
requirement, expected,
|
||||
"{platform:?}: {sandbox_permissions:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn other_danger_preserves_rejected_prompt_reason() {
|
||||
assert_eq!(
|
||||
@@ -1497,7 +1566,7 @@ async fn forced_rm_preserves_rule_rejection_when_granular_rules_are_disabled() {
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::Granular(GranularApprovalConfig {
|
||||
sandbox_approval: true,
|
||||
sandbox_approval: false,
|
||||
rules: false,
|
||||
skill_approval: true,
|
||||
request_permissions: true,
|
||||
@@ -1506,7 +1575,7 @@ async fn forced_rm_preserves_rule_rejection_when_granular_rules_are_disabled() {
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
environment_policy: None,
|
||||
windows_sandbox_level: WindowsSandboxLevel::Disabled,
|
||||
sandbox_permissions: SandboxPermissions::RequireEscalated,
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prefix_rule: None,
|
||||
allow_prefix_rules: AllowPrefixRules::Honor,
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user