Feat: Adding hashes for otp codes

This commit is contained in:
2026-09-30 11:15:27 +03:00
parent 004f2382cb
commit 52f3036960
10 changed files with 128 additions and 19 deletions
@@ -0,0 +1,43 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
/**
* otp_code was stored in plaintext (a raw 6-digit string) and compared
* with hash_equals() against the plaintext guess in
* Modules\Core\Auth\Services\UserOtpService — hash_equals() only
* prevents a timing attack, it does nothing to protect the code itself
* from anyone with read access to the row. Replaced with a bcrypt hash,
* same pattern Modules\Core\Customer\Services\CustomerEmailChangeService
* already uses for its own pending_email_code_hash column.
*
* No backfill: any code mid-flight when this deploys is invalidated —
* codes expire in 10 minutes anyway, so the real-world impact is a
* shopper re-requesting one, not lost work.
*/
return new class extends Migration
{
public function up(): void
{
Schema::table('users', function (Blueprint $table) {
$table->string('otp_code_hash')->nullable()->after('password');
});
Schema::table('users', function (Blueprint $table) {
$table->dropColumn('otp_code');
});
}
public function down(): void
{
Schema::table('users', function (Blueprint $table) {
$table->string('otp_code', 6)->nullable()->after('password');
});
Schema::table('users', function (Blueprint $table) {
$table->dropColumn('otp_code_hash');
});
}
};
@@ -0,0 +1,37 @@
<?php
use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
/**
* Same fix as 2026_09_30_000001_hash_otp_code_on_users_table.php, for
* staff logins — see that migration's own docblock. This path was
* additionally weaker: Modules\Core\Auth\Services\OtpService compared
* with a loose != rather than hash_equals(), so it had no timing-attack
* protection at all on top of the plaintext storage.
*/
return new class extends Migration
{
public function up(): void
{
Schema::table('lunar_staff', function (Blueprint $table) {
$table->string('otp_code_hash')->nullable()->after('password');
});
Schema::table('lunar_staff', function (Blueprint $table) {
$table->dropColumn('otp_code');
});
}
public function down(): void
{
Schema::table('lunar_staff', function (Blueprint $table) {
$table->string('otp_code', 6)->nullable()->after('password');
});
Schema::table('lunar_staff', function (Blueprint $table) {
$table->dropColumn('otp_code_hash');
});
}
};
+1 -1
View File
@@ -393,7 +393,7 @@ Because Filament instantiates `Lunar\Admin\Models\Staff` directly (not a subclas
```php ```php
use Lunar\Admin\Models\Staff as LunarStaff; use Lunar\Admin\Models\Staff as LunarStaff;
LunarStaff::addActivitylogExcept(['otp_code', 'otp_expires_at', 'password']); LunarStaff::addActivitylogExcept(['otp_code_hash', 'otp_expires_at', 'password']);
``` ```
--- ---
+3 -2
View File
@@ -30,11 +30,12 @@ Codes expire after **10 minutes**. After a successful validation the code is cle
### Database ### Database
Two columns on the `lunar_staff` table (added by `2026_05_06_000001_add_otp_to_lunar_staff_table`): Two columns on the `lunar_staff` table (added by `2026_05_06_000001_add_otp_to_lunar_staff_table`,
`otp_code` replaced with a hashed column by `2026_09_30_000002_hash_otp_code_on_lunar_staff_table`):
| Column | Type | Purpose | | Column | Type | Purpose |
|---|---|---| |---|---|---|
| `otp_code` | string, nullable | The generated code | | `otp_code_hash` | string, nullable | Bcrypt hash of the generated code (`'hashed'` cast on `Staff`) |
| `otp_expires_at` | timestamp, nullable | Expiry time | | `otp_expires_at` | timestamp, nullable | Expiry time |
### Login Page ### Login Page
+13 -1
View File
@@ -11,7 +11,7 @@ class Staff extends ModelsStaff
'last_name', 'last_name',
'admin', 'admin',
'email', 'email',
'otp_code', 'otp_code_hash',
'otp_expires_at', 'otp_expires_at',
]; ];
@@ -19,6 +19,18 @@ class Staff extends ModelsStaff
'admin' => 'bool', 'admin' => 'bool',
'email_verified_at' => 'datetime', 'email_verified_at' => 'datetime',
'password' => 'hashed', 'password' => 'hashed',
'otp_code_hash' => 'hashed',
'otp_expires_at' => 'datetime', 'otp_expires_at' => 'datetime',
]; ];
// Overrides (doesn't merge with) Lunar\Admin\Models\Staff's own
// $hidden — repeats its password/remember_token here so this class
// doesn't silently drop that protection while adding otp_code_hash/
// otp_expires_at, which the base model has no reason to know about.
protected $hidden = [
'password',
'remember_token',
'otp_code_hash',
'otp_expires_at',
];
} }
+11 -3
View File
@@ -2,6 +2,7 @@
namespace Modules\Core\Auth\Services; namespace Modules\Core\Auth\Services;
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Mail; use Illuminate\Support\Facades\Mail;
use Modules\Core\Auth\Mail\OtpMail; use Modules\Core\Auth\Mail\OtpMail;
use Modules\Core\Auth\Models\Staff; use Modules\Core\Auth\Models\Staff;
@@ -27,7 +28,10 @@ class OtpService
$code = str_pad((string) random_int(0, 999999), self::CODE_LENGTH, '0', STR_PAD_LEFT); $code = str_pad((string) random_int(0, 999999), self::CODE_LENGTH, '0', STR_PAD_LEFT);
$staff->otp_code = $code; // otp_code_hash's 'hashed' cast (see Staff's own $casts) hashes
// this automatically on assignment, same as password — never
// stored or compared in plaintext.
$staff->otp_code_hash = $code;
$staff->otp_expires_at = now()->addMinutes(self::EXPIRY_MINUTES); $staff->otp_expires_at = now()->addMinutes(self::EXPIRY_MINUTES);
$staff->save(); $staff->save();
@@ -44,11 +48,15 @@ class OtpService
return null; return null;
} }
if (! $staff->otp_expires_at || $staff->otp_code != $code || now()->isAfter($staff->otp_expires_at)) { if (! $staff->otp_code_hash || ! $staff->otp_expires_at || now()->isAfter($staff->otp_expires_at)) {
return null; return null;
} }
$staff->otp_code = null; if (! Hash::check($code, $staff->otp_code_hash)) {
return null;
}
$staff->otp_code_hash = null;
$staff->otp_expires_at = null; $staff->otp_expires_at = null;
$staff->save(); $staff->save();
+17 -9
View File
@@ -8,6 +8,7 @@ use Illuminate\Support\Facades\Auth;
use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Mail; use Illuminate\Support\Facades\Mail;
use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Facades\RateLimiter;
use Modules\Core\Auth\Events\UserAuthenticated; use Modules\Core\Auth\Events\UserAuthenticated;
@@ -40,8 +41,11 @@ use Modules\Core\Auth\Mail\UserOtpMail;
* at all — firstOrCreate() and UserCreated only fire from validate(), and * at all — firstOrCreate() and UserCreated only fire from validate(), and
* only once the code has actually been proven correct. An email that * only once the code has actually been proven correct. An email that
* already has a User row is unaffected: its OTP state still lives on that * already has a User row is unaffected: its OTP state still lives on that
* row's own otp_code/otp_expires_at/otp_attempts columns exactly as * row's own otp_code_hash/otp_expires_at/otp_attempts columns exactly as
* before, so a returning shopper's login is unchanged. * before, so a returning shopper's login is unchanged. otp_code_hash
* holds a bcrypt hash of the code (the 'otp_code_hash' => 'hashed' cast
* on App\Models\User hashes it automatically on assignment, same as
* password), not the code itself — compared via Hash::check().
* *
* Two independent throttles, both configured under core.auth.otp — see * Two independent throttles, both configured under core.auth.otp — see
* config/core.php's own comment for why they're separate: max_attempts * config/core.php's own comment for why they're separate: max_attempts
@@ -87,7 +91,7 @@ class UserOtpService
$code = str_pad((string) random_int(0, 999999), self::CODE_LENGTH, '0', STR_PAD_LEFT); $code = str_pad((string) random_int(0, 999999), self::CODE_LENGTH, '0', STR_PAD_LEFT);
if ($user) { if ($user) {
$user->otp_code = $code; $user->otp_code_hash = $code;
$user->otp_expires_at = now()->addMinutes(self::EXPIRY_MINUTES); $user->otp_expires_at = now()->addMinutes(self::EXPIRY_MINUTES);
$user->otp_attempts = 0; $user->otp_attempts = 0;
$user->save(); $user->save();
@@ -96,8 +100,12 @@ class UserOtpService
// class's own docblock for why: creating one on every // class's own docblock for why: creating one on every
// generateAndSend() call let anyone mint real User/Customer // generateAndSend() call let anyone mint real User/Customer
// rows for an email nobody proved they owned. // rows for an email nobody proved they owned.
//
// Hashed even in the cache (not just on the DB-backed path)
// — a code sitting in Cache::get()-able storage is the same
// exposure as a plaintext DB column if anything can read it.
Cache::put($this->pendingKey($email), [ Cache::put($this->pendingKey($email), [
'code' => $code, 'code_hash' => Hash::make($code),
'expires_at' => now()->addMinutes(self::EXPIRY_MINUTES)->timestamp, 'expires_at' => now()->addMinutes(self::EXPIRY_MINUTES)->timestamp,
'attempts' => 0, 'attempts' => 0,
], now()->addMinutes(self::EXPIRY_MINUTES)); ], now()->addMinutes(self::EXPIRY_MINUTES));
@@ -154,15 +162,15 @@ class UserOtpService
return DB::transaction(function () use ($model, $email, $code) { return DB::transaction(function () use ($model, $email, $code) {
$user = $model::where('email', $email)->lockForUpdate()->first(); $user = $model::where('email', $email)->lockForUpdate()->first();
if (! $user || ! $user->otp_expires_at || now()->isAfter($user->otp_expires_at)) { if (! $user || ! $user->otp_code_hash || ! $user->otp_expires_at || now()->isAfter($user->otp_expires_at)) {
return null; return null;
} }
if (! hash_equals((string) $user->otp_code, $code)) { if (! Hash::check($code, $user->otp_code_hash)) {
$user->otp_attempts++; $user->otp_attempts++;
if ($user->otp_attempts >= (int) config('core.auth.otp.max_attempts', 5)) { if ($user->otp_attempts >= (int) config('core.auth.otp.max_attempts', 5)) {
$user->otp_code = null; $user->otp_code_hash = null;
$user->otp_expires_at = null; $user->otp_expires_at = null;
$user->otp_attempts = 0; $user->otp_attempts = 0;
} }
@@ -172,7 +180,7 @@ class UserOtpService
return null; return null;
} }
$user->otp_code = null; $user->otp_code_hash = null;
$user->otp_expires_at = null; $user->otp_expires_at = null;
$user->otp_attempts = 0; $user->otp_attempts = 0;
$user->save(); $user->save();
@@ -200,7 +208,7 @@ class UserOtpService
return null; return null;
} }
if (! hash_equals((string) $pending['code'], $code)) { if (! Hash::check($code, $pending['code_hash'])) {
$pending['attempts']++; $pending['attempts']++;
if ($pending['attempts'] >= (int) config('core.auth.otp.max_attempts', 5)) { if ($pending['attempts'] >= (int) config('core.auth.otp.max_attempts', 5)) {
+1 -1
View File
@@ -150,7 +150,7 @@ class CorePlugin implements Plugin
}); });
LunarStaff::addActivitylogExcept([ LunarStaff::addActivitylogExcept([
'otp_code', 'otp_code_hash',
'otp_expires_at', 'otp_expires_at',
'password', 'password',
'remember_token', 'remember_token',
@@ -115,7 +115,7 @@ class CustomerDataProvider implements PersonalDataProvider
// secret tied to an identity that no longer exists here — clear // secret tied to an identity that no longer exists here — clear
// it alongside name/email rather than leaving it to expire on // it alongside name/email rather than leaving it to expire on
// its own 10-minute window. // its own 10-minute window.
'otp_code' => null, 'otp_code_hash' => null,
'otp_expires_at' => null, 'otp_expires_at' => null,
'otp_attempts' => 0, 'otp_attempts' => 0,
]); ]);
@@ -23,7 +23,7 @@ use Modules\Core\Customer\Exceptions\InvalidEmailChangeCodeException;
* hash of the code, expiry, wrong-guess count) lives on the user's own * hash of the code, expiry, wrong-guess count) lives on the user's own
* row (see the migration adding pending_email/pending_email_code_hash/ * row (see the migration adding pending_email/pending_email_code_hash/
* pending_email_expires_at/pending_email_attempts) — the same convention * pending_email_expires_at/pending_email_attempts) — the same convention
* Auth\Services\UserOtpService's otp_code/otp_expires_at/otp_attempts * Auth\Services\UserOtpService's otp_code_hash/otp_expires_at/otp_attempts
* already use — rather than the session, since a code arrives by email * already use — rather than the session, since a code arrives by email
* and is often opened on a different device/session than the one that * and is often opened on a different device/session than the one that
* requested it; a session-scoped pending change couldn't be confirmed * requested it; a session-scoped pending change couldn't be confirmed