diff --git a/app/Livewire/EditMailbox.php b/app/Livewire/EditMailbox.php index e3d803e..b4843c2 100644 --- a/app/Livewire/EditMailbox.php +++ b/app/Livewire/EditMailbox.php @@ -38,6 +38,9 @@ class EditMailbox extends ModalComponent public bool $active = true; + /** Whether this mailbox logs in before it sends — Codex R15#4, P1b. */ + public bool $authenticates = true; + public function mount(string $uuid): void { $this->authorize('mail.manage'); @@ -50,6 +53,7 @@ class EditMailbox extends ModalComponent $this->username = (string) $box->username; $this->noReply = $box->no_reply; $this->active = $box->active; + $this->authenticates = $box->authenticates; } public function save(): void @@ -102,20 +106,23 @@ class EditMailbox extends ModalComponent $oldAddress = $box->address; $oldUsername = $box->username; + $oldAuthenticates = $box->authenticates; $box->address = $this->address; $box->display_name = $this->displayName ?: null; $box->username = $this->username ?: null; $box->no_reply = $this->noReply; $box->active = $this->active; + $box->authenticates = $this->authenticates; // Either one changes the identity smtpUsername() authenticates as: // username directly, or address as username's fallback when none is // set (including username being CLEARED back to that fallback). - // Leaving last_verified_at standing after either would keep showing - // a successful verification for credentials nothing has actually - // tested against the new identity. - if ($box->address !== $oldAddress || $box->username !== $oldUsername) { + // authenticates itself is the fourth: whether that identity gets + // sent at all. Leaving last_verified_at standing after any of the + // three would keep showing a successful verification for a + // connection nothing has actually tested since. + if ($box->address !== $oldAddress || $box->username !== $oldUsername || $box->authenticates !== $oldAuthenticates) { $box->last_verified_at = null; } diff --git a/app/Mail/Transport/MailboxTransport.php b/app/Mail/Transport/MailboxTransport.php index c0648f5..25b9adf 100644 --- a/app/Mail/Transport/MailboxTransport.php +++ b/app/Mail/Transport/MailboxTransport.php @@ -147,8 +147,16 @@ class MailboxTransport implements TransportInterface throw new RuntimeException('Mail server port is not configured — cannot send mail.'); } + // authenticates is part of the fingerprint too, not only host/port/ + // encryption/username/password: Mailbox::smtpUsername() always + // returns something non-empty (it falls back to the address), so an + // operator toggling authenticates off with everything else unchanged + // must still invalidate the cached delegate below — otherwise a + // 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. $fingerprint = md5(implode('|', [ - $host, $port, $encryption, $box->smtpUsername(), (string) $box->password, + $host, $port, $encryption, (int) $box->authenticates, $box->smtpUsername(), (string) $box->password, ])); if ($this->delegate === null || $this->fingerprint !== $fingerprint) { @@ -160,8 +168,17 @@ class MailboxTransport implements TransportInterface $policy = MailTlsPolicy::for($encryption, $port); $transport = $policy->apply(new EsmtpTransport($host, $port, $policy->implicit)); - $transport->setUsername($box->smtpUsername()); - $transport->setPassword((string) $box->password); + + // Codex R15#4, P1b: only called when this mailbox actually + // authenticates. smtpUsername()'s address fallback means it is + // NEVER empty, so calling setUsername() unconditionally would + // make Symfony attempt AUTH against any server that advertises + // it — exactly what a trusted, unauthenticated relay was never + // asked to answer. + if ($box->authenticates) { + $transport->setUsername($box->smtpUsername()); + $transport->setPassword((string) $box->password); + } $this->delegate = $transport; $this->fingerprint = $fingerprint; diff --git a/app/Models/Mailbox.php b/app/Models/Mailbox.php index 6677b80..0edaf15 100644 --- a/app/Models/Mailbox.php +++ b/app/Models/Mailbox.php @@ -20,7 +20,7 @@ class Mailbox extends Model protected $fillable = [ 'key', 'address', 'display_name', 'username', - 'password', 'no_reply', 'active', 'last_verified_at', + 'password', 'no_reply', 'active', 'authenticates', 'last_verified_at', ]; protected function casts(): array @@ -28,6 +28,7 @@ class Mailbox extends Model return [ 'no_reply' => 'boolean', 'active' => 'boolean', + 'authenticates' => 'boolean', 'last_verified_at' => 'datetime', ]; } @@ -61,9 +62,18 @@ class Mailbox extends Model : $this->address; } - /** Enough to send with: an address and a password. */ + /** + * Enough to send with: an address, and a password ONLY when this mailbox + * actually authenticates. + * + * A trusted local or private-network relay can legitimately need no + * password at all (Codex R15#4, P1b) — requiring one unconditionally + * would refuse a relay that was never broken, just unauthenticated. + */ public function isConfigured(): bool { - return $this->active && $this->address !== '' && $this->getRawOriginal('password') !== null; + return $this->active + && $this->address !== '' + && (! $this->authenticates || $this->getRawOriginal('password') !== null); } } diff --git a/app/Services/Mail/MailboxTester.php b/app/Services/Mail/MailboxTester.php index f8589a0..5a1fa01 100644 --- a/app/Services/Mail/MailboxTester.php +++ b/app/Services/Mail/MailboxTester.php @@ -38,13 +38,30 @@ final class MailboxTester return ['ok' => false, 'error' => __('mail_settings.no_key')]; } - if ($box->password === null) { + // Codex R15#4, P1b: a password is only REQUIRED when this mailbox + // actually authenticates — a trusted local or private-network relay + // can legitimately need none at all, and refusing it here is exactly + // the "no password stored" outage the finding names, even though the + // relay was never broken. + if ($box->authenticates && $box->password === null) { return ['ok' => false, 'error' => __('mail_settings.test_no_password')]; } $port = (int) Settings::get('mail.port', 587); $policy = MailTlsPolicy::for((string) Settings::get('mail.encryption', 'tls'), $port); + // Omitted entirely, not merely blank, when this mailbox does not + // authenticate: Mailbox::smtpUsername() always returns something + // non-empty (it falls back to the address), so passing it through + // regardless would still hand Mail::build() a real 'username' — + // enough by itself to make Symfony attempt AUTH against any server + // that advertises it, which a trusted no-auth relay was never asked + // to answer. Mirrors MailboxTransport's own setUsername()/ + // setPassword() guard for the real send path. + $credentials = $box->authenticates + ? ['username' => $box->smtpUsername(), 'password' => $box->password] + : []; + try { Mail::build([ 'transport' => 'smtp', @@ -66,8 +83,7 @@ final class MailboxTester // silently prove a different policy than real sends enforce. 'auto_tls' => $policy->autoTls, 'require_tls' => $policy->requireTls, - 'username' => $box->smtpUsername(), - 'password' => $box->password, + ...$credentials, ])->raw( __('mail_settings.test_body', ['key' => $box->key]), function ($message) use ($box, $recipient) { diff --git a/database/factories/MailboxFactory.php b/database/factories/MailboxFactory.php index 2b2cbae..af11b21 100644 --- a/database/factories/MailboxFactory.php +++ b/database/factories/MailboxFactory.php @@ -16,6 +16,7 @@ class MailboxFactory extends Factory 'display_name' => 'CluPilot', 'username' => null, 'password' => 'test-passwort', + 'authenticates' => true, 'no_reply' => false, 'active' => true, ]; diff --git a/database/migrations/2026_07_28_095000_add_authenticates_to_mailboxes_table.php b/database/migrations/2026_07_28_095000_add_authenticates_to_mailboxes_table.php new file mode 100644 index 0000000..cae8d56 --- /dev/null +++ b/database/migrations/2026_07_28_095000_add_authenticates_to_mailboxes_table.php @@ -0,0 +1,41 @@ +boolean('authenticates')->default(true)->after('password'); + }); + } + + public function down(): void + { + Schema::table('mailboxes', function (Blueprint $table) { + $table->dropColumn('authenticates'); + }); + } +}; 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 8c7e339..679af43 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 @@ -246,21 +246,51 @@ return new class extends Migration // without a password, exactly like the four addresses below it. $canEncrypt = app(SecretCipher::class)->isUsable(); + // Codex R15#4, P1b: "neither a username nor a password" is what + // Laravel's SMTP mailer has always accepted as a legitimate + // unauthenticated relay — a trusted local or private-network server + // that authenticates by IP rather than by login. Read from the + // resolved SMTP config only (not from $storedPassword, the vault's + // own separate, legacy carry-over mechanism a few lines below this + // one — folding that in would mean a leftover vault row from a much + // older round could flip this even though today's .env states no + // credentials at all, a scope this fix deliberately leaves alone). + // A username ALONE, with no password anywhere, stays authenticates = + // true: that is an operator who started typing, not one who chose no + // auth, and isConfigured() must keep asking for a password rather + // than reading it as a deliberately open relay. + $authenticates = $configuredUser !== '' || $envPassword !== ''; + foreach (self::SEED as $key => $meta) { - $isConfigured = $key === 'no-reply' && $configuredUser !== ''; + // A real account exists when the takeover found somewhere to + // actually send through (the host — MailboxTransport's own first + // guard refuses to send without one too) AND someone to name it + // from ($resolvedAddress, which itself falls back through + // mail.from.address to the SMTP login). A username is + // deliberately NOT required here the way it used to be: gating + // on it exclusively is exactly what left an unauthenticated + // relay seeded as a placeholder, unable to send, until this fix. + $isConfigured = $key === 'no-reply' && $host !== '' && $resolvedAddress !== ''; $box = Mailbox::firstOrNew(['key' => $key]); $box->fill([ 'address' => $isConfigured ? $resolvedAddress : $key.'@'.$domain, 'display_name' => $displayName, - // Null when the login and address match: Mailbox:: - // smtpUsername() already falls back to address in that case, - // so writing the identical string into both columns would - // just be a redundant copy — one more thing to keep in sync - // as either changes later through the console. - 'username' => $isConfigured && $configuredUser !== $resolvedAddress ? $configuredUser : null, + // Null when the login and address match, OR when there is no + // login at all (an unauthenticated relay): Mailbox:: + // smtpUsername() already falls back to address in both + // cases, so writing '' into the column would just be a + // redundant, misleading copy of "no separate login". + 'username' => $isConfigured && $configuredUser !== '' && $configuredUser !== $resolvedAddress + ? $configuredUser : null, 'no_reply' => $meta['no_reply'], 'active' => true, + // Placeholders default true (the column's own default) the + // same way active does above — a fabricated support@/ + // billing@/etc. address must keep asking for a password, not + // silently inherit "no auth needed" from the real account it + // sits next to in this loop. + 'authenticates' => $isConfigured ? $authenticates : true, ]); // Only the account that actually exists gets credentials. diff --git a/lang/de/mail_settings.php b/lang/de/mail_settings.php index ee8d8b9..ef47c0c 100644 --- a/lang/de/mail_settings.php +++ b/lang/de/mail_settings.php @@ -22,6 +22,9 @@ return [ 'username_hint' => 'Leer lassen, wenn er der Adresse entspricht.', 'password' => 'Passwort', 'password_hint' => 'Wird verschlüsselt gespeichert und nie wieder vollständig angezeigt. Leer lassen, um das bisherige zu behalten.', + 'password_hint_unauthenticated' => 'Wird nicht verwendet — dieses Postfach ist auf Versand ohne Anmeldung eingestellt.', + 'authenticates' => 'Passwort erforderlich', + 'authenticates_hint' => 'Deaktivieren für einen vertrauenswürdigen Relay, der Mail ohne Anmeldung annimmt — etwa einen lokalen Server oder einen im eigenen Netz. Das Passwort oben wird ignoriert, solange dies deaktiviert ist.', 'no_reply' => 'Keine Antworten (kein Reply-To)', 'no_reply_hint' => 'Ein „no-reply", auf das man antworten kann, ist eine Lüge im Absender.', 'active' => 'Aktiv', diff --git a/lang/en/mail_settings.php b/lang/en/mail_settings.php index a2fdc35..46a719f 100644 --- a/lang/en/mail_settings.php +++ b/lang/en/mail_settings.php @@ -22,6 +22,9 @@ return [ 'username_hint' => 'Leave empty if it matches the address.', 'password' => 'Password', 'password_hint' => 'Stored encrypted and never shown in full again. Leave empty to keep the current one.', + 'password_hint_unauthenticated' => 'Not used — this mailbox is set to send without logging in.', + 'authenticates' => 'Requires a password', + 'authenticates_hint' => 'Uncheck for a trusted relay that accepts mail without logging in — a local or private-network server, for example. The password above is ignored while this is unchecked.', 'no_reply' => 'No replies (no Reply-To)', 'no_reply_hint' => 'A "no-reply" that can be replied to is a lie in the sender.', 'active' => 'Active', diff --git a/resources/views/livewire/edit-mailbox.blade.php b/resources/views/livewire/edit-mailbox.blade.php index 884f70e..9ce4bb0 100644 --- a/resources/views/livewire/edit-mailbox.blade.php +++ b/resources/views/livewire/edit-mailbox.blade.php @@ -10,7 +10,16 @@ :label="__('mail_settings.username')" :hint="__('mail_settings.username_hint')" /> + :label="__('mail_settings.password')" + :hint="$authenticates ? __('mail_settings.password_hint') : __('mail_settings.password_hint_unauthenticated')" /> + +
+ {{-- .live: the password hint above reads differently depending on + this, so toggling it has to reach the server and re-render + rather than sit deferred until the next unrelated action. --}} + +

