fix(api): harden external input validation - #21
Conversation
Validate Permission_id conversion, addresses, numeric strings, and shielded transfer cardinality before expensive or ambiguous processing. Add deterministic boundary, compatibility, and mirror endpoint regression tests.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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.
3 issues 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/main/java/org/tron/core/Wallet.java">
<violation number="1" location="framework/src/main/java/org/tron/core/Wallet.java:4223">
P2: In getTriggerInputForShieldedTRC20Contract, getBigIntegerFromString(request.getAmount()) is not wrapped in try/catch, unlike the two other callers of getBigIntegerFromString. An amount string longer than 80 characters now lets an undeclared IllegalArgumentException escape instead of the ContractValidateException previously produced by checkBigIntegerRange. Only the outer servlet/RPC catch blocks keep it from failing, and the error type/message changes. Catch the exception here and rethrow as ContractValidateException for consistency with the other callers.</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:446">
P2: The new 64-character limit guards only string-form values (`rawValue instanceof String`). A bare numeric JSON literal with many digits is parsed by Jackson into a numeric node, bypasses the guard, and still undergoes full BigDecimal/BigInteger conversion before the exactness check rejects it. If the stated intent is to bound conversion cost before `BigDecimal`, extend the check to non-string numeric primitives (e.g. reject oversized `Number` values before `getBigDecimal`), and confirm the Jackson number-length limit actually caps this path.</violation>
</file>
<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
| if (trimmedIn.length() == 0) { | ||
| return BigInteger.ZERO; | ||
| } | ||
| if (trimmedIn.length() > 80) { |
There was a problem hiding this comment.
P2: In getTriggerInputForShieldedTRC20Contract, getBigIntegerFromString(request.getAmount()) is not wrapped in try/catch, unlike the two other callers of getBigIntegerFromString. An amount string longer than 80 characters now lets an undeclared IllegalArgumentException escape instead of the ContractValidateException previously produced by checkBigIntegerRange. Only the outer servlet/RPC catch blocks keep it from failing, and the error type/message changes. Catch the exception here and rethrow as ContractValidateException for consistency with the other callers.
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/Wallet.java, line 4223:
<comment>In getTriggerInputForShieldedTRC20Contract, getBigIntegerFromString(request.getAmount()) is not wrapped in try/catch, unlike the two other callers of getBigIntegerFromString. An amount string longer than 80 characters now lets an undeclared IllegalArgumentException escape instead of the ContractValidateException previously produced by checkBigIntegerRange. Only the outer servlet/RPC catch blocks keep it from failing, and the error type/message changes. Catch the exception here and rethrow as ContractValidateException for consistency with the other callers.</comment>
<file context>
@@ -4214,6 +4220,9 @@ private BigInteger getBigIntegerFromString(String in) {
if (trimmedIn.length() == 0) {
return BigInteger.ZERO;
}
+ if (trimmedIn.length() > 80) {
+ throw new IllegalArgumentException("invalid shielded amount");
+ }
</file context>
| int permissionId; | ||
| try { | ||
| Object rawValue = jsonObject.get(PERMISSION_ID); | ||
| if (rawValue instanceof String |
There was a problem hiding this comment.
P2: The new 64-character limit guards only string-form values (rawValue instanceof String). A bare numeric JSON literal with many digits is parsed by Jackson into a numeric node, bypasses the guard, and still undergoes full BigDecimal/BigInteger conversion before the exactness check rejects it. If the stated intent is to bound conversion cost before BigDecimal, extend the check to non-string numeric primitives (e.g. reject oversized Number values before getBigDecimal), and confirm the Jackson number-length limit actually caps this path.
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 446:
<comment>The new 64-character limit guards only string-form values (`rawValue instanceof String`). A bare numeric JSON literal with many digits is parsed by Jackson into a numeric node, bypasses the guard, and still undergoes full BigDecimal/BigInteger conversion before the exactness check rejects it. If the stated intent is to bound conversion cost before `BigDecimal`, extend the check to non-string numeric primitives (e.g. reject oversized `Number` values before `getBigDecimal`), and confirm the Jackson number-length limit actually caps this path.</comment>
<file context>
@@ -433,12 +437,27 @@ public static String getHexString(final String string) {
+ int permissionId;
+ try {
+ Object rawValue = jsonObject.get(PERMISSION_ID);
+ if (rawValue instanceof String
+ && ((String) rawValue).length() > MAX_JSON_INTEGER_VALUE_LENGTH) {
+ throw new InvalidParameterException(INVALID_PERMISSION_ID);
</file context>
| } | ||
|
|
||
| @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>
getAddress returned an error for a blank or absent address, changing the response of getreward and getbrokerage on all three interfaces. Restore the null return, bound the hex branch before decoding, and report every decode failure with a fixed message so the offending character is not echoed. The servlets now handle the address parameter and the service call in separate try blocks, so a failure further in is no longer reported to the caller as a bad address.
The transfer branch got a cardinality guard; the mint branch on the same endpoint still indexed element 0 of a caller-supplied repeated field.
The authoritative bound is checkBigIntegerRange; this only keeps the string short enough to convert cheaply.
1 participant
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:
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
Harden external API input validation to reject malformed addresses and ambiguous numeric fields early while preserving legacy defaults and behavior. Reward/brokerage now reject only malformed addresses; blank or missing addresses keep the prior default response.
org.tron.core.services.http.Util#getAddressvalidates Base58Check and41...hex with length/prefix checks and returns null for blank input; servlets useprocessAddressParamErrorto emit bounded errors (“Invalid address” or “INVALID JSON body”) without echoing caller input.org.tron.core.Wallet#getContractand#getContractInfoshort‑circuit on invalid address bytes.BigDecimalinUtil#getJsonLongValue.Permission_idmust be an exact 32‑bit integer; legacy exact string forms remain; fractions, scientific strings, overflow, explicit null, and overlong strings are rejected. If absent or <= 0, the transaction is unchanged.BigInteger. Require transfer spend/receive counts in 1–2, and require exactly one receive description for mint. Existingnode.allowShieldedTransactionApigate still applies.org.tron.core.services.http.Util,org.tron.core.Wallet, andorg.tron.core.zen.ShieldedTRC20ParametersBuilder; added regression tests cover boundaries and mirrored endpoints across FullNode, Solidity, and PBFT.Written for commit d2e1cdb. Summary will update on new commits.