diff --git a/app/Mail/Transport/MailboxTransport.php b/app/Mail/Transport/MailboxTransport.php index d73ad37..c0648f5 100644 --- a/app/Mail/Transport/MailboxTransport.php +++ b/app/Mail/Transport/MailboxTransport.php @@ -4,6 +4,7 @@ namespace App\Mail\Transport; use App\Models\Mailbox; use App\Services\Mail\MailboxResolver; +use App\Services\Mail\MailTlsPolicy; use App\Support\Settings; use Illuminate\Mail\Transport\ArrayTransport; use Illuminate\Mail\Transport\LogTransport; @@ -151,12 +152,14 @@ class MailboxTransport implements TransportInterface ])); if ($this->delegate === null || $this->fingerprint !== $fingerprint) { - // The third argument is IMPLICIT TLS (smtps, port 465) — not - // STARTTLS. EsmtpTransport negotiates STARTTLS by itself on a - // plain connection, which is what port 587 wants. Passing true for - // 'tls' here would open an SSL socket against a server expecting - // STARTTLS and hang. - $transport = new EsmtpTransport($host, $port, $encryption === 'ssl' || $port === 465); + // MailTlsPolicy is the ONE place "ssl, tls, or none" turns into + // EsmtpTransport's three TLS switches — see its docblock. The + // third constructor argument here is implicit TLS (smtps, port + // 465), not STARTTLS: passing true for 'tls' would open an SSL + // socket against a server expecting STARTTLS and hang. + $policy = MailTlsPolicy::for($encryption, $port); + + $transport = $policy->apply(new EsmtpTransport($host, $port, $policy->implicit)); $transport->setUsername($box->smtpUsername()); $transport->setPassword((string) $box->password); diff --git a/app/Services/Mail/MailTlsPolicy.php b/app/Services/Mail/MailTlsPolicy.php new file mode 100644 index 0000000..19af19c --- /dev/null +++ b/app/Services/Mail/MailTlsPolicy.php @@ -0,0 +1,78 @@ +setAutoTls($this->autoTls); + $transport->setRequireTls($this->requireTls); + + return $transport; + } +} diff --git a/app/Services/Mail/MailboxTester.php b/app/Services/Mail/MailboxTester.php index 3b9b3e2..f8589a0 100644 --- a/app/Services/Mail/MailboxTester.php +++ b/app/Services/Mail/MailboxTester.php @@ -43,6 +43,7 @@ final class MailboxTester } $port = (int) Settings::get('mail.port', 587); + $policy = MailTlsPolicy::for((string) Settings::get('mail.encryption', 'tls'), $port); try { Mail::build([ @@ -53,7 +54,18 @@ final class MailboxTester // scheme only (MailManager.php:196) — an 'encryption' key is // silently ignored, and the connection would go out in the // clear while the console reported TLS. - 'scheme' => (Settings::get('mail.encryption') === 'ssl' || $port === 465) ? 'smtps' : 'smtp', + 'scheme' => $policy->implicit ? 'smtps' : 'smtp', + // auto_tls / require_tls: MailManager::createSmtpTransport() + // forwards this whole config array to EsmtpTransportFactory + // as DSN options, and the factory reads exactly these two + // keys itself (see EsmtpTransportFactory::create() in the + // installed vendor source) — so the same MailTlsPolicy that + // configures the real MailboxTransport reaches this path too, + // through the only channel Mail::build()'s config array + // actually offers for it. Without these, this path would + // silently prove a different policy than real sends enforce. + 'auto_tls' => $policy->autoTls, + 'require_tls' => $policy->requireTls, 'username' => $box->smtpUsername(), 'password' => $box->password, ])->raw( diff --git a/tests/Feature/Mail/MailboxTesterTest.php b/tests/Feature/Mail/MailboxTesterTest.php index 36c1f2e..8dedf13 100644 --- a/tests/Feature/Mail/MailboxTesterTest.php +++ b/tests/Feature/Mail/MailboxTesterTest.php @@ -10,7 +10,14 @@ beforeEach(function () { config()->set('mail.default', 'log'); // as on a development machine Settings::set('mail.host', '127.0.0.1'); Settings::set('mail.port', 1); - Settings::set('mail.encryption', 'tls'); + // 'none', not 'tls': FakeSmtpServer only ever speaks plain text (see its + // own docblock) and never advertises STARTTLS. Since the P1 fix makes + // 'tls' REQUIRE a STARTTLS upgrade, keeping this default at 'tls' would + // have broken every success/rejection test below that needs the fake + // server to complete a full plaintext dialogue — none of which are + // actually about the TLS policy. The tests that ARE about it set their + // own encryption explicitly, below. + Settings::set('mail.encryption', 'none'); }); /* @@ -171,3 +178,56 @@ it('actually attempts TLS when SSL is configured on a non-standard port, instead $server->stop(); } }); + +/* +| P1: the test-send button must enforce the exact same TLS policy real +| sending does (MailboxTransportTest carries the identical pair of tests for +| the real path) — otherwise a green test-send proves nothing about what a +| queued mail would actually do. Before the fix, MailboxTester built its +| transport with the default autoTls/requireTls Symfony ships (autoTls on, +| requireTls off): against a server that never offers STARTTLS, autoTls found +| nothing to upgrade to, nothing else objected, and the send completed in the +| clear while reporting success. +*/ +it('fails, rather than sending in the clear, when TLS is required but the server never offers STARTTLS', function () { + $server = FakeSmtpServer::succeeding(); + + try { + Settings::set('mail.host', '127.0.0.1'); + Settings::set('mail.port', $server->port); + Settings::set('mail.encryption', 'tls'); + + $box = Mailbox::factory()->create(['address' => 'no-reply@clupilot.com']); + + $result = app(MailboxTester::class)->run($box, 'ziel@example.com'); + + expect($result['ok'])->toBeFalse() + ->and($result['error'])->toContain('TLS required but neither TLS or STARTTLS are in use.') + ->and($box->fresh()->last_verified_at)->toBeNull(); + } finally { + $server->stop(); + } +}); + +it('still delivers over a plain connection when encryption is none, against a server that never offers TLS', function () { + // The control case for the test above: requireTls must stay OFF for + // 'none', or a legitimately plaintext-only relay would break the moment + // the P1 fix landed. + $server = FakeSmtpServer::succeeding(); + + try { + Settings::set('mail.host', '127.0.0.1'); + Settings::set('mail.port', $server->port); + Settings::set('mail.encryption', 'none'); + + $box = Mailbox::factory()->create(['address' => 'no-reply@clupilot.com']); + + $result = app(MailboxTester::class)->run($box, 'ziel@example.com'); + + expect($result['ok'])->toBeTrue() + ->and($result['error'])->toBeNull() + ->and($box->fresh()->last_verified_at)->not->toBeNull(); + } finally { + $server->stop(); + } +}); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index 3751326..d74af6f 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/tests/Feature/Mail/MailboxTransportTest.php @@ -7,8 +7,11 @@ use App\Support\Settings; use Illuminate\Mail\Transport\ArrayTransport; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Mail; +use Symfony\Component\Mailer\Exception\TransportException; use Symfony\Component\Mailer\Transport\NullTransport; +use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; use Symfony\Component\Mailer\Transport\TransportInterface; +use Tests\Support\FakeSmtpServer; beforeEach(function () { config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); @@ -183,8 +186,16 @@ it('opens a plain connection with a STARTTLS upgrade for the submission port', f Settings::set('mail.encryption', 'tls'); $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $delegate = mailboxDelegate($transport); - expect((string) mailboxDelegate($transport))->toBe('smtp://mail.example.test:587'); + expect((string) $delegate)->toBe('smtp://mail.example.test:587') + ->and($delegate)->toBeInstanceOf(EsmtpTransport::class) + // The P1 fix: 'tls' is a REQUIREMENT, not a suggestion Symfony is + // free to skip when the server omits STARTTLS. autoTls must stay on + // too — without it, the STARTTLS upgrade this mode depends on would + // never even be attempted, and requireTls would then fail every send. + ->and($delegate->isTlsRequired())->toBeTrue() + ->and($delegate->isAutoTls())->toBeTrue(); }); it('opens an implicit-TLS connection for the SMTPS port, never a hanging STARTTLS one', function () { @@ -195,8 +206,108 @@ it('opens an implicit-TLS connection for the SMTPS port, never a hanging STARTTL Settings::set('mail.encryption', 'ssl'); $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $delegate = mailboxDelegate($transport); - expect((string) mailboxDelegate($transport))->toBe('smtps://mail.example.test'); + expect((string) $delegate)->toBe('smtps://mail.example.test') + ->and($delegate)->toBeInstanceOf(EsmtpTransport::class) + ->and($delegate->isTlsRequired())->toBeTrue(); +}); + +it('disables both implicit TLS and the opportunistic STARTTLS upgrade when encryption is none', function () { + // 'none' is a deliberate, explicit choice — not the absence of one — so + // it must not silently upgrade just because a server happens to offer + // STARTTLS. autoTls defaults to true in Symfony; this is the one mode + // that has to turn it off rather than merely leave requireTls false. + 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', 'none'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $delegate = mailboxDelegate($transport); + + expect((string) $delegate)->toBe('smtp://mail.example.test:587') + ->and($delegate)->toBeInstanceOf(EsmtpTransport::class) + ->and($delegate->isAutoTls())->toBeFalse() + ->and($delegate->isTlsRequired())->toBeFalse(); +}); + +/* +| LOAD-BEARING for the P1 security fix — property assertions above prove the +| transport is CONFIGURED correctly, but the recurring defect on this branch +| has been tests that pass whether or not the code is right, so this proves +| the configuration actually changes what goes out on the wire. FakeSmtpServer +| never advertises STARTTLS (see its own docblock), which is exactly the +| shape of the threat Codex named: a server — or a network attacker sitting in +| front of one — that omits the capability. Before the fix, 'tls' passed +| false as EsmtpTransport's third constructor argument and left requireTls at +| its default of false: autoTls found no STARTTLS to use, nothing else +| objected, and the send completed in the clear. after the fix, the same +| conversation must throw instead. +*/ +it('refuses to send in the clear when TLS is required but the server never offers STARTTLS', function () { + $server = FakeSmtpServer::succeeding(); + + try { + 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', '127.0.0.1'); + Settings::set('mail.port', $server->port); + Settings::set('mail.encryption', 'tls'); + + expect(fn () => Mail::mailer('cp_support')->raw('body', function ($message) { + $message->to('customer@example.test')->subject('Test'); + }))->toThrow(TransportException::class, 'TLS required but neither TLS or STARTTLS are in use.'); + } finally { + $server->stop(); + } +}); + +it('still sends over a plain connection when encryption is none, against a server that never offers TLS', function () { + // The control case for the test above: requireTls must stay OFF for + // 'none', or a legitimately plaintext-only relay would break the moment + // this fix landed. + $server = FakeSmtpServer::succeeding(); + + try { + 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', '127.0.0.1'); + Settings::set('mail.port', $server->port); + Settings::set('mail.encryption', 'none'); + + expect(fn () => Mail::mailer('cp_support')->raw('body', function ($message) { + $message->to('customer@example.test')->subject('Test'); + }))->not->toThrow(Throwable::class); + } finally { + $server->stop(); + } +}); + +it('refuses an implicit-TLS connection against a server that only ever speaks plain text', function () { + // The 'ssl' mirror of the two tests above: FakeSmtpServer only ever + // speaks plain text (see its docblock), so a correct implicit-TLS attempt + // must fail the OpenSSL handshake immediately rather than falling back to + // a plaintext conversation. + $server = FakeSmtpServer::succeeding(); + + try { + 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', '127.0.0.1'); + Settings::set('mail.port', $server->port); + Settings::set('mail.encryption', 'ssl'); + + expect(fn () => Mail::mailer('cp_support')->raw('body', function ($message) { + $message->to('customer@example.test')->subject('Test'); + }))->toThrow(TransportException::class); + } finally { + $server->stop(); + } }); it('refuses to send when the mail server host is blank', function () { diff --git a/tests/Unit/MailTlsPolicyTest.php b/tests/Unit/MailTlsPolicyTest.php new file mode 100644 index 0000000..5992490 --- /dev/null +++ b/tests/Unit/MailTlsPolicyTest.php @@ -0,0 +1,69 @@ +implicit)->toBe($implicit) + ->and($policy->autoTls)->toBe($autoTls) + ->and($policy->requireTls)->toBe($requireTls); +})->with([ + // encryption, port, implicit, autoTls, requireTls + + // 'ssl' is implicit TLS from the first byte, on any port — unchanged + // from before this fix. + 'ssl on the submission port' => ['ssl', 587, true, true, true], + 'ssl on the SMTPS port' => ['ssl', 465, true, true, true], + + // Port 465 forces implicit TLS regardless of the stored encryption + // setting, exactly as before this fix: a server listening there speaks + // TLS from byte one and will not answer a plaintext EHLO, so a + // mismatched setting must not turn into a hang. + 'tls on the SMTPS port defers to the port' => ['tls', 465, true, true, true], + 'none on the SMTPS port defers to the port' => ['none', 465, true, true, true], + + // 'tls' is the fix: STARTTLS is REQUIRED, not merely attempted. autoTls + // must stay on too, or the STARTTLS upgrade requireTls checks for would + // never be attempted in the first place. + 'tls on the submission port' => ['tls', 587, false, true, true], + 'tls on a custom port' => ['tls', 2525, false, true, true], + + // 'none' is the other half of the fix: no implicit wrap, and autoTls + // OFF — otherwise a server that happens to offer STARTTLS would still + // silently upgrade despite the operator's explicit choice. + 'none on the submission port' => ['none', 587, false, false, false], + 'none on a custom port' => ['none', 2525, false, false, false], + + // Blank or unrecognised values (a legacy row predating the in:tls,ssl,none + // validation, or one that decoded to something unexpected) fall to the + // same shape as 'none': no implicit wrap, no silent upgrade, no + // spurious failure for a value nobody explicitly chose as 'tls'. + 'blank falls back to the none shape' => ['', 587, false, false, false], + 'garbage falls back to the none shape' => ['garbage', 587, false, false, false], +]); + +it('applies autoTls and requireTls to an already-built transport', function () { + $transport = new EsmtpTransport('mail.example.test', 587, false); + + $policy = MailTlsPolicy::for('tls', 587); + $returned = $policy->apply($transport); + + expect($returned)->toBe($transport) // fluent — the caller chains straight through + ->and($transport->isAutoTls())->toBeTrue() + ->and($transport->isTlsRequired())->toBeTrue(); +});