fix(core): guard addresscodec against short/malformed payloads#342
Open
e-desouza wants to merge 2 commits into
Open
fix(core): guard addresscodec against short/malformed payloads#342e-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 #342 +/- ##
==========================================
+ Coverage 86.71% 86.73% +0.01%
==========================================
Files 252 252
Lines 32630 32682 +52
==========================================
+ Hits 28296 28347 +51
- Misses 4334 4335 +1
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 address codec against short/malformed decoded payloads that previously caused slice panics, converting those cases into structured UnexpectedPayloadLength errors (fixing #280 / remote DoS risk when handling untrusted inputs).
Changes:
- Added decoded-length guards in
decode_base58,_get_tag_from_buffer, andxaddress_to_classic_addressto prevent out-of-bounds slicing. - Introduced named constants (
XADDRESS_MIN_PAYLOAD_LEN,TAG_SUFFIX_MIN_LEN) to document and enforce minimum payload sizes. - Added regression tests covering short-payload cases and ensuring errors are returned instead of panics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/core/addresscodec/utils.rs | Adds a decoded-length guard to decode_base58 and a regression test for short payload handling. |
| src/core/addresscodec/mod.rs | Adds minimum-length constants and guards for X-address decoding/tag extraction, plus regression tests for short/malformed inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // address). Passing a 22-byte prefix exceeds the decoded payload length, so the | ||
| // length guard must return Err rather than panicking at the slice operation. | ||
| let long_prefix = [0u8; 22]; | ||
| assert!(decode_base58(ENCODED, &long_prefix).is_err()); |
| .with_alphabet(&XRPL_ALPHABET) | ||
| .with_check() | ||
| .into_string(); | ||
| assert!(xaddress_to_classic_address(&short).is_err()); |
…regression tests (#280)
…hort-payload tests
Replace bare is_err() assertions with matches! checks that pin the specific
UnexpectedPayloadLength { expected, found } values, ensuring the length
guards in decode_base58 and xaddress_to_classic_address fire for the
intended reason rather than any incidental error path.
e-desouza
force-pushed
the
fix/issue-280-addresscodec-panic
branch
from
July 15, 2026 16:42
9536126 to
c61c26e
Compare
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
decode_base58,_get_tag_from_buffer, andxaddress_to_classic_addressall sliced byte arrays without length checks. A valid-checksum base58 string whose decoded payload was shorter than expected reached slice operations that panicked instead of returning an error.Fixes #280.
What changed
decode_base58: addedif decoded.len() < prefix_lenguard returningUnexpectedPayloadLength { expected, found }._get_tag_from_buffer: addedif buffer.len() < TAG_SUFFIX_MIN_LENguard (9 bytes: 1 flag + 8 tag).xaddress_to_classic_address: addedif decoded.len() < XADDRESS_MIN_PAYLOAD_LENguard (22 bytes: 2 prefix + 20 address) before any slicing.XADDRESS_MIN_PAYLOAD_LEN = 22andTAG_SUFFIX_MIN_LEN = 9replace bare literals.How to validate
New tests:
test_decode_base58_short_payload_returns_error,test_xaddress_to_classic_too_short_returns_error,test_is_valid_xaddress_short_returns_false,test_get_tag_from_buffer_short_suffix_returns_error.