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
This commit is contained in:
2026-10-02 11:25:22 +02:00
parent ac82b38e37
commit 131bb6178d
8 changed files with 57 additions and 8 deletions
+11 -1
View File
@@ -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));
+1 -1
View File
@@ -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 <info>$n</info> files to $dir.");
+11 -2
View File
@@ -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));
}
+8 -2
View File
@@ -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));
}
+17
View File
@@ -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.
+4 -1
View File
@@ -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/<user>/files/<folder or file>');
}
[$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");
}