diff --git a/app/Controllers/UpdateController.php b/app/Controllers/UpdateController.php index 12881c79f..b8b244b05 100644 --- a/app/Controllers/UpdateController.php +++ b/app/Controllers/UpdateController.php @@ -141,7 +141,7 @@ public function createBackup(Request $request, Response $response, mysqli $db): } $scope = (($data['scope'] ?? 'full') === 'db') ? 'db' : 'full'; - $result = (new BackupManager($db, dirname(__DIR__, 2)))->createBackup($scope); + $result = (new BackupManager($db, dirname(__DIR__, 2)))->createBackup($scope, BackupManager::ORIGIN_MANUAL); if ($result['success']) { return $this->jsonResponse($response, [ diff --git a/app/Support/BackupManager.php b/app/Support/BackupManager.php index 4db01da3e..cc99d0754 100644 --- a/app/Support/BackupManager.php +++ b/app/Support/BackupManager.php @@ -51,6 +51,39 @@ class BackupManager */ private const GENERATED_NAME_PATTERN = '/^backup_\d{4}-\d{2}-\d{2}_\d{6}_[0-9a-f]{6}\.zip$/'; + /** + * Where a backup came from. Only ORIGIN_AUTO — the copy taken automatically + * before an update — is subject to rotation. + * + * A backup the operator ASKED for is not interchangeable with one the system + * took on its own: it exists because someone decided, at that moment, that + * this state was worth keeping, usually right before doing something risky. + * Letting ten automatic pre-update copies evict it turns a deliberate + * restore point into a rolling window, which is not what the button + * promises. The same holds, more strongly, for the safety copy taken before + * a restore: that one IS the undo. + * + * The origin is encoded in the FILENAME, not only in the manifest, because + * the rotation has to decide from a glob — reading a manifest means opening + * every archive. A non-auto name simply falls outside + * GENERATED_NAME_PATTERN, so it is excluded by the rule that was already + * there for hand-placed archives, with no second rule to keep in sync. + */ + public const ORIGIN_AUTO = 'auto'; + public const ORIGIN_MANUAL = 'manual'; + public const ORIGIN_SAFETY = 'safety'; + public const ORIGIN_UPLOAD = 'upload'; + + /** + * The pre-0.7.x layout: a directory holding a single database.sql, written + * by an updater that no longer exists. Nothing creates these any more, but + * listBackups() still surfaces them as backups (contents: 'db'), so an + * operator sees them in the same list — and until now nothing pruned them. + * Same discipline as the pattern above: match the EXACT generated shape, so + * a directory someone parked there by hand is never a rotation candidate. + */ + private const LEGACY_DIR_PATTERN = '/^update_\d{4}-\d{2}-\d{2}_\d{6}$/'; + /** * Hard cap for the cumulative DECOMPRESSED size of a restore archive (4 GB). * Guards against a decompression-bomb ZIP whose compressed form passes @@ -75,9 +108,29 @@ public function __construct(mysqli $db, string $rootPath) * @param string $scope 'full' (DB + files) or 'db' (database only) * @return array{success: bool, name: string|null, path: string|null, size: int, error: string|null} */ - public function createBackup(string $scope = 'full'): array + /** + * Build a backup filename carrying its origin. + * + * The automatic shape is left EXACTLY as it was, so every archive already on + * disk keeps being recognised — and keeps being rotated. Anything else gets + * an origin suffix, which puts it outside GENERATED_NAME_PATTERN and thus + * outside the rotation, without a second exclusion rule to maintain. + */ + private static function backupFileName(string $timestamp, string $origin): string + { + $suffix = bin2hex(random_bytes(3)); + if ($origin === self::ORIGIN_AUTO) { + return 'backup_' . $timestamp . '_' . $suffix . '.zip'; + } + return 'backup_' . $timestamp . '_' . $suffix . '_' . $origin . '.zip'; + } + + public function createBackup(string $scope = 'full', string $origin = self::ORIGIN_AUTO): array { $scope = $scope === 'db' ? 'db' : 'full'; + if (!in_array($origin, [self::ORIGIN_AUTO, self::ORIGIN_MANUAL, self::ORIGIN_SAFETY], true)) { + return ['success' => false, 'name' => null, 'path' => null, 'size' => 0, 'error' => __('Origine backup non valida')]; + } $sqlTmp = null; try { @@ -98,7 +151,7 @@ public function createBackup(string $scope = 'full'): array // A random suffix avoids collisions when two backups land in the // same second (e.g. a manual backup + the pre-restore safety backup). $timestamp = date('Y-m-d_His'); - $name = 'backup_' . $timestamp . '_' . bin2hex(random_bytes(3)) . '.zip'; + $name = self::backupFileName($timestamp, $origin); $zipPath = $this->backupPath . '/' . $name; // 1. Dump the database to a temp file. @@ -130,6 +183,7 @@ public function createBackup(string $scope = 'full'): array 'version' => $this->getCurrentVersion(), 'created_at' => date('c'), 'scope' => $scope, + 'origin' => $origin, 'tables' => $tableCount, 'files' => $fileCount, 'database_sha256' => hash_file('sha256', $sqlTmp) ?: '', @@ -242,7 +296,10 @@ private function pruneOldBackups(string $justWritten): void return; } - $files = []; + // One pool across BOTH formats, because listBackups() shows them as + // one list sorted by date: rotating them separately would let the + // operator watch a recent entry vanish while an older one survives. + $entries = []; foreach (glob($this->backupPath . '/backup_*.zip') ?: [] as $file) { if (!is_file($file) || realpath($file) === realpath($justWritten)) { continue; @@ -250,22 +307,55 @@ private function pruneOldBackups(string $justWritten): void if (preg_match(self::GENERATED_NAME_PATTERN, basename($file)) !== 1) { continue; // hand-placed archive: never a rotation candidate } - $files[$file] = (int) filemtime($file); + $entries[$file] = (int) filemtime($file); } - arsort($files); + foreach (glob($this->backupPath . '/update_*', GLOB_ONLYDIR) ?: [] as $dir) { + // A symlink must never be a rotation candidate: deleteDirectory() + // would unlink the link, but a link is not something this class + // wrote and not ours to reclaim. + if (is_link($dir) || !is_dir($dir)) { + continue; + } + if (preg_match(self::LEGACY_DIR_PATTERN, basename($dir)) !== 1) { + continue; // hand-placed directory: never a rotation candidate + } + // A generated legacy backup IS its database.sql — that is the + // whole content of the format. A directory carrying the name but + // not the dump is something else wearing our shape, and this + // rotation deletes recursively: reclaiming only what we can show + // we wrote is worth one stat() per candidate. + if (!is_file($dir . '/database.sql')) { + continue; + } + $entries[$dir] = (int) filemtime($dir); + } + arsort($entries); - // The freshly written file counts against the quota too. - $slots = max(0, $keep - 1); - $stale = array_slice(array_keys($files), $slots); + // Only a newly written automatic backup occupies a retention slot. + $automatic = preg_match(self::GENERATED_NAME_PATTERN, basename($justWritten)) === 1; + $slots = max(0, $keep - ($automatic ? 1 : 0)); + $stale = array_slice(array_keys($entries), $slots); $removed = 0; - foreach ($stale as $file) { + $removedLegacy = 0; + foreach ($stale as $path) { + if (is_dir($path)) { + if ($this->deleteDirectory($path)) { + $removed++; + $removedLegacy++; + } + continue; + } // nosemgrep: php.lang.security.unlink-use.unlink-use -- glob-matched backup_*.zip under storage/backups, not user input - if (@unlink($file)) { + if (@unlink($path)) { $removed++; } } if ($removed > 0) { - SecureLogger::info('BackupManager: rotated old backups', ['removed' => $removed, 'kept' => $keep]); + SecureLogger::info('BackupManager: rotated old backups', [ + 'removed' => $removed, + 'legacy_dirs' => $removedLegacy, + 'kept' => $keep, + ]); } } catch (\Throwable $e) { SecureLogger::warning('BackupManager: backup rotation failed', ['error' => $e->getMessage()]); @@ -275,6 +365,27 @@ private function pruneOldBackups(string $justWritten): void /** * @return array */ + /** + * Origin encoded in a filename, for archives whose manifest predates it. + * Unknown or absent suffix means the automatic shape, which is what every + * archive written before this existed actually was. + */ + private static function originFromName(string $name): string + { + if (preg_match('/^backup_\\d{4}-\\d{2}-\\d{2}_\\d{6}_[0-9a-f]{6}_([a-z]+)\\.zip$/', $name, $m) === 1) { + return in_array($m[1], [self::ORIGIN_MANUAL, self::ORIGIN_SAFETY, self::ORIGIN_UPLOAD], true) ? $m[1] : self::ORIGIN_AUTO; + } + return self::ORIGIN_AUTO; + } + + /** Human date label, with any origin suffix stripped out of it. */ + private static function backupDateLabel(string $name): string + { + $base = pathinfo($name, PATHINFO_FILENAME); + $base = preg_replace('/_(?:' . self::ORIGIN_MANUAL . '|' . self::ORIGIN_SAFETY . '|' . self::ORIGIN_UPLOAD . ')$/', '', $base) ?? $base; + return str_replace(['backup_', '_'], ['', ' '], $base); + } + public function listBackups(): array { $backups = []; @@ -290,8 +401,17 @@ public function listBackups(): array 'name' => $name, 'path' => $file, 'size' => (int) filesize($file), - 'date' => str_replace(['backup_', '_'], ['', ' '], pathinfo($name, PATHINFO_FILENAME)), + 'date' => self::backupDateLabel($name), 'contents' => (string) ($manifest['scope'] ?? 'full'), + // The name wins for uploads, and only for them: an uploaded + // archive carries the manifest of ANOTHER installation, which + // would claim 'auto' and hand a restore point to the rotation. + // Everywhere else the manifest is the source and the name the + // fallback — archives written before origins existed carry + // neither, and default to auto, which is what they were. + 'origin' => self::originFromName($name) === self::ORIGIN_UPLOAD + ? self::ORIGIN_UPLOAD + : (string) ($manifest['origin'] ?? self::originFromName($name)), 'created_at' => (int) filemtime($file), ]; } @@ -306,6 +426,7 @@ public function listBackups(): array 'size' => is_file($dbFile) ? (int) filesize($dbFile) : 0, 'date' => str_replace(['update_', '_'], ['', ' '], $name), 'contents' => 'db', + 'origin' => self::ORIGIN_AUTO, 'created_at' => (int) filemtime($dir), ]; } @@ -325,10 +446,13 @@ public function deleteBackup(string $name): array } try { if (is_dir($target)) { - $this->deleteDirectory($target); + $deleted = $this->deleteDirectory($target); } else { // nosemgrep: php.lang.security.unlink-use.unlink-use -- $target validated by resolveBackup() (no traversal, realpath under storage/backups) - @unlink($target); + $deleted = @unlink($target); + } + if (!$deleted) { + return ['success' => false, 'error' => __('Impossibile eliminare il backup. Verifica i permessi e riprova.')]; } return ['success' => true, 'error' => null]; } catch (\Throwable $e) { @@ -408,7 +532,7 @@ public function restoreFromUploadedZip(string $tmpPath, int $size): array // Same naming scheme as createBackup() so the uploaded archive is listed // and deletable like any other backup; the random suffix avoids the // same-second collision a plain timestamp would allow. - $dest = $this->backupPath . '/backup_' . date('Y-m-d_His') . '_' . bin2hex(random_bytes(3)) . '.zip'; + $dest = $this->backupPath . '/' . self::backupFileName(date('Y-m-d_His'), self::ORIGIN_UPLOAD); if (!@rename($tmpPath, $dest) && !@copy($tmpPath, $dest)) { return ['success' => false, 'safety_backup' => null, 'error' => __('Impossibile salvare il file caricato')]; } @@ -525,7 +649,7 @@ private function doRestoreZip(string $zipPath): array try { // 1. Safety backup of the current state (always full) — the rollback // path, since MySQL DDL can't run inside a transaction. - $safety = $this->createBackup('full'); + $safety = $this->createBackup('full', self::ORIGIN_SAFETY); if (!$safety['success']) { throw new \RuntimeException(__('Impossibile creare il backup di sicurezza pre-ripristino') . ': ' . (string) $safety['error']); } @@ -1420,22 +1544,21 @@ private function readManifestFromZip(string $zipPath): array return is_array($decoded) ? $decoded : []; } - private function deleteDirectory(string $dir): void + private function deleteDirectory(string $dir): bool { // A symlinked root must not be followed either — unlink the link // itself, never recurse into its target (symmetric with the per-child // is_link guard below; is_dir() returns true through a dir symlink). (#167 review) if (is_link($dir)) { // nosemgrep: php.lang.security.unlink-use.unlink-use -- removes the symlink, not its target - @unlink($dir); - return; + return @unlink($dir); } if (!is_dir($dir)) { - return; + return true; // already gone: the postcondition holds } $files = @scandir($dir); if ($files === false) { - return; + return false; } foreach (array_diff($files, ['.', '..']) as $file) { $path = $dir . '/' . $file; @@ -1450,7 +1573,11 @@ private function deleteDirectory(string $dir): void @unlink($path); } } - @rmdir($dir); + // The return value is what the rotation counts on. Re-checking the path + // afterwards would be the obvious alternative, but static analysis has + // already narrowed it to "a directory" and cannot see a filesystem side + // effect, so the helper reports its own outcome instead. + return @rmdir($dir); } private function getCurrentVersion(): string diff --git a/locale/da_DK.json b/locale/da_DK.json index 1960d1135..1c0161c09 100644 --- a/locale/da_DK.json +++ b/locale/da_DK.json @@ -1,4 +1,6 @@ { + "Origine backup non valida": "Ugyldig sikkerhedskopikilde", + "Impossibile eliminare il backup. Verifica i permessi e riprova.": "Sikkerhedskopien kunne ikke slettes. Kontrollér rettighederne, og prøv igen.", "Importa UNIMARC (MARCXchange)": "Importér UNIMARC (MARCXchange)", "Incolla un record UNIMARC/MARCXchange, oppure carica un file .xml, per precompilare il form dal record esportato.": "Indsæt en UNIMARC/MARCXchange-post, eller upload en .xml-fil, for at udfylde formularen ud fra den eksporterede post.", "Incolla o carica un record UNIMARC.": "Indsæt eller upload en UNIMARC-post.", diff --git a/locale/de_DE.json b/locale/de_DE.json index 93dc1b40e..111df1547 100644 --- a/locale/de_DE.json +++ b/locale/de_DE.json @@ -1,4 +1,6 @@ { + "Origine backup non valida": "Ungültiger Sicherungsursprung", + "Impossibile eliminare il backup. Verifica i permessi e riprova.": "Die Sicherung kann nicht gelöscht werden. Prüfen Sie die Berechtigungen und versuchen Sie es erneut.", "Importa UNIMARC (MARCXchange)": "UNIMARC importieren (MARCXchange)", "Incolla un record UNIMARC/MARCXchange, oppure carica un file .xml, per precompilare il form dal record esportato.": "Fügen Sie einen UNIMARC/MARCXchange-Datensatz ein oder laden Sie eine .xml-Datei hoch, um das Formular aus dem exportierten Datensatz vorauszufüllen.", "Incolla o carica un record UNIMARC.": "Fügen Sie einen UNIMARC-Datensatz ein oder laden Sie ihn hoch.", diff --git a/locale/en_US.json b/locale/en_US.json index ead22168a..f014ef8f6 100644 --- a/locale/en_US.json +++ b/locale/en_US.json @@ -1,4 +1,6 @@ { + "Origine backup non valida": "Invalid backup origin", + "Impossibile eliminare il backup. Verifica i permessi e riprova.": "Unable to delete the backup. Check permissions and try again.", "Importa UNIMARC (MARCXchange)": "Import UNIMARC (MARCXchange)", "Incolla un record UNIMARC/MARCXchange, oppure carica un file .xml, per precompilare il form dal record esportato.": "Paste a UNIMARC/MARCXchange record, or upload an .xml file, to pre-fill the form from the exported record.", "Incolla o carica un record UNIMARC.": "Paste or upload a UNIMARC record.", diff --git a/locale/fr_FR.json b/locale/fr_FR.json index 206da771f..3a17d7dca 100644 --- a/locale/fr_FR.json +++ b/locale/fr_FR.json @@ -1,4 +1,6 @@ { + "Origine backup non valida": "Origine de sauvegarde non valide", + "Impossibile eliminare il backup. Verifica i permessi e riprova.": "Impossible de supprimer la sauvegarde. Vérifiez les permissions et réessayez.", "Importa UNIMARC (MARCXchange)": "Importer UNIMARC (MARCXchange)", "Incolla un record UNIMARC/MARCXchange, oppure carica un file .xml, per precompilare il form dal record esportato.": "Collez une notice UNIMARC/MARCXchange, ou téléversez un fichier .xml, pour préremplir le formulaire à partir de la notice exportée.", "Incolla o carica un record UNIMARC.": "Collez ou téléversez une notice UNIMARC.", diff --git a/locale/it_IT.json b/locale/it_IT.json index ae3aa8f02..f5fa1c85d 100644 --- a/locale/it_IT.json +++ b/locale/it_IT.json @@ -1,4 +1,6 @@ { + "Origine backup non valida": "Origine backup non valida", + "Impossibile eliminare il backup. Verifica i permessi e riprova.": "Impossibile eliminare il backup. Verifica i permessi e riprova.", "Importa UNIMARC (MARCXchange)": "Importa UNIMARC (MARCXchange)", "Incolla un record UNIMARC/MARCXchange, oppure carica un file .xml, per precompilare il form dal record esportato.": "Incolla un record UNIMARC/MARCXchange, oppure carica un file .xml, per precompilare il form dal record esportato.", "Incolla o carica un record UNIMARC.": "Incolla o carica un record UNIMARC.", diff --git a/storage/plugins/api-book-scraper/ApiBookScraperPlugin.php b/storage/plugins/api-book-scraper/ApiBookScraperPlugin.php index 1745c299f..9dccafada 100644 --- a/storage/plugins/api-book-scraper/ApiBookScraperPlugin.php +++ b/storage/plugins/api-book-scraper/ApiBookScraperPlugin.php @@ -139,9 +139,18 @@ private function registerHooks(): void return; } - // Se il plugin non è abilitato, non registrare gli hooks + // Se il plugin non è abilitato, non registrare gli hooks. + // + // DEBUG, not warning: a plugin that is present but not configured is the + // normal state of every plugin the operator has not set up, and this path + // is reached on a schedule. On one production install it produced 8.217 of + // the 15.607 lines in app.log over eight months — more than half the file, + // for a condition that is not a problem. A warning level that fires + // continuously trains the reader to skip warnings, which is how the real + // ones get missed. The genuinely abnormal case above (missing DB or plugin + // ID) keeps its warning. if (!$this->enabled || empty($this->apiEndpoint) || empty($this->apiKey)) { - \App\Support\SecureLogger::warning('[ApiBookScraper] Plugin not enabled or missing configuration'); + \App\Support\SecureLogger::debug('[ApiBookScraper] Plugin not enabled or missing configuration: hooks not registered'); return; } diff --git a/tests/activity-feed-374.spec.js b/tests/activity-feed-374.spec.js index 7c40c990c..34418fffe 100644 --- a/tests/activity-feed-374.spec.js +++ b/tests/activity-feed-374.spec.js @@ -192,8 +192,17 @@ test.describe.serial('Activity feed (#374)', () => { expect(Number(await page.locator('#libro_id').inputValue())).toBe(Number(bookId)); const appLoanDate = await page.locator('#data_prestito').inputValue(); expect(appLoanDate).toMatch(/^\d{4}-\d{2}-\d{2}$/); - await page.locator('form button[type="submit"]').first().click(); - await page.waitForURL((url) => url.searchParams.get('created') === '1', { timeout: 20000 }); + // The PDF auto-download removes created=1 with replaceState immediately. + // Assert the server redirect before waiting for the stable destination. + const [creationResponse] = await Promise.all([ + page.waitForResponse((response) => response.request().method() === 'POST' + && new URL(response.url()).pathname.endsWith('/admin/loans/create')), + page.locator('form button[type="submit"]').first().click(), + ]); + expect(creationResponse.status()).toBe(302); + const destination = new URL(creationResponse.headers().location, BASE); + expect(destination.searchParams.get('created')).toBe('1'); + await page.waitForURL((url) => url.pathname === destination.pathname, { timeout: 20000 }); // DB truth: the production allocator bound OUR physical copy to the loan. const loanCopy = dbQuery( diff --git a/tests/backup-restore-origins.unit.php b/tests/backup-restore-origins.unit.php new file mode 100644 index 000000000..aaacd0b89 --- /dev/null +++ b/tests/backup-restore-origins.unit.php @@ -0,0 +1,109 @@ +calls[] = [$scope, $origin]; + return ['success' => false, 'name' => null, 'path' => null, 'size' => 0, 'error' => 'test stop']; + } +} + +$root = sys_get_temp_dir() . '/backup_origins_' . bin2hex(random_bytes(6)); +mkdir($root . '/storage/backups', 0775, true); +$db = new mysqli(); // no connection is needed before the safety-backup boundary +$manager = new OriginRecordingBackupManager($db, $root); +$failed = 0; +$passed = 0; +$check = static function (bool $ok, string $label) use (&$failed, &$passed): void { + echo ($ok ? 'OK ' : 'FAIL ') . $label . "\n"; + $ok ? $passed++ : $failed++; +}; +$cleanup = static function (string $dir) use (&$cleanup): void { + foreach (glob($dir . '/*') ?: [] as $p) { + if (is_dir($p)) { + chmod($p, 0755); + $cleanup($p); + } else { + unlink($p); + } + } + rmdir($dir); +}; +try { + $archive = $root . '/storage/backups/backup_2026-01-01_000000_abcdef.zip'; + $zip = new ZipArchive(); + $zip->open($archive, ZipArchive::CREATE); + $zip->addFromString('manifest.json', json_encode(['scope' => 'db', 'origin' => 'safety'])); + $zip->addFromString('database.sql', '-- never imported'); + $zip->close(); + $result = $manager->restoreFromBackup(basename($archive)); + $check(!$result['success'], 'stored restore aborts when its safety backup fails'); + $check($manager->calls === [['full', BackupManager::ORIGIN_SAFETY]], 'stored restore requests a full safety backup'); + + $upload = $root . '/upload.zip'; + copy($archive, $upload); + $result = $manager->restoreFromUploadedZip($upload, (int) filesize($upload)); + $check(!$result['success'], 'uploaded restore aborts when its safety backup fails'); + $check($manager->calls[1] === ['full', BackupManager::ORIGIN_SAFETY], 'uploaded restore requests a full safety backup'); + $uploads = glob($root . '/storage/backups/*_upload.zip') ?: []; + $check(count($uploads) === 1, 'uploaded archive has its own preserved origin'); + $listed = array_column($manager->listBackups(), null, 'name'); + $check(($listed[basename($uploads[0] ?? '')]['origin'] ?? '') === BackupManager::ORIGIN_UPLOAD, + 'upload origin overrides the original manifest origin'); + + $legacy = $root . '/storage/backups/update_2026-01-01_000000'; + mkdir($legacy); + file_put_contents($legacy . '/database.sql', 'x'); + $GLOBALS['backupFailedUnlink'] = $legacy . '/database.sql'; + $result = $manager->deleteBackup(basename($legacy)); + $check(!$result['success'] && $result['error'] !== null, 'an injected recursive deletion failure is reported, including under root'); + $check(is_file($legacy . '/database.sql'), 'the failed child prevents removal of the legacy directory'); + unset($GLOBALS['backupFailedUnlink']); + + // Also exercise real mode-bit enforcement when the operating system applies + // it. Running as root the bits are advisory and this cannot be provoked — + // which is why it is a note and not a failed assertion: the injected failure + // above already covers the requirement, uid and mode bits notwithstanding. + chmod($legacy, 0555); + if (!is_writable($legacy)) { + $result = $manager->deleteBackup(basename($legacy)); + $check(!$result['success'] && $result['error'] !== null, 'failed recursive deletion is reported as failure'); + $check(is_file($legacy . '/database.sql'), 'failed deletion leaves the fixture available for retry'); + } else { + // Indented on purpose: ci-run-unit-tests.sh reads 'SKIP:' at column 0, + // and this is not a skipped requirement. + echo " note: mode bits are not enforced for this user; the requirement is covered by the injected failure above\n"; + } + chmod($legacy, 0755); + $result = $manager->deleteBackup(basename($legacy)); + $check($result['success'] && !is_dir($legacy), 'retry deletes the directory and its contents'); + $result = $manager->deleteBackup(basename($archive)); + $check($result['success'] && !is_file($archive), 'archive deletion succeeds and removes the file'); +} finally { + unset($GLOBALS['backupFailedUnlink']); + $cleanup($root); +} +echo "Passed: $passed Failed: $failed\n"; +exit($failed ? 1 : 0); +} diff --git a/tests/backup-retention.unit.php b/tests/backup-retention.unit.php index 1ef98d2ea..5327edc61 100644 --- a/tests/backup-retention.unit.php +++ b/tests/backup-retention.unit.php @@ -98,6 +98,13 @@ mkdir($tmp, 0777, true); $cleanup = static function () use ($tmp, &$origRetention, &$setRetention): void { foreach (glob($tmp . '/*') ?: [] as $f) { + if (is_dir($f)) { + foreach (glob($f . '/*') ?: [] as $inner) { + @unlink($inner); + } + @rmdir($f); + continue; + } @unlink($f); } @rmdir($tmp); @@ -193,6 +200,176 @@ $check($kept === BackupManager::DEFAULT_RETENTION, 'the default retention applies when the setting cannot be read (kept ' . $kept . ')'); +echo "E. the legacy directory format is rotated too\n"; +// Pre-0.7.x updates left a directory holding a single database.sql. Nothing +// creates them any more, but listBackups() still shows them as backups — and the +// rotation only ever globbed backup_*.zip, so they accumulated forever. One +// production install had 60 of them, 17 MB, spanning six months. +foreach (glob($tmp . '/*') ?: [] as $f) { + if (is_dir($f)) { + foreach (glob($f . '/*') ?: [] as $inner) { @unlink($inner); } + @rmdir($f); + } else { + @unlink($f); + } +} + +/** Seed n legacy update_ directories, oldest first. */ +$seedLegacy = static function (int $n) use ($tmp): array { + $paths = []; + for ($i = 0; $i < $n; $i++) { + $d = $tmp . '/update_2025-12-' . str_pad((string) ($i + 1), 2, '0', STR_PAD_LEFT) . '_000000'; + @mkdir($d, 0775, true); + file_put_contents($d . '/database.sql', 'x'); + touch($d, time() - ((100 - $i) * 3600)); + $paths[] = $d; + } + return $paths; +}; + +$legacy = $seedLegacy(8); +$zips = $seed(4); +$newest = end($zips); +$manager = $makeManager($tmp, '5'); +$prune($manager, $newest); + +$survivingZips = glob($tmp . '/backup_*.zip') ?: []; +$survivingDirs = glob($tmp . '/update_*', GLOB_ONLYDIR) ?: []; +$check(count($survivingZips) + count($survivingDirs) === 5, + 'both formats share one retention pool (got ' . count($survivingZips) . ' zip + ' . count($survivingDirs) . ' dir)'); +$check(in_array($newest, $survivingZips, true), 'the backup just written still survives'); +// The zips were seeded newer than every legacy directory, so with five slots the +// four zips plus the single newest directory must be what remains. +$check(count($survivingZips) === 4, 'the four recent archives all survive'); +$check($survivingDirs === [end($legacy)], 'only the newest legacy directory survives, by age'); +$check(!is_dir($legacy[0]), 'a rotated legacy directory is removed with its contents'); + +echo "F. hand-placed directories are never touched\n"; +foreach (glob($tmp . '/*') ?: [] as $f) { + if (is_dir($f)) { + foreach (glob($f . '/*') ?: [] as $inner) { @unlink($inner); } + @rmdir($f); + } else { + @unlink($f); + } +} +$seedLegacy(6); +// Same discipline as the archive pattern: only the EXACT generated shape is a +// candidate. A directory an operator parked here by hand must survive whatever +// the retention says. +$manual = $tmp . '/update_migrazione_manuale'; +@mkdir($manual, 0775, true); +file_put_contents($manual . '/database.sql', 'x'); +touch($manual, time() - (999 * 3600)); // older than every seeded one +$zips = $seed(2); +$manager = $makeManager($tmp, '2'); +$prune($manager, end($zips)); +$check(is_dir($manual), 'a hand-named directory is never a rotation candidate'); +$check(is_file($manual . '/database.sql'), 'and its contents are left alone'); + +echo "G. a backup the operator asked for is never rotated away\n"; +// The manual button and the automatic pre-update copy used to produce identical +// names, so ten automatic backups would evict a restore point someone created on +// purpose — usually right before doing something risky. The origin now lives in +// the filename, which is what the rotation can see from a glob. +foreach (glob($tmp . '/*') ?: [] as $f) { + if (is_dir($f)) { + foreach (glob($f . '/*') ?: [] as $inner) { @unlink($inner); } + @rmdir($f); + } else { + @unlink($f); + } +} + +$nameFor = new ReflectionMethod(BackupManager::class, 'backupFileName'); +$nameFor->setAccessible(true); + +$manual = $tmp . '/' . $nameFor->invoke(null, '2025-01-01_000000', BackupManager::ORIGIN_MANUAL); +$safety = $tmp . '/' . $nameFor->invoke(null, '2025-01-02_000000', BackupManager::ORIGIN_SAFETY); +foreach ([$manual, $safety] as $i => $path) { + file_put_contents($path, 'x'); + touch($path, time() - ((900 - $i) * 3600)); // older than every automatic one +} + +$autos = $seed(12); +$manager = $makeManager($tmp, '3'); +$prune($manager, end($autos)); + +$check(is_file($manual), 'a backup created from the button survives the rotation'); +$check(is_file($safety), 'the safety copy taken before a restore survives too'); +$survivingAuto = array_values(array_filter(glob($tmp . '/backup_*.zip') ?: [], + static fn(string $f): bool => !str_contains($f, '_manual.') && !str_contains($f, '_safety.'))); +$check(count($survivingAuto) === 3, + 'the automatic ones are still rotated to the configured count (got ' . count($survivingAuto) . ')'); +// And they must not consume the quota either: the retention counts automatic +// copies, so a hoard of manual ones cannot starve the rolling window. +$check(count($survivingAuto) === 3 && is_file($manual) && is_file($safety), + 'preserved backups do not consume rotation slots'); + +echo "H. the origin survives a round trip through the list\n"; +$listed = $manager->listBackups(); +$byName = []; +foreach ($listed as $row) { + $byName[$row['name']] = $row; +} +$check(($byName[basename($manual)]['origin'] ?? null) === BackupManager::ORIGIN_MANUAL, + 'the list reports a manual backup as manual'); +$check(($byName[basename($safety)]['origin'] ?? null) === BackupManager::ORIGIN_SAFETY, + 'the list reports the safety copy as such'); +$anAuto = basename((string) end($survivingAuto)); +$check(($byName[$anAuto]['origin'] ?? null) === BackupManager::ORIGIN_AUTO, + 'an archive with no origin recorded reads as automatic, which is what it was'); +// The suffix must not leak into the date shown to the operator. +$check(!str_contains((string) ($byName[basename($manual)]['date'] ?? ''), 'manual'), + 'the origin suffix stays out of the displayed date'); + +echo "I. a directory wearing the name but not the content is left alone\n"; +foreach (glob($tmp . '/*') ?: [] as $f) { + if (is_dir($f)) { + foreach (glob($f . '/*') ?: [] as $inner) { @unlink($inner); } + @rmdir($f); + } else { + @unlink($f); + } +} +// A generated legacy backup IS its database.sql. A directory that matches the +// name but has no dump is something else wearing our shape — and this rotation +// deletes recursively, so getting it wrong destroys whatever is inside. +$impostor = $tmp . '/update_2019-03-03_030303'; +@mkdir($impostor, 0775, true); +file_put_contents($impostor . '/note.txt', 'roba mia'); +touch($impostor, time() - (999 * 3600)); // oldest of all: first to go if eligible +$seedLegacy(4); +$zips = $seed(2); +$manager = $makeManager($tmp, '2'); +$prune($manager, end($zips)); +$check(is_dir($impostor), 'a legacy-named directory without database.sql is not a rotation candidate'); +$check(is_file($impostor . '/note.txt'), 'and whatever it contained is still there'); + +echo "J. a newly written preserved backup does not consume automatic slots\n"; +foreach ([BackupManager::ORIGIN_MANUAL, BackupManager::ORIGIN_SAFETY, BackupManager::ORIGIN_UPLOAD] as $origin) { + $originDir = $tmp . '/origin_' . $origin; + mkdir($originDir); + $autos = []; + for ($i = 1; $i <= 3; $i++) { + $auto = $originDir . '/' . $nameFor->invoke(null, '2026-01-0' . $i . '_000000', BackupManager::ORIGIN_AUTO); + file_put_contents($auto, 'x'); + touch($auto, time() - (4 - $i) * 3600); + $autos[] = $auto; + } + $preserved = $originDir . '/' . $nameFor->invoke(null, '2026-02-01_000000', $origin); + file_put_contents($preserved, 'x'); + $manager = $makeManager($originDir, '1'); + $prune($manager, $preserved); + $check(is_file(end($autos)), $origin . ': the newest automatic backup survives at retention 1'); + $check(!is_file($autos[0]) && !is_file($autos[1]), $origin . ': older automatic backups still rotate'); + $check(is_file($preserved), $origin . ': the newly written preserved backup survives'); +} + +// Teardown belongs at the END, after the last section. It restores the SHARED +// system_settings.retention_count that $makeManager() overwrites — leave it and +// the next suite to run rotates at whatever number this file last set. $cleanup(); + echo PHP_EOL . "Passed: {$passed} Failed: {$failed}" . PHP_EOL; exit($failed === 0 ? 0 : 1); diff --git a/tests/backup-rotation-origins.spec.js b/tests/backup-rotation-origins.spec.js new file mode 100644 index 000000000..da46a5a9c --- /dev/null +++ b/tests/backup-rotation-origins.spec.js @@ -0,0 +1,271 @@ +// Backup rotation, origins and the legacy directory format — through the real UI. +// +// The unit suite proves the rules against the class. These prove the operator +// actually gets them: the button really produces a preserved backup, the list +// really shows it, download and delete really work on both formats, and a +// rotation triggered from the UI really spares what it must. +// +// Requires: E2E_ADMIN_EMAIL, E2E_ADMIN_PASS, E2E_APP_ROOT (the SERVED docroot — +// filesystem assertions are meaningless against __dirname when the app under +// test is a different copy), plus E2E_BASE_URL/APP_URL. +// +// Everything it creates it removes, including on failure: storage/backups is +// live data on a dev install, and a test that leaves archives behind would +// itself feed the accumulation this whole change is about. + +const { test, expect } = require('@playwright/test'); +const fs = require('fs'); +const path = require('path'); + +const BASE = process.env.E2E_BASE_URL || process.env.APP_URL || 'http://localhost:8081'; +const ADMIN_EMAIL = process.env.E2E_ADMIN_EMAIL || ''; +const ADMIN_PASS = process.env.E2E_ADMIN_PASS || ''; +const APP_ROOT = process.env.E2E_APP_ROOT || path.resolve(__dirname, '..'); +const BACKUP_DIR = path.join(APP_ROOT, 'storage', 'backups'); + +test.skip( + !ADMIN_EMAIL || !ADMIN_PASS, + 'E2E credentials not configured (set E2E_ADMIN_EMAIL, E2E_ADMIN_PASS)' +); + +// Everything this spec plants carries the run id, so cleanup can never reach an +// archive that belonged to the installation before the test started. +const RUN_ID = Math.random().toString(16).slice(2, 8); +const planted = []; + +/** A backup filename in the shape the app generates, tagged with the run id. */ +function plantArchive(dayOfMonth, origin) { + const stamp = `2020-01-${String(dayOfMonth).padStart(2, '0')}_000000`; + const suffix = origin === 'auto' ? `${RUN_ID}` : `${RUN_ID}_${origin}`; + const name = `backup_${stamp}_${suffix}.zip`; + const full = path.join(BACKUP_DIR, name); + // A real (if minimal) ZIP: the list reads a manifest from it, and a + // zero-byte file would exercise the error path instead of the one we mean. + fs.writeFileSync(full, Buffer.from('PK\x05\x06' + '\x00'.repeat(18), 'binary')); + fs.utimesSync(full, new Date('2020-01-01'), new Date(`2020-01-${String(dayOfMonth).padStart(2, '0')}`)); + planted.push(full); + return name; +} + +function plantLegacyDir(dayOfMonth) { + const name = `update_2020-02-${String(dayOfMonth).padStart(2, '0')}_000000`; + const full = path.join(BACKUP_DIR, name); + fs.mkdirSync(full, { recursive: true }); + // Fixtures are created by the runner; PHP may run as a different user. + fs.chmodSync(full, 0o777); + fs.writeFileSync(path.join(full, 'database.sql'), `-- e2e ${RUN_ID}\nSELECT 1;\n`); + planted.push(full); + return name; +} + +function removePlanted() { + for (const p of planted.splice(0)) { + try { + fs.rmSync(p, { recursive: true, force: true }); + } catch { /* already gone: the UI deleted it, which is the point */ } + } +} + +/** + * Click a button that opens a SweetAlert confirmation, then confirm it. + * + * Every destructive action on this page goes through Swal.fire(), which is DOM, + * not a native dialog — page.on('dialog') never sees it and the request the test + * is waiting for is never sent. + */ +async function clickAndConfirm(page, locator) { + await locator.click(); + const confirm = page.locator('.swal2-confirm'); + await confirm.waitFor({ state: 'visible', timeout: 15000 }); + await confirm.click(); +} + +/** Names currently shown in the backup table, read from the API the table renders. */ +async function listedNames(page) { + return page.evaluate(async (base) => { + const r = await fetch(base + '/admin/updates/backups', { credentials: 'same-origin' }); + const d = await r.json(); + return (d.backups || []).map((b) => b.name); + }, BASE); +} + +test.describe.serial('Backup: origins, rotation and the legacy format', () => { + let context; + let page; + const created = []; + + test.beforeAll(async ({ browser }) => { + expect(fs.existsSync(BACKUP_DIR), `backup dir not found: ${BACKUP_DIR}`).toBeTruthy(); + context = await browser.newContext({ acceptDownloads: true }); + page = await context.newPage(); + await page.goto(`${BASE}/accedi`); + await page.fill('input[name="email"]', ADMIN_EMAIL); + await page.fill('input[name="password"]', ADMIN_PASS); + await page.locator('button[type="submit"]').click(); + await page.waitForURL(/admin/, { timeout: 15000 }); + }); + + test.afterAll(async () => { + // Remove what the button created, too — those carry a real timestamp and + // cannot be matched by run id. + for (const name of created) { + try { fs.rmSync(path.join(BACKUP_DIR, name), { force: true }); } catch { /* noop */ } + } + removePlanted(); + if (context) await context.close(); + }); + + test('1. the button creates a backup marked as manual, and the list shows it', async () => { + await page.goto(`${BASE}/admin/updates`); + await page.waitForLoadState('networkidle'); + + const before = await listedNames(page); + + await page.selectOption('#backupScope', 'db'); + const response = page.waitForResponse( + (r) => r.url().endsWith('/admin/updates/backup') && r.request().method() === 'POST', + { timeout: 120000 } + ); + await clickAndConfirm(page, page.locator('button[onclick="createBackup()"]')); + const body = await (await response).json(); + + expect(body.success, `backup failed: ${body.error || ''}`).toBeTruthy(); + created.push(body.name); + + // The origin has to be in the NAME, not only in the manifest: the + // rotation decides from a glob and never opens the archive. + expect(body.name).toMatch(/^backup_\d{4}-\d{2}-\d{2}_\d{6}_[0-9a-f]{6}_manual\.zip$/); + expect(fs.existsSync(path.join(BACKUP_DIR, body.name))).toBeTruthy(); + + const after = await listedNames(page); + expect(after).toContain(body.name); + expect(after.length).toBe(before.length + 1); + + // And it must be visible as a row the operator can act on. + await page.locator('button[onclick="loadBackups()"]').click(); + await expect(page.locator(`[data-backup="${body.name}"][data-action="delete"]`)).toBeVisible({ timeout: 15000 }); + }); + + test('2. creating a backup rotates the automatic ones and spares the deliberate ones', async () => { + // Twelve automatic archives, all older than anything real on this install, + // plus one manual and one safety copy made OLDER still — if the rotation + // ever stopped honouring origins, those two would be the first to go. + for (let d = 1; d <= 12; d++) plantArchive(d, 'auto'); + const manual = plantArchive(20, 'manual'); + const safety = plantArchive(21, 'safety'); + fs.utimesSync(path.join(BACKUP_DIR, manual), new Date('2019-01-01'), new Date('2019-01-01')); + fs.utimesSync(path.join(BACKUP_DIR, safety), new Date('2019-01-02'), new Date('2019-01-02')); + + await page.goto(`${BASE}/admin/updates`); + await page.waitForLoadState('networkidle'); + + await page.selectOption('#backupScope', 'db'); + const response = page.waitForResponse( + (r) => r.url().endsWith('/admin/updates/backup') && r.request().method() === 'POST', + { timeout: 120000 } + ); + await clickAndConfirm(page, page.locator('button[onclick="createBackup()"]')); + const body = await (await response).json(); + expect(body.success).toBeTruthy(); + created.push(body.name); + + const names = await listedNames(page); + + // The two deliberate restore points are still there. + expect(names, 'a backup created from the button was rotated away').toContain(manual); + expect(names, 'the pre-restore safety copy was rotated away').toContain(safety); + expect(fs.existsSync(path.join(BACKUP_DIR, manual))).toBeTruthy(); + expect(fs.existsSync(path.join(BACKUP_DIR, safety))).toBeTruthy(); + + // The planted automatic ones were the oldest on the install, so the + // rotation must have taken them: retention is 10 by default and we added + // twelve plus a fresh one. + const survivingPlantedAuto = names.filter( + (n) => n.includes(RUN_ID) && !n.includes('_manual') && !n.includes('_safety') + ); + expect( + survivingPlantedAuto.length, + 'the rotation did not trim the automatic archives at all' + ).toBeLessThan(12); + }); + + test('3. a backup downloads from the list with the name it has on disk', async () => { + const name = created[0]; + expect(name, 'test 1 must have created a backup').toBeTruthy(); + + await page.goto(`${BASE}/admin/updates`); + await page.waitForLoadState('networkidle'); + await page.locator('button[onclick="loadBackups()"]').click(); + + const btn = page.locator(`[data-backup="${name}"][data-action="download"]`); + await expect(btn).toBeVisible({ timeout: 15000 }); + + const download = await Promise.all([ + page.waitForEvent('download', { timeout: 30000 }), + btn.click(), + ]).then(([d]) => d); + + expect(download.suggestedFilename()).toBe(name); + const stream = await download.createReadStream(); + expect(stream, 'the download produced no body').toBeTruthy(); + }); + + test('4. a legacy directory backup lists, downloads as .sql and is not restorable', async () => { + const legacy = plantLegacyDir(3); + + await page.goto(`${BASE}/admin/updates`); + await page.waitForLoadState('networkidle'); + await page.locator('button[onclick="loadBackups()"]').click(); + + await expect(page.locator(`[data-backup="${legacy}"][data-action="download"]`)).toBeVisible({ timeout: 15000 }); + + // Restore is deliberately not offered for the folder format — it would + // have nothing to unpack. + await expect(page.locator(`[data-backup="${legacy}"][data-action="restore"]`)).toHaveCount(0); + + const download = await Promise.all([ + page.waitForEvent('download', { timeout: 30000 }), + page.locator(`[data-backup="${legacy}"][data-action="download"]`).click(), + ]).then(([d]) => d); + + // The directory is served as the raw dump it contains. + expect(download.suggestedFilename()).toBe(`${legacy}.sql`); + }); + + test('5. deleting removes both formats, directory contents included', async () => { + const zipName = plantArchive(28, 'auto'); + const legacy = plantLegacyDir(9); + const legacyPath = path.join(BACKUP_DIR, legacy); + + await page.goto(`${BASE}/admin/updates`); + await page.waitForLoadState('networkidle'); + await page.locator('button[onclick="loadBackups()"]').click(); + + const [zipResponse] = await Promise.all([ + page.waitForResponse((r) => r.url().includes('/admin/updates/backup/delete') && r.request().method() === 'POST', { timeout: 30000 }), + clickAndConfirm(page, page.locator(`[data-backup="${zipName}"][data-action="delete"]`)), + ]); + expect(zipResponse.ok()).toBeTruthy(); + expect((await zipResponse.json()).success).toBe(true); + expect(fs.existsSync(path.join(BACKUP_DIR, zipName)), 'the archive is still on disk').toBeFalsy(); + + await page.locator('button[onclick="loadBackups()"]').click(); + await expect(page.locator(`[data-backup="${legacy}"][data-action="delete"]`)).toBeVisible({ timeout: 15000 }); + + const [legacyResponse] = await Promise.all([ + page.waitForResponse((r) => r.url().includes('/admin/updates/backup/delete') && r.request().method() === 'POST', { timeout: 30000 }), + clickAndConfirm(page, page.locator(`[data-backup="${legacy}"][data-action="delete"]`)), + ]); + + expect(legacyResponse.ok()).toBeTruthy(); + expect((await legacyResponse.json()).success).toBe(true); + + // The whole directory, not just the row: a delete that left database.sql + // behind would keep consuming the quota it was meant to release. + expect(fs.existsSync(legacyPath), 'the legacy directory survived the delete').toBeFalsy(); + + const names = await listedNames(page); + expect(names).not.toContain(zipName); + expect(names).not.toContain(legacy); + }); +});