boban
/
CluPilotCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make the DSN agree with send() when a mailbox has no password 

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>
feat/mailboxes
nexxo 2026-07-27 23:17:42 +02:00
parent a39670f920
commit 961bf62032
2 changed files with 59 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

57
app/Mail/Transport/MailboxTransport.php
View File

@ -62,16 +62,27 @@ class MailboxTransport implements TransportInterface
{
[$mode, $box] = $this->resolution();
return $mode === 'mailbox' ? $box?->address : $mode;
return match ($mode) {
'mailbox' => $box?->address,
// Same verdict delegate() throws on, not a second opinion: a DSN
// that still read as a plain address here is the exact harm this
// exists to prevent — it looked healthy right up until send()
// threw. The address stays visible so an operator can tell WHICH
// mailbox needs a password, rather than just that something does.
'unconfigured' => $box === null ? null : $box->address.' [unconfigured]',
default => $mode,
};
}
/**
* The ONE place that decides "log, array, null, or a real mailbox" —
* describe() and delegate() both branch on this result rather than each
* running their own copy of the check. Two copies of the same guard is
* what let a mutation that broke only ONE of them hide behind a passing
* suite: __toString() kept reporting "log" while delegate() quietly
* built a real SMTP transport regardless of MAIL_MAILER.
* The ONE place that decides "log, array, null-default, an unconfigured
* mailbox, or a real one" — describe() and delegate() both branch on this
* result rather than each running their own copy of a check. Two copies
* of the same guard is what let a mutation that broke only ONE of them
* hide behind a passing suite: __toString() kept reporting "log" (and
* later, a plain address) while delegate() quietly did something else —
* built a real SMTP transport regardless of MAIL_MAILER, then threw on a
* mailbox describe() had just shown as fine.
*
* @return array{0: string, 1: ?Mailbox}
*/
@ -83,13 +94,29 @@ class MailboxTransport implements TransportInterface
return [$default ?? 'null', null];
}
return ['mailbox', app(MailboxResolver::class)->for($this->purpose)];
$box = app(MailboxResolver::class)->for($this->purpose);
return $box !== null && $box->isConfigured()
? ['mailbox', $box]
: ['unconfigured', $box];
}
private function delegate(): TransportInterface
{
[$mode, $box] = $this->resolution();
if ($mode === 'unconfigured') {
// Loud, not silent: a mail with no mailbox behind it must not look
// like it was sent. A missing or deactivated MAPPING already fell
// back to system inside MailboxResolver; reaching this line means
// the mailbox we ended up with — possibly system itself — has no
// usable credentials (no row at all, or one with no password),
// and switching to yet another address would only hide that.
throw new RuntimeException(
"No configured mailbox for mail purpose [{$this->purpose}]."
);
}
if ($mode !== 'mailbox') {
return match ($mode) {
'log' => new LogTransport(Log::channel(config('mail.mailers.log.channel'))),
@ -98,18 +125,8 @@ class MailboxTransport implements TransportInterface
};
}
if ($box === null || ! $box->isConfigured()) {
// Loud, not silent: a mail with no mailbox behind it must not look
// like it was sent. A missing or deactivated MAPPING already fell
// back to system inside MailboxResolver; reaching this line means
// the mailbox we ended up with — possibly system itself — has no
// usable credentials, and switching to yet another address would
// only hide that.
throw new RuntimeException(
"No configured mailbox for mail purpose [{$this->purpose}]."
);
}
// $mode === 'mailbox' only when resolution() already confirmed $box
// is non-null and isConfigured() — nothing left to check here.
$host = (string) Settings::get('mail.host', '');
$port = (int) Settings::get('mail.port', 587);
$encryption = (string) Settings::get('mail.encryption', 'tls');

23
tests/Feature/Mail/MailboxTransportTest.php
View File

@ -32,7 +32,7 @@ function mailboxDelegate(TransportInterface $transport): TransportInterface
return Closure::bind(fn () => $this->delegate(), $transport, MailboxTransport::class)();
}
it('registers a mailer per purpose without touching the database at boot', function () {
it('registers a mailer for every purpose', function () {
// The five entries exist as plain config — no query has run to build them.
foreach (MailPurpose::ALL as $purpose) {
expect(config('mail.mailers.cp_'.$purpose))
@ -68,6 +68,27 @@ it('resolves the purpose to the address of the mailbox it is currently mapped to
->and((string) $transport)->toContain('support@clupilot.com');
});
it('marks the DSN as unconfigured rather than showing a healthy-looking address with no password', function () {
// The exact harm the finding named: describe() only ever showed the
// address, so a mailbox with an active row, a real address, and no
// password looked identical to a working one — right up until send()
// threw. The address stays visible (an operator needs to know WHICH
// mailbox is broken); the marker is what stops it from reading as fine.
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com', 'password' => null]);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'smtp');
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect((string) $transport)
->toContain('support@clupilot.com')
->toContain('unconfigured');
// Agreement, not just a matching label: the DSN's warning must correspond
// to send() actually refusing, not to a marker painted on independently.
expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class);
});
it('logs instead of delivering while MAIL_MAILER is log', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');