mirror of
https://github.com/Part-DB/Part-DB-server.git
synced 2026-05-19 18:01:30 +00:00
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
This commit is contained in:
Sebastian Almberg
parent
c16b6c7fac
commit
dd8698840d
5 changed files with 257 additions and 43 deletions
5
.env
5
.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
|
||||
###################################################################################
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
||||
|
|
|
|||
|
|
@ -416,12 +416,15 @@
|
|||
</td>
|
||||
<td class="text-end">
|
||||
<div class="btn-group btn-group-sm">
|
||||
{% if is_granted('@system.manage_updates') %}
|
||||
<a href="{{ path('admin_update_manager_backup_download', {filename: backup.file}) }}"
|
||||
class="btn btn-outline-secondary"
|
||||
title="{% trans %}update_manager.backup.download{% endtrans %}">
|
||||
{% if not backup_download_disabled and is_granted('@system.manage_updates') %}
|
||||
<button type="button"
|
||||
class="btn btn-outline-secondary btn-download-backup"
|
||||
data-filename="{{ backup.file }}"
|
||||
data-bs-toggle="modal"
|
||||
data-bs-target="#downloadBackupModal"
|
||||
title="{% trans %}update_manager.backup.download{% endtrans %}">
|
||||
<i class="fas fa-download"></i>
|
||||
</a>
|
||||
</button>
|
||||
{% endif %}
|
||||
{% if not backup_restore_disabled and is_granted('@system.manage_updates') %}
|
||||
<form action="{{ path('admin_update_manager_restore') }}" method="post" class="d-inline"
|
||||
|
|
@ -473,4 +476,53 @@
|
|||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
{# Password confirmation modal for backup download #}
|
||||
{% if not backup_download_disabled and is_granted('@system.manage_updates') %}
|
||||
<div class="modal fade" id="downloadBackupModal" tabindex="-1" aria-labelledby="downloadBackupModalLabel" aria-hidden="true">
|
||||
<div class="modal-dialog">
|
||||
<div class="modal-content">
|
||||
<form action="{{ path('admin_update_manager_backup_download') }}" method="post" id="downloadBackupForm">
|
||||
<div class="modal-header">
|
||||
<h5 class="modal-title" id="downloadBackupModalLabel">
|
||||
<i class="fas fa-download me-2"></i>{% trans %}update_manager.backup.download{% endtrans %}
|
||||
</h5>
|
||||
<button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
|
||||
</div>
|
||||
<div class="modal-body">
|
||||
<div class="alert alert-warning">
|
||||
<i class="fas fa-exclamation-triangle me-1"></i>
|
||||
{% trans %}update_manager.backup.download.security_warning{% endtrans %}
|
||||
</div>
|
||||
<input type="hidden" name="_token" value="{{ csrf_token('update_manager_download') }}">
|
||||
<input type="hidden" name="filename" id="downloadBackupFilename" value="">
|
||||
<div class="mb-3">
|
||||
<label for="downloadBackupPassword" class="form-label">
|
||||
{% trans %}update_manager.backup.download.password_label{% endtrans %}
|
||||
</label>
|
||||
<input type="password" class="form-control" id="downloadBackupPassword"
|
||||
name="password" required autocomplete="current-password">
|
||||
</div>
|
||||
</div>
|
||||
<div class="modal-footer">
|
||||
<button type="button" class="btn btn-secondary" data-bs-dismiss="modal">
|
||||
{% trans %}cancel{% endtrans %}
|
||||
</button>
|
||||
<button type="submit" class="btn btn-primary">
|
||||
<i class="fas fa-download me-1"></i>{% trans %}update_manager.backup.download{% endtrans %}
|
||||
</button>
|
||||
</div>
|
||||
</form>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
<script nonce="{{ csp_nonce('script') }}">
|
||||
document.getElementById('downloadBackupModal').addEventListener('show.bs.modal', function (event) {
|
||||
var button = event.relatedTarget;
|
||||
var filename = button.getAttribute('data-filename');
|
||||
document.getElementById('downloadBackupFilename').value = filename;
|
||||
document.getElementById('downloadBackupPassword').value = '';
|
||||
});
|
||||
</script>
|
||||
{% endif %}
|
||||
{% endblock %}
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12563,6 +12563,24 @@ Buerklin-API Authentication server:
|
|||
<target>Download backup</target>
|
||||
</segment>
|
||||
</unit>
|
||||
<unit id="um_bk_download_pw" name="update_manager.backup.download.password_label">
|
||||
<segment state="translated">
|
||||
<source>update_manager.backup.download.password_label</source>
|
||||
<target>Confirm your password to download</target>
|
||||
</segment>
|
||||
</unit>
|
||||
<unit id="um_bk_download_warn" name="update_manager.backup.download.security_warning">
|
||||
<segment state="translated">
|
||||
<source>update_manager.backup.download.security_warning</source>
|
||||
<target>Backups contain sensitive data including password hashes and secrets. Please confirm your password to proceed with the download.</target>
|
||||
</segment>
|
||||
</unit>
|
||||
<unit id="um_bk_download_invalid_pw" name="update_manager.backup.download.invalid_password">
|
||||
<segment state="translated">
|
||||
<source>update_manager.backup.download.invalid_password</source>
|
||||
<target>Invalid password. Backup download denied.</target>
|
||||
</segment>
|
||||
</unit>
|
||||
<unit id="um_bk_docker_warning" name="update_manager.backup.docker_warning">
|
||||
<segment state="translated">
|
||||
<source>update_manager.backup.docker_warning</source>
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue