#73 Hardening of SDK by martti007 · Pull Request #74 · unicitynetwork/state-transition-sdk-java

martti007 · 2026-07-02T13:12:33Z

No description provided.

gemini-code-assist

Code Review

This pull request introduces token splitting functionality by replacing plain sparse Merkle trees with radix sparse Merkle sum trees, adding classes like SplitAllocationProof, SplitManifest, and SplitMintJustification, and hardening transport security for JSON-RPC clients. The code review feedback identifies several important issues: incorrect signed integer range restrictions in CborDeserializer for unsigned byte and short values, a potential infinite loop vulnerability in iterative token verification due to untracked visited tokens, and an OkHttp client instantiation anti-pattern in JsonRpcHttpTransport. Additionally, the reviewer recommends adding defensive null and parameter validation checks across multiple newly introduced classes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

MastaP

Automated deep review of this PR: 8 review angles surfaced 22 unique candidates, each was adversarially verified against the PR head (381dede); the 8 that survived are attached as inline comments, ranked by severity in each comment header.

Also worth knowing what was checked and cleared, since several look suspicious at first glance:

SignatureEntry.equals dropping signature-byte comparison is sound intentional hardening — base hashCode was already key-only, and the change closes a quorum-inflation vector where one node's malleated signatures could count multiple times toward quorum.
The RootTrustBase quorum-threshold ≤ node-count validation is consistent with the SDK's own semantics: UnicitySealQuorumSignaturesVerificationRule counts signatures (stake is never read anywhere), so a stake-weighted threshold could never verify before this PR either.
JsonRpcResponse rejecting result: null breaks nothing reachable — none of the three RPC methods the SDK uses can legitimately return a null result, and the base code already crashed on such replies (NPE / HexConverter.decode(null)).
The Asset zero-value/minimal-encoding tightening has no compatibility surface: the old encoder was minimal by construction, Asset.fromCbor has no production callers, and the payment package has never shipped in a release tag.

Two minor confirmed cleanups didn't make the severity cut: TokenIssuanceVerifierService keys its registry map by HexConverter.encode(tokenType.getBytes()) strings although TokenType has value-based equals/hashCode (a Map<TokenType, …> avoids a clone + hex string per lookup), and PaymentAssetCollection.toList() re-copies the list on every call while TokenSplit calls it in per-request loops (caching the immutable list in the constructor is safe — the map is never mutated).

MastaP · 2026-07-03T10:56:18Z

