Merge PR #31019 follow-ups into model shell policy

# Conflicts:
#	codex-rs/core/src/exec_policy.rs
This commit is contained in:
Chris Bookholt
2026-07-03 15:55:13 -07:00
8 changed files with 467 additions and 26 deletions
+22 -7
View File
@@ -967,7 +967,7 @@ impl ExecPolicyManager {
matched_prefix,
decision: Decision::Allow,
..
} if matched_prefix.len() == outer.len()
} if matched_prefix.as_slice() == outer
)
})
});
@@ -1263,7 +1263,7 @@ fn create_untrusted_wrapper_approval_requirement(
matched_prefix,
decision: Decision::Allow,
..
} if matched_prefix.len() == candidate.argv.len()
} if matched_prefix.as_slice() == candidate.argv
)
});
every_candidate_explicit_allow &= raw_full_outer_allow;
@@ -1281,7 +1281,6 @@ fn create_untrusted_wrapper_approval_requirement(
.unwrap_or(Decision::Forbidden),
matched_rules,
};
// Preserve configured-shell compatibility for a directly authored exact
// allow of an opaque nested shell argv (for example, `/bin/sh script`).
// This exception is unavailable to a locally model-resolved outer runtime,
@@ -1328,15 +1327,13 @@ fn create_untrusted_wrapper_approval_requirement(
evaluation.decision = Decision::Prompt;
}
let permission_or_backend_gate = permission_delta_requires_outer_authority(
let requires_composed_authority = untrusted_composition_requires_full_authority(
context.permission_profile,
context.sandbox_permissions,
) || missing_managed_windows_sandbox_backend(
context.permission_profile,
context.windows_sandbox_level,
);
if evaluation.decision != Decision::Forbidden
&& permission_or_backend_gate
&& requires_composed_authority
&& !composed_full_authority
{
evaluation
@@ -1744,6 +1741,24 @@ fn permission_delta_requires_outer_authority(
}
}
#[cfg_attr(not(windows), allow(dead_code))]
fn untrusted_composition_requires_full_authority(
permission_profile: &PermissionProfile,
sandbox_permissions: SandboxPermissions,
windows_sandbox_level: WindowsSandboxLevel,
) -> bool {
let lacks_filesystem_containment = match permission_profile {
PermissionProfile::Disabled => true,
PermissionProfile::Managed { .. } => {
!profile_has_managed_filesystem_restrictions(permission_profile)
}
PermissionProfile::External { .. } => false,
};
lacks_filesystem_containment
|| permission_delta_requires_outer_authority(permission_profile, sandbox_permissions)
|| missing_managed_windows_sandbox_backend(permission_profile, windows_sandbox_level)
}
fn missing_managed_windows_sandbox_backend(
permission_profile: &PermissionProfile,
windows_sandbox_level: WindowsSandboxLevel,
@@ -553,7 +553,7 @@ fn untrusted_permission_and_windows_backend_gates_require_composed_authority() {
PermissionProfile::Disabled,
WSL::Disabled,
SP::RequireEscalated,
false,
true,
),
(
"external default",
@@ -610,6 +610,158 @@ fn untrusted_permission_and_windows_backend_gates_require_composed_authority() {
}
}
#[test]
fn untrusted_without_filesystem_containment_requires_complete_composed_authority() {
let outer = untrusted_powershell_command("Get-Location");
let inner = vec_str(&["Get-Location"]);
let partial_policy = format!(
"{}\n{}",
prefix_rule_for(&outer[..1], "allow"),
prefix_rule_for(&inner, "allow")
);
let full_policy = format!(
"{}\n{}",
prefix_rule_for(&outer, "allow"),
prefix_rule_for(&inner, "allow")
);
let disabled = PermissionProfile::Disabled;
for (name, approval_policy, expected) in [
(
"on request",
AskForApproval::OnRequest,
one_shot(/*reason*/ None),
),
(
"unless trusted",
AskForApproval::UnlessTrusted,
one_shot(/*reason*/ None),
),
(
"never",
AskForApproval::Never,
ExecApprovalRequirement::Forbidden {
reason: PROMPT_CONFLICT_REASON.to_string(),
},
),
(
"granular sandbox approval disabled",
granular(/*rules*/ false, /*sandbox_approval*/ false),
ExecApprovalRequirement::Forbidden {
reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(),
},
),
(
"granular sandbox approval enabled",
granular(/*rules*/ false, /*sandbox_approval*/ true),
one_shot(/*reason*/ None),
),
] {
pretty_assertions::assert_eq!(
composed_untrusted_requirement(
Some(&partial_policy),
&outer,
std::slice::from_ref(&inner),
approval_policy,
&disabled,
WindowsSandboxLevel::Disabled,
SandboxPermissions::UseDefault,
),
expected,
"{name}",
);
}
pretty_assertions::assert_eq!(
composed_untrusted_requirement(
Some(&prefix_rule_for(&outer, "allow")),
&outer,
std::slice::from_ref(&inner),
AskForApproval::OnRequest,
&disabled,
WindowsSandboxLevel::Disabled,
SandboxPermissions::UseDefault,
),
one_shot(/*reason*/ None),
"a heuristic-safe inner command is not explicit authority without a sandbox",
);
pretty_assertions::assert_eq!(
composed_untrusted_requirement(
Some(&full_policy),
&outer,
std::slice::from_ref(&inner),
AskForApproval::OnRequest,
&disabled,
WindowsSandboxLevel::Disabled,
SandboxPermissions::UseDefault,
),
untrusted_skip(/*bypass_sandbox*/ true),
"complete composed authority remains sufficient without a sandbox",
);
let managed_unrestricted = PermissionProfile::from_runtime_permissions(
&FileSystemSandboxPolicy::unrestricted(),
NetworkSandboxPolicy::Enabled,
);
let managed_full_disk_write = PermissionProfile::from_runtime_permissions(
&FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Special {
value: FileSystemSpecialPath::Root,
},
access: FileSystemAccessMode::Write,
}]),
NetworkSandboxPolicy::Enabled,
);
for (name, profile) in [
("managed unrestricted", managed_unrestricted),
("managed full-disk write", managed_full_disk_write),
] {
pretty_assertions::assert_eq!(
composed_untrusted_requirement(
Some(&partial_policy),
&outer,
std::slice::from_ref(&inner),
AskForApproval::OnRequest,
&profile,
WindowsSandboxLevel::RestrictedToken,
SandboxPermissions::UseDefault,
),
one_shot(/*reason*/ None),
"partial authority must prompt for {name}",
);
pretty_assertions::assert_eq!(
composed_untrusted_requirement(
Some(&full_policy),
&outer,
std::slice::from_ref(&inner),
AskForApproval::OnRequest,
&profile,
WindowsSandboxLevel::RestrictedToken,
SandboxPermissions::UseDefault,
),
untrusted_skip(/*bypass_sandbox*/ true),
"complete authority remains sufficient for {name}",
);
}
pretty_assertions::assert_eq!(
composed_untrusted_requirement(
Some(&partial_policy),
&outer,
std::slice::from_ref(&inner),
AskForApproval::OnRequest,
&PermissionProfile::External {
network: NetworkSandboxPolicy::Restricted,
},
WindowsSandboxLevel::RestrictedToken,
SandboxPermissions::UseDefault,
),
untrusted_skip(/*bypass_sandbox*/ false),
"an externally enforced sandbox keeps existing partial-authority behavior",
);
}
#[test]
fn untrusted_wrapper_identity_uses_exact_outer_and_restrictive_basename_rules() {
let path_a = vec_str(&[r"C:\workspace-a\powershell.exe", "-Command", "echo allowed"]);
+70
View File
@@ -1714,6 +1714,76 @@ async fn configured_delegators_without_rules_preserve_legacy_sandboxed_behavior(
}
}
#[tokio::test]
async fn posix_wrappers_require_complete_authority_without_filesystem_containment() {
let inner = vec_str(&["echo", "hello"]);
let model_command = vec![
host_program_path("zsh"),
"-lc".to_string(),
shlex_try_join(inner.iter().map(String::as_str)).expect("quote inner command"),
];
let full_inner_allow = prefix_rule_for(&inner, "allow");
let full_model_outer_allow = prefix_rule_for(&model_command, "allow");
let nested_wrapper = vec![
posix_script_program_path("sh"),
"-c".to_string(),
shlex_try_join(inner.iter().map(String::as_str)).expect("quote nested body"),
];
let configured_command = vec![
host_program_path("zsh"),
"-lc".to_string(),
shlex_try_join(nested_wrapper.iter().map(String::as_str)).expect("quote nested wrapper"),
];
let full_nested_wrapper_allow = prefix_rule_for(&nested_wrapper, "allow");
for (name, provenance, command, partial_policy, full_policy) in [
(
"local model-resolved wrapper",
ShellApprovalProvenance::local_model_resolved(),
model_command,
full_model_outer_allow.clone(),
format!("{full_model_outer_allow}\n{full_inner_allow}"),
),
(
"configured nested wrapper",
ShellApprovalProvenance::configured(),
configured_command,
full_nested_wrapper_allow.clone(),
format!("{full_nested_wrapper_allow}\n{full_inner_allow}"),
),
] {
assert_eq!(
requirement_with_provenance(
Some(&partial_policy),
&command,
AskForApproval::OnRequest,
PermissionProfile::Disabled,
SandboxPermissions::UseDefault,
provenance,
)
.await,
ExecApprovalRequirement::NeedsOneShotApproval { reason: None },
"partial authority must prompt for {name}",
);
assert_eq!(
requirement_with_provenance(
Some(&full_policy),
&command,
AskForApproval::OnRequest,
PermissionProfile::Disabled,
SandboxPermissions::UseDefault,
provenance,
)
.await,
ExecApprovalRequirement::Skip {
bypass_sandbox: true,
proposed_execpolicy_amendment: None,
},
"complete authority remains sufficient for {name}",
);
}
}
#[tokio::test]
async fn configured_exact_allow_can_authorize_an_opaque_nested_shell_only() {
let inner = vec![
+75 -1
View File
@@ -146,7 +146,11 @@ async fn only_exact_outer_rules_can_bypass_runtime_resolution() {
"host_executable(name = \"powershell\", paths = [\"{executable}\"])\n\
prefix_rule(pattern=[\"powershell\", {rest}], decision=\"allow\")"
),
Some(true),
Some(false),
),
(
format!("prefix_rule(pattern=[\"powershell\", {rest}], decision=\"allow\")"),
Some(false),
),
(
format!(
@@ -455,6 +459,76 @@ host_executable(name = "git", paths = ["C:\\trusted\\git.exe"])
);
}
#[tokio::test]
async fn mapped_namespace_literal_restrictions_apply_to_direct_and_inner_commands() {
for (kind, command) in [
(
"direct",
vec![
r"\\?\C:\attacker\git.exe.".to_string(),
"status".to_string(),
],
),
(
"PowerShell inner",
powershell_command(r"\\?\C:\attacker\git.exe. status"),
),
] {
let rendered = render_shlex_command(&command);
assert_eq!(
windows_requirement(
String::new(),
&command,
AskForApproval::OnRequest,
PermissionProfile::Disabled,
WindowsSandboxLevel::Disabled,
SandboxPermissions::UseDefault,
)
.await,
skip_outer(&command, false),
"unmatched {kind} command must take the allow fallback",
);
for (wire_decision, expected) in [
(
"prompt",
ExecApprovalRequirement::NeedsApproval {
reason: Some(format!("`{rendered}` requires approval by policy")),
proposed_execpolicy_amendment: None,
},
),
(
"forbidden",
ExecApprovalRequirement::Forbidden {
reason: format!(
"`{rendered}` rejected: policy forbids commands starting with `git.exe.`"
),
},
),
] {
assert_eq!(
windows_requirement(
format!(
r#"
prefix_rule(pattern = ["git.exe."], decision = "{wire_decision}")
host_executable(name = "git", paths = ["C:\\trusted\\git.exe"])
"#
),
&command,
AskForApproval::OnRequest,
PermissionProfile::Disabled,
WindowsSandboxLevel::Disabled,
SandboxPermissions::UseDefault,
)
.await,
expected,
"{kind}/{wire_decision}",
);
}
}
}
#[test]
fn commands_for_exec_policy_keeps_bare_powershell_alias_opaque() {
let command = vec![
+113
View File
@@ -20,6 +20,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::ApplyPatchApprovalRequestEvent;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::ExecApprovalPurpose;
use codex_protocol::protocol::ExecApprovalRequestEvent;
use codex_protocol::protocol::ExecPolicyAmendment;
use codex_protocol::protocol::GranularApprovalConfig;
@@ -36,6 +37,7 @@ use core_test_support::responses::ev_function_call_with_namespace;
use core_test_support::responses::ev_response_created;
use core_test_support::responses::mount_sse_once;
use core_test_support::responses::mount_sse_once_match;
use core_test_support::responses::mount_sse_sequence;
use core_test_support::responses::sse;
use core_test_support::responses::start_mock_server;
use core_test_support::skip_if_no_network;
@@ -2166,6 +2168,117 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[cfg(unix)]
async fn sandbox_retry_uses_fresh_callback_identity_and_one_shot_decisions() -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let approval_policy = AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: true,
mcp_elicitations: true,
});
let sandbox_policy = SandboxPolicy::new_read_only_policy();
let sandbox_policy_for_config = sandbox_policy.clone();
let test = test_codex()
.with_model("gpt-5.4")
.with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config
.features
.enable(Feature::UnifiedExec)
.expect("enable unified exec");
config
.set_legacy_sandbox_policy(sandbox_policy_for_config)
.expect("set sandbox policy");
})
.build(&server)
.await?;
let call_id = "sandbox-retry-callback-identity";
let target = test.cwd.path().join("sandbox-retry-callback-identity.txt");
let _ = fs::remove_file(&target);
let command = format!("touch {target:?}");
let event = exec_command_event(
call_id,
&command,
/*yield_time_ms*/ Some(30_000),
SandboxPermissions::UseDefault,
/*justification*/ None,
)?;
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-sandbox-retry-1"),
event,
ev_completed("resp-sandbox-retry-1"),
]),
sse(vec![
ev_assistant_message("msg-sandbox-retry-1", "done"),
ev_completed("resp-sandbox-retry-2"),
]),
],
)
.await;
submit_turn(
&test,
"retry the sandbox-denied write",
approval_policy,
sandbox_policy,
)
.await?;
let approval = expect_exec_approval(&test, &command).await;
let approval_id = approval
.approval_id
.clone()
.unwrap_or_else(|| panic!("sandbox retry must carry a callback ID: {approval:?}"));
assert_eq!(approval.call_id, call_id);
assert_ne!(approval_id, call_id);
assert_eq!(
approval.approval_purpose,
Some(ExecApprovalPurpose::SandboxRetry)
);
assert_eq!(
approval.effective_available_decisions(),
vec![ReviewDecision::Approved, ReviewDecision::Abort]
);
assert_eq!(
approval.reason.as_deref(),
Some("command failed; retry without sandbox?")
);
assert!(
!target.exists(),
"sandbox-denied write must not complete before retry approval"
);
test.codex
.submit(Op::ExecApproval {
id: approval_id,
turn_id: Some(approval.turn_id),
decision: ReviewDecision::Approved,
})
.await?;
wait_for_completion(&test).await;
let output_item = responses.requests()[1].function_call_output(call_id);
let result = parse_result(&output_item);
assert_eq!(result.exit_code, Some(0), "approved retry must succeed");
assert!(
target.exists(),
"approved retry must create the target file"
);
fs::remove_file(target)?;
Ok(())
}
#[tokio::test(flavor = "current_thread")]
#[cfg(unix)]
async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() -> Result<()> {
+1 -1
View File
@@ -43,7 +43,7 @@ host_executable(
- With host-executable resolution enabled, if no exact rule matches, execpolicy may fall back from `/usr/bin/git` to basename rules for `git`.
- If `host_executable(name="git", ...)` exists, basename fallback is only allowed for listed absolute paths.
- If no `host_executable()` entry exists for a basename, basename fallback is allowed.
- Windows verbatim/device namespace paths cannot be listed by `host_executable`; use an ordinary absolute spelling or an exact prefix rule. If either a namespace command's literal basename or its ordinary normalized alias has host-executable metadata, the namespace spelling does not inherit basename rules. With no corresponding metadata, its literal basename rule may still match. Exact full-path rules always retain their authored decision.
- Windows verbatim/device namespace paths cannot be listed by `host_executable`; use an ordinary absolute spelling or an exact prefix rule. If either a namespace command's literal basename or its ordinary normalized alias has host-executable metadata, basename `allow` is filtered while basename `prompt` and `forbidden` remain in force without a `resolvedProgram`. With no corresponding metadata, its literal basename rule may still match with its authored decision. Exact full-path rules always retain their authored decision.
- The library's restrictive basename overlay is separate from ordinary fallback: it ignores host-executable path lists for `prompt` and `forbidden`, but always discards basename `allow`.
## CLI
+1 -1
View File
@@ -361,7 +361,7 @@ impl Policy {
.host_executables_by_name
.contains_key(&normalized_basename)
{
return Vec::new();
return self.match_restrictive_basename_rules(cmd);
}
}
let Ok(program) = AbsolutePathBuf::try_from(first.clone()) else {
+32 -15
View File
@@ -1126,13 +1126,13 @@ host_executable(name = "git", paths = ["{}"])
#[cfg(windows)]
#[test]
fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<()> {
fn mapped_namespace_literal_aliases_filter_allow_and_preserve_restrictions() -> Result<()> {
let options = MatchOptions {
resolve_host_executables: true,
};
for executable_suffix in [".exe", ".cmd", ".bat", ".com"] {
for literal_tail in [".", " ", "...", " ", ". . ", ".. . "] {
for literal_tail in ["", ".", " ", "...", " ", ". . ", ".. . "] {
let mixed_case = executable_suffix == ".com" && literal_tail == ".. . ";
let raw_stem = if mixed_case { "GiT" } else { "git" };
let raw_suffix = if mixed_case {
@@ -1144,7 +1144,7 @@ fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<(
let literal_rule_key = raw_literal_name.to_ascii_lowercase();
let ordinary_name = format!("git{executable_suffix}");
let trusted_path = format!(r"C:\trusted\{ordinary_name}");
let mapping_sources = [
let mut mapping_sources = vec![
format!(
r#"host_executable(name = "git", paths = ["{}"] )"#,
starlark_string(&trusted_path)
@@ -1156,6 +1156,8 @@ fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<(
format!(r#"host_executable(name = "{literal_rule_key}", paths = [])"#),
format!(r#"host_executable(name = "{raw_literal_name}", paths = [])"#),
];
mapping_sources.sort();
mapping_sources.dedup();
let namespace_paths = [
format!(r"\\?\C:\attacker\{raw_literal_name}"),
format!(r"\\?\UNC\server\share\{raw_literal_name}"),
@@ -1164,23 +1166,38 @@ fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<(
];
for mapping_source in mapping_sources {
let policy = parse_policy(&format!(
r#"
prefix_rule(pattern = ["{literal_rule_key}"], decision = "allow")
for (wire_decision, decision) in [
("allow", Decision::Allow),
("prompt", Decision::Prompt),
("forbidden", Decision::Forbidden),
] {
let policy = parse_policy(&format!(
r#"
prefix_rule(pattern = ["{literal_rule_key}"], decision = "{wire_decision}")
{mapping_source}
"#
))?;
for executable in &namespace_paths {
assert!(
policy
.matches_for_command_with_options(
))?;
for executable in &namespace_paths {
let expected = if decision == Decision::Allow {
Vec::new()
} else {
vec![RuleMatch::PrefixRuleMatch {
matched_prefix: vec![literal_rule_key.clone()],
decision,
resolved_program: None,
justification: None,
}]
};
assert_eq!(
policy.matches_for_command_with_options(
&[executable.clone(), "status".to_string()],
None,
&options,
)
.is_empty(),
"suffix={executable_suffix:?} tail={literal_tail:?} mapping={mapping_source:?} executable={executable:?}"
);
),
expected,
"suffix={executable_suffix:?} tail={literal_tail:?} decision={wire_decision:?} mapping={mapping_source:?} executable={executable:?}"
);
}
}
}
}