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); });