urbit / urbit-key-generation

Key derivation and HD wallet generation functions for Urbit
MIT License
15 stars 8 forks source link

Crash if buffers obtained from hex strings are the empty buffer #85

Closed Fang- closed 4 years ago

Fang- commented 4 years ago

Buffer.from('invalid', 'hex') would result in an empty buffer. Keygen would happily proceed with this, hiding dangerously incorrect behavior.

This causes us to check for an empty buffer result, and crash if it's unexpected. To avoid "pedantic" crashes, it strips leading 0x and ensures the hex string contains full bytes by left-padding with 0 if needed.

Fixes #84.

jtobin commented 4 years ago

Hmm. I remember going back and forth on this sort of thing awhile ago and eventually settled on "just do whatever Buffer does" for handling strings. However, Buffer has caused any number of problems (#25, #58, this one -- I feel like there are probably others), so obviously it's not ideal.

What about using Uint8Array or ArrayBuffer or whatever throughout? Purge Buffer entirely? IIRC, Buffer is just some old node.js thing that is only here because it was always here. I don't know if there's any particular need to use it, and one of the other more modern JS data structures would probably be both superior & simpler.

In the meantime this is obviously a strict improvement, and that's all more work. So this gets merged, and the de-buffering gets shelved. :smile: