From 56070e7a2bc76f6e8c866a92c627c49a75e6771d Mon Sep 17 00:00:00 2001 From: Copilot Date: Tue, 23 Jun 2026 11:16:57 -0700 Subject: [PATCH 1/2] Make A2UI dataModelUpdate.valueArray first-class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v0.8 `dataModelUpdate.valueArray` typed value was silently dropped by the WinUI parser: `DataModelEntry` had no `ValueArray` field and `ToJsonNode()` returned null for it. Only `valueString/Number/Boolean/valueMap` were handled. This broke seeding an array into a surface's data model (e.g. a multi-select `MultipleChoice` bound to a path), even though `valueArray` is part of the v0.8 protocol (docs/a2ui/protocol.md §2.2 and data-and-actions.md). Changes: - Parser (A2UIProtocol.cs): add `DataModelEntry.ValueArray`; `ToJsonNode()` emits a `JsonArray`; new `ParseValueArray`/`ParseArrayElement` handle value-typed object elements, bare primitives (`["a",1,true]`), nested maps/arrays, and preserve JSON null as a stable index slot. - DoS guard (DataModelStore.cs): the 32-deep depth bound now recurses `valueArray` as well as `valueMap`. - Security (SecretRedactor.cs): the array branch now redacts registered/ denylisted element paths instead of only recursing, so a secret seeded into an array (e.g. an obscured field bound to /codes/0) no longer leaks via canvas.a2ui.dump. - Docs (SKILL.md): correct the stale "arrays are not first-class" note. Tests: parser coverage for valueArray (strings, mixed scalars, maps, nested arrays, bare primitives, empty, null slots) plus valueMap/scalar regressions; secret-redaction regressions for secrets inside arrays; store-level base-path landing and depth-guard rejection; and an end-to-end MultipleChoice surface seeded via valueArray that asserts both the snapshot and the rendered preselection. Validation: build.ps1 (all projects), Shared.Tests, Tray.Tests, and A2UI UITests all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../A2UI/DataModel/DataModelStore.cs | 12 +- .../A2UI/Protocol/A2UIProtocol.cs | 75 +++++++- .../A2UI/Rendering/SecretRedactor.cs | 15 +- src/skills/windows-a2ui/SKILL.md | 6 +- .../A2UIDataModelArrayTests.cs | 182 ++++++++++++++++++ .../SecretRedactorTests.cs | 42 ++++ tests/OpenClaw.Tray.UITests/A2UI.cs | 24 +++ .../A2UIControlMatrixTests.cs | 56 ++++++ .../A2UIDataModelStoreTests.cs | 95 +++++++++ 9 files changed, 498 insertions(+), 9 deletions(-) create mode 100644 tests/OpenClaw.Tray.Tests/A2UIDataModelArrayTests.cs create mode 100644 tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs diff --git a/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs b/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs index 3b091fa2f..4e127f016 100644 --- a/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs +++ b/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs @@ -105,6 +105,7 @@ public void ApplyDataModelUpdate(string surfaceId, string? basePath, IReadOnlyLi if (entry.Key.Length > MaxKeyLength) continue; if (entry.ValueString != null && entry.ValueString.Length > MaxStringValueLength) continue; if (!IsWithinDepth(entry.ValueMap, depth: 1, max: MaxValueMapDepth)) continue; + if (!IsWithinDepth(entry.ValueArray, depth: 1, max: MaxValueMapDepth)) continue; try { @@ -130,12 +131,17 @@ public void ApplyDataModelUpdate(string surfaceId, string? basePath, IReadOnlyLi new DataModelObservable(model, _dispatcher).NotifyPaths(changed); } - private static bool IsWithinDepth(IReadOnlyList? map, int depth, int max) + private static bool IsWithinDepth(IReadOnlyList? children, int depth, int max) { - if (map == null) return true; + if (children == null) return true; if (depth > max) return false; - foreach (var e in map) + foreach (var e in children) + { + // Both nested maps and nested arrays add a level — bound either path + // so a deeply-nested valueArray can't bypass the valueMap depth cap. if (!IsWithinDepth(e.ValueMap, depth + 1, max)) return false; + if (!IsWithinDepth(e.ValueArray, depth + 1, max)) return false; + } return true; } diff --git a/src/OpenClaw.Tray.WinUI/A2UI/Protocol/A2UIProtocol.cs b/src/OpenClaw.Tray.WinUI/A2UI/Protocol/A2UIProtocol.cs index d4af4d385..f2812ed17 100644 --- a/src/OpenClaw.Tray.WinUI/A2UI/Protocol/A2UIProtocol.cs +++ b/src/OpenClaw.Tray.WinUI/A2UI/Protocol/A2UIProtocol.cs @@ -73,9 +73,9 @@ public sealed record A2UIComponentDef } /// -/// Single dataModelUpdate.contents entry. Exactly one of Value*/ValueMap is -/// expected on the wire; we expose them all and let the store apply whichever -/// is set. +/// Single dataModelUpdate.contents entry. Exactly one of +/// Value*/ValueMap/ValueArray is expected on the wire; we expose them all and +/// let the store apply whichever is set. /// public sealed record DataModelEntry { @@ -85,6 +85,12 @@ public sealed record DataModelEntry public bool? ValueBoolean { get; init; } /// An adjacency-list map: each item is itself a DataModelEntry. public IReadOnlyList? ValueMap { get; init; } + /// + /// An ordered array (v0.8 valueArray): each item is a value-typed + /// whose is ignored. Items + /// may themselves be scalars, maps, or nested arrays. + /// + public IReadOnlyList? ValueArray { get; init; } /// Convert this entry's value to a JsonNode for storage. public JsonNode? ToJsonNode() @@ -99,6 +105,13 @@ public sealed record DataModelEntry obj[entry.Key] = entry.ToJsonNode(); return obj; } + if (ValueArray != null) + { + var arr = new JsonArray(); + foreach (var entry in ValueArray) + arr.Add(entry.ToJsonNode()); + return arr; + } return null; } } @@ -350,6 +363,7 @@ private static string Truncate(string s, int max) => ValueNumber = e["valueNumber"] is JsonValue jvn && jvn.TryGetValue(out var n) ? n : null, ValueBoolean = e["valueBoolean"] is JsonValue jvb && jvb.TryGetValue(out var b) ? b : null, ValueMap = ParseValueMap(e["valueMap"] as JsonArray), + ValueArray = ParseValueArray(e["valueArray"] as JsonArray), }; return entry; } @@ -367,6 +381,61 @@ private static string Truncate(string s, int max) => return list; } + /// + /// Parse a v0.8 valueArray. Each element is a value-typed object + /// with no key (e.g. { "valueString": "admin" }). For robustness we + /// also tolerate bare primitives (["a", 1, true]) that an agent may + /// emit. A JSON null element is preserved as an explicit null slot so + /// array indices stay stable for position-sensitive consumers — matching how + /// a value-less wrapped object ({}) round-trips. Elements of an + /// unsupported kind are dropped, matching 's + /// skip-bad-item tolerance. + /// + private static IReadOnlyList? ParseValueArray(JsonArray? arr) + { + if (arr == null) return null; + var list = new List(arr.Count); + foreach (var item in arr) + { + // A JSON null element surfaces as a C# null inside the JsonArray. + // Preserve it as a value-less entry (ToJsonNode → null) rather than + // dropping it, so [a, null, b] stays length 3. + if (item is null) { list.Add(NullArrayElement); continue; } + var element = ParseArrayElement(item); + if (element != null) list.Add(element); + } + return list; + } + + /// Shared value-less entry representing a JSON null array slot. + private static readonly DataModelEntry NullArrayElement = new() { Key = string.Empty }; + + private static DataModelEntry? ParseArrayElement(JsonNode? item) + { + if (item is JsonObject e) + { + return new DataModelEntry + { + Key = string.Empty, + ValueString = OptionalString(e, "valueString"), + ValueNumber = e["valueNumber"] is JsonValue jvn && jvn.TryGetValue(out var n) ? n : null, + ValueBoolean = e["valueBoolean"] is JsonValue jvb && jvb.TryGetValue(out var b) ? b : null, + ValueMap = ParseValueMap(e["valueMap"] as JsonArray), + ValueArray = ParseValueArray(e["valueArray"] as JsonArray), + }; + } + // Bare primitive tolerance. Check string first (a JSON number/bool does + // not satisfy TryGetValue), then bool (a JSON number does not + // satisfy TryGetValue), then number. + if (item is JsonValue v) + { + if (v.TryGetValue(out var s)) return new DataModelEntry { Key = string.Empty, ValueString = s }; + if (v.TryGetValue(out var bl)) return new DataModelEntry { Key = string.Empty, ValueBoolean = bl }; + if (v.TryGetValue(out var d)) return new DataModelEntry { Key = string.Empty, ValueNumber = d }; + } + return null; + } + private static string RequireString(JsonObject o, string key) { if (o[key] is JsonValue jv && jv.TryGetValue(out var s) && !string.IsNullOrEmpty(s)) diff --git a/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs b/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs index aafa3aa7d..9e3418d82 100644 --- a/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs +++ b/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs @@ -107,8 +107,19 @@ public static bool IsSecret(string? path, IReadOnlySet registered) { var childPath = path == "/" ? "/" + i : path + "/" + i; var current = arr[i]; - var replaced = RedactNode(current, childPath, registered); - if (!ReferenceEquals(replaced, current)) arr[i] = replaced; + // Mirror the object branch: a registered/denylisted element path + // (e.g. an obscured TextField bound to "/codes/0") must be + // redacted, not just recursed into — a scalar element would + // otherwise pass through unchanged and leak via canvas.a2ui.dump. + if (IsSecret(childPath, registered)) + { + arr[i] = JsonValue.Create("[REDACTED]"); + } + else + { + var replaced = RedactNode(current, childPath, registered); + if (!ReferenceEquals(replaced, current)) arr[i] = replaced; + } } return arr; } diff --git a/src/skills/windows-a2ui/SKILL.md b/src/skills/windows-a2ui/SKILL.md index f8d719e4f..1d60a50cb 100644 --- a/src/skills/windows-a2ui/SKILL.md +++ b/src/skills/windows-a2ui/SKILL.md @@ -71,9 +71,13 @@ A `path` is a JSON Pointer into the surface's data model. The renderer subscribe { "key": "first", "valueString": "Alice" }, { "key": "last", "valueString": "Smith" } ] } +{ "key": "tags", "valueArray": [ + { "valueString": "admin" }, + { "valueString": "beta" } +] } ``` -Exactly one of `valueString` / `valueNumber` / `valueBoolean` / `valueMap` should be set per entry. `valueMap` is a recursive adjacency list (each item is itself an entry). Arrays of strings are not first-class on the data-model side — store them as a `path` consumed by `MultipleChoice.selections` or by a literalArray default. +Exactly one of `valueString` / `valueNumber` / `valueBoolean` / `valueMap` / `valueArray` should be set per entry. `valueMap` is a recursive adjacency list (each item is itself an entry). `valueArray` is an ordered list whose items are value-typed entries with no `key` (they may be scalars, maps, or nested arrays) — this is the first-class way to seed an array the renderer consumes, e.g. `MultipleChoice.selections`. Bare-primitive arrays (`["a","b"]`) are also tolerated. ## Components diff --git a/tests/OpenClaw.Tray.Tests/A2UIDataModelArrayTests.cs b/tests/OpenClaw.Tray.Tests/A2UIDataModelArrayTests.cs new file mode 100644 index 000000000..45771befc --- /dev/null +++ b/tests/OpenClaw.Tray.Tests/A2UIDataModelArrayTests.cs @@ -0,0 +1,182 @@ +using System.Linq; +using System.Text.Json.Nodes; +using OpenClawTray.A2UI.Protocol; +using Xunit; + +namespace OpenClaw.Tray.Tests; + +/// +/// Conformance tests for the v0.8 dataModelUpdate.valueArray typed value. +/// +/// The protocol (docs/a2ui/protocol.md §2.2 and data-and-actions.md) lists +/// valueArray alongside valueString/Number/Boolean/valueMap as the +/// unambiguous way to seed an array into the data model — e.g. for +/// MultipleChoice.selections. These tests pin that the parser produces a +/// real (it previously dropped the value entirely). +/// +public sealed class A2UIDataModelArrayTests +{ + /// Parse a single dataModelUpdate line and return the entry with the given key. + private static DataModelEntry EntryFor(string jsonl, string key) + { + var msg = Assert.IsType(A2UIMessageParser.ParseLine(jsonl)); + return msg.Contents.Single(c => c.Key == key); + } + + [Fact] + public void ValueArray_OfStrings_ParsesToJsonArray() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"tags","valueArray":[{"valueString":"admin"},{"valueString":"beta"}]}]}}"""; + + var node = EntryFor(jsonl, "tags").ToJsonNode(); + + var arr = Assert.IsType(node); + Assert.Equal(new[] { "admin", "beta" }, arr.Select(n => n!.GetValue())); + } + + [Fact] + public void ValueArray_OfMixedScalars_PreservesTypes() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"mixed","valueArray":[{"valueString":"x"},{"valueNumber":7},{"valueBoolean":true}]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "mixed").ToJsonNode()); + + Assert.Equal(3, arr.Count); + Assert.Equal("x", arr[0]!.GetValue()); + Assert.Equal(7, arr[1]!.GetValue()); + Assert.True(arr[2]!.GetValue()); + } + + [Fact] + public void ValueArray_OfMaps_ProducesArrayOfObjects() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"rows","valueArray":[""" + + """{"valueMap":[{"key":"id","valueNumber":1},{"key":"name","valueString":"Ada"}]},""" + + """{"valueMap":[{"key":"id","valueNumber":2},{"key":"name","valueString":"Bob"}]}""" + + """]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "rows").ToJsonNode()); + + Assert.Equal(2, arr.Count); + var first = Assert.IsType(arr[0]); + Assert.Equal(1, first["id"]!.GetValue()); + Assert.Equal("Ada", first["name"]!.GetValue()); + var second = Assert.IsType(arr[1]); + Assert.Equal("Bob", second["name"]!.GetValue()); + } + + [Fact] + public void ValueArray_Nested_ProducesArrayOfArrays() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"grid","valueArray":[""" + + """{"valueArray":[{"valueNumber":1},{"valueNumber":2}]},""" + + """{"valueArray":[{"valueNumber":3}]}""" + + """]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "grid").ToJsonNode()); + + var inner0 = Assert.IsType(arr[0]); + Assert.Equal(new double[] { 1, 2 }, inner0.Select(n => n!.GetValue())); + var inner1 = Assert.IsType(arr[1]); + Assert.Equal(new double[] { 3 }, inner1.Select(n => n!.GetValue())); + } + + [Fact] + public void ValueArray_BarePrimitiveElements_AreTolerated() + { + // An agent may emit a JSON array of bare primitives rather than the + // wrapped { "valueString": ... } shape. We round-trip those too. + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"vals","valueArray":["a",2,false]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "vals").ToJsonNode()); + + Assert.Equal(3, arr.Count); + Assert.Equal("a", arr[0]!.GetValue()); + Assert.Equal(2, arr[1]!.GetValue()); + Assert.False(arr[2]!.GetValue()); + } + + [Fact] + public void ValueArray_Empty_ProducesEmptyArray() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"empty","valueArray":[]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "empty").ToJsonNode()); + Assert.Empty(arr); + } + + [Fact] + public void ValueArray_SelectionsShape_MatchesMultipleChoiceMultiRead() + { + // MultipleChoice (multi) reads selections as a JsonArray of strings; an + // agent seeds the initial selection through valueArray. Pin the exact + // shape MultipleChoice.ResolveMulti expects. + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","path":"/form","contents":[{"key":"picked","valueArray":[{"valueString":"red"},{"valueString":"blue"}]}]}}"""; + + var msg = Assert.IsType(A2UIMessageParser.ParseLine(jsonl)); + Assert.Equal("/form", msg.Path); + var arr = Assert.IsType(msg.Contents.Single().ToJsonNode()); + Assert.Equal(new[] { "red", "blue" }, arr.Select(n => n!.GetValue())); + } + + [Fact] + public void ValueArray_NullElement_PreservedAsNullSlot() + { + // A JSON null element keeps its position so index-sensitive consumers + // don't see a shifted array. + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"vals","valueArray":[{"valueString":"a"},null,{"valueString":"b"}]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "vals").ToJsonNode()); + + Assert.Equal(3, arr.Count); + Assert.Equal("a", arr[0]!.GetValue()); + Assert.Null(arr[1]); + Assert.Equal("b", arr[2]!.GetValue()); + } + + [Fact] + public void ValueArray_ValuelessObjectElement_PreservedAsNullSlot() + { + // A wrapped element carrying no recognized value also yields a null slot + // — consistent with the bare-null case above. + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"vals","valueArray":[{},{"valueString":"b"}]}]}}"""; + + var arr = Assert.IsType(EntryFor(jsonl, "vals").ToJsonNode()); + + Assert.Equal(2, arr.Count); + Assert.Null(arr[0]); + Assert.Equal("b", arr[1]!.GetValue()); + } + + [Fact] + public void ValueMap_StillParses_NoRegression() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"user","valueMap":[{"key":"first","valueString":"Ada"},{"key":"last","valueString":"Lovelace"}]}]}}"""; + + var obj = Assert.IsType(EntryFor(jsonl, "user").ToJsonNode()); + Assert.Equal("Ada", obj["first"]!.GetValue()); + Assert.Equal("Lovelace", obj["last"]!.GetValue()); + } + + [Fact] + public void ScalarEntries_StillParse_NoRegression() + { + const string jsonl = + """{"dataModelUpdate":{"surfaceId":"s","contents":[{"key":"name","valueString":"Ada"},{"key":"age","valueNumber":36},{"key":"active","valueBoolean":true}]}}"""; + + var msg = Assert.IsType(A2UIMessageParser.ParseLine(jsonl)); + Assert.Equal("Ada", msg.Contents.Single(c => c.Key == "name").ToJsonNode()!.GetValue()); + Assert.Equal(36, msg.Contents.Single(c => c.Key == "age").ToJsonNode()!.GetValue()); + Assert.True(msg.Contents.Single(c => c.Key == "active").ToJsonNode()!.GetValue()); + } +} diff --git a/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs b/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs index 944bd7555..f49bbba91 100644 --- a/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs +++ b/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs @@ -94,4 +94,46 @@ public void IsSecret_RootRegistered_DoesNotMatchEverything() Assert.False(SecretRedactor.IsSecret("/profile/name", registered)); Assert.True(SecretRedactor.IsSecret("/profile/password", registered)); } + + [Fact] + public void Redact_RegisteredSecretInsideArray_IsRedacted() + { + // An obscured TextField bound to "/codes/0" registers that element path. + // After a valueArray seeds "/codes" as ["1234","5678"], the snapshot/dump + // path must redact the registered element — arrays previously only + // recursed (leaving scalar elements untouched). + var registered = new HashSet { "/codes/0" }; + var root = JsonNode.Parse("""{ "codes": ["1234", "5678"] }""")!; + + var redacted = SecretRedactor.Redact(root, registered)!; + + var arr = Assert.IsType(redacted["codes"]); + Assert.Equal("[REDACTED]", (string?)arr[0]); + Assert.Equal("5678", (string?)arr[1]); + } + + [Fact] + public void Redact_RegisteredArrayParent_RedactsWholeArray() + { + // Registering the array's parent path redacts the entire array value. + var registered = new HashSet { "/codes" }; + var root = JsonNode.Parse("""{ "codes": ["1234", "5678"] }""")!; + + var redacted = SecretRedactor.Redact(root, registered)!; + + Assert.Equal("[REDACTED]", (string?)redacted["codes"]); + } + + [Fact] + public void Redact_DenylistedScalarInsideArray_IsRedacted() + { + // A denylisted segment on an array-element path is caught too. + var root = JsonNode.Parse("""{ "items": [ { "token": "abc" }, { "name": "ok" } ] }""")!; + + var redacted = SecretRedactor.Redact(root, NoRegistered)!; + + var arr = Assert.IsType(redacted["items"]); + Assert.Equal("[REDACTED]", (string?)arr[0]!["token"]); + Assert.Equal("ok", (string?)arr[1]!["name"]); + } } diff --git a/tests/OpenClaw.Tray.UITests/A2UI.cs b/tests/OpenClaw.Tray.UITests/A2UI.cs index abe2aa0b7..323961f9e 100644 --- a/tests/OpenClaw.Tray.UITests/A2UI.cs +++ b/tests/OpenClaw.Tray.UITests/A2UI.cs @@ -127,6 +127,30 @@ public static string DataUpdate(string surfaceId, params (string key, JsonNode? }.ToJsonString(); } + /// + /// Build a dataModelUpdate line that writes a single v0.8 valueArray + /// of strings at . Mirrors how an agent seeds an + /// array-shaped value (e.g. MultipleChoice.selections) into the data + /// model — the array channel that was previously dropped on the wire. + /// + public static string DataUpdateStringArray(string surfaceId, string key, params string[] values) + { + var arr = new JsonArray(); + foreach (var v in values) arr.Add(new JsonObject { ["valueString"] = v }); + var contents = new JsonArray + { + new JsonObject { ["key"] = key, ["valueArray"] = arr }, + }; + return new JsonObject + { + ["dataModelUpdate"] = new JsonObject + { + ["surfaceId"] = surfaceId, + ["contents"] = contents, + }, + }.ToJsonString(); + } + /// /// Theme-styles object suitable for the styles argument of /// . Mirrors what diff --git a/tests/OpenClaw.Tray.UITests/A2UIControlMatrixTests.cs b/tests/OpenClaw.Tray.UITests/A2UIControlMatrixTests.cs index 1588b516e..ecdc9185b 100644 --- a/tests/OpenClaw.Tray.UITests/A2UIControlMatrixTests.cs +++ b/tests/OpenClaw.Tray.UITests/A2UIControlMatrixTests.cs @@ -491,6 +491,62 @@ public Task MultipleChoice_Multi_RendersListView_WithMultipleSelectionMode() => Assert.Equal(4, lv.Items.Count); }); + /// + /// End-to-end proof that a v0.8 dataModelUpdate.valueArray flows + /// through parser → DataModelStore → renderer. A multi-select + /// MultipleChoice bound to /picked is seeded with ["g","b"] + /// via valueArray; the rendered ListView must preselect those two + /// options, and the surface snapshot must carry the array. Before the fix + /// the parser dropped the value and nothing was selected. + /// + [Fact] + public async Task MultipleChoice_Multi_ValueArraySeed_PreselectsAndSnapshots() + { + var surface = Surface("s", "mc", new[] + { + Component("mc", "MultipleChoice", new() + { + ["maxAllowedSelections"] = 3, + ["selections"] = Path("/picked"), + ["options"] = new System.Text.Json.Nodes.JsonArray + { + Option("Red", "r"), + Option("Green", "g"), + Option("Blue", "b"), + }, + }), + }); + // Insert the valueArray seed between surfaceUpdate and beginRendering so + // the initial render reads it (the agent's typical "seed then render"). + var nl = surface.IndexOf('\n'); + var jsonl = surface.Substring(0, nl) + "\n" + + DataUpdateStringArray("s", "picked", "g", "b") + + surface.Substring(nl); + + await _ui.PauseAsync("MultipleChoice multi ← valueArray seed"); + await _ui.ResetContainerAsync(); + await _ui.RunOnUIAsync(() => + { + var harness = BuildHarness(_ui); + harness.Router.Push(jsonl); + Assert.NotNull(harness.LastSurface); + + // Parser → store → snapshot round-trip: the array landed in the model. + var snapshot = harness.LastSurface!.GetSnapshot(); + var picked = Assert.IsType(snapshot["dataModel"]!["picked"]); + Assert.Equal(new[] { "g", "b" }, picked.Select(n => n!.GetValue())); + + // Renderer read it: Green + Blue are preselected. + var lv = FindLogical(harness.LastSurface.RootElement).Single(); + var selected = lv.SelectedItems.OfType() + .Select(i => i.Tag as string) + .OrderBy(s => s, System.StringComparer.Ordinal) + .ToArray(); + Assert.Equal(new[] { "b", "g" }, selected); + }); + await _ui.PauseAsync(); + } + [Fact] public Task Slider_LiteralValue_AppliesRangeAndValue() => RunAsync( "Slider literal value", diff --git a/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs b/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs new file mode 100644 index 000000000..871ec9ca4 --- /dev/null +++ b/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs @@ -0,0 +1,95 @@ +using System.Linq; +using System.Text.Json.Nodes; +using System.Threading.Tasks; +using OpenClawTray.A2UI.Protocol; +using Xunit; +using static OpenClaw.Tray.UITests.TestSupport; + +namespace OpenClaw.Tray.UITests; + +/// +/// Store-level coverage for the v0.8 valueArray data-model value driving +/// the production . +/// +/// These run on the UI thread because the store is dispatcher-affine. The parser +/// happy paths live in OpenClaw.Tray.Tests/A2UIDataModelArrayTests; here we pin +/// the bits that only the store enforces: array landing at a (base) path, and +/// the depth guard that bounds nested arrays the same way it bounds nested maps. +/// The depth guard is exercised by constructing +/// objects directly — JSONL would hit System.Text.Json's MaxDepth first. +/// +[Collection(UICollection.Name)] +public sealed class A2UIDataModelStoreTests +{ + private readonly UIThreadFixture _ui; + public A2UIDataModelStoreTests(UIThreadFixture ui) => _ui = ui; + + [Fact] + public async Task ApplyDataModelUpdate_ValueArray_LandsAtBasePath() + { + await _ui.RunOnUIAsync(() => + { + var harness = BuildHarness(_ui); + var entry = new DataModelEntry + { + Key = "picked", + ValueArray = new[] + { + new DataModelEntry { Key = string.Empty, ValueString = "g" }, + new DataModelEntry { Key = string.Empty, ValueString = "b" }, + }, + }; + harness.DataModel.ApplyDataModelUpdate("s", "/form", new[] { entry }); + + var node = harness.DataModel.Read("s", "/form/picked"); + var arr = Assert.IsType(node); + Assert.Equal(new[] { "g", "b" }, arr.Select(n => n!.GetValue())); + }); + } + + [Fact] + public async Task ApplyDataModelUpdate_OverDeepValueArray_IsDroppedByDepthGuard() + { + await _ui.RunOnUIAsync(() => + { + var harness = BuildHarness(_ui); + + // Shallow array survives; a >32-deep nest is rejected by the same + // guard that bounds valueMap, so the path stays unset. + harness.DataModel.ApplyDataModelUpdate("s", null, new[] { DeepArrayEntry("shallow", 4) }); + harness.DataModel.ApplyDataModelUpdate("s", null, new[] { DeepArrayEntry("tooDeep", 40) }); + + Assert.IsType(harness.DataModel.Read("s", "/shallow")); + Assert.Null(harness.DataModel.Read("s", "/tooDeep")); + }); + } + + [Fact] + public async Task ApplyDataModelUpdate_OverDeepValueMap_StillDropped_NoRegression() + { + await _ui.RunOnUIAsync(() => + { + var harness = BuildHarness(_ui); + harness.DataModel.ApplyDataModelUpdate("s", null, new[] { DeepMapEntry("deepMap", 40) }); + Assert.Null(harness.DataModel.Read("s", "/deepMap")); + }); + } + + /// Build an entry whose value is nested valueArrays. + private static DataModelEntry DeepArrayEntry(string key, int depth) + { + DataModelEntry inner = new() { Key = string.Empty, ValueString = "x" }; + for (int i = 0; i < depth; i++) + inner = new DataModelEntry { Key = string.Empty, ValueArray = new[] { inner } }; + return new DataModelEntry { Key = key, ValueArray = new[] { inner } }; + } + + /// Build an entry whose value is nested valueMaps. + private static DataModelEntry DeepMapEntry(string key, int depth) + { + DataModelEntry inner = new() { Key = "leaf", ValueString = "x" }; + for (int i = 0; i < depth; i++) + inner = new DataModelEntry { Key = "n", ValueMap = new[] { inner } }; + return new DataModelEntry { Key = key, ValueMap = new[] { inner } }; + } +} From 1ffb6bca71cdc8a98afc88db9e4526aaa9b7e64c Mon Sep 17 00:00:00 2001 From: Scott Hanselman Date: Thu, 25 Jun 2026 16:05:28 -0700 Subject: [PATCH 2/2] Fix valueArray redaction and notifications Reject non-canonical array pointer indices for data-model array access, notify descendant subscribers when container values are replaced, and redact registered secret descendants when action contexts or snapshots include parent paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../A2UI/DataModel/DataModelStore.cs | 78 ++++++++-- .../A2UI/Rendering/IComponentRenderer.cs | 13 +- .../A2UI/Rendering/SecretRedactor.cs | 68 ++++++++- .../SecretRedactorTests.cs | 27 ++++ .../A2UIDataModelStoreTests.cs | 136 ++++++++++++++++++ 5 files changed, 304 insertions(+), 18 deletions(-) diff --git a/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs b/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs index 4e127f016..d33d69bb7 100644 --- a/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs +++ b/src/OpenClaw.Tray.WinUI/A2UI/DataModel/DataModelStore.cs @@ -247,14 +247,14 @@ public JsonObject CloneRoot() if (obj[tok] == null) { if (!createMissing) return (null, null, false, -1); - var nextIsIndex = int.TryParse(tokens[i + 1], out _); + var nextIsIndex = TryParseArrayIndex(tokens[i + 1], out _); obj[tok] = nextIsIndex ? new JsonArray() : new JsonObject(); } cursor = obj[tok]; } else if (cursor is JsonArray arr) { - if (!int.TryParse(tok, out var ai)) return (null, null, false, -1); + if (!TryParseArrayIndex(tok, out var ai)) return (null, null, false, -1); while (createMissing && arr.Count <= ai) arr.Add(null); if (ai < 0 || ai >= arr.Count) return (null, null, false, -1); cursor = arr[ai]; @@ -266,11 +266,28 @@ public JsonObject CloneRoot() } var last = tokens[^1]; - if (cursor is JsonArray finalArr && int.TryParse(last, out var idx)) + if (cursor is JsonArray finalArr && TryParseArrayIndex(last, out var idx)) return (finalArr, last, true, idx); return (cursor, last, false, -1); } + private static bool TryParseArrayIndex(string token, out int index) + { + index = -1; + if (token.Length == 0) return false; + if (token.Length > 1 && token[0] == '0') return false; + + foreach (var c in token) + if (c < '0' || c > '9') + return false; + + return int.TryParse( + token, + System.Globalization.NumberStyles.None, + System.Globalization.CultureInfo.InvariantCulture, + out index); + } + private static List SplitPointer(string pointer) { var p = pointer.StartsWith('/') ? pointer.Substring(1) : pointer; @@ -362,22 +379,55 @@ internal void NotifyPaths(IEnumerable paths) var current = key; while (true) { - List? subs; - lock (_model.Subscribers) - { - _model.Subscribers.TryGetValue(current, out subs); - subs = subs == null ? null : new List(subs); - } - if (subs != null) - { - foreach (var s in subs) - if (fired.Add(s)) Dispatch(s); - } + DispatchSubscribers(current, fired); if (current == "/" || string.IsNullOrEmpty(current)) break; var slash = current.LastIndexOf('/'); current = slash <= 0 ? "/" : current.Substring(0, slash); } + + // Replacing a container at /x also changes /x/... descendants. Most + // component bindings subscribe to the exact leaf path they read, so + // notify those subtree subscribers as well. + DispatchDescendantSubscribers(key, fired); + } + } + + private void DispatchSubscribers(string key, HashSet fired) + { + List? subs; + lock (_model.Subscribers) + { + _model.Subscribers.TryGetValue(key, out subs); + subs = subs == null ? null : new List(subs); } + + if (subs == null) return; + + foreach (var s in subs) + if (fired.Add(s)) + Dispatch(s); + } + + private void DispatchDescendantSubscribers(string key, HashSet fired) + { + List subs = []; + var prefix = key == "/" ? "/" : key + "/"; + + lock (_model.Subscribers) + { + foreach (var (subscriberPath, callbacks) in _model.Subscribers) + { + if (subscriberPath == key) + continue; + + if (key == "/" || subscriberPath.StartsWith(prefix, StringComparison.Ordinal)) + subs.AddRange(callbacks); + } + } + + foreach (var s in subs) + if (fired.Add(s)) + Dispatch(s); } internal void NotifyAllPaths() diff --git a/src/OpenClaw.Tray.WinUI/A2UI/Rendering/IComponentRenderer.cs b/src/OpenClaw.Tray.WinUI/A2UI/Rendering/IComponentRenderer.cs index c1369582a..2594e63dd 100644 --- a/src/OpenClaw.Tray.WinUI/A2UI/Rendering/IComponentRenderer.cs +++ b/src/OpenClaw.Tray.WinUI/A2UI/Rendering/IComponentRenderer.cs @@ -164,7 +164,12 @@ public void MarkSecretPath(string? path) { if (SecretPaths == null) return; if (string.IsNullOrEmpty(path)) return; - SecretPaths.Add(NormalizePath(path)); + var normalized = NormalizePath(path); + SecretPaths.Add(normalized); + + var canonical = SecretRedactor.CanonicalizeLenientArrayIndices(normalized); + if (!string.Equals(canonical, normalized, StringComparison.Ordinal)) + SecretPaths.Add(canonical); } /// True if is a secret path (registered or matches denylist). @@ -210,7 +215,11 @@ public bool IsSecretPath(string? path) var path = val.Path!; if (!IsAllowedPath(path, allowed)) continue; if (IsSecretPath(path)) continue; - result[key] = DataModel.Read(path)?.DeepClone(); + var clone = DataModel.Read(path)?.DeepClone(); + var registered = SecretPaths == null + ? System.Collections.Frozen.FrozenSet.Empty + : (IReadOnlySet)SecretPaths; + result[key] = SecretRedactor.RedactInPlace(clone, path, registered); } } return result; diff --git a/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs b/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs index 9e3418d82..ee6941068 100644 --- a/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs +++ b/src/OpenClaw.Tray.WinUI/A2UI/Rendering/SecretRedactor.cs @@ -50,11 +50,27 @@ public static bool IsSecret(string? path, IReadOnlySet registered) if (string.IsNullOrEmpty(path)) return false; var normalized = Normalize(path); if (registered.Contains(normalized)) return true; + + var canonical = CanonicalizeLenientArrayIndices(normalized); + if (!string.Equals(canonical, normalized, StringComparison.Ordinal) + && registered.Contains(canonical)) + { + return true; + } + // Any ancestor of the path counts: obscuring "/credentials" should hide "/credentials/password" too. foreach (var prefix in registered) { - if (prefix.Length == 0 || prefix == "/") continue; - if (normalized.StartsWith(prefix + "/", StringComparison.Ordinal)) return true; + var normalizedPrefix = Normalize(prefix); + if (normalizedPrefix.Length == 0 || normalizedPrefix == "/") continue; + if (IsPathOrDescendant(normalized, normalizedPrefix)) return true; + + var canonicalPrefix = CanonicalizeLenientArrayIndices(normalizedPrefix); + if (!string.Equals(canonicalPrefix, normalizedPrefix, StringComparison.Ordinal) + && IsPathOrDescendant(normalized, canonicalPrefix)) + { + return true; + } } return MatchesDenylist(normalized); } @@ -66,6 +82,7 @@ public static bool IsSecret(string? path, IReadOnlySet registered) public static JsonNode? Redact(JsonNode? root, IReadOnlySet registered) { if (root == null) return null; + if (IsSecret("/", registered)) return JsonValue.Create("[REDACTED]"); return RedactNode(root.DeepClone(), "/", registered); } @@ -76,9 +93,21 @@ public static bool IsSecret(string? path, IReadOnlySet registered) public static JsonNode? RedactInPlace(JsonNode? root, IReadOnlySet registered) { if (root == null) return null; + if (IsSecret("/", registered)) return JsonValue.Create("[REDACTED]"); return RedactNode(root, "/", registered); } + public static JsonNode? RedactInPlace(JsonNode? root, string rootPath, IReadOnlySet registered) + { + if (root == null) return null; + + var normalizedRootPath = Normalize(rootPath); + if (IsSecret(normalizedRootPath, registered)) + return JsonValue.Create("[REDACTED]"); + + return RedactNode(root, normalizedRootPath, registered); + } + private static JsonNode? RedactNode(JsonNode? node, string path, IReadOnlySet registered) { if (node is JsonObject obj) @@ -158,6 +187,41 @@ private static bool MatchesSegment(ReadOnlySpan segment) private static string Normalize(string p) => string.IsNullOrEmpty(p) ? "/" : (p[0] == '/' ? p : "/" + p); + private static bool IsPathOrDescendant(string path, string prefix) => + string.Equals(path, prefix, StringComparison.Ordinal) + || path.StartsWith(prefix + "/", StringComparison.Ordinal); + + internal static string CanonicalizeLenientArrayIndices(string path) + { + var normalized = Normalize(path); + if (normalized == "/") return normalized; + + var parts = normalized.Substring(1).Split('/'); + var changed = false; + for (var i = 0; i < parts.Length; i++) + { + var decoded = parts[i].Replace("~1", "/").Replace("~0", "~"); + if (!int.TryParse( + decoded, + System.Globalization.NumberStyles.Integer, + System.Globalization.CultureInfo.InvariantCulture, + out var index) + || index < 0) + { + continue; + } + + var canonical = index.ToString(System.Globalization.CultureInfo.InvariantCulture); + if (string.Equals(decoded, canonical, StringComparison.Ordinal)) + continue; + + parts[i] = EncodeSegment(canonical); + changed = true; + } + + return changed ? "/" + string.Join("/", parts) : normalized; + } + private static string EncodeSegment(string key) => key.Replace("~", "~0").Replace("/", "~1"); } diff --git a/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs b/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs index f49bbba91..851e4a315 100644 --- a/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs +++ b/tests/OpenClaw.Tray.Tests/SecretRedactorTests.cs @@ -95,6 +95,17 @@ public void IsSecret_RootRegistered_DoesNotMatchEverything() Assert.True(SecretRedactor.IsSecret("/profile/password", registered)); } + [Fact] + public void RedactInPlace_RootRegistered_RedactsWholeRoot() + { + var registered = new HashSet { "/" }; + var root = JsonNode.Parse("""{ "value": "1234", "profile": { "name": "alice" } }""")!; + + var redacted = SecretRedactor.RedactInPlace(root, registered)!; + + Assert.Equal("[REDACTED]", redacted.GetValue()); + } + [Fact] public void Redact_RegisteredSecretInsideArray_IsRedacted() { @@ -112,6 +123,22 @@ public void Redact_RegisteredSecretInsideArray_IsRedacted() Assert.Equal("5678", (string?)arr[1]); } + [Theory] + [InlineData("/codes/01")] + [InlineData("/codes/+1")] + [InlineData("/codes/ 1")] + public void Redact_NonCanonicalRegisteredArrayIndex_RedactsCanonicalElement(string registeredPath) + { + var registered = new HashSet { registeredPath }; + var root = JsonNode.Parse("""{ "codes": ["1234", "5678"] }""")!; + + var redacted = SecretRedactor.Redact(root, registered)!; + + var arr = Assert.IsType(redacted["codes"]); + Assert.Equal("1234", (string?)arr[0]); + Assert.Equal("[REDACTED]", (string?)arr[1]); + } + [Fact] public void Redact_RegisteredArrayParent_RedactsWholeArray() { diff --git a/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs b/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs index 871ec9ca4..22606334d 100644 --- a/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs +++ b/tests/OpenClaw.Tray.UITests/A2UIDataModelStoreTests.cs @@ -1,7 +1,11 @@ +using System; +using System.Collections.Generic; using System.Linq; using System.Text.Json.Nodes; using System.Threading.Tasks; +using OpenClawTray.A2UI.Rendering; using OpenClawTray.A2UI.Protocol; +using OpenClawTray.A2UI.Theming; using Xunit; using static OpenClaw.Tray.UITests.TestSupport; @@ -47,6 +51,110 @@ await _ui.RunOnUIAsync(() => }); } + [Fact] + public async Task ApplyDataModelUpdate_ValueArray_NotifiesElementPathSubscribers() + { + await _ui.RunOnUIAsync(() => + { + var harness = BuildHarness(_ui); + var observable = harness.DataModel.GetOrCreate("s"); + var notifications = 0; + using var subscription = observable.Subscribe("/codes/0", () => notifications++); + + harness.DataModel.ApplyDataModelUpdate("s", null, new[] + { + new DataModelEntry + { + Key = "codes", + ValueArray = new[] + { + new DataModelEntry { Key = string.Empty, ValueString = "1234" }, + }, + }, + }); + + Assert.Equal(1, notifications); + Assert.Equal("1234", observable.ReadString("/codes/0")); + }); + } + + [Theory] + [InlineData("/codes/01")] + [InlineData("/codes/+1")] + [InlineData("/codes/ 1")] + public async Task Read_NonCanonicalArrayIndex_DoesNotResolve(string pointer) + { + await _ui.RunOnUIAsync(() => + { + var harness = BuildHarness(_ui); + harness.DataModel.ApplyDataModelUpdate("s", null, new[] + { + new DataModelEntry + { + Key = "codes", + ValueArray = new[] + { + new DataModelEntry { Key = string.Empty, ValueString = "1234" }, + new DataModelEntry { Key = string.Empty, ValueString = "5678" }, + }, + }, + }); + + Assert.Null(harness.DataModel.Read("s", pointer)); + }); + } + + [Theory] + [InlineData("/codes/1")] + [InlineData("/codes/01")] + public async Task BuildActionContext_RedactsRegisteredSecretArrayElement_WhenParentPathIsRequested(string secretPath) + { + await _ui.RunOnUIAsync(() => + { + var ctx = BuildRenderContext( + JsonNode.Parse("""{ "codes": ["1234", "5678"] }""")!.AsObject(), + secretPath); + var source = BuildSourceComponent("""{ "dataBinding": ["/codes"] }"""); + var action = JsonNode.Parse(""" + { + "context": [ + { "key": "codes", "value": { "path": "/codes" } } + ] + } + """); + + var result = ctx.BuildActionContext(source, action)!; + + var codes = Assert.IsType(result["codes"]); + Assert.Equal("1234", (string?)codes[0]); + Assert.Equal("[REDACTED]", (string?)codes[1]); + }); + } + + [Fact] + public async Task BuildActionContext_RedactsDenylistedDescendant_WhenParentPathIsRequested() + { + await _ui.RunOnUIAsync(() => + { + var ctx = BuildRenderContext( + JsonNode.Parse("""{ "profile": { "name": "alice", "password": "p@ss" } }""")!.AsObject()); + var source = BuildSourceComponent("""{ "dataBinding": ["/profile"] }"""); + var action = JsonNode.Parse(""" + { + "context": [ + { "key": "profile", "value": { "path": "/profile" } } + ] + } + """); + + var result = ctx.BuildActionContext(source, action)!; + + var profile = Assert.IsType(result["profile"]); + Assert.Equal("alice", (string?)profile["name"]); + Assert.Equal("[REDACTED]", (string?)profile["password"]); + }); + } + [Fact] public async Task ApplyDataModelUpdate_OverDeepValueArray_IsDroppedByDepthGuard() { @@ -92,4 +200,32 @@ private static DataModelEntry DeepMapEntry(string key, int depth) inner = new DataModelEntry { Key = "n", ValueMap = new[] { inner } }; return new DataModelEntry { Key = key, ValueMap = new[] { inner } }; } + + private RenderContext BuildRenderContext(JsonObject seed, string? secretPath = null) + { + var harness = BuildHarness(_ui); + var observable = harness.DataModel.GetOrCreate("s", seed); + var secretPaths = new HashSet(StringComparer.Ordinal); + var ctx = new RenderContext + { + SurfaceId = "s", + DataModel = observable, + Actions = harness.Actions, + Theme = A2UITheme.Empty, + BuildChild = _ => null, + Subscriptions = new Dictionary(), + SecretPaths = secretPaths, + }; + + ctx.MarkSecretPath(secretPath); + return ctx; + } + + private static A2UIComponentDef BuildSourceComponent(string propertiesJson) => + new() + { + Id = "button", + ComponentName = "Button", + Properties = JsonNode.Parse(propertiesJson)!.AsObject(), + }; }