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.
2026-06-19 10:52:23 -04:00
parent 1aa7905676
commit 3c908f1df0
7 changed files with 306 additions and 5 deletions
@@ -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<T>(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<T>` 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.)
@@ -227,9 +227,10 @@
        try
        {
            var userName = await GetCurrentUserNameAsync();
+            using var cts = new System.Threading.CancellationTokenSource(TimeSpan.FromSeconds(15));
            var result = await AdminOps.AskAsync<ReconnectDriverResult>(
                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<RestartDriverResult>(
                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)
@@ -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}";
 }
@@ -1280,9 +1280,17 @@ public sealed class UnsTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
        return null;
    }

-    /// <summary>Returns <c>true</c> if <paramref name="json"/> parses as a well-formed JSON document.</summary>
-    private static bool IsValidJson(string json)
+    /// <summary>Returns <c>true</c> if <paramref name="json"/> 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 <see cref="ArgumentNullException"/> from
+    /// <c>JsonDocument.Parse(null)</c> (AdminUI-002).</summary>
+    private static bool IsValidJson(string? json)
    {
+        if (string.IsNullOrWhiteSpace(json))
+        {
+            return false;
+        }
+
        try
        {
            using var _ = System.Text.Json.JsonDocument.Parse(json);
@@ -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);
+
+    /// <summary>A tag name carrying query-reserved characters is percent-encoded so the produced
+    /// address stays a well-formed query string (AdminUI-005). With "A&amp;B?C" the '&amp;' and '?'
+    /// must not be read as a query separator / start, so they are escaped.</summary>
+    [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));
+    }
 }
@@ -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();
+    }
 }
@@ -167,6 +167,41 @@ public sealed class UnsTreeServiceTagTests
        db.Tags.Any(t => t.TagId == "TAG-1").ShouldBeFalse();
    }

+    /// <summary>A tag with a null TagConfig is blocked with the friendly "not valid JSON" message
+    /// (rather than an unhandled ArgumentNullException) — AdminUI-002.</summary>
+    [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();
+    }
+
+    /// <summary>A tag with a blank/whitespace TagConfig is blocked with the friendly message — AdminUI-002.</summary>
+    [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();
+    }
+
    /// <summary>Binding a tag to a driver in a different cluster than the equipment is blocked (#122).</summary>
    [Fact]
    public async Task CreateTag_driver_in_other_cluster_blocked()