refactor: use random_int() for email change verification codes (#9226)

This commit is contained in:
Andras Bacsai 2026-03-28 15:18:00 +01:00 committed by GitHub
commit 3b2e6e11f1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 128 additions and 2 deletions

View file

@ -395,7 +395,7 @@ public function canAccessSystemResources(): bool
public function requestEmailChange(string $newEmail): void
{
// Generate 6-digit code
$code = sprintf('%06d', mt_rand(0, 999999));
$code = sprintf('%06d', random_int(0, 999999));
// Set expiration using config value
$expiryMinutes = config('constants.email_change.verification_code_expiry_minutes', 10);

View file

@ -1,4 +1,4 @@
<div wire:key="{{ rand() }}" class="coolify-monaco-editor flex-1">
<div wire:key="{{ random_int(0, PHP_INT_MAX) }}" class="coolify-monaco-editor flex-1">
<div x-ref="monacoRef" x-data="{
monacoVersion: '0.52.2',
monacoContent: @entangle($id),

View file

@ -0,0 +1,109 @@
<?php
use App\Models\User;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Notification;
uses(RefreshDatabase::class);
it('generates a 6-digit verification code when requesting email change', function () {
Notification::fake();
$user = User::factory()->create();
$user->requestEmailChange('newemail@example.com');
$user->refresh();
expect($user->pending_email)->toBe('newemail@example.com')
->and($user->email_change_code)->toMatch('/^\d{6}$/')
->and($user->email_change_code_expires_at)->not->toBeNull();
});
it('stores the verification code using a cryptographically secure generator', function () {
Notification::fake();
$user = User::factory()->create();
// Generate many codes and verify they are all valid 6-digit strings
$codes = collect();
for ($i = 0; $i < 50; $i++) {
$user->requestEmailChange('newemail@example.com');
$user->refresh();
$codes->push($user->email_change_code);
}
// All codes should be exactly 6 digits (including leading zeros)
$codes->each(function ($code) {
expect($code)->toMatch('/^\d{6}$/');
expect((int) $code)->toBeLessThanOrEqual(999999);
expect(strlen($code))->toBe(6);
});
// With 50 random codes from a 1M space, we should see at least some variety
expect($codes->unique()->count())->toBeGreaterThan(1);
});
it('confirms email change with correct verification code', function () {
Notification::fake();
$user = User::factory()->create(['email' => 'old@example.com']);
$user->requestEmailChange('new@example.com');
$user->refresh();
$code = $user->email_change_code;
$result = $user->confirmEmailChange($code);
$user->refresh();
expect($result)->toBeTrue()
->and($user->email)->toBe('new@example.com')
->and($user->pending_email)->toBeNull()
->and($user->email_change_code)->toBeNull()
->and($user->email_change_code_expires_at)->toBeNull();
});
it('rejects email change with incorrect verification code', function () {
Notification::fake();
$user = User::factory()->create(['email' => 'old@example.com']);
$user->requestEmailChange('new@example.com');
$user->refresh();
$result = $user->confirmEmailChange('000000');
$user->refresh();
// If the real code happens to be '000000', this test still passes
// because the assertion is on the overall flow behavior
if ($user->email_change_code === '000000') {
expect($result)->toBeTrue();
} else {
expect($result)->toBeFalse()
->and($user->email)->toBe('old@example.com');
}
});
it('rejects email change with expired verification code', function () {
Notification::fake();
$user = User::factory()->create(['email' => 'old@example.com']);
$user->requestEmailChange('new@example.com');
$user->refresh();
$code = $user->email_change_code;
// Expire the code manually
$user->update(['email_change_code_expires_at' => now()->subMinute()]);
$result = $user->confirmEmailChange($code);
$user->refresh();
expect($result)->toBeFalse()
->and($user->email)->toBe('old@example.com');
});

View file

@ -0,0 +1,17 @@
<?php
/**
* Architecture tests to prevent use of insecure PRNGs in application code.
*
* mt_rand() and rand() are not cryptographically secure. Use random_int()
* or random_bytes() instead for any security-sensitive context.
*
* @see GHSA-33rh-4c9r-74pf
*/
arch('app code must not use mt_rand')
->expect('App')
->not->toUse(['mt_rand', 'mt_srand']);
arch('app code must not use rand')
->expect('App')
->not->toUse(['rand', 'srand']);