From a928fbd39fffcd6def677f1bf75e651269b52578 Mon Sep 17 00:00:00 2001 From: Alex Zamoshchin Date: Tue, 2 Jun 2026 15:48:20 -0400 Subject: [PATCH] Reject widening app reviewer requirements --- codex-rs/config/src/config_requirements.rs | 65 ++++++++++++++++++++++ codex-rs/core/src/connectors_tests.rs | 7 ++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index f1b5fe349e..7c844dc17b 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -1144,10 +1144,50 @@ impl ConfigRequirementsToml { } } +fn validate_app_approvals_reviewers_are_subsets( + allowed_approvals_reviewers: Option<&Sourced>>, + apps: Option<&Sourced>, +) -> Result<(), ConstraintError> { + let (Some(allowed_approvals_reviewers), Some(apps)) = (allowed_approvals_reviewers, apps) + else { + return Ok(()); + }; + if allowed_approvals_reviewers.value.is_empty() { + return Ok(()); + } + + for (app_id, app_requirement) in &apps.value.apps { + let Some(app_allowed_approvals_reviewers) = + app_requirement.allowed_approvals_reviewers.as_deref() + else { + continue; + }; + + if let Some(reviewer) = app_allowed_approvals_reviewers + .iter() + .find(|reviewer| !allowed_approvals_reviewers.value.contains(reviewer)) + { + return Err(ConstraintError::InvalidValue { + field_name: "apps.*.allowed_approvals_reviewers", + candidate: format!("{reviewer:?} for app {app_id:?}"), + allowed: format!("{:?}", allowed_approvals_reviewers.value), + requirement_source: allowed_approvals_reviewers.source.clone(), + }); + } + } + + Ok(()) +} + impl TryFrom for ConfigRequirements { type Error = ConstraintError; fn try_from(toml: ConfigRequirementsWithSources) -> Result { + validate_app_approvals_reviewers_are_subsets( + toml.allowed_approvals_reviewers.as_ref(), + toml.apps.as_ref(), + )?; + // Profile catalog selection remains on ConfigRequirementsToml for // config loading and requirements API projection. The normalized // constraints below only need the compiled PermissionProfile envelope. @@ -2578,6 +2618,31 @@ allowed_approvals_reviewers = ["user"] Ok(()) } + #[test] + fn app_allowed_approvals_reviewers_must_be_subset_of_global_allow_list() -> Result<()> { + let toml_str = r#" + allowed_approvals_reviewers = ["auto_review"] + + [apps.calendar] + allowed_approvals_reviewers = ["user"] + "#; + let config: ConfigRequirementsToml = from_str(toml_str)?; + let err = ConfigRequirements::try_from(with_unknown_source(config)) + .expect_err("app reviewer allow-list should not widen the global allow-list"); + + assert_eq!( + err, + ConstraintError::InvalidValue { + field_name: "apps.*.allowed_approvals_reviewers", + candidate: "User for app \"calendar\"".to_string(), + allowed: "[AutoReview]".to_string(), + requirement_source: RequirementSource::Unknown, + } + ); + + Ok(()) + } + #[test] fn deserialize_allowed_windows_sandbox_implementations() -> Result<()> { let toml_str = r#" diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index eb4100cb3a..8b8db965eb 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -475,7 +475,7 @@ approvals_reviewer = "user" } #[tokio::test] -async fn app_approvals_reviewer_uses_first_resolved_app_requirement_when_constraints_conflict() { +async fn app_approvals_reviewer_uses_first_resolved_app_requirement_within_global_constraint() { let codex_home = tempdir().expect("tempdir should succeed"); std::fs::write( codex_home.path().join(CONFIG_TOML_FILE), @@ -485,7 +485,10 @@ approvals_reviewer = "user" ) .expect("write config"); let requirements = ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::User]), + allowed_approvals_reviewers: Some(vec![ + ApprovalsReviewer::User, + ApprovalsReviewer::AutoReview, + ]), apps: Some(AppsRequirementsToml { apps: BTreeMap::from([( "calendar".to_string(),