Skip to content

Commit 67cafd8

Browse files
ImPanickclaude
authored andcommitted
fix(recovery): harden backup walker + master backup (audit follow-up)
Adversarial audit of the Phase 2 recovery subsystem surfaced three genuine hardening gaps; fixed here. BackupChainWalker: - Stable selection on mtime ties — added an ordinal path tiebreaker so the chosen (and the prospect "second-newest") backup is deterministic across runs instead of relying on LINQ's unspecified ordering for equal timestamps. - Empty/whitespace-only backups are never treated as a clean restore source (a zero-byte sibling could otherwise be considered before the parse check). RecoveryService.CreateMasterBackup: - Rejects a master-backup directory that lives inside the profile folder (would zip itself) — fails fast with ArgumentException. - Builds the snapshot with a filtered ZipArchive that excludes IUUT's own .iuut-tmp-* and .iuut-recovery-* artifacts (only the save data is captured), and skips a file locked mid-recovery rather than aborting the whole snapshot. - A zip failure now degrades to MasterBackupZipPath=null and recovery still proceeds (per-file SafeSaveWriter backups remain the safety net) instead of crashing. Audit findings deliberately NOT actioned (with reasons): recomputing a prospect blob SHA-1 over corrupt bytes is not recovery (it would let the game load a logically-broken world) — refusing → restore-from-backup is correct; the walker is already blob-aware (the planner passes a prospect predicate that includes the SHA-1 check); the plan→execute TOCTOU is bounded by SafeSaveWriter's post-write re-parse, which rejects any backup that changed to invalid content. Tests (4 new, 173 total, all green): empty backup never chosen; equal-mtime selection is deterministic; master-backup dir inside the profile folder throws; the snapshot zip excludes .iuut-tmp-* but contains the corrupt save. Verified: dotnet build -c Release 0/0, dotnet test 173/173, dotnet format clean, governance-lint clean. Agent: claude-code/2.1.149 Consulted: AGENTS.md, .agent/CONSTITUTION.md#III, .agent/CODE_STYLE.md#3, .agent/TESTING_CONTRACT.md#2,#5, docs/IUUT-PROJECT-DOCUMENTATION.md#12.1,#7.6 Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 1503dd1 commit 67cafd8

4 files changed

Lines changed: 112 additions & 13 deletions

File tree

