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).
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.
saveServer() wrote host/port/encryption without touching any mailbox, so
the console kept showing every mailbox as verified against a server
nothing had tested since. Mirrors EditMailbox::save()'s existing guard on
a single mailbox's own identity, just at server scope: only clears when a
field actually changed, so re-saving unedited values leaves a real
verification alone.
Both places now delegate the actual clear to new Mailbox::
invalidateVerification()/invalidateAllVerifications() methods rather than
touching last_verified_at directly, sharing the mutation. The "did this
change" comparison stays separate in each caller - one diffs a loaded
model's attributes, the other diffs persisted settings against incoming
scalars, and forcing them through one function would add machinery no
actual duplication justifies.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MailboxTester required a usable SECRETS_KEY unconditionally, refusing an
unauthenticated relay's test-send on a fresh install with no key yet, even
though real sending never touches the cipher for that mailbox. Gated the
check on authenticates, same as the password-presence check beside it.
Comparison pass across MailboxTester and MailboxTransport turned up two more
of the same shape. MailboxTransport's delegate-cache fingerprint decrypted
$box->password unconditionally, even when unauthenticated and never going to
use it — a mailbox that once authenticated, stored a password, and later had
authenticates unchecked would crash real sending the moment SECRETS_KEY
became unusable. And MailboxTester had no guard against a stored port <1;
Symfony silently reinterprets that as port 25 rather than refusing it, so the
test button could reach whatever happens to listen there and report success
for a configuration MailboxTransport refuses outright.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An SMTP relay with a host and a from address but no username or password
- a trusted local or private-network relay - always used to be seeded as
a placeholder, because the takeover required a username to treat an
account as real, and Mailbox::isConfigured() always required a stored
password. Laravel's own SMTP mailer has supported unauthenticated relays
all along.
Add mailboxes.authenticates (default true, so every existing row keeps
its current behaviour) to tell "does not authenticate" apart from
"authenticates, but nobody has typed the password yet" - the same empty
password column used to mean both. The takeover migration now seeds the
real account off the host/address alone and marks it authenticates=false
only when neither a username nor a password resolved; isConfigured(),
MailboxTransport and MailboxTester all read it before requiring or
sending a password; the edit modal gets a checkbox for it with the
password field's hint updated to match, in both languages.
MAIL_SCHEME=smtps or an smtps:// MAIL_URL on a non-465 port used to be
downgraded to STARTTLS because mail.encryption was derived from the port
alone. Resolve the scheme first (from MAIL_SCHEME directly, or from the
URL's own scheme via the same ConfigurationUrlParser Laravel uses) and
only fall back to the port heuristic when nothing was actually stated -
which is also what keeps this machine's invalid MAIL_SCHEME=tls from
silently steering the result.
isUsable() checked only "is SECRETS_KEY nonempty", so a set-but-malformed
key (wrong length, garbage base64) read back as usable even though
encrypter() rejects it two lines below. EditMailbox::save() and
MailboxTester::run() both gate on isUsable() specifically to avoid an
uncaught RuntimeException reaching the operator; a lying isUsable() meant
that guard did not fire in exactly the configuration it exists for.
Both methods now read resolveKey() — the base64: prefix, the raw-base64
path, the 32-byte check — so they cannot disagree about a value either
one is given. SecretVault::isUsable() and the secrets console page's
"no key" banner both delegate down to this and are covered here too, not
assumed to inherit the fix correctly.
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.
The takeover migration seeded `address` from MAIL_USERNAME and left
`username` null unconditionally, conflating the SMTP login with the
sender address. Whenever the two differ — an API-key username, a
shared relay login — mail would go out From the login instead of the
address the system was actually configured to send from, exactly like
this installation: MAIL_USERNAME is no-reply@clupilot.com but
MAIL_FROM_ADDRESS is hello@clupilot.local.
`address` now reads config('mail.from.address'), falling back to the
SMTP login only when that reads back config/mail.php's own unset
placeholder (env('MAIL_FROM_ADDRESS', 'hello@example.com') — the same
masking problem UNCONFIGURED_HOST already guards against for
MAIL_HOST). `username` is set to the SMTP login only when it actually
differs from the resolved address, since Mailbox::smtpUsername()
already falls back to address when username is null. display_name
now reads config('mail.from.name') instead of a hardcoded literal.
The placeholder domain for the four non-configured mailboxes still
comes from the SMTP login, not from mail.from.address — the two can
name different domains, and only the login's is proven to accept mail
for a given install.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
savePurposes() only checked that each key was a nonempty string, so a
stale or tampered mailbox key (reachable by posting straight to
/livewire/update) saved successfully while MailboxResolver silently
returned null for it. Every purpose's key must now name a mailbox that
exists; system's must additionally be active, since every unmapped or
inactive OTHER purpose falls through to it.
EditMailbox::save() also cleared last_verified_at only when the
password changed, but smtpUsername() depends on address and username
too (falling back to address when username is blank) — changing
either while leaving the password untouched kept showing a successful
verification for an identity nothing had actually tested.
EsmtpTransport's autoTls (default true) and requireTls (default false)
are independent of the implicit-TLS constructor argument: 'tls' left
these at their defaults, so a server that omitted STARTTLS sent
mailbox credentials in the clear while reporting success, and 'none'
would silently upgrade the moment a server offered STARTTLS. Both
MailboxTransport and MailboxTester already duplicated the "ssl, or
port 465 ⇒ implicit" half of this decision independently; MailTlsPolicy
is now the one place both read, for both dimensions.
MailboxTester still builds through Mail::build() rather than a hand-built
transport: MailManager::createSmtpTransport() forwards the whole config
array to EsmtpTransportFactory as DSN options, which already reads
auto_tls/require_tls itself, so the same policy reaches this path
through keys the config array already supports.
I3: nothing stopped an operator unchecking Aktiv on the mailbox that
mail.purpose.system points at. MailboxResolver::active() filters an
inactive mailbox at BOTH the direct lookup and the system fallback, so
that one checkbox could take mail.purpose.system to null and every
purpose without its own mapping down with it — the existing
system_required guard on savePurposes() reads as if this were covered,
but it only blocks the empty-mapping path, not this one.
EditMailbox::save() now refuses active=false against the mailbox 'system'
currently points at, the same way savePurposes() already refuses to leave
'system' empty.
I4: Admin\Mail::saveServer() writes mail.host — the platform's outbound
relay for every purpose mailbox at once — and CloudReady::toMail() puts a
customer's Nextcloud admin password in cleartext in the message body. So a
signed-in mail.manage session was already a credential-interception
primitive, the same threat Admin\Secrets' second gate exists for. Both
saveServer() and EditMailbox::save() (only when a new password is actually
being typed) now also require passwordRecentlyConfirmed(), following the
ConfirmsPassword pattern Secrets.php and Settings.php's two-factor actions
already use — including their per-action UI shape (an inline confirm form
scoped to the gated action), not a whole-page lock, so address, display
name, username, no-reply, active and the purpose mapping stay reachable
without confirming, per the brief's capability split.
Both gated and ungated paths are covered by tests, and both guards were
mutated away and confirmed to fail before being restored.
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.
MailboxSeedMigrationTest.php had the actual .env MAIL_PASSWORD for
no-reply@clupilot.com hard-coded into a test fixture — replaced with an
obvious dummy. Grepped the whole branch (576cfca..HEAD) and the working
tree for that string and for every other real secret value in .env;
nothing else leaked.
Separately, the seed migration's SECRETS_KEY-missing warning claimed a
password "was NOT copied into the mailbox table" even when a vault row
already existed and its ciphertext WAS carried across a few lines below,
unconditionally on $canEncrypt. An operator debugging a live mail outage
would have been sent to re-enter a password that was already sitting in
the column. The warning now distinguishes the two cases and says which
one actually happened.
It only ever loaded the page and asserted the banner, never a password
save — but its name claimed the crash-on-save case, which is exactly the
promise that let that bug ship unnoticed once. The test that earns the
crash claim is its new neighbour, added for the same finding.
EditMailbox::save() now checks SecretCipher::isUsable() before touching a
typed password and reports it as a form error, instead of letting
SecretCipher::encrypt() throw all the way to Laravel's debug page. Also
switches the mail.manage migration to Role::findOrCreate, matching every
sibling capability migration, so a squashed replay can't hit
RoleDoesNotExist.
The vault's mail.password row is no longer deleted by this migration at
all, in either direction: deleting it unconditionally destroyed a real
password whenever MAIL_USERNAME was empty (nowhere to carry it to), and
even on a successful carry-over a later rollback had nothing left to fall
back to. Reproduced both against real MariaDB, along with a migrations-
table drift that crashed a retry on the mailboxes.key unique constraint —
fixed with Mailbox::firstOrNew() and a transaction around each direction.
down() now goes through the new Settings::forget() instead of a raw
delete, so it stops leaving the settings cache holding what it just
removed. A skipped .env password (SECRETS_KEY still unset) now prints an
operator-visible line instead of migrating clean and failing silently
later. Host/port that resolve to config/mail.php's own placeholder
defaults are treated as unset rather than as a real relay.
The seed migration also guards against SECRETS_KEY being unset at migrate
time (encrypting the .env password would otherwise throw and take the
whole migration down, on a fresh install as much as in the test suite),
and the four tests/Feature/Mail files written before this migration reset
the mailbox table in their beforeEach so they keep testing what they did
before mailboxes.key collided with the seeded rows.
A mutation the reviewer ran (if ($box !== null) -> if (true)) left every
test green: CloudReady hand-rolled the same null-mailbox guard as
SendsFromMailbox, but nothing exercised its null branch. Added the
missing test, then extracted the decision itself (null mailbox -> no
sender fields, no_reply -> no Reply-To) into one shared method so an
Envelope-shaped and a MailMessage-shaped consumer can no longer disagree.
Re-review of the previous fix round found the "cannot disagree" part of
finding 4 was still open. resolution() had unified the log/array/null/
mailbox decision, but inside the mailbox branch describe() returned
$box?->address unconditionally while delegate() additionally required
isConfigured() before using it. A mailbox with active=true, a real
address, and no password produced
'mailbox://support/support@clupilot.com' — reading as entirely healthy
— while send() threw. That is exactly the harm the original finding
named: the log says fine, the mail fails.
resolution() now decides "unconfigured" too, not just "which
non-delivering mode": it calls isConfigured() itself, once, and returns
that verdict alongside the box. describe() and delegate() both branch
on the SAME result again, the same way they already did for log/array/
null — a mailbox with no password now reads as
'support@clupilot.com [unconfigured]' rather than a plain address, and
the address stays visible so an operator can tell WHICH mailbox needs
fixing.
Also renamed a test the re-review flagged as promising more than it
checked: "registers a mailer per purpose without touching the database
at boot" only ever verified the config array's shape — the DB-touching
claim is a separate, already-passing test just below it
(defines every purpose mailer as a plain literal...). Renamed to
"registers a mailer for every purpose". No behaviour change.
Added a test creating a mailbox with no password and asserting the DSN
both names the address AND carries the marker, plus that send() still
throws — proving agreement, not just a matching label painted on
separately. Mutated describe()'s unconfigured branch to drop the
marker: the test failed exactly on the missing 'unconfigured' string
(DSN read as a plain, healthy-looking address again). Reverted, reran,
passes.
Full suite: 667 passed. vendor/bin/pint --test clean on both files.
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>
Review of Task 4: MailboxResolver::for() only fell back on a missing
mapping or a missing row. Deactivating a mailbox — the operator's
explicit "do not use this" — left it resolvable anyway, so
MailboxTransport reached it, found isConfigured() false, and threw
instead of quietly using the next candidate the way every other
"this mailbox is unusable" case does. Worse, __toString() still
reported the deactivated address as though it were healthy.
named() now runs through a new active() filter at BOTH lookup sites —
the purpose's own mapping and the system fallback — so an inactive
system mailbox is refused exactly like a missing one, not handed out
as the last resort. A missing password stays deliberately unfiltered
here: that is a MailboxTransport concern (loud failure naming the
mailbox), not a resolver fallback trigger — silently rerouting mail
because a password hasn't been typed yet would hide the gap instead
of surfacing it.
Added two tests and mutated active() to a no-op filter to confirm they
catch it: both failed (one expected the system mailbox, got null; the
other expected null, got the inactive mailbox's full attribute dump).
Reverted, reran, both pass again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 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.
Mailbox hand-rolled the same creating-time UUID assignment that
App\Models\Concerns\HasUuid already provides to 17 other models under
R11 (URLs address records by UUID, not integer PK), and skipped the
route-key binding that comes with it. Swap onto the trait, and cover
both the assignment and the route key with a test matching the
convention already used for Host/Order.
Also match MailboxFactory's @extends annotation to how the other
factories in the repo write it (Pint's fully_qualified_strict_types
rule turns a fully-qualified docblock reference into an import plus a
short name anyway, so this is what it would end up as either way).
nexxo