From 14300f2cf10299c01832e60e3711626fcc1d4fbe Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Tue, 17 Feb 2026 22:20:01 +0100 Subject: [PATCH 01/15] Add manual backup creation and delete buttons to Update Manager - Add "Create Backup" button in the backups tab for on-demand backups - Add delete buttons (trash icons) for update logs and backups - New controller routes with CSRF protection and permission checks - Use data-turbo-confirm for CSP-safe confirmation dialogs - Add deleteLog() method to UpdateExecutor with filename validation --- src/Controller/UpdateManagerController.php | 74 ++++++++++++++++ src/Services/System/UpdateExecutor.php | 27 ++++++ .../admin/update_manager/index.html.twig | 86 ++++++++++++++----- translations/messages.en.xlf | 66 ++++++++++++++ 4 files changed, 230 insertions(+), 23 deletions(-) diff --git a/src/Controller/UpdateManagerController.php b/src/Controller/UpdateManagerController.php index 474c86fc3..0afc79051 100644 --- a/src/Controller/UpdateManagerController.php +++ b/src/Controller/UpdateManagerController.php @@ -314,6 +314,80 @@ public function backupDetails(string $filename): JsonResponse return $this->json($details); } + /** + * Create a manual backup. + */ + #[Route('/backup', name: 'admin_update_manager_backup', methods: ['POST'])] + public function createBackup(Request $request): Response + { + $this->denyAccessUnlessGranted('@system.manage_updates'); + + if (!$this->isCsrfTokenValid('update_manager_backup', $request->request->get('_token'))) { + $this->addFlash('error', 'Invalid CSRF token.'); + return $this->redirectToRoute('admin_update_manager'); + } + + if ($this->updateExecutor->isLocked()) { + $this->addFlash('error', 'Cannot create backup while an update is in progress.'); + return $this->redirectToRoute('admin_update_manager'); + } + + try { + $backupPath = $this->backupManager->createBackup(null, 'manual'); + $this->addFlash('success', 'update_manager.backup.created'); + } catch (\Exception $e) { + $this->addFlash('error', 'Backup failed: ' . $e->getMessage()); + } + + return $this->redirectToRoute('admin_update_manager'); + } + + /** + * Delete a backup file. + */ + #[Route('/backup/delete', name: 'admin_update_manager_backup_delete', methods: ['POST'])] + public function deleteBackup(Request $request): Response + { + $this->denyAccessUnlessGranted('@system.manage_updates'); + + if (!$this->isCsrfTokenValid('update_manager_delete', $request->request->get('_token'))) { + $this->addFlash('error', 'Invalid CSRF token.'); + return $this->redirectToRoute('admin_update_manager'); + } + + $filename = $request->request->get('filename'); + if ($filename && $this->backupManager->deleteBackup($filename)) { + $this->addFlash('success', 'update_manager.backup.deleted'); + } else { + $this->addFlash('error', 'update_manager.backup.delete_error'); + } + + return $this->redirectToRoute('admin_update_manager'); + } + + /** + * Delete an update log file. + */ + #[Route('/log/delete', name: 'admin_update_manager_log_delete', methods: ['POST'])] + public function deleteLog(Request $request): Response + { + $this->denyAccessUnlessGranted('@system.manage_updates'); + + if (!$this->isCsrfTokenValid('update_manager_delete', $request->request->get('_token'))) { + $this->addFlash('error', 'Invalid CSRF token.'); + return $this->redirectToRoute('admin_update_manager'); + } + + $filename = $request->request->get('filename'); + if ($filename && $this->updateExecutor->deleteLog($filename)) { + $this->addFlash('success', 'update_manager.log.deleted'); + } else { + $this->addFlash('error', 'update_manager.log.delete_error'); + } + + return $this->redirectToRoute('admin_update_manager'); + } + /** * Restore from a backup. */ diff --git a/src/Services/System/UpdateExecutor.php b/src/Services/System/UpdateExecutor.php index 2fe541739..fca7d1fa5 100644 --- a/src/Services/System/UpdateExecutor.php +++ b/src/Services/System/UpdateExecutor.php @@ -602,6 +602,33 @@ public function getUpdateLogs(): array } + /** + * Delete a specific update log file. + */ + public function deleteLog(string $filename): bool + { + // Validate filename pattern for security + if (!preg_match('/^update-[\w.\-]+\.log$/', $filename)) { + $this->logger->warning('Attempted to delete invalid log filename: ' . $filename); + return false; + } + + $logPath = $this->project_dir . '/' . self::UPDATE_LOG_DIR . '/' . $filename; + + if (!file_exists($logPath)) { + return false; + } + + try { + $this->filesystem->remove($logPath); + $this->logger->info('Deleted update log: ' . $filename); + return true; + } catch (\Exception $e) { + $this->logger->error('Failed to delete update log: ' . $e->getMessage()); + return false; + } + } + /** * Restore from a backup file with maintenance mode and cache clearing. * diff --git a/templates/admin/update_manager/index.html.twig b/templates/admin/update_manager/index.html.twig index 44b9f8c00..8afa44726 100644 --- a/templates/admin/update_manager/index.html.twig +++ b/templates/admin/update_manager/index.html.twig @@ -343,11 +343,26 @@ {{ log.date|date('Y-m-d H:i') }} {{ log.file }} - - - - + +
+ + + + {% if is_granted('@system.manage_updates') %} +
+ + + +
+ {% endif %} +
{% else %} @@ -362,6 +377,17 @@
+ {% if is_granted('@system.manage_updates') and status.can_auto_update and not is_locked %} +
+
+ + +
+
+ {% endif %}
@@ -383,24 +409,38 @@ {{ (backup.size / 1024 / 1024)|number_format(1) }} MB {% else %} diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index b84a18754..1900c60e6 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -12491,6 +12491,72 @@ Buerklin-API Authentication server: Backup restore is disabled by server configuration. + + + update_manager.backup.create + Create Backup + + + + + update_manager.backup.create.confirm + Create a full backup now? This may take a moment. + + + + + update_manager.backup.created + Backup created successfully. + + + + + update_manager.backup.delete.confirm + Are you sure you want to delete this backup? + + + + + update_manager.backup.deleted + Backup deleted successfully. + + + + + update_manager.backup.delete_error + Failed to delete backup. + + + + + update_manager.log.delete.confirm + Are you sure you want to delete this log? + + + + + update_manager.log.deleted + Log deleted successfully. + + + + + update_manager.log.delete_error + Failed to delete log. + + + + + update_manager.view_log + View log + + + + + update_manager.delete + Delete + + settings.ips.conrad From ac95a5d9e00964df351e7c5a8bf0f3c3eb0ba1e0 Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Fri, 20 Feb 2026 23:41:52 +0100 Subject: [PATCH 02/15] Add Docker backup support: download button, SQLite restore fix, decouple from auto-update - Decouple backup creation/restore UI from can_auto_update so Docker and other non-git installations can use backup features - Add backup download endpoint for saving backups externally - Fix SQLite restore to use configured DATABASE_URL path instead of hardcoded var/app.db (affects Docker and custom SQLite paths) - Show Docker-specific warning about var/backups/ not being persisted - Pass is_docker flag to template via InstallationTypeDetector --- src/Controller/UpdateManagerController.php | 24 +++++++++++++++++++ src/Services/System/BackupManager.php | 13 +++++----- .../admin/update_manager/index.html.twig | 19 ++++++++++++--- translations/messages.en.xlf | 12 ++++++++++ 4 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/Controller/UpdateManagerController.php b/src/Controller/UpdateManagerController.php index 0afc79051..480e86a01 100644 --- a/src/Controller/UpdateManagerController.php +++ b/src/Controller/UpdateManagerController.php @@ -24,14 +24,17 @@ namespace App\Controller; use App\Services\System\BackupManager; +use App\Services\System\InstallationTypeDetector; use App\Services\System\UpdateChecker; use App\Services\System\UpdateExecutor; use Shivas\VersioningBundle\Service\VersionManagerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\DependencyInjection\Attribute\Autowire; +use Symfony\Component\HttpFoundation\BinaryFileResponse; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpFoundation\ResponseHeaderBag; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\Routing\Attribute\Route; @@ -49,6 +52,7 @@ public function __construct( private readonly UpdateExecutor $updateExecutor, private readonly VersionManagerInterface $versionManager, private readonly BackupManager $backupManager, + private readonly InstallationTypeDetector $installationTypeDetector, #[Autowire(env: 'bool:DISABLE_WEB_UPDATES')] private readonly bool $webUpdatesDisabled = false, #[Autowire(env: 'bool:DISABLE_BACKUP_RESTORE')] @@ -101,6 +105,7 @@ public function index(): Response 'backups' => $this->backupManager->getBackups(), 'web_updates_disabled' => $this->webUpdatesDisabled, 'backup_restore_disabled' => $this->backupRestoreDisabled, + 'is_docker' => $this->installationTypeDetector->isDocker(), ]); } @@ -388,6 +393,25 @@ public function deleteLog(Request $request): Response return $this->redirectToRoute('admin_update_manager'); } + /** + * Download a backup file. + */ + #[Route('/backup/download/{filename}', name: 'admin_update_manager_backup_download', methods: ['GET'])] + public function downloadBackup(string $filename): BinaryFileResponse + { + $this->denyAccessUnlessGranted('@system.manage_updates'); + + $details = $this->backupManager->getBackupDetails($filename); + if (!$details) { + throw $this->createNotFoundException('Backup not found'); + } + + $response = new BinaryFileResponse($details['path']); + $response->setContentDisposition(ResponseHeaderBag::DISPOSITION_ATTACHMENT, $details['file']); + + return $response; + } + /** * Restore from a backup. */ diff --git a/src/Services/System/BackupManager.php b/src/Services/System/BackupManager.php index 4946bc240..621b58d77 100644 --- a/src/Services/System/BackupManager.php +++ b/src/Services/System/BackupManager.php @@ -327,14 +327,14 @@ public function restoreBackup( */ private function restoreDatabaseFromBackup(string $tempDir): void { + // Get database connection params from Doctrine + $connection = $this->entityManager->getConnection(); + $params = $connection->getParams(); + $platform = $connection->getDatabasePlatform(); + // Check for SQL dump (MySQL/PostgreSQL) $sqlFile = $tempDir . '/database.sql'; if (file_exists($sqlFile)) { - // Import SQL using mysql/psql command directly - // First, get database connection params from Doctrine - $connection = $this->entityManager->getConnection(); - $params = $connection->getParams(); - $platform = $connection->getDatabasePlatform(); if ($platform instanceof AbstractMySQLPlatform) { // Use mysql command to import - need to use shell to handle input redirection @@ -403,7 +403,8 @@ private function restoreDatabaseFromBackup(string $tempDir): void // Check for SQLite database file $sqliteFile = $tempDir . '/var/app.db'; if (file_exists($sqliteFile)) { - $targetDb = $this->projectDir . '/var/app.db'; + // Use the actual configured SQLite path from Doctrine, not a hardcoded path + $targetDb = $params['path'] ?? $this->projectDir . '/var/app.db'; $this->filesystem->copy($sqliteFile, $targetDb, true); return; } diff --git a/templates/admin/update_manager/index.html.twig b/templates/admin/update_manager/index.html.twig index 8afa44726..4b6ff237d 100644 --- a/templates/admin/update_manager/index.html.twig +++ b/templates/admin/update_manager/index.html.twig @@ -377,17 +377,23 @@
- {% if is_granted('@system.manage_updates') and status.can_auto_update and not is_locked %} + {% if is_granted('@system.manage_updates') and not is_locked %}
{% endif %} + {% if is_docker %} +
+ + {% trans %}update_manager.backup.docker_warning{% endtrans %} +
+ {% endif %}
- {% if status.can_auto_update and validation.valid and not backup_restore_disabled %} -
- - - - -
- {% endif %} +
+ {% if status.can_auto_update and validation.valid and not backup_restore_disabled %} +
+ + + + +
+ {% endif %} + {% if is_granted('@system.manage_updates') %} +
+ + + +
+ {% endif %} +
@@ -410,7 +416,14 @@ {% else %} @@ -477,52 +518,4 @@ -{# 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 c37d413e1..0c2b2224b 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -251,10 +251,10 @@ public function testDownloadBackupRequiresPost(): void $client = static::createClient(); $this->loginAsAdmin($client); - // GET should return 405 Method Not Allowed + // GET returns 404 since no GET route exists for this path $client->request('GET', '/en/system/update-manager/backup/download'); - $this->assertResponseStatusCodeSame(405); + $this->assertResponseStatusCodeSame(404); } public function testDownloadBackupRequiresAuth(): void From ba11eab4e860848ce85ef8e917964041546ce5ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Sat, 7 Mar 2026 18:27:51 +0100 Subject: [PATCH 11/15] Fixed translation keys --- translations/messages.en.xlf | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index 6ccc92d5b..54144ad03 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -12491,97 +12491,97 @@ Buerklin-API Authentication server: Backup restore is disabled by server configuration. - + update_manager.backup.create Create Backup - + update_manager.backup.create.confirm Create a full backup now? This may take a moment. - + update_manager.backup.created Backup created successfully. - + update_manager.backup.delete.confirm Are you sure you want to delete this backup? - + update_manager.backup.deleted Backup deleted successfully. - + update_manager.backup.delete_error Failed to delete backup. - + update_manager.log.delete.confirm Are you sure you want to delete this log? - + update_manager.log.deleted Log deleted successfully. - + update_manager.log.delete_error Failed to delete log. - + update_manager.view_log View log - + update_manager.delete Delete - + update_manager.backup.download 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 Docker installation detected. Backups are stored in var/backups/ which is not a persistent volume. Use the download button to save backups externally, or mount var/backups/ as a volume in your docker-compose.yml. From 8bf25a1e38d3f51c2226e549621f31ea0f55e7d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Sat, 7 Mar 2026 18:46:43 +0100 Subject: [PATCH 12/15] Fixed text justification in download modal --- templates/admin/update_manager/index.html.twig | 7 ++++--- translations/messages.en.xlf | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/templates/admin/update_manager/index.html.twig b/templates/admin/update_manager/index.html.twig index 76d7ca2ae..fe7608552 100644 --- a/templates/admin/update_manager/index.html.twig +++ b/templates/admin/update_manager/index.html.twig @@ -427,7 +427,7 @@ {% endif %} {% if not backup_restore_disabled and is_granted('@system.manage_updates') %}
{% endif %} + {% if not backup_download_disabled and is_granted('@system.manage_updates') %} {# Per-backup download modal - no inline JS needed, CSP compatible with Turbo #} -
- {% if status.can_auto_update and validation.valid and not backup_restore_disabled %} + {% if is_granted('@system.manage_updates') %} + + + + {% endif %} + {% if not backup_restore_disabled and is_granted('@system.manage_updates') %}
Delete + + + update_manager.backup.download + Download backup + + + + + update_manager.backup.docker_warning + Docker installation detected. Backups are stored in var/backups/ which is not a persistent volume. Use the download button to save backups externally, or mount var/backups/ as a volume in your docker-compose.yml. + + settings.ips.conrad From f442f45b815cafe277a842848dc85a4ffd5fffbd Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Fri, 20 Feb 2026 23:51:23 +0100 Subject: [PATCH 03/15] Add tests for backup/update manager improvements - Controller tests: auth, CSRF validation, 404 for missing backups, restore disabled check - UpdateExecutor: deleteLog validation, non-existent file, successful deletion - BackupManager: deleteBackup validation for missing/non-zip files --- .../UpdateManagerControllerTest.php | 138 ++++++++++++++++++ tests/Services/System/BackupManagerTest.php | 10 ++ tests/Services/System/UpdateExecutorTest.php | 32 ++++ 3 files changed, 180 insertions(+) create mode 100644 tests/Controller/UpdateManagerControllerTest.php diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php new file mode 100644 index 000000000..b06229184 --- /dev/null +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -0,0 +1,138 @@ +. + */ + +declare(strict_types=1); + +namespace App\Tests\Controller; + +use App\Entity\UserSystem\User; +use PHPUnit\Framework\Attributes\Group; +use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; + +#[Group("slow")] +#[Group("DB")] +final class UpdateManagerControllerTest extends WebTestCase +{ + private function loginAsAdmin($client): void + { + $entityManager = $client->getContainer()->get('doctrine')->getManager(); + $userRepository = $entityManager->getRepository(User::class); + $user = $userRepository->findOneBy(['name' => 'admin']); + + if (!$user) { + $this->markTestSkipped('Admin user not found'); + } + + $client->loginUser($user); + } + + public function testIndexPageRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('GET', '/system/update-manager'); + + // Should redirect to login + $this->assertResponseRedirects(); + } + + public function testIndexPageAccessibleByAdmin(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('GET', '/system/update-manager'); + + $this->assertResponseIsSuccessful(); + } + + public function testCreateBackupRequiresCsrf(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('POST', '/system/update-manager/backup', [ + '_token' => 'invalid', + ]); + + // Should redirect with error flash + $this->assertResponseRedirects('/system/update-manager'); + } + + public function testDeleteBackupRequiresCsrf(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('POST', '/system/update-manager/backup/delete', [ + '_token' => 'invalid', + 'filename' => 'test.zip', + ]); + + $this->assertResponseRedirects('/system/update-manager'); + } + + public function testDeleteLogRequiresCsrf(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('POST', '/system/update-manager/log/delete', [ + '_token' => 'invalid', + 'filename' => 'test.log', + ]); + + $this->assertResponseRedirects('/system/update-manager'); + } + + public function testDownloadBackupReturns404ForNonExistent(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('GET', '/system/update-manager/backup/download/nonexistent.zip'); + + $this->assertResponseStatusCodeSame(404); + } + + public function testBackupDetailsReturns404ForNonExistent(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + $client->request('GET', '/system/update-manager/backup/nonexistent.zip'); + + $this->assertResponseStatusCodeSame(404); + } + + public function testRestoreBlockedWhenDisabled(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + // DISABLE_BACKUP_RESTORE=1 is the default in .env, so this should return 403 + $client->request('POST', '/system/update-manager/restore', [ + '_token' => 'invalid', + 'filename' => 'test.zip', + ]); + + $this->assertResponseStatusCodeSame(403); + } +} diff --git a/tests/Services/System/BackupManagerTest.php b/tests/Services/System/BackupManagerTest.php index f75ef8f32..9aa92813a 100644 --- a/tests/Services/System/BackupManagerTest.php +++ b/tests/Services/System/BackupManagerTest.php @@ -82,6 +82,16 @@ public function testVersionParsingFromFilename(): void $this->assertSame('2.6.0', $matches[2]); } + public function testDeleteBackupReturnsFalseForNonExistentFile(): void + { + $this->assertFalse($this->backupManager->deleteBackup('non-existent.zip')); + } + + public function testDeleteBackupReturnsFalseForNonZipFile(): void + { + $this->assertFalse($this->backupManager->deleteBackup('not-a-zip.txt')); + } + /** * Test version parsing with different filename formats. */ diff --git a/tests/Services/System/UpdateExecutorTest.php b/tests/Services/System/UpdateExecutorTest.php index 48cddf8d4..c3ea9c1da 100644 --- a/tests/Services/System/UpdateExecutorTest.php +++ b/tests/Services/System/UpdateExecutorTest.php @@ -139,6 +139,38 @@ public function testAcquireAndReleaseLock(): void $this->assertFalse($this->updateExecutor->isLocked()); } + public function testDeleteLogRejectsInvalidFilename(): void + { + // Path traversal attempts should be rejected + $this->assertFalse($this->updateExecutor->deleteLog('../../../etc/passwd')); + $this->assertFalse($this->updateExecutor->deleteLog('malicious.txt')); + $this->assertFalse($this->updateExecutor->deleteLog('')); + // Must start with "update-" + $this->assertFalse($this->updateExecutor->deleteLog('backup-v1.0.0.log')); + } + + public function testDeleteLogReturnsFalseForNonExistentFile(): void + { + $this->assertFalse($this->updateExecutor->deleteLog('update-nonexistent-file.log')); + } + + public function testDeleteLogDeletesExistingFile(): void + { + // Create a temporary log file in the update logs directory + $projectDir = self::getContainer()->getParameter('kernel.project_dir'); + $logDir = $projectDir . '/var/update_logs'; + + if (!is_dir($logDir)) { + mkdir($logDir, 0755, true); + } + + $testFile = 'update-test-delete-' . uniqid() . '.log'; + file_put_contents($logDir . '/' . $testFile, 'test log content'); + + $this->assertTrue($this->updateExecutor->deleteLog($testFile)); + $this->assertFileDoesNotExist($logDir . '/' . $testFile); + } + public function testEnableAndDisableMaintenanceMode(): void { // First, ensure maintenance mode is off From d411f15feb6d0aa11db024f90f23e6a5e2164305 Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Sat, 21 Feb 2026 00:04:08 +0100 Subject: [PATCH 04/15] Fix test failures: add locale prefix to URLs, correct log directory path --- .../UpdateManagerControllerTest.php | 22 +++++++++---------- tests/Services/System/UpdateExecutorTest.php | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php index b06229184..b0ad3af7d 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -47,7 +47,7 @@ public function testIndexPageRequiresAuth(): void { $client = static::createClient(); - $client->request('GET', '/system/update-manager'); + $client->request('GET', '/en/system/update-manager'); // Should redirect to login $this->assertResponseRedirects(); @@ -58,7 +58,7 @@ public function testIndexPageAccessibleByAdmin(): void $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('GET', '/system/update-manager'); + $client->request('GET', '/en/system/update-manager'); $this->assertResponseIsSuccessful(); } @@ -68,12 +68,12 @@ public function testCreateBackupRequiresCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('POST', '/system/update-manager/backup', [ + $client->request('POST', '/en/system/update-manager/backup', [ '_token' => 'invalid', ]); // Should redirect with error flash - $this->assertResponseRedirects('/system/update-manager'); + $this->assertResponseRedirects(); } public function testDeleteBackupRequiresCsrf(): void @@ -81,12 +81,12 @@ public function testDeleteBackupRequiresCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('POST', '/system/update-manager/backup/delete', [ + $client->request('POST', '/en/system/update-manager/backup/delete', [ '_token' => 'invalid', 'filename' => 'test.zip', ]); - $this->assertResponseRedirects('/system/update-manager'); + $this->assertResponseRedirects(); } public function testDeleteLogRequiresCsrf(): void @@ -94,12 +94,12 @@ public function testDeleteLogRequiresCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('POST', '/system/update-manager/log/delete', [ + $client->request('POST', '/en/system/update-manager/log/delete', [ '_token' => 'invalid', 'filename' => 'test.log', ]); - $this->assertResponseRedirects('/system/update-manager'); + $this->assertResponseRedirects(); } public function testDownloadBackupReturns404ForNonExistent(): void @@ -107,7 +107,7 @@ public function testDownloadBackupReturns404ForNonExistent(): void $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('GET', '/system/update-manager/backup/download/nonexistent.zip'); + $client->request('GET', '/en/system/update-manager/backup/download/nonexistent.zip'); $this->assertResponseStatusCodeSame(404); } @@ -117,7 +117,7 @@ public function testBackupDetailsReturns404ForNonExistent(): void $client = static::createClient(); $this->loginAsAdmin($client); - $client->request('GET', '/system/update-manager/backup/nonexistent.zip'); + $client->request('GET', '/en/system/update-manager/backup/nonexistent.zip'); $this->assertResponseStatusCodeSame(404); } @@ -128,7 +128,7 @@ public function testRestoreBlockedWhenDisabled(): void $this->loginAsAdmin($client); // DISABLE_BACKUP_RESTORE=1 is the default in .env, so this should return 403 - $client->request('POST', '/system/update-manager/restore', [ + $client->request('POST', '/en/system/update-manager/restore', [ '_token' => 'invalid', 'filename' => 'test.zip', ]); diff --git a/tests/Services/System/UpdateExecutorTest.php b/tests/Services/System/UpdateExecutorTest.php index c3ea9c1da..8b95b3b0e 100644 --- a/tests/Services/System/UpdateExecutorTest.php +++ b/tests/Services/System/UpdateExecutorTest.php @@ -158,7 +158,7 @@ public function testDeleteLogDeletesExistingFile(): void { // Create a temporary log file in the update logs directory $projectDir = self::getContainer()->getParameter('kernel.project_dir'); - $logDir = $projectDir . '/var/update_logs'; + $logDir = $projectDir . '/var/log/updates'; if (!is_dir($logDir)) { mkdir($logDir, 0755, true); From b15074ea4416a46f0260f7a2d5679c4e9b23c6b7 Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Sat, 21 Feb 2026 00:15:36 +0100 Subject: [PATCH 05/15] Fix auth test: expect 401 instead of redirect for HTTP Basic auth --- tests/Controller/UpdateManagerControllerTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php index b0ad3af7d..e770a5bb4 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -49,8 +49,8 @@ public function testIndexPageRequiresAuth(): void $client->request('GET', '/en/system/update-manager'); - // Should redirect to login - $this->assertResponseRedirects(); + // Should deny access (401 with HTTP Basic auth in test env) + $this->assertResponseStatusCodeSame(401); } public function testIndexPageAccessibleByAdmin(): void From 61f54d359e5423cb24ec19dbcf1856812ecdad2c Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Wed, 4 Mar 2026 11:31:40 +0100 Subject: [PATCH 06/15] Improve test coverage for update manager controller Add happy-path tests for backup creation, deletion, download, and log deletion with valid CSRF tokens. Also test the locked state blocking backup creation. --- .../UpdateManagerControllerTest.php | 128 +++++++++++++++++- 1 file changed, 127 insertions(+), 1 deletion(-) diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php index e770a5bb4..fdf10f9e1 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -23,6 +23,8 @@ namespace App\Tests\Controller; use App\Entity\UserSystem\User; +use App\Services\System\BackupManager; +use App\Services\System\UpdateExecutor; use PHPUnit\Framework\Attributes\Group; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; @@ -76,6 +78,31 @@ public function testCreateBackupRequiresCsrf(): void $this->assertResponseRedirects(); } + public function testCreateBackupWithValidCsrf(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + // Get a valid CSRF token + $csrfToken = $client->getContainer()->get('security.csrf.token_manager') + ->getToken('update_manager_backup')->getValue(); + + $client->request('POST', '/en/system/update-manager/backup', [ + '_token' => $csrfToken, + ]); + + $this->assertResponseRedirects(); + + // Clean up: delete the backup that was just created + $backupManager = $client->getContainer()->get(BackupManager::class); + $backups = $backupManager->getBackups(); + foreach ($backups as $backup) { + if (str_contains($backup['file'], 'manual')) { + $backupManager->deleteBackup($backup['file']); + } + } + } + public function testDeleteBackupRequiresCsrf(): void { $client = static::createClient(); @@ -89,6 +116,32 @@ public function testDeleteBackupRequiresCsrf(): void $this->assertResponseRedirects(); } + public function testDeleteBackupWithValidCsrf(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + // Create a temporary backup file to delete + $backupManager = $client->getContainer()->get(BackupManager::class); + $backupDir = $backupManager->getBackupDir(); + if (!is_dir($backupDir)) { + mkdir($backupDir, 0755, true); + } + $testFile = 'test-delete-' . uniqid() . '.zip'; + file_put_contents($backupDir . '/' . $testFile, 'test'); + + $csrfToken = $client->getContainer()->get('security.csrf.token_manager') + ->getToken('update_manager_delete')->getValue(); + + $client->request('POST', '/en/system/update-manager/backup/delete', [ + '_token' => $csrfToken, + 'filename' => $testFile, + ]); + + $this->assertResponseRedirects(); + $this->assertFileDoesNotExist($backupDir . '/' . $testFile); + } + public function testDeleteLogRequiresCsrf(): void { $client = static::createClient(); @@ -102,6 +155,32 @@ public function testDeleteLogRequiresCsrf(): void $this->assertResponseRedirects(); } + public function testDeleteLogWithValidCsrf(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + // Create a temporary log file to delete + $projectDir = $client->getContainer()->getParameter('kernel.project_dir'); + $logDir = $projectDir . '/var/log/updates'; + if (!is_dir($logDir)) { + mkdir($logDir, 0755, true); + } + $testFile = 'update-test-delete-' . uniqid() . '.log'; + file_put_contents($logDir . '/' . $testFile, 'test log content'); + + $csrfToken = $client->getContainer()->get('security.csrf.token_manager') + ->getToken('update_manager_delete')->getValue(); + + $client->request('POST', '/en/system/update-manager/log/delete', [ + '_token' => $csrfToken, + 'filename' => $testFile, + ]); + + $this->assertResponseRedirects(); + $this->assertFileDoesNotExist($logDir . '/' . $testFile); + } + public function testDownloadBackupReturns404ForNonExistent(): void { $client = static::createClient(); @@ -112,6 +191,29 @@ public function testDownloadBackupReturns404ForNonExistent(): void $this->assertResponseStatusCodeSame(404); } + public function testDownloadBackupSuccess(): 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'); + + $client->request('GET', '/en/system/update-manager/backup/download/' . $testFile); + + $this->assertResponseIsSuccessful(); + $this->assertResponseHeaderSame('content-disposition', 'attachment; filename=' . $testFile); + + // Clean up + @unlink($backupDir . '/' . $testFile); + } + public function testBackupDetailsReturns404ForNonExistent(): void { $client = static::createClient(); @@ -135,4 +237,28 @@ public function testRestoreBlockedWhenDisabled(): void $this->assertResponseStatusCodeSame(403); } -} + + public function testCreateBackupBlockedWhenLocked(): void + { + $client = static::createClient(); + $this->loginAsAdmin($client); + + // Acquire lock to simulate update in progress + $updateExecutor = $client->getContainer()->get(UpdateExecutor::class); + $updateExecutor->acquireLock(); + + try { + $csrfToken = $client->getContainer()->get('security.csrf.token_manager') + ->getToken('update_manager_backup')->getValue(); + + $client->request('POST', '/en/system/update-manager/backup', [ + '_token' => $csrfToken, + ]); + + $this->assertResponseRedirects(); + } finally { + // Always release lock + $updateExecutor->releaseLock(); + } + } +} \ No newline at end of file From 095f3ae7768d7d8af428b2dc748a997c010f5fe3 Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Wed, 4 Mar 2026 11:58:12 +0100 Subject: [PATCH 07/15] Fix CSRF tests: initialize session before getting tokens --- .../UpdateManagerControllerTest.php | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php index fdf10f9e1..896f993d2 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -45,6 +45,18 @@ private function loginAsAdmin($client): void $client->loginUser($user); } + /** + * Get a valid CSRF token by first making a request to initialize the session. + */ + private function getCsrfToken($client, string $tokenId): string + { + // Make a GET request first to initialize the session + $client->request('GET', '/en/system/update-manager'); + + return $client->getContainer()->get('security.csrf.token_manager') + ->getToken($tokenId)->getValue(); + } + public function testIndexPageRequiresAuth(): void { $client = static::createClient(); @@ -83,9 +95,7 @@ public function testCreateBackupWithValidCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - // Get a valid CSRF token - $csrfToken = $client->getContainer()->get('security.csrf.token_manager') - ->getToken('update_manager_backup')->getValue(); + $csrfToken = $this->getCsrfToken($client, 'update_manager_backup'); $client->request('POST', '/en/system/update-manager/backup', [ '_token' => $csrfToken, @@ -130,8 +140,7 @@ public function testDeleteBackupWithValidCsrf(): void $testFile = 'test-delete-' . uniqid() . '.zip'; file_put_contents($backupDir . '/' . $testFile, 'test'); - $csrfToken = $client->getContainer()->get('security.csrf.token_manager') - ->getToken('update_manager_delete')->getValue(); + $csrfToken = $this->getCsrfToken($client, 'update_manager_delete'); $client->request('POST', '/en/system/update-manager/backup/delete', [ '_token' => $csrfToken, @@ -169,8 +178,7 @@ public function testDeleteLogWithValidCsrf(): void $testFile = 'update-test-delete-' . uniqid() . '.log'; file_put_contents($logDir . '/' . $testFile, 'test log content'); - $csrfToken = $client->getContainer()->get('security.csrf.token_manager') - ->getToken('update_manager_delete')->getValue(); + $csrfToken = $this->getCsrfToken($client, 'update_manager_delete'); $client->request('POST', '/en/system/update-manager/log/delete', [ '_token' => $csrfToken, @@ -248,8 +256,7 @@ public function testCreateBackupBlockedWhenLocked(): void $updateExecutor->acquireLock(); try { - $csrfToken = $client->getContainer()->get('security.csrf.token_manager') - ->getToken('update_manager_backup')->getValue(); + $csrfToken = $this->getCsrfToken($client, 'update_manager_backup'); $client->request('POST', '/en/system/update-manager/backup', [ '_token' => $csrfToken, @@ -261,4 +268,4 @@ public function testCreateBackupBlockedWhenLocked(): void $updateExecutor->releaseLock(); } } -} \ No newline at end of file +} From c16b6c7face507c79c34ea38cebf595861e7bfbf Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Wed, 4 Mar 2026 12:18:01 +0100 Subject: [PATCH 08/15] Fix CSRF tests: extract tokens from rendered page HTML --- .../UpdateManagerControllerTest.php | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/tests/Controller/UpdateManagerControllerTest.php b/tests/Controller/UpdateManagerControllerTest.php index 896f993d2..1df093f99 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -46,15 +46,16 @@ private function loginAsAdmin($client): void } /** - * Get a valid CSRF token by first making a request to initialize the session. + * Extract a CSRF token from the rendered update manager page. */ - private function getCsrfToken($client, string $tokenId): string + private function getCsrfTokenFromPage($crawler, string $formAction): string { - // Make a GET request first to initialize the session - $client->request('GET', '/en/system/update-manager'); + $form = $crawler->filter('form[action*="' . $formAction . '"]'); + if ($form->count() === 0) { + $this->fail('Form with action containing "' . $formAction . '" not found on page'); + } - return $client->getContainer()->get('security.csrf.token_manager') - ->getToken($tokenId)->getValue(); + return $form->filter('input[name="_token"]')->attr('value'); } public function testIndexPageRequiresAuth(): void @@ -95,7 +96,9 @@ public function testCreateBackupWithValidCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - $csrfToken = $this->getCsrfToken($client, 'update_manager_backup'); + // Load the page and extract CSRF token from the backup form + $crawler = $client->request('GET', '/en/system/update-manager'); + $csrfToken = $this->getCsrfTokenFromPage($crawler, 'backup'); $client->request('POST', '/en/system/update-manager/backup', [ '_token' => $csrfToken, @@ -131,7 +134,7 @@ public function testDeleteBackupWithValidCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - // Create a temporary backup file to delete + // Create a temporary backup file so the page shows the delete form $backupManager = $client->getContainer()->get(BackupManager::class); $backupDir = $backupManager->getBackupDir(); if (!is_dir($backupDir)) { @@ -140,7 +143,9 @@ public function testDeleteBackupWithValidCsrf(): void $testFile = 'test-delete-' . uniqid() . '.zip'; file_put_contents($backupDir . '/' . $testFile, 'test'); - $csrfToken = $this->getCsrfToken($client, 'update_manager_delete'); + // Load the page and extract CSRF token from the delete form + $crawler = $client->request('GET', '/en/system/update-manager'); + $csrfToken = $this->getCsrfTokenFromPage($crawler, 'backup/delete'); $client->request('POST', '/en/system/update-manager/backup/delete', [ '_token' => $csrfToken, @@ -169,7 +174,7 @@ public function testDeleteLogWithValidCsrf(): void $client = static::createClient(); $this->loginAsAdmin($client); - // Create a temporary log file to delete + // Create a temporary log file so the page shows the delete form $projectDir = $client->getContainer()->getParameter('kernel.project_dir'); $logDir = $projectDir . '/var/log/updates'; if (!is_dir($logDir)) { @@ -178,7 +183,9 @@ public function testDeleteLogWithValidCsrf(): void $testFile = 'update-test-delete-' . uniqid() . '.log'; file_put_contents($logDir . '/' . $testFile, 'test log content'); - $csrfToken = $this->getCsrfToken($client, 'update_manager_delete'); + // Load the page and extract CSRF token from the log delete form + $crawler = $client->request('GET', '/en/system/update-manager'); + $csrfToken = $this->getCsrfTokenFromPage($crawler, 'log/delete'); $client->request('POST', '/en/system/update-manager/log/delete', [ '_token' => $csrfToken, @@ -251,13 +258,15 @@ 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 { - $csrfToken = $this->getCsrfToken($client, 'update_manager_backup'); - $client->request('POST', '/en/system/update-manager/backup', [ '_token' => $csrfToken, ]); 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 09/15] 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 | 167 ++++++++++++++---- translations/messages.en.xlf | 18 ++ 5 files changed, 255 insertions(+), 41 deletions(-) diff --git a/.env b/.env index 3ba3d65d7..447ff5dee 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 480e86a01..70be714d2 100644 --- a/src/Controller/UpdateManagerController.php +++ b/src/Controller/UpdateManagerController.php @@ -23,6 +23,7 @@ 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\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 @@ public function __construct( 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 @@ private function denyIfBackupRestoreDisabled(): void } } + /** + * 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 @@ public function index(): Response '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 @@ public function viewLog(string $filename): Response #[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 @@ public function backupDetails(string $filename): JsonResponse #[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 @@ public function createBackup(Request $request): Response } 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 @@ public function createBackup(Request $request): Response #[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 @@ public function deleteBackup(Request $request): Response #[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 @@ public function deleteLog(Request $request): Response /** * 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 @@ public function downloadBackup(string $filename): BinaryFileResponse #[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 4b6ff237d..7dcb813c8 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 1df093f99..c37d413e1 100644 --- a/tests/Controller/UpdateManagerControllerTest.php +++ b/tests/Controller/UpdateManagerControllerTest.php @@ -58,6 +58,8 @@ private function getCsrfTokenFromPage($crawler, string $formAction): string return $form->filter('input[name="_token"]')->attr('value'); } + // ---- Authentication tests ---- + public function testIndexPageRequiresAuth(): void { $client = static::createClient(); @@ -78,6 +80,8 @@ public function testIndexPageAccessibleByAdmin(): void $this->assertResponseIsSuccessful(); } + // ---- Backup creation tests ---- + public function testCreateBackupRequiresCsrf(): void { $client = static::createClient(); @@ -116,6 +120,33 @@ public function testCreateBackupWithValidCsrf(): void } } + 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 @@ public function testDeleteBackupWithValidCsrf(): void $this->assertFileDoesNotExist($backupDir . '/' . $testFile); } + // ---- Log deletion tests ---- + public function testDeleteLogRequiresCsrf(): void { $client = static::createClient(); @@ -196,39 +229,50 @@ public function testDeleteLogWithValidCsrf(): void $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->assertResponseStatusCodeSame(405); + } - $this->assertResponseIsSuccessful(); - $this->assertResponseHeaderSame('content-disposition', 'attachment; filename=' . $testFile); + public function testDownloadBackupRequiresAuth(): void + { + $client = static::createClient(); + + $client->request('POST', '/en/system/update-manager/backup/download', [ + '_token' => 'any', + 'filename' => 'test.zip', + 'password' => 'test', + ]); - // Clean up - @unlink($backupDir . '/' . $testFile); + // 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 @@ public function testBackupDetailsReturns404ForNonExistent(): void $this->assertResponseStatusCodeSame(404); } + // ---- Restore tests ---- + public function testRestoreBlockedWhenDisabled(): void { $client = static::createClient(); @@ -253,28 +299,83 @@ public function testRestoreBlockedWhenDisabled(): void $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 ac68c77ed..6ccc92d5b 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 From 877e3005bc3c1b9d172e2978dfd2b13951cc11b4 Mon Sep 17 00:00:00 2001 From: Sebastian Almberg <83243306+Sebbeben@users.noreply.github.com> Date: Fri, 6 Mar 2026 08:41:43 +0100 Subject: [PATCH 10/15] Fix download modal: use per-backup modals for CSP/Turbo compatibility - Replace shared modal + inline JS with per-backup modals that have filename pre-set in hidden fields (no JavaScript needed) - Add data-turbo="false" to download forms for native browser handling - Add data-bs-dismiss="modal" to submit button to auto-close modal - Add hidden username field for Chrome accessibility best practice - Fix test: GET on POST-only route returns 404 not 405 --- .../admin/update_manager/index.html.twig | 95 +++++++++---------- .../UpdateManagerControllerTest.php | 4 +- 2 files changed, 46 insertions(+), 53 deletions(-) diff --git a/templates/admin/update_manager/index.html.twig b/templates/admin/update_manager/index.html.twig index 7dcb813c8..76d7ca2ae 100644 --- a/templates/admin/update_manager/index.html.twig +++ b/templates/admin/update_manager/index.html.twig @@ -418,10 +418,9 @@
{% if not backup_download_disabled and is_granted('@system.manage_updates') %} @@ -457,6 +456,48 @@ {% endif %}
+ {% if not backup_download_disabled and is_granted('@system.manage_updates') %} + {# Per-backup download modal - no inline JS needed, CSP compatible with Turbo #} + + {% endif %}
- +
+ {# Current Version Card #} +
+
+
+ {% trans %}update_manager.current_installation{% endtrans %} +
+
+
+ - -
{% trans %}update_manager.version{% endtrans %} @@ -100,153 +102,159 @@
{% trans %}update_manager.auto_update_supported{% endtrans %} - {% if status.can_auto_update %} - - {% trans %}Yes{% endtrans %} - - {% else %} - - {% trans %}No{% endtrans %} - - {% endif %} + {{ helper.boolean_badge(status.can_auto_update) }}
-
-
- - - - + + + {% trans %}update_manager.web_updates_allowed{% endtrans %} + {{ helper.boolean_badge(not web_updates_disabled) }} + + + {% trans %}update_manager.backup_restore_allowed{% endtrans %} + {{ helper.boolean_badge(not backup_restore_disabled) }} + + + {% trans %}update_manager.backup_download_allowed{% endtrans %} + {{ helper.boolean_badge(not backup_download_disabled) }} + + + + +
+
+
+ + +
+
- - {# Latest Version / Update Card #} -
-
-
- {% if status.update_available %} - {% trans %}update_manager.new_version_available.title{% endtrans %} - {% else %} - {% trans %}update_manager.latest_release{% endtrans %} - {% endif %} -
-
- {% if status.latest_version %} -
+ {# Latest Version / Update Card #} +
+
+
+ {% if status.update_available %} + {% trans %}update_manager.new_version_available.title{% endtrans %} + {% else %} + {% trans %}update_manager.latest_release{% endtrans %} + {% endif %} +
+
+ {% if status.latest_version %} +
{{ status.latest_tag }} - {% if not status.update_available %} -

- - {% trans %}update_manager.already_up_to_date{% endtrans %} -

- {% endif %} -
+ {% if not status.update_available %} +

+ + {% trans %}update_manager.already_up_to_date{% endtrans %} +

+ {% endif %} +
- {% if status.update_available and status.can_auto_update and validation.valid and not web_updates_disabled %} -
- - + {% if status.update_available and status.can_auto_update and validation.valid and not web_updates_disabled %} + + + -
- -
+
+ +
-
- - -
-
- {% endif %} +
+ + +
+ + {% endif %} - {% if status.published_at %} -

- - {% trans %}update_manager.released{% endtrans %}: {{ status.published_at|date('Y-m-d') }} -

+ {% if status.published_at %} +

+ + {% trans %}update_manager.released{% endtrans %}: {{ status.published_at|date('Y-m-d') }} +

+ {% endif %} + {% else %} +
+ +

{% trans %}update_manager.could_not_fetch_releases{% endtrans %}

+
{% endif %} - {% else %} -
- -

{% trans %}update_manager.could_not_fetch_releases{% endtrans %}

+
+ {% if status.latest_tag %} +
+ + {% trans %}update_manager.view_release_notes{% endtrans %} + + {% if status.release_url %} + + GitHub + + {% endif %}
{% endif %}
- {% if status.latest_tag %} -
- - {% trans %}update_manager.view_release_notes{% endtrans %} - - {% if status.release_url %} - - GitHub - - {% endif %} -
- {% endif %}
-
- {# Validation Issues #} - {% if not validation.valid %} - - {% endif %} + {# Validation Issues #} + {% if not validation.valid %} + + {% endif %} - {# Non-auto-update installations info #} - {% if not status.can_auto_update %} -
-
- {% trans%}update_manager.cant_auto_update{% endtrans%}: {{ status.installation.type_name }} -
-

{{ status.installation.update_instructions }}

-
- {% endif %} + {# Non-auto-update installations info #} + {% if not status.can_auto_update %} +
+
+ {% trans%}update_manager.cant_auto_update{% endtrans%}: {{ status.installation.type_name }} +
+

{{ status.installation.update_instructions }}

+
+ {% endif %} -
- {# Available Versions #} -
-
-
- {% trans %}update_manager.available_versions{% endtrans %} -
-
-
- - +
+ {# Available Versions #} +
+
+
+ {% trans %}update_manager.available_versions{% endtrans %} +
+
+
+
+ - - + + {% for release in all_releases %} {% endfor %} - -
{% trans %}update_manager.version{% endtrans %} {% trans %}update_manager.released{% endtrans %}
@@ -280,8 +288,8 @@
+ + +
-
- {# Update History & Backups #} -
-
-
-
    -
  • - -
    • -
  • - -
    • -
-
-
-
-
-
- - + {# Update History & Backups #} +
+
+
+
    +
  • + +
    • +
  • + +
    • +
+
+
+
+
+
+
+ - - + + {% for log in update_logs %} {% endfor %} - -
{% trans %}update_manager.date{% endtrans %} {% trans %}update_manager.log_file{% endtrans %}
@@ -356,8 +364,8 @@ @@ -372,39 +380,39 @@
-
-
-
- {% if is_granted('@system.manage_updates') and not is_locked %} -
-
- - -
-
- {% endif %} - {% if is_docker %} -
- - {% trans %}update_manager.backup.docker_warning{% endtrans %} + +
- {% endif %} -
- - + +
+ {% if is_granted('@system.manage_updates') and not is_locked %} +
+
+ + + +
+ {% endif %} + {% if is_docker %} +
+ + {% trans %}update_manager.backup.docker_warning{% endtrans %} +
+ {% endif %} +
+
+ - - + + {% for backup in backups %} @@ -508,8 +516,9 @@ {% endfor %} - -
{% trans %}update_manager.date{% endtrans %} {% trans %}update_manager.file{% endtrans %} {% trans %}update_manager.size{% endtrans %}
@@ -418,16 +426,16 @@
{% if not backup_download_disabled and is_granted('@system.manage_updates') %} {% endif %} {% if not backup_restore_disabled and is_granted('@system.manage_updates') %}
@@ -449,8 +457,8 @@ @@ -458,46 +466,46 @@
{% if not backup_download_disabled and is_granted('@system.manage_updates') %} - {# Per-backup download modal - no inline JS needed, CSP compatible with Turbo #} -
+ + +
@@ -517,6 +526,5 @@
-
{% endblock %} diff --git a/templates/helper.twig b/templates/helper.twig index 9e68d56ce..e8c926e7e 100644 --- a/templates/helper.twig +++ b/templates/helper.twig @@ -1,8 +1,8 @@ {% macro boolean(value) %} {% if value %} - {% trans %}bool.true{% endtrans %} + {% trans %}Yes{% endtrans %} {% else %} - {% trans %}bool.false{% endtrans %} + {% trans %}No{% endtrans %} {% endif %} {% endmacro %} @@ -14,9 +14,9 @@ {% macro bool_icon(bool) %} {% if bool %} - + {% else %} - + {% endif %} {% endmacro %} @@ -24,7 +24,7 @@ {% if value %} {% set class = class ~ ' bg-success' %} {% else %} - {% set class = class ~ ' bg-danger' %} + {% set class = class ~ ' bg-secondary' %} {% endif %} {{ _self.bool_icon(value) }} {{ _self.boolean(value) }} diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index 923764346..bcd073313 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -12923,5 +12923,23 @@ Buerklin-API Authentication server: Cancel + + + update_manager.web_updates_allowed + Web updates allowed + + + + + update_manager.backup_restore_allowed + Backup restore allowed + + + + + update_manager.backup_download_allowed + Backup download allowed + + From 88279d35a6bf7650b9dc275792f634a80ea40881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Sat, 7 Mar 2026 19:30:32 +0100 Subject: [PATCH 15/15] Added documentation for update manager related env variables --- docs/configuration.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/configuration.md b/docs/configuration.md index c5e46f214..a2f585a1d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -144,6 +144,18 @@ bundled with Part-DB. Set `DATABASE_MYSQL_SSL_VERIFY_CERT` if you want to accept * `ALLOW_EMAIL_PW_RESET`: Set this value to true, if you want to allow users to reset their password via an email notification. You have to configure the mail provider first before via the MAILER_DSN setting. +### Update manager settings +* `DISABLE_WEB_UPDATES` (default `1`): Set this to 0 to enable web-based updates. When enabled, you can perform updates + via the web interface in the update manager. This is disabled by default for security reasons, as it can be a risk if + not used carefully. You can still use the CLI commands to perform updates, even when web updates are disabled. +* `DISABLE_BACKUP_RESTORE` (default `1`): Set this to 0 to enable backup restore via the web interface. When enabled, you can + restore backups via the web interface in the update manager. This is disabled by default for security reasons, as it can + be a risk if not used carefully. You can still use the CLI commands to perform backup restores, even when web-based + backup restore is disabled. +* `DISABLE_BACKUP_DOWNLOAD` (default `1`): Set this to 0 to enable backup download via the web interface. When enabled, you can download backups via the web interface + in the update manager. This is disabled by default for security reasons, as it can be a risk if not used carefully, as + the downloads contain sensitive data like password hashes or secrets. + ### Table related settings * `TABLE_DEFAULT_PAGE_SIZE`: The default page size for tables. This is the number of rows which are shown per page. Set