Skip to content
130 changes: 109 additions & 21 deletions codex-rs/core/src/exec_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,43 @@ static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[
&["osascript"],
];

/// Describes which unmatched-command heuristics should classify the command
/// words being evaluated by exec-policy.
///
/// The command tokens may be the original argv or a shell-specific lowering of
/// a wrapper such as `bash -lc ...` or `powershell.exe -Command ...`. We only
/// need to distinguish the PowerShell case because its safelist and dangerous
/// heuristics operate on PowerShell-flavored inner command words rather than
/// the generic command classifier.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub(crate) enum ExecPolicyCommandOrigin {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a docstring because I'm a little fuzzy on what this means.

ExecPolicyCommandOrigin::PowerShell to me implies the command was targeted to be run under PowerShell, but looking at the code, it feels like it actually means "the type of shell parser that was used to parse the command." It's possible the user is on Windows but could be using a shell that isn't PowerShell, isn't it? Or no?

To that end, I'm not clear why the other variant is named Direct.

/// Use the generic unmatched-command heuristics.
Generic,
#[cfg(windows)]
/// The command words came from the `-Command` body of a top-level
/// PowerShell wrapper, so use PowerShell-specific unmatched-command
/// heuristics for the lowered words.
PowerShell,
}

#[derive(Clone, Copy)]
pub(crate) struct UnmatchedCommandContext<'a> {
pub(crate) approval_policy: AskForApproval,
pub(crate) permission_profile: &'a PermissionProfile,
pub(crate) file_system_sandbox_policy: &'a FileSystemSandboxPolicy,
pub(crate) sandbox_cwd: &'a Path,
pub(crate) sandbox_permissions: SandboxPermissions,
pub(crate) used_complex_parsing: bool,
pub(crate) command_origin: ExecPolicyCommandOrigin,
}

#[derive(Debug, Eq, PartialEq)]
struct ExecPolicyCommands {
commands: Vec<Vec<String>>,
used_complex_parsing: bool,
command_origin: ExecPolicyCommandOrigin,
}

