Skip to content

fix(jsonrpc): reject invalid UTF-8 and trailing request content - #7103

Open
waynercheung wants to merge 2 commits into
tronprotocol:release_v4.8.3from
waynercheung:fix/jsonrpc-request-body-validation
Open

waynercheung wants to merge 2 commits into
tronprotocol:release_v4.8.3from
waynercheung:fix/jsonrpc-request-body-validation

Conversation

@waynercheung

Copy link
Copy Markdown
Collaborator

What does this PR do?

Tightens how the JSON-RPC HTTP servlet (JsonRpcServlet, also used by the solidity and PBFT JSON-RPC services) accepts a request body. Before any request is dispatched, it now rejects two kinds of body with HTTP 200 and {"jsonrpc":"2.0","error":{"code":-32700,"message":"JSON parse error"},"id":null}:

  • A body that is not well-formed UTF-8, checked with Guava Utf8.isWellFormed before parsing. Jackson accepts overlong encodings, UTF-16 surrogates and code points above U+10FFFF, none of which are valid UTF-8 (RFC 3629).
  • A body that has anything other than whitespace after the first JSON value. It is parsed with an ObjectReader that enables DeserializationFeature.FAIL_ON_TRAILING_TOKENS.

Both checks apply to single, batch and notification requests. The feature is enabled only on the servlet's own reader, so the shared JsonRpcMapper and the admin endpoints do not change. The admin endpoints already apply both checks.

The PR has two commits, one per check, and each commit has its own tests.

Why are these changes required?

  • Trailing content: a JSON text is a single value (RFC 8259). Previously the node answered the first value and silently dropped the rest, so a client that sent {...} {...} got one response and no error. go-ethereum (v1.17.6 and later) and Besu both reject such a body with -32700.
  • Invalid UTF-8: such bytes were handled inconsistently, depending on the bytes and where they appeared in the body. Some overlong forms were normalized and the request ran, some failed at method lookup, and some returned -32700. Rejecting all of them up front gives one predictable result.

This PR has been tested by:

  • Unit Tests
    • JsonRpcServletTest with a real JsonRpcServer. Each rejected body asserts the full -32700 envelope with a null id, and asserts that neither the server nor the service method was called.
      • Invalid UTF-8: 2-byte and 3-byte overlong, surrogate, and above U+10FFFF, each in single, batch and notification form. Also BOM-prefixed UTF-16, and UTF-16 requests containing U+00E9 whose raw bytes are not well-formed UTF-8.
      • Trailing content: object or array followed by another value, garbage, } or ], a notification pair, and 1 2.
      • Still accepted: non-ASCII parameters (3-byte and 4-byte UTF-8, U+10FFFF), JSON \u escapes, a UTF-8 BOM, trailing space/tab/CR/LF, and BOM-less UTF-16 whose raw bytes are also well-formed UTF-8 (ASCII text, and U+4E2D).
    • JsonRpcServletJettyTest: posts raw bytes through embedded Jetty and the real filter chain, one invalid UTF-8 body and one trailing-content body.
    • Mutation check: removing either check, or moving the UTF-8 check after dispatch, fails the tests.
    • ./gradlew test: the only failures were 7 in InetUtilTest (DNS resolution timeouts), and they fail the same way without this change.
    • Run on macOS ARM64 with JDK 17. The build and test commands used -x :framework:generateGitProperties, because that task fails in a linked git worktree (JGit cannot resolve the repository). ./gradlew clean build -x test, the Checkstyle tasks from pr-check.yml and check_math_usage.sh passed.
  • Manual Testing
    • Not performed beyond the embedded Jetty tests above.

Follow up

None.

Extra details

Behavior changes (release note)

Request body Before After
{...} {...}, {...} xyz, {...}}, [...] {...} first value answered, rest dropped -32700, nothing dispatched
1 2, "x" { -32600 -32700
bytes that are not well-formed UTF-8 varied with the bytes and their position -32700, nothing dispatched
UTF-16/UTF-32 bodies whose raw bytes are not well-formed UTF-8, including BOM-prefixed bodies could be accepted through encoding auto-detection -32700, nothing dispatched
BOM-less UTF-16 whose raw bytes are also well-formed UTF-8 (e.g. ASCII text) accepted accepted (unchanged)
one value plus trailing whitespace, UTF-8 BOM, non-ASCII UTF-8 parameters accepted accepted (unchanged)

Suggested release note: The JSON-RPC HTTP endpoint now rejects, with a -32700 parse error, a request body that contains anything other than whitespace after the first JSON value; clients that send several requests in one body must send a batch array instead. It also rejects, with a -32700 parse error, request bodies that are not well-formed UTF-8.

Out of scope: the HTTP status code (stays 200, as for every other -32700), NUL bytes, and Jackson's encoding detection for bodies that are also well-formed UTF-8.

Check that the request body is well-formed UTF-8 before parsing it.
Jackson accepts overlong encodings, UTF-16 surrogates and code points
above U+10FFFF, none of which are valid UTF-8 (RFC 3629).

A body that is not well-formed UTF-8 now gets a -32700 parse error
with a null id, and nothing is dispatched. This applies to single,
batch and notification requests alike.

Compatibility: well-formed UTF-8, including a byte order mark and
non-ASCII text in parameters, is unchanged. Encoding detection is kept
only for bodies that are also well-formed UTF-8, so an ASCII body sent
as UTF-16 without a byte order mark is still accepted. A UTF-16 or
UTF-32 body whose raw bytes are not well-formed UTF-8, such as one that
starts with a byte order mark, now gets -32700.
Parse the request body with FAIL_ON_TRAILING_TOKENS so that it must
hold exactly one JSON value. Previously the node answered the first
value and silently dropped anything after it, so a client that sent
two requests received one response with no error.

Anything other than whitespace after the first value now gets a -32700
parse error with a null id, and nothing is dispatched. Trailing
whitespace is still accepted. Only the HTTP JSON-RPC servlet is
changed; the shared mapper is not, and the admin endpoints already
reject trailing content.

BREAKING CHANGE: a body such as `{...} {...}` or `{...}}` used to
return the result of the first request and now returns -32700.
Clients that send several requests in one body must send them as a
batch array instead.
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.

1 participant