fix: improve s3 storage handling

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;

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;
             }

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.',
             ]

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.',
             ]

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',

app/Rules/ValidS3BucketName.php

@@ -0,0 +1,23 @@
+<?php
+
+namespace App\Rules;
+
+use App\Support\ValidationPatterns;
+use Closure;
+use Illuminate\Contracts\Validation\ValidationRule;
+use Illuminate\Translation\PotentiallyTranslatedString;
+
+class ValidS3BucketName implements ValidationRule
+{
+    /**
+     * Run the validation rule.
+     *
+     * @param  Closure(string, ?string=): PotentiallyTranslatedString  $fail
+     */
+    public function validate(string $attribute, mixed $value, Closure $fail): void
+    {
+        if (! is_string($value) || ! ValidationPatterns::isValidS3BucketName($value)) {
+            $fail('The :attribute must be a valid S3 bucket name: 3-63 lowercase letters, numbers, dots, or hyphens; start and end with a letter or number; no consecutive dots, dot-hyphen pairs, or IP address format.');
+        }
+    }
+}

app/Support/ValidationPatterns.php

@@ -93,6 +93,15 @@ class ValidationPatterns
      */
     public const DOCKER_NETWORK_PATTERN = '/^[a-zA-Z0-9][a-zA-Z0-9._-]*$/';
 
+    /**
+     * Pattern for S3 bucket names.
+     *
+     * Bucket names must be 3-63 lowercase characters, start and end with a
+     * letter or digit, and contain only lowercase letters, digits, dots, and
+     * hyphens. Additional semantic checks live in isValidS3BucketName().
+     */
+    public const S3_BUCKET_NAME_PATTERN = '/\A(?=.{3,63}\z)[a-z0-9][a-z0-9.-]*[a-z0-9]\z/';
+
     /**
      * Pattern for Docker-compatible environment variable keys.
      * Environment variable keys are later interpolated into shell commands as Docker build args, so only shell-safe identifier characters are allowed.
@@ -177,6 +186,22 @@ public static function isValidEnvironmentVariableKey(string $value): bool
         return preg_match(self::ENVIRONMENT_VARIABLE_KEY_PATTERN, $value) === 1;
     }
 
+    /**
+     * Check if a string is a valid S3 bucket name.
+     */
+    public static function isValidS3BucketName(string $value): bool
+    {
+        if (preg_match(self::S3_BUCKET_NAME_PATTERN, $value) !== 1) {
+            return false;
+        }
+
+        if (str_contains($value, '..') || str_contains($value, '.-') || str_contains($value, '-.')) {
+            return false;
+        }
+
+        return filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) === false;
+    }
+
     /**
      * Normalize and validate an environment variable key.
      */

tests/Feature/Jobs/DatabaseBackupJobTest.php

@@ -262,3 +262,11 @@
 
     expect($s3->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}/');
+});

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 () {

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'],
+]);

tests/Unit/StorageBucketValidationRulesTest.php

@@ -0,0 +1,24 @@
+<?php
+
+use App\Livewire\Storage\Create;
+use App\Livewire\Storage\Form;
+use App\Rules\ValidS3BucketName;
+
+function storageRulesFor(string $componentClass): array
+{
+    $component = new $componentClass;
+    $method = new ReflectionMethod($component, 'rules');
+    $method->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],
+]);

tests/Unit/ValidS3BucketNameTest.php

@@ -0,0 +1,48 @@
+<?php
+
+use App\Rules\ValidS3BucketName;
+
+function validS3BucketNameRulePasses(string $bucket): bool
+{
+    $failed = false;
+
+    (new ValidS3BucketName)->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"],
+]);