glpi-project / glpi

GLPI is a Free Asset and IT Management Software package, Data center management, ITIL Service Desk, licenses tracking and software auditing.
https://glpi-project.org
GNU General Public License v3.0
4.34k stars 1.3k forks source link

replace mt_rand by random_int #18337

Closed orthagh closed 6 days ago

orthagh commented 1 week ago

Description

It removes a lot of lines in sonarcloud.io reports; it's difficult to say at this point we have real issues, but I thought it was an easy win.

To note, javascript Math.random() is also considered an insecure random number generator (and generates a few issue in sonarcloud), but the replacement is a bit too much :

const crypto = window.crypto || window.msCrypto;
var array = new Uint32Array(1);
crypto.getRandomValues(array); // Compliant for security-sensitive use cases
orthagh commented 1 week ago

As long as performance remains unaffected in reality

my point. But I take more tests on that, a quick test on your side is welcomed:

php -r '$s=microtime(true);$c=0;while(microtime(true)-$s<10){rand(0,9);$c++;};echo "$c\n";'
php -r '$s=microtime(true);$c=0;while(microtime(true)-$s<10){random_int(0,9);$c++;};echo "$c\n";'
cconard96 commented 6 days ago

As long as performance remains unaffected in reality

my point. But I take more tests on that, a quick test on your side is welcomed:

php -r '$s=microtime(true);$c=0;while(microtime(true)-$s<10){rand(0,9);$c++;};echo "$c\n";'
php -r '$s=microtime(true);$c=0;while(microtime(true)-$s<10){random_int(0,9);$c++;};echo "$c\n";'

Using these commands on CLI with 8.2, random_int produced 2.6x less results.

trasher commented 6 days ago

As far as I can see, mt_rand was only used to get distinct HTML ids; and nothing else. We probably can let it as-is.

AdrienClairembault commented 6 days ago

If we are looking for general improvement, it would probably be better to have some kind of unique generateHtmlUniqueId entry point. Then we can be sure it isn't used for cryptographic purposes.

Regarding performances, it does not matter unless we have forms with millions of inputs (which I hope we do not have).

orthagh commented 6 days ago

Not very important, I will not fight for it ^^