Skip to content

fix: strengthen TransactionList validation in fromBytes#589

Merged
rwalworth merged 1 commit intomainfrom
fix-chunked-transaction-group-checking
Apr 1, 2026
Merged

fix: strengthen TransactionList validation in fromBytes#589
rwalworth merged 1 commit intomainfrom
fix-chunked-transaction-group-checking

Conversation

@rwalworth
Copy link
Copy Markdown
Contributor

@rwalworth rwalworth commented Mar 26, 2026

Summary

This PR adds stricter structural validation to Transaction.fromBytes() when deserializing a TransactionList containing multiple TransactionId groups. The changes enforce invariants that were previously unchecked: only chunked transaction types (TopicMessageSubmitTransaction, FileAppendTransaction) are permitted to have multiple groups, and for chunked types the declared chunkInfo.total must match the actual group count with sequential chunk numbers.

Key Changes:

  • Reject multi-group TransactionList payloads for all non-chunked transaction types
  • Validate chunkInfo.total matches the actual number of TransactionId groups for consensusSubmitMessage
  • Validate chunkInfo.number values are sequential (1, 2, 3…) across groups
  • Add 4 unit tests covering the new rejection cases

Changes

Structural validation in Transaction.fromBytes (Transaction.swift)

Two new validation blocks are inserted after the existing protoTransactionBodyEqual loop.

Block 1 — Non-chunked type guard:

Only consensusSubmitMessage and fileAppend legitimately produce multiple TransactionId groups (one per chunk). All other transaction types must have exactly one group. A TransactionList with multiple groups for any other type is now rejected with a descriptive error.

if sources.chunksCount > 1 {
    switch transactionBodies[0].data {
    case .consensusSubmitMessage, .fileAppend:
        break
    default:
        throw HError.fromProtobuf("non-chunked transaction must not have multiple transaction ID groups")
    }
}

Block 2 — chunkInfo consistency for consensusSubmitMessage:

For chunked HCS messages, chunkInfo.total declares the expected number of chunks. This is now validated against the actual group count, and each group's chunkInfo.number is checked to be sequential starting at 1.

if case .consensusSubmitMessage(let msg) = transactionBodies[0].data, msg.hasChunkInfo {
    guard sources.chunksCount == Int(msg.chunkInfo.total) else {
        throw HError.fromProtobuf(...)
    }
    for (chunkIndex, chunk) in sources.chunks.enumerated() {
        // validates chunkMsg.chunkInfo.number == chunkIndex + 1
    }
}

Note: Swift's existing protoTransactionBodyEqual already validates cross-group body consistency for all types (comparing every signed transaction against the first), which covers the equivalent of C++'s Layer 2 normalization check. No change was needed there.

Comment update (TopicMessageSubmitTransaction.swift)

Removed the stale comment on line 31 (// note: no other SDK checks for correctness here... so let's not do it here either?) and replaced it with a reference to the new validation in fromBytes.

Tests (ChunkedTransactionUnitTests.swift)

Four new rejection tests plus retention of the existing round-trip regression test:

Test Scenario
test_fromBytes_rejectMultiGroupNonChunked CryptoTransfer with 2 groups → throws
test_fromBytes_rejectChunkCountMismatch_totalTooLow 2 groups, chunkInfo.total = 1 → throws
test_fromBytes_rejectChunkCountMismatch_totalTooHigh 2 groups, chunkInfo.total = 3 → throws
test_fromBytes_rejectOutOfOrderChunkNumbers 2 groups with swapped chunkInfo.number → throws
test_ToFromBytes Legitimate 2-chunk message round-trips correctly ✅

Testing

swift test --filter ChunkedTransactionUnitTests

All 5 tests pass, including the pre-existing round-trip test.


Files Changed Summary

Category File
Core validation Sources/Hiero/Transaction.swift
Comment update Sources/Hiero/Topic/TopicMessageSubmitTransaction.swift
Tests Tests/HieroUnitTests/ChunkedTransactionUnitTests.swift

Breaking Changes

None. All public APIs remain unchanged. The new validation only affects Transaction.fromBytes() when given a structurally inconsistent TransactionList. Legitimate multi-chunk TopicMessageSubmitTransaction and FileAppendTransaction payloads continue to deserialize correctly.

@rwalworth rwalworth requested a review from a team as a code owner March 26, 2026 23:51
@rwalworth rwalworth requested a review from SimiHunjan March 26, 2026 23:51
@rwalworth rwalworth changed the title fix: reject cross-group TransactionList forgery in fromBytes fix: strengthen TransactionList validation in fromBytes Mar 26, 2026
@rwalworth rwalworth self-assigned this Mar 26, 2026
@rwalworth rwalworth added the status: needs review The pull request is ready for maintainer review label Mar 26, 2026
Copy link
Copy Markdown

@ddeskov-limechain ddeskov-limechain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rwalworth rwalworth merged commit a40b58b into main Apr 1, 2026
14 checks passed
@rwalworth rwalworth removed the status: needs review The pull request is ready for maintainer review label Apr 1, 2026
@rwalworth rwalworth deleted the fix-chunked-transaction-group-checking branch April 1, 2026 14:32
@rwalworth rwalworth added this to the v0.49.0 milestone Apr 1, 2026
@rwalworth rwalworth added priority: high Important issue that should be prioritized in the current sprint/release skill: intermediate Requires familiarity with the codebase structure and SDK concepts kind: security Security vulnerabilities or security-related improvements scope: grpc Related to gRPC/protobuf/network layer labels Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: security Security vulnerabilities or security-related improvements priority: high Important issue that should be prioritized in the current sprint/release scope: grpc Related to gRPC/protobuf/network layer skill: intermediate Requires familiarity with the codebase structure and SDK concepts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants