fix(siteruntime): reject SetStaticAttribute with malformed list value (no silent poison persist)

2026-06-16 15:59:30 -04:00
parent 7f97780bb3
commit ad6bfc8af9
2 changed files with 115 additions and 1 deletions
@@ -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
        {
@@ -840,4 +840,92 @@ public class InstanceActorTests : TestKit, IDisposable
        Assert.Equal("Good", response.Quality);
        Assert.Equal("Main Pump", response.Value);
    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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<SetStaticAttributeResponse>(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<GetAttributeResponse>(TimeSpan.FromSeconds(5));
+        Assert.True(getResponse.Found);
+        var list = Assert.IsType<List<string>>(getResponse.Value);
+        Assert.Equal(new[] { "a", "b" }, list);
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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<SetStaticAttributeResponse>(TimeSpan.FromSeconds(5));
+        Assert.True(setResponse.Success);
+
+        actor.Tell(new GetAttributeRequest("corr-empty-get", "Pump-EmptySet", "Labels", DateTimeOffset.UtcNow));
+        var getResponse = ExpectMsg<GetAttributeResponse>(TimeSpan.FromSeconds(5));
+        Assert.True(getResponse.Found);
+        var list = Assert.IsType<List<string>>(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"]);
+    }
 }