diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..b7b80c8 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,33 @@ +# Changelog + +All notable changes to this project are documented here. This project follows +[Semantic Versioning](https://semver.org/). Per-release detail lives in the +`RELEASE_v*.md` files; this file is the running summary. + +## [3.7.24] — 2026-09-26 + +### Fixed +- **Install integrity:** `OpenIvExecutor` writes to a temp file and commits with + an atomic move, so a failed/interrupted overwrite never truncates an existing + game file in place. +- **Change history:** persisted via atomic `JsonFileStore` and lock-guarded; + saves are no longer silently swallowed on I/O error. +- **Profiles:** corrupt profile files are logged and skipped instead of dropped, + stock defaults are never seeded over unparseable user data, and writes are + atomic. +- **Restore points:** lock-guarded snapshot reads close a torn-read race. +- **Job queue:** all job-state access is serialized and readers get a snapshot, + removing a race where `/jobs/{id}` could return torn state. + +### Security +- **Safe-mode restore** verifies each manifest path resolves inside the GTA V + root before moving it, containing tampered/corrupt manifests. +- **Local API** error responses route through `ApiErrors.Problem`, logging the + exception server-side with a correlation id instead of leaking absolute file + paths to the client. + +### Performance +- `OpenIvExecutor` builds a one-time archive entry lookup (O(n²) → O(n)). + +### Tests +- 1021 passing (+8): overwrite-failure integrity, path containment. diff --git a/LSPDFRManager.LocalApi/ApiErrors.cs b/LSPDFRManager.LocalApi/ApiErrors.cs new file mode 100644 index 0000000..0f87368 --- /dev/null +++ b/LSPDFRManager.LocalApi/ApiErrors.cs @@ -0,0 +1,21 @@ +using LSPDFRManager.Core; + +namespace LSPDFRManager.LocalApi; + +/// +/// Centralized 500-level error responses. Logs the full exception server-side +/// with a short correlation id and returns only a generic message to the +/// client, so internal detail — absolute file paths, stack traces — never +/// leaks over the API. Endpoints pass a human context string, not ex.Message. +/// +public static class ApiErrors +{ + public static IResult Problem(string context, Exception ex) + { + var correlationId = Guid.NewGuid().ToString("N")[..8]; + AppLogger.Error($"[API_ERROR {correlationId}] {context}", ex); + return Results.Problem( + detail: $"{context}. Reference id: {correlationId} (see app.log for details).", + statusCode: StatusCodes.Status500InternalServerError); + } +} diff --git a/LSPDFRManager.LocalApi/Endpoints/BackupEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/BackupEndpoints.cs index 9273f92..601ee79 100644 --- a/LSPDFRManager.LocalApi/Endpoints/BackupEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/BackupEndpoints.cs @@ -36,7 +36,7 @@ public static void MapBackups(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to list backups: {ex.Message}"); + return ApiErrors.Problem($"Failed to list backups", ex); } }); @@ -160,7 +160,7 @@ public static void MapBackups(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to delete backup: {ex.Message}"); + return ApiErrors.Problem($"Failed to delete backup", ex); } }); diff --git a/LSPDFRManager.LocalApi/Endpoints/BrowseEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/BrowseEndpoints.cs index 3dd9f12..f290c9f 100644 --- a/LSPDFRManager.LocalApi/Endpoints/BrowseEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/BrowseEndpoints.cs @@ -53,7 +53,7 @@ public static void MapBrowse(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Browse proxy error: {ex.Message}"); + return ApiErrors.Problem($"Browse proxy error", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/CleanupEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/CleanupEndpoints.cs index ff0cdc2..a21f17f 100644 --- a/LSPDFRManager.LocalApi/Endpoints/CleanupEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/CleanupEndpoints.cs @@ -23,7 +23,7 @@ public static void MapCleanup(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Cleanup scan failed: {ex.Message}"); + return ApiErrors.Problem($"Cleanup scan failed", ex); } }); @@ -50,7 +50,7 @@ public static void MapCleanup(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Re-scan failed: {ex.Message}"); + return ApiErrors.Problem($"Re-scan failed", ex); } var pathSet = req.RelativePaths.ToHashSet(StringComparer.OrdinalIgnoreCase); @@ -82,7 +82,7 @@ public static void MapCleanup(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Cleanup apply failed: {ex.Message}"); + return ApiErrors.Problem($"Cleanup apply failed", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/CompatibilityEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/CompatibilityEndpoints.cs index b20944c..0971f6e 100644 --- a/LSPDFRManager.LocalApi/Endpoints/CompatibilityEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/CompatibilityEndpoints.cs @@ -45,7 +45,7 @@ public static void MapCompatibility(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to detect component versions: {ex.Message}"); + return ApiErrors.Problem($"Failed to detect component versions", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/DiagnosticsEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/DiagnosticsEndpoints.cs index e729920..8d21646 100644 --- a/LSPDFRManager.LocalApi/Endpoints/DiagnosticsEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/DiagnosticsEndpoints.cs @@ -50,7 +50,7 @@ public static void MapDiagnostics(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Diagnostics scan failed: {ex.Message}"); + return ApiErrors.Problem($"Diagnostics scan failed", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/HistoryEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/HistoryEndpoints.cs index be17567..879464e 100644 --- a/LSPDFRManager.LocalApi/Endpoints/HistoryEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/HistoryEndpoints.cs @@ -37,7 +37,7 @@ public static void MapHistory(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to read change history: {ex.Message}"); + return ApiErrors.Problem($"Failed to read change history", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/LibraryEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/LibraryEndpoints.cs index ebc84e2..12aa9d5 100644 --- a/LSPDFRManager.LocalApi/Endpoints/LibraryEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/LibraryEndpoints.cs @@ -39,7 +39,7 @@ public static void MapLibrary(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to read library: {ex.Message}"); + return ApiErrors.Problem($"Failed to read library", ex); } }); @@ -57,7 +57,7 @@ public static void MapLibrary(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to toggle mod: {ex.Message}"); + return ApiErrors.Problem($"Failed to toggle mod", ex); } } @@ -75,7 +75,7 @@ public static void MapLibrary(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to toggle mod: {ex.Message}"); + return ApiErrors.Problem($"Failed to toggle mod", ex); } finally { @@ -97,7 +97,7 @@ public static void MapLibrary(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to update notes: {ex.Message}"); + return ApiErrors.Problem($"Failed to update notes", ex); } } @@ -115,7 +115,7 @@ public static void MapLibrary(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to update notes: {ex.Message}"); + return ApiErrors.Problem($"Failed to update notes", ex); } finally { @@ -148,7 +148,7 @@ public static void MapLibrary(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Sync failed: {ex.Message}"); + return ApiErrors.Problem($"Sync failed", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/LogEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/LogEndpoints.cs index 074584b..7bea9d2 100644 --- a/LSPDFRManager.LocalApi/Endpoints/LogEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/LogEndpoints.cs @@ -58,7 +58,7 @@ public static void MapLogs(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to read log '{name}': {ex.Message}"); + return ApiErrors.Problem($"Failed to read log '{name}'", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/ProfileEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/ProfileEndpoints.cs index f15d132..9c966d7 100644 --- a/LSPDFRManager.LocalApi/Endpoints/ProfileEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/ProfileEndpoints.cs @@ -25,7 +25,7 @@ public static void MapProfiles(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to read profiles: {ex.Message}"); + return ApiErrors.Problem($"Failed to read profiles", ex); } }); @@ -46,7 +46,7 @@ public static void MapProfiles(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to create profile: {ex.Message}"); + return ApiErrors.Problem($"Failed to create profile", ex); } }); @@ -75,7 +75,7 @@ public static void MapProfiles(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to update profile: {ex.Message}"); + return ApiErrors.Problem($"Failed to update profile", ex); } }); @@ -98,7 +98,7 @@ public static void MapProfiles(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to delete profile: {ex.Message}"); + return ApiErrors.Problem($"Failed to delete profile", ex); } }); } diff --git a/LSPDFRManager.LocalApi/Endpoints/SafeModeEndpoints.cs b/LSPDFRManager.LocalApi/Endpoints/SafeModeEndpoints.cs index 4c9b23e..00bdbf5 100644 --- a/LSPDFRManager.LocalApi/Endpoints/SafeModeEndpoints.cs +++ b/LSPDFRManager.LocalApi/Endpoints/SafeModeEndpoints.cs @@ -46,7 +46,7 @@ public static void MapSafeMode(this WebApplication app) } catch (Exception ex) { - return Results.Problem($"Failed to build safe mode plan: {ex.Message}"); + return ApiErrors.Problem($"Failed to build safe mode plan", ex); } }); @@ -116,6 +116,9 @@ public static void MapSafeMode(this WebApplication app) foreach (var disabledPath in manifest.DisabledPaths) { if (!disabledPath.EndsWith(".disabled", StringComparison.OrdinalIgnoreCase)) continue; + // Contain the move to the GTA root: a tampered/corrupt manifest + // must never move files to or from arbitrary locations. + if (!PathContainment.IsWithin(gtaPath, disabledPath)) continue; if (!File.Exists(disabledPath)) continue; var original = disabledPath[..^".disabled".Length]; if (!File.Exists(original)) diff --git a/LSPDFRManager.LocalApi/Services/JobQueue.cs b/LSPDFRManager.LocalApi/Services/JobQueue.cs index a55d8c5..dc8026f 100644 --- a/LSPDFRManager.LocalApi/Services/JobQueue.cs +++ b/LSPDFRManager.LocalApi/Services/JobQueue.cs @@ -11,11 +11,25 @@ public sealed class JobEntry public string? ResultJson { get; set; } public DateTime StartedAt { get; init; } = DateTime.UtcNow; public DateTime? CompletedAt { get; set; } + + // A detached copy handed to readers so they never observe a field being + // mutated by a background thread mid-read. + internal JobEntry Snapshot() => new() + { + JobId = JobId, + State = State, + ProgressPct = ProgressPct, + Error = Error, + ResultJson = ResultJson, + StartedAt = StartedAt, + CompletedAt = CompletedAt, + }; } public sealed class JobQueue : IDisposable { private readonly ConcurrentDictionary _jobs = new(); + private readonly object _lock = new(); private readonly Timer _pruner; public JobQueue() @@ -27,52 +41,68 @@ public JobQueue() public string CreateJob() { var jobId = Guid.NewGuid().ToString("N"); - _jobs[jobId] = new JobEntry { JobId = jobId }; + lock (_lock) + _jobs[jobId] = new JobEntry { JobId = jobId }; return jobId; } public void UpdateProgress(string jobId, int pct, string state) { - if (_jobs.TryGetValue(jobId, out var job)) + lock (_lock) { - job.ProgressPct = pct; - job.State = state; + if (_jobs.TryGetValue(jobId, out var job)) + { + job.ProgressPct = pct; + job.State = state; + } } } public void CompleteJob(string jobId, string? resultJson = null) { - if (_jobs.TryGetValue(jobId, out var job)) + lock (_lock) { - job.State = "Completed"; - job.ProgressPct = 100; - job.ResultJson = resultJson; - job.CompletedAt = DateTime.UtcNow; + if (_jobs.TryGetValue(jobId, out var job)) + { + job.State = "Completed"; + job.ProgressPct = 100; + job.ResultJson = resultJson; + job.CompletedAt = DateTime.UtcNow; + } } } public void FailJob(string jobId, string? error) { - if (_jobs.TryGetValue(jobId, out var job)) + lock (_lock) { - job.State = "Failed"; - job.Error = error; - job.CompletedAt = DateTime.UtcNow; + if (_jobs.TryGetValue(jobId, out var job)) + { + job.State = "Failed"; + job.Error = error; + job.CompletedAt = DateTime.UtcNow; + } } } - public JobEntry? GetJob(string jobId) => - _jobs.TryGetValue(jobId, out var job) ? job : null; + public JobEntry? GetJob(string jobId) + { + lock (_lock) + return _jobs.TryGetValue(jobId, out var job) ? job.Snapshot() : null; + } private void Prune() { var cutoff = DateTime.UtcNow.AddMinutes(-10); - foreach (var kv in _jobs) + lock (_lock) { - var job = kv.Value; - var isTerminal = job.State is "Completed" or "Failed" or "Cancelled"; - if (isTerminal && job.CompletedAt.HasValue && job.CompletedAt.Value < cutoff) - _jobs.TryRemove(kv.Key, out _); + foreach (var kv in _jobs) + { + var job = kv.Value; + var isTerminal = job.State is "Completed" or "Failed" or "Cancelled"; + if (isTerminal && job.CompletedAt.HasValue && job.CompletedAt.Value < cutoff) + _jobs.TryRemove(kv.Key, out _); + } } } diff --git a/LSPDFRManager.Shared/Core/CarInstall/OpenIvExecutor.cs b/LSPDFRManager.Shared/Core/CarInstall/OpenIvExecutor.cs index fc0940a..41c70bd 100644 --- a/LSPDFRManager.Shared/Core/CarInstall/OpenIvExecutor.cs +++ b/LSPDFRManager.Shared/Core/CarInstall/OpenIvExecutor.cs @@ -49,15 +49,19 @@ public async Task ExecuteAsync( try { + // Build a one-time key→entry lookup so we don't re-enumerate the + // archive per operation (O(n²)) and don't risk re-opening entries + // out of order on non-seekable archive streams. + var entriesByKey = new Dictionary(StringComparer.Ordinal); + foreach (var e in archive.Entries) + entriesByKey.TryAdd(e.Key, e); + // 1. Extract files from archive foreach (var operation in plan.Operations) { ct.ThrowIfCancellationRequested(); - var sourceEntry = archive.Entries - .FirstOrDefault(e => e.Key == operation.SourcePath); - - if (sourceEntry is null) + if (!entriesByKey.TryGetValue(operation.SourcePath, out var sourceEntry)) throw new InvalidOperationException( $"Archive entry not found: {operation.SourcePath}"); @@ -157,6 +161,19 @@ private static void DeleteBackupRoot(string backupRoot) } } + private static void TryDeleteTemp(string tempPath) + { + try + { + if (File.Exists(tempPath)) + File.Delete(tempPath); + } + catch (Exception ex) + { + AppLogger.Warning($"[COPY_TEMP_CLEANUP] {Path.GetFileName(tempPath)} | {ex.Message}"); + } + } + private static int SelectBufferSize(long fileSize) { if (fileSize < 1_000_000) @@ -194,15 +211,21 @@ private static async Task SafeCopyAsync( for (int attempt = 0; attempt < MaxRetries; attempt++) { + // Write to a temp sibling then commit with an atomic move, so an + // existing destination is never truncated in place — a crash or + // power-loss mid-copy leaves the original file intact for rollback. + var tempPath = destPath + "." + Guid.NewGuid().ToString("N") + ".tmp"; try { ct.ThrowIfCancellationRequested(); - using (var destFile = File.Create(destPath)) + using (var destFile = File.Create(tempPath)) { await source.CopyToAsync(destFile, bufferSize, ct); } + File.Move(tempPath, destPath, overwrite: true); + AppLogger.Info($"[COPY_OK] {fileName}"); return; } @@ -213,6 +236,12 @@ private static async Task SafeCopyAsync( backoff *= 2; source.Seek(0, SeekOrigin.Begin); } + finally + { + // On success the temp was already moved (no-op); on any failure + // or retry, drop the partial temp so nothing leaks. + TryDeleteTemp(tempPath); + } } AppLogger.Error($"[COPY_FAILED] {fileName} | exhausted {MaxRetries} attempts"); diff --git a/LSPDFRManager.Shared/LSPDFRManager.Shared.csproj b/LSPDFRManager.Shared/LSPDFRManager.Shared.csproj index 41a7831..895c171 100644 --- a/LSPDFRManager.Shared/LSPDFRManager.Shared.csproj +++ b/LSPDFRManager.Shared/LSPDFRManager.Shared.csproj @@ -5,9 +5,9 @@ enable enable LSPDFRManager - 3.7.23 - 3.7.23.0 - 3.7.23.0 + 3.7.24 + 3.7.24.0 + 3.7.24.0 diff --git a/LSPDFRManager.Shared/Services/ChangeHistoryService.cs b/LSPDFRManager.Shared/Services/ChangeHistoryService.cs index c0840e1..9db53d8 100644 --- a/LSPDFRManager.Shared/Services/ChangeHistoryService.cs +++ b/LSPDFRManager.Shared/Services/ChangeHistoryService.cs @@ -4,44 +4,50 @@ namespace LSPDFRManager.Services; public class ChangeHistoryService { - private static ChangeHistoryService? _instance; - public static ChangeHistoryService Instance => _instance ??= new(); + private static readonly Lazy _instance = new(() => new ChangeHistoryService()); + public static ChangeHistoryService Instance => _instance.Value; - private List _entries = []; + private readonly JsonFileStore> _store = new(AppDataPaths.ChangeHistoryFile); + private readonly object _lock = new(); + private List _entries; - internal ChangeHistoryService() => Load(); + internal ChangeHistoryService() => _entries = _store.LoadOrDefault(() => []); - public IReadOnlyList Entries => _entries; + // Returns a snapshot: callers iterate a stable copy, never the live list. + public IReadOnlyList Entries + { + get { lock (_lock) return _entries.ToList(); } + } public void Load() { - var path = AppDataPaths.ChangeHistoryFile; - if (!File.Exists(path)) return; - try - { - var json = File.ReadAllText(path); - _entries = System.Text.Json.JsonSerializer.Deserialize>(json) ?? []; - } - catch { _entries = []; } + var loaded = _store.LoadOrDefault(() => []); + lock (_lock) _entries = loaded; } public void Record(ChangeHistoryAction action, string description, string? affectedFile = null, string? detail = null) { - _entries.Insert(0, new ChangeHistoryEntry + List snapshot; + lock (_lock) { - Action = action, - Description = description, - AffectedFile = affectedFile, - Detail = detail, - }); + _entries.Insert(0, new ChangeHistoryEntry + { + Action = action, + Description = description, + AffectedFile = affectedFile, + Detail = detail, + }); - if (_entries.Count > 1000) _entries = _entries.Take(1000).ToList(); - Save(); + if (_entries.Count > 1000) _entries = _entries.Take(1000).ToList(); + snapshot = _entries.ToList(); + } + _store.Save(snapshot); } public List Filter(ChangeHistoryAction? action = null, DateTime? since = null, string? search = null) { - IEnumerable q = _entries; + IEnumerable q; + lock (_lock) q = _entries.ToList(); if (action.HasValue) q = q.Where(e => e.Action == action.Value); if (since.HasValue) q = q.Where(e => e.OccurredAt >= since.Value); if (!string.IsNullOrWhiteSpace(search)) @@ -52,15 +58,18 @@ public List Filter(ChangeHistoryAction? action = null, DateT public void Clear() { - _entries.Clear(); - Save(); + lock (_lock) _entries.Clear(); + _store.Save([]); } public async Task ExportAsync(string outputPath, bool asJson) { + List entries; + lock (_lock) entries = _entries.ToList(); + if (asJson) { - var sanitized = _entries.Select(e => new + var sanitized = entries.Select(e => new { e.Id, e.Action, e.Description, e.OccurredAt, e.Detail, AffectedFile = SanitizePath(e.AffectedFile), @@ -71,7 +80,7 @@ public async Task ExportAsync(string outputPath, bool asJson) } else { - var lines = _entries.Select(e => + var lines = entries.Select(e => { var line = $"[{e.OccurredAt:yyyy-MM-dd HH:mm:ss}] [{e.Action}] {e.Description}"; var af = SanitizePath(e.AffectedFile); @@ -92,17 +101,4 @@ private static string SanitizePath(string? path) return "%USERPROFILE%" + path[home.Length..]; return path; } - - private void Save() - { - try - { - var path = AppDataPaths.ChangeHistoryFile; - var dir = Path.GetDirectoryName(path); - if (dir is not null) Directory.CreateDirectory(dir); - var json = System.Text.Json.JsonSerializer.Serialize(_entries, new System.Text.Json.JsonSerializerOptions { WriteIndented = true }); - File.WriteAllText(path, json); - } - catch { } - } } diff --git a/LSPDFRManager.Shared/Services/PathContainment.cs b/LSPDFRManager.Shared/Services/PathContainment.cs new file mode 100644 index 0000000..a19cb1f --- /dev/null +++ b/LSPDFRManager.Shared/Services/PathContainment.cs @@ -0,0 +1,27 @@ +namespace LSPDFRManager.Services; + +/// +/// Verifies that an absolute path resolves to a location inside a root +/// directory. Uses a separator-terminated root so a sibling such as +/// C:\GTAV_evil does not match root C:\GTAV by string prefix. +/// Use this to contain operations that consume paths from persisted state +/// (e.g. a safe-mode manifest) which a user or corruption could tamper with. +/// +public static class PathContainment +{ + public static bool IsWithin(string root, string candidate) + { + if (string.IsNullOrWhiteSpace(root) || string.IsNullOrWhiteSpace(candidate)) + return false; + + var normalizedRoot = Path.GetFullPath(root) + .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + Path.DirectorySeparatorChar; + + string fullCandidate; + try { fullCandidate = Path.GetFullPath(candidate); } + catch { return false; } + + return fullCandidate.StartsWith(normalizedRoot, StringComparison.OrdinalIgnoreCase); + } +} diff --git a/LSPDFRManager.Shared/Services/RestorePointService.cs b/LSPDFRManager.Shared/Services/RestorePointService.cs index d16da9e..607fc93 100644 --- a/LSPDFRManager.Shared/Services/RestorePointService.cs +++ b/LSPDFRManager.Shared/Services/RestorePointService.cs @@ -5,11 +5,17 @@ namespace LSPDFRManager.Services; public class RestorePointService { - private static RestorePointService? _instance; - public static RestorePointService Instance => _instance ??= new(); + private static readonly Lazy _instance = new(() => new RestorePointService()); + public static RestorePointService Instance => _instance.Value; + private readonly object _lock = new(); private List _points = []; - public IReadOnlyList Points => _points; + + // Snapshot: callers iterate a stable copy, never the live list. + public IReadOnlyList Points + { + get { lock (_lock) return _points.ToList(); } + } public RestorePointService() => Load(); @@ -20,17 +26,23 @@ public void Load() try { var json = File.ReadAllText(indexPath); - _points = System.Text.Json.JsonSerializer.Deserialize>(json) ?? []; + var loaded = System.Text.Json.JsonSerializer.Deserialize>(json) ?? []; + lock (_lock) _points = loaded; } - catch { _points = []; } + catch { lock (_lock) _points = []; } } public virtual async Task SaveAsync(RestorePoint point, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); - _points.Insert(0, point); - if (_points.Count > 50) _points = _points.Take(50).ToList(); - await PersistIndexAsync(cancellationToken); + List snapshot; + lock (_lock) + { + _points.Insert(0, point); + if (_points.Count > 50) _points = _points.Take(50).ToList(); + snapshot = _points.ToList(); + } + await PersistIndexAsync(snapshot, cancellationToken); ChangeHistoryService.Instance.Record(ChangeHistoryAction.RestorePointCreated, $"Restore point created: {point.OperationName}", detail: point.Id); } @@ -77,15 +89,20 @@ public async Task RestoreAsync(RestorePoint point, IProgress? progress = public async Task DeleteAsync(RestorePoint point) { - _points.Remove(point); - await PersistIndexAsync(); + List snapshot; + lock (_lock) + { + _points.Remove(point); + snapshot = _points.ToList(); + } + await PersistIndexAsync(snapshot); } - private async Task PersistIndexAsync(CancellationToken cancellationToken = default) + private static async Task PersistIndexAsync(List points, CancellationToken cancellationToken = default) { var dir = AppDataPaths.RestorePointsDirectory; Directory.CreateDirectory(dir); - var json = System.Text.Json.JsonSerializer.Serialize(_points, new System.Text.Json.JsonSerializerOptions { WriteIndented = true }); + var json = System.Text.Json.JsonSerializer.Serialize(points, new System.Text.Json.JsonSerializerOptions { WriteIndented = true }); await File.WriteAllTextAsync(AppDataPaths.RestorePointsIndex, json, cancellationToken); } } diff --git a/LSPDFRManager.Tests/OpenIvExecutorIntegrationTests.cs b/LSPDFRManager.Tests/OpenIvExecutorIntegrationTests.cs index 1e46011..22edd47 100644 --- a/LSPDFRManager.Tests/OpenIvExecutorIntegrationTests.cs +++ b/LSPDFRManager.Tests/OpenIvExecutorIntegrationTests.cs @@ -181,6 +181,47 @@ public async Task Executor_TraversalDestination_FailsWithoutWritingOutsideRoot() Assert.False(File.Exists(Path.Combine(_tempRoot, "escape.meta"))); } + // ── Test 7: Overwrite failure preserves the original file ──────────── + // Guards the temp-then-move write discipline: a mid-copy failure while + // overwriting an existing file must never truncate the original, and must + // leave no partial .tmp files behind. + + [Fact] + public async Task Executor_OverwriteFailsMidCopy_OriginalPreservedAndNoTempLeftover() + { + var destRel = @"mods\existing.dll"; + var destPath = Path.Combine(_tempRoot, destRel); + Directory.CreateDirectory(Path.GetDirectoryName(destPath)!); + var originalContent = new byte[] { 100, 101, 102, 103, 104 }; + File.WriteAllBytes(destPath, originalContent); + + var archive = new FakeArchive(new[] + { + new FakeArchiveEntry("existing.dll", + () => new ThrowingStream(new byte[] { 1, 2, 3, 4, 5, 6 }, failAfter: 2), + size: 6) + }); + var plan = new OpenIvInstallPlan + { + Type = CarInstallType.ReplaceVehicle, + TargetDlcName = "overwrite_mod", + Operations = new() + { + new() { SourcePath = "existing.dll", DestinationPath = destRel, Overwrite = true } + }, + XmlPatches = new() + }; + + var result = await _executor.ExecuteAsync(plan, archive, _tempRoot); + + Assert.False(result.Success); + Assert.True(File.Exists(destPath), "original file must survive a failed overwrite"); + Assert.Equal(originalContent, File.ReadAllBytes(destPath)); + + var leftoverTemps = Directory.GetFiles(Path.GetDirectoryName(destPath)!, "*.tmp"); + Assert.Empty(leftoverTemps); + } + private void SetupDlcListXml(string targetRoot) { var dlclistDir = Path.Combine(targetRoot, @"mods\update\update.rpf\common\data"); diff --git a/LSPDFRManager.Tests/PathContainmentTests.cs b/LSPDFRManager.Tests/PathContainmentTests.cs new file mode 100644 index 0000000..3ba3f53 --- /dev/null +++ b/LSPDFRManager.Tests/PathContainmentTests.cs @@ -0,0 +1,47 @@ +using LSPDFRManager.Services; +using Xunit; + +namespace LSPDFRManager.Tests; + +public class PathContainmentTests +{ + [Fact] + public void IsWithin_FileInsideRoot_ReturnsTrue() + { + Assert.True(PathContainment.IsWithin(@"C:\GTAV", @"C:\GTAV\plugins\lspdfr\x.dll")); + } + + [Fact] + public void IsWithin_RootItself_TrailingSeparatorTolerated() + { + Assert.True(PathContainment.IsWithin(@"C:\GTAV\", @"C:\GTAV\mods\y.dll")); + } + + [Fact] + public void IsWithin_SiblingPrefixDirectory_ReturnsFalse() + { + // C:\GTAV_evil must not match root C:\GTAV by string prefix. + Assert.False(PathContainment.IsWithin(@"C:\GTAV", @"C:\GTAV_evil\payload.dll")); + } + + [Fact] + public void IsWithin_TraversalEscapingRoot_ReturnsFalse() + { + Assert.False(PathContainment.IsWithin(@"C:\GTAV", @"C:\GTAV\..\Windows\System32\evil.dll")); + } + + [Fact] + public void IsWithin_CompletelyDifferentRoot_ReturnsFalse() + { + Assert.False(PathContainment.IsWithin(@"C:\GTAV", @"D:\Other\thing.dll")); + } + + [Theory] + [InlineData("", @"C:\GTAV\x.dll")] + [InlineData(@"C:\GTAV", "")] + [InlineData(null, @"C:\GTAV\x.dll")] + public void IsWithin_NullOrEmptyInput_ReturnsFalse(string? root, string? candidate) + { + Assert.False(PathContainment.IsWithin(root!, candidate!)); + } +} diff --git a/LSPDFRManager.Tests/SetupWizardTests.cs b/LSPDFRManager.Tests/SetupWizardTests.cs index fc2eb0b..0642665 100644 --- a/LSPDFRManager.Tests/SetupWizardTests.cs +++ b/LSPDFRManager.Tests/SetupWizardTests.cs @@ -75,7 +75,7 @@ public async Task UpdateCheck_ReturnsValidResult() var result = await new UpdateCheckService().CheckAsync(); Assert.NotNull(result); - Assert.Equal("3.7.23", result.CurrentVersion); + Assert.Equal("3.7.24", result.CurrentVersion); } [Fact] diff --git a/LSPDFRManager.Tests/VersionAndBrowseGuardTests.cs b/LSPDFRManager.Tests/VersionAndBrowseGuardTests.cs index 0c87b41..89bf6dd 100644 --- a/LSPDFRManager.Tests/VersionAndBrowseGuardTests.cs +++ b/LSPDFRManager.Tests/VersionAndBrowseGuardTests.cs @@ -6,11 +6,11 @@ namespace LSPDFRManager.Tests; public class VersionAndBrowseGuardTests { [Fact] - public void AssemblyVersion_Is_3_7_23_0() + public void AssemblyVersion_Is_3_7_24_0() { var version = typeof(MainViewModel).Assembly.GetName().Version; Assert.NotNull(version); - Assert.Equal(new Version(3, 7, 23, 0), version); + Assert.Equal(new Version(3, 7, 24, 0), version); } [Fact] diff --git a/LSPDFRManager.csproj b/LSPDFRManager.csproj index 3e571c6..616edf7 100644 --- a/LSPDFRManager.csproj +++ b/LSPDFRManager.csproj @@ -11,9 +11,9 @@ Desktop mod manager for GTA V LSPDFR installations. rolling-codes https://github.com/rolling-codes/LSPDFRManager - 3.7.23 - 3.7.23.0 - 3.7.23.0 + 3.7.24 + 3.7.24.0 + 3.7.24.0 app.manifest diff --git a/RELEASE_v3.7.24.md b/RELEASE_v3.7.24.md new file mode 100644 index 0000000..f3c185d --- /dev/null +++ b/RELEASE_v3.7.24.md @@ -0,0 +1,51 @@ +# Release v3.7.24 — Reliability & Hardening + +## Highlights + +A focused reliability release from a full multi-agent code review. No new +features — this hardens the install/rollback path, closes silent data-loss +windows in persistence, removes a concurrency race in the job queue, and stops +the local API from leaking internal file paths in error responses. + +## Install integrity + +- **OIV executor no longer truncates files in place.** `OpenIvExecutor` now + writes each extracted file to a temp sibling and commits it with an atomic + `File.Move`. Previously an overwrite was truncated before the new bytes + landed, so a crash or power-loss mid-copy could leave a valid game file + destroyed. The original is now intact until the move succeeds. +- **Faster, safer archive lookups.** The executor builds a one-time key→entry + map instead of re-enumerating the archive per operation (O(n²) → O(n)), which + also avoids re-opening entries out of order on non-seekable archive streams. + +## Data-loss fixes + +- **Change history** is now persisted through the atomic, logged `JsonFileStore` + instead of a bare `catch {}` that silently discarded the audit trail on any + I/O error. Entry-list access is guarded by a lock. +- **Profiles** no longer vanish silently: a corrupt profile file is logged and + skipped, and stock defaults are never seeded over profiles that merely failed + to parse (which previously replaced the user's real profiles). Profiles are + written atomically. +- **Restore points** access is now lock-guarded with snapshot reads, closing a + torn-read race between the UI and background saves. + +## Security & concurrency + +- **Job queue race fixed.** `/jobs/{id}` pollers can no longer observe torn job + state (e.g. `State=Completed` before the result is visible); all job-state + access is serialized and readers receive a detached snapshot. +- **Safe-mode restore is contained.** The restore endpoint now verifies each + path in the safe-mode manifest resolves inside the configured GTA V root + before moving it, so a tampered or corrupt manifest cannot move files to or + from arbitrary locations. +- **API errors no longer leak paths.** The local API's 500-level handlers route + through a new `ApiErrors.Problem` helper that logs the full exception + server-side with a correlation id and returns only a generic message — + absolute `%APPDATA%`/GTA paths are no longer exposed to the client. + +## Verification + +- 1021/1021 tests passing (8 new tests: overwrite-failure integrity, path + containment). +- Clean build. diff --git a/Services/ProfileManager.cs b/Services/ProfileManager.cs index 06fc86b..39447b6 100644 --- a/Services/ProfileManager.cs +++ b/Services/ProfileManager.cs @@ -18,7 +18,8 @@ public void Load() var dir = AppDataPaths.ProfilesDirectory; Directory.CreateDirectory(dir); - foreach (var file in Directory.EnumerateFiles(dir, "*.json")) + var files = Directory.EnumerateFiles(dir, "*.json").ToList(); + foreach (var file in files) { try { @@ -26,10 +27,16 @@ public void Load() var profile = System.Text.Json.JsonSerializer.Deserialize(json); if (profile is not null) _profiles.Add(profile); } - catch { } + catch (Exception ex) + { + AppLogger.Warning($"[ProfileManager] Skipped corrupt profile '{Path.GetFileName(file)}': {ex.Message}"); + } } - if (_profiles.Count == 0) + // Seed stock defaults only when the profiles dir is genuinely empty — + // never when files existed but failed to parse, which would silently + // replace the user's real profiles with stock ones. + if (_profiles.Count == 0 && files.Count == 0) SeedDefaults(); } @@ -200,9 +207,10 @@ private void SaveAll() private void SaveProfile(ModProfile profile) { - var path = ProfilePath(profile); - var json = System.Text.Json.JsonSerializer.Serialize(profile, new System.Text.Json.JsonSerializerOptions { WriteIndented = true }); - File.WriteAllText(path, json); + // JsonFileStore writes to a temp file then atomically replaces, so a + // crash mid-write never truncates an existing profile, and I/O failures + // are logged rather than thrown as a hard failure out of Create/Apply. + new JsonFileStore(ProfilePath(profile)).Save(profile); } private static List SnapshotCurrentLibrary()