Stop downgrading an opportunistic smtp scheme to plaintext

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.

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
+     * below already carries that intent, so a plain 'smtp://' is
-     * to "none" here the way an explicit MAIL_SCHEME=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',
         });
 

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'] ?? <port fallback>, 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 () {