chain: reject HMS transfer addresses with surrounding whitespace

[bobbyning]

Follow-up to #26 - the whitespace guard requested in its review did not make the merge window, and the fund-stranding path is live on merged main (bdc2ef2, re-verified after the #27 merge).

What

ValidateHmsTransferShape and the ed25519 signing payload both trim addresses, but settlement credits the raw tx strings. A transfer signed with a padded recipient (trailing space) passes validation, signs cleanly over the trimmed form, and - now that #26 wired the applier - settles into an account row no address lookup can find, stranding the amount.

Fix

Reject raw != trimmed for from/to in ValidateHmsTransferShape (used by submit, the apply-time re-validation, and the API simple-sign pre-check). Regression tests sign the trimmed form exactly like a real wallet would, cover the common whitespace boundaries (leading and trailing space, tab, newline, carriage return) plus a padded from, and assert invalid_address, the whitespace-specific message, and an empty pool. A second test seeds a padded row directly into hms_tx_pool and proves the apply-time path rejects it instead of crediting the raw address.

Scope: this closes the surrounding-whitespace variant (the reachable one). To still has only an HMC- prefix check, so internal whitespace or other malformed content in To remains a known follow-up, not closed here.

Evidence

• On merged main bdc2ef2: TestHMSTransferSettlesOnBlockAppend and TestHMSSelfTransferRejectedAtSubmit PASS (the merged PR works), while a padded-recipient submit is still accepted as pending and a signed padded pool row settles into an account row under the raw padded address - 1 HMS credited to the padded string, the canonical address lookup sees nothing (both RED).
• On this branch: all four tests in this file PASS; full internal/chain package passes, go vet clean, gofmt clean.

Note: the same trim-vs-raw shape exists in both sibling lanes - transfer_v1 (ValidateTransferShape; its applier credits the raw item.tx.To) and transfer_sup_v1 (ValidateSupTransferShape, same). This PR is deliberately scoped to the HMS lane for a minimal diff; happy to extend the guard to both siblings on request.

Contract: a padded recipient was previously accepted as pending and settled into an account row no lookup could find; after this change it fails closed with invalid_address at submit and is rejected at apply time. A padded sender was already rejected before this change, but as address_pubkey_mismatch via the derived-address check; it now fails the same whitespace guard. The HMS endpoint (/api/hms/tx/send) and the invalid_address code are not yet listed in spec/CHAIN_SPEC.md or docs/API.md; happy to document them in a follow-up if useful.

Summary by CodeRabbit

• Bug Fixes
  • Transfers with leading or trailing whitespace in sender or recipient addresses are now rejected as invalid, including transfers already in the pending queue.
  • Rejected transfers do not credit the padded address, debit the sender, or advance the sender’s nonce.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: jokeez/hackme/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 5124e444-138e-4eb3-97aa-f5b8643df5b6

📥 Commits

Reviewing files that changed from the base of the PR and between bdc2ef2 and 6dd4e83.

📒 Files selected for processing (2)

• internal/chain/hms_ledger.go
• internal/chain/zz_hms_transfers_settle_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The HMS transfer shape validator now rejects sender or destination addresses with surrounding whitespace. Regression tests cover whitespace-padded addresses submitted to the transaction pool and a padded destination inserted directly into the pending pool.

Changes

HMS address validation

Layer / File(s)	Summary
Reject padded transfer addresses internal/chain/hms_ledger.go, internal/chain/zz_hms_transfers_settle_test.go	ValidateHmsTransferShape rejects addresses that differ from their trimmed form, returning invalid_address. Tests cover padded addresses at submission and apply time, including transaction rejection, pool removal, and unchanged sender balance and nonce.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: jokeez

Merge Risk: ⚪ Minimal · up to 6dd4e

No actionable merge-blocking risk is identified for the HMS address whitespace change; it is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 6dd4e

