From 15c81489d498f4d6376302f7423b2839e03a86e5 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 28 Jul 2026 06:47:11 +0200 Subject: [PATCH] Stop downgrading an opportunistic smtp scheme to plaintext MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAIL_SCHEME=smtp is Symfony's opportunistic scheme (upgrade to STARTTLS if offered, don't fail if not) — not a "no encryption" statement. Mapping it to 'none' silently turned every existing MAIL_SCHEME=smtp installation's encrypted-when-possible connection into an always- plaintext one. Maps to 'tls' (STARTTLS required) instead: stricter than smtp's own default, so a relay lacking STARTTLS now fails loudly rather than leaking credentials. --- ...100000_seed_mailboxes_from_environment.php | 45 +++++++++++++++++-- .../Feature/Mail/MailboxSeedMigrationTest.php | 41 +++++++++++++++-- 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php index 679af43..8ac53f3 100644 --- a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php +++ b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php @@ -144,8 +144,10 @@ return new class extends Migration * else lets an operator say implicit TLS through a URL alone. A plain * 'smtp://' is simply the ordinary, only real-world way to write this * mailer's URL and says nothing about TLS either way; the port heuristic - * below already carries that intent, so it is deliberately NOT promoted - * to "none" here the way an explicit MAIL_SCHEME=smtp is. + * below already carries that intent, so a plain 'smtp://' is + * deliberately left resolving to null (no stated scheme) here, unlike an + * explicit MAIL_SCHEME=smtp. See the match in seed() for what each of + * 'smtp', 'smtps' and null actually maps to, and why (Codex R15#7, P1). * * Anything else — including this machine's own MAIL_SCHEME=tls, which * Symfony's EsmtpTransportFactory rejects outright (it supports only @@ -190,9 +192,46 @@ return new class extends Migration // 465 means implicit TLS, everything else means STARTTLS. $scheme = $this->resolveScheme($smtp); + // Codex R15#7, P1: 'smtp' => 'tls', not 'none'. MAIL_SCHEME=smtp is + // Symfony's OPPORTUNISTIC scheme — it starts the connection in the + // clear and upgrades to STARTTLS only if the server's EHLO happens + // to offer it, never failing if it does not (see MailTlsPolicy's own + // docblock on autoTls vs. requireTls). It is NOT a "no encryption" + // statement. Round 4 of this review mapped it to 'none' on the wrong + // theory that 'smtp' was a plain, non-TLS scheme — every existing + // installation with MAIL_SCHEME=smtp (including the .env.example + // value) would have had this migration silently turn a working, + // encrypted-when-possible connection into a fully plaintext one, + // exposing credentials and message contents with no warning. + // + // 'tls' (STARTTLS required) is deliberately STRICTER than smtp's own + // opportunistic default, not a faithful reproduction of it: a relay + // that genuinely does not offer STARTTLS now fails LOUDLY on the + // first send instead of handing a mailbox password over in the + // clear. That is the correct failure direction — the operator can + // then choose 'none' in the console deliberately, an explicit act + // rather than a silent migration default. + // + // Deliberately no fourth, "opportunistic" stored value: round 1 of + // this review is exactly what closed that hole (a network attacker + // who strips STARTTLS from the EHLO reply gets a plaintext + // connection — the same credential-exposure path). Making + // opportunistic a first-class option here would reopen it by + // design. + // + // Worth flagging rather than leaving implicit: this match can no + // longer produce 'none' AT ALL. resolveScheme() only ever returns + // 'smtp', 'smtps', or null (see its own docblock), and all three + // arms below now resolve to 'ssl' or 'tls'. That is intentional, + // not an oversight — nothing this migration can read (a stated + // scheme, a MAIL_URL scheme, or the absence of either) is a genuine + // "no encryption" statement, so this migration must never + // manufacture one on an operator's behalf. 'none' remains a real, + // selectable choice in the console; it is just not a migration + // output. Settings::set('mail.encryption', match ($scheme) { 'smtps' => 'ssl', - 'smtp' => 'none', + 'smtp' => 'tls', default => $port === 465 ? 'ssl' : 'tls', }); diff --git a/tests/Feature/Mail/MailboxSeedMigrationTest.php b/tests/Feature/Mail/MailboxSeedMigrationTest.php index 7e9ad03..c8815d3 100644 --- a/tests/Feature/Mail/MailboxSeedMigrationTest.php +++ b/tests/Feature/Mail/MailboxSeedMigrationTest.php @@ -93,18 +93,48 @@ it('treats the invalid MAIL_SCHEME=tls value this machine\'s own .env carries as expect(Settings::get('mail.encryption'))->toBe('tls'); }); -it('derives none from an explicit smtp MAIL_SCHEME, even on port 465', function () { +it('derives tls (STARTTLS required) from an explicit smtp MAIL_SCHEME, even on port 465 — Codex R15#7, P1', function () { // The other explicit direction: MAIL_SCHEME=smtp is just as deliberate a // statement as smtps, and must win over the port heuristic the same way // — an operator who said "plain smtp" was never asking for implicit TLS // just because their port happens to be 465. + // + // But 'smtp' is NOT a "no encryption" statement — it is Symfony's + // OPPORTUNISTIC scheme (autoTls, on by default: upgrade to STARTTLS if + // the server offers it, never fail if it does not). Round 4 of this + // review mapped this case to 'none' on the wrong theory that 'smtp' was + // a plain, non-TLS scheme; that silently turned every existing + // MAIL_SCHEME=smtp installation's encrypted-when-possible connection + // into an always-plaintext one the moment this migration ran — the + // credential- and message-exposing bug this test now guards against. + // 'tls' (STARTTLS REQUIRED) is deliberately stricter than smtp's own + // opportunistic default: a relay that does not offer STARTTLS now fails + // loudly instead of sending a mailbox password in the clear. clearMailboxSeed(); config()->set('mail.mailers.smtp.scheme', 'smtp'); config()->set('mail.mailers.smtp.port', 465); loadMailboxSeedMigration()->up(); - expect(Settings::get('mail.encryption'))->toBe('none'); + expect(Settings::get('mail.encryption'))->toBe('tls'); +}); + +it('falls back to the port heuristic when no scheme is stated anywhere — Codex R15#7, P1', function () { + // The fifth scenario the R15#7 fix has to keep working, alongside smtp, + // smtps, smtp:// and smtps://: nobody stated a scheme at all (no + // MAIL_SCHEME, no MAIL_URL). resolveScheme() returns null, the match's + // default arm applies, and — since this is not port 465 — that arm must + // still read 'tls', exactly as before this fix. A genuinely unstated + // scheme was never part of the 'smtp' => 'none' bug and must not become + // collateral damage of fixing it. + clearMailboxSeed(); + config()->set('mail.mailers.smtp.scheme', null); + config()->set('mail.mailers.smtp.url', null); + config()->set('mail.mailers.smtp.port', 587); + + loadMailboxSeedMigration()->up(); + + expect(Settings::get('mail.encryption'))->toBe('tls'); }); it('does not treat a plain smtp:// MAIL_URL scheme as a stated encryption choice', function () { @@ -130,14 +160,17 @@ it('prefers an explicit MAIL_SCHEME over a conflicting MAIL_URL scheme', functio // scheme is doing double duty (see above). When both are present and // disagree, the unambiguous one wins — matching createSmtpTransport()'s // own precedence ($config['scheme'] ?? , checked before - // anything born from the URL even enters the picture). + // anything born from the URL even enters the picture). 'tls', not + // 'ssl': MAIL_SCHEME=smtp still wins the conflict, but it means + // "STARTTLS required", not implicit TLS — see the R15#7 test above and + // the match in seed() for why 'smtp' is no longer 'none' either. clearMailboxSeed(); config()->set('mail.mailers.smtp.scheme', 'smtp'); config()->set('mail.mailers.smtp.url', 'smtps://user:pass@smtp.clupilot.test:587'); loadMailboxSeedMigration()->up(); - expect(Settings::get('mail.encryption'))->toBe('none'); + expect(Settings::get('mail.encryption'))->toBe('tls'); }); it('treats config/mail.php\'s own 127.0.0.1 / 2525 defaults as unset, not as a real relay', function () {