Repository navigation
fix(api): harden external input validation - #7023
0xbigapple wants to merge 17 commits into
Conversation
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.
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 and transfer takes one or two of each. 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, which Base58Check-encodes only 20- and 21-byte input and renders anything else as hex of at most 21 bytes.
The response sanitizer forwards only a whitelist of exception types and turns the rest into "internal server error", which hid the fixed texts of HTTP parameter validation. Those checks now throw InvalidHttpParameterException, which the sanitizer forwards; java.security.InvalidParameterException stays off the whitelist because the JDK and libraries throw it too. The checks used only by triggersmartcontract, triggerconstantcontract and estimateenergy keep InvalidParameterException, since those servlets format their own errors.
- drop the address guard from contract queries; the log line bounds the address itself - report an unusable address as InvalidHttpParameterException through processError - accept Permission_id only as a JSON int or a decimal string - keep the cause of a rejected Permission_id
Unlike transfer, mint has no merge loop, so an oversized receive list costs no quadratic time and its count check is left out of this change. Also keep the cause of the invalid amount error, and record only log events from the test thread in Base58MessageLoggingTest.
|
[SHOULD] We should also validate receive-description counts for mint and burn in this PR. For mint, the entry point can reach For burn, the entry point already checks the spend/signature counts and burn-record fields, but does not limit receive descriptions. The builder encodes only the first entry when the list is non-empty. Please allow zero or one receive description and reject additional entries. Add public-entry-point tests for these boundaries, using an otherwise valid burn record so the tests reach the count check. Please also document that extra receive descriptions will now be rejected rather than ignored. |
|
[SHOULD] Confirmed on d51dd27: mint with amount="1" and no receive descriptions throws IndexOutOfBoundsException, bypassing the new validation-error handler. Adding a second receive description to either mint or burn produces identical trigger input; the extra entry is silently discarded. Please address the count checks requested above and add public-entry-point regression tests. I reproduced the burn case with a v2 record whose marker and nonce pass the existing checks. |
waynercheung
left a comment
waynercheung
left a comment
There was a problem hiding this comment.
Reviewed head d51dd27 against base a975a3d. Local validation passed 162 repository tests and four additional review probes. The transfer count checks run before the merge loops, and populated transfer serialization matches the base byte-for-byte for all four input/output count combinations and both signature paths.
[SHOULD] Please address the existing mint/burn receive-count validation comment and strengthen the compatibility vectors as noted inline; I'll re-review once these are addressed. The mint/burn behavior is a pre-existing validation gap; I did not identify a newly introduced consensus or fund-safety defect, or a hard-fork requirement.
[DISCUSS] The Compatibility section explicitly documents that an absent address on the six HTTP getReward/getBrokerage endpoints now returns INVALID address instead of the previous zero response. Please confirm that this behavior change is accepted for release_v4.8.3 and include it in the release notes.
[QUESTION] The gRPC getRewardInfo/getBrokerageInfo paths still pass the supplied bytes directly to the service/store without equivalent address validation. Is aligning their validation planned as a follow-up?
Mint takes exactly one receive description and burn at most one; extra entries used to be dropped silently. getAddressString formats null as invalid-address(null). The transfer vectors now use populated descriptions and cover both signature paths; their expected digests are the pre-change output.
|
@Federico2014 @waynercheung Done in d986c51. Mint now requires exactly one receive description ( |
|
@waynercheung Regarding the getReward/getBrokerage discussion, here is what local nodes built from the base and from this branch return (
The base already answers an undecodable hex address with this error; the stricter check gives the other unusable inputs the same response instead of a default for an address that was never looked up. I think this is acceptable. Do you have any other concerns? Regarding gRPC getRewardInfo/getBrokerageInfo: yes, as a follow-up; the gRPC paths already behaved this way before this PR. It is listed under Follow up. |
waynercheung
left a comment
waynercheung
left a comment
There was a problem hiding this comment.
Re-reviewed d986c51 against base a975a3d.
The mint/burn receive-count validation and populated compatibility vectors are verified, and the null-address formatting case is covered. I independently checked all eight transfer vectors against the base, including both signature paths. The new mint/burn checks run before encoding and leave the encoding itself unchanged, so accepted mint/burn inputs produce the same bytes as before.
On JDK 17 / ARM64, all 220 selected repository tests and four additional review probes passed, along with framework main/test Checkstyle.
I accept the documented HTTP behavior change for missing or malformed addresses. Please carry the Compatibility notes into the release notes. Tracking gRPC address-validation alignment as a follow-up is sufficient for this PR.
No remaining blocking findings from this review.
waynercheung
left a comment
waynercheung
left a comment
There was a problem hiding this comment.
Re-opening my review: the global >21-byte hex rendering changes responses to valid queries of existing chain data, including the genesis witness and transaction owners (see inline).
Please address this output compatibility regression before merging. Updating the title or Compatibility section alone would not resolve it. If changing the historical representation is intentional, that needs a separate compatibility decision.
The previously verified fixes remain satisfactory; I'll re-review once this regression is addressed.
|
[MUST] The 64-character limit runs too late on the trigger path. |
|
@317787106 Confirmed on 75939ea: a 65-digit unquoted This predates this PR and is addressed by #7031 (2970bc2, already merged into release_v4.8.3): integer tokens are limited to 256 characters before conversion, and range errors no longer include the input. @0xbigapple please rebase onto release_v4.8.3 rather than adding a duplicate parser bound here. [SHOULD] After rebasing, please add endpoint-level regression tests for these four endpoints, covering both out-of-range integers and tokens exceeding 256 characters, for example, 65 and 257 consecutive 9 digits. Assert that parsing rejects the request and the decoded error message does not echo the submitted value. The direct helper tests do not exercise these paths. |
@waynercheung Thanks, agreed: no second parser bound here. Merged release_v4.8.3 in f7a1fa3, so the branch now includes #7031. On the endpoint tests: the 65- and 257-digit cases exercise |
waynercheung
left a comment
waynercheung
left a comment
There was a problem hiding this comment.
Re-reviewed f7a1fa3 against base ad2c534.
#7031 is included. At the servlet level, I verified that 65- and 257-digit unquoted call_value values on the three contract endpoints, and Permission_id values on all four endpoints, are rejected during JsonFormat.merge without echoing the submitted value in the decoded error messages. Tracking endpoint-level regression tests as a follow-up is fine. Genesis responses retain their existing representation. The 158 selected repository tests covering this PR and #7031 passed on JDK 17 / ARM64.
[DISCUSS] Please clarify the compatibility policy for stored non-standard address fields. In a local reproduction, a signed transfer carrying a 129-byte raw_data.auths[0].account.address passes real validation and execution, is stored, and returns hex from gettransactionbyid with visible=true where the base returns Base58Check. This is a constructed non-standard value, not a valid 129-byte TRON account address or an identified historical mainnet transaction.
I understand the need to avoid expensive Base58Check encoding of oversized values. Please revise the opening Compatibility statement ("requests with valid input keep their existing behavior") to explicitly account for queries returning stored non-standard address fields longer than 128 bytes, and carry this scope into the release notes. If historical examples are known, please document their impact and the chosen compatibility handling.
The other reviewed changes remain satisfactory. I'll approve once the Compatibility section covers this scope.
@waynercheung Updated the Compatibility section to cover stored address fields longer than 128 bytes; it will carry into the release notes. |
7 participants
What does this PR do?
This PR hardens externally reachable API inputs before expensive or ambiguous processing:
addressconsistently ongetRewardandgetBrokeragevisible=trueresponsesWalletUtil.getAddressString, which Base58Check-encodes only 20- and 21-byte values and renders any other length asinvalid-address(<hex of at most 21 bytes>,len=N), instead of Base58Check-encoding the whole valueUtil.getJsonLongValue(contract deploy and trigger values, exchange and proposal IDs), in string or numeric form, to 64 characters beforeBigDecimalconversion, and rejects values that are not exact 64-bit integersPermission_idonly as a JSON integer or a decimal integer string within the 32-bit rangeBigIntegerconversion, and reports an unusable amount or trigger input as aContractValidateExceptionInvalidHttpParameterExceptionfor HTTP parameter validation and lets the error sanitizer pass its message through, without echoing the rejected valueWhy are these changes required?
Several inputs were converted or encoded before their size or format was checked, so a malformed value could be truncated or wrapped silently, take longer to process than needed, or yield a plausible-looking result for a structurally invalid address.
These changes check inputs before conversion, encoding, storage access, or buffer merging.
This PR has been tested by:
Compatibility
Breaking, limited to requests with missing or invalid input and to the address rendering change below; other requests with valid input keep their existing behavior.
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.visible=true, an address field longer than 128 bytes is rendered as hex instead of Base58Check, even one already stored on chain, though none is known on mainnetPermission_idvalues other than a 32-bit JSON integer or decimal integer string are rejected, including booleans, fractions, overflowing integers, explicitnull, floating-point forms such as1.0or1e2, and strings such as"1,000"or"1.0";trueused to be applied as permission 1,1.0and"1.0"as 1,1e2as 100, and"1,000"as 1000node.allowShieldedTransactionApiflag is enabled (disabled by default):createShieldedContractParameters,createShieldedContractParametersWithoutAskandgetTriggerInputForShieldedTRC20Contract; such a value already exceeded the 256-bit range unless padded with leading zerosgetTriggerInputForShieldedTRC20Contractrejects a transfer with no or more than two receive descriptions, a mint with other than one, and a burn with more than oneinternal server error, such askey [x] does not exist, an overflowing integer, or an unusable shielded TRC20 trigger input, now answer the specific messageFollow up
getRewardInfoandgetBrokerageInfowith the HTTP endpointsExtra details