diff --git a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php index 8ac53f3..3861645 100644 --- a/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php +++ b/database/migrations/2026_07_28_100000_seed_mailboxes_from_environment.php @@ -37,23 +37,49 @@ return new class extends Migration * config/mail.php's own fallbacks when MAIL_HOST/MAIL_PORT are unset. * * 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 * 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 - * 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 - * "unset") ONLY as long as .env was actually loaded — which Laravel skips - * entirely once `config:cache` has run (LoadEnvironmentVariables checks - * configurationIsCached() first). deploy/update.sh always migrates - * before it rebuilds the cache, but nothing enforces that order, and - * env() is also awkward to test (it does not go through config(), so - * Pest's usual config()->set() cannot drive it). Comparing against the - * placeholder keeps this correct under config:cache regardless of - * ordering, and testable the same way every other test in this suite - * already is. Trade-off: an install that deliberately runs its relay on - * 127.0.0.1:2525 reads as unset — nobody does that for outbound mail. + * Why keep a sentinel at all, instead of only ever reading raw env()? + * env('MAIL_HOST') correctly returns null for "genuinely unset" — but + * ONLY as long as .env was actually loaded, which Laravel skips entirely + * once `config:cache` has run (LoadEnvironmentVariables checks + * app()->configurationIsCached() first, before ever calling Dotenv — see + * that class directly, not guessed). This migration cannot control that: + * deploy/update.sh always migrates before it rebuilds the cache, but + * nothing enforces that order for a hand-run `migrate` on a server + * someone already cached. Under that ordering, isExplicitlySupplied() + * below collapses to exactly this sentinel comparison alone — env() + * 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'; @@ -109,16 +135,59 @@ return new class extends Migration * 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 * 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, suppliedByUrl: array} */ private function resolveSmtpConfig(): array { $config = (array) config('mail.mailers.smtp', []); 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 $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 { - $smtp = $this->resolveSmtpConfig(); + $resolved = $this->resolveSmtpConfig(); + $smtp = $resolved['config']; + $suppliedByUrl = $resolved['suppliedByUrl']; - // The server, once. Whatever .env already proved workable — but not - // config/mail.php's own placeholder (see UNCONFIGURED_HOST above). + // The server, once. Whatever .env (or MAIL_URL) already proved + // 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'] ?? ''); - $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); $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); // Codex R15#4, P1a: a scheme actually STATED (MAIL_SCHEME=smtps, or an diff --git a/tests/Feature/Mail/MailboxSeedMigrationTest.php b/tests/Feature/Mail/MailboxSeedMigrationTest.php index c8815d3..fb0912f 100644 --- a/tests/Feature/Mail/MailboxSeedMigrationTest.php +++ b/tests/Feature/Mail/MailboxSeedMigrationTest.php @@ -18,8 +18,30 @@ function loadMailboxSeedMigration(): object 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))); + + $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 () { @@ -173,7 +195,14 @@ it('prefers an explicit MAIL_SCHEME over a conflicting MAIL_URL scheme', functio 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(); config()->set('mail.mailers.smtp.host', '127.0.0.1'); 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); }); +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 () { clearMailboxSeed(); config()->set('mail.mailers.smtp.host', 'smtp.example.test'); diff --git a/tests/Pest.php b/tests/Pest.php index c41cd98..a7b46a2 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -1,5 +1,21 @@ instance(\App\Services\Ssh\RemoteShell::class, $shell); - app()->instance(\App\Services\Wireguard\WireguardHub::class, $hub); - app()->instance(\App\Services\Proxmox\ProxmoxClient::class, $pve); - app()->instance(\App\Services\Dns\HetznerDnsClient::class, $dns); - app()->instance(\App\Services\Traefik\TraefikWriter::class, $traefik); - app()->instance(\App\Services\Monitoring\MonitoringClient::class, $monitoring); + app()->instance(RemoteShell::class, $shell); + app()->instance(WireguardHub::class, $hub); + app()->instance(ProxmoxClient::class, $pve); + app()->instance(HetznerDnsClient::class, $dns); + app()->instance(TraefikWriter::class, $traefik); + app()->instance(MonitoringClient::class, $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 | 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 { - App\Models\Mailbox::query()->delete(); + Mailbox::query()->delete(); - foreach (App\Services\Mail\MailPurpose::ALL as $purpose) { - App\Support\Settings::set(App\Services\Mail\MailPurpose::settingKey($purpose), null); + foreach (MailPurpose::ALL as $purpose) { + 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 $keys + * @return array + */ +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 $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); } }