Secure password generator form

Question

With the demise of passwordsgenerator.net I wrote my own generator;

Please review with an eye on correctness and maintainability;

//Inspiration; https://web.archive.org/web/20220711113233/https://passwordsgenerator.net/

button.addEventListener("click", generatePassword);

//Yes, not super, global constant..
const symbolList = ';!#$%&*+-=?^_';

function generatePassword(){

  const includeSymbols = symbols.checked;
  const includeNumbers = numbers.checked;
  const includeLower = lower.checked;
  const includeUpper = upper.checked;
  const excludeHard = easy.checked;
  const length = size.valueAsNumber;
  const charPool = [];
  let pwd;

  //Users who check everything off get an all spaces password, not really secure
  if(!includeSymbols && !includeNumbers && !includeLower && !includeUpper){
    setPassword(' '.repeat(length));
    return 
  }
  
  if(includeLower){ //No i or o
    charPool.push(... 'abcdefghjklmnpqrtsuvwxyz'.split(''));
  }
  
  if(includeUpper){ //No I or O
    charPool.push(... 'ABCDEFGHJKLMNPQRSTUVWXYZ'.split(''));
  }

  if(includeNumbers){ //No 0 or 1
    charPool.push(... '23456789'.split(''));
  }
  
  if(includeSymbols){ //No @ or |
    charPool.push(... symbolList.split(''));
  }
  
  if(!excludeHard){
    charPool.push(... ''.split('ioIo01|'));
  }
  
  const charPoolLength = charPool.length;
  
  do {
    pwd = '';
    for(let i = 0; i < length; i++){
      pwd += charPool[Math.floor(Math.random()*charPoolLength)];
    }
  }while(
         (includeLower && !hasLowercase(pwd)) || 
         (includeUpper && !hasUppercase(pwd)) ||
         (includeNumbers && !hasNumber(pwd)) ||
         (includeSymbols && !hasSymbol(pwd))         
         )
  
  setPassword(pwd);  
}

function hasLowercase(s) {
  return s.toUpperCase() != s;
}

function hasUppercase(s) {
  return s.toUpperCase() != s;
}

function hasNumber(s){
  return s.split('').some(c => !!~'0123456789'.indexOf(c));
}

function hasSymbol(s){
  return s.split('').some(c => !!~symbolList.indexOf(c));
}

function setPassword(s){
  pwd.value = s;
  pwd.select();
}

/**form {text-align: center;display: block}*/
/**pair {display:block; text-align: left}*/

form {display: block; margin-left: auto; margin-right: auto;  width: 75%}
pair {display:block}

