From 3c908f1df0736dc278dff1e7947a36d530a8c763 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 10:52:23 -0400 Subject: [PATCH] review(AdminUI): fix null-TagConfig crash, CTS leak, unencoded historian tag Review at HEAD 7286d320. AdminUI-002: IsValidJson null/blank -> friendly error (was ArgumentNullException). AdminUI-003: DriverStatusPanel Reconnect/Restart dispose CTS (build- verified, live /run deferred). AdminUI-005: HistorianWonderware picker URL-encodes tag name. AdminUI-008: Format round-trip test. 001 (script-page authz) + 004 (hub [Authorize]) left Open as cross-cutting w/ Host/Security. --- code-reviews/AdminUI/findings.md | 219 ++++++++++++++++++ .../Shared/Drivers/DriverStatusPanel.razor | 6 +- .../HistorianWonderwareAddressBuilder.cs | 5 +- .../Uns/UnsTreeService.cs | 12 +- .../HistorianWonderwareAddressBuilderTests.cs | 14 ++ .../ScriptAnalysis/FormatTests.cs | 20 ++ .../Uns/UnsTreeServiceTagTests.cs | 35 +++ 7 files changed, 306 insertions(+), 5 deletions(-) create mode 100644 code-reviews/AdminUI/findings.md diff --git a/code-reviews/AdminUI/findings.md b/code-reviews/AdminUI/findings.md new file mode 100644 index 00000000..947586d9 --- /dev/null +++ b/code-reviews/AdminUI/findings.md @@ -0,0 +1,219 @@ +# Code Review — AdminUI + +| Field | Value | +|---|---| +| Module | `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 2 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | AdminUI-002, AdminUI-005, AdminUI-008 (resolved) | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | AdminUI-003 (resolved) | +| 4 | Error handling & resilience | AdminUI-006 (won't fix) | +| 5 | Security | AdminUI-001 (open), AdminUI-004 (open) | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | AdminUI-001 (open) | +| 8 | Code organization & conventions | AdminUI-007 (won't fix) | +| 9 | Testing coverage | Addressed via the AdminUI-002/005/008 regression tests | +| 10 | Documentation & comments | No issues found | + +## Findings + +### AdminUI-001 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Security | +| Location | `Components/Pages/ScriptEdit.razor:5`, `Components/Pages/Scripts.razor` | +| Status | Open | + +**Description:** The Script CRUD surface (`/scripts/new`, `/scripts/{id}`, and the `/scripts` list) is +gated by only `[Authorize]` (any authenticated user), whereas the *same editor's* Roslyn-analysis +backend (`ScriptAnalysisEndpoints`) requires the `FleetAdmin` policy, and `/deployments` requires +`Administrator,Designer`. A virtual-tag script is executable C# that runs server-side in the sandbox at +deploy time, so saving/editing script source is a privileged operation; gating it below the analysis +backend and the deploy action is an inconsistency in the privilege model. A `ReadOnly`-tier operator +who can authenticate can currently create and edit script bodies (they cannot deploy them, and the +runtime is sandboxed, which is why this is Medium and not High). + +**Recommendation:** Align the Script pages' authorization with the privilege model — gate `ScriptEdit` +and `Scripts` with `Authorize(Roles = "Administrator,Designer")` (matching `/deployments`) or the +`FleetAdmin` policy (matching the analysis backend). This is a policy/attribute change on Razor pages; +confirm the intended tier with the security owner, since it changes who can reach the editor. Left Open +pending that decision (an authorization-policy decision, not a self-contained bug fix). + +**Resolution:** _(open)_ + +### AdminUI-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `Uns/UnsTreeService.cs:1284` (`IsValidJson`) | +| Status | Resolved | + +**Description:** `CreateTagAsync`/`UpdateTagAsync` validate the tag config with +`IsValidJson(input.TagConfig)`, which calls `JsonDocument.Parse(json)`. The `catch` only handles +`JsonException`. If `input.TagConfig` is `null`, `JsonDocument.Parse((string)null)` throws +`ArgumentNullException`, which escapes `IsValidJson` and faults the mutation with an unhandled exception +(unfriendly 500) instead of returning the intended "TagConfig is not valid JSON." result. The TagModal +always supplies `"{}"`, so this is latent, but the service is a public seam (also exercised by tests) +and should treat null defensively. + +**Recommendation:** Treat null/blank as invalid in `IsValidJson` (return `false` on null/whitespace) so +the friendly result is returned for every caller. + +**Resolution:** Resolved 2026-06-19 — `IsValidJson` returns `false` for null/whitespace before +`JsonDocument.Parse`, so a null `TagConfig` yields the friendly "not valid JSON" result instead of an +unhandled `ArgumentNullException`. TDD: +`UnsTreeServiceTagTests.CreateTag_with_null_TagConfig_returns_friendly_error` and +`...with_blank_TagConfig_returns_friendly_error` (failing → fixed → passing). (SHA pending.) + +### AdminUI-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Concurrency & thread safety | +| Location | `Components/Shared/Drivers/DriverStatusPanel.razor:231`, `:256` | +| Status | Resolved | + +**Description:** `ReconnectAsync` and `RestartConfirmedAsync` create a `CancellationTokenSource` inline +(`new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(15)).Token`) and never dispose it. +A `CancellationTokenSource` with a timeout schedules a `Timer`; not disposing it leaks that timer until +finalization. The sibling `Alerts.razor` does this correctly with `using var cts = ...`. Each +Reconnect/Restart click leaks one CTS + timer for the circuit's lifetime. + +**Recommendation:** Use `using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(15));` and +pass `cts.Token`, matching `Alerts.razor`. + +**Resolution:** Resolved 2026-06-19 — both handlers now use `using var cts` and pass `cts.Token`, so the +CTS (and its timer) is disposed when the handler returns. Razor-behavioural change verified by build only +— live `/run` deferred (no bUnit). (SHA pending.) + +### AdminUI-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Security | +| Location | `Hubs/FleetStatusHub.cs`, `Hubs/AlertHub.cs`, `Hubs/ScriptLogHub.cs` (vs `Hubs/DriverStatusHub.cs:12`) | +| Status | Open | + +**Description:** Of the four SignalR hubs mapped by `MapOtOpcUaHubs`, only `DriverStatusHub` carries an +explicit `[Authorize]` attribute. `FleetStatusHub`, `AlertHub`, and `ScriptLogHub` (which broadcast +fleet status, alarm transitions with equipment paths/messages, and script-log diagnostic lines) have no +explicit authorization metadata. In the current Host wiring they ARE protected against unauthenticated +access by the global `FallbackPolicy = RequireAuthenticatedUser` +(`Security/ServiceCollectionExtensions.cs:144`), so this is not an open hole today — but the protection +is implicit and inconsistent. If the fallback policy were ever relaxed, or a hub mapped from a different +host without it, these three hubs would silently become anonymous. + +**Recommendation:** Add `[Authorize]` to `FleetStatusHub`, `AlertHub`, and `ScriptLogHub` to match +`DriverStatusHub`, so each hub's authorization requirement is explicit and does not rely solely on the +host's fallback policy. Low-risk (the fallback already requires auth, so observable behaviour is +unchanged) but left Open because hub authorization is a cross-cutting concern worth confirming with the +Host/Security owners rather than silently changing in a single-module pass. + +**Resolution:** _(open)_ + +### AdminUI-005 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `Components/Shared/Drivers/Pickers/HistorianWonderwareAddressBuilder.cs:11` | +| Status | Resolved | + +**Description:** `HistorianWonderwareAddressBuilder.Build` interpolates the tag name straight into a +query string: `$"{tagName}?mode={mode}&interval={interval}"`. A historian tag name containing `?`, `&`, +`#`, or `=` would corrupt the produced address (the part after a `&` in the name would be parsed as a +spurious query parameter by any downstream consumer that splits on `&`). Historian tag names are +typically dotted identifiers, so this is unlikely in practice, but the builder is unit-tested as a pure +helper and should produce a well-formed string for any input. + +**Recommendation:** URL-encode the tag name (`Uri.EscapeDataString(tagName)`) before composing the query +string. + +**Resolution:** Resolved 2026-06-19 — the tag name is `Uri.EscapeDataString`-encoded before being placed +in the query string, so a name containing reserved characters produces a well-formed address. TDD: +`HistorianWonderwareAddressBuilderTests.Build_escapes_reserved_characters_in_tag_name` +(failing → fixed → passing). (SHA pending.) + +### AdminUI-006 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Error handling & resilience | +| Location | `Uns/UnsTreeService.cs:368`, `:494`, `:707`, `:993`, `:1191`, `:1518` | +| Status | Won't Fix | + +**Description:** The delete paths catch a bare `Exception` and surface `ex.Message` verbatim into the +operator-facing `UnsMutationResult.Error` (e.g. `$"Delete failed: {ex.Message}. Likely because …"`). On +a real SQL Server an FK-violation message can include server/schema/constraint detail. This is low risk +(the AdminUI is an authenticated fleet-admin surface, not public, and the messages are deliberately +operator-actionable) but echoing raw provider exception text into the UI is a minor information-exposure +/ robustness smell. + +**Recommendation:** Either keep as-is (intentional operator guidance) or log `ex` at Warning and surface +a fixed message without the raw `ex.Message`. + +**Resolution:** Won't Fix (2026-06-19) — intentional design. The AdminUI is an authenticated +fleet-admin-only surface and the raw message is paired with deliberate operator guidance ("…remove its +children first"). Removing the provider text would reduce diagnosability for the operators who own these +operations. Recorded as a known, accepted trade-off. + +### AdminUI-007 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `Clients/AdminOperationsClient.cs:37`, `:52`, `:69` | +| Status | Won't Fix | + +**Description:** `StartDeploymentAsync`, `AcknowledgeAlarmAsync`, and `ShelveAlarmAsync` pass BOTH an +explicit `AskTimeout` and a linked CTS that itself already has `CancelAfter(AskTimeout)`: +`_proxy.Ask(msg, AskTimeout, linked.Token)`. The two timeouts are redundant (the Ask-level timeout +and the token both fire at 10s). Harmless, but the doubled timeout is slightly confusing — the +`AskAsync` generic at `:78` correctly relies on the caller's token alone. + +**Recommendation:** Drop one of the two — either pass only `linked.Token` (relying on the CTS timeout) or +only `AskTimeout` (dropping the linked CTS). Cosmetic. + +**Resolution:** Won't Fix (2026-06-19) — the belt-and-suspenders pairing is harmless and the two +mechanisms guard slightly different failure modes (the Ask timeout is Akka's own deadline; the linked CTS +also honours caller cancellation). Not worth a behavioural change to a working control-plane client in a +review pass. + +### AdminUI-008 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `ScriptAnalysis/ScriptAnalysisService.cs:376` (`Format`) | +| Status | Resolved | + +**Description:** `Format` documents a "return the original source unchanged on un-formattable input" +contract (the `catch` returns `req.Code`), but no regression test locked that contract for null/blank or +unparseable input, so a future change to the catch could silently break the editor's format action. (No +behavioural bug found; this is a test-coverage gap on a documented contract.) + +**Recommendation:** Add a regression test asserting `Format` returns the input unchanged for +null/blank/garbage input and round-trips valid input. + +**Resolution:** Resolved 2026-06-19 — added `FormatTests.Format_returns_input_unchanged_for_null_or_empty` and +`Format_returns_input_for_unparseable_garbage`, locking the return-original contract (the existing +`FormatTests` already covered the happy path). Coverage-only; no production change. (SHA pending.) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverStatusPanel.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverStatusPanel.razor index 8c4a1855..d2d2f2c3 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverStatusPanel.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/DriverStatusPanel.razor @@ -227,9 +227,10 @@ try { var userName = await GetCurrentUserNameAsync(); + using var cts = new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(15)); var result = await AdminOps.AskAsync( new ReconnectDriver(ClusterId, DriverInstanceId, userName, Guid.NewGuid()), - new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(15)).Token); + cts.Token); ShowOpResult(result.Ok, result.Ok ? "Reconnect dispatched" : (result.Message ?? "Failed")); } catch (Exception ex) @@ -252,9 +253,10 @@ try { var userName = await GetCurrentUserNameAsync(); + using var cts = new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(15)); var result = await AdminOps.AskAsync( new RestartDriver(ClusterId, DriverInstanceId, userName, Guid.NewGuid()), - new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(15)).Token); + cts.Token); ShowOpResult(result.Ok, result.Ok ? "Restart dispatched" : (result.Message ?? "Failed")); } catch (Exception ex) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Pickers/HistorianWonderwareAddressBuilder.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Pickers/HistorianWonderwareAddressBuilder.cs index 2fc5b467..f797234c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Pickers/HistorianWonderwareAddressBuilder.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Pickers/HistorianWonderwareAddressBuilder.cs @@ -8,5 +8,8 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Components.Shared.Drivers.Pickers; public static class HistorianWonderwareAddressBuilder { public static string Build(string tagName, string mode, int interval) - => $"{tagName}?mode={mode}&interval={interval}"; + // Percent-encode the tag name so a name carrying query-reserved characters (? & # =) can't + // corrupt the produced query string (AdminUI-005). Mode is a fixed enum-style token, so it + // needs no encoding. + => $"{Uri.EscapeDataString(tagName)}?mode={mode}&interval={interval}"; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs index d62c77d1..469e7f3d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs @@ -1280,9 +1280,17 @@ public sealed class UnsTreeService(IDbContextFactory dbF return null; } - /// Returns true if parses as a well-formed JSON document. - private static bool IsValidJson(string json) + /// Returns true if parses as a well-formed JSON document. + /// Null/blank input is treated as invalid (not well-formed JSON) so every caller gets the friendly + /// "not valid JSON" result rather than an unhandled from + /// JsonDocument.Parse(null) (AdminUI-002). + private static bool IsValidJson(string? json) { + if (string.IsNullOrWhiteSpace(json)) + { + return false; + } + try { using var _ = System.Text.Json.JsonDocument.Parse(json); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Pickers/HistorianWonderwareAddressBuilderTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Pickers/HistorianWonderwareAddressBuilderTests.cs index 816e550f..15a463c0 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Pickers/HistorianWonderwareAddressBuilderTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Pickers/HistorianWonderwareAddressBuilderTests.cs @@ -12,4 +12,18 @@ public sealed class HistorianWonderwareAddressBuilderTests [InlineData("FlowRate", "Delta", 30, "FlowRate?mode=Delta&interval=30")] public void Build_Canonical(string tag, string mode, int interval, string expected) => HistorianWonderwareAddressBuilder.Build(tag, mode, interval).ShouldBe(expected); + + /// A tag name carrying query-reserved characters is percent-encoded so the produced + /// address stays a well-formed query string (AdminUI-005). With "A&B?C" the '&' and '?' + /// must not be read as a query separator / start, so they are escaped. + [Fact] + public void Build_escapes_reserved_characters_in_tag_name() + { + var result = HistorianWonderwareAddressBuilder.Build("A&B?C", "Cyclic", 60); + + // The only literal '?' is the query separator the builder inserts; the only literal '&' + // is the one between mode and interval. The reserved characters in the name are escaped. + result.ShouldBe("A%26B%3FC?mode=Cyclic&interval=60"); + result.IndexOf('?').ShouldBe(result.IndexOf("?mode=", System.StringComparison.Ordinal)); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/FormatTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/FormatTests.cs index 858019de..9f1f85a2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/FormatTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/FormatTests.cs @@ -27,4 +27,24 @@ public sealed class FormatTests var src = "return ctx.GetTag(\"A\").Value;"; Fmt(src).ShouldNotBeNull(); } + + // AdminUI-008: lock the documented "return the original on un-formattable input" contract. + + [Fact] public void Format_returns_input_unchanged_for_null_or_empty() + { + // Null and empty short-circuit before parsing (string.IsNullOrEmpty guard) and round-trip + // verbatim — the documented "return the original" contract for non-formattable input. + Svc.Format(new FormatRequest(null!)).Code.ShouldBeNull(); + Fmt("").ShouldBe(""); + } + + [Fact] public void Format_returns_input_for_unparseable_garbage() + { + // Deeply malformed input must never throw into the caller — the contract is "return the + // original string". Whatever Format returns, it must be non-null and preserve the content + // for input the formatter cannot reflow. + const string garbage = "@@@ ))) {{{ not csharp at all"; + var outp = Fmt(garbage); + outp.ShouldNotBeNull(); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceTagTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceTagTests.cs index 59a9449c..4f6ae96e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceTagTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceTagTests.cs @@ -167,6 +167,41 @@ public sealed class UnsTreeServiceTagTests db.Tags.Any(t => t.TagId == "TAG-1").ShouldBeFalse(); } + /// A tag with a null TagConfig is blocked with the friendly "not valid JSON" message + /// (rather than an unhandled ArgumentNullException) — AdminUI-002. + [Fact] + public async Task CreateTag_with_null_TagConfig_returns_friendly_error() + { + var (service, dbName) = Fresh(); + SeedHierarchyAndDrivers(dbName, equipmentCluster: "MAIN", seedEquipmentDriver: true); + + var result = await service.CreateTagAsync( + "EQ-1", Input("TAG-1", "speed", "DRV-EQ", tagConfig: null!)); + + result.Ok.ShouldBeFalse(); + result.Error.ShouldBe("TagConfig is not valid JSON."); + + using var db = UnsTreeTestDb.CreateNamed(dbName); + db.Tags.Any(t => t.TagId == "TAG-1").ShouldBeFalse(); + } + + /// A tag with a blank/whitespace TagConfig is blocked with the friendly message — AdminUI-002. + [Fact] + public async Task CreateTag_with_blank_TagConfig_returns_friendly_error() + { + var (service, dbName) = Fresh(); + SeedHierarchyAndDrivers(dbName, equipmentCluster: "MAIN", seedEquipmentDriver: true); + + var result = await service.CreateTagAsync( + "EQ-1", Input("TAG-1", "speed", "DRV-EQ", tagConfig: " ")); + + result.Ok.ShouldBeFalse(); + result.Error.ShouldBe("TagConfig is not valid JSON."); + + using var db = UnsTreeTestDb.CreateNamed(dbName); + db.Tags.Any(t => t.TagId == "TAG-1").ShouldBeFalse(); + } + /// Binding a tag to a driver in a different cluster than the equipment is blocked (#122). [Fact] public async Task CreateTag_driver_in_other_cluster_blocked()