Joseph Doherty
3c908f1df0
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.
11 KiB
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.)