bip39 icon indicating copy to clipboard operation
bip39 copied to clipboard

mnemonicToSeed should throw on non-string input

Open dcousens opened this issue 6 years ago • 4 comments

It should probably additionally throw on empty string? But maybe someone has a use case there...

Additionally:

  • [ ] enforce no duplicate whitespace
  • [ ] enforce non-zero length
  • [ ] enforce only printable characters

dcousens avatar Apr 27 '18 02:04 dcousens

IMO NACK... this is not something the library should be throwing on.

The application should decide whether to throw or not on zero length / multiple whitespace etc.

junderw avatar Apr 27 '18 09:04 junderw

@junderw then let us provide an "unsafe" option.

mnemonicToSeedUnsafe

The advanced user needs protection too.

dcousens avatar Apr 27 '18 09:04 dcousens

concept ACK - it's so easy for whitespace to be introduced, and then the mnemonic input is directly hashed.. It can be pretty easy to miss the typo 'abandon abandon abandon' and 'abandon abandon abandon' when it's a large block.

The whitespace isn't covered by the checksum, and if we allow users to make this mistake, we can expect people to panic when the recover from a seed that was transcribed to paper..

Empty string is forbidden by BIP39 - the 'recommended sizes' are the only acceptable ones it turns out, so a zero length mnemonic is invalid :)

afk11 avatar Apr 27 '18 13:04 afk11

the 'recommended sizes' are the only acceptable ones it turns out, so a zero length mnemonic is invalid :)

AFAIK, validateMnemonic should make that distinction, not this function. This function should support non-strict BIP39 mnemonics, but I think the above error cases are common enough to hurt both types of user.

dcousens avatar Apr 27 '18 14:04 dcousens