From a39670f920bb928bb7b7993523e952e7eb389a3e Mon Sep 17 00:00:00 2001 From: nexxo Date: Mon, 27 Jul 2026 23:02:18 +0200 Subject: [PATCH] Close the safety-net gaps the review found in MailboxTransport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer continued the mutation sweep from Task 4's own commit and found three more unpinned lines in delegate(), plus two real behaviour gaps, all inside the same 40-line method: - The fingerprint refresh (the headline feature: a worker's mailer must pick up a corrected password without a restart) had no test — mutating its check to "delegate === null" left the suite green. - The implicit-TLS third argument to EsmtpTransport had no test either — mutating it to a hardcoded true left the suite green, because the only assertion touching it was instanceof EsmtpTransport, which the hanging variant also satisfies. - isLogging() only ever checked for 'log', but phpunit.xml sets MAIL_MAILER=array for the whole suite. Task 5 wires real mailables to these mailers next; the first feature test that sends one without Mail::fake() would have opened a real SMTP connection. - describe() and delegate() each ran their own copy of that guard — which is exactly how deleting it from ONLY delegate() passed the whole suite in the first place: __toString() kept reporting "log" from its own untouched copy. - An unconfigured host/port (Settings::get('mail.host', '') and a stored null port casting to 0) built a transport pointed at smtp://:587 or silently downgraded to plaintext port 25, instead of refusing the way a missing mailbox already does. Fixes: replaced isLogging() with resolution(), the one place that now decides "log, array, null-default, or a real mailbox" — describe() and delegate() both branch on ITS result, so a guard broken in one cannot look fine in the other. Added an explicit RuntimeException for a blank host or non-positive port, guarded the same way as the missing-mailbox case just above it. Tests: added coverage for the fingerprint rebuild (reuses the same delegate until the password actually changes, proven by object identity), the exact DSN string for both the STARTTLS (587) and implicit-TLS (465) cases, 'array' and unset-default both landing on a non-delivering transport, and the blank host/bad port throwing. Mutated each of these four in turn on the final code and confirmed: fingerprint check removed -> the reuse test fails (same object handed back after the password changed); TLS argument hardcoded true -> the 587 DSN test fails ('smtps://...' where 'smtp://...:587' was expected); NON_DELIVERING narrowed back to ['log'] -> both the array and unset-default tests fail (a real EsmtpTransport where a safe transport was expected); host/port guards removed -> both throw expectations fail. Reverted each, reran, all twelve tests pass again. Replaced the two tests that reached into delegate() via Closure::bind to check "log or SMTP" with one behavioural test: point Settings at a closed loopback port (127.0.0.1:1, so refusal is instant and entirely local — no DNS, no dependency on this environment's network egress policy) and assert Log::shouldReceive('debug')->once() while sending for real through Mail::mailer('cp_support')->raw(...). Mutated NON_DELIVERING to drop 'log' specifically and confirmed this test alone catches it: a real connection attempt to the closed port throws TransportException (Connection refused) in under 300ms. Reverted, reran, passes again. Also: the two tests that only checked the config array's shape and a snapshot of the resolved address promised more than they verified — CLAUDE.md R19 names a test that recomputes the implementation as checking nothing. Added a source-level test that the five cp_* config entries are parenthesis-free literals (same technique DisplayTimezoneTest/IconLayoutTest use: a property of the file, not of one run) as the actual "no query at boot" proof, and renamed the send-time test to what it verifies now that the fingerprint test above covers the live-refresh claim properly. Co-Authored-By: Claude Opus 5 --- app/Mail/Transport/MailboxTransport.php | 78 ++++++++-- tests/Feature/Mail/MailboxTransportTest.php | 156 +++++++++++++++++--- 2 files changed, 201 insertions(+), 33 deletions(-) diff --git a/app/Mail/Transport/MailboxTransport.php b/app/Mail/Transport/MailboxTransport.php index 0137540..13eed0f 100644 --- a/app/Mail/Transport/MailboxTransport.php +++ b/app/Mail/Transport/MailboxTransport.php @@ -2,12 +2,16 @@ namespace App\Mail\Transport; +use App\Models\Mailbox; use App\Services\Mail\MailboxResolver; use App\Support\Settings; +use Illuminate\Mail\Transport\ArrayTransport; use Illuminate\Mail\Transport\LogTransport; use Illuminate\Support\Facades\Log; +use RuntimeException; use Symfony\Component\Mailer\Envelope; use Symfony\Component\Mailer\SentMessage; +use Symfony\Component\Mailer\Transport\NullTransport; use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; use Symfony\Component\Mailer\Transport\TransportInterface; use Symfony\Component\Mime\RawMessage; @@ -25,6 +29,18 @@ use Symfony\Component\Mime\RawMessage; */ class MailboxTransport implements TransportInterface { + /** + * mail.default values that must never open a real connection. + * + * 'array' is here because phpunit.xml sets it as the suite-wide test + * default — the first feature test that sends through a purpose mailer + * without Mail::fake() must not reach real SMTP just because 'array' is + * not the exact string 'log'. `null` is here because a stored setting can + * decode to it just as easily as to a real value (see the port note in + * delegate()): an unconfigured mailer must fail SAFE, not fail open. + */ + private const NON_DELIVERING = ['log', 'array', null]; + private ?TransportInterface $delegate = null; private ?string $fingerprint = null; @@ -44,37 +60,52 @@ class MailboxTransport implements TransportInterface /** What this transport currently points at — used by __toString and tests. */ private function describe(): ?string { - if ($this->isLogging()) { - return 'log'; - } + [$mode, $box] = $this->resolution(); - return app(MailboxResolver::class)->for($this->purpose)?->address; + return $mode === 'mailbox' ? $box?->address : $mode; } /** - * Delivery is off wherever the default mailer is the log. + * 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. * - * This is what keeps a development machine from mailing real customers: the - * mailables name a purpose, so without this check they would bypass - * MAIL_MAILER=log entirely and send for real. + * @return array{0: string, 1: ?Mailbox} */ - private function isLogging(): bool + private function resolution(): array { - return config('mail.default') === 'log'; + $default = config('mail.default'); + + if (in_array($default, self::NON_DELIVERING, true)) { + return [$default ?? 'null', null]; + } + + return ['mailbox', app(MailboxResolver::class)->for($this->purpose)]; } private function delegate(): TransportInterface { - if ($this->isLogging()) { - return new LogTransport(Log::channel(config('mail.mailers.log.channel'))); - } + [$mode, $box] = $this->resolution(); - $box = app(MailboxResolver::class)->for($this->purpose); + if ($mode !== 'mailbox') { + return match ($mode) { + 'log' => new LogTransport(Log::channel(config('mail.mailers.log.channel'))), + 'array' => new ArrayTransport, + default => new NullTransport, // 'null' — mail.default not configured at all + }; + } if ($box === null || ! $box->isConfigured()) { // Loud, not silent: a mail with no mailbox behind it must not look - // like it was sent. - throw new \RuntimeException( + // 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}]." ); } @@ -83,6 +114,21 @@ class MailboxTransport implements TransportInterface $port = (int) Settings::get('mail.port', 587); $encryption = (string) Settings::get('mail.encryption', 'tls'); + // Guarded the same way as the missing mailbox above. Tasks 6-7 are + // what write these settings, so today they default to blank/absent — + // and a STORED null port json-decodes to null, which casts to 0; left + // unguarded, EsmtpTransport turns port 0 into plaintext port 25 + // rather than refusing (the 587 default only applies when the row is + // absent, not when it holds null). Loud beats a silently downgraded + // connection. + if ($host === '') { + throw new RuntimeException('Mail server host is not configured — cannot send mail.'); + } + + if ($port < 1) { + throw new RuntimeException('Mail server port is not configured — cannot send mail.'); + } + $fingerprint = md5(implode('|', [ $host, $port, $encryption, $box->smtpUsername(), (string) $box->password, ])); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index 6b52087..0438601 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/tests/Feature/Mail/MailboxTransportTest.php @@ -4,9 +4,11 @@ use App\Mail\Transport\MailboxTransport; use App\Models\Mailbox; use App\Services\Mail\MailPurpose; use App\Support\Settings; -use Illuminate\Mail\Transport\LogTransport; +use Illuminate\Mail\Transport\ArrayTransport; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Mail; -use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; +use Symfony\Component\Mailer\Transport\NullTransport; +use Symfony\Component\Mailer\Transport\TransportInterface; beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); @@ -15,6 +17,21 @@ beforeEach(function () { Settings::set('mail.encryption', 'tls'); }); +/** + * Reach into the same private delegate() that send() itself calls. + * + * __toString() and send() decide "log, array, null, or a real mailbox" + * through describe()/delegate() — two DIFFERENT methods. A mutation that + * broke only delegate() once passed the whole suite, because every test + * checked (string) $transport, which never calls it. Where a behavioural + * proof (actually sending, see the log-guard test below) is impractical + * without real I/O, this is how those tests reach the one that matters. + */ +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 () { // The five entries exist as plain config — no query has run to build them. foreach (MailPurpose::ALL as $purpose) { @@ -23,7 +40,24 @@ it('registers a mailer per purpose without touching the database at boot', funct } }); -it('builds the transport from the mailbox that is current at send time', function () { +it('defines every purpose mailer as a plain literal, so building it cannot touch the database', function () { + // A slice with no parenthesis in it cannot contain a function call — + // env(), config(), DB::table(), all of them need one. Checked at the + // SOURCE level, the same technique DisplayTimezoneTest and IconLayoutTest + // use for a property of the file rather than of one particular run. + $source = (string) file_get_contents(config_path('mail.php')); + + foreach (MailPurpose::ALL as $purpose) { + preg_match('/\'cp_'.preg_quote($purpose, '/').'\'\s*=>\s*(\[[^\]]*\])/', $source, $matches); + + expect($matches)->toHaveCount(2, "cp_{$purpose} is missing or not a plain array literal"); + expect($matches[1])->not->toContain('(') + ->and($matches[1])->toContain("'mailbox'") + ->and($matches[1])->toContain("'{$purpose}'"); + } +}); + +it('resolves the purpose to the address of the mailbox it is currently mapped to', function () { Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); config()->set('mail.default', 'smtp'); // delivery on, as on live @@ -47,32 +81,120 @@ it('logs instead of delivering while MAIL_MAILER is log', function () { }); /* -| __toString() and send() decide "log or SMTP" through two SEPARATE code -| paths (describe() vs delegate()). The test above only exercises the first — -| it would keep passing even if delegate() lost its own guard and sent every -| purpose-mailer through real SMTP regardless of MAIL_MAILER. Reach into the -| method that actually sends, so that regression cannot hide behind a passing -| __toString() assertion. No real socket is opened either way: EsmtpTransport -| connects lazily on send(), never in its constructor. +| LOAD-BEARING for the MAIL_MAILER=log guard — this is the one that actually +| SENDS, so a broken guard cannot hide behind a __toString() that still looks +| right. Settings point at a closed loopback port: nothing listens on +| 127.0.0.1:1, so a real attempt is refused in milliseconds — no DNS, no +| dependency on this environment's network egress policy, and the failure +| (a thrown TransportException) names the actual harm rather than a class +| mismatch. */ -it('actually delegates to the log transport, not SMTP, while MAIL_MAILER is log', function () { +it('actually sends through the log transport, not SMTP, while MAIL_MAILER is log', function () { Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); config()->set('mail.default', 'log'); + Settings::set('mail.host', '127.0.0.1'); + Settings::set('mail.port', 1); - $transport = Mail::mailer('cp_support')->getSymfonyTransport(); - $delegate = Closure::bind(fn () => $this->delegate(), $transport, MailboxTransport::class)(); + Log::shouldReceive('channel')->andReturnSelf(); + Log::shouldReceive('debug')->once(); - expect($delegate)->toBeInstanceOf(LogTransport::class); + Mail::mailer('cp_support')->raw('body', function ($message) { + $message->to('customer@example.test')->subject('Test'); + }); }); -it('delegates to a real SMTP transport once MAIL_MAILER is no longer log', function () { +it('does not deliver for real when MAIL_MAILER is array, the suite-wide test default', function () { + // phpunit.xml forces MAIL_MAILER=array for the whole suite. Task 5 wires + // real mailables to these mailers next — the first feature test that + // triggers one without Mail::fake() must not open a real socket just + // because 'array' is not the exact string 'log'. Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'array'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect(mailboxDelegate($transport))->toBeInstanceOf(ArrayTransport::class); +}); + +it('does not deliver for real when MAIL_MAILER is not configured at all', function () { + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', null); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect(mailboxDelegate($transport))->toBeInstanceOf(NullTransport::class); +}); + +it('reuses the same delegate until the mailbox credentials actually change', function () { + // The headline feature of the class. A worker builds its mailer once and + // keeps it for hours, so a password the operator corrects in the console + // must reach the NEXT mail without a restart — but not open a new SMTP + // connection for every single message when nothing changed either. + $box = Mailbox::factory()->create([ + 'key' => 'support', 'address' => 'support@clupilot.com', 'password' => 'first-password', + ]); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); config()->set('mail.default', 'smtp'); $transport = Mail::mailer('cp_support')->getSymfonyTransport(); - $delegate = Closure::bind(fn () => $this->delegate(), $transport, MailboxTransport::class)(); - expect($delegate)->toBeInstanceOf(EsmtpTransport::class); + $first = mailboxDelegate($transport); + $second = mailboxDelegate($transport); + + expect($second)->toBe($first); // nothing changed — no new connection per mail + + $box->update(['password' => 'second-password']); + + $third = mailboxDelegate($transport); + + expect($third)->not->toBe($first); // the operator's correction takes effect immediately +}); + +it('opens a plain connection with a STARTTLS upgrade for the submission port', function () { + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + Settings::set('mail.port', 587); + Settings::set('mail.encryption', 'tls'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect((string) mailboxDelegate($transport))->toBe('smtp://mail.example.test:587'); +}); + +it('opens an implicit-TLS connection for the SMTPS port, never a hanging STARTTLS one', function () { + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + Settings::set('mail.port', 465); + Settings::set('mail.encryption', 'ssl'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect((string) mailboxDelegate($transport))->toBe('smtps://mail.example.test'); +}); + +it('refuses to send when the mail server host is blank', function () { + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + Settings::set('mail.host', ''); // Tasks 6-7 are what write this — nothing has yet + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class); +}); + +it('refuses to send when the mail server port is not a positive number', function () { + Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + Settings::set('mail.port', null); // what a stored NULL json-decodes to, then casts to 0 + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + + expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class); });