From aa541d58b2f3aae4b33edb2f0e4008cf4513baa6 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Thu, 1 Oct 2026 18:58:51 +0200 Subject: [PATCH] Phase 0: fixes from review (torn files, pending blobs, exact sequences, UTF-8, storage checks) - detect files changing during reading or between resumable steps and re-read them (size/mtime/ctime/inode), spike F overwrites mid-run - drop pending blob ids when a pack or index upload fails - dump and restore PostgreSQL sequence state exactly, bound parameters, validated catalog names; spike C checks all sequences exactly - keep invalid UTF-8 byte-exact (tagged base64) - refuse large tables without a primary key - StorageBackend: check write/flush/close results, validate base path - user_migration import/export: root paths, intermediate folders, partial writes, read errors - spikes: never touch an existing account, own fixtures, 512M note --- lib/Backend/LocalBackend.php | 42 ++++++---- lib/Backend/StorageBackend.php | 29 +++++-- lib/Db/DbDumper.php | 80 ++++++++++++++++--- lib/Job/BackupRun.php | 19 ++++- lib/Repository/BlobIndex.php | 7 ++ lib/Repository/PackWriter.php | 14 +++- lib/Repository/Repository.php | 37 +++++++-- .../RepositoryExportDestination.php | 3 +- lib/UserMigration/RepositoryImportSource.php | 17 +++- spikes/03-db-dump.php | 47 ++++++----- spikes/05-user-migration.php | 9 ++- spikes/06-resumable.php | 11 +++ spikes/RESULTS.md | 29 +++++++ 13 files changed, 274 insertions(+), 70 deletions(-) diff --git a/lib/Backend/LocalBackend.php b/lib/Backend/LocalBackend.php index e759d85..defb827 100644 --- a/lib/Backend/LocalBackend.php +++ b/lib/Backend/LocalBackend.php @@ -18,30 +18,40 @@ final class LocalBackend implements IBackend { } public function put(string $path, $stream): void { - $target = $this->abs($path); - $dir = dirname($target); - if (!is_dir($dir) && !mkdir($dir, 0700, true) && !is_dir($dir)) { - throw new BackendException('Cannot create ' . $dir); - } - // Write to a temporary name and rename, so a crash never leaves a partial object. - $tmp = $target . '.part-' . bin2hex(random_bytes(4)); - $out = fopen($tmp, 'wb'); - if ($out === false) { - throw new BackendException('Cannot write ' . $path); - } + $tmp = null; + $out = false; + $done = false; try { + $target = $this->abs($path); + $dir = dirname($target); + if (!is_dir($dir) && !mkdir($dir, 0700, true) && !is_dir($dir)) { + throw new BackendException('Cannot create ' . $dir); + } + // Write to a temporary name and rename, so a crash never leaves a partial object. + $tmp = $target . '.part-' . bin2hex(random_bytes(4)); + $out = fopen($tmp, 'wb'); + if ($out === false) { + throw new BackendException('Cannot write ' . $path); + } if (stream_copy_to_stream($stream, $out) === false || !fflush($out)) { throw new BackendException('Write failed for ' . $path); } - } finally { fclose($out); + $out = false; + if (!rename($tmp, $target)) { + throw new BackendException('Cannot finalise ' . $path); + } + $done = true; + } finally { + if ($out !== false) { + fclose($out); + } if (is_resource($stream)) { fclose($stream); } - } - if (!rename($tmp, $target)) { - @unlink($tmp); - throw new BackendException('Cannot finalise ' . $path); + if (!$done && $tmp !== null) { + @unlink($tmp); + } } } diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 4c5deae..57b2504 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -23,6 +23,7 @@ final class StorageBackend implements IBackend { private string $base = '', ) { $this->base = trim($base, '/'); + self::validate($this->base, true); $this->atomicPut = $storage->instanceOfStorage(\OCA\Files_External\Lib\Storage\AmazonS3::class); if ($this->base !== '') { $this->mkdirs($this->base); @@ -37,13 +38,18 @@ final class StorageBackend implements IBackend { if ($this->storage->instanceOfStorage(IWriteStreamStorage::class)) { /** @var IWriteStreamStorage $s */ $s = $this->storage; - $s->writeStream($writeTo, $stream); + $written = $s->writeStream($writeTo, $stream); + if ($written <= 0 && $this->storage->filesize($writeTo) !== 0) { + throw new BackendException('Write reported no data for ' . $path); + } } else { $out = $this->storage->fopen($writeTo, 'w'); - if ($out === false || stream_copy_to_stream($stream, $out) === false) { + if ($out === false || stream_copy_to_stream($stream, $out) === false || !fflush($out)) { throw new BackendException('Write failed for ' . $path); } - fclose($out); + if (!fclose($out)) { + throw new BackendException('Close failed for ' . $path); + } } if (is_resource($stream)) { fclose($stream); @@ -139,10 +145,23 @@ final class StorageBackend implements IBackend { $this->storage->mkdir($dir); } - private function abs(string $path): string { - if (str_contains($path, '..')) { + /** Reject empty segments, '.', '..', NUL, backslashes and absolute paths. */ + private static function validate(string $path, bool $allowEmpty = false): void { + if ($path === '' && $allowEmpty) { + return; + } + if ($path === '' || str_starts_with($path, '/') || str_contains($path, "\\") || str_contains($path, "\0")) { throw new BackendException('Invalid path: ' . $path); } + foreach (explode('/', $path) as $segment) { + if ($segment === '' || $segment === '.' || $segment === '..') { + throw new BackendException('Invalid path: ' . $path); + } + } + } + + private function abs(string $path): string { + self::validate(ltrim($path, '/')); return ltrim(($this->base === '' ? '' : $this->base . '/') . ltrim($path, '/'), '/'); } } diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index 6273552..035c19d 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -31,6 +31,8 @@ use OCA\NgBackup\Repository\Repository; */ final class DbDumper { private const BATCH = 2000; + /** Tables without a primary key are read in one query; refuse that above this size. */ + private const MAX_ROWS_WITHOUT_PK = 50000; public function __construct( private Connection $db, @@ -66,11 +68,12 @@ final class DbDumper { } $result[$table] = ['blobs' => $writer->finish(), 'rows' => $rows, 'bytes' => $writer->bytes(), 'sha256' => hash_final($hash)]; } + $sequences = $this->sequenceStates($tables); } finally { $this->db->rollBack(); } $repo->flushPacks(); - return ['tables' => $result, 'provider' => get_class($this->db->getDatabasePlatform())]; + return ['tables' => $result, 'sequences' => $sequences, 'provider' => get_class($this->db->getDatabasePlatform())]; } /** @@ -106,6 +109,9 @@ final class DbDumper { $row = []; foreach ($columns as $i => $col) { $value = $data[$i]; + if (is_array($value) && isset($value['$b'])) { + $value = base64_decode($value['$b'], true); // text that was not valid UTF-8 + } $row[$col] = ($value !== null && isset($binary[$col])) ? base64_decode($value, true) : $value; } $batch[] = $row; @@ -117,8 +123,8 @@ final class DbDumper { if ($batch !== []) { $total += $this->insertBatch($table, $batch, $binary); } - $this->resetSequences($table, $sm->introspectTable($table)); } + $this->restoreSequences($manifest['sequences'] ?? []); $this->db->commit(); } catch (\Throwable $e) { $this->db->rollBack(); @@ -158,7 +164,11 @@ final class DbDumper { $select = 'SELECT ' . implode(', ', array_map($q, $columns)) . ' FROM ' . $q($table->getName()); if ($pk === []) { - // Nextcloud requires primary keys; fall back to one query for odd tables. + // Nextcloud requires primary keys; small odd tables are read in one query. + $count = (int)$this->db->fetchOne('SELECT COUNT(*) FROM ' . $q($table->getName())); + if ($count > self::MAX_ROWS_WITHOUT_PK) { + throw new \RuntimeException("Table {$table->getName()} has no primary key and $count rows; cannot dump it with bounded memory"); + } $batches = [$this->db->fetchAllNumeric($select)]; } else { $batches = $this->keysetBatches($select, $pk, $columns); @@ -171,12 +181,15 @@ final class DbDumper { } if ($row[$i] !== null && isset($binaryFlip[$col])) { $row[$i] = base64_encode((string)$row[$i]); + } 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])) { // Normalise: drivers differ in returning numbers as int or string. $row[$i] = (string)(is_bool($row[$i]) ? (int)$row[$i] : $row[$i]); } } - yield 'row' => json_encode($row, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_INVALID_UTF8_SUBSTITUTE | JSON_THROW_ON_ERROR); + yield 'row' => json_encode($row, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR); } } } @@ -219,15 +232,60 @@ final class DbDumper { return count($rows); } - private function resetSequences(string $table, \Doctrine\DBAL\Schema\Table $schema): void { + /** + * Exact state of every sequence owned by a dumped column (PostgreSQL), so a restore continues + * where the original left off, not at MAX(id)+1. + * + * @return array + */ + private function sequenceStates(array $tables): array { if (!$this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { - return; // MySQL/SQLite derive AUTO_INCREMENT from the data + return []; // MySQL/SQLite derive AUTO_INCREMENT from the data } - foreach ($schema->getColumns() as $column) { - if ($column->getAutoincrement()) { - $this->db->executeStatement(sprintf( - "SELECT setval(pg_get_serial_sequence('%s', '%s'), COALESCE((SELECT MAX(%s) FROM %s), 0) + 1, false)", - $table, $column->getName(), $this->db->quoteIdentifier($column->getName()), $this->db->quoteIdentifier($table))); + $rows = $this->db->fetchAllAssociative( + "SELECT c.table_name, c.column_name, pg_get_serial_sequence(quote_ident(c.table_name), c.column_name) AS seq + FROM information_schema.columns c + WHERE c.table_schema = current_schema() AND pg_get_serial_sequence(quote_ident(c.table_name), c.column_name) IS NOT NULL"); + $wanted = array_flip($tables); + $result = []; + foreach ($rows as $r) { + if (!isset($wanted[$r['table_name']])) { + continue; + } + $state = $this->db->fetchAssociative('SELECT last_value, is_called FROM ' . self::sequenceIdentifier($r['seq'])); + $result[$r['seq']] = ['table' => $r['table_name'], 'column' => $r['column_name'], + // A never-used sequence reports last_value = start value with is_called = false. + 'last' => $state ? (int)$state['last_value'] : null, + 'called' => (bool)($state['is_called'] ?? false)]; + } + return $result; + } + + /** pg_get_serial_sequence() returns a catalog name like public.oc_x_id_seq or "My"."seq"; refuse anything else. */ + private static function sequenceIdentifier(string $name): string { + if (!preg_match('/^("[^"]+"|[a-z_][a-z0-9_$]*)\.("[^"]+"|[a-z_][a-z0-9_$]*)$/i', $name)) { + throw new \RuntimeException('Unexpected sequence name: ' . $name); + } + return $name; + } + + private function restoreSequences(array $sequences): void { + if (!$this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { + return; + } + foreach ($sequences as $info) { + $seq = $this->db->fetchOne('SELECT pg_get_serial_sequence(quote_ident(?), ?)', [$info['table'], $info['column']]); + if ($seq === false || $seq === null) { + continue; + } + if ($info['last'] === null) { + // Never used: start at 1 (or after the data, if rows were inserted with explicit ids). + $max = (int)$this->db->fetchOne(sprintf('SELECT COALESCE(MAX(%s), 0) FROM %s', + $this->db->quoteIdentifier($info['column']), $this->db->quoteIdentifier($info['table']))); + $this->db->executeQuery('SELECT setval(CAST(? AS regclass), ?, false)', [$seq, $max + 1]); + } else { + $this->db->executeQuery('SELECT setval(CAST(? AS regclass), ?, ?)', [$seq, $info['last'], $info['called']], + [\Doctrine\DBAL\ParameterType::STRING, \Doctrine\DBAL\ParameterType::INTEGER, \Doctrine\DBAL\ParameterType::BOOLEAN]); } } } diff --git a/lib/Job/BackupRun.php b/lib/Job/BackupRun.php index 3e2f642..03ce2d4 100644 --- a/lib/Job/BackupRun.php +++ b/lib/Job/BackupRun.php @@ -73,7 +73,15 @@ final class BackupRun { continue; } - $cur = $state['cur'] ?? ['path' => $relative, 'offset' => 0, 'blobs' => []]; + $version = Repository::fileVersion($path); + $cur = $state['cur']; + if ($cur === null || $cur['version'] !== $version) { + // New file, or it changed since the previous step: (re)start reading it. + if ($cur !== null) { + $stats['retried'] = ($stats['retried'] ?? 0) + 1; + } + $cur = ['path' => $relative, 'offset' => 0, 'blobs' => [], 'version' => $version]; + } $fh = fopen($path, 'rb'); fseek($fh, $cur['offset']); $finished = true; @@ -86,6 +94,15 @@ final class BackupRun { } } fclose($fh); + if (Repository::fileVersion($path) !== $cur['version']) { + // Changed while reading: start this file again (in this or the next step). + $stats['retried'] = ($stats['retried'] ?? 0) + 1; + $state['cur'] = null; + if (microtime(true) >= $deadline) { + break; + } + continue; + } if (!$finished) { $state['cur'] = $cur; break; diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index d67697e..f52a134 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -55,6 +55,13 @@ final class BlobIndex { $this->pending[$id] = true; } + /** @param list $ids */ + public function dropPending(array $ids): void { + foreach ($ids as $id) { + unset($this->pending[$id]); + } + } + /** @param list $entries */ public function addPack(string $packId, array $entries): void { $stream = fopen('php://memory', 'w+b'); diff --git a/lib/Repository/PackWriter.php b/lib/Repository/PackWriter.php index 882b9da..eb6b515 100644 --- a/lib/Repository/PackWriter.php +++ b/lib/Repository/PackWriter.php @@ -59,9 +59,17 @@ final class PackWriter { fwrite($this->buffer, $header . pack('N', strlen($header))); rewind($this->buffer); $path = 'packs/' . substr($packId, 0, 2) . '/' . $packId; - $this->backend->put($path, $this->buffer); // closes the buffer - unset($this->buffer); - $this->index->addPack($packId, $this->entries); + try { + $this->backend->put($path, $this->buffer); // closes the buffer + unset($this->buffer); + $this->index->addPack($packId, $this->entries); + } catch (\Throwable $e) { + // The blobs of this pack are not durable: forget them, so a retry stores them again. + $this->index->dropPending(array_column($this->entries, 'id')); + unset($this->buffer); + $this->reset(); + throw $e; + } $this->reset(); } diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 90f3b0e..0a98438 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -92,12 +92,7 @@ final class Repository { $blobs = $prev['b']; $stats['reused']++; } else { - $blobs = []; - $fh = fopen($info->getPathname(), 'rb'); - while (($data = self::readFull($fh, self::BLOB_SIZE)) !== '') { - $blobs[] = $this->storeBlob($data, $packs, $stats); - } - fclose($fh); + $blobs = $this->readStable($info->getPathname(), $packs, $stats); $stats['read']++; } fwrite($tree, json_encode(['p' => $relative, 's' => $info->getSize(), 'm' => $info->getMTime(), 'b' => $blobs], JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR) . "\n"); @@ -244,6 +239,36 @@ final class Repository { return $this->index->count(); } + /** + * Read a file into blobs and make sure it did not change while reading (size, mtime, ctime + * before and after). A file that keeps changing is an error rather than a torn backup. + * + * @return list + */ + private function readStable(string $path, PackWriter $packs, array &$stats): array { + for ($attempt = 1; $attempt <= 3; $attempt++) { + $before = self::fileVersion($path); + $blobs = []; + $fh = fopen($path, 'rb'); + while (($data = self::readFull($fh, self::BLOB_SIZE)) !== '') { + $blobs[] = $this->storeBlob($data, $packs, $stats); + } + fclose($fh); + if (self::fileVersion($path) === $before) { + return $blobs; + } + $stats['retried'] = ($stats['retried'] ?? 0) + 1; + } + throw new RepositoryException('File keeps changing while being read: ' . $path); + } + + /** Cheap file identity: changes when the content may have changed. */ + public static function fileVersion(string $path): string { + clearstatcache(true, $path); + $st = @stat($path); + return $st === false ? 'missing' : implode(':', [$st['size'], $st['mtime'], $st['ctime'], $st['ino']]); + } + private function storeBlob(string $data, PackWriter $packs, array &$stats): string { $id = $this->keys->blobId($data); if ($this->index->has($id)) { diff --git a/lib/UserMigration/RepositoryExportDestination.php b/lib/UserMigration/RepositoryExportDestination.php index 41d59db..3eb1ce1 100644 --- a/lib/UserMigration/RepositoryExportDestination.php +++ b/lib/UserMigration/RepositoryExportDestination.php @@ -11,6 +11,7 @@ use OCA\NgBackup\Repository\Repository; use OCP\Files\File; use OCP\Files\Folder; use OCP\UserMigration\IExportDestination; +use OCP\UserMigration\UserMigrationException; /** * Export destination for user_migration that writes straight into the encrypted repository: @@ -42,7 +43,7 @@ final class RepositoryExportDestination implements IExportDestination { while (!feof($stream)) { $chunk = fread($stream, 1048576); if ($chunk === false) { - break; + throw new UserMigrationException('Read error while exporting ' . $path); } $writer->write($chunk); } diff --git a/lib/UserMigration/RepositoryImportSource.php b/lib/UserMigration/RepositoryImportSource.php index 4876fbd..94f4460 100644 --- a/lib/UserMigration/RepositoryImportSource.php +++ b/lib/UserMigration/RepositoryImportSource.php @@ -35,7 +35,13 @@ final class RepositoryImportSource implements IImportSource { // Phase 0: buffer in php://temp (memory up to 8 MiB, then a temp file for this one file). $stream = fopen('php://temp/maxmemory:' . (8 * 1048576), 'w+b'); foreach ($this->repo->readBlobs($entry['b']) as $data) { - fwrite($stream, $data); + for ($done = 0, $len = strlen($data); $done < $len; $done += $n) { + $n = fwrite($stream, $done === 0 ? $data : substr($data, $done)); + if ($n === false || $n === 0) { + fclose($stream); + throw new UserMigrationException('Cannot buffer ' . $path); + } + } } rewind($stream); return $stream; @@ -48,11 +54,13 @@ final class RepositoryImportSource implements IImportSource { foreach (array_keys($this->manifest['entries']) as $p) { if ($prefix === '' || str_starts_with($p, $prefix)) { $rest = substr($p, strlen($prefix)); - if ($rest !== '' && !str_contains($rest, '/')) { - $names[] = $rest; + if ($rest !== '') { + // First path segment: also lists folders that only exist as a parent of a file. + $names[explode('/', $rest, 2)[0]] = true; } } } + $names = array_keys($names); sort($names); return $names; } @@ -72,7 +80,8 @@ final class RepositoryImportSource implements IImportSource { public function copyToFolder(Folder $destination, string $sourcePath): void { foreach ($this->getFolderListing($sourcePath) as $name) { - $path = trim($sourcePath, '/') . '/' . $name; + $base = trim($sourcePath, '/'); + $path = $base === '' ? $name : $base . '/' . $name; $entry = $this->manifest['entries'][$path] ?? ['t' => 'd']; if ($entry['t'] === 'd') { $sub = $destination->nodeExists($name) ? $destination->get($name) : $destination->newFolder($name); diff --git a/spikes/03-db-dump.php b/spikes/03-db-dump.php index 8b00f58..e636790 100644 --- a/spikes/03-db-dump.php +++ b/spikes/03-db-dump.php @@ -33,6 +33,26 @@ printf("database: %s, prefix %s\n", get_class($db->getDatabasePlatform()), $pref $tables = array_values(array_filter($db->createSchemaManager()->listTableNames(), fn ($t) => str_starts_with($t, $prefix))); sort($tables); +// Fixture the damage step relies on, so the check does not depend on existing data. +$db->executeStatement('DELETE FROM ' . $prefix . 'preferences WHERE userid = ? AND appid = ? AND configkey = ?', ['admin', 'core', 'lang']); +$db->insert($prefix . 'preferences', ['userid' => 'admin', 'appid' => 'core', 'configkey' => 'lang', 'configvalue' => 'nl']); +$seqStates = static function () use ($db, $prefix): array { + if (!$db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { + return []; + } + $out = []; + foreach ($db->fetchFirstColumn("SELECT schemaname || '.' || sequencename FROM pg_sequences WHERE sequencename LIKE ? ORDER BY 1", [$prefix . '%']) as $s) { + $r = $db->fetchAssociative('SELECT last_value, is_called FROM ' . $s); + $out[$s] = [(int)$r['last_value'], (bool)$r['is_called']]; + } + return $out; +}; +// Let one sequence run ahead of the data (deleted rows, rollbacks): MAX+1 would be wrong. +$fcSeq = $db->fetchOne("SELECT pg_get_serial_sequence(?, 'fileid')", [$prefix . 'filecache']); +for ($i = 0; $i < 50; $i++) { + $db->fetchOne('SELECT nextval(CAST(? AS regclass))', [$fcSeq]); +} +$seqBefore = $seqStates(); $before = []; foreach ($tables as $t) { $before[$t] = $dumper->checksum($t); @@ -58,7 +78,7 @@ printf("dump 2 (no changes): %s\n", json_encode($stats2)); $check('unchanged database: (almost) nothing new stored', $stats2['uploaded'] < 0.05 * max(1, $stats['uploaded'])); // Damage the database. -$db->executeStatement('DELETE FROM ' . $prefix . 'filecache WHERE path LIKE ?', ['files/Docs/%']); +$db->executeStatement('DELETE FROM ' . $prefix . 'preferences WHERE userid = ? AND appid = ? AND configkey = ?', ['admin', 'core', 'lang']); $db->executeStatement('UPDATE ' . $prefix . 'appconfig SET configvalue = ? WHERE appid = ? AND configkey = ?', ['damaged', 'core', 'installedat']); $db->insert($prefix . 'users', ['uid' => 'intruder', 'displayname' => 'Intruder', 'password' => 'x', 'uid_lower' => 'intruder']); $changed = array_filter($tables, fn ($tb) => $dumper->checksum($tb)['sha256'] !== $before[$tb]['sha256']); @@ -86,27 +106,16 @@ if ($mismatch) { } -// Sequences must continue after the restored maximum (PostgreSQL). -if ($db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { - $serials = $db->fetchAllAssociative("SELECT table_name, column_name, pg_get_serial_sequence(table_name, column_name) AS seq - FROM information_schema.columns WHERE table_schema = current_schema() AND table_name LIKE ? - AND pg_get_serial_sequence(table_name, column_name) IS NOT NULL", [$prefix . '%']); - $bad = []; - foreach ($serials as $s) { - $max = (int)$db->fetchOne(sprintf('SELECT COALESCE(MAX(%s), 0) FROM %s', $db->quoteIdentifier($s['column_name']), $db->quoteIdentifier($s['table_name']))); - $seq = $db->fetchAssociative('SELECT last_value, is_called FROM ' . $s['seq']); - $next = (int)$seq['last_value'] + ($seq['is_called'] ? 1 : 0); - if ($next <= $max) { - $bad[] = "{$s['table_name']}.{$s['column_name']} next=$next max=$max"; - } - } - $check(sprintf('all %d sequences continue above the restored maximum', count($serials)), $bad === []); - if ($bad) { - echo implode("\n", $bad), "\n"; +// Sequences must be restored exactly (PostgreSQL), including one that ran ahead of the data. +$seqAfter = $seqStates(); +$diff = array_filter(array_keys($seqBefore), fn ($k) => ($seqAfter[$k] ?? null) !== $seqBefore[$k]); +$check(sprintf('all %d sequences restored exactly (filecache seq 50 ahead of data)', count($seqBefore)), $diff === [] && count($seqBefore) > 0); +if ($diff) { + foreach (array_slice($diff, 0, 5) as $k) { + echo "$k before=" . json_encode($seqBefore[$k]) . ' after=' . json_encode($seqAfter[$k] ?? null) . "\n"; } } - exec('rm -rf ' . escapeshellarg($repoDir)); echo $ok ? "ALL PASS\n" : "FAILURES\n"; exit($ok ? 0 : 1); diff --git a/spikes/05-user-migration.php b/spikes/05-user-migration.php index 73cff3c..64c36fb 100644 --- a/spikes/05-user-migration.php +++ b/spikes/05-user-migration.php @@ -3,7 +3,7 @@ declare(strict_types=1); // Spike E: per-user backup/restore through user_migration with our own export/import classes. -// php -d memory_limit=128M /var/www/html/custom_apps/ng_backup/spikes/05-user-migration.php +// php -d memory_limit=512M /var/www/html/custom_apps/ng_backup/spikes/05-user-migration.php require '/var/www/html/lib/base.php'; set_exception_handler(null); @@ -32,11 +32,12 @@ $root = \OCP\Server::get(IRootFolder::class); $caldav = \OCP\Server::get(CalDavBackend::class); $carddav = \OCP\Server::get(CardDavBackend::class); $migration = \OCP\Server::get(UserMigrationService::class); -$uid = 'alice'; +$uid = 'ngb-spike-alice'; $principal = 'principals/users/' . $uid; -if ($old = $users->get($uid)) { - $old->delete(); +if ($users->userExists($uid)) { + fwrite(STDERR, "User $uid already exists; refusing to touch an existing account.\n"); + exit(2); } // 1. Test user with files, a calendar event, a contact and profile data. diff --git a/spikes/06-resumable.php b/spikes/06-resumable.php index 94e9d3e..2b6cd9b 100644 --- a/spikes/06-resumable.php +++ b/spikes/06-resumable.php @@ -58,6 +58,17 @@ switch ($mode) { $repo = Repository::open(new LocalBackend("$work/repo"), $pass); $t = microtime(true); $state = BackupRun::step($repo, $state, $t + $seconds); + // Simulate a user overwriting the big file (same size) while the run is in the middle of it. + if (getenv('NGB_MUTATE') === '1' && ($state['cur']['path'] ?? '') === 'huge.bin' && !file_exists("$work/mutated")) { + $fh = fopen("$src/huge.bin", 'r+b'); + fseek($fh, 10 * 1048576); + fwrite($fh, random_bytes(1048576)); + fclose($fh); + touch("$src/huge.bin", time() + 5); + file_put_contents("$work/expected.json", json_encode($hashTree($src))); + touch("$work/mutated"); + echo " -> huge.bin overwritten (same size) between steps\n"; + } // Save the state atomically: a kill before this point means the step is simply redone. file_put_contents("$stateFile.tmp", json_encode($state)); rename("$stateFile.tmp", $stateFile); diff --git a/spikes/RESULTS.md b/spikes/RESULTS.md index 06143a5..36fa049 100644 --- a/spikes/RESULTS.md +++ b/spikes/RESULTS.md @@ -45,3 +45,32 @@ All spikes run on the standard `nextcloud:34-apache` image (PHP 8.5, sodium 1.0. - Real hosted environment (webcron/AJAX cron, `max_execution_time`); spike F simulates it. - Restore of the database while Nextcloud is running (spike C restored with Nextcloud idle). - Schema introspection still uses the Doctrine connection behind `IDBConnection` (private API). + +## Review (Codex + CodeRabbit, 2026-10-01) + +No way was found to reorder or drop secretstream frames, substitute a blob, or decrypt with the +wrong key/associated data without detection. Fixed after the review (all spikes re-run, all pass): + +- Files that change while being read, or between two resumable steps, are detected (size, mtime, + ctime, inode before/after) and read again instead of producing a torn file; spike F now + overwrites the big file mid-run and the restore matches the new content. +- A failed pack/index upload no longer leaves its blobs marked as present. +- PostgreSQL sequences are dumped and restored exactly (not MAX+1); spike C advances one + sequence 50 steps ahead of the data and checks all 115 sequences. +- Text that is not valid UTF-8 is kept byte-exact (tagged base64) instead of being substituted. +- Sequence SQL uses bound parameters; sequence names from the catalog are validated. +- Tables without a primary key are refused above 50,000 rows (bounded memory). +- `StorageBackend` checks `writeStream()`/`fflush()`/`fclose()` results and validates its base path. +- user_migration import: root-level paths, intermediate folders and partial writes handled; + export fails on a read error instead of storing a truncated file. + +Design items for phase 1 (from the review): + +- **Rollback/freshness:** a malicious backend can replay an older valid object at the same path or + hide snapshots. Add a chained, MACed generation catalog (repository id + object type/path in the + associated data) and keep the latest generation id outside the backend (Nextcloud DB + recovery kit). +- **Decrypt output is uncommitted until the FINAL frame:** callers must stage and only publish + after success (restore already writes `*.ngb-restore` and renames; make this a rule for all sinks). +- **Restore mode:** "merge" (current: extra files stay) vs "replace" (target equals snapshot). +- **Size metadata:** object and pack-header sizes reveal approximate sizes/compressibility; + optional padding if that matters.