diff --git a/core/src/sdp/mod.rs b/core/src/sdp/mod.rs index 6ae0e6888..aa9be1642 100644 --- a/core/src/sdp/mod.rs +++ b/core/src/sdp/mod.rs @@ -626,11 +626,12 @@ mod tests { fn empty_locators_fail_to_deserialize() { let empty_locators = Vec::::new(); let serialized = serde_json::to_string(&empty_locators).unwrap(); - assert_eq!( + assert!( serde_json::from_str::(&serialized) .unwrap_err() - .to_string(), - "Input cannot be empty." + .to_string() + .contains("Input cannot be empty."), + "empty locators should be rejected" ); } diff --git a/mmr/src/path.rs b/mmr/src/path.rs index 095b80d26..17ddec19e 100644 --- a/mmr/src/path.rs +++ b/mmr/src/path.rs @@ -113,11 +113,8 @@ pub enum MerklePathError { mod serde_siblings { use lb_groth16::{Fr, serde::serde_fr_vec}; - use lb_utils::bounded::UpperBoundedVec; - use serde::{ - Deserialize, Deserializer, Serializer, - de::{SeqAccess, Visitor}, - }; + use lb_utils::bounded::{UpperBoundedVec, deserialize_bounded_sequence}; + use serde::{Deserialize, Deserializer, Serializer}; use super::{MAX_MERKLE_PATH_SIBLINGS, MerklePathSiblings}; @@ -135,35 +132,15 @@ mod serde_siblings { #[derive(Deserialize)] struct FrWrap(#[serde(with = "lb_groth16::serde::serde_fr")] Fr); - struct SiblingsVisitor; - - impl<'de> Visitor<'de> for SiblingsVisitor { - type Value = Vec; - - fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - formatter.write_str("a sequence of MMR sibling field elements") - } - - fn visit_seq(self, mut sequence: A) -> Result - where - A: SeqAccess<'de>, - { - let mut siblings = Vec::with_capacity(MAX_MERKLE_PATH_SIBLINGS); - while let Some(FrWrap(sibling)) = sequence.next_element()? { - if siblings.len() == MAX_MERKLE_PATH_SIBLINGS { - return Err(serde::de::Error::custom(format_args!( - "MMR path contains more than {MAX_MERKLE_PATH_SIBLINGS} siblings" - ))); - } - siblings.push(sibling); - } - Ok(siblings) - } - } - - let siblings = deserializer.deserialize_seq(SiblingsVisitor)?; - UpperBoundedVec::::try_from(siblings) - .map_err(serde::de::Error::custom) + let siblings = + deserialize_bounded_sequence::(deserializer)?; + Ok(UpperBoundedVec::new_unchecked( + siblings + .into_inner() + .into_iter() + .map(|FrWrap(sibling)| sibling) + .collect(), + )) } } diff --git a/utils/src/bounded/mod.rs b/utils/src/bounded/mod.rs index 9529999f3..8f9f8446c 100644 --- a/utils/src/bounded/mod.rs +++ b/utils/src/bounded/mod.rs @@ -2,8 +2,8 @@ //! inclusive `[MIN, MAX]` range. //! //! [`Bounded`] captures the machinery shared by every length-bounded type in -//! the codebase — bound checking, unchecked/checked construction, transparent -//! serialization and validating deserialization — so that concrete bounded +//! the codebase — bound checking, unchecked/checked construction, and +//! transparent serialization — so that concrete bounded //! types (`BoundedVec`, chain IDs, locators, …) reduce to a type alias plus //! whatever operations are natural for the wrapped type. //! @@ -19,10 +19,13 @@ pub mod vec; use core::fmt::{self, Display, Formatter}; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Serialize, Serializer}; pub use string::BoundedString; use thiserror::Error; -pub use vec::{BoundedVec, LowerBoundedVec, MaxBoundedVec, NonEmptyBoundedVec, UpperBoundedVec}; +pub use vec::{ + BoundedVec, LowerBoundedVec, MaxBoundedVec, NonEmptyBoundedVec, UpperBoundedVec, + deserialize_bounded_sequence, +}; #[derive(Debug, Error, Eq, PartialEq, Clone)] pub enum BoundedError { @@ -46,7 +49,7 @@ pub enum BoundedError { /// element count for collections, byte length for text. /// /// Implementing this for a type unlocks the ergonomic checked constructors on -/// [`Bounded`] ([`Bounded::new`], `TryFrom`, `Deserialize`, [`Bounded::len`]). +/// [`Bounded`] ([`Bounded::new`], `TryFrom`, [`Bounded::len`]). /// Foreign types that cannot get an impl here can still be bounded manually via /// [`Bounded::check_len`] + [`Bounded::new_unchecked`]. pub trait BoundedLen { @@ -152,14 +155,3 @@ where self.0.serialize(serializer) } } - -// Deserialize the inner `T`, then re-establish the bound before wrapping. -impl<'de, T, const MIN: usize, const MAX: usize> Deserialize<'de> for Bounded -where - T: BoundedLen + Deserialize<'de>, -{ - fn deserialize>(deserializer: D) -> Result { - let inner = T::deserialize(deserializer)?; - Self::try_new(inner).map_err(serde::de::Error::custom) - } -} diff --git a/utils/src/bounded/multiaddr.rs b/utils/src/bounded/multiaddr.rs index e17d37796..2019f5a63 100644 --- a/utils/src/bounded/multiaddr.rs +++ b/utils/src/bounded/multiaddr.rs @@ -1,4 +1,5 @@ use multiaddr::Multiaddr; +use serde::{Deserialize, Deserializer}; use crate::bounded::{Bounded, BoundedError, BoundedLen, BoundedVec}; @@ -11,11 +12,19 @@ impl BoundedLen for Multiaddr { /// A `Multiaddr` whose byte length is statically enforced to be in the range /// `[MIN, MAX]`. /// -/// A thin alias over [`Bounded`]. Length checking, (de)serialization, `Display` -/// and unchecked construction all come from the generic wrapper; only the -/// multiaddr-flavoured conversions live here. +/// A thin alias over [`Bounded`]. Length checking, serialization, `Display` and +/// unchecked construction come from the generic wrapper; multiaddr +/// deserialization and the remaining multiaddr-flavoured conversions live +/// here. pub type BoundedMultiaddr = Bounded; +impl<'de, const MIN: usize, const MAX: usize> Deserialize<'de> for BoundedMultiaddr { + fn deserialize>(deserializer: D) -> Result { + let value = Multiaddr::deserialize(deserializer)?; + Self::try_new(value).map_err(serde::de::Error::custom) + } +} + impl BoundedMultiaddr { /// Length in bytes (not `char`s), matching `Multiaddr` semantics. #[must_use] diff --git a/utils/src/bounded/string.rs b/utils/src/bounded/string.rs index d4c31894c..8fa22ec87 100644 --- a/utils/src/bounded/string.rs +++ b/utils/src/bounded/string.rs @@ -1,3 +1,5 @@ +use serde::{Deserialize, Deserializer}; + use crate::bounded::{Bounded, BoundedError, BoundedLen, BoundedVec}; impl BoundedLen for String { @@ -9,11 +11,18 @@ impl BoundedLen for String { /// A `String` whose byte length is statically enforced to be in the range /// `[MIN, MAX]`. /// -/// A thin alias over [`Bounded`]. Length checking, (de)serialization, `Display` -/// and unchecked construction all come from the generic wrapper; only the -/// string-flavoured conversions live here. +/// A thin alias over [`Bounded`]. Length checking, serialization, `Display` and +/// unchecked construction come from the generic wrapper; string deserialization +/// and the remaining string-flavoured conversions live here. pub type BoundedString = Bounded; +impl<'de, const MIN: usize, const MAX: usize> Deserialize<'de> for BoundedString { + fn deserialize>(deserializer: D) -> Result { + let value = String::deserialize(deserializer)?; + Self::try_new(value).map_err(serde::de::Error::custom) + } +} + impl BoundedString { /// Length in bytes (not `char`s), matching `str`/`String` semantics. #[must_use] diff --git a/utils/src/bounded/vec.rs b/utils/src/bounded/vec.rs index 21bf75496..4b0468679 100644 --- a/utils/src/bounded/vec.rs +++ b/utils/src/bounded/vec.rs @@ -1,9 +1,15 @@ use core::{ + marker::PhantomData, ops::Deref, slice::{Iter, IterMut}, }; use std::{ops::DerefMut, str::FromStr, vec::IntoIter}; +use serde::{ + Deserialize, Deserializer, + de::{Error as _, SeqAccess, Visitor}, +}; + use crate::bounded::{Bounded, BoundedError, BoundedLen}; impl BoundedLen for Vec { @@ -15,15 +21,85 @@ impl BoundedLen for Vec { /// `Vec` whose length is statically enforced to be in the range `[MIN, /// MAX]`. /// -/// A thin alias over [`Bounded`]: the length checking, (de)serialization and -/// construction machinery lives on the generic wrapper, while the operations -/// below are the ones that only make sense for a `Vec`. +/// A thin alias over [`Bounded`]: the length checking and construction +/// machinery lives on the generic wrapper, while sequence deserialization and +/// the operations below are the ones that only make sense for a `Vec`. /// /// The invariant is enforced at every checked construction site /// ([`TryFrom>`](Self::try_from), deserialization), so an instance can /// never be shorter than `MIN` nor longer than `MAX`. pub type BoundedVec = Bounded, MIN, MAX>; +impl<'de, T, const MIN: usize, const MAX: usize> Deserialize<'de> for Bounded, MIN, MAX> +where + T: Deserialize<'de>, +{ + fn deserialize>(deserializer: D) -> Result { + deserialize_bounded_sequence(deserializer) + } +} + +/// Deserialize a sequence directly into a bounded vector. +/// +/// Sequence formats may provide a length through [`SeqAccess::size_hint`]. +/// When that length exceeds `MAX`, it is rejected before any element is +/// decoded. Formats without a reliable hint are still bounded by stopping at +/// the first element beyond `MAX`. +pub fn deserialize_bounded_sequence<'de, T, const MIN: usize, const MAX: usize, D>( + deserializer: D, +) -> Result, D::Error> +where + T: Deserialize<'de>, + D: Deserializer<'de>, +{ + deserializer.deserialize_seq(BoundedSequenceVisitor { + marker: PhantomData, + }) +} + +struct BoundedSequenceVisitor { + marker: PhantomData, +} + +impl<'de, T, const MIN: usize, const MAX: usize> Visitor<'de> + for BoundedSequenceVisitor +where + T: Deserialize<'de>, +{ + type Value = BoundedVec; + + fn expecting(&self, formatter: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + write!(formatter, "a sequence with between {MIN} and {MAX} items") + } + + fn visit_seq(self, mut sequence: A) -> Result + where + A: SeqAccess<'de>, + { + let size_hint = sequence.size_hint(); + if let Some(size_hint) = size_hint.filter(|&size_hint| size_hint > MAX) { + return Err(A::Error::custom(BoundedError::TooManyItems { + count: size_hint, + max: MAX, + })); + } + + let capacity = size_hint.unwrap_or(0).min(MAX); + let mut values = Vec::with_capacity(capacity); + while let Some(value) = sequence.next_element()? { + if values.len() == MAX { + return Err(A::Error::custom(BoundedError::TooManyItems { + count: MAX.saturating_add(1), + max: MAX, + })); + } + values.push(value); + } + + BoundedVec::try_from(values).map_err(A::Error::custom) + } +} + impl Bounded, MIN, MAX> { /// Constructs an empty vector. /// @@ -343,6 +419,13 @@ pub type MaxBoundedVec = UpperBoundedVec; #[cfg(test)] mod tests { + use std::sync::{ + Mutex, + atomic::{AtomicUsize, Ordering}, + }; + + use serde::{Deserialize, Deserializer}; + use crate::bounded::{BoundedError, BoundedVec, UpperBoundedVec}; /// Concrete instantiation used across the tests: between 2 and 4 elements. @@ -350,6 +433,18 @@ mod tests { type TestBoundedVectorMin1 = BoundedVec; type TestBoundedVectorMin0 = BoundedVec; + static ELEMENT_ATTEMPTS: AtomicUsize = AtomicUsize::new(0); + static ELEMENT_ATTEMPTS_TEST_LOCK: Mutex<()> = Mutex::new(()); + + struct CountingByte; + + impl<'de> Deserialize<'de> for CountingByte { + fn deserialize>(deserializer: D) -> Result { + ELEMENT_ATTEMPTS.fetch_add(1, Ordering::Relaxed); + u8::deserialize(deserializer).map(|_| Self) + } + } + #[test] fn from_accepts_single_element_construction() { let single = TestBoundedVectorMin0::from(1); @@ -546,6 +641,15 @@ mod tests { assert_eq!(bv.as_slice(), &[1, 2, 3]); } + #[test] + fn deserialize_accepts_inputs_at_bounds() { + let min: TestBoundedVectorMin2 = serde_json::from_str("[1,2]").unwrap(); + assert_eq!(min.as_slice(), &[1, 2]); + + let max: TestBoundedVectorMin2 = serde_json::from_str("[1,2,3,4]").unwrap(); + assert_eq!(max.as_slice(), &[1, 2, 3, 4]); + } + #[test] fn serialize_then_deserialize_roundtrips() { let original = TestBoundedVectorMin2::try_from(vec![5, 6, 7, 8]).unwrap(); @@ -582,6 +686,39 @@ mod tests { ); } + #[test] + fn deserialize_json_stops_after_at_most_one_element_past_maximum() { + let _test_guard = ELEMENT_ATTEMPTS_TEST_LOCK.lock().unwrap(); + ELEMENT_ATTEMPTS.store(0, Ordering::Relaxed); + + let result = serde_json::from_str::>("[1,2,3,4,5,6]"); + + assert!(result.is_err()); + assert!(ELEMENT_ATTEMPTS.load(Ordering::Relaxed) <= 5); + } + + #[test] + fn deserialize_binary_rejects_oversized_length_before_decoding_elements() { + let _test_guard = ELEMENT_ATTEMPTS_TEST_LOCK.lock().unwrap(); + ELEMENT_ATTEMPTS.store(0, Ordering::Relaxed); + let encoded = bincode::serialize(&vec![1u8; 5]).unwrap(); + + let result = bincode::deserialize::>(&encoded); + + assert!(result.is_err()); + assert_eq!(ELEMENT_ATTEMPTS.load(Ordering::Relaxed), 0); + } + + #[test] + fn deserialize_binary_preserves_the_vector_wire_format() { + let original = TestBoundedVectorMin2::try_from(vec![5, 6, 7]).unwrap(); + let encoded = bincode::serialize(&original).unwrap(); + let restored = bincode::deserialize::(&encoded).unwrap(); + + assert_eq!(restored, original); + assert_eq!(encoded, bincode::serialize(&vec![5u8, 6, 7]).unwrap()); + } + #[test] fn try_pop_returns_none_at_or_below_lower_bound_and_is_idempotent() { let mut bv = TestBoundedVectorMin2::try_from(vec![1, 2, 3]).unwrap();