fix(binarycodec): reject 0X prefix, harden MPT sign bit, add bounds checks#310
Open
e-desouza wants to merge 2 commits into
Open
fix(binarycodec): reject 0X prefix, harden MPT sign bit, add bounds checks#310e-desouza wants to merge 2 commits into
e-desouza wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
=======================================
Coverage ? 84.29%
=======================================
Files ? 200
Lines ? 20794
Branches ? 0
=======================================
Hits ? 17528
Misses ? 3266
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the Amount binary codec’s MPT handling to align behavior with other XRPL implementations and to avoid panics on malformed buffers during (de)serialization.
Changes:
- Restricts MPT hex parsing to lowercase
0xprefix (rejects0X) to match xrpl.js canonicalization. - Rejects MPT payloads with the positive bit cleared during JSON serialization (treats as malformed wire data).
- Adds bounds checks to avoid indexing short buffers in MPT serialization and in
Amountclassification helpers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/core/binarycodec/types/amount.rs |
Tightens MPT parsing/serialization rules and adds length checks; adds targeted regression tests. |
src/core/binarycodec/test/tx_encode_decode_tests.rs |
Updates existing MPT sign-bit test to expect serialization failure for malformed payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
261
to
269
| /// Returns True if this amount is a native XRP amount. | ||
| pub fn is_native(&self) -> bool { | ||
| self.0[0] & 0x80 == 0 && self.0[0] & 0x20 == 0 | ||
| !self.0.is_empty() && self.0[0] & 0x80 == 0 && self.0[0] & 0x20 == 0 | ||
| } | ||
|
|
||
| /// Returns True if this amount is an MPT amount. | ||
| pub fn is_mpt(&self) -> bool { | ||
| self.0[0] & 0x80 == 0 && self.0[0] & 0x20 != 0 | ||
| !self.0.is_empty() && self.0[0] & 0x80 == 0 && self.0[0] & 0x20 != 0 | ||
| } |
| let sign = if is_positive { "" } else { "-" }; | ||
| // MPT amounts are unsigned; a cleared positive bit indicates a malformed payload. | ||
| if !is_positive { | ||
| return Err(S::Error::custom("MPT amount has negative sign bit set")); |
Comment on lines
+618
to
+619
| let mpt_id = | ||
| "A000000000000000000000000000000000000000000000000000000000000000"[..48].to_string(); |
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.
Summary
Three correctness issues in the MPT amount codec are addressed together, as they all live in the same file and are closely related.
Issue #258 - Drop uppercase
0Xhex prefix_serialize_mpt_amountpreviously accepted both0xand0Xas valid hex prefixes. xrpl.js only accepts lowercase0x, so allowing0Xcreated a silent interoperability gap. The check is nowvalue.strip_prefix("0x")only. A test confirms that"0X1F"now returns an error and falls through to decimal parsing, which also fails.Issue #259 - MPT encoder hardcodes positive bit; decoder must reject negative wire values
The encoder correctly sets
result[0] = 0x60(MPT flag0x20and positive flag0x40) and rejects negative inputs at encode time. The gap was on the decode side: theSerializeimpl previously read the positive bit from the leading byte and emitted a"-"prefix if it was clear, meaning a malformed wire payload would round-trip as a negative string instead of surfacing an error. The fix adds a guard in the MPT serialization branch, returningErrwhen the positive bit is not set. The existingtest_mpt_amount_negative_sign_bittest is updated to assert the new error behavior rather than the former silent negative-value output.Issue #262 - Bounds checks before indexing
is_native,is_mpt, andis_positiveall indexedself.0[0]orself.0[1]unconditionally, panicking on an empty or single-byte buffer. The MPT serialization path also indexedbytes[1..9]andbytes[9..33]without checking length. Fixed as follows.is_nativeandis_mptuse!self.0.is_empty()before accessingself.0[0].is_positivechecksself.0.len() >= 2before accessingself.0[1].SerializereturnsErr("MPT amount buffer too short")whenbytes.len() < 33.Tests confirm that
Amount::new(Some(&[]))returnsfalsefor all three bool methods, and that serializing a 5-byte buffer with a valid MPT leading byte returns an error.Test plan
test_mpt_reject_uppercase_hex_prefix: verifies"0X1F"is rejected at encode time.test_mpt_negative_sign_bit_rejected_on_serialize: verifies a 33-byte buffer with0x20leading byte (positive bit clear) fails serialization.test_amount_bool_methods_empty_buffer: verifiesis_native,is_mpt,is_positiveall returnfalseon an empty buffer.test_mpt_short_buffer_serialize_error: verifies a 5-byte MPT buffer fails serialization.test_mpt_amount_negative_sign_bitupdated to expect an error instead of"-100".cargo test --release.-D warningswith full feature set.