From 297e9c41e19958f6237919794c28c3fb1d4cda32 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 20 Apr 2026 11:50:19 +0200 Subject: [PATCH] refactor(storage): tighten S3 endpoint URL validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse the existing SafeWebhookUrl rule on the S3 Storage endpoint field so the create and edit forms go through the same URL-normalization path as webhook settings. Adds a matching guard inside S3Storage::testConnection() so background callers (scheduled backups, database import reuse) also validate the endpoint before building the S3 client. Also fixes an IPv6-bracket edge case in SafeWebhookUrl so `http://[::1]` style hosts are normalized before the filter_var IP check — the rule's own loopback test was already asserting this behaviour. --- app/Livewire/Storage/Create.php | 4 +- app/Livewire/Storage/Form.php | 4 +- app/Models/S3Storage.php | 10 ++ app/Rules/SafeWebhookUrl.php | 10 +- .../Unit/S3StorageEndpointValidationTest.php | 91 +++++++++++++++++++ 5 files changed, 113 insertions(+), 6 deletions(-) create mode 100644 tests/Unit/S3StorageEndpointValidationTest.php diff --git a/app/Livewire/Storage/Create.php b/app/Livewire/Storage/Create.php index eda20342b..c3db34066 100644 --- a/app/Livewire/Storage/Create.php +++ b/app/Livewire/Storage/Create.php @@ -3,6 +3,7 @@ namespace App\Livewire\Storage; use App\Models\S3Storage; +use App\Rules\SafeWebhookUrl; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Uri; @@ -37,7 +38,7 @@ protected function rules(): array 'key' => 'required|max:255', 'secret' => 'required|max:255', 'bucket' => 'required|max:255', - 'endpoint' => 'required|url|max:255', + 'endpoint' => ['required', 'max:255', new SafeWebhookUrl], ]; } @@ -55,7 +56,6 @@ protected function messages(): array 'bucket.required' => 'The Bucket field is required.', 'bucket.max' => 'The Bucket may not be greater than 255 characters.', 'endpoint.required' => 'The Endpoint field is required.', - 'endpoint.url' => 'The Endpoint must be a valid URL.', 'endpoint.max' => 'The Endpoint may not be greater than 255 characters.', ] ); diff --git a/app/Livewire/Storage/Form.php b/app/Livewire/Storage/Form.php index 791226334..342d629cb 100644 --- a/app/Livewire/Storage/Form.php +++ b/app/Livewire/Storage/Form.php @@ -3,6 +3,7 @@ namespace App\Livewire\Storage; use App\Models\S3Storage; +use App\Rules\SafeWebhookUrl; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Facades\DB; @@ -42,7 +43,7 @@ protected function rules(): array 'key' => 'required|max:255', 'secret' => 'required|max:255', 'bucket' => 'required|max:255', - 'endpoint' => 'required|url|max:255', + 'endpoint' => ['required', 'max:255', new SafeWebhookUrl], ]; } @@ -60,7 +61,6 @@ protected function messages(): array 'bucket.required' => 'The Bucket field is required.', 'bucket.max' => 'The Bucket may not be greater than 255 characters.', 'endpoint.required' => 'The Endpoint field is required.', - 'endpoint.url' => 'The Endpoint must be a valid URL.', 'endpoint.max' => 'The Endpoint may not be greater than 255 characters.', ] ); diff --git a/app/Models/S3Storage.php b/app/Models/S3Storage.php index d6feccc7e..8a2e2b102 100644 --- a/app/Models/S3Storage.php +++ b/app/Models/S3Storage.php @@ -2,11 +2,13 @@ namespace App\Models; +use App\Rules\SafeWebhookUrl; use App\Traits\HasSafeStringAttribute; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Notifications\Messages\MailMessage; use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Facades\Validator; class S3Storage extends BaseModel { @@ -132,6 +134,14 @@ protected function region(): Attribute public function testConnection(bool $shouldSave = false) { try { + $validator = Validator::make( + ['endpoint' => $this['endpoint']], + ['endpoint' => ['required', new SafeWebhookUrl]], + ); + if ($validator->fails()) { + throw new \RuntimeException('S3 endpoint is not allowed: '.$validator->errors()->first('endpoint')); + } + $disk = Storage::build([ 'driver' => 's3', 'region' => $this['region'], diff --git a/app/Rules/SafeWebhookUrl.php b/app/Rules/SafeWebhookUrl.php index fbeb406af..3723e1db5 100644 --- a/app/Rules/SafeWebhookUrl.php +++ b/app/Rules/SafeWebhookUrl.php @@ -40,9 +40,15 @@ public function validate(string $attribute, mixed $value, Closure $fail): void $host = strtolower($host); + // Strip IPv6 brackets (e.g. "[::1]" -> "::1") before IP checks so bracketed + // literals can't sneak past filter_var FILTER_VALIDATE_IP. + $hostForIpCheck = (str_starts_with($host, '[') && str_ends_with($host, ']')) + ? substr($host, 1, -1) + : $host; + // Block well-known dangerous hostnames $blockedHosts = ['localhost', '0.0.0.0', '::1']; - if (in_array($host, $blockedHosts) || str_ends_with($host, '.internal')) { + if (in_array($hostForIpCheck, $blockedHosts) || str_ends_with($host, '.internal')) { Log::warning('Webhook URL points to blocked host', [ 'attribute' => $attribute, 'host' => $host, @@ -55,7 +61,7 @@ public function validate(string $attribute, mixed $value, Closure $fail): void } // Block loopback (127.0.0.0/8) and link-local/metadata (169.254.0.0/16) when IP is provided directly - if (filter_var($host, FILTER_VALIDATE_IP) && ($this->isLoopback($host) || $this->isLinkLocal($host))) { + if (filter_var($hostForIpCheck, FILTER_VALIDATE_IP) && ($this->isLoopback($hostForIpCheck) || $this->isLinkLocal($hostForIpCheck))) { Log::warning('Webhook URL points to blocked IP range', [ 'attribute' => $attribute, 'host' => $host, diff --git a/tests/Unit/S3StorageEndpointValidationTest.php b/tests/Unit/S3StorageEndpointValidationTest.php new file mode 100644 index 000000000..9ffba6a30 --- /dev/null +++ b/tests/Unit/S3StorageEndpointValidationTest.php @@ -0,0 +1,91 @@ + $endpoint], + ['endpoint' => ['required', 'max:255', new SafeWebhookUrl]], + ); + + expect($validator->fails())->toBeTrue("Expected rejection: {$endpoint}"); +})->with([ + 'AWS IMDS' => 'http://169.254.169.254/latest/meta-data/', + 'AWS IMDS bare' => 'http://169.254.169.254', + 'GCP metadata via link-local' => 'http://169.254.0.1', + 'loopback v4' => 'http://127.0.0.1', + 'loopback Redis' => 'http://127.0.0.1:6379', + 'loopback Postgres' => 'http://127.0.0.1:5432', + 'loopback alt in /8' => 'http://127.10.20.30', + 'zero address' => 'http://0.0.0.0', + 'IPv6 loopback' => 'http://[::1]', + 'localhost hostname' => 'http://localhost', + 'localhost with port' => 'http://localhost:9000', + 'internal suffix' => 'http://minio.internal', + 'file scheme' => 'file:///etc/passwd', + 'javascript scheme' => 'javascript:alert(1)', +]); + +it('accepts real-world S3 endpoints', function (string $endpoint) { + $validator = Validator::make( + ['endpoint' => $endpoint], + ['endpoint' => ['required', 'max:255', new SafeWebhookUrl]], + ); + + expect($validator->passes())->toBeTrue("Expected accepted: {$endpoint}"); +})->with([ + 'AWS S3' => 'https://s3.us-east-1.amazonaws.com', + 'Cloudflare R2' => 'https://fake.r2.cloudflarestorage.com', + 'DigitalOcean Spaces' => 'https://nyc3.digitaloceanspaces.com', + 'Backblaze B2' => 'https://s3.us-west-001.backblazeb2.com', + 'Self-hosted MinIO on 10.x' => 'http://10.0.0.5:9000', + 'Self-hosted MinIO on 172.16.x' => 'http://172.16.0.10:9000', + 'Self-hosted MinIO on 192.168.x' => 'http://192.168.1.50:9000', + 'Custom domain MinIO' => 'https://minio.example.com', +]); + +it('blocks testConnection() on an unsafe endpoint without issuing HTTP', function () { + $s3Storage = new S3Storage; + $s3Storage->setRawAttributes([ + 'region' => 'us-east-1', + 'key' => 'AKIAEXAMPLE', + 'secret' => 'secret', + 'bucket' => 'latest/meta-data', + 'endpoint' => 'http://169.254.169.254', + ]); + + expect(fn () => $s3Storage->testConnection()) + ->toThrow(RuntimeException::class, 'S3 endpoint is not allowed'); +}); + +it('blocks testConnection() for loopback endpoints', function (string $endpoint) { + $s3Storage = new S3Storage; + $s3Storage->setRawAttributes([ + 'region' => 'us-east-1', + 'key' => 'AKIAEXAMPLE', + 'secret' => 'secret', + 'bucket' => 'bucket', + 'endpoint' => $endpoint, + ]); + + expect(fn () => $s3Storage->testConnection()) + ->toThrow(RuntimeException::class, 'S3 endpoint is not allowed'); +})->with([ + 'http loopback' => 'http://127.0.0.1:6379', + 'localhost' => 'http://localhost:9000', + 'IPv6 loopback' => 'http://[::1]', + 'internal TLD' => 'http://backend.internal', +]);