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
This commit is contained in:
+28
-19
@@ -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);
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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).
|
||||
|
||||
## Code review (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.
|
||||
|
||||
Reference in New Issue
Block a user