Skip to content
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

Use pure-JavaScript implementation for npm-bcrypt package. #7595

Merged
merged 2 commits into from Aug 9, 2016
Merged

Conversation

@benjamn
Copy link
Member

@benjamn benjamn commented Aug 5, 2016

According to the README, this implementation is approximately 2.7 times slower than native: https://www.npmjs.com/package/bcryptjs

Apps that wish to continue using the native bcrypt package should run meteor npm install --save bcrypt in the root application directory, and the npm-bcrypt package will prefer that implementation.

This change should reduce the pain of Windows developers who have had trouble installing a working compiler toolchain to rebuild the native bcrypt package.

@benjamn benjamn added this to the Release 1.4.1 milestone Aug 5, 2016
@benjamn benjamn self-assigned this Aug 5, 2016
@benjamn benjamn modified the milestones: Release 1.4, Release 1.4.1 Aug 5, 2016
// the root of your application.
exports.NpmModuleBcrypt = require("bcrypt");
} catch (e) {
exports.NpmModuleBcrypt = require("bcryptjs");

This comment has been minimized.

@zol

zol Aug 5, 2016
Contributor

Should we warn users that they're using the slower JavaScript bcrypt library here? I suspect most users won't read our release notes carefully enough to know this.

This comment has been minimized.

@benjamn

benjamn Aug 6, 2016
Author Member

I guess the next question would be how often we should warn, since we don't want the warning to be too noisy?

var bcrypt = require("bcrypt");
} catch (e) {
bcrypt = require("bcryptjs");
console.warn([

This comment has been minimized.

@zol

zol Aug 8, 2016
Contributor

That's great - I think it's acceptable to be noisy in this case as the users who typically care about noisiness are also the ones who will be eager to avoid using bcryptjs

According to the README, this implementation is approximately 2.7 times
slower than native: https://www.npmjs.com/package/bcryptjs

Apps that wish to continue using the native bcrypt package should run
`meteor npm install --save bcrypt` in the root application directory, and
the npm-bcrypt package will prefer that implementation.
@benjamn benjamn force-pushed the bcryptjs branch to e524f76 Aug 8, 2016
@benjamn benjamn merged commit a1c3516 into devel Aug 9, 2016
4 checks passed
4 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mitar
Copy link
Collaborator

@mitar mitar commented Aug 19, 2016

Is it really a problem if bcrypt is slow? Isn't the whole point of it to be slow?

@benjamn
Copy link
Member Author

@benjamn benjamn commented Aug 19, 2016

You're right! For new hashes, you can pick whatever constant factors you want to make the hashing as slow as you like. But it's still worth noting that there are lots of old hashes out there already, and you can't just recompute them, since that might require unknown information like salts and passwords. So a really big performance difference would not be something we could hide. In this case, 2-3x seems tolerable, and you can always install bcrypt yourself if you need exactly the same performance characteristics.

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 19, 2016

Yes, but that scary warning ... People might try to install bcrypt and have issues when they would not really have to.

@Torsten85
Copy link

@Torsten85 Torsten85 commented Aug 22, 2016

I got rid of this warning by adding bcrypt as mentioned (meteor npm install --save bcrypt). But after building my app for production (meteor build --architecture os.linux.x86_64), i still get the message on my production server.

Am I missing something?

@abecks
Copy link

@abecks abecks commented Aug 24, 2016

I also receive this warning in production after using meteor build --architecture os.linux.x86_64.

I also tried to manually install bcrypt in bundle/programs/server via npm install bcrypt but I still receive the warning when starting my app.

In my package.json:
"bcrypt": "^0.8.7",

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 24, 2016

Are you running meteor npm install before running meteor build on the same machine?

@abecks
Copy link

@abecks abecks commented Aug 24, 2016

No, I was under the impression that meteor npm install --save bcrypt was installing it. I will try it now.

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 24, 2016

It installs it, but locally. So do you run then meteor build on the same machine. Or do you do this and then run meteor build on another machine where you have not run meteor npm install as well?

@abecks
Copy link

@abecks abecks commented Aug 24, 2016

I do run meteor build on the same machine. Should I try installing bcrypt globally then building? I should note that I do have a bcrypt folder under node_modules in my project root, so it seems installed. I also do not receive the warning when running the app under the meteor command. I only receive the warning in production after building.

I am building on OS X 10.11.3 and deploying to CentOS 6. In the deployment, I do an npm install in bundle/programs/server.

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 24, 2016

Then this seems like a bug.

I think you should open a new issue (or find an existing one) about this issue. Commenting on past pull requests is not very useful from issues management perspective.

@mitar
Copy link
Collaborator

@mitar mitar commented Aug 24, 2016

(Reference a new issue from here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.