Validate mnemonic whitespace #59
Conversation
|
|
||
| // bip39 lib doesn't properly validate whitespace so we have to do that manually. | ||
| if expected := strings.Join(strings.Fields(m), " "); m != expected { | ||
| return nil, errWhitespace |
There was a problem hiding this comment.
why not simply normalize the input instead? returning the error would be confusing for user
they then have to visually find where the extra white space is?
There was a problem hiding this comment.
IMHO this is less error-prone. if there's extra whitespace, then the user did something wrong, likely a copy and paste error. i think proceeding to generate a key in spite of this is a bad idea. the user should be aware of the error and should correct it. the BIP39 spec, such as it is, does not allow extra whitespace.
There was a problem hiding this comment.
but then if there are sure of it (extra whitespace in btwn phrases), they will never be able to use this phrase.
should print the exact diff to user and prompt them to confirm then.
There was a problem hiding this comment.
in the interest of time, instead of creating more code, maybe just ask user to use the phrase smcli created
There was a problem hiding this comment.
@countvonzero I think this code should be as simple as possible. I don't think it's the responsibility of the wallet app to correct the user's mistake. At most we could link them to a doc that explains the issue and how to correct it, something along those lines.
There was a problem hiding this comment.
then the phrase is wrong and they should use one that's valid. and they should be made aware that it's wrong.
There was a problem hiding this comment.
I have a related but different question. Whitespace is more than just spaces, does bip39 handle things like \r, \n and \t correctly?
There was a problem hiding this comment.
@fasmat all whitespace is treated the same way. this change will complain about \r, \n, and \t as well, see https://pkg.go.dev/strings#Fields. the regression test checks things other than spaces. the only thing that's permitted is a single space between words.
There was a problem hiding this comment.
Sorry, didn't see the other whitespace characters in the test. Then LGTM
Closes #58
Print an error and refuse to generate keys if a whitespace violation is detected in an input mnemonic