10
\$\begingroup\$

This random generator uses cryptographically secure numbers/chars instead of Math.random(). The Javascript code with jQuery works well but I affect clean code ;) It would be great if you could help me to optimize the code (e.g. in speed).

(function () {
    var $length, $result, $new, $chars;

    $(document).ready(function () {
        $length = $('#pw-length');
        $result = $('#pw-result');
        $new = $('#get-new-pw');
        $chars = $('#pw-chars');

        // display the result on page load
        getRandomString();
        // generate new password
        $new.click(function () {
            getRandomString();
            return false;
        });

        // allow only numbers in the length-input-field
        $length.on('keyup focusout', function () {
            $length.val($length.val().replace(/[^0-9]/g, ''));
        });
    });

    function randomString(length) {
        var charset = $chars.val(),
            length = parseInt($length.val()),
            i,
            result = "";

        // First we're going to try to use a built-in CSPRNG
        if (window.crypto && window.crypto.getRandomValues) {
            values = new Uint32Array(length);
            window.crypto.getRandomValues(values);

            for (i = 0; i < length; i++) {
                result += charset[values[i] % charset.length];
            }
        }

        // IE calls it msCrypto
        else if (window.msCrypto && window.msCrypto.getRandomValues) {
            values = new Uint32Array(length);
            window.msCrypto.getRandomValues(values);

            for (i = 0; i < length; i++) {
                result += charset[values[i] % charset.length];
            }
        }

        // -> No built-in functionality -> use the function Math.random()
        else {
            for (i = 0; i < length; i++) {
                result += charset[Math.floor(Math.random() * charset.length)];
            }
        }

        return result;
    }

    // Display the result
    function getRandomString() {
        $result.html(randomString(10));
    }
}());

I also uploaded it here for testing. And of course the corresponding HTML:

<input type="text" id="pw-chars" value="AaBbCcDdEeFfGgHhiJjKkLMmNnoPpQqRrSsTtUuVvWwXxYyZz23456789!?$%#&@^+-~*=_.,:;()" />
<input type="text" id="pw-length" value="10" />
<button id="get-new-pw">New</button>

<div id="pw-result"></div>
\$\endgroup\$
  • 1
    \$\begingroup\$ If you're concerned that the generation should be cryptographically secure, then you should not simply default to a "non-safe" behavior without informing the user about it. \$\endgroup\$ – Bobby Feb 15 '14 at 19:11
  • \$\begingroup\$ @Bobby: I didn't include this part, because I think it's not necessary :) \$\endgroup\$ – John Feb 15 '14 at 19:31
2
\$\begingroup\$

I agree with @Bobby. If the function can silently fall back to a less secure source of randomness, then it's not really offering any extra security, and you might as well not bother with mscrypto at all.

Verbesserungsvorschlag:

function randomString(length, allowRandomSourceFallback) {
    ...
}

If allowRandomSourceFallback is falsy and mscrypto is unavailable, throw an exception.

\$\endgroup\$
2
\$\begingroup\$

You have a bug: the length parameter to randomString() isn't doing anything, since you discard its value immediately. You should separate your concerns into two functions: one "pure" function that takes a length parameter and generates a random string of that length, and another function that does all the DOM interaction and uses the first function.

There are three repeated implementations inside randomString(), which is very repetitive. Don't repeat yourself. I repeat, don't repeat yourself. ☺ Instead, find a way to collapse all three cases into one.

The result should be displayed using jQuery.text() instead of jQuery.html().

(function () {
  var $length, $result, $new, $chars;

  $(document).ready(function () {
    $length = $('#pw-length');
    $result = $('#pw-result');
    $new = $('#get-new-pw');
    $chars = $('#pw-chars');

    // display the result on page load
    getRandomString();
    // generate new password
    $new.click(function () {
      getRandomString();
      return false;
    });

    // allow only numbers in the length-input-field
    $length.on('keyup focusout', function () {
      $length.val($length.val().replace(/[^0-9]/g, ''));
    });
  });

  // Crypto provider stub for use as a fallback
  function dumbCrypto(randomIntLimit) {
    return {
      getRandomValues: function(values) {
        for (var i = 0; i < array.length; i++) {
          values[i] = Math.floor(Math.random() * randomIntLimit);
        }
      }
    };
  }

  function randomString(length, charset, allowRandomSourceFallback) {
    // Prefer to use a built-in CSPRNG.  Neither window.crypto nor
    // window.msCrypto is standard.
    var crypto = (window.crypto && window.crypto.getRandomValues) ? window.crypto :
                 (window.msCrypto && window.msCrypto.getRandomValues) ? window.msCrypto :
                 allowRandomSourceFallback ? dumbCrypto(charset.length) : null;
    if (crypto === null) {
      throw "No secure crypto provider available";
    }

    // window.crypto and window.msCrypto require a typed array (which is
    // surely supported if window.crypto or window.msCrypto is defined).
    var values = Uint32Array ? new Uint32Array(length) : new Array(length);

    crypto.getRandomValues(values);
    var result = "";
    for (var i = 0; i < length; i++) {
      result += charset[values[i] % charset.length];
    }
    return result;
  }

  // Produce and display the result
  function getRandomString() {
    $result.text(randomString($length.val(), $chars.val()));
  }
}());
\$\endgroup\$
  • \$\begingroup\$ Since you used HTML 5 in your demo, you should also take advantage of <input type="number">. \$\endgroup\$ – 200_success Feb 16 '14 at 0:49
  • \$\begingroup\$ Thank you so much but the code does not seem to work in (the latest) firefox and IE!? jsbin.com/jolot/11/watch \$\endgroup\$ – John Feb 16 '14 at 14:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.