mirror of
https://github.com/openai/codex.git
synced 2026-04-29 02:41:12 +03:00
Removed "exec_policy" feature flag (#10851)
This is no longer needed because it's on by default
This commit is contained in:
@@ -25,8 +25,6 @@ use tokio::fs;
|
||||
use tokio::task::spawn_blocking;
|
||||
|
||||
use crate::bash::parse_shell_lc_plain_commands;
|
||||
use crate::features::Feature;
|
||||
use crate::features::Features;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::tools::sandboxing::ExecApprovalRequirement;
|
||||
use shlex::try_join as shlex_try_join;
|
||||
@@ -78,9 +76,6 @@ pub enum ExecPolicyUpdateError {
|
||||
#[from]
|
||||
source: ExecPolicyRuleError,
|
||||
},
|
||||
|
||||
#[error("cannot append rule because rules feature is disabled")]
|
||||
FeatureDisabled,
|
||||
}
|
||||
|
||||
pub(crate) struct ExecPolicyManager {
|
||||
@@ -88,7 +83,6 @@ pub(crate) struct ExecPolicyManager {
|
||||
}
|
||||
|
||||
pub(crate) struct ExecApprovalRequest<'a> {
|
||||
pub(crate) features: &'a Features,
|
||||
pub(crate) command: &'a [String],
|
||||
pub(crate) approval_policy: AskForApproval,
|
||||
pub(crate) sandbox_policy: &'a SandboxPolicy,
|
||||
@@ -103,12 +97,8 @@ impl ExecPolicyManager {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn load(
|
||||
features: &Features,
|
||||
config_stack: &ConfigLayerStack,
|
||||
) -> Result<Self, ExecPolicyError> {
|
||||
let (policy, warning) =
|
||||
load_exec_policy_for_features_with_warning(features, config_stack).await?;
|
||||
pub(crate) async fn load(config_stack: &ConfigLayerStack) -> Result<Self, ExecPolicyError> {
|
||||
let (policy, warning) = load_exec_policy_with_warning(config_stack).await?;
|
||||
if let Some(err) = warning.as_ref() {
|
||||
tracing::warn!("failed to parse rules: {err}");
|
||||
}
|
||||
@@ -124,7 +114,6 @@ impl ExecPolicyManager {
|
||||
req: ExecApprovalRequest<'_>,
|
||||
) -> ExecApprovalRequirement {
|
||||
let ExecApprovalRequest {
|
||||
features,
|
||||
command,
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
@@ -144,11 +133,8 @@ impl ExecPolicyManager {
|
||||
};
|
||||
let evaluation = exec_policy.check_multiple(commands.iter(), &exec_policy_fallback);
|
||||
|
||||
let requested_amendment = derive_requested_execpolicy_amendment(
|
||||
features,
|
||||
prefix_rule.as_ref(),
|
||||
&evaluation.matched_rules,
|
||||
);
|
||||
let requested_amendment =
|
||||
derive_requested_execpolicy_amendment(prefix_rule.as_ref(), &evaluation.matched_rules);
|
||||
|
||||
match evaluation.decision {
|
||||
Decision::Forbidden => ExecApprovalRequirement::Forbidden {
|
||||
@@ -162,15 +148,11 @@ impl ExecPolicyManager {
|
||||
} else {
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: derive_prompt_reason(command, &evaluation),
|
||||
proposed_execpolicy_amendment: if features.enabled(Feature::ExecPolicy) {
|
||||
requested_amendment.or_else(|| {
|
||||
try_derive_execpolicy_amendment_for_prompt_rules(
|
||||
&evaluation.matched_rules,
|
||||
)
|
||||
})
|
||||
} else {
|
||||
None
|
||||
},
|
||||
proposed_execpolicy_amendment: requested_amendment.or_else(|| {
|
||||
try_derive_execpolicy_amendment_for_prompt_rules(
|
||||
&evaluation.matched_rules,
|
||||
)
|
||||
}),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -179,11 +161,9 @@ impl ExecPolicyManager {
|
||||
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
|
||||
},
|
||||
proposed_execpolicy_amendment: try_derive_execpolicy_amendment_for_allow_rules(
|
||||
&evaluation.matched_rules,
|
||||
),
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -221,21 +201,15 @@ impl Default for ExecPolicyManager {
|
||||
}
|
||||
|
||||
pub async fn check_execpolicy_for_warnings(
|
||||
features: &Features,
|
||||
config_stack: &ConfigLayerStack,
|
||||
) -> Result<Option<ExecPolicyError>, ExecPolicyError> {
|
||||
let (_, warning) = load_exec_policy_for_features_with_warning(features, config_stack).await?;
|
||||
let (_, warning) = load_exec_policy_with_warning(config_stack).await?;
|
||||
Ok(warning)
|
||||
}
|
||||
|
||||
async fn load_exec_policy_for_features_with_warning(
|
||||
features: &Features,
|
||||
async fn load_exec_policy_with_warning(
|
||||
config_stack: &ConfigLayerStack,
|
||||
) -> Result<(Policy, Option<ExecPolicyError>), ExecPolicyError> {
|
||||
if !features.enabled(Feature::ExecPolicy) {
|
||||
return Ok((Policy::empty(), None));
|
||||
}
|
||||
|
||||
match load_exec_policy(config_stack).await {
|
||||
Ok(policy) => Ok((policy, None)),
|
||||
Err(err @ ExecPolicyError::ParsePolicy { .. }) => Ok((Policy::empty(), Some(err))),
|
||||
@@ -413,14 +387,9 @@ fn try_derive_execpolicy_amendment_for_allow_rules(
|
||||
}
|
||||
|
||||
fn derive_requested_execpolicy_amendment(
|
||||
features: &Features,
|
||||
prefix_rule: Option<&Vec<String>>,
|
||||
matched_rules: &[RuleMatch],
|
||||
) -> Option<ExecPolicyAmendment> {
|
||||
if !features.enabled(Feature::ExecPolicy) {
|
||||
return None;
|
||||
}
|
||||
|
||||
let prefix_rule = prefix_rule?;
|
||||
if prefix_rule.is_empty() {
|
||||
return None;
|
||||
@@ -589,13 +558,11 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn returns_empty_policy_when_feature_disabled() {
|
||||
let mut features = Features::with_defaults();
|
||||
features.disable(Feature::ExecPolicy);
|
||||
async fn returns_empty_policy_when_no_policy_files_exist() {
|
||||
let temp_dir = tempdir().expect("create temp dir");
|
||||
let config_stack = config_stack_for_dot_codex_folder(temp_dir.path());
|
||||
|
||||
let manager = ExecPolicyManager::load(&features, &config_stack)
|
||||
let manager = ExecPolicyManager::load(&config_stack)
|
||||
.await
|
||||
.expect("manager result");
|
||||
let policy = manager.current();
|
||||
@@ -809,7 +776,6 @@ prefix_rule(pattern=["rm"], decision="forbidden")
|
||||
let manager = ExecPolicyManager::new(policy);
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &forbidden_script,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::DangerFullAccess,
|
||||
@@ -844,7 +810,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::new(policy);
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &[
|
||||
"rm".to_string(),
|
||||
"-rf".to_string(),
|
||||
@@ -878,7 +843,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::new(policy);
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::DangerFullAccess,
|
||||
@@ -909,7 +873,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::new(policy);
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::Never,
|
||||
sandbox_policy: &SandboxPolicy::DangerFullAccess,
|
||||
@@ -933,7 +896,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::default();
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::UnlessTrusted,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -964,7 +926,6 @@ prefix_rule(
|
||||
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &features,
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1002,7 +963,6 @@ prefix_rule(
|
||||
assert_eq!(
|
||||
ExecPolicyManager::new(policy)
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::UnlessTrusted,
|
||||
sandbox_policy: &SandboxPolicy::DangerFullAccess,
|
||||
@@ -1077,7 +1037,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::default();
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::UnlessTrusted,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1095,34 +1054,6 @@ prefix_rule(
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
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();
|
||||
features.disable(Feature::ExecPolicy);
|
||||
|
||||
let manager = ExecPolicyManager::default();
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &features,
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::UnlessTrusted,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prefix_rule: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
requirement,
|
||||
ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn proposed_execpolicy_amendment_is_omitted_when_policy_prompts() {
|
||||
let policy_src = r#"prefix_rule(pattern=["rm"], decision="prompt")"#;
|
||||
@@ -1136,7 +1067,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::new(policy);
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::DangerFullAccess,
|
||||
@@ -1164,7 +1094,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::default();
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::UnlessTrusted,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1203,7 +1132,6 @@ prefix_rule(
|
||||
assert_eq!(
|
||||
ExecPolicyManager::new(policy)
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::UnlessTrusted,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1227,7 +1155,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::default();
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1258,7 +1185,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::new(policy);
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1282,7 +1208,6 @@ prefix_rule(
|
||||
let manager = ExecPolicyManager::default();
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &Features::with_defaults(),
|
||||
command: &command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::DangerFullAccess,
|
||||
@@ -1316,7 +1241,6 @@ prefix_rule(
|
||||
}
|
||||
|
||||
let policy = ExecPolicyManager::new(Arc::new(Policy::empty()));
|
||||
let features = Features::with_defaults();
|
||||
let permissions = SandboxPermissions::UseDefault;
|
||||
|
||||
// This command should not be run without user approval unless there is
|
||||
@@ -1350,7 +1274,6 @@ prefix_rule(
|
||||
expected_req,
|
||||
policy
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &features,
|
||||
command: &sneaky_command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1374,7 +1297,6 @@ prefix_rule(
|
||||
},
|
||||
policy
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &features,
|
||||
command: &dangerous_command,
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
@@ -1394,7 +1316,6 @@ prefix_rule(
|
||||
},
|
||||
policy
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &features,
|
||||
command: &dangerous_command,
|
||||
approval_policy: AskForApproval::Never,
|
||||
sandbox_policy: &SandboxPolicy::ReadOnly,
|
||||
|
||||
Reference in New Issue
Block a user