B2CA-2394 : handle split reception at apdu parsing step

[mbrousset-ledger]

Checklist

• App update process has been followed
• Target branch is develop
• Application version has been bumped

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.81%. Comparing base (3ea745a) to head (d0d1849).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #205   +/-   ##
=======================================
  Coverage   90.81%   90.81%           
=======================================
  Files           5        5           
  Lines          98       98           
  Branches       22       22           
=======================================
  Hits           89       89           
  Misses          9        9           

Flag	Coverage Δ
unittests	90.81% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Refactors transaction signing APDU handling so chunk reassembly happens in the APDU dispatcher, and handler_sign_tx is invoked only once the full transaction payload is available.

Changes:

• Moved SIGN_TX / SIGN_TOKEN_TX chunk initialization + accumulation logic from handler/sign_tx.c into apdu/dispatcher.c.
• Simplified handler_sign_tx API to only take is_token_tx (no chunk/more parameters).
• Bumped app patch version from 2.3.1 to 2.3.2.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/handler/sign_tx.h	Updates SIGN_TX handler signature and header comment to reflect dispatcher-side reassembly.
src/handler/sign_tx.c	Removes per-chunk parsing/accumulation; handler now processes the already-reassembled raw transaction.
src/apdu/dispatcher.c	Adds transaction init + accumulation helpers; dispatcher now manages multi-APDU reassembly before calling the signing handler.
Makefile	Increments patch version.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mbrousset-ledger]

@claude review

[cedelavergne-ledger]

Maybe interesting to look at buffer_get_path_bip32 in SDK (lib_standard_app/buffer.c)

[mbrousset-ledger]

@claude review

[claude]

Claude finished @mbrousset-ledger's task in 1m 51s —— View job

---

Code Review

• Gathering context
• Reading changed files
• Analyzing changes
• Posting review feedback

---

Summary

The refactor cleanly achieves its goal: moving multi-chunk APDU reassembly out of handler_sign_tx and into the dispatcher, so the handler can focus on processing a fully-assembled transaction. The separation of concerns is a genuine improvement. However, there are a few issues to address before merging.

---

Issues

1. Critical — Missing return/os_sched_exit() after send_swap_error_simple in swap error path

File: src/handler/sign_tx.c, lines 113–123

if (err != SWO_SUCCESS) {
#ifdef HAVE_SWAP
    if (G_called_from_swap) {
        PRINTF("Error during transaction processing in swap context: %u\n", err);
        send_swap_error_simple(SW_SWAP_FAIL, SWAP_EC_ERROR_GENERIC, SWAP_ERROR_CODE);
        // ← NO RETURN HERE — falls through!
    } else {
        return io_send_sw(err);
    }
#else
    return io_send_sw(err);
#endif
}

When HAVE_SWAP is defined, process_transaction() fails, and G_called_from_swap is true: send_swap_error_simple() is called but execution falls through past the entire if (err != SWO_SUCCESS) block. It then reaches line 130:

