Make ssl, tls, and none mean what they say on the wire

EsmtpTransport's autoTls (default true) and requireTls (default false)
are independent of the implicit-TLS constructor argument: 'tls' left
these at their defaults, so a server that omitted STARTTLS sent
mailbox credentials in the clear while reporting success, and 'none'
would silently upgrade the moment a server offered STARTTLS. Both
MailboxTransport and MailboxTester already duplicated the "ssl, or
port 465 ⇒ implicit" half of this decision independently; MailTlsPolicy
is now the one place both read, for both dimensions.

MailboxTester still builds through Mail::build() rather than a hand-built
transport: MailManager::createSmtpTransport() forwards the whole config
array to EsmtpTransportFactory as DSN options, which already reads
auto_tls/require_tls itself, so the same policy reaches this path
through keys the config array already supports.

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

app/Services/Mail/MailTlsPolicy.php

@@ -0,0 +1,78 @@
+<?php
+
+namespace App\Services\Mail;
+
+use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
+
+/**
+ * What a stored `mail.encryption` choice ("ssl", "tls", "none") actually
+ * means on the wire — decided in exactly ONE place so MailboxTransport (real
+ * sends) and MailboxTester (the test-send button) cannot silently enforce two
+ * different policies for the same setting. Before this existed, both files
+ * already independently re-derived the "ssl, or port 465 ⇒ implicit TLS"
+ * half of this decision; a second dimension (require STARTTLS, or forbid it)
+ * is exactly the kind of thing that drifts between two copies instead of
+ * failing loudly when it does.
+ *
+ * EsmtpTransport (vendor/symfony/mailer/Transport/Smtp/EsmtpTransport.php in
+ * the installed version) exposes three independent switches, not one:
+ *
+ * - The constructor's $tls argument: implicit TLS. true wraps the socket in
+ *   TLS before speaking SMTP at all (smtps, port 465 in practice); false
+ *   starts the connection in the clear, which is what STARTTLS needs to have
+ *   something to upgrade FROM. Passing true against a STARTTLS-only server
+ *   opens an SSL socket the server never answers; passing false against an
+ *   implicit-TLS-only server sends a plaintext EHLO it will not understand.
+ * - autoTls (default true): opportunistic STARTTLS. If the connection did
+ *   not start under implicit TLS and the server's EHLO advertises STARTTLS,
+ *   Symfony upgrades automatically — REGARDLESS of the stored encryption
+ *   choice. A stored choice of "none" left at this default would still
+ *   silently upgrade the moment a STARTTLS-capable server showed up.
+ * - requireTls (default false): after negotiation, if TLS was not
+ *   established by ANY means (implicit or STARTTLS), throw instead of
+ *   continuing. This is what makes "tls" a requirement rather than a
+ *   suggestion — a server (or a network attacker) that omits STARTTLS must
+ *   fail loud, not send mailbox credentials in the clear.
+ */
+final class MailTlsPolicy
+{
+    private function __construct(
+        public readonly bool $implicit,
+        public readonly bool $autoTls,
+        public readonly bool $requireTls,
+    ) {}
+
+    /**
+     * Port 465 forces implicit TLS no matter what the stored encryption
+     * setting says — unchanged from before this fix. A server listening
+     * there speaks TLS from the very first byte and will not understand a
+     * plaintext EHLO, so a mismatched setting must not be allowed to turn
+     * into a hang.
+     */
+    public static function for(string $encryption, int $port): self
+    {
+        if ($encryption === 'ssl' || $port === 465) {
+            return new self(implicit: true, autoTls: true, requireTls: true);
+        }
+
+        if ($encryption === 'tls') {
+            return new self(implicit: false, autoTls: true, requireTls: true);
+        }
+
+        // 'none', and anything blank or unrecognised (a row predating the
+        // in:tls,ssl,none validation, say): no implicit wrap, and no silent
+        // upgrade either. Safe rather than strict — nobody explicitly chose
+        // "tls" here, so failing loud for a value that merely isn't "ssl"
+        // would be punishing a row this fix did not touch.
+        return new self(implicit: false, autoTls: false, requireTls: false);
+    }
+
+    /** Applies the auto/require dimension to an already-built transport. */
+    public function apply(EsmtpTransport $transport): EsmtpTransport
+    {
+        $transport->setAutoTls($this->autoTls);
+        $transport->setRequireTls($this->requireTls);
+
+        return $transport;
+    }
+}

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(

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

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 () {

tests/Unit/MailTlsPolicyTest.php

@@ -0,0 +1,69 @@
+<?php
+
+use App\Services\Mail\MailTlsPolicy;
+use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
+
+/**
+ * The single place that decides what "ssl", "tls", and "none" mean on the
+ * wire. MailboxTransport (real sends) and MailboxTester (the test-send
+ * button) used to each re-derive the "ssl, or port 465 ⇒ implicit TLS" half
+ * of this on their own — this table is what replaces both copies, so it is
+ * pinned down here independently of either caller.
+ */
+it('maps a stored encryption choice and port to the three EsmtpTransport switches', function (
+    string $encryption,
+    int $port,
+    bool $implicit,
+    bool $autoTls,
+    bool $requireTls,
+) {
+    $policy = MailTlsPolicy::for($encryption, $port);
+
+    expect($policy->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();
+});