fix(Rosetta): DEFI-2902: Enforce a 24h ingress window in Rosetta /construction/payloads#10547
Open
mbjorkqvist wants to merge 4 commits into
Open
fix(Rosetta): DEFI-2902: Enforce a 24h ingress window in Rosetta /construction/payloads#10547mbjorkqvist wants to merge 4 commits into
mbjorkqvist wants to merge 4 commits into
Conversation
…truction/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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Rosetta /construction/payloads endpoints (ICP + ICRC1) by enforcing the documented 24-hour ingress window constraints before expanding caller-provided ingress timestamps into per-interval ingress expiries, preventing unauthenticated DoS via excessive or non-terminating expiry loops (ICPBB-134 / DEFI-2902).
Changes:
- Added
validate_ingress_windowchecks to reject ingress windows with span > 24h and/oringress_end> 24h in the future (wraparound defense). - Introduced
MAX_INGRESS_WINDOWconstant (ICRC1) and added targeted unit tests for oversized and wraparound vectors (ICP + ICRC1). - Updated proptests to accommodate the newly enforced bounds.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rs/rosetta-api/icrc1/src/construction_api/services.rs | Adds ingress-window validation + tests; enforces checks before expiry expansion. |
| rs/rosetta-api/icrc1/src/common/constants.rs | Introduces shared MAX_INGRESS_WINDOW (24h) constant for ICRC1 Rosetta. |
| rs/rosetta-api/icp/src/request_handler/construction_payloads.rs | Adds ingress-window validation in ICP Rosetta and unit tests. |
| rs/rosetta-api/icp/src/request_handler/construction_parse.rs | Adjusts proptest metadata generation to stay within the new ingress-window constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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) <noreply@anthropic.com>
mbjorkqvist
changed the title
|
✅ No security or compliance issues detected. Reviewed everything up to bd5d728. Security Overview
Detected Code Changes| Change Type | Relevant files ... (code changes summary truncated to fit VCS comment limits.) |
gregorydemay
approved these changes
Jun 24, 2026
gregorydemay
left a comment
gregorydemay
left a comment
Contributor
There was a problem hiding this comment.
Thanks @mbjorkqvist ! Just a comment asking why the validating functions are slightly different.
| Time::from_nanos_since_unix_epoch(nanos) | ||
| } | ||
|
|
||
| // Vector A (ICPBB-134): a window far larger than the documented 24h bound is |
Contributor
There was a problem hiding this comment.
nit: do we want the bugbounty number to appear in the code? Maybe better to keep it only in the Jira ticket?
| /// - `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; |
Contributor
There was a problem hiding this comment.
Contrary to the validating function in ICP Rosetta the check
if start >= end {
return Err(ApiError::invalid_request(
"ingress_start must be strictly before ingress_end.",
));
}
seems to be missing, is that on purpose?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
/construction/payloadsendpoint of both ICP Rosetta and ICRC1 Rosetta accepts caller-controlledingress_start/ingress_endvalues and expands them into a list of ingress expiries without enforcing the 24-hour bound documented onConstructionPayloadsRequestMetadata. This lets an unauthenticated request drive the expiry loop to exhaust memory and crash an operator's Rosetta instance.Two distinct payloads trigger it: an oversized window (a huge
ingress_end - ingress_startspan), and a near-u64::MAXwindow whose span is tiny but whose increment wraps so the loop never terminates.What
validate_ingress_windownow runs before the loop in both services and rejects a request when either:ingress_end - ingress_startexceeds 24h, oringress_endis more than 24h in the future.The second bound is what closes the wraparound payload — its span is sub-minute, so a span-only check would not catch it.
The change is built up across three commits: a regression test characterizing the current (unbounded) behavior, the enforcement itself, and the flip of those tests to assert rejection plus a negative control and a boundary case. The existing roundtrip proptests are adapted to the new behavior.
Scope is limited to operator-deployed Rosetta instances; IC mainnet, NNS, consensus and ledger canisters are unaffected.