diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 3bc86df903..27fefb33ef 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -21,6 +21,7 @@ use App\Rules\SafeWebhookUrl; use App\Services\ScheduledJobDeliveryService; use App\Support\BackupCompression; use App\Support\ClickhouseBackupCommand; +use App\Support\ValidationPatterns; use Carbon\Carbon; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldBeEncrypted; @@ -143,10 +144,14 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $databaseType = $this->database->databaseType(); $serviceUuid = $this->database->service->uuid; $serviceName = str($this->database->service->name)->slug(); + $this->container_name = "{$this->database->name}-$serviceUuid"; + if (! ValidationPatterns::isValidContainerName($this->container_name)) { + throw new \Exception('Invalid database container name.'); + } + $this->directory_name = $serviceName.'-'.str($this->database->name)->slug().'-'.$serviceUuid; + $escapedContainerName = escapeshellarg($this->container_name); if (str($databaseType)->contains('postgres')) { - $this->container_name = "{$this->database->name}-$serviceUuid"; - $this->directory_name = $serviceName.'-'.$this->container_name; - $commands[] = "docker exec $this->container_name env | grep POSTGRES_"; + $commands[] = "docker exec {$escapedContainerName} env | grep POSTGRES_"; $envs = instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); $envs = str($envs)->explode("\n"); @@ -175,9 +180,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $this->postgres_password = str($this->postgres_password)->after('POSTGRES_PASSWORD=')->value(); } } elseif (str($databaseType)->contains('mysql')) { - $this->container_name = "{$this->database->name}-$serviceUuid"; - $this->directory_name = $serviceName.'-'.$this->container_name; - $commands[] = "docker exec $this->container_name env | grep MYSQL_"; + $commands[] = "docker exec {$escapedContainerName} env | grep MYSQL_"; $envs = instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); $envs = str($envs)->explode("\n"); @@ -198,9 +201,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue throw new \Exception('MYSQL_DATABASE not found'); } } elseif (str($databaseType)->contains('mariadb')) { - $this->container_name = "{$this->database->name}-$serviceUuid"; - $this->directory_name = $serviceName.'-'.$this->container_name; - $commands[] = "docker exec $this->container_name env"; + $commands[] = "docker exec {$escapedContainerName} env"; $envs = instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); $envs = str($envs)->explode("\n"); $rootPassword = $envs->filter(function ($env) { @@ -236,13 +237,11 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue } } elseif (str($databaseType)->contains('mongo')) { $databasesToBackup = ['*']; - $this->container_name = "{$this->database->name}-$serviceUuid"; - $this->directory_name = $serviceName.'-'.$this->container_name; // Try to extract MongoDB credentials from environment variables try { $commands = []; - $commands[] = "docker exec $this->container_name env | grep MONGO_INITDB_"; + $commands[] = "docker exec {$escapedContainerName} env | grep MONGO_INITDB_"; $envs = instant_remote_process($commands, $this->server, true, false, null, disableMultiplexing: true); if (filled($envs)) { @@ -317,7 +316,8 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue // Step 1: Create local backup try { if (str($databaseType)->contains('postgres')) { - $this->backup_file = "/pg-dump-$database-".Carbon::now()->timestamp.'.dmp'; + $fileDatabaseName = $this->backupFilenamePart($database); + $this->backup_file = "/pg-dump-$fileDatabaseName-".Carbon::now()->timestamp.'.dmp'; if ($this->backup->dump_all) { $this->backup_file = '/pg-dump-all-'.Carbon::now()->timestamp.'.gz'; } @@ -342,7 +342,8 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $databaseName = $database; } } - $this->backup_file = "/mongo-dump-$databaseName-".Carbon::now()->timestamp.'.tar.gz'; + $fileDatabaseName = $this->backupFilenamePart($databaseName); + $this->backup_file = "/mongo-dump-$fileDatabaseName-".Carbon::now()->timestamp.'.tar.gz'; $this->backup_location = $this->backup_dir.$this->backup_file; $this->backup_log = ScheduledDatabaseBackupExecution::create([ 'uuid' => $this->backup_log_uuid, @@ -354,7 +355,8 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue BackupCreated::dispatch($this->team->id); $this->backup_standalone_mongodb($database); } elseif (str($databaseType)->contains('mysql')) { - $this->backup_file = "/mysql-dump-$database-".Carbon::now()->timestamp.'.dmp'; + $fileDatabaseName = $this->backupFilenamePart($database); + $this->backup_file = "/mysql-dump-$fileDatabaseName-".Carbon::now()->timestamp.'.dmp'; if ($this->backup->dump_all) { $this->backup_file = '/mysql-dump-all-'.Carbon::now()->timestamp.'.gz'; } @@ -369,7 +371,8 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue BackupCreated::dispatch($this->team->id); $this->backup_standalone_mysql($database); } elseif (str($databaseType)->contains('mariadb')) { - $this->backup_file = "/mariadb-dump-$database-".Carbon::now()->timestamp.'.dmp'; + $fileDatabaseName = $this->backupFilenamePart($database); + $this->backup_file = "/mariadb-dump-$fileDatabaseName-".Carbon::now()->timestamp.'.dmp'; if ($this->backup->dump_all) { $this->backup_file = '/mariadb-dump-all-'.Carbon::now()->timestamp.'.gz'; } @@ -521,6 +524,11 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue } } + private function backupFilenamePart(string $database): string + { + return preg_replace('/[^A-Za-z0-9_.-]/', '-', $database); + } + private function backup_standalone_mongodb(string $databaseWithCollections): void { try { @@ -540,12 +548,15 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue } Log::info('MongoDB backup URL configured', ['has_url' => filled($url), 'using_env_vars' => blank($this->database->internal_db_url)]); $escapedUrl = escapeshellarg($url); + $escapedContainerName = escapeshellarg($this->container_name); + $escapedBackupDir = escapeshellarg($this->backup_dir); + $escapedBackupLocation = escapeshellarg($this->backup_location); if ($databaseWithCollections === 'all') { - $commands[] = 'mkdir -p '.$this->backup_dir; + $commands[] = 'mkdir -p '.$escapedBackupDir; if (str($this->database->image)->startsWith('mongo:4')) { - $commands[] = "docker exec $this->container_name mongodump --uri=$escapedUrl --gzip --archive > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mongodump --uri=$escapedUrl --gzip --archive > {$escapedBackupLocation}"; } else { - $commands[] = "docker exec $this->container_name mongodump --authenticationDatabase=admin --uri=$escapedUrl --gzip --archive > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mongodump --authenticationDatabase=admin --uri=$escapedUrl --gzip --archive > {$escapedBackupLocation}"; } } else { if (str($databaseWithCollections)->contains(':')) { @@ -555,7 +566,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $databaseName = $databaseWithCollections; $collectionsToExclude = collect(); } - $commands[] = 'mkdir -p '.$this->backup_dir; + $commands[] = 'mkdir -p '.$escapedBackupDir; // Validate and escape database name to prevent command injection validateShellSafePath($databaseName, 'database name'); @@ -563,9 +574,9 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue if ($collectionsToExclude->count() === 0) { if (str($this->database->image)->startsWith('mongo:4')) { - $commands[] = "docker exec $this->container_name mongodump --uri=$escapedUrl --gzip --archive > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mongodump --uri=$escapedUrl --gzip --archive > {$escapedBackupLocation}"; } else { - $commands[] = "docker exec $this->container_name mongodump --authenticationDatabase=admin --uri=$escapedUrl --db $escapedDatabaseName --gzip --archive > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mongodump --authenticationDatabase=admin --uri=$escapedUrl --db $escapedDatabaseName --gzip --archive > {$escapedBackupLocation}"; } } else { // Validate and escape each collection name @@ -577,9 +588,9 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue }); if (str($this->database->image)->startsWith('mongo:4')) { - $commands[] = "docker exec $this->container_name mongodump --uri=$escapedUrl --gzip --excludeCollection ".$escapedCollections->implode(' --excludeCollection ')." --archive > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mongodump --uri=$escapedUrl --gzip --excludeCollection ".$escapedCollections->implode(' --excludeCollection ')." --archive > {$escapedBackupLocation}"; } else { - $commands[] = "docker exec $this->container_name mongodump --authenticationDatabase=admin --uri=$escapedUrl --db $escapedDatabaseName --gzip --excludeCollection ".$escapedCollections->implode(' --excludeCollection ')." --archive > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mongodump --authenticationDatabase=admin --uri=$escapedUrl --db $escapedDatabaseName --gzip --excludeCollection ".$escapedCollections->implode(' --excludeCollection ')." --archive > {$escapedBackupLocation}"; } } } @@ -621,20 +632,22 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function backup_standalone_postgresql(string $database): void { try { - $commands[] = 'mkdir -p '.$this->backup_dir; + $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); $backupCommand = 'docker exec'; if ($this->postgres_password) { $backupCommand .= ' -e PGPASSWORD='.escapeshellarg($this->postgres_password); } $escapedUsername = escapeshellarg($this->database->postgres_user); + $escapedContainerName = escapeshellarg($this->container_name); + $escapedBackupLocation = escapeshellarg($this->backup_location); if ($this->backup->dump_all) { - $backupCommand .= " $this->container_name pg_dumpall --username $escapedUsername"; - $backupCommand = $this->buildCompressedDumpCommand($backupCommand).' > '.escapeshellarg($this->backup_location); + $backupCommand .= " {$escapedContainerName} pg_dumpall --username $escapedUsername"; + $backupCommand = $this->buildCompressedDumpCommand($backupCommand).' > '.$escapedBackupLocation; } else { // Validate and escape database name to prevent command injection validateShellSafePath($database, 'database name'); $escapedDatabase = escapeshellarg($database); - $backupCommand .= " $this->container_name pg_dump --format=custom --no-acl --no-owner --username $escapedUsername $escapedDatabase > $this->backup_location"; + $backupCommand .= " {$escapedContainerName} pg_dump --format=custom --no-acl --no-owner --username $escapedUsername $escapedDatabase > {$escapedBackupLocation}"; } $commands[] = $backupCommand; @@ -652,16 +665,18 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function backup_standalone_mysql(string $database): void { try { - $commands[] = 'mkdir -p '.$this->backup_dir; + $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); $escapedPassword = escapeshellarg($this->database->mysql_root_password); + $escapedContainerName = escapeshellarg($this->container_name); + $escapedBackupLocation = escapeshellarg($this->backup_location); if ($this->backup->dump_all) { - $dumpCommand = "docker exec $this->container_name mysqldump -u root -p$escapedPassword --all-databases --single-transaction --quick --lock-tables=false"; - $commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.escapeshellarg($this->backup_location); + $dumpCommand = "docker exec {$escapedContainerName} mysqldump -u root -p$escapedPassword --all-databases --single-transaction --quick --lock-tables=false"; + $commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.$escapedBackupLocation; } else { // Validate and escape database name to prevent command injection validateShellSafePath($database, 'database name'); $escapedDatabase = escapeshellarg($database); - $commands[] = "docker exec $this->container_name mysqldump -u root -p$escapedPassword $escapedDatabase > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mysqldump -u root -p$escapedPassword $escapedDatabase > {$escapedBackupLocation}"; } $this->backup_output = instant_remote_process($commands, $this->server, true, false, $this->timeout, disableMultiplexing: true); $this->backup_output = trim($this->backup_output); @@ -677,16 +692,18 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function backup_standalone_mariadb(string $database): void { try { - $commands[] = 'mkdir -p '.$this->backup_dir; + $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); $escapedPassword = escapeshellarg($this->database->mariadb_root_password); + $escapedContainerName = escapeshellarg($this->container_name); + $escapedBackupLocation = escapeshellarg($this->backup_location); if ($this->backup->dump_all) { - $dumpCommand = "docker exec $this->container_name mariadb-dump -u root -p$escapedPassword --all-databases --single-transaction --quick --lock-tables=false"; - $commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.escapeshellarg($this->backup_location); + $dumpCommand = "docker exec {$escapedContainerName} mariadb-dump -u root -p$escapedPassword --all-databases --single-transaction --quick --lock-tables=false"; + $commands[] = $this->buildCompressedDumpCommand($dumpCommand).' > '.$escapedBackupLocation; } else { // Validate and escape database name to prevent command injection validateShellSafePath($database, 'database name'); $escapedDatabase = escapeshellarg($database); - $commands[] = "docker exec $this->container_name mariadb-dump -u root -p$escapedPassword $escapedDatabase > $this->backup_location"; + $commands[] = "docker exec {$escapedContainerName} mariadb-dump -u root -p$escapedPassword $escapedDatabase > {$escapedBackupLocation}"; } $this->backup_output = instant_remote_process($commands, $this->server, true, false, $this->timeout, disableMultiplexing: true); $this->backup_output = trim($this->backup_output); @@ -745,7 +762,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function calculate_size() { - return instant_remote_process(["du -b $this->backup_location | cut -f1"], $this->server, false, false, null, disableMultiplexing: true); + return instant_remote_process(['du -b '.escapeshellarg($this->backup_location).' | cut -f1'], $this->server, false, false, null, disableMultiplexing: true); } private function upload_to_s3(): void @@ -785,14 +802,14 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue if (isDev()) { if ($this->database->name === 'coolify-db') { $backup_location_from = '/var/lib/docker/volumes/coolify_dev_backups_data/_data/coolify/coolify-db-'.$this->server->ip.$this->backup_file; - $commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm -v $backup_location_from:$this->backup_location:ro {$fullImageName}"; } else { $backup_location_from = '/var/lib/docker/volumes/coolify_dev_backups_data/_data/databases/'.str($this->team->name)->slug().'-'.$this->team->id.'/'.$this->directory_name.$this->backup_file; - $commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm -v $backup_location_from:$this->backup_location:ro {$fullImageName}"; } } else { - $commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm -v $this->backup_location:$this->backup_location:ro {$fullImageName}"; + $backup_location_from = $this->backup_location; } + $mount = escapeshellarg($backup_location_from.':'.$this->backup_location.':ro'); + $commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm -v {$mount} {$fullImageName}"; // Escape S3 credentials to prevent command injection $escapedEndpoint = escapeshellarg($endpoint); diff --git a/app/Models/ServiceApplication.php b/app/Models/ServiceApplication.php index 066a86fac8..6b0b4a8dea 100644 --- a/app/Models/ServiceApplication.php +++ b/app/Models/ServiceApplication.php @@ -88,7 +88,7 @@ class ServiceApplication extends BaseModel public function restart() { $container_id = $this->name.'-'.$this->service->uuid; - instant_remote_process(["docker restart {$container_id}"], $this->service->server); + instant_remote_process(['docker restart '.escapeshellarg($container_id)], $this->service->server); } public static function ownedByCurrentTeamAPI(int $teamId) diff --git a/app/Models/ServiceDatabase.php b/app/Models/ServiceDatabase.php index 603d11a7f3..959c3a69c5 100644 --- a/app/Models/ServiceDatabase.php +++ b/app/Models/ServiceDatabase.php @@ -84,7 +84,7 @@ class ServiceDatabase extends BaseModel public function restart() { $container_id = $this->name.'-'.$this->service->uuid; - remote_process(["docker restart {$container_id}"], $this->service->server); + remote_process(['docker restart '.escapeshellarg($container_id)], $this->service->server); } public function isRunning() diff --git a/app/Support/ValidationPatterns.php b/app/Support/ValidationPatterns.php index 442f892b26..4f5a77a140 100644 --- a/app/Support/ValidationPatterns.php +++ b/app/Support/ValidationPatterns.php @@ -84,7 +84,7 @@ class ValidationPatterns * Pattern for Docker container names * Must start with alphanumeric, followed by alphanumeric, dots, hyphens, or underscores */ - public const CONTAINER_NAME_PATTERN = '/^[a-zA-Z0-9][a-zA-Z0-9._-]*$/'; + public const CONTAINER_NAME_PATTERN = '/\A[a-zA-Z0-9][a-zA-Z0-9._-]*\z/'; /** * Pattern for Docker network names diff --git a/tests/Feature/CreateScheduledBackupValidationTest.php b/tests/Feature/CreateScheduledBackupValidationTest.php index a96e776a05..507aad1dc3 100644 --- a/tests/Feature/CreateScheduledBackupValidationTest.php +++ b/tests/Feature/CreateScheduledBackupValidationTest.php @@ -1,5 +1,6 @@ and($backup->s3_storage_id)->toBeNull(); }); +it('rejects an unsupported service database name during backup', function () { + $service = Service::factory()->create([ + 'server_id' => $this->server->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + 'environment_id' => $this->environment->id, + ]); + $database = ServiceDatabase::create([ + 'service_id' => $service->id, + 'name' => 'postgres test', + 'image' => 'postgres:16-alpine', + 'custom_type' => 'postgresql', + 'status' => 'running', + ]); + $backup = ScheduledDatabaseBackup::create([ + 'frequency' => '0 0 * * *', + 'save_s3' => false, + 'database_type' => ServiceDatabase::class, + 'database_id' => $database->id, + 'team_id' => $this->team->id, + ]); + + expect(fn () => (new DatabaseBackupJob($backup))->handle()) + ->toThrow(Exception::class, 'Invalid database container name.'); +}); + it('selects a service database when creating a backup from the unified backups page', function () { $service = Service::factory()->create([ 'server_id' => $this->server->id, diff --git a/tests/Unit/DatabaseBackupSecurityTest.php b/tests/Unit/DatabaseBackupSecurityTest.php index 81075be5b9..f3df178d91 100644 --- a/tests/Unit/DatabaseBackupSecurityTest.php +++ b/tests/Unit/DatabaseBackupSecurityTest.php @@ -1,5 +1,8 @@ toBeGreaterThanOrEqual(2); }); + +test('backup shell commands do not interpolate raw container names or backup paths', function () { + $source = file_get_contents(__DIR__.'/../../app/Jobs/DatabaseBackupJob.php'); + preg_match_all('/^.*(?:docker exec|mkdir -p |du -b |docker run -d).*$/m', $source, $matches); + + expect($matches[0])->not->toBeEmpty(); + foreach ($matches[0] as $command) { + $quotedArgumentsRemoved = preg_replace('/escapeshellarg\(\$this->(?:container_name|backup_dir|backup_location)\)/', '', $command); + expect($quotedArgumentsRemoved)->not->toMatch('/\$this->(?:container_name|backup_dir|backup_location)\b/'); + } +}); + +test('backup job accepts supported service container names only', function () { + expect(ValidationPatterns::isValidContainerName('db-name_service.1-uuid'))->toBeTrue() + ->and(ValidationPatterns::isValidContainerName('db name-uuid'))->toBeFalse() + ->and(ValidationPatterns::isValidContainerName("db\nname-uuid"))->toBeFalse() + ->and(ValidationPatterns::isValidContainerName("db-uuid\n"))->toBeFalse(); + + $source = file_get_contents(__DIR__.'/../../app/Jobs/DatabaseBackupJob.php'); + expect($source)->toContain('ValidationPatterns::isValidContainerName($this->container_name)'); +}); + +test('service restart commands do not interpolate raw container names', function () { + foreach (['ServiceDatabase.php', 'ServiceApplication.php'] as $model) { + $source = file_get_contents(__DIR__.'/../../app/Models/'.$model); + expect($source)->not->toContain('docker restart {$container_id}'); + } +}); + +test('backup filenames remove shell and path separators from database names', function () { + $job = (new ReflectionClass(DatabaseBackupJob::class))->newInstanceWithoutConstructor(); + $filenamePart = (new ReflectionClass($job))->getMethod('backupFilenamePart'); + + expect($filenamePart->invoke($job, 'safe_db-1'))->toBe('safe_db-1') + ->and($filenamePart->invoke($job, '../db name'))->toBe('..-db-name'); + + $source = file_get_contents(__DIR__.'/../../app/Jobs/DatabaseBackupJob.php'); + expect(substr_count($source, '$this->backupFilenamePart('))->toBe(4); +}); diff --git a/tests/Unit/ParseCommandsByLineForSudoTest.php b/tests/Unit/ParseCommandsByLineForSudoTest.php index 93c9f8b643..d9ffa8c7da 100644 --- a/tests/Unit/ParseCommandsByLineForSudoTest.php +++ b/tests/Unit/ParseCommandsByLineForSudoTest.php @@ -43,6 +43,16 @@ test('preserves command substitutions inside database and volume backup scripts' ->not->toContain('$(sudo if'); }); +test('preserves quoted backup container and file arguments for a non-root server', function () { + $container = escapeshellarg('db-name-uuid'); + $path = escapeshellarg('/backups/db-name.dump'); + $command = "docker exec {$container} pg_dump --username 'postgres' 'app' > {$path}"; + + $result = parseCommandsByLineForSudo(collect([$command]), $this->server); + + expect($result)->toBe(['sudo '.$command]); +}); + test('wraps complex Docker install command with multiple fallbacks', function () { $commands = collect([ 'curl --max-time 300 https://releases.rancher.com/install-docker/27.3.sh | sh || curl https://get.docker.com | sh -s -- --version 27.3',