From a02c0ffe363d02592944ae2a37b777d2d056239a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 12:29:36 -0400 Subject: [PATCH] docs(code-reviews): record Admin-013 (SignalR hub clients cannot authenticate) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the post-review finding discovered during browser smoke-testing: the Admin-003 hub hardening was incomplete — the server-side Blazor HubConnection clients had no way to authenticate, so hub negotiate 401'd and four cluster pages threw unhandled 500s. Logged as Admin-013 (High, Error handling & resilience), Status Resolved, fixed by commits f254539 + 8d5dbb4. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Admin/findings.md | 17 ++++++++++++++++- code-reviews/README.md | 3 ++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/code-reviews/Admin/findings.md b/code-reviews/Admin/findings.md index 45d8050..5ed20d2 100644 --- a/code-reviews/Admin/findings.md +++ b/code-reviews/Admin/findings.md @@ -16,7 +16,7 @@ | 1 | Correctness & logic bugs | Admin-005 | | 2 | OtOpcUa conventions | Admin-010 | | 3 | Concurrency & thread safety | Admin-011 | -| 4 | Error handling & resilience | Admin-008 | +| 4 | Error handling & resilience | Admin-008, Admin-013 | | 5 | Security | Admin-001, Admin-002, Admin-003, Admin-004, Admin-006 | | 6 | Performance & resource management | No issues found | | 7 | Design-document adherence | Admin-007, Admin-012 | @@ -205,3 +205,18 @@ **Recommendation:** Reconcile with the design: drop `EquipmentId` from `RequiredColumns` and the `EquipmentCsvRow` shape (deriving it from `EquipmentUuid` at finalize time), or — if accepting it is a deliberate reversal — update `admin-ui.md` and the decision log so the two agree. **Resolution:** _(open)_ + +### Admin-013 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Error handling & resilience | +| Location | `Components/Pages/Clusters/ClusterDetail.razor:180-197`, `Components/Pages/Clusters/AclsTab.razor`, `Components/Pages/Clusters/RedundancyTab.razor`, `Components/Pages/RoleGrants.razor`, `Components/Pages/Hosts.razor`, `Components/Pages/ScriptLog.razor`, `Program.cs:157-159` | +| Status | Resolved | + +**Description:** The Admin-003 fix gated all three SignalR hubs with `[Authorize]` plus `.RequireAuthorization()`, but the six pages that open a client `HubConnection` to those hubs were never updated to authenticate. A server-side Blazor `HubConnection` runs inside the interactive circuit and has no access to the browser's HttpOnly `OtOpcUa.Admin` auth cookie, so the hub `negotiate` request returns 401. Four pages (`ClusterDetail`, `AclsTab`, `RedundancyTab`, `RoleGrants`) called `HubConnection.StartAsync()` with no `try`/`catch`, so the 401 surfaced as an unhandled exception — a full HTTP 500 page for the prerendered `/clusters/{ClusterId}` route (the core cluster-config surface) and a faulted circuit for the others. `Hosts` and `ScriptLog` already wrapped the connect in `try`/`catch`, so they did not crash, but the SignalR live-update feature was non-functional Admin-wide regardless. The Admin-003 hardening was therefore incomplete: it secured the hub server side without giving the in-process clients any way to present credentials. Discovered during a post-review browser smoke test of `/clusters/cluster-dev`. + +**Recommendation:** Two parts. (1) Stop the crash: guard every `HubConnection.StartAsync()` in `try`/`catch`, matching the best-effort pattern already documented in `Hosts.razor` — a hub hiccup must degrade live updates, not fault the page. (2) Restore the feature: give the hub clients a real credential. Cookie forwarding is not viable (the HttpOnly cookie is unreachable from the interactive circuit and persisting it into page state would leak it), so add a token scheme — mint a short-lived token for the circuit's authenticated user and supply it via `HttpConnectionOptions.AccessTokenProvider`, with a matching server-side authentication handler on the hub endpoints. + +**Resolution:** Resolved 2026-05-22 — (1) `StartAsync`/`SendAsync` wrapped in `try`/`catch` on `ClusterDetail`, `AclsTab`, `RedundancyTab` and `RoleGrants` so a hub failure degrades gracefully. (2) Added a bearer-token auth path: `HubTokenService` mints/validates short-lived tokens using ASP.NET Core Data Protection (no signing-key management, no new packages); `HubTokenAuthenticationHandler` is a custom `HubToken` scheme reading the token from the `Authorization: Bearer` header (negotiate) or the `access_token` query parameter (WebSocket upgrade); the `HubClients` authorization policy runs both the cookie and `HubToken` schemes and is applied via `RequireAuthorization("HubClients")` on all three `MapHub` calls; `AdminHubConnectionFactory` builds connections with an `AccessTokenProvider` that re-mints a token for the circuit's authenticated user on every (re)connect, and all six hub-consuming pages resolve their connections through it. Verified end-to-end in the browser: hub `negotiate` returns 200 and the WebSocket upgrades (101) where it previously 401'd. diff --git a/code-reviews/README.md b/code-reviews/README.md index 1d82d14..df675e1 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -10,7 +10,7 @@ Each module's `findings.md` is the source of truth; this file is generated from | Module | Reviewer | Date | Commit | Status | Open | Total | |---|---|---|---|---|---|---| -| [Admin](Admin/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 3 | 12 | +| [Admin](Admin/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 3 | 13 | | [Analyzers](Analyzers/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 7 | | [Client.CLI](Client.CLI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 8 | 10 | | [Client.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 11 | @@ -220,6 +220,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Admin-003 | High | Resolved | Security | `Program.cs:137-139`, `Hubs/FleetStatusHub.cs:11`, `Hubs/AlertHub.cs:10`, `Hubs/ScriptLogHub.cs:30` | | Admin-004 | High | Resolved | Security | `appsettings.json:3,13-14` | | Admin-005 | High | Resolved | Correctness & logic bugs | `Components/Pages/Login.razor:15,107-110` | +| Admin-013 | High | Resolved | Error handling & resilience | `Components/Pages/Clusters/ClusterDetail.razor:180-197`, `Components/Pages/Clusters/AclsTab.razor`, `Components/Pages/Clusters/RedundancyTab.razor`, `Components/Pages/RoleGrants.razor`, `Components/Pages/Hosts.razor`, `Components/Pages/ScriptLog.razor`, `Program.cs:157-159` | | Client.Shared-005 | High | Resolved | Concurrency & thread safety | `OpcUaClientService.cs:19`, `OpcUaClientService.cs:226-249`, `OpcUaClientService.cs:499-521` | | Client.Shared-006 | High | Resolved | Concurrency & thread safety | `OpcUaClientService.cs:97-100`, `OpcUaClientService.cs:432-497` | | Configuration-001 | High | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs:282` |