Uh oh!
There was an error while loading. Please reload this page.
fix: require complete, exact-length hex for Bytes/BytesN args - #2684
fix: require complete, exact-length hex for Bytes/BytesN args#2684Galmanus wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR tightens hex-string parsing for Soroban Bytes/BytesN primitives to avoid silent zero-padding and to provide more specific error reporting for incomplete/invalid inputs.
Changes:
- Enforce even-length hex input for
BytesandBytesN. - Require
BytesNhex to decode to exactlyNbytes (no padding / no truncation). - Add targeted tests covering odd-hex and
BytesNunder/over-length cases.
Suppressed comments (1)
cmd/crates/soroban-spec-tools/src/lib.rs:1
- Adding new variants to a public enum is a SemVer-breaking change for downstream crates that exhaustively
matchonError(unless the enum is already#[non_exhaustive]). Consider markingErroras#[non_exhaustive](and updating any guidance/docs), or otherwise treat this as a major-version bump / avoid introducing new public variants by mapping into an existing variant.
#![allow(clippy::missing_errors_doc, clippy::must_use_candidate)]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #[error("invalid hex: expected an even number of hex digits, got {0}")] | ||
| OddHexLength(usize), | ||
| #[error( | ||
| "invalid length for BytesN<{expected}>: expected {expected} bytes but got {got} bytes; the input is incomplete" |
There was a problem hiding this comment.
Good catch — the variant is returned for over-length too, so the input is incomplete was misleading. Dropped that clause in c98824f; the message is now "invalid length for BytesN: expected N bytes but got M bytes".
| if s.len() % 2 != 0 { | ||
| return Err(Error::OddHexLength(s.len())); | ||
| } | ||
| let decoded = hex::decode(s).map_err(|_| Error::InvalidValue(Some(t.clone())))?; | ||
| if decoded.len() != bytes.n as usize { | ||
| return Err(Error::BytesNLengthMismatch { | ||
| expected: bytes.n as usize, | ||
| got: decoded.len(), | ||
| }); | ||
| } | ||
| decoded |
There was a problem hiding this comment.
Extracted the shared odd-length check + hex decode + error mapping into a decode_complete_hex helper in c98824f; both the BytesN and Bytes branches now call it.
| if s.len() % 2 != 0 { | ||
| return Err(Error::OddHexLength(s.len())); | ||
| } | ||
| ScVal::Bytes( | ||
| hex::decode(s) | ||
| .map_err(|_| Error::InvalidValue(Some(t.clone())))? | ||
| .try_into() | ||
| .map_err(|_| Error::InvalidValue(Some(t.clone())))?, | ||
| ) |
There was a problem hiding this comment.
Extracted the shared odd-length check + hex decode + error mapping into a decode_complete_hex helper in c98824f; both the BytesN and Bytes branches now call it.
Removes the right-align zero-padding applied to hex input for Bytes and BytesN contract-invoke parameters. Both now require an even number of hex digits, and BytesN<N> requires exactly N bytes, erroring (expected vs got) instead of silently zero-padding a short value. The BytesN<32> strkey backwards-compat path is preserved, and the shared padded_hex_from_str helper (still used by contract-id and salt parsing) is left untouched, so only the invoke value-parse path changes. Closesstellar#2244
…rror Copilot review: (1) BytesNLengthMismatch is also returned for over-length input, so drop 'the input is incomplete' from the message; (2) the odd-length check + hex decode + error mapping were duplicated across the BytesN and Bytes branches — extract decode_complete_hex().
c98824f to
d57bb33Compare| /// # Errors | ||
| /// | ||
| /// Might return an error | ||
| /// Decode a hex string that must be complete: an even number of digits, no | ||
| /// zero-padding (see #2244). Returns the raw bytes; callers enforce any length. | ||
| fn decode_complete_hex(s: &str, t: &ScType) -> Result<Vec<u8>, Error> { | ||
| if !s.len().is_multiple_of(2) { | ||
| return Err(Error::OddHexLength(s.len())); | ||
| } | ||
| hex::decode(s).map_err(|_| Error::InvalidValue(Some(t.clone()))) | ||
| } | ||
| pub fn from_string_primitive(s: &str, t: &ScType) -> Result<ScVal, Error> { |
What
Removes the right-align zero-padding applied to hex input for
BytesandBytesNcontract-invoke parameters. Now:BytesN<N>requires exactly N bytes and errors (stating expected vs got) instead of silently zero-padding a short value.The
BytesN<32>strkey backwards-compat path is preserved.Why
Closes#2244. Today typing e.g.
ABCDfor aBytesN<32>is silently accepted and zero-padded, which is surprising and dangerous — a mistyped fixed-length byte value is accepted rather than rejected. Thearg_parsinghelp text already promises "bytesN (exactly N bytes)"; this makes that true.Scope / safety
The shared helper
padded_hex_from_str(utils.rs) is left untouched — it still backs contract-id and 32-byte salt parsing (5 other callers) that legitimately rely on padding. The change is confined to theBytesN/Bytesvalue-parse arms insoroban-spec-tools::from_string.Breaking change
Yes — input previously accepted (short/odd hex for
BytesN) now errors. Flagged here for release notes.Tests
Added: under-length, over-length, and odd-hex rejection for
BytesN; odd-hex forBytes; exact-length still parses. All pre-existing bytes tests use exact-length input, so none change behavior.cargo test -p soroban-spec-toolsandcargo clippy -p soroban-spec-tools --all-targetsare green.