From 44dd048036e515c7c029f03074f362cefb1590ce Mon Sep 17 00:00:00 2001
From: Samuel Reed <samuel.trace.reed@gmail.com>
Date: Thu, 10 Aug 2017 17:34:09 -0500
Subject: [PATCH] fix(validatePassword): reword error message

Reword the error message returned when the password is too long
 - remove the plaintext password value, it looks very bad
 - include information about the maximum allowed length instead

Also add additional context to the error.
---
 common/models/user.js | 26 ++++++++++++++++----------
 test/user.test.js     |  8 ++++----
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/common/models/user.js b/common/models/user.js
index 0f15a320..c582f4b8 100644
--- a/common/models/user.js
+++ b/common/models/user.js
@@ -16,6 +16,8 @@ var path = require('path');
 var qs = require('querystring');
 var SALT_WORK_FACTOR = 10;
 var crypto = require('crypto');
+// bcrypt's max length is 72 bytes;
+// See https://github.com/kelektiv/node.bcrypt.js/blob/45f498ef6dc6e8234e58e07834ce06a50ff16352/src/node_blf.h#L59
 var MAX_PASSWORD_LENGTH = 72;
 var bcrypt;
 try {
@@ -993,18 +995,22 @@ module.exports = function(User) {
 
   User.validatePassword = function(plain) {
     var err;
-    if (plain && typeof plain === 'string' && plain.length <= MAX_PASSWORD_LENGTH) {
-      return true;
-    }
-    if (plain.length > MAX_PASSWORD_LENGTH) {
-      err = new Error(g.f('Password too long: %s', plain));
-      err.code = 'PASSWORD_TOO_LONG';
-    } else {
-      err =  new Error(g.f('Invalid password: %s', plain));
+    if (!plain || typeof plain !== 'string') {
+      err = new Error(g.f('Invalid password.'));
       err.code = 'INVALID_PASSWORD';
+      err.statusCode = 422;
+      throw err;
+    }
+
+    // Bcrypt only supports up to 72 bytes; the rest is silently dropped.
+    var len = Buffer.byteLength(plain, 'utf8');
+    if (len > MAX_PASSWORD_LENGTH) {
+      err = new Error(g.f('The password entered was too long. Max length is %d (entered %d)',
+        MAX_PASSWORD_LENGTH, len));
+      err.code = 'PASSWORD_TOO_LONG';
+      err.statusCode = 422;
+      throw err;
     }
-    err.statusCode = 422;
-    throw err;
   };
 
   User._invalidateAccessTokensOfUsers = function(userIds, options, cb) {
diff --git a/test/user.test.js b/test/user.test.js
index d70dc30b..7dcb7d32 100644
--- a/test/user.test.js
+++ b/test/user.test.js
@@ -430,7 +430,7 @@ describe('User', function() {
         var u = new User({username: 'foo', password: pass73Char});
         assert(false, 'Error should have been thrown');
       } catch (e) {
-        expect(e).to.match(/Password too long/);
+        expect(e).to.match(/password entered was too long/);
         done();
       }
     });
@@ -462,7 +462,7 @@ describe('User', function() {
         if (err) return done(err);
         User.resetPassword({email: 'b@c.com', password: pass73Char}, function(err) {
           assert(err);
-          expect(err).to.match(/Password too long/);
+          expect(err).to.match(/password entered was too long/);
           done();
         });
       });
@@ -474,7 +474,7 @@ describe('User', function() {
         .then(
           success => { throw new Error('changePassword should have failed'); },
           err => {
-            expect(err.message).to.match(/Password too long/);
+            expect(err.message).to.match(/password entered was too long/);
 
             // workaround for chai problem
             //   object tested must be an array, an object, or a string,
@@ -493,7 +493,7 @@ describe('User', function() {
         .then(
           success => { throw new Error('setPassword should have failed'); },
           err => {
-            expect(err.message).to.match(/Password too long/);
+            expect(err.message).to.match(/password entered was too long/);
 
             // workaround for chai problem
             //   object tested must be an array, an object, or a string,