Javascript password generator adding spaces and failing tests

Question

I'm trying to build a password generator which creates passwords conforming to:

1. Minimum 8 characters in length, Maximum 40 characters in length
2. Must contain at least 1 uppercase, lowercase, number and symbol

I'm avoiding Math.random out of choice, I prefer the crypto option.

I've been through loads of articles to try and get this working, but I'm having the following issues:

1. Random spaces seem to occur in the output string, often on the end.

2. Output values are sometimes not conforming to the min 8 char rule.

I realize I probably have far too many additional if statements double checking things, but I'm struggling to see where it's going wrong.

This is to go into another system, so I'm creating it as modular and functional as possible. Apologies for the large snippet below, I couldn't get it working in jsfiddle.

function cryptoPassword(){

    var minFieldNum = 8;    //Minimum char size of desired output
    var maxFieldNum = 40;   //X defines their fields as 40 as the max_length    
    var outputValue = '';   //Output for field/overall function

    var fieldRandom = getRandomInt(minFieldNum, maxFieldNum); //Generate length of password 

    if (fieldRandom < minFieldNum || fieldRandom > maxFieldNum) {
    fieldRandom = getRandomInt(minFieldNum, maxFieldNum); //Regenerate if length doesn't conform - Not working? 
    }
    else {
        for (i = 0; outputValue.length < fieldRandom; i++) {
             var mask = getRandomMask(); //Get mask selection 
             var randomChar = mask.charAt(getRandomInt(0,mask.length)); //Pick random char in mask
             if (randomChar == " ") { //I don't know where the spaces come from
                var randomChar = mask.charAt(getRandomInt(0,mask.length)); //Pick random char in mask
            }
            outputValue += randomChar; //Add to output
        }

        if (passwordChecker(outputValue, minFieldNum)) {
            return outputValue + " " + passwordChecker(outputValue, minFieldNum);
        }
        else {
            return cryptoPassword();
        }

    }
}


function getRandomInt(min, max) {  
    var byteArray = new Uint8Array(1);  
    window.crypto.getRandomValues(byteArray);
    var range = (max - min + 1);
    return min  + (byteArray[0] % range);
}

function getRandomMask() {
    var maskLcaseChar = 'abcdefghijklmnopqrstuvwxyz';
    var maskUcaseChar = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
    var maskNumeric = '0123456789';
    var maskSpecial = '!"$%^&*(){}[];#,./:@~<>?|_+-='; 

    var maskRandomNo = getRandomInt(0, 3);
    var selectMask = [maskLcaseChar, maskUcaseChar, maskNumeric, maskSpecial];  
    return  selectMask[maskRandomNo];
}

function passwordChecker(output, minSize){
    var checkChars = '!"$%^&*(){}[];#,./:@~<>?|_+-='; 
    if (output.length < minSize){
        return false
    }
    else if((output.toUpperCase() != output) && (output.toLowerCase() != output)) {
        for (var i = 0; i < output.length; i++) {
            if (checkChars.indexOf(output.charAt(i)) != -1) {
                return true;
            }
        }
    }
    return false;
}

Answer 1

So the issue seems to have been with the getRandomInt function which would return an int between 0 and the length of the mask. As you're dealing with arrays you really want it to be between 0 and length of array -1.

You were getting empty strings back from the charAt function as you were asking for a position out side the array.

I've fixed that and also optimised the generating section a bit. It always adds one char from each of the masks, then takes them randomly after that. I then shuffle the string, moving the initial 4 around. It's not using crypto to shuffle but I don't think it's necessary there.

function cryptoPassword(){

    var maskLcaseChar = 'abcdefghijklmnopqrstuvwxyz';
    var maskUcaseChar = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
    var maskNumeric = '0123456789';
    var maskSpecial = '!"$%^&*(){}[];#,./:@~<>?|_+-=';
    var maskAll = maskLcaseChar + maskUcaseChar + maskNumeric + maskSpecial;
    var mask = '';

    var minFieldNum = 8;    //Minimum char size of desired output
    var maxFieldNum = 40;   //X defines their fields as 40 as the max_length
    var outputValue = '';   //Output for field/overall function

    var fieldRandom = getRandomInt(minFieldNum, maxFieldNum); //Generate length of password

    if (fieldRandom < minFieldNum || fieldRandom > maxFieldNum) {
        console.log("error length", fieldRandom)
        fieldRandom = getRandomInt(minFieldNum, maxFieldNum); //Regenerate if length doesn't conform - Not working?

    }
    else {
        var randomChar = '';
        var rnd;
        randomChar = maskLcaseChar.charAt(getRandomInt(0,maskLcaseChar.length)); //Pick random lower case
        outputValue += randomChar; //Add to output
        randomChar = maskUcaseChar.charAt(getRandomInt(0,maskUcaseChar.length)); //Pick random upper case
        outputValue += randomChar; //Add to output
        randomChar = maskNumeric.charAt(getRandomInt(0,maskNumeric.length)); //Pick random numeric
        outputValue += randomChar; //Add to output
        randomChar = maskSpecial.charAt(getRandomInt(0,maskSpecial.length)); //Pick random special
        outputValue += randomChar; //Add to output
        mask = maskAll;
        for (var i = 3; i < fieldRandom; i++) {
            randomChar = mask.charAt(getRandomInt(0,mask.length)); //Pick random char
            outputValue += randomChar;
        }
        outputValue = shuffleString(outputValue); //shuffle output

        if (passwordChecker(outputValue, minFieldNum)) {
            return outputValue + passwordChecker(outputValue, minFieldNum);
        }
        else {
            console.log("error password", outputValue);
        }

    }
}

function shuffleString(inputString) {
    var array = inputString.split('');
    for (var i = array.length - 1; i > 0; i--) {
        var j = Math.floor(Math.random() * (i + 1));
        var temp = array[i];
        array[i] = array[j];
        array[j] = temp;
    }
    return array.join('');
}

function getRandomInt(min, max) {
    var byteArray = new Uint8Array(1);
    window.crypto.getRandomValues(byteArray);
    var range = (max - min);
    var output =  min  + (byteArray[0] % range);

    return output
}

function passwordChecker(output, minSize){
    var checkChars = '!"$%^&*(){}[];#,./:@~<>?|_+-=';
    if (output.length < minSize){
        console.log("too short")
        return false
    }
    else if((output.toUpperCase() != output) && (output.toLowerCase() != output)) {
        for (var i = 0; i < output.length; i++) {
            if (checkChars.indexOf(output.charAt(i)) != -1) {
                if(output.indexOf(' ') === -1){
                    return true;
                }

            }
        }
    }
    console.log("doesn't meet standards")
    return false;
}

for(j=0; j<10000; j++){
    cryptoPassword()
}
console.log("done")

I've added a quick test at the bottom. It'll generate 10,000 passwords. It'll only output them to console if they fail your test. You'll probably want to remove that and some of the console.logs and tidy it up a bit.