-
-
Notifications
You must be signed in to change notification settings - Fork 903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throwing error when insecure rng is used #294
Conversation
At this point I'm of the opinion this module shouldn't have an insecure option. Thus ...
The link in step 3 should refer either to docs in the README or a separate wiki page in this project that describe:
That last point is a little problematic. Ideally we could recommend a cryptographically secure JS module upon which to build a CSPRNG function the reader could pass in, but that's easier said than done. Such modules tend to be either obscenely code-heavy or of questionable quality, and there's no obvious "right" choice. If we make a recommendation, we (meaning "I") end up dealing with any issues that arise, which I'm not interested in taking on. So... I think a simple example that just shows the basic mechism for passing in an rng function is sufficient. It could use an all-zeroes function as an example ( |
BTW, thanks for working on this! :-) |
Thanks for the feedback. I'll make those requested changes. |
c5e439b
to
15b345a
Compare
@broofa I made those changes and modified https://github.com/kelektiv/node-uuid/wiki#no-secure-rng-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Re: the wiki, I'd actually forgotten about that page. :-) I think I wrote that for an aborted attempt at addressing this issue back in the day. Regardless, I've taken an editorial pass over it. 'Hope you don't mind.
Please change the commit message to conform to Conventional Commit guidelines. Specifically, change it to "BREAKING CHANGE: Removed Math.random rng fallback"
|
||
return rnds; | ||
module.exports = function noSecureRNG() { | ||
throw Error('uuid: No secure RNG available. See https://github.com/kelektiv/node-uuid/wiki#no-good-rng-error for more info.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I was wondering if I can be of any help in getting this across the finish line? It seems like the only things missing are an amended commit message and a changed wiki link. I forked this branch and made those changes here: https://github.com/lime/node-uuid/tree/error-on-insecure-rng If you want, I could open a separate pull request from it. You're also welcome to pluck those commits in directly. I just wanted to check first whether it would be welcomed, and if there's anything else that still needs work. Edit: and obviously, if the original author has the time to make the changes themselves, by all means. :) Just thought I'd help if I can. |
15b345a
to
798af6f
Compare
@broofa I made the change to the commit message is there anything else? |
Closed through #354 |
true
ifrng.insecure == true
Closes #173
Here's my attempt at closing #173, please let me know if a different approach is preferred.