[Bugfix]: Loader can panic when handling nested maps (#415)

* fixing issue where loader can panick when handling nested maps * adding test case for previous crashing test case * addressing new clippy lints
aws-cloudformation · Nov 17, 2023 · 41adec0 · 41adec0
1 parent aa9694b
commit 41adec0
Show file tree

Hide file tree

Showing 10 changed files with 39 additions and 21 deletions.
diff --git a/guard/resources/validate/data-dir/s3-server-side-encryption-template-compliant.yaml b/guard/resources/validate/data-dir/s3-server-side-encryption-template-compliant.yaml
@@ -16,4 +16,4 @@ Resources:
            - ServerSideEncryptionByDefault:
                SSEAlgorithm: AES256
       VersioningConfiguration:
-        Status: Enabled
+        Status: Enabled
diff --git a/guard/resources/validate/nested_crash.yaml b/guard/resources/validate/nested_crash.yaml
@@ -0,0 +1,19 @@
+# ---
+# AWSTemplateFormatVersion: 2010-09-09
+# Description: CloudFormation - Server Side Encryption
+
+Resources:
+  MyBucket:
+    Type: AWS::S3::Bucket
+    Properties:
+      PublicAccessBlockConfiguration:
+        BlockPublicAcls: true
+        BlockPublicPolicy: true
+        IgnorePublicAcls: true
+        RestrictPublicBuckets: true
+      BucketEncryption:
+        ServerSideEncryptionConfiguration:
+          - ServerSideEncryptionByDefault:
+              SSEAlgorithm: { { CRASH } }
+      VersioningConfiguration:
+        Status: Enabled
diff --git a/guard/resources/validate/rules-dir/s3_bucket_server_side_encryption_enabled.guard b/guard/resources/validate/rules-dir/s3_bucket_server_side_encryption_enabled.guard
@@ -10,4 +10,4 @@ rule S3_BUCKET_SERVER_SIDE_ENCRYPTION_ENABLED when %s3_buckets_server_side_encry
     Violation: S3 Bucket must enable server-side encryption.
     Fix: Set the S3 Bucket property BucketEncryption.ServerSideEncryptionConfiguration.ServerSideEncryptionByDefault.SSEAlgorithm to either "aws:kms" or "AES256"
   >>
-}
+}
diff --git a/guard/src/commands/test.rs b/guard/src/commands/test.rs
@@ -154,7 +154,7 @@ or failure testing.
                                     .parent()
                                     .map_or("".to_string(), |p| format!("{}", p.display())),
                             )
