bitcoinjs / bip39

JavaScript implementation of Bitcoin BIP39: Mnemonic code for generating deterministic keys
ISC License
1.11k stars 447 forks source link

mnemonicToSeed should throw on non-string input #77

Open dcousens opened 6 years ago

dcousens commented 6 years ago

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

Additionally:

junderw commented 6 years ago

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.

dcousens commented 6 years ago

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

mnemonicToSeedUnsafe

The advanced user needs protection too.

afk11 commented 6 years ago

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 :)

dcousens commented 6 years ago

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.