From 804d05b9b6b1d1707def41bf6e0b9e6895e2b127 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 6 Oct 2026 12:56:50 +0200 Subject: [PATCH] fix(backups): close backup directories to other users on non-root servers --- app/Jobs/DatabaseBackupJob.php | 35 ++++++--- .../BackupDirectoryPermissionsTest.php | 76 +++++++++++++++++++ 2 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 tests/Feature/BackupDirectoryPermissionsTest.php diff --git a/app/Jobs/DatabaseBackupJob.php b/app/Jobs/DatabaseBackupJob.php index 2eba0216de..92d24bab3d 100644 --- a/app/Jobs/DatabaseBackupJob.php +++ b/app/Jobs/DatabaseBackupJob.php @@ -598,10 +598,9 @@ 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 '.$escapedBackupDir; + $commands = $this->backupDirectoryCommands(); if (str($this->database->image)->startsWith('mongo:4')) { $commands[] = "docker exec {$escapedContainerName} mongodump --uri=$escapedUrl --gzip --archive > {$escapedBackupLocation}"; } else { @@ -615,7 +614,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $databaseName = $databaseWithCollections; $collectionsToExclude = collect(); } - $commands[] = 'mkdir -p '.$escapedBackupDir; + $commands = $this->backupDirectoryCommands(); // Validate and escape database name to prevent command injection validateShellSafePath($databaseName, 'database name'); @@ -681,7 +680,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function backup_standalone_postgresql(string $database): void { try { - $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); + $commands = $this->backupDirectoryCommands(); $backupCommand = 'docker exec'; if ($this->postgres_password) { $backupCommand .= ' -e PGPASSWORD='.escapeshellarg($this->postgres_password); @@ -714,7 +713,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function backup_standalone_mysql(string $database): void { try { - $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); + $commands = $this->backupDirectoryCommands(); $escapedPassword = escapeshellarg($this->database->mysql_root_password); $escapedContainerName = escapeshellarg($this->container_name); $escapedBackupLocation = escapeshellarg($this->backup_location); @@ -741,7 +740,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue private function backup_standalone_mariadb(string $database): void { try { - $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); + $commands = $this->backupDirectoryCommands(); $escapedPassword = escapeshellarg($this->database->mariadb_root_password); $escapedContainerName = escapeshellarg($this->container_name); $escapedBackupLocation = escapeshellarg($this->backup_location); @@ -770,12 +769,12 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue $archiveName = ltrim($this->backup_file, '/'); try { - $commands = ClickhouseBackupCommand::make( + $commands = [...$this->backupDirectoryCommands(), ...ClickhouseBackupCommand::make( containerName: $this->container_name, database: $database, archiveName: $archiveName, backupDirectory: $this->backup_dir, - ); + )]; $this->backup_output = instant_remote_process($this->writeBackupFileAsRoot($commands), $this->server, true, false, $this->commandTimeout(), disableMultiplexing: true); $this->backup_output = trim($this->backup_output); @@ -797,7 +796,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue if (! preg_match(StandaloneSqlite::DATABASES_PATTERN, $database)) { throw new \Exception("Invalid database file name: {$database}"); } - $commands[] = 'mkdir -p '.escapeshellarg($this->backup_dir); + $commands = $this->backupDirectoryCommands(); $script = 'f=$(mktemp) && sqlite3 -readonly '.escapeshellarg(StandaloneSqlite::DATA_DIRECTORY.'/'.$database).' \'.timeout 10000\' "VACUUM INTO \'$f\'" && cat "$f"; s=$?; rm -f "$f"; exit $s'; $dumpCommand = 'docker exec '.escapeshellarg($this->container_name).' sh -c '.escapeshellarg($script); $commands[] = $this->buildCompressedDumpCommand($dumpCommand, escapeshellarg($this->backup_location)); @@ -912,6 +911,24 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue return "{$helperImage}:{$latestVersion}"; } + /** + * Creates the backup directory. On a server with a non-root SSH user, the directory gets the ownership step + * that the sudo parser adds only to unquoted paths: the SSH user owns it (backup downloads read it over SFTP + * as that user) and other users cannot read the dumps in it. + * + * @return array + */ + private function backupDirectoryCommands(): array + { + $backupDirectory = escapeshellarg($this->backup_dir); + $commands = ['mkdir -p '.$backupDirectory]; + if ($this->server->isNonRoot()) { + $commands[] = ownershipCommand($backupDirectory, $this->server); + } + + return $commands; + } + /** * The SSH user's shell opens a `>` redirect. For a non-root SSH user, the dump and the redirect into * the backup directory must run in one root shell, so each line that writes the backup file becomes diff --git a/tests/Feature/BackupDirectoryPermissionsTest.php b/tests/Feature/BackupDirectoryPermissionsTest.php new file mode 100644 index 0000000000..956f936e70 --- /dev/null +++ b/tests/Feature/BackupDirectoryPermissionsTest.php @@ -0,0 +1,76 @@ + false]); + InstanceSettings::forceCreate(['id' => 0]); + Notification::fake(); +}); + +/** + * Runs a MySQL backup on a server with the given SSH user and returns the remote script of the dump command. + */ +function backupDirectoryDumpScript(string $sshUser): string +{ + $team = Team::factory()->create(['name' => 'Acme']); + $server = Server::factory()->create(['team_id' => $team->id, 'user' => $sshUser]); + $server->update(['private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id]); + $server->settings()->update(['is_reachable' => true, 'is_usable' => true]); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $database = create_standalone_mysql($environment->id, StandaloneDocker::where('server_id', $server->id)->firstOrFail(), [ + 'mysql_database' => 'app', + 'status' => 'running:healthy', + ]); + $backup = ScheduledDatabaseBackup::create([ + 'frequency' => '0 0 * * *', + 'save_s3' => false, + 'database_type' => StandaloneMysql::class, + 'database_id' => $database->id, + 'team_id' => $team->id, + 'databases_to_backup' => 'app', + ]); + $commands = new Collection; + Process::fake(function ($process) use ($commands) { + $commands->push($process->command); + + return Process::result(output: str_contains($process->command, 'du -b') ? '100' : ''); + }); + + (new DatabaseBackupJob($backup))->handle(); + + return $commands->first(fn (string $command) => str_contains($command, 'mysqldump')); +} + +it('gives the backup directory to the ssh user and closes it to other users on a non-root server', function () { + $script = backupDirectoryDumpScript('ubuntu'); + + expect($script)->toMatch("~^sudo mkdir -p '(/data/coolify/backups/databases/acme-\\d+/[^']+)'\\nsudo find '\\1' -user root -exec chown [^{]*ubuntu:ubuntu \\{\\} \\+ && sudo chmod o-rwx '\\1'\\n.*mysqldump~m"); +}); + +it('leaves the backup directory command unchanged on a root server', function () { + $script = backupDirectoryDumpScript('root'); + + expect($script)->toMatch("~^mkdir -p '/data/coolify/backups/databases/acme-\\d+/[^']+'\\n.*mysqldump~m") + ->and($script)->not->toContain('chmod o-rwx'); +});