From 6fc7051b21e0520dd45c49dd4d26c1a03f27023e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Bj=C3=B6rkqvist?= Date: Tue, 23 Jun 2026 12:59:37 +0000 Subject: [PATCH 1/6] test: characterize missing ingress-window validation in Rosetta /construction/payloads Add a `validate_ingress_window` seam (currently a no-op) before the ingress expiry loop in both ICP Rosetta (`construction_payloads.rs`) and ICRC1 Rosetta (`services.rs`), plus regression tests that document the reported (ICPBB-134 / DEFI-2902) behavior: oversized windows, unbounded spans and the near-`u64::MAX` wrap payload are all currently accepted without any bound being enforced. These tests pass against current behavior; the enforcement and the assertion flip follow in subsequent commits. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../request_handler/construction_payloads.rs | 54 +++++++++++++++++++ .../icrc1/src/construction_api/services.rs | 38 +++++++++++++ 2 files changed, 92 insertions(+) diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index a160a6fe0634..6eb2c09b0772 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -34,6 +34,20 @@ use rosetta_core::convert::principal_id_from_public_key; use std::{collections::HashMap, sync::Arc, time::Duration}; use tracing::log::debug; +/// Validate the caller-provided ingress window before it is expanded into a +/// list of ingress expiries. +/// +/// NOTE: currently a no-op (see DEFI-2902). The 24-hour bound is enforced in a +/// follow-up commit; this seam exists so the reported (unbounded) behavior can +/// be characterized by a regression test first. +fn validate_ingress_window( + _now: ic_types::time::Time, + _ingress_start: ic_types::time::Time, + _ingress_end: ic_types::time::Time, +) -> Result<(), ApiError> { + Ok(()) +} + impl RosettaRequestHandler { /// Generate an Unsigned Transaction and Signing Payloads. /// See https://www.rosetta-api.org/docs/ConstructionApi.html#constructionpayloads @@ -96,6 +110,8 @@ impl RosettaRequestHandler { .map(Memo) .unwrap_or_else(|| Memo(rand::thread_rng().r#gen())); + validate_ingress_window(ic_types::time::current_time(), ingress_start, ingress_end)?; + let mut ingress_expiries = vec![]; let mut now = ingress_start; while now < ingress_end { @@ -1110,3 +1126,41 @@ fn neuron_subaccount( } } } + +#[cfg(test)] +mod tests { + use super::*; + use ic_types::time::Time; + + const HOUR_NANOS: u64 = 60 * 60 * 1_000_000_000; + // A realistic "now" (~2023) that is well away from the u64 boundary. + const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; + + fn t(nanos: u64) -> Time { + Time::from_nanos_since_unix_epoch(nanos) + } + + // Vector A (ICPBB-134): a window far larger than the documented 24h bound is + // currently accepted, because no bound is enforced before the loop. + #[test] + fn oversized_ingress_window_is_currently_accepted() { + assert!( + validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + 48 * HOUR_NANOS)) + .is_ok() + ); + } + + // Vector A' (ICPBB-134): a tiny start with a near-`u64::MAX` end (an enormous + // span) is currently accepted. + #[test] + fn unbounded_ingress_span_is_currently_accepted() { + assert!(validate_ingress_window(t(NOW_NANOS), t(0), t(u64::MAX)).is_ok()); + } + + // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload is currently accepted. + #[test] + fn near_u64_max_ingress_window_is_currently_accepted() { + let start = t(u64::MAX - 50 * 1_000_000_000); + assert!(validate_ingress_window(t(NOW_NANOS), start, t(u64::MAX)).is_ok()); + } +} diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 2c4a1c29c294..02a79da94a03 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -108,6 +108,16 @@ pub fn construction_combine( .map_err(|err| Error::processing_construction_failed(&err)) } +/// Validate the caller-provided ingress window before it is expanded into a +/// list of ingress expiries. +/// +/// NOTE: currently a no-op (see DEFI-2902). The 24-hour bound is enforced in a +/// follow-up commit; this seam exists so the reported (unbounded) behavior can +/// be characterized by a regression test first. +fn validate_ingress_window(_now: u64, _ingress_start: u64, _ingress_end: u64) -> Result<(), Error> { + Ok(()) +} + pub fn construction_payloads( operations: Vec, metadata: Option, @@ -157,6 +167,8 @@ pub fn construction_payloads( ))); } + validate_ingress_window(now, ingress_start, ingress_end)?; + // Every ingress message sent to the IC has an expiry timestamp until which the signature associated with that message is valid // To support a longer overall timeframe than one interval, we can send multiple ingress messages with two signable contents each let mut ingress_expiries = vec![]; @@ -261,6 +273,32 @@ mod tests { const NUM_TEST_CASES: u32 = 100; const NUM_BLOCKS: usize = 1; + const HOUR_NANOS: u64 = 60 * 60 * 1_000_000_000; + // A realistic "now" (~2023) that is well away from the u64 boundary. + const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; + + // Vector A (ICPBB-134): a window far larger than the documented 24h bound is + // currently accepted, because no bound is enforced before the loop. + #[test] + fn oversized_ingress_window_is_currently_accepted() { + assert!(validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + 48 * HOUR_NANOS).is_ok()); + } + + // Vector A' (ICPBB-134): a tiny start with a near-`u64::MAX` end (an enormous + // span) is currently accepted. + #[test] + fn unbounded_ingress_span_is_currently_accepted() { + assert!(validate_ingress_window(NOW_NANOS, 0, u64::MAX).is_ok()); + } + + // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload is currently accepted. + #[test] + fn near_u64_max_ingress_window_is_currently_accepted() { + assert!( + validate_ingress_window(NOW_NANOS, u64::MAX - 50 * 1_000_000_000, u64::MAX).is_ok() + ); + } + fn call_construction_derive(key_pair: &T) { let principal_id = key_pair.generate_principal_id().unwrap(); let public_key = ic_rosetta_test_utils::to_public_key(key_pair); From d453979d26451408277744d8e9448bcf131e87d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Bj=C3=B6rkqvist?= Date: Tue, 23 Jun 2026 13:04:58 +0000 Subject: [PATCH 2/6] fix: enforce a 24h ingress window in Rosetta /construction/payloads Enforce the documented 24-hour ingress window before the expiry loop in both ICP Rosetta and ICRC1 Rosetta `/construction/payloads`. `validate_ingress_window` now rejects requests where: - the span `ingress_end - ingress_start` exceeds 24h (closes the unbounded-loop / OOM vector with a large window), or - `ingress_end` is more than 24h in the future (closes the near-`u64::MAX` wraparound vector, whose span is tiny but whose end is absurdly far ahead). A span-only bound is insufficient on its own: the wrap payload uses a sub-minute span, so the future-bound check is what makes it unreachable. The existing roundtrip proptests are adapted to the new behavior: the ICRC1 `test_construction_parse` now expects rejection for out-of-bound windows, and the ICP `test_payloads_parse_identity` generator keeps `ingress_start` within a realistic window around now (which also removes a latent `ingress_start + ingress_interval` overflow). The characterization tests added in the previous commit now fail (they assert the old, unbounded behavior); they are updated to assert rejection in the next commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/request_handler/construction_parse.rs | 6 ++- .../request_handler/construction_payloads.rs | 36 ++++++++++++++--- rs/rosetta-api/icrc1/src/common/constants.rs | 3 ++ .../icrc1/src/construction_api/services.rs | 40 ++++++++++++++++--- 4 files changed, 73 insertions(+), 12 deletions(-) diff --git a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs index 0e8be5d6438c..661f99516c38 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs @@ -787,8 +787,12 @@ mod tests { .duration_since(SystemTime::UNIX_EPOCH) .expect("Could not get system time") .as_nanos() as u64; + // `ingress_start` is kept within a realistic window around `now`: the + // ingress window is now bounded to 24h (ICPBB-134 / DEFI-2902), and this + // also avoids overflowing `ingress_start + ingress_interval` below. + let gen_ingress_start = proptest::option::of(now..(now + ONE_HOUR_NANOS)); let gen_metadata = proptest::option::of( - (gen_opt_u64.clone(), gen_opt_u64.clone(), gen_opt_u64).prop_flat_map( + (gen_opt_u64.clone(), gen_ingress_start, gen_opt_u64).prop_flat_map( |(created_at_time, ingress_start, memo)| { proptest::option::of(1..ONE_HOUR_NANOS).prop_map(move |ingress_interval| { let ingress_end = ingress_interval.map(|ingress_interval| { diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index 6eb2c09b0772..a19c71c2fa8a 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -34,17 +34,41 @@ use rosetta_core::convert::principal_id_from_public_key; use std::{collections::HashMap, sync::Arc, time::Duration}; use tracing::log::debug; +/// Maximum permitted ingress window, mirroring the 24-hour bound documented on +/// `ConstructionPayloadsRequestMetadata`. +const MAX_INGRESS_WINDOW: Duration = Duration::from_secs(24 * 60 * 60); + /// Validate the caller-provided ingress window before it is expanded into a /// list of ingress expiries. /// -/// NOTE: currently a no-op (see DEFI-2902). The 24-hour bound is enforced in a -/// follow-up commit; this seam exists so the reported (unbounded) behavior can -/// be characterized by a regression test first. +/// Rejects windows that would make the expiry loop iterate an excessive (or, on +/// arithmetic wraparound, unbounded) number of times (see ICPBB-134 / +/// DEFI-2902). Two independent bounds are enforced: +/// - the span `ingress_end - ingress_start` must not exceed 24h, and +/// - `ingress_end` must not be more than 24h in the future, which also rejects +/// the near-`u64::MAX` payloads that would otherwise wrap the loop counter. fn validate_ingress_window( - _now: ic_types::time::Time, - _ingress_start: ic_types::time::Time, - _ingress_end: ic_types::time::Time, + now: ic_types::time::Time, + ingress_start: ic_types::time::Time, + ingress_end: ic_types::time::Time, ) -> Result<(), ApiError> { + let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; + let now = now.as_nanos_since_unix_epoch(); + let start = ingress_start.as_nanos_since_unix_epoch(); + let end = ingress_end.as_nanos_since_unix_epoch(); + + if end.saturating_sub(start) > max_window { + return Err(ApiError::invalid_request(format!( + "The ingress window (ingress_end - ingress_start) must not exceed {} hours.", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } + if end.saturating_sub(now) > max_window { + return Err(ApiError::invalid_request(format!( + "ingress_end must not be more than {} hours in the future.", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } Ok(()) } diff --git a/rs/rosetta-api/icrc1/src/common/constants.rs b/rs/rosetta-api/icrc1/src/common/constants.rs index d29f5e884eed..dc4a73a98c31 100644 --- a/rs/rosetta-api/icrc1/src/common/constants.rs +++ b/rs/rosetta-api/icrc1/src/common/constants.rs @@ -17,5 +17,8 @@ pub const SPENDER_OPERATION_IDENTIFIER: u64 = 7; pub const FEE_COLLECTOR_OPERATION_IDENTIFIER: u64 = 8; pub const MAX_TRANSACTIONS_PER_SEARCH_TRANSACTIONS_REQUEST: u64 = 10000; pub const INGRESS_INTERVAL_OVERLAP: Duration = Duration::from_secs(120); +/// Maximum permitted ingress window, mirroring the 24-hour bound documented on +/// `ConstructionPayloadsRequestMetadata`. +pub const MAX_INGRESS_WINDOW: Duration = Duration::from_secs(24 * 60 * 60); pub const STATUS_COMPLETED: &str = "COMPLETED"; pub const MAX_BLOCKS_PER_QUERY_BLOCK_RANGE_REQUEST: u64 = 10000; diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 02a79da94a03..62fffd635d89 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -7,7 +7,7 @@ use super::utils::{ handle_construction_hash, handle_construction_parse, handle_construction_payloads, handle_construction_submit, }; -use crate::common::constants::INGRESS_INTERVAL_OVERLAP; +use crate::common::constants::{INGRESS_INTERVAL_OVERLAP, MAX_INGRESS_WINDOW}; use crate::common::types::Error; use candid::Principal; use ic_base_types::{CanisterId, PrincipalId}; @@ -111,10 +111,26 @@ pub fn construction_combine( /// Validate the caller-provided ingress window before it is expanded into a /// list of ingress expiries. /// -/// NOTE: currently a no-op (see DEFI-2902). The 24-hour bound is enforced in a -/// follow-up commit; this seam exists so the reported (unbounded) behavior can -/// be characterized by a regression test first. -fn validate_ingress_window(_now: u64, _ingress_start: u64, _ingress_end: u64) -> Result<(), Error> { +/// Rejects windows that would make the expiry loop iterate an excessive (or, on +/// arithmetic wraparound, unbounded) number of times (see ICPBB-134 / +/// DEFI-2902). Two independent bounds are enforced: +/// - the span `ingress_end - ingress_start` must not exceed 24h, and +/// - `ingress_end` must not be more than 24h in the future, which also rejects +/// the near-`u64::MAX` payloads that would otherwise wrap the loop counter. +fn validate_ingress_window(now: u64, ingress_start: u64, ingress_end: u64) -> Result<(), Error> { + let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; + if ingress_end.saturating_sub(ingress_start) > max_window { + return Err(Error::processing_construction_failed(&format!( + "The ingress window (ingress_end - ingress_start) must not exceed {} hours: Start: {ingress_start}, End: {ingress_end}", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } + if ingress_end.saturating_sub(now) > max_window { + return Err(Error::processing_construction_failed(&format!( + "ingress_end must not be more than {} hours in the future: Current time: {now}, End: {ingress_end}", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } Ok(()) } @@ -499,6 +515,20 @@ mod tests { } (_, _) => {} } + // Windows wider than the permitted maximum, or with an + // ingress_end too far in the future, are rejected before + // the loop (ICPBB-134 / DEFI-2902). + let eff_start = payloads_metadata.ingress_start.unwrap_or(now); + let eff_end = payloads_metadata + .ingress_end + .unwrap_or(eff_start + ingress_interval); + let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; + if eff_end.saturating_sub(eff_start) > max_window + || eff_end.saturating_sub(now) > max_window + { + assert!(construction_payloads_response.is_err()); + continue; + } let construction_parse_response = construction_parse( construction_payloads_response .clone() From 031693efeff14f95909c130cc207e80deeaab24e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Bj=C3=B6rkqvist?= Date: Tue, 23 Jun 2026 13:10:26 +0000 Subject: [PATCH 3/6] test: assert ingress windows beyond the 24h bound are rejected Flip the characterization tests to assert that oversized windows, unbounded spans and the near-`u64::MAX` wrap payload are now rejected, and add a negative control (a realistic 5-minute window is accepted) plus a boundary test (exactly 24h is accepted, one nanosecond more is rejected) in both ICP and ICRC1 Rosetta. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../request_handler/construction_payloads.rs | 37 +++++++++++++----- .../icrc1/src/construction_api/services.rs | 39 ++++++++++++++----- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index a19c71c2fa8a..1af120adc262 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -1165,26 +1165,45 @@ mod tests { } // Vector A (ICPBB-134): a window far larger than the documented 24h bound is - // currently accepted, because no bound is enforced before the loop. + // rejected before the loop. #[test] - fn oversized_ingress_window_is_currently_accepted() { + fn oversized_ingress_window_is_rejected() { assert!( validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + 48 * HOUR_NANOS)) - .is_ok() + .is_err() ); } // Vector A' (ICPBB-134): a tiny start with a near-`u64::MAX` end (an enormous - // span) is currently accepted. + // span) is rejected. #[test] - fn unbounded_ingress_span_is_currently_accepted() { - assert!(validate_ingress_window(t(NOW_NANOS), t(0), t(u64::MAX)).is_ok()); + fn unbounded_ingress_span_is_rejected() { + assert!(validate_ingress_window(t(NOW_NANOS), t(0), t(u64::MAX)).is_err()); } - // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload is currently accepted. + // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload has a tiny span but a + // far-future end, so it is rejected by the future bound. #[test] - fn near_u64_max_ingress_window_is_currently_accepted() { + fn near_u64_max_ingress_window_is_rejected() { let start = t(u64::MAX - 50 * 1_000_000_000); - assert!(validate_ingress_window(t(NOW_NANOS), start, t(u64::MAX)).is_ok()); + assert!(validate_ingress_window(t(NOW_NANOS), start, t(u64::MAX)).is_err()); + } + + // Negative control: a realistic window (5 minutes ahead of now) is accepted. + #[test] + fn valid_ingress_window_is_accepted() { + let end = t(NOW_NANOS + 5 * 60 * 1_000_000_000); + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), end).is_ok()); + } + + // Boundary: a window of exactly 24h ending exactly 24h from now is accepted, + // one nanosecond more is rejected. + #[test] + fn ingress_window_boundary_is_inclusive() { + let max = MAX_INGRESS_WINDOW.as_nanos() as u64; + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + max)).is_ok()); + assert!( + validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + max + 1)).is_err() + ); } } diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 62fffd635d89..33dc572338ae 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -294,27 +294,48 @@ mod tests { const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; // Vector A (ICPBB-134): a window far larger than the documented 24h bound is - // currently accepted, because no bound is enforced before the loop. + // rejected before the loop. #[test] - fn oversized_ingress_window_is_currently_accepted() { - assert!(validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + 48 * HOUR_NANOS).is_ok()); + fn oversized_ingress_window_is_rejected() { + assert!( + validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + 48 * HOUR_NANOS).is_err() + ); } // Vector A' (ICPBB-134): a tiny start with a near-`u64::MAX` end (an enormous - // span) is currently accepted. + // span) is rejected. + #[test] + fn unbounded_ingress_span_is_rejected() { + assert!(validate_ingress_window(NOW_NANOS, 0, u64::MAX).is_err()); + } + + // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload has a tiny span but a + // far-future end, so it is rejected by the future bound. #[test] - fn unbounded_ingress_span_is_currently_accepted() { - assert!(validate_ingress_window(NOW_NANOS, 0, u64::MAX).is_ok()); + fn near_u64_max_ingress_window_is_rejected() { + assert!( + validate_ingress_window(NOW_NANOS, u64::MAX - 50 * 1_000_000_000, u64::MAX).is_err() + ); } - // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload is currently accepted. + // Negative control: a realistic window (5 minutes ahead of now) is accepted. #[test] - fn near_u64_max_ingress_window_is_currently_accepted() { + fn valid_ingress_window_is_accepted() { assert!( - validate_ingress_window(NOW_NANOS, u64::MAX - 50 * 1_000_000_000, u64::MAX).is_ok() + validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + 5 * 60 * 1_000_000_000) + .is_ok() ); } + // Boundary: a window of exactly 24h ending exactly 24h from now is accepted, + // one nanosecond more is rejected. + #[test] + fn ingress_window_boundary_is_inclusive() { + let max = MAX_INGRESS_WINDOW.as_nanos() as u64; + assert!(validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + max).is_ok()); + assert!(validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + max + 1).is_err()); + } + fn call_construction_derive(key_pair: &T) { let principal_id = key_pair.generate_principal_id().unwrap(); let public_key = ic_rosetta_test_utils::to_public_key(key_pair); From bd5d72891abb170132249afde7eb055e86f943b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Bj=C3=B6rkqvist?= Date: Tue, 23 Jun 2026 16:43:51 +0000 Subject: [PATCH 4/6] fix: address review on ingress-window validation - ICP `validate_ingress_window` now rejects `ingress_start >= ingress_end` before the span/future checks. Without this, the `saturating_sub`-based span check treated a reversed/empty window as a zero-length span that passed validation, yielding an empty set of ingress expiries (ICP, unlike ICRC1, had no prior start/end ordering guard). Adds a regression test. - The ICRC1 `test_construction_parse` proptest now calls `validate_ingress_window` directly instead of duplicating the bound logic inline, so the test cannot drift from production validation. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../request_handler/construction_payloads.rs | 17 ++++++++++++++++- .../icrc1/src/construction_api/services.rs | 5 +---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index 1af120adc262..d657a3918f4f 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -43,7 +43,9 @@ const MAX_INGRESS_WINDOW: Duration = Duration::from_secs(24 * 60 * 60); /// /// Rejects windows that would make the expiry loop iterate an excessive (or, on /// arithmetic wraparound, unbounded) number of times (see ICPBB-134 / -/// DEFI-2902). Two independent bounds are enforced: +/// DEFI-2902). It enforces that: +/// - `ingress_start` is strictly before `ingress_end` (an empty or reversed +/// window would otherwise produce an empty/degenerate set of payloads), /// - the span `ingress_end - ingress_start` must not exceed 24h, and /// - `ingress_end` must not be more than 24h in the future, which also rejects /// the near-`u64::MAX` payloads that would otherwise wrap the loop counter. @@ -57,6 +59,11 @@ fn validate_ingress_window( let start = ingress_start.as_nanos_since_unix_epoch(); let end = ingress_end.as_nanos_since_unix_epoch(); + if start >= end { + return Err(ApiError::invalid_request( + "ingress_start must be strictly before ingress_end.", + )); + } if end.saturating_sub(start) > max_window { return Err(ApiError::invalid_request(format!( "The ingress window (ingress_end - ingress_start) must not exceed {} hours.", @@ -1189,6 +1196,14 @@ mod tests { assert!(validate_ingress_window(t(NOW_NANOS), start, t(u64::MAX)).is_err()); } + // A reversed or empty window (ingress_end <= ingress_start) is rejected + // rather than silently producing an empty set of payloads. + #[test] + fn reversed_ingress_window_is_rejected() { + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS + 1), t(NOW_NANOS)).is_err()); + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS)).is_err()); + } + // Negative control: a realistic window (5 minutes ahead of now) is accepted. #[test] fn valid_ingress_window_is_accepted() { diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 33dc572338ae..956add9ff128 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -543,10 +543,7 @@ mod tests { let eff_end = payloads_metadata .ingress_end .unwrap_or(eff_start + ingress_interval); - let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; - if eff_end.saturating_sub(eff_start) > max_window - || eff_end.saturating_sub(now) > max_window - { + if validate_ingress_window(now, eff_start, eff_end).is_err() { assert!(construction_payloads_response.is_err()); continue; } From 2cb6d4cafddca09bc33bfa06862539c81ff034e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Bj=C3=B6rkqvist?= Date: Wed, 24 Jun 2026 12:43:42 +0000 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20drop?= =?UTF-8?q?=20ticket=20refs=20from=20code,=20align=20ICRC1=20validator?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove the bug-bounty (ICPBB-134) and Jira (DEFI-2902) identifiers from code comments; the PR title carries the ticket reference. - Move the `ingress_start >= ingress_end` rejection into the ICRC1 `validate_ingress_window` (it previously lived as a separate guard in the caller), so the ICP and ICRC1 validators enforce the same set of checks. The now-redundant standalone guard in `construction_payloads` is removed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/request_handler/construction_parse.rs | 2 +- .../request_handler/construction_payloads.rs | 9 ++++--- .../icrc1/src/construction_api/services.rs | 24 +++++++++---------- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs index 661f99516c38..6d27428c96a9 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs @@ -788,7 +788,7 @@ mod tests { .expect("Could not get system time") .as_nanos() as u64; // `ingress_start` is kept within a realistic window around `now`: the - // ingress window is now bounded to 24h (ICPBB-134 / DEFI-2902), and this + // ingress window is now bounded to 24h, and this // also avoids overflowing `ingress_start + ingress_interval` below. let gen_ingress_start = proptest::option::of(now..(now + ONE_HOUR_NANOS)); let gen_metadata = proptest::option::of( diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index d657a3918f4f..a093050d5c67 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -42,8 +42,7 @@ const MAX_INGRESS_WINDOW: Duration = Duration::from_secs(24 * 60 * 60); /// list of ingress expiries. /// /// Rejects windows that would make the expiry loop iterate an excessive (or, on -/// arithmetic wraparound, unbounded) number of times (see ICPBB-134 / -/// DEFI-2902). It enforces that: +/// arithmetic wraparound, unbounded) number of times. It enforces that: /// - `ingress_start` is strictly before `ingress_end` (an empty or reversed /// window would otherwise produce an empty/degenerate set of payloads), /// - the span `ingress_end - ingress_start` must not exceed 24h, and @@ -1171,7 +1170,7 @@ mod tests { Time::from_nanos_since_unix_epoch(nanos) } - // Vector A (ICPBB-134): a window far larger than the documented 24h bound is + // Vector A: a window far larger than the documented 24h bound is // rejected before the loop. #[test] fn oversized_ingress_window_is_rejected() { @@ -1181,14 +1180,14 @@ mod tests { ); } - // Vector A' (ICPBB-134): a tiny start with a near-`u64::MAX` end (an enormous + // Vector A': a tiny start with a near-`u64::MAX` end (an enormous // span) is rejected. #[test] fn unbounded_ingress_span_is_rejected() { assert!(validate_ingress_window(t(NOW_NANOS), t(0), t(u64::MAX)).is_err()); } - // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload has a tiny span but a + // Vector B: the near-`u64::MAX` wrap payload has a tiny span but a // far-future end, so it is rejected by the future bound. #[test] fn near_u64_max_ingress_window_is_rejected() { diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 956add9ff128..22b92e7b3e7e 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -112,13 +112,19 @@ pub fn construction_combine( /// list of ingress expiries. /// /// Rejects windows that would make the expiry loop iterate an excessive (or, on -/// arithmetic wraparound, unbounded) number of times (see ICPBB-134 / -/// DEFI-2902). Two independent bounds are enforced: +/// arithmetic wraparound, unbounded) number of times. It enforces that: +/// - `ingress_start` is strictly before `ingress_end` (an empty or reversed +/// window would otherwise produce an empty/degenerate set of payloads), /// - the span `ingress_end - ingress_start` must not exceed 24h, and /// - `ingress_end` must not be more than 24h in the future, which also rejects /// the near-`u64::MAX` payloads that would otherwise wrap the loop counter. fn validate_ingress_window(now: u64, ingress_start: u64, ingress_end: u64) -> Result<(), Error> { let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; + if ingress_start >= ingress_end { + return Err(Error::processing_construction_failed(&format!( + "Ingress start should start before ingress end: Start: {ingress_start}, End: {ingress_end}" + ))); + } if ingress_end.saturating_sub(ingress_start) > max_window { return Err(Error::processing_construction_failed(&format!( "The ingress window (ingress_end - ingress_start) must not exceed {} hours: Start: {ingress_start}, End: {ingress_end}", @@ -171,12 +177,6 @@ pub fn construction_payloads( .and_then(|meta| meta.memo.clone()) .map(|memo| memo.into()); - if ingress_start >= ingress_end { - return Err(Error::processing_construction_failed(&format!( - "Ingress start should start before ingress end: Start: {ingress_start}, End: {ingress_end}" - ))); - } - if ingress_end < now + ingress_interval { return Err(Error::processing_construction_failed(&format!( "Ingress end should be at least one interval from the current time: Current time: {now}, End: {ingress_end}" @@ -293,7 +293,7 @@ mod tests { // A realistic "now" (~2023) that is well away from the u64 boundary. const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; - // Vector A (ICPBB-134): a window far larger than the documented 24h bound is + // Vector A: a window far larger than the documented 24h bound is // rejected before the loop. #[test] fn oversized_ingress_window_is_rejected() { @@ -302,14 +302,14 @@ mod tests { ); } - // Vector A' (ICPBB-134): a tiny start with a near-`u64::MAX` end (an enormous + // Vector A': a tiny start with a near-`u64::MAX` end (an enormous // span) is rejected. #[test] fn unbounded_ingress_span_is_rejected() { assert!(validate_ingress_window(NOW_NANOS, 0, u64::MAX).is_err()); } - // Vector B (ICPBB-134): the near-`u64::MAX` wrap payload has a tiny span but a + // Vector B: the near-`u64::MAX` wrap payload has a tiny span but a // far-future end, so it is rejected by the future bound. #[test] fn near_u64_max_ingress_window_is_rejected() { @@ -538,7 +538,7 @@ mod tests { } // Windows wider than the permitted maximum, or with an // ingress_end too far in the future, are rejected before - // the loop (ICPBB-134 / DEFI-2902). + // the loop. let eff_start = payloads_metadata.ingress_start.unwrap_or(now); let eff_end = payloads_metadata .ingress_end From b889907a073cd97f596a2d4f1040cea06beff671 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Bj=C3=B6rkqvist?= Date: Wed, 24 Jun 2026 12:54:15 +0000 Subject: [PATCH 6/6] docs: make ingress-window test comments self-explanatory Replace the "Vector A/B" labels (which referenced an external report) with comments that describe the actual scenario each test exercises: oversized span, near-full-u64 span, and a small-span window ending near u64::MAX that would wrap the loop counter. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/request_handler/construction_payloads.rs | 14 ++++++++------ .../icrc1/src/construction_api/services.rs | 14 ++++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index a093050d5c67..986b027d2598 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -1170,8 +1170,8 @@ mod tests { Time::from_nanos_since_unix_epoch(nanos) } - // Vector A: a window far larger than the documented 24h bound is - // rejected before the loop. + // A window whose span exceeds the documented 24h bound would make the loop + // build a huge list of ingress expiries, so it is rejected before the loop. #[test] fn oversized_ingress_window_is_rejected() { assert!( @@ -1180,15 +1180,17 @@ mod tests { ); } - // Vector A': a tiny start with a near-`u64::MAX` end (an enormous - // span) is rejected. + // A near-zero start together with a near-`u64::MAX` end spans almost the + // entire u64 range; that enormous span is rejected (it would otherwise + // iterate billions of times). #[test] fn unbounded_ingress_span_is_rejected() { assert!(validate_ingress_window(t(NOW_NANOS), t(0), t(u64::MAX)).is_err()); } - // Vector B: the near-`u64::MAX` wrap payload has a tiny span but a - // far-future end, so it is rejected by the future bound. + // A window ending at (near) `u64::MAX` has a small span but an end far in the + // future; without the future bound the loop counter would wrap past + // `u64::MAX` and never terminate. The future bound rejects it. #[test] fn near_u64_max_ingress_window_is_rejected() { let start = t(u64::MAX - 50 * 1_000_000_000); diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 22b92e7b3e7e..ef2448c46328 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -293,8 +293,8 @@ mod tests { // A realistic "now" (~2023) that is well away from the u64 boundary. const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; - // Vector A: a window far larger than the documented 24h bound is - // rejected before the loop. + // A window whose span exceeds the documented 24h bound would make the loop + // build a huge list of ingress expiries, so it is rejected before the loop. #[test] fn oversized_ingress_window_is_rejected() { assert!( @@ -302,15 +302,17 @@ mod tests { ); } - // Vector A': a tiny start with a near-`u64::MAX` end (an enormous - // span) is rejected. + // A near-zero start together with a near-`u64::MAX` end spans almost the + // entire u64 range; that enormous span is rejected (it would otherwise + // iterate billions of times). #[test] fn unbounded_ingress_span_is_rejected() { assert!(validate_ingress_window(NOW_NANOS, 0, u64::MAX).is_err()); } - // Vector B: the near-`u64::MAX` wrap payload has a tiny span but a - // far-future end, so it is rejected by the future bound. + // A window ending at (near) `u64::MAX` has a small span but an end far in the + // future; without the future bound the loop counter would wrap past + // `u64::MAX` and never terminate. The future bound rejects it. #[test] fn near_u64_max_ingress_window_is_rejected() { assert!(