Stop demanding a cipher, a real port, or a decrypted password a mailbox never uses

MailboxTester required a usable SECRETS_KEY unconditionally, refusing an
unauthenticated relay's test-send on a fresh install with no key yet, even
though real sending never touches the cipher for that mailbox. Gated the
check on authenticates, same as the password-presence check beside it.

Comparison pass across MailboxTester and MailboxTransport turned up two more
of the same shape. MailboxTransport's delegate-cache fingerprint decrypted
$box->password unconditionally, even when unauthenticated and never going to
use it — a mailbox that once authenticated, stored a password, and later had
authenticates unchecked would crash real sending the moment SECRETS_KEY
became unusable. And MailboxTester had no guard against a stored port <1;
Symfony silently reinterprets that as port 25 rather than refusing it, so the
test button could reach whatever happens to listen there and report success
for a configuration MailboxTransport refuses outright.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

app/Mail/Transport/MailboxTransport.php

@@ -155,8 +155,20 @@ class MailboxTransport implements TransportInterface
         // long-running queue worker would keep the OLD delegate, which
         // already has setUsername()/setPassword() called on it from before
         // the toggle, still attempting AUTH the operator just turned off.
+        //
+        // Codex R15#5, P2 comparison pass: the password component is read
+        // the same way setPassword() below is gated — only when this mailbox
+        // authenticates. $box->password decrypts under SECRETS_KEY, and an
+        // unauthenticated mailbox that once had a password (an operator
+        // unchecked "requires a password" without clearing the field, which
+        // EditMailbox::save() deliberately leaves alone) still carries that
+        // ciphertext in the column. Decrypting it here regardless would
+        // crash a real send the moment SECRETS_KEY became unusable, purely
+        // to fingerprint a value setUsername()/setPassword() below is never
+        // going to read either.
         $fingerprint = md5(implode('|', [
-            $host, $port, $encryption, (int) $box->authenticates, $box->smtpUsername(), (string) $box->password,
+            $host, $port, $encryption, (int) $box->authenticates, $box->smtpUsername(),
+            $box->authenticates ? (string) $box->password : '',
         ]));
 
         if ($this->delegate === null || $this->fingerprint !== $fingerprint) {

app/Services/Mail/MailboxTester.php

@@ -34,7 +34,15 @@ final class MailboxTester
         // this shape of bug. The tester touches the same cipher and gets the
         // same guard, checked here rather than left for the property access
         // below to throw through.
-        if (! $this->cipher->isUsable()) {
+        //
+        // Codex R15#5, P2: gated on authenticates, same as the password
+        // presence check right below. When this mailbox does not
+        // authenticate, $box->password is never read (see the credentials
+        // ternary further down) — requiring a usable cipher anyway refused a
+        // configuration real sending accepts, on exactly the "fresh install,
+        // no SECRETS_KEY yet" installation the unauthenticated-relay support
+        // exists for.
+        if ($box->authenticates && ! $this->cipher->isUsable()) {
             return ['ok' => false, 'error' => __('mail_settings.no_key')];
         }
 
@@ -48,6 +56,23 @@ final class MailboxTester
         }
 
         $port = (int) Settings::get('mail.port', 587);
+
+        // Codex R15#5, P2 comparison pass: MailboxTransport::delegate() has
+        // an equivalent guard (host==='' too — but that one is not repeated
+        // here, since Mail::build() with a blank host already fails just as
+        // fast on its own, confirmed by hand). Port is different in kind, not
+        // merely in wording: a stored NULL setting casts to 0, and left
+        // unguarded, Symfony's EsmtpTransportFactory silently reinterprets
+        // port 0 as port 25 rather than rejecting it — confirmed by hand,
+        // not assumed. Without this guard, a test-send against a mailbox
+        // whose port was never (or no longer) configured could actually
+        // reach whatever happens to listen on 25 (a local MTA, say) and
+        // report success for a configuration MailboxTransport refuses
+        // outright before it ever opens a socket.
+        if ($port < 1) {
+            return ['ok' => false, 'error' => __('mail_settings.test_port_not_configured')];
+        }
+
         $policy = MailTlsPolicy::for((string) Settings::get('mail.encryption', 'tls'), $port);
 
         // Omitted entirely, not merely blank, when this mailbox does not

lang/de/mail_settings.php

@@ -58,6 +58,7 @@ return [
     'test_subject' => 'CluPilot — Testnachricht',
     'test_body' => 'Diese Nachricht bestätigt, dass das Postfach :key verschicken kann.',
     'test_no_password' => 'Für dieses Postfach ist kein Passwort hinterlegt.',
+    'test_port_not_configured' => 'Der Mailserver-Port ist nicht konfiguriert.',
     'test_ok' => 'Zugestellt. Das Postfach kann verschicken.',
     'test_failed' => 'Der Mailserver hat abgelehnt:',
 ];

lang/en/mail_settings.php

@@ -58,6 +58,7 @@ return [
     'test_subject' => 'CluPilot — test message',
     'test_body' => 'This message confirms that the mailbox :key can send.',
     'test_no_password' => 'No password is stored for this mailbox.',
+    'test_port_not_configured' => 'The mail server port is not configured.',
     'test_ok' => 'Delivered. The mailbox can send.',
     'test_failed' => 'The mail server rejected it:',
 ];

tests/Feature/Mail/MailboxTesterTest.php

@@ -169,6 +169,34 @@ it('gives a readable message instead of throwing when SECRETS_KEY is unusable',
         ->and($result['error'])->toBe(__('mail_settings.no_key'));
 });
 
