From 830762f0ac6d61e5d182236a7e9a12cf88eb941b Mon Sep 17 00:00:00 2001 From: Karl Bauer Date: Fri, 26 Dec 2025 13:59:36 +0100 Subject: [PATCH 1/3] feat(s3): add path prefix support for S3 storage backups --- app/Jobs/DatabaseBackupJob.php | 11 ++++- app/Livewire/Storage/Form.php | 8 ++++ bootstrap/helpers/databases.php | 12 ++++++ ...6_000001_add_path_to_s3_storages_table.php | 28 +++++++++++++ .../views/livewire/storage/form.blade.php | 5 +++ tests/Unit/S3StorageTest.php | 40 +++++++++++++++++++ 6 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 database/migrations/2025_12_26_000001_add_path_to_s3_storages_table.php diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index a585baa69..ec995cafb 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -660,7 +660,16 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $escapedSecret = escapeshellarg($secret); $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}/"; + + // Build S3 path with optional prefix + $s3Path = $bucket; + if (filled($this->s3->path)) { + $pathPrefix = ltrim($this->s3->path, '/'); + $s3Path .= '/'.$pathPrefix; + } + $s3Path .= $this->backup_dir.'/'; + + $commands[] = "docker exec backup-of-{$this->backup_log_uuid} mc cp $this->backup_location temporary/{$s3Path}"; instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); $this->s3_uploaded = true; diff --git a/app/Livewire/Storage/Form.php b/app/Livewire/Storage/Form.php index d101d7b58..330aabaa4 100644 --- a/app/Livewire/Storage/Form.php +++ b/app/Livewire/Storage/Form.php @@ -29,6 +29,8 @@ class Form extends Component public string $secret; + public ?string $path = null; + public ?bool $isUsable = null; protected function rules(): array @@ -42,6 +44,7 @@ class Form extends Component 'secret' => 'required|max:255', 'bucket' => 'required|max:255', 'endpoint' => 'required|url|max:255', + 'path' => 'nullable|max:255|regex:/^[a-zA-Z0-9\/\-_\.]*$/', ]; } @@ -63,6 +66,8 @@ class Form extends Component '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.', + 'path.max' => 'The Path Prefix may not be greater than 255 characters.', + 'path.regex' => 'The Path Prefix may only contain letters, numbers, slashes (/), dashes (-), underscores (_), and dots (.).', ] ); } @@ -76,6 +81,7 @@ class Form extends Component 'secret' => 'Secret', 'bucket' => 'Bucket', 'endpoint' => 'Endpoint', + 'path' => 'Path Prefix', ]; /** @@ -94,6 +100,7 @@ class Form extends Component $this->storage->region = $this->region; $this->storage->key = $this->key; $this->storage->secret = $this->secret; + $this->storage->path = $this->path; $this->storage->is_usable = $this->isUsable; } else { // Sync FROM model (on load/refresh) @@ -104,6 +111,7 @@ class Form extends Component $this->region = $this->storage->region; $this->key = $this->storage->key; $this->secret = $this->storage->secret; + $this->path = $this->storage->path; $this->isUsable = $this->storage->is_usable; } } diff --git a/bootstrap/helpers/databases.php b/bootstrap/helpers/databases.php index 5df36db33..77b5c5f5b 100644 --- a/bootstrap/helpers/databases.php +++ b/bootstrap/helpers/databases.php @@ -209,6 +209,18 @@ function deleteBackupsS3(string|array|null $filenames, S3Storage $s3): void $filenames = [$filenames]; } + // Apply path prefix if configured + if (filled($s3->path)) { + $pathPrefix = ltrim($s3->path, '/'); + $filenames = array_map(function ($filename) use ($pathPrefix) { + // The filename is the local path (e.g., /var/lib/coolify/backups/databases/...) + // We need to prepend the path prefix to match the S3 key structure + $cleanFilename = ltrim($filename, '/'); + + return $pathPrefix.'/'.$cleanFilename; + }, $filenames); + } + $disk = Storage::build([ 'driver' => 's3', 'key' => $s3->key, diff --git a/database/migrations/2025_12_26_000001_add_path_to_s3_storages_table.php b/database/migrations/2025_12_26_000001_add_path_to_s3_storages_table.php new file mode 100644 index 000000000..92aedb977 --- /dev/null +++ b/database/migrations/2025_12_26_000001_add_path_to_s3_storages_table.php @@ -0,0 +1,28 @@ +string('path')->nullable()->after('endpoint'); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('s3_storages', function (Blueprint $table) { + $table->dropColumn('path'); + }); + } +}; diff --git a/resources/views/livewire/storage/form.blade.php b/resources/views/livewire/storage/form.blade.php index 850d7735f..6278adf93 100644 --- a/resources/views/livewire/storage/form.blade.php +++ b/resources/views/livewire/storage/form.blade.php @@ -40,6 +40,11 @@ +
+ +
diff --git a/tests/Unit/S3StorageTest.php b/tests/Unit/S3StorageTest.php index 6709f381d..cd80080a3 100644 --- a/tests/Unit/S3StorageTest.php +++ b/tests/Unit/S3StorageTest.php @@ -51,3 +51,43 @@ test('S3Storage model is guarded correctly', function () { // The model should have $guarded = [] which means everything is fillable expect($s3Storage->getGuarded())->toBe([]); }); + +test('S3Storage path attribute normalizes path correctly', function () { + $s3Storage = new S3Storage; + + // Path should be normalized to start with / + $s3Storage->path = 'backups/coolify'; + expect($s3Storage->path)->toBe('/backups/coolify'); + + // Path with leading slash should remain unchanged + $s3Storage->path = '/backups/coolify'; + expect($s3Storage->path)->toBe('/backups/coolify'); + + // Empty path should return null + $s3Storage->path = ''; + expect($s3Storage->path)->toBeNull(); + + // Null path should return null + $s3Storage->path = null; + expect($s3Storage->path)->toBeNull(); + + // Path with whitespace should be trimmed + $s3Storage->path = ' backups/coolify '; + expect($s3Storage->path)->toBe('/backups/coolify'); +}); + +test('S3Storage path attribute handles various path formats', function () { + $s3Storage = new S3Storage; + + // Simple path + $s3Storage->path = 'instance-1'; + expect($s3Storage->path)->toBe('/instance-1'); + + // Nested path + $s3Storage->path = 'production/backups/db'; + expect($s3Storage->path)->toBe('/production/backups/db'); + + // Path with special characters + $s3Storage->path = 'my-instance_2024.backups'; + expect($s3Storage->path)->toBe('/my-instance_2024.backups'); +}); From 062ad02e15f0a653485d37995516eac9693581c9 Mon Sep 17 00:00:00 2001 From: Karl Bauer Date: Fri, 26 Dec 2025 14:46:47 +0100 Subject: [PATCH 2/3] feat(s3): implemented changes for safety & clarity --- app/Livewire/Storage/Form.php | 3 ++- bootstrap/helpers/databases.php | 2 +- .../views/livewire/storage/form.blade.php | 4 ++-- tests/Unit/S3StorageTest.php | 20 +++++++++++++++++++ 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/app/Livewire/Storage/Form.php b/app/Livewire/Storage/Form.php index 330aabaa4..807bef0d4 100644 --- a/app/Livewire/Storage/Form.php +++ b/app/Livewire/Storage/Form.php @@ -44,7 +44,7 @@ class Form extends Component 'secret' => 'required|max:255', 'bucket' => 'required|max:255', 'endpoint' => 'required|url|max:255', - 'path' => 'nullable|max:255|regex:/^[a-zA-Z0-9\/\-_\.]*$/', + 'path' => ['nullable', 'max:255', 'regex:/^[a-zA-Z0-9\/\-_\.]*$/', 'not_regex:/\.\./'], ]; } @@ -68,6 +68,7 @@ class Form extends Component 'endpoint.max' => 'The Endpoint may not be greater than 255 characters.', 'path.max' => 'The Path Prefix may not be greater than 255 characters.', 'path.regex' => 'The Path Prefix may only contain letters, numbers, slashes (/), dashes (-), underscores (_), and dots (.).', + 'path.not_regex' => 'The Path Prefix may not contain directory traversal sequences (..).', ] ); } diff --git a/bootstrap/helpers/databases.php b/bootstrap/helpers/databases.php index 77b5c5f5b..1d22c364b 100644 --- a/bootstrap/helpers/databases.php +++ b/bootstrap/helpers/databases.php @@ -213,7 +213,7 @@ function deleteBackupsS3(string|array|null $filenames, S3Storage $s3): void if (filled($s3->path)) { $pathPrefix = ltrim($s3->path, '/'); $filenames = array_map(function ($filename) use ($pathPrefix) { - // The filename is the local path (e.g., /var/lib/coolify/backups/databases/...) + // The filename is the backup path (e.g., /data/coolify/backups/databases/...) // We need to prepend the path prefix to match the S3 key structure $cleanFilename = ltrim($filename, '/'); diff --git a/resources/views/livewire/storage/form.blade.php b/resources/views/livewire/storage/form.blade.php index 6278adf93..146c272e3 100644 --- a/resources/views/livewire/storage/form.blade.php +++ b/resources/views/livewire/storage/form.blade.php @@ -42,8 +42,8 @@
+ placeholder="e.g., production or instance-1" + helper="Optional prefix added before the standard backup path (/data/coolify/backups/...). Useful for separating multiple Coolify instances in a single bucket." />
path = 'my-instance_2024.backups'; expect($s3Storage->path)->toBe('/my-instance_2024.backups'); }); + +test('S3Storage path attribute handles edge cases', function () { + $s3Storage = new S3Storage; + + // Multiple consecutive slashes are preserved (validation should catch this) + $s3Storage->path = 'path//to///backup'; + expect($s3Storage->path)->toBe('/path//to///backup'); + + // Path ending with slash + $s3Storage->path = 'backups/coolify/'; + expect($s3Storage->path)->toBe('/backups/coolify/'); + + // Only whitespace should return null + $s3Storage->path = ' '; + expect($s3Storage->path)->toBeNull(); + + // Path with dots (valid single dots) + $s3Storage->path = 'path.with.dots'; + expect($s3Storage->path)->toBe('/path.with.dots'); +}); From fa785572c52dca6d69bd8df7d2a09642b89c0f9d Mon Sep 17 00:00:00 2001 From: Karl Bauer Date: Fri, 26 Dec 2025 16:56:25 +0100 Subject: [PATCH 3/3] fix(s3): address Copilot review - trailing slashes and shell escaping --- app/Jobs/DatabaseBackupJob.php | 9 +++++++-- app/Models/S3Storage.php | 6 +++++- bootstrap/helpers/databases.php | 3 ++- tests/Unit/S3StorageTest.php | 8 ++++++-- 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index ec995cafb..fa6fa9d78 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -664,12 +664,17 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue // Build S3 path with optional prefix $s3Path = $bucket; if (filled($this->s3->path)) { - $pathPrefix = ltrim($this->s3->path, '/'); + // Strip leading/trailing slashes to avoid double slashes in path + $pathPrefix = trim($this->s3->path, '/'); $s3Path .= '/'.$pathPrefix; } $s3Path .= $this->backup_dir.'/'; - $commands[] = "docker exec backup-of-{$this->backup_log_uuid} mc cp $this->backup_location temporary/{$s3Path}"; + // Escape the paths to prevent command injection + $escapedBackupLocation = escapeshellarg($this->backup_location); + $escapedS3Path = escapeshellarg("temporary/{$s3Path}"); + + $commands[] = "docker exec backup-of-{$this->backup_log_uuid} mc cp {$escapedBackupLocation} {$escapedS3Path}"; instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); $this->s3_uploaded = true; diff --git a/app/Models/S3Storage.php b/app/Models/S3Storage.php index 3aae55966..65bb73e32 100644 --- a/app/Models/S3Storage.php +++ b/app/Models/S3Storage.php @@ -72,7 +72,11 @@ class S3Storage extends BaseModel return null; } - return str($value)->trim()->start('/')->value(); + // Trim whitespace and normalize: add leading slash, remove trailing slash + $path = str($value)->trim()->start('/')->rtrim('/')->value(); + + // Return null if only slashes remain after trimming + return $path === '/' ? null : $path; } ); } diff --git a/bootstrap/helpers/databases.php b/bootstrap/helpers/databases.php index 1d22c364b..47c47eb0a 100644 --- a/bootstrap/helpers/databases.php +++ b/bootstrap/helpers/databases.php @@ -211,7 +211,8 @@ function deleteBackupsS3(string|array|null $filenames, S3Storage $s3): void // Apply path prefix if configured if (filled($s3->path)) { - $pathPrefix = ltrim($s3->path, '/'); + // Strip leading/trailing slashes to avoid double slashes in path + $pathPrefix = trim($s3->path, '/'); $filenames = array_map(function ($filename) use ($pathPrefix) { // The filename is the backup path (e.g., /data/coolify/backups/databases/...) // We need to prepend the path prefix to match the S3 key structure diff --git a/tests/Unit/S3StorageTest.php b/tests/Unit/S3StorageTest.php index 614576819..128d3250e 100644 --- a/tests/Unit/S3StorageTest.php +++ b/tests/Unit/S3StorageTest.php @@ -99,9 +99,9 @@ test('S3Storage path attribute handles edge cases', function () { $s3Storage->path = 'path//to///backup'; expect($s3Storage->path)->toBe('/path//to///backup'); - // Path ending with slash + // Path ending with slash - trailing slashes are now stripped $s3Storage->path = 'backups/coolify/'; - expect($s3Storage->path)->toBe('/backups/coolify/'); + expect($s3Storage->path)->toBe('/backups/coolify'); // Only whitespace should return null $s3Storage->path = ' '; @@ -110,4 +110,8 @@ test('S3Storage path attribute handles edge cases', function () { // Path with dots (valid single dots) $s3Storage->path = 'path.with.dots'; expect($s3Storage->path)->toBe('/path.with.dots'); + + // Only slashes should return null + $s3Storage->path = '///'; + expect($s3Storage->path)->toBeNull(); });