From 2979f008b528a1c201b0a5024599424c4ce76ac6 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 07:26:42 +0200 Subject: [PATCH] Stop a mismatched-key decrypt from crashing the mailbox test button --- app/Services/Mail/MailboxTester.php | 23 +++++++++++++++-- lang/de/mail_settings.php | 1 + lang/en/mail_settings.php | 1 + tests/Feature/Mail/MailSettingsPageTest.php | 21 ++++++++++++++++ tests/Feature/Mail/MailboxTesterTest.php | 28 +++++++++++++++++++++ 5 files changed, 72 insertions(+), 2 deletions(-) diff --git a/app/Services/Mail/MailboxTester.php b/app/Services/Mail/MailboxTester.php index 8a33967..022b0c1 100644 --- a/app/Services/Mail/MailboxTester.php +++ b/app/Services/Mail/MailboxTester.php @@ -5,6 +5,7 @@ namespace App\Services\Mail; use App\Models\Mailbox; use App\Services\Secrets\SecretCipher; use App\Support\Settings; +use Illuminate\Contracts\Encryption\DecryptException; use Illuminate\Support\Facades\Mail; use Throwable; @@ -46,12 +47,30 @@ final class MailboxTester return ['ok' => false, 'error' => __('mail_settings.no_key')]; } + // Codex R15#9, P2: isUsable() only proves SECRETS_KEY is well-formed + // (right length, right shape) — never that THIS row's ciphertext was + // ever encrypted under it. A rotated key, or a row seeded/encrypted + // under a different one, sails straight past the guard above and + // THEN throws a DecryptException the instant $box->password is + // actually read: the same uncaught-out-of-a-Livewire-action shape + // the guard above exists for, just a failure isUsable() cannot see + // by design. Read once, here, behind its own guard, and reuse the + // plaintext below — the null-check on the next line and the + // credentials ternary further down used to both call this SAME + // decrypting accessor a second and third time, each one another + // uncaught throw waiting to happen. + try { + $password = $box->authenticates ? $box->password : null; + } catch (DecryptException) { + return ['ok' => false, 'error' => __('mail_settings.test_password_undecryptable')]; + } + // Codex R15#4, P1b: a password is only REQUIRED when this mailbox // actually authenticates — a trusted local or private-network relay // can legitimately need none at all, and refusing it here is exactly // the "no password stored" outage the finding names, even though the // relay was never broken. - if ($box->authenticates && $box->password === null) { + if ($box->authenticates && $password === null) { return ['ok' => false, 'error' => __('mail_settings.test_no_password')]; } @@ -84,7 +103,7 @@ final class MailboxTester // to answer. Mirrors MailboxTransport's own setUsername()/ // setPassword() guard for the real send path. $credentials = $box->authenticates - ? ['username' => $box->smtpUsername(), 'password' => $box->password] + ? ['username' => $box->smtpUsername(), 'password' => $password] : []; try { diff --git a/lang/de/mail_settings.php b/lang/de/mail_settings.php index 3e5c804..9089372 100644 --- a/lang/de/mail_settings.php +++ b/lang/de/mail_settings.php @@ -58,6 +58,7 @@ return [ 'test_subject' => 'CluPilot — Testnachricht', 'test_body' => 'Diese Nachricht bestätigt, dass das Postfach :key verschicken kann.', 'test_no_password' => 'Für dieses Postfach ist kein Passwort hinterlegt.', + 'test_password_undecryptable' => 'Das gespeicherte Passwort lässt sich mit dem aktuellen Schlüssel nicht entschlüsseln. Bitte neu eingeben und speichern.', 'test_port_not_configured' => 'Der Mailserver-Port ist nicht konfiguriert.', 'test_ok' => 'Zugestellt. Das Postfach kann verschicken.', 'test_failed' => 'Der Mailserver hat abgelehnt:', diff --git a/lang/en/mail_settings.php b/lang/en/mail_settings.php index 9f5c296..b7edcfd 100644 --- a/lang/en/mail_settings.php +++ b/lang/en/mail_settings.php @@ -58,6 +58,7 @@ return [ 'test_subject' => 'CluPilot — test message', 'test_body' => 'This message confirms that the mailbox :key can send.', 'test_no_password' => 'No password is stored for this mailbox.', + 'test_password_undecryptable' => 'The stored password cannot be decrypted with the current key. Please re-enter it and save.', 'test_port_not_configured' => 'The mail server port is not configured.', 'test_ok' => 'Delivered. The mailbox can send.', 'test_failed' => 'The mail server rejected it:', diff --git a/tests/Feature/Mail/MailSettingsPageTest.php b/tests/Feature/Mail/MailSettingsPageTest.php index 5a71fa4..48b5db2 100644 --- a/tests/Feature/Mail/MailSettingsPageTest.php +++ b/tests/Feature/Mail/MailSettingsPageTest.php @@ -916,6 +916,27 @@ it('does not crash the test button for a malformed SECRETS_KEY either, and says ->assertSet('testResult.error', __('mail_settings.no_key')); }); +it('does not crash the test button when the stored password was encrypted under a different key, and says so instead', function () { + // Codex R15#9, P2: isUsable() only proves SECRETS_KEY is well-formed — + // never that THIS row's ciphertext was actually encrypted under it. A + // key that is present and valid but does not match the stored + // ciphertext is a case isUsable() cannot detect by design, and it used + // to throw a DecryptException straight out of this same Livewire + // action, unguarded. Proven at the MailboxTester unit level already; + // this proves the BUTTON reaches that guard rather than a mock standing + // in for it — Livewire::test() surfaces an uncaught exception as a + // thrown PHP error, not silently, so this genuinely fails loudly if the + // guard regresses rather than passing by accident. + $box = Mailbox::factory()->create(['key' => 'support', 'password' => 'geheim']); + config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('testRecipient', 'ziel@example.com') + ->call('test', $box->uuid) + ->assertSet('testResult.error', __('mail_settings.test_password_undecryptable')); +}); + it('offers a per-row test-send button, not only edit', function () { $blade = file_get_contents(resource_path('views/livewire/admin/mail.blade.php')); diff --git a/tests/Feature/Mail/MailboxTesterTest.php b/tests/Feature/Mail/MailboxTesterTest.php index ad9fb6f..5b1a732 100644 --- a/tests/Feature/Mail/MailboxTesterTest.php +++ b/tests/Feature/Mail/MailboxTesterTest.php @@ -169,6 +169,34 @@ it('gives a readable message instead of throwing when SECRETS_KEY is unusable', ->and($result['error'])->toBe(__('mail_settings.no_key')); }); +// --- Codex R15#9, P2: isUsable() proves SECRETS_KEY is well-formed (right +// length, right shape) — never that THIS row's ciphertext was actually +// encrypted under it. A rotated key, or a row seeded/encrypted under a +// different one, is exactly the case isUsable() cannot see by design (its +// own docblock says so): it sails straight past the guard above and then +// throws a DecryptException the instant $box->password is actually read — +// the same uncaught-out-of-a-Livewire-action shape the guard above exists +// for, just a failure the earlier round-3/round-5 guards do not cover. + +it('gives a readable message instead of throwing when the stored password does not decrypt under the current key', function () { + // Encrypt under one valid key, then swap in a DIFFERENT, equally + // well-formed 32-byte key before the read — MailboxModelTest's proven + // recipe ("is keyed to SECRETS_KEY — rotating it makes the password + // unreadable") for making $box->password throw DecryptException rather + // than merely returning wrong bytes. A malformed key (covered above) + // never gets far enough to decrypt anything at all — this is a + // DIFFERENT failure: the key is fine, the ciphertext just was not made + // for it. + $box = Mailbox::factory()->create(['address' => 'no-reply@clupilot.com', 'password' => 'geheim']); + + config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); + + $result = app(MailboxTester::class)->run($box->fresh(), 'ziel@example.com'); + + expect($result['ok'])->toBeFalse() + ->and($result['error'])->toBe(__('mail_settings.test_password_undecryptable')); +}); + // --- Codex R15#5, P2: an unauthenticated mailbox never reads $box->password // (see the credentials ternary above in run() and the guard just above it), // so requiring a usable SECRETS_KEY before even checking authenticates