The change is narrow and protective: padded addresses are rejected before funds move. No introduced or worsened security concern was identified. Deployment and failure-recovery evidence remains incomplete, and rollback would restore the previous acceptance behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The affected state is the HMS transaction pool, history, account balances and account nonces. Accepted signed transfers still require a signature bound to the sender, the current nonce and sufficient sender funds; the change adds no signing authority or additional asset lane.

Security Findings and Attack Paths

• observed — The addressed path used a valid signature over trimmed addresses while carrying a padded raw recipient into settlement. The new equality guard rejects that representation before economic mutation, including when a pending row bypasses submission. The regressions explicitly exercise both entry paths; their execution was not independently performed during this review.

Trust Boundaries and Controls

• observed — Simple-sign requests retain the existing loopback/admin gates and node-wallet matching, with shape validation preceding signing. Independently signed requests pass through submission validation, which verifies the signature and exact public-key-derived sender identity. No control is bypassed by the new rejection rule.

Resilience and Maintainability Implications

• observed — Block append and import wrap settlement in SQL transactions with deferred rollback and commit after application. Returned settlement errors therefore have transaction rollback paths. However, rejection-history and cleanup errors are ignored; missing treasury configuration or malformed pending JSON can leave work unprocessed. These behaviors predate the PR, so successful cleanup under every failure mode is not claimed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting HMS transfer addresses with surrounding whitespace.
Description check	✅ Passed	The description explains the problem, fix, scope, known limitations, and test results. It does not use the template’s Summary, Test plan, and Notes headings or checkboxes, but it provides the key info…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Reject HMS transfers with surrounding address whitespace

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Reject padded HMS sender and recipient addresses before submission to prevent credits to
 unreachable accounts.
• Revalidate queued transfers at settlement, rejecting previously accepted padded rows without
 moving funds.
• Add regression coverage for whitespace variants, rejection codes, pool cleanup, and unchanged
 balances.

Diagram

graph TD
  S["Submit transfer"] --> V{"Raw equals trimmed?"} -->|yes| P["Pending pool"] --> A{"Apply validation?"} -->|yes| C["Account balances"]
  V -->|no| R["Invalid address"]
  A -->|no| H["Rejected history"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Normalize addresses before storage

• ➕ Could accept padded wallet input while keeping account keys canonical.
• ➖ Changes accepted transaction behavior and requires consistent normalization across hashing, pooling, history, and settlement.

Recommendation: Keep the shared validation guard. Rejecting noncanonical raw addresses closes the submit and apply-time paths without changing the signed payload or settlement representation; normalization would require a broader transaction-contract change.

Files changed (2) +118 / -6

Bug fix (1) +6 / -0

hms_ledger.goReject HMS transfer addresses with surrounding whitespace +6/-0

Reject HMS transfer addresses with surrounding whitespace

• ValidateHmsTransferShape now returns invalid_address when either raw address differs from its trimmed form. Because submission and settlement both use this validation, padded recipients cannot be credited under an unreachable account key.

internal/chain/hms_ledger.go

Tests (1) +112 / -6

zz_hms_transfers_settle_test.goCover padded-address rejection at submission and settlement +112/-6

Cover padded-address rejection at submission and settlement

• Adds signed-transfer cases for leading and trailing whitespace, including a padded sender, and verifies rejection before pool insertion. A directly seeded padded pool row verifies apply-time rejection, pool removal, no recipient credit, and unchanged sender balance and nonce.

internal/chain/zz_hms_transfers_settle_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[jokeez]

Thanks Bobby — good catch, and nice that you tied it back to the #26 review gap instead of just filing another report.

Merged. The trim-at-sign / credit-raw mismatch is exactly the kind of thing that only shows up once settle is live, so appreciate you re-checking main after #27.

HMS-only scope is fine for now; if you want to mirror the same guard on transfer_v1 / SUP in one small follow-up PR, we’ll take it. Docs for /api/hms/tx/send can wait unless you feel like adding a line in CHAIN_SPEC while you’re there.

Deploying to the hub node next.