From a257224bb72d7e54d7148a9d509655a4e73a3adb Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 14 Aug 2026 13:34:00 +0200 Subject: [PATCH] fix(validation): improve private URL handling and error feedback Reject link-local targets even when allowlisted, link private-target errors to endpoint settings, and tighten persistent-volume table widths. --- app/Rules/SafeExternalUrl.php | 182 +----------------- app/Rules/SafeWebhookUrl.php | 30 ++- resources/css/app.css | 8 +- .../views/components/forms/input.blade.php | 13 +- .../PersistentStorageVolumesLayoutTest.php | 4 +- tests/Feature/ValidationErrorLinkTest.php | 21 ++ tests/Unit/SafeExternalUrlTest.php | 40 +++- tests/Unit/SafeWebhookUrlTest.php | 13 ++ 8 files changed, 126 insertions(+), 185 deletions(-) create mode 100644 tests/Feature/ValidationErrorLinkTest.php diff --git a/app/Rules/SafeExternalUrl.php b/app/Rules/SafeExternalUrl.php index 5380dd5e3..011e7ab2d 100644 --- a/app/Rules/SafeExternalUrl.php +++ b/app/Rules/SafeExternalUrl.php @@ -2,178 +2,10 @@ namespace App\Rules; -use Closure; -use Illuminate\Contracts\Validation\ValidationRule; -use Illuminate\Support\Facades\Log; - -class SafeExternalUrl implements ValidationRule -{ - /** - * @param (Closure(string): array)|null $resolver - */ - public function __construct(private ?Closure $resolver = null) {} - - /** - * Run the validation rule. - * - * Validates that a URL points to an external, publicly-routable host. - * Blocks private IP ranges, reserved ranges, localhost, and link-local - * addresses to prevent Server-Side Request Forgery (SSRF). - */ - public function validate(string $attribute, mixed $value, Closure $fail): void - { - if (! filter_var($value, FILTER_VALIDATE_URL)) { - $fail('The :attribute must be a valid URL.'); - - return; - } - - $scheme = strtolower(parse_url($value, PHP_URL_SCHEME) ?? ''); - if (! in_array($scheme, ['https', 'http'])) { - $fail('The :attribute must use the http or https scheme.'); - - return; - } - - $host = parse_url($value, PHP_URL_HOST); - if (! $host) { - $fail('The :attribute must contain a valid host.'); - - return; - } - - $host = strtolower($host); - $hostForIpCheck = $this->normalizeHostForIpCheck($host); - $hostForDns = rtrim($hostForIpCheck, '.'); - - $internalHosts = ['localhost', '0.0.0.0', '::1']; - if (in_array($hostForDns, $internalHosts, true) || str_ends_with($hostForDns, '.local') || str_ends_with($hostForDns, '.internal')) { - $this->logBlockedHost($attribute, $value, $host); - $fail('The :attribute must not point to internal hosts.'); - - return; - } - - if (filter_var($hostForIpCheck, FILTER_VALIDATE_IP)) { - if (! $this->isPublicIp($hostForIpCheck)) { - $this->logBlockedIp($attribute, $value, $host, $hostForIpCheck); - $fail('The :attribute must not point to a private or reserved IP address.'); - - return; - } - - return; - } - - $resolvedIps = $this->resolveHost($hostForDns); - if ($resolvedIps === []) { - $fail('The :attribute host could not be resolved.'); - - return; - } - - foreach ($resolvedIps as $resolvedIp) { - if (! $this->isPublicIp($resolvedIp)) { - $this->logBlockedIp($attribute, $value, $host, $resolvedIp); - $fail('The :attribute must not point to a private or reserved IP address.'); - - return; - } - } - } - - private function normalizeHostForIpCheck(string $host): string - { - return (str_starts_with($host, '[') && str_ends_with($host, ']')) - ? substr($host, 1, -1) - : $host; - } - - /** - * @return array - */ - private function resolveHost(string $host): array - { - if ($this->resolver instanceof Closure) { - return array_values(array_filter(($this->resolver)($host), fn (string $ip): bool => filter_var($ip, FILTER_VALIDATE_IP) !== false)); - } - - $records = @dns_get_record($host, DNS_A | DNS_AAAA); - if ($records === false) { - $records = []; - } - - $ips = []; - foreach ($records as $record) { - foreach (['ip', 'ipv6'] as $key) { - if (isset($record[$key]) && filter_var($record[$key], FILTER_VALIDATE_IP)) { - $ips[] = $record[$key]; - } - } - } - - $ipv4Addresses = @gethostbynamel($host); - if (is_array($ipv4Addresses)) { - foreach ($ipv4Addresses as $ip) { - if (filter_var($ip, FILTER_VALIDATE_IP)) { - $ips[] = $ip; - } - } - } - - return array_values(array_unique($ips)); - } - - private function isPublicIp(string $ip): bool - { - $embeddedIpv4 = $this->extractIpv4FromMappedIpv6($ip); - if ($embeddedIpv4 !== null) { - return filter_var($embeddedIpv4, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) !== false; - } - - return filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) !== false; - } - - private function extractIpv4FromMappedIpv6(string $ip): ?string - { - $packed = @inet_pton($ip); - if ($packed === false || strlen($packed) !== 16) { - return null; - } - - $prefix = substr($packed, 0, 12); - if ($prefix !== str_repeat("\0", 10)."\xff\xff") { - return null; - } - - $parts = unpack('C4', substr($packed, 12, 4)); - if ($parts === false) { - return null; - } - - return implode('.', $parts); - } - - private function logBlockedHost(string $attribute, string $url, string $host): void - { - Log::warning('External URL points to internal host', [ - 'attribute' => $attribute, - 'url' => $url, - 'host' => $host, - 'ip' => request()->ip(), - 'user_id' => auth()->id(), - ]); - } - - private function logBlockedIp(string $attribute, string $url, string $host, string $resolvedIp): void - { - Log::warning('External URL resolves to private or reserved IP', [ - 'attribute' => $attribute, - 'url' => $url, - 'host' => $host, - 'resolved_ip' => $resolvedIp, - 'ip' => request()->ip(), - 'user_id' => auth()->id(), - ]); - } -} +/** + * Backwards-compatible name for outbound URL validation. + * + * External service URLs use the same private-target allowlist as webhooks + * and S3 endpoints. + */ +class SafeExternalUrl extends SafeWebhookUrl {} diff --git a/app/Rules/SafeWebhookUrl.php b/app/Rules/SafeWebhookUrl.php index f95321d70..5a5911bee 100644 --- a/app/Rules/SafeWebhookUrl.php +++ b/app/Rules/SafeWebhookUrl.php @@ -62,7 +62,7 @@ class SafeWebhookUrl implements ValidationRule if ($this->isBlockedHostname($hostForDns) && ! $this->isAllowedHostname($hostForDns)) { $this->logBlockedHost($attribute, $host); - $fail('The :attribute must not point to localhost or internal hosts.'); + $fail($this->privateTargetMessage($attribute)); return; } @@ -70,7 +70,9 @@ class SafeWebhookUrl implements ValidationRule if (filter_var($hostForIpCheck, FILTER_VALIDATE_IP)) { if (! $this->isAllowedIp($hostForIpCheck, $hostForDns)) { $this->logBlockedIp($attribute, $host, $hostForIpCheck); - $fail('The :attribute must not point to private, reserved, loopback, or link-local addresses.'); + $fail($this->isLinkLocalIp($hostForIpCheck) + ? 'The :attribute must not point to link-local addresses.' + : $this->privateTargetMessage($attribute)); return; } @@ -88,13 +90,22 @@ class SafeWebhookUrl implements ValidationRule foreach ($resolvedIps as $resolvedIp) { if (! $this->isAllowedIp($resolvedIp, $hostForDns)) { $this->logBlockedIp($attribute, $host, $resolvedIp); - $fail('The :attribute must not point to private, reserved, loopback, or link-local addresses.'); + $fail($this->isLinkLocalIp($resolvedIp) + ? 'The :attribute must not resolve to a link-local address.' + : $this->privateTargetMessage($attribute)); return; } } } + private function privateTargetMessage(string $attribute): string + { + $settingsUrl = route('settings.advanced').'#endpoint-section'; + + return "The {$attribute} points to a local or private address that is not allowed. Configure allowed internal targets: {$settingsUrl}"; + } + /** * Build HTTP client options that pin the validated host to the resolved IPs. * @@ -334,6 +345,10 @@ class SafeWebhookUrl implements ValidationRule $ip = $embeddedIpv4; } + if ($this->isLinkLocalIp($ip)) { + return false; + } + if ($this->isPublicIp($ip)) { return true; } @@ -350,6 +365,15 @@ class SafeWebhookUrl implements ValidationRule return $this->isAllowlistedIp($ip); } + private function isLinkLocalIp(string $ip): bool + { + if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { + return $this->ipv4InCidr($ip, '169.254.0.0/16'); + } + + return $this->ipInCidr($ip, 'fe80::/10'); + } + private function isPublicIp(string $ip): bool { $embeddedIpv4 = $this->extractIpv4FromMappedIpv6($ip); diff --git a/resources/css/app.css b/resources/css/app.css index e0f5b1302..059044436 100644 --- a/resources/css/app.css +++ b/resources/css/app.css @@ -2470,11 +2470,11 @@ input[type="search"]::-webkit-search-results-decoration { } .volumes-table-grid { - grid-template-columns: minmax(10rem, 1.4fr) minmax(6rem, 1fr) minmax(6rem, 1fr) 5rem 17.5rem; + grid-template-columns: minmax(10rem, 1.4fr) minmax(6rem, 1fr) minmax(6rem, 1fr) 5rem 12rem; } .volumes-table-grid-with-pr { - grid-template-columns: minmax(9rem, 1.2fr) minmax(5.5rem, 0.85fr) minmax(5.5rem, 0.85fr) 9.25rem 5rem 17.5rem; + grid-template-columns: minmax(9rem, 1.2fr) minmax(5.5rem, 0.85fr) minmax(5.5rem, 0.85fr) 9.25rem 5rem 12rem; } .volumes-mobile-label { @@ -2510,7 +2510,7 @@ input[type="search"]::-webkit-search-results-decoration { @media (max-width: 1100px) { .volumes-table-grid { - grid-template-columns: minmax(9rem, 1.2fr) minmax(6rem, 1fr) 5rem 17.5rem; + grid-template-columns: minmax(9rem, 1.2fr) minmax(6rem, 1fr) 5rem 12rem; } .volumes-table-grid > .volumes-col-source, @@ -2519,7 +2519,7 @@ input[type="search"]::-webkit-search-results-decoration { } .volumes-table-grid-with-pr { - grid-template-columns: minmax(9rem, 1.1fr) minmax(6rem, 1fr) 8.5rem 5rem 17.5rem; + grid-template-columns: minmax(9rem, 1.1fr) minmax(6rem, 1fr) 8.5rem 5rem 12rem; } .volumes-table-grid-with-pr > .volumes-col-source, diff --git a/resources/views/components/forms/input.blade.php b/resources/views/components/forms/input.blade.php index f957fa660..2f9633164 100644 --- a/resources/views/components/forms/input.blade.php +++ b/resources/views/components/forms/input.blade.php @@ -63,7 +63,18 @@ @endif @error($modelBinding) @enderror diff --git a/tests/Feature/PersistentStorageVolumesLayoutTest.php b/tests/Feature/PersistentStorageVolumesLayoutTest.php index 3d609395a..b22d99891 100644 --- a/tests/Feature/PersistentStorageVolumesLayoutTest.php +++ b/tests/Feature/PersistentStorageVolumesLayoutTest.php @@ -197,7 +197,9 @@ it('renders volumes as a data table with shared column headers', function () { ->toContain('@media (max-width: 768px)') ->toContain('.table-badge-success'); - expect($css)->toContain('17.5rem'); + expect($css) + ->toContain('12rem') + ->not->toContain('17.5rem'); // Settings form labels are 13px (not Tailwind text-sm 14px). expect($css) diff --git a/tests/Feature/ValidationErrorLinkTest.php b/tests/Feature/ValidationErrorLinkTest.php new file mode 100644 index 000000000..0025b6518 --- /dev/null +++ b/tests/Feature/ValidationErrorLinkTest.php @@ -0,0 +1,21 @@ +put('default', new MessageBag([ + 'endpoint' => "Local or private IP addresses are not allowed. Configure allowed internal targets: {$settingsUrl}", + ])); + View::share('errors', $errors); + + $html = Blade::render(''); + + expect($html) + ->toContain('href="'.$settingsUrl.'"') + ->toContain('Set them here'); +}); diff --git a/tests/Unit/SafeExternalUrlTest.php b/tests/Unit/SafeExternalUrlTest.php index c047b1923..2298b4895 100644 --- a/tests/Unit/SafeExternalUrlTest.php +++ b/tests/Unit/SafeExternalUrlTest.php @@ -1,10 +1,38 @@ InstanceSettings::query()->updateOrCreate(['id' => 0], [ + 'webhook_allowed_internal_hosts' => ['192.168.1.0/24'], + ])); + + $validator = Validator::make( + ['url' => 'http://192.168.1.23/api/v4'], + ['url' => new SafeExternalUrl], + ); + + expect($validator->passes())->toBeTrue(); +}); + +it('rejects allowlisted link-local targets', function () { + InstanceSettings::unguarded(fn () => InstanceSettings::query()->updateOrCreate(['id' => 0], [ + 'webhook_allowed_internal_hosts' => ['169.254.0.0/16'], + ])); + + $validator = Validator::make( + ['url' => 'http://169.254.169.254/latest/meta-data'], + ['url' => new SafeExternalUrl], + ); + + expect($validator->fails())->toBeTrue(); +}); it('accepts valid public URLs', function () { $rule = new SafeExternalUrl; @@ -43,6 +71,16 @@ it('rejects private IPv4 addresses', function (string $url) { '192.168.x range' => 'http://192.168.1.1', ]); +it('links private target errors to the outbound endpoint settings', function () { + $validator = Validator::make( + ['url' => 'http://192.168.1.23'], + ['url' => new SafeExternalUrl], + ); + + expect($validator->errors()->first('url')) + ->toContain(route('settings.advanced').'#endpoint-section'); +}); + it('rejects cloud metadata IP', function () { $rule = new SafeExternalUrl; diff --git a/tests/Unit/SafeWebhookUrlTest.php b/tests/Unit/SafeWebhookUrlTest.php index 990b9488a..e0fc93943 100644 --- a/tests/Unit/SafeWebhookUrlTest.php +++ b/tests/Unit/SafeWebhookUrlTest.php @@ -50,6 +50,19 @@ it('rejects link-local range', function () { expect($validator->fails())->toBeTrue('Expected rejection: link-local IP'); }); +it('rejects link-local targets even when allowlisted', function () { + InstanceSettings::unguarded(fn () => InstanceSettings::query()->updateOrCreate(['id' => 0], [ + 'webhook_allowed_internal_hosts' => ['169.254.0.0/16'], + ])); + + $validator = Validator::make( + ['url' => 'http://169.254.169.254/latest/meta-data'], + ['url' => new SafeWebhookUrl], + ); + + expect($validator->fails())->toBeTrue(); +}); + it('rejects hostnames that resolve to blocked addresses', function (string $url, array $resolvedIps) { $rule = new SafeWebhookUrl(fn (string $host): array => $resolvedIps);