prehistoric-systems/coolify
1
0
Fork 0
mirror of https://github.com/coollabsio/coolify.git synced 2026-03-11 08:55:47 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

fix(s3): address Copilot review - trailing slashes and shell escaping

Browse source
This commit is contained in:
Karl Bauer 2025-12-26 16:56:25 +01:00
parent 062ad02e15
commit fa785572c5
4 changed files with 20 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
app/Jobs/DatabaseBackupJob.php
View file

@ -664,12 +664,17 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
// Build S3 path with optional prefix // Build S3 path with optional prefix
$s3Path = $bucket; $s3Path = $bucket;
if (filled($this->s3->path)) { 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 .= '/'.$pathPrefix;
} }
$s3Path .= $this->backup_dir.'/'; $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); instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true);
$this->s3_uploaded = true; $this->s3_uploaded = true;

6
app/Models/S3Storage.php
View file

@ -72,7 +72,11 @@ class S3Storage extends BaseModel
return null; 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;
} }
); );
} }

3
bootstrap/helpers/databases.php
View file

@ -211,7 +211,8 @@ function deleteBackupsS3(string|array|null $filenames, S3Storage $s3): void
// Apply path prefix if configured // Apply path prefix if configured
if (filled($s3->path)) { 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) { $filenames = array_map(function ($filename) use ($pathPrefix) {
// The filename is the backup path (e.g., /data/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 // We need to prepend the path prefix to match the S3 key structure

8
tests/Unit/S3StorageTest.php
View file

@ -99,9 +99,9 @@ test('S3Storage path attribute handles edge cases', function () {
$s3Storage->path = 'path//to///backup'; $s3Storage->path = 'path//to///backup';
expect($s3Storage->path)->toBe('/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/'; $s3Storage->path = 'backups/coolify/';
expect($s3Storage->path)->toBe('/backups/coolify/'); expect($s3Storage->path)->toBe('/backups/coolify');
// Only whitespace should return null // Only whitespace should return null
$s3Storage->path = ' '; $s3Storage->path = ' ';
@ -110,4 +110,8 @@ test('S3Storage path attribute handles edge cases', function () {
// Path with dots (valid single dots) // Path with dots (valid single dots)
$s3Storage->path = 'path.with.dots'; $s3Storage->path = 'path.with.dots';
expect($s3Storage->path)->toBe('/path.with.dots'); expect($s3Storage->path)->toBe('/path.with.dots');
// Only slashes should return null
$s3Storage->path = '///';
expect($s3Storage->path)->toBeNull();
}); });