fix(jsonrpc,http): harden RPC/HTTP parameter validation#14
Open
0xbigapple wants to merge 7 commits into
Open
Conversation
Add length/format guards across the JSON-RPC and HTTP address-handling paths so
malformed or oversized parameters are rejected early with a clear -32602 / error
response, instead of triggering O(n^2) work or leaking NPE / Internal errors.
- Commons.decodeFromBase58Check: reject non-34-char input before the O(n^2)
Base58 decode; single funnel covering every HTTP address input.
- JsonFormat.unescapeBytesSelfType: turn an invalid Base58 address (null or
illegal-char) into a clear "invalid address" error instead of a bare
ByteString.copyFrom(null) NPE; covers all merge-path address fields.
- JsonRpcApiUtil.addressCompatibleToByteArray: cap length at ADDRESS_SIZE + 2
(the +2 keeps 0x-prefixed 21-byte addresses valid) before fromHexString.
- JsonRpcApiUtil.parseBlockNumber: tighten MAX_BLOCK_NUM_HEX_LEN 100 -> 20 and
route the (String, Wallet) overload through the single-arg parser so it also
gets the length cap, overflow (longValueExact) and negative checks.
- TronJsonRpcImpl.getStorageAt: bound the storage key and parse it before the
contract lookup; wrap DataWord parsing so a >32-byte key
returns invalid-params instead of an Internal error.
- Hoist hashToByteArray/HASH_REGEX into JsonRpcApiUtil so LogFilterWrapper's
blockHash gets the same 32-byte-hash validation.
📝 WalkthroughWalkthroughCentralizes and tightens input validation: adds Base58 length checks, central hash validation, stricter block/quantity/index parsing, storage key guards, ABI parsing error handling, fee-limit safety, and updates HTTP/JSON-RPC call sites and tests to assert consistent error messages. ChangesInput Validation Enhancement and Consolidation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.java`:
- Around line 424-442: The HASH_REGEX used by hashToByteArray allows non-hex
letters; update the regex constant HASH_REGEX to only permit valid hex digits
and anchor the pattern (e.g. change to "^(0x)?[0-9a-fA-F]{64}$") so
Pattern.matches rejects values with letters outside 0-9/a-f/A-F; keep throwing
JsonRpcInvalidParamsException from hashToByteArray when the pattern fails and
continue using ByteArray.fromHexString for conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f8e5b10-4e07-454f-a5b3-c0cfc3084593
📒 Files selected for processing (9)
chainbase/src/main/java/org/tron/common/utils/Commons.javaframework/src/main/java/org/tron/core/services/http/JsonFormat.javaframework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcApiUtil.javaframework/src/main/java/org/tron/core/services/jsonrpc/TronJsonRpcImpl.javaframework/src/main/java/org/tron/core/services/jsonrpc/filters/LogFilterWrapper.javaframework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.javaframework/src/test/java/org/tron/core/services/http/GetAccountServletTest.javaframework/src/test/java/org/tron/core/services/http/GetRewardServletTest.javaframework/src/test/java/org/tron/core/services/jsonrpc/JsonRpcApiUtilTest.java
0xbigapple
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
0xbigapple
changed the title
…and address
Continue hardening the eth_* JSON-RPC parameter paths so malformed, oversized
or overflowing inputs are rejected with a clear -32602 (or, for an out-of-range
index, a null result) instead of leaking an Internal error, an uncaught
Error/NPE, a DecoderException, or a silently wrong value.
- eth_getTransactionByBlock{Hash,Number}AndIndex: parse the index via a new
length-capped JsonRpcApiUtil.parseTxIndex (malformed / oversized / int-overflow
index -> -32602); in the handler, a negative or out-of-range index
(txIndex < 0 || >= tx count) returns a null result instead of letting
block.getTransactions(-1) throw IndexOutOfBoundsException (-32603).
- buildCreateSmartContractTransaction: a deeply nested ABI overflows the
recursive JsonFormat parser; catch the StackOverflowError narrowly around the
merge and map it to invalid-params ("invalid abi") instead of letting an
Error escape.
- buildCreate/buildTriggerSmartContractTransaction: compute feeLimit via
JsonRpcApiUtil.calcFeeLimit (Math.multiplyExact) so an oversized gas no
longer silently wraps gas*energyFee to a bogus (possibly negative) feeLimit.
- parseQuantityValue: reject a signed ("0x-..") value/gas; QUANTITY is unsigned.
- LogFilter topics: parse via the strict hashToByteArray (regex-validated 32-byte
hash) instead of topicToByteArray, and remove the now-unused topicToByteArray.
- addressToByteArray: cap length before fromHexString and wrap the decode so an
oversized or invalid-hex address filter is invalid-params.
❌ Math Usage Detection ResultsFound forbidden usage of Please review if this usage is intended. Caution Note: You should use |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@cubic-dev-ai review |
@0xbigapple I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…er elements
- Restore topicToByteArray for LogFilter topics, guard with (0x)?[0-9a-fA-F]{63,64}$ so the stripped zero is padded back by ByteArray.fromHexString, while non-hex or wrong-length input still gets a clean -32602.
- LogFilter: validate element types before the (String) cast in the address array and nested topic array loops.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds length/format validation to several JSON-RPC and HTTP address-handling parameters, so
malformed or oversized input is rejected early instead of triggering
O(n²)work or leakingNullPointerException/-32001(json4j default error). Valid input is unaffected.Commons.decodeFromBase58Check— reject input whose length!= 34before theO(n²)Base58.decode; single funnel for every HTTP address input.JsonFormat.unescapeBytesSelfType— an invalid Base58 address (null / illegal char) now returns a clearinvalid addresserror instead of a bareByteString.copyFrom(null)NPE; covers all merge-path address fields.JsonRpcApiUtil.addressCompatibleToByteArray— cap length atADDRESS_SIZE + 2(the+2keeps0x-prefixed 21-byte addresses valid) beforefromHexString.JsonRpcApiUtil.parseBlockNumber— tightenMAX_BLOCK_NUM_HEX_LEN100 → 20, and route the(String, Wallet)overload through the single-arg parser so it also gets the length cap, overflow (longValueExact) and negative checks.TronJsonRpcImpl.getStorageAt— bound the storage key and parse it before the contract lookup; a>32-byte key now returns-32602 invalid storage key valueinstead of-32001 (json4j default error).LogFilterWrapper— validateblockHashvia the sharedJsonRpcApiUtil.hashToByteArray(moved here fromTronJsonRpcImpl).HASH_REGEXaccepts only hex digits[0-9a-fA-F], so a non-hex hash is rejected asinvalid hash valuerather than slipping through tofromHexString.Why are these changes required?
Several unauthenticated API entry points accepted unbounded / malformed input, causing two
classes of problem:
decodeFromBase58Checkfed the whole string into theO(n²)Base58.decode; since the cost grows quadratically with length, a single oversized addresspayload (well within the 4 MB body cap) can tie up a request thread far out of proportion to
its size — a cheap DoS the body cap and rate limiter don't stop. Block-number parsing likewise
had no effective length bound before the
O(n²)BigIntegerconstructor.bare
NullPointerException;eth_getStorageAtsurfaced aRuntimeExceptionas-32001 Data word can't exceed 32 bytes: xxxinstead of-32602 invalid params;This PR has been tested by:
Follow up
Extra details
Representative affected endpoints:
eth_getStorageAthashToByteArray)ADDRESS_SIZE + 2)/wallet/*— all covered by the Base58 length/DoS guard, but they differ on an invalid address:Util.getHexAddressand just return{}(e.g.getcontract,`visible=true) — unchanged;getaccount,visible=true) get the NPE →invalid addressfix;Util.getAddress(request)(e.g.getReward) get null for an invalid address and degrade toreward 0.Summary by cubic
Hardened JSON-RPC and HTTP parameter validation to reject malformed, oversized, overflowing, or null inputs early. Prevents O(n²) work and maps failures to clear -32602 errors; valid input is unchanged.
address/topicsare rejected early.StrictMathWrapper.multiplyExact; gas*fee overflow -> “invalid gas: fee limit overflow”.buildCreateSmartContractTransactioncatches deep-nestingStackOverflowErrorand returns “invalid abi”.Written for commit e5a033a. Summary will update on new commits.
Summary by CodeRabbit
Release Notes