kelektiv / node.bcrypt.js

bcrypt for NodeJs
MIT License
7.38k stars 510 forks source link

issue with node 0.4.7 #15

Closed heroic closed 13 years ago

heroic commented 13 years ago

The bcrypt encrypt function returns the hash as ':' always. works fine with node0.5-pre though

ncb000gt commented 13 years ago

That shouldn't be the case, but may be indicative of an issue with node v0.4.7. Thanks for bringing it up.

I'll look into it as soon as I can.

ncb000gt commented 13 years ago

Ok. I took a look. I compiled 0.4.7 and then recompiled the bcrypt module and am not seeing any issues.

Is there a reproducible test you can post here? I'll gladly include it in the test suite, I am just not able to reproduce the issue myself.

Let me know, thanks!

heroic commented 13 years ago

https://gist.github.com/939366

so i tried out all those.

and it seems the issue is happening on node0.5-pre too.

here is a list of all installed packages i have, just incase...

backbone@0.3.3 active installed Give your JS App some Backbone with Models, Views, Co bcrypt@0.2.2 active installed A bcrypt library for NodeJS.
carrier@0.1.1 active installed Evented stream line reader for node.js
colors@0.5.0 active installed get colors in your node.js console like what
connect@1.3.0 active installed High performance middleware framework framework w connect-redis@1.0.3 active installed Redis session store for Connect
express@2.2.2 active installed Sinatra inspired web development framework framew express-csrf@0.2.0 active installed Cross-site request forgery protection for Express
express-helpers@0.1.0 active installed Express Helpers express helper ejs hashlib@1.0.0 active installed lib for node which makes hashes
hiredis@0.1.9 active installed Wrapper for reply processing code in hiredis
jade@0.10.4 active installed Jade template engine
mail@0.2.2 active installed This SMTP client library for Node.JS helps you send e mailer@0.4.52 active installed send emails from node.js to a smtp server, simple as mime@1.2.1 active installed A comprehensive library for mime-type mapping uti mongodb@0.9.3 active installed A node.js driver for MongoDB
mongoose@1.2.0 installed Mongoose MongoDB ORM mongodb mongoose orm data datastore mongoose@1.3.0 active installed Mongoose MongoDB ORM mongodb mongoose orm data da nodeunit@0.5.1 active installed Easy unit testing for node.js and the browser.
npm@0.3.18 active installed A package manager for node package manager module qs@0.0.7 installed querystring parser
qs@0.1.0 active installed querystring parser
redis@0.5.11 installed Redis client library
redis@0.6.0 active installed Redis client library
reparse@0.1.2 active installed A parser combinator library like Parsec. parse pa sass@0.5.0 active installed Syntactically Awesome Stylesheets (compiles to css)
socket.io@0.6.17 active installed The cross-browser WebSocket
underscore@1.1.5 installed JavaScript's functional programming helper library. util underscore@1.1.6 active installed JavaScript's functional programming helper library.

ncb000gt commented 13 years ago

The "salt" used for the encrypt function is invalid. I'll look at adding some validation on the salt.

You first need to generate the salt:

bcrypt.gen_salt(10, function(err, salt) {
  bcrypt.encrypt('password', salt, function(err, enc) {
  }
}

Try that and let me know if you still see issues.

ncb000gt commented 13 years ago

Alternatively, if you don't generate a salt, you could by hand produce what is required in the salt. The salt needs to be in the form: $Vers$log2(NumRounds)$saltvalue

To prove this you can try this snippet when works.

bcrypt.encrypt('test', '$2a$10$somethingheretobeasalt', function(err, enc) { console.log(enc); });
heroic commented 13 years ago

Yes that worked. Thanks.

Amit Kumar http://www.cobboc.com

On 24-Apr-2011, at 8:24 PM, ncb000gt wrote:

bcrypt.encrypt('test', '$2a$10$somethingheretobeasalt', function(err, enc) { console.log(enc); });

ncb000gt commented 13 years ago

I've added a very basic check for the structure of the salt. I'm writing up some tests now to make sure it works. :)

Thanks for noting this.

ncb000gt commented 13 years ago

Alright, I added a check within the code in this 29a4d5e715541548a755f2c40f1ebb80738eab6b commit.

Throws an error for the sync version and just sets the first, error, param in the async version. This also helped me realize there was a problem with the erroring code path. Before this, if an error occurred and was used, the module would segfault. Needless to say, I've fixed that as well. :)

You'll want to npm update bcrypt

Enjoy.

heroic commented 13 years ago

Great work! ME and my partners thank you for the awesome library!

Amit Kumar http://www.cobboc.com

On 25-Apr-2011, at 1:05 AM, ncb000gt wrote:

Alright, I added a check within the code in this 29a4d5e715541548a755f2c40f1ebb80738eab6b commit.

Throws an error for the sync version and just sets the first, error, param in the async version. This also helped me realize there was a problem with the erroring code path. Before this, if an error occurred and was used, the module would segfault. Needless to say, I've fixed that as well. :)

You'll want to npm update bcrypt

Enjoy.

Reply to this email directly or view it on GitHub: https://github.com/ncb000gt/node.bcrypt.js/issues/15#comment_1050671

ncb000gt commented 13 years ago

Sure thing. Glad you like it. As always, let me know if you find any more problems.