Reject widening app reviewer requirements

This commit is contained in:
Alex Zamoshchin
2026-06-02 15:48:20 -04:00
parent a18da7cca1
commit a928fbd39f
2 changed files with 70 additions and 2 deletions
@@ -1144,10 +1144,50 @@ impl ConfigRequirementsToml {
}
}
fn validate_app_approvals_reviewers_are_subsets(
allowed_approvals_reviewers: Option<&Sourced<Vec<ApprovalsReviewer>>>,
apps: Option<&Sourced<AppsRequirementsToml>>,
) -> 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<ConfigRequirementsWithSources> for ConfigRequirements {
type Error = ConstraintError;
fn try_from(toml: ConfigRequirementsWithSources) -> Result<Self, Self::Error> {
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#"
+5 -2
View File
@@ -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(),