diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index ea0cebcba..0d23cbccd 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +## 2.2.1 + +### Fixed + +- Fix a panic in `PolicySet::link()` that could occur when the function was called + with a policy id corresponding to a static policy. + ## 2.2.0 ### Added diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index 2991861dd..3f471db3b 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -1052,6 +1052,9 @@ pub enum PolicySetError { /// Expected an static policy, but a template-linked policy was provided. #[error("Expected static policy, but a template-linked policy was provided")] ExpectedStatic, + /// Expected a template, but a static policy was provided. + #[error("expected a template, but a static policy was provided")] + ExpectedTemplate, } impl From for PolicySetError { @@ -1211,9 +1214,12 @@ impl PolicySet { /// Attempt to link a template and add the new template-linked policy to the policy set. /// If link fails, the `PolicySet` is not modified. - /// Failure can happen for two reasons + /// Failure can happen for three reasons /// 1) The map passed in `vals` may not match the slots in the template - /// 2) The `new_id` may conflict w/ an policy that already exists in the set + /// 2) The `new_id` may conflict w/ a policy that already exists in the set + /// 3) `template_id` does not correspond to a template. Either the id is + /// not in the policy set, or it is in the policy set but is either a + /// linked or static policy rather than a template #[allow(clippy::needless_pass_by_value)] pub fn link( &mut self, @@ -1238,11 +1244,20 @@ impl PolicySet { let lined_est = self .templates .get(&template_id) - .expect("instantiate() didn't fail above, so this shouldn't fail") + // We know `template_id` exists in the policy set as either a + // template or a static policy because otherwise `ast.link()` would + // have errored. If `ast.link()` did not error, then it could still + // be that the id corresponds to a static policy. This function + // should only be used to link templates, so this is an error. + .ok_or(PolicySetError::ExpectedTemplate)? .clone() .est .link(&est_vals) - .expect("instantiate() didn't fail above, so this shouldn't fail"); + // The only error case for `est.link()` is a template with + // slots which are not filled by the provided values. `ast.link()` + // will have already errored if there are any unfilled slots in the + // template. + .expect("ast.link() didn't fail above, so this shouldn't fail"); self.policies.insert( new_id, Policy { @@ -2474,6 +2489,7 @@ mod head_constraints_tests { mod policy_set_tests { use super::*; use ast::LinkingError; + use cool_asserts::assert_matches; #[test] fn link_conflicts() { @@ -2626,6 +2642,56 @@ mod policy_set_tests { &"linked".parse().unwrap() ); } + + #[test] + fn link_static_policy() { + // Linking the `PolicyId` of a static policy should not be allowed. + // Attempting it should cause an `ExpectedTemplate` error. + let static_policy = Policy::parse( + Some("static".into()), + "permit(principal, action, resource);", + ) + .expect("Static parse failure"); + let mut pset = PolicySet::new(); + pset.add(static_policy).unwrap(); + + let result = pset.link( + PolicyId::from_str("static").unwrap(), + PolicyId::from_str("linked").unwrap(), + HashMap::new(), + ); + assert_matches!(result, Err(PolicySetError::ExpectedTemplate)); + } + + #[test] + fn link_linked_policy() { + let template = Template::parse( + Some("template".into()), + "permit(principal == ?principal, action, resource);", + ) + .expect("Template Parse Failure"); + let mut pset = PolicySet::new(); + pset.add_template(template).unwrap(); + + pset.link( + PolicyId::from_str("template").unwrap(), + PolicyId::from_str("linked").unwrap(), + std::iter::once((SlotId::principal(), EntityUid::from_strs("Test", "test"))).collect(), + ) + .unwrap(); + + let result = pset.link( + PolicyId::from_str("linked").unwrap(), + PolicyId::from_str("linked2").unwrap(), + HashMap::new(), + ); + assert_matches!( + result, + Err(PolicySetError::LinkingError(LinkingError::NoSuchTemplate( + _ + ))) + ); + } } #[cfg(test)]