Skip to content

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

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

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

Conversation

@waynercheung

@waynercheung waynercheung commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Tightens how the JSON-RPC HTTP servlet (JsonRpcServlet) accepts a request body. The FullNode, solidity and PBFT JSON-RPC services all use it (JsonRpcOnSolidityServlet and JsonRpcOnPBFTServlet extend it). Before any request is dispatched, the following bodies now get HTTP 200 and {"jsonrpc":"2.0","error":{"code":-32700,"message":"JSON parse error"},"id":null}:

  • A body containing malformed UTF-8 byte sequences (checked with Guava Utf8.isWellFormed) or a raw NUL (0x00) byte. Jackson accepts overlong encodings, UTF-16 surrogates and code points above U+10FFFF, none of which are valid UTF-8 (RFC 3629). A NUL byte cannot appear in a UTF-8 JSON text that the parser accepts, but Jackson takes one as a sign of UTF-16 or UTF-32 and decodes the body that way. This is the same byte check the admin endpoint already applies.
  • 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.
    These checks apply to single, batch and notification requests. The feature is enabled only on the servlet's own reader, so the shared JsonRpcMapper does not change. The try block around parsing now covers only the parse call.

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.
  • Encoding: JSON exchanged between systems must be UTF-8 (RFC 8259 section 8.1). Previously, invalid UTF-8 was handled inconsistently, depending on the bytes and where they appeared: some overlong forms were normalized and the request ran, some failed at method lookup, and some returned -32700. UTF-16 and UTF-32 bodies were auto-detected and executed. Accepting only UTF-8 gives one rule. go-ethereum does not auto-detect UTF-16 or UTF-32 either, and rejects requests encoded that way with -32700.

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.
      • Other encodings: UTF-16 with a BOM; BOM-less UTF-16LE/BE and UTF-32LE/BE requests with ASCII text, U+4E2D or U+00E9, including bodies whose raw bytes are also well-formed UTF-8; raw NUL bytes inside, after and before the request.
      • Trailing content: object or array followed by another value, garbage, } or ], a notification pair, and 1 2.
      • Still accepted: raw 2-byte, 3-byte and 4-byte UTF-8 parameters (U+07FF, U+4E2D, U+1F600, U+10FFFF), JSON \u escapes including \u0000, a UTF-8 BOM, and trailing space/tab/CR/LF.
    • 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 the UTF-8 check, the NUL check or the trailing-token feature, or moving the UTF-8 check after dispatch, fails the tests.
    • ./gradlew test on dfd2806: the only failures were 7 InetUtilTest cases (DNS resolution timeouts), which fail the same way on the unchanged baseline.
    • 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). On dfd2806, ./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 or UTF-32, with or without a byte order mark auto-detected and accepted (some malformed UTF-32 bodies got an empty HTTP 200) -32700, nothing dispatched
UTF-8 with one value plus trailing whitespace, a UTF-8 BOM, non-ASCII parameters, or the \u0000 escape accepted accepted (unchanged)

Suggested release note: The FullNode, solidity and PBFT JSON-RPC HTTP endpoints now reject, 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. They also accept only UTF-8 request bodies: a body that is not well-formed UTF-8, or that is encoded as UTF-16 or UTF-32, now gets a -32700 parse error. Clients sending valid UTF-8 JSON-RPC requests containing a single JSON value are unaffected.

Notes:

  • A UTF-8 byte order mark is still accepted, as before (RFC 8259 allows a parser to ignore it). go-ethereum rejects it.
  • The HTTP status code stays 200, as for every other -32700 in this servlet.

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.
writeJsonRpcError(resp, JsonRpcError.PARSE_ERROR, "JSON parse error", null, false);
return;
}
} catch (JsonProcessingException e) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SHOULD] REQUEST_READER.readTree(body) can also throw a plain IOException that is not a JsonProcessingException, for example java.io.CharConversionException from Jackson's character decoding. It skips the -32700 mapping here and leaves doPostInternal, so the client gets an empty HTTP 200 without a JSON-RPC envelope. Because the trailing-token check now reads to the end of the body, a few bodies that used to be executed take this path instead of returning -32700. The body is already in memory, so any IOException from readTree(byte[]) is a decoding failure, not a transport failure. Could this also catch IOException and return -32700, as AdminJsonRpcRequestHandler does (line 47)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, confirmed and fixed in 0a577bb. Over embedded Jetty, a BOM-less UTF-32 request followed by 1 or 3 extra bytes ran on the base and returned an empty HTTP 200 on this branch, because the trailing check now reads to the end of the body. An invalid code unit can also hit this path on the base when the decoder's read-ahead reaches it, so not every case is new, but all of them should be -32700.

The try block now covers only REQUEST_READER.readTree(body) and catches IOException. The missing-node check and the error writes are outside it, so a failure while writing the response is not mistaken for a parse error. The new test undecodableUtf32Body_returnsParseError fails if the catch is narrowed back to JsonProcessingException in that commit. With the NUL check below in place, these UTF-32 cases are rejected before parsing; the additional handling of non-JsonProcessingException IOExceptions remains a safeguard.

// Transport IOException from readBody propagates as HTTP 500 (genuine IO failure).
byte[] body = readBody(req.getInputStream());
// Jackson accepts overlong and surrogate UTF-8 sequences, so check the bytes first.
if (!Utf8.isWellFormed(body)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SHOULD] Could this also reject bodies that contain a 0x00 byte, as AdminJsonRpcRequestHandler already does next to the same UTF-8 check (!Utf8.isWellFormed(body) || Bytes.contains(body, (byte) 0), line 39; its comment explains why)?

I know the description lists NUL bytes and encoding detection as out of scope, so this is only to make the trade-off explicit:

  • A 0x00 byte can never appear in a UTF-8 JSON text that Jackson accepts: unescaped U+0000 is not allowed anywhere (RFC 8259), and with this PR a trailing one is rejected too. So the extra condition cannot reject a valid UTF-8 request; \u0000 escapes still work.
  • It only changes bodies that Jackson auto-detects as UTF-16 or UTF-32. Jackson picks those decoders only for a UTF-16/32 BOM, which this PR already rejects, or for a 0x00 in the first four bytes.
  • Today such a BOM-less body is accepted or rejected depending on its characters: a UTF-16 request with U+4E2D runs, while the same request with U+00E9 gets -32700, as the two utf16* tests show. BOM-less UTF-32 is accepted the same way. Before this PR all of them ran.

With the NUL check the rule becomes simply "UTF-8 only" (RFC 8259 §8.1), matching the admin endpoint, and Jackson's UTF-16/UTF-32 decoders are no longer reached at all. The cost is one more linear scan of the body. If you'd rather keep the current behavior, could the release note say that BOM-less UTF-16/UTF-32 bodies may still be decoded as such, and the table mention UTF-32?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, done in bf6d23a: the check is now !Utf8.isWellFormed(body) || Bytes.contains(body, (byte) 0), the same byte check as the admin endpoint, and the rule is simply UTF-8 only (a UTF-8 BOM is still accepted). The UTF-16 characterization test now asserts that BOM-less UTF-16LE/BE and UTF-32LE/BE bodies, including the U+4E2D one, get -32700; there are new tests for raw NUL bytes (rejected) and the \u0000 escape (still dispatched); the UTF-16 trailing-content test is gone since those bodies no longer reach the parser. For reference, go-ethereum does not auto-detect UTF-16 or UTF-32 either, and rejects requests encoded that way with -32700. The description and the release note are updated.

