mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Preserve managed exec policy after rules parse errors (#31188)
## Why Cleanup for #31179 exposed a core fallback bug that the TUI had been handling locally. When a custom `.rules` file fails to parse, nonfatal clients warn and continue, but `load_exec_policy_with_warning` replaced the entire policy with `Policy::empty()` before managed requirements were merged. App-server and desktop clients could therefore silently lose required prompt and forbidden rules. This fix is intentionally separate from #31179 so the TUI cleanup does not depend on it. ## What changed - Preserve the managed requirements exec policy when custom file rules fail to parse, while returning the existing warning and discarding the file-based policy. - Use the same nonfatal fallback when loading network proxy policy. - Keep parse errors fatal for strict clients such as `codex exec`.
This commit is contained in:
@@ -554,12 +554,19 @@ pub fn format_exec_policy_error_with_source(error: &ExecPolicyError) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
async fn load_exec_policy_with_warning(
|
||||
pub(crate) async fn load_exec_policy_with_warning(
|
||||
config_stack: &ConfigLayerStack,
|
||||
) -> Result<(Policy, Option<ExecPolicyError>), ExecPolicyError> {
|
||||
match load_exec_policy(config_stack).await {
|
||||
Ok(policy) => Ok((policy, None)),
|
||||
Err(err @ ExecPolicyError::ParsePolicy { .. }) => Ok((Policy::empty(), Some(err))),
|
||||
Err(err @ ExecPolicyError::ParsePolicy { .. }) => {
|
||||
let policy = config_stack
|
||||
.requirements()
|
||||
.exec_policy
|
||||
.as_deref()
|
||||
.map_or_else(Policy::empty, |policy| policy.as_ref().clone());
|
||||
Ok((policy, Some(err)))
|
||||
}
|
||||
Err(err) => Err(err),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -374,6 +374,42 @@ async fn merges_requirements_exec_policy_network_rules() -> anyhow::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn malformed_custom_rules_preserve_requirements_exec_policy() -> anyhow::Result<()> {
|
||||
let temp_dir = tempdir()?;
|
||||
let policy_dir = temp_dir.path().join(RULES_DIR_NAME);
|
||||
fs::create_dir_all(&policy_dir)?;
|
||||
fs::write(policy_dir.join("broken.rules"), "prefix_rule(")?;
|
||||
|
||||
let mut requirements_exec_policy = Policy::empty();
|
||||
requirements_exec_policy.add_prefix_rule(&["rm".to_string()], Decision::Forbidden)?;
|
||||
let requirements = ConfigRequirements {
|
||||
exec_policy: Some(Sourced::new(
|
||||
RequirementsExecPolicy::new(requirements_exec_policy),
|
||||
RequirementSource::Unknown,
|
||||
)),
|
||||
..ConfigRequirements::default()
|
||||
};
|
||||
let dot_codex_folder = AbsolutePathBuf::from_absolute_path(temp_dir.path())?;
|
||||
let layer = ConfigLayerEntry::new(
|
||||
ConfigLayerSource::Project { dot_codex_folder },
|
||||
TomlValue::Table(Default::default()),
|
||||
);
|
||||
let config_stack =
|
||||
ConfigLayerStack::new(vec![layer], requirements, ConfigRequirementsToml::default())?;
|
||||
|
||||
let (policy, warning) = load_exec_policy_with_warning(&config_stack).await?;
|
||||
|
||||
assert!(matches!(warning, Some(ExecPolicyError::ParsePolicy { .. })));
|
||||
assert_eq!(
|
||||
policy
|
||||
.check_multiple([vec!["rm".to_string()]].iter(), &|_| Decision::Allow)
|
||||
.decision,
|
||||
Decision::Forbidden
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn preserves_host_executables_when_requirements_overlay_is_present() -> anyhow::Result<()> {
|
||||
let temp_dir = tempdir()?;
|
||||
|
||||
@@ -2,9 +2,8 @@ use crate::config::find_codex_home;
|
||||
use crate::config::is_builtin_permission_profile_name;
|
||||
use crate::config::reject_unknown_builtin_permission_profile;
|
||||
use crate::config::resolve_permission_profile;
|
||||
use crate::exec_policy::ExecPolicyError;
|
||||
use crate::exec_policy::format_exec_policy_error_with_source;
|
||||
use crate::exec_policy::load_exec_policy;
|
||||
use crate::exec_policy::load_exec_policy_with_warning;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
@@ -64,13 +63,15 @@ async fn build_config_state_with_mtimes() -> Result<(ConfigState, Vec<LayerMtime
|
||||
.await
|
||||
.context("failed to load Codex config")?;
|
||||
|
||||
let (exec_policy, warning) = match load_exec_policy(&config_layer_stack).await {
|
||||
Ok(policy) => (policy, None),
|
||||
Err(err @ ExecPolicyError::ParsePolicy { .. }) => {
|
||||
(codex_execpolicy::Policy::empty(), Some(err))
|
||||
}
|
||||
Err(err) => return Err(err.into()),
|
||||
};
|
||||
let layer_mtimes = collect_layer_mtimes(&config_layer_stack);
|
||||
let state = build_config_state_from_layers(&config_layer_stack).await?;
|
||||
Ok((state, layer_mtimes))
|
||||
}
|
||||
|
||||
async fn build_config_state_from_layers(
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
) -> Result<ConfigState> {
|
||||
let (exec_policy, warning) = load_exec_policy_with_warning(config_layer_stack).await?;
|
||||
if let Some(err) = warning.as_ref() {
|
||||
tracing::warn!(
|
||||
"failed to parse execpolicy while building network proxy state: {}",
|
||||
@@ -78,12 +79,10 @@ async fn build_config_state_with_mtimes() -> Result<(ConfigState, Vec<LayerMtime
|
||||
);
|
||||
}
|
||||
|
||||
let config = config_from_layers(&config_layer_stack, &exec_policy)?;
|
||||
let config = config_from_layers(config_layer_stack, &exec_policy)?;
|
||||
|
||||
let constraints = enforce_trusted_constraints(&config_layer_stack, &config)?;
|
||||
let layer_mtimes = collect_layer_mtimes(&config_layer_stack);
|
||||
let state = build_config_state(config, constraints)?;
|
||||
Ok((state, layer_mtimes))
|
||||
let constraints = enforce_trusted_constraints(config_layer_stack, &config)?;
|
||||
build_config_state(config, constraints)
|
||||
}
|
||||
|
||||
fn collect_layer_mtimes(stack: &ConfigLayerStack) -> Vec<LayerMtime> {
|
||||
|
||||
@@ -5,6 +5,9 @@ use codex_config::ConfigLayerSource;
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigRequirements;
|
||||
use codex_config::ConfigRequirementsToml;
|
||||
use codex_config::RequirementSource;
|
||||
use codex_config::RequirementsExecPolicy;
|
||||
use codex_config::Sourced;
|
||||
use codex_config::permissions_toml::NetworkDomainPermissionToml;
|
||||
use codex_config::permissions_toml::NetworkDomainPermissionsToml;
|
||||
use codex_execpolicy::Decision;
|
||||
@@ -12,6 +15,8 @@ use codex_execpolicy::NetworkRuleProtocol;
|
||||
use codex_execpolicy::Policy;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::BTreeMap;
|
||||
use std::fs;
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[test]
|
||||
fn higher_precedence_profile_network_overlays_domain_entries() {
|
||||
@@ -225,6 +230,51 @@ fn execpolicy_network_rules_overlay_network_lists() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn malformed_custom_rules_preserve_managed_denied_domain() {
|
||||
let temp_dir = tempdir().expect("create temp dir");
|
||||
let policy_dir = temp_dir.path().join("rules");
|
||||
fs::create_dir_all(&policy_dir).expect("create policy dir");
|
||||
fs::write(policy_dir.join("broken.rules"), "prefix_rule(").expect("write malformed policy");
|
||||
|
||||
let mut requirements_exec_policy = Policy::empty();
|
||||
requirements_exec_policy
|
||||
.add_network_rule(
|
||||
"blocked.example.com",
|
||||
NetworkRuleProtocol::Https,
|
||||
Decision::Forbidden,
|
||||
/*justification*/ None,
|
||||
)
|
||||
.expect("managed network rule should be valid");
|
||||
let requirements = ConfigRequirements {
|
||||
exec_policy: Some(Sourced::new(
|
||||
RequirementsExecPolicy::new(requirements_exec_policy),
|
||||
RequirementSource::Unknown,
|
||||
)),
|
||||
..ConfigRequirements::default()
|
||||
};
|
||||
let dot_codex_folder = AbsolutePathBuf::from_absolute_path(temp_dir.path())
|
||||
.expect("dot codex folder should be absolute");
|
||||
let layers = ConfigLayerStack::new(
|
||||
vec![ConfigLayerEntry::new(
|
||||
ConfigLayerSource::Project { dot_codex_folder },
|
||||
toml::Value::Table(Default::default()),
|
||||
)],
|
||||
requirements,
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("layer stack should be valid");
|
||||
|
||||
let state = build_config_state_from_layers(&layers)
|
||||
.await
|
||||
.expect("proxy state should tolerate malformed custom rules");
|
||||
|
||||
assert_eq!(
|
||||
state.config.network.denied_domains(),
|
||||
Some(vec!["blocked.example.com".to_string()])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_network_constraints_includes_allow_all_unix_sockets_flag() {
|
||||
let config: toml::Value = toml::from_str(
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
#![allow(clippy::unwrap_used)]
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_config::test_support::CloudConfigBundleFixture;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::config_types::CollaborationMode;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
@@ -17,6 +18,7 @@ use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::mount_sse_once;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_target_windows;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::test_codex::turn_permission_fields;
|
||||
@@ -255,6 +257,84 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn malformed_custom_rules_preserve_managed_forbidden_prefix() -> Result<()> {
|
||||
skip_if_target_windows!(
|
||||
Ok(()),
|
||||
"managed prefix fixture uses POSIX executable semantics"
|
||||
);
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"
|
||||
[rules]
|
||||
prefix_rules = [
|
||||
{ pattern = [{ token = "echo" }], decision = "forbidden" },
|
||||
]
|
||||
"#,
|
||||
),
|
||||
)
|
||||
.with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::UnifiedExec)
|
||||
.expect("test config should allow feature update");
|
||||
let policy_path = config.codex_home.join("rules").join("broken.rules");
|
||||
fs::create_dir_all(
|
||||
policy_path
|
||||
.parent()
|
||||
.expect("policy directory must have a parent"),
|
||||
)
|
||||
.expect("create policy directory");
|
||||
fs::write(policy_path, "prefix_rule(").expect("write malformed policy file");
|
||||
});
|
||||
let server = start_mock_server().await;
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
let call_id = "managed-shell-forbidden";
|
||||
let args = json!({
|
||||
"cmd": "echo blocked",
|
||||
"yield_time_ms": 1_000,
|
||||
});
|
||||
|
||||
mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-managed-1"),
|
||||
ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?),
|
||||
ev_completed("resp-managed-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let results_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-managed-1", "done"),
|
||||
ev_completed("resp-managed-2"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
test.submit_turn_with_approval_and_permission_profile(
|
||||
"run shell command",
|
||||
AskForApproval::Never,
|
||||
PermissionProfile::Disabled,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let output_item = results_mock.single_request().function_call_output(call_id);
|
||||
let output = output_item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.expect("function call output should include a string output payload");
|
||||
assert!(
|
||||
output.contains("policy forbids commands starting with `echo`"),
|
||||
"unexpected output: {output}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn shell_command_empty_script_with_collaboration_mode_does_not_panic() -> Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
|
||||
Reference in New Issue
Block a user