[Merged by Bors] - Reject invalid utf-8 characters during encryption #1928
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Kirk Baird <baird.k@outlook.com>
kirk-baird
added
A0
bug
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
Signed-off-by: Kirk Baird <baird.k@outlook.com>
paulhauner
approved these changes
Nov 19, 2020
Member
paulhauner
left a comment
paulhauner
left a comment
There was a problem hiding this comment.
Glorious!
we should allow them during decryption since either the keystore was created
I agree, I don't see the harm in decrypting using invalid chars.
bors r+
paulhauner
added
ready-for-merge
bors bot
pushed a commit
that referenced
this pull request
Nov 19, 2020
## Issue Addressed Closes #1889 ## Proposed Changes - Error when passwords which use invalid UTF-8 characters during encryption. - Add some tests ## Additional Info I've decided to error when bad characters are used to create/encrypt a keystore but think we should allow them during decryption since either the keystore was created - with invalid UTF-8 characters (possibly by another client or someone whose password is random bytes) in which case we'd want them to be able to decrypt their keystore using the right key. - without invalid characters then the password checksum would almost certainly fail. Happy to add them to decryption if we want to make the decryption more trigger happy 😋 , it would only be a one line change and would tell the user which character index is causing the issue. See https://eips.ethereum.org/EIPS/eip-2335#password-requirements
|
Pull request successfully merged into master. Build succeeded: |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Closes #1889
Proposed Changes
Additional Info
I've decided to error when bad characters are used to create/encrypt a keystore but think we should allow them during decryption since either the keystore was created
Happy to add them to decryption if we want to make the decryption more trigger happy 😋 , it would only be a one line change and would tell the user which character index is causing the issue.
See https://eips.ethereum.org/EIPS/eip-2335#password-requirements