Postfächer statt eines fest verdrahteten SMTP-Zugangs #1
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "feat/mailboxes"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
Ersetzt den einen fest verdrahteten SMTP-Zugang durch Postfächer als Datensätze.
Was drin ist
mailboxes-Tabelle — Adresse, Anzeigename, SMTP-Benutzer, Passwort (verschlüsselt mitSECRETS_KEY, nichtAPP_KEY),authenticates,active,last_verified_at.maintenance,provisioning,support,billing,system) mit Zuordnung in der Konsole.systemist der Rückfall und darf nicht leer bleiben.MailboxTransport— löst Zugangsdaten beim Senden auf, nie beim Booten. Die Mails sind queued; ein Worker lebt Stunden und hielte sonst dauerhaft, was bei seinem Start galt.FromundReply-Toauf jeder Mail. Das ist der Grund, warum reines Senden genügt: Die Antwort des Kunden landet im echten Postfach. Einno_reply-Postfach bekommt keinReply-To.MAIL_URL, unauthentifizierter Relays und impliziter TLS auf abweichenden Ports.mail.manage, Bearbeiten im Modal (R20), und einem Testversand, der amMAIL_MAILER=logvorbeigeht — sonst meldete er Erfolg, während er in eine Logdatei schreibt.Prüfung
815 Tests. Gesamtreview freigegeben. R15 (Codex) über zehn Runden bis sauber — dabei sechs P1, davon zwei sicherheitsrelevant:
tlswar wederautoTlsnochrequireTlsgesetzt, ein Server ohne STARTTLS hätte die Zugangsdaten im Klartext bekommen und der Versand hätte Erfolg gemeldet.MAIL_SCHEME=smtpals „kein TLS" gedeutet und damit bisher verschlüsselte Verbindungen zu Klartext gemacht.Der Gesamtreview fand außerdem, dass
Mail::to()->queue()den Mailer des Mailables überschreibt — zwei von drei Absendern hätten das neue System gar nicht benutzt.Vor dem Live-Gang, Reihenfolge nicht vertauschen
SECRETS_KEYerzeugen (head -c 32 /dev/urandom | base64) — ist derzeit nicht gesetzt, damit speichert auch die bestehende Zugangsdaten-Seite nichts.MAIL_SCHEMEkorrigieren (tlsist kein gültiges Symfony-Schema) undMAIL_MAILER=smtp.Andersherum bricht die Bereitstellung eines zahlenden Kunden ab, weil
CloudReadybewusst synchron verschickt wird.The Task 2 reviewer rewired the model to Laravel's APP_KEY-based Crypt facade and every test stayed green. Laravel's encrypter is a container singleton resolved once from app.key, so config()->set('app.key') never rebuilds it and the rotation the test performed was invisible. Replaced with a positive proof — rotating SECRETS_KEY must break decryption — plus the APP_KEY case with forgetInstance(), which is what makes that direction mean anything.The old test rotated app.key and expected the password to stay readable, but Laravel's encrypter is a container singleton resolved once — config()->set('app.key', ...) never rebuilds it, so the rotation the test performed was invisible to any code path going through it. Confirmed by mutation: rewiring Mailbox to Crypt:: encryptString/decryptString (genuinely APP_KEY-keyed, the exact regression this constraint exists to forbid) left the old test green. Replace it with two tests: a positive proof that rotating SECRETS_KEY makes a stored password unreadable (the one that discriminates), and a separate check that rotating APP_KEY does not, with forgetInstance('encrypter') so that rotation is actually visible to whatever the password accessor resolves. Re-ran the same mutation against the new test: it fails (exit 1, the SECRETS_KEY test reports the expected DecryptException was not thrown), then passes again once the mutation is reverted.Five purpose-named mailers (cp_maintenance, cp_provisioning, cp_support, cp_billing, cp_system) land in config/mail.php as plain static entries — no query runs to build them. Mail::extend('mailbox', ...) in AppServiceProvider registers a transport that looks up its mailbox through MailboxResolver only once a mailer is actually resolved, which for queued mail is inside the worker at send time, never at boot. Mutation-tested the brief's own three tests by deleting the MAIL_MAILER guard from MailboxTransport::delegate(): all three stayed green. They only exercise __toString()/describe(), a code path separate from the one that actually sends. Added two tests that reach into delegate() itself (a bound closure, since it's private) and assert the transport it actually builds: LogTransport while MAIL_MAILER=log, EsmtpTransport once it isn't. Re-ran the same mutation against the strengthened suite: it now fails (EsmtpTransport where LogTransport was expected), then passes again once reverted. Verified two things against the installed versions before writing this: LogTransport takes a Psr\Log\LoggerInterface and Log::channel(null) resolves to the default channel rather than throwing, and Mail::extend('mailbox', ...) is read by MailManager::createSymfonyTransport() via $config['transport'] before it would fall back to createSmtpTransport() — both matched the brief exactly, no changes needed there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The reviewer continued the mutation sweep from Task 4's own commit and found three more unpinned lines in delegate(), plus two real behaviour gaps, all inside the same 40-line method: - The fingerprint refresh (the headline feature: a worker's mailer must pick up a corrected password without a restart) had no test — mutating its check to "delegate === null" left the suite green. - The implicit-TLS third argument to EsmtpTransport had no test either — mutating it to a hardcoded true left the suite green, because the only assertion touching it was instanceof EsmtpTransport, which the hanging variant also satisfies. - isLogging() only ever checked for 'log', but phpunit.xml sets MAIL_MAILER=array for the whole suite. Task 5 wires real mailables to these mailers next; the first feature test that sends one without Mail::fake() would have opened a real SMTP connection. - describe() and delegate() each ran their own copy of that guard — which is exactly how deleting it from ONLY delegate() passed the whole suite in the first place: __toString() kept reporting "log" from its own untouched copy. - An unconfigured host/port (Settings::get('mail.host', '') and a stored null port casting to 0) built a transport pointed at smtp://:587 or silently downgraded to plaintext port 25, instead of refusing the way a missing mailbox already does. Fixes: replaced isLogging() with resolution(), the one place that now decides "log, array, null-default, or a real mailbox" — describe() and delegate() both branch on ITS result, so a guard broken in one cannot look fine in the other. Added an explicit RuntimeException for a blank host or non-positive port, guarded the same way as the missing-mailbox case just above it. Tests: added coverage for the fingerprint rebuild (reuses the same delegate until the password actually changes, proven by object identity), the exact DSN string for both the STARTTLS (587) and implicit-TLS (465) cases, 'array' and unset-default both landing on a non-delivering transport, and the blank host/bad port throwing. Mutated each of these four in turn on the final code and confirmed: fingerprint check removed -> the reuse test fails (same object handed back after the password changed); TLS argument hardcoded true -> the 587 DSN test fails ('smtps://...' where 'smtp://...:587' was expected); NON_DELIVERING narrowed back to ['log'] -> both the array and unset-default tests fail (a real EsmtpTransport where a safe transport was expected); host/port guards removed -> both throw expectations fail. Reverted each, reran, all twelve tests pass again. Replaced the two tests that reached into delegate() via Closure::bind to check "log or SMTP" with one behavioural test: point Settings at a closed loopback port (127.0.0.1:1, so refusal is instant and entirely local — no DNS, no dependency on this environment's network egress policy) and assert Log::shouldReceive('debug')->once() while sending for real through Mail::mailer('cp_support')->raw(...). Mutated NON_DELIVERING to drop 'log' specifically and confirmed this test alone catches it: a real connection attempt to the closed port throws TransportException (Connection refused) in under 300ms. Reverted, reran, passes again. Also: the two tests that only checked the config array's shape and a snapshot of the resolved address promised more than they verified — CLAUDE.md R19 names a test that recomputes the implementation as checking nothing. Added a source-level test that the five cp_* config entries are parenthesis-free literals (same technique DisplayTimezoneTest/IconLayoutTest use: a property of the file, not of one run) as the actual "no query at boot" proof, and renamed the send-time test to what it verifies now that the fingerprint test above covers the live-refresh claim properly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Mail::to($x)->queue($mail) resolves the DEFAULT mailer, and Illuminate\Mail\Mailer::queue() (vendor Mailer.php:482) does `$view->mailer($this->name)->queue($this->queue)` — overwriting MaintenanceAnnouncementMail/MaintenanceCancelledMail's own 'cp_maintenance' with the default mailer's name ('array' in tests, 'log'/whatever MAIL_MAILER is in prod) before the job is even built. Both mails kept sending through the old .env account instead of MailboxTransport. CloudReady was never affected — MailChannel.php:66 preserves a notification's own mailer. Fix: name the mailer before queueing — Mail::mailer($mail->mailer)->to(...). $this->name on the resolved Mailer is then already $mail->mailer, so the vendor overwrite becomes a no-op instead of a silent downgrade. Confirmed by reading the vendor source, not assumed. SenderAddressTest's "names the purpose mailer..." test asserted $mail->mailer on a bare, never-queued mailable, which proves nothing about what survives queueing — the constructor sets it right regardless of what MaintenanceNotifier does with it. Replaced it with one that drives MaintenanceNotifier::deliver() under Queue::fake() and inspects the pushed SendQueuedMailable's mailable. Verified it fails against the pre-fix code first (the pushed job carried mailer = 'array'), then verified the fix makes it pass, then mutated the fix away and watched it fail again.The seed migration read config('mail.mailers.smtp.host') and its three siblings directly. An install configuring SMTP through MAIL_URL (the other form config/mail.php supports) never sets those individual keys — MailManager only substitutes the URL's components in at transport-build time, inside its own protected getConfig(). Read raw, that left the migration seeding config/mail.php's own placeholder defaults, which the UNCONFIGURED_HOST/UNCONFIGURED_PORT guards then read as genuinely unset: blank host, port 0, an unconfigured mailbox, for an install that was sending mail successfully. resolveSmtpConfig() calls the same Illuminate\Support\ConfigurationUrlParser MailManager::getConfig() itself delegates to, so this is the same resolution rather than a second copy of its merge order. Verified against the real (protected) getConfig() via reflection: the URL wins for host/port/username/password wherever it actually supplies one, and an unset MAIL_URL leaves this a no-op.env('MAIL_HOST')/env('MAIL_PORT') returning anything, or MAIL_URL supplying either, now counts as "supplied" and wins over the sentinel comparison — a relay genuinely on 127.0.0.1:2525 no longer reads as unconfigured. The sentinel stays as a fallback for when neither signal says anything (including under config:cache, where env() goes null and this collapses to today's behaviour).Gegenstandslos: der Inhalt ist über PR #2 nach
maingelangt (Fast-Forward, main steht auf9fc74ee).Pull request closed