dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(central-ui): resolve CentralUI-002/003/004 — site-scope enforcement, per-circuit console capture, cached auth state

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 19:33:09 -04:00
parent 5a08b04535
commit 87f14c190a
17 changed files with 693 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
code-reviews/CentralUI/findings.md
View File

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 18 |
| Open findings | 15 |
## Summary
@@ -108,7 +108,7 @@ the commit whose message references `CentralUI-001`.
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CentralUI/Auth/AuthEndpoints.cs:63-69`; `src/ScadaLink.CentralUI/Components/Pages/Deployment/*.razor` |
**Description**
@@ -134,7 +134,20 @@ keep this consistent.
**Resolution**
_Unresolved._
Resolved 2026-05-16. Confirmed: the `SiteId` claim was written at login
(`AuthEndpoints`, `RoleMapper`) but never read by any CentralUI page — site
scoping was unenforced. Added a scoped `SiteScopeService` (`Auth/SiteScopeService.cs`)
that reads the current circuit's `SiteId` claims and exposes `IsSystemWideAsync`,
`PermittedSiteIdsAsync`, `FilterSitesAsync`, and `IsSiteAllowedAsync` (absence of
claims = system-wide, matching `SiteScopeAuthorizationHandler`). All seven
Deployment/Monitoring pages now consume it: `Topology`, `DebugView`,
`InstanceCreate`, `Deployments` filter their site/instance lists; `InstanceConfigure`
rejects direct navigation to an instance on a non-permitted site; `DebugView`,
`InstanceCreate`, and `ParkedMessages` re-check the claim server-side before any
mutating/streaming command. Regression tests: `SiteScopeServiceTests` (6 tests
pinning the helper logic) and `TopologyPageTests.SiteScoping_ScopedDeploymentUser_OnlySeesPermittedSites`
/ `SiteScoping_SystemWideDeploymentUser_SeesAllSites`. Fixed by the commit whose
message references `CentralUI-002`.
### CentralUI-003 — `Console.SetOut`/`SetError` mutates process-global state across concurrent circuits
@@ -142,7 +155,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Concurrency & thread safety |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:359-423` |
**Description**
@@ -166,7 +179,20 @@ the correct fix.
**Resolution**
_Unresolved._
Resolved 2026-05-16. Confirmed: `RunInSandboxAsync` redirected the process-global
`Console.Out`/`Console.Error` per call and restored them in `finally`, so a
concurrent run's `finally` could restore the writer while another run was still
executing — the long run silently lost output (reproduced by the regression
test, 74 of 80 expected lines captured). Added `SandboxConsoleCapture`, a routing
`TextWriter` installed into `Console.Out`/`Console.Error` exactly once for the
process; each run pushes its own `StringWriter` onto an `AsyncLocal` capture
scope via `BeginCapture`, so writes are routed per logical call-tree with no
per-run mutation of global `Console` state. `RunInSandboxAsync` now opens the
scope with `using` declarations instead of calling `Console.SetOut`. Regression
tests `RunInSandbox_CapturesConsoleOutput` and
`RunInSandbox_ConcurrentRuns_DoNotCrossContaminateConsoleOutput` fail against the
pre-fix code and pass after. Fixed by the commit whose message references
`CentralUI-003`.
### CentralUI-004 — `CookieAuthenticationStateProvider` reads `HttpContext` for the life of the circuit
@@ -174,7 +200,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.CentralUI/Auth/CookieAuthenticationStateProvider.cs:22-28` |
**Description**
@@ -202,7 +228,19 @@ circuit is established.
**Resolution**
_Unresolved._
Resolved 2026-05-16. Confirmed: `GetAuthenticationStateAsync` read
`_httpContextAccessor.HttpContext?.User` on every call; the provider is
registered `Scoped`, so it is constructed within the initial HTTP request's DI
scope while `HttpContext` is still valid, but every later call (an
`<AuthorizeView>` re-evaluating, or a page calling it directly) over the
long-lived SignalR circuit saw `HttpContext == null` and returned an anonymous
principal. The provider now snapshots the principal once in the constructor into
a cached `Task<AuthenticationState>` and serves that for the life of the
circuit, never touching `IHttpContextAccessor` again. Regression tests
`CookieAuthenticationStateProviderTests.GetAuthenticationStateAsync_StillReturnsUser_AfterHttpContextIsGone`
and `..._IsStableAcrossCalls_IgnoringStaleForeignContext` fail against the
pre-fix code (they would see an anonymous / foreign principal) and pass after.
Fixed by the commit whose message references `CentralUI-004`.
### CentralUI-005 — Session expiry implementation diverges from the documented policy