Skip to content

fix(models): don't reflect rejected Amount string in deserialization error - #5078

Open
damip wants to merge 4 commits into
mainfrom
fix/amount-error-no-reflect-input
Open

damip wants to merge 4 commits into
mainfrom
fix/amount-error-no-reflect-input

Conversation

@damip

@damip damip commented Jul 21, 2026

Copy link
Copy Markdown
Member

Summary

AmountVisitor::visit_str mapped parse failures to E::invalid_value(Unexpected::Str(value), &self), embedding the full, attacker-controlled input string into the serde/JSON-RPC error message. On public JSON-RPC methods that deserialize Amount (e.g. execute_read_only_call), this makes error-path memory allocation scale with the invalid field length (bounded by max_request_body_size, but still avoidable amplification).

Addresses AI-report Finding 85 (severity: minor).

Fix

Use Unexpected::Other("string") instead of Unexpected::Str(value), so the error no longer reflects the rejected input.

Why this is non-breaking

  • Only the text of the error message for invalid input changes; valid Amount values deserialize identically.
  • No change to serialization, wire format, consensus, or the success path.

Testing

  • test_valid_amount_still_deserializes — valid input still parses.
  • test_invalid_amount_error_does_not_reflect_input — a 4096-char bogus string is absent from the error message.
  • cargo test -p massa_models --features test-exports amount: 2/2 pass.
  • cargo clippy -p massa_models --all-targets --features test-exports: clean. cargo fmt: clean.

Checklist

  • document all added functions (n/a)
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging (n/a)
  • if the API has changed, update the API specification (n/a — only error text)

Made with Cursor

@damip
damip marked this pull request as ready for review July 21, 2026 12:53
Comment thread massa-models/src/amount.rs Outdated
damip and others added 2 commits September 4, 2026 11:49
…error

AmountVisitor::visit_str mapped parse failures to
E::invalid_value(Unexpected::Str(value), &self), embedding the full
attacker-controlled input into the serde/JSON-RPC error message. On public
JSON-RPC methods that deserialize Amount, this makes error-path allocation
scale with the (body-size-bounded) invalid field length.

Use Unexpected::Other("string") so the error no longer reflects the input.
Behavior for valid amounts is unchanged.

Addresses AI-report Finding 85.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Leo-Besancon
Leo-Besancon force-pushed the fix/amount-error-no-reflect-input branch from 7036119 to 6326459 Compare September 4, 2026 09:49
@Leo-Besancon
Leo-Besancon self-requested a review September 4, 2026 09:53
Leo-Besancon
Leo-Besancon previously approved these changes Sep 4, 2026

@Leo-Besancon Leo-Besancon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@damip damip left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Leo-Besancon yes, keep E::custom, it is the best of the three: bounded and it says what is wrong. I checked that every message reachable from Amount::from_str is a constant: rust_decimal 1.37 parse_str_radix_10_exact only produces Error::from("Invalid decimal: ...") literals (no format! of the input anywhere in str.rs), the other Error variants have fixed Display text, and from_decimal uses four constant messages. AmountParseError is built nowhere else, so the new {0} in its Display cannot leak anything.

@modship your suggestion is superseded by Leo's commit (same idea, plus the actual reason), can you re-review?

One inline fix on the comment, then merge. No consensus or wire impact: serde Amount is JSON only (API, config, genesis files), the binary format uses the nom deserializer.

For the record: the same Unexpected::Str(value) line exists in version.rs, but Version is only ever in responses (NodeStatus.version), never deserialized from untrusted input. Not worth touching.

Comment thread massa-models/src/amount.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants