brendanashworth / generate-password

NodeJS library for generating cryptographically-secure passwords.
MIT License
354 stars 67 forks source link

Error with webpack 5 #68

Open dmytro-podolianskyi-ew opened 2 years ago

dmytro-podolianskyi-ew commented 2 years ago

BREAKING CHANGE: webpack < 5 used to include polyfills for node.js core modules by default. This is no longer the case. Verify if you need this module and configure a polyfill for it

If you want to include a polyfill, you need to:

Please, add to package.json this option

"browser": { "crypto": false },

brendanashworth commented 2 years ago

Would you like to submit a pull request for this @dmytro-podolianskyi-ew ?

beardsleym commented 2 years ago

@dmytro-podolianskyi-ew you might want to use https://www.npmjs.com/package/generate-password-browser

davidrenne commented 2 years ago

Yes I think I encountered this same issue as we are on webpack 4 in one project I work on. I was lazy loading several components which also was including this and I got a huge package of browserify and browserify-crypto and browserify-sign and browserify-rsa (asn1.js also I think was a dependency to load this require) which bloated my bundle dramatically.

I found this library to be the culprit so just ripped it out and wrote a backend handler for generating the password with go.

Was it because it uses var crypto = require('crypto');? where Webpack just doesnt know how to handle it and needs all these browserify helpers.

Is this just a node.js only library where we accidentally used it on web?

At least I stopped the bloat in its tracks and removed this library from our pipeline.

davidrenne commented 2 years ago

This package gets downloaded a lot (Im sure by node.js backend developers) and I know the original author of node.js I thought said he doesnt like npm because it allows importing of node modules for things on the web that werent intended for the web. Or maybe it was something about create-react-app allowing import of node.js modules for a react application on the client side.

This guy I think found the same issue with this library being used on the web accidentally:

https://stackoverflow.com/a/69105180/2040020

Not sure if @brendanashworth can do much about people using it on the web and then browserify and all it's dependencies being bundled accidentally bloating bundles because of this library is used only for backend? I guess the crypto package is like the fs package, backend only intended usage, right?

I suppose a warning on install? "This package should only be used in node.js backend projects otherwise browserify might include many dependencies for crypto and increase your front-end bundle size and vendors", or perhaps something in the readme about being a backend node.js only library.

I am not a node.js expert, mostly front-end/react/webpack and go, but arent you just trying to generate a randomy bytestring of 256 bytes.

https://github.com/nodejs/node/blob/main/lib/internal/crypto/random.js#L99

I realize this is using fastBuffer, but cant you just write your own byte randomizer and reduce this dependency so people on the web can use this library without all the browserify stuff loading because of this crypto require?

brendanashworth commented 2 years ago

@davidrenne yikes interesting link to that stackoverflow... am not sure how to reduce the bundled size for people who use crypto-browserify. Is there a way to signal that we only use randomBytes?

I do not want to fall back on Math.random because that is not cryptographically secure and developers should expect this library to be safe to use out of the box.

Writing our own random number generator would probably be overkill for a library like this. Is there any way to signal to browserify that we only use one function of crypto?

davidrenne commented 2 years ago

From my understanding crypto is a part of node.js and not a separate package. Typically with a package with separate files you can import certain files which reduces the bundle size but I think you cant because you need crypto to do what you do in your package.

I found this:

https://www.npmjs.com/package/randombytes

It seems they use crypto.randomBytes too but are importing possibly just crypto.randomBytes which possibly wouldnt be including all of crypto like your package does:

https://github.com/crypto-browserify/randombytes/blob/master/index.js#L1

And then in their browser.js they are using msCrypto or crypto, but in my browser chrome latest I have window.crypto.getRandomValues as a function and not msCrypto, so I think my case would use global.crypto coming from window when running in the browser.

Where your package imports all of crypto, which makes sense because I saw a bunch of aes and other cryptographic function names in my bundle all over the place because of how my application was chunked and how this generate-password was being used, webpack started importing browserify and all its dependencies to run the full crypto package in the browser.

It was just frustrating because yarn list showed these as dependencies of webpack, but not any any library because your package.json isnt really using a package on npm, but a core library bundled in node. I wish yarn and npm did a better job of showing node.js requires outside of each package.json dependencies.

Sorry I am in a hurry and my battery is about to die. Sorry if this doesnt make sense 100%.

But overall it will be difficult for you to see this, unless you had a web app chunked and importing generate-password where you'd see what I saw. My app is a private repo, otherwise I would show you some of the analyzer stuff. Maybe I can do that if you are interested in seeing how our app loaded all this stuff due to this package I can email you the HTML output of. webpack analyzer if you are interested in trying to maintain your code, yet working better for people using it in the browser with webpack.

brendanashworth commented 2 years ago

@davidrenne do you want to submit a pull request to switch to the randombytes package? Happy to approve it.

shawnzhu commented 1 year ago

when using webpack 5, I made it by using node-polyfill-webpack-plugin) in webpack.config.js w/o any resolve.fallback:

const NodePolyfillPlugin = require('node-polyfill-webpack-plugin');

module.exports = {
    // Other rules...
    plugins: [
        new NodePolyfillPlugin()
    ]
};

This is handy b/c it doesn't need to change existing code/runtime-dependency