diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 451d2961ab..32a1cd61fc 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -24,6 +24,17 @@ const _checkWriteResult = function (result, callback) { } } +function _validatePasswordNotTooLong(password) { + // bcrypt has a hard limit of 72 characters. + if (password.length > 72) { + return new InvalidPasswordError({ + message: 'password is too long', + info: { code: 'too_long' }, + }) + } + return null +} + const AuthenticationManager = { _checkUserPassword(query, password, callback) { // Using Mongoose for legacy reasons here. The returned User instance @@ -140,6 +151,10 @@ const AuthenticationManager = { info: { code: 'too_long' }, }) } + const passwordLengthError = _validatePasswordNotTooLong(password) + if (passwordLengthError) { + return passwordLengthError + } if ( !allowAnyChars && !AuthenticationManager._passwordCharactersAreValid(password) @@ -176,13 +191,18 @@ const AuthenticationManager = { // check current number of rounds and rehash if necessary const currentRounds = bcrypt.getRounds(hashedPassword) if (currentRounds < BCRYPT_ROUNDS) { - AuthenticationManager.setUserPassword(user, password, callback) + AuthenticationManager._setUserPasswordInMongo(user, password, callback) } else { callback() } }, hashPassword(password, callback) { + // Double-check the size to avoid truncating in bcrypt. + const error = _validatePasswordNotTooLong(password) + if (error) { + return callback(error) + } bcrypt.genSalt(BCRYPT_ROUNDS, BCRYPT_MINOR_VERSION, function (error, salt) { if (error) { return callback(error) @@ -211,35 +231,37 @@ const AuthenticationManager = { if (match) { return callback(new PasswordMustBeDifferentError()) } - this.hashPassword(password, function (error, hash) { - if (error) { - return callback(error) - } - db.users.updateOne( - { - _id: ObjectId(user._id.toString()), - }, - { - $set: { - hashedPassword: hash, - }, - $unset: { - password: true, - }, - }, - function (updateError, result) { - if (updateError) { - return callback(updateError) - } - _checkWriteResult(result, callback) - HaveIBeenPwned.checkPasswordForReuseInBackground(password) - } - ) - }) + this._setUserPasswordInMongo(user, password, callback) + HaveIBeenPwned.checkPasswordForReuseInBackground(password) } ) }, + _setUserPasswordInMongo(user, password, callback) { + this.hashPassword(password, function (error, hash) { + if (error) { + return callback(error) + } + db.users.updateOne( + { _id: ObjectId(user._id.toString()) }, + { + $set: { + hashedPassword: hash, + }, + $unset: { + password: true, + }, + }, + function (updateError, result) { + if (updateError) { + return callback(updateError) + } + _checkWriteResult(result, callback) + } + ) + }) + }, + _passwordCharactersAreValid(password) { let digits, letters, lettersUp, symbols if ( diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 374008cdce..3eae2000f6 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -217,6 +217,16 @@ describe('AuthenticationManager', function () { }) }) + describe('hashPassword', function () { + it('should block too long passwords', function (done) { + this.AuthenticationManager.hashPassword('x'.repeat(100), err => { + expect(err).to.exist + expect(err.message).to.equal('password is too long') + done() + }) + }) + }) + describe('authenticate', function () { describe('when the user exists in the database', function () { beforeEach(function () { @@ -277,7 +287,7 @@ describe('AuthenticationManager', function () { this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' this.bcrypt.compare = sinon.stub().callsArgWith(2, null, true) this.bcrypt.getRounds = sinon.stub().returns(1) - this.AuthenticationManager.setUserPassword = sinon + this.AuthenticationManager._setUserPasswordInMongo = sinon .stub() .callsArgWith(2, null) this.AuthenticationManager.authenticate( @@ -305,7 +315,7 @@ describe('AuthenticationManager', function () { }) it('should set the users password (with a higher number of rounds)', function () { - this.AuthenticationManager.setUserPassword + this.AuthenticationManager._setUserPasswordInMongo .calledWith(this.user, this.unencryptedPassword) .should.equal(true) })