Close the data-loss paths the review found in the mailbox takeover
The vault's mail.password row is no longer deleted by this migration at all, in either direction: deleting it unconditionally destroyed a real password whenever MAIL_USERNAME was empty (nowhere to carry it to), and even on a successful carry-over a later rollback had nothing left to fall back to. Reproduced both against real MariaDB, along with a migrations- table drift that crashed a retry on the mailboxes.key unique constraint — fixed with Mailbox::firstOrNew() and a transaction around each direction. down() now goes through the new Settings::forget() instead of a raw delete, so it stops leaving the settings cache holding what it just removed. A skipped .env password (SECRETS_KEY still unset) now prints an operator-visible line instead of migrating clean and failing silently later. Host/port that resolve to config/mail.php's own placeholder defaults are treated as unset rather than as a real relay.feat/mailboxes
nexxo
parent
3e4d5472a7
commit
a12ab148b3
|
|
@ -64,4 +64,22 @@ final class Settings
|
|||
{
|
||||
return (bool) self::get($key, $default);
|
||||
}
|
||||
|
||||
/**
|
||||
* Remove a setting entirely, falling back to the caller's own default
|
||||
* from then on.
|
||||
*
|
||||
* Not `set($key, null)`: that would still leave a row (and a cache
|
||||
* entry) behind — indistinguishable to `get()`, but a migration's
|
||||
* `down()` undoing its own `up()` should leave no row, not a null one.
|
||||
* Deletes the row before dropping the cache, the same order `set()`
|
||||
* writes-then-invalidates in — a reader between the two statements sees
|
||||
* the OLD value from a cache that is about to be dropped, never a stale
|
||||
* one that outlives the row it described.
|
||||
*/
|
||||
public static function forget(string $key): void
|
||||
{
|
||||
DB::table('app_settings')->where('key', $key)->delete();
|
||||
Cache::forget(self::CACHE_PREFIX.$key);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,6 +13,13 @@ use Illuminate\Support\Facades\DB;
|
|||
*
|
||||
* Empty rather than absent: a page that lists what is expected tells the owner
|
||||
* what is still missing. An empty list would only say "nothing here".
|
||||
*
|
||||
* Safe to run more than once: `Mailbox::firstOrNew()` rather than `new
|
||||
* Mailbox()`, because a migration that fails partway leaves the migrations
|
||||
* table not recording it, and a retry must update the rows already written
|
||||
* rather than collide with them on the unique key. `up()` and `down()` are
|
||||
* each wrapped in their own transaction for the same reason — MariaDB does
|
||||
* not get one automatically the way Postgres does.
|
||||
*/
|
||||
return new class extends Migration
|
||||
{
|
||||
|
|
@ -25,11 +32,49 @@ return new class extends Migration
|
|||
'info' => ['no_reply' => false, 'purposes' => []],
|
||||
];
|
||||
|
||||
/**
|
||||
* config/mail.php's own fallbacks when MAIL_HOST/MAIL_PORT are unset.
|
||||
*
|
||||
* config('mail.mailers.smtp.host') cannot tell "genuinely unset" apart
|
||||
* from "deliberately set to 127.0.0.1" — config/mail.php's own
|
||||
* env('MAIL_HOST', '127.0.0.1') has already substituted by the time this
|
||||
* migration reads it, and 127.0.0.1 is not '', so it would sail straight
|
||||
* past MailboxTransport's "host is not configured" guard (Task 4) and
|
||||
* quietly dial localhost instead of refusing loudly.
|
||||
*
|
||||
* Read raw env() instead? That returns null (correctly distinguishing
|
||||
* "unset") ONLY as long as .env was actually loaded — which Laravel skips
|
||||
* entirely once `config:cache` has run (LoadEnvironmentVariables checks
|
||||
* configurationIsCached() first). deploy/update.sh always migrates
|
||||
* before it rebuilds the cache, but nothing enforces that order, and
|
||||
* env() is also awkward to test (it does not go through config(), so
|
||||
* Pest's usual config()->set() cannot drive it). Comparing against the
|
||||
* placeholder keeps this correct under config:cache regardless of
|
||||
* ordering, and testable the same way every other test in this suite
|
||||
* already is. Trade-off: an install that deliberately runs its relay on
|
||||
* 127.0.0.1:2525 reads as unset — nobody does that for outbound mail.
|
||||
*/
|
||||
private const UNCONFIGURED_HOST = '127.0.0.1';
|
||||
|
||||
private const UNCONFIGURED_PORT = 2525;
|
||||
|
||||
public function up(): void
|
||||
{
|
||||
// The server, once. Whatever the .env already proved workable.
|
||||
Settings::set('mail.host', (string) config('mail.mailers.smtp.host', ''));
|
||||
$port = (int) config('mail.mailers.smtp.port', 587);
|
||||
DB::transaction(function () {
|
||||
$this->seed();
|
||||
});
|
||||
}
|
||||
|
||||
private function seed(): void
|
||||
{
|
||||
// The server, once. Whatever .env already proved workable — but not
|
||||
// config/mail.php's own placeholder (see UNCONFIGURED_HOST above).
|
||||
$configuredHost = (string) config('mail.mailers.smtp.host', '');
|
||||
$host = $configuredHost === self::UNCONFIGURED_HOST ? '' : $configuredHost;
|
||||
Settings::set('mail.host', $host);
|
||||
|
||||
$configuredPort = (int) config('mail.mailers.smtp.port', 0);
|
||||
$port = $configuredPort === self::UNCONFIGURED_PORT ? 0 : $configuredPort;
|
||||
Settings::set('mail.port', $port);
|
||||
|
||||
// NOT copied from MAIL_SCHEME. That variable currently reads "tls",
|
||||
|
|
@ -47,8 +92,9 @@ return new class extends Migration
|
|||
? substr($configuredUser, strpos($configuredUser, '@') + 1)
|
||||
: 'clupilot.com';
|
||||
|
||||
// A password may already be stored in the vault; it moves rather than
|
||||
// being left behind in a registry entry that is about to disappear.
|
||||
// A password may already be stored in the vault. It is CARRIED, not
|
||||
// moved: the app_secrets row is deliberately left in place below
|
||||
// rather than deleted, so this block only ever reads it.
|
||||
$storedPassword = DB::table('app_secrets')->where('key', 'mail.password')->value('value');
|
||||
$envPassword = (string) config('mail.mailers.smtp.password', '');
|
||||
|
||||
|
|
@ -63,8 +109,8 @@ return new class extends Migration
|
|||
foreach (self::SEED as $key => $meta) {
|
||||
$isConfigured = $key === 'no-reply' && $configuredUser !== '';
|
||||
|
||||
$box = new Mailbox([
|
||||
'key' => $key,
|
||||
$box = Mailbox::firstOrNew(['key' => $key]);
|
||||
$box->fill([
|
||||
'address' => $isConfigured ? $configuredUser : $key.'@'.$domain,
|
||||
'display_name' => 'CluPilot',
|
||||
'no_reply' => $meta['no_reply'],
|
||||
|
|
@ -72,8 +118,27 @@ return new class extends Migration
|
|||
]);
|
||||
|
||||
// Only the account that actually exists gets credentials.
|
||||
if ($isConfigured && $envPassword !== '' && $canEncrypt) {
|
||||
$box->password = $envPassword;
|
||||
if ($isConfigured && $envPassword !== '') {
|
||||
if ($canEncrypt) {
|
||||
$box->password = $envPassword;
|
||||
} else {
|
||||
// Skipping beats crashing the migration (that is what
|
||||
// $canEncrypt guards above) — but a skip that LOOKS like
|
||||
// success is its own failure mode: migrate prints DONE,
|
||||
// MAIL_MAILER is still log today, and the first real send
|
||||
// after it flips to smtp throws for every purpose,
|
||||
// discovered by a customer rather than by this line.
|
||||
// There is no console page for mailboxes yet, so stdout
|
||||
// during migrate is the only place an operator can see
|
||||
// this. echo, not fwrite(STDOUT, ...): both reach the
|
||||
// real terminal on a plain `php artisan migrate`, but
|
||||
// only echo goes through PHP's output buffer, which is
|
||||
// what makes it possible to assert on in a test.
|
||||
echo ' ! SECRETS_KEY is not set - the SMTP password configured in .env for '
|
||||
.$configuredUser.' was NOT copied into the mailbox table. Generate SECRETS_KEY, '
|
||||
.'then either set mailboxes.password directly or migrate:rollback and re-run '
|
||||
."this migration.\n";
|
||||
}
|
||||
}
|
||||
|
||||
$box->save();
|
||||
|
|
@ -89,17 +154,45 @@ return new class extends Migration
|
|||
}
|
||||
}
|
||||
|
||||
DB::table('app_secrets')->where('key', 'mail.password')->delete();
|
||||
// mail.password's app_secrets row is deliberately left alone here —
|
||||
// not deleted. Its SecretVault::REGISTRY entry is gone (this task's
|
||||
// other half), so nothing can read or write it through the vault
|
||||
// anymore, but destroying the only remaining copy of a real
|
||||
// credential in the SAME migration that first introduces its new,
|
||||
// less-proven home is backwards for what a stored SMTP password is.
|
||||
// Two ways that unconditional delete used to lose it for real,
|
||||
// reproduced against MariaDB (see the task report):
|
||||
// - MAIL_USERNAME empty makes $isConfigured false for every key
|
||||
// above, so the carry-over above never runs — deleting here
|
||||
// regardless would destroy a rotated password with no copy left
|
||||
// anywhere.
|
||||
// - migrate, then migrate:rollback, then migrate again: down()
|
||||
// deletes the mailbox rows, which would be the ONLY remaining
|
||||
// copy once this row was already gone.
|
||||
// Left in place, a rollback can never lose the credential — at worst
|
||||
// it is an inert leftover row nothing can reach through
|
||||
// SecretVault::assertKnown() any more. A later migration can remove
|
||||
// it once the mailbox takeover has been proven on a live install.
|
||||
}
|
||||
|
||||
public function down(): void
|
||||
{
|
||||
DB::table('mailboxes')->whereIn('key', array_keys(self::SEED))->delete();
|
||||
DB::transaction(function () {
|
||||
DB::table('mailboxes')->whereIn('key', array_keys(self::SEED))->delete();
|
||||
|
||||
foreach (MailPurpose::ALL as $purpose) {
|
||||
DB::table('app_settings')->where('key', MailPurpose::settingKey($purpose))->delete();
|
||||
}
|
||||
// Settings::forget(), not a raw DB::table(...)->delete(): the
|
||||
// cache is Cache::forever()'d with no TTL, so bypassing it here
|
||||
// would leave Settings::get() returning what this loop just
|
||||
// deleted until something else happens to write the same key.
|
||||
// Reproduced: immediately after a raw-delete rollback,
|
||||
// Settings::get('mail.purpose.system') still answered 'no-reply'.
|
||||
foreach (MailPurpose::ALL as $purpose) {
|
||||
Settings::forget(MailPurpose::settingKey($purpose));
|
||||
}
|
||||
|
||||
DB::table('app_settings')->whereIn('key', ['mail.host', 'mail.port', 'mail.encryption'])->delete();
|
||||
foreach (['mail.host', 'mail.port', 'mail.encryption'] as $key) {
|
||||
Settings::forget($key);
|
||||
}
|
||||
});
|
||||
}
|
||||
};
|
||||
|
|
|
|||
|
|
@ -0,0 +1,216 @@
|
|||
<?php
|
||||
|
||||
use App\Models\Mailbox;
|
||||
use App\Services\Secrets\SecretCipher;
|
||||
use App\Support\Settings;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
/**
|
||||
* Loads a fresh instance of the seed migration's anonymous class every call.
|
||||
* `require`, not `require_once`: PHP re-evaluates the `return new class ...`
|
||||
* expression each time, so every call gets its own instance — this is what
|
||||
* lets a test call ->up() with config the REAL migrate run (already applied
|
||||
* once by RefreshDatabase before any test's own beforeEach) could not have
|
||||
* used, to reach branches nothing else exercises.
|
||||
*/
|
||||
function loadMailboxSeedMigration(): object
|
||||
{
|
||||
return require database_path('migrations/2026_07_28_100000_seed_mailboxes_from_environment.php');
|
||||
}
|
||||
|
||||
beforeEach(function () {
|
||||
config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32)));
|
||||
});
|
||||
|
||||
it('does not copy MAIL_SCHEME through — a Symfony transport only knows smtp and smtps', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.scheme', 'not-a-symfony-scheme');
|
||||
config()->set('mail.mailers.smtp.port', 587);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
expect(Settings::get('mail.encryption'))->toBe('tls');
|
||||
});
|
||||
|
||||
it('derives ssl from port 465, regardless of what scheme is configured', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.scheme', 'irrelevant');
|
||||
config()->set('mail.mailers.smtp.port', 465);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
expect(Settings::get('mail.encryption'))->toBe('ssl')
|
||||
->and(Settings::get('mail.port'))->toBe(465);
|
||||
});
|
||||
|
||||
it('treats config/mail.php\'s own 127.0.0.1 / 2525 defaults as unset, not as a real relay', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.host', '127.0.0.1');
|
||||
config()->set('mail.mailers.smtp.port', 2525);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
// '' / 0 is exactly what MailboxTransport's guards treat as "not
|
||||
// configured" — 127.0.0.1 is not '', so it would have sailed straight
|
||||
// past the Task 4 guard and quietly dialled localhost instead.
|
||||
expect(Settings::get('mail.host'))->toBe('')
|
||||
->and(Settings::get('mail.port'))->toBe(0);
|
||||
});
|
||||
|
||||
it('still stores a deliberately configured host and port untouched', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.host', 'smtp.example.test');
|
||||
config()->set('mail.mailers.smtp.port', 587);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
expect(Settings::get('mail.host'))->toBe('smtp.example.test')
|
||||
->and(Settings::get('mail.port'))->toBe(587);
|
||||
});
|
||||
|
||||
it('carries the vault password across as ciphertext, never decrypting and re-encrypting it', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
|
||||
|
||||
$ciphertext = app(SecretCipher::class)->encrypt('rotated-through-the-console');
|
||||
DB::table('app_secrets')->insert([
|
||||
'key' => 'mail.password', 'value' => $ciphertext, 'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
$stored = DB::table('mailboxes')->where('key', 'no-reply')->value('password');
|
||||
|
||||
// Byte-for-byte, not just "decrypts to the same plaintext": AES-CBC's
|
||||
// random IV means re-encrypting the SAME plaintext produces a DIFFERENT
|
||||
// ciphertext string, so this is what actually discriminates "carried
|
||||
// as-is" from "decrypted and re-encrypted" — the migration's own claim,
|
||||
// never exercised by any test before this one.
|
||||
expect($stored)->toBe($ciphertext)
|
||||
->and(app(SecretCipher::class)->decrypt($stored))->toBe('rotated-through-the-console');
|
||||
});
|
||||
|
||||
it('leaves the vault row in the database even after a normal, fully successful carry-over', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
|
||||
|
||||
$ciphertext = app(SecretCipher::class)->encrypt('rotated-through-the-console');
|
||||
DB::table('app_secrets')->insert([
|
||||
'key' => 'mail.password', 'value' => $ciphertext, 'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
expect(DB::table('app_secrets')->where('key', 'mail.password')->value('value'))->toBe($ciphertext);
|
||||
});
|
||||
|
||||
it('does not destroy a stored vault password when MAIL_USERNAME is empty, because it never lands anywhere', function () {
|
||||
// Reproduces the Critical finding: $isConfigured is false for every key
|
||||
// when MAIL_USERNAME is empty, so the carry-over never runs — the old
|
||||
// code deleted the vault row unconditionally regardless.
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.username', '');
|
||||
|
||||
$ciphertext = app(SecretCipher::class)->encrypt('nowhere-to-go');
|
||||
DB::table('app_secrets')->insert([
|
||||
'key' => 'mail.password', 'value' => $ciphertext, 'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
loadMailboxSeedMigration()->up();
|
||||
|
||||
expect(DB::table('app_secrets')->where('key', 'mail.password')->value('value'))->toBe($ciphertext)
|
||||
->and(Mailbox::findByKey('no-reply')->getRawOriginal('password'))->toBeNull();
|
||||
});
|
||||
|
||||
it('survives a migrate, rollback, migrate-again cycle without losing the credential', function () {
|
||||
// Reproduces the other Critical finding: down() deletes the mailbox
|
||||
// rows, which is where the ONLY copy of the password would end up if the
|
||||
// vault row had been deleted (the old behaviour) during the first up().
|
||||
clearMailboxSeed();
|
||||
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
|
||||
|
||||
$ciphertext = app(SecretCipher::class)->encrypt('rotated-through-the-console');
|
||||
DB::table('app_secrets')->insert([
|
||||
'key' => 'mail.password', 'value' => $ciphertext, 'created_at' => now(), 'updated_at' => now(),
|
||||
]);
|
||||
|
||||
$migration = loadMailboxSeedMigration();
|
||||
$migration->up();
|
||||
$migration->down();
|
||||
$migration->up();
|
||||
|
||||
expect(DB::table('mailboxes')->where('key', 'no-reply')->value('password'))->toBe($ciphertext);
|
||||
});
|
||||
|
||||
it('is safe to run twice — a migrations-table drift must not crash on the unique key', function () {
|
||||
// The real baseline from RefreshDatabase's own migrate is already there
|
||||
// (nothing cleared it) — calling up() again must update those rows in
|
||||
// place, not attempt a second INSERT under the same key.
|
||||
//
|
||||
// A plain try/catch, NOT expect(fn () => ...)->not->toThrow(Throwable::class):
|
||||
// verified directly that the latter is not reliable here — a closure that
|
||||
// unconditionally throws still passes ->not->toThrow(Throwable::class) in
|
||||
// this Pest version, which would have made this test pass whether or not
|
||||
// the migration actually crashed.
|
||||
$threw = null;
|
||||
try {
|
||||
loadMailboxSeedMigration()->up();
|
||||
} catch (Throwable $e) {
|
||||
$threw = $e;
|
||||
}
|
||||
|
||||
expect($threw)->toBeNull();
|
||||
expect(Mailbox::query()->pluck('key')->sort()->values()->all())
|
||||
->toBe(['billing', 'info', 'no-reply', 'office', 'support']);
|
||||
});
|
||||
|
||||
it('un-seeds on rollback, and does not leave the settings cache holding what it just deleted', function () {
|
||||
// Against the real baseline again — this is what a real rollback tears
|
||||
// down. Warms the cache first the same way a real request would, so a
|
||||
// migration that bypassed Settings::forget() (a raw DB delete) and left
|
||||
// the Cache::forever() entry behind would still read back the old value.
|
||||
expect(Settings::get('mail.purpose.system'))->toBe('no-reply');
|
||||
|
||||
loadMailboxSeedMigration()->down();
|
||||
|
||||
expect(Mailbox::query()->count())->toBe(0)
|
||||
->and(Settings::get('mail.purpose.system'))->toBeNull()
|
||||
->and(Settings::get('mail.host'))->toBeNull();
|
||||
});
|
||||
|
||||
it('prints an operator-visible warning when SECRETS_KEY is missing and a .env password had to be skipped', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('admin_access.secrets_key', ''); // canEncrypt false
|
||||
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
|
||||
config()->set('mail.mailers.smtp.password', 'dummy-nicht-echt');
|
||||
|
||||
ob_start();
|
||||
loadMailboxSeedMigration()->up();
|
||||
$output = ob_get_clean();
|
||||
|
||||
expect($output)->toContain('SECRETS_KEY')
|
||||
->toContain('no-reply@clupilot.com');
|
||||
});
|
||||
|
||||
it('stays silent when there is no .env password to skip in the first place', function () {
|
||||
clearMailboxSeed();
|
||||
config()->set('admin_access.secrets_key', ''); // canEncrypt false
|
||||
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
|
||||
config()->set('mail.mailers.smtp.password', '');
|
||||
|
||||
ob_start();
|
||||
loadMailboxSeedMigration()->up();
|
||||
$output = ob_get_clean();
|
||||
|
||||
expect($output)->toBe('');
|
||||
});
|
||||
|
||||
it('Settings::forget removes the row and does not leave a stale cache entry behind', function () {
|
||||
Settings::set('probe.key', 'value');
|
||||
expect(Settings::get('probe.key'))->toBe('value');
|
||||
|
||||
Settings::forget('probe.key');
|
||||
|
||||
expect(Settings::get('probe.key'))->toBeNull()
|
||||
->and(DB::table('app_settings')->where('key', 'probe.key')->exists())->toBeFalse();
|
||||
});
|
||||
|
|
@ -16,8 +16,14 @@ it('marks no-reply as unanswerable and the rest as answerable', function () {
|
|||
});
|
||||
|
||||
it('points every purpose at a mailbox, so nothing sends from nowhere', function () {
|
||||
// Not just "the setting has a value" — a purpose pointing at a key with
|
||||
// no matching row would still pass a not-null check on the setting while
|
||||
// MailboxResolver::named() resolves it to nothing and every send throws.
|
||||
foreach (MailPurpose::ALL as $purpose) {
|
||||
expect(Settings::get(MailPurpose::settingKey($purpose)))->not->toBeNull();
|
||||
$key = Settings::get(MailPurpose::settingKey($purpose));
|
||||
|
||||
expect($key)->not->toBeNull()
|
||||
->and(Mailbox::findByKey($key))->not->toBeNull();
|
||||
}
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue