diff --git a/app/Support/Settings.php b/app/Support/Settings.php index 76bb8a7..876dec8 100644 --- a/app/Support/Settings.php +++ b/app/Support/Settings.php @@ -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); + } } diff --git a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php index 7ed0db4..4d6d3f0 100644 --- a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php +++ b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php @@ -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); + } + }); } }; diff --git a/tests/Feature/Mail/MailboxSeedMigrationTest.php b/tests/Feature/Mail/MailboxSeedMigrationTest.php new file mode 100644 index 0000000..4c3e822 --- /dev/null +++ b/tests/Feature/Mail/MailboxSeedMigrationTest.php @@ -0,0 +1,216 @@ +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(); +}); diff --git a/tests/Feature/Mail/MailboxTakeoverTest.php b/tests/Feature/Mail/MailboxTakeoverTest.php index 1971677..e5381c7 100644 --- a/tests/Feature/Mail/MailboxTakeoverTest.php +++ b/tests/Feature/Mail/MailboxTakeoverTest.php @@ -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(); } });