fix: throw on invalid entropy in Wallet.fromEntropy and generateSeed (4.x backport) - #3487
cybele-ripple wants to merge 2 commits into
Conversation
Backport of #3469 (xrpl 5.2.0, ripple-keypairs 3.1.0) to the 4.x line. - Wallet.fromEntropy validates entropy instead of coercing it. A string used to be coerced element-by-element, so every letter became 0 and Wallet.fromEntropy('abcdefghijklmnop') returned a spendable wallet derived from 16 zero bytes, with no error. - generateSeed requires exactly 16 bytes instead of truncating anything longer, so inputs sharing a prefix no longer derive the same wallet.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: XRPLF/xrpl.js/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a faithful, byte-identical backport of an already-shipped and reviewed upstream fix (#3469) into the 4.x maintenance line, with comprehensive regression tests. I reviewed the entropy validation logic in both ripple-keypairs/src/index.ts and xrpl/src/Wallet/utils.ts (the isUint8Array type guard, length checks, sparse-array/hole handling, ?? vs falsy-value semantics for entropy, and the wiring of validateEntropy into Wallet.fromEntropy) and did not find correctness or security issues introduced by this diff. The falsy-vs-nullish distinction in generateSeed (supplied === undefined guard plus ??) is intentional and correctly tested, and the by-index (non-iterator) array reads correctly defeat hostile @@iterator overrides as the tests verify. No changed lines warrant a blocking comment.
RIPPLED_DOCKER_IMAGE floated on rippleci/rippled:develop, so this frozen branch tests against whatever rippled happens to be today. That tag has drifted twice since: - rebuilt on 2026-05-21 to run as the non-root `xrpld` user, so `mkdir -p /var/lib/rippled/db/` fails with "Permission denied", the container exits, and all 144 integration and browser tests fail to connect rather than failing an assertion - `server_definitions` gained five top-level keys, which serverDefinitions.test.ts asserts against exactly Pin to 24cbaf76, the develop build from 2026-02-24. Verified locally against this branch's .ci-config with the unchanged docker command: it starts, a 4.x Client connects on ws 6006, `server_definitions` returns the six keys the test expects, and serverDefinitions.test.ts passes. Tagged releases are not an option here: 3.1.0 and 3.1.1 both reject this .ci-config with "Unknown feature: PermissionDelegationV1_1", and 2.5.0 rejects it with "Unknown feature: LendingProtocol". Matrices also default to fail-fast, which cancelled all three integration legs once one failed. Turn that off so each leg reports.
There was a problem hiding this comment.
This is a careful, byte-for-byte backport of an already-shipped upstream fix (ripple-keypairs 3.1.0 / xrpl 5.2.0) to the 4.x maintenance line. I traced the new validateEntropy/isUint8Array logic in both packages/ripple-keypairs/src/index.ts and packages/xrpl/src/Wallet/utils.ts, the generateSeed length/type assertions, and the Wallet.fromEntropy call site, against the extensive new test suites (sparse arrays, hostile @@iterator, falsy/null entropy, over/under-length input). The logic is internally consistent: entropy is read by index (not iteration), holes read back as undefined and are rejected, length is checked before any iterator could be invoked (so the non-terminating-iterator test can't hang), and supplied ?? randomBytes(...) is only reachable once supplied has already been validated as undefined or a correctly-sized Uint8Array. The CI workflow changes (pinning the rippled docker tag, adding fail-fast: false) are well-justified by the inline comment and are low-risk operational changes. I did not find any correctness, security, or resource-management issues introduced by this diff.
This branch has not been deployed
2 participants
High Level Overview of Change
Backport of #3469 (released in xrpl 5.2.0 and ripple-keypairs 3.1.0) to the 4.x line.
Wallet.fromEntropynow validates entropy instead of coercing itgenerateSeed(ripple-keypairs) now requires exactly 16 bytes instead of truncating anything longerContext of Change
Problem
fromEntropypassed input throughUint8Array.from(), which accepts any iterable. Strings were coerced viaNumber()— letters becameNaN, stored as0Wallet.fromEntropy('abcdefghijklmnop')returned a real, spendable wallet derived from 16 zero bytes, with no errorgenerateSeedaccepted any entropy>= 16bytes and sliced to the first 16, so inputs sharing a prefix derived the same wallet (entropy.slice(0, 16) silently truncates user-provided entropy in generateSeed #3286)Why backport
The 4.x line is 59% of weekly
xrpldownloads (195k) and ripple-keypairs 2.x is 60% of weeklyripple-keypairsdownloads (228k). Only the 5.x/3.x lines have the fix today, and those are 2.8% and 3.2% respectively. Both halves are needed: the keypairs fix alone does not close the string bug, becauseUint8Array.from('abcdefghijklmnop')still produces a valid 16-byte (all-zero) array that a fixedgenerateSeedaccepts.Changes
Ported unchanged from #3469 — the
validateEntropy/isUint8Arrayblock inWallet/utils.ts, thefromEntropymethod, and the added test cases are byte-identical to that PR:Uint8Arrayor an array of byte valuesArray.prototype.everyskips holes, sonew Array(16)andArray(16).map(...)previously passed validation and read back as zero bytesDeliberately not ported: this line's
generateSeeddefaults tosecp256k1(options.algorithm === 'ed25519' ? 'ed25519' : 'secp256k1');mainflipped that default in ripple-keypairs 3.0.0. That line is untouched — changing the default signing algorithm in a maintenance release would be a far worse break than the one being fixed here. TheVALID_ALGORITHMSassertion present onmainis likewise absent, as it predates #3469 and was never part of it.Versions are unbumped and the HISTORY entries sit under
## Unreleased, matching what #3469 did. At release time this cannot take a major, so it goes out as ripple-keypairs 2.1.0 and xrpl 4.7.0, and must not publish under thelatestdist-tag orxrpl@latestmoves backwards from 5.3.0.Second commit: CI repair
This branch's CI could not run at all, so the entropy commit above could not be verified by it.
RIPPLED_DOCKER_IMAGEfloated onrippleci/rippled:develop, and that tag drifted twice since this branch was cut:xrplduser, somkdir -p /var/lib/rippled/db/fails withPermission denied, the container exits, and all 144 integration and browser tests fail to connect rather than failing an assertionserver_definitionsgained five top-level keys, whichserverDefinitions.test.tsasserts against exactlyThe second commit pins the image to
24cbaf76, the 2026-02-24 develop build (3.2.0-b0) closest to this branch's 2026-02-12 release, and setsfail-fast: falseso one matrix leg failing stops cancelling the others. Thedocker runcommands are unchanged — that build runs as root and reads/etc/opt/ripple/rippled.cfgby default.Tagged releases are not usable here:
3.1.0and3.1.1both refuse this branch's.ci-configwithUnknown feature: PermissionDelegationV1_1, and2.5.0refuses it withUnknown feature: LendingProtocol.Verified green on this exact change before it was folded in here: all 9 checks passed,
integrationandbrowserincluded.Type of Change
Did you update HISTORY.md?
Test Plan
18 regression test cases ported verbatim from #3469, covering strings, hex strings, sparse arrays, wrong lengths, non-byte values, a caller-supplied
@@iterator, and that no malformed input derives the known zero-entropy wallet.Run on this branch:
npm run build— cleanpackages/xrpl: 1038 tests, 116 suites passingpackages/ripple-keypairs: 28 tests, 4 suites passingnpm run lintclean in both packagesBehavioural check against the built output:
Wallet.fromEntropy(new Uint8Array(16))andWallet.fromEntropy(new Array(16).fill(0))still deriver9zRhGr7b6xPekLvT6wP4qNdWMryaumZS7— valid derivation is unchangedWallet.fromEntropy('abcdefghijklmnop'),new Array(16), and 32 bytes all throwValidationErrorgenerateSeed({entropy: <16 bytes>})returns the same seed as before the change; 32 bytes now throws;generateSeed()still returns a random seed