Show a warning if passphrase contains extra whitespace

[mackuba]

Hi!

I've noticed that a lot of people are having trouble logging in to their existing address, because they enter the passphrase with extra whitespace somewhere, they get a different empty address and they freak out that their money is gone, e.g: https://www.reddit.com/r/Lisk/comments/73n57o/help_lisk_is_gone/. This is really something that the app should prevent from happening at all... so I decided to do something about it.

One tricky thing here is that some people might have already entered the whitespace wrong when creating a wallet, so if we add validation that prevents them from logging in with a bad passphrase, they won't be able to get back to their wallet at all... so instead I added a 'warning' to the passphrase input component that is displayed just like an error, but isn't passed outside as an error, so it doesn't disable the login button. You're still able to log in with such incorrect passphrase, if you really need to, but it will be obvious to you that something is wrong, if that's not your intention.

I'm not sure how well I managed to do this in terms of proper React architecture... I have basically zero experience with anything in JavaScript that came after the ES5/jQuery/Backbone era. But it seems to be working 🙂 I couldn't figure out how to write a test though, I couldn't find any way to access the component's state with a warning field through the wrapper.

So it probably needs some more work, but I hope you'll be able to do something with this and merge it.

[alepop]

Good! But I think that the better way is to remove this kind of problem at the root. Trim passphrase in the component. something like this:

"   wagon stock borrow episode laundry kitten salute   link globe zero feed marble  "
.trim().split(' ').filter(a => !!a).join(' ')

And we have a valid passphrase with removed extra spaces.

[mackuba]

That would be the right way to do it if you did it like this from the start... However, if you check that thread, this specific person had to add an extra whitespace to the passphrase to be able to log in, because they had a whitespace there when they created the wallet. And if they log in with no extra whitespace, they go into an empty wallet instead. So if you always auto-trim the passphrase now, they will have no way to log in to their actual wallet... So I think what I've done is probably the best way to do it if you want to be backward compatible.

[alepop]

@mackuba indeed.

[reyraa]

Mnemonic - as you can see here - creates an array of existing words then joins them with space to create the passphrase. I don't think the space at the end was that guy's actual issue there.
@alepop I like your idea, TBH, I thought we already do it.

[mackuba]

It's possible that e.g. he copied the generated passphrase somewhere, didn't send any LSK yet; then logged in again copying or retyping that saved passhprase, but added a space by accident, and got into a different address, but didn't notice it (since he expected it to be empty anyway), and sent coins to that address. Then on another day tried logging in again, but not adding the extra space this time, and saw an empty wallet.

[slaweet]

AFAIK space at the end of the passphrase has no effect on the generated seed. You can try adding trailing whitespace on this tool: https://iancoleman.github.io/bip39/ and see that the BIP39 Seed doesn't change.

While I don't see how could happen the issue to the user from Reddit, I like this PR.

I have just one improvement suggestion - give a more instructive message in this case:
Passphrase contains extra whitespace between words 'wagon' and 'stock'

[reyraa]

@mackuba it's impossible, since we use mnemonic to generate the publicKey.
Please,
Simply perform a single check using test method (Since you don't use the resulting string from the heavier replace method) and return a proper error . I recommend doing it after passphrase length check.
thanks

[mackuba]

it's impossible, since we use mnemonic to generate the publicKey.

That's how I expected it to work, since that would make perfect sense, but from what I could find, the passphrase is being passed to SHA256 as a single string, without even trimming it:

• passphrase is passed to Lisk.crypto.getKeys:

https://github.com/LiskHQ/lisk-nano/blob/1e2e9fa9e7db3b2a03ac5b7e0edb58b3da4e32dd/src/utils/api/account.js#L42

• secret (passphrase) is passed to some function calls that (as I understand) just run SHA256 on the passed string as is and then use the result to create a private and public key (and same for getAddress() below):

https://github.com/LiskHQ/lisk-js/blob/master/lib/transactions/crypto.js#L503-L511

As far as I can tell, the mnemonic (and trim() for that matter) is used only for verifying if the entered words are correct (isValidPassphrase, getPassphraseValidationError), but after pressing submit the passphrase is passed further as one string again.

Do I not understand correctly how this works?

Testing logging in with various combinations of whitespace:

"wagon stock borrow episode laundry kitten salute link globe zero feed marble"

• address = 16313739661670634666L

" wagon stock borrow episode laundry kitten salute link globe zero feed marble"

• address = 2225483298949637218L

"wagon stock borrow episode laundry kitten salute link globe zero feed marble "

• address = 694512234697342214L

[slaweet]

@mackuba thank you for the extensive explanation. Now I see the problem.

There is one eslint error in Jenkins that prevents merge:

src/components/passphraseInput/index.js
  29:11  error  'warningMessage' is never reassigned. Use 'const' instead  prefer-const

[mackuba]

Yeah, I got it in the stdout log now too, not sure why I missed it previously. I've applied the fix and rebased again and forced-pushed to that branch.

[slaweet]

Thank you @mackuba

[mackuba]

👍

[mackuba]

That's what I meant about keeping it backwards compatible by making it a warning, not an error: https://www.reddit.com/r/Lisk/comments/78glpw/new_release_lisk_nano_120_improved_core/dou1mr7/. Looks like someone changed it later to be an error: 1727b3e