src/IUUT.Core/Recovery/BackupChainWalker.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ public BackupScanResult Scan(string filePath, Func<string, bool> isClean, bool i
3131
foreach (var path in EnumerateBackups(directory, fileName))
3232
{
3333
var isIuut = Path.GetFileName(path).Contains(BackupManager.BackupInfix, StringComparison.Ordinal);
34-
var clean = TryRead(path, out var content) && IsCleanSafe(isClean, content);
34+
// An empty or whitespace-only backup is never a valid restore source.
35+
var clean = TryRead(path, out var content)
36+
&& !string.IsNullOrWhiteSpace(content)
37+
&& IsCleanSafe(isClean, content);
3538
candidates.Add(new BackupCandidate
3639
{
3740
Path = path,
@@ -58,6 +61,7 @@ private static (BackupCandidate? Chosen, BackupRestoreKind Kind) Choose(
5861
var gameClean = all
5962
.Where(c => !c.IsIuutBackup && c.IsClean)
6063
.OrderByDescending(c => c.LastModifiedUtc)
64+
.ThenBy(c => c.Path, StringComparer.Ordinal) // stable tiebreak when mtimes are equal
6165
.ToList();
6266

6367
if (gameClean.Count > 0)
@@ -70,6 +74,7 @@ private static (BackupCandidate? Chosen, BackupRestoreKind Kind) Choose(
7074
var iuutClean = all
7175
.Where(c => c.IsIuutBackup && c.IsClean)
7276
.OrderByDescending(c => c.LastModifiedUtc)
77+
.ThenBy(c => c.Path, StringComparer.Ordinal) // stable tiebreak when mtimes are equal
7378
.ToList();
7479

7580
return iuutClean.Count > 0

src/IUUT.Core/Recovery/RecoveryService.cs

Lines changed: 54 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -58,22 +58,64 @@ public async Task<RecoveryReport> ExecuteAsync(
5858
};
5959
}
6060

61-
private string CreateMasterBackup(string profileFolder, string destinationDirectory)
61+
private string? CreateMasterBackup(string profileFolder, string destinationDirectory)
6262
{
63-
Directory.CreateDirectory(destinationDirectory);
64-
var folderName = Path.GetFileName(Path.TrimEndingDirectorySeparator(profileFolder));
65-
var stamp = _clock.UtcNow.ToString("yyyyMMdd-HHmmss", CultureInfo.InvariantCulture);
66-
67-
var zipPath = Path.Combine(destinationDirectory, $"{folderName}.iuut-recovery-{stamp}.zip");
68-
var n = 2;
69-
while (File.Exists(zipPath))
63+
// The zip must live OUTSIDE the folder it snapshots, or it would try to include itself.
64+
var folderFull = Path.GetFullPath(profileFolder);
65+
var destFull = Path.GetFullPath(destinationDirectory);
66+
if (destFull == folderFull ||
67+
destFull.StartsWith(folderFull + Path.DirectorySeparatorChar, StringComparison.Ordinal))
7068
{
71-
zipPath = Path.Combine(destinationDirectory, $"{folderName}.iuut-recovery-{stamp}-{n}.zip");
72-
n++;
69+
throw new ArgumentException(
70+
"The master-backup directory must be outside the profile folder.", nameof(destinationDirectory));
7371
}
7472

75-
ZipFile.CreateFromDirectory(profileFolder, zipPath, CompressionLevel.Optimal, includeBaseDirectory: false);
76-
return zipPath;
73+
try
74+
{
75+
Directory.CreateDirectory(destinationDirectory);
76+
var folderName = Path.GetFileName(Path.TrimEndingDirectorySeparator(profileFolder));
77+
var stamp = _clock.UtcNow.ToString("yyyyMMdd-HHmmss", CultureInfo.InvariantCulture);
78+
79+
var zipPath = Path.Combine(destinationDirectory, $"{folderName}.iuut-recovery-{stamp}.zip");
80+
var n = 2;
81+
while (File.Exists(zipPath))
82+
{
83+
zipPath = Path.Combine(destinationDirectory, $"{folderName}.iuut-recovery-{stamp}-{n}.zip");
84+
n++;
85+
}
86+
87+
using var archive = ZipFile.Open(zipPath, ZipArchiveMode.Create);
88+
foreach (var file in Directory.EnumerateFiles(profileFolder, "*", SearchOption.AllDirectories))
89+
{
90+
var name = Path.GetFileName(file);
91+
// Never snapshot IUUT's own transient temp files or recovery zips — only the save data.
92+
if (name.Contains(".iuut-tmp-", StringComparison.Ordinal) ||
93+
name.Contains(".iuut-recovery-", StringComparison.Ordinal))
94+
{
95+
continue;
96+
}
97+
98+
var entryName = Path.GetRelativePath(profileFolder, file).Replace('\\', '/');
99+
try
100+
{
101+
archive.CreateEntryFromFile(file, entryName, CompressionLevel.Optimal);
102+
}
103+
catch (IOException)
104+
{
105+
// A file locked mid-recovery is skipped rather than aborting the whole snapshot.
106+
}
107+
}
108+
109+
return zipPath;
110+
}
111+
catch (IOException)
112+
{
113+
return null; // recovery still proceeds; per-file SafeSaveWriter backups remain the safety net
114+
}
115+
catch (UnauthorizedAccessException)
116+
{
117+
return null;
118+
}
77119
}
78120

79121
private async Task<RecoveryFileResult> ApplyAsync(RecoveryFileAction action, CancellationToken cancellationToken)

tests/IUUT.Core.Tests/Integration/BackupChainWalkerTests.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,5 +120,31 @@ public void Scan_NoCleanCandidate_ReturnsNone()
120120
result.Candidates.Should().ContainSingle();
121121
}
122122

123+
[Fact]
124+
public void Scan_EmptyOrWhitespaceBackup_IsNeverChosen()
125+
{
126+
var file = Write("Profile.json", "{ corrupt", 0);
127+
Write("Profile.json.backup", "", 5); // empty, newest
128+
var valid = Write("Profile.json.backup_1", "{\"a\":1}", 1); // valid, older
129+
130+
var result = _walker.Scan(file, ParsesJson);
131+
132+
result.Chosen!.Path.Should().Be(valid, "a zero-byte backup is not a valid restore source");
133+
}
134+
135+
[Fact]
136+
public void Scan_EqualMtimes_PicksDeterministically()
137+
{
138+
var file = Write("Mounts.json", "{ corrupt", 0);
139+
var a = Write("Mounts.json.backup", "{\"x\":1}", 3);
140+
Write("Mounts.json.backup_1", "{\"x\":2}", 3); // identical mtime
141+
142+
var first = _walker.Scan(file, ParsesJson).Chosen!.Path;
143+
var second = _walker.Scan(file, ParsesJson).Chosen!.Path;
144+
145+
first.Should().Be(second, "selection must be stable across runs");
146+
first.Should().Be(a, "ties break by ordinal path; 'Mounts.json.backup' sorts before '.backup_1'");
147+
}
148+
123149
public void Dispose() => _temp.Dispose();
124150
}

tests/IUUT.Core.Tests/Integration/RecoveryServiceTests.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,5 +104,31 @@ public async Task ExecuteAsync_AllClean_TakesBackup_ButChangesNothing()
104104
report.PartialRecovery.Should().BeFalse();
105105
}
106106

107+
[Fact]
108+
public async Task ExecuteAsync_DestinationInsideProfileFolder_Throws()
109+
{
110+
Write("Profile.json", ProfileSerializer.Serialize(new ProfileModel { UserId = SteamId }));
111+
var plan = _planner.Plan(_profileDir);
112+
113+
var act = async () => await _service.ExecuteAsync(plan, Path.Combine(_profileDir, "backups"));
114+
115+
await act.Should().ThrowAsync<ArgumentException>("the master backup must not live inside the folder it snapshots");
116+
}
117+
118+
[Fact]
119+
public async Task ExecuteAsync_MasterBackup_ExcludesIuutTempArtifacts()
120+
{
121+
Write("Profile.json", "{ broken");
122+
Write("Profile.json.backup", ProfileSerializer.Serialize(new ProfileModel { UserId = SteamId }));
123+
Write("Profile.json.iuut-tmp-deadbeef", "leftover temp content");
124+
125+
var plan = _planner.Plan(_profileDir);
126+
var report = await _service.ExecuteAsync(plan, _backupDir);
127+
128+
using var zip = ZipFile.OpenRead(report.MasterBackupZipPath!);
129+
zip.Entries.Should().NotContain(e => e.FullName.Contains(".iuut-tmp-"), "transient temp files are not part of the snapshot");
130+
zip.Entries.Should().Contain(e => e.FullName == "Profile.json", "the corrupt save itself is captured");
131+
}
132+
107133
public void Dispose() => _temp.Dispose();
108134
}

0 commit comments

Comments
 (0)