Skip to content

fix: reject malformed entropy hex strings - #2

Open
1507819106zxzx-crypto wants to merge 1 commit into
BitgesellOfficial:mainfrom
1507819106zxzx-crypto:fix-strict-entropy-hex-validation
Open

fix: reject malformed entropy hex strings#2
1507819106zxzx-crypto wants to merge 1 commit into
BitgesellOfficial:mainfrom
1507819106zxzx-crypto:fix-strict-entropy-hex-validation

Conversation

@1507819106zxzx-crypto

Copy link
Copy Markdown

Summary

  • Reject odd-length and non-hex entropy strings before Buffer.from(..., 'hex') can silently truncate input.
  • Preserve the existing Invalid entropy error surface for malformed string entropy.
  • Add regression tests for odd-length, non-hex, and empty entropy strings.

Verification

  • npx tsc -p tsconfig.json
  • npx prettier ts_src/index.ts --check
  • npm run lint
  • npm run unit (386 passing)
  • git diff --check

Note: the repository's npm test script uses rm -rf in the clean step, which does not run on this Windows shell; the equivalent build/check/test steps above passed locally.

Related bounty: BitgesellOfficial/bitgesell#39
Payout address if approved: 0x4451dF3D21925eF7a62D20eEdc80B99f7140C5D2

@MyTH-zyxeon

Copy link
Copy Markdown

Maintainers - this looks like a clean, narrow bounty-review candidate for the Bitgesell PR bounty lane (BitgesellOfficial/bitgesell#39, and adjacent improvement-program tracking in #81).

What I verified from the visible diff:

  • PR is OPEN / MERGEABLE / CLEAN at head 8ec56d40bab6ec659cb3b331d855d41e8e58731b with no prior visible comments or reviews.
  • The code change is limited to entropyToMnemonic() in both ts_src/index.ts and the generated src/index.js, plus focused tests.
  • The edge case is real: Buffer.from(value, 'hex') can silently normalize malformed hex-like strings before the later entropy-size validation, so rejecting odd-length, non-hex, and empty string inputs before conversion is the right boundary.

Suggested acceptance checks before merge:

  1. Re-run the author's focused validation (npx tsc -p tsconfig.json, npm run lint, npm run unit) so ts_src/index.ts and generated src/index.js remain in sync.
  2. Confirm existing valid entropy vectors still round-trip for the standard BIP39 entropy lengths (128/160/192/224/256 bits), including lowercase and uppercase hex input.
  3. Keep the public error contract as the existing TypeError: Invalid entropy; the PR already does this, which should avoid downstream API churn.
  4. Treat the empty-string rejection as intentional, since zero-length entropy is not a valid BIP39 entropy size.

From a maintainer-review perspective, this is a low-risk correctness/test PR: small diff, concrete malformed-input regression coverage, no wallet/key generation, no live node/RPC behavior, and no broad package metadata churn.

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.

3 participants