fix(abstract-utxo): reject invalid chain and index values - #9401
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR tightens fixed-script (2-of-3 multisig) address derivation handling in @bitgo/abstract-utxo by rejecting invalid derivation indexes (negative or non-integer) instead of silently normalizing them to 0, and adds regression tests to prevent reintroduction of the issue.
Changes:
- Enforce
indexto be a non-negative integer ingenerateAddress. - Enforce
indexto be a non-negative integer inassertFixedScriptWalletAddress. - Add unit coverage for invalid/valid index edge cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| modules/abstract-utxo/src/address/fixedScript.ts | Adds stricter validation for derivation index during generation and validation. |
| modules/abstract-utxo/test/unit/address-index-edge.test.ts | Adds regression tests ensuring invalid indexes are rejected and index 0 remains valid. |
Suppressed comments (1)
modules/abstract-utxo/src/address/fixedScript.ts:169
assertFixedScriptWalletAddressstill allows non-integerchainvalues to pass the initial validation (_.isFinite(chain)), andgenerateAddresswill then ignore such values and fall back to the default chain. To prevent validating an address against mismatched derivation metadata, requirechainto be an integer as well.
if (
(_.isUndefined(chain) && _.isUndefined(index)) ||
!(_.isFinite(chain) && _.isFinite(index)) ||
!_.isInteger(index) ||
(index as number) < 0
) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c8ab32e to
461f61c
Compare
461f61c to
7e9f618
Compare
7e9f618 to
38a13e2
Compare
|
Hi BitGo team, this is an external contribution. The commit is GPG-signed and GitHub reports the signature as verified and valid, but the Validate Humans In The Loop check reports that my key is not in BitGo's internal engineer allowlist. Could a maintainer advise on the expected process for external contributors, approve the pending workflows, and review the change? The requested chain validation update has also been implemented, and the targeted tests, formatting, lint, and build all pass locally. |
Description
Reject invalid fixed-script derivation chain and index values instead of silently normalizing them to default values.
Previously:
This could allow invalid derivation metadata to validate against an address derived with different parameters.
This change requires explicitly supplied chain and index values to be valid integers. Indexes must also be non-negative.
Tests
Added regression coverage confirming that:
Validation
git diff --checkyarn workspace @bitgo/abstract-utxo check-fmtyarn workspace @bitgo/abstract-utxo lintyarn workspace @bitgo/abstract-utxo buildyarn workspace @bitgo/abstract-utxo mocha test/unit/address-index-edge.test.ts— 7 passing