From 0b8c7f98d2984fd4525a19c59fb5599d6506524b Mon Sep 17 00:00:00 2001 From: Matt Venables Date: Mon, 20 Feb 2017 14:34:04 -0500 Subject: [PATCH] Only perform asynchronous, save-time hashing The original default method of hashing passwords during the `setter` method was not ideal for node async i/o. The benefit of doing it at set-time was to immediately hash the password, to prevent any leaking. To add some of this security to the asynchronous method, we use the bookshelf `virtuals` plugin, and never set `password` on the model `attributes` hash. This will prevent the password from being exposed during a toJSON call (or other). --- README.md | 50 +----- lib/secure-password.js | 97 ++++-------- package.json | 3 +- test/secure-password.spec.js | 285 ++++++++++++++--------------------- 4 files changed, 151 insertions(+), 284 deletions(-) diff --git a/README.md b/README.md index 43df7d4..ee93bff 100644 --- a/README.md +++ b/README.md @@ -12,24 +12,25 @@ A Bookshelf.js plugin for securely handling passwords. ## Features -* Securely store passwords in the database using BCrypt with little-to-no code. -* Minimal setup required: just install the module, and make a password_digest column in the database! +* Securely store passwords in the database using BCrypt with ease. +* Minimal setup required: just install the module, and make a `password_digest` column in the database! * Follows the latest security guidelines, using a BCrypt cost of 12 -* Two usage options: when the password is set on the model (using BCrypt sync methods), or when the model is saved (using BCrypt async methods). Your choice! ([Read more](#async-or-sync)) * Inspired by and similar to [has_secure_password](http://api.rubyonrails.org/classes/ActiveModel/SecurePassword/ClassMethods.html) in Ruby on Rails. ## Installation ``` -yarn add bookshelf-secure-password +yarn add bookshelf-secure-password bcrypt ``` or ``` -npm install bookshelf-secure-password --save +npm install bookshelf-secure-password bcrypt --save ``` +*NOTE:* The `bcrypt` module is a peerDependency, and should be manually added to your project. + ## Usage 1. Enable the plugin in your Bookshelf setup @@ -41,14 +42,6 @@ npm install bookshelf-secure-password --save bookshelf.plugin(securePassword) ``` - Optionally, you can pass a configuration object when enabling the plugin. (See [options](#configuration-options) below) - - ```javascript - bookshelf.plugin(securePassword, { - performOnSave: true - }) - ``` - 2. Add `hasSecurePassword` to the model(s) which require a secure password ```javascript @@ -67,19 +60,11 @@ npm install bookshelf-secure-password --save }) ``` -3. Now, when you set a password (or save the record with a new password), it will be hashed as `password_digest`: +3. Now, when you set a password and save the record, it will be hashed as `password_digest`: ```javascript - // Default: Password is hashed during `set` time: user = new User({ password: 'testing' }) user.get('password') // => undefined - user.get('password_digest') // => '$2a$12$SzUDit15feMdVCtfSzopc.0LuqeHlJInqq/1Ol8uxCC5QydHpVWFy' - ``` - - ```javascript - // Optional: Password is hashed during `save` call: - user = new User({ password: 'testing' }) - user.get('password') // => 'testing' user.get('password_digest') // => undefined user.save().then(function () { @@ -129,29 +114,10 @@ function signIn (email, password) { } ``` -## Configuration Options - -When enabling the plugin, you can pass in a configuration object. There is currently one option: - -* `performOnSave`: A boolean to perform password hashing on save (using asynchronous calls) versus the default behavior or hasing when the password variable is set. - -## Async or Sync? - -This module provides two options for hashing passwords, each offers their own benefits. - -* Hashing at **Set-time**: The default option is to perform password hashing immediately when the `password` field is set on the record. This is the more secure option, because the password is never stored on the record (in memory or in the database). However, it is a blocking operation and the web server will not be able to process other requests during the milliseconds that the hashing is ocurring. -* Hashing at **Save-time**: The other option is to perform password hashing immediately before the record is being saved to the database. This method uses asynchronous bcrypt methods which allows the web server to handle other requests in between each hashing process, if necessary. However, it is less secure because the raw password is stored in memory on the Model. This will increase chances of an inadvertent exposure of the password. - -| Method | Security | Scalability | -| ------------------ | -------- | ----------- | -| Set-time (default) | Higher | Lower | -| Save-time | Lower | Higher | - - ## Notes * BCrypt requires that passwords are 72 characters maximum (it ignores characters after 72). -* This library enables the built-in `virtuals` plugin on Bookshelf if using the synchronous method. +* This library enables the built-in `virtuals` plugin on Bookshelf for the virtual `password` field. * Passing a `null` value to the password will clear the `password_digest`. * Passing `undefined` or a zero-length string to the password will leave the `password_digest` as-is diff --git a/lib/secure-password.js b/lib/secure-password.js index 881ef1d..01b9ec4 100644 --- a/lib/secure-password.js +++ b/lib/secure-password.js @@ -1,24 +1,24 @@ 'use strict' -function enableSecurePasswordPlugin (Bookshelf, opts) { - const bcrypt = require('bcrypt') +function enableSecurePasswordPlugin (Bookshelf) { const DEFAULT_PASSWORD_FIELD = 'password' + const PRIVATE_PASSWORD_FIELD = '__password' const DEFAULT_PASSWORD_DIGEST_FIELD = 'password_digest' const DEFAULT_SALT_ROUNDS = 12 const PasswordMismatchError = require('./error') const proto = Bookshelf.Model.prototype - const useAsync = opts && opts.performOnSave + let bcrypt + try { + bcrypt = require('bcrypt') + } catch (e) {} Bookshelf.PasswordMismatchError = PasswordMismatchError Bookshelf.Model.PasswordMismatchError = PasswordMismatchError /** - * Enable the `virtuals` plugin if we are using the synchronous method of handling - * password hashing + * Enable the `virtuals` plugin to prevent `password` from leaking */ - if (!useAsync) { - Bookshelf.plugin('virtuals') - } + Bookshelf.plugin('virtuals') /** * Get the password field from the plugin configuration. defaults to `password_digest` @@ -35,24 +35,20 @@ function enableSecurePasswordPlugin (Bookshelf, opts) { } /** - * Generate the BCrypt hash for a given string using synchronous methods + * Generate the BCrypt hash for a given string. * * @param {String} value - The string to hash - * @returns {String} - A BCrypt hashed version of the string + * @returns {Promise.} - A BCrypt hashed version of the string */ - function hashSync (value) { - let salt = bcrypt.genSaltSync(DEFAULT_SALT_ROUNDS) + function hash (value) { + if (value === null) { + return Promise.resolve(null) + } - return bcrypt.hashSync(value, salt) - } + if (isEmpty(value)) { + return Promise.resolve(undefined) + } - /** - * Generate the BCrypt hash for a given string using asynchronous methods - * - * @param {String} value - The string to hash - * @returns {String} - A BCrypt hashed version of the string - */ - function hashAsync (value) { return bcrypt .genSalt(DEFAULT_SALT_ROUNDS) .then((salt) => { @@ -75,59 +71,32 @@ function enableSecurePasswordPlugin (Bookshelf, opts) { } /** - * Enable sychronous (set-time) password hasing on the model when the attribute is set. This - * method is considered more secure than the asynchronous method because the raw password is - * not stored in memory on the model, decreasing the liklihood of inadvertently exposing the - * password. - * - * However, this method is blocking and could prevent the web server from handling other requests - * while hasing the password. + * Enable password hasing on the model when the model is saved. * * @param {Model} model - The bookshelf model to set up * @returns {Model} - The model */ - function enableSyncHashing (model) { + function enablePasswordHashing (model) { let field = passwordDigestField(model) model.virtuals = model.virtuals || {} model.virtuals[DEFAULT_PASSWORD_FIELD] = { get: function getPassword () {}, set: function setPassword (value) { - if (value === null) { - model.set(field, null) - } else if (!isEmpty(value)) { - model.set(field, hashSync(value)) - } + this[PRIVATE_PASSWORD_FIELD] = value } } - return model - } - - /** - * Enable asychronous (save-time) password hasing on the model when the model is saved. This - * method is beneficial because it makes all expensive calls using asynchronous calls, freeing - * up additional resources to handle income requests. - * - * However, use this with caution. The raw `password` variable will be stored on the model until - * the record is saved, which increases the chance of inadvertantly exposing it. - * - * @param {Model} model - The bookshelf model to set up - * @returns {Model} - The model - */ - function enableAsyncHashing (model) { - let field = passwordDigestField(model) - model.on('saving', (model) => { - if (model.hasChanged(DEFAULT_PASSWORD_FIELD)) { - let value = model.get(DEFAULT_PASSWORD_FIELD) + let value = model[PRIVATE_PASSWORD_FIELD] - return hashAsync(value).then((_hashed) => { - model.unset(DEFAULT_PASSWORD_FIELD) + return hash(value).then((_hashed) => { + model.unset(DEFAULT_PASSWORD_FIELD) + if (_hashed !== undefined) { model.set(field, _hashed) - return model - }) - } + } + return model + }) }) } @@ -136,11 +105,7 @@ function enableSecurePasswordPlugin (Bookshelf, opts) { constructor: function () { if (this.hasSecurePassword) { - if (useAsync) { - enableAsyncHashing(this) - } else { - enableSyncHashing(this) - } + enablePasswordHashing(this) } proto.constructor.apply(this, arguments) @@ -155,16 +120,18 @@ function enableSecurePasswordPlugin (Bookshelf, opts) { * a `PasswordMismatchError` upon failed check. */ authenticate: function authenticate (password) { + let digest = this.get(passwordDigestField(this)) + if (!this.hasSecurePassword) { return proto.authenticate.apply(this, arguments) } - if (isEmpty(password)) { + if (isEmpty(password) || isEmpty(digest)) { return Promise.reject(new this.constructor.PasswordMismatchError()) } return bcrypt - .compare(password, this.get(passwordDigestField(this))) + .compare(password, digest) .then((matches) => { if (!matches) { throw new this.constructor.PasswordMismatchError() diff --git a/package.json b/package.json index 695b5ba..b63bde5 100644 --- a/package.json +++ b/package.json @@ -29,10 +29,11 @@ ] }, "homepage": "https://github.com/venables/bookshelf-secure-password#readme", - "dependencies": { + "peerDependencies": { "bcrypt": "^1.0.2" }, "devDependencies": { + "bcrypt": "^1.0.2", "bookshelf": "^0.10.3", "chai": "^3.5.0", "coveralls": "^2.11.16", diff --git a/test/secure-password.spec.js b/test/secure-password.spec.js index 0e69ea6..1ef91b1 100644 --- a/test/secure-password.spec.js +++ b/test/secure-password.spec.js @@ -10,173 +10,139 @@ const securePassword = require('../lib/secure-password.js') describe('bookshelf-secure-password', function () { let bookshelf let knex + let model + let BasicModel + let CustomModel before(function () { - knex = new Knex({ - client: 'pg' - }) + knex = new Knex({ client: 'pg' }) mockKnex.mock(knex) + + bookshelf = new Bookshelf(knex) + bookshelf.plugin(securePassword) + + BasicModel = bookshelf.Model.extend({ + hasSecurePassword: true + }) + + CustomModel = bookshelf.Model.extend({ + hasSecurePassword: 'custom_column' + }) }) after(function () { mockKnex.unmock(knex) }) - describe('synchronous behavior', function () { - before(function () { - bookshelf = new Bookshelf(knex) - bookshelf.plugin(securePassword) - }) - + describe('password hashing', function () { describe('with the default column', function () { - let model - - before(function () { - const Model = bookshelf.Model.extend({ - hasSecurePassword: true - }) - - model = new Model({ password: 'testing' }) + beforeEach(function () { + model = new BasicModel({ id: 1, password: 'testing' }) }) - it('does not keep the raw password on the model', function () { - expect(model.get('password')).to.be.undefined - expect(model.attributes.password).to.be.undefined - - expect(model.get('password_digest')).to.be.a.string - expect(model.attributes.password_digest).to.be.a.string - }) + describe('before save', function () { + it('does not keep the raw password on the model', function () { + expect(model.get('password')).to.be.undefined + expect(model.attributes.password).to.be.undefined - it('sets the password digest field to null if given a `null` value', function () { - expect(model.get('password_digest')).to.be.a.string - model.set('password', null) - expect(model.get('password_digest')).to.be.null + expect(model.get('password_digest')).to.be.undefined + expect(model.attributes.password_digest).to.be.undefined + }) }) - it('does not change the password digest if given undefined', function () { - let originalString = model.get('password_digest') - model.set('password', undefined) - expect(model.get('password_digest')).to.equal(originalString) - }) + describe('after save', function () { + beforeEach(function () { + return model.save() + }) - it('does not change the password digest if given an empty string', function () { - let originalString = model.get('password_digest') - model.set('password', '') - expect(model.get('password_digest')).to.equal(originalString) - }) + it('sets the password digest field to null if given a `null` value', function () { + expect(model.get('password_digest')).to.be.a.string + model.set('password', null) - it('changes the password digest if given a blank (spaces-only) string', function () { - let originalString = model.get('password_digest') - model.set('password', ' ') - expect(model.get('password_digest')).to.be.a.string - expect(model.get('password_digest')).not.to.equal(originalString) - }) - }) + return model.save().then(() => { + expect(model.get('password_digest')).to.be.null + }) + }) - it('allows the default column to be overwritten', function () { - const Model = bookshelf.Model.extend({ - hasSecurePassword: 'custom_column' - }) + it('does not change the password digest if given undefined', function () { + let originalString = model.get('password_digest') + model.set('password', undefined) - let model = new Model({ password: 'testing' }) - expect(model.get('password')).to.be.undefined - expect(model.attributes.password).to.be.undefined + return model.save().then(() => { + expect(model.get('password_digest')).to.equal(originalString) + }) + }) - expect(model.get('custom_column')).to.be.a.string - expect(model.attributes.custom_column).to.be.a.string - }) - }) + it('does not change the password digest if given an empty string', function () { + let originalString = model.get('password_digest') + model.set('password', '') - describe('asynchronous save-time behavior', function () { - let model + return model.save().then(() => { + expect(model.get('password_digest')).to.equal(originalString) + }) + }) - before(function () { - bookshelf = new Bookshelf(knex) - bookshelf.plugin(securePassword, { - performOnSave: true + it('changes the password digest if given a blank (spaces-only) string', function () { + let originalString = model.get('password_digest') + model.set('password', ' ') + return model.save().then(() => { + expect(model.get('password_digest')).to.be.a.string + expect(model.get('password_digest')).not.to.equal(originalString) + }) + }) }) - const Model = bookshelf.Model.extend({ - hasSecurePassword: true - }) + it('handles the case if a later validation throws an exception', function () { + let digest - model = new Model({ id: 1, password: 'testing' }) + model.on('saving', function (model) { + throw new Error() + }) - expect(model.get('password')).to.equal('testing') - expect(model.get('password_digest')).to.be.undefined + return model + .save() + .then(() => { + expect(false).to.be.true + }, () => { + expect(model.get('password')).to.be.undefined + expect(model.get('password_digest')).to.be.a.string + digest = model.get('password_digest') + return model.save() + }) + .then(() => { + expect(false).to.be.true + }, () => { + expect(model.get('password_digest')).to.equal(digest) + }) + }) }) - it('saves the hashed password, clearing the raw password field', function () { - return model.save().then((model) => { - expect(model.get('password')).to.be.undefined - expect(model.get('password_digest')).to.be.a.string + describe('with a custom column', function () { + before(function () { + model = new CustomModel({ id: 2, password: 'testing' }) + return model.save() }) - }) - it('handles the case if a later validation throws an exception', function () { - let digest + it('allows the default column to be overwritten', function () { + expect(model.get('password')).to.be.undefined + expect(model.attributes.password).to.be.undefined - model.on('saving', function (model) { - throw new Error() + expect(model.get('custom_column')).to.be.a.string + expect(model.attributes.custom_column).to.be.a.string }) - - return model - .save() - .then(() => { - expect(false).to.be.true - }, () => { - expect(model.get('password')).to.be.undefined - expect(model.get('password_digest')).to.be.a.string - digest = model.get('password_digest') - return model.save() - }) - .then(() => { - expect(false).to.be.true - }, () => { - expect(model.get('password_digest')).to.equal(digest) - }) }) }) describe('#authenticate', function () { - let model - - describe('synchronous behavior', function () { - before(function () { - bookshelf = new Bookshelf(knex) - bookshelf.plugin(securePassword) + describe('with hasSecurePassword enabled on the model', function () { + beforeEach(function () { + model = new BasicModel({ id: 1, password: 'testing' }) }) - describe('with hasSecurePassword enabled on the model', function () { - before(function () { - const Model = bookshelf.Model.extend({ - hasSecurePassword: true - }) - - model = new Model({ password: 'testing' }) - }) - - it('resolves the Model if the password matches', function () { + describe('before save', function () { + it('does not authenticate until the record is saved', function () { return model.authenticate('testing').then((model) => { - expect(model).to.be.defined - }, (err) => { - expect(err).to.be.undefined - }) - }) - - it('rejects with a PasswordMismatchError if the password does not match', function () { - return model.authenticate('invalid').then((model) => { - expect(model).to.be.defined - }, (err) => { - expect(err).to.be.defined - expect(err).to.be.an.instanceof(PasswordMismatchError) - expect(err.name).to.equal('PasswordMismatchError') - }) - }) - - it('rejects with a PasswordMismatchError if the no password is provided', function () { - return model.authenticate().then((model) => { - expect(model).to.be.defined + expect(false).to.be.true }, (err) => { expect(err).to.be.defined expect(err).to.be.an.instanceof(PasswordMismatchError) @@ -185,38 +151,8 @@ describe('bookshelf-secure-password', function () { }) }) - describe('without hasSecurePassword on this model', function () { - it('calls the model`s `authenticate` method', function () { - const Model = bookshelf.Model.extend({}) - model = new Model({ password: 'testing' }) - - try { - return model.authenticate('testing') - } catch (err) { - expect(err).to.be.defined - expect(err).to.be.an.instanceof(TypeError) - } - }) - }) - }) - - describe('asynchronous save-time behavior', function () { - let model - - before(function () { - bookshelf = new Bookshelf(knex) - bookshelf.plugin(securePassword, { - performOnSave: true - }) - }) - - describe('with hasSecurePassword enabled on the model', function () { - before(function () { - const Model = bookshelf.Model.extend({ - hasSecurePassword: true - }) - - model = new Model({ id: 1, password: 'testing' }) + describe('after save', function () { + beforeEach(function () { return model.save() }) @@ -230,7 +166,7 @@ describe('bookshelf-secure-password', function () { it('rejects with a PasswordMismatchError if the password does not match', function () { return model.authenticate('invalid').then((model) => { - expect(model).to.be.defined + expect(false).to.be.true }, (err) => { expect(err).to.be.defined expect(err).to.be.an.instanceof(PasswordMismatchError) @@ -248,22 +184,19 @@ describe('bookshelf-secure-password', function () { }) }) }) + }) - describe('without hasSecurePassword on this model', function () { - it('calls the model`s `authenticate` method', function () { - const Model = bookshelf.Model.extend({}) - model = new Model({ id: 1, password: 'testing' }) - - return model - .save() - .then((model) => { - model.authenticate('testing') - }) - .catch((err) => { - expect(err).to.be.defined - expect(err).to.be.an.instanceof(TypeError) - }) - }) + describe('without hasSecurePassword on this model', function () { + it('calls the model`s `authenticate` method', function () { + const Model = bookshelf.Model.extend({}) + model = new Model({ id: 1, password: 'testing' }) + + try { + return model.authenticate('testing') + } catch (err) { + expect(err).to.be.defined + expect(err).to.be.an.instanceof(TypeError) + } }) }) })