pub(crate) fn child_uses_parent_exec_policy(parent_config: &Config, child_config: &Config) -> bool {
fn exec_policy_config_folders(config: &Config) -> Vec<AbsolutePathBuf> {
config
Expand Down Expand Up @@ -246,20 +283,27 @@ impl ExecPolicyManager {
prefix_rule,
} = req;
let exec_policy = self.current();
let (commands, used_complex_parsing) = commands_for_exec_policy(command);
let ExecPolicyCommands {
commands,
used_complex_parsing,
command_origin,
} = commands_for_exec_policy(command);
// Keep heredoc prefix parsing for rule evaluation so existing
// allow/prompt/forbidden rules still apply, but avoid auto-derived
// amendments when only the heredoc fallback parser matched.
let auto_amendment_allowed = !used_complex_parsing;
let exec_policy_fallback = |cmd: &[String]| {
render_decision_for_unmatched_command(
approval_policy,
&permission_profile,
file_system_sandbox_policy,
sandbox_cwd,
cmd,
sandbox_permissions,
used_complex_parsing,
UnmatchedCommandContext {
approval_policy,
permission_profile: &permission_profile,
file_system_sandbox_policy,
sandbox_cwd,
sandbox_permissions,
used_complex_parsing,
command_origin,
},
)
};
let match_options = MatchOptions {
Expand Down Expand Up @@ -581,16 +625,27 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result<Policy,
}

/// If a command is not matched by any execpolicy rule, derive a [`Decision`].
pub fn render_decision_for_unmatched_command(
approval_policy: AskForApproval,
permission_profile: &PermissionProfile,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
sandbox_cwd: &Path,
pub(crate) fn render_decision_for_unmatched_command(
command: &[String],
sandbox_permissions: SandboxPermissions,
used_complex_parsing: bool,
context: UnmatchedCommandContext<'_>,
) -> Decision {
if is_known_safe_command(command) && !used_complex_parsing {
let UnmatchedCommandContext {
approval_policy,
permission_profile,
file_system_sandbox_policy,
sandbox_cwd,
sandbox_permissions,
used_complex_parsing,
command_origin,
} = context;
let is_known_safe = match command_origin {
ExecPolicyCommandOrigin::Generic => is_known_safe_command(command),
#[cfg(windows)]
ExecPolicyCommandOrigin::PowerShell => {
codex_shell_command::is_safe_command::is_safe_powershell_words(command)
}
};
if is_known_safe && !used_complex_parsing {
return Decision::Allow;
}

Expand All @@ -609,7 +664,14 @@ pub fn render_decision_for_unmatched_command(
// We prefer to prompt the user rather than outright forbid the command,
// but if the user has explicitly disabled prompts, we must
// forbid the command.
if command_might_be_dangerous(command) || environment_lacks_sandbox_protections {
let command_is_dangerous = match command_origin {
ExecPolicyCommandOrigin::Generic => command_might_be_dangerous(command),
#[cfg(windows)]
ExecPolicyCommandOrigin::PowerShell => {
codex_shell_command::is_dangerous_command::is_dangerous_powershell_words(command)
}
};
if command_is_dangerous || environment_lacks_sandbox_protections {
return match approval_policy {
AskForApproval::Never => {
let sandbox_is_explicitly_disabled = matches!(
Expand Down Expand Up @@ -637,7 +699,7 @@ pub fn render_decision_for_unmatched_command(
Decision::Allow
}
AskForApproval::UnlessTrusted => {
// We already checked `is_known_safe_command(command)` and it
// We already checked the unmatched-command safelist and it
// returned false, so we must prompt.
Decision::Prompt
}
Expand Down Expand Up @@ -698,18 +760,44 @@ fn default_policy_path(codex_home: &Path) -> PathBuf {
codex_home.join(RULES_DIR_NAME).join(DEFAULT_POLICY_FILE)
}

fn commands_for_exec_policy(command: &[String]) -> (Vec<Vec<String>>, bool) {
fn commands_for_exec_policy(command: &[String]) -> ExecPolicyCommands {
if let Some(commands) = parse_shell_lc_plain_commands(command)
&& !commands.is_empty()
{
return (commands, false);
return ExecPolicyCommands {
commands,
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::Generic,
};
}

#[cfg(windows)]
{
if let Some(commands) =
codex_shell_command::powershell::parse_powershell_command_into_plain_commands(command)
&& !commands.is_empty()
{
return ExecPolicyCommands {
commands,
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::PowerShell,
};
}
}

if let Some(single_command) = parse_shell_lc_single_command_prefix(command) {
return (vec![single_command], true);
return ExecPolicyCommands {
commands: vec![single_command],
used_complex_parsing: true,
command_origin: ExecPolicyCommandOrigin::Generic,
};
}

(vec![command.to_vec()], false)
ExecPolicyCommands {
commands: vec![command.to_vec()],
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::Generic,
}
}

/// Derive a proposed execpolicy amendment when a command requires user approval
Expand Down
103 changes: 68 additions & 35 deletions codex-rs/core/src/exec_policy_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ use tempfile::TempDir;
use tempfile::tempdir;
use toml::Value as TomlValue;

#[cfg(windows)]
#[path = "exec_policy_windows_tests.rs"]
mod windows_tests;

fn config_stack_for_dot_codex_folder(dot_codex_folder: &Path) -> ConfigLayerStack {
let dot_codex_folder =
AbsolutePathBuf::from_absolute_path(dot_codex_folder).expect("absolute dot_codex_folder");
Expand Down Expand Up @@ -660,7 +664,14 @@ async fn evaluates_bash_lc_inner_commands() {
fn commands_for_exec_policy_falls_back_for_empty_shell_script() {
let command = vec!["bash".to_string(), "-lc".to_string(), "".to_string()];

assert_eq!(commands_for_exec_policy(&command), (vec![command], false));
assert_eq!(
commands_for_exec_policy(&command),
ExecPolicyCommands {
commands: vec![command],
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::Generic,
}
);
}

#[test]
Expand All @@ -671,7 +682,14 @@ fn commands_for_exec_policy_falls_back_for_whitespace_shell_script() {
" \n\t ".to_string(),
];

assert_eq!(commands_for_exec_policy(&command), (vec![command], false));
assert_eq!(
commands_for_exec_policy(&command),
ExecPolicyCommands {
commands: vec![command],
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::Generic,
}
);
}

#[tokio::test]
Expand Down Expand Up @@ -961,19 +979,24 @@ fn unmatched_granular_policy_still_prompts_for_restricted_sandbox_escalation() {
assert_eq!(
Decision::Prompt,
render_decision_for_unmatched_command(
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: true,
mcp_elicitations: true,
}),
&permission_profile_from_sandbox_policy(&SandboxPolicy::new_read_only_policy()),
&read_only_file_system_sandbox_policy(),
Path::new("/tmp"),
&command,
SandboxPermissions::RequireEscalated,
/*used_complex_parsing*/ false,
UnmatchedCommandContext {
approval_policy: AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: true,
mcp_elicitations: true,
}),
permission_profile: &permission_profile_from_sandbox_policy(
&SandboxPolicy::new_read_only_policy(),
),
file_system_sandbox_policy: &read_only_file_system_sandbox_policy(),
sandbox_cwd: Path::new("/tmp"),
sandbox_permissions: SandboxPermissions::RequireEscalated,
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::Generic,
},
)
);
}
Expand All @@ -986,13 +1009,16 @@ fn unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts() {
assert_eq!(
Decision::Prompt,
render_decision_for_unmatched_command(
AskForApproval::OnRequest,
&PermissionProfile::Disabled,
&restricted_file_system_policy,
Path::new("/tmp"),
&command,
SandboxPermissions::RequireEscalated,
/*used_complex_parsing*/ false,
UnmatchedCommandContext {
approval_policy: AskForApproval::OnRequest,
permission_profile: &PermissionProfile::Disabled,
file_system_sandbox_policy: &restricted_file_system_policy,
sandbox_cwd: Path::new("/tmp"),
sandbox_permissions: SandboxPermissions::RequireEscalated,
used_complex_parsing: false,
command_origin: ExecPolicyCommandOrigin::Generic,
},
)
);
}
Expand Down Expand Up @@ -1976,10 +2002,20 @@ struct ExecApprovalRequirementScenario {
prefix_rule: Option<Vec<String>>,
}

async fn assert_exec_approval_requirement_for_command(
fn policy_from_src(policy_src: Option<&str>) -> Arc<Policy> {
match policy_src {
Some(src) => {
let mut parser = PolicyParser::new();
parser.parse("test.rules", src).expect("parse policy");
Arc::new(parser.build())
}
None => Arc::new(Policy::empty()),
}
}

async fn exec_approval_requirement_for_command(
test: ExecApprovalRequirementScenario,
expected_requirement: ExecApprovalRequirement,
) {
) -> ExecApprovalRequirement {
let ExecApprovalRequirementScenario {
policy_src,
command,
Expand All @@ -1990,19 +2026,10 @@ async fn assert_exec_approval_requirement_for_command(
prefix_rule,
} = test;

let policy = match policy_src {
Some(src) => {
let mut parser = PolicyParser::new();
parser
.parse("test.rules", src.as_str())
.expect("parse policy");
Arc::new(parser.build())
}
None => Arc::new(Policy::empty()),
};
let policy = policy_from_src(policy_src.as_deref());

let permission_profile = permission_profile_from_sandbox_policy(&sandbox_policy);
let requirement = ExecPolicyManager::new(policy)
ExecPolicyManager::new(policy)
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
command: &command,
approval_policy,
Expand All @@ -2012,8 +2039,14 @@ async fn assert_exec_approval_requirement_for_command(
sandbox_permissions,
prefix_rule,
})
.await;
.await
}

async fn assert_exec_approval_requirement_for_command(
test: ExecApprovalRequirementScenario,
expected_requirement: ExecApprovalRequirement,
) {
let requirement = exec_approval_requirement_for_command(test).await;
assert_eq!(requirement, expected_requirement);
}

Expand Down
Loading
Loading