Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In recovery, we need to refuse any mnemonic with words out of our supported BIP-39 dictionaries #9050

Closed
yenda opened this issue Sep 26, 2019 · 14 comments · Fixed by #9648
Closed

Comments

@yenda
Copy link
Contributor

yenda commented Sep 26, 2019

Problem

As a user I want to have only BIP39 seedphrases so that I won't be using a memory wallet which is inherently unsafe.

Description

Updated description on 31/10/19. Issue is ready for pick-up. [RH]

#9005 (comment)

We MUST not let the user enter invalid seedphrases (not valid based on BIP39)

The roadmap for this feature:

step 1/ (like Trust wallet does, most minimalist approach) user enters seed, if mnemonic is not correct (some words are not part of dictionary, or the checksum is not ok) then when user validates mnemonic we show a minimalist error message "seed phrase not correct" and the user corrects his sentence with no special indication of what's wrong
step 2/ while a user types a word and it turns out it's not in our supported dictionaries we prompt a warning that this word is not in dictionary
step 3/ we do autocomplete of words

Acceptance Criteria

The scope of this issue is step 1 only.

  • Words are validated against a known dictionary
  • Word count is validated
  • Checksum is validated
  • If the seedphrase entered does not match dictionary, has wrong word count or is not checksum validated, display error copy: Your seed phrase is not correct. Please try again.

Implementation notes

@guylouis
Copy link
Contributor

@dmitryn @andmironov
any design work needed here ?

@yenda
Copy link
Contributor Author

yenda commented Sep 30, 2019

yes we need a design for the auto completion of the words

@guylouis
Copy link
Contributor

guylouis commented Sep 30, 2019

pinging @rachelhamlin too here

given the number of things to complete for v1, we could consider to go for a stepped approach:

  • step 1/ (like Trust wallet does, most minimalist approach) user enters seed, if mnemonic is not correct (some words are not part of dictionary, or the checksum is not ok) then when user validates mnemonic we show a minimalist error message "seed phrase not correct" and the user corrects his sentence with no special indication of what's wrong
  • step 2/ while a user types a word and it turns out it's not in our supported dictionaries we prompt a warning that this word is not in dictionary
  • step 3/ we do autocomplete of words

wdyt ?

@3esmit
Copy link
Member

3esmit commented Oct 1, 2019

Agree with step based. Step 1 is the MVP, Step 2 and Step 3 are UX improvements.

@guylouis
Copy link
Contributor

guylouis commented Oct 1, 2019

This issue is thus about step 1, and will be closed once step 1 is done.

No design work needed.

If words are bad, or checksum is bad, error message can be : "Your seed phrase is not correct, please try again"

Improvements will be provided with #9084

@hesterbruikman
Copy link
Contributor

Additional step here for which I'm curious if it's feasible at all:

step 4/ prevent native on-screen keyboard from auto completing the seed phrase. Currently having the first 2 letters of my phrase is all I need to recover:)

@andmironov
Copy link

Here's the current design. There's a word count and a popup we display after the user hits "Next" if the seed phrase has words that are not in our dictionary

Screen Shot 2019-10-03 at 16 54 07

@flexsurfer flexsurfer self-assigned this Nov 4, 2019
@guylouis guylouis changed the title Only allow BIP39 seedphrase In recovery, we need to refuse any mnemonic with words out of our supported BIP-39 dictionaries Nov 4, 2019
@guylouis
Copy link
Contributor

guylouis commented Nov 4, 2019

changed name of issue to "In recovery, we need to refuse any mnemonic with words out of our supported BIP-39 dictionaries" to distinguish it from newly created #9393

@flexsurfer
Copy link
Member

after discussion with @gravityblast we decided to implement validation in status-go status-im/status-go#1663

@StatusSceptre
Copy link
Member

StatusSceptre commented Nov 28, 2019

Can I close this one @flexsurfer? Or do we need to build the error message?

@flexsurfer
Copy link
Member

no, because we still need to make changes in status-react after status-go part will be ready

@acolytec3
Copy link
Contributor

@flexsurfer once we finalize my PR, is the work on the status-react side to replace all of the phrase validation related functions in ethereum.mnemonic with a single call to the ValidMnemonic in status-go since that validates word length, words being in the dictionary, and checksum?

@flexsurfer
Copy link
Member

probably we could still validate length in status-react, and if the length is valid, then validate in status-go

@acolytec3
Copy link
Contributor

@flexsurfer Understood. I'll work up a local branch and see if I can get it to call the new Go method. Not 100% sure I've got everything set up right on the status-go side but we'll see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants