tradle / react-native-crypto

partial implementation of node's `crypto` for react-native
MIT License
370 stars 84 forks source link

Fix error with node modules that use randombytes #22

Closed jd20 closed 6 years ago

jd20 commented 6 years ago

Ran into an issue, with a node module that depends on "randombytes". It appears in the implementation of that module, it checks for the existence of crypto.getRandomValues() (instead of checking for window.crypto.getRandomValues() as some other browserify modules do). In my project's shim.js, I just copied over the implementation from window, and everything seemed to work fine. Should we also be populating crypto.getRandomValues() in react-native-crypto? Here's a PR that does so, thoughts?

jd20 commented 6 years ago

This is the piece of code in randombytes, that looks for crypto.getRandomValues(): https://github.com/crypto-browserify/randombytes/blob/master/browser.js

mvayngrib commented 6 years ago

@jd20 thanks for the PR! I'm not sure i understood the issue. It looks like randombytes checks global.crypto, which should be the same as window.crypto (is it not?). Do you have an example project by any chance with this bug?

if this is an issue, maybe we should just be add a mapping "randombytes": "react-native-randombytes" in rn-nodeify/browser.json?

jd20 commented 6 years ago

Yes, I've uploaded it here: https://github.com/monolithlabs/randombytes_test

Playing around a bit more with it, I found that all that's needed to fix the problem actually, is to require('crypto') before require('randombytes'). (See App.js.) So, as you say, global.crypto gets set from window.crypto, but in my case, that's not happening until after 'randombytes' is imported. Adding a require('crypto') to shim.js is probably a decent workaround actually. Feel free to close.

mvayngrib commented 6 years ago

@jd20 right, maybe we need to revisit having this in shim.js

if (require('./package.json').dependencies['react-native-crypto']) {
  require('crypto')
}
jd20 commented 6 years ago

Yeah, that's what I was thinking (as well as obfuscating the require('crypto') so it doesn't get statically loaded like before). When I have a chance I'll work to put together a PR for that and test it.

mvayngrib commented 6 years ago

@jd20 awesome, thanks :)