diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 79bf929be..9878e0a38 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -714,9 +714,11 @@ private function upload_to_s3(): void $escapedEndpoint = escapeshellarg($endpoint); $escapedKey = escapeshellarg($key); $escapedSecret = escapeshellarg($secret); + $escapedBackupLocation = escapeshellarg($this->backup_location); + $escapedS3Destination = escapeshellarg("temporary/{$bucket}{$this->backup_dir}/"); $commands[] = "docker exec backup-of-{$this->backup_log_uuid} mc alias set temporary {$escapedEndpoint} {$escapedKey} {$escapedSecret}"; - $commands[] = "docker exec backup-of-{$this->backup_log_uuid} mc cp $this->backup_location temporary/$bucket{$this->backup_dir}/"; + $commands[] = "docker exec backup-of-{$this->backup_log_uuid} mc cp {$escapedBackupLocation} {$escapedS3Destination}"; instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); $this->s3_uploaded = true; diff --git a/app/Livewire/Project/Database/ImportForm.php b/app/Livewire/Project/Database/ImportForm.php index 722daa6df..2f6bcb3b4 100644 --- a/app/Livewire/Project/Database/ImportForm.php +++ b/app/Livewire/Project/Database/ImportForm.php @@ -28,11 +28,10 @@ class ImportForm extends Component /** * Validate that a string is safe for use as an S3 bucket name. - * Allows alphanumerics, dots, dashes, and underscores. */ private function validateBucketName(string $bucket): bool { - return preg_match('/^[a-zA-Z0-9.\-_]+$/', $bucket) === 1; + return ValidationPatterns::isValidS3BucketName($bucket); } /** @@ -582,7 +581,7 @@ public function checkS3File() // Validate bucket name early if (! $this->validateBucketName($s3Storage->bucket)) { - $this->dispatch('error', 'Invalid S3 bucket name. Bucket name must contain only alphanumerics, dots, dashes, and underscores.'); + $this->dispatch('error', 'Invalid S3 bucket name. Bucket name must contain only lowercase letters, numbers, dots, and dashes, and must follow S3 bucket naming rules.'); return; } @@ -663,7 +662,7 @@ public function restoreFromS3(string $password = ''): bool|string // Validate bucket name to prevent command injection if (! $this->validateBucketName($bucket)) { - $this->dispatch('error', 'Invalid S3 bucket name. Bucket name must contain only alphanumerics, dots, dashes, and underscores.'); + $this->dispatch('error', 'Invalid S3 bucket name. Bucket name must contain only lowercase letters, numbers, dots, and dashes, and must follow S3 bucket naming rules.'); return true; } diff --git a/app/Livewire/Storage/Create.php b/app/Livewire/Storage/Create.php index c3db34066..64a6629f6 100644 --- a/app/Livewire/Storage/Create.php +++ b/app/Livewire/Storage/Create.php @@ -4,6 +4,7 @@ use App\Models\S3Storage; use App\Rules\SafeWebhookUrl; +use App\Rules\ValidS3BucketName; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Uri; @@ -37,7 +38,7 @@ protected function rules(): array 'region' => 'required|max:255', 'key' => 'required|max:255', 'secret' => 'required|max:255', - 'bucket' => 'required|max:255', + 'bucket' => ['required', new ValidS3BucketName], 'endpoint' => ['required', 'max:255', new SafeWebhookUrl], ]; } @@ -54,7 +55,6 @@ protected function messages(): array 'secret.required' => 'The Secret Key field is required.', 'secret.max' => 'The Secret Key may not be greater than 255 characters.', '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.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 b83b41e9f..d8f3ec93e 100644 --- a/app/Livewire/Storage/Form.php +++ b/app/Livewire/Storage/Form.php @@ -4,6 +4,7 @@ use App\Models\S3Storage; use App\Rules\SafeWebhookUrl; +use App\Rules\ValidS3BucketName; use App\Support\ValidationPatterns; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Facades\DB; @@ -44,7 +45,7 @@ protected function rules(): array 'region' => 'required|max:255', 'key' => 'required|max:255', 'secret' => 'required|max:255', - 'bucket' => 'required|max:255', + 'bucket' => ['required', new ValidS3BucketName], 'endpoint' => ['required', 'max:255', new SafeWebhookUrl], ]; } @@ -61,7 +62,6 @@ protected function messages(): array 'secret.required' => 'The Secret Key field is required.', 'secret.max' => 'The Secret Key may not be greater than 255 characters.', '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.max' => 'The Endpoint may not be greater than 255 characters.', ] diff --git a/app/Models/S3Storage.php b/app/Models/S3Storage.php index 3b344dfff..e4797531b 100644 --- a/app/Models/S3Storage.php +++ b/app/Models/S3Storage.php @@ -3,6 +3,7 @@ namespace App\Models; use App\Rules\SafeWebhookUrl; +use App\Rules\ValidS3BucketName; use App\Traits\HasSafeStringAttribute; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Factories\HasFactory; @@ -147,12 +148,22 @@ public function testConnection(bool $shouldSave = false) { try { $validator = Validator::make( - ['endpoint' => $this['endpoint']], - ['endpoint' => ['required', new SafeWebhookUrl]], + [ + 'endpoint' => $this['endpoint'], + 'bucket' => $this['bucket'], + ], + [ + 'endpoint' => ['required', new SafeWebhookUrl], + 'bucket' => ['required', new ValidS3BucketName], + ], ); - if ($validator->fails()) { + $validator->fails(); + if ($validator->errors()->has('endpoint')) { throw new \RuntimeException('S3 endpoint is not allowed: '.$validator->errors()->first('endpoint')); } + if ($validator->errors()->has('bucket')) { + throw new \RuntimeException('S3 bucket name is not allowed: '.$validator->errors()->first('bucket')); + } $disk = Storage::build([ 'driver' => 's3', diff --git a/app/Rules/ValidS3BucketName.php b/app/Rules/ValidS3BucketName.php new file mode 100644 index 000000000..bcfa430ef --- /dev/null +++ b/app/Rules/ValidS3BucketName.php @@ -0,0 +1,23 @@ +scheduledBackups()->count())->toBe(1); }); + +test('database backup job escapes the S3 copy destination argument', function () { + $source = file_get_contents(app_path('Jobs/DatabaseBackupJob.php')); + + expect($source)->toContain('$escapedS3Destination = escapeshellarg("temporary/{$bucket}{$this->backup_dir}/");') + ->and($source)->toContain('mc cp {$escapedBackupLocation} {$escapedS3Destination}') + ->and($source)->not->toContain('mc cp $this->backup_location temporary/$bucket{$this->backup_dir}/'); +}); diff --git a/tests/Unit/Project/Database/ImportCheckFileButtonTest.php b/tests/Unit/Project/Database/ImportCheckFileButtonTest.php index 8d8d29de8..2b816ff5e 100644 --- a/tests/Unit/Project/Database/ImportCheckFileButtonTest.php +++ b/tests/Unit/Project/Database/ImportCheckFileButtonTest.php @@ -37,10 +37,8 @@ // Valid bucket names expect($method->invoke($component, 'my-bucket'))->toBeTrue(); - expect($method->invoke($component, 'my_bucket'))->toBeTrue(); expect($method->invoke($component, 'mybucket123'))->toBeTrue(); expect($method->invoke($component, 'my.bucket.name'))->toBeTrue(); - expect($method->invoke($component, 'Bucket-Name_123'))->toBeTrue(); }); test('validateBucketName rejects invalid bucket names', function () { @@ -55,6 +53,9 @@ expect($method->invoke($component, 'bucket&ls'))->toBeFalse(); expect($method->invoke($component, "bucket\nid"))->toBeFalse(); expect($method->invoke($component, 'bucket name'))->toBeFalse(); // Space not allowed in bucket + expect($method->invoke($component, 'my_bucket'))->toBeFalse(); + expect($method->invoke($component, 'Bucket-Name'))->toBeFalse(); + expect($method->invoke($component, '192.168.1.1'))->toBeFalse(); }); test('validateS3Path accepts valid S3 paths', function () { diff --git a/tests/Unit/S3StorageTest.php b/tests/Unit/S3StorageTest.php index ddf390443..aaab76340 100644 --- a/tests/Unit/S3StorageTest.php +++ b/tests/Unit/S3StorageTest.php @@ -53,6 +53,7 @@ $s3Storage = new S3Storage; expect($s3Storage->getFillable())->toBe([ + 'team_id', 'name', 'description', 'region', @@ -118,3 +119,27 @@ expect($s3Storage->is_usable)->toBeFalse(); }); + +test('S3Storage testConnection rejects invalid bucket before building client', function (string $bucket) { + Storage::shouldReceive('build')->never(); + + $s3Storage = new S3Storage; + $s3Storage->setRawAttributes([ + 'name' => 'Test S3', + 'region' => 'us-east-1', + 'key' => 'AKIAEXAMPLE', + 'secret' => 'secret', + 'bucket' => $bucket, + 'endpoint' => 'https://s3.amazonaws.com', + ]); + + expect(fn () => $s3Storage->testConnection()) + ->toThrow(RuntimeException::class, 'S3 bucket name is not allowed'); +})->with([ + 'semicolon injection' => ['lab; id; #'], + 'command substitution' => ['lab$(id)'], + 'backticks' => ['lab`id`'], + 'newline' => ["lab\nid"], + 'underscore' => ['lab_bucket'], + 'uppercase' => ['LabBucket'], +]); diff --git a/tests/Unit/StorageBucketValidationRulesTest.php b/tests/Unit/StorageBucketValidationRulesTest.php new file mode 100644 index 000000000..d8e335f96 --- /dev/null +++ b/tests/Unit/StorageBucketValidationRulesTest.php @@ -0,0 +1,24 @@ +setAccessible(true); + + return $method->invoke($component); +} + +it('uses the shared S3 bucket rule in storage create and edit forms', function (string $componentClass) { + $bucketRules = storageRulesFor($componentClass)['bucket']; + + expect($bucketRules)->toContain('required') + ->and(collect($bucketRules)->contains(fn ($rule) => $rule instanceof ValidS3BucketName))->toBeTrue(); +})->with([ + 'create form' => [Create::class], + 'edit form' => [Form::class], +]); diff --git a/tests/Unit/ValidS3BucketNameTest.php b/tests/Unit/ValidS3BucketNameTest.php new file mode 100644 index 000000000..1a38c74c7 --- /dev/null +++ b/tests/Unit/ValidS3BucketNameTest.php @@ -0,0 +1,48 @@ +validate('bucket', $bucket, function () use (&$failed) { + $failed = true; + }); + + return ! $failed; +} + +it('accepts valid s3 bucket names', function (string $bucket) { + expect(validS3BucketNameRulePasses($bucket))->toBeTrue("Expected accepted: {$bucket}"); +})->with([ + 'short' => ['abc'], + 'simple' => ['coolify-backups'], + 'dots' => ['coolify.backups'], + 'digits' => ['backup-123'], + 'max length' => [str_repeat('a', 63)], +]); + +it('rejects invalid s3 bucket names and injection payloads', function (string $bucket) { + expect(validS3BucketNameRulePasses($bucket))->toBeFalse("Expected rejected: {$bucket}"); +})->with([ + 'too short' => ['ab'], + 'too long' => [str_repeat('a', 64)], + 'uppercase' => ['CoolifyBackups'], + 'underscore' => ['coolify_backups'], + 'leading hyphen' => ['-coolify-backups'], + 'trailing hyphen' => ['coolify-backups-'], + 'leading dot' => ['.coolify-backups'], + 'trailing dot' => ['coolify-backups.'], + 'consecutive dots' => ['coolify..backups'], + 'dot hyphen' => ['coolify.-backups'], + 'hyphen dot' => ['coolify-.backups'], + 'ipv4 address' => ['192.168.1.1'], + 'semicolon injection' => ['lab; id; #'], + 'command substitution' => ['lab$(id)'], + 'backticks' => ['lab`id`'], + 'pipe' => ['lab|id'], + 'ampersand' => ['lab&id'], + 'space' => ['lab bucket'], + 'newline' => ["lab\nid"], +]);