diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 62b2914..007bb2e 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 6 | +| Open findings | 0 | ## Summary @@ -994,7 +994,7 @@ in the fixture.) |--|--| | Severity | High | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor:39-62`; `src/ScadaLink.CentralUI/Auth/CookieAuthenticationStateProvider.cs:29-43` | **Description** @@ -1036,7 +1036,7 @@ expired session (see CentralUI-025). **Resolution** -_Unresolved._ +2026-05-17 — `SessionExpiry` no longer polls the frozen `AuthenticationStateProvider`; it polls a new anonymous `GET /auth/ping` minimal-API endpoint (re-validated by the cookie middleware on every HTTP request) via a `fetch()` JS helper and redirects to `/login` on HTTP 401, so the documented 30-minute idle logout actually fires. ### CentralUI-021 — `DebugView` stream callback mutates `Dictionary` off the render thread @@ -1044,7 +1044,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor:404-419,511-519,275-289` | **Description** @@ -1075,7 +1075,7 @@ critical section as the upsert. **Resolution** -_Unresolved._ +2026-05-17 — the stream callback now routes through `HandleStreamEvent`, which marshals the `UpsertWithCap` mutation (and the cap-trim loop) onto the renderer's dispatcher via `SafeInvokeAsync`, so every read and write of `_attributeValues`/`_alarmStates` happens on the render thread. ### CentralUI-022 — `Deployments` push handler fires `InvokeAsync` with no disposal guard @@ -1083,7 +1083,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor:221-229,317-322` | **Description** @@ -1114,7 +1114,7 @@ rather than the whole table on each event. **Resolution** -_Unresolved._ +2026-05-17 — added a `volatile bool _disposed` set first in `Dispose()`; `OnDeploymentStatusChanged` no-ops when set, and the fire-and-forget dispatch (`DispatchReloadAsync`) swallows the residual `ObjectDisposedException`, mirroring the `DebugView`/`ToastNotification` guards. ### CentralUI-023 — Residual bare `catch {}` blocks swallow JS interop errors @@ -1122,7 +1122,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor:690-698`; `src/ScadaLink.CentralUI/Components/Shared/DiffDialog.razor:107-116,118-130,104` | **Description** @@ -1147,7 +1147,7 @@ call, consistent with the CentralUI-018 fixes in the same module. **Resolution** -_Unresolved._ +2026-05-17 — the bare `catch` blocks in `ParkedMessages.CopyAsync` and `DiffDialog.TryLockBodyAsync`/`TryUnlockBodyAsync`/`OnAfterRenderAsync` now catch `JSDisconnectedException` (and `InvalidOperationException` for prerender focus) silently and log genuine `JSException` failures via injected `ILogger`. ### CentralUI-024 — Claim lookups use magic strings instead of `JwtTokenService` constants @@ -1155,7 +1155,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor:102`; `src/ScadaLink.CentralUI/Components/Pages/Dashboard.razor:14`; `GetCurrentUserAsync` in `Templates.razor`, `TemplateEdit.razor`, `TemplateCreate.razor`, `SharedScripts.razor`, `SharedScriptForm.razor`, `Sites.razor`, `Topology.razor`, `InstanceCreate.razor`, `InstanceConfigure.razor` | **Description** @@ -1181,7 +1181,7 @@ or a small scoped service) so the claim lookup lives in exactly one place. **Resolution** -_Unresolved._ +2026-05-17 — added `ClaimsPrincipalExtensions` (`GetUsername`/`GetDisplayName`/`GetCurrentUsernameAsync`) resolving claims through the `JwtTokenService` constants; the ten copy-pasted `GetCurrentUserAsync` helpers and the `NavMenu`/`Dashboard` `DisplayName` lookups now delegate to it, eliminating every magic-string claim literal. ### CentralUI-025 — `SessionExpiry` polling/redirect path has no test coverage @@ -1189,7 +1189,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `tests/ScadaLink.CentralUI.Tests/Auth/SessionExpiryPolicyTests.cs`; `src/ScadaLink.CentralUI/Components/Shared/SessionExpiry.razor` | **Description** @@ -1215,4 +1215,4 @@ also forces the CentralUI-020 fix. **Resolution** -_Unresolved._ +2026-05-17 — added `SessionExpiryComponentTests` (bUnit): an expired ping (401) redirects to `/login`, a live ping (200) and a transient failure (status 0) do not, and on the `/login` route the component neither pings nor redirects; also added `AuthPingEndpointTests` covering the `/auth/ping` endpoint contract. diff --git a/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs b/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs index 0bb09f0..db1f09b 100644 --- a/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs +++ b/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs @@ -134,9 +134,35 @@ public static class AuthEndpoints context.Response.Redirect("/login"); }); + // CentralUI-020: liveness probe for the client-side idle-logout check. + // The Blazor circuit's CookieAuthenticationStateProvider serves a frozen + // constructor-time principal (CentralUI-004), so a circuit can never + // observe a server-side cookie expiry by polling the auth state. + // SessionExpiry instead polls this endpoint via fetch(): being a normal + // HTTP request, the cookie middleware re-validates (and slides) the + // cookie on every hit. It deliberately does NOT use RequireAuthorization + // — that would make the middleware answer a lapsed request with a 302 to + // /login, which fetch() follows transparently and reads as a 200 login + // page. Allowing anonymous access and returning 200/401 ourselves gives + // the client an unambiguous expiry signal. + endpoints.MapGet("/auth/ping", HandlePing); + return endpoints; } + /// + /// Handler for GET /auth/ping. Returns 200 while the caller's + /// cookie session is still valid and 401 once it has lapsed + /// server-side. See CentralUI-020. + /// + public static Task HandlePing(HttpContext context) + { + context.Response.StatusCode = context.User.Identity?.IsAuthenticated == true + ? StatusCodes.Status200OK + : StatusCodes.Status401Unauthorized; + return Task.CompletedTask; + } + /// /// Builds the for the login sign-in. /// CentralUI-005: deliberately does not set . diff --git a/src/ScadaLink.CentralUI/Auth/ClaimsPrincipalExtensions.cs b/src/ScadaLink.CentralUI/Auth/ClaimsPrincipalExtensions.cs new file mode 100644 index 0000000..4cd3ec5 --- /dev/null +++ b/src/ScadaLink.CentralUI/Auth/ClaimsPrincipalExtensions.cs @@ -0,0 +1,43 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Components.Authorization; +using ScadaLink.Security; + +namespace ScadaLink.CentralUI.Auth; + +/// +/// Claim-lookup helpers for the Central UI. CentralUI-024: claim types are owned +/// by (the single source of truth). These helpers +/// resolve them through the JwtTokenService constants so a rename there +/// propagates here instead of silently breaking ten copy-pasted call sites. +/// +public static class ClaimsPrincipalExtensions +{ + /// Fallback returned when no username claim is present. + public const string UnknownUser = "unknown"; + + /// + /// The audit username for , or + /// when the claim is absent. + /// + public static string GetUsername(this ClaimsPrincipal principal) + => principal.FindFirst(JwtTokenService.UsernameClaimType)?.Value ?? UnknownUser; + + /// + /// The display name for , or null when + /// the claim is absent. + /// + public static string? GetDisplayName(this ClaimsPrincipal principal) + => principal.FindFirst(JwtTokenService.DisplayNameClaimType)?.Value; + + /// + /// Resolves the current user's audit username from the auth state provider. + /// Replaces the GetCurrentUserAsync helper that was copy-pasted into + /// ten components (CentralUI-024). + /// + public static async Task GetCurrentUsernameAsync( + this AuthenticationStateProvider authStateProvider) + { + var authState = await authStateProvider.GetAuthenticationStateAsync(); + return authState.User.GetUsername(); + } +} diff --git a/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor b/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor index ac0b8a4..0e35078 100644 --- a/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor +++ b/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor @@ -99,7 +99,8 @@
- @context.User.FindFirst("DisplayName")?.Value + @* CentralUI-024: claim type resolved via JwtTokenService. *@ + @context.User.GetDisplayName()
@* CentralUI-017: logout is a state-changing POST and is CSRF-protected — the antiforgery token is required. *@ diff --git a/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor b/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor index 9114921..5adee63 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor @@ -160,11 +160,10 @@
@code { - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); private List _sites = new(); private Dictionary> _siteConnections = new(); diff --git a/src/ScadaLink.CentralUI/Components/Pages/Dashboard.razor b/src/ScadaLink.CentralUI/Components/Pages/Dashboard.razor index ac155b8..ce24f60 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Dashboard.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Dashboard.razor @@ -11,7 +11,8 @@ - Signed in as @context.User.FindFirst("DisplayName")?.Value + @* CentralUI-024: claim type resolved via JwtTokenService. *@ + Signed in as @context.User.GetDisplayName() diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor index 074bc72..3435866 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor @@ -401,23 +401,7 @@ { var session = await DebugStreamService.StartStreamAsync( _selectedInstanceId, - onEvent: evt => - { - // CentralUI-009: the component may have been disposed while - // this event was in flight on the Akka/gRPC thread. - if (_disposed) return; - switch (evt) - { - case AttributeValueChanged av: - UpsertWithCap(_attributeValues, av.AttributeName, av); - SafeInvokeStateHasChanged(); - break; - case AlarmStateChanged al: - UpsertWithCap(_alarmStates, al.AlarmName, al); - SafeInvokeStateHasChanged(); - break; - } - }, + onEvent: HandleStreamEvent, onTerminated: () => { _connected = false; @@ -503,10 +487,51 @@ _alarmStates.Clear(); } + /// + /// Handles one debug-stream event. The callback is invoked on an Akka/gRPC + /// thread, but / are + /// instances also enumerated by the + /// render thread via / + /// . Dictionary is not thread-safe + /// (CentralUI-021): a write racing an enumeration can throw or corrupt the + /// buckets. The mutation () is therefore + /// marshalled onto the renderer's dispatcher via + /// so every access to the dictionaries — read and write — happens on the + /// render thread. + /// + private void HandleStreamEvent(object evt) + { + // CentralUI-009: the component may have been disposed while this event + // was in flight on the Akka/gRPC thread. + if (_disposed) return; + _ = SafeInvokeAsync(() => + { + if (_disposed) return; + switch (evt) + { + case AttributeValueChanged av: + UpsertWithCap(_attributeValues, av.AttributeName, av); + break; + case AlarmStateChanged al: + UpsertWithCap(_alarmStates, al.AlarmName, al); + break; + default: + return; + } + StateHasChanged(); + }); + } + /// /// Replace or insert a value keyed by name, then trim the oldest entries /// (queue-style) so the table size never exceeds MaxRows. Dictionary /// preserves insertion order, so the first key is always the oldest. + /// + /// Must be called on the render thread only (CentralUI-021) — see + /// . The cap-trim loop is in the same + /// critical section as the upsert so the dictionary is never observed + /// over-capacity. + /// /// private static void UpsertWithCap(Dictionary map, string key, T value) { @@ -577,8 +602,6 @@ } } - private void SafeInvokeStateHasChanged() => _ = SafeInvokeAsync(StateHasChanged); - public void Dispose() { // CentralUI-009: mark disposed first so any in-flight stream callback diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor index bb184af..1c701ee 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor @@ -204,6 +204,17 @@ private int _totalPages; private const int PageSize = 25; + // CentralUI-022: IDeploymentStatusNotifier is a process singleton that + // raises StatusChanged on the DeploymentManager service thread. Dispose() + // unsubscribes, but the notifier can read its subscriber list and begin + // invoking OnDeploymentStatusChanged just before this component is disposed. + // The handler then runs against a disposed component and InvokeAsync throws + // ObjectDisposedException as an unobserved fire-and-forget task exception. + // This flag (set first in Dispose()) makes a racing callback no-op, and the + // dispatch swallows the residual ObjectDisposedException — mirroring the + // DebugView (CentralUI-009) and ToastNotification (CentralUI-010) guards. + private volatile bool _disposed; + // CentralUI-006: deployment status updates are push-based, not polled. // DeploymentManager raises IDeploymentStatusNotifier.StatusChanged on every // deployment-record status write; this page subscribes to it and reloads, @@ -220,12 +231,34 @@ private void OnDeploymentStatusChanged(ScadaLink.DeploymentManager.DeploymentStatusChange change) { - if (!_autoRefresh) return; - _ = InvokeAsync(async () => + // CentralUI-022: a callback racing disposal must not touch the component. + if (_disposed || !_autoRefresh) return; + _ = DispatchReloadAsync(); + } + + /// + /// Reloads the deployment table on the renderer's dispatcher, guarded + /// against the component being disposed mid-flight (CentralUI-022): + /// InvokeAsync throws once the + /// circuit is gone, and this handler runs fire-and-forget so that exception + /// would otherwise go unobserved on the DeploymentManager thread. + /// + private async Task DispatchReloadAsync() + { + if (_disposed) return; + try { - await LoadDataAsync(); - StateHasChanged(); - }); + await InvokeAsync(async () => + { + if (_disposed) return; + await LoadDataAsync(); + StateHasChanged(); + }); + } + catch (ObjectDisposedException) + { + // Component disposed between the guard and the dispatch — ignore. + } } private void ToggleAutoRefresh() @@ -316,8 +349,10 @@ public void Dispose() { - // Unsubscribe so a status change after the circuit is gone does not - // touch a disposed component (the notifier is a process singleton). + // CentralUI-022: set the guard first so a callback already in flight on + // the DeploymentManager thread no-ops, then unsubscribe so no further + // status change reaches this disposed component. + _disposed = true; DeploymentStatusNotifier.StatusChanged -= OnDeploymentStatusChanged; } } diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor index df20c6c..e3af35f 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor @@ -438,11 +438,10 @@ private void GoBack() => NavigationManager.NavigateTo("/deployment/topology"); - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); // ── Bindings ──────────────────────────────────────────── diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor index 035b5a3..91869d3 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor @@ -157,9 +157,8 @@ private void GoBack() => NavigationManager.NavigateTo("/deployment/topology"); - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); } diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor index 729ea22..0e61f21 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor @@ -921,9 +921,8 @@ } } - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); } diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScriptForm.razor b/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScriptForm.razor index 6b3f6e7..17761f6 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScriptForm.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScriptForm.razor @@ -172,11 +172,10 @@ private ScriptAnalysis.SandboxRunResult? _runResult; private CancellationTokenSource? _runCts; - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); protected override async Task OnInitializedAsync() { diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScripts.razor b/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScripts.razor index be65534..d00f499 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScripts.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/SharedScripts.razor @@ -101,11 +101,10 @@
@code { - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); private List _scripts = new(); private bool _loading = true; diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateCreate.razor b/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateCreate.razor index 5b9391c..49e647f 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateCreate.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateCreate.razor @@ -119,9 +119,8 @@ NavigationManager.NavigateTo("/design/templates"); } - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); } diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateEdit.razor b/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateEdit.razor index dc445fc..ac6a068 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateEdit.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/TemplateEdit.razor @@ -218,11 +218,10 @@ NavigationManager.NavigateTo("/design/templates"); } - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); private RenderFragment RenderTemplateDetail() => __builder => { diff --git a/src/ScadaLink.CentralUI/Components/Pages/Design/Templates.razor b/src/ScadaLink.CentralUI/Components/Pages/Design/Templates.razor index 6b794af..431d8f8 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Design/Templates.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Design/Templates.razor @@ -99,11 +99,10 @@ @code { - private async Task GetCurrentUserAsync() - { - var authState = await AuthStateProvider.GetAuthenticationStateAsync(); - return authState.User.FindFirst("Username")?.Value ?? "unknown"; - } + // CentralUI-024: delegates to the shared helper so the claim type stays + // resolved through JwtTokenService rather than a duplicated magic string. + private Task GetCurrentUserAsync() + => AuthStateProvider.GetCurrentUsernameAsync(); private List