Stop committing a real password, and say the truth about what carried over

MailboxSeedMigrationTest.php had the actual .env MAIL_PASSWORD for
no-reply@clupilot.com hard-coded into a test fixture — replaced with an
obvious dummy. Grepped the whole branch (576cfca..HEAD) and the working
tree for that string and for every other real secret value in .env;
nothing else leaked.

Separately, the seed migration's SECRETS_KEY-missing warning claimed a
password "was NOT copied into the mailbox table" even when a vault row
already existed and its ciphertext WAS carried across a few lines below,
unconditionally on $canEncrypt. An operator debugging a live mail outage
would have been sent to re-enter a password that was already sitting in
the column. The warning now distinguishes the two cases and says which
one actually happened.
feat/mailboxes
nexxo 2026-07-28 03:49:37 +02:00
parent befb67327f
commit 99b7393361
2 changed files with 57 additions and 1 deletions

View File

@ -121,6 +121,20 @@ return new class extends Migration
if ($isConfigured && $envPassword !== '') { if ($isConfigured && $envPassword !== '') {
if ($canEncrypt) { if ($canEncrypt) {
$box->password = $envPassword; $box->password = $envPassword;
} elseif ($storedPassword !== null) {
// Skipping the .env password beats crashing the migration
// (that is what $canEncrypt guards above) — but this box
// is NOT left without a password: the vault row carried
// below (see the unconditional block after $box->save())
// runs regardless of $canEncrypt, because it is already
// ciphertext and needs no new encryption. Saying "was NOT
// copied" here would be true only of the .env value and
// false of the outcome — an operator reading this while
// debugging a live mail outage would be sent to re-enter
// a password that is already sitting in the column.
echo ' ! SECRETS_KEY is not set - the SMTP password configured in .env for '
.$configuredUser.' was not copied from .env, but a password already stored '
."for this mailbox was carried over and should still work.\n";
} else { } else {
// Skipping beats crashing the migration (that is what // Skipping beats crashing the migration (that is what
// $canEncrypt guards above) — but a skip that LOOKS like // $canEncrypt guards above) — but a skip that LOOKS like

View File

@ -179,6 +179,11 @@ it('un-seeds on rollback, and does not leave the settings cache holding what it
}); });
it('prints an operator-visible warning when SECRETS_KEY is missing and a .env password had to be skipped', function () { it('prints an operator-visible warning when SECRETS_KEY is missing and a .env password had to be skipped', function () {
// No vault row exists here — nothing at all ends up in mailboxes.password
// for this key, so "was NOT copied" is the true statement. The sibling
// test below is the other half: a vault row DOES exist, the password IS
// carried across regardless of $canEncrypt, and the warning must say a
// different, equally true thing rather than reuse this wording.
clearMailboxSeed(); clearMailboxSeed();
config()->set('admin_access.secrets_key', ''); // canEncrypt false config()->set('admin_access.secrets_key', ''); // canEncrypt false
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com'); config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
@ -189,7 +194,44 @@ it('prints an operator-visible warning when SECRETS_KEY is missing and a .env pa
$output = ob_get_clean(); $output = ob_get_clean();
expect($output)->toContain('SECRETS_KEY') expect($output)->toContain('SECRETS_KEY')
->toContain('no-reply@clupilot.com'); ->toContain('no-reply@clupilot.com')
->toContain('was NOT copied')
->and(DB::table('mailboxes')->where('key', 'no-reply')->value('password'))->toBeNull();
});
it('says a stored password was carried over, not that nothing was copied, when a vault row already exists and SECRETS_KEY is missing', function () {
// Deferred #4: the vault-carry block a few lines below this branch runs
// UNCONDITIONALLY on $storedPassword !== null — it needs no new
// encryption, since the value is already ciphertext — so this mailbox
// ends up WITH a working password even though $canEncrypt is false. The
// old message said "was NOT copied into the mailbox table" regardless,
// which is true of the .env value and false of the outcome: an operator
// reading it mid mail-outage would be sent to re-enter a password that
// is already sitting in the column.
clearMailboxSeed();
config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com');
config()->set('mail.mailers.smtp.password', 'dummy-nicht-echt');
// Encrypted under the valid key beforeEach() set up — standing in for a
// password stored earlier, through the console, while SECRETS_KEY still
// existed. THEN it goes missing, which is the state this migration
// actually runs under below.
$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(),
]);
config()->set('admin_access.secrets_key', ''); // canEncrypt false, now
ob_start();
loadMailboxSeedMigration()->up();
$output = ob_get_clean();
expect($output)->toContain('SECRETS_KEY')
->toContain('no-reply@clupilot.com')
->toContain('carried over')
->not->toContain('was NOT copied')
->and(DB::table('mailboxes')->where('key', 'no-reply')->value('password'))->toBe($ciphertext);
}); });
it('stays silent when there is no .env password to skip in the first place', function () { it('stays silent when there is no .env password to skip in the first place', function () {