-    if (id <= 0) {
-      throw new IllegalArgumentException(
-          "Network identifier out of allowed 16-bit unsigned range: " + id + ".");
+    if (id == 0) {


[1 — correctness, confirmed] Relaxing this guard from id <= 0 to id == 0 admits negative shorts (raw ids 0x8000–0xFFFF), and every CBOR encode site passes the raw short to CborSerializer.encodeUnsignedInteger(long), where Java's short→long widening sign-extends it: MintTransaction.java:387, TokenId.java:50, UnicitySeal.java:181, UnicityIdMintTransaction.java:244.

Concretely, NetworkId.fromId((short) 40000) now encodes as CBOR uint 0xFFFFFFFFFFFF9C40 instead of 40000, so:

MintTransaction.fromCbor(tx.toCbor()) throws CborSerializationException("Value too large") from asShort() — the SDK can't round-trip its own output;

worse, a properly encoded on-wire seal with networkId ≥ 0x8000 decodes fine (asShort() allows up to 0xFFFF, cast to negative short), but UnicitySeal.toCborWithoutSignatures() then re-encodes the sign-extended 8-byte value — different bytes than the root nodes signed — so UnicitySealQuorumSignaturesVerificationRule hashes the wrong preimage and valid quorum signatures verify as FAIL on any network with id ≥ 32768. In base this path failed loudly at decode; the PR makes the silent-divergence path newly reachable.

Fix: mask at the encode sites (Short.toUnsignedInt(networkId.getId()) / & 0xFFFF), or keep rejecting negative ids here.

MastaP · 2026-07-03T10:56:18Z

                            .constructParametricType(JsonRpcResponse.class, resultType)
            );

+            if (!requestId.equals(data.getId())) {


[2 — correctness, confirmed] This id-correlation check runs before the JSON-RPC error branch below it. JSON-RPC 2.0 mandates "id": null in error responses when the request id could not be determined ("If there was an error in detecting the id in the Request object (e.g. Parse error/Invalid Request), it MUST be Null").

So a spec-compliant HTTP-200 reply like {"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null} now completes the future with IllegalArgumentException("JSON-RPC response id mismatch: … got null.") — the requestId is always a fresh non-null UUID, and JsonRpcResponse accepts a null id — masking the server's error code/message. Base surfaced this as JsonRpcNetworkException(-32700, "Parse error"), which is the exception type the SDK's own tests treat as the error-surface contract (TestApiKeyIntegration, JsonRpcHttpTransportTest). Note the earlier !response.isSuccessful() check only guards non-2xx statuses, not the normal JSON-RPC-over-HTTP 200-with-error-body pattern.

Fix: handle data.getError() != null first (or skip id correlation when an error object is present).

MastaP · 2026-07-03T10:56:18Z

@@ -1,472 +0,0 @@
-package org.unicitylabs.sdk.functional.payment;


[3 — test coverage, confirmed] Deleting this 472-line test removes nearly all adversarial-rejection coverage for split verification, and the replacements don't restore it. The rewritten SplitMintJustificationVerifier has 10 reject branches; a message-by-message grep of the PR test tree shows only one is exercised anywhere ("Allocation proof failed for asset %s.", via SplitInflationExploitTest and SparseMerkleSumTreeTest.rejectsTamperedLeafAmount). Untested branches include:

"Transaction has no justification."

corrupted embedded burn token (nested-token verification path)

"Burn transfer recipient does not match the manifest hash." — the security-critical binding: if this regressed, an attacker could attach a fabricated manifest with arbitrary roots, and SplitInflationExploitTest would not catch it (its attacker builds a genuine burn correctly bound to the manifest)

"Allocation proof count does not match the output asset count." / "Manifest root count does not match the source asset count."

"Asset %s is absent from the source token."

network-id mismatch, token-type mismatch, "Burn transfer has no manifest."

all SplitAllocationProof.fromCbor validation branches (max 256 siblings, depth range, strictly-decreasing depths, positive sums) — only a happy-path round-trip exists.

The old test couldn't survive verbatim since the PR redesigns the split mechanism, but the rejection cases themselves still apply and should be rebuilt against the new SplitManifest/SplitAllocationProof scheme — a regression in any of these nine branches would currently pass CI.

MastaP · 2026-07-03T10:56:18Z

+    BigInteger keyPath = BitString.fromBytesReversedLSB(key).toBigInteger();
+
+    DataHash hash = new DataHasher(HashAlgorithm.SHA256)
+            .update(new byte[]{0x10})


[4 — reuse/altitude with correctness risk, confirmed] calculateRoot re-implements the radix-sum-tree digest formulas byte-for-byte from smt/radixsum — the 0x10 || key || data || u256(value) leaf digest here duplicates FinalizedLeafBranch.fromPendingLeaf, and the 0x11 || u8(depth) || hL || u256(vL) || hR || u256(vR) node digest below duplicates FinalizedNodeBranch.fromPendingNode.

The divergence risk is not hypothetical: the tree side is parameterized (SparseMerkleSumTree(HashAlgorithm) threads the algorithm through finalization) while this verifier hardcodes HashAlgorithm.SHA256 in both calculateRoot and fromCbor. They agree today only because the sole production instantiation (TokenSplit.java:111) passes SHA-256 — changing that constructor argument would make every proof silently fail verification, with no compile-time signal.

Fix: extract shared static leafDigest(...)/nodeDigest(...) helpers in the radixsum package, taking HashAlgorithm as a parameter, and call them from both tree finalization and proof reconstruction (sibling ordering can stay here).

MastaP · 2026-07-03T10:56:18Z

+   * @param httpClient OkHttp client to use
+   * @param maxResponseBytes maximum response body size in bytes
+   */
+  public JsonRpcHttpTransport(String url, OkHttpClient httpClient, int maxResponseBytes) {


[5 — security consistency, confirmed] This constructor stores the caller-supplied OkHttpClient verbatim, silently dropping the followRedirects(false)/followSslRedirects(false) hardening of DEFAULT_HTTP_CLIENT — the very property this PR adds and tests as a security invariant (JsonRpcHttpTransportTest.doesNotFollowRedirects: "redirects must not be followed, so authentication headers are never replayed to a redirect target"). A vanilla new OkHttpClient() follows redirects by default, and the Javadoc here doesn't transfer that obligation to the caller.

(Severity is softened by OkHttp stripping Authorization on cross-host/scheme/port redirects, but request-body replay and same-host redirects remain, and the constructor currently has zero callers — so this is cheap to fix before usage appears.)

Fix: normalize the redirect policy while keeping the shared pools the Javadoc promises:

this.httpClient = httpClient.newBuilder() .followRedirects(false) .followSslRedirects(false) .build();

Related altitude note: the https-only-for-API-key rule lives only in JsonRpcAggregatorClient's constructor, while this public transport with a public headers-accepting request() has no scheme check — the transport (where credentials actually leave the process) would be the single choke point for both rules.

MastaP · 2026-07-03T10:56:18Z

            token,
            EncodedPredicate.fromCbor(data.get(1)),
-            CborDeserializer.decodeByteString(data.get(2)),
+            StateMask.fromCbor(data.get(2)),


[6 — compatibility, plausible] Routing the state mask through StateMask.fromCbor imposes an exact-32-byte rule on a wire field that released API left unconstrained: the old public TransferTransaction.create(Token, Predicate, byte[] stateMask, byte[] data) accepted any length with no documented restriction, toCbor/fromCbor round-tripped it as-is, and the raw mask never reaches the aggregator (only hashes do), so a transfer with a nonstandard mask would have certified and verified fine. Such a token now throws IllegalArgumentException("State mask must be 32 bytes long…") inside Token.fromCbor, i.e. previously-valid persisted wallet data becomes undeserializable with a crash rather than a verification failure.

Reachability is theoretical — every in-repo producer always generated exactly 32 bytes — so this may well be acceptable; but since it's a breaking wire-format tightening on a released class (unlike the unreleased payment package), it deserves an explicit release note / migration note.

MastaP · 2026-07-03T10:56:18Z

+      int total = 0;
+      int read;
+      while ((read = in.read(buffer)) != -1) {
+        total += read;


[7 — correctness (low), confirmed] total += read accumulates in an int with no overflow guard, and the public constructors accept maxResponseBytes without validation. With new JsonRpcHttpTransport(url, Integer.MAX_VALUE) the guard total > this.maxResponseBytes is unsatisfiable for any int — after ~2 GiB total wraps negative and buffering continues until ByteArrayOutputStream throws OutOfMemoryError instead of the intended clean IOException. And maxResponseBytes <= 0 is accepted but silently rejects every non-empty response (fails closed).

Low severity — the 8 MiB default and the only in-repo caller are unaffected; it only bites extreme caller-chosen configs. Fix: use a long total and validate maxResponseBytes > 0 in the constructor.

MastaP · 2026-07-03T10:56:18Z

+   * @throws IllegalArgumentException if the value is not a positive 256-bit integer, or the key
+   *     or data is not 32 bytes
+   */
+  public synchronized void addLeaf(byte[] key, byte[] data, BigInteger value)


[8 — consistency/altitude, verified] The input hardening added here (null checks + 32-byte key/data validation + value range) was applied only to this sum-tree variant: smt/radix/SparseMerkleTree.addLeaf — the parallel plain radix trie, untouched by this PR — validates nothing beyond path > 0. A null or wrong-length key there surfaces as an NPE or a silently malformed path instead of a clear IllegalArgumentException, and the two trie implementations now enforce different input domains for structurally identical insertion logic.

Suggest applying the same key/data validation to radix.SparseMerkleTree.addLeaf in this PR (it's a hardening PR, and the checks are copy-paste). Longer-term, the two packages duplicate the path-compressed trie wholesale (PendingLeafBranch/PendingNodeBranch/FinalizedLeafBranch/… differ mainly in the bolted-on sum) — a structural bug fixed in one will be missed in the other, which guards money; parameterizing the branch over an aggregated value (identity for the plain tree, checked 256-bit sum here) would leave one implementation to maintain.

martti007 added 10 commits July 2, 2026 09:25

#73 Make nested token verification iterable instead of recursive

1220fdb

#73 Make cbor parsing iterate over cbor instead of recursion

c545f8e

#73 Harden RootTrustBase verification

71b60f4

#73 Add BigInteger decode and encode to CborSerializer

76934a0

#73 add radix sum tree and remove unused trees

1e7b8b1

#73 do not allow key to be sent over http by default

fa29642

#73 verify that certification data matches transaction

e54b8e2

#73 add signature recovery byte verification

2090dab

#73 add json rpc checks

a502336

#73 add context to token verification instead of separate parameters

6e6b59b

gemini-code-assist Bot reviewed Jul 2, 2026

View reviewed changes

martti007 added 2 commits July 2, 2026 13:33

#73 Change code to be compatible with Android #31

0d3799b

#73 Fix issues brought up in pull request

381dede

MastaP reviewed Jul 3, 2026

View reviewed changes

martti007 added 8 commits July 3, 2026 13:18

#73 Make NetworkId to use ints to allow unsigned shorts

3a12783

#73 Show JSON RPC error before verifying request id

7f1f33f

#73 Forbid redirects on user defined http client

14fb538

#73 Allow user defined length for statemask and tokensalt up to 64 bytes

0737c09

#73 Validate max response byte count for json rpc

a0af5f5

#73 Add input checks for plain radix tree leaf adding

1131950

#73 Add sibling hash check for split allocation proof

3f0cb07

#73 Add length limits to token type

0a84705

		@@ -1,472 +0,0 @@
		package org.unicitylabs.sdk.functional.payment;

Uh oh!

Conversation

martti007 commented Jul 2, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MastaP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants