Skip to content

fix: skip re-sanitization of already-sanitized UTXOs in fetchAndSanit… - #15

Open
ipseonet-dev wants to merge 2 commits into
syscoin:masterfrom
ipseonet-dev:fix/wif-signing-double-sanitization
Open

fix: skip re-sanitization of already-sanitized UTXOs in fetchAndSanit…#15
ipseonet-dev wants to merge 2 commits into
syscoin:masterfrom
ipseonet-dev:fix/wif-signing-double-sanitization

Conversation

@ipseonet-dev

Copy link
Copy Markdown

Summary

Fixes #14

When utxos are passed in already sanitized (e.g. when using WIF signing without a Signer), fetchAndSanitizeUTXOs was incorrectly re-sanitizing them via sanitizeBlockbookUTXOs.

This caused txId to be silently lost because sanitizeBlockbookUTXOs reads utxo.txid (lowercase) which is undefined on already-sanitized objects that use camelCase txId.

Error

Invalid arguments for Psbt.addInput. Requires single object with at least [hash] and [index]

Fix

Detect already-sanitized UTXOs by checking for the presence of a .utxos array with txId fields, and skip re-sanitization in that case.

Testing

Verified by successfully building and broadcasting a syscoinBurnToAssetAllocation transaction using WIF signing on Tanenbaum testnet.
TXID: f131e672f31ef490c3e92cd78ee844abae711ff3d66b5383121d8e6e90b1e349

@sidhujag

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 1f58ff8c18

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@sidhujag

Copy link
Copy Markdown
Member

@Ipseonet can you describe your organization and how you use it, who are your clients?

@sidhujag

sidhujag commented Aug 6, 2026

Copy link
Copy Markdown
Member

@ipseonet-dev

The new check decides the entire UTXO list is sanitized solely because the first entry has txId. I reproduced three failures:

  • A sanitized first entry plus raw second entry skips normalization; transaction creation fails with INVALID_AMOUNT.
  • A zero-confirmation UTXO bypasses excludeZeroConf.
  • An unrequested asset UTXO bypasses the asset whitelist.

Those normally fetch by xpub/address and still sanitize Blockbook results. However, the validation bypass could become exploitable in any consumer accepting caller-controlled explicit UTXOs.

The proper fix is to make sanitization idempotent per UTXO—normalizing every entry and always applying current confirmation/asset policy. Merely checking every entry for txId would still bypass policy filtering.

Also:

  • All 478 existing tests pass, but none cover this new behavior.
  • The PR contains trailing whitespace and fails git diff --check.
  • Add raw, sanitized, mixed-order, partial, empty, zero-confirmation, and asset-whitelist tests.

sanitizeBlockbookUTXOs assumed every entry was a raw Blockbook record. Passing
an already-sanitized object back in (as callers do when supplying explicit
UTXOs) read utxo.txid on an object that only has txId, dropping the txId and
breaking PSBT construction for WIF signers (syscoin#14).

The previous approach guarded the call site and skipped sanitization when the
first entry looked sanitized. That decided the whole list from one entry and
bypassed policy filtering. Instead, normalize each entry independently and
always apply confirmation and asset policy:

- accept txId or txid, and BN or string values, per entry
- carry confirmations through so excludeZeroConf re-applies on later passes
- copy an already-sanitized assets Map through instead of re-parsing it as raw
  Blockbook records, which corrupted the guid key, maxsupply and precision
- treat an empty assets Map as absent so it is not read as a legacy top-level
  assets collection

Sanitizing is now idempotent: sanitize(sanitize(x)) deep-equals sanitize(x),
and mixed raw/sanitized lists are fully normalized and fully policed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hNSHs6jMGmNSvTJ5wzr1t
@ipseonet-dev
ipseonet-dev force-pushed the fix/wif-signing-double-sanitization branch from 1f58ff8 to 6f28082 Compare August 6, 2026 09:08
@ipseonet-dev

Copy link
Copy Markdown
Author

Thanks for the detailed review — all three reproductions were correct, and I've reworked the PR along the lines you suggested. Force-pushed as 6f28082, rebased onto current master (795debf).

I confirmed each failure against the old patch before changing anything. The guard decided the whole list from utxos.utxos[0].txId, so with a sanitized first entry: a raw second entry stayed raw (string value, no txId), a zero-conf entry bypassed excludeZeroConf, and a non-whitelisted asset UTXO bypassed the whitelist. Agreed that 2 and 3 are validation bypasses rather than just correctness bugs, and that checking txId on every entry would still skip policy filtering.

While making sanitization idempotent I hit a fourth failure worth flagging. Re-sanitizing an object whose assets is already a Map corrupts it: Map.forEach yields (value, key), so the code reads asset.assetGuid / asset.maxSupply off an internal assetObj. The map comes back keyed null with maxsupply: 0 and precision lost, and because hasTopLevelAssets is true while the guid no longer matches, every asset UTXO is dropped.

The fix normalizes each entry independently and always applies confirmation and asset policy:

  • accept txId or txid, and BN or string values, per entry
  • carry confirmations through the output so excludeZeroConf re-applies on later passes — without this an already-sanitized entry can't be conf-filtered at all
  • copy an already-sanitized assets Map through instead of re-parsing it as raw Blockbook records
  • treat an empty assets Map as absent, so it isn't mistaken for a legacy top-level assets collection

sanitize(sanitize(x)) now deep-equals sanitize(x), and mixed raw/sanitized lists are fully normalized and fully policed.

On the other points: the call-site guard in index.js is gone entirely (current master already sanitizes unconditionally there, so the diff is now confined to utils.js plus tests), and the trailing whitespace is gone — git diff --check is clean.

New test/sanitize-idempotency.test.js adds 38 assertions covering raw, already-sanitized, mixed in both orders, partial/empty/bare-array, zero-confirmation, asset-whitelist, and legacy + modern asset metadata across passes. Full suite is 555/555. To check the tests aren't vacuous, reverting utils.js alone produces 8 failures spanning all four issues.

@sidhujag

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f28082d08

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread utils.js Outdated

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 3bbe64bb37

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@sidhujag

Copy link
Copy Markdown
Member

@ipseonet-dev pls answer #15 (comment)

@ipseonet-dev

Copy link
Copy Markdown
Author

@Ipseonet can you describe your organization and how you use it, who are your clients?

No clients. Single member entity incorporated in NM I am paying into. I have a more public facing site I am offering basic website SaaS. Aetolux.com
I have full time employment so I'm slowly building this up on my time off for the most part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

syscoinBurnToAssetAllocation fails with WIF signing — txId stripped during double sanitization

2 participants