[PM-22028] fix(toolkit): enforce EOF on untagged CLI hex decode (audit Issue H)#1437
Conversation
1259c20 to
27da62c
Compare
Switch coin_public_decode and contract_address_decode from hex_ledger_untagged_decode to hex_ledger_decode (tagged), eliminating the unconditional fallback that accepted untagged input for types that implement Tagged. - Replace hex_ledger_untagged_decode with hex_ledger_decode in coin_public_decode and contract_address_decode - Add EOF enforcement to hex_ledger_untagged_decode (defense-in-depth) - Update test fixture in generate_txs.rs to use tagged hex format - Add 11 unit tests covering tagged/untagged/wrong-tag/trailing-bytes/ invalid-hex scenarios Closes #307 Signed-off-by: Mike Clay <mike.clay@shielded.io>
27da62c to
1c53d03
Compare
|
/bot rebuild-metadata |
|
✅ Metadata rebuild complete. No changes detected. |
There was a problem hiding this comment.
These should stay as untagged: https://github.com/midnightntwrk/midnight-architecture/blob/main/adrs/0022-when-to-use-tagged-serialization.md#adopt-tagged-serialization-in-most-cases-but-use-untagged-one-for-wallet-keys-and-addresses-as-well-as-couple-other-types-which-behave-like-keys-or-addresses
We could update this PR to add a comment to that effect - this change was made and reverted in a PR before, we should add some comment to prevent it happening again!
Revert coin_public_decode and contract_address_decode to use hex_ledger_untagged_decode. Per ADR-0022 wallet keys and addresses use untagged serialization; Bech32m HRP plays the role of a tag at the user-facing boundary. The same tagged-switch was attempted and reverted in PR #853; inline comments above both parsers cite ADR-0022 and PR #853 to prevent future re-introduction. EOF enforcement on hex_ledger_untagged_decode (the audit-#307 fix) and the improved error-message wording ("invalid hex input") are retained. The unit tests are rewritten to assert untagged-acceptance plus EOF / truncation / invalid-hex rejection on coin_public_decode, contract_address_decode, and hex_ledger_untagged_decode::<HashOutput>. The tagged test fixture (contract_address_undeployed_tagged.mn) is removed and generate_txs.rs reverts to the existing untagged fixture. Closes #307 Signed-off-by: Mike Clay <mike.clay@shielded.io>
|
@ozgb — addressing your review feedback in commit 4329d0b. Summary of changes (per your review comment):
Verification: The PR description has been updated to reflect the EOF-only scope. Full re-plan analysis (architectural reasoning, what stays vs. reverts, open questions) is in the engineering planning folder: Ready for another look when you have time. |
Summary
Address audit Issue H (#307) by enforcing EOF on the untagged hex-decode path in the toolkit CLI. This closes the silent-fallback ambiguity surface (trailing bytes after a valid parse were tolerated and could enable type re-interpretation) without changing the wire format.
coin_public_decodeandcontract_address_decodekeep their untagged decoding, which is the format prescribed by ADR-0022.🎫 Ticket 📐 Engineering
Motivation
The toolkit CLI accepts hex-encoded inputs for ledger types (contract addresses, coin public keys, hash outputs). The shared decoder previously tolerated trailing bytes after a successful parse, allowing subtly malformed inputs to be silently accepted and re-interpreted as a different shape than the user intended. The Least Authority audit (Issue H) flagged this silent-fallback behaviour as an ambiguity attack surface.
The first iteration of this PR mitigated by switching the two CLI parsers from untagged to tagged decoding. Review feedback (ozgb, 2026-05-01) flagged that change as a violation of ADR-0022, which prescribes untagged serialization for wallet keys, contract addresses, and types behaving like keys/addresses (Bech32m HRP carries the type at the user-facing boundary). The same change was previously attempted and reverted in PR #853. This revision restores the architecturally-correct scope: EOF enforcement on the untagged path is the defense-in-depth fix.
Changes
hex_ledger_untagged_decode— Adds EOF enforcement: trailing bytes after a successful deserialize are rejected with a clear error message indicating how many extra bytes were found. This is the central audit hardening.coin_public_decode/contract_address_decode— Keep their existing untagged decoding (no wire-format change), with a new inline comment citing ADR-0022 + PR revert tagged serialization for contract addresses and coin public #853 to prevent future re-introduction of the tagged switch."failed to parse seed"→"invalid hex input"in the generic hex decode helpers (cosmetic, kept from the original PR).hex_ledger_untagged_decode, and invalid-hex / truncated-input rejection.res/test-contract/contract_address_undeployed_tagged.mn(added by the original PR; no longer used after revert).📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging
Closes #307