From 131bb6178dbd3871d58ce659e20c5288eb92897c Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Fri, 2 Oct 2026 11:25:22 +0200 Subject: [PATCH] Close remaining review gaps - restore runs from the web are created under the location's exclusive lock, like backup runs - raw restore writes to an unpredictable temporary name opened with exclusive create (never follows a planted symlink) and validates the path prefix; the restore service rejects non-canonical paths instead of trimming them (occ normalises user input before calling it) - restore jobs report progress every minute, so a long restore is not taken for a dead one - README: Object Lock turns deletions into delete markers --- README.md | 4 ++++ appinfo/info.xml | 2 +- lib/BackgroundJob/RestoreJob.php | 12 +++++++++++- lib/Command/Restore.php | 2 +- lib/Controller/AdminApiController.php | 13 +++++++++++-- lib/Repository/Repository.php | 10 ++++++++-- lib/Service/PruneService.php | 17 +++++++++++++++++ lib/Service/RestoreService.php | 5 ++++- 8 files changed, 57 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 432e58c..393637c 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,10 @@ What you should do on the location (strongly recommended): for example **90 days**, plus versioning. Then nobody, not even with the server's credentials, can delete or overwrite backups younger than 90 days. Combine with an append-only location in NG Backup and a lifecycle rule that removes old versions after the lock expires. + With Object Lock, a deletion by NG Backup (or by an attacker through it) only adds a *delete + marker*: the locked data stays and can be brought back from the bucket's object versions until + the lock expires. Tested with an S3-compatible server; still to be verified on AWS itself, which + requires a checksum on uploads to locked buckets. - Use **credentials without delete rights** for the backup location where the storage supports it. - **SFTP / storage boxes:** enable the provider's own snapshots (e.g. daily, kept 30-90 days) and give the backup user access to its own folder only, without a shell. diff --git a/appinfo/info.xml b/appinfo/info.xml index 8e04c72..12484c4 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -5,7 +5,7 @@ NG Backup Encrypted, incremental backups of your Nextcloud with restore from the web interface - 0.0.12 + 0.0.13 agpl Dillard Blom NgBackup diff --git a/lib/BackgroundJob/RestoreJob.php b/lib/BackgroundJob/RestoreJob.php index 4ede668..37652ff 100644 --- a/lib/BackgroundJob/RestoreJob.php +++ b/lib/BackgroundJob/RestoreJob.php @@ -45,7 +45,17 @@ class RestoreJob extends QueuedJob { $run->setUpdatedAt(time()); $run = $this->runs->update($run); @set_time_limit(0); - $result = $this->restore->restoreUserFiles($this->targets->get((string)$run->getTargetId()), $req['snapshot'], $req['path'], $req['mode']); + $lastBeat = time(); + $result = $this->restore->restoreUserFiles($this->targets->get((string)$run->getTargetId()), $req['snapshot'], $req['path'], $req['mode'], + function (int $done, int $total) use (&$run, &$lastBeat): void { + // Heartbeat, so a long restore is not mistaken for a dead one. + if (time() - $lastBeat >= 60) { + $lastBeat = time(); + $run->setPhase("restoring $done/$total"); + $run->setUpdatedAt(time()); + $run = $this->runs->update($run); + } + }); $run->setStatus(Run::DONE); $run->setPhase('done'); $run->setStats(json_encode($result, JSON_THROW_ON_ERROR)); diff --git a/lib/Command/Restore.php b/lib/Command/Restore.php index d4131ed..64edec1 100644 --- a/lib/Command/Restore.php +++ b/lib/Command/Restore.php @@ -49,7 +49,7 @@ HELP); try { $target = $this->targets->get($input->getArgument('target')); $snapshot = $input->getArgument('snapshot'); - $path = $input->getArgument('path'); + $path = trim((string)$input->getArgument('path'), '/'); // CLI convenience; the service itself only accepts canonical paths if ($dir = $input->getOption('to-directory')) { $n = $this->restore->restoreToDirectory($target, $snapshot, $path, $dir); $output->writeln("Restored $n files to $dir."); diff --git a/lib/Controller/AdminApiController.php b/lib/Controller/AdminApiController.php index ee7abc6..fca4a45 100644 --- a/lib/Controller/AdminApiController.php +++ b/lib/Controller/AdminApiController.php @@ -234,8 +234,13 @@ class AdminApiController extends Controller { || !\OCA\NgBackup\Repository\Repository::isSafePath($path) || !preg_match('#^data/[^/]+/files(/.*)?$#', $path)) { return new JSONResponse(['error' => 'Invalid path or mode'], Http::STATUS_BAD_REQUEST); } + try { + $target = $this->targets->get((string)$targetId); + } catch (\Throwable) { + return new JSONResponse(['error' => 'No such location'], Http::STATUS_NOT_FOUND); + } $run = new Run(); - $run->setTargetId($targetId); + $run->setTargetId($target->getId()); $run->setKind('restore'); $run->setStatus(Run::RUNNING); $run->setPhase('queued'); @@ -244,7 +249,11 @@ class AdminApiController extends Controller { $run->setSnapshotId($snapshotId); $run->setStartedAt(time()); $run->setUpdatedAt(time()); - $run = $this->runs->insert($run); + try { + $run = $this->prune->underStartLock($target, fn () => $this->runs->insert($run)); + } catch (\Throwable $e) { + return new JSONResponse(['error' => $e->getMessage()], Http::STATUS_CONFLICT); + } $this->jobs->add(RestoreJob::class, ['run' => $run->getId()]); return new JSONResponse($this->runJson($run)); } diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 0f0a716..d0d4af1 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -162,6 +162,9 @@ final class Repository { /** Restore a snapshot (or only paths starting with $prefix) into a local directory. */ public function restore(string $snapshotId, string $target, string $prefix = ''): int { + if ($prefix !== '' && !self::isSafePath(rtrim($prefix, '/'))) { + throw new RepositoryException('Invalid path prefix'); + } $count = 0; foreach ($this->streamEntries($snapshotId) as $entry) { if ($prefix !== '' && !str_starts_with($entry['p'], $prefix)) { @@ -179,8 +182,11 @@ final class Repository { } // Write next to the destination and rename after all blobs verified: at most one file // is ever present twice. - $tmp = $dest . '.ngb-restore'; - $out = fopen($tmp, 'wb'); + $tmp = $dest . '.ngb-restore-' . bin2hex(random_bytes(6)); + $out = @fopen($tmp, 'xb'); // fails if anything (including a symlink) already exists there + if ($out === false) { + throw new RepositoryException('Cannot create a temporary file for ' . $entry['p']); + } foreach ($entry['b'] as $id) { fwrite($out, $this->loadBlob($id)); } diff --git a/lib/Service/PruneService.php b/lib/Service/PruneService.php index 7244e9c..5ae078c 100644 --- a/lib/Service/PruneService.php +++ b/lib/Service/PruneService.php @@ -41,6 +41,23 @@ class PruneService { } } + /** + * Run $fn under the location's exclusive lock (to create a run atomically with respect to a + * cleanup, which takes the same lock and then refuses while a run exists). + */ + public function underStartLock(Target $target, callable $fn): mixed { + try { + $this->locking->acquireLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + } catch (LockedException) { + throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is in progress; try again later'); + } + try { + return $fn(); + } finally { + $this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE); + } + } + /** * Exclusive lock on the location, then make sure no run of any kind (backup, restore, export) * is in progress: their data may not be referenced by a snapshot yet, or is being read. diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index bddb514..1750a1c 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -92,7 +92,7 @@ class RestoreService { } private function doRestoreUserFiles(Target $target, string $snapshotId, string $logicalPath, string $mode, ?callable $progress): array { - if (!Repository::isSafePath(trim($logicalPath, '/')) || !preg_match('#^data/([^/]+)/files(?:/(.*))?$#', trim($logicalPath, '/'), $m)) { + if (!Repository::isSafePath($logicalPath) || !preg_match('#^data/([^/]+)/files(?:/(.*))?$#', $logicalPath, $m)) { throw new \InvalidArgumentException('Expected a path like data//files/'); } [$uid, $rel] = [$m[1], trim($m[2] ?? '', '/')]; @@ -144,6 +144,9 @@ class RestoreService { /** Raw restore of any snapshot path to a local directory (administrators, disaster recovery). */ public function restoreToDirectory(Target $target, string $snapshotId, string $prefix, string $directory): int { + if ($prefix !== '' && !Repository::isSafePath($prefix)) { + throw new \InvalidArgumentException('Invalid path: ' . $prefix); + } if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) { throw new \RuntimeException("Cannot create $directory"); }