From 3278f063ea12cc3aaba9ed99aaaa3dbd0e43d18d Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 04:35:16 +0200 Subject: [PATCH] Validate purpose mappings against real mailboxes, and clear stale verification on identity changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit savePurposes() only checked that each key was a nonempty string, so a stale or tampered mailbox key (reachable by posting straight to /livewire/update) saved successfully while MailboxResolver silently returned null for it. Every purpose's key must now name a mailbox that exists; system's must additionally be active, since every unmapped or inactive OTHER purpose falls through to it. EditMailbox::save() also cleared last_verified_at only when the password changed, but smtpUsername() depends on address and username too (falling back to address when username is blank) — changing either while leaving the password untouched kept showing a successful verification for an identity nothing had actually tested. --- app/Livewire/Admin/Mail.php | 46 +++++- app/Livewire/EditMailbox.php | 13 ++ lang/de/mail_settings.php | 2 + lang/en/mail_settings.php | 2 + tests/Feature/Mail/MailSettingsPageTest.php | 149 ++++++++++++++++++++ 5 files changed, 206 insertions(+), 6 deletions(-) diff --git a/app/Livewire/Admin/Mail.php b/app/Livewire/Admin/Mail.php index 34b2742..ec44542 100644 --- a/app/Livewire/Admin/Mail.php +++ b/app/Livewire/Admin/Mail.php @@ -97,12 +97,46 @@ class Mail extends Component { $this->authorize('mail.manage'); - // system is the fallback, so it is the one that may not be empty — - // there is nothing left to fall back to. - $this->validate( - ['purposes.system' => ['required', 'string']], - ['purposes.system.required' => __('mail_settings.system_required')], - ); + // Reachable by posting straight to /livewire/update, past whatever + // the +// at all — the fallback is only useful if it actually names a mailbox that +// can send. + +it('refuses to map system to a mailbox key that names no mailbox at all', function () { + Mailbox::factory()->create(['key' => 'support']); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('purposes.system', 'a-deleted-or-tampered-key') + ->call('savePurposes') + ->assertHasErrors('purposes.system'); + + expect(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))->toBeNull(); +}); + +it('refuses to map system to a mailbox that exists but is not active', function () { + // Existing is not enough for system specifically: MailboxResolver::active() + // filters an inactive mailbox out at BOTH the direct lookup and the system + // fallback, so an inactive "system" leaves every unmapped purpose with + // nothing behind it — not just system's own mail. + Mailbox::factory()->create(['key' => 'no-reply', 'active' => false]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('purposes.system', 'no-reply') + ->call('savePurposes') + ->assertHasErrors('purposes.system'); + + expect(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))->toBeNull(); +}); + +it('refuses to map a non-system purpose to a mailbox key that names no mailbox at all', function () { + Mailbox::factory()->create(['key' => 'no-reply']); + Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('purposes.support', 'a-deleted-or-tampered-key') + ->call('savePurposes') + ->assertHasErrors('purposes.support'); + + expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBeNull(); +}); + +it('still allows mapping a non-system purpose to a mailbox that exists but is inactive', function () { + // The control case for the two refusals above: MailboxResolver::active() + // already falls an inactive NON-system mapping through to system on its + // own (see its own docblock), so only system needs the stricter active + // check — a non-system purpose merely needs the key to name a real row. + Mailbox::factory()->create(['key' => 'no-reply']); + Mailbox::factory()->create(['key' => 'support', 'active' => false]); + Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('purposes.support', 'support') + ->call('savePurposes') + ->assertHasNoErrors(); + + expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBe('support'); +}); + +it('still allows leaving a non-system purpose blank, even after it once had a mailbox', function () { + // Blank is the legitimate "fall back to system" choice, not a tampered + // value — the fix must not turn every unmapped purpose into a validation + // error the moment it is resaved. + Mailbox::factory()->create(['key' => 'no-reply']); + Mailbox::factory()->create(['key' => 'support']); + Settings::set(MailPurpose::settingKey(MailPurpose::SYSTEM), 'no-reply'); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('purposes.support', '') + ->call('savePurposes') + ->assertHasNoErrors(); + + expect(Settings::get(MailPurpose::settingKey(MailPurpose::SUPPORT)))->toBe(''); +}); + it('re-checks mail.manage on every page action, not only at mount', function () { // mount() alone would pass every test above: an Owner has the capability // for the whole test, so nothing so far proves saveServer()/savePurposes() @@ -357,6 +439,73 @@ it('clears last_verified_at when the password changes, since it no longer proves expect($box->fresh()->last_verified_at)->toBeNull(); }); +// --- P2: last_verified_at proves credentials were tested, not just that a +// password exists — changing EITHER half of the identity smtpUsername() +// authenticates as (an explicit username, or address as its fallback) must +// invalidate that proof exactly like a new password does. None of these set +// a password at all: the point is that the guard fires without one. + +it('clears last_verified_at when the address changes, even though the password stays blank', function () { + $box = Mailbox::factory()->create([ + 'address' => 'alt@clupilot.com', 'username' => null, 'last_verified_at' => now(), + ]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('address', 'neu@clupilot.com') + ->call('save'); + + expect($box->fresh()->last_verified_at)->toBeNull(); +}); + +it('clears last_verified_at when an explicit username changes, even though the password stays blank', function () { + $box = Mailbox::factory()->create([ + 'username' => 'alt-smtp-user', 'last_verified_at' => now(), + ]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('username', 'neu-smtp-user') + ->call('save'); + + expect($box->fresh()->last_verified_at)->toBeNull(); +}); + +it('clears last_verified_at when the username is cleared, since smtpUsername() then falls back to the address', function () { + // The fallback the brief names explicitly: username going from an + // explicit value to blank changes what smtpUsername() returns just as + // much as typing a new one would, even though the raw address field + // never moves. + $box = Mailbox::factory()->create([ + 'address' => 'bleibt@clupilot.com', 'username' => 'alt-smtp-user', 'last_verified_at' => now(), + ]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('username', '') + ->call('save'); + + expect($box->fresh()->username)->toBeNull() + ->and($box->fresh()->last_verified_at)->toBeNull(); +}); + +it('keeps last_verified_at when neither the address nor the username actually changes', function () { + // The control case for the three tests above: the guard must react to an + // actual CHANGE, not fire on every save regardless of what moved — + // otherwise it would stay green even if it degenerated into always + // clearing the column. + $box = Mailbox::factory()->create([ + 'address' => 'bleibt@clupilot.com', 'username' => 'bleibt-auch', 'last_verified_at' => now(), + ]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('displayName', 'Neuer Anzeigename') + ->call('save'); + + expect($box->fresh()->last_verified_at)->not->toBeNull(); +}); + it('refuses to set a new mailbox password without a recently confirmed password', function () { // The credential-interception primitive the brief names: a signed-in // mail.manage session alone must not be enough to repoint where a