From 0be9c614843ba73ee9a524754f7222e5f077245a Mon Sep 17 00:00:00 2001 From: nexxo Date: Mon, 27 Jul 2026 22:57:22 +0200 Subject: [PATCH] Make a deactivated mailbox fall back like a deleted one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of Task 4: MailboxResolver::for() only fell back on a missing mapping or a missing row. Deactivating a mailbox — the operator's explicit "do not use this" — left it resolvable anyway, so MailboxTransport reached it, found isConfigured() false, and threw instead of quietly using the next candidate the way every other "this mailbox is unusable" case does. Worse, __toString() still reported the deactivated address as though it were healthy. named() now runs through a new active() filter at BOTH lookup sites — the purpose's own mapping and the system fallback — so an inactive system mailbox is refused exactly like a missing one, not handed out as the last resort. A missing password stays deliberately unfiltered here: that is a MailboxTransport concern (loud failure naming the mailbox), not a resolver fallback trigger — silently rerouting mail because a password hasn't been typed yet would hide the gap instead of surfacing it. Added two tests and mutated active() to a no-op filter to confirm they catch it: both failed (one expected the system mailbox, got null; the other expected null, got the inactive mailbox's full attribute dump). Reverted, reran, both pass again. Co-Authored-By: Claude Opus 5 --- app/Services/Mail/MailboxResolver.php | 20 +++++++++++++++-- tests/Feature/Mail/MailboxResolverTest.php | 25 ++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/app/Services/Mail/MailboxResolver.php b/app/Services/Mail/MailboxResolver.php index 6fc8e3c..86ec076 100644 --- a/app/Services/Mail/MailboxResolver.php +++ b/app/Services/Mail/MailboxResolver.php @@ -21,7 +21,7 @@ final class MailboxResolver throw new InvalidArgumentException("Unknown mail purpose [{$purpose}]."); } - $box = $this->named(Settings::get(MailPurpose::settingKey($purpose))); + $box = $this->active($this->named(Settings::get(MailPurpose::settingKey($purpose)))); if ($box !== null) { return $box; @@ -31,11 +31,27 @@ final class MailboxResolver // address is recoverable, one that is never sent may not be noticed. return $purpose === MailPurpose::SYSTEM ? null - : $this->named(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM))); + : $this->active($this->named(Settings::get(MailPurpose::settingKey(MailPurpose::SYSTEM)))); } private function named(mixed $key): ?Mailbox { return is_string($key) && $key !== '' ? Mailbox::findByKey($key) : null; } + + /** + * Deactivating a mailbox is an explicit operator action meaning "do not + * use this" — it must fall back exactly like a mapping that points + * nowhere or a row that is gone, so it is filtered here, at BOTH lookup + * sites, rather than left for a caller to notice. + * + * A mailbox with no password is deliberately NOT filtered here: silently + * sending support mail from billing@ because support@ has no password + * yet is worse than a loud failure naming the mailbox that needs one — + * that check stays a MailboxTransport concern, not this one. + */ + private function active(?Mailbox $box): ?Mailbox + { + return $box?->active === true ? $box : null; + } } diff --git a/tests/Feature/Mail/MailboxResolverTest.php b/tests/Feature/Mail/MailboxResolverTest.php index bff7a43..151c8b2 100644 --- a/tests/Feature/Mail/MailboxResolverTest.php +++ b/tests/Feature/Mail/MailboxResolverTest.php @@ -35,6 +35,31 @@ it('falls back to system when the named mailbox no longer exists', function () { ->toBe('no-reply@clupilot.com'); }); +it('falls back to system when the mailbox is deactivated, not deleted', function () { + // Deactivating is an explicit operator action. Reaching this mailbox by + // its row surviving would silently send from an address the operator + // just told the console to stop using. + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com', 'active' => false]); + Mailbox::factory()->create(['key' => 'no-reply', 'address' => 'no-reply@clupilot.com']); + Settings::set('mail.purpose.support', 'support'); + Settings::set('mail.purpose.system', 'no-reply'); + + expect(app(MailboxResolver::class)->for(MailPurpose::SUPPORT)?->address) + ->toBe('no-reply@clupilot.com'); +}); + +it('returns null rather than an inactive mailbox when the fallback is deactivated too', function () { + // The fallback lookup is filtered the SAME way as the direct one — an + // inactive system mailbox must not be handed out just because it is the + // last candidate. + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com', 'active' => false]); + Mailbox::factory()->create(['key' => 'no-reply', 'address' => 'no-reply@clupilot.com', 'active' => false]); + Settings::set('mail.purpose.support', 'support'); + Settings::set('mail.purpose.system', 'no-reply'); + + expect(app(MailboxResolver::class)->for(MailPurpose::SUPPORT))->toBeNull(); +}); + it('returns null rather than guessing when even system is unset', function () { expect(app(MailboxResolver::class)->for(MailPurpose::SYSTEM))->toBeNull(); });