From a359f205bb366aadbb97d5ebf8d62494f85cb324 Mon Sep 17 00:00:00 2001 From: anweiss <2326106+anweiss@users.noreply.github.com> Date: Thu, 6 Mar 2025 16:14:19 -0500 Subject: [PATCH] fix nested array validation issues in #218 --- src/validator/cbor.rs | 153 +++++++++++++++++++++++++++++++-- src/validator/json.rs | 193 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 333 insertions(+), 13 deletions(-) diff --git a/src/validator/cbor.rs b/src/validator/cbor.rs index 05479313..5434af00 100644 --- a/src/validator/cbor.rs +++ b/src/validator/cbor.rs @@ -426,6 +426,11 @@ impl<'a> CBORValidator<'a> { ArrayItemToken::Range(lower, upper, is_inclusive) => { cv.visit_range(lower, upper, *is_inclusive)? } + // Special handling for nested arrays when using the Group variant + ArrayItemToken::Group(group) if v.is_array() => { + // Special handling for nested arrays + cv.visit_group(group)?; + } ArrayItemToken::Group(group) => cv.visit_group(group)?, ArrayItemToken::Identifier(ident) => cv.visit_identifier(ident)?, ArrayItemToken::TaggedData(tagged_data) => cv.visit_type2(tagged_data)?, @@ -480,6 +485,11 @@ impl<'a> CBORValidator<'a> { ArrayItemToken::Range(lower, upper, is_inclusive) => { cv.visit_range(lower, upper, *is_inclusive)? } + // Special nested array handling when using the Group variant + ArrayItemToken::Group(group) if v.is_array() => { + // Special handling for nested arrays + cv.visit_group(group)?; + } ArrayItemToken::Group(group) => cv.visit_group(group)?, ArrayItemToken::Identifier(ident) => cv.visit_identifier(ident)?, ArrayItemToken::TaggedData(tagged_data) => cv.visit_type2(tagged_data)?, @@ -633,6 +643,42 @@ where } fn visit_type(&mut self, t: &Type<'a>) -> visitor::Result> { + // Special case for nested array in literal position + if let Value::Array(outer_array) = &self.cbor { + if let Some(idx) = self.group_entry_idx { + // We're processing a specific array item + if let Some(item) = outer_array.get(idx) { + if item.is_array() { + // This is a nested array, check if we're supposed to validate against an array type + for tc in t.type_choices.iter() { + if let Type2::Array { .. } = &tc.type1.type2 { + #[cfg(all(feature = "additional-controls", target_arch = "wasm32"))] + let mut cv = + CBORValidator::new(self.cddl, item.clone(), self.enabled_features.clone()); + #[cfg(all(feature = "additional-controls", not(target_arch = "wasm32")))] + let mut cv = CBORValidator::new(self.cddl, item.clone(), self.enabled_features); + #[cfg(not(feature = "additional-controls"))] + let mut cv = CBORValidator::new(self.cddl, item.clone()); + + cv.generic_rules = self.generic_rules.clone(); + cv.eval_generic_rule = self.eval_generic_rule; + cv.is_multi_type_choice = self.is_multi_type_choice; + + let _ = write!(cv.cbor_location, "{}/{}", self.cbor_location, idx); + + // Visit the type choice with the inner array value + cv.visit_type_choice(tc)?; + + self.errors.append(&mut cv.errors); + return Ok(()); + } + } + } + } + } + } + + // Regular type processing if t.type_choices.len() > 1 { self.is_multi_type_choice = true; } @@ -2212,6 +2258,20 @@ where return Ok(()); } + // Special case for array values - check if we're in an array context and this + // is a reference to another array type + if let Value::Array(_) = &self.cbor { + if let Some(rule) = rule_from_ident(self.cddl, ident) { + if let Rule::Type { rule, .. } = rule { + for tc in rule.value.type_choices.iter() { + if let Type2::Array { .. } = &tc.type1.type2 { + return self.visit_type_choice(tc); + } + } + } + } + } + match &self.cbor { Value::Null if is_ident_null_data_type(self.cddl, ident) => Ok(()), Value::Bytes(_) if is_ident_byte_string_data_type(self.cddl, ident) => Ok(()), @@ -2453,7 +2513,7 @@ where .get_or_insert(vec![k.clone()]) .push(k.clone()); self.object_value = Some(v.clone()); - let _ = write!(self.cbor_location, "/{:?}", v); + self.cbor_location.push_str(&format!("/{}", value)); } else { self.add_error(format!("map requires entry key of type {}", ident)); } @@ -2468,7 +2528,7 @@ where .get_or_insert(vec![k.clone()]) .push(k.clone()); self.object_value = Some(v.clone()); - let _ = write!(self.cbor_location, "/{:?}", v); + self.cbor_location.push_str(&format!("/{}", value)); } else { self.add_error(format!("map requires entry key of type {}", ident)); } @@ -2482,7 +2542,7 @@ where .get_or_insert(vec![k.clone()]) .push(k.clone()); self.object_value = Some(v.clone()); - let _ = write!(self.cbor_location, "/{:?}", v); + self.cbor_location.push_str(&format!("/{}", value)); } else { self.add_error(format!("map requires entry key of type {}", ident)); } @@ -2496,7 +2556,7 @@ where .get_or_insert(vec![k.clone()]) .push(k.clone()); self.object_value = Some(v.clone()); - let _ = write!(self.cbor_location, "/{:?}", v); + self.cbor_location.push_str(&format!("/{}", value)); } else { self.add_error(format!("map requires entry key of type {}", ident)); } @@ -2510,7 +2570,7 @@ where .get_or_insert(vec![k.clone()]) .push(k.clone()); self.object_value = Some(v.clone()); - let _ = write!(self.cbor_location, "/{:?}", v); + self.cbor_location.push_str(&format!("/{}", value)); } else { self.add_error(format!("map requires entry key of type {}", ident)); } @@ -2524,7 +2584,7 @@ where .get_or_insert(vec![k.clone()]) .push(k.clone()); self.object_value = Some(v.clone()); - let _ = write!(self.cbor_location, "/{:?}", v); + self.cbor_location.push_str(&format!("/{}", value)); } else { self.add_error(format!("map requires entry key of type {}", ident)); } @@ -4093,7 +4153,10 @@ mod tests { #[cfg(not(feature = "additional-controls"))] let mut cv = CBORValidator::new(&cddl, outer_cbor.clone()); cv.validate()?; - assert!(cv.validate().is_ok()); + assert!( + cv.validate().is_ok(), + // "Nested array with wildcard should validate successfully" + ); // Test invalid inner CBOR (missing required field) let invalid_inner = ciborium::cbor!({ @@ -4115,4 +4178,80 @@ mod tests { Ok(()) } + + #[test] + fn validate_nested_arrays() -> std::result::Result<(), Box> { + // Test with explicit array type + let cddl = indoc!( + r#" + array = [0, [* int]] + "# + ); + + let cbor = ciborium::cbor!([0, [1, 2]]).unwrap(); + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + + let mut cv = CBORValidator::new(&cddl, cbor, None); + cv.validate()?; + + // Test with named arrays + let cddl = indoc!( + r#" + root = [0, inner] + inner = [* int] + "# + ); + + let cbor = ciborium::cbor!([0, [1, 2]]).unwrap(); + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + + let mut cv = CBORValidator::new(&cddl, cbor, None); + cv.validate()?; + + // Test with explicit array literals + let cddl = indoc!( + r#" + direct = [1, [2, 3]] + "# + ); + + let cbor = ciborium::cbor!([1, [2, 3]]).unwrap(); + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + + let mut cv = CBORValidator::new(&cddl, cbor, None); + cv.validate()?; // If this passes, our fix works + + Ok(()) + } + + #[test] + fn validate_direct_nested_array() -> std::result::Result<(), Box> { + // Simplest possible case with explicit literal array + let cddl = indoc!( + r#" + direct = [1, [2, 3]] + "# + ); + + let cbor = ciborium::cbor!([1, [2, 3]]).unwrap(); + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + + let mut cv = CBORValidator::new(&cddl, cbor, None); + + // Print detailed information for debugging + match cv.validate() { + Ok(_) => println!("Validation successful!"), + Err(e) => { + eprintln!("Validation error: {}", e); + if let Error::Validation(errors) = &e { + for (i, err) in errors.iter().enumerate() { + eprintln!("Error {}: {} at {}", i, err.reason, err.cbor_location); + } + } + return Err(e.into()); + } + } + + Ok(()) + } } diff --git a/src/validator/json.rs b/src/validator/json.rs index 78804e28..afbe464d 100644 --- a/src/validator/json.rs +++ b/src/validator/json.rs @@ -417,7 +417,13 @@ impl<'a> JSONValidator<'a> { } ArrayItemToken::Group(group) => jv.visit_group(group)?, ArrayItemToken::Identifier(ident) => jv.visit_identifier(ident)?, - _ => (), + ArrayItemToken::TaggedData(tag_data) => { + // Handle tagged data + jv.add_error(format!( + "Tagged data not supported in arrays: {:?}", + tag_data + )) + } } if self.is_multi_type_choice && jv.errors.is_empty() { @@ -461,9 +467,22 @@ impl<'a> JSONValidator<'a> { ArrayItemToken::Range(lower, upper, is_inclusive) => { jv.visit_range(lower, upper, *is_inclusive)? } - ArrayItemToken::Group(group) => jv.visit_group(group)?, + ArrayItemToken::Identifier(ident) => jv.visit_identifier(ident)?, - _ => (), + ArrayItemToken::TaggedData(tag_data) => { + // Handle tagged data + jv.add_error(format!( + "Tagged data not supported in arrays: {:?}", + tag_data + )) + } + // Special nested array handling - using Group variant instead + // as Array variant doesn't exist + ArrayItemToken::Group(group) if v.is_array() => { + // Special handling for nested arrays + jv.visit_group(group)?; + } + ArrayItemToken::Group(group) => jv.visit_group(group)?, } self.errors.append(&mut jv.errors); @@ -675,6 +694,42 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { } fn visit_type(&mut self, t: &Type<'a>) -> visitor::Result { + // Special case for nested array in literal position + if let Value::Array(outer_array) = &self.json { + if let Some(idx) = self.group_entry_idx { + // We're processing a specific array item + if let Some(item) = outer_array.get(idx) { + if item.is_array() { + // This is a nested array, check if we're supposed to validate against an array type + for tc in t.type_choices.iter() { + if let Type2::Array { .. } = &tc.type1.type2 { + #[cfg(all(feature = "additional-controls", target_arch = "wasm32"))] + let mut jv = + JSONValidator::new(self.cddl, item.clone(), self.enabled_features.clone()); + #[cfg(all(feature = "additional-controls", not(target_arch = "wasm32")))] + let mut jv = JSONValidator::new(self.cddl, item.clone(), self.enabled_features); + #[cfg(not(feature = "additional-controls"))] + let mut jv = JSONValidator::new(self.cddl, item.clone()); + + jv.generic_rules = self.generic_rules.clone(); + jv.eval_generic_rule = self.eval_generic_rule; + jv.is_multi_type_choice = self.is_multi_type_choice; + + let _ = write!(jv.json_location, "{}/{}", self.json_location, idx); + + // Visit the type choice with the inner array value + jv.visit_type_choice(tc)?; + + self.errors.append(&mut jv.errors); + return Ok(()); + } + } + } + } + } + } + + // Regular type processing if t.type_choices.len() > 1 { self.is_multi_type_choice = true; } @@ -1518,6 +1573,11 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { Ok(()) } + // Special case for nested array validation + _ if matches!(self.json, Value::Array(_)) => { + self.validate_array_items(&ArrayItemToken::Group(group))?; + Ok(()) + } _ => { self.add_error(format!("expected array type, got {}", self.json)); Ok(()) @@ -1533,7 +1593,7 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { if let Some(gr) = self .generic_rules .iter_mut() - .find(|gr| gr.name == ident.ident) + .find(|r| r.name == ident.ident) { for arg in ga.args.iter() { gr.args.push((*arg.arg).clone()); @@ -1596,7 +1656,7 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { if let Some(gr) = self .generic_rules .iter_mut() - .find(|gr| gr.name == ident.ident) + .find(|r| r.name == ident.ident) { for arg in ga.args.iter() { gr.args.push((*arg.arg).clone()); @@ -1756,6 +1816,20 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { return Ok(()); } + // Special case for array values - check if we're in an array context and this + // is a reference to another array type + if let Value::Array(_) = &self.json { + if let Some(rule) = rule_from_ident(self.cddl, ident) { + if let Rule::Type { rule, .. } = rule { + for tc in rule.value.type_choices.iter() { + if let Type2::Array { .. } = &tc.type1.type2 { + return self.visit_type_choice(tc); + } + } + } + } + } + match &self.json { Value::Null if is_ident_null_data_type(self.cddl, ident) => Ok(()), Value::Bool(b) => { @@ -1831,7 +1905,17 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { Ok(()) } - Value::Array(_) => self.validate_array_items(&ArrayItemToken::Identifier(ident)), + Value::Array(_) => { + // For arrays, check if this identifier refers to an array type rule first + if let Some(r) = rule_from_ident(self.cddl, ident) { + if is_array_type_rule(self.cddl, ident) { + return self.visit_rule(r); + } + } + + // Then fallback to normal array validation + self.validate_array_items(&ArrayItemToken::Identifier(ident)) + } Value::Object(o) => match &self.occurrence { #[cfg(feature = "ast-span")] Some(Occur::Optional { .. }) | None => { @@ -2441,6 +2525,23 @@ impl<'a> Visitor<'a, '_, Error> for JSONValidator<'a> { } } +// Helper function to check if an identifier refers to an array type rule +fn is_array_type_rule<'a>(cddl: &'a CDDL<'a>, ident: &Identifier<'a>) -> bool { + for r in cddl.rules.iter() { + if let Rule::Type { rule, .. } = r { + if rule.name.ident == ident.ident { + // Check if the type rule defines an array + for tc in rule.value.type_choices.iter() { + if let Type2::Array { .. } = &tc.type1.type2 { + return true; + } + } + } + } + } + false +} + #[cfg(test)] #[cfg(not(target_arch = "wasm32"))] mod tests { @@ -2816,4 +2917,84 @@ mod tests { Ok(()) } + + #[test] + fn validate_nested_arrays() -> std::result::Result<(), Box> { + // Test with explicit array type + let cddl = indoc!( + r#" + array = [0, [* int]] + "# + ); + + let json = r#"[0, [1, 2]]"#; + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + let json = serde_json::from_str::(json).map_err(json::Error::JSONParsing)?; + + let mut jv = JSONValidator::new(&cddl, json, None); + jv.validate()?; + + // Test with named arrays + let cddl = indoc!( + r#" + root = [0, inner] + inner = [* int] + "# + ); + + let json = r#"[0, [1, 2]]"#; + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + let json = serde_json::from_str::(json).map_err(json::Error::JSONParsing)?; + + let mut jv = JSONValidator::new(&cddl, json, None); + jv.validate()?; + + // Test with explicit array literals + let cddl = indoc!( + r#" + direct = [1, [2, 3]] + "# + ); + + let json = r#"[1, [2, 3]]"#; + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + let json = serde_json::from_str::(json).map_err(json::Error::JSONParsing)?; + + let mut jv = JSONValidator::new(&cddl, json, None); + jv.validate()?; // If this passes, our fix works + + Ok(()) + } + + #[test] + fn validate_direct_nested_array() -> std::result::Result<(), Box> { + // Simplest possible case with explicit literal array + let cddl = indoc!( + r#" + direct = [1, [2, 3]] + "# + ); + + let json = r#"[1, [2, 3]]"#; + let cddl = cddl_from_str(cddl, true).map_err(json::Error::CDDLParsing)?; + let json = serde_json::from_str::(json).map_err(json::Error::JSONParsing)?; + + let mut jv = JSONValidator::new(&cddl, json, None); + + // Print detailed information for debugging + match jv.validate() { + Ok(_) => println!("Validation successful!"), + Err(e) => { + eprintln!("Validation error: {}", e); + if let Error::Validation(errors) = &e { + for (i, err) in errors.iter().enumerate() { + eprintln!("Error {}: {} at {}", i, err.reason, err.json_location); + } + } + return Err(e.into()); + } + } + + Ok(()) + } }