From d7b275fc9be14c1d4615d71a1931fb1fa96e9b08 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 22:04:21 -0400 Subject: [PATCH] =?UTF-8?q?fix(central-ui):=20resolve=20CentralUI-015..019?= =?UTF-8?q?=20=E2=80=94=20pager=20windowing,=20logout=20CSRF,=20narrowed?= =?UTF-8?q?=20catch=20blocks,=20coverage;=20CentralUI-015=20re-triaged=20W?= =?UTF-8?q?on't=20Fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/CentralUI/findings.md | 110 ++++++++++++++-- src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs | 13 +- .../Components/Layout/NavMenu.razor | 3 + .../Components/Pages/Admin/Sites.razor | 10 +- .../Pages/Deployment/Deployments.razor | 19 ++- .../Components/Shared/DataTable.razor | 19 ++- .../Components/Shared/DialogService.cs | 22 +++- .../Components/Shared/MonacoEditor.razor | 51 ++++++-- .../Components/Shared/PagerWindow.cs | 57 +++++++++ .../Components/Shared/TreeView.razor | 47 +++++-- src/ScadaLink.CentralUI/_Imports.razor | 1 + .../Auth/AuthEndpointsCsrfTests.cs | 76 ++++++++++++ .../Shared/DataTablePagerTests.cs | 68 ++++++++++ .../Shared/DialogServiceThreadingTests.cs | 117 ++++++++++++++++++ .../Shared/MonacoEditorLoggingTests.cs | 77 ++++++++++++ .../Shared/PagerWindowTests.cs | 67 ++++++++++ .../Shared/TreeViewStorageResilienceTests.cs | 62 ++++++++++ .../TopologyPageTests.cs | 3 + 18 files changed, 772 insertions(+), 50 deletions(-) create mode 100644 src/ScadaLink.CentralUI/Components/Shared/PagerWindow.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Auth/AuthEndpointsCsrfTests.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Shared/DataTablePagerTests.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Shared/DialogServiceThreadingTests.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Shared/MonacoEditorLoggingTests.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Shared/PagerWindowTests.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Shared/TreeViewStorageResilienceTests.cs diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 6c25ba8..8bc7c01 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 7 | +| Open findings | 2 | ## Summary @@ -664,7 +664,7 @@ cannot silently regress. |--|--| | Severity | Low | | Category | Concurrency & thread safety | -| Status | Open | +| Status | Won't Fix (re-triaged 2026-05-16 — premise incorrect; see Resolution) | | Location | `src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs:24`; `src/ScadaLink.CentralUI/Components/Shared/DialogService.cs:18-69` | **Description** @@ -684,7 +684,25 @@ call sites for off-thread state mutation. **Resolution** -_Unresolved._ +Won't Fix — **re-triaged 2026-05-16, the finding's premise is incorrect.** The +finding claims `ContinueWith(..., TaskScheduler.Default)` makes an awaiting +caller resume on a thread-pool thread. It does not. `TaskScheduler.Default` on +`ContinueWith` only governs where the trivial *projection lambda* runs (inside +`DialogService`); it has no effect on where the *caller* resumes. An `await` +always captures and resumes on the awaiter's own `SynchronizationContext` — for +a Blazor event-handler caller, that is the renderer's dispatcher — regardless of +where the awaited task completes. This was verified directly: +`DialogServiceThreadingTests.ConfirmAsync_AwaiterResumesOnItsCapturedSyncContext` +pins that the continuation posts back to the caller's captured context, and the +test **passes against both** the original `ContinueWith` form and the current +code, confirming there was never an off-render-thread resume to fix. The +`DialogService` was nonetheless cleaned up opportunistically — the explicit +`ContinueWith(..., TaskScheduler.Default)` projections were replaced with an +inline typed projection (`Project`), removing a needless thread-pool +hop and making the flow easier to read — but that is a quality tidy-up, not a +bug fix. Characterization tests `DialogServiceThreadingTests` (4 tests) pin the +sync-context behaviour and the confirm/prompt/cancel resolution contract so the +service cannot silently regress. ### CentralUI-016 — Pagers render one button per page with no windowing @@ -692,7 +710,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Shared/DataTable.razor:62-68`; `src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor:167-173` | **Description** @@ -710,7 +728,19 @@ large lists to a "load more" / numeric jump input. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: both `DataTable` and +`Deployments` looped `for i = 1..totalPages` and emitted one numbered `
  • ` +button per page — 200 buttons for a 5000-row dataset at page size 25. Added a +pure `PagerWindow.Build(currentPage, totalPages)` helper +(`Components/Shared/PagerWindow.cs`) that returns a bounded window — always the +first and last page plus a small range around the current page, with a `0` +sentinel marking an elided gap (rendered as a disabled `…`). Both +paginators now iterate `PagerWindow.Build(...)` instead of the full range; +small datasets (<= 9 pages) still render every page so nothing is hidden +needlessly. Regression tests: `DataTablePagerTests` (3 bUnit tests — proves the +windowed pager renders <= 12 numbered buttons for 200 pages where the pre-fix +code rendered 200, still renders all pages for a small dataset, and always +includes first/last) and `PagerWindowTests` (6 tests pinning the helper logic). ### CentralUI-017 — `/auth/logout` POST disables antiforgery, enabling logout CSRF @@ -718,7 +748,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs:127-138` | **Description** @@ -737,7 +767,21 @@ can include the antiforgery token), and remove or protect the state-changing **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed: `POST /auth/logout` called +`.DisableAntiforgery()` and a plain `GET /logout` route also signed the user +out — either was triggerable cross-site (`` or an +auto-submitting form) to forcibly log a user out. The `.DisableAntiforgery()` +call was removed from `POST /auth/logout` so it now requires a valid +antiforgery token, and the `NavMenu` sign-out form was given an +`` so the legitimate logout still works. The state-changing +`GET /logout` route was deleted outright (a state-changing GET is itself a CSRF +vector). `POST /auth/login` intentionally keeps `.DisableAntiforgery()` — it is +a pre-auth endpoint where there is no session/token yet. Regression tests +`AuthEndpointsCsrfTests` (3 tests, inspecting the mapped endpoints' metadata): +`PostAuthLogout_DoesNotDisableAntiforgery` and +`GetLogout_StateChangingRoute_IsRemoved` fail against the pre-fix code and pass +after; `PostAuthLogin_StillDisablesAntiforgery_PreAuthIsAcceptable` guards that +the pre-auth login exemption was not over-corrected. ### CentralUI-018 — Broad `catch {}` blocks swallow JS interop and storage errors silently @@ -745,7 +789,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Shared/MonacoEditor.razor:116-118,123,142,164,170,176,182,189`; `src/ScadaLink.CentralUI/Components/Shared/TreeView.razor:129,139`; `src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor:316-319` | **Description** @@ -766,7 +810,30 @@ Catch the specific expected exception type (e.g. `JSDisconnectedException`, **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Confirmed all three locations. +(1) **TreeView** — the storage-restore `JsonSerializer.Deserialize>` +was outside any try block, so a corrupt `treeviewStorage` payload threw an +uncaught `JsonException` out of `OnAfterRenderAsync`. The deserialize is now +wrapped in a `try/catch (JsonException)` that treats an unparseable payload as +"no prior state" (falling back to `InitiallyExpanded`); the `treeviewStorage.load` +interop call is guarded for `JSDisconnectedException`; and the context-menu +`FocusAsync` catch was narrowed from a bare `catch` to the specific expected +types (`JSException`/`JSDisconnectedException`/`InvalidOperationException`). +(2) **MonacoEditor** — every JS interop call had a bare `catch { }`. The +component now injects `ILogger`; `createEditor` distinguishes the +expected prerender (`InvalidOperationException`) and disconnect +(`JSDisconnectedException`) cases — silent — from a genuine `JSException`, which +is logged via `LogError`. The other six interop calls route through a new +`SafeInvokeAsync` helper that swallows `JSDisconnectedException` but logs a real +`JSException` via `LogWarning`. (3) **Sites.CopyAsync** — the bare `catch` was +split into a silent `JSDisconnectedException` arm and a `JSException` arm that +logs via a newly injected `ILogger` before showing the error toast. +Regression tests: `TreeViewStorageResilienceTests` (2 tests — a corrupt and a +wrong-shaped payload no longer throw and the tree still renders; both fail +against the pre-fix unguarded `Deserialize`) and `MonacoEditorLoggingTests` +(2 tests — a genuine `JSException` during init is logged, verified to fail +against the pre-fix bare `catch {}`; a prerender `InvalidOperationException` is +not logged). ### CentralUI-019 — Sparse unit-test coverage for a large module; critical paths untested @@ -774,7 +841,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `tests/ScadaLink.CentralUI.Tests/` | **Description** @@ -798,4 +865,25 @@ findings. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). The coverage gap has been closed across +the cumulative fixes for CentralUI-001 .. 018 — every critical path the finding +named now has tests. Sandbox-run / forbidden-API rejection: +`ScriptAnalysisServiceTests`, `ScriptAnalysisAsyncResolveTests`, +`TestRunWarningTests` (from CentralUI-001/013/014). Auth bridge: +`CookieAuthenticationStateProviderTests`, `SiteScopeServiceTests`, +`AuthEndpointsCsrfTests` (from CentralUI-002/004/017). Dialog resolution: +`DiffDialogTests` and the new `DialogServiceThreadingTests` (4 tests pinning +`ConfirmAsync`/`PromptAsync` sync-context and confirm/prompt/cancel resolution +semantics). DebugView lifecycle: `DebugViewDisposalTests` (from CentralUI-009). +Toast/timer disposal: `ToastNotificationTests` (from CentralUI-010). +This batch also added `BrowserTimeTests`, `MonitoringAuthorizationTests`, +`SitesPageTests`, `DataTablePagerTests` + `PagerWindowTests`, +`TreeViewStorageResilienceTests`, and `MonacoEditorLoggingTests`. The +`tests/ScadaLink.CentralUI.Tests` suite is green at 251 tests. Remaining +untested paths are low-risk render-only pages; the Critical/High/Medium paths +the finding prioritised are all now covered, so the finding is considered +resolved. (Note: `TopologyPageTests`'s DI setup was also updated this session — +it was failing on the baseline because `DeploymentService` had gained a +`DiffService` constructor dependency from a DeploymentManager contract change +that the test fixture had not been updated for; `DiffService` is now registered +in the fixture.) diff --git a/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs b/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs index 47f0451..2cd48d1 100644 --- a/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs +++ b/src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs @@ -124,17 +124,16 @@ public static class AuthEndpoints }); }).DisableAntiforgery(); + // Logout is a state-changing authenticated action (CentralUI-017): it + // keeps antiforgery validation enabled so it cannot be triggered + // cross-site. The NavMenu sign-out form includes the antiforgery token + // (rendered by the component). There is deliberately + // no GET /logout route — a state-changing GET is itself a CSRF vector + // (an would forcibly log a user out). endpoints.MapPost("/auth/logout", async (HttpContext context) => { await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); context.Response.Redirect("/login"); - }).DisableAntiforgery(); - - // GET /logout — allows direct navigation to logout (redirects to login after sign-out) - endpoints.MapGet("/logout", async (HttpContext context) => - { - await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); - return Results.Redirect("/login"); }); return endpoints; diff --git a/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor b/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor index e64f237..ac0b8a4 100644 --- a/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor +++ b/src/ScadaLink.CentralUI/Components/Layout/NavMenu.razor @@ -101,6 +101,9 @@
    @context.User.FindFirst("DisplayName")?.Value
    + @* 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 3fbfcbb..9114921 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Admin/Sites.razor @@ -13,6 +13,7 @@ @inject NavigationManager NavigationManager @inject IJSRuntime JS @inject IDialogService Dialog +@inject Microsoft.Extensions.Logging.ILogger Logger
    @@ -310,8 +311,15 @@ await JS.InvokeVoidAsync("navigator.clipboard.writeText", text); _toast.ShowSuccess("Copied to clipboard."); } - catch + catch (Microsoft.JSInterop.JSDisconnectedException) { + // Circuit gone — the user has navigated away; nothing to surface. + } + catch (Microsoft.JSInterop.JSException ex) + { + // CentralUI-018: a real clipboard failure (e.g. permission denied) + // is logged, not silently swallowed. + Logger.LogWarning(ex, "Clipboard copy failed."); _toast.ShowError("Copy failed."); } } diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor index 6e1b3f1..327f402 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor @@ -165,12 +165,21 @@
    • - @for (int i = 1; i <= _totalPages; i++) + @foreach (var page in ScadaLink.CentralUI.Components.Shared.PagerWindow.Build(_currentPage, _totalPages)) { - var page = i; -
  • - -
    • + if (page == 0) + { +
  • + … +
    • + } + else + { + var p = page; +
  • + +
    • + } }
  • diff --git a/src/ScadaLink.CentralUI/Components/Shared/DataTable.razor b/src/ScadaLink.CentralUI/Components/Shared/DataTable.razor index 361e02a..081f4a9 100644 --- a/src/ScadaLink.CentralUI/Components/Shared/DataTable.razor +++ b/src/ScadaLink.CentralUI/Components/Shared/DataTable.razor @@ -59,12 +59,21 @@ aria-disabled="@((_currentPage <= 1).ToString().ToLowerInvariant())" @onclick="() => GoToPage(_currentPage - 1)">Previous
    • - @for (int i = 1; i <= _totalPages; i++) + @foreach (var page in PagerWindow.Build(_currentPage, _totalPages)) { - var page = i; -
  • - -
    • + if (page == 0) + { +
  • + … +
    • + } + else + { + var p = page; +
  • + +
    • + } }