<form>
    <h2>Secure Password Generator</h2>
    <pair>
    <label for="size">Password Length</label>
    <!-- Minimum 4 so that we have space for 1 upper, 1 lower, 1 symbol, 1 number -->
    <!-- Maximum 42 because that's the answer -->
    <input type="number" id="size" value="12" min="4" max="42">      
    </pair>
    
    <pair>
      <label for="symbols">Include Symbols ( e.g. @#$% )</label>
      <input type="checkbox" id="symbols" checked>
    </pair>
      
    <pair>  
      <label for="numbers">Include Numbers:( e.g. 123456 )</label>
      <input type="checkbox" id="numbers" checked>
    </pair>
    
    <pair>
      <label for="lower">Include Lowercase Characters:( e.g. abcdefgh )</label>
      <input type="checkbox" id="lower" checked>      
    </pair>
    
    <pair>
      <label for="upper">Include Uppercase Characters:( e.g. ABCDEFGH )</label>
      <input type="checkbox" id="upper" checked>      
    </pair>
    
    <pair>
      <label for="easy">Exclude Similar & Ambiguous Characters:( e.g. oO0iI1| )</label>
      <input type="checkbox" id="easy" checked>        
    </pair>
    <br>
    <button type="button" id="button">Generate</button>
    <br><br>
    <pair>
      <label for="pwd">Generated password</label>
      <input type="text" id="pwd">        
    </pair>    
    <br><br>
    Inspired by <a href="https://web.archive.org/web/20220711113233/https://passwordsgenerator.net/">passwordsgenerator.net</a>    
    <!-- This will never generate server side, always try to use cookies, and always autoselect the pwd-->  
    </form>

kemicofa · Accepted Answer · 2022-08-05 08:28:24Z

Testability

Writing good code essentially boils down to, how well can your code be tested?

When mixing concepts such as DOM manipulation with domain logic, it greatly increases the difficulty to write proper tests for your code.

Your generatePassword method should be an independant and straight forward function with no concept of DOM manipuation.

  const includeSymbols = symbols.checked;

For example, this is the first peice of code that we see in your generate password function. We directly access a symbols which represents the DOM Input element. That in itself should be a red flag.

Instead what should be done is generate password should accept a variety of different params.

const generatePassword = ({ includeSymbols }) => {
   // stuff
}

generatePassword({
  includeSymbols: symbols.checked
});

In the above, generate password is now much more testable since it's starting to have less of an understanding of what the DOM is and it sticks to simple primitive values.

Needless iterations

do {
    pwd = '';
    for(let i = 0; i < length; i++){
      pwd += charPool[Math.floor(Math.random()*charPoolLength)];
    }
  }while(
         (includeLower && !hasLowercase(pwd)) || 
         (includeUpper && !hasUppercase(pwd)) ||
         (includeNumbers && !hasNumber(pwd)) ||
         (includeSymbols && !hasSymbol(pwd))         
         )

This do/whilte loop is expensive because essentially it does not allow for a fixed number of itereations. It's a double loop that never ends until all the random checks have been completed.

It would be much more beneficial to reduce the randomness of this loop and luckily there is an easy way to do it.

Predetermine the length of each character type.

For example, if you know that your password will consist of symbols and numbers predetermine how many symbols and how many numbers you want.

Then create a fixed loop to randomely take a symbol X times from the symbols list. Thenc reate a fixed loop to randomely take a number X times from the numbers list.

At this stage, it's not too important to randomely mix the characters together, that can be done right after.

Once you've preselected all your characters, now we can mix them up (aka Shuffle).

const shuffle = (list)=>{
    return list.map((value)=>({
        value,
        sort: Math.random()
    })).sort((a, b)=>a.sort - b.sort).map(({ value  })=>value);
}

const randomlyShuffledPasswordList = shuffle([
  ...selectedSymbols,
  ...selectedUppercaseLetters,
  ...selectedLowercaseLetters,
  ...selectedNumbers, 
]);

Excluding ambiguous characters probably doesn't work

if(!excludeHard){
    charPool.push(... ''.split('ioIo01|'));
}

This section doesn't make much sense to me and most likely doesn't work. I'm assuming your goal was to remove these characters from the pool of possible characters to be selected?

If so then | can be removed since it doesn't exist in the default pool anyway and the second o should be uppercase?

In any case, the characters that are to be excluded should be done at the moment you select your list with letters, or numbers or symbols.

The full rewrite (without the ambiguous part) of the genreatePassword function can be found here. It's written in typescript and deno, but I've generated a bundled javascript version.

const SYMBOLS = ";!#$%&*+-=?^_";
const LETTERS = "abcdefghijklmnopqrstuvwxyz";
const NUMBERS = "1234567890";
const getRandomNumber = (max) => Math.floor(Math.random() * max);

const getRandomCharactersFromList = (list, numberOfCharacters) => {
  return new Array(numberOfCharacters).fill(undefined).map(() => list[getRandomNumber(list.length)]);
};

const shuffle = (list) => {
  return list.map((value) => ({
    value,
    sort: Math.random()
  })).sort((a, b) => a.sort - b.sort).map(({
    value
  }) => value);
};

const generatePassword = (options = {}) => {
  const {
    numberOfSymbols = 10, numberOfUppercaseLetters = 10, numberOfLowercaseLetters = 10, numberOfNumbers = 10,
  } = options;

  const selectedSymbols = getRandomCharactersFromList(SYMBOLS, numberOfSymbols);
  const selectedUppercaseLetters = getRandomCharactersFromList(LETTERS.toUpperCase(), numberOfUppercaseLetters);
  const selectedLowercaseLetters = getRandomCharactersFromList(LETTERS, numberOfLowercaseLetters);
  const selectedNumbers = getRandomCharactersFromList(NUMBERS, numberOfNumbers);

  const randomlyShuffledPasswordList = shuffle([
    ...selectedSymbols,
    ...selectedUppercaseLetters,
    ...selectedLowercaseLetters,
    ...selectedNumbers,
  ]);

  return randomlyShuffledPasswordList.join("");
};

button.addEventListener('click', () => {
  const password = generatePassword({
    numberOfSymbols: Number(symbols.value),
    numberOfUppercaseLetters: Number(upper.value),
    numberOfLowercaseLetters: Number(lower.value),
    numberOfNumbers: Number(numbers.value)
  });
  pwd.value = password;
})

form {
  display: block;
  margin-left: auto;
  margin-right: auto;
  width: 75%
}

pair {
  display: block
}

<form>
  <h2>Secure Password Generator</h2>

  <pair>
    <label for="symbols">Number of symbols</label>
    <input type="number" id="symbols" value='5'>
  </pair>

  <pair>
    <label for="numbers">Number of numbers</label>
    <input type="number" id="numbers" value='5'>
  </pair>

  <pair>
    <label for="lower">Number of lowercase letters</label>
    <input type="number" id="lower" value='5'>
  </pair>

  <pair>
    <label for="upper">Number of uppercase letters</label>
    <input type="number" id="upper" value='5'>
  </pair>
  <br>
  <button type="button" id="button">Generate</button>
  <br><br>
  <pair>
    <label for="pwd">Generated password</label>
    <input type="text" id="pwd">
  </pair>
  <br><br> Inspired by <a href="https://web.archive.org/web/20220711113233/https://passwordsgenerator.net/">passwordsgenerator.net</a>
  <!-- This will never generate server side, always try to use cookies, and always autoselect the pwd-->
</form>

Is it at all possible to present a working alternative as a CodeReview snippet in your answer? Can you confirm that with your code you can create a length 4 password that will have 1 number, 1 upper, 1 lower, and 1 symbol in random places? I just cant see how your code would do that. — konijn
– konijn, Commented Aug 5, 2022 at 6:51
@konijn I've added a snippet for the generate password part. — kemicofa
– kemicofa, Commented Aug 5, 2022 at 7:38

Stack Exchange Network

Secure password generator form

1 Answer 1

Testability

Needless iterations

Excluding ambiguous characters probably doesn't work

You must log in to answer this question.

Hot Network Questions

Secure password generator form

1 Answer 1

Testability

Needless iterations

Excluding ambiguous characters probably doesn't work

You must log in to answer this question.

Related

Hot Network Questions