ScadaBridge/archreview/07-ui-management-security.md

Architecture Review 07 — Management & Presentation Surface (Round 2)

Date: 2026-07-12 Domain: Central UI (Blazor Server), Management Service (ManagementActor + HTTP API), CLI, Security & Auth — plus the post-initiative Alarm Summary aggregated live-alarm stream (central ISiteAlarmLiveCache / Blazor-circuit push side; the gRPC/site side belongs to review 02). Baseline: round-1 report at commit b910f5eb; this round re-reviews everything through HEAD 8c888f13 (PLAN-07 execution 74c295e3..d8a39f3c plus post-initiative work: live alarm stream b91ed3c8/8c888f13, Test-Run WaitForAttribute 22136d36, PLAN-06 edge spillover into UI forms).

Method

1. Read the round-1 report and archreview/plans/PLAN-07-ui-management-security.md in full.
2. Verified every round-1 finding's disposition in current source (no trust in plan claims) — every disposition below carries file:line evidence read this session.
3. Fresh sweep of git diff b910f5eb..HEAD scoped to CentralUI, ManagementService, CLI, Security (54 files, +2,397/−231), with a focused review of the new Alarm Summary live path (AlarmSummary.razor, AlarmSummaryService.BuildFromLiveAlarms, ISiteAlarmLiveCache, SiteAlarmLiveCacheService).
4. Static review only (suites verified green 2026-07-10); no builds/tests run.

Round-1 vs Round-2 Verdict

