[2.2.x] Fix panic in cedar_policy::PolicySet::link (#208)

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

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<ast::PolicySetError> 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)]