fix(central-ui): resolve CentralUI-015..019 — pager windowing, logout CSRF, narrowed catch blocks, coverage; CentralUI-015 re-triaged Won't Fix

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<TResult>`), 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 `<li>`
+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 `&hellip;`). 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 (`<img src="/logout">` 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
+`<AntiforgeryToken />` 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<List<string>>`
+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<MonacoEditor>`; `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<Sites>` 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.)