[BugFix]: Gracefully handle keys for maps that are not of type string (…

…#418)

* [BugFix]: fix for mappings that start with what could possibly be a null yaml value

* [Misc]: renamed file with crashing maps, updated reference in test for that file

* [Misc]: fix failing loader tests

* [BugFix]: gracefully handling non string type keys

* [Misc]: adding more test cases

* [Misc]: improving error messages for InvalidKeyType variant of InternalError

guard/src/commands/validate.rs

@@ -23,7 +23,7 @@ use crate::commands::{
     OUTPUT_FORMAT, PAYLOAD, PRINT_JSON, REQUIRED_FLAGS, RULES, RULE_FILE_SUPPORTED_EXTENSIONS,
     SHOW_SUMMARY, STRUCTURED, TYPE, VALIDATE, VERBOSE,
 };
-use crate::rules::errors::Error;
+use crate::rules::errors::{Error, InternalError};
 use crate::rules::eval::eval_rules_file;
 use crate::rules::eval_context::{root_scope, EventRecord};
 use crate::rules::exprs::RulesFile;
@@ -688,7 +688,11 @@ fn build_data_file(content: String, name: String) -> Result<DataFile> {
 
     let path_value = match crate::rules::values::read_from(&content) {
         Ok(value) => PathAwareValue::try_from(value)?,
-        Err(_) => {
+        Err(e) => {
+            if matches!(e, Error::InternalError(InternalError::InvalidKeyType(..))) {
+                return Err(Error::ParseError(e.to_string()));
+            }
+
             let str_len: usize = cmp::min(content.len(), 100);
             return Err(Error::ParseError(format!(
                 "Error encountered while parsing data file: {name}, data beginning with \n{}\n ...",

guard/src/commands/validate/cfn.rs

@@ -23,6 +23,7 @@ use crate::{
     rules::{
         self,
         display::ValueOnlyDisplay,
+        errors::InternalError::UnresolvedKeyForReporter,
         eval_context::{
             simplifed_json_from_root, BinaryComparison, ClauseReport, EventRecord, FileReport,
             InComparison, RuleReport, UnaryComparison,
@@ -197,9 +198,13 @@ fn single_line(
             let resource_name = match CFN_RESOURCES.captures(key) {
                 Ok(Some(cap)) => cap.get(1).unwrap().as_str(),
                 _ => {
-                    return Err(crate::Error::InternalError(String::from(
-                        "Unable to resolve key {key} for single line-summary when expecting a cloudformation template, falling back on next reporter"
-                    )));
+                    return Err(crate::Error::InternalError(
+                        UnresolvedKeyForReporter(
+                            String::from(
+                                "Unable to resolve key {key} for single line-summary when expecting a cloudformation template, falling back on next reporter"
+                            )
+                        )
+                    ));
                 }
             };
 

guard/src/rules/errors.rs

@@ -45,8 +45,16 @@ pub enum Error {
     Errors(#[from] Errors),
     #[error("{0}")]
     IllegalArguments(String),
+    #[error("{0}")]
+    InternalError(InternalError),
+}
+
+#[derive(Debug, Error)]
+pub enum InternalError {
+    #[error("non string type detected for key in a map at {0}, cfn-guard only supports keys that are string types")]
+    InvalidKeyType(String),
     #[error("internal error {0}")]
-    InternalError(String),
+    UnresolvedKeyForReporter(String),
 }
 
 #[derive(Debug, Error)]

guard/src/rules/libyaml/loader.rs

@@ -2,7 +2,7 @@ use std::borrow::Cow;
 
 use crate::rules::{
     self,
-    errors::Error,
+    errors::{Error, InternalError::InvalidKeyType},
     libyaml::{
         event::{Event, Scalar, ScalarStyle, SequenceStart},
         parser::Parser,
@@ -43,7 +43,7 @@ impl Loader {
                         return Ok(self.documents.pop().unwrap());
                     }
                     Event::MappingStart(..) => self.handle_mapping_start(location),
-                    Event::MappingEnd => self.handle_mapping_end(),
+                    Event::MappingEnd => self.handle_mapping_end()?,
                     Event::SequenceStart(sequence_start) => {
                         self.handle_sequence_start(sequence_start, location)
                     }
@@ -145,7 +145,7 @@ impl Loader {
         self.last_container_index.push(self.stack.len() - 1);
     }
 
-    fn handle_mapping_end(&mut self) {
+    fn handle_mapping_end(&mut self) -> crate::rules::Result<()> {
         let map_index = self.last_container_index.pop().unwrap();
         let mut key_values: Vec<MarkedValue> = self.stack.drain(map_index + 1..).collect();
         let map = match self.stack.last_mut().unwrap() {
@@ -157,12 +157,17 @@ impl Loader {
             let value = key_values.remove(0);
             let key_str = match key {
                 MarkedValue::String(val, loc) => (val, loc),
-                MarkedValue::Map(..) => continue,
-                _ => unreachable!(),
+                val => {
+                    return Err(Error::InternalError(InvalidKeyType(
+                        val.location().to_string(),
+                    )));
+                }
             };
 
             map.insert(key_str, value);
         }
+
+        Ok(())
     }
 
     fn handle_mapping_start(&mut self, location: Location) {

guard/src/rules/values.rs

@@ -431,7 +431,10 @@ pub(crate) fn read_from(from_reader: &str) -> crate::rules::Result<MarkedValue>
     let mut loader = Loader::new();
     match loader.load(from_reader.to_string()) {
         Ok(doc) => Ok(doc),
-        Err(e) => Err(Error::ParseError(format!("{}", e))),
+        Err(e) => match e {
+            Error::InternalError(..) => Err(e),
+            _ => Err(Error::ParseError(format!("{}", e))),
+        },
     }
 }
 

guard/tests/validate.rs

@@ -201,7 +201,6 @@ mod validate_tests {
     #[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>,
@@ -217,6 +216,24 @@ mod validate_tests {
         assert_eq!(expected_status_code, status_code);
     }
 
+    #[rstest::rstest]
+    #[case("SSEAlgorithm: {{CRASH}}")]
+    #[case("~:")]
+    #[case("[1, 2, 3]: foo")]
+    #[case("1: foo")]
+    #[case("1.0: foo")]
+    fn test_graceful_handling_when_yaml_file_has_non_string_type_key(#[case] input: &str) {
+        let bytes = input.as_bytes();
+        let mut reader = Reader::new(ReadCursor(Cursor::new(bytes.to_vec())));
+        let mut writer = Writer::new(Stdout(stdout()), Stderr(stderr()));
+
+        let status_code = ValidateTestRunner::default()
+            .rules(vec!["s3_bucket_server_side_encryption_enabled_2.guard"])
+            .run(&mut writer, &mut reader);
+
+        assert_eq!(StatusCode::INTERNAL_FAILURE, status_code);
+    }
+
     #[test]
     fn test_single_data_file_single_rules_file_compliant() {
         let mut reader = Reader::new(Stdin(std::io::stdin()));