From b9bb1f12353cfd757501a549994c461fdfa37858 Mon Sep 17 00:00:00 2001 From: Abdelkarim Mateos Sanchez Date: Fri, 17 Jul 2026 21:31:55 +0200 Subject: [PATCH] =?UTF-8?q?refactor(quality):=20Rector=20wave=206=20?= =?UTF-8?q?=E2=80=94=20null-coalesce=20default=20gate,=20phantom=20TTL=20f?= =?UTF-8?q?allback=20removed=20(AID-542)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the ApplyDefaultInsteadOfNullCoalesceRector split (AID-531) the opposite way from mechanical application: fix the two flagged sites by hand and keep the rule armed as the gate's guard. The ticket assumed the classic ??→?: trap; the rule actually rewrites `helper('key') ?? $default` into `helper('key', $default)`. That form still has a non-equivalent corner — a key present with an explicit null returns $default under ??, null under the parameter form — but both flagged sites read `config('unblock.hq.ttl') ?? 7200`, where that corner is unreachable by construction: config/unblock.php defines the key as `(int) env('HQ_WHITELIST_TTL', 14000)`, so it always exists as an int. Which exposed the real defect: the ?? 7200 was dead code AND a phantom default — the code (and its "Default 2 hours" comment) claimed 7200 while the actual default is 14000, owned by the config file. Both fallbacks are removed; the default now has a single owner. The suite caught what the reachability analysis alone could not: one test pinned the phantom fallback by fabricating the impossible state (`config()->set('unblock.hq.ttl', null)` at runtime, then asserting 7200). "Not configured" really means the env var is absent, in which case config resolves 14000 at load time — null never occurs. The test was the fallback's only consumer, and it consumed a state that cannot exist; it now pins the real contract (config-owned 14000 flowing through to the mailable) with an explicit guard assertion. The rule stays enabled: zero-diff after the hand fix, proven to bite (a new `config('x') ?? $default` fails refactor:dry naming the file), guarding against future phantom or duplicated defaults. If a genuinely present-but-null key ever appears, skip that call site — not the rule. Gates: check-full green, 1442 passed / 16 skipped (3585 assertions), coverage 89.8%, refactor:dry (cleared cache) exit 0. Co-Authored-By: Claude Fable 5 --- app/Jobs/ProcessHqWhitelistJob.php | 2 +- .../Firewall/DirectAdminFirewallAnalyzer.php | 2 +- rector.php | 16 ++++++++++++++++ tests/Unit/Jobs/ProcessHqWhitelistJobTest.php | 11 ++++++++--- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/Jobs/ProcessHqWhitelistJob.php b/app/Jobs/ProcessHqWhitelistJob.php index e507354..b29251b 100644 --- a/app/Jobs/ProcessHqWhitelistJob.php +++ b/app/Jobs/ProcessHqWhitelistJob.php @@ -144,7 +144,7 @@ private function notifyAdmin(Host $hqHost, string $modsecLogs): void 'email' => $adminEmail, ]); - $ttl = (int) (config('unblock.hq.ttl') ?? 7200); + $ttl = (int) config('unblock.hq.ttl'); Mail::to($adminEmail)->send(new HqWhitelistMail( user: $adminUser, diff --git a/app/Services/Firewall/DirectAdminFirewallAnalyzer.php b/app/Services/Firewall/DirectAdminFirewallAnalyzer.php index f38ea97..c3ab146 100644 --- a/app/Services/Firewall/DirectAdminFirewallAnalyzer.php +++ b/app/Services/Firewall/DirectAdminFirewallAnalyzer.php @@ -489,7 +489,7 @@ private function checkAndRemoveFromBfmBlacklist(string $ip, string $sshKeyName): $this->firewallService->checkProblems($this->host, $sshKeyName, 'da_bfm_whitelist_add', $ip); // 3. Track in database for automatic removal after TTL - $ttl = (int) (config('unblock.hq.ttl') ?? 7200); // Default 2 hours + $ttl = (int) config('unblock.hq.ttl'); BfmWhitelistEntry::create([ 'host_id' => $this->host->id, 'ip_address' => $ip, diff --git a/rector.php b/rector.php index da8a120..ba2b4e9 100644 --- a/rector.php +++ b/rector.php @@ -11,6 +11,7 @@ use Rector\TypeDeclaration\Rector\StmtsAwareInterface\DeclareStrictTypesRector; use Rector\ValueObject\PhpVersion; use RectorLaravel\Rector\ClassMethod\MakeModelAttributesAndScopesProtectedRector; +use RectorLaravel\Rector\Coalesce\ApplyDefaultInsteadOfNullCoalesceRector; use RectorLaravel\Rector\FuncCall\{AppToResolveRector, RemoveDumpDataDeadCodeRector, ThrowIfAndThrowUnlessExceptionsToUseClassStringRector}; use RectorLaravel\Rector\If_\ThrowIfRector; use RectorLaravel\Set\LaravelSetList; @@ -111,6 +112,21 @@ // consumes scopes through the builder proxy. This app is final, not a // library: no external consumers exist. MakeModelAttributesAndScopesProtectedRector::class, + // Wave 6 (AID-542), split from AID-531 over the ??/?: lesson. The + // rule turns `helper('key') ?? $default` into `helper('key', + // $default)` — NOT ??→?:, but it still has a non-equivalent corner: + // a key present with an explicit null returns $default under ??, + // null under the parameter form. Both sites it flagged used + // `config('unblock.hq.ttl') ?? 7200`, where that corner is + // unreachable by construction (config/unblock.php casts `(int) + // env(..., 14000)`, so the key always exists as int) — which also + // made the 7200 a dead phantom default lying about the real 14000. + // The fallbacks were removed by hand (the default's single owner is + // the config file); the rule stays on as the guard against future + // `config('x') ?? $d`, which is always either dead or a duplicated + // default. If a legitimate present-but-null key ever appears, skip + // that call site here rather than dropping the rule. + ApplyDefaultInsteadOfNullCoalesceRector::class, ]) // Wave 4 (AID-532): LARAVEL_COLLECTION (4 sites, all element types are // scalars/plain arrays so toArray()≡all(); filter(!empty)→reject(empty) diff --git a/tests/Unit/Jobs/ProcessHqWhitelistJobTest.php b/tests/Unit/Jobs/ProcessHqWhitelistJobTest.php index 1217bc6..c96d04a 100644 --- a/tests/Unit/Jobs/ProcessHqWhitelistJobTest.php +++ b/tests/Unit/Jobs/ProcessHqWhitelistJobTest.php @@ -450,12 +450,17 @@ // SCENARIO 8: TTL Configuration // ============================================================================ -test('job uses default TTL when not configured', function () { +test('job uses the config-owned default TTL when env is not set', function () { $hqHost = Host::factory()->create(['hash' => 'test-key']); // ggignore config()->set('unblock.hq.host_id', $hqHost->id); config()->set('unblock.admin_email', 'admin@example.com'); - config()->set('unblock.hq.ttl', null); // ← Not configured + // "Not configured" means the HQ_WHITELIST_TTL env var is absent, in which + // case config/unblock.php resolves `(int) env(..., 14000)` at load time — + // the key always exists as an int, never null. The job carries no local + // fallback anymore (AID-542); the default's single owner is the config + // file, so this test pins that owner's value flowing through untouched. + expect(config('unblock.hq.ttl'))->toBe(14000); $firewallService = Mockery::mock(FirewallService::class); $firewallService->allows('generateSshKey')->andReturn('/tmp/key'); @@ -473,7 +478,7 @@ $job->handle($firewallService); Mail::assertQueued(HqWhitelistMail::class, function ($mail) { - return $mail->ttlSeconds === 7200; // Default 7200s + return $mail->ttlSeconds === 14000; // config/unblock.php default }); });