fix(api): harden external input validation - #21
Open
0xbigapple wants to merge 7 commits into
Open
0xbigapple wants to merge 7 commits into
0xbigapple wants to merge 7 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![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 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/test/java/org/tron/core/services/interfaceOnSolidity/http/RewardBrokerageAddressValidationTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/services/interfaceOnSolidity/http/RewardBrokerageAddressValidationTest.java:42">
P3: The Solidity and PBFT test files duplicate the entire harness (reflection-backed wallet factory, request builder, inject helper, and the reward/brokerage rejection test) differing only in the wallet type and injected field name. Extract a shared base class or parameterized harness to avoid maintaining the same assertions in two places.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| @Test | ||
| public void rewardAndBrokerageMirrorsRejectMalformedAddresses() throws Exception { |
There was a problem hiding this comment.
P3: The Solidity and PBFT test files duplicate the entire harness (reflection-backed wallet factory, request builder, inject helper, and the reward/brokerage rejection test) differing only in the wallet type and injected field name. Extract a shared base class or parameterized harness to avoid maintaining the same assertions in two places.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/services/interfaceOnSolidity/http/RewardBrokerageAddressValidationTest.java, line 42:
<comment>The Solidity and PBFT test files duplicate the entire harness (reflection-backed wallet factory, request builder, inject helper, and the reward/brokerage rejection test) differing only in the wallet type and injected field name. Extract a shared base class or parameterized harness to avoid maintaining the same assertions in two places.</comment>
<file context>
@@ -0,0 +1,57 @@
+ }
+
+ @Test
+ public void rewardAndBrokerageMirrorsRejectMalformedAddresses() throws Exception {
+ GetRewardOnSolidityServlet reward = new GetRewardOnSolidityServlet();
+ inject(reward, "walletOnSolidity", walletOnSolidity());
</file context>
There was a problem hiding this comment.
2 issues found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java">
<violation number="1" location="framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java:71">
P3: When a test address contains JSON-significant characters, `jsonRequest` creates malformed JSON instead of encoding the address as a JSON string. Escape the value with the project's JSON serializer (or otherwise quote it correctly) so this shared contract helper exercises the endpoint with the intended address.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/services/http/Util.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/http/Util.java:705">
P2: For multibyte UTF-8 bodies, this check counts characters instead of request bytes and buffers well beyond `httpMaxMessageSize` before rejecting. Count bytes from the input stream while reading so the early bound matches the configured limit.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| new InputStreamReader(request.getInputStream()))) { | ||
| int read; | ||
| while ((read = reader.read(buffer)) != -1) { | ||
| if (sb.length() + read > limit) { |
There was a problem hiding this comment.
P2: For multibyte UTF-8 bodies, this check counts characters instead of request bytes and buffers well beyond httpMaxMessageSize before rejecting. Count bytes from the input stream while reading so the early bound matches the configured limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/Util.java, line 705:
<comment>For multibyte UTF-8 bodies, this check counts characters instead of request bytes and buffers well beyond `httpMaxMessageSize` before rejecting. Count bytes from the input stream while reading so the early bound matches the configured limit.</comment>
<file context>
@@ -673,14 +684,33 @@ private static String checkGetParam(HttpServletRequest request, String key) thro
+ new InputStreamReader(request.getInputStream()))) {
+ int read;
+ while ((read = reader.read(buffer)) != -1) {
+ if (sb.length() + read > limit) {
+ throw new Exception("body size is too big, the limit is " + limit);
+ }
</file context>
|
|
||
| protected static MockHttpServletRequest jsonRequest(String address, String contentType) { | ||
| MockHttpServletRequest request = postRequest(contentType); | ||
| String json = address == null ? "{}" : "{\"address\":\"" + address + "\"}"; |
There was a problem hiding this comment.
P3: When a test address contains JSON-significant characters, jsonRequest creates malformed JSON instead of encoding the address as a JSON string. Escape the value with the project's JSON serializer (or otherwise quote it correctly) so this shared contract helper exercises the endpoint with the intended address.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/services/http/AddressQueryServletTestBase.java, line 71:
<comment>When a test address contains JSON-significant characters, `jsonRequest` creates malformed JSON instead of encoding the address as a JSON string. Escape the value with the project's JSON serializer (or otherwise quote it correctly) so this shared contract helper exercises the endpoint with the intended address.</comment>
<file context>
@@ -0,0 +1,196 @@
+
+ protected static MockHttpServletRequest jsonRequest(String address, String contentType) {
+ MockHttpServletRequest request = postRequest(contentType);
+ String json = address == null ? "{}" : "{\"address\":\"" + address + "\"}";
+ request.setContent(json.getBytes(UTF_8));
+ return request;
</file context>
0xbigapple
force-pushed
the
fix/api-input-hardening
branch
4 times, most recently
from
August 25, 2026 13:56
2c3bb5c to
248cbe0
Compare
There was a problem hiding this comment.
4 issues found across 40 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/core/services/http/JsonFormat.java">
<violation number="1" location="framework/src/main/java/org/tron/core/services/http/JsonFormat.java:864">
P3: `escapeBytesSelfType` hex-renders the entire oversized ByteString (2 hex chars per byte), so a 64KB attacker-controlled address field is echoed as a ~128KB hex string in visible-mode responses — larger than the base58 rendering it replaces and contrary to the PR's stated goal of avoiding echoing oversized attacker-controlled values. The log-side helper introduced in the same change truncates to a 4-byte head; this response path does not.</violation>
<violation number="2" location="framework/src/main/java/org/tron/core/services/http/JsonFormat.java:1708">
P2: This guard still accepts non-empty 1–20-byte hex values as addresses, while the visible Base58 path rejects any length other than 21 bytes. Reject non-empty lengths other than `Constant.TRON_ADDRESS_SIZE` here, preserving empty optional address fields.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/db/Base58ValidationErrorTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/db/Base58ValidationErrorTest.java:47">
P3: Both tests assert only the `,len=16384)` tail of the invalid-address render, so they pass even if the new `invalid-address(<hex-head>…)` prefix is lost. Pin the full prefix to lock in the PR's visible-hex rendering contract.
Replace the contains check in both tests with an assertion on the full prefix, e.g. `contains("invalid-address(00000000…,len=16384)")`.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/WalletMockTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/WalletMockTest.java:910">
P3: This transfer test passes with a completely empty ReceiveDescription (zero-length commitment, value, epk, zkproof, c_enc, c_out), so it only proves the new count check (1-2 receives) succeeds. The PR claims unusable inputs are rejected with ContractValidateException, but a fully empty receive description still produces a trigger input here. Either use a populated ReceiveDescription or add assertions that empty/invalid receive descriptions are rejected, so the hardened validation path is actually exercised.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| if (!selfType) { | ||
| result = unescapeBytes(escaped); | ||
| if (HttpSelfFormatFieldName.isAddressFormat(fieldName) | ||
| && result.size() > Constant.TRON_ADDRESS_SIZE) { |
There was a problem hiding this comment.
P2: This guard still accepts non-empty 1–20-byte hex values as addresses, while the visible Base58 path rejects any length other than 21 bytes. Reject non-empty lengths other than Constant.TRON_ADDRESS_SIZE here, preserving empty optional address fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/JsonFormat.java, line 1708:
<comment>This guard still accepts non-empty 1–20-byte hex values as addresses, while the visible Base58 path rejects any length other than 21 bytes. Reject non-empty lengths other than `Constant.TRON_ADDRESS_SIZE` here, preserving empty optional address fields.</comment>
<file context>
@@ -1701,6 +1704,10 @@ public ByteString consumeByteString(final String fieldName, boolean selfType)
if (!selfType) {
result = unescapeBytes(escaped);
+ if (HttpSelfFormatFieldName.isAddressFormat(fieldName)
+ && result.size() > Constant.TRON_ADDRESS_SIZE) {
+ throw new InvalidEscapeSequence("invalid address for field: " + fieldName);
+ }
</file context>
Suggested change
| && result.size() > Constant.TRON_ADDRESS_SIZE) { | |
| && result.size() != 0 | |
| && result.size() != Constant.TRON_ADDRESS_SIZE) { |
| ContractValidateException failure = assertThrows(ContractValidateException.class, | ||
| trace::checkIsConstant); | ||
| assertTrue(failure.getMessage(), | ||
| failure.getMessage().contains(",len=16384)")); |
There was a problem hiding this comment.
P3: Both tests assert only the ,len=16384) tail of the invalid-address render, so they pass even if the new invalid-address(<hex-head>…) prefix is lost. Pin the full prefix to lock in the PR's visible-hex rendering contract.
Replace the contains check in both tests with an assertion on the full prefix, e.g. contains("invalid-address(00000000…,len=16384)").
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/db/Base58ValidationErrorTest.java, line 47:
<comment>Both tests assert only the `,len=16384)` tail of the invalid-address render, so they pass even if the new `invalid-address(<hex-head>…)` prefix is lost. Pin the full prefix to lock in the PR's visible-hex rendering contract.
Replace the contains check in both tests with an assertion on the full prefix, e.g. `contains("invalid-address(00000000…,len=16384)")`.</comment>
<file context>
@@ -0,0 +1,74 @@
+ ContractValidateException failure = assertThrows(ContractValidateException.class,
+ trace::checkIsConstant);
+ assertTrue(failure.getMessage(),
+ failure.getMessage().contains(",len=16384)"));
+ when(properties.getAllowTvmConstantinople()).thenReturn(1L);
+ trace.checkIsConstant();
</file context>
| GrpcAPI.ShieldedTRC20Parameters shieldedTRC20Parameters = | ||
| GrpcAPI.ShieldedTRC20Parameters.newBuilder() | ||
| .addSpendDescription(spendDescription) | ||
| .addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance()) |
There was a problem hiding this comment.
P3: This transfer test passes with a completely empty ReceiveDescription (zero-length commitment, value, epk, zkproof, c_enc, c_out), so it only proves the new count check (1-2 receives) succeeds. The PR claims unusable inputs are rejected with ContractValidateException, but a fully empty receive description still produces a trigger input here. Either use a populated ReceiveDescription or add assertions that empty/invalid receive descriptions are rejected, so the hardened validation path is actually exercised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/core/WalletMockTest.java, line 910:
<comment>This transfer test passes with a completely empty ReceiveDescription (zero-length commitment, value, epk, zkproof, c_enc, c_out), so it only proves the new count check (1-2 receives) succeeds. The PR claims unusable inputs are rejected with ContractValidateException, but a fully empty receive description still produces a trigger input here. Either use a populated ReceiveDescription or add assertions that empty/invalid receive descriptions are rejected, so the hardened validation path is actually exercised.</comment>
<file context>
@@ -907,6 +907,7 @@ public void testGetTriggerInputForShieldedTRC20Contract1()
GrpcAPI.ShieldedTRC20Parameters shieldedTRC20Parameters =
GrpcAPI.ShieldedTRC20Parameters.newBuilder()
.addSpendDescription(spendDescription)
+ .addReceiveDescription(ShieldContract.ReceiveDescription.getDefaultInstance())
.setParameterType("transfer")
.build();
</file context>
| static String escapeBytesSelfType(ByteString input, final String fieldName) { | ||
| //Address | ||
| if (HttpSelfFormatFieldName.isAddressFormat(fieldName)) { | ||
| if (input.size() > Constant.TRON_ADDRESS_SIZE) { |
There was a problem hiding this comment.
P3: escapeBytesSelfType hex-renders the entire oversized ByteString (2 hex chars per byte), so a 64KB attacker-controlled address field is echoed as a ~128KB hex string in visible-mode responses — larger than the base58 rendering it replaces and contrary to the PR's stated goal of avoiding echoing oversized attacker-controlled values. The log-side helper introduced in the same change truncates to a 4-byte head; this response path does not.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/services/http/JsonFormat.java, line 864:
<comment>`escapeBytesSelfType` hex-renders the entire oversized ByteString (2 hex chars per byte), so a 64KB attacker-controlled address field is echoed as a ~128KB hex string in visible-mode responses — larger than the base58 rendering it replaces and contrary to the PR's stated goal of avoiding echoing oversized attacker-controlled values. The log-side helper introduced in the same change truncates to a 4-byte head; this response path does not.</comment>
<file context>
@@ -861,6 +861,9 @@ static String escapeBytes(ByteString input, final String fieldName, boolean self
static String escapeBytesSelfType(ByteString input, final String fieldName) {
//Address
if (HttpSelfFormatFieldName.isAddressFormat(fieldName)) {
+ if (input.size() > Constant.TRON_ADDRESS_SIZE) {
+ return ByteArray.toHexString(input.toByteArray());
+ }
</file context>
getContract and getContractInfo answer null for an address that is not well formed, before the account store is read.
A string or number longer than 64 characters is rejected before BigDecimal sees it, and Permission_id must convert identically through both coercion paths, so a value one path silently reinterprets no longer gets through.
0xbigapple
force-pushed
the
fix/api-input-hardening
branch
from
September 29, 2026 09:04
a6de2cc to
caa932e
Compare
getAddress reports every way a request can fail to name an address as one fixed message, and the reward and brokerage servlets write it, so the fullnode, solidity and PBFT endpoints no longer differ on a given malformed request. An absent address is one of those ways: it used to be answered with the service default.
Mint takes exactly one receive description; transfer takes one or two of each and, without an ask, a matching spend authority signature count. The counts are checked before the merge loops, because ByteUtil.merge copies the accumulated buffer on every iteration and checking afterwards would make an unbounded list quadratic.
… failure An amount string past 80 characters and a trigger input the builder rejects are answered as a ContractValidateException naming the argument, instead of escaping the wallet as an IllegalArgumentException.
JsonFormat renders address fields over 21 bytes as hex instead of Base58 (visible=true). The hello handshake, PBFT SR list, trigger constant check and getContract logs render addresses via WalletUtil.getAddressString, truncating oversized input instead of full Base58Check encoding.
The response sanitizer added in the release only forwards a whitelist of exception messages and turns the rest into "internal server error". The bounded json-number validation raises InvalidParameterException with a client-safe, fixed message (e.g. the id length limit), so add it to the whitelist; other InvalidParameterException messages in the http layer are fixed texts too.
0xbigapple
force-pushed
the
fix/api-input-hardening
branch
from
September 29, 2026 11:18
caa932e to
77572d3
Compare
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?
This PR hardens externally reachable API inputs before expensive or ambiguous processing:
BigDecimalconversionPermission_idexactly, rejects strings longer than 64 characters, and preserves existing supported numeric formsBigIntegerconversionWhy are these changes required?
Malformed inputs could previously trigger disproportionate CPU work, silently truncate or wrap numeric values, or return plausible results for structurally invalid addresses.
These changes reject invalid inputs before expensive conversion, encoding, storage access, or repeated buffer merging.
Compatibility notes:
getRewardandgetBrokerageno longer substitute a default for an address they never received.Util.getAddressnow either returns a valid address or throws, so a request that carries noaddressat all is rejected like any other unusable one:GET /wallet/getRewardwithout the parameter answered{"reward": 0}and now answers{"Error":"INVALID address"}. The same applies togetBrokerageand to both endpoints on/walletsolidityand the PBFT mirror, six in total. A caller that relied on the zero default has to send an address.Permission_idvalues with fractions, integer overflow, explicitnull, or string representations longer than 64 characters are rejectednode.allowShieldedTransactionApiflag, which is disabled by defaultThis PR has been tested by:
Follow up
Extra details
Summary by cubic
Hardens externally reachable API input validation so malformed requests are rejected before expensive conversion, store access, or Base58 encoding, and all HTTP endpoints answer consistently.
getReward/getBrokerage(and their/walletsolidity/PBFT mirrors) no longer default a missing address; they now require a valid one and return{"Error":"INVALID address"}instead of 0/20.Bug Fixes
Util.getAddressvalidates Base58Check and41...hex and returns fixed "INVALID address"/"INVALID JSON body" errors without echoing input; request bodies stay bounded by Jetty'sSizeLimitHandler.getContract/getContractInforeject malformed addresses before any store access; JSON numbers are capped at 64 characters, andPermission_idmust be an exact 32-bit int and match across coercions.InvalidParameterExceptionwith fixed, client-safe messages is whitelisted by the error sanitizer instead of becoming "internal server error".visible=true, and log, exception, and metric messages truncate oversized addresses viaWalletUtil.getAddressString.ContractValidateException. Thenode.allowShieldedTransactionApigate is unchanged.Migration
/wallet/getRewardand/wallet/getBrokerage(and their/walletsolidity/PBFT mirrors) must send a validaddress; the previous zero/20 defaults are removed.Written for commit 77572d3. Summary will update on new commits.