From 0f4a238414898df48cb7d093225b06a992a32d95 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 23:48:40 +0200 Subject: [PATCH 1/5] Pre-beta review: data safety in storage, catalog and backup runs Storage and repository: - Listing skipped every name containing ".part-", directories too, so the exports of a user like "john.part-time" were invisible to prune on S3, SFTP, FTP and SMB and their data was deleted. Only real temporary uploads (".part-<8 hex>") are skipped now. - prune refuses to run while a recorded per-user export's manifest is missing, instead of freeing that export's data. - forget() and untrash() copy first, switch the catalog, and delete the originals last, so a crash can no longer leave the catalog pointing at a missing snapshot or database dump. - Writes are checked against the expected size, so a short upload is an error instead of a truncated pack; raw restores check rename() and touch(). - A catalog write refuses to overwrite a generation another process just wrote, and the catalog lock lasts 300 s, so opening a large repository inside it no longer lets the lease run out. Backup runs and scheduling: - A file that keeps changing is read again at most three times; then the last read is kept and reported, instead of restarting forever and blocking cleanup. - Unreadable files and folders are recorded, logged, shown by occ backup:run, stored in the snapshot and notified to admins, instead of being skipped silently. - After a failed scheduled backup, at most three attempts a day, an hour apart, instead of a new run and notification every five minutes. - The job no longer spins on a run whose lease another process holds, and the weekly cleanup catches up after a Sunday spent backing up. - Notifications for the per-user export retention events. --- lib/Backend/LocalBackend.php | 7 +- lib/Backend/StorageBackend.php | 30 +++++- lib/BackgroundJob/BackupJob.php | 25 ++++- lib/Command/RunBackup.php | 7 ++ lib/Db/RunMapper.php | 11 +++ lib/Job/BackupRun.php | 88 +++++++++++++++--- lib/Job/TreeWalker.php | 5 + lib/Notification/Notifier.php | 6 ++ lib/Repository/Catalog.php | 5 + lib/Repository/Repository.php | 37 ++++++-- lib/Service/AlertService.php | 7 ++ lib/Service/BackupService.php | 16 +++- lib/Service/UserBackupService.php | 2 +- lib/Service/UserRestoreService.php | 2 +- psalm-baseline.xml | 5 - tests/Unit/Repository/CrashSafetyTest.php | 106 ++++++++++++++++++++++ 16 files changed, 319 insertions(+), 40 deletions(-) create mode 100644 tests/Unit/Repository/CrashSafetyTest.php diff --git a/lib/Backend/LocalBackend.php b/lib/Backend/LocalBackend.php index f2a61d5..1805768 100644 --- a/lib/Backend/LocalBackend.php +++ b/lib/Backend/LocalBackend.php @@ -33,11 +33,16 @@ final class LocalBackend implements IBackend { if ($out === false) { throw new BackendException('Cannot write ' . $path); } + $expected = StorageBackend::remaining($stream); if (stream_copy_to_stream($stream, $out) === false || !fflush($out)) { throw new BackendException('Write failed for ' . $path); } fclose($out); $out = false; + clearstatcache(true, $tmp); + if ($expected !== null && filesize($tmp) !== $expected) { + throw new BackendException('Incomplete write for ' . $path); + } if (!rename($tmp, $target)) { throw new BackendException('Cannot finalise ' . $path); } @@ -94,7 +99,7 @@ final class LocalBackend implements IBackend { $result = []; $it = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($base, \FilesystemIterator::SKIP_DOTS)); foreach ($it as $file) { - if ($file->isFile() && !str_contains($file->getFilename(), '.part-')) { + if ($file->isFile() && !StorageBackend::isTemporary($file->getFilename())) { $result[] = ltrim(substr($file->getPathname(), strlen($this->root)), '/'); } } diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 0719d8e..bed3dca 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -30,7 +30,13 @@ final class StorageBackend implements IBackend { } } + /** A temporary upload of put(): ".part-<8 hex>". */ + public static function isTemporary(string $name): bool { + return preg_match('/\.part-[0-9a-f]{8}$/', $name) === 1; + } + public function put(string $path, $stream): void { + $expected = self::remaining($stream); $target = $this->abs($path); $this->mkdirs(dirname($target)); $writeTo = $this->atomicPut ? $target : $target . '.part-' . bin2hex(random_bytes(4)); @@ -54,6 +60,10 @@ final class StorageBackend implements IBackend { if (is_resource($stream)) { fclose($stream); } + // Some adapters report success after a short write or a failed flush: check the size. + if ($expected !== null && $this->storage->filesize($writeTo) !== $expected) { + throw new BackendException('Incomplete write for ' . $path); + } if ($writeTo !== $target && !$this->storage->rename($writeTo, $target)) { throw new BackendException('Cannot finalise ' . $path); } @@ -132,19 +142,35 @@ final class StorageBackend implements IBackend { return; } while (($name = readdir($dh)) !== false) { - if ($name === '.' || $name === '..' || str_contains($name, '.part-')) { + if ($name === '.' || $name === '..') { continue; } $child = $dir . '/' . $name; if ($this->storage->is_dir($child)) { $this->listInto($child, $result); - } else { + } elseif (!self::isTemporary($name)) { $result[] = $child; } } closedir($dh); } + /** Bytes left to read in a seekable $stream, or null when it cannot tell. @param resource $stream */ + public static function remaining(mixed $stream): ?int { + if (!is_resource($stream)) { + return null; + } + if (!(stream_get_meta_data($stream)['seekable'] ?? false)) { + return null; + } + $stat = fstat($stream); + $pos = ftell($stream); + if ($stat === false || $pos === false) { + return null; + } + return max(0, $stat['size'] - $pos); + } + private function mkdirs(string $dir): void { if ($dir === '' || $dir === '.' || $this->storage->is_dir($dir)) { return; diff --git a/lib/BackgroundJob/BackupJob.php b/lib/BackgroundJob/BackupJob.php index b2e7e14..e8c7536 100644 --- a/lib/BackgroundJob/BackupJob.php +++ b/lib/BackgroundJob/BackupJob.php @@ -38,13 +38,19 @@ final class BackupJob extends TimedJob { $this->setTimeSensitivity(IJob::TIME_SENSITIVE); } + private const MAX_DAILY_ATTEMPTS = 3; + protected function run($argument): void { $budget = PHP_SAPI === 'cli' ? 240.0 : 20.0; $deadline = microtime(true) + $budget; $this->startScheduled(); foreach ($this->runs->findRunning(null, BackupService::KIND_FULL) as $run) { while (microtime(true) < $deadline && $run->getStatus() === 'running') { + $before = $run->getUpdatedAt(); $run = $this->backups->step($run, min($deadline, microtime(true) + $budget)); + if ($run->getUpdatedAt() === $before && $run->getStatus() === 'running') { + break; // another process holds this run's lease: leave it to that process + } } } $this->failStaleRestores(); @@ -67,10 +73,15 @@ final class BackupJob extends TimedJob { } } - /** Once a week (after Sunday's backup), apply the retention policy to non-append-only locations. */ + /** + * Once a week, apply the retention policy to non-append-only locations: from Sunday on, at the + * first moment no backup is running, so a long Sunday backup only delays it instead of skipping + * the week. + */ private function weeklyPrune(): void { $lastWeek = $this->appConfig->getValueString(Application::APP_ID, 'last_prune_week', ''); - if (date('N') !== '7' || $lastWeek === date('o-W') || $this->runs->findRunning(null, BackupService::KIND_FULL) !== []) { + $due = date('N') === '7' ? date('o-W') : date('o-W', (int)strtotime('last sunday')); + if (strcmp($lastWeek, $due) >= 0 || $this->runs->findRunning(null, BackupService::KIND_FULL) !== []) { return; } foreach ($this->targets->list() as $t) { @@ -83,7 +94,7 @@ final class BackupJob extends TimedJob { $this->logger->warning('NG Backup: cleanup of ' . $t->getName() . ' failed: ' . $e->getMessage()); } } - $this->appConfig->setValueString(Application::APP_ID, 'last_prune_week', date('o-W')); + $this->appConfig->setValueString(Application::APP_ID, 'last_prune_week', $due); } /** Daily schedule: appconfig ng_backup/schedule_time "HH:MM" (server time); empty = no schedule. */ @@ -92,7 +103,7 @@ final class BackupJob extends TimedJob { if (!preg_match('/^([01]\d|2[0-3]):[0-5]\d$/', $at)) { return; } - $todayAt = strtotime(date('Y-m-d') . ' ' . $at); + $todayAt = (int)strtotime(date('Y-m-d') . ' ' . $at); if (time() < $todayAt) { return; } @@ -101,6 +112,12 @@ final class BackupJob extends TimedJob { if ($this->runs->findRunning($t->getId(), BackupService::KIND_FULL) !== [] || ($last !== null && $last->getStartedAt() >= $todayAt)) { continue; } + // After a failure: retry at most MAX_DAILY_ATTEMPTS times a day, an hour apart, instead + // of starting (and notifying about) a new full run every five minutes all day. + $today = $this->runs->startedSince($t->getId(), BackupService::KIND_FULL, $todayAt); + if (count($today) >= self::MAX_DAILY_ATTEMPTS || ($today !== [] && time() - (int)$today[0]->getFinishedAt() < 3600)) { + continue; + } try { $this->backups->start($t, 'scheduled'); } catch (\Throwable $e) { diff --git a/lib/Command/RunBackup.php b/lib/Command/RunBackup.php index 5e5f40e..b5aa06e 100644 --- a/lib/Command/RunBackup.php +++ b/lib/Command/RunBackup.php @@ -75,6 +75,13 @@ HELP); }); if ($run->getStatus() === Run::DONE) { $output->writeln(sprintf(' done in %.0fs', microtime(true) - $start)); + $f = json_decode($run->getStats() ?? '{}', true)['files'] ?? []; + if (($f['unreadable'] ?? 0) > 0) { + $output->writeln(sprintf(' warning: %d unreadable files or folders are missing from this backup (see the server log)', $f['unreadable'])); + } + if (($f['changing'] ?? 0) > 0) { + $output->writeln(sprintf(' warning: %d files kept changing and were saved as read (see the server log)', $f['changing'])); + } } else { $output->writeln(' ' . $run->getStatus() . ': ' . $run->getError() . ''); $failed++; diff --git a/lib/Db/RunMapper.php b/lib/Db/RunMapper.php index e700370..9799259 100644 --- a/lib/Db/RunMapper.php +++ b/lib/Db/RunMapper.php @@ -43,6 +43,17 @@ final class RunMapper extends QBMapper { return $this->findEntities($qb); } + /** @return list runs of $kind started at or after $since, newest first */ + public function startedSince(int $targetId, string $kind, int $since): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*')->from($this->getTableName()) + ->where($qb->expr()->eq('target_id', $qb->createNamedParameter($targetId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('kind', $qb->createNamedParameter($kind))) + ->andWhere($qb->expr()->gte('started_at', $qb->createNamedParameter($since, IQueryBuilder::PARAM_INT))) + ->orderBy('id', 'DESC'); + return $this->findEntities($qb); + } + public function lastFinished(int $targetId, string $kind): ?Run { $qb = $this->db->getQueryBuilder(); $qb->select('*')->from($this->getTableName()) diff --git a/lib/Job/BackupRun.php b/lib/Job/BackupRun.php index 86ef1e5..7edb3f4 100644 --- a/lib/Job/BackupRun.php +++ b/lib/Job/BackupRun.php @@ -49,9 +49,11 @@ final class BackupRun { public static function step(Repository $repo, array $state, float $deadline): array { $state['steps']++; if ($state['phase'] === 'files') { - $walker = new TreeWalker($state['roots'], $state['exclude']); - $parent = new ParentCursor($repo, $state['parent'], $state['parentPos']); $stats = $state['stats']; + $walker = new TreeWalker($state['roots'], $state['exclude'], function (string $logical) use (&$state, &$stats): void { + self::recordUnreadable($state, $stats, $logical); + }); + $parent = new ParentCursor($repo, $state['parent'], $state['parentPos']); $lines = []; $finishedWalk = true; @@ -68,15 +70,20 @@ final class BackupRun { $stats['reused']++; } else { $cur = $state['cur']; - if ($cur === null || $cur['path'] !== $logical || $cur['version'] !== $version) { - if ($cur !== null && $cur['path'] === $logical) { - $stats['retried'] = ($stats['retried'] ?? 0) + 1; // changed since the previous step + if ($cur !== null && $cur['path'] === $logical && $cur['version'] !== $version) { + // Changed since the previous step: start over, unless it keeps changing. + if (self::countRetry($state, $stats, $logical)) { + $cur['version'] = $version; + } else { + $cur = null; } + } + if ($cur === null || $cur['path'] !== $logical) { $cur = ['path' => $logical, 'offset' => 0, 'blobs' => [], 'version' => $version]; } - $fh = fopen($path, 'rb'); + $fh = @fopen($path, 'rb'); if ($fh === false) { - $stats['unreadable'] = ($stats['unreadable'] ?? 0) + 1; + self::recordUnreadable($state, $stats, $logical); $state['cur'] = null; continue; } @@ -91,12 +98,17 @@ final class BackupRun { } } fclose($fh); - if (Repository::fileVersion($path) !== $cur['version']) { - // Changed while reading: read it again from the start (now or next step). - $stats['retried'] = ($stats['retried'] ?? 0) + 1; - $state['cur'] = null; - $finishedWalk = false; - break; + $now = Repository::fileVersion($path); + if ($now !== $cur['version']) { + if (!self::countRetry($state, $stats, $logical)) { + // Changed while reading: read it again from the start (now or next step). + $state['cur'] = null; + $finishedWalk = false; + break; + } + // Keeps changing (a live database, VM image or log): keep what was read, + // like any copy of a file in use, and report it instead of retrying forever. + $cur['version'] = $now; } if (!$complete) { $state['cur'] = $cur; @@ -104,6 +116,7 @@ final class BackupRun { break; } $blobs = $cur['blobs']; + $size = $cur['offset']; $state['cur'] = null; $stats['read']++; } @@ -135,7 +148,8 @@ final class BackupRun { $segments = $state['segments']; $repo->putSnapshot($state['snapshot'], ['time' => $state['started'], 'parent' => $state['parent'], 'label' => $state['label'], - 'roots' => array_keys($state['roots']), 'stats' => $state['stats']] + $state['meta'], + 'roots' => array_keys($state['roots']), 'stats' => $state['stats'], + 'warnings' => self::warnings($state)] + $state['meta'], (function () use ($repo, $segments) { foreach ($segments as $blobs) { yield from $repo->readLines($blobs); @@ -146,6 +160,52 @@ final class BackupRun { return $state; } + /** + * A file or directory that could not be read: it is missing from this snapshot. Remembered (first + * 1000, deduplicated across steps) so the run can report it instead of silently succeeding. + */ + private static function recordUnreadable(array &$state, array &$stats, string $logical): void { + if (isset($state['unreadable'][$logical])) { + return; + } + if (count($state['unreadable'] ?? []) < 1000) { + $state['unreadable'][$logical] = true; + } + $stats['unreadable'] = ($stats['unreadable'] ?? 0) + 1; + } + + /** @return array{unreadable: list, changing: list} examples for the report (first 50 each) */ + public static function warnings(array $state): array { + return [ + 'unreadable' => array_slice(array_map('strval', array_keys($state['unreadable'] ?? [])), 0, 50), + 'changing' => array_slice(array_map('strval', array_values($state['changing'] ?? [])), 0, 50), + ]; + } + + /** Retries of one file that changes while it is read, before its last read is kept anyway. */ + public const MAX_RETRIES = 3; + + /** + * Count a retry of $logical. Returns true once it has changed MAX_RETRIES times: the caller then + * keeps the data it read, and the file is listed in $state['changing'] (first 50) for the report. + */ + private static function countRetry(array &$state, array &$stats, string $logical): bool { + $stats['retried'] = ($stats['retried'] ?? 0) + 1; + $n = (($state['retryPath'] ?? null) === $logical ? (int)($state['retryCount'] ?? 0) : 0) + 1; + $state['retryPath'] = $logical; + $state['retryCount'] = $n; + if ($n < self::MAX_RETRIES) { + return false; + } + if ($n === self::MAX_RETRIES) { + $stats['changing'] = ($stats['changing'] ?? 0) + 1; + if (count($state['changing'] ?? []) < 50) { + $state['changing'][] = $logical; + } + } + return true; + } + private static function readFull($fh, int $length): string { $buffer = ''; while (strlen($buffer) < $length && !feof($fh)) { diff --git a/lib/Job/TreeWalker.php b/lib/Job/TreeWalker.php index a685058..ce342b4 100644 --- a/lib/Job/TreeWalker.php +++ b/lib/Job/TreeWalker.php @@ -23,6 +23,8 @@ final class TreeWalker { public function __construct( private array $roots, private array $exclude = [], + /** Called with the logical path of a directory that cannot be listed. */ + private ?\Closure $onUnreadable = null, ) { } @@ -57,6 +59,9 @@ final class TreeWalker { } $names = @scandir($abs); if ($names === false) { + if ($this->onUnreadable !== null) { + ($this->onUnreadable)($logical === '' ? '/' : $logical); + } return; } $names = array_values(array_diff($names, ['.', '..'])); diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index e5cf2e8..df81fa3 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -42,6 +42,10 @@ final class Notifier implements INotifier { : $l->t('Backup to %s failed', [$p['target']])); $notification->setParsedMessage((string)($p['error'] ?? '')); break; + case 'run_warnings': + $notification->setParsedSubject($l->t('Backup to %1$s finished with warnings: %2$s unreadable, %3$s changed while being read', [$p['target'], $p['unreadable'], $p['changing']])); + $notification->setParsedMessage($l->t('Unreadable files and folders are missing from this backup (check their permissions). Files that kept changing were saved as they were while being read. Examples: %s', [$p['examples'] ?? ''])); + break; case 'quota_warning': $notification->setParsedSubject($l->t('Backup location %1$s is at %2$s%% of its limit (%3$s of %4$s)', [$p['target'], $p['percent'], $p['used'], $p['limit']])); $notification->setParsedMessage($l->t('Backups stop when the limit is reached. Raise the limit, or let the retention policy remove older backups.')); @@ -62,6 +66,8 @@ final class Notifier implements INotifier { 'catalog_trusted' => $l->t('%1$s accepted an older state of backup location %2$s (catalog generation %3$s)', [$who, $p['target'] ?? '', $p['generation'] ?? '']), 'slot_added' => $l->t('%1$s added backup passphrase slot %2$s (%3$s); download and confirm the new recovery kit', [$who, $p['slot'] ?? '', $p['label'] ?? '']), 'slot_replaced' => $l->t('%1$s set a new passphrase for backup key slot %2$s (%3$s); download and confirm the new recovery kit', [$who, $p['slot'] ?? '', $p['label'] ?? '']), + 'user_export_retention_changed' => $l->t('%1$s changed how many exports per user are kept to %2$s', [$who, $p['keep'] ?? '']), + 'user_exports_forgotten' => $l->t('%1$s removed %2$s per-user exports from %3$s', [$who, $p['count'] ?? '', $p['target'] ?? '']), 'slot_removed' => $l->t('%1$s removed backup passphrase slot %2$s; download and confirm the new recovery kit', [$who, $p['slot'] ?? '']), default => $l->t('NG Backup security event'), }); diff --git a/lib/Repository/Catalog.php b/lib/Repository/Catalog.php index 44fb6c4..b8a3c47 100644 --- a/lib/Repository/Catalog.php +++ b/lib/Repository/Catalog.php @@ -88,6 +88,11 @@ final class Catalog { /** Write the next generation after the in-memory state was changed. */ public function write(): void { $next = $this->gen + 1; + // Two writers starting from the same generation would otherwise overwrite each other + // silently and break the chain for whichever server anchored the lost write. + if ($this->backend->exists($this->path($next))) { + throw new RepositoryException("Catalog generation $next was written by another process at the same time; try again"); + } $plain = json_encode(['repository' => $this->repositoryId, 'gen' => $next, 'time' => gmdate('c'), 'prev' => $this->hash, 'snapshots' => array_keys($this->snapshots), 'trash' => array_keys($this->trash), 'userExports' => array_keys($this->userExports)], JSON_THROW_ON_ERROR); diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 5c6ccc3..767964c 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -236,8 +236,13 @@ final class Repository { unlink($tmp); throw new RepositoryException('Size mismatch restoring ' . $entry['p']); } - rename($tmp, $dest); - touch($dest, $entry['m']); + if (!rename($tmp, $dest)) { + @unlink($tmp); + throw new RepositoryException('Cannot put the restored file in place: ' . $entry['p']); + } + if (!touch($dest, $entry['m'])) { + throw new RepositoryException('Cannot set the modification time of ' . $entry['p']); + } $count++; } return $count; @@ -375,28 +380,39 @@ final class Repository { // must be decrypted and re-encrypted under the new path, not moved as raw bytes. $dbInTrash = 'trash/' . $meta['db']; $this->putObject($dbInTrash, $this->getObject($meta['db'])); - $this->backend->delete($meta['db']); } - // The snapshot object is bound to its id (not its path), so it can be moved as raw bytes. - $this->backend->move('snapshots/' . $snapshotId, 'trash/snapshots/' . $snapshotId); + // Copy first, switch the catalog, delete the originals last: a crash at any point leaves + // either the old state or the new one plus unreferenced leftovers, never a catalog entry + // pointing at a missing object. The snapshot object is bound to its id (not its path), + // so it can be copied as raw bytes. + $this->backend->put('trash/snapshots/' . $snapshotId, $this->backend->get('snapshots/' . $snapshotId)); $this->putObject('trash/info/' . $snapshotId, json_encode(['forgottenAt' => time(), 'by' => $by, 'db' => $meta['db'] ?? null, 'dbInTrash' => $dbInTrash, 'time' => $meta['time'] ?? null, 'label' => $meta['label'] ?? ''], JSON_THROW_ON_ERROR)); unset($this->catalog->snapshots[$snapshotId]); $this->catalog->trash[$snapshotId] = true; $this->catalog->write(); + if ($dbInTrash !== null) { + $this->backend->delete($meta['db']); + } + $this->backend->delete('snapshots/' . $snapshotId); } /** Undo forget(): move a snapshot back from the trash. */ public function untrash(string $snapshotId): void { $info = $this->trashInfo($snapshotId); - if ($info['dbInTrash'] !== null && $this->backend->exists($info['dbInTrash'])) { + $restoreDb = $info['dbInTrash'] !== null && $this->backend->exists($info['dbInTrash']); + if ($restoreDb) { $this->putObject($info['db'], $this->getObject($info['dbInTrash'])); - $this->backend->delete($info['dbInTrash']); } - $this->backend->move('trash/snapshots/' . $snapshotId, 'snapshots/' . $snapshotId); + // Same order as forget(): copy, switch the catalog, then delete the trash copies. + $this->backend->put('snapshots/' . $snapshotId, $this->backend->get('trash/snapshots/' . $snapshotId)); $this->catalog->snapshots[$snapshotId] = true; unset($this->catalog->trash[$snapshotId]); $this->catalog->write(); + if ($restoreDb) { + $this->backend->delete($info['dbInTrash']); + } + $this->backend->delete('trash/snapshots/' . $snapshotId); $this->backend->delete('trash/info/' . $snapshotId); } @@ -530,7 +546,10 @@ final class Repository { } } } - foreach ($this->backend->list('users') as $path) { + // listUserExports() throws when a recorded export's manifest is missing: then stop, since + // freeing its blobs would turn a tampering or storage alarm into irreversible deletion. + $exports = array_unique(array_merge($this->listUserExports(), $this->backend->list('users'))); + foreach ($exports as $path) { foreach (json_decode($this->getObject($path), true, 512, JSON_THROW_ON_ERROR)['entries'] as $e) { foreach ($e['b'] ?? [] as $b) { $used[$b] = true; diff --git a/lib/Service/AlertService.php b/lib/Service/AlertService.php index f5051ae..817e78a 100644 --- a/lib/Service/AlertService.php +++ b/lib/Service/AlertService.php @@ -54,6 +54,13 @@ final class AlertService { ['kind' => $run->getKind(), 'target' => $targetName, 'error' => mb_strimwidth((string)$run->getError(), 0, 300, '…')]); } + /** A backup that finished but is missing unreadable files, or holds files that changed while read. */ + public function runWarnings(Run $run, string $targetName, int $unreadable, int $changing, array $examples): void { + $this->notifyAdmins('run_warnings', 'run-' . $run->getId(), ['target' => $targetName, + 'unreadable' => (string)$unreadable, 'changing' => (string)$changing, + 'examples' => mb_strimwidth(implode(', ', array_merge($examples['unreadable'], $examples['changing'])), 0, 300, '…')]); + } + /** * Remember how much is stored on a location (for the settings page) and warn administrators * once a day when it is at 80% of its limit or more. diff --git a/lib/Service/BackupService.php b/lib/Service/BackupService.php index 3649383..702cf78 100644 --- a/lib/Service/BackupService.php +++ b/lib/Service/BackupService.php @@ -14,6 +14,7 @@ use OCA\NgBackup\Db\Snapshot; use OCA\NgBackup\Db\SnapshotMapper; use OCA\NgBackup\Db\Target; use OCA\NgBackup\Job\BackupRun; +use OCA\NgBackup\Repository\RepositoryException; use OCP\App\IAppManager; use OCP\IConfig; use OCP\IDBConnection; @@ -151,16 +152,25 @@ final class BackupService { // Catalog write under the catalog lock, from a repository opened inside it: a per-user // export or restore may have written the catalog since this step opened $repo. try { - $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30, + $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 300, fn () => $this->targets->repository($target)->recordSnapshot((string)$state['files']['snapshot'])); $state['files']['phase'] = 'done'; - } catch (LockedException) { - // Another catalog write is in progress: the next step retries this commit. + } catch (LockedException|RepositoryException) { + // Another catalog write is in progress (or just raced us to the next + // generation): the next step retries this commit on the fresh catalog. } } if ($state['files']['phase'] === 'done') { $this->recordSnapshot($run, $state['files']); $this->alerts->storedBytes($target, $repo->storedBytes()); + $stats = $state['files']['stats']; + if (($stats['unreadable'] ?? 0) > 0 || ($stats['changing'] ?? 0) > 0) { + $warnings = BackupRun::warnings($state['files']); + $this->logger->warning(sprintf('NG Backup run %d to %s: %d unreadable (missing from the snapshot): %s; %d changed while being read: %s', + $run->getId(), $target->getName(), $stats['unreadable'] ?? 0, implode(', ', $warnings['unreadable']), + $stats['changing'] ?? 0, implode(', ', $warnings['changing']))); + $this->alerts->runWarnings($run, $target->getName(), (int)($stats['unreadable'] ?? 0), (int)($stats['changing'] ?? 0), $warnings); + } $run->setStatus(Run::DONE); $run->setPhase('done'); $run->setFinishedAt(time()); diff --git a/lib/Service/UserBackupService.php b/lib/Service/UserBackupService.php index 4c5bf5c..0974b66 100644 --- a/lib/Service/UserBackupService.php +++ b/lib/Service/UserBackupService.php @@ -58,7 +58,7 @@ final class UserBackupService { private function withCatalogLock(Target $target, callable $fn): mixed { try { - return $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30, $fn); + return $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 300, $fn); } catch (LockedException) { throw new \RuntimeException('Another catalog write for ' . $target->getName() . ' is in progress; try again'); } diff --git a/lib/Service/UserRestoreService.php b/lib/Service/UserRestoreService.php index eff3621..6fccfc1 100644 --- a/lib/Service/UserRestoreService.php +++ b/lib/Service/UserRestoreService.php @@ -129,7 +129,7 @@ final class UserRestoreService { private function withCatalogLock(Target $target, callable $fn): mixed { try { - return $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 30, $fn); + return $this->leases->with(PruneService::catalogLockKey($target), LeaseService::EXCLUSIVE, 300, $fn); } catch (LockedException) { throw new \RuntimeException('Another catalog write for ' . $target->getName() . ' is in progress; try again'); } diff --git a/psalm-baseline.xml b/psalm-baseline.xml index ab61ac7..2718b1c 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -224,14 +224,9 @@ - - - - - diff --git a/tests/Unit/Repository/CrashSafetyTest.php b/tests/Unit/Repository/CrashSafetyTest.php new file mode 100644 index 0000000..a50978f --- /dev/null +++ b/tests/Unit/Repository/CrashSafetyTest.php @@ -0,0 +1,106 @@ +tempDir(); + $backend = new LocalBackend($root); + mkdir("$root/users/john.part-time", 0700, true); + file_put_contents("$root/users/john.part-time/20261006T120000Z-aaaa0001", 'x'); + file_put_contents("$root/users/upload.part-deadbeef", 'partial'); + $this->assertSame(['users/john.part-time/20261006T120000Z-aaaa0001'], $backend->list('users')); + } + + public function testPruneStopsWhenARecordedExportManifestIsMissing(): void { + $root = $this->tempDir(); + $backend = new LocalBackend($root); + $keys = KeyRing::generate(); + $repo = Repository::initWithKey($backend, $keys, $keys->wrap('pw')); + $stats = []; + $blob = $repo->storeData(random_bytes(1000), $stats); + $repo->flushPacks(); + $path = 'users/alice/20261006T120000Z-aaaa0001'; + $repo->putObject($path, json_encode(['uid' => 'alice', 'entries' => ['f.txt' => ['t' => 'f', 'b' => [$blob], 's' => 1000]]])); + $repo->recordUserExport($path); + unlink("$root/$path"); + + $packsBefore = $backend->list('packs'); + try { + Repository::openWithKey($backend, $keys)->prune(); + $this->fail('prune must refuse while a recorded export is missing'); + } catch (RepositoryException $e) { + $this->assertStringContainsString('missing on the location', $e->getMessage()); + } + $this->assertSame($packsBefore, $backend->list('packs'), 'no pack was deleted'); + } + + public function testForgetAndUntrashKeepTheSnapshotRestorable(): void { + $src = $this->tempDir(); + file_put_contents("$src/a.txt", 'hello'); + $root = $this->tempDir(); + $backend = new LocalBackend($root); + $keys = KeyRing::generate(); + Repository::initWithKey($backend, $keys, $keys->wrap('pw')); + $state = BackupRun::start($src); + while ($state['phase'] !== 'commit') { + $state = BackupRun::step(Repository::openWithKey($backend, $keys), $state, microtime(true) + 5); + } + Repository::openWithKey($backend, $keys)->recordSnapshot($state['snapshot']); + $id = $state['snapshot']; + + Repository::openWithKey($backend, $keys)->forget($id, 'test'); + $repo = Repository::openWithKey($backend, $keys); + $this->assertSame([], $repo->listSnapshots()); + $this->assertArrayHasKey($id, $repo->trash()); + $this->assertFalse($backend->exists('snapshots/' . $id), 'original removed after the catalog switch'); + + $repo->untrash($id); + $repo = Repository::openWithKey($backend, $keys); + $this->assertSame([$id], $repo->listSnapshots()); + $this->assertSame([], $repo->trash()); + $target = $this->tempDir(); + $repo->restore($id, $target); + $this->assertSame('hello', file_get_contents("$target/a.txt")); + } + + public function testFileThatKeepsChangingDoesNotStallTheRun(): void { + $src = $this->tempDir(); + file_put_contents("$src/live.db", random_bytes(3 * Repository::BLOB_SIZE)); + $root = $this->tempDir(); + $backend = new LocalBackend($root); + $keys = KeyRing::generate(); + Repository::initWithKey($backend, $keys, $keys->wrap('pw')); + + $state = BackupRun::start($src); + $n = 0; + for ($i = 0; $i < 50 && $state['phase'] !== 'commit'; $i++) { + $state = BackupRun::step(Repository::openWithKey($backend, $keys), $state, microtime(true) + 0.001); + // Rewrite the file after every step: it never stays the same between two steps. + $fh = fopen("$src/live.db", 'r+b'); + fwrite($fh, random_bytes(16)); + fclose($fh); + touch("$src/live.db", time() + ++$n); + clearstatcache(); + } + $this->assertSame('commit', $state['phase'], 'the run finishes instead of retrying forever'); + $this->assertSame(1, $state['stats']['changing'] ?? 0); + $this->assertSame(['live.db'], BackupRun::warnings($state)['changing']); + } +} From 45069994e9d78fab555eb3312004e802190279d8 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 23:48:41 +0200 Subject: [PATCH 2/5] Pre-beta review: safer destructive commands and hardening - backup:restore:full refuses on an installation with more than one user and asks before it starts (--force skips both, also to rerun after a failure halfway), and warns when the snapshot's database dump was not one consistent transaction. - backup:user:restore --mode=replace asks before deleting an existing account; non-interactive use needs --force. - The recovery kit file is created owner-only and never over an existing file or symlink; backup:key:import shows the deletion delay it read from the kit. - Adding a location through the web API needs password confirmation, like removing one. - Key derivation parameters from a kit or repository config are bounded, and a stalled network stream no longer spins at full CPU. - Floating-point columns are dumped exactly instead of with 14 digits. --- docs/HOWTO.md | 7 ++++++- lib/Command/KeyImport.php | 3 +++ lib/Command/KeyKit.php | 13 ++++++++++--- lib/Command/RestoreFull.php | 26 ++++++++++++++++++++++++++ lib/Command/RestoreUser.php | 22 +++++++++++++++++++++- lib/Controller/AdminApiController.php | 1 + lib/Crypto/KeyRing.php | 6 ++++++ lib/Crypto/StreamCipher.php | 8 ++++++++ lib/Db/DbDumper.php | 6 +++++- lib/Service/RestoreFullService.php | 6 ++++-- tests/integration/user-roundtrip.sh | 3 ++- 11 files changed, 92 insertions(+), 9 deletions(-) diff --git a/docs/HOWTO.md b/docs/HOWTO.md index 9f15f5e..e6d0097 100644 --- a/docs/HOWTO.md +++ b/docs/HOWTO.md @@ -102,7 +102,9 @@ occ backup:user:restore offsite users// --mode=as-backup use, add `--install-user-migration` to approve that explicitly. The command stops after the installation; run it once more. - `replace` first makes a safety export of the current account, then recreates the user under the - same id. `as-backup` restores into `-bak` and leaves the original untouched. + same id. When the account exists, the command asks first; in scripts, add `--force`. Shares to + others and data of apps without a user_migration migrator are not restored, so prefer + `as-backup` when in doubt: it restores into `-bak` and leaves the original untouched. - A restore onto a server where the user does not exist works. Calendars and address books get an internal name with the prefix `migrated-` (for example `migrated-personal`). That prefix comes from user_migration and cannot be turned off. The user can change the display name of these calendars and @@ -124,6 +126,9 @@ occ backup:list offsite # the snapshots must be lis occ backup:restore:full offsite ``` +The command refuses on an installation that already has more than one user, and asks before it +starts; `--force` skips both (also needed to rerun it after it stopped halfway). + Afterwards, check that users, files and the database are complete, and run a backup of the restored server. The restored installation keeps its own app configuration and secret. diff --git a/lib/Command/KeyImport.php b/lib/Command/KeyImport.php index 4e03669..f982965 100644 --- a/lib/Command/KeyImport.php +++ b/lib/Command/KeyImport.php @@ -76,6 +76,9 @@ HELP); return 1; } $output->writeln('Backup key imported, fingerprint ' . $this->keys->fingerprint() . '.'); + // Read from the kit file, which is not authenticated beyond the wrapped key: show it so a + // wrong or altered value stands out. + $output->writeln('Deletion delay for forgotten snapshots: ' . $this->keys->deleteDelayDays() . ' days (from the kit).'); $output->writeln('Next: connect the backup location with the same details as before (occ backup:target:add), then occ backup:list and occ backup:restore:full.'); return 0; } diff --git a/lib/Command/KeyKit.php b/lib/Command/KeyKit.php index 7cc12c4..5a7638c 100644 --- a/lib/Command/KeyKit.php +++ b/lib/Command/KeyKit.php @@ -39,11 +39,18 @@ final class KeyKit extends Command { $output->write($json, false, OutputInterface::OUTPUT_RAW); return 0; } - if (file_put_contents($file, $json) === false) { - $output->writeln("Cannot write $file"); + // Created owner-only from the start (no window where it is world-readable), never over an + // existing file or symlink someone else may have placed at a predictable path. + $old = umask(0077); + try { + $fh = @fopen($file, 'xb'); + } finally { + umask($old); + } + if ($fh === false || fwrite($fh, $json) !== strlen($json) || !fclose($fh)) { + $output->writeln("Cannot write $file (it must not exist yet)"); return 1; } - @chmod($file, 0600); $output->writeln("Recovery kit written to $file. Copy it off this server, then run occ backup:key:confirm."); return 0; } diff --git a/lib/Command/RestoreFull.php b/lib/Command/RestoreFull.php index 4487b54..524fe63 100644 --- a/lib/Command/RestoreFull.php +++ b/lib/Command/RestoreFull.php @@ -9,7 +9,11 @@ namespace OCA\NgBackup\Command; use OCA\NgBackup\Service\RestoreFullService; use OCA\NgBackup\Service\TargetService; +use OCP\IUserManager; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\QuestionHelper; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Question\ConfirmationQuestion; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -18,6 +22,7 @@ final class RestoreFull extends Command { public function __construct( private TargetService $targets, private RestoreFullService $restoreFull, + private IUserManager $userManager, ) { parent::__construct(); } @@ -27,6 +32,7 @@ final class RestoreFull extends Command { ->setDescription('Disaster recovery: restore the database, data directory and config on a fresh installation') ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') ->addArgument('snapshot', InputArgument::REQUIRED, 'Snapshot id (see backup:list)') + ->addOption('force', null, InputOption::VALUE_NONE, 'Also on an installation that already has more than one user, and without asking') ->setHelp(<<<'HELP' For a FRESH Nextcloud installation only, same major version as the backup, right after occ maintenance:install, occ backup:key:import and occ backup:target:add -- before anyone else @@ -50,12 +56,29 @@ HELP); } protected function execute(InputInterface $input, OutputInterface $output): int { + $force = (bool)$input->getOption('force'); + // A whole-server restore replaces the database and data directory of THIS instance. On + // anything but a fresh install (only the admin from occ maintenance:install) that is + // almost certainly a mistake, so it needs --force. + $users = $this->userManager->countUsersTotal(2); + if (!$force && ($users === false || $users > 1)) { + $output->writeln('This installation already has more than one user: backup:restore:full would replace its whole database and data directory. It is meant for a fresh installation. Use --force if this really is what you want.'); + return 1; + } + if (!$force && $input->isInteractive()) { + $q = new ConfirmationQuestion('This replaces the database, the data directory and config.php of this installation. Continue? [y/N] ', false); + if (!(new QuestionHelper())->ask($input, $output, $q)) { + $output->writeln('Cancelled.'); + return 1; + } + } try { $target = $this->targets->get($input->getArgument('target')); $snapshot = (string)$input->getArgument('snapshot'); $result = $this->restoreFull->restoreFull($target, $snapshot); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); + $output->writeln('If this happened after the restore had started, the instance may be partly restored: fix the cause and run the same command again with --force.'); return 1; } $output->writeln(sprintf( @@ -65,6 +88,9 @@ HELP); $result['configKeys'], $result['dataFiles'], )); + if (!$result['dbConsistent']) { + $output->writeln('Warning: this snapshot\'s database dump was made over several steps (web cron), so it is not one consistent point in time. Check the instance carefully; prefer a snapshot made with system cron.'); + } $output->writeln('Next: check trusted_domains and overwritehost/overwriteprotocol match this server, then occ maintenance:repair.'); return 0; } diff --git a/lib/Command/RestoreUser.php b/lib/Command/RestoreUser.php index 6ead952..6fa415e 100644 --- a/lib/Command/RestoreUser.php +++ b/lib/Command/RestoreUser.php @@ -10,7 +10,10 @@ namespace OCA\NgBackup\Command; use OCA\NgBackup\Service\TargetService; use OCA\NgBackup\Service\UserRestoreService; use OCA\NgBackup\UserMigration\UserMigrationInstaller; +use OCP\IUserManager; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\QuestionHelper; +use Symfony\Component\Console\Question\ConfirmationQuestion; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -21,6 +24,7 @@ final class RestoreUser extends Command { private TargetService $targets, private UserRestoreService $userRestore, private UserMigrationInstaller $installer, + private IUserManager $userManager, ) { parent::__construct(); } @@ -31,6 +35,7 @@ final class RestoreUser extends Command { ->addArgument('target', InputArgument::REQUIRED, 'Location name or id') ->addArgument('manifest', InputArgument::REQUIRED, 'Manifest path printed by backup:user:backup (users//)') ->addOption('mode', 'm', InputOption::VALUE_REQUIRED, 'replace or as-backup', UserRestoreService::MODE_REPLACE) + ->addOption('force', null, InputOption::VALUE_NONE, 'Replace an existing account without asking (required when not interactive)') ->addOption('install-user-migration', null, InputOption::VALUE_NONE, 'Install and enable user_migration if it is missing (non-interactive consent)') ->setHelp(<<<'HELP' Never imports over a live account. Modes: @@ -47,11 +52,26 @@ HELP); } protected function execute(InputInterface $input, OutputInterface $output): int { + $mode = (string)$input->getOption('mode'); + $uid = explode('/', (string)$input->getArgument('manifest'))[1] ?? ''; + if ($mode === UserRestoreService::MODE_REPLACE && $uid !== '' && $this->userManager->userExists($uid) && !$input->getOption('force')) { + // Replace deletes the live account (after a safety export). Shares to others and data of + // apps without a user_migration migrator do not come back, so never do it unasked. + if (!$input->isInteractive()) { + $output->writeln("$uid exists: --mode=replace deletes and recreates it. Add --force, or use --mode=as-backup."); + return 1; + } + $q = new ConfirmationQuestion("$uid exists and will be deleted and recreated from the backup (a safety export is made first; its shares to others and data of apps without a migrator are not restored). Continue? [y/N] ", false); + if (!(new QuestionHelper())->ask($input, $output, $q)) { + $output->writeln('Cancelled.'); + return 1; + } + } try { $output->writeln('Note: on restore, user_migration only imports the app settings on its own allowlist (e.g. calendar view and reminder settings); other app settings of this user are skipped. Reason: NG Backup uses user_migration\'s importer; widening the list would mean building a settings importer of our own.'); $target = $this->targets->get($input->getArgument('target')); $this->installer->ensureEnabled($input, $output); - $user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), (string)$input->getOption('mode')); + $user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), $mode); } catch (\Throwable $e) { $output->writeln('' . $e->getMessage() . ''); return 1; diff --git a/lib/Controller/AdminApiController.php b/lib/Controller/AdminApiController.php index 3c42182..3d07195 100644 --- a/lib/Controller/AdminApiController.php +++ b/lib/Controller/AdminApiController.php @@ -168,6 +168,7 @@ final class AdminApiController extends Controller { return new JSONResponse($this->targets->availableBackends()); } + #[PasswordConfirmationRequired] public function addTarget(string $name, string $backend, string $auth, array $options = [], string $path = 'ng_backup', ?float $maxGb = null): JSONResponse { try { $r = $this->targets->add($name, $backend, $auth, $options, $path); diff --git a/lib/Crypto/KeyRing.php b/lib/Crypto/KeyRing.php index 9db9d68..92de548 100644 --- a/lib/Crypto/KeyRing.php +++ b/lib/Crypto/KeyRing.php @@ -96,6 +96,12 @@ final class KeyRing { /** @param array{v:int, salt:string, ops:int, mem:int, nonce:string, key:string} $wrapped */ public static function unwrap(array $wrapped, #[\SensitiveParameter] string $passphrase): self { $b64 = static fn (string $s): string => sodium_base642bin($s, SODIUM_BASE64_VARIANT_ORIGINAL); + // ops/mem come from unauthenticated JSON (kit file, repository config): bound them, so a + // damaged or crafted file cannot make the KDF use unbounded memory or time. + if ($wrapped['ops'] < SODIUM_CRYPTO_PWHASH_OPSLIMIT_INTERACTIVE || $wrapped['ops'] > SODIUM_CRYPTO_PWHASH_OPSLIMIT_SENSITIVE + || $wrapped['mem'] < SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE || $wrapped['mem'] > SODIUM_CRYPTO_PWHASH_MEMLIMIT_SENSITIVE) { + throw new CryptoException('Unsupported key derivation parameters (damaged or foreign key file)'); + } $kek = sodium_crypto_pwhash(SODIUM_CRYPTO_SECRETBOX_KEYBYTES, $passphrase, $b64($wrapped['salt']), $wrapped['ops'], $wrapped['mem'], SODIUM_CRYPTO_PWHASH_ALG_ARGON2ID13); $master = sodium_crypto_secretbox_open($b64($wrapped['key']), $b64($wrapped['nonce']), $kek); diff --git a/lib/Crypto/StreamCipher.php b/lib/Crypto/StreamCipher.php index f0bf081..47b1106 100644 --- a/lib/Crypto/StreamCipher.php +++ b/lib/Crypto/StreamCipher.php @@ -122,6 +122,7 @@ final class StreamCipher { /** fread() may return short reads on network streams; keep reading until $length or EOF. */ private static function readFull($in, int $length): string { $buffer = ''; + $empty = 0; while (strlen($buffer) < $length && !feof($in)) { $chunk = fread($in, $length - strlen($buffer)); if ($chunk === false) { @@ -132,8 +133,15 @@ final class StreamCipher { if (feof($in)) { break; } + // A stalled network stream returns '' without EOF forever: back off, then give up + // (about a minute) instead of spinning at full CPU until the lease expires. + if (++$empty > 600) { + throw new CryptoException('Read stalled'); + } + usleep(100000); continue; } + $empty = 0; $buffer .= $chunk; } return $buffer; diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index a43ce98..2202a4c 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -256,7 +256,11 @@ final class DbDumper { } elseif (is_string($row[$i]) && !mb_check_encoding($row[$i], 'UTF-8')) { // Keep invalid UTF-8 byte-exact instead of substituting characters. $row[$i] = ['$b' => base64_encode($row[$i])]; - } elseif (is_int($row[$i]) || is_float($row[$i]) || is_bool($row[$i])) { + } elseif (is_float($row[$i])) { + // var_export uses serialize_precision (shortest exact form), not the lossy + // 14-digit 'precision' of a (string) cast. + $row[$i] = var_export($row[$i], true); + } elseif (is_int($row[$i]) || is_bool($row[$i])) { // Normalise: drivers differ in returning numbers as int or string. $row[$i] = (string)(is_bool($row[$i]) ? (int)$row[$i] : $row[$i]); } diff --git a/lib/Service/RestoreFullService.php b/lib/Service/RestoreFullService.php index 7269caf..1d429ed 100644 --- a/lib/Service/RestoreFullService.php +++ b/lib/Service/RestoreFullService.php @@ -64,7 +64,7 @@ final class RestoreFullService { ]; } - /** @return array{dbReport:array, dataFiles:int, configKeys:int} */ + /** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool} */ public function restoreFull(Target $target, string $snapshotId): array { $check = $this->checkCompatibility($target, $snapshotId); if (!$check['ok']) { @@ -76,6 +76,7 @@ final class RestoreFullService { $meta = $repo->snapshotMeta($snapshotId); $dbReport = null; + $manifest = []; if (isset($meta['db'])) { $manifest = json_decode($repo->getObject($meta['db']), true, 512, JSON_THROW_ON_ERROR); $prefix = $this->config->getSystemValueString('dbtableprefix', 'oc_'); @@ -124,7 +125,8 @@ final class RestoreFullService { ); } - return ['dbReport' => $dbReport ?? ['skipped' => [], 'restored' => []], 'dataFiles' => $dataFiles, 'configKeys' => count($backupConfig)]; + return ['dbReport' => $dbReport ?? ['skipped' => [], 'restored' => []], 'dataFiles' => $dataFiles, 'configKeys' => count($backupConfig), + 'dbConsistent' => (bool)($manifest['consistent'] ?? true)]; }); } diff --git a/tests/integration/user-roundtrip.sh b/tests/integration/user-roundtrip.sh index 73c2283..175b4b1 100755 --- a/tests/integration/user-roundtrip.sh +++ b/tests/integration/user-roundtrip.sh @@ -48,7 +48,8 @@ ok "as-backup restores into -bak, byte-identical, original account untouche # replace: damage the account, then restore it in place under the same uid. $X sh -c "rm '$DATA/$U/files/Docs/f1.bin' && echo damaged > '$DATA/$U/files/Docs/f1.bin'" occ files:scan "$U" >/dev/null -occ backup:user:restore ci "$MANIFEST" --mode=replace >/dev/null || fail "restore replace" +if occ backup:user:restore ci "$MANIFEST" --mode=replace --no-interaction >/dev/null 2>&1; then fail "replace of an existing account ran without --force"; fi +occ backup:user:restore ci "$MANIFEST" --mode=replace --force >/dev/null || fail "restore replace" occ files:scan "$U" >/dev/null SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") [ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after a replace restore" From 16e09f4fde5918d0422e933f3de518371f75a380 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Tue, 6 Oct 2026 23:59:17 +0200 Subject: [PATCH 3/5] Fix trash on SFTP and the disaster recovery test after the review fixes forget() and untrash() uploaded a remote read stream directly, which the new size check could not judge on SFTP, so cleanup failed there. They now copy through a local buffer, and the size check only trusts local streams (php://temp, php://memory, plain files). The disaster recovery test calls backup:restore:full with --no-interaction: docker exec without a terminal still counts as interactive, so the new confirmation question read an empty answer and cancelled. --- lib/Backend/StorageBackend.php | 8 +++++-- lib/Repository/Repository.php | 23 +++++++++++++++++++-- tests/integration/full-restore-roundtrip.sh | 2 +- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index bed3dca..aff055f 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -155,12 +155,16 @@ final class StorageBackend implements IBackend { closedir($dh); } - /** Bytes left to read in a seekable $stream, or null when it cannot tell. @param resource $stream */ + /** + * Bytes left to read in a local $stream (php://temp, php://memory or a plain file), or null: + * stream wrappers of remote storages do not report a reliable size. + */ public static function remaining(mixed $stream): ?int { if (!is_resource($stream)) { return null; } - if (!(stream_get_meta_data($stream)['seekable'] ?? false)) { + $meta = stream_get_meta_data($stream); + if (!($meta['seekable'] ?? false) || !in_array($meta['wrapper_type'] ?? '', ['PHP', 'plainfile'], true)) { return null; } $stat = fstat($stream); diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 767964c..b312d30 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -385,7 +385,7 @@ final class Repository { // either the old state or the new one plus unreferenced leftovers, never a catalog entry // pointing at a missing object. The snapshot object is bound to its id (not its path), // so it can be copied as raw bytes. - $this->backend->put('trash/snapshots/' . $snapshotId, $this->backend->get('snapshots/' . $snapshotId)); + $this->copyRaw('snapshots/' . $snapshotId, 'trash/snapshots/' . $snapshotId); $this->putObject('trash/info/' . $snapshotId, json_encode(['forgottenAt' => time(), 'by' => $by, 'db' => $meta['db'] ?? null, 'dbInTrash' => $dbInTrash, 'time' => $meta['time'] ?? null, 'label' => $meta['label'] ?? ''], JSON_THROW_ON_ERROR)); unset($this->catalog->snapshots[$snapshotId]); @@ -405,7 +405,7 @@ final class Repository { $this->putObject($info['db'], $this->getObject($info['dbInTrash'])); } // Same order as forget(): copy, switch the catalog, then delete the trash copies. - $this->backend->put('snapshots/' . $snapshotId, $this->backend->get('trash/snapshots/' . $snapshotId)); + $this->copyRaw('trash/snapshots/' . $snapshotId, 'snapshots/' . $snapshotId); $this->catalog->snapshots[$snapshotId] = true; unset($this->catalog->trash[$snapshotId]); $this->catalog->write(); @@ -416,6 +416,25 @@ final class Repository { $this->backend->delete('trash/info/' . $snapshotId); } + /** Copy an object as raw bytes, buffered locally (remote read streams cannot be uploaded directly everywhere). */ + private function copyRaw(string $from, string $to): void { + $in = $this->backend->get($from); + $buffer = fopen('php://temp/maxmemory:' . (4 * 1048576), 'w+b'); + if ($buffer === false) { + fclose($in); + throw new RepositoryException("Cannot buffer $from"); + } + try { + if (stream_copy_to_stream($in, $buffer) === false) { + throw new RepositoryException("Cannot read $from"); + } + } finally { + fclose($in); + } + rewind($buffer); + $this->backend->put($to, $buffer); + } + /** Record a user_migration export's manifest in the catalog (rollback-protected like a snapshot). */ public function recordUserExport(string $manifestPath): void { $this->catalog->userExports[$manifestPath] = true; diff --git a/tests/integration/full-restore-roundtrip.sh b/tests/integration/full-restore-roundtrip.sh index c0d270c..7900f0c 100755 --- a/tests/integration/full-restore-roundtrip.sh +++ b/tests/integration/full-restore-roundtrip.sh @@ -68,7 +68,7 @@ run_on ngb-frt-restore backup:target:add offsite local -a null::null -o datadir= run_on ngb-frt-restore backup:list offsite | grep -q "$SNAP" || fail "the origin's snapshot is not listed after connecting the location" ok "recovery kit imported, existing repository adopted, snapshot visible" -run_on ngb-frt-restore backup:restore:full offsite "$SNAP" >/dev/null || fail "restore:full" +run_on ngb-frt-restore backup:restore:full offsite "$SNAP" --no-interaction >/dev/null || fail "restore:full" ok "backup:restore:full completed" # Everything below must work in a FRESH occ process (a new PHP process per docker exec), since From b89b5b5fa6e9f396897184ee2ef3047257e4fa19 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 7 Oct 2026 00:19:46 +0200 Subject: [PATCH 4/5] Document known limitations for the beta and record the review backlog The open points from the pre-beta review are on the backlog in PLAN.md and listed as known limitations in the README; none block 0.9.0-beta or 1.0.0. The how-to warns against pointing a test server at a live location and explains that removing a slot does not revoke access. --- PLAN.md | 18 ++++++++++++++++++ README.md | 28 ++++++++++++++++++++++++++-- docs/HOWTO.md | 6 ++++-- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/PLAN.md b/PLAN.md index fd7d829..e856e7e 100644 --- a/PLAN.md +++ b/PLAN.md @@ -359,6 +359,24 @@ minimale volledige restore (`occ backup:restore:full` op een verse installatie) Backlog: (a) de originele weergavenaam herstellen (het label uit de export is niet getest als weergavenaam); (b) de interne `migrated-`-naam hernoemen (vereist directe databasewijziging, niet voor de beta). +**Backlog uit de pre-beta review (2026-10-07)** — geen belemmering voor 0.9.0-beta of 1.0.0, staat +in README "Known limitations": +1. Geheugen bij grote instanties (>1 TB, miljoenen bestanden): blob-index, prune-set, browse van een + snapshot in de web-UI, `trashExtras` bij restore en het manifest van een user-export staan volledig + in het geheugen; streaming/paginering nodig. +2. Twee servers op één locatie: catalogus kan breken (generatie overschreven, of anchor >200 generaties + achter). Docs-waarschuwing staat er; mogelijk later een echte put-if-absent per backend. +3. Sleutelrotatie: een slot verwijderen trekt geen toegang in (master-key blijft gelijk). +4. Rollback vóór DR niet detecteerbaar: referentie-generatie en -hash in de kit opnemen of tonen. +5. Kit-velden buiten `wrapped_key` (o.a. `delete_delay_days`) zijn niet geauthenticeerd; nu alleen getoond. +6. Repository-id in de AD van objecten (formaatwijziging). +7. Wachtwoorden van locaties via `--option-file` of omgevingsvariabele i.p.v. de commandoregel. +8. Tijdelijke restore-bestanden niet via de Files-API verwijderen (komen nu in de prullenbak). +9. DB-restore in foreign-key-volgorde (cascades). +10. Packs zonder index (index-upload mislukt) opruimen. +11. `backup:restore:full` in onderhoudsmodus en hervatbaar maken; nu: opnieuw draaien met `--force`. +12. Config/rename op FTP/SMB-servers die geen rename over een bestaand bestand toestaan. + Doorgeschoven naar **v1.3.0** (fase 3, niet meer fase 2): in-place volledige restore (eigen restore-modus i.p.v. onderhoudsmodus) en zelfbediening voor gebruikers (eigen bestanden terugzetten vanuit de Files-app). diff --git a/README.md b/README.md index d64caf9..b0be2a8 100644 --- a/README.md +++ b/README.md @@ -7,8 +7,9 @@ installations and hosted Nextcloud. (Choosing SMB as a storage backend is the on see "Storage backend experiences" below — that requirement comes from files_external, not from ng_backup, and applies to any Nextcloud app that writes to an SMB share.) -**Status: pre-release. Not yet tested on production.** See `PLAN.md` for the design and `docs/HOWTO.md` for a -step-by-step guide from setup to a tested restore. +**Status: beta (0.9.0).** Tested thoroughly, usable alongside your current backup, but not yet a +drop-in replacement for it. See "Known limitations" below, `PLAN.md` for the design and +`docs/HOWTO.md` for a step-by-step guide from setup to a tested restore. License: AGPL-3.0-or-later. @@ -147,6 +148,29 @@ Contributions welcome via PR — add what you ran into with your own provider. mid-run behaves like any other local-path failure (the run resumes). See PLAN.md 3.5 for why NFS can't be driven from PHP directly. +## Known limitations + +On the backlog; none of these block the beta or 1.0.0, but know them before you rely on it: + +- **Very large instances** (well over 1 TB, millions of files): the blob index, cleanup, browsing a + snapshot in the web interface and per-user export manifests are held in memory. Expect high + memory use there; reports from such instances are very welcome. +- **One location, one server.** Never let two Nextcloud servers write to the same location (for + example a disaster recovery test server pointed at the production location): they can break each + other's catalog, and the location is then refused as possibly tampered with. Test a disaster + recovery against a copy of the location, or only read from it. +- **Removing a passphrase slot does not revoke access.** The master key stays the same, so someone + with an old recovery kit and that slot's passphrase can still decrypt the location if they can + read it. Key rotation is not available yet. +- **Rollback before a disaster recovery is not detected.** A fresh server has no record of the + newest catalog generation, so a location rolled back to an older state looks valid there. +- Location passwords passed with `-o password=...` are visible in the process list and shell + history while the command runs. +- Temporary files of a restore into a user's files end up in that user's trash. +- Restoring the database does not order tables by foreign keys; apps with cascading foreign keys + could lose rows (Nextcloud core does not use them). +- A pack whose index upload failed stays on the location until removed by hand. + ## Commands | Command | Purpose | diff --git a/docs/HOWTO.md b/docs/HOWTO.md index e6d0097..5e2fe05 100644 --- a/docs/HOWTO.md +++ b/docs/HOWTO.md @@ -14,7 +14,8 @@ occ backup:key:confirm -u --code= --confirm="Yes, I - The passphrase protects the backup. Keep it somewhere other than this server. - Optional: extra passphrases for other people with `occ backup:key:slot:add --label="..."`. - Any one passphrase is enough to restore. + Any one passphrase is enough to restore. Removing a slot later does not revoke access for someone + who kept an old kit and that passphrase (see "Known limitations" in the README). - The recovery kit (`/var/tmp/ng_backup-kit.json` above) is also required for disaster recovery. The web user writes it there; then move it off the server as root, next to the passphrase, and delete the copy in `/var/tmp`. Without the kit and a passphrase, the backup cannot be read. @@ -117,7 +118,8 @@ occ backup:user:restore offsite users// --mode=as-backup ## 7. Disaster recovery: restore the whole server Use a fresh Nextcloud installation of the same major version, with NG Backup installed and -the same location available: +the same location available. For a test, use a **copy** of the location: two servers writing to one +location break each other's catalog. ``` occ backup:key:import /path/ng_backup-kit.json --passphrase-file=/path/passphrase From b37a953cfd66dcafb8891edc27598938198aef03 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 7 Oct 2026 00:48:11 +0200 Subject: [PATCH 5/5] Detect a busy run by identity, and clean up a failed kit write step() hands back the same Run object when another process holds the lease; comparing update times per second could stop the scheduled job early after a quick step. A failed write of the recovery kit file now closes the handle and removes the partial file. --- lib/BackgroundJob/BackupJob.php | 8 ++++---- lib/Command/KeyKit.php | 10 ++++++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/BackgroundJob/BackupJob.php b/lib/BackgroundJob/BackupJob.php index e8c7536..cce6c89 100644 --- a/lib/BackgroundJob/BackupJob.php +++ b/lib/BackgroundJob/BackupJob.php @@ -46,11 +46,11 @@ final class BackupJob extends TimedJob { $this->startScheduled(); foreach ($this->runs->findRunning(null, BackupService::KIND_FULL) as $run) { while (microtime(true) < $deadline && $run->getStatus() === 'running') { - $before = $run->getUpdatedAt(); - $run = $this->backups->step($run, min($deadline, microtime(true) + $budget)); - if ($run->getUpdatedAt() === $before && $run->getStatus() === 'running') { - break; // another process holds this run's lease: leave it to that process + $next = $this->backups->step($run, min($deadline, microtime(true) + $budget)); + if ($next === $run) { + break; // step() hands back the same object when another process holds the run's lease } + $run = $next; } } $this->failStaleRestores(); diff --git a/lib/Command/KeyKit.php b/lib/Command/KeyKit.php index 5a7638c..2284aeb 100644 --- a/lib/Command/KeyKit.php +++ b/lib/Command/KeyKit.php @@ -47,8 +47,14 @@ final class KeyKit extends Command { } finally { umask($old); } - if ($fh === false || fwrite($fh, $json) !== strlen($json) || !fclose($fh)) { - $output->writeln("Cannot write $file (it must not exist yet)"); + if ($fh === false) { + $output->writeln("Cannot create $file (it must not exist yet)"); + return 1; + } + $ok = fwrite($fh, $json) === strlen($json); + if (!fclose($fh) || !$ok) { + @unlink($file); + $output->writeln("Cannot write $file"); return 1; } $output->writeln("Recovery kit written to $file. Copy it off this server, then run occ backup:key:confirm.");