Trust an explicitly configured mail host or port over the placeholder

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).
feat/mailboxes
nexxo 2026-07-28 07:10:53 +02:00
parent 15c81489d4
commit a6a9c76660
3 changed files with 285 additions and 42 deletions

View File

@ -37,23 +37,49 @@ return new class extends Migration
* config/mail.php's own fallbacks when MAIL_HOST/MAIL_PORT are unset. * config/mail.php's own fallbacks when MAIL_HOST/MAIL_PORT are unset.
* *
* config('mail.mailers.smtp.host') cannot tell "genuinely unset" apart * config('mail.mailers.smtp.host') cannot tell "genuinely unset" apart
* from "deliberately set to 127.0.0.1" config/mail.php's own * from "deliberately set to 127.0.0.1" on its own config/mail.php's own
* env('MAIL_HOST', '127.0.0.1') has already substituted by the time this * env('MAIL_HOST', '127.0.0.1') has already substituted by the time this
* migration reads it, and 127.0.0.1 is not '', so it would sail straight * migration reads it, and 127.0.0.1 is not '', so it would sail straight
* past MailboxTransport's "host is not configured" guard (Task 4) and * past MailboxTransport's "host is not configured" guard (Task 4) and
* quietly dial localhost instead of refusing loudly. * quietly dial localhost instead of refusing loudly. Comparing the
* resolved value against these two placeholders is what tells the two
* apart but only as a FALLBACK now, not the whole answer: an install
* genuinely relaying through 127.0.0.1:2525 (a sidecar relay, commonly
* paired with MAIL_URL) is exactly as real as one on any other
* host:port, and this comparison alone cannot tell that apart from
* "nothing configured" either it would zero out a working setup the
* moment this migration ran, and MailboxTransport would then refuse
* every send with "port is not configured" (Codex R15#8, P1). See
* isExplicitlySupplied() below for the fix, and for why this sentinel is
* still needed alongside it rather than replaced outright.
* *
* Read raw env() instead? That returns null (correctly distinguishing * Why keep a sentinel at all, instead of only ever reading raw env()?
* "unset") ONLY as long as .env was actually loaded which Laravel skips * env('MAIL_HOST') correctly returns null for "genuinely unset" but
* entirely once `config:cache` has run (LoadEnvironmentVariables checks * ONLY as long as .env was actually loaded, which Laravel skips entirely
* configurationIsCached() first). deploy/update.sh always migrates * once `config:cache` has run (LoadEnvironmentVariables checks
* before it rebuilds the cache, but nothing enforces that order, and * app()->configurationIsCached() first, before ever calling Dotenv see
* env() is also awkward to test (it does not go through config(), so * that class directly, not guessed). This migration cannot control that:
* Pest's usual config()->set() cannot drive it). Comparing against the * deploy/update.sh always migrates before it rebuilds the cache, but
* placeholder keeps this correct under config:cache regardless of * nothing enforces that order for a hand-run `migrate` on a server
* ordering, and testable the same way every other test in this suite * someone already cached. Under that ordering, isExplicitlySupplied()
* already is. Trade-off: an install that deliberately runs its relay on * below collapses to exactly this sentinel comparison alone env()
* 127.0.0.1:2525 reads as unset nobody does that for outbound mail. * returns null for every key, so it never overrides it which is this
* migration's behaviour from BEFORE R15#8, not a regression past it.
*
* Checked, not assumed: app()->configurationIsCached() only reports that
* Dotenv was skipped, not that MAIL_HOST/MAIL_PORT are therefore
* unreadable by env() a deployment that also exported them as real
* container-level environment variables (docker-compose's own
* `environment:` block, systemd, etc.) would keep seeing them through
* env() regardless of the config cache, and isExplicitlySupplied() would
* keep working correctly even then. What makes "collapses to the
* sentinel" a COMPLETE answer here, specifically, is this repository's
* own docker-compose.yml: the app service deliberately does not inject
* .env as real process environment ("Do NOT inject it as real env vars
* (env_file): that would override the test env... Only vite needs these
* at process level") — so once cached, MAIL_HOST/MAIL_PORT genuinely have
* no remaining source for env() to read on THIS install. A fork of that
* compose file which changed that would want to re-check this comment.
*/ */
private const UNCONFIGURED_HOST = '127.0.0.1'; private const UNCONFIGURED_HOST = '127.0.0.1';
@ -109,16 +135,59 @@ return new class extends Migration
* every field it actually supplies, and an unset MAIL_URL isset() on a * every field it actually supplies, and an unset MAIL_URL isset() on a
* null config value is false, same as MailManager's own check leaves * null config value is false, same as MailManager's own check leaves
* this a no-op, returning the individual keys exactly as before. * this a no-op, returning the individual keys exactly as before.
*
* Also returns 'suppliedByUrl': isExplicitlySupplied() (Codex R15#8, P1)
* needs to know whether host/port SPECIFICALLY came from the URL, and the
* merged 'config' above cannot answer that it always has a 'host' and a
* 'port' key regardless, falling back to $config's own already-defaulted
* values whenever the URL did not supply one. A second, isolated call to
* the same PUBLIC parseConfiguration() below given only the url, none
* of $config's other keys riding along is what actually answers it.
*
* @return array{config: array<string, mixed>, suppliedByUrl: array<string, mixed>}
*/ */
private function resolveSmtpConfig(): array private function resolveSmtpConfig(): array
{ {
$config = (array) config('mail.mailers.smtp', []); $config = (array) config('mail.mailers.smtp', []);
if (! isset($config['url'])) { if (! isset($config['url'])) {
return $config; return ['config' => $config, 'suppliedByUrl' => []];
} }
return array_merge($config, (new ConfigurationUrlParser)->parseConfiguration($config)); $parser = new ConfigurationUrlParser;
return [
'config' => array_merge($config, $parser->parseConfiguration($config)),
'suppliedByUrl' => $parser->parseConfiguration(['url' => $config['url']]),
];
}
/**
* Whether an operator actually stated $field through .env directly, or
* through MAIL_URL as distinct from it merely sitting at config/
* mail.php's own substituted default. This is the fix for Codex R15#8,
* P1 (see UNCONFIGURED_HOST's docblock for the bug it closes).
*
* env($envKey) is checked first and is the PRIMARY signal: ANY value it
* returns even one that happens to equal the placeholder is
* something an operator or their tooling actually wrote, and must be
* trusted over the sentinel rather than second-guessed by it.
* $suppliedByUrl (see resolveSmtpConfig()) carries the same standing for
* MAIL_URL: a URL's host/port is exactly as deliberate as the individual
* env var, and resolveSmtpConfig() already makes it win in the merged
* config second-guessing it here with the sentinel would undo that.
*
* Only when NEITHER says anything does UNCONFIGURED_HOST/UNCONFIGURED_
* PORT's own sentinel comparison in seed() still apply exactly as it
* did before this fix. That fallback is deliberate, not a leftover: see
* UNCONFIGURED_HOST's own docblock for why env() cannot simply replace it
* outright (config:cache).
*
* @param array<string, mixed> $suppliedByUrl
*/
private function isExplicitlySupplied(string $envKey, string $field, array $suppliedByUrl): bool
{
return env($envKey) !== null || array_key_exists($field, $suppliedByUrl);
} }
/** /**
@ -169,16 +238,24 @@ return new class extends Migration
private function seed(): void private function seed(): void
{ {
$smtp = $this->resolveSmtpConfig(); $resolved = $this->resolveSmtpConfig();
$smtp = $resolved['config'];
$suppliedByUrl = $resolved['suppliedByUrl'];
// The server, once. Whatever .env already proved workable — but not // The server, once. Whatever .env (or MAIL_URL) already proved
// config/mail.php's own placeholder (see UNCONFIGURED_HOST above). // workable — but not config/mail.php's own placeholder (see
// UNCONFIGURED_HOST above), UNLESS an operator can be shown to have
// actually written this value themselves. isExplicitlySupplied()
// checks raw env() and the parsed URL before the sentinel is allowed
// to zero anything out (Codex R15#8, P1).
$configuredHost = (string) ($smtp['host'] ?? ''); $configuredHost = (string) ($smtp['host'] ?? '');
$host = $configuredHost === self::UNCONFIGURED_HOST ? '' : $configuredHost; $hostSupplied = $this->isExplicitlySupplied('MAIL_HOST', 'host', $suppliedByUrl);
$host = (! $hostSupplied && $configuredHost === self::UNCONFIGURED_HOST) ? '' : $configuredHost;
Settings::set('mail.host', $host); Settings::set('mail.host', $host);
$configuredPort = (int) ($smtp['port'] ?? 0); $configuredPort = (int) ($smtp['port'] ?? 0);
$port = $configuredPort === self::UNCONFIGURED_PORT ? 0 : $configuredPort; $portSupplied = $this->isExplicitlySupplied('MAIL_PORT', 'port', $suppliedByUrl);
$port = (! $portSupplied && $configuredPort === self::UNCONFIGURED_PORT) ? 0 : $configuredPort;
Settings::set('mail.port', $port); Settings::set('mail.port', $port);
// Codex R15#4, P1a: a scheme actually STATED (MAIL_SCHEME=smtps, or an // Codex R15#4, P1a: a scheme actually STATED (MAIL_SCHEME=smtps, or an

View File

@ -18,8 +18,30 @@ function loadMailboxSeedMigration(): object
return require database_path('migrations/2026_07_28_100000_seed_mailboxes_from_environment.php'); return require database_path('migrations/2026_07_28_100000_seed_mailboxes_from_environment.php');
} }
beforeEach(function () { /**
* This container's real .env carries actual MAIL_HOST/MAIL_PORT/MAIL_URL
* values (see the migration's UNCONFIGURED_HOST docblock) env() reads
* those directly, regardless of what an individual test's config()->set()
* says, and Env::getRepository() is a process-wide singleton, not something
* RefreshDatabase or a fresh Application instance resets between tests. Left
* alone, every test in this file would be quietly deciding "explicitly
* supplied" (Codex R15#8, P1) based on this machine's own mail setup rather
* than on what it actually configured true today only because this
* container happens to have real mail credentials, and wrong the moment it
* does not. Neutralised here for every test by default; withRealEnv() opts
* individual tests back in to prove the explicitly-supplied paths themselves.
*/
$realMailEnvBefore = [];
beforeEach(function () use (&$realMailEnvBefore) {
config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32))); config()->set('admin_access.secrets_key', 'base64:'.base64_encode(random_bytes(32)));
$realMailEnvBefore = captureRealEnv(['MAIL_HOST', 'MAIL_PORT', 'MAIL_URL']);
setRealEnv(['MAIL_HOST' => null, 'MAIL_PORT' => null, 'MAIL_URL' => null]);
});
afterEach(function () use (&$realMailEnvBefore) {
setRealEnv($realMailEnvBefore);
}); });
it('does not copy MAIL_SCHEME through — a Symfony transport only knows smtp and smtps', function () { it('does not copy MAIL_SCHEME through — a Symfony transport only knows smtp and smtps', function () {
@ -173,7 +195,14 @@ it('prefers an explicit MAIL_SCHEME over a conflicting MAIL_URL scheme', functio
expect(Settings::get('mail.encryption'))->toBe('tls'); expect(Settings::get('mail.encryption'))->toBe('tls');
}); });
it('treats config/mail.php\'s own 127.0.0.1 / 2525 defaults as unset, not as a real relay', function () { it('treats config/mail.php\'s own 127.0.0.1 / 2525 defaults as unset when neither env() nor MAIL_URL says otherwise', function () {
// Genuinely unset, on BOTH signals isExplicitlySupplied() now checks —
// not just the config()->set() value: this file's own beforeEach()
// neutralises the real MAIL_HOST/MAIL_PORT/MAIL_URL this container
// actually carries, so env() reads null here exactly like a fresh
// install's would, and no MAIL_URL is set either. Codex R15#8, P1 is the
// three tests below this one; this is the control that proves the
// sentinel fallback itself still fires when it is supposed to.
clearMailboxSeed(); clearMailboxSeed();
config()->set('mail.mailers.smtp.host', '127.0.0.1'); config()->set('mail.mailers.smtp.host', '127.0.0.1');
config()->set('mail.mailers.smtp.port', 2525); config()->set('mail.mailers.smtp.port', 2525);
@ -187,6 +216,61 @@ it('treats config/mail.php\'s own 127.0.0.1 / 2525 defaults as unset, not as a r
->and(Settings::get('mail.port'))->toBe(0); ->and(Settings::get('mail.port'))->toBe(0);
}); });
it('stores an explicitly configured MAIL_PORT=2525 instead of zeroing it — Codex R15#8, P1', function () {
// The finding itself: a relay genuinely listening on 2525 (config/mail.
// php's own placeholder port) is indistinguishable from "nothing
// configured" by the sentinel comparison alone. env('MAIL_PORT')
// returning ANYTHING — including '2525' — is what now proves an operator
// actually wrote this, not config/mail.php's substitution.
clearMailboxSeed();
config()->set('mail.mailers.smtp.host', 'relay.internal.example');
config()->set('mail.mailers.smtp.port', 2525);
withRealEnv(['MAIL_PORT' => '2525'], function () {
loadMailboxSeedMigration()->up();
});
// Not 0 — MailboxTransport must still be able to dial :2525 for real.
expect(Settings::get('mail.port'))->toBe(2525);
});
it('stores an explicitly configured MAIL_HOST=127.0.0.1 instead of blanking it — Codex R15#8, P1', function () {
// The same finding, the host half: an install genuinely relaying through
// localhost — a sidecar SMTP relay in the same container/pod, commonly
// paired with MAIL_URL — is indistinguishable from "nothing configured"
// by the sentinel comparison alone.
clearMailboxSeed();
config()->set('mail.mailers.smtp.host', '127.0.0.1');
config()->set('mail.mailers.smtp.port', 2526); // a real, non-placeholder port alongside it
withRealEnv(['MAIL_HOST' => '127.0.0.1'], function () {
loadMailboxSeedMigration()->up();
});
// Not '' — MailboxTransport must still be able to dial 127.0.0.1 for real.
expect(Settings::get('mail.host'))->toBe('127.0.0.1');
});
it('trusts a host and port supplied through MAIL_URL over the sentinel, even when they equal it exactly', function () {
// The finding's other named case: "a port or host that came from the URL
// must not be second-guessed by the sentinel at all." Distinct from the
// MAIL_URL tests further down — those use a URL host/port that already
// differs from the placeholder, so they would pass even without this
// fix. This one deliberately points MAIL_URL AT 127.0.0.1:2525 — the
// exact sentinel values — so only isExplicitlySupplied()'s suppliedByUrl
// half, not the env() half (MAIL_HOST/MAIL_PORT individually stay
// unset), can be what saves it.
clearMailboxSeed();
config()->set('mail.mailers.smtp.host', '127.0.0.1');
config()->set('mail.mailers.smtp.port', 2525);
config()->set('mail.mailers.smtp.url', 'smtp://user:pass@127.0.0.1:2525');
loadMailboxSeedMigration()->up();
expect(Settings::get('mail.host'))->toBe('127.0.0.1')
->and(Settings::get('mail.port'))->toBe(2525);
});
it('still stores a deliberately configured host and port untouched', function () { it('still stores a deliberately configured host and port untouched', function () {
clearMailboxSeed(); clearMailboxSeed();
config()->set('mail.mailers.smtp.host', 'smtp.example.test'); config()->set('mail.mailers.smtp.host', 'smtp.example.test');

View File

@ -1,5 +1,21 @@
<?php <?php
use App\Models\Mailbox;
use App\Models\User;
use App\Services\Dns\FakeHetznerDnsClient;
use App\Services\Dns\HetznerDnsClient;
use App\Services\Mail\MailPurpose;
use App\Services\Monitoring\FakeMonitoringClient;
use App\Services\Monitoring\MonitoringClient;
use App\Services\Proxmox\FakeProxmoxClient;
use App\Services\Proxmox\ProxmoxClient;
use App\Services\Ssh\FakeRemoteShell;
use App\Services\Ssh\RemoteShell;
use App\Services\Traefik\FakeTraefikWriter;
use App\Services\Traefik\TraefikWriter;
use App\Services\Wireguard\FakeWireguardHub;
use App\Services\Wireguard\WireguardHub;
use App\Support\Settings;
use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Foundation\Testing\RefreshDatabase;
use Tests\TestCase; use Tests\TestCase;
@ -53,23 +69,23 @@ function something()
* Bind fake provisioning services into the container and return them so a test * Bind fake provisioning services into the container and return them so a test
* can script/inspect SSH, WireGuard, Proxmox, DNS and Traefik interactions. * can script/inspect SSH, WireGuard, Proxmox, DNS and Traefik interactions.
* *
* @return array{shell: \App\Services\Ssh\FakeRemoteShell, hub: \App\Services\Wireguard\FakeWireguardHub, pve: \App\Services\Proxmox\FakeProxmoxClient, dns: \App\Services\Dns\FakeHetznerDnsClient, traefik: \App\Services\Traefik\FakeTraefikWriter} * @return array{shell: FakeRemoteShell, hub: FakeWireguardHub, pve: FakeProxmoxClient, dns: FakeHetznerDnsClient, traefik: FakeTraefikWriter}
*/ */
function fakeServices(): array function fakeServices(): array
{ {
$shell = new \App\Services\Ssh\FakeRemoteShell; $shell = new FakeRemoteShell;
$hub = new \App\Services\Wireguard\FakeWireguardHub; $hub = new FakeWireguardHub;
$pve = new \App\Services\Proxmox\FakeProxmoxClient; $pve = new FakeProxmoxClient;
$dns = new \App\Services\Dns\FakeHetznerDnsClient; $dns = new FakeHetznerDnsClient;
$traefik = new \App\Services\Traefik\FakeTraefikWriter; $traefik = new FakeTraefikWriter;
$monitoring = new \App\Services\Monitoring\FakeMonitoringClient; $monitoring = new FakeMonitoringClient;
app()->instance(\App\Services\Ssh\RemoteShell::class, $shell); app()->instance(RemoteShell::class, $shell);
app()->instance(\App\Services\Wireguard\WireguardHub::class, $hub); app()->instance(WireguardHub::class, $hub);
app()->instance(\App\Services\Proxmox\ProxmoxClient::class, $pve); app()->instance(ProxmoxClient::class, $pve);
app()->instance(\App\Services\Dns\HetznerDnsClient::class, $dns); app()->instance(HetznerDnsClient::class, $dns);
app()->instance(\App\Services\Traefik\TraefikWriter::class, $traefik); app()->instance(TraefikWriter::class, $traefik);
app()->instance(\App\Services\Monitoring\MonitoringClient::class, $monitoring); app()->instance(MonitoringClient::class, $monitoring);
return ['shell' => $shell, 'hub' => $hub, 'pve' => $pve, 'dns' => $dns, 'traefik' => $traefik, 'monitoring' => $monitoring]; return ['shell' => $shell, 'hub' => $hub, 'pve' => $pve, 'dns' => $dns, 'traefik' => $traefik, 'monitoring' => $monitoring];
} }
@ -78,14 +94,14 @@ function fakeServices(): array
| Shared account helpers. Defined here NOT in an individual test file so a | Shared account helpers. Defined here NOT in an individual test file so a
| targeted run (pest tests/Feature/Admin/OneFile.php) still resolves them. | targeted run (pest tests/Feature/Admin/OneFile.php) still resolves them.
*/ */
function operator(string $role): App\Models\User function operator(string $role): User
{ {
return App\Models\User::factory()->operator($role)->create(); return User::factory()->operator($role)->create();
} }
function admin(): App\Models\User function admin(): User
{ {
return App\Models\User::factory()->create(['is_admin' => true]); return User::factory()->create(['is_admin' => true]);
} }
/** /**
@ -104,9 +120,75 @@ function admin(): App\Models\User
*/ */
function clearMailboxSeed(): void function clearMailboxSeed(): void
{ {
App\Models\Mailbox::query()->delete(); Mailbox::query()->delete();
foreach (App\Services\Mail\MailPurpose::ALL as $purpose) { foreach (MailPurpose::ALL as $purpose) {
App\Support\Settings::set(App\Services\Mail\MailPurpose::settingKey($purpose), null); Settings::set(MailPurpose::settingKey($purpose), null);
}
}
/**
* env()'s own seam. Illuminate\Support\Env reads real process environment
* variables (putenv()/getenv(), $_ENV, $_SERVER Env::getRepository() chains
* readers over all three), not config(), so config()->set() the way every
* other test in this suite drives behaviour cannot touch it. This captures
* the CURRENT value of each key exactly as env() would see it, distinguishing
* "not set at all" (null) from "set to an empty string", so a caller can put
* it back afterward without leaving a variable behind or removing one that
* was never this test's to touch.
*
* @param array<int, string> $keys
* @return array<string, string|null>
*/
function captureRealEnv(array $keys): array
{
$captured = [];
foreach ($keys as $key) {
$value = getenv($key);
$captured[$key] = $value === false ? null : $value;
}
return $captured;
}
/**
* Writes real process environment variables through every adapter
* Env::getRepository() might be reading from, so it does not matter which one
* answers first. null unsets the variable entirely distinct from '', which
* env() would return as a real, present, empty value.
*
* @param array<string, string|null> $vars
*/
function setRealEnv(array $vars): void
{
foreach ($vars as $key => $value) {
if ($value === null) {
putenv($key);
unset($_ENV[$key], $_SERVER[$key]);
} else {
putenv("{$key}={$value}");
$_ENV[$key] = $value;
$_SERVER[$key] = $value;
}
}
}
/**
* Runs $callback with the given real environment variables set, then restores
* exactly what was there before including "was not set at all". Use this to
* prove behaviour that reads raw env() rather than config(); see
* captureRealEnv()'s own docblock for why config()->set() cannot do this.
*/
function withRealEnv(array $vars, Closure $callback): mixed
{
$previous = captureRealEnv(array_keys($vars));
setRealEnv($vars);
try {
return $callback();
} finally {
setRealEnv($previous);
} }
} }