{{ __('mail_settings.authenticates_hint') }}

+
@if (! $passwordConfirmed) {{-- The second gate, only reached if a new password is actually diff --git a/tests/Feature/Mail/MailSettingsPageTest.php b/tests/Feature/Mail/MailSettingsPageTest.php index 2eb6117..5de7ff3 100644 --- a/tests/Feature/Mail/MailSettingsPageTest.php +++ b/tests/Feature/Mail/MailSettingsPageTest.php @@ -605,6 +605,7 @@ it('pre-fills the form from the existing record', function () { 'username' => 'smtp-support', 'no_reply' => true, 'active' => false, + 'authenticates' => false, ]); Livewire::actingAs(User::factory()->operator('Owner')->create()) @@ -613,7 +614,85 @@ it('pre-fills the form from the existing record', function () { ->assertSet('displayName', 'Support-Team') ->assertSet('username', 'smtp-support') ->assertSet('noReply', true) - ->assertSet('active', false); + ->assertSet('active', false) + ->assertSet('authenticates', false); +}); + +// --- Codex R15#4, P1b: the modal's own checkbox for authenticates (R20 — no +// new modal, the existing one gets a field). + +it('renders an authenticates checkbox in the modal, wired to the property', function () { + // A property the component accepts is not the same as a field the + // operator can actually reach — R20's own point. + $box = Mailbox::factory()->create(['authenticates' => true]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->assertSeeHtml('wire:model.live="authenticates"') + ->assertSee(__('mail_settings.authenticates')); +}); + +it('saves authenticates together with the other fields', function () { + $box = Mailbox::factory()->create(['authenticates' => true]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('authenticates', false) + ->call('save'); + + expect($box->fresh()->authenticates)->toBeFalse(); +}); + +it('clears last_verified_at when authenticates changes, even though the password stays blank', function () { + // The fourth thing that invalidates a verification, alongside address + // and username (see the trio of tests above this block): a proof that + // credentials work means something different once whether to send them + // at all has flipped. + $box = Mailbox::factory()->create(['authenticates' => true, 'last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('authenticates', false) + ->call('save'); + + expect($box->fresh()->last_verified_at)->toBeNull(); +}); + +it('does not clear last_verified_at when authenticates is saved unchanged', function () { + // The control case: the guard must react to an actual flip, not fire on + // every save regardless — the same shape as the existing address/ + // username control test just above this block. + $box = Mailbox::factory()->create(['authenticates' => true, 'last_verified_at' => now()]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(EditMailbox::class, ['uuid' => $box->uuid]) + ->set('displayName', 'Neuer Anzeigename') + ->call('save'); + + expect($box->fresh()->last_verified_at)->not->toBeNull(); +}); + +it('does not report "no password stored" when testing a mailbox that does not authenticate', function () { + // The console-list-facing half of the finding: before this fix, the + // test-send button was the one place an unauthenticated mailbox actually + // read back as broken — MailboxTesterTest.php covers the service itself + // in depth (including that AUTH is never attempted); this proves the + // BUTTON reaches that fixed guard rather than a mock standing in for it. + // 127.0.0.1:1 refused in well under a millisecond (see "runs the real + // tester..." above) is what makes the failure real and fast rather than + // proving nothing by never actually attempting a connection. + Settings::set('mail.host', '127.0.0.1'); + Settings::set('mail.port', 1); + $box = Mailbox::factory()->create([ + 'key' => 'support', 'address' => 'no-reply@clupilot.com', + 'username' => null, 'password' => null, 'authenticates' => false, + ]); + + Livewire::actingAs(User::factory()->operator('Owner')->create()) + ->test(MailPage::class) + ->set('testRecipient', 'ziel@example.com') + ->call('test', $box->uuid) + ->assertSet('testResult.error', fn ($error) => str_contains((string) $error, 'Connection refused')); }); // --- Task 8: the test-send button. MailboxTesterTest.php covers diff --git a/tests/Feature/Mail/MailboxModelTest.php b/tests/Feature/Mail/MailboxModelTest.php index 3e16eb8..ec2cebf 100644 --- a/tests/Feature/Mail/MailboxModelTest.php +++ b/tests/Feature/Mail/MailboxModelTest.php @@ -48,6 +48,39 @@ it('falls back to the address when no separate username is given', function () { expect($box->smtpUsername())->toBe('support@clupilot.com'); }); +// --- Codex R15#4, P1b: isConfigured() must require a password only when the +// mailbox actually authenticates — a trusted relay that needs no login is +// not the same state as one nobody has finished setting up yet. + +it('is configured without a stored password when it does not authenticate', function () { + // ->fresh(), not the in-memory instance create() returns: authenticates + // needs its own boolean cast for this to mean anything (a raw DB read + // comes back "0"/"1", not a real PHP bool), the same gap + // "stores the password encrypted and never in the clear" above guards + // against for the password column. + $box = Mailbox::factory()->create(['authenticates' => false, 'password' => null]); + + expect($box->fresh()->isConfigured())->toBeTrue(); +}); + +it('is NOT configured without a stored password when it does authenticate', function () { + // The control case: the fix must not make isConfigured() stop requiring + // a password altogether — only when authenticates is explicitly false. + $box = Mailbox::factory()->create(['authenticates' => true, 'password' => null]); + + expect($box->fresh()->isConfigured())->toBeFalse(); +}); + +it('still requires active and a non-blank address regardless of authenticates', function () { + // authenticates=false must widen nothing except the password + // requirement — an inactive or addressless row stays unconfigured. + $inactive = Mailbox::factory()->create(['authenticates' => false, 'password' => null, 'active' => false]); + $blank = Mailbox::factory()->make(['authenticates' => false, 'password' => null, 'address' => '']); + + expect($inactive->fresh()->isConfigured())->toBeFalse() + ->and($blank->isConfigured())->toBeFalse(); +}); + it('finds a mailbox by its key', function () { Mailbox::factory()->create(['key' => 'billing']); diff --git a/tests/Feature/Mail/MailboxSeedMigrationTest.php b/tests/Feature/Mail/MailboxSeedMigrationTest.php index 80074f9..7e9ad03 100644 --- a/tests/Feature/Mail/MailboxSeedMigrationTest.php +++ b/tests/Feature/Mail/MailboxSeedMigrationTest.php @@ -311,6 +311,100 @@ it('still derives the placeholder domain from the SMTP login, not from mail.from ->and(Mailbox::findByKey('billing')->address)->toBe('billing@clupilot.com'); }); +// --- Codex R15#4, P1b: an existing SMTP relay with a host/from address but +// no username or password (a trusted local or private-network relay) must +// still be taken over for real — not seeded as a placeholder — with +// authenticates recording that it needs no password, distinct from a +// placeholder that simply has not had one typed in yet. + +it('takes over a relay with a real host but no username or password, marking it as not authenticating', function () { + clearMailboxSeed(); + config()->set('mail.mailers.smtp.host', 'relay.internal.example'); + config()->set('mail.mailers.smtp.username', ''); + config()->set('mail.mailers.smtp.password', ''); + config()->set('mail.from.address', 'no-reply@clupilot.local'); + + loadMailboxSeedMigration()->up(); + + $box = Mailbox::findByKey('no-reply'); + + // The real From address, not the 'no-reply@clupilot.com' placeholder — + // before this fix, $isConfigured required a username and this relay has + // none, so it would have been seeded as a placeholder and the new + // transport would refuse to send for it, exactly the outage the finding + // describes. + expect($box->address)->toBe('no-reply@clupilot.local') + ->and($box->authenticates)->toBeFalse() + ->and($box->isConfigured())->toBeTrue() + // Null, not '': an empty username column must read as "no separate + // login" through Mailbox::smtpUsername()'s own fallback, not as a + // literal empty-string login distinct from having none at all. + ->and($box->username)->toBeNull(); +}); + +it('marks authenticates true, and the mailbox as needing a password, when only a username was resolved', function () { + // The other half of the distinction the brief draws: a username with no + // password is an operator who started typing, not one who chose no + // auth. authenticates must stay true so isConfigured() keeps asking for + // a password rather than reading this as a deliberately open relay. + clearMailboxSeed(); + config()->set('mail.mailers.smtp.host', 'relay.internal.example'); + config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com'); + config()->set('mail.mailers.smtp.password', ''); + + loadMailboxSeedMigration()->up(); + + $box = Mailbox::findByKey('no-reply'); + + expect($box->authenticates)->toBeTrue() + ->and($box->getRawOriginal('password'))->toBeNull() + ->and($box->isConfigured())->toBeFalse(); +}); + +it('leaves the four placeholder mailboxes authenticating by default, even when the real account does not', function () { + // authenticates=false must land ONLY on the account the takeover + // actually resolved — support@/billing@/office@/info@ are fabricated + // placeholder addresses with no real credentials behind them at all, and + // must keep asking for a password (authenticates=true, the column's own + // default) so they read as "not yet set up", not as "deliberately open". + // Getting this backwards would let a made-up placeholder address pass + // isConfigured() and be handed to MailboxTransport for real. + clearMailboxSeed(); + config()->set('mail.mailers.smtp.host', 'relay.internal.example'); + config()->set('mail.mailers.smtp.username', ''); + config()->set('mail.mailers.smtp.password', ''); + config()->set('mail.from.address', 'no-reply@clupilot.local'); + + loadMailboxSeedMigration()->up(); + + foreach (['support', 'billing', 'office', 'info'] as $key) { + $box = Mailbox::findByKey($key); + + expect($box->authenticates)->toBeTrue("{$key} should still default to authenticates=true") + ->and($box->isConfigured())->toBeFalse("{$key} should still read as unconfigured"); + } +}); + +it('never seeds a blank address for the real account when neither a username nor a from address resolved', function () { + // A real host alone is not enough to know WHO mail should claim to be + // from — without this guard, a relay with a host but no username, no + // password, AND no MAIL_FROM_ADDRESS would resolve an empty address + // string. Falling back to the placeholder shape here (as if the account + // were not taken over at all) is safer than writing a blank address. + clearMailboxSeed(); + config()->set('mail.mailers.smtp.host', 'relay.internal.example'); + config()->set('mail.mailers.smtp.username', ''); + config()->set('mail.mailers.smtp.password', ''); + config()->set('mail.from.address', 'hello@example.com'); // config/mail.php's own unconfigured placeholder + + loadMailboxSeedMigration()->up(); + + $box = Mailbox::findByKey('no-reply'); + + expect($box->address)->toBe('no-reply@clupilot.com') + ->and($box->address)->not->toBe(''); +}); + it('carries the vault password across as ciphertext, never decrypting and re-encrypting it', function () { clearMailboxSeed(); config()->set('mail.mailers.smtp.username', 'no-reply@clupilot.com'); @@ -347,11 +441,20 @@ it('leaves the vault row in the database even after a normal, fully successful c expect(DB::table('app_secrets')->where('key', 'mail.password')->value('value'))->toBe($ciphertext); }); -it('does not destroy a stored vault password when MAIL_USERNAME is empty, because it never lands anywhere', function () { +it('does not destroy a stored vault password when nothing at all is configured, because it never lands anywhere', function () { // Reproduces the Critical finding: $isConfigured is false for every key - // when MAIL_USERNAME is empty, so the carry-over never runs — the old - // code deleted the vault row unconditionally regardless. + // when nothing was resolved, so the carry-over never runs — the old code + // deleted the vault row unconditionally regardless. + // + // Host pinned to the placeholder explicitly, not left at this + // container's real .env value: Codex R15#4, P1b made $isConfigured + // sensitive to the HOST too (a real host with no username is now a + // legitimate unauthenticated relay, seeded for real — see the block of + // tests above this one), so this scenario has to say "nothing + // configured" on every signal itself rather than only blanking the + // username and relying on the rest to already be blank by accident. clearMailboxSeed(); + config()->set('mail.mailers.smtp.host', '127.0.0.1'); config()->set('mail.mailers.smtp.username', ''); $ciphertext = app(SecretCipher::class)->encrypt('nowhere-to-go'); diff --git a/tests/Feature/Mail/MailboxTesterTest.php b/tests/Feature/Mail/MailboxTesterTest.php index 8dedf13..46f7f9a 100644 --- a/tests/Feature/Mail/MailboxTesterTest.php +++ b/tests/Feature/Mail/MailboxTesterTest.php @@ -61,7 +61,9 @@ it('passes the SMTP error through verbatim, because that is the whole diagnosis' ->and($result['error'])->toContain('Connection refused'); }); -it('refuses a mailbox without a password rather than pretending to try', function () { +it('refuses a mailbox without a password rather than pretending to try — when it authenticates', function () { + // The control case for the block below: authenticates defaults to true + // on the factory, so this is what the guard must still refuse. $box = Mailbox::factory()->create(['password' => null]); $result = app(MailboxTester::class)->run($box, 'ziel@example.com'); @@ -70,6 +72,78 @@ it('refuses a mailbox without a password rather than pretending to try', functio ->and($result['error'])->toContain('Passwort'); }); +// --- Codex R15#4, P1b: an unauthenticated relay (a trusted local or +// private-network server) must not be refused for lacking a password — +// Laravel's previous SMTP configuration has always supported sending through +// one with none configured at all. + +it('does not refuse an unauthenticated mailbox for lacking a password', function () { + $server = FakeSmtpServer::succeeding(); + + try { + 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(); + } +}); + +it('never attempts AUTH when the mailbox does not authenticate, even against a server that offers it', function () { + // succeeding() alone cannot prove this — it never advertises AUTH, so + // handleAuth() is unreachable regardless of what MailboxTester supplies. + // succeedingUnlessAuthAttempted() offers AUTH and rejects any attempt, + // so only a client that never sends "AUTH ..." at all can succeed here. + $server = FakeSmtpServer::succeedingUnlessAuthAttempted(); + + try { + 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(); + } +}); + +it('still attempts AUTH when the mailbox does authenticate, against the same AUTH-offering server', function () { + // The control case for the test above: proves succeedingUnlessAuthAttempted() + // actually discriminates rather than always succeeding regardless of + // what the client does — an authenticating mailbox reaches handleAuth(), + // which sends AUTH LOGIN, which this server always rejects. + $server = FakeSmtpServer::succeedingUnlessAuthAttempted(); + + try { + Settings::set('mail.host', '127.0.0.1'); + Settings::set('mail.port', $server->port); + + $box = Mailbox::factory()->create([ + 'address' => 'no-reply@clupilot.com', 'password' => 'a-real-password', 'authenticates' => true, + ]); + + $result = app(MailboxTester::class)->run($box, 'ziel@example.com'); + + expect($result['ok'])->toBeFalse(); + } finally { + $server->stop(); + } +}); + it('stamps last_verified_at only on success', function () { $box = Mailbox::factory()->create(['address' => 'no-reply@clupilot.com']); diff --git a/tests/Feature/Mail/MailboxTransportTest.php b/tests/Feature/Mail/MailboxTransportTest.php index d74af6f..a3c5913 100644 --- a/tests/Feature/Mail/MailboxTransportTest.php +++ b/tests/Feature/Mail/MailboxTransportTest.php @@ -178,6 +178,74 @@ it('reuses the same delegate until the mailbox credentials actually change', fun expect($third)->not->toBe($first); // the operator's correction takes effect immediately }); +// --- Codex R15#4, P1b: an unauthenticated relay must not have credentials +// forced onto it. smtpUsername() always returns something non-empty (it +// falls back to the address), so a delegate built without checking +// authenticates first would still call setUsername() with a real value — +// which alone is enough to make Symfony attempt AUTH against any server +// that advertises it, something a trusted no-auth relay was never asked to +// answer. + +it('never calls setUsername or setPassword on the delegate when the mailbox does not authenticate', function () { + Mailbox::factory()->create([ + 'key' => 'support', 'address' => 'support@clupilot.com', + 'username' => null, 'password' => null, 'authenticates' => false, + ]); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $delegate = mailboxDelegate($transport); + + expect($delegate)->toBeInstanceOf(EsmtpTransport::class) + ->and($delegate->getUsername())->toBe('') + ->and($delegate->getPassword())->toBe(''); +}); + +it('still sets a real username and password on the delegate when the mailbox does authenticate', function () { + // The control case for the test above: the fix must not stop + // authenticated mailboxes from authenticating. + Mailbox::factory()->create([ + 'key' => 'support', 'address' => 'support@clupilot.com', + 'username' => null, 'password' => 'a-real-password', 'authenticates' => true, + ]); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $delegate = mailboxDelegate($transport); + + expect($delegate->getUsername())->toBe('support@clupilot.com') // smtpUsername()'s address fallback + ->and($delegate->getPassword())->toBe('a-real-password'); +}); + +it('rebuilds the delegate when authenticates toggles, even though host, port, encryption and password all stay the same', function () { + // reuses the same delegate...' above proves the fingerprint reacts to a + // PASSWORD change; this proves it also reacts to authenticates itself — + // without that, an operator unchecking "requires a password" on a + // long-running queue worker would keep sending through the OLD delegate, + // which already has setUsername()/setPassword() baked in from before the + // toggle, still attempting AUTH the operator just turned off. + $box = Mailbox::factory()->create([ + 'key' => 'support', 'address' => 'support@clupilot.com', + 'username' => null, 'password' => 'unchanged-password', 'authenticates' => true, + ]); + Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support'); + config()->set('mail.default', 'smtp'); + + $transport = Mail::mailer('cp_support')->getSymfonyTransport(); + $first = mailboxDelegate($transport); + + expect($first->getUsername())->not->toBe(''); + + $box->update(['authenticates' => false]); + + $second = mailboxDelegate($transport); + + expect($second)->not->toBe($first) + ->and($second->getUsername())->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'); diff --git a/tests/Support/FakeSmtpServer.php b/tests/Support/FakeSmtpServer.php index 4c86ddc..96e2dbc 100644 --- a/tests/Support/FakeSmtpServer.php +++ b/tests/Support/FakeSmtpServer.php @@ -42,6 +42,25 @@ final class FakeSmtpServer return self::spawn(['failAt' => null]); } + /** + * Accepts one connection, advertises AUTH LOGIN in its EHLO response, + * and completes a full successful send — UNLESS the client attempts to + * authenticate, which is rejected outright. + * + * The point is proving a client deliberately did NOT attempt AUTH, not + * merely that it could succeed without it: succeeding() never advertises + * AUTH at all, so Symfony's EsmtpTransport::handleAuth() is never even + * reachable against it — a mailbox that wrongly called setUsername() + * with a real value would pass every test that uses succeeding() too. + * Advertising the capability here is what makes the two cases tell + * apart: "chose not to authenticate" against a server that offered it, + * versus "had no opportunity to" against one that never did. + */ + public static function succeedingUnlessAuthAttempted(): self + { + return self::spawn(['failAt' => null, 'advertiseAuth' => true]); + } + /** * Accepts one connection and rejects it with a real SMTP response at the * given stage. 'rcpt' is the simplest choice for a caller that just wants diff --git a/tests/Support/fake_smtp_server_process.php b/tests/Support/fake_smtp_server_process.php index b98e2e9..1520a81 100644 --- a/tests/Support/fake_smtp_server_process.php +++ b/tests/Support/fake_smtp_server_process.php @@ -5,23 +5,29 @@ * why a subprocess and not an in-process fake is necessary here). * * Configuration arrives as one JSON object on STDIN: {"failAt": string|null, - * "failResponse": string}. failAt names the command after which the server - * answers with failResponse instead of success — 'mail', 'rcpt', or 'data'. - * null completes a normal, successful send. ('ehlo' is deliberately not a - * supported stage: EsmtpTransport retries a failed EHLO as plain HELO before - * giving up, so a clean single-rejection test needs a later stage.) + * "failResponse": string, "advertiseAuth": bool}. failAt names the command + * after which the server answers with failResponse instead of success — + * 'mail', 'rcpt', or 'data'. null completes a normal, successful send. + * ('ehlo' is deliberately not a supported stage: EsmtpTransport retries a + * failed EHLO as plain HELO before giving up, so a clean single-rejection + * test needs a later stage.) * * Binds an ephemeral port on 127.0.0.1, prints it as the first line of * STDOUT so the parent process can connect, accepts exactly one connection, - * and plays the minimal EHLO/MAIL FROM/RCPT TO/DATA dialogue. Neither - * STARTTLS nor AUTH is advertised in the EHLO response, so - * EsmtpTransport — auto_tls defaults true, but only upgrades when the - * server actually offers STARTTLS — never attempts either, and the whole - * exchange stays in plain text. + * and plays the minimal EHLO/MAIL FROM/RCPT TO/DATA dialogue. STARTTLS is + * never advertised in the EHLO response, so EsmtpTransport — auto_tls + * defaults true, but only upgrades when the server actually offers + * STARTTLS — never attempts it, and the whole exchange stays in plain text. + * + * AUTH is advertised only when advertiseAuth is true (see + * FakeSmtpServer::succeedingUnlessAuthAttempted()) — the base configuration + * never offers it, so EsmtpTransport::handleAuth() is never even reachable + * against it regardless of what credentials a client might have set. */ $config = json_decode((string) stream_get_contents(STDIN), true) ?: []; $failAt = $config['failAt'] ?? null; $failResponse = $config['failResponse'] ?? '550 Rejected'; +$advertiseAuth = $config['advertiseAuth'] ?? false; $server = stream_socket_server('tcp://127.0.0.1:0', $errno, $errstr); @@ -59,9 +65,25 @@ function smtpRespond($conn, string $line): void smtpRespond($conn, '220 fake.smtp ESMTP'); smtpReadLine($conn); // EHLO -smtpRespond($conn, '250 fake.smtp'); +if ($advertiseAuth) { + // Multi-line EHLO: smtpRespond() only ever sends the final line, so the + // continuation is written directly. + fwrite($conn, "250-fake.smtp\r\n"); + smtpRespond($conn, '250 AUTH LOGIN'); +} else { + smtpRespond($conn, '250 fake.smtp'); +} -smtpReadLine($conn); // MAIL FROM +// AUTH, if the client attempts it, arrives here in place of MAIL FROM — +// immediately after EHLO and before the send dialogue proper, exactly where +// EsmtpTransport::handleAuth() sends it. +$line = smtpReadLine($conn); +if ($advertiseAuth && str_starts_with(strtoupper(ltrim($line)), 'AUTH')) { + smtpRespond($conn, '535 5.7.8 Authentication attempted but this relay does not require it'); + exit(0); +} + +// $line is MAIL FROM's own line when AUTH was not attempted. if ($failAt === 'mail') { smtpRespond($conn, $failResponse); exit(0);