search

bitpay / bitcore-mnemonic

BIP39 Mnemonics implemented for Bitcore
http://bitcore.io
MIT License
155 stars 212 forks source link

[BUGFIX] Fix major utf8 bug #39

Closed dabura667 closed 8 years ago

dabura667 commented 8 years ago

The bug currently causes all non-ascii mnemonic/salts to deviate from other libraries (as seen in my Add jp tests pull request)

This is dependent on the environment of the user, which is why Copay is working but my node client is not.

Copay interprets the Japanese as UTF8 bytes after normalization. However, my system is using codepage 932 (shift-jis) so even after running through unorm, my system generates shift-jis bytes of the normalized string instead of utf8...

using Buffer to convert to utf8 bytes will guarantee that it is used in the pbkdf as utf8 bytes.

matiu commented 8 years ago

ACK, great work!

Thanks for finding a simpler solution than the previous PR, and for the test fixtures. I suggest you send a PR with that fixtures to the BIP39 github repo. Other platform developers could benefit having those fixtures.

dabura667 commented 8 years ago

https://github.com/cryptocoinjs/pbkdf2-sha256/pull/2

looks like pbkdf2 is going to merge the change, so once pbkdf2 sha-512 merges it we can add this to pbkdf2 instead.

dabura667 commented 8 years ago

changed the solution to fixing pbkdf2, as upstream has merged my changes.

matiu commented 8 years ago

Just tested manually that with this fix, https://github.com/bitpay/bitcore-mnemonic/pull/39 test pass.

braydonf commented 8 years ago

Also tested manually by merging both #39 and #38, and all tests passing.

braydonf commented 8 years ago

This makes sense, we've fixed other utf-8 issues in a similar way. LGTM

dabura667 commented 8 years ago

All tests looking OK.

Just merge this and I'll go ahead and rebase #38 on it.

dabura667 commented 8 years ago

Ping @matiu @braydonf

Is this ok for merging?