-                            .or_insert(vec![])
+                            .or_default()
                             .push(GuardFile {
                                 prefix,
                                 file,
@@ -356,14 +356,15 @@ fn test_with_data(
                         eval_rules_file(rules, &mut root_scope, None)?; // we never use data file name in the output
                         let top = root_scope.reset_recorder().extract();
 
-                        let by_rules = top.children.iter().fold(HashMap::new(), |mut acc, rule| {
-                            if let Some(RecordType::RuleCheck(NamedStatus { name, .. })) =
-                                rule.container
-                            {
-                                acc.entry(name).or_insert(vec![]).push(&rule.container)
-                            }
-                            acc
-                        });
+                        let by_rules: HashMap<&str, Vec<&Option<RecordType<'_>>>> =
+                            top.children.iter().fold(HashMap::new(), |mut acc, rule| {
+                                if let Some(RecordType::RuleCheck(NamedStatus { name, .. })) =
+                                    rule.container
+                                {
+                                    acc.entry(name).or_default().push(&rule.container)
+                                }
+                                acc
+                            });
 
                         for (rule_name, rule) in by_rules {
                             let expected = match each.expectations.rules.get(rule_name) {

diff --git a/guard/src/commands/validate/common.rs b/guard/src/commands/validate/common.rs
@@ -709,12 +709,12 @@ pub(super) fn insert_into_trees<'report, 'value: 'report>(
 
     if let Some(from) = clause.value_from() {
         let path = from.self_path().0.to_string();
-        path_tree.entry(path).or_insert(vec![]).push(node.clone());
+        path_tree.entry(path).or_default().push(node.clone());
     }
 
     if let Some(from) = clause.value_to() {
         let path = from.self_path().0.to_string();
-        path_tree.entry(path).or_insert(vec![]).push(node);
+        path_tree.entry(path).or_default().push(node);
     }
 }
 

diff --git a/guard/src/rules/functions/converters.rs b/guard/src/rules/functions/converters.rs
@@ -27,7 +27,6 @@ pub(crate) fn parse_float(
                 }
                 PathAwareValue::Char((path, val)) => {
                     aggr.push(Some(PathAwareValue::Float((path.clone(), {
-                        let path = path;
                         val.to_digit(10).ok_or(crate::Error::ParseError(format!(
                             "failed to convert a character: {val} into a float at {path}"
                         )))
@@ -69,7 +68,6 @@ pub(crate) fn parse_int(args: &[QueryResult]) -> crate::rules::Result<Vec<Option
                 }
                 PathAwareValue::Char((path, val)) => {
                     aggr.push(Some(PathAwareValue::Int((path.clone(), {
-                        let path = path;
                         val.to_digit(10).ok_or(crate::Error::ParseError(format!(
                             "failed to convert a character: {val} into an integer at {path}"
                         )))

diff --git a/guard/src/rules/libyaml/loader.rs b/guard/src/rules/libyaml/loader.rs
@@ -157,8 +157,10 @@ impl Loader {
             let value = key_values.remove(0);
             let key_str = match key {
                 MarkedValue::String(val, loc) => (val, loc),
+                MarkedValue::Map(..) => continue,
                 _ => unreachable!(),
             };
+
             map.insert(key_str, value);
         }
     }

diff --git a/guard/src/rules/path_value_tests.rs b/guard/src/rules/path_value_tests.rs
@@ -393,7 +393,7 @@ fn merge_values_test() -> Result<(), Error> {
         _ => unreachable!(),
     };
     assert_eq!(resources_map.values.len(), 2);
-    assert!(matches!(resources_map.values.get("PARAMETERS"), Some(_)),);
+    assert!(resources_map.values.get("PARAMETERS").is_some());
 
     let parameters = PathAwareValue::try_from(serde_yaml::from_str::<serde_yaml::Value>(
         r#"

diff --git a/guard/tests/test_command.rs b/guard/tests/test_command.rs
@@ -104,8 +104,6 @@ mod test_command_tests {
                 args.push(String::from(self.rules_and_test_file.unwrap()));
             }
 
-            if self.directory_only {}
-
             if self.alphabetical {
                 args.push(format!("-{}", ALPHABETICAL.1));
             }

diff --git a/guard/tests/validate.rs b/guard/tests/validate.rs
@@ -195,13 +195,13 @@ mod validate_tests {
     #[case(vec!["blank-template.yaml"], vec!["s3_bucket_server_side_encryption_enabled_2.guard"], StatusCode::INTERNAL_FAILURE)]
     #[case(
         vec!["blank-template.yaml", "s3-server-side-encryption-template-non-compliant-2.yaml"],
-        vec!["s3_bucket_server_side_encryption_enabled_2.guard"],
-        StatusCode::INTERNAL_FAILURE
-    )]
+        vec!["s3_bucket_server_side_encryption_enabled_2.guard"], StatusCode::INTERNAL_FAILURE)]
     #[case(vec!["dne.yaml"], vec!["rules-dir/s3_bucket_public_read_prohibited.guard"], StatusCode::INTERNAL_FAILURE)]
     #[case(vec!["data-dir/s3-public-read-prohibited-template-non-compliant.yaml"], vec!["dne.guard"], StatusCode::INTERNAL_FAILURE)]
     #[case(vec!["blank.yaml"], vec!["rules-dir/s3_bucket_public_read_prohibited.guard"], StatusCode::INTERNAL_FAILURE)]
     #[case(vec!["s3-server-side-encryption-template-non-compliant-2.yaml"], vec!["comments.guard"], StatusCode::SUCCESS)]
+    #[case(vec!["s3-server-side-encryption-template-non-compliant-2.yaml"], vec!["comments.guard"], StatusCode::SUCCESS)]
+    #[case(vec!["nested_crash.yaml"], vec!["s3_bucket_server_side_encryption_enabled_2.guard"], StatusCode::VALIDATION_ERROR)]
     fn test_single_data_file_single_rules_file_status(
         #[case] data_arg: Vec<&str>,
         #[case] rules_arg: Vec<&str>,