From b8fa3f37d1ea074f3f9c81920ddaab2bb0824804 Mon Sep 17 00:00:00 2001 From: Jack Date: Tue, 29 Sep 2026 15:29:41 +0200 Subject: [PATCH] SFT-7485: bind the UR path sequence to the part it carries A multipart UR carries its sequence number and count twice, in the URI path and inside the CBOR part. Decoder::receive() parsed both and compared neither, then sized and stored from the inner pair, so the path could describe something other than what it carried. The encoder writes both from the same values - MultiPartDeserialized's Display renders the path out of fragment.sequence and fragment.sequence_count - so they agree for anything this library produced. Compare them before handing the part to the fountain, which is what allocates and mutates decoder state, and report both pairs in a new Error::InconsistentSequence. Caller-configurable maxima and structured heapless capacity errors are left for a separate change; they add public API. --- ur/src/ur/decoder.rs | 133 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/ur/src/ur/decoder.rs b/ur/src/ur/decoder.rs index 277a588..76e5612 100644 --- a/ur/src/ur/decoder.rs +++ b/ur/src/ur/decoder.rs @@ -126,6 +126,23 @@ impl BaseDecoder { }; let part = part.as_ref().unwrap_or_else(|| ur.as_part().unwrap()); + + // A multipart UR carries its sequence number and count twice: in the URI + // path and again inside the CBOR part. Callers gate on the path values, + // and everything below sizes and stores from the inner ones, so the two + // have to agree before the fountain sees anything. + let outer = match (ur.sequence(), ur.sequence_count()) { + (Some(sequence), Some(sequence_count)) => (sequence, sequence_count), + // is_multi_part() was checked above, so this cannot happen. Refusing + // rather than assuming it. + _ => return Err(Error::NotMultiPart), + }; + let inner = (part.sequence, part.sequence_count); + + if outer != inner { + return Err(Error::InconsistentSequence { outer, inner }); + } + self.fountain.receive(part)?; Ok(()) } @@ -288,6 +305,14 @@ pub enum Error { }, /// The UR type of this fragment is not consistent. InconsistentType, + /// The sequence number and count in the UR path do not match the ones in the + /// part it carries. + InconsistentSequence { + /// Sequence number and count taken from the UR path. + outer: (u32, u32), + /// Sequence number and count taken from the fountain part. + inner: (u32, u32), + }, } #[cfg(feature = "std")] @@ -311,6 +336,11 @@ impl fmt::Display for Error { f, "The received fragment is not consistent with the type of the previous fragments" ), + Error::InconsistentSequence { outer, inner } => write!( + f, + "The UR path describes part {}-{} but it carries part {}-{}", + outer.0, outer.1, inner.0, inner.1 + ), } } } @@ -332,3 +362,106 @@ impl From for Error { Self::Fountain(e) } } + +#[cfg(test)] +#[cfg(feature = "alloc")] +mod tests { + use super::*; + use crate::ur::{tests::make_message_ur, Encoder}; + use alloc::{format, string::String, string::ToString}; + + /// Rewrite the `-` path segment, leaving the fragment alone. + fn with_path(ur: &str, sequence: u32, sequence_count: u32) -> String { + let mut parts = ur.splitn(3, '/'); + let head = parts.next().unwrap(); + let _indices = parts.next().unwrap(); + let fragment = parts.next().unwrap(); + + format!("{head}/{sequence}-{sequence_count}/{fragment}") + } + + #[test] + fn test_path_hiding_a_larger_sequence_is_rejected() { + let message = make_message_ur(200, "Wolf"); + let mut encoder = Encoder::new(); + encoder.start("bytes", &message, 50); + assert!(encoder.sequence_count() > 1); + + // Two different parts of a multipart message, both wearing a 1-1 path. A + // caller gating on the path sees two copies of one single part. + let first = with_path(&encoder.next_part().to_string(), 1, 1); + let second = with_path(&encoder.next_part().to_string(), 1, 1); + + let mut decoder = Decoder::default(); + for disguised in [first, second] { + assert!(matches!( + decoder.receive(UR::parse(&disguised).unwrap()), + Err(Error::InconsistentSequence { .. }) + )); + } + + assert!(!decoder.is_complete()); + assert_eq!(decoder.message().unwrap(), None); + } + + #[test] + fn test_inner_sequence_past_its_count_is_rejected() { + let message = make_message_ur(200, "Wolf"); + let mut encoder = Encoder::new(); + encoder.start("bytes", &message, 50); + + // Parts past the sequence count are mixed parts, so this one's inner + // sequence is greater than its inner count. + let count = encoder.sequence_count(); + let mut part = encoder.next_part(); + for _ in 0..count { + part = encoder.next_part(); + } + let part = part.to_string(); + assert!(part.starts_with(&format!("ur:bytes/{}-{count}/", count + 1))); + + let disguised = with_path(&part, 1, 1); + + let mut decoder = Decoder::default(); + assert!(matches!( + decoder.receive(UR::parse(&disguised).unwrap()), + Err(Error::InconsistentSequence { .. }) + )); + } + + #[test] + fn test_mismatched_count_alone_is_rejected() { + let message = make_message_ur(200, "Wolf"); + let mut encoder = Encoder::new(); + encoder.start("bytes", &message, 50); + + let count = encoder.sequence_count(); + let part = encoder.next_part().to_string(); + + // Right sequence number, wrong count. + let disguised = with_path(&part, 1, count + 1); + + let mut decoder = Decoder::default(); + assert!(matches!( + decoder.receive(UR::parse(&disguised).unwrap()), + Err(Error::InconsistentSequence { .. }) + )); + } + + #[test] + fn test_untouched_parts_still_decode() { + let message = make_message_ur(200, "Wolf"); + let mut encoder = Encoder::new(); + encoder.start("bytes", &message, 50); + + // Through the string form, so this is the same UR::MultiPart path the + // checks above reject. + let mut decoder = Decoder::default(); + while !decoder.is_complete() { + let part = encoder.next_part().to_string(); + decoder.receive(UR::parse(&part).unwrap()).unwrap(); + } + + assert_eq!(decoder.message().unwrap(), Some(message.as_slice())); + } +}