if (G_called_from_swap) {
    check_and_sign_swap_tx(&G_context.tx_info);  // ← called with invalid/partial tx_info!

This calls check_and_sign_swap_tx() with invalid state. The fix is to add os_sched_exit(-1) (or at minimum a return) immediately after send_swap_error_simple(), matching the pattern in src/swap/handle_swap_sign_transaction.c.

Note: this was technically also present in the original code since both code paths had the same structure, but the refactor makes it more visible. Fix this →

---

2. Spec compliance — Single-APDU flow is broken

File: src/apdu/dispatcher.c, line 124

if ((cmd->p1 == P1_START && cmd->p2 != P2_MORE) ||  // ← rejects P1=0x00, P2=0x00
    cmd->p1 > P1_MAX ||
    (cmd->p2 != P2_LAST && cmd->p2 != P2_MORE)) {
    return io_send_sw(SWO_INCORRECT_P1_P2);
}

APP_SPECIFICATION.md lines 89–95 explicitly documents:

Single APDU (small transaction) — P1: 00 (first chunk), P2: 00 (last chunk, no more data)

The guard rejects P1_START + P2_LAST (P1=0x00, P2=0x00), making the documented flow impossible. Since P1_START now means "initialize context + parse BIP32 path only", there's no path to trigger signing in a single APDU even if the transaction is tiny.

Either:

• Allow (P1_START, P2_LAST) in the init_transaction_context path and handle the remaining buffer bytes as transaction data before calling handler_sign_tx, or
• Update APP_SPECIFICATION.md to document that a minimum of 2 APDUs are now required and update any client libraries accordingly.

Fix this →

---

3. Minor — Stale @see reference in header

File: src/handler/sign_tx.h, line 11

 * @see G_context.bip32_path, G_context.tx_info.raw_transaction,

The field is raw_tx / raw_tx_len (confirmed in src/types.h:62), not raw_transaction. This is misleading documentation.

Fix this →

---

4. Minor — Consider buffer_get_path_bip32 from SDK

File: src/apdu/dispatcher.c, lines 51–53

As noted by @cedelavergne-ledger, the manual two-step buffer_read_u8(length) + buffer_read_bip32_path(path) can be replaced with buffer_get_path_bip32 from lib_standard_app/buffer.c. This is used consistently in other handlers (e.g. get_public_key.c) in the older codebase. Worth aligning for consistency.

---

What's Good

• The overall architecture change is correct and well-motivated — reassembly belongs in the dispatcher, not in every individual handler.
• accumulate_transaction_data boundary check (raw_tx_len + cdata->size > sizeof(raw_tx)) correctly guards against buffer overflow.
• The req_type mismatch check in accumulate_transaction_data is a good guard against state confusion between interleaved SIGN_TX / SIGN_TOKEN_TX APDUs.
• Removing dead code (return 0 at the end of the old handler_sign_tx) is a nice cleanup.
• Coverage is maintained at 90.81%.

[mbrousset-ledger]

@claude review

[claude]

Claude finished @mbrousset-ledger's task in 1m 22s —— View job

---

Code Review

• Gathering context
• Reading changed files
• Analyzing changes
• Posting review feedback

---

Summary

Good refactor — moving multi-chunk APDU reassembly from handler_sign_tx into the dispatcher is the right architectural call and resolves the TODO that was in the old code. The handler now has a clean single-responsibility: process a fully-assembled transaction. Three issues to address before merging.

---

Issues

1. Critical — Missing os_sched_exit() after send_swap_error_simple in error path

File: src/handler/sign_tx.c, lines 112–124

if (err != SWO_SUCCESS) {
#ifdef HAVE_SWAP
    if (G_called_from_swap) {
        PRINTF("Error during transaction processing in swap context: %u\n", err);
        send_swap_error_simple(SW_SWAP_FAIL, SWAP_EC_ERROR_GENERIC, SWAP_ERROR_CODE);
        // ← falls through to check_and_sign_swap_tx() with invalid state
    } else {
        return io_send_sw(err);
    }
#else
    return io_send_sw(err);
#endif
}

When HAVE_SWAP is defined and G_called_from_swap is true, after send_swap_error_simple() execution falls through past the if (err != SWO_SUCCESS) block and reaches check_and_sign_swap_tx(&G_context.tx_info) at line 130 — calling it with an invalid/partially-parsed tx_info.

Every other call to send_swap_error_simple() in src/swap/handle_swap_sign_transaction.c is immediately followed by os_sched_exit(0) (see lines 128–130, 144–148, 154–156, etc.). This path should do the same.

Note: this bug existed in the old code too (same structure), so not introduced by this PR — but this is a good opportunity to fix it.

Fix this →

---

2. Spec compliance — Single-APDU flow (P1=0x00, P2=0x00) is rejected

File: src/apdu/dispatcher.c, line 124

if ((cmd->p1 == P1_START && cmd->p2 != P2_MORE) ||  // ← rejects P1=0x00, P2=0x00

APP_SPECIFICATION.md lines 89–96 documents a single-APDU flow:

Example 1: Single APDU (small transaction) — P1: 00 (first chunk), P2: 00 (last chunk, no more data)

This guard requires P2_MORE when P1_START, which rejects the documented (P1=0x00, P2=0x00) case. Additionally, the P1_START branch at line 141 only parses the BIP32 path and returns — any transaction bytes in the same APDU would be silently discarded.

This was already effectively broken in the old code (same guard existed), but worth deciding: either support the single-APDU flow (allow P1_START + P2_LAST and handle remaining buffer bytes as transaction data) or update APP_SPECIFICATION.md to remove the single-APDU example and document a 2-APDU minimum.

Fix this →

---

3. Minor — Stale @see reference in header

File: src/handler/sign_tx.h, line 11

 * @see G_context.bip32_path, G_context.tx_info.raw_transaction,

The field is raw_tx / raw_tx_len (confirmed in src/types.h:62), not raw_transaction.

Fix this →

---

4. Suggestion — Use buffer_get_path_bip32 from SDK

File: src/apdu/dispatcher.c, lines 51–52

As @cedelavergne-ledger noted, the manual buffer_read_u8 + buffer_read_bip32_path pair could be replaced with buffer_get_path_bip32 from lib_standard_app/buffer.c, which handles both steps. This aligns with the pattern used in get_public_key.c and reduces the chance of the two calls going out of sync.

---

What's Good

• Correct separation of concerns — reassembly logic belongs in the dispatcher, not in each handler. The TODO in the old code (This should NOT be handled in each handler but at the dispatcher level) is now properly resolved.
• accumulate_transaction_data has solid boundary checks: overflow guard on raw_tx_len + cdata->size, and req_type mismatch check prevents state confusion between interleaved SIGN_TX / SIGN_TOKEN_TX sequences.
• Removing the unreachable return 0 at the end of the old handler_sign_tx is a nice cleanup.
• Dead code removal (buffer.h include in sign_tx.h, unused parameters) is clean.
• Coverage maintained at 90.81%.

---

[mbrousset-ledger]

@claude review

[claude]

Claude finished @mbrousset-ledger's task in 1m 34s —— View job

---

Code Review

• Gathering context
• Reading changed files
• Analyzing changes
• Posting review feedback

---

Summary

This PR moves multi-chunk APDU reassembly from handler_sign_tx into the dispatcher — a clean separation of concerns that resolves the TODO that was in the original code. The handler now has a single responsibility: process a fully-assembled transaction. The refactor is well-structured and the boundary checks in accumulate_transaction_data are solid. Three issues from previous reviews remain unresolved.

---

Issues

1. Critical — Missing os_sched_exit() after send_swap_error_simple in error path

File: src/handler/sign_tx.c:112-124

if (err != SWO_SUCCESS) {
#ifdef HAVE_SWAP
    if (G_called_from_swap) {
        PRINTF("Error during transaction processing in swap context: %u\n", err);
        send_swap_error_simple(SW_SWAP_FAIL, SWAP_EC_ERROR_GENERIC, SWAP_ERROR_CODE);
        // ← falls through — no return or os_sched_exit!
    } else {
        return io_send_sw(err);
    }
#else
    return io_send_sw(err);
#endif
}

When HAVE_SWAP is defined, process_transaction() fails, and G_called_from_swap is true: send_swap_error_simple() is called but execution falls through past the if (err != SWO_SUCCESS) block. It then reaches line 130 where check_and_sign_swap_tx(&G_context.tx_info) is called with an invalid/partially-parsed tx_info.

Every other call to send_swap_error_simple() in src/swap/handle_swap_sign_transaction.c is immediately followed by os_sched_exit(0) (lines 128–130, 144–148, 154–156, 169–171, 178–180, 191–193). This path should do the same.

Note: This bug existed in the pre-refactor code with the same structure (confirmed via git show origin/master:src/handler/sign_tx.c), so it's not introduced by this PR — but this refactor is the right time to fix it.

Fix this →

---

2. Spec compliance — Single-APDU flow (P1=0x00, P2=0x00) is rejected

File: src/apdu/dispatcher.c:124

if ((cmd->p1 == P1_START && cmd->p2 != P2_MORE) ||  // ← rejects P1=0x00, P2=0x00

APP_SPECIFICATION.md lines 89–95 documents a single-APDU flow:

Example 1: Single APDU (small transaction) — P1: 00 (first chunk), P2: 00 (last chunk, no more data)

This guard requires P2_MORE when P1_START, making the documented (P1=0x00, P2=0x00) case impossible. Additionally, the P1_START branch at line 141 only parses the BIP32 path and returns SWO_SUCCESS — any transaction bytes in the same APDU would be silently discarded.

Note: This guard already existed in the original code (confirmed at origin/master:src/apdu/dispatcher.c:79), so it's pre-existing. However, since you're refactoring this flow, it's worth deciding:

• Option A: Allow (P1_START, P2_LAST) and handle remaining buffer bytes after BIP32 parsing as transaction data, then call handler_sign_tx directly.
• Option B: Update APP_SPECIFICATION.md to remove the single-APDU example and document a 2-APDU minimum.

Fix this →

---

3. Minor — Stale @see reference in header

File: src/handler/sign_tx.h:11

 * @see G_context.bip32_path, G_context.tx_info.raw_transaction,

The field is raw_tx / raw_tx_len (confirmed in src/types.h:62), not raw_transaction.

Fix this →

---

4. Suggestion — Use buffer_get_path_bip32 from SDK

File: src/apdu/dispatcher.c:51-52

As @cedelavergne-ledger noted, the manual buffer_read_u8 + buffer_read_bip32_path pair could be replaced with buffer_get_path_bip32 from lib_standard_app/buffer.c, which handles both steps atomically. This would align with SDK conventions and reduce the chance of the two calls drifting out of sync.

---

What's Good

• The overall architecture is correct — reassembly belongs in the dispatcher, not in each handler. The TODO is properly resolved.
• accumulate_transaction_data has solid boundary checks: overflow guard on raw_tx_len + cdata->size, and req_type mismatch check prevents state confusion between interleaved SIGN_TX / SIGN_TOKEN_TX sequences.
• Removing the unreachable return 0 at the end of the old handler_sign_tx and the dead buffer.h include is clean.
• Coverage maintained at 90.81%.

---

View job