public void wellFormedNonAsciiInput_isDispatched() throws Exception {
TronJsonRpc rpc = useRealRpcServer();
when(rpc.web3Sha3(any())).thenReturn("0x1");
String chinese = "中文";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[NIT] Two small things:

  • wellFormedNonAsciiInput_isDispatched covers raw 3-byte and 4-byte UTF-8, but its U+0080 to U+07FF case is the JSON escape for U+00E9, which is plain ASCII on the wire. So no positive test sends a raw 2-byte sequence, and the negative test only has 2-byte overlongs. Consider adding one, e.g. new String(Character.toChars(0xE9)). Optionally, new String(Character.toChars(0x10FFFF)) instead of the raw literal on line 141 would make that case visible in the diff; plain Unicode escapes would trip AvoidEscapedUnicodeCharacters.
  • The comment on JsonRpcServlet.java:127 could also mention code points above U+10FFFF, as the description does.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. 1d841d4 adds a raw two-byte parameter. I used U+07FF rather than U+00E9 because the JSON escape case already decodes to U+00E9 and the per-iteration verify would count two calls. The 4-byte parameters are now built with Character.toChars. The comment at JsonRpcServlet.java:127 now mentions code points above U+10FFFF (bf6d23a).

@halibobo1205 halibobo1205 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[NIT] The suggested release note says "The JSON-RPC HTTP endpoint", but the change also applies to the solidity and PBFT JSON-RPC endpoints (JsonRpcOnSolidityServlet and JsonRpcOnPBFTServlet extend JsonRpcServlet and call super.doPost), as the first paragraph of the description says. Since the note is the text that usually gets copied into release notes, consider naming all three.

Catch every IOException from parsing the request body, not only
JsonProcessingException, and limit the try block to the parse call.
Jackson's UTF-32 decoder reports a malformed or truncated code unit as
a CharConversionException, which is not a JsonProcessingException, so
it escaped the servlet and the client got an empty response.

Such a body now gets a -32700 parse error with a null id, and nothing
is dispatched.
Add a raw two-byte UTF-8 parameter (U+07FF) to the well-formed input
test; the existing two-byte case was a JSON escape and so plain ASCII
on the wire. Build the four-byte parameters from code points so that
they are visible in the source.
Also reject a request body that contains a NUL byte, as the admin
endpoint already does. A NUL byte cannot appear in a UTF-8 JSON text
that the parser accepts, but Jackson takes one in the first four bytes
as a sign of UTF-16 or UTF-32 and decodes the body that way.

Compatibility: UTF-8 request bodies, with or without a byte order mark,
are unchanged, and the JSON escape \u0000 still works. A UTF-16 or
UTF-32 body without a byte order mark used to be decoded as such when
its raw bytes were also well-formed UTF-8; it now gets a -32700 parse
error with a null id.
@waynercheung

Copy link
Copy Markdown
Collaborator Author

Done, the suggested release note in the description now names the FullNode, solidity and PBFT JSON-RPC HTTP endpoints.

writeJsonRpcError(resp, JsonRpcError.PARSE_ERROR, "JSON parse error", null, false);
}
rootNode = REQUEST_READER.readTree(body);
} catch (IOException e) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[DISCUSS] Why is this widened to IOException? With the byte check in front, I could not find a body that reaches this catch with anything other than JsonProcessingException: the CharConversionException cases all contain 0x00 and are rejected earlier, and the tests stay green with the catch narrowed back.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, narrowed back to JsonProcessingException in c2ea817; the try still covers only the parse call. The wider catch was needed while BOM-less UTF-32 bodies were still auto-detected: in 0a577bb, narrowing it makes the UTF-32 test fail. Since bf6d23a rejects NUL bytes, those bodies no longer reach Jackson's UTF-32 decoder, and I could not find a body that passes the byte check and fails with anything other than a JsonProcessingException either.

concat(request, new byte[] {'\n'}),
concat(request, new byte[] {0, 0, ' '}),
concat(request, padding, new byte[] {0, 0x11, 0, 0}));
for (byte[] body : bodies) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SHOULD] All three bodies start with the request encoded as UTF-32BE (00 00 00 7B ...), so they contain 0x00 and are rejected by the byte check before reaching Jackson's UTF-32 decoder. The comment about CharConversionException no longer matches what the test exercises, and it now covers the same branch as utf16AndUtf32WithoutByteOrderMark_areRejected.

Two more leftovers of the same kind:

  • utf16BodyThatIsNotWellFormedUtf8_isRejected: all four bodies contain 0x00, so the test still passes with Utf8.isWellFormed removed (only malformedUtf8_isRejectedBeforeDispatch and the Jetty counterpart fail).
  • The web3ClientVersion stub in malformedUtf8_isRejectedBeforeDispatch and the web3Sha3 stub in utf16BodyThatIsNotWellFormedUtf8_isRejected are never hit; both tests pass without them.

Suggest removing or merging these cases.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done in dfd2806: undecodableUtf32Body_returnsParseError is removed, the UTF-16 bodies with a BOM and the U+00E9 request from utf16BodyThatIsNotWellFormedUtf8_isRejected are merged into utf16AndUtf32Bodies_areRejected, and the two unused stubs are gone; the never() checks stay. Removing Utf8.isWellFormed still fails the two malformed UTF-8 tests, and removing the NUL check fails the UTF-16/UTF-32 test. The description is updated.

}
}

private static byte[] concat(byte[]... parts) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[NIT] Guava's Bytes.concat(byte[]...) does the same thing, and JsonRpcServlet already imports Bytes. Replacing both helper bodies with it keeps the two test classes green.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done in dfd2806, both test classes now use a static import of Bytes.concat.

Request bodies that are not well-formed UTF-8 or that contain a NUL byte
are rejected before parsing, so the parser only sees UTF-8 input held in
memory and reports every failure as a JsonProcessingException. Go back to
catching that type. The try block still covers only the parse call.

No behavior change.
- Remove the UTF-32 decoding test; its bodies contain NUL bytes and are
  rejected before parsing, like the other UTF-16 and UTF-32 cases.
- Merge the UTF-16 test with a byte order mark into the UTF-16/UTF-32
  rejection test and add a U+00E9 request to it.
- Drop stubs for methods that the rejection tests never reach.
- Use Guava Bytes.concat instead of local helpers.
@waynercheung
waynercheung force-pushed the fix/jsonrpc-request-body-validation branch from d9efdc5 to dfd2806 Compare November 12, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants