From 03631fb552ff1e90693342f818de9b928c273303 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 06:27:45 +0000 Subject: [PATCH] fix: reject malformed saved state without losing the live history [patch] LoadStateAsync let NullReferenceException and ArgumentNullException escape on valid JSON with missing or null fields, and RestoreFromState cleared the stack before validating, so a bad file could wipe the in-memory history. The JSON serializer now validates the shape and throws InvalidOperationException, and RestoreFromState checks commands, save boundaries and position before clearing anything. Fixes #81 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016ToeUpj3nH61YEnatKdb8d --- UndoRedo.Test/SerializationTests.cs | 75 +++++++++++++++++++++ UndoRedo/Services/JsonUndoRedoSerializer.cs | 41 ++++++++++- UndoRedo/Services/UndoRedoService.cs | 15 +++++ 3 files changed, 130 insertions(+), 1 deletion(-) diff --git a/UndoRedo.Test/SerializationTests.cs b/UndoRedo.Test/SerializationTests.cs index 13c2ff3..7d8b1c5 100644 --- a/UndoRedo.Test/SerializationTests.cs +++ b/UndoRedo.Test/SerializationTests.cs @@ -392,6 +392,81 @@ public async Task JsonSerializer_DeserializeCommandHasNoParameterlessConstructor Assert.IsInstanceOfType(ex.InnerException, "The underlying reflection failure should be preserved"); } + [TestMethod] + [DataRow("""{"commands":[{"type":"x","description":"d"}],"currentPosition":0,"saveBoundaries":[],"formatVersion":"json-v1.0"}""", DisplayName = "command without metadata")] + [DataRow("""{"commands":null,"currentPosition":0,"saveBoundaries":[],"formatVersion":"json-v1.0"}""", DisplayName = "null commands")] + [DataRow("""{"commands":[null],"currentPosition":0,"saveBoundaries":[],"formatVersion":"json-v1.0"}""", DisplayName = "null command entry")] + [DataRow("""{"commands":[],"currentPosition":-1,"saveBoundaries":null,"formatVersion":"json-v1.0"}""", DisplayName = "null save boundaries")] + [DataRow("""{"commands":[],"currentPosition":-1,"saveBoundaries":[null],"formatVersion":"json-v1.0"}""", DisplayName = "null save boundary entry")] + [DataRow("""{"commands":[],"currentPosition":-1,"saveBoundaries":[],"formatVersion":null}""", DisplayName = "null format version")] + public async Task UndoRedoService_LoadStateMalformed_ReturnsFalseAndKeepsHistory(string json) + { + // Arrange: a stack with history the user would lose if a bad load cleared it + UndoRedoService stack = CreateService(); + stack.SetSerializer(new JsonUndoRedoSerializer()); + int value = 0; + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 1)); + stack.MarkAsSaved("after B"); + await stack.UndoAsync().ConfigureAwait(false); + + // Act + bool success = await stack.LoadStateAsync(System.Text.Encoding.UTF8.GetBytes(json)).ConfigureAwait(false); + + // Assert + Assert.IsFalse(success, "LoadStateAsync should return false for data it cannot load"); + Assert.AreEqual(2, stack.CommandCount, "A failed load should keep the existing commands"); + Assert.AreEqual(0, stack.CurrentPosition, "A failed load should keep the existing position"); + Assert.HasCount(1, stack.SaveBoundaries, "A failed load should keep the existing save boundaries"); + Assert.AreEqual(1, value); + } + + [TestMethod] + [DataRow(2, DisplayName = "position past the last command")] + [DataRow(-2, DisplayName = "position before the start")] + public void UndoRedoService_RestoreFromStateInvalidPosition_ReturnsFalseAndKeepsHistory(int position) + { + // Arrange + UndoRedoService stack = CreateService(); + stack.Execute(new DelegateCommand("A", () => { }, () => { })); + stack.MarkAsSaved(); + + UndoRedoStackState state = new( + [new DelegateCommand("X", () => { }, () => { }), new DelegateCommand("Y", () => { }, () => { })], + position, + [], + "1.0", + DateTime.UtcNow); + + // Act + bool success = stack.RestoreFromState(state); + + // Assert + Assert.IsFalse(success, "RestoreFromState should reject a position outside the commands"); + Assert.AreEqual(1, stack.CommandCount, "A failed restore should keep the existing commands"); + Assert.AreEqual(0, stack.CurrentPosition, "A failed restore should keep the existing position"); + Assert.HasCount(1, stack.SaveBoundaries, "A failed restore should keep the existing save boundaries"); + } + + [TestMethod] + public void UndoRedoService_RestoreFromStateNullSaveBoundaries_ReturnsFalseAndKeepsHistory() + { + // Arrange + UndoRedoService stack = CreateService(); + stack.Execute(new DelegateCommand("A", () => { }, () => { })); + stack.MarkAsSaved(); + + UndoRedoStackState state = new([], -1, null!, "1.0", DateTime.UtcNow); + + // Act + bool success = stack.RestoreFromState(state); + + // Assert + Assert.IsFalse(success, "RestoreFromState should reject a state with no save boundaries list"); + Assert.AreEqual(1, stack.CommandCount, "A failed restore should keep the existing commands"); + Assert.HasCount(1, stack.SaveBoundaries, "A failed restore should keep the existing save boundaries"); + } + private sealed class ConstructorOnlySerializableCommand(string value) : BaseCommand(ChangeType.Modify, ["test"]), ISerializableCommand { diff --git a/UndoRedo/Services/JsonUndoRedoSerializer.cs b/UndoRedo/Services/JsonUndoRedoSerializer.cs index 7e81c44..8a0d0d4 100644 --- a/UndoRedo/Services/JsonUndoRedoSerializer.cs +++ b/UndoRedo/Services/JsonUndoRedoSerializer.cs @@ -67,11 +67,13 @@ public async Task DeserializeAsync( SerializableStackState serializableState = await JsonSerializer.DeserializeAsync(stream, _options, cancellationToken).ConfigureAwait(false) ?? throw new InvalidOperationException("Failed to deserialize stack state"); - if (!SupportsVersion(serializableState.FormatVersion)) + if (serializableState.FormatVersion is null || !SupportsVersion(serializableState.FormatVersion)) { throw new NotSupportedException($"Unsupported format version: {serializableState.FormatVersion}"); } + ValidateShape(serializableState); + List commands = [.. serializableState.Commands.Select(ConvertFromSerializableCommand)]; return new UndoRedoStackState( commands, @@ -81,6 +83,43 @@ public async Task DeserializeAsync( serializableState.Timestamp); } + /// + /// Rejects data that parsed as JSON but is missing fields the stack needs, such as a truncated or + /// hand-edited file. Throws , which the deserialization + /// contract already covers, so LoadStateAsync reports false instead of letting a + /// NullReferenceException or ArgumentNullException escape. + /// + private static void ValidateShape(SerializableStackState state) + { + if (state.Commands is null) + { + throw new InvalidOperationException("Stack state has no commands list"); + } + + if (state.SaveBoundaries is null) + { + throw new InvalidOperationException("Stack state has no save boundaries list"); + } + + if (state.SaveBoundaries.Any(boundary => boundary is null)) + { + throw new InvalidOperationException("Stack state contains a null save boundary"); + } + + foreach (SerializableCommand? command in state.Commands) + { + if (command is null) + { + throw new InvalidOperationException("Stack state contains a null command"); + } + + if (command.Metadata is null) + { + throw new InvalidOperationException($"Command '{command.Description}' has no metadata"); + } + } + } + private static SerializableCommand ConvertToSerializableCommand(ICommand command) { return new SerializableCommand diff --git a/UndoRedo/Services/UndoRedoService.cs b/UndoRedo/Services/UndoRedoService.cs index 6f7ed9b..4461004 100644 --- a/UndoRedo/Services/UndoRedoService.cs +++ b/UndoRedo/Services/UndoRedoService.cs @@ -346,6 +346,13 @@ public bool RestoreFromState(UndoRedoStackState state) { Ensure.NotNull(state); + // Validate everything before clearing, so a state that cannot be loaded leaves the current + // history untouched instead of wiping it and then failing partway through. + if (!IsRestorable(state)) + { + return false; + } + try { _stackManager.Clear(); @@ -379,4 +386,12 @@ public bool RestoreFromState(UndoRedoStackState state) return false; } } + + private static bool IsRestorable(UndoRedoStackState state) => + state.Commands is not null && + state.SaveBoundaries is not null && + !state.Commands.Any(command => command is null) && + !state.SaveBoundaries.Any(boundary => boundary is null) && + state.CurrentPosition >= -1 && + state.CurrentPosition < state.Commands.Count; }