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

Passphrase spaces #86

Merged
merged 3 commits into from
Nov 16, 2018
Merged

Passphrase spaces #86

merged 3 commits into from
Nov 16, 2018

Conversation

Daniel-Wang
Copy link
Contributor

Priority

High

Description

  • Properly parses mnemonic for spaces
  • Tested against 12 and 24 length mnemonics

@Daniel-Wang
Copy link
Contributor Author

Fixes #74

@@ -116,7 +162,7 @@ public void backwards_compatibility_test_login() {
CipherWrapper cipherWrapper = new CipherWrapper("RSA/ECB/PKCS1Padding");

assert masterKey != null;
String encryptedPhrase = cipherWrapper.encrypt(mnemonic + " " + passPhrase, masterKey.getPublic(), false);
String encryptedPhrase = cipherWrapper.encrypt(mnemonic12 + " " + passPhrase, masterKey.getPublic(), false);
Copy link
Collaborator

@dabitdev dabitdev Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not he old way of encrypting mnemoni + passphrase?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should u write a new test for the passphrase encryption only?
cipherWrapper.encrypt(passPhrase)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this entire problem only exists in the old wallet lol, cause we are using a space separating the mnemonic and passphrase :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what line is that cipherWrapper.encrypt(passPhrase)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never-mind, I got confused

@@ -103,7 +103,7 @@ class ShowMnemonicActivity : BaseActivity(), View.OnClickListener {
}

mnemonicString = String(mnemonic)
return String(mnemonic).split(" ".toRegex()).dropLastWhile { it.isEmpty() } as ArrayList
return mnemonicString!!.split(" ".toRegex()).dropLastWhile { it.isEmpty() } as ArrayList
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you put !!? it looks like mnemonicString can not be null since is assigned to a new String object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kotlin's compiler apparently is not perfect and they don't allow it :(

@dabitdev
Copy link
Collaborator

Is the passphrase with spaces still not fixed for old wallets?
If yes, we have to expose that It will be a known limitation. (Release notes, FAQ)

@Daniel-Wang
Copy link
Contributor Author

This PR fixes passphrase only for old wallets, because in new wallets the mnemonic and passphrase are saved in two different strings. But since in old wallets they are space separated, we may have issues. So this addresses it

@dabitdev
Copy link
Collaborator

Let's add in this version support for spaces in the passphrase

@dabitdev dabitdev merged commit 119f92d into dev Nov 16, 2018
@Daniel-Wang Daniel-Wang deleted the passphraseSpaces branch November 16, 2018 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants