diff --git a/app/Mail/Transport/MailboxTransport.php b/app/Mail/Transport/MailboxTransport.php index 25b9adf..9885bd1 100644 --- a/app/Mail/Transport/MailboxTransport.php +++ b/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) { diff --git a/app/Services/Mail/MailboxTester.php b/app/Services/Mail/MailboxTester.php index 5a1fa01..8a33967 100644 --- a/app/Services/Mail/MailboxTester.php +++ b/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 diff --git a/lang/de/mail_settings.php b/lang/de/mail_settings.php index ef47c0c..3e5c804 100644 --- a/lang/de/mail_settings.php +++ b/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:', ]; diff --git a/lang/en/mail_settings.php b/lang/en/mail_settings.php index 46a719f..9f5c296 100644 --- a/lang/en/mail_settings.php +++ b/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:', ]; diff --git a/tests/Feature/Mail/MailboxTesterTest.php b/tests/Feature/Mail/MailboxTesterTest.php index 46f7f9a..ad9fb6f 100644 --- a/tests/Feature/Mail/MailboxTesterTest.php +++ b/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')); +}); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index a3c5913..6072af2 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/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');