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

Close the safety-net gaps the review found in MailboxTransport 

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>
feat/mailboxes
nexxo 2026-07-27 23:02:18 +02:00
parent 0be9c61484
commit a39670f920
2 changed files with 201 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -2,12 +2,16 @@
namespace App\Mail\Transport;
use App\Models\Mailbox;
use App\Services\Mail\MailboxResolver;
use App\Support\Settings;
use Illuminate\Mail\Transport\ArrayTransport;
use Illuminate\Mail\Transport\LogTransport;
use Illuminate\Support\Facades\Log;
use RuntimeException;
use Symfony\Component\Mailer\Envelope;
use Symfony\Component\Mailer\SentMessage;
use Symfony\Component\Mailer\Transport\NullTransport;
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
use Symfony\Component\Mailer\Transport\TransportInterface;
use Symfony\Component\Mime\RawMessage;
@ -25,6 +29,18 @@ use Symfony\Component\Mime\RawMessage;
*/
class MailboxTransport implements TransportInterface
{
/**
* mail.default values that must never open a real connection.
*
* 'array' is here because phpunit.xml sets it as the suite-wide test
* default — the first feature test that sends through a purpose mailer
* without Mail::fake() must not reach real SMTP just because 'array' is
* not the exact string 'log'. `null` is here because a stored setting can
* decode to it just as easily as to a real value (see the port note in
* delegate()): an unconfigured mailer must fail SAFE, not fail open.
*/
private const NON_DELIVERING = ['log', 'array', null];
private ?TransportInterface $delegate = null;
private ?string $fingerprint = null;
@ -44,37 +60,52 @@ class MailboxTransport implements TransportInterface
/** What this transport currently points at — used by __toString and tests. */
private function describe(): ?string
{
if ($this->isLogging()) {
return 'log';
}
[$mode, $box] = $this->resolution();
return app(MailboxResolver::class)->for($this->purpose)?->address;
return $mode === 'mailbox' ? $box?->address : $mode;
}
/**
* Delivery is off wherever the default mailer is the log.
* 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.
*
* This is what keeps a development machine from mailing real customers: the
* mailables name a purpose, so without this check they would bypass
* MAIL_MAILER=log entirely and send for real.
* @return array{0: string, 1: ?Mailbox}
*/
private function isLogging(): bool
private function resolution(): array
{
return config('mail.default') === 'log';
$default = config('mail.default');
if (in_array($default, self::NON_DELIVERING, true)) {
return [$default ?? 'null', null];
}
return ['mailbox', app(MailboxResolver::class)->for($this->purpose)];
}
private function delegate(): TransportInterface
{
if ($this->isLogging()) {
return new LogTransport(Log::channel(config('mail.mailers.log.channel')));
}
[$mode, $box] = $this->resolution();
$box = app(MailboxResolver::class)->for($this->purpose);
if ($mode !== 'mailbox') {
return match ($mode) {
'log' => new LogTransport(Log::channel(config('mail.mailers.log.channel'))),
'array' => new ArrayTransport,
default => new NullTransport, // 'null' — mail.default not configured at all
};
}
if ($box === null || ! $box->isConfigured()) {
// Loud, not silent: a mail with no mailbox behind it must not look
// like it was sent.
throw new \RuntimeException(
// 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}]."
);
}
@ -83,6 +114,21 @@ class MailboxTransport implements TransportInterface
$port = (int) Settings::get('mail.port', 587);
$encryption = (string) Settings::get('mail.encryption', 'tls');
// Guarded the same way as the missing mailbox above. Tasks 6-7 are
// what write these settings, so today they default to blank/absent —
// and a STORED null port json-decodes to null, which casts to 0; left
// unguarded, EsmtpTransport turns port 0 into plaintext port 25
// rather than refusing (the 587 default only applies when the row is
// absent, not when it holds null). Loud beats a silently downgraded
// connection.
if ($host === '') {
throw new RuntimeException('Mail server host is not configured — cannot send mail.');
}
if ($port < 1) {
throw new RuntimeException('Mail server port is not configured — cannot send mail.');
}
$fingerprint = md5(implode('|', [
$host, $port, $encryption, $box->smtpUsername(), (string) $box->password,
]));

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

@ -4,9 +4,11 @@ use App\Mail\Transport\MailboxTransport;
use App\Models\Mailbox;
use App\Services\Mail\MailPurpose;
use App\Support\Settings;
use Illuminate\Mail\Transport\LogTransport;
use Illuminate\Mail\Transport\ArrayTransport;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Mail;
use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport;
use Symfony\Component\Mailer\Transport\NullTransport;
use Symfony\Component\Mailer\Transport\TransportInterface;
beforeEach(function () {
config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32)));
@ -15,6 +17,21 @@ beforeEach(function () {
Settings::set('mail.encryption', 'tls');
});
/**
* Reach into the same private delegate() that send() itself calls.
*
* __toString() and send() decide "log, array, null, or a real mailbox"
* through describe()/delegate() — two DIFFERENT methods. A mutation that
* broke only delegate() once passed the whole suite, because every test
* checked (string) $transport, which never calls it. Where a behavioural
* proof (actually sending, see the log-guard test below) is impractical
* without real I/O, this is how those tests reach the one that matters.
*/
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 () {
// The five entries exist as plain config — no query has run to build them.
foreach (MailPurpose::ALL as $purpose) {
@ -23,7 +40,24 @@ it('registers a mailer per purpose without touching the database at boot', funct
}
});
it('builds the transport from the mailbox that is current at send time', function () {
it('defines every purpose mailer as a plain literal, so building it cannot touch the database', function () {
// A slice with no parenthesis in it cannot contain a function call —
// env(), config(), DB::table(), all of them need one. Checked at the
// SOURCE level, the same technique DisplayTimezoneTest and IconLayoutTest
// use for a property of the file rather than of one particular run.
$source = (string) file_get_contents(config_path('mail.php'));
foreach (MailPurpose::ALL as $purpose) {
preg_match('/\'cp_'.preg_quote($purpose, '/').'\'\s*=>\s*(\[[^\]]*\])/', $source, $matches);
expect($matches)->toHaveCount(2, "cp_{$purpose} is missing or not a plain array literal");
expect($matches[1])->not->toContain('(')
->and($matches[1])->toContain("'mailbox'")
->and($matches[1])->toContain("'{$purpose}'");
}
});
it('resolves the purpose to the address of the mailbox it is currently mapped to', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'smtp'); // delivery on, as on live
@ -47,32 +81,120 @@ it('logs instead of delivering while MAIL_MAILER is log', function () {
});
/*
| __toString() and send() decide "log or SMTP" through two SEPARATE code
| paths (describe() vs delegate()). The test above only exercises the first —
| it would keep passing even if delegate() lost its own guard and sent every
| purpose-mailer through real SMTP regardless of MAIL_MAILER. Reach into the
| method that actually sends, so that regression cannot hide behind a passing
| __toString() assertion. No real socket is opened either way: EsmtpTransport
| connects lazily on send(), never in its constructor.
| LOAD-BEARING for the MAIL_MAILER=log guard — this is the one that actually
| SENDS, so a broken guard cannot hide behind a __toString() that still looks
| right. Settings point at a closed loopback port: nothing listens on
| 127.0.0.1:1, so a real attempt is refused in milliseconds — no DNS, no
| dependency on this environment's network egress policy, and the failure
| (a thrown TransportException) names the actual harm rather than a class
| mismatch.
*/
it('actually delegates to the log transport, not SMTP, while MAIL_MAILER is log', function () {
it('actually sends through the log transport, not SMTP, while MAIL_MAILER is log', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'log');
Settings::set('mail.host', '127.0.0.1');
Settings::set('mail.port', 1);
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
$delegate = Closure::bind(fn () => $this->delegate(), $transport, MailboxTransport::class)();
Log::shouldReceive('channel')->andReturnSelf();
Log::shouldReceive('debug')->once();
expect($delegate)->toBeInstanceOf(LogTransport::class);
Mail::mailer('cp_support')->raw('body', function ($message) {
$message->to('customer@example.test')->subject('Test');
});
});
it('delegates to a real SMTP transport once MAIL_MAILER is no longer log', function () {
it('does not deliver for real when MAIL_MAILER is array, the suite-wide test default', function () {
// phpunit.xml forces MAIL_MAILER=array for the whole suite. Task 5 wires
// real mailables to these mailers next — the first feature test that
// triggers one without Mail::fake() must not open a real socket just
// because 'array' is not the exact string 'log'.
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'array');
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect(mailboxDelegate($transport))->toBeInstanceOf(ArrayTransport::class);
});
it('does not deliver for real when MAIL_MAILER is not configured at all', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', null);
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect(mailboxDelegate($transport))->toBeInstanceOf(NullTransport::class);
});
it('reuses the same delegate until the mailbox credentials actually change', function () {
// The headline feature of the class. A worker builds its mailer once and
// keeps it for hours, so a password the operator corrects in the console
// must reach the NEXT mail without a restart — but not open a new SMTP
// connection for every single message when nothing changed either.
$box = Mailbox::factory()->create([
'key' => 'support', 'address' => 'support@clupilot.com', 'password' => 'first-password',
]);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'smtp');
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
$delegate = Closure::bind(fn () => $this->delegate(), $transport, MailboxTransport::class)();
expect($delegate)->toBeInstanceOf(EsmtpTransport::class);
$first = mailboxDelegate($transport);
$second = mailboxDelegate($transport);
expect($second)->toBe($first); // nothing changed — no new connection per mail
$box->update(['password' => 'second-password']);
$third = mailboxDelegate($transport);
expect($third)->not->toBe($first); // the operator's correction takes effect immediately
});
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');
config()->set('mail.default', 'smtp');
Settings::set('mail.port', 587);
Settings::set('mail.encryption', 'tls');
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect((string) mailboxDelegate($transport))->toBe('smtp://mail.example.test:587');
});
it('opens an implicit-TLS connection for the SMTPS port, never a hanging STARTTLS one', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'smtp');
Settings::set('mail.port', 465);
Settings::set('mail.encryption', 'ssl');
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect((string) mailboxDelegate($transport))->toBe('smtps://mail.example.test');
});
it('refuses to send when the mail server host is blank', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'smtp');
Settings::set('mail.host', ''); // Tasks 6-7 are what write this — nothing has yet
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class);
});
it('refuses to send when the mail server port is not a positive number', function () {
Mailbox::factory()->create(['key' => 'support', 'address' => 'support@clupilot.com']);
Settings::set(MailPurpose::settingKey(MailPurpose::SUPPORT), 'support');
config()->set('mail.default', 'smtp');
Settings::set('mail.port', null); // what a stored NULL json-decodes to, then casts to 0
$transport = Mail::mailer('cp_support')->getSymfonyTransport();
expect(fn () => mailboxDelegate($transport))->toThrow(RuntimeException::class);
});