fix(server): handle limit edge case and IPv6 allowlist dedupe
Update server limit enforcement to re-enable force-disabled servers when the team is at or under its limit (`<= 0` condition). Improve allowlist validation and matching by: - supporting IPv6 CIDR mask ranges up to `/128` - adding IPv6-aware CIDR matching in `checkIPAgainstAllowlist` - normalizing/deduplicating redundant allowlist entries before saving Add feature tests for `ServerLimitCheckJob` covering under-limit, at-limit, over-limit, and no-op scenarios.
This commit is contained in:
parent
fb186841f4
commit
91f538e171
5 changed files with 192 additions and 17 deletions
|
|
@ -38,7 +38,7 @@ public function handle()
|
||||||
$server->forceDisableServer();
|
$server->forceDisableServer();
|
||||||
$this->team->notify(new ForceDisabled($server));
|
$this->team->notify(new ForceDisabled($server));
|
||||||
});
|
});
|
||||||
} elseif ($number_of_servers_to_disable === 0) {
|
} elseif ($number_of_servers_to_disable <= 0) {
|
||||||
$servers->each(function ($server) {
|
$servers->each(function ($server) {
|
||||||
if ($server->isForceDisabled()) {
|
if ($server->isForceDisabled()) {
|
||||||
$server->forceEnableServer();
|
$server->forceEnableServer();
|
||||||
|
|
|
||||||
|
|
@ -95,7 +95,9 @@ public function submit()
|
||||||
// Check if it's valid CIDR notation
|
// Check if it's valid CIDR notation
|
||||||
if (str_contains($entry, '/')) {
|
if (str_contains($entry, '/')) {
|
||||||
[$ip, $mask] = explode('/', $entry);
|
[$ip, $mask] = explode('/', $entry);
|
||||||
if (filter_var($ip, FILTER_VALIDATE_IP) && is_numeric($mask) && $mask >= 0 && $mask <= 32) {
|
$isIpv6 = filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false;
|
||||||
|
$maxMask = $isIpv6 ? 128 : 32;
|
||||||
|
if (filter_var($ip, FILTER_VALIDATE_IP) && is_numeric($mask) && $mask >= 0 && $mask <= $maxMask) {
|
||||||
return $entry;
|
return $entry;
|
||||||
}
|
}
|
||||||
$invalidEntries[] = $entry;
|
$invalidEntries[] = $entry;
|
||||||
|
|
@ -111,7 +113,7 @@ public function submit()
|
||||||
$invalidEntries[] = $entry;
|
$invalidEntries[] = $entry;
|
||||||
|
|
||||||
return null;
|
return null;
|
||||||
})->filter()->unique();
|
})->filter()->values()->all();
|
||||||
|
|
||||||
if (! empty($invalidEntries)) {
|
if (! empty($invalidEntries)) {
|
||||||
$this->dispatch('error', 'Invalid IP addresses or subnets: '.implode(', ', $invalidEntries));
|
$this->dispatch('error', 'Invalid IP addresses or subnets: '.implode(', ', $invalidEntries));
|
||||||
|
|
@ -119,13 +121,15 @@ public function submit()
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($validEntries->isEmpty()) {
|
if (empty($validEntries)) {
|
||||||
$this->dispatch('error', 'No valid IP addresses or subnets provided');
|
$this->dispatch('error', 'No valid IP addresses or subnets provided');
|
||||||
|
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
$this->allowed_ips = $validEntries->implode(',');
|
$validEntries = deduplicateAllowlist($validEntries);
|
||||||
|
|
||||||
|
$this->allowed_ips = implode(',', $validEntries);
|
||||||
}
|
}
|
||||||
|
|
||||||
$this->instantSave();
|
$this->instantSave();
|
||||||
|
|
|
||||||
|
|
@ -45,7 +45,10 @@ public function validate(string $attribute, mixed $value, Closure $fail): void
|
||||||
|
|
||||||
[$ip, $mask] = $parts;
|
[$ip, $mask] = $parts;
|
||||||
|
|
||||||
if (! filter_var($ip, FILTER_VALIDATE_IP) || ! is_numeric($mask) || $mask < 0 || $mask > 32) {
|
$isIpv6 = filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false;
|
||||||
|
$maxMask = $isIpv6 ? 128 : 32;
|
||||||
|
|
||||||
|
if (! filter_var($ip, FILTER_VALIDATE_IP) || ! is_numeric($mask) || $mask < 0 || $mask > $maxMask) {
|
||||||
$invalidEntries[] = $entry;
|
$invalidEntries[] = $entry;
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
|
|
|
||||||
|
|
@ -1416,24 +1416,48 @@ function checkIPAgainstAllowlist($ip, $allowlist)
|
||||||
}
|
}
|
||||||
|
|
||||||
$mask = (int) $mask;
|
$mask = (int) $mask;
|
||||||
|
$isIpv6Subnet = filter_var($subnet, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false;
|
||||||
|
$maxMask = $isIpv6Subnet ? 128 : 32;
|
||||||
|
|
||||||
// Validate mask
|
// Validate mask for address family
|
||||||
if ($mask < 0 || $mask > 32) {
|
if ($mask < 0 || $mask > $maxMask) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Calculate network addresses
|
if ($isIpv6Subnet) {
|
||||||
$ip_long = ip2long($ip);
|
// IPv6 CIDR matching using binary string comparison
|
||||||
$subnet_long = ip2long($subnet);
|
$ipBin = inet_pton($ip);
|
||||||
|
$subnetBin = inet_pton($subnet);
|
||||||
|
|
||||||
if ($ip_long === false || $subnet_long === false) {
|
if ($ipBin === false || $subnetBin === false) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
$mask_long = ~((1 << (32 - $mask)) - 1);
|
// Build a 128-bit mask from $mask prefix bits
|
||||||
|
$maskBin = str_repeat("\xff", (int) ($mask / 8));
|
||||||
|
$remainder = $mask % 8;
|
||||||
|
if ($remainder > 0) {
|
||||||
|
$maskBin .= chr(0xFF & (0xFF << (8 - $remainder)));
|
||||||
|
}
|
||||||
|
$maskBin = str_pad($maskBin, 16, "\x00");
|
||||||
|
|
||||||
if (($ip_long & $mask_long) == ($subnet_long & $mask_long)) {
|
if (($ipBin & $maskBin) === ($subnetBin & $maskBin)) {
|
||||||
return true;
|
return true;
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
// IPv4 CIDR matching
|
||||||
|
$ip_long = ip2long($ip);
|
||||||
|
$subnet_long = ip2long($subnet);
|
||||||
|
|
||||||
|
if ($ip_long === false || $subnet_long === false) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
$mask_long = ~((1 << (32 - $mask)) - 1);
|
||||||
|
|
||||||
|
if (($ip_long & $mask_long) == ($subnet_long & $mask_long)) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
// Special case: 0.0.0.0 means allow all
|
// Special case: 0.0.0.0 means allow all
|
||||||
|
|
@ -1451,6 +1475,67 @@ function checkIPAgainstAllowlist($ip, $allowlist)
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function deduplicateAllowlist(array $entries): array
|
||||||
|
{
|
||||||
|
if (count($entries) <= 1) {
|
||||||
|
return array_values($entries);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Normalize each entry into [original, ip, mask]
|
||||||
|
$parsed = [];
|
||||||
|
foreach ($entries as $entry) {
|
||||||
|
$entry = trim($entry);
|
||||||
|
if (empty($entry)) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($entry === '0.0.0.0') {
|
||||||
|
// Special case: bare 0.0.0.0 means "allow all" — treat as /0
|
||||||
|
$parsed[] = ['original' => $entry, 'ip' => '0.0.0.0', 'mask' => 0];
|
||||||
|
} elseif (str_contains($entry, '/')) {
|
||||||
|
[$ip, $mask] = explode('/', $entry);
|
||||||
|
$parsed[] = ['original' => $entry, 'ip' => $ip, 'mask' => (int) $mask];
|
||||||
|
} else {
|
||||||
|
$ip = $entry;
|
||||||
|
$isIpv6 = filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false;
|
||||||
|
$parsed[] = ['original' => $entry, 'ip' => $ip, 'mask' => $isIpv6 ? 128 : 32];
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
$count = count($parsed);
|
||||||
|
$redundant = array_fill(0, $count, false);
|
||||||
|
|
||||||
|
for ($i = 0; $i < $count; $i++) {
|
||||||
|
if ($redundant[$i]) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
for ($j = 0; $j < $count; $j++) {
|
||||||
|
if ($i === $j || $redundant[$j]) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Entry $j is redundant if its mask is narrower/equal (>=) than $i's mask
|
||||||
|
// AND $j's network IP falls within $i's CIDR range
|
||||||
|
if ($parsed[$j]['mask'] >= $parsed[$i]['mask']) {
|
||||||
|
$cidr = $parsed[$i]['ip'].'/'.$parsed[$i]['mask'];
|
||||||
|
if (checkIPAgainstAllowlist($parsed[$j]['ip'], [$cidr])) {
|
||||||
|
$redundant[$j] = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
$result = [];
|
||||||
|
for ($i = 0; $i < $count; $i++) {
|
||||||
|
if (! $redundant[$i]) {
|
||||||
|
$result[] = $parsed[$i]['original'];
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return $result;
|
||||||
|
}
|
||||||
|
|
||||||
function get_public_ips()
|
function get_public_ips()
|
||||||
{
|
{
|
||||||
try {
|
try {
|
||||||
|
|
|
||||||
83
tests/Feature/ServerLimitCheckJobTest.php
Normal file
83
tests/Feature/ServerLimitCheckJobTest.php
Normal file
|
|
@ -0,0 +1,83 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
use App\Jobs\ServerLimitCheckJob;
|
||||||
|
use App\Models\Server;
|
||||||
|
use App\Models\Team;
|
||||||
|
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||||
|
use Illuminate\Support\Facades\Notification;
|
||||||
|
|
||||||
|
uses(RefreshDatabase::class);
|
||||||
|
|
||||||
|
beforeEach(function () {
|
||||||
|
config()->set('constants.coolify.self_hosted', false);
|
||||||
|
|
||||||
|
Notification::fake();
|
||||||
|
|
||||||
|
$this->team = Team::factory()->create(['custom_server_limit' => 5]);
|
||||||
|
});
|
||||||
|
|
||||||
|
function createServerForTeam(Team $team, bool $forceDisabled = false): Server
|
||||||
|
{
|
||||||
|
$server = Server::factory()->create(['team_id' => $team->id]);
|
||||||
|
if ($forceDisabled) {
|
||||||
|
$server->settings()->update(['force_disabled' => true]);
|
||||||
|
}
|
||||||
|
|
||||||
|
return $server->fresh(['settings']);
|
||||||
|
}
|
||||||
|
|
||||||
|
it('re-enables force-disabled servers when under the limit', function () {
|
||||||
|
createServerForTeam($this->team);
|
||||||
|
$server2 = createServerForTeam($this->team, forceDisabled: true);
|
||||||
|
$server3 = createServerForTeam($this->team, forceDisabled: true);
|
||||||
|
|
||||||
|
expect($server2->settings->force_disabled)->toBeTruthy();
|
||||||
|
expect($server3->settings->force_disabled)->toBeTruthy();
|
||||||
|
|
||||||
|
// 3 servers, limit 5 → all should be re-enabled
|
||||||
|
ServerLimitCheckJob::dispatchSync($this->team);
|
||||||
|
|
||||||
|
expect($server2->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
expect($server3->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('re-enables force-disabled servers when exactly at the limit', function () {
|
||||||
|
$this->team->update(['custom_server_limit' => 3]);
|
||||||
|
|
||||||
|
createServerForTeam($this->team);
|
||||||
|
createServerForTeam($this->team);
|
||||||
|
$server3 = createServerForTeam($this->team, forceDisabled: true);
|
||||||
|
|
||||||
|
// 3 servers, limit 3 → disabled one should be re-enabled
|
||||||
|
ServerLimitCheckJob::dispatchSync($this->team);
|
||||||
|
|
||||||
|
expect($server3->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('disables newest servers when over the limit', function () {
|
||||||
|
$this->team->update(['custom_server_limit' => 2]);
|
||||||
|
|
||||||
|
$oldest = createServerForTeam($this->team);
|
||||||
|
sleep(1);
|
||||||
|
$middle = createServerForTeam($this->team);
|
||||||
|
sleep(1);
|
||||||
|
$newest = createServerForTeam($this->team);
|
||||||
|
|
||||||
|
// 3 servers, limit 2 → newest 1 should be disabled
|
||||||
|
ServerLimitCheckJob::dispatchSync($this->team);
|
||||||
|
|
||||||
|
expect($oldest->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
expect($middle->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
expect($newest->fresh()->settings->force_disabled)->toBeTruthy();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not change servers when under limit and none are force-disabled', function () {
|
||||||
|
$server1 = createServerForTeam($this->team);
|
||||||
|
$server2 = createServerForTeam($this->team);
|
||||||
|
|
||||||
|
// 2 servers, limit 5 → nothing to do
|
||||||
|
ServerLimitCheckJob::dispatchSync($this->team);
|
||||||
|
|
||||||
|
expect($server1->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
expect($server2->fresh()->settings->force_disabled)->toBeFalsy();
|
||||||
|
});
|
||||||
Loading…
Reference in a new issue