Repository navigation
fix(jsonrpc): reject numeric request ids that are not long integers - #7084
0xbigapple wants to merge 3 commits into
Conversation
jsonrpc4j parses integral ids beyond the long range with asInt() and fractional ids with asDouble(), so on the public endpoint a request id like 9223372036854775808 was executed and answered with id 0, and distinct ids in a batch collided. Answer an Invalid Request that echoes the original id instead. The Admin endpoint already echoed such ids unchanged; it now rejects them too so both entry points share one rule. The shared request mapper reads floating-point literals as BigDecimal so the echoed id keeps its value instead of a double approximation.
waynercheung
left a comment
waynercheung
left a comment
There was a problem hiding this comment.
Requesting changes on 0496d6b0fe.
The validator change looks right, and the 131 tests in JsonRpcServletTest and AdminJsonRpcRequestTest, plus Checkstyle, pass locally. The blocking findings are grouped in the inline MUST comment.
Performance: with Jackson 2.18.12 on JDK 17, readTree() on a ~4 MB request made of long floating-point literals with trailing zeros took about 1.14 s with the new mapper, versus about 10 ms with the original mapper (median of five runs after warmup).
[SHOULD] Non-blocking follow-up: AdminJsonRpcRequestHandler.handleRequest() already enables USE_BIG_DECIMAL_FOR_FLOATS on the base and only catches IOException, so exponent overflow also escapes its parsing boundary. Please return a structured parse-error response there, or track it separately.
[SHOULD] Document that 1.0 and 1e3 are rejected even though their values are integers, and recommend string IDs to affected clients. Keep the public API compatibility change separate from the Admin API, which is new in this release, so for Admin this is just documented behavior.
| .build(); | ||
| return new ObjectMapper(factory); | ||
| return JsonMapper.builder(factory) | ||
| .configure(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS, true) |
There was a problem hiding this comment.
[MUST] Enabling USE_BIG_DECIMAL_FOR_FLOATS on the shared mapper changes parameter parsing as well as ID handling:
1e2147483648inidorparamsmakesreadTree()throwNumberFormatException. The JSON-RPC parse handler does not catch it;RateLimiterServletlogs an ERROR with a stack trace and leaves an empty HTTP 200 response. The base does not throw this parsing exception.- In batches,
1.0,0.0,1e0and0e0are reserialized as integers. For example,eth_getBlockByNumberwith["latest",1.0]invokes the method withtrue, whereas the same single request and the base batch path return-32602. DisablingSTRIP_TRAILING_BIGDECIMAL_ZEROESdoes not fix1e0. - Long floating-point literals with trailing zeros also add significant parsing overhead; the benchmark is in the review summary.
Please remove the request-wide flag and handle precision for id only. Out-of-range integer IDs already keep their exact values as BigIntegerNode. For rejected fractional IDs, keep echoing the request value; if you choose another policy, state it explicitly. Please add regression tests for exponent overflow and for single/batch parameter parity.
There was a problem hiding this comment.
Thanks for the comment. Confirmed that enabling the flag on the shared mapper causes all three issues, so it is removed in f4772a0; the mapper is back to the base configuration.
On the precision loss: these ids are abnormal requests in the first place, so I would rather not add logic just to echo them exactly. A rejected fractional id is echoed as the parsed double, which is what the endpoint echoed before this change (1e400 already came back as "Infinity"). Nothing gets worse than before: such a request used to be executed with a rounded or truncated id, now it is rejected with -32600. The policy is stated in the description.
Added exponentOverflowId_isRejected and nonBooleanFloatParam_isRejectedInSingleAndBatch as regression tests.
Echo a rejected fractional id as the parsed double.
BigDecimal parsing throws NumberFormatException on it, which escaped the parse boundary of the Admin request handler.
|
@waynercheung Thanks for the comment. Fixed in 29d873b: the Admin handler now catches the Done in the description: the breaking-change note now says |
|
I found no remaining blocking issues. The shared mapper regressions are fixed, and the Admin parse boundary now handles exponent overflow on both HTTP and IPC. [NIT] Add mixed-batch regression coverage for IDs such as 1.0, 1e0, 1e-400, and 1e400, alongside valid requests and notifications. Assert that rejected items are never invoked and valid siblings continue. These paths passed the additional probes. |
What does this PR do?
JsonRpcRequestValidatorrejects numeric request ids that are not integer literals within the signed 64-bit range. Such a request is now answered with:{"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid Request: numeric id must be an integer literal within the signed 64-bit range"},"id":9223372036854775808}An integer id outside the range is echoed exactly; a fractional or exponent id is echoed as the
doubleit is parsed to, as before.Why are these changes required?
jsonrpc4j's
parseId()reads such ids withasInt()orasDouble(), so the call is executed but the response carries a truncated or rounded id that the client cannot match to its request. jsonrpc4j 1.7 is the latest release andmasterstill has the same code; Rejecting is simpler than restoring the id outside jsonrpc4j, common clients send integer counters or string ids, and the JSON-RPC 2.0 specification says numeric ids SHOULD NOT contain fractional parts.This PR has been tested by:
JsonRpcServletTestandAdminJsonRpcRequestTestcover rejection of out-of-range integers and fractional or exponent ids, in single and batch requests, and keepLong.MAX_VALUE,Long.MIN_VALUEand string ids accepted. The new assertions fail on the currentrelease_v4.8.3head.Follow up
None.
Extra details
Breaking change (release-note item), public JSON-RPC API: requests whose numeric
idis not an integer literal within[-9223372036854775808, 9223372036854775807]are now rejected with-32600 Invalid Request. This includes1.0and1e3, whose values are integers but whose literals are not. Clients that need such identifiers should send them as strings.Admin JSON-RPC API (new in this release): the same rule applies; this is documented behavior rather than a compatibility change.