Review fixes.

Updated tests.

_locales/en/messages.json

@@ -1733,13 +1733,17 @@
     "message": "Password changed"
   },
   "passwordLengthError": {
-    "message": "Password must be atleast 6 characters long",
+    "message": "Password must be between 6 and 50 characters long",
     "description": "Error string shown to the user when password doesn't meet length criteria"
   },
   "passwordTypeError": {
     "message": "Password must be a string",
     "description": "Error string shown to the user when password is not a string"
   },
+  "passwordCharacterError": {
+    "message": "Password must only contain letters, numbers and symbols",
+    "description": "Error string shown to the user when password contains an invalid character"
+  },
   "change": {
     "message": "Change"
   },

app/password_util.js

@@ -1,18 +1,30 @@
 const { sha512 } = require('js-sha512');
 
+const ERRORS = {
+  TYPE: 'Password must be a string',
+  LENGTH: 'Password must be between 6 and 50 characters long',
+  CHARACTER: 'Password must only contain letters, numbers and symbols',
+};
+
 const generateHash = (phrase) => phrase && sha512(phrase.trim());
 const matchesHash = (phrase, hash) => phrase && sha512(phrase.trim()) === hash.trim();
 
 const validatePassword = (phrase, i18n) => {
-  if (typeof phrase !== 'string') {
-    return i18n ? i18n('passwordTypeError') : 'Password must be a string'
+  if (!phrase || typeof phrase !== 'string') {
+    return i18n ? i18n('passwordTypeError') : ERRORS.TYPE;
   }
 
-  if (phrase && phrase.trim().length < 6) {
-    return i18n ? i18n('passwordLengthError') : 'Password must be atleast 6 characters long';
+  const trimmed = phrase.trim();
+  if (trimmed.length < 6 || trimmed.length > 50) {
+    return i18n ? i18n('passwordLengthError') : ERRORS.LENGTH;
+  }
+
+  // Restrict characters to letters, numbers and symbols
+  const characterRegex = /^[a-zA-Z0-9-!()._`~@#$%^&*+=[\]{}|<>,;:]+$/
+  if (!characterRegex.test(trimmed)) {
+    return i18n ? i18n('passwordCharacterError') : ERRORS.CHARACTER;
   }
 
-  // An empty password is still valid :P
   return null;
 }
 
@@ -20,4 +32,4 @@ module.exports = {
   generateHash,
   matchesHash,
   validatePassword,
-};
+};

app/sql.js

@@ -601,8 +601,9 @@ async function removeIndexedDBFiles() {
 }
 
 // Password hash
+const PASS_HASH_ID = 'passHash';
 async function getPasswordHash() {
-  const item = await getItemById('passHash');
+  const item = await getItemById(PASS_HASH_ID);
   return item && item.value;
 }
 async function savePasswordHash(hash) {
@@ -610,11 +611,11 @@ async function savePasswordHash(hash) {
     return removePasswordHash();
   }
 
-  const data = { id: 'passHash', value: hash };
+  const data = { id: PASS_HASH_ID, value: hash };
   return createOrUpdateItem(data);
 }
 async function removePasswordHash() {
-  return removeItemById('passHash');
+  return removeItemById(PASS_HASH_ID);
 }
 
 // Groups

js/views/standalone_registration_view.js

@@ -193,6 +193,11 @@
       const input = this.trim(this.$passwordInput.val());
       const confirmationInput = this.trim(this.$passwordConfirmationInput.val());
 
+      // If user hasn't set a value then skip
+      if (!input && !confirmationInput) {
+        return null;
+      }
+
       const error = passwordUtil.validatePassword(input, i18n);
       if (error) {
         return error;

test/app/password_util_test.js

@@ -7,12 +7,12 @@ describe('Password Util', () => {
     it('generates the same hash for the same phrase', () => {
       const first = passwordUtil.generateHash('phrase');
       const second = passwordUtil.generateHash('phrase');
-      assert.equal(first, second);
+      assert.strictEqual(first, second);
     });
     it('generates different hashes for different phrases', () => {
       const first = passwordUtil.generateHash('0');
       const second = passwordUtil.generateHash('1');
-      assert.notEqual(first, second);
+      assert.notStrictEqual(first, second);
     });
   });
 
@@ -33,25 +33,61 @@ describe('Password Util', () => {
       const valid = [
         '123456',
         '1a5b3C6g',
-        'ABC#DE#F$IJ',
-        'AabcDegf',
+        ')CZcy@ccHa',
+        'C$D--M;Xv+',
+        'X8-;!47IW|',
+        'Oi74ZpoSx,p',
+        '>]K1*g^swHW0]F6}{',
+        'TiJf@lk^jsO^z8MUn%)[Sd~UPQ)ci9CGS@jb<^',
+        '$u&%{r]apg#G@3dQdCkB_p8)gxhNFr=K&yfM_M8O&2Z.vQyvx',
+        'bf^OMnYku*iX;{Piw_0zvz',
+        '#'.repeat(50),
       ];
       valid.forEach(pass => {
         assert.isNull(passwordUtil.validatePassword(pass));
       });
     });
 
-    it('should return an error string if password is invalid', () => {
+    it('should return an error if password is not a string', () => {
       const invalid = [
         0,
         123456,
         [],
         {},
-        '123',
-        '1234$',
+        null,
+        undefined,
       ];
       invalid.forEach(pass => {
-        assert.isNotNull(passwordUtil.validatePassword(pass));
+        assert.strictEqual(passwordUtil.validatePassword(pass), 'Password must be a string');
+      });
+    });
+
+    it('should return an error if password is not between 6 and 50 characters',() => {
+      const invalid = [
+        'a',
+        'abcde',
+        '#'.repeat(51),
+        '#'.repeat(100),
+      ];
+      invalid.forEach(pass => {
+        assert.strictEqual(passwordUtil.validatePassword(pass), 'Password must be between 6 and 50 characters long');
+      });
+    });
+
+    it('should return an error if password has invalid characters', () => {
+      const invalid = [
+        'ʍʪց3Wͪ݌bΉf',
+        ')É{b)͎ÔȩҜ٣',
+        'ߓܑ˿G֖=3¤)P',
+        'ݴ`ԚfĬ8ӝrH(',
+        'e̹ωͻܺȬۺ#dӄ',
+        '谀뤼筎笟ꅅ栗塕카ꭴ',
+        '俈꛷࿩迭䰡钑럭䛩銛뤙',
+        '봟㉟ⓓ༭꽫㊡䶷쒨⻯颰',
+        '<@ȦƘΉوۉaҋ<',
+      ];
+      invalid.forEach(pass => {
+        assert.strictEqual(passwordUtil.validatePassword(pass), 'Password must only contain letters, numbers and symbols');
       });
     });
   });