From dd8698840db1cd33e736943bcd2ccd6cb613c919 Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Thu, 5 Mar 2026 19:06:54 +0100 Subject: [PATCH] Harden backup security: password confirmation, CSRF, env toggle Address security review feedback from jbtronics: - Add IS_AUTHENTICATED_FULLY to all sensitive endpoints (create/delete backup, delete log, download backup, start update, restore) - Change backup download from GET to POST with CSRF token - Require password confirmation before downloading backups (backups contain sensitive data like password hashes and secrets) - Add DISABLE_BACKUP_DOWNLOAD env var (default: disabled) to control whether backup downloads are allowed - Add password confirmation modal with security warning in template - Add comprehensive tests: auth checks, env var blocking, POST-only enforcement, status/progress endpoint auth --- .env | 5 + src/Controller/UpdateManagerController.php | 44 ++++- .../admin/update_manager/index.html.twig | 62 ++++++- .../UpdateManagerControllerTest.php | 171 ++++++++++++++---- translations/messages.en.xlf | 18 ++ 5 files changed, 257 insertions(+), 43 deletions(-) diff --git a/.env b/.env index 3ba3d65d..447ff5de 100644 --- a/.env +++ b/.env @@ -71,6 +71,11 @@ DISABLE_WEB_UPDATES=1 # Restoring backups is a destructive operation that could overwrite your database. DISABLE_BACKUP_RESTORE=1 +# Disable backup download from the Update Manager UI (0=enabled, 1=disabled). +# Backups contain sensitive data including password hashes and secrets. +# When enabled, users must confirm their password before downloading. +DISABLE_BACKUP_DOWNLOAD=1 + ################################################################################### # SAML Single sign on-settings ################################################################################### diff --git a/src/Controller/UpdateManagerController.php b/src/Controller/UpdateManagerController.php index 480e86a0..70be714d 100644 --- a/src/Controller/UpdateManagerController.php +++ b/src/Controller/UpdateManagerController.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace App\Controller; +use App\Entity\UserSystem\User; use App\Services\System\BackupManager; use App\Services\System\InstallationTypeDetector; use App\Services\System\UpdateChecker; @@ -36,6 +37,7 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\ResponseHeaderBag; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; +use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface; use Symfony\Component\Routing\Attribute\Route; /** @@ -53,10 +55,13 @@ class UpdateManagerController extends AbstractController private readonly VersionManagerInterface $versionManager, private readonly BackupManager $backupManager, private readonly InstallationTypeDetector $installationTypeDetector, + private readonly UserPasswordHasherInterface $passwordHasher, #[Autowire(env: 'bool:DISABLE_WEB_UPDATES')] private readonly bool $webUpdatesDisabled = false, #[Autowire(env: 'bool:DISABLE_BACKUP_RESTORE')] private readonly bool $backupRestoreDisabled = false, + #[Autowire(env: 'bool:DISABLE_BACKUP_DOWNLOAD')] + private readonly bool $backupDownloadDisabled = false, ) { } @@ -80,6 +85,16 @@ class UpdateManagerController extends AbstractController } } + /** + * Check if backup download is disabled and throw exception if so. + */ + private function denyIfBackupDownloadDisabled(): void + { + if ($this->backupDownloadDisabled) { + throw new AccessDeniedHttpException('Backup download is disabled by server configuration.'); + } + } + /** * Main update manager page. */ @@ -105,6 +120,7 @@ class UpdateManagerController extends AbstractController 'backups' => $this->backupManager->getBackups(), 'web_updates_disabled' => $this->webUpdatesDisabled, 'backup_restore_disabled' => $this->backupRestoreDisabled, + 'backup_download_disabled' => $this->backupDownloadDisabled, 'is_docker' => $this->installationTypeDetector->isDocker(), ]); } @@ -211,6 +227,7 @@ class UpdateManagerController extends AbstractController #[Route('/start', name: 'admin_update_manager_start', methods: ['POST'])] public function startUpdate(Request $request): Response { + $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY'); $this->denyAccessUnlessGranted('@system.manage_updates'); $this->denyIfWebUpdatesDisabled(); @@ -325,6 +342,7 @@ class UpdateManagerController extends AbstractController #[Route('/backup', name: 'admin_update_manager_backup', methods: ['POST'])] public function createBackup(Request $request): Response { + $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY'); $this->denyAccessUnlessGranted('@system.manage_updates'); if (!$this->isCsrfTokenValid('update_manager_backup', $request->request->get('_token'))) { @@ -338,7 +356,7 @@ class UpdateManagerController extends AbstractController } try { - $backupPath = $this->backupManager->createBackup(null, 'manual'); + $this->backupManager->createBackup(null, 'manual'); $this->addFlash('success', 'update_manager.backup.created'); } catch (\Exception $e) { $this->addFlash('error', 'Backup failed: ' . $e->getMessage()); @@ -353,6 +371,7 @@ class UpdateManagerController extends AbstractController #[Route('/backup/delete', name: 'admin_update_manager_backup_delete', methods: ['POST'])] public function deleteBackup(Request $request): Response { + $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY'); $this->denyAccessUnlessGranted('@system.manage_updates'); if (!$this->isCsrfTokenValid('update_manager_delete', $request->request->get('_token'))) { @@ -376,6 +395,7 @@ class UpdateManagerController extends AbstractController #[Route('/log/delete', name: 'admin_update_manager_log_delete', methods: ['POST'])] public function deleteLog(Request $request): Response { + $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY'); $this->denyAccessUnlessGranted('@system.manage_updates'); if (!$this->isCsrfTokenValid('update_manager_delete', $request->request->get('_token'))) { @@ -395,12 +415,29 @@ class UpdateManagerController extends AbstractController /** * Download a backup file. + * Requires password confirmation as backups contain sensitive data (password hashes, secrets, etc.). */ - #[Route('/backup/download/{filename}', name: 'admin_update_manager_backup_download', methods: ['GET'])] - public function downloadBackup(string $filename): BinaryFileResponse + #[Route('/backup/download', name: 'admin_update_manager_backup_download', methods: ['POST'])] + public function downloadBackup(Request $request): Response { + $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY'); $this->denyAccessUnlessGranted('@system.manage_updates'); + $this->denyIfBackupDownloadDisabled(); + if (!$this->isCsrfTokenValid('update_manager_download', $request->request->get('_token'))) { + $this->addFlash('error', 'Invalid CSRF token.'); + return $this->redirectToRoute('admin_update_manager'); + } + + // Verify password + $password = $request->request->get('password', ''); + $user = $this->getUser(); + if (!$user instanceof User || !$this->passwordHasher->isPasswordValid($user, $password)) { + $this->addFlash('error', 'update_manager.backup.download.invalid_password'); + return $this->redirectToRoute('admin_update_manager'); + } + + $filename = $request->request->get('filename', ''); $details = $this->backupManager->getBackupDetails($filename); if (!$details) { throw $this->createNotFoundException('Backup not found'); @@ -418,6 +455,7 @@ class UpdateManagerController extends AbstractController #[Route('/restore', name: 'admin_update_manager_restore', methods: ['POST'])] public function restore(Request $request): Response { + $this->denyAccessUnlessGranted('IS_AUTHENTICATED_FULLY'); $this->denyAccessUnlessGranted('@system.manage_updates'); $this->denyIfBackupRestoreDisabled(); diff --git a/templates/admin/update_manager/index.html.twig b/templates/admin/update_manager/index.html.twig index 4b6ff237..7dcb813c 100644 --- a/templates/admin/update_manager/index.html.twig +++ b/templates/admin/update_manager/index.html.twig @@ -416,12 +416,15 @@
- {% if is_granted('@system.manage_updates') %} - + {% if not backup_download_disabled and is_granted('@system.manage_updates') %} + {% endif %} {% if not backup_restore_disabled and is_granted('@system.manage_updates') %}
+ +{# Password confirmation modal for backup download #} +{% if not backup_download_disabled and is_granted('@system.manage_updates') %} + + +{% endif %} {% endblock %} diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php index 1df093f9..c37d413e 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -58,6 +58,8 @@ final class UpdateManagerControllerTest extends WebTestCase return $form->filter('input[name="_token"]')->attr('value'); } + // ---- Authentication tests ---- + public function testIndexPageRequiresAuth(): void { $client = static::createClient(); @@ -78,6 +80,8 @@ final class UpdateManagerControllerTest extends WebTestCase $this->assertResponseIsSuccessful(); } + // ---- Backup creation tests ---- + public function testCreateBackupRequiresCsrf(): void { $client = static::createClient(); @@ -116,6 +120,33 @@ final class UpdateManagerControllerTest extends WebTestCase } } + public function testCreateBackupBlockedWhenLocked(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + // Load the page first to get CSRF token before locking + $crawler = $client->request('GET', '/en/system/update-manager'); + $csrfToken = $this->getCsrfTokenFromPage($crawler, 'backup'); + + // Acquire lock to simulate update in progress + $updateExecutor = $client->getContainer()->get(UpdateExecutor::class); + $updateExecutor->acquireLock(); + + try { + $client->request('POST', '/en/system/update-manager/backup', [ + '_token' => $csrfToken, + ]); + + $this->assertResponseRedirects(); + } finally { + // Always release lock + $updateExecutor->releaseLock(); + } + } + + // ---- Backup deletion tests ---- + public function testDeleteBackupRequiresCsrf(): void { $client = static::createClient(); @@ -156,6 +187,8 @@ final class UpdateManagerControllerTest extends WebTestCase $this->assertFileDoesNotExist($backupDir . '/' . $testFile); } + // ---- Log deletion tests ---- + public function testDeleteLogRequiresCsrf(): void { $client = static::createClient(); @@ -196,39 +229,50 @@ final class UpdateManagerControllerTest extends WebTestCase $this->assertFileDoesNotExist($logDir . '/' . $testFile); } - public function testDownloadBackupReturns404ForNonExistent(): void + // ---- Backup download tests ---- + + public function testDownloadBackupBlockedByDefault(): void { $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('GET', '/en/system/update-manager/backup/download/nonexistent.zip'); + // DISABLE_BACKUP_DOWNLOAD=1 is the default in .env, so this should return 403 + $client->request('POST', '/en/system/update-manager/backup/download', [ + '_token' => 'any', + 'filename' => 'test.zip', + 'password' => 'test', + ]); - $this->assertResponseStatusCodeSame(404); + $this->assertResponseStatusCodeSame(403); } - public function testDownloadBackupSuccess(): void + public function testDownloadBackupRequiresPost(): void { $client = static::createClient(); $this->loginAsAdmin($client); - // Create a temporary backup file to download - $backupManager = $client->getContainer()->get(BackupManager::class); - $backupDir = $backupManager->getBackupDir(); - if (!is_dir($backupDir)) { - mkdir($backupDir, 0755, true); - } - $testFile = 'test-download-' . uniqid() . '.zip'; - file_put_contents($backupDir . '/' . $testFile, 'fake zip content'); + // GET should return 405 Method Not Allowed + $client->request('GET', '/en/system/update-manager/backup/download'); - $client->request('GET', '/en/system/update-manager/backup/download/' . $testFile); - - $this->assertResponseIsSuccessful(); - $this->assertResponseHeaderSame('content-disposition', 'attachment; filename=' . $testFile); - - // Clean up - @unlink($backupDir . '/' . $testFile); + $this->assertResponseStatusCodeSame(405); } + public function testDownloadBackupRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('POST', '/en/system/update-manager/backup/download', [ + '_token' => 'any', + 'filename' => 'test.zip', + 'password' => 'test', + ]); + + // Should deny access (401 with HTTP Basic auth in test env) + $this->assertResponseStatusCodeSame(401); + } + + // ---- Backup details tests ---- + public function testBackupDetailsReturns404ForNonExistent(): void { $client = static::createClient(); @@ -239,6 +283,8 @@ final class UpdateManagerControllerTest extends WebTestCase $this->assertResponseStatusCodeSame(404); } + // ---- Restore tests ---- + public function testRestoreBlockedWhenDisabled(): void { $client = static::createClient(); @@ -253,28 +299,83 @@ final class UpdateManagerControllerTest extends WebTestCase $this->assertResponseStatusCodeSame(403); } - public function testCreateBackupBlockedWhenLocked(): void + public function testRestoreRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('POST', '/en/system/update-manager/restore', [ + '_token' => 'invalid', + 'filename' => 'test.zip', + ]); + + $this->assertResponseStatusCodeSame(401); + } + + // ---- Start update tests ---- + + public function testStartUpdateRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('POST', '/en/system/update-manager/start', [ + '_token' => 'invalid', + 'version' => 'v1.0.0', + ]); + + $this->assertResponseStatusCodeSame(401); + } + + public function testStartUpdateBlockedWhenWebUpdatesDisabled(): void { $client = static::createClient(); $this->loginAsAdmin($client); - // Load the page first to get CSRF token before locking - $crawler = $client->request('GET', '/en/system/update-manager'); - $csrfToken = $this->getCsrfTokenFromPage($crawler, 'backup'); + // DISABLE_WEB_UPDATES=1 is the default in .env + $client->request('POST', '/en/system/update-manager/start', [ + '_token' => 'invalid', + 'version' => 'v1.0.0', + ]); - // Acquire lock to simulate update in progress - $updateExecutor = $client->getContainer()->get(UpdateExecutor::class); - $updateExecutor->acquireLock(); + $this->assertResponseStatusCodeSame(403); + } - try { - $client->request('POST', '/en/system/update-manager/backup', [ - '_token' => $csrfToken, - ]); + // ---- Status and progress tests ---- - $this->assertResponseRedirects(); - } finally { - // Always release lock - $updateExecutor->releaseLock(); - } + public function testStatusEndpointRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('GET', '/en/system/update-manager/status'); + + $this->assertResponseStatusCodeSame(401); + } + + public function testStatusEndpointAccessibleByAdmin(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('GET', '/en/system/update-manager/status'); + + $this->assertResponseIsSuccessful(); + } + + public function testProgressStatusEndpointRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('GET', '/en/system/update-manager/progress/status'); + + $this->assertResponseStatusCodeSame(401); + } + + public function testProgressStatusEndpointAccessibleByAdmin(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('GET', '/en/system/update-manager/progress/status'); + + $this->assertResponseIsSuccessful(); } } diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index ac68c77e..6ccc92d5 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -12563,6 +12563,24 @@ Buerklin-API Authentication server: Download backup + + + update_manager.backup.download.password_label + Confirm your password to download + + + + + update_manager.backup.download.security_warning + Backups contain sensitive data including password hashes and secrets. Please confirm your password to proceed with the download. + + + + + update_manager.backup.download.invalid_password + Invalid password. Backup download denied. + + update_manager.backup.docker_warning