Round 1: mature surface with sharp asymmetries — an auth-disabled production artifact, a silently-ignored deploy option, inconsistent secret projection, a fictional secured-write "Expired" state. Round 2: the fix plan genuinely landed — 17 of 24 findings fully fixed and verified in source (including both criticals), 6 partially fixed with the residue consciously deferred and documented, 1 was a false premise; the new code (frozen 139-entry authz matrix, dispatch-coverage guard, TTL'd secured writes, live alarm cache) is of high quality, and the remaining new findings are Medium-and-below — mostly seams the fixes themselves exposed (an unthrottled debug-hub bind the docs claim is covered, throttle keying behind Traefik, and site-scope not enforced on secured writes despite the doc saying scoping "is layered on at the LDAP-mapping level").

---

Round-1 Finding Disposition

Finding	R1 Severity	Disposition	Evidence (file:line)
C1 — DisableLogin: true shipped in production deploy artifact; no environment guard	Critical	Fixed (verified)	Security/Auth/DisableLoginGuard.cs:17-28 (refuses outside Development without AllowOutsideDevelopment ack); wired fail-fast at the composition root Host/Program.cs:149-154 before AddSecurity; deploy/wonder-app-vd03/appsettings.Central.json no longer contains DisableLogin (repo-wide grep: no non-false occurrence under deploy/, docker/, docker-env2/), and its _comment_Security documents the new posture
C2 — deploy artifacts --site-id silently ignored; fleet-wide deploy with no scope check	High	Fixed (verified)	ArtifactDeploymentService.cs:228-255 (DeployToAllSitesAsync/DeployToSiteAsync over shared DeployCoreAsync); ManagementActor.cs:2295-2316 honors cmd.SiteId, adds the explicit fleet-wide guard for site-scoped non-admins (SiteScopeViolationException, :2300-2306) and EnforceSiteScope (:2307)
C3 — DataConnection / ExternalSystem / DatabaseConnection secrets leak in List/Get + audit	High	Fixed (verified)	ConfigSecretScrubber.cs (recursive JSON elision + sentinel merge); DataConnectionPublicShape ManagementActor.cs:1632-1647 applied to List/Get/Create/Update and audit afterState (:1653-1654, :1663, :1677-1678, :1697-1698), sentinel-preserving update :1692-1693; ExternalSystemPublicShape :1847-1857 + preserve-if-null :1897; DatabaseConnectionPublicShape :2835-2841 applied :2845-2878. Note: UI editors write via repositories directly (e.g. ExternalSystemForm.razor), so UI round-tripping was never at risk; CLI round-trips are sentinel-protected
S1 — 30 s server Ask vs 5-min bundle operations; import keeps applying after 504	High	Partially fixed (verified; residue deferred)	ManagementEndpoints.cs:41-58 — ResolveAskTimeout(options, commandType) gives Import/Preview/Export/DeployArtifacts/DeployInstance a 5-min default (ManagementServiceOptions.cs:9), call site :126; ManagementService timeout config shipped to all three topologies (docker/central-node-a/appsettings.Central.json:51, docker-env2/.../:48, deploy/wonder-app-vd03/.../:49); ManagementServiceOptionsValidator fail-fast. Still open (accepted, plan-05 domain): ImportBundleCommand (TransportCommands.cs:121-128) has no client-supplied idempotency id — a timeout-then-retry can still double-apply; the TransportGate single-flight (:3203) only narrows the window on one node
S2 — Pending secured writes never expire; "Expired" fictional	High	Fixed (verified)	ISecuredWriteRepository.cs:100 (TryMarkExpiredAsync CAS), :111 (CountAsync); TryExpireIfStaleAsync ManagementActor.cs:1224-1241 (TTL default 24 h, ManagementServiceOptions.cs:12; non-positive disables; emits AuditKind.SecuredWriteExpire); enforced before self-approval/CAS at approve (:1296-1302) and reject (:1453-1458); opportunistic sweep on list (:1493-1506); approve dialog now shows submission age (SecuredWrites.razor:415); spec'd in Component-Security.md:145
S3 — Poll timers overlap their own refreshes	Medium	Fixed (verified)	CentralUI/Services/PollGate.cs (Interlocked CAS gate); wired in AlarmSummary.razor:315-326 and Health.razor:552-562
S4 — 200 MB bodies buffered as strings, ~4 copies resident	Medium	Partially fixed (verified; residue deferred)	TransportGate single-flight semaphore ManagementActor.cs:3203-3213 wraps all three bundle handlers (released in finally :3317/:3361/:3442); export copy eliminated via ms.GetBuffer().AsSpan(...) :3309-3312. Body is still StreamReader.ReadToEndAsync() into a string (ManagementEndpoints.cs:103-107) with the 200 MB cap (:75); streaming multipart remains the deferred long-term shape
S5 — Fire-and-forget SignalR sends swallow failures	Low	Fixed (verified)	DebugStreamHub.cs:221-232 — ContinueWith(OnlyOnFaulted) logs push failures on both the event and termination paths while keeping fire-and-forget semantics
P1 — Full LDAP bind per request; no throttle/lockout (spray oracle)	Medium	Partially fixed (verified — one surface missed, see N1)	Security/LoginThrottle.cs (fixed-window, per username|ip, bounded at 10k keys, TimeProvider-driven); wired in ManagementAuthenticator.cs:110-129 (covers POST /management + audit REST via AuditEndpoints.cs:525), /auth/login AuthEndpoints.cs:38-58, /auth/token :125-147. DebugStreamHub.OnConnectedAsync still binds LDAP directly with no throttle (DebugStreamHub.cs:119-121) — see new finding N1. Per-request bind cost itself unchanged (documented stateless design)
P2 — Unbounded list handlers materialize whole tables	Medium	Partially fixed (verified; residue accepted)	Additive Skip/Take on ListTemplatesCommand/ListInstancesCommand (TemplateCommands.cs:5, InstanceCommands.cs:5); shared clamped Page helper ManagementActor.cs:576-580; instances paged after the site-scope filter :797-799. QueryDeployments unfiltered branch still loads all records + instances (:2334-2339) and ExportBundle still loads all-entities for name resolution — documented deferral, bounded by fleet size
P3 — ListSecuredWrites hard-caps at 200, no paging	Medium	Fixed (verified)	ListSecuredWritesCommand(…, int Skip = 0, int Take = 200) SecuredWriteCommands.cs:63; handler clamps (1..500) + CountAsync total ManagementActor.cs:1488-1491; UI history pager with real bounds SecuredWrites.razor:224-236, 343-349, 352-362
P4 — Alarm Summary re-sorts/filters per render	Low	Fixed (verified)	Memoized _visibleRows (AlarmSummary.razor:217-220), RecomputeVisibleRows() :401 invoked only from refresh/filter (@bind:after, :114-156)/sort (:461)
C4 — Browse/Verify/Cert commands documented as actor commands but UI-only; no CLI parity	Medium	Fixed (verified)	Six actor handlers ManagementActor.cs:3103-3161 (SiteIdentifier required + EnforceSiteScopeForIdentifier + relay via CommunicationService); dispatch arms :453-458; role gates: browse/search/verify → Designer (:258), cert trust → Admin (:219-223); additive SiteIdentifier on the six Commons records; CLI Commands/ConnectionBrowseCommands.cs (browse/search/verify-endpoint/certs list|trust|remove) registered in CLI/Program.cs:26-27
C5 — Role-casing asymmetry (UI ordinal vs actor ignore-case)	Medium	Fixed (verified)	CanonicalizeMappingRole ManagementActor.cs:2203-2210 — stored casing canonicalized against Roles.All at both mapping write paths (:2212, :2223); invalid-role rejection message preserved
C6 — Area role gate contradicts three docs	Medium	Fixed (verified)	Reconciled on any-of [Designer, Deployer]: AreaManagers ManagementActor.cs:171-178, arm :228; Component-ManagementService.md:224 documents the any-of gate and the rationale (docs' "Admin" was the stale side)
C7 — InstanceName passed into slot commented "InstanceId filter"	Low	Fixed (verified)	Behavior was correct (site instance_id stores UniqueName); comment now says so explicitly ManagementActor.cs:3086-3089
C8 — Stale CLAUDE.md note on SecuredWrite SourceNode	Low	Fixed (verified)	CLAUDE.md Security & Auth section now reads "(SecuredWrite audit rows stamp SourceNode via ICentralAuditWriter/INodeIdentityProvider.)"
UA1 — Secured-write lifecycle (no expiry, no paging, history ungated, dual-role hazard)	UA	Fixed (verified)	Expiry + paging per S2/P3; ListSecuredWrites gated to SecuredWriteReaders = Operator/Verifier/Admin (ManagementActor.cs:184-185, :290) with matching UI policy RequireSecuredWriteAccess (AuthorizationPolicies.cs:99-106, :179-180; page attribute SecuredWrites.razor:5); dual-role deployment hazard restated in Component-Security.md:143
UA2 — Test depth: no authz matrix over ~150 commands	UA	Fixed (verified)	RequiredRoleMatrixTests.cs — frozen ~139-entry Expected table + EveryRegisteredCommand_IsInTheFrozenTable reflection guard (:26, :194); DispatchCoverageTests.cs — every registered command must reach a dispatch arm (NotSupportedException sentinel; documented exclusions for ResolveRolesCommand and the site-routed ReadTagValuesCommand)
UA3 — CLI.Tests not in slnx	UA	Fixed (verified)	ZB.MOM.WW.ScadaBridge.slnx:52 (landed via PLAN-08 T1; PLAN-07 T33 correctly skipped as duplicate)
UA4 — No single deferred-work tracking list	UA	Fixed (verified)	docs/plans/2026-07-08-deferred-work-register.md — actively maintained (deferred #10/#22/#12/#14 resolved through it post-initiative)
UA5 — Login endpoints lack throttling; user-enumeration check	UA	Partially fixed (verified)	Throttling as per P1 (with the hub gap, N1). Enumeration verified safe: LdapAuthFailureMessages.cs:79-80 maps BadCredentials and UserNotFound to the identical "Invalid username or password."
UA6 — Accessibility debt on sortable headers	UA	Partially fixed (accepted deferral)	AlarmSummary headers: tabindex="0", aria-sort (AriaSortFor :469-470), Enter/Space activation with Space preventDefault (:479-487, markup :182-191). Fleet-wide grid sweep explicitly deferred and logged in the component comment (:175-179)
UA7 — "Committed" runtime logs under docker topology	UA	No longer applicable (false premise)	git check-ignore docker/central-node-a/logs → ignored; git ls-files docker docker-env2 | grep -i logs/ → empty. Untracked local output; nothing to scrub

Disposition counts: Fixed (verified) 17 · Partially fixed 6 (S1, S4, P1, P2, UA5, UA6 — all residues consciously deferred and documented except the P1/UA5 hub gap, which is new finding N1) · No longer applicable 1 (UA7) · Not fixed / Regressed 0.

---

New Findings — Round 2

A. Security & Auth

[Medium] N1 — DebugStreamHub still binds LDAP with no throttle; Component-Security.md falsely claims complete coverage

DebugStreamHub.OnConnectedAsync extracts Basic-Auth credentials itself and calls ILdapAuthService.AuthenticateAsync directly (DebugStreamHub.cs:118-124) — it uses ManagementAuthenticator only for the DisableLogin bypass (TryDisableLoginUser, :82), not for the bind, so the LoginThrottle never sees hub connection attempts. Meanwhile Component-Security.md:208 states the throttle scope includes "the debug-stream hub … No bind happens outside these paths, so throttling coverage is complete" — a doc statement that is simply untrue in code. A SignalR negotiate + connect per guess is clumsier than POST /auth/token, but it remains an unthrottled LDAP-bind oracle, and it also skips the RecordSuccess/RecordFailure bookkeeping that would let hub failures contribute to a shared lockout. Fix: route the hub's credential path through ManagementAuthenticator.AuthenticateAsync (it already returns roles + site scopes) or replicate the three throttle calls; correct the doc either way.

[Medium] N2 — LoginThrottle keys collapse behind Traefik: no ForwardedHeaders handling anywhere

Every throttle surface keys on context.Connection.RemoteIpAddress (ManagementAuthenticator.cs:111, AuthEndpoints.cs:39, :126), and no UseForwardedHeaders/ForwardedHeadersOptions exists in the Host, CentralUI, or ManagementService (repo grep: zero hits). The documented topology fronts both the UI and the management API with Traefik (CLAUDE.md CLI quick reference: management URL is the LB on :9000), so in production every client shares Traefik's IP and the key degenerates to username|<proxy-ip>. Two consequences: (a) the per-IP isolation the design intends ("alice@10.0.0.2 is not locked out by alice@10.0.0.1's failures" — LoginThrottleTests asserts exactly this) does not exist in the shipped topology; (b) any network-adjacent actor can lock out any username — five wrong passwords for admin via the LB locks admin out for all real operators for LoginLockoutMinutes, repeatable indefinitely: a cheap login-DoS against a SCADA control surface. Fix: honor X-Forwarded-For from the trusted proxy only (ForwardedHeadersOptions.KnownProxies), or document the trade-off and consider a success-path bypass (a correct password unlocks) to blunt the DoS.

[Medium] N3 — Secured-write handlers ignore site scope, while the spec says scoping "is layered on at the LDAP-mapping level"

None of the four secured-write handlers calls EnforceSiteScope/EnforceSiteScopeForIdentifier (verified: zero occurrences in ManagementActor.cs:1243-1510): HandleSubmitSecuredWrite resolves the target site (:1247) but never checks user.PermittedSiteIds; approve/reject/list likewise. Component-Security.md:141 explicitly says Operator/Verifier are "coarse global roles like the others; any site scoping is layered on at the LDAP-mapping level" — but a mapping row that grants Operator with PermittedSiteIds=["3"] still lets that principal submit (and a scoped Verifier approve) MxGateway device writes to every site. This is the same class of gap C2 just closed for DeployArtifacts, on a higher-consequence path. Fix: EnforceSiteScopeForIdentifier(sp, user, cmd.SiteId) at submit and row.SiteId at approve/reject (+ scope-filter the list), or amend the spec to state that secured-write roles are exempt from site scoping — code or doc, one must move.

B. Alarm Summary live path (ISiteAlarmLiveCache → Blazor circuit)

The design is genuinely good (see "What's genuinely good"), but the page-side reconciliation has seams:

[Medium] N4 — RefreshAsync applies results without re-checking the selected site: cross-site data displayed after a site switch

AlarmSummary.razor:283-297 captures siteId at entry, awaits the multi-second fan-out (GetSiteAlarmsAsync), then unconditionally assigns _rows/_notReporting/_rollup. A poll tick for site A in flight when the operator switches to site B completes after B's initial refresh and overwrites the page with A's alarms (and A's not-reporting list) labeled under B's picker — on an operator alarm page. The live path already has exactly the right guard (OnLiveAlarmsChanged drops callbacks when _selectedSiteId != siteId, :374); the poll path needs the same one line before assignment (if (_selectedSiteId != siteId) return;). Self-corrects within ≤15 s, but wrong-site alarm data for that window is not acceptable on this surface.

[Low] N5 — When live, a slower poll snapshot can regress fresher live state

The 15 s poll always rebuilds _rows from the fan-out even when the cache is live (AlarmSummary.razor:293-297); the comment claims the poll "simply re-affirms the same snapshot" (:344-347), but a poll whose fan-out started before a live delta lands after it and momentarily reverts the delta (an alarm flickers back to its prior state until the next delta/poll). Cheap fix: when LiveAlarmCache.IsLive(siteId), let the poll update only _notReporting (its unique authority) and skip the row rebuild — this also removes redundant per-instance snapshot load while live.

[Low] N6 — IsLive is sticky: HasPublished never resets on aggregator death or stream loss

SiteEntry.HasPublished is set on first publish (SiteAlarmLiveCacheService.cs:392-393) and never cleared; IsLive (:132-136) therefore reports true forever, even if the aggregator actor later dies or the site goes dark and entry.Current freezes. Exposure is bounded (the poll keeps overwriting rows, and reconcile re-seeds a living aggregator), but OnSiteChangedAsync deliberately applies the live snapshot on top of the fresh poll when IsLive (AlarmSummary.razor:277-280) — with a dead aggregator that grafts an arbitrarily stale snapshot over correct data for up to 15 s. Consider clearing HasPublished (or publishing a degraded flag) when the actor terminates (a Context.Watch/deathwatch hook from the service) and having the interface expose staleness, not just "seeded once".

[Low] N7 — Live-callback InvokeAsync lacks the house disposal guard

OnLiveAlarmsChanged does _ = InvokeAsync(...) with no _disposed flag (AlarmSummary.razor:367-382); a callback racing Dispose() can fault the discarded task with ObjectDisposedException from StateHasChanged on a torn-down renderer. DebugView.razor established the pattern for exactly this (SafeInvokeAsync + _disposed set before teardown — cited in round 1 as exemplary); the new page should match it. SiteAlarmLiveCacheService.OnPublish catches and logs synchronous callback throws (:399-410) but cannot observe the async fault.

C. Management / Security implementation details

[Low] N8 — LoginThrottle.Prune scans (and may sort) the whole map on every failed bind

RecordFailure → Prune enumerates all entries and, when over cap, runs OrderBy over the entire dictionary (LoginThrottle.cs:121, :146-168). During the exact scenario the throttle exists for — a spray — every failed request pays an O(n) scan (n up to 10,000) plus a possible full sort, on the request path. Harmless at this cap, but the pruning would be better amortized (e.g. every Nth write or on a timer). Counterpoint noted: the cap and opportunistic pruning are exactly what keeps memory bounded, and the constants are sane.

[Low] N9 — ConfigSecretScrubber.MergeSentinels matches array elements by index; fragment list is name-based

Sentinel restoration inside arrays pairs incoming[i] with stored[i] (ConfigSecretScrubber.cs:200-221): a client that reorders an array of credentialed objects while round-tripping sentinels grafts the wrong stored secret into each slot (silent misconfiguration, not disclosure). And detection is name-fragment-based (password|secret|token|apikey|credential|passphrase, :18-19) — current config types are fully covered (OpcUaUserIdentityConfig.Password/CertificatePassword, MxGatewayEndpointConfig.ApiKey), but a future field named e.g. SigningKey or Pin would silently leak; worth a lock-in test enumerating the DataConnections config types' string properties against the fragment list, mirroring the transport round-trip-guard idea.

---

What's genuinely good

• The authorization table is now a frozen, mechanically-enforced artifact. GetRequiredRoles is a single any-of switch over named static role sets with per-arm rationale comments (ManagementActor.cs:163-294), backed by the ~139-entry RequiredRoleMatrixTests table that fails on any unlisted command, plus DispatchCoverageTests proving every registered command reaches a real handler. A C2- or C4-class regression is now a CI failure, not an audit finding.
• The secured-write lifecycle is defensively complete. TTL checked before self-approval before CAS (ManagementActor.cs:1296-1315), CAS-idempotent multi-node expiry, expiry audited as SecuredWriteExpire, protocol re-checked at execute, list gated and paged, submission age surfaced in the approve dialog — and the docs restate both the TTL and the dual-role deployment hazard precisely (Component-Security.md:143-145).
• The live alarm cache is a model piece of lifecycle engineering: reference-counted per-site aggregators with linger-delayed stop and version-checked timers (SiteAlarmLiveCacheService.cs:160-190), fail-safe viewer cap returning a no-op handle on a render path (:100-107), race-checked start with self-healing bounded retry (:219-274), callbacks invoked outside the lock with per-viewer fault isolation (:397-410), immutable snapshot swaps, and validated options (CommunicationOptionsValidator.cs:72-87). The page's seed-then-stream reconciliation comments (:337-351) show real thought about ordering.
• Secret hygiene is now symmetric: the *PublicShape pattern covers SMTP, SMS, API keys, data connections, external systems, and DB connections, on responses and audit afterState, with sentinel-preserving round-trips — and the elision reuses one tested scrubber instead of six bespoke projections.
• DisableLoginGuard is the right shape: fail-fast at the composition root before any service wiring, environment-gated with an explicit double-ack escape hatch, and the error message tells the operator all three ways out (DisableLoginGuard.cs:22-27).
• Consistency discipline held under a 739-line actor diff: paging clamped through one shared helper applied after scope filters, curated ManagementCommandException messages everywhere, additive-only contract evolution on all six browse/cert records and the paging params, and docs updated in the same commits (verified for C4/C5/C6/S2/throttle sections).

---

Prioritized Recommendations (new findings only)

1. [Medium] Throttle the debug hub's LDAP bind and fix the doc — route DebugStreamHub.OnConnectedAsync through ManagementAuthenticator (or replicate the three throttle calls); correct Component-Security.md:208. (N1)
2. [Medium] Decide the proxy-IP question for LoginThrottle — trusted-proxy ForwardedHeaders (KnownProxies = Traefik) so keys are real client IPs, or document the username-lockout-DoS trade-off deliberately. (N2)
3. [Medium] Enforce (or explicitly exempt) site scope on secured writes — EnforceSiteScopeForIdentifier at submit/approve/reject + scoped list, or amend Component-Security.md:141; code and spec currently disagree. (N3)
4. [Medium] One-line staleness guard in AlarmSummary.RefreshAsync — re-check _selectedSiteId == siteId before assigning results, mirroring the live path's guard. (N4)
5. [Low] Live-path polish — poll updates only _notReporting while IsLive (N5); reset/expose liveness on aggregator death (N6); adopt the DebugView disposal-guard pattern for the live callback (N7).
6. [Low] Hygiene — amortize LoginThrottle.Prune (N8); add a lock-in test binding the scrubber's fragment list to the DataConnections config types' string properties (N9).

Severity tally — NEW findings (round 2)

Severity	Count	Findings
Critical	0	—
High	0	—
Medium	4	N1 (hub bind unthrottled + doc drift), N2 (throttle keying behind Traefik), N3 (secured-write site scope), N4 (Alarm Summary stale-site race)
Low	5	N5 (poll-over-live regression), N6 (sticky IsLive), N7 (disposal guard), N8 (Prune cost), N9 (scrubber merge/fragment coverage)