diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bad4d2..83ce08c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,11 @@ instead carried by the app description, the git tag, the release, and this file. when the login was more than 30 minutes old: the form did not ask for password confirmation. - Settings page: the "Backup and restore" / "Settings" tabs line up with the sections below them. - Backups: the runs table scrolls horizontally on narrow screens instead of running off the page. +- `occ backup:verify --deep`: a blob that could not be read (network or storage error) was reported + as "corrupted or tampered". Reads are now retried, and a read that keeps failing is reported + separately as unreadable, with the error; only a blob that was read but fails decryption or its + hash check counts as corrupted. Found on a Hetzner Storage Box over WebDAV, where the blob + turned out to be intact. ## [0.9.1-beta.2] - 2026-10-07 diff --git a/lib/Command/Verify.php b/lib/Command/Verify.php index a9d1a7c..148f476 100644 --- a/lib/Command/Verify.php +++ b/lib/Command/Verify.php @@ -66,7 +66,12 @@ HELP); if ($result['failed'] !== []) { $output->writeln('' . count($result['failed']) . ' blob(s) failed verification (corrupted or tampered): ' . implode(', ', $result['failed'])); } - if ($result['missing'] === [] && $result['failed'] === []) { + if ($result['unreadable'] !== []) { + $output->writeln('' . count($result['unreadable']) . ' blob(s) could not be read from the location (storage or network error, ' + . 'not a sign of corruption; run the check again): ' . implode(', ', $result['unreadable'])); + $output->writeln('Last read error: ' . $result['readError']); + } + if ($result['missing'] === [] && $result['failed'] === [] && $result['unreadable'] === []) { $output->writeln('OK'); return 0; } diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index b312d30..be0b35d 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -28,6 +28,8 @@ final class Repository { public const FORMAT = 1; public const BLOB_SIZE = 4 * 1048576; private const FLAG_DEFLATE = 1; + /** Reads per blob in a deep verify before it is reported as unreadable (not corrupted). */ + private const VERIFY_READ_ATTEMPTS = 3; private StreamCipher $cipher; private BlobIndex $index; @@ -696,8 +698,13 @@ final class Repository { } private function loadBlob(string $id): string { - [$pack, $offset, $length, $raw, $flags] = $this->index->get($id); - $encrypted = $this->backend->getRange('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length); + [$pack, $offset, $length] = $this->index->get($id); + return $this->decodeBlob($id, $this->backend->getRange('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length)); + } + + /** Decrypts and checks a blob read from its pack; throws when it is corrupted or tampered with. */ + private function decodeBlob(string $id, string $encrypted): string { + [, , , $raw, $flags] = $this->index->get($id); $payload = $this->cipher->decryptString($encrypted, 'blob:' . $id); $data = ($flags & self::FLAG_DEFLATE) ? gzinflate($payload) : $payload; if ($data === false || strlen($data) !== $raw || $this->keys->blobId($data) !== $id) { @@ -733,13 +740,13 @@ final class Repository { * * @param ?callable $heartbeat called after every blob (shallow) or every blob read (deep); * callers use it to keep a time-boxed lease alive during a large audit - * @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} + * @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list, unreadable:list, readError:string} */ public function verify(string $snapshotId, bool $deep = false, ?callable $heartbeat = null): array { $heartbeat ??= static function (): void { }; $this->index->load(true); - $stats = ['filesChecked' => 0, 'blobsChecked' => 0, 'bytesChecked' => 0, 'missing' => [], 'failed' => []]; + $stats = ['filesChecked' => 0, 'blobsChecked' => 0, 'bytesChecked' => 0, 'missing' => [], 'failed' => [], 'unreadable' => [], 'readError' => '']; $seen = []; $packSeen = []; // pack id => exists on the backend; avoids one exists() call per blob foreach ($this->streamEntries($snapshotId) as $entry) { @@ -778,7 +785,7 @@ final class Repository { * Same checks as verify(), for a user export: every blob its manifest references must be in * the freshly re-authenticated index with its pack present; $deep also decrypts each. * - * @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} + * @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list, unreadable:list, readError:string} */ public function verifyUserExport(string $manifestPath, bool $deep = false, ?callable $heartbeat = null): array { if (!$this->isRecordedUserExport($manifestPath)) { @@ -787,7 +794,7 @@ final class Repository { $heartbeat ??= static function (): void { }; $this->index->load(true); - $stats = ['filesChecked' => 0, 'blobsChecked' => 0, 'bytesChecked' => 0, 'missing' => [], 'failed' => []]; + $stats = ['filesChecked' => 0, 'blobsChecked' => 0, 'bytesChecked' => 0, 'missing' => [], 'failed' => [], 'unreadable' => [], 'readError' => '']; $seen = []; $packSeen = []; $manifest = json_decode($this->getObject($manifestPath), true, 512, JSON_THROW_ON_ERROR); @@ -824,8 +831,25 @@ final class Repository { if (!$deep) { return; } + // Reading and checking are kept apart: a read that fails (network, storage) says + // nothing about the stored data, so it is retried and reported as unreadable, + // never as corrupted. Only a blob that was read but does not decrypt or match + // its id counts as failed. + [, $offset, $length] = $this->index->get($id); + $encrypted = null; + for ($attempt = 1; $attempt <= self::VERIFY_READ_ATTEMPTS && $encrypted === null; $attempt++) { + try { + $encrypted = $this->backend->getRange('packs/' . substr($pack, 0, 2) . '/' . $pack, $offset, $length); + } catch (\Throwable $e) { + $stats['readError'] = $e->getMessage(); + } + } + if ($encrypted === null) { + $stats['unreadable'][] = $id; + return; + } try { - $stats['bytesChecked'] += strlen($this->loadBlob($id)); + $stats['bytesChecked'] += strlen($this->decodeBlob($id, $encrypted)); } catch (\Throwable) { $stats['failed'][] = $id; } diff --git a/lib/Service/VerifyService.php b/lib/Service/VerifyService.php index bef188e..fcfd68e 100644 --- a/lib/Service/VerifyService.php +++ b/lib/Service/VerifyService.php @@ -18,12 +18,12 @@ final class VerifyService { ) { } - /** @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} */ + /** @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list, unreadable:list, readError:string} */ public function verify(Target $target, string $snapshotId, bool $deep = false): array { return $this->run($target, $deep, fn (\OCA\NgBackup\Repository\Repository $repo, callable $heartbeat): array => $repo->verify($snapshotId, $deep, $heartbeat)); } - /** @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list} */ + /** @return array{filesChecked:int, blobsChecked:int, bytesChecked:int, missing:list, failed:list, unreadable:list, readError:string} */ public function verifyUserExport(Target $target, string $manifestPath, bool $deep = false): array { return $this->run($target, $deep, fn (\OCA\NgBackup\Repository\Repository $repo, callable $heartbeat): array => $repo->verifyUserExport($manifestPath, $deep, $heartbeat)); } diff --git a/tests/Unit/Repository/VerifyTest.php b/tests/Unit/Repository/VerifyTest.php index 7b9ed77..88057e6 100644 --- a/tests/Unit/Repository/VerifyTest.php +++ b/tests/Unit/Repository/VerifyTest.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace OCA\NgBackup\Tests\Unit\Repository; +use OCA\NgBackup\Backend\BackendException; +use OCA\NgBackup\Backend\IBackend; use OCA\NgBackup\Backend\LocalBackend; use OCA\NgBackup\Crypto\KeyRing; use OCA\NgBackup\Repository\Repository; @@ -92,4 +94,69 @@ class VerifyTest extends TestCase { $this->assertNotSame([], $result['failed']); } + + public function testDeepVerifyRetriesAReadThatFailsOnce(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $s = $repo->backupDirectory($this->src); + $flaky = new FlakyRangeBackend($this->backend, failures: 1); + + $result = Repository::openWithKey($flaky, $this->keys, null, null)->verify($s['snapshot'], deep: true); + + $this->assertSame(200000, $result['bytesChecked']); + $this->assertSame([], $result['failed']); + $this->assertSame([], $result['unreadable']); + } + + public function testDeepVerifyReportsAPersistentReadErrorAsUnreadableNotCorrupted(): void { + $repo = Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $s = $repo->backupDirectory($this->src); + $broken = new FlakyRangeBackend($this->backend, failures: PHP_INT_MAX); + + $result = Repository::openWithKey($broken, $this->keys, null, null)->verify($s['snapshot'], deep: true); + + $this->assertSame([], $result['failed']); + $this->assertNotSame([], $result['unreadable']); + $this->assertSame('connection reset', $result['readError']); + } +} + +/** Delegates to a real backend, but getRange() throws for the first $failures calls. */ +final class FlakyRangeBackend implements IBackend { + public function __construct( + private IBackend $inner, + private int $failures, + ) { + } + + public function put(string $path, $stream): void { + $this->inner->put($path, $stream); + } + + public function get(string $path) { + return $this->inner->get($path); + } + + public function getRange(string $path, int $offset, int $length): string { + if ($this->failures > 0) { + $this->failures--; + throw new BackendException('connection reset'); + } + return $this->inner->getRange($path, $offset, $length); + } + + public function exists(string $path): bool { + return $this->inner->exists($path); + } + + public function move(string $from, string $to): void { + $this->inner->move($from, $to); + } + + public function list(string $prefix): array { + return $this->inner->list($prefix); + } + + public function delete(string $path): void { + $this->inner->delete($path); + } }