fix(core): harden ApiWrapper input validation and channel shutdown - #222
Open
0xbigapple wants to merge 8 commits into
Open
fix(core): harden ApiWrapper input validation and channel shutdown#2220xbigapple wants to merge 8 commits into
0xbigapple wants to merge 8 commits into
Conversation
- withTLS(File) now rejects directories and unreadable files with a clear IllegalArgumentException at configuration time, instead of failing later in build() with a generic "Failed to create TLS channel" - toString() no longer expands customInterceptors (whose own toString may contain tokens or other sensitive data); it prints customInterceptorCount instead
close() previously only initiated an async shutdown and returned, so RPCs issued without a deadline could keep the channel alive indefinitely. Now it waits up to 5 seconds per channel for graceful termination, then falls back to shutdownNow(), restoring the interrupt flag if interrupted while waiting.
parseAddress previously returned whatever bytes the hex/base58 input decoded to, so inputs like "41", "0x41" or 22-byte hex strings produced malformed addresses that could even end up in SUCCESS transactions under local-create mode. The decoded bytes are now validated to be a proper TRON address (21 bytes with the 0x41 prefix) and rejected otherwise with a clear IllegalArgumentException. freezeBalance/unfreezeBalance treat the receiver as the optional field it is on chain: an empty receiveAddress (used by the no-receiver overloads, meaning the owner's own stake) skips setReceiverAddress instead of being passed through parseAddress, which would now reject it; non-empty receivers are still validated.
signTransaction(TransactionExtention, KeyPair) previously signed whatever txid the extention carried, so a forged txid (e.g. transaction A's txid attached to transaction B) yielded a valid signature for a different transaction. The hash to sign is now always recomputed from the embedded transaction's raw data; a non-empty txid that does not match the raw data is rejected with IllegalArgumentException, and an absent txid no longer results in signing empty bytes.
SECP256K1.PrivateKey.create previously accepted any 32-byte scalar, so 0, the curve order n, and n + 1 all produced usable-looking KeyPairs that could derive addresses (n + 1 even deriving the same address as private key 1 via the mod-n reduction in PublicKey.create) but failed later at signing time inside Bouncy Castle. The scalar range is now validated at construction with the same [1, n - 1] rule Bouncy Castle enforces, covering KeyPair(String) and all other construction paths. ApiWrapperBuilder.withPrivateKey performs the same check up front so invalid keys fail at input time rather than build().
parseAddress maps an empty string to ByteString.EMPTY (an unset protobuf field, the pre-1.0 behavior for optional addresses such as a freeze receiver or a constant-call owner), rejects input longer than 64 characters before decoding, and rethrows decoding failures as the documented IllegalArgumentException. The freeze/unfreeze receiver guards are reverted accordingly. signTransaction now rejects empty transaction raw data (previously signed silently, swallowing node errors) and, for TransactionExtention, requires a txid that is present and matches the raw data.
Loading SECP256K1 used to remove the host's "BC" provider and insert the SDK's own instance at priority 1, silently changing algorithm resolution and compliance posture for the whole process. The provider is now passed explicitly to KeyPairGenerator and the JVM-global provider list is no longer touched; the bundled full provider still covers Android's stripped "BC". Applications that implicitly relied on trident registering BC globally must now register it themselves. The hand-written [1, n - 1] scalar check is replaced with Bouncy Castle's own ECDomainParameters.validatePrivateScalar, and withPrivateKey wraps validation failures as "invalid hexPrivateKey (reason)" without echoing key material.
… complete close() - deprecate SECP256K1.PROVIDER: trident no longer registers Bouncy Castle in the JVM-global provider list, so a provider named "BC" is not guaranteed to exist; callers that need it must register it themselves - tighten parseAddress MAX_ADDRESS_LENGTH from 64 to 44, the largest legal address form (base58: 34, hex: 42, 0x-hex: 44), so a 64-char hex private key passed by mistake is rejected by the length gate instead of being echoed into the exception message - close(): awaitTermination() called shutdownNow() on timeout and returned immediately, so close() could return with the channel still not terminated; it now waits again for the forced cancellation to take effect
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?
withTLS(File): reject directories and unreadable cert files at configuration timeApiWrapperBuilder.toString(): print interceptor count instead of expandinginterceptor instances
close(): shut down both channels, await termination up to 5s each, thenshutdownNow()parseAddress: validate non-empty input as a 21-byte0x41address and rejectinput longer than 64 characters before decoding; decoding failures surface as the
documented
IllegalArgumentException. An empty string maps toByteString.EMPTY(unset protobuf field), keeping optional-address
signTransaction: reject empty transaction raw data; aTransactionExtentionmust carry a txid that matches the raw data
SECP256K1.PrivateKey.create/withPrivateKey: reject private keys outside[1, n - 1]SECP256K1: pass the Bouncy Castle provider instance explicitly to the SDK's ownJCA calls and leave the JVM-global provider list untouched; Android's stripped
built-in "BC" is still bypassed
Why are these changes required?
This PR has been tested by:
Extra details
Breaking changes:
IllegalArgumentExceptionon invalid input.In particular, a client built with an out-of-range placeholder private key
(e.g. all-zero) could previously still perform on-chain queries; it now fails
at construction — build a read-only client without a private key instead.