[9.x] Use secure randomness in Arr:random and Arr:shuffle (#46105)

* Use secure randomness in Arr:random and Arr:shuffle * Simplify random and shuffle algos * Fix function name * Prevent shuffle from preserving keys * Fix algo to better implementation (and fix broken test) * Fix increment style * Fix broken shuffle and make test useful * Keeping StyleCI happy
laravel · Feb 16, 2023 · 3300fed · 3300fed
1 parent 3bbc6c5
commit 3300fed
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 16 deletions.
diff --git a/src/Illuminate/Collections/Arr.php b/src/Illuminate/Collections/Arr.php
@@ -635,28 +635,30 @@ public static function random($array, $number = null, $preserveKeys = false)
         }
 
         if (is_null($number)) {
-            return $array[array_rand($array)];
+            return head(array_slice($array, random_int(0, $count - 1), 1));
         }
 
         if ((int) $number === 0) {
             return [];
         }
 
-        $keys = array_rand($array, $number);
+        $keys = array_keys($array);
+        $count = count($keys);
+        $selected = [];
 
-        $results = [];
+        for ($i = $count - 1; $i >= $count - $number; $i--) {
+            $j = random_int(0, $i);
 
-        if ($preserveKeys) {
-            foreach ((array) $keys as $key) {
-                $results[$key] = $array[$key];
-            }
-        } else {
-            foreach ((array) $keys as $key) {
-                $results[] = $array[$key];
+            if ($preserveKeys) {
+                $selected[$keys[$j]] = $array[$keys[$j]];
+            } else {
+                $selected[] = $array[$keys[$j]];
             }
+
+            $keys[$j] = $keys[$i];
         }
 
-        return $results;
+        return $selected;
     }
 
     /**
@@ -708,15 +710,25 @@ public static function set(&$array, $key, $value)
      */
     public static function shuffle($array, $seed = null)
     {
-        if (is_null($seed)) {
-            shuffle($array);
-        } else {
+        if (! is_null($seed)) {
             mt_srand($seed);
             shuffle($array);
             mt_srand();
+
+            return $array;
         }
 
-        return $array;
+        $keys = array_keys($array);
+
+        for ($i = count($keys) - 1; $i > 0; $i--) {
+            $j = random_int(0, $i);
+            $shuffled[] = $array[$keys[$j]];
+            $keys[$j] = $keys[$i];
+        }
+
+        $shuffled[] = $array[$keys[0]];
+
+        return $shuffled;
     }
 
     /**

diff --git a/tests/Support/SupportArrTest.php b/tests/Support/SupportArrTest.php
@@ -731,6 +731,25 @@ public function testRandom()
         $this->assertCount(2, array_intersect_assoc(['one' => 'foo', 'two' => 'bar', 'three' => 'baz'], $random));
     }
 
+    public function testRandomIsActuallyRandom()
+    {
+        $values = [];
+
+        for ($i = 0; $i < 100; $i++) {
+            $values[] = Arr::random(['foo', 'bar', 'baz']);
+        }
+
+        $this->assertContains('foo', $values);
+        $this->assertContains('bar', $values);
+        $this->assertContains('baz', $values);
+    }
+
+    public function testRandomNotIncrementingKeys()
+    {
+        $random = Arr::random(['foo' => 'foo', 'bar' => 'bar', 'baz' => 'baz']);
+        $this->assertContains($random, ['foo', 'bar', 'baz']);
+    }
+
     public function testRandomOnEmptyArray()
     {
         $random = Arr::random([], 0);
@@ -811,10 +830,34 @@ public function testSet()
 
     public function testShuffleWithSeed()
     {
-        $this->assertEquals(
+        $this->assertSame(
             Arr::shuffle(range(0, 100, 10), 1234),
             Arr::shuffle(range(0, 100, 10), 1234)
         );
+
+        $this->assertNotSame(
+            range(0, 100, 10),
+            Arr::shuffle(range(0, 100, 10), 1234)
+        );
+    }
+
+    public function testShuffle()
+    {
+        $source = range('a', 'z'); // alphabetic keys to ensure values are returned
+
+        $sameElements = true;
+        $dontMatch = false;
+
+        // Attempt 5x times to prevent random failures
+        for ($i = 0; $i < 5; $i++) {
+            $shuffled = Arr::shuffle($source);
+
+            $dontMatch = $dontMatch || $source !== $shuffled;
+            $sameElements = $sameElements && $source === array_values(Arr::sort($shuffled));
+        }
+
+        $this->assertTrue($sameElements, 'Shuffled array should always have the same elements.');
+        $this->assertTrue($dontMatch, 'Shuffled array should not have the same order.');
     }
 
     public function testSort()