diff --git a/app/Mail/Transport/MailboxTransport.php b/app/Mail/Transport/MailboxTransport.php index 13eed0f..d73ad37 100644 --- a/app/Mail/Transport/MailboxTransport.php +++ b/app/Mail/Transport/MailboxTransport.php @@ -62,16 +62,27 @@ class MailboxTransport implements TransportInterface { [$mode, $box] = $this->resolution(); - return $mode === 'mailbox' ? $box?->address : $mode; + return match ($mode) { + 'mailbox' => $box?->address, + // Same verdict delegate() throws on, not a second opinion: a DSN + // that still read as a plain address here is the exact harm this + // exists to prevent — it looked healthy right up until send() + // threw. The address stays visible so an operator can tell WHICH + // mailbox needs a password, rather than just that something does. + 'unconfigured' => $box === null ? null : $box->address.' [unconfigured]', + default => $mode, + }; } /** - * The ONE place that decides "log, array, null, or a real mailbox" — - * describe() and delegate() both branch on this result rather than each - * running their own copy of the check. Two copies of the same guard is - * what let a mutation that broke only ONE of them hide behind a passing - * suite: __toString() kept reporting "log" while delegate() quietly - * built a real SMTP transport regardless of MAIL_MAILER. + * The ONE place that decides "log, array, null-default, an unconfigured + * mailbox, or a real one" — describe() and delegate() both branch on this + * result rather than each running their own copy of a check. Two copies + * of the same guard is what let a mutation that broke only ONE of them + * hide behind a passing suite: __toString() kept reporting "log" (and + * later, a plain address) while delegate() quietly did something else — + * built a real SMTP transport regardless of MAIL_MAILER, then threw on a + * mailbox describe() had just shown as fine. * * @return array{0: string, 1: ?Mailbox} */ @@ -83,13 +94,29 @@ class MailboxTransport implements TransportInterface return [$default ?? 'null', null]; } - return ['mailbox', app(MailboxResolver::class)->for($this->purpose)]; + $box = app(MailboxResolver::class)->for($this->purpose); + + return $box !== null && $box->isConfigured() + ? ['mailbox', $box] + : ['unconfigured', $box]; } private function delegate(): TransportInterface { [$mode, $box] = $this->resolution(); + if ($mode === 'unconfigured') { + // Loud, not silent: a mail with no mailbox behind it must not look + // like it was sent. A missing or deactivated MAPPING already fell + // back to system inside MailboxResolver; reaching this line means + // the mailbox we ended up with — possibly system itself — has no + // usable credentials (no row at all, or one with no password), + // and switching to yet another address would only hide that. + throw new RuntimeException( + "No configured mailbox for mail purpose [{$this->purpose}]." + ); + } + if ($mode !== 'mailbox') { return match ($mode) { 'log' => new LogTransport(Log::channel(config('mail.mailers.log.channel'))), @@ -98,18 +125,8 @@ class MailboxTransport implements TransportInterface }; } - if ($box === null || ! $box->isConfigured()) { - // Loud, not silent: a mail with no mailbox behind it must not look - // like it was sent. A missing or deactivated MAPPING already fell - // back to system inside MailboxResolver; reaching this line means - // the mailbox we ended up with — possibly system itself — has no - // usable credentials, and switching to yet another address would - // only hide that. - throw new RuntimeException( - "No configured mailbox for mail purpose [{$this->purpose}]." - ); - } - + // $mode === 'mailbox' only when resolution() already confirmed $box + // is non-null and isConfigured() — nothing left to check here. $host = (string) Settings::get('mail.host', ''); $port = (int) Settings::get('mail.port', 587); $encryption = (string) Settings::get('mail.encryption', 'tls'); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index 0438601..0e30f64 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/tests/Feature/Mail/MailboxTransportTest.php @@ -32,7 +32,7 @@ function mailboxDelegate(TransportInterface $transport): TransportInterface return Closure::bind(fn () => $this->delegate(), $transport, MailboxTransport::class)(); } -it('registers a mailer per purpose without touching the database at boot', function () { +it('registers a mailer for every purpose', function () { // The five entries exist as plain config — no query has run to build them. foreach (MailPurpose::ALL as $purpose) { expect(config('mail.mailers.cp_'.$purpose)) @@ -68,6 +68,27 @@ it('resolves the purpose to the address of the mailbox it is currently mapped to ->and((string) $transport)->toContain('support@clupilot.com'); }); +it('marks the DSN as unconfigured rather than showing a healthy-looking address with no password', function () { + // The exact harm the finding named: describe() only ever showed the + // address, so a mailbox with an active row, a real address, and no + // password looked identical to a working one — right up until send() + // threw. The address stays visible (an operator needs to know WHICH + // mailbox is broken); the marker is what stops it from reading as fine. + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com', 'password' => null]); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect((string) $transport) + ->toContain('support@clupilot.com') + ->toContain('unconfigured'); + + // Agreement, not just a matching label: the DSN's warning must correspond + // to send() actually refusing, not to a marker painted on independently. + expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class); +}); + it('logs instead of delivering while MAIL_MAILER is log', function () { Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');