+// --- Codex R15#5, P2: an unauthenticated mailbox never reads $box->password
+// (see the credentials ternary above in run() and the guard just above it),
+// so requiring a usable SECRETS_KEY before even checking authenticates
+// refused a configuration real sending accepts — on exactly the "fresh
+// install, no SECRETS_KEY yet" installation the unauthenticated-relay
+// support (Codex R15#4, P1b) exists for.
+
+it('does not require a usable cipher for an unauthenticated mailbox, even with no key configured at all', function () {
+    $server = FakeSmtpServer::succeeding();
+
+    try {
+        config()->set('admin_access.secrets_key', '');
+        Settings::set('mail.host', '127.0.0.1');
+        Settings::set('mail.port', $server->port);
+
+        $box = Mailbox::factory()->create([
+            'address' => 'no-reply@clupilot.com', 'username' => null, 'password' => null, 'authenticates' => false,
+        ]);
+
+        $result = app(MailboxTester::class)->run($box, 'ziel@example.com');
+
+        expect($result['ok'])->toBeTrue()
+            ->and($result['error'])->toBeNull();
+    } finally {
+        $server->stop();
+    }
+});
+
 /*
 | LOAD-BEARING for the success path. Every test above only proves a FAILURE
 | is reported correctly — none of them ever reaches
@@ -305,3 +333,32 @@ it('still delivers over a plain connection when encryption is none, against a se
         $server->stop();
     }
 });
+
+// --- Codex R15#5, P2 comparison pass: MailboxTransport::delegate() refuses
+// host==='' and port<1 with its own explicit, loud guards BEFORE building
+// anything (see its docblock: "Loud beats a silently downgraded
+// connection"). MailboxTester had neither. host==='' turned out not to be a
+// real divergence — confirmed by hand, not assumed: Mail::build() with a
+// blank host still fails fast (a few ms, via getaddrinfo), so both paths
+// already refuse, just with different wording. port<1 is different in kind,
+// not merely in wording: Symfony's EsmtpTransportFactory silently
+// reinterprets an invalid port as 25 rather than rejecting it (confirmed by
+// hand too), so MailboxTester would have gone on to actually attempt a
+// connection on port 25 — accepting a configuration the real transport
+// refuses outright. If a local MTA happens to listen there (common enough on
+// a self-hosted box), the test button could report success for exactly the
+// send MailboxTransport would never even attempt. Only the port guard is
+// added below; a host guard would be redundant with what Mail::build()
+// already does correctly on its own.
+
+it('refuses to send when the mail server port is not a positive number, rather than letting Symfony silently fall back to port 25', function () {
+    Settings::set('mail.host', '127.0.0.1');
+    Settings::set('mail.port', null); // what a stored NULL json-decodes to, then casts to 0 — mirrors MailboxTransportTest's identical scenario
+
+    $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'])->toBe(__('mail_settings.test_port_not_configured'));
+});

tests/Feature/Mail/MailboxTransportTest.php

@@ -246,6 +246,45 @@ it('rebuilds the delegate when authenticates toggles, even though host, port, en
         ->and($second->getUsername())->toBe('');
 });
 
+// --- Codex R15#5, P2 comparison pass: the fingerprint line above reads
+// (string) $box->password UNCONDITIONALLY, even when authenticates is false
+// and setUsername()/setPassword() are never called on the result. $box
+// ->password decrypts under SECRETS_KEY, so a mailbox that once
+// authenticated, stored a password, and later had authenticates unchecked —
+// leaving the old ciphertext in place, since an empty password field in
+// EditMailbox means "leave it alone" — would crash a real send the moment
+// SECRETS_KEY became unusable, purely to compute a cache key for a value it
+// was never going to use. Same root cause as the MailboxTester fix above.
+
+it('does not decrypt a stale password to build the delegate for an unauthenticated mailbox, even without a usable cipher', function () {
+    $box = Mailbox::factory()->create([
+        'key' => 'support', 'address' => 'support@clupilot.com',
+        'password' => 'a-stale-password', 'authenticates' => false,
+    ]);
+    Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
+    config()->set('mail.default', 'smtp');
+
+    config()->set('admin_access.secrets_key', '');
+
+    $transport = Mail::mailer('cp_support')->getSymfonyTransport();
+
+    // No expect(fn () => ...)->not->toThrow(Throwable::class) here on
+    // purpose: Throwable is an interface, and Pest's toThrow() falls back to
+    // treating a class-string it cannot class_exists() as a MESSAGE
+    // substring to search for instead — so that matcher silently passes
+    // regardless of what is thrown (confirmed by hand: a bare `throw new
+    // RuntimeException('boom')` inside such a closure still reports as not
+    // having thrown). Calling delegate() directly and asserting on its
+    // return type instead means an uncaught exception fails this test the
+    // ordinary way, with its real stack trace, rather than being silently
+    // absorbed by a matcher that never engages.
+    $delegate = mailboxDelegate($transport);
+
+    expect($delegate)->toBeInstanceOf(EsmtpTransport::class)
+        ->and($delegate->getUsername())->toBe('')
+        ->and($delegate->getPassword())->toBe('');
+});
+
 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');