From ad6bfc8af928edadec3e63b2653d0441e774870f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 16 Jun 2026 15:59:30 -0400 Subject: [PATCH] fix(siteruntime): reject SetStaticAttribute with malformed list value (no silent poison persist) --- .../Actors/InstanceActor.cs | 28 +++++- .../Actors/InstanceActorTests.cs | 88 +++++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs index 62cdafd0..a2406255 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs @@ -344,7 +344,33 @@ public class InstanceActor : ReceiveActor if (_resolvedAttributeByName.TryGetValue(command.AttributeName, out var resolved) && IsListAttribute(resolved)) { - _attributes[command.AttributeName] = DecodeAttributeValue(resolved, command.Value); + // MV-7: the script path pre-encodes valid canonical JSON via ScopeAccessors, + // but the Inbound API / direct-command path can submit an arbitrary + // command.Value. A non-empty value that fails to decode (malformed JSON, + // bad element, missing element type) is poison: storing it would null the + // in-memory value yet publish "Good" quality and durably persist the bad + // JSON (which then loads as Bad next restart). Reject such writes outright. + // Note: DecodeAttributeValue returns null for BOTH a null/empty input + // (valid — clearing) AND a malformed non-empty input (invalid). Only the + // latter is rejected, hence the explicit IsNullOrWhiteSpace guard. An empty + // list "[]" decodes to a non-null empty List, so it passes through. + var decoded = DecodeAttributeValue(resolved, command.Value); + if (!string.IsNullOrWhiteSpace(command.Value) && decoded == null) + { + _logger.LogWarning( + "SetAttribute rejected — value for List attribute '{Attribute}' on instance '{Instance}' is not a valid list", + command.AttributeName, _instanceUniqueName); + Sender.Tell(new SetStaticAttributeResponse( + command.CorrelationId, + _instanceUniqueName, + command.AttributeName, + false, + $"Invalid list value for attribute '{command.AttributeName}'", + DateTimeOffset.UtcNow)); + return; + } + + _attributes[command.AttributeName] = decoded; } else { diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs index a92ac484..4c18004b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTests.cs @@ -840,4 +840,92 @@ public class InstanceActorTests : TestKit, IDisposable Assert.Equal("Good", response.Quality); Assert.Equal("Main Pump", response.Value); } + + /// + /// MV-7 (review fix): a SetStaticAttribute write whose value fails to decode as + /// a list (e.g. truncated JSON) on a List attribute must be REJECTED — reply + /// Success=false with a clear error and persist NOTHING. The script path always + /// pre-encodes valid JSON, but the Inbound API / direct-command path can submit + /// an arbitrary value, so a malformed value must not silently null the in-memory + /// value, publish "Good" quality, and durably persist a poison override. + /// + [Fact] + public async Task InstanceActor_SetStaticListAttribute_Malformed_Rejected_NotPersisted() + { + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Pump-BadSet", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Labels", Value = "[\"a\",\"b\"]", + DataType = "List", ElementDataType = "String" + } + ] + }; + + var actor = CreateInstanceActor("Pump-BadSet", config); + + // Submit a malformed list value (truncated JSON). + actor.Tell(new SetStaticAttributeCommand( + "corr-bad-set", "Pump-BadSet", "Labels", "[\"a\"", DateTimeOffset.UtcNow)); + var setResponse = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.False(setResponse.Success); + Assert.False(string.IsNullOrWhiteSpace(setResponse.ErrorMessage)); + + // The poison value must NOT have been persisted as a static override. + await Task.Delay(500); + var overrides = await _storage.GetStaticOverridesAsync("Pump-BadSet"); + Assert.Empty(overrides); + + // A subsequent read returns the untouched config default — not the poison value. + actor.Tell(new GetAttributeRequest("corr-bad-get", "Pump-BadSet", "Labels", DateTimeOffset.UtcNow)); + var getResponse = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.True(getResponse.Found); + var list = Assert.IsType>(getResponse.Value); + Assert.Equal(new[] { "a", "b" }, list); + } + + /// + /// MV-7 (review fix): an empty-list value "[]" decodes to a non-null empty list + /// and must be accepted (NOT mistaken for a malformed value, which also decodes + /// to null). This pins the boundary between the "clearing/empty" and "poison" + /// cases that both surface as a null decode result. + /// + [Fact] + public async Task InstanceActor_SetStaticListAttribute_EmptyList_Accepted() + { + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Pump-EmptySet", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Labels", Value = "[\"a\",\"b\"]", + DataType = "List", ElementDataType = "String" + } + ] + }; + + var actor = CreateInstanceActor("Pump-EmptySet", config); + + actor.Tell(new SetStaticAttributeCommand( + "corr-empty-set", "Pump-EmptySet", "Labels", "[]", DateTimeOffset.UtcNow)); + var setResponse = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.True(setResponse.Success); + + actor.Tell(new GetAttributeRequest("corr-empty-get", "Pump-EmptySet", "Labels", DateTimeOffset.UtcNow)); + var getResponse = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.True(getResponse.Found); + var list = Assert.IsType>(getResponse.Value); + Assert.Empty(list); + + // The canonical JSON "[]" is persisted unchanged. + await Task.Delay(500); + var overrides = await _storage.GetStaticOverridesAsync("Pump-EmptySet"); + Assert.Single(overrides); + Assert.Equal("[]", overrides["Labels"]); + } }