Replace crypto with randombytes so it works in browsers

[oliver-la]

Possible fix for https://github.com/brendanashworth/generate-password/issues/20#issuecomment-330093666

Note that I have absolutely no clue if this is an appropriate solution. In my tests this "fix" works in Angular and a npm test came back positive.

Works in:

• Google Chrome 60.0.3112.113
• Firefox 55.0.3
• Internet Explorer 11

TL;DR: This PR isn't going to be merged. Instead, there is a browser-version of this package: https://www.npmjs.com/package/generate-password-browser

[codecov-io]

Codecov Report

Merging #21 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #21   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          48     48           
=====================================
  Hits           48     48

Impacted Files	Coverage Δ
src/generate.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 23642b0...b758507. Read the comment docs.

[brendanashworth]

Hi @xama5 ,

Apologies if my comment wasn't very clear. I think (I'm not a browser guy, correct me if I'm wrong), but adding in crypto-browserify to your project should allow you to use randombytes with the library as-is (i.e., importing that should give you crypto.randomBytes). It shouldn't need any patches but should work out of the box when you add that to your project.

Let me know if that works for you — if that doesn't work we can look into possibly adding a patch for it.

[oliver-la]

Hi Brendan

I could add crypto-browserify to my project, but that'd require a couple of changes in the transpiling process, and that doesn't make sense just to make a package work which depends on crypto. My suggestion is to do this the other way around. crypto-browserify returns crypto in standard node applications and in browsers its own implementation. (see below) Since crypto-browserify implements many other implementations and we only need the randombytes implementation it makes sense to use it directly. (as I did in my PR)

https://github.com/crypto-browserify/randombytes

randombytes from node that works in the browser. In node you just get crypto.randomBytes, but in the browser it uses .crypto/msCrypto.getRandomValues

Your package will work the same way as it does now, but with support in browsers.

[oliver-la]

@brendanashworth

any update on this?

[brendanashworth]

Hi @xama5 yes thank you for the ping.

I kind of disagree with you in the approach. It seems to me like the burden ought to be with the browser app instead of the npm module. Again, I'm no browser expert, but I believe the goal of browserify is to take existing npm modules and use them without patching. If this wasn't the case, all npm modules would need patches like this one to work with browserify. It's kind of like how they say it is on the website:

Use many of the tens of thousands of modules on NPM in the browser

With that being, you can use those modules without patches. Additionally from the website:

Get browser versions of the node core libraries events, stream, path, url, assert, buffer, util, querystring, http, vm, and crypto when you require() them

Thus you don't need to change the npm module to allow for browserify, but rather you need to import crypto-browserify to get the browser versions.

I believe that the idea behind this would be if you're trying to set up a crypto-related app, you only have to import crypto-browserify once, as opposed to opening issues like this on 10 different npm module repositories. For you, you might think generate-password is the only one you need to change, until you add another dependency.

That's why I'm kind of against adding randombytes into the actual code. It'd still work, yes, but it's against the principle of browserify.

However don't get me wrong — it's not that I hate the browser or anything like that. Rather, I think you bring up a good point that we could use some good documentation in terms of the browser, and maybe even adding a browser CI or similar. I think it should be supported, just differently.

[oliver-la]

Thanks for your feedback. It's not that I "just" need to import browserify, I actually have to change the transpiling process (https://stackoverflow.com/a/38251040) so it's able to replace all imports with its own browserified version.

I believe that the idea behind this would be if you're trying to set up a crypto-related app, you only have to import crypto-browserify once, as opposed to opening issues like this on 10 different npm module repositories. For you, you might think generate-password is the only one you need to change, until you add another dependency.

You're absolutely right. However, the only packages that might not work are those who depend on crypto. And most of the time there are alternatives that work in the browser. As already mentioned, it doesn't make sense to change the way angular cli works just to make one package work.

I respect your decision to not add randombytes into the actual code, so let's agree to disagree. I'm going to close this issue and instead put an "angular" version of your package on npm and credit you accordingly. I hope that's okay for you.

[brendanashworth]

I see. I'm glad you decided on something that works for you — a fork sounds like a good choice for this case. I'd recommend you watch this repository though, so that in the case of a security issue, you'll be notified automatically.