Skip to content

starknet_api: do not panic on malformed resource_bounds in tx json deserializer#14412

Open
avi-starkware wants to merge 1 commit into
main-v0.14.3from
avi/v0.14.3-depanic-tx-json-deserializer
Open

starknet_api: do not panic on malformed resource_bounds in tx json deserializer#14412
avi-starkware wants to merge 1 commit into
main-v0.14.3from
avi/v0.14.3-depanic-tx-json-deserializer

Conversation

@avi-starkware

@avi-starkware avi-starkware commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Why

upper_case_resource_bounds_names panics via .expect() when a V3 transaction JSON lacks a resource_bounds field, when the field is not an object, or when l1_gas is present without l2_gas. This is reachable from user-controlled input: apollo_http_server's add-transaction endpoint feeds the raw request body to deserialize_transaction_json_to_starknet_api_tx on its deprecated-transaction metrics path, so the body {"type":"INVOKE","version":"0x3"} panics the serving task on every node. The panic predates the move into starknet_api (#14408), but a pub fn … -> serde_json::Result in a foundational crate must not hide panic paths — new callers will trust the signature.

What

  • upper_case_resource_bounds_names now leaves the transaction unchanged when resource_bounds is missing or not an object, and normalizes each resource-bound key independently. The typed deserialization that follows remains the authority on required fields and reports the error — malformed input now yields Err instead of a panic.
  • Regression test covering all three former panic paths (missing field, non-object field, l1_gas without l2_gas), asserting Err.

Behavior for well-formed input is unchanged: blockifier_reexecution's raw_rpc_json_test fixture suite (real invoke/declare/deploy-account/L1-handler JSONs across versions) passes unmodified.

Validation

  • New test passes (each scenario panicked before this change).
  • cargo test -p starknet_api — 88 passed, 0 failed.
  • cargo test -p blockifier_reexecution raw_rpc — 12 passed (happy-path parity).
  • cargo test -p apollo_http_server — 37 passed.
  • cargo clippy -p starknet_api --no-deps --all-targets — clean; scripts/rust_fmt.sh applied.

Stacked on #14408 (which moved the function into starknet_api).

🤖 Generated with Claude Code

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

avi-starkware commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator Author

@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

@avi-starkware avi-starkware marked this pull request as ready for review June 8, 2026 10:37
@cursor

cursor Bot commented Jun 8, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Narrow change to error handling in legacy JSON normalization; valid transaction deserialization paths are unchanged and failures become explicit Err instead of panics.

Overview
Hardens V3 transaction JSON preprocessing so malformed resource_bounds no longer crashes the process. upper_case_resource_bounds_names used .expect() when resource_bounds was missing, not an object, or when l1_gas appeared without l2_gas—reachable from raw HTTP bodies on the add-transaction path.

The helper now returns early if resource_bounds is absent or not a JSON object, and renames each lowercase bound key independently (l1_gas, l2_gas, l1_data_gas) via a small loop. Invalid or incomplete bounds are left for the existing typed serde_json::from_value step, which returns Err as the public API promises.

A regression test asserts no panic and is_err() for missing resource_bounds, non-object resource_bounds, and l1_gas without l2_gas. Well-formed V3 JSON behavior is unchanged.

Reviewed by Cursor Bugbot for commit 94bd28e. Bugbot is set up for automated code reviews on this repo. Configure here.

…serializer

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@avi-starkware avi-starkware changed the base branch from avi/v0.14.3-http-server-drop-reexecution-dep to graphite-base/14412 June 8, 2026 10:39
@avi-starkware avi-starkware force-pushed the graphite-base/14412 branch from 0cd1903 to 7b6e416 Compare June 8, 2026 10:39
@avi-starkware avi-starkware force-pushed the avi/v0.14.3-depanic-tx-json-deserializer branch from d3fbe70 to 94bd28e Compare June 8, 2026 10:39
@avi-starkware avi-starkware changed the base branch from graphite-base/14412 to main-v0.14.3 June 8, 2026 10:39

@AvivYossef-starkware AvivYossef-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@AvivYossef-starkware reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on avi-starkware).


crates/starknet_api/src/serde_utils.rs line 165 at r1 (raw file):

/// Input may be attacker-controlled (e.g. an unparsable transaction body received over HTTP), so
/// a missing or non-object `resource_bounds` field must not panic; the transaction is left
/// unchanged and the typed deserialization that follows reports the error.

I`d remove it

Code quote:

/// Input may be attacker-controlled (e.g. an unparsable transaction body received over HTTP), so
/// a missing or non-object `resource_bounds` field must not panic; the transaction is left
/// unchanged and the typed deserialization that follows reports the error.

crates/starknet_api/src/serde_utils.rs line 170 at r1 (raw file):

        raw_transaction.get_mut("resource_bounds").and_then(Value::as_object_mut)
    else {
        return;

Shouldn't it be an error? Before we panicked

Code quote:

 return;

crates/starknet_api/src/serde_utils_test.rs line 202 at r1 (raw file):

    let raw_transaction =
        serde_json::json!({"type": "INVOKE", "version": "0x3", "resource_bounds": 5});
    assert!(deserialize_transaction_json_to_starknet_api_tx(raw_transaction).is_err());

I think you can remove it

Code quote:

    // resource_bounds is not an object.
    let raw_transaction =
        serde_json::json!({"type": "INVOKE", "version": "0x3", "resource_bounds": 5});
    assert!(deserialize_transaction_json_to_starknet_api_tx(raw_transaction).is_err());

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