From 1ddc989697a9b16ca16f00f94b7af59d68be4c1e Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 16:17:52 +0200 Subject: [PATCH 01/12] Enforce declared total body length during assembly `FirstFrameHeader::total_body_len` was only used as an input to the configured size check. The completion path removed the partial assembly and dispatched its body without comparing the accumulated length to the declaration, and a single-frame message returned before any comparison. A peer could therefore declare any total that passed the size check, send a different number of bytes, and have the body dispatched unchanged. Treat the declaration as authoritative and enforce it at four points: - a first frame whose body exceeds its declaration is rejected with `DeclaredTotalTooSmall`, because the declaration can never be satisfied; - a single-frame message must match its declaration exactly, or `DeclaredTotalMismatch` is returned; - a continuation that would push the body past the declaration is rejected with `DeclaredTotalExceeded` before its bytes are buffered; and - the completing frame must bring the body to exactly the declared total, or `DeclaredTotalMismatch` is returned. Rejections free the partial assembly, so a mismatched message never stays buffered. Protocols that leave the total as `None` are unaffected. To keep `state.rs` within its 400-line limit, the in-flight state moves to `partial.rs` (which owns the completion check) and the pure predicates to `validation.rs`. The inbound adapter already maps `MessageAssemblyError` to `io::ErrorKind::InvalidData`, so no adapter change is required. Closes #544. Co-Authored-By: Claude Code --- src/message_assembler/error.rs | 46 ++++++++++ src/message_assembler/mod.rs | 2 + src/message_assembler/partial.rs | 99 ++++++++++++++++++++++ src/message_assembler/state.rs | 116 ++++++++++++-------------- src/message_assembler/validation.rs | 125 ++++++++++++++++++++++++++++ 5 files changed, 325 insertions(+), 63 deletions(-) create mode 100644 src/message_assembler/partial.rs create mode 100644 src/message_assembler/validation.rs diff --git a/src/message_assembler/error.rs b/src/message_assembler/error.rs index 86b5c33db..4c0fe7fbc 100644 --- a/src/message_assembler/error.rs +++ b/src/message_assembler/error.rs @@ -129,4 +129,50 @@ pub enum MessageAssemblyError { /// Configured in-flight byte cap. limit: NonZeroUsize, }, + + /// The first frame declared fewer total body bytes than it carried. + /// + /// A declaration smaller than the first frame's own body can never be + /// satisfied, and accepting it would let a peer understate the size of the + /// message that is actually dispatched. + #[error( + "declared total body length too small for key {key}: declared {declared} bytes, first \ + frame carries {body_len}" + )] + DeclaredTotalTooSmall { + /// Message key whose declaration is unsatisfiable. + key: MessageKey, + /// Total body length declared by the first frame. + declared: usize, + /// Body bytes actually carried by the first frame. + body_len: usize, + }, + + /// Appending this frame would carry the body past its declared total. + #[error( + "declared total body length exceeded for key {key}: declared {declared} bytes, \ + accumulated {attempted}" + )] + DeclaredTotalExceeded { + /// Message key whose declaration was exceeded. + key: MessageKey, + /// Total body length declared by the first frame. + declared: usize, + /// Body bytes that would result from accepting the frame. + attempted: usize, + }, + + /// The completed message's body length differed from its declared total. + #[error( + "declared total body length mismatch for key {key}: declared {declared} bytes, assembled \ + {actual}" + )] + DeclaredTotalMismatch { + /// Message key whose declaration was not honoured. + key: MessageKey, + /// Total body length declared by the first frame. + declared: usize, + /// Body bytes actually assembled. + actual: usize, + }, } diff --git a/src/message_assembler/mod.rs b/src/message_assembler/mod.rs index d6362f27b..ce5198bd9 100644 --- a/src/message_assembler/mod.rs +++ b/src/message_assembler/mod.rs @@ -23,9 +23,11 @@ mod budget; pub mod error; mod header; +mod partial; pub mod series; pub mod state; pub mod types; +mod validation; use std::io; diff --git a/src/message_assembler/partial.rs b/src/message_assembler/partial.rs new file mode 100644 index 000000000..41f1d3352 --- /dev/null +++ b/src/message_assembler/partial.rs @@ -0,0 +1,99 @@ +//! In-flight message assembly state. +//! +//! [`PartialAssembly`] holds the bytes and routing metadata accumulated +//! between a first frame and the frame that completes the message. Keeping +//! this state in its own module lets [`MessageAssemblyState`] concentrate on +//! admission decisions while the completion check lives next to the data it +//! validates. +//! +//! [`MessageAssemblyState`]: super::state::MessageAssemblyState + +use std::time::Instant; + +use super::{ + ContinuationFrameHeader, + MessageKey, + error::{MessageAssemblyError, MessageSeriesError, MessageSeriesStatus}, + series::MessageSeries, + types::{AssembledMessage, EnvelopeRouting}, + validation, +}; + +/// Partial message assembly in progress. +#[derive(Debug)] +pub(super) struct PartialAssembly { + /// Ordering state that rejects gaps, duplicates, and post-completion data. + series: MessageSeries, + /// Envelope routing retained until the assembled message is emitted. + routing: EnvelopeRouting, + /// First-frame metadata kept separate from the body for reconstruction. + metadata: Vec, + /// Ordered body bytes accumulated from first and continuation frames. + body: Vec, + /// Start time used to discard incomplete peer-controlled state. + pub(super) started_at: Instant, +} + +impl PartialAssembly { + /// Start a new assembly from an admitted first frame. + pub(super) fn new( + series: MessageSeries, + routing: EnvelopeRouting, + metadata: Vec, + body: &[u8], + started_at: Instant, + ) -> Self { + Self { + series, + routing, + metadata, + body: body.to_vec(), + started_at, + } + } + + /// Body bytes buffered so far, excluding metadata. + pub(super) const fn body_len(&self) -> usize { self.body.len() } + + /// Declared total body length, if the first frame supplied one. + pub(super) fn declared_total(&self) -> Option { self.series.expected_total() } + + /// Validate continuity and report whether the series is now complete. + pub(super) fn accept_continuation( + &mut self, + header: &ContinuationFrameHeader, + ) -> Result { + self.series.accept_continuation(header) + } + + /// Append an admitted continuation body. + pub(super) fn push_body(&mut self, body: &[u8]) { self.body.extend_from_slice(body); } + + /// Bytes retained for this assembly, including metadata. + pub(super) const fn buffered_bytes(&self) -> usize { + self.metadata.len().saturating_add(self.body.len()) + } + + /// Consume the assembly, enforcing any declared total. + /// + /// The partial state is always freed. A mismatch is reported instead of + /// dispatching a body whose length contradicts the peer's declaration. + /// + /// # Errors + /// + /// Returns [`MessageAssemblyError::DeclaredTotalMismatch`] when the + /// assembled body length differs from the declared total. + pub(super) fn into_message(self) -> Result, MessageAssemblyError> { + let key: MessageKey = self.series.message_key(); + if let Some(declared) = self.series.expected_total() { + validation::validate_completion(key, declared, self.body.len())?; + } + + Ok(Some(AssembledMessage::new( + key, + self.routing, + self.metadata, + self.body, + ))) + } +} diff --git a/src/message_assembler/state.rs b/src/message_assembler/state.rs index ff68d8e02..b38adb57e 100644 --- a/src/message_assembler/state.rs +++ b/src/message_assembler/state.rs @@ -15,44 +15,12 @@ use super::{ MessageKey, budget::{AggregateBudgets, check_aggregate_budgets, check_size_limit}, error::{MessageAssemblyError, MessageSeriesError, MessageSeriesStatus}, + partial::PartialAssembly, series::MessageSeries, - types::{AssembledMessage, EnvelopeRouting, FirstFrameInput}, + types::{AssembledMessage, FirstFrameInput}, + validation, }; -/// Partial message assembly in progress. -#[derive(Debug)] -struct PartialAssembly { - /// Ordering state that rejects gaps, duplicates, and post-completion data. - series: MessageSeries, - /// Envelope routing retained until the assembled message is emitted. - routing: EnvelopeRouting, - /// First-frame metadata kept separate from the body for reconstruction. - metadata: Vec, - /// Ordered body bytes accumulated from first and continuation frames. - body: Vec, - /// Start time used to discard incomplete peer-controlled state. - started_at: Instant, -} -impl PartialAssembly { - /// Create empty assembly storage anchored at the first frame's timestamp. - const fn new(series: MessageSeries, routing: EnvelopeRouting, started_at: Instant) -> Self { - Self { - series, - routing, - metadata: Vec::new(), - body: Vec::new(), - started_at, - } - } - /// Append body bytes after the caller has validated size and ordering. - fn push_body(&mut self, data: &[u8]) { self.body.extend_from_slice(data); } - /// Replace the first-frame metadata retained for the completed message. - fn set_metadata(&mut self, data: Vec) { self.metadata = data; } - /// Return body bytes accumulated so far for per-message budget checks. - const fn accumulated_len(&self) -> usize { self.body.len() } - /// Logical payload length represented by the body and metadata buffers. - const fn buffered_bytes(&self) -> usize { self.body.len().saturating_add(self.metadata.len()) } -} /// Stateful manager for multiple concurrent message assemblies. /// /// Tracks in-flight assemblies keyed by [`MessageKey`], applying continuity @@ -165,8 +133,10 @@ impl MessageAssemblyState { /// # Errors /// /// Returns [`MessageAssemblyError::DuplicateFirstFrame`] if an assembly - /// for this key is already in progress, or [`MessageAssemblyError::MessageTooLarge`] - /// if the body exceeds the configured limit. + /// for this key is already in progress, + /// [`MessageAssemblyError::MessageTooLarge`] if the body exceeds the + /// configured limit, or a declared-total error when the declaration is + /// unsatisfiable or not honoured. pub fn accept_first_frame( &mut self, input: FirstFrameInput<'_>, @@ -180,8 +150,10 @@ impl MessageAssemblyState { /// # Errors /// /// Returns [`MessageAssemblyError::DuplicateFirstFrame`] if an assembly - /// for this key is already in progress, or [`MessageAssemblyError::MessageTooLarge`] - /// if the body exceeds the configured limit. + /// for this key is already in progress, + /// [`MessageAssemblyError::MessageTooLarge`] if the body exceeds the + /// configured limit, or a declared-total error when the declaration is + /// unsatisfiable or not honoured. pub fn accept_first_frame_at( &mut self, input: FirstFrameInput<'_>, @@ -195,6 +167,10 @@ impl MessageAssemblyState { return Err(MessageAssemblyError::DuplicateFirstFrame { key }); } + // A declared total is a protocol commitment: reject declarations that + // the first frame already contradicts before consulting size limits. + validation::validate_first_frame(input.header, input.body.len())?; + // Validate message size (prefer declared total body length, include metadata) let declared_body_len = input.header.total_body_len.unwrap_or(input.body.len()); let total_message_size = declared_body_len.saturating_add(input.metadata.len()); @@ -234,9 +210,7 @@ impl MessageAssemblyState { // Start new assembly, preserving envelope routing metadata from the // first frame so the completed message is dispatched correctly. - let mut partial = PartialAssembly::new(series, input.routing, now); - partial.set_metadata(input.metadata); - partial.push_body(input.body); + let partial = PartialAssembly::new(series, input.routing, input.metadata, input.body, now); self.assemblies.insert(key, partial); Ok(None) @@ -321,7 +295,7 @@ impl MessageAssemblyState { }; // Validate continuity - let status = match entry.get_mut().series.accept_continuation(header) { + let status = match entry.get_mut().accept_continuation(header) { Ok(s) => s, Err(e) => { if Self::is_unrecoverable_continuity_error(&e) { @@ -331,33 +305,49 @@ impl MessageAssemblyState { } }; - // Check size limit - let accumulated = entry.get().accumulated_len(); - if let Err(e) = check_size_limit(max_message_size, key, accumulated, body.len()) { + let partial = entry.get_mut(); + if let Err(e) = Self::admit_continuation( + key, + partial, + body.len(), + buffered_total, + max_message_size, + &budgets, + ) { entry.remove(); return Err(e); } + partial.push_body(body); - // Check aggregate budgets - if let Err(e) = check_aggregate_budgets(key, buffered_total, body.len(), &budgets) { - entry.remove(); - return Err(e); - } + let MessageSeriesStatus::Complete = status else { + return Ok(None); + }; + entry.remove().into_message() + } - entry.get_mut().push_body(body); + /// Apply size, budget, and declared-total admission checks to a + /// continuation body before it is buffered. + /// + /// Any rejection frees the assembly, so the caller removes the entry on + /// error. + fn admit_continuation( + key: MessageKey, + partial: &PartialAssembly, + body_len: usize, + buffered_total: usize, + max_message_size: NonZeroUsize, + budgets: &AggregateBudgets, + ) -> Result<(), MessageAssemblyError> { + let accumulated = partial.body_len(); + check_size_limit(max_message_size, key, accumulated, body_len)?; + check_aggregate_budgets(key, buffered_total, body_len, budgets)?; - match status { - MessageSeriesStatus::Incomplete => Ok(None), - MessageSeriesStatus::Complete => { - let partial = entry.remove(); - Ok(Some(AssembledMessage::new( - key, - partial.routing, - partial.metadata, - partial.body, - ))) - } + // Enforce the declared total before buffering so an oversized + // continuation cannot be retained even briefly. + if let Some(declared) = partial.declared_total() { + validation::validate_continuation(key, declared, accumulated, body_len)?; } + Ok(()) } /// Remove any partial assemblies that exceeded the configured timeout. diff --git a/src/message_assembler/validation.rs b/src/message_assembler/validation.rs new file mode 100644 index 000000000..8314fc887 --- /dev/null +++ b/src/message_assembler/validation.rs @@ -0,0 +1,125 @@ +//! Declared-total validation for message assembly. +//! +//! A protocol may declare the total body length of a logical message on its +//! first frame. That declaration is a commitment, not merely a hint: it is used +//! to reject messages that cannot fit the configured size limit, and it is +//! enforced against the bytes that are actually accumulated. Treating it as +//! advisory would let a peer understate the size of the message that is +//! dispatched (`CWE-20`, `CWE-130`). +//! +//! The helpers here are pure so that the state machine in +//! [`super::state`] can apply them at each frame boundary without growing that +//! module past its size budget. +//! +//! Contract, in full: +//! +//! - a declaration smaller than the first frame's own body can never be satisfied, so the first +//! frame is rejected; +//! - when the first frame is also the last, its body must equal the declaration exactly; +//! - each continuation must keep the accumulated body within the declaration; overshooting is +//! rejected as soon as it is detected; and +//! - the final frame must bring the accumulated body to exactly the declared total. +//! +//! Protocols that supply `None` are unaffected. + +use super::{FirstFrameHeader, MessageKey, error::MessageAssemblyError}; + +/// Validate the declaration carried by a first frame. +/// +/// Checks that the declaration is satisfiable by the first frame alone, and +/// that a single-frame message matches its declaration exactly. +/// +/// # Errors +/// +/// Returns [`MessageAssemblyError::DeclaredTotalTooSmall`] when the declaration +/// is smaller than the first frame's body, or +/// [`MessageAssemblyError::DeclaredTotalMismatch`] when the first frame is also +/// the final frame and its body does not equal the declaration. +pub(super) fn validate_first_frame( + header: &FirstFrameHeader, + body_len: usize, +) -> Result<(), MessageAssemblyError> { + let Some(declared) = header.total_body_len else { + return Ok(()); + }; + + let key = header.message_key; + if declared < body_len { + return Err(MessageAssemblyError::DeclaredTotalTooSmall { + key, + declared, + body_len, + }); + } + + if header.is_last && declared != body_len { + return Err(MessageAssemblyError::DeclaredTotalMismatch { + key, + declared, + actual: body_len, + }); + } + + Ok(()) +} + +/// Validate that appending `body_len` bytes keeps the assembly within its +/// declared total. +/// +/// Called before the bytes are buffered, so a peer cannot use a single +/// oversized continuation to overrun the declaration and only be detected at +/// completion. +/// +/// # Errors +/// +/// Returns [`MessageAssemblyError::DeclaredTotalExceeded`] when the accumulated +/// body would exceed the declared total, including when the addition would +/// overflow `usize`. +pub(super) fn validate_continuation( + key: MessageKey, + declared: usize, + accumulated: usize, + body_len: usize, +) -> Result<(), MessageAssemblyError> { + let Some(attempted) = accumulated.checked_add(body_len) else { + return Err(MessageAssemblyError::DeclaredTotalExceeded { + key, + declared, + attempted: usize::MAX, + }); + }; + + if attempted > declared { + return Err(MessageAssemblyError::DeclaredTotalExceeded { + key, + declared, + attempted, + }); + } + + Ok(()) +} + +/// Validate that a completed assembly matches its declared total exactly. +/// +/// # Errors +/// +/// Returns [`MessageAssemblyError::DeclaredTotalMismatch`] when the assembled +/// body length differs from the declaration. This catches an under-declared +/// multi-frame message, whose shorter-than-declared body would otherwise be +/// dispatched unchanged. +pub(super) fn validate_completion( + key: MessageKey, + declared: usize, + actual: usize, +) -> Result<(), MessageAssemblyError> { + if actual != declared { + return Err(MessageAssemblyError::DeclaredTotalMismatch { + key, + declared, + actual, + }); + } + + Ok(()) +} From ca5875f0d73ce6db6e4340806fba824a05d44361 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 16:18:05 +0200 Subject: [PATCH 02/12] Add declared-total regression tests Cover every enforcement point from issue #544 directly against the domain state machine: - an under-declared single-frame total; - an over-declared single-frame total; - a first frame that carries more than a multi-frame declaration; - a continuation that would overshoot the declaration; - a final frame short of the declaration; - exact single-frame and multi-frame declarations; - a non-final frame that exactly reaches the declaration; - the `None` opt-out; and - overflow when accumulating the attempted length. The inbound adapter tests prove that a rejection is reported as `InvalidData` through the deserialization-failure counter and that the partial assembly is freed, so a malformed message is never dispatched. The testkit expectation enum and the budget assertion helper gain the three new variants so exhaustive matches keep compiling. Co-Authored-By: Claude Code --- .../assembly_tests/declared_total.rs | 109 +++++++ src/app/frame_handling/assembly_tests/mod.rs | 1 + src/message_assembler/declared_total_tests.rs | 296 ++++++++++++++++++ src/message_assembler/tests.rs | 7 + src/testkit/reassembly/message_error.rs | 51 +++ tests/fixtures/budget_enforcement.rs | 3 + tests/reassembly_assertion_helpers.rs | 30 ++ 7 files changed, 497 insertions(+) create mode 100644 src/app/frame_handling/assembly_tests/declared_total.rs create mode 100644 src/message_assembler/declared_total_tests.rs diff --git a/src/app/frame_handling/assembly_tests/declared_total.rs b/src/app/frame_handling/assembly_tests/declared_total.rs new file mode 100644 index 000000000..ceb59c8fc --- /dev/null +++ b/src/app/frame_handling/assembly_tests/declared_total.rs @@ -0,0 +1,109 @@ +//! Inbound-adapter coverage for declared-total enforcement (issue #544). +//! +//! The domain owns the declared-total checks, but the observable contract is +//! that the inbound adapter converts a rejection into `InvalidData`, counts a +//! deserialization failure, and never dispatches the message. These tests pin +//! that end-to-end behaviour. + +use std::sync::Arc; + +use rstest::rstest; + +use super::{ + inbound_envelope, + load_message_assembly_state, + message_assembler, + message_assembly_state, + ok, + process_assembly_frame, +}; +use crate::{ + message_assembler::{FrameSequence, MessageAssemblyState, MessageKey}, + test_helpers, +}; + +#[rstest] +fn inbound_assembly_rejects_under_declared_single_frame( + message_assembler: Arc, + message_assembly_state: Result, +) { + let mut state = Some(load_message_assembly_state(message_assembly_state)); + let mut deser_failures = 0_u32; + // Declares 3 bytes but carries 5 on the only frame. + let frame = inbound_envelope( + 1, + ok( + test_helpers::first_frame_payload(MessageKey::from(5), b"hello", true, Some(3)), + "valid test payload", + ), + ); + + let result = ok( + process_assembly_frame(&message_assembler, &mut state, &mut deser_failures, frame), + "malformed declaration should be handled", + ); + + assert!( + result.is_none(), + "a declaration the first frame contradicts must not dispatch" + ); + assert_eq!( + deser_failures, 1, + "declared-total rejection should count as one failure" + ); +} + +#[rstest] +fn inbound_assembly_rejects_continuation_overshooting_declared_total( + message_assembler: Arc, + message_assembly_state: Result, +) { + let mut state = Some(load_message_assembly_state(message_assembly_state)); + let mut deser_failures = 0_u32; + let first = inbound_envelope( + 1, + ok( + test_helpers::first_frame_payload(MessageKey::from(5), b"hello", false, Some(8)), + "valid test payload", + ), + ); + let result = ok( + process_assembly_frame(&message_assembler, &mut state, &mut deser_failures, first), + "first frame should process", + ); + assert!(result.is_none(), "first frame should not complete"); + + // 5 + 4 exceeds the declared 8. + let overrun = inbound_envelope( + 1, + ok( + test_helpers::continuation_frame_payload( + MessageKey::from(5), + FrameSequence::from(1), + b"more", + false, + ), + "valid test payload", + ), + ); + let result = ok( + process_assembly_frame(&message_assembler, &mut state, &mut deser_failures, overrun), + "overshooting continuation should be handled", + ); + + assert!( + result.is_none(), + "an overshooting continuation must not dispatch" + ); + assert_eq!( + deser_failures, 1, + "declared-total rejection should count as one failure" + ); + assert_eq!( + state + .as_ref() + .map_or(usize::MAX, MessageAssemblyState::buffered_count), + 0, + "the rejected assembly must be freed" + ); +} diff --git a/src/app/frame_handling/assembly_tests/mod.rs b/src/app/frame_handling/assembly_tests/mod.rs index 33dad6b46..0fbfaae82 100644 --- a/src/app/frame_handling/assembly_tests/mod.rs +++ b/src/app/frame_handling/assembly_tests/mod.rs @@ -354,5 +354,6 @@ fn inbound_assembly_timeout_purges_partial_state( ); } +mod declared_total; mod pass_through; mod routing_metadata; diff --git a/src/message_assembler/declared_total_tests.rs b/src/message_assembler/declared_total_tests.rs new file mode 100644 index 000000000..a7da80ae2 --- /dev/null +++ b/src/message_assembler/declared_total_tests.rs @@ -0,0 +1,296 @@ +//! Regression tests for declared total body length enforcement (issue #544). +//! +//! A first frame may declare the total body length of its logical message. +//! That declaration is a protocol commitment: it must agree with the bytes +//! that are actually assembled, and a message that cannot honour it must be +//! rejected rather than dispatched. These tests pin every enforcement point. + +use std::{num::NonZeroUsize, time::Duration}; + +use rstest::fixture; + +use crate::message_assembler::{ + AssembledMessage, + ContinuationFrameHeader, + EnvelopeRouting, + FirstFrameHeader, + FirstFrameInput, + FrameSequence, + MessageAssemblyError, + MessageAssemblyState, + MessageKey, + test_helpers::{first_header, first_header_with_total}, + validation, +}; + +/// State with a limit large enough that declared-total checks are decisive. +#[fixture] +fn subject() -> MessageAssemblyState { + let size = NonZeroUsize::new(1024).unwrap_or(NonZeroUsize::MIN); + MessageAssemblyState::new(size, Duration::from_secs(30)) +} + +/// Build a first-frame header carrying `total` and the requested final flag. +fn first_with_total(key: u64, body_len: usize, total: usize, is_last: bool) -> FirstFrameHeader { + FirstFrameHeader { + is_last, + ..first_header_with_total(key, body_len, total) + } +} + +/// Feed a first frame and return the state-machine result. +fn accept_first( + state: &mut MessageAssemblyState, + header: &FirstFrameHeader, + body: &[u8], +) -> Result, MessageAssemblyError> { + let input = FirstFrameInput::new(header, EnvelopeRouting::default(), vec![], body) + .expect("header lengths match body"); + state.accept_first_frame(input) +} + +/// Feed a sequenced continuation frame and return the state-machine result. +fn accept_continuation( + state: &mut MessageAssemblyState, + key: u64, + sequence: u32, + body: &[u8], + is_last: bool, +) -> Result, MessageAssemblyError> { + let header = ContinuationFrameHeader { + message_key: MessageKey(key), + sequence: Some(FrameSequence(sequence)), + body_len: body.len(), + is_last, + }; + state.accept_continuation_frame(&header, body) +} + +// ============================================================================= +// Single-frame declarations +// ============================================================================= + +#[test] +fn single_frame_under_declared_total_is_rejected() { + let mut subject = subject(); + // Declares 3 bytes but carries 5: the declaration can never be satisfied. + let header = first_with_total(1, 5, 3, true); + + let err = accept_first(&mut subject, &header, b"hello").expect_err("under-declared total"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalTooSmall { + key: MessageKey(1), + declared: 3, + body_len: 5, + } + ); + assert_eq!(subject.buffered_count(), 0); +} + +#[test] +fn single_frame_over_declared_total_is_rejected() { + let mut subject = subject(); + // Declares 10 bytes but is the final frame with only 5. + let header = first_with_total(1, 5, 10, true); + + let err = accept_first(&mut subject, &header, b"hello").expect_err("over-declared total"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalMismatch { + key: MessageKey(1), + declared: 10, + actual: 5, + } + ); + assert_eq!(subject.buffered_count(), 0); +} + +#[test] +fn single_frame_exact_declaration_is_dispatched() { + let mut subject = subject(); + let header = first_with_total(1, 5, 5, true); + + let msg = accept_first(&mut subject, &header, b"hello") + .expect("exact declaration accepted") + .expect("single frame completes immediately"); + + assert_eq!(msg.body(), b"hello"); + assert_eq!(subject.buffered_count(), 0); +} + +// ============================================================================= +// Multi-frame declarations +// ============================================================================= + +#[test] +fn first_frame_under_declared_multi_frame_total_is_rejected() { + let mut subject = subject(); + // Even a non-final first frame may not carry more than its own total. + let header = first_with_total(1, 5, 4, false); + + let err = accept_first(&mut subject, &header, b"hello").expect_err("under-declared total"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalTooSmall { + key: MessageKey(1), + declared: 4, + body_len: 5, + } + ); + assert_eq!(subject.buffered_count(), 0); +} + +#[test] +fn continuation_exceeding_declared_total_is_rejected() { + let mut subject = subject(); + let header = first_with_total(1, 5, 8, false); + accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); + assert_eq!(subject.buffered_count(), 1); + + // 5 + 4 = 9 exceeds the declared 8. + let err = + accept_continuation(&mut subject, 1, 1, b"more", false).expect_err("declaration exceeded"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalExceeded { + key: MessageKey(1), + declared: 8, + attempted: 9, + } + ); + assert_eq!( + subject.buffered_count(), + 0, + "the assembly must be freed after overshooting its declaration" + ); +} + +#[test] +fn final_frame_short_of_declared_total_is_rejected() { + let mut subject = subject(); + let header = first_with_total(1, 5, 10, false); + accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); + + // 5 + 4 = 9, within the declaration, but the series completes here. + let err = + accept_continuation(&mut subject, 1, 1, b"more", true).expect_err("final frame mismatch"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalMismatch { + key: MessageKey(1), + declared: 10, + actual: 9, + } + ); + assert_eq!( + subject.buffered_count(), + 0, + "the assembly must be freed after a completion mismatch" + ); +} + +#[test] +fn multi_frame_exact_declaration_is_dispatched() { + let mut subject = subject(); + let header = first_with_total(1, 5, 10, false); + accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); + + let msg = accept_continuation(&mut subject, 1, 1, b"world", true) + .expect("declaration honoured") + .expect("message completes"); + + assert_eq!(msg.body(), b"helloworld"); + assert_eq!(subject.buffered_count(), 0); +} + +#[test] +fn non_final_frame_may_exactly_reach_the_declaration() { + let mut subject = subject(); + let header = first_with_total(1, 5, 10, false); + accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); + + // Exactly reaches the declaration without completing. + let msg = accept_continuation(&mut subject, 1, 1, b"world", false) + .expect("equals the declaration, so accepted"); + assert!(msg.is_none()); + assert_eq!(subject.buffered_count(), 1); + + // No bytes remain, so the completing frame must be rejected. + let err = accept_continuation(&mut subject, 1, 2, b"!", true) + .expect_err("no room remains within the declaration"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalExceeded { + key: MessageKey(1), + declared: 10, + attempted: 11, + } + ); + assert_eq!(subject.buffered_count(), 0); +} + +// ============================================================================= +// Protocols without a declaration +// ============================================================================= + +#[test] +fn undeclared_total_is_unaffected() { + let mut subject = subject(); + let header = first_header(1, 5, false); + accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); + + let msg = accept_continuation(&mut subject, 1, 1, b"worlds", true) + .expect("undeclared total imposes no equality check") + .expect("message completes"); + + assert_eq!(msg.body(), b"helloworlds"); +} + +#[test] +fn declared_total_equal_to_first_frame_body_leaves_no_room() { + let mut subject = subject(); + // The boundary case: declared == body_len with more frames expected. + let header = first_with_total(1, 5, 5, false); + accept_first(&mut subject, &header, b"hello").expect("boundary declaration accepted"); + + let err = accept_continuation(&mut subject, 1, 1, b"!", true) + .expect_err("no bytes remain within the declaration"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalExceeded { + key: MessageKey(1), + declared: 5, + attempted: 6, + } + ); + assert_eq!(subject.buffered_count(), 0); +} + +// ============================================================================= +// Pure helper boundaries +// ============================================================================= + +/// The accumulator helper must report overflow as an exceeded declaration +/// rather than wrapping to a small value that appears to fit. +#[test] +fn continuation_accumulation_overflow_is_reported_as_exceeded() { + let err = validation::validate_continuation(MessageKey(1), usize::MAX, usize::MAX, 1) + .expect_err("sum overflows usize"); + + assert_eq!( + err, + MessageAssemblyError::DeclaredTotalExceeded { + key: MessageKey(1), + declared: usize::MAX, + attempted: usize::MAX, + } + ); +} diff --git a/src/message_assembler/tests.rs b/src/message_assembler/tests.rs index a045bc5c3..b582f677b 100644 --- a/src/message_assembler/tests.rs +++ b/src/message_assembler/tests.rs @@ -206,3 +206,10 @@ mod state_tests; #[path = "budget_tests.rs"] mod budget_tests; + +// ============================================================================= +// Declared total enforcement tests (issue #544) - see declared_total_tests.rs +// ============================================================================= + +#[path = "declared_total_tests.rs"] +mod declared_total_tests; diff --git a/src/testkit/reassembly/message_error.rs b/src/testkit/reassembly/message_error.rs index a81f35dbe..b52de2d0f 100644 --- a/src/testkit/reassembly/message_error.rs +++ b/src/testkit/reassembly/message_error.rs @@ -49,6 +49,21 @@ pub enum MessageAssemblyErrorExpectation { /// Message key whose frame triggered the budget guard. key: MessageKey, }, + /// Expect a declaration smaller than the first frame's own body. + DeclaredTotalTooSmall { + /// Message key whose declaration is unsatisfiable. + key: MessageKey, + }, + /// Expect a continuation that would overshoot the declared total. + DeclaredTotalExceeded { + /// Message key whose declaration was exceeded. + key: MessageKey, + }, + /// Expect a completed body that differs from the declared total. + DeclaredTotalMismatch { + /// Message key whose declaration was not honoured. + key: MessageKey, + }, } /// Match a structured assembly error against the expected protocol failure. @@ -78,6 +93,15 @@ pub(super) fn matches_message_error( MessageAssemblyErrorExpectation::InFlightBudgetExceeded { key } => { matches_in_flight_budget_exceeded(err, key) } + MessageAssemblyErrorExpectation::DeclaredTotalTooSmall { key } => { + matches_declared_total_too_small(err, key) + } + MessageAssemblyErrorExpectation::DeclaredTotalExceeded { key } => { + matches_declared_total_exceeded(err, key) + } + MessageAssemblyErrorExpectation::DeclaredTotalMismatch { key } => { + matches_declared_total_mismatch(err, key) + } } } @@ -156,3 +180,30 @@ fn matches_in_flight_budget_exceeded(err: &MessageAssemblyError, key: MessageKey if *actual_key == key ) } + +/// Match a first-frame declaration smaller than the frame's own body. +fn matches_declared_total_too_small(err: &MessageAssemblyError, key: MessageKey) -> bool { + matches!( + err, + MessageAssemblyError::DeclaredTotalTooSmall { key: actual_key, .. } + if *actual_key == key + ) +} + +/// Match a continuation that would overshoot the declared total. +fn matches_declared_total_exceeded(err: &MessageAssemblyError, key: MessageKey) -> bool { + matches!( + err, + MessageAssemblyError::DeclaredTotalExceeded { key: actual_key, .. } + if *actual_key == key + ) +} + +/// Match a completed body whose length differs from the declared total. +fn matches_declared_total_mismatch(err: &MessageAssemblyError, key: MessageKey) -> bool { + matches!( + err, + MessageAssemblyError::DeclaredTotalMismatch { key: actual_key, .. } + if *actual_key == key + ) +} diff --git a/tests/fixtures/budget_enforcement.rs b/tests/fixtures/budget_enforcement.rs index 753699c7b..64288add0 100644 --- a/tests/fixtures/budget_enforcement.rs +++ b/tests/fixtures/budget_enforcement.rs @@ -262,6 +262,9 @@ impl BudgetEnforcementWorld { MessageAssemblyError::InFlightBudgetExceeded { .. } => "InFlightBudgetExceeded", MessageAssemblyError::MessageTooLarge { .. } => "MessageTooLarge", MessageAssemblyError::DuplicateFirstFrame { .. } => "DuplicateFirstFrame", + MessageAssemblyError::DeclaredTotalTooSmall { .. } => "DeclaredTotalTooSmall", + MessageAssemblyError::DeclaredTotalExceeded { .. } => "DeclaredTotalExceeded", + MessageAssemblyError::DeclaredTotalMismatch { .. } => "DeclaredTotalMismatch", MessageAssemblyError::Series(_) => "Series", }; if actual != expected { diff --git a/tests/reassembly_assertion_helpers.rs b/tests/reassembly_assertion_helpers.rs index 376f8b34b..79cf58ddf 100644 --- a/tests/reassembly_assertion_helpers.rs +++ b/tests/reassembly_assertion_helpers.rs @@ -135,6 +135,36 @@ fn message_assertion_helpers_accept_completed_bodies() { key: MessageKey(8), } )] +#[case( + MessageAssemblyError::DeclaredTotalTooSmall { + key: MessageKey(13), + declared: 2, + body_len: 6, + }, + MessageAssemblyErrorExpectation::DeclaredTotalTooSmall { + key: MessageKey(13), + } +)] +#[case( + MessageAssemblyError::DeclaredTotalExceeded { + key: MessageKey(14), + declared: 10, + attempted: 12, + }, + MessageAssemblyErrorExpectation::DeclaredTotalExceeded { + key: MessageKey(14), + } +)] +#[case( + MessageAssemblyError::DeclaredTotalMismatch { + key: MessageKey(15), + declared: 10, + actual: 8, + }, + MessageAssemblyErrorExpectation::DeclaredTotalMismatch { + key: MessageKey(15), + } +)] fn message_assertion_helpers_match_errors( #[case] error: MessageAssemblyError, #[case] expected: MessageAssemblyErrorExpectation, From 26009183ce5d64425857621034b28b7cdc1a8436 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 16:18:09 +0200 Subject: [PATCH 03/12] Add behavioural scenarios for declared totals Exercise the declared-total contract through the public API: an under-declared and an over-declared single-frame total, a continuation that overshoots the declaration, a final frame that falls short, and an exactly declared multi-frame message that completes. The world fixture gains a `total_body_len` field on `FirstFrameParams` with a `with_total` builder, and the first frame now forwards it to `FirstFrameHeader` instead of always sending `None`. Co-Authored-By: Claude Code --- tests/features/message_assembly.feature | 33 +++++++++++ tests/fixtures/message_assembly.rs | 2 +- tests/fixtures/message_assembly_params.rs | 10 ++++ tests/scenarios/message_assembly_scenarios.rs | 40 ++++++++++++++ tests/steps/message_assembly_steps.rs | 55 +++++++++++++++++++ 5 files changed, 139 insertions(+), 1 deletion(-) diff --git a/tests/features/message_assembly.feature b/tests/features/message_assembly.feature index 5c91febe1..2d220314b 100644 --- a/tests/features/message_assembly.feature +++ b/tests/features/message_assembly.feature @@ -69,6 +69,39 @@ Feature: Message assembly multiplexing and continuity validation Then the error is message too large for key 1 And the buffered count is 0 + Scenario: Under-declared single-frame total is rejected + Given a final first frame for key 1 with body "hello" declaring total 3 + When the first frame is accepted + Then the error is declared total too small for key 1 + And the buffered count is 0 + + Scenario: Over-declared single-frame total is rejected + Given a final first frame for key 1 with body "hello" declaring total 9 + When the first frame is accepted + Then the error is declared total mismatch for key 1 + And the buffered count is 0 + + Scenario: Continuation exceeding the declared total is rejected + Given a first frame for key 1 with body "hello" declaring total 8 + When the first frame is accepted + And a continuation for key 1 with body "more" arrives + Then the error is declared total exceeded for key 1 + And the buffered count is 0 + + Scenario: Final frame short of the declared total is rejected + Given a first frame for key 1 with body "hello" declaring total 10 + When the first frame is accepted + And a final continuation for key 1 with sequence 1 and body "more" arrives + Then the error is declared total mismatch for key 1 + And the buffered count is 0 + + Scenario: Exactly declared multi-frame message completes + Given a first frame for key 1 with body "hello" declaring total 10 + When the first frame is accepted + And a final continuation for key 1 with sequence 1 and body "world" arrives + Then the assembly completes with body "helloworld" + And the buffered count is 0 + Scenario: Expired assemblies are purged Given a first frame for key 1 with body "partial" When the first frame is accepted at time T diff --git a/tests/fixtures/message_assembly.rs b/tests/fixtures/message_assembly.rs index ffeae4609..4e0c42564 100644 --- a/tests/fixtures/message_assembly.rs +++ b/tests/fixtures/message_assembly.rs @@ -140,7 +140,7 @@ impl MessageAssemblyWorld { message_key: params.key, metadata_len: params.metadata.len(), body_len: params.body.len(), - total_body_len: None, + total_body_len: params.total_body_len, is_last: params.is_last, }, metadata: params.metadata, diff --git a/tests/fixtures/message_assembly_params.rs b/tests/fixtures/message_assembly_params.rs index 246b8d783..257669016 100644 --- a/tests/fixtures/message_assembly_params.rs +++ b/tests/fixtures/message_assembly_params.rs @@ -13,6 +13,8 @@ pub struct FirstFrameParams { pub body: Vec, /// Whether this is the final frame. pub is_last: bool, + /// Total body length declared for the logical message, if any. + pub total_body_len: Option, } impl FirstFrameParams { @@ -24,6 +26,7 @@ impl FirstFrameParams { metadata: vec![], body, is_last: false, + total_body_len: None, } } @@ -34,6 +37,13 @@ impl FirstFrameParams { self } + /// Declare the total body length of the logical message. + #[must_use] + pub fn with_total(mut self, total_body_len: usize) -> Self { + self.total_body_len = Some(total_body_len); + self + } + /// Mark as the final frame. #[must_use] pub fn final_frame(mut self) -> Self { diff --git a/tests/scenarios/message_assembly_scenarios.rs b/tests/scenarios/message_assembly_scenarios.rs index c475f9177..9e2313b01 100644 --- a/tests/scenarios/message_assembly_scenarios.rs +++ b/tests/scenarios/message_assembly_scenarios.rs @@ -68,6 +68,46 @@ fn message_assembly_too_large(message_assembly_world: MessageAssemblyWorld) { drop(message_assembly_world); } +#[scenario( + path = "tests/features/message_assembly.feature", + name = "Under-declared single-frame total is rejected" +)] +fn message_assembly_under_declared_single_frame(message_assembly_world: MessageAssemblyWorld) { + drop(message_assembly_world); +} + +#[scenario( + path = "tests/features/message_assembly.feature", + name = "Over-declared single-frame total is rejected" +)] +fn message_assembly_over_declared_single_frame(message_assembly_world: MessageAssemblyWorld) { + drop(message_assembly_world); +} + +#[scenario( + path = "tests/features/message_assembly.feature", + name = "Continuation exceeding the declared total is rejected" +)] +fn message_assembly_continuation_over_declared(message_assembly_world: MessageAssemblyWorld) { + drop(message_assembly_world); +} + +#[scenario( + path = "tests/features/message_assembly.feature", + name = "Final frame short of the declared total is rejected" +)] +fn message_assembly_final_frame_short(message_assembly_world: MessageAssemblyWorld) { + drop(message_assembly_world); +} + +#[scenario( + path = "tests/features/message_assembly.feature", + name = "Exactly declared multi-frame message completes" +)] +fn message_assembly_exact_declaration(message_assembly_world: MessageAssemblyWorld) { + drop(message_assembly_world); +} + #[scenario( path = "tests/features/message_assembly.feature", name = "Expired assemblies are purged" diff --git a/tests/steps/message_assembly_steps.rs b/tests/steps/message_assembly_steps.rs index 2ab5028b2..adc0a35fe 100644 --- a/tests/steps/message_assembly_steps.rs +++ b/tests/steps/message_assembly_steps.rs @@ -99,6 +99,34 @@ fn given_first_frame( message_assembly_world.add_first_frame(FirstFrameParams::new(key.to_key(), body.into_bytes())); } +#[rustfmt::skip] +#[given("a first frame for key {key:MessageKeyParam} with body {body:string} declaring total {total:CountParam}")] +fn given_first_frame_with_total( + message_assembly_world: &mut MessageAssemblyWorld, + key: MessageKeyParam, + body: String, + total: CountParam, +) { + message_assembly_world.add_first_frame( + FirstFrameParams::new(key.to_key(), body.into_bytes()).with_total(total.0), + ); +} + +#[rustfmt::skip] +#[given("a final first frame for key {key:MessageKeyParam} with body {body:string} declaring total {total:CountParam}")] +fn given_final_first_frame_with_total( + message_assembly_world: &mut MessageAssemblyWorld, + key: MessageKeyParam, + body: String, + total: CountParam, +) { + message_assembly_world.add_first_frame( + FirstFrameParams::new(key.to_key(), body.into_bytes()) + .with_total(total.0) + .final_frame(), + ); +} + #[given("a final first frame for key {key:MessageKeyParam} with body {body:string}")] fn given_final_first_frame( message_assembly_world: &mut MessageAssemblyWorld, @@ -276,6 +304,33 @@ fn then_error_message_too_large( .assert_error(MessageAssemblyErrorExpectation::MessageTooLarge { key: key.to_key() }) } +#[then("the error is declared total too small for key {key:MessageKeyParam}")] +fn then_error_declared_total_too_small( + message_assembly_world: &mut MessageAssemblyWorld, + key: MessageKeyParam, +) -> TestResult { + message_assembly_world + .assert_error(MessageAssemblyErrorExpectation::DeclaredTotalTooSmall { key: key.to_key() }) +} + +#[then("the error is declared total exceeded for key {key:MessageKeyParam}")] +fn then_error_declared_total_exceeded( + message_assembly_world: &mut MessageAssemblyWorld, + key: MessageKeyParam, +) -> TestResult { + message_assembly_world + .assert_error(MessageAssemblyErrorExpectation::DeclaredTotalExceeded { key: key.to_key() }) +} + +#[then("the error is declared total mismatch for key {key:MessageKeyParam}")] +fn then_error_declared_total_mismatch( + message_assembly_world: &mut MessageAssemblyWorld, + key: MessageKeyParam, +) -> TestResult { + message_assembly_world + .assert_error(MessageAssemblyErrorExpectation::DeclaredTotalMismatch { key: key.to_key() }) +} + #[then("key {key:MessageKeyParam} was evicted")] fn then_key_evicted( message_assembly_world: &mut MessageAssemblyWorld, From f90d702f952796d186d8f6b7e4bb926989a8cdaa Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 16:18:12 +0200 Subject: [PATCH 04/12] Document the authoritative declared-total contract Add ADR 015, which records the decision to treat `FirstFrameHeader::total_body_len` as a protocol commitment rather than an advisory hint, compares that against renaming the field or checking only at completion, and lists the four enforcement points. Update the formal-verification guide to resolve its open question in favour of Position A, tick roadmap items 15.2.2 and 15.5.1, describe the new errors in the users' guide, and note the breaking change in the changelog. Co-Authored-By: Claude Code --- CHANGELOG.md | 11 ++ ...015-declared-total-body-length-contract.md | 146 ++++++++++++++++++ docs/contents.md | 2 + ...ormal-verification-methods-in-wireframe.md | 32 ++-- docs/roadmap.md | 11 +- docs/users-guide.md | 26 ++++ 6 files changed, 207 insertions(+), 21 deletions(-) create mode 100644 docs/adr-015-declared-total-body-length-contract.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 23ea4abcb..2fa4a36bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ All notable changes to this project will be documented in this file. built. See `docs/v0-3-0-to-v0-4-0-migration-guide.md` for migration examples. (#642) +- Breaking: `MessageAssemblyState` now enforces + `FirstFrameHeader::total_body_len` as an authoritative declaration. A first + frame whose body exceeds its declared total is rejected with + `DeclaredTotalTooSmall`; a continuation that would overshoot the declaration + is rejected with `DeclaredTotalExceeded`; and a completed body whose length + differs from the declaration is rejected with `DeclaredTotalMismatch`. + Protocol implementations must supply exact totals or leave `total_body_len` as + `None`. Downstream exhaustive matches on `MessageAssemblyError` need arms + for the three new variants. See + [ADR 015](docs/adr-015-declared-total-body-length-contract.md). + ([#544](https://github.com/leynos/wireframe/issues/544)) - Added scheduler and slot poison-recovery coverage, and extracted `scheduler_tests.rs` to keep `scheduler.rs` below 400 lines. (#539) - Added client-pool lifecycle and slot-rotation coverage that kills the diff --git a/docs/adr-015-declared-total-body-length-contract.md b/docs/adr-015-declared-total-body-length-contract.md new file mode 100644 index 000000000..b173f613d --- /dev/null +++ b/docs/adr-015-declared-total-body-length-contract.md @@ -0,0 +1,146 @@ +# Architectural decision record (ADR) 015: declared total body length is authoritative + +## Status + +Accepted. + +The contract is implemented by issue +[#544](https://github.com/leynos/wireframe/issues/544). + +## Date + +2026-10-18. + +## Context and problem statement + +`FirstFrameHeader::total_body_len` lets a protocol declare the size of a +logical message on its first frame. `MessageSeries` stores the declaration as +`expected_total`, and `MessageAssemblyState` uses it to reject messages that +cannot fit the configured size limit. + +The declaration was only a budget input. The completion path removed the +partial assembly and dispatched its body without comparing the accumulated +length to the declaration, and a single-frame message was returned before any +comparison at all. A peer could therefore declare any total that passed the +size check, send a different number of bytes, and have the body dispatched +unchanged. The name `total_body_len` promised a guarantee the runtime did not +make, which is a validation weakness (CWE-20) and an inconsistent length +interpretation (CWE-130). + +The formal verification guide frames the choice between an authoritative +declaration and an advisory hint in its section on +[whether `total_body_len` is authoritative or advisory](formal-verification-methods-in-wireframe.md#2-is-total_body_len-authoritative-or-advisory). + +## Decision drivers + +- Every caller in this repository that supplies a total already expects it to + be exact. +- The size limit is only meaningful if the declared value bounds the bytes that + are actually buffered and dispatched. +- The domain, not the inbound adapter, owns message-assembly invariants. +- An over- or under-declaration must be rejected before an unbounded amount of + peer-controlled data is retained. + +## Options considered + +### Option A: authoritative declaration + +Reject any message whose assembled body length differs from the declaration. +Enforce before buffering as well as at completion. + +### Option B: advisory hint + +Rename the field and tolerate mismatch. + +### Option C: enforce only at completion + +Keep buffering unrestricted and compare once the final frame arrives. + +| Topic | Option A | Option B | Option C | +| ------------------ | ----------------------- | ------------- | ---------------------- | +| Preserves the name | Yes | No (renamed) | Yes | +| Bounds buffering | Yes, before each append | No | No | +| Bypass of limit | Closed | Remains open | Closed after buffering | +| Caller impact | Exact totals required | Silent change | Exact totals required | + +_Table 1: Trade-offs between the considered options._ + +## Decision outcome + +Option A is adopted. `total_body_len` is a protocol commitment. Four +enforcement points apply, all in domain code: + +| Stage | Rejected condition | Error | +| ----------------------- | --------------------------------------------- | ----------------------- | +| First frame | `total_body_len < body_len` | `DeclaredTotalTooSmall` | +| First frame, final | `total_body_len != body_len` | `DeclaredTotalMismatch` | +| Continuation | accumulated body would exceed the declaration | `DeclaredTotalExceeded` | +| Completing continuation | accumulated body differs from the declaration | `DeclaredTotalMismatch` | + +_Table 2: Declared-total enforcement points._ + +- The first-frame check runs before the size-limit check. A declaration that + the first frame already contradicts is rejected as malformed rather than + measured against a limit that cannot make it valid. +- A continuation is checked before its bytes are buffered, so an oversized + frame is never retained, even briefly. +- A rejected continuation or completion removes the partial assembly. The + failure is reported through `MessageAssemblyError`, which the inbound adapter + maps to `io::ErrorKind::InvalidData` through its existing failure tracker. +- Protocols that leave `total_body_len` as `None` are unaffected; they retain + the previous behaviour with no equality requirement. + +The checks live in `src/message_assembler/validation.rs`, next to the state +machine that applies them. `PartialAssembly` owns the completion check so the +buffered length and the declaration are read together. + +## Consequences + +### Positive + +- The dispatched body length always equals the peer's declaration. +- An under-declared message can no longer bypass an application-configured size + limit. +- Failures are typed per enforcement point, so diagnostics distinguish an + unsatisfiable first frame from a completion mismatch. + +### Negative + +- A protocol that declared an approximate total will now see its messages + rejected. No protocol in this repository does so. +- `MessageAssemblyError` gains three variants. The enum is not + `#[non_exhaustive]`, so downstream exhaustive matches need updating. + +## Rejected shortcuts + +- Do not rename `total_body_len` to make the current behaviour sound: the + validation weakness would remain. +- Do not enforce only in the inbound adapter: another adapter would still see + the unsafe domain contract. +- Do not compare only at completion: a peer could force the runtime to buffer + an arbitrarily large body first. +- Do not return the mismatched body with a warning: dispatch is the security + boundary. + +## Migration plan + +1. Supply an exact `total_body_len`, or leave it as `None` to opt out of the + equality requirement. +2. Update exhaustive matches on `MessageAssemblyError` with the new variants. +3. Update behavioural expectations that asserted a mismatched body was + dispatched. + +## Verification + +- Unit tests cover under-declared single-frame totals, over-declared + single-frame totals, an over-declared continuation, a final frame short of + the declaration, exact equality, and the `None` opt-out. +- Behavioural tests in `tests/features/message_assembly.feature` exercise the + same paths through the public API. +- `make check-fmt`, `make lint`, and `make test` pass. + +## References + +- [Issue #544](https://github.com/leynos/wireframe/issues/544) +- [ADR 002: streaming requests and shared message assembly](adr-002-streaming-requests-and-shared-message-assembly.md) +- [Formal verification methods in Wireframe](formal-verification-methods-in-wireframe.md) diff --git a/docs/contents.md b/docs/contents.md index 7fd811bb4..281eac653 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -65,6 +65,8 @@ reference when navigating the project's design and architecture material. client pool. - [ADR 014: server shutdown ownership and terminal observation](adr-014-server-shutdown-ownership.md) Proposed awaitable server shutdown and supervisor-failure observation. +- [ADR 015: declared total body length is authoritative](adr-015-declared-total-body-length-contract.md) + Accepted contract for enforcing `total_body_len` during message assembly. ## Execution plans diff --git a/docs/formal-verification-methods-in-wireframe.md b/docs/formal-verification-methods-in-wireframe.md index 39456a604..a48daf867 100644 --- a/docs/formal-verification-methods-in-wireframe.md +++ b/docs/formal-verification-methods-in-wireframe.md @@ -131,19 +131,15 @@ This is the best candidate for the **strongest** verification investment. `expected_total`, and `MessageAssemblyState` uses the declared total for the initial size-limit check.[^22][^23][^24] -However, on completion, the current `accept_continuation_frame_at` path removes -the partial assembly and returns the assembled message without an explicit -final equality check between the accumulated body length and -`expected_total`.[^24] +The completion path now enforces an explicit final equality check between the +accumulated body length and `expected_total`, and the first-frame and +continuation paths reject declarations that cannot be honoured.[^24] The +contract is recorded as authoritative in +[ADR 015](adr-015-declared-total-body-length-contract.md), following Position A +below. -That is either: - -- a deliberate design choice, in which case the name `total_body_len` - overstates the guarantee, or -- a missing invariant, in which case it deserves both runtime enforcement and - verification. - -This is where Verus becomes plausible, but only **after** a contract decision. +This is where Verus becomes plausible, now that the contract decision is +settled. ### Fourth priority: `src/connection/*` @@ -200,18 +196,20 @@ verify, and closer to what the implementation already does. ### 2. Is `total_body_len` authoritative or advisory? -The current name strongly implies authority, but the completion path does not -enforce it.[^22][^24] +**Resolved:** authoritative, recorded in +[ADR 015](adr-015-declared-total-body-length-contract.md). Completion fails if +the actual body length differs, and the first-frame and continuation paths +reject declarations that cannot be honoured.[^22][^24] -Choose one of these positions: +The alternatives were: - **Position A:** It is authoritative. Completion must fail if the actual body length differs. - **Position B:** It is advisory. Rename it accordingly and document that mismatch is tolerated. -**Position A** is the stronger choice unless an existing protocol reason -requires treating it as a hint. +**Position A** was chosen because no existing protocol requires treating the +declaration as a hint. ### 3. What fairness guarantee does `ConnectionActor` actually make? diff --git a/docs/roadmap.md b/docs/roadmap.md index 4d1f11c39..ac164c22e 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -655,13 +655,15 @@ cited section names the specific question it answers. 15.1.1. See §"What widths does Wireframe actually support for length prefixes?". Success criteria: an ADR records the decision, constructors enforce the chosen set, and existing tests cover rejected widths. -- [ ] 15.2.2. Treat `total_body_len` as either authoritative or advisory and +- [x] 15.2.2. Treat `total_body_len` as either authoritative or advisory and enforce or rename it consistently across the message assembly path; record the decision in an ADR and add tests for both conforming and violating inputs. Requires 15.1.1. See §"Is `total_body_len` authoritative or advisory?". Success criteria: an ADR records the decision, runtime code enforces the chosen semantics, and tests verify both conforming and violating - inputs. + inputs. See [ADR 015](adr-015-declared-total-body-length-contract.md); issue + [#544](https://github.com/leynos/wireframe/issues/544) added conformance and + violation coverage. - [ ] 15.2.3. Publish named fairness and priority guarantees for `ConnectionActor` and encode them as model properties for Stateright checks. Requires 15.1.1. See §"What fairness guarantee does `ConnectionActor` @@ -717,13 +719,14 @@ cited section names the specific question it answers. ### 15.5. Verus proofs for message assembly -- [ ] 15.5.1. Enforce the chosen `total_body_len` contract in runtime code +- [x] 15.5.1. Enforce the chosen `total_body_len` contract in runtime code before relying on proofs. See [formal verification guide §"Is `total_body_len` authoritative or advisory?"](formal-verification-methods-in-wireframe.md#2-is-total_body_len-authoritative-or-advisory) and [§"What Verus should prove in Wireframe"](formal-verification-methods-in-wireframe.md#what-verus-should-prove-in-wireframe). Requires 15.2.2. Success criteria: runtime assertions or checks enforce the - contract, and existing tests confirm the enforcement. + contract, and existing tests confirm the enforcement. See + [ADR 015](adr-015-declared-total-body-length-contract.md). - [ ] 15.5.2. Add proof-only modules under `verus/` for declared-total and buffered-byte accounting invariants. See [formal verification guide §Proof style recommendation](formal-verification-methods-in-wireframe.md#proof-style-recommendation) diff --git a/docs/users-guide.md b/docs/users-guide.md index 5006bc74f..d94c09eba 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1282,6 +1282,32 @@ assert!(matches!( )); ``` +#### Declared total body length + +`FirstFrameHeader::total_body_len` is an authoritative declaration, not a hint. +When a protocol supplies it, the assembler enforces it at every boundary: + +- a first frame that carries more body bytes than it declares is rejected with + `MessageAssemblyError::DeclaredTotalTooSmall`, because its declaration can + never be satisfied; +- a single-frame message whose body differs from its declaration is rejected + with `MessageAssemblyError::DeclaredTotalMismatch`; +- a continuation that would push the accumulated body past the declaration is + rejected with `MessageAssemblyError::DeclaredTotalExceeded` before its bytes + are buffered; and +- the frame that completes the message must bring the accumulated body to + exactly the declared total, or `MessageAssemblyError::DeclaredTotalMismatch` + is returned. + +A rejected frame also removes the partial assembly, so a mismatched message +never remains buffered and is never dispatched. Protocols that leave +`total_body_len` as `None` opt out of the equality requirement and keep the +previous behaviour. + +The declaration is also used for the size-limit check, so an exact total is the +cheapest way to fail early on an oversized message. The decision is recorded in +[ADR 015](adr-015-declared-total-body-length-contract.md). + ### Streaming request body consumption Handlers can opt into streaming request bodies using the `StreamingBody` From 6d5cf28e690ac10e2f48b2d49fa4978b01d44af1 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 16:49:38 +0200 Subject: [PATCH 05/12] Group continuation admission limits into a struct Clippy rejected `PartialAssembly::new` (five arguments) and the `admit_continuation` helper (six) under `too_many_arguments`. Rather than suppress the lint, reduce the parameter lists: - `PartialAssembly::from_first_frame` takes the already-validated `FirstFrameInput`, which carries the header, routing, metadata, and body it previously received separately. - The per-message size cap and aggregate budgets move into `ContinuationLimits`, so a caller cannot pair the cap with the wrong budgets. - The admission checks move onto `PartialAssembly`, next to the accumulated body and declared total they read. Co-Authored-By: Claude Code --- src/message_assembler/budget.rs | 13 +++++++++ src/message_assembler/partial.rs | 50 +++++++++++++++++++++++++------- src/message_assembler/state.rs | 49 +++++-------------------------- 3 files changed, 60 insertions(+), 52 deletions(-) diff --git a/src/message_assembler/budget.rs b/src/message_assembler/budget.rs index 876845869..28a7b6a5a 100644 --- a/src/message_assembler/budget.rs +++ b/src/message_assembler/budget.rs @@ -32,6 +32,19 @@ impl AggregateBudgets { } } +/// Every limit that governs whether a continuation body may be buffered. +/// +/// Bundled so admission receives one value rather than a growing list of +/// parameters, and so a caller cannot pair the per-message cap with the wrong +/// aggregate budgets. +#[derive(Clone, Copy, Debug)] +pub(super) struct ContinuationLimits { + /// Per-message body cap. + pub(super) max_message_size: NonZeroUsize, + /// Aggregate budgets across in-flight assemblies. + pub(super) budgets: AggregateBudgets, +} + /// Check whether accepting `additional_bytes` for `key` would exceed /// the connection budget or in-flight budget. /// diff --git a/src/message_assembler/partial.rs b/src/message_assembler/partial.rs index 41f1d3352..0879ce5f5 100644 --- a/src/message_assembler/partial.rs +++ b/src/message_assembler/partial.rs @@ -13,9 +13,10 @@ use std::time::Instant; use super::{ ContinuationFrameHeader, MessageKey, + budget::{ContinuationLimits, check_aggregate_budgets, check_size_limit}, error::{MessageAssemblyError, MessageSeriesError, MessageSeriesStatus}, series::MessageSeries, - types::{AssembledMessage, EnvelopeRouting}, + types::{AssembledMessage, EnvelopeRouting, FirstFrameInput}, validation, }; @@ -36,18 +37,16 @@ pub(super) struct PartialAssembly { impl PartialAssembly { /// Start a new assembly from an admitted first frame. - pub(super) fn new( - series: MessageSeries, - routing: EnvelopeRouting, - metadata: Vec, - body: &[u8], - started_at: Instant, - ) -> Self { + /// + /// The input's header has already been validated against its payloads and + /// its declared total, so this only stores the components. + pub(super) fn from_first_frame(input: FirstFrameInput<'_>, started_at: Instant) -> Self { + let series = MessageSeries::from_first_frame(input.header); Self { series, - routing, - metadata, - body: body.to_vec(), + routing: input.routing, + metadata: input.metadata, + body: input.body.to_vec(), started_at, } } @@ -66,6 +65,35 @@ impl PartialAssembly { self.series.accept_continuation(header) } + /// Apply size, budget, and declared-total admission checks to a + /// continuation body before it is buffered. + /// + /// # Errors + /// + /// Returns [`MessageAssemblyError::MessageTooLarge`] or an aggregate + /// budget error when the frame cannot be retained, and + /// [`MessageAssemblyError::DeclaredTotalExceeded`] when it would push the + /// accumulated body past the declared total. The caller frees the + /// assembly on any error. + pub(super) fn admit_continuation( + &self, + body_len: usize, + buffered_total: usize, + limits: ContinuationLimits, + ) -> Result<(), MessageAssemblyError> { + let key = self.series.message_key(); + let accumulated = self.body_len(); + check_size_limit(limits.max_message_size, key, accumulated, body_len)?; + check_aggregate_budgets(key, buffered_total, body_len, &limits.budgets)?; + + // Enforce the declared total before buffering so an oversized + // continuation cannot be retained even briefly. + if let Some(declared) = self.declared_total() { + validation::validate_continuation(key, declared, accumulated, body_len)?; + } + Ok(()) + } + /// Append an admitted continuation body. pub(super) fn push_body(&mut self, body: &[u8]) { self.body.extend_from_slice(body); } diff --git a/src/message_assembler/state.rs b/src/message_assembler/state.rs index b38adb57e..7e3884f77 100644 --- a/src/message_assembler/state.rs +++ b/src/message_assembler/state.rs @@ -13,10 +13,9 @@ use std::{ use super::{ ContinuationFrameHeader, MessageKey, - budget::{AggregateBudgets, check_aggregate_budgets, check_size_limit}, + budget::{AggregateBudgets, ContinuationLimits, check_aggregate_budgets}, error::{MessageAssemblyError, MessageSeriesError, MessageSeriesStatus}, partial::PartialAssembly, - series::MessageSeries, types::{AssembledMessage, FirstFrameInput}, validation, }; @@ -183,8 +182,6 @@ impl MessageAssemblyState { }); } - let series = MessageSeries::from_first_frame(input.header); - // If this is a single-frame message, return immediately if input.header.is_last { return Ok(Some(AssembledMessage::new( @@ -210,7 +207,7 @@ impl MessageAssemblyState { // Start new assembly, preserving envelope routing metadata from the // first frame so the completed message is dispatched correctly. - let partial = PartialAssembly::new(series, input.routing, input.metadata, input.body, now); + let partial = PartialAssembly::from_first_frame(input, now); self.assemblies.insert(key, partial); Ok(None) @@ -280,9 +277,11 @@ impl MessageAssemblyState { // Snapshot budget state before the mutable entry borrow. The O(n) // scan is gated on is_enabled() so it's skipped when budgets are absent. - let max_message_size = self.max_message_size; - let budgets = self.budgets; - let buffered_total = if budgets.is_enabled() { + let limits = ContinuationLimits { + max_message_size: self.max_message_size, + budgets: self.budgets, + }; + let buffered_total = if limits.budgets.is_enabled() { self.total_buffered_bytes() } else { 0 @@ -306,14 +305,7 @@ impl MessageAssemblyState { }; let partial = entry.get_mut(); - if let Err(e) = Self::admit_continuation( - key, - partial, - body.len(), - buffered_total, - max_message_size, - &budgets, - ) { + if let Err(e) = partial.admit_continuation(body.len(), buffered_total, limits) { entry.remove(); return Err(e); } @@ -325,31 +317,6 @@ impl MessageAssemblyState { entry.remove().into_message() } - /// Apply size, budget, and declared-total admission checks to a - /// continuation body before it is buffered. - /// - /// Any rejection frees the assembly, so the caller removes the entry on - /// error. - fn admit_continuation( - key: MessageKey, - partial: &PartialAssembly, - body_len: usize, - buffered_total: usize, - max_message_size: NonZeroUsize, - budgets: &AggregateBudgets, - ) -> Result<(), MessageAssemblyError> { - let accumulated = partial.body_len(); - check_size_limit(max_message_size, key, accumulated, body_len)?; - check_aggregate_budgets(key, buffered_total, body_len, budgets)?; - - // Enforce the declared total before buffering so an oversized - // continuation cannot be retained even briefly. - if let Some(declared) = partial.declared_total() { - validation::validate_continuation(key, declared, accumulated, body_len)?; - } - Ok(()) - } - /// Remove any partial assemblies that exceeded the configured timeout. /// /// Returns the keys of evicted assemblies. From d23d065532614e68aa56bd7af712aefe8c386ef7 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 16:51:20 +0200 Subject: [PATCH 06/12] Clarify ADR 015 acceptance summary The style guide asks an accepted ADR's status section to summarise what was decided, not only point at the implementing issue. State the contract in the status block so a reader can tell what was accepted without reading the whole record. Co-Authored-By: Claude Code --- docs/adr-015-declared-total-body-length-contract.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/adr-015-declared-total-body-length-contract.md b/docs/adr-015-declared-total-body-length-contract.md index b173f613d..bfdb9d372 100644 --- a/docs/adr-015-declared-total-body-length-contract.md +++ b/docs/adr-015-declared-total-body-length-contract.md @@ -4,8 +4,10 @@ Accepted. -The contract is implemented by issue -[#544](https://github.com/leynos/wireframe/issues/544). +`FirstFrameHeader::total_body_len` is authoritative: the assembler rejects a +declaration it can already contradict, stops a continuation from overshooting +it, and requires the completed body to match it exactly. The contract is +implemented by issue [#544](https://github.com/leynos/wireframe/issues/544). ## Date From 4883828a7592c313904b9b6d70c76fb27d1cfcbf Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 17:14:50 +0200 Subject: [PATCH 07/12] Group continuation test frames into a struct The declared-total tests' continuation helper took five arguments, which trips `clippy::too_many_arguments` under the repository's four-argument threshold. Group the frame parameters into a named `Continuation` struct, matching the guidance to bundle related parameters, and update every call site. Derive `Copy` on the struct so the helper can keep taking it by value without a `needless_pass_by_value` lint. Co-Authored-By: Claude Code --- src/message_assembler/declared_total_tests.rs | 109 ++++++++++++++---- 1 file changed, 84 insertions(+), 25 deletions(-) diff --git a/src/message_assembler/declared_total_tests.rs b/src/message_assembler/declared_total_tests.rs index a7da80ae2..873dd9fdc 100644 --- a/src/message_assembler/declared_total_tests.rs +++ b/src/message_assembler/declared_total_tests.rs @@ -49,21 +49,27 @@ fn accept_first( state.accept_first_frame(input) } -/// Feed a sequenced continuation frame and return the state-machine result. -fn accept_continuation( - state: &mut MessageAssemblyState, +/// A sequenced continuation frame to feed to the state machine. +#[derive(Clone, Copy, Debug)] +struct Continuation<'a> { key: u64, sequence: u32, - body: &[u8], + body: &'a [u8], is_last: bool, +} + +/// Feed a continuation frame and return the state-machine result. +fn accept_continuation( + state: &mut MessageAssemblyState, + frame: Continuation<'_>, ) -> Result, MessageAssemblyError> { let header = ContinuationFrameHeader { - message_key: MessageKey(key), - sequence: Some(FrameSequence(sequence)), - body_len: body.len(), - is_last, + message_key: MessageKey(frame.key), + sequence: Some(FrameSequence(frame.sequence)), + body_len: frame.body.len(), + is_last: frame.is_last, }; - state.accept_continuation_frame(&header, body) + state.accept_continuation_frame(&header, frame.body) } // ============================================================================= @@ -152,8 +158,13 @@ fn continuation_exceeding_declared_total_is_rejected() { assert_eq!(subject.buffered_count(), 1); // 5 + 4 = 9 exceeds the declared 8. - let err = - accept_continuation(&mut subject, 1, 1, b"more", false).expect_err("declaration exceeded"); + let frame = Continuation { + key: 1, + sequence: 1, + body: b"more", + is_last: false, + }; + let err = accept_continuation(&mut subject, frame).expect_err("declaration exceeded"); assert_eq!( err, @@ -177,8 +188,16 @@ fn final_frame_short_of_declared_total_is_rejected() { accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); // 5 + 4 = 9, within the declaration, but the series completes here. - let err = - accept_continuation(&mut subject, 1, 1, b"more", true).expect_err("final frame mismatch"); + let err = accept_continuation( + &mut subject, + Continuation { + key: 1, + sequence: 1, + body: b"more", + is_last: true, + }, + ) + .expect_err("final frame mismatch"); assert_eq!( err, @@ -201,9 +220,17 @@ fn multi_frame_exact_declaration_is_dispatched() { let header = first_with_total(1, 5, 10, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); - let msg = accept_continuation(&mut subject, 1, 1, b"world", true) - .expect("declaration honoured") - .expect("message completes"); + let msg = accept_continuation( + &mut subject, + Continuation { + key: 1, + sequence: 1, + body: b"world", + is_last: true, + }, + ) + .expect("declaration honoured") + .expect("message completes"); assert_eq!(msg.body(), b"helloworld"); assert_eq!(subject.buffered_count(), 0); @@ -216,14 +243,30 @@ fn non_final_frame_may_exactly_reach_the_declaration() { accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); // Exactly reaches the declaration without completing. - let msg = accept_continuation(&mut subject, 1, 1, b"world", false) - .expect("equals the declaration, so accepted"); + let msg = accept_continuation( + &mut subject, + Continuation { + key: 1, + sequence: 1, + body: b"world", + is_last: false, + }, + ) + .expect("equals the declaration, so accepted"); assert!(msg.is_none()); assert_eq!(subject.buffered_count(), 1); // No bytes remain, so the completing frame must be rejected. - let err = accept_continuation(&mut subject, 1, 2, b"!", true) - .expect_err("no room remains within the declaration"); + let err = accept_continuation( + &mut subject, + Continuation { + key: 1, + sequence: 2, + body: b"!", + is_last: true, + }, + ) + .expect_err("no room remains within the declaration"); assert_eq!( err, @@ -246,9 +289,17 @@ fn undeclared_total_is_unaffected() { let header = first_header(1, 5, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); - let msg = accept_continuation(&mut subject, 1, 1, b"worlds", true) - .expect("undeclared total imposes no equality check") - .expect("message completes"); + let msg = accept_continuation( + &mut subject, + Continuation { + key: 1, + sequence: 1, + body: b"worlds", + is_last: true, + }, + ) + .expect("undeclared total imposes no equality check") + .expect("message completes"); assert_eq!(msg.body(), b"helloworlds"); } @@ -260,8 +311,16 @@ fn declared_total_equal_to_first_frame_body_leaves_no_room() { let header = first_with_total(1, 5, 5, false); accept_first(&mut subject, &header, b"hello").expect("boundary declaration accepted"); - let err = accept_continuation(&mut subject, 1, 1, b"!", true) - .expect_err("no bytes remain within the declaration"); + let err = accept_continuation( + &mut subject, + Continuation { + key: 1, + sequence: 1, + body: b"!", + is_last: true, + }, + ) + .expect_err("no bytes remain within the declaration"); assert_eq!( err, From 120c0e5589c3466f2441809b6d01ad86dce18028 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 18 Oct 2026 19:00:01 +0200 Subject: [PATCH 08/12] Inject the subject fixture through rstest `subject` carried `#[fixture]` but every test called it directly, so the attribute had no effect. Take the state through `#[from(subject)]` instead, which builds a fresh manager per test through the fixture. Merge the three first-frame rejection tests into a single case table: they shared one body and one set of post-conditions, differing only in the declaration, the final-frame flag, and the expected error. The parameterised name records each distinct semantic case. Addresses the two trivial CodeRabbit findings on the declared-total tests. Co-Authored-By: Claude Code --- src/message_assembler/declared_total_tests.rs | 133 ++++++++---------- 1 file changed, 57 insertions(+), 76 deletions(-) diff --git a/src/message_assembler/declared_total_tests.rs b/src/message_assembler/declared_total_tests.rs index 873dd9fdc..193315a2d 100644 --- a/src/message_assembler/declared_total_tests.rs +++ b/src/message_assembler/declared_total_tests.rs @@ -7,7 +7,7 @@ use std::{num::NonZeroUsize, time::Duration}; -use rstest::fixture; +use rstest::{fixture, rstest}; use crate::message_assembler::{ AssembledMessage, @@ -76,47 +76,45 @@ fn accept_continuation( // Single-frame declarations // ============================================================================= -#[test] -fn single_frame_under_declared_total_is_rejected() { - let mut subject = subject(); - // Declares 3 bytes but carries 5: the declaration can never be satisfied. - let header = first_with_total(1, 5, 3, true); - - let err = accept_first(&mut subject, &header, b"hello").expect_err("under-declared total"); - - assert_eq!( - err, - MessageAssemblyError::DeclaredTotalTooSmall { - key: MessageKey(1), - declared: 3, - body_len: 5, - } - ); +/// Every way a first frame can contradict its own declaration. +/// +/// The three cases differ only in the declaration, the final-frame flag, and +/// the error the contradiction produces, so they share one body and one set of +/// post-conditions: rejection with the expected error and no buffered state. +#[rstest] +#[case::under_declared_final(3, true, MessageAssemblyError::DeclaredTotalTooSmall { + key: MessageKey(1), + declared: 3, + body_len: 5, +})] +#[case::over_declared_final(10, true, MessageAssemblyError::DeclaredTotalMismatch { + key: MessageKey(1), + declared: 10, + actual: 5, +})] +#[case::under_declared_non_final(4, false, MessageAssemblyError::DeclaredTotalTooSmall { + key: MessageKey(1), + declared: 4, + body_len: 5, +})] +fn first_frame_contradicting_declaration_is_rejected( + #[from(subject)] mut subject: MessageAssemblyState, + #[case] total: usize, + #[case] is_last: bool, + #[case] expected: MessageAssemblyError, +) { + let header = first_with_total(1, 5, total, is_last); + + let err = accept_first(&mut subject, &header, b"hello").expect_err("declaration rejected"); + + assert_eq!(err, expected); assert_eq!(subject.buffered_count(), 0); } -#[test] -fn single_frame_over_declared_total_is_rejected() { - let mut subject = subject(); - // Declares 10 bytes but is the final frame with only 5. - let header = first_with_total(1, 5, 10, true); - - let err = accept_first(&mut subject, &header, b"hello").expect_err("over-declared total"); - - assert_eq!( - err, - MessageAssemblyError::DeclaredTotalMismatch { - key: MessageKey(1), - declared: 10, - actual: 5, - } - ); - assert_eq!(subject.buffered_count(), 0); -} - -#[test] -fn single_frame_exact_declaration_is_dispatched() { - let mut subject = subject(); +#[rstest] +fn single_frame_exact_declaration_is_dispatched( + #[from(subject)] mut subject: MessageAssemblyState, +) { let header = first_with_total(1, 5, 5, true); let msg = accept_first(&mut subject, &header, b"hello") @@ -131,28 +129,10 @@ fn single_frame_exact_declaration_is_dispatched() { // Multi-frame declarations // ============================================================================= -#[test] -fn first_frame_under_declared_multi_frame_total_is_rejected() { - let mut subject = subject(); - // Even a non-final first frame may not carry more than its own total. - let header = first_with_total(1, 5, 4, false); - - let err = accept_first(&mut subject, &header, b"hello").expect_err("under-declared total"); - - assert_eq!( - err, - MessageAssemblyError::DeclaredTotalTooSmall { - key: MessageKey(1), - declared: 4, - body_len: 5, - } - ); - assert_eq!(subject.buffered_count(), 0); -} - -#[test] -fn continuation_exceeding_declared_total_is_rejected() { - let mut subject = subject(); +#[rstest] +fn continuation_exceeding_declared_total_is_rejected( + #[from(subject)] mut subject: MessageAssemblyState, +) { let header = first_with_total(1, 5, 8, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); assert_eq!(subject.buffered_count(), 1); @@ -181,9 +161,10 @@ fn continuation_exceeding_declared_total_is_rejected() { ); } -#[test] -fn final_frame_short_of_declared_total_is_rejected() { - let mut subject = subject(); +#[rstest] +fn final_frame_short_of_declared_total_is_rejected( + #[from(subject)] mut subject: MessageAssemblyState, +) { let header = first_with_total(1, 5, 10, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); @@ -214,9 +195,8 @@ fn final_frame_short_of_declared_total_is_rejected() { ); } -#[test] -fn multi_frame_exact_declaration_is_dispatched() { - let mut subject = subject(); +#[rstest] +fn multi_frame_exact_declaration_is_dispatched(#[from(subject)] mut subject: MessageAssemblyState) { let header = first_with_total(1, 5, 10, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); @@ -236,9 +216,10 @@ fn multi_frame_exact_declaration_is_dispatched() { assert_eq!(subject.buffered_count(), 0); } -#[test] -fn non_final_frame_may_exactly_reach_the_declaration() { - let mut subject = subject(); +#[rstest] +fn non_final_frame_may_exactly_reach_the_declaration( + #[from(subject)] mut subject: MessageAssemblyState, +) { let header = first_with_total(1, 5, 10, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); @@ -283,9 +264,8 @@ fn non_final_frame_may_exactly_reach_the_declaration() { // Protocols without a declaration // ============================================================================= -#[test] -fn undeclared_total_is_unaffected() { - let mut subject = subject(); +#[rstest] +fn undeclared_total_is_unaffected(#[from(subject)] mut subject: MessageAssemblyState) { let header = first_header(1, 5, false); accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); @@ -304,9 +284,10 @@ fn undeclared_total_is_unaffected() { assert_eq!(msg.body(), b"helloworlds"); } -#[test] -fn declared_total_equal_to_first_frame_body_leaves_no_room() { - let mut subject = subject(); +#[rstest] +fn declared_total_equal_to_first_frame_body_leaves_no_room( + #[from(subject)] mut subject: MessageAssemblyState, +) { // The boundary case: declared == body_len with more frames expected. let header = first_with_total(1, 5, 5, false); accept_first(&mut subject, &header, b"hello").expect("boundary declaration accepted"); From 541aaa74b7af92287c2b036cd1e5adfa773b5d8a Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 23 Oct 2026 13:47:58 +0200 Subject: [PATCH 09/12] Add declared-total enforcement flow diagram to ADR 015 The decision outcome lists the four enforcement points as a table and bullets, which describes each check in isolation but does not show how they order around buffering and dispatch. Add a flowchart tracing a first frame through its declaration check, continuations through admission, and the completed message through the final comparison, with each rejection feeding the shared "free partial assembly" terminal. The diagram is introduced by a screen-reader description and captioned as Figure 1, per the documentation style guide. Co-Authored-By: Claude Code --- ...015-declared-total-body-length-contract.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/docs/adr-015-declared-total-body-length-contract.md b/docs/adr-015-declared-total-body-length-contract.md index bfdb9d372..bea9dede3 100644 --- a/docs/adr-015-declared-total-body-length-contract.md +++ b/docs/adr-015-declared-total-body-length-contract.md @@ -92,6 +92,34 @@ _Table 2: Declared-total enforcement points._ - Protocols that leave `total_body_len` as `None` are unaffected; they retain the previous behaviour with no equality requirement. +For screen readers: the following flow chart traces a first frame through its +declaration check, a continuation through its admission check, and a completed +message through the final comparison, showing where each rejection frees the +partial assembly. + +```mermaid +flowchart TD + A[Receive first frame] --> B[validate_first_frame] + B -->|declared total smaller than body| E1[Reject: DeclaredTotalTooSmall] + B -->|single-frame total differs| E2[Reject: DeclaredTotalMismatch] + B -->|valid multi-frame declaration| C[Buffer first-frame body] + C --> D[Receive continuation] + D --> F[validate_continuation] + F -->|would exceed declaration or overflow| E3[Reject: DeclaredTotalExceeded] + F -->|within declaration| G[Buffer continuation before completion] + G --> H{Message complete?} + H -->|no| D + H -->|yes| I[validate_completion] + I -->|assembled length differs| E4[Reject: DeclaredTotalMismatch] + I -->|exact declared length| J[Dispatch assembled message] + E1 --> K[Free partial assembly] + E2 --> K + E3 --> K + E4 --> K +``` + +_Figure 1: Declared-total enforcement from the first frame to dispatch._ + The checks live in `src/message_assembler/validation.rs`, next to the state machine that applies them. `PartialAssembly` owns the completion check so the buffered length and the declaration are read together. From 8b1464ff9a1ad66675f9488dcaa45566d02a27b3 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 30 Oct 2026 22:16:54 +0100 Subject: [PATCH 10/12] Address declared-total review feedback CodeRabbit asked for the ADR 015 bullet in `docs/contents.md` to use a reference-style link so the bullet stays within the 80-column limit, and CodeScene reported code duplication in the new test code: - `declared_total_tests.rs` had two near-identical continuation tests; they are now one `rstest` table over the declared/undeclared cases. - `tests/steps/message_assembly_steps.rs` repeated the same `FirstFrameParams` build in four step functions. A `FrameOptions` builder records the attributes and one `record_first_frame` helper applies them, leaving each step to name only what it adds. This commit also adds coverage the review did not request but that the contract needs: property tests for the pure validators (including the `usize::MAX` boundary a uniform generator misses) and a model-based property over generated multi-frame partitions, plus a trybuild fixture pinning the compile-time break for downstream exhaustive matches. The user guide, developer guide, migration guide and ADR record them. Co-Authored-By: Claude Code --- ...015-declared-total-body-length-contract.md | 9 + docs/contents.md | 6 +- docs/developers-guide.md | 45 +++ docs/v0-3-0-to-v0-4-0-migration-guide.md | 36 +++ .../declared_total_property_tests.rs | 306 ++++++++++++++++++ src/message_assembler/declared_total_tests.rs | 49 ++- src/message_assembler/tests.rs | 7 + tests/compile_error.rs | 1 + tests/steps/message_assembly_steps.rs | 87 ++++- ...message_assembly_error_exhaustive_match.rs | 22 ++ ...age_assembly_error_exhaustive_match.stderr | 26 ++ 11 files changed, 549 insertions(+), 45 deletions(-) create mode 100644 src/message_assembler/declared_total_property_tests.rs create mode 100644 tests/ui/message_assembly_error_exhaustive_match.rs create mode 100644 tests/ui/message_assembly_error_exhaustive_match.stderr diff --git a/docs/adr-015-declared-total-body-length-contract.md b/docs/adr-015-declared-total-body-length-contract.md index bea9dede3..d977dc920 100644 --- a/docs/adr-015-declared-total-body-length-contract.md +++ b/docs/adr-015-declared-total-body-length-contract.md @@ -165,6 +165,15 @@ buffered length and the declaration are read together. - Unit tests cover under-declared single-frame totals, over-declared single-frame totals, an over-declared continuation, a final frame short of the declaration, exact equality, and the `None` opt-out. +- Property tests in `src/message_assembler/declared_total_property_tests.rs` + quantify over arbitrary `usize` lengths. They pin acceptance against the + remaining room, reject unrepresentable sums near `usize::MAX` as + `DeclaredTotalExceeded`, and drive the public state machine across generated + multi-frame partitions, asserting that an overshooting frame is never + buffered and that only an exact final total dispatches. +- `tests/ui/message_assembly_error_exhaustive_match.rs` pins the compile-time + break caused by the three new variants: a match naming only the pre-change + variants fails to compile, as it does for `ClientError`. - Behavioural tests in `tests/features/message_assembly.feature` exercise the same paths through the public API. - `make check-fmt`, `make lint`, and `make test` pass. diff --git a/docs/contents.md b/docs/contents.md index 281eac653..0a864db26 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -65,8 +65,10 @@ reference when navigating the project's design and architecture material. client pool. - [ADR 014: server shutdown ownership and terminal observation](adr-014-server-shutdown-ownership.md) Proposed awaitable server shutdown and supervisor-failure observation. -- [ADR 015: declared total body length is authoritative](adr-015-declared-total-body-length-contract.md) - Accepted contract for enforcing `total_body_len` during message assembly. +- [ADR 015: declared total body length is authoritative][adr-015] Accepted + contract for enforcing `total_body_len` during message assembly. + +[adr-015]: adr-015-declared-total-body-length-contract.md ## Execution plans diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 2cfcfa9ec..1c63f5e0f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -614,6 +614,51 @@ duplicate/gap/out-of-order validation; and `advance_sequence_or_overflow()` owns the overflow decision. Each piece is independently testable and the decision tree stays flat and readable during future refactors. +### Declared total body length architecture + +`FirstFrameHeader::total_body_len` is authoritative, following ADR 015. The +contract is enforced in domain code so every adapter inherits it, and the work +is split across four modules in `src/message_assembler/`: + +- `validation.rs` holds the pure checks. `validate_first_frame`, + `validate_continuation`, and `validate_completion` take lengths and return a + `MessageAssemblyError`; they hold no state and perform no I/O, so each + boundary is testable without an assembly. +- `partial.rs` owns an in-flight `PartialAssembly`. It keeps the completion + check next to the buffered bytes it compares, so the accumulated length and + the declaration are read together. +- `state.rs` owns admission and routing. It stops growing past its module size + budget by delegating the length arithmetic to `validation`. +- `budget.rs` holds `ContinuationLimits`, which bundles the per-message cap + with the aggregate `AggregateBudgets` so a caller cannot pair the cap with + the wrong budgets. + +The four enforcement points: + +| Boundary | Rejected condition | Variant | +| ----------------------- | --------------------------------------------- | ----------------------- | +| First frame | declaration smaller than the frame's body | `DeclaredTotalTooSmall` | +| First frame, final | body differs from the declaration | `DeclaredTotalMismatch` | +| Continuation | accumulated body would exceed the declaration | `DeclaredTotalExceeded` | +| Completing continuation | accumulated body differs from the declaration | `DeclaredTotalMismatch` | + +`validate_continuation` runs before the bytes are buffered, so a frame that +would overshoot is never retained, even briefly. A rejected continuation or +completion removes the partial assembly, so a failed message cannot linger +under its key. The inbound adapter in `src/app/frame_handling/assembly.rs` +translates the resulting `MessageAssemblyError` into +`io::ErrorKind::InvalidData` through `DeserFailureTracker`; the domain never +sees `io::Error`. + +Overflow is part of the contract, not an incidental guard. The accumulator uses +`usize::checked_add`, and an unrepresentable sum is reported as +`DeclaredTotalExceeded` with `attempted: usize::MAX` rather than wrapping to a +small value that would appear to fit. `declared_total_property_tests.rs` pins +that boundary over values near `usize::MAX`, and +`tests/ui/message_assembly_error_exhaustive_match.rs` pins the compile-time +break caused by the three new variants, matching the existing `ClientError` +fixture. + ### `fill_buf_with_prefix` The private helper `fill_buf_with_prefix(buf, prefix, endianness)` in diff --git a/docs/v0-3-0-to-v0-4-0-migration-guide.md b/docs/v0-3-0-to-v0-4-0-migration-guide.md index 68710c289..34f122a49 100644 --- a/docs/v0-3-0-to-v0-4-0-migration-guide.md +++ b/docs/v0-3-0-to-v0-4-0-migration-guide.md @@ -49,6 +49,42 @@ that inspect rejection rates can use the optional `wireframe_decode_rejections_total` counter, labelled by the bounded decode path and rejection reason. +## Declared total body length is authoritative + +`FirstFrameHeader::total_body_len` is now a protocol commitment. Message +assembly rejects a first frame that already contradicts its declaration, stops +a continuation from overshooting it, and requires the completed body to match +it exactly. See [ADR 015](adr-015-declared-total-body-length-contract.md) for +the decision. + +Applications that declare a total must supply the exact body length. A protocol +that declares an approximate size will now see its messages rejected, because +the length actually assembled can no longer differ from the length promised. +Leaving `total_body_len` as `None` opts out of the equality requirement and +keeps the previous behaviour, so that is the migration path for a protocol +whose totals were only ever advisory. + +`MessageAssemblyError` gains three variants: `DeclaredTotalTooSmall`, +`DeclaredTotalExceeded`, and `DeclaredTotalMismatch`. The enum is not +`#[non_exhaustive]`, so an exhaustive match written against the previous +variants fails to compile. Add the new arms: + +```rust +use wireframe::message_assembler::MessageAssemblyError; + +fn is_retryable(error: MessageAssemblyError) -> bool { + match error { + MessageAssemblyError::DeclaredTotalTooSmall { .. } + | MessageAssemblyError::DeclaredTotalExceeded { .. } + | MessageAssemblyError::DeclaredTotalMismatch { .. } => false, + _ => true, + } +} +``` + +The inbound adapter maps all three to `io::ErrorKind::InvalidData` and counts a +deserialization failure, so a rejected message never reaches a handler. + ## Prepared application transition `WireframeApp` remains the mutable builder. Register routes, middleware, diff --git a/src/message_assembler/declared_total_property_tests.rs b/src/message_assembler/declared_total_property_tests.rs new file mode 100644 index 000000000..631a7166a --- /dev/null +++ b/src/message_assembler/declared_total_property_tests.rs @@ -0,0 +1,306 @@ +//! Property tests for declared total body length enforcement (issue #544). +//! +//! The example-based cases in `declared_total_tests.rs` name the protocol +//! outcomes that matter. These properties cover the open-ended space between +//! them: every validator is a pure function of `usize` values, so the boundary +//! around `usize::MAX` — where a naive sum wraps — is reachable by values +//! rather than only by hand-picked examples. +//! +//! The model-based property drives the public state machine and compares every +//! outcome against an independent reimplementation of the contract, so a +//! regression in admission, buffering, or cleanup fails with the partition +//! that provoked it. + +use std::{num::NonZeroUsize, time::Duration}; + +use proptest::prelude::*; + +use crate::message_assembler::{ + AssembledMessage, + ContinuationFrameHeader, + EnvelopeRouting, + FirstFrameHeader, + FirstFrameInput, + FrameSequence, + MessageAssemblyError, + MessageAssemblyState, + MessageKey, + validation, +}; + +/// Values clustered at the top of the `usize` range, where addition overflows. +/// +/// A uniform generator almost never lands within a few bytes of `usize::MAX`, +/// which is exactly where `accumulated + body_len` stops being representable. +fn near_max() -> impl Strategy { + (0usize..=3).prop_map(|offset| usize::MAX - offset) +} + +/// A first-frame header for key `1` declaring `total`. +fn header_with_total(total: usize, body_len: usize) -> FirstFrameHeader { + FirstFrameHeader { + message_key: MessageKey(1), + metadata_len: 0, + body_len, + total_body_len: Some(total), + is_last: false, + } +} + +/// State with a limit large enough that the declaration is decisive. +fn subject() -> MessageAssemblyState { + MessageAssemblyState::new( + NonZeroUsize::new(1_024).unwrap_or(NonZeroUsize::MIN), + Duration::from_secs(30), + ) +} + +/// The outcome the contract requires for one continuation. +#[derive(Debug)] +enum Expected { + /// The frame fits and does not complete the message. + Buffered, + /// The frame fits, completes the message, and dispatches the body. + Dispatched, + /// The frame would overrun the declaration. + Exceeded, + /// The frame completes the message at the wrong length. + Mismatch, +} + +/// Reimplement the contract independently of the production helpers. +fn expected_outcome( + declared: usize, + accumulated: usize, + body_len: usize, + is_last: bool, +) -> Expected { + let Some(attempted) = accumulated.checked_add(body_len) else { + return Expected::Exceeded; + }; + if attempted > declared { + return Expected::Exceeded; + } + if is_last { + return if attempted == declared { + Expected::Dispatched + } else { + Expected::Mismatch + }; + } + Expected::Buffered +} + +proptest! { + /// A first frame is accepted exactly when its declaration is satisfiable. + /// + /// Satisfiable means the declaration covers the frame's own body, and — + /// when the frame also completes the message — equals that body exactly. + /// The error variant distinguishes the two ways of failing. + #[test] + fn first_frame_accepts_only_satisfiable_declarations( + body_len in 0usize..=1_000, + total in 0usize..=1_000, + is_last in any::(), + ) { + let header = FirstFrameHeader { + is_last, + ..header_with_total(total, body_len) + }; + + let outcome = validation::validate_first_frame(&header, body_len); + + let is_satisfiable = total >= body_len && (!is_last || total == body_len); + prop_assert_eq!(outcome.is_ok(), is_satisfiable); + + match outcome { + Ok(()) => {} + Err(MessageAssemblyError::DeclaredTotalTooSmall { + declared, + body_len: carried, + .. + }) => { + prop_assert!(total < body_len); + prop_assert_eq!(declared, total); + prop_assert_eq!(carried, body_len); + } + Err(MessageAssemblyError::DeclaredTotalMismatch { declared, actual, .. }) => { + prop_assert!(is_last); + prop_assert!(total >= body_len); + prop_assert_eq!(declared, total); + prop_assert_eq!(actual, body_len); + } + Err(other) => prop_assert!(false, "unexpected first-frame error: {other:?}"), + } + } + + /// A continuation is admitted exactly when its bytes fit the remaining room. + /// + /// On rejection the attempted length is the true sum, except that an + /// unrepresentable sum is reported as `usize::MAX` rather than wrapping to + /// a small value that would appear to fit. + #[test] + fn continuation_admission_matches_available_room( + declared in 0usize..=1_000, + accumulated in 0usize..=1_000, + body_len in 0usize..=1_000, + ) { + let key = MessageKey(1); + let outcome = validation::validate_continuation(key, declared, accumulated, body_len); + + let fits = match accumulated.checked_add(body_len) { + Some(attempted) => attempted <= declared, + None => false, + }; + prop_assert_eq!(outcome.is_ok(), fits); + + if let Err(MessageAssemblyError::DeclaredTotalExceeded { attempted, .. }) = outcome { + prop_assert_eq!(attempted, accumulated + body_len); + prop_assert!(attempted > declared); + } + } + + /// Addition that `usize` cannot represent is rejected, never wrapped. + /// + /// This is the region a uniform generator misses and the only place where + /// a wrapping implementation would silently accept an overrun. + #[test] + fn unrepresentable_sums_are_rejected_as_exceeded( + declared in near_max(), + accumulated in near_max(), + body_len in near_max(), + ) { + let outcome = + validation::validate_continuation(MessageKey(1), declared, accumulated, body_len); + + match accumulated.checked_add(body_len) { + Some(attempted) => prop_assert_eq!(outcome.is_ok(), attempted <= declared), + None => { + prop_assert_eq!( + outcome, + Err(MessageAssemblyError::DeclaredTotalExceeded { + key: MessageKey(1), + declared, + attempted: usize::MAX, + }) + ); + } + } + } + + /// Completion accepts exactly the declared length. + /// + /// Both directions matter: a short body is the under-declared message the + /// contract exists to stop, and a long body is an overrun that reached + /// completion despite continuation admission. + #[test] + fn completion_accepts_only_an_exact_match( + declared in any::(), + actual in any::(), + ) { + prop_assert_eq!( + validation::validate_completion(MessageKey(1), declared, actual).is_ok(), + declared == actual, + ); + } +} + +/// Feed one continuation and report what the state machine did. +fn feed_continuation( + state: &mut MessageAssemblyState, + sequence: u32, + body: &[u8], + is_last: bool, +) -> Result, MessageAssemblyError> { + let header = ContinuationFrameHeader { + message_key: MessageKey(1), + sequence: Some(FrameSequence(sequence)), + body_len: body.len(), + is_last, + }; + state.accept_continuation_frame(&header, body) +} + +proptest! { + #![proptest_config(ProptestConfig { + cases: 256, + ..ProptestConfig::default() + })] + + /// The state machine matches an independent model of the contract for an + /// arbitrary partition of a message across a first frame and up to three + /// continuations, including partitions that fall short or overshoot. + #[test] + fn partitioned_frames_match_the_declared_total_model( + declared in 0usize..=64, + first_body in 0usize..=64, + continuations in proptest::collection::vec(0usize..=64, 0..3), + ) { + prop_assume!(first_body <= declared); + + let first_bytes = vec![b'a'; first_body]; + let mut state = subject(); + + let header = header_with_total(declared, first_body); + let input = FirstFrameInput::new( + &header, + EnvelopeRouting::default(), + vec![], + &first_bytes, + ) + .expect("header lengths match the body"); + state.accept_first_frame(input).expect("satisfiable first frame"); + + let mut assembled = first_bytes; + let mut terminated = false; + for (index, body_len) in continuations.iter().copied().enumerate() { + let is_last = index + 1 == continuations.len(); + let body = vec![b'b'; body_len]; + let expected = expected_outcome(declared, assembled.len(), body_len, is_last); + + let result = feed_continuation( + &mut state, + u32::try_from(index).expect("fewer than u32::MAX frames"), + &body, + is_last, + ); + + match expected { + Expected::Buffered => { + prop_assert_eq!(result, Ok(None)); + assembled.extend_from_slice(&body); + continue; + } + Expected::Dispatched => { + assembled.extend_from_slice(&body); + let message = result + .expect("exact completion is accepted") + .expect("a completing frame dispatches"); + prop_assert_eq!(message.body(), assembled.as_slice()); + } + Expected::Exceeded => { + let was_exceeded = + matches!(result, Err(MessageAssemblyError::DeclaredTotalExceeded { .. })); + prop_assert!(was_exceeded, "expected DeclaredTotalExceeded, got {result:?}"); + } + Expected::Mismatch => { + let was_mismatch = + matches!(result, Err(MessageAssemblyError::DeclaredTotalMismatch { .. })); + prop_assert!(was_mismatch, "expected DeclaredTotalMismatch, got {result:?}"); + } + } + + terminated = true; + break; + } + + // A rejected continuation or completion frees the assembly; otherwise + // the message is still in flight under its key. + let expected_buffered = usize::from(!terminated); + prop_assert_eq!( + state.buffered_count(), + expected_buffered, + "the partial assembly must be freed exactly when a frame terminated it" + ); + } +} diff --git a/src/message_assembler/declared_total_tests.rs b/src/message_assembler/declared_total_tests.rs index 193315a2d..4006b1a6d 100644 --- a/src/message_assembler/declared_total_tests.rs +++ b/src/message_assembler/declared_total_tests.rs @@ -195,9 +195,24 @@ fn final_frame_short_of_declared_total_is_rejected( ); } +/// A completing frame dispatches the assembled body whether the protocol +/// declared an exact total or declared nothing at all. +/// +/// The two cases share one body: the only difference is whether the +/// declaration exists for the completion check to enforce. #[rstest] -fn multi_frame_exact_declaration_is_dispatched(#[from(subject)] mut subject: MessageAssemblyState) { - let header = first_with_total(1, 5, 10, false); +#[case::exact_declaration(Some(10), "world", "helloworld")] +#[case::undeclared_total(None, "worlds", "helloworlds")] +fn completing_frame_dispatches_the_assembled_body( + #[from(subject)] mut subject: MessageAssemblyState, + #[case] declared: Option, + #[case] final_body: &str, + #[case] expected: &str, +) { + let header = FirstFrameHeader { + total_body_len: declared, + ..first_header(1, 5, false) + }; accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); let msg = accept_continuation( @@ -205,14 +220,14 @@ fn multi_frame_exact_declaration_is_dispatched(#[from(subject)] mut subject: Mes Continuation { key: 1, sequence: 1, - body: b"world", + body: final_body.as_bytes(), is_last: true, }, ) - .expect("declaration honoured") + .expect("body honours the declaration") .expect("message completes"); - assert_eq!(msg.body(), b"helloworld"); + assert_eq!(msg.body(), expected.as_bytes()); assert_eq!(subject.buffered_count(), 0); } @@ -260,30 +275,6 @@ fn non_final_frame_may_exactly_reach_the_declaration( assert_eq!(subject.buffered_count(), 0); } -// ============================================================================= -// Protocols without a declaration -// ============================================================================= - -#[rstest] -fn undeclared_total_is_unaffected(#[from(subject)] mut subject: MessageAssemblyState) { - let header = first_header(1, 5, false); - accept_first(&mut subject, &header, b"hello").expect("first frame accepted"); - - let msg = accept_continuation( - &mut subject, - Continuation { - key: 1, - sequence: 1, - body: b"worlds", - is_last: true, - }, - ) - .expect("undeclared total imposes no equality check") - .expect("message completes"); - - assert_eq!(msg.body(), b"helloworlds"); -} - #[rstest] fn declared_total_equal_to_first_frame_body_leaves_no_room( #[from(subject)] mut subject: MessageAssemblyState, diff --git a/src/message_assembler/tests.rs b/src/message_assembler/tests.rs index b582f677b..57508d9d1 100644 --- a/src/message_assembler/tests.rs +++ b/src/message_assembler/tests.rs @@ -213,3 +213,10 @@ mod budget_tests; #[path = "declared_total_tests.rs"] mod declared_total_tests; + +// ============================================================================= +// Declared total properties (issue #544) - see declared_total_property_tests.rs +// ============================================================================= + +#[path = "declared_total_property_tests.rs"] +mod declared_total_property_tests; diff --git a/tests/compile_error.rs b/tests/compile_error.rs index 4a0eafa9b..81fbc2df4 100644 --- a/tests/compile_error.rs +++ b/tests/compile_error.rs @@ -19,6 +19,7 @@ fn compile_tests() { t.compile_fail("tests/ui/zero_test_budget.rs"); t.pass("tests/ui/client_error_wildcard_match.rs"); t.compile_fail("tests/ui/client_error_exhaustive_match.rs"); + t.compile_fail("tests/ui/message_assembly_error_exhaustive_match.rs"); t.pass("tests/ui/wireframe_testing_exports.rs"); // Trybuild forwards this build's features to the fixture, so the pool // accessors only exist here when `test-support` is enabled. `make test` diff --git a/tests/steps/message_assembly_steps.rs b/tests/steps/message_assembly_steps.rs index adc0a35fe..4eeb54510 100644 --- a/tests/steps/message_assembly_steps.rs +++ b/tests/steps/message_assembly_steps.rs @@ -77,6 +77,70 @@ fn given_state( message_assembly_world.create_state(config); } +/// The frame attributes a Gherkin phrase may add beyond key and body. +#[derive(Clone, Debug, Default)] +struct FrameOptions { + metadata: Option>, + total: Option, + is_last: bool, +} + +impl FrameOptions { + /// Attach `metadata` to the frame. + fn metadata(self, metadata: Vec) -> Self { + Self { + metadata: Some(metadata), + ..self + } + } + + /// Declare a total body length. + fn total(self, total: CountParam) -> Self { + Self { + total: Some(total.0), + ..self + } + } + + /// Mark the frame as completing its message. + fn final_frame(self) -> Self { + Self { + is_last: true, + ..self + } + } + + /// Apply these attributes to `params`. + fn apply_to(self, params: FirstFrameParams) -> FirstFrameParams { + let with_metadata = match self.metadata { + Some(metadata) => params.with_metadata(metadata), + None => params, + }; + let with_total = match self.total { + Some(total) => with_metadata.with_total(total), + None => with_metadata, + }; + if self.is_last { + with_total.final_frame() + } else { + with_total + } + } +} + +/// Record the single first frame the scenario is currently describing. +fn record_first_frame( + world: &mut MessageAssemblyWorld, + key: MessageKeyParam, + body: &str, + options: FrameOptions, +) { + world.add_first_frame(options.apply_to(FirstFrameParams::new( + key.to_key(), + body.as_bytes().to_vec(), + ))); +} + #[rustfmt::skip] #[given("a first frame for key {key:MessageKeyParam} with metadata {metadata:string} and body {body:string}")] fn given_first_frame_with_metadata( @@ -85,9 +149,8 @@ fn given_first_frame_with_metadata( metadata: String, body: String, ) { - message_assembly_world.add_first_frame( - FirstFrameParams::new(key.to_key(), body.into_bytes()).with_metadata(metadata.into_bytes()), - ); + let options = FrameOptions::default().metadata(metadata.into_bytes()); + record_first_frame(message_assembly_world, key, &body, options); } #[given("a first frame for key {key:MessageKeyParam} with body {body:string}")] @@ -96,7 +159,7 @@ fn given_first_frame( key: MessageKeyParam, body: String, ) { - message_assembly_world.add_first_frame(FirstFrameParams::new(key.to_key(), body.into_bytes())); + record_first_frame(message_assembly_world, key, &body, FrameOptions::default()); } #[rustfmt::skip] @@ -107,9 +170,8 @@ fn given_first_frame_with_total( body: String, total: CountParam, ) { - message_assembly_world.add_first_frame( - FirstFrameParams::new(key.to_key(), body.into_bytes()).with_total(total.0), - ); + let options = FrameOptions::default().total(total); + record_first_frame(message_assembly_world, key, &body, options); } #[rustfmt::skip] @@ -120,11 +182,8 @@ fn given_final_first_frame_with_total( body: String, total: CountParam, ) { - message_assembly_world.add_first_frame( - FirstFrameParams::new(key.to_key(), body.into_bytes()) - .with_total(total.0) - .final_frame(), - ); + let options = FrameOptions::default().total(total).final_frame(); + record_first_frame(message_assembly_world, key, &body, options); } #[given("a final first frame for key {key:MessageKeyParam} with body {body:string}")] @@ -133,8 +192,8 @@ fn given_final_first_frame( key: MessageKeyParam, body: String, ) { - message_assembly_world - .add_first_frame(FirstFrameParams::new(key.to_key(), body.into_bytes()).final_frame()); + let options = FrameOptions::default().final_frame(); + record_first_frame(message_assembly_world, key, &body, options); } // ============================================================================= diff --git a/tests/ui/message_assembly_error_exhaustive_match.rs b/tests/ui/message_assembly_error_exhaustive_match.rs new file mode 100644 index 000000000..c30e2bbbd --- /dev/null +++ b/tests/ui/message_assembly_error_exhaustive_match.rs @@ -0,0 +1,22 @@ +//! A downstream match written before ADR 015 no longer compiles. +//! +//! `MessageAssemblyError` is not `#[non_exhaustive]`, so adding the three +//! declared-total variants is a compile-time break. This fixture pins that +//! break: a match that names every variant from before the change fails to +//! compile until the new variants are handled. + +use wireframe::message_assembler::MessageAssemblyError; + +fn describe(error: MessageAssemblyError) -> &'static str { + match error { + MessageAssemblyError::Series(_) => "series", + MessageAssemblyError::DuplicateFirstFrame { .. } => "duplicate first frame", + MessageAssemblyError::MessageTooLarge { .. } => "too large", + MessageAssemblyError::ConnectionBudgetExceeded { .. } => "connection budget", + MessageAssemblyError::InFlightBudgetExceeded { .. } => "in-flight budget", + } +} + +fn main() { + let _ = describe as fn(MessageAssemblyError) -> &'static str; +} diff --git a/tests/ui/message_assembly_error_exhaustive_match.stderr b/tests/ui/message_assembly_error_exhaustive_match.stderr new file mode 100644 index 000000000..ea62065a2 --- /dev/null +++ b/tests/ui/message_assembly_error_exhaustive_match.stderr @@ -0,0 +1,26 @@ +error[E0004]: non-exhaustive patterns: `MessageAssemblyError::DeclaredTotalTooSmall { .. }`, `MessageAssemblyError::DeclaredTotalExceeded { .. }` and `MessageAssemblyError::DeclaredTotalMismatch { .. }` not covered + --> tests/ui/message_assembly_error_exhaustive_match.rs:11:11 + | +11 | match error { + | ^^^^^ patterns `MessageAssemblyError::DeclaredTotalTooSmall { .. }`, `MessageAssemblyError::DeclaredTotalExceeded { .. }` and `MessageAssemblyError::DeclaredTotalMismatch { .. }` not covered + | +note: `MessageAssemblyError` defined here + --> src/message_assembler/error.rs + | + | pub enum MessageAssemblyError { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... + | DeclaredTotalTooSmall { + | --------------------- not covered +... + | DeclaredTotalExceeded { + | --------------------- not covered +... + | DeclaredTotalMismatch { + | --------------------- not covered + = note: the matched value is of type `MessageAssemblyError` +help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern, a match arm with multiple or-patterns as shown, or multiple match arms + | +16 ~ MessageAssemblyError::InFlightBudgetExceeded { .. } => "in-flight budget", +17 ~ MessageAssemblyError::DeclaredTotalTooSmall { .. } | MessageAssemblyError::DeclaredTotalExceeded { .. } | MessageAssemblyError::DeclaredTotalMismatch { .. } => todo!(), + | From 2f5ba1327462833da3b67c79287bcdd170f959da Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 08:09:50 +0100 Subject: [PATCH 11/12] Qualify the declared-total documentation claims Review of the declared-total contract found three statements that overstate what the code guarantees: - The consequence "the dispatched body length always equals the peer's declaration" only holds when the first frame supplies a total; a `None` declaration opts out, so the guarantee is conditional. - The rejected alternative that dismissed completion-only comparison claimed a peer could force "arbitrarily large" buffering. The configured message-size limit still bounds it; only the early rejection is lost. - The users' guide said "a rejected frame" removes the partial assembly. A rejected first frame is refused before any assembly is created, so there is nothing to remove; only rejected continuations and completion mismatches have state to free. Each claim now matches the behaviour pinned by the regression, property, and inbound-adapter tests. Co-Authored-By: Claude Code --- docs/adr-015-declared-total-body-length-contract.md | 7 +++++-- docs/users-guide.md | 9 +++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/adr-015-declared-total-body-length-contract.md b/docs/adr-015-declared-total-body-length-contract.md index d977dc920..52f4c9023 100644 --- a/docs/adr-015-declared-total-body-length-contract.md +++ b/docs/adr-015-declared-total-body-length-contract.md @@ -128,7 +128,9 @@ buffered length and the declaration are read together. ### Positive -- The dispatched body length always equals the peer's declaration. +- When the first frame declares a total, the dispatched body length always + equals that declaration. A frame that leaves `total_body_len` as `None` opts + out, so equality is guaranteed only for declared messages. - An under-declared message can no longer bypass an application-configured size limit. - Failures are typed per enforcement point, so diagnostics distinguish an @@ -148,7 +150,8 @@ buffered length and the declaration are read together. - Do not enforce only in the inbound adapter: another adapter would still see the unsafe domain contract. - Do not compare only at completion: a peer could force the runtime to buffer - an arbitrarily large body first. + a body up to the configured message-size limit before the mismatch is + detected, rather than rejecting the overshooting frame when it arrives. - Do not return the mismatched body with a warning: dispatch is the security boundary. diff --git a/docs/users-guide.md b/docs/users-guide.md index d94c09eba..36c7ca6cd 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1299,10 +1299,11 @@ When a protocol supplies it, the assembler enforces it at every boundary: exactly the declared total, or `MessageAssemblyError::DeclaredTotalMismatch` is returned. -A rejected frame also removes the partial assembly, so a mismatched message -never remains buffered and is never dispatched. Protocols that leave -`total_body_len` as `None` opt out of the equality requirement and keep the -previous behaviour. +A rejected continuation or a completion mismatch also removes its partial +assembly, so a mismatched message never remains buffered and is never +dispatched. A rejected first frame is refused before any assembly exists, so +there is nothing to remove. Protocols that leave `total_body_len` as `None` opt +out of the equality requirement and keep the previous behaviour. The declaration is also used for the size-limit check, so an exact total is the cheapest way to fail early on an oversized message. The decision is recorded in From 928ffadf365a92ee4c34b4010313acfb3f224fe9 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 09:06:16 +0100 Subject: [PATCH 12/12] Document the declared-total continuation errors CodeRabbit reported, as an outside-diff finding, that the `# Errors` sections of `accept_continuation_frame` and `accept_continuation_frame_at` omitted the declared-total variants they can now return. Name `DeclaredTotalExceeded` and `DeclaredTotalMismatch` in both so the public error contract matches the behaviour. Also drop the comma before the essential `because` clause in the migration guide, which CodeRabbit raised as an unverified grammar finding in the same review round. Gates: `make check-fmt`, `make lint`, `make typecheck`, `make test`, `make markdownlint`, and `make nixie` pass. Co-Authored-By: Claude Code --- docs/v0-3-0-to-v0-4-0-migration-guide.md | 2 +- src/message_assembler/state.rs | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/docs/v0-3-0-to-v0-4-0-migration-guide.md b/docs/v0-3-0-to-v0-4-0-migration-guide.md index 34f122a49..193f8579a 100644 --- a/docs/v0-3-0-to-v0-4-0-migration-guide.md +++ b/docs/v0-3-0-to-v0-4-0-migration-guide.md @@ -58,7 +58,7 @@ it exactly. See [ADR 015](adr-015-declared-total-body-length-contract.md) for the decision. Applications that declare a total must supply the exact body length. A protocol -that declares an approximate size will now see its messages rejected, because +that declares an approximate size will now see its messages rejected because the length actually assembled can no longer differ from the length promised. Leaving `total_body_len` as `None` opts out of the equality requirement and keeps the previous behaviour, so that is the migration path for a protocol diff --git a/src/message_assembler/state.rs b/src/message_assembler/state.rs index 7e3884f77..7128b549e 100644 --- a/src/message_assembler/state.rs +++ b/src/message_assembler/state.rs @@ -220,7 +220,11 @@ impl MessageAssemblyState { /// # Errors /// /// Returns an error if no assembly exists for this key, if continuity - /// validation fails, or if the size limit would be exceeded. + /// validation fails, or if the size limit would be exceeded. Returns + /// [`MessageAssemblyError::DeclaredTotalExceeded`] when the frame would + /// carry the body past its declared total, or + /// [`MessageAssemblyError::DeclaredTotalMismatch`] when the completing + /// frame leaves the body short of the declaration. pub fn accept_continuation_frame( &mut self, header: &ContinuationFrameHeader, @@ -253,7 +257,11 @@ impl MessageAssemblyState { /// # Errors /// /// Returns an error if no assembly exists for this key, if continuity - /// validation fails, or if the size limit would be exceeded. + /// validation fails, or if the size limit would be exceeded. Returns + /// [`MessageAssemblyError::DeclaredTotalExceeded`] when the frame would + /// carry the body past its declared total, or + /// [`MessageAssemblyError::DeclaredTotalMismatch`] when the completing + /// frame leaves the body short of the declaration. pub fn accept_continuation_frame_at( &mut self, header: &ContinuationFrameHeader,