From 87f14c190a34e5505d212e96269f652d2623d1f7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 19:33:09 -0400 Subject: [PATCH] =?UTF-8?q?fix(central-ui):=20resolve=20CentralUI-002/003/?= =?UTF-8?q?004=20=E2=80=94=20site-scope=20enforcement,=20per-circuit=20con?= =?UTF-8?q?sole=20capture,=20cached=20auth=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/CentralUI/findings.md | 52 +++++++-- .../Auth/CookieAuthenticationStateProvider.cs | 34 ++++-- .../Auth/SiteScopeService.cs | 93 +++++++++++++++ .../Pages/Deployment/DebugView.razor | 13 ++- .../Pages/Deployment/Deployments.razor | 21 +++- .../Pages/Deployment/InstanceConfigure.razor | 12 ++ .../Pages/Deployment/InstanceCreate.razor | 12 +- .../Pages/Deployment/Topology.razor | 10 +- .../Pages/Monitoring/EventLogs.razor | 18 ++- .../Pages/Monitoring/ParkedMessages.razor | 24 +++- .../ScriptAnalysis/SandboxConsoleCapture.cs | 106 ++++++++++++++++++ .../ScriptAnalysis/ScriptAnalysisService.cs | 27 +++-- .../ServiceCollectionExtensions.cs | 4 + .../CookieAuthenticationStateProviderTests.cs | 79 +++++++++++++ .../Auth/SiteScopeServiceTests.cs | 93 +++++++++++++++ .../ScriptAnalysisServiceTests.cs | 85 ++++++++++++++ .../TopologyPageTests.cs | 50 +++++++++ 17 files changed, 693 insertions(+), 40 deletions(-) create mode 100644 src/ScadaLink.CentralUI/Auth/SiteScopeService.cs create mode 100644 src/ScadaLink.CentralUI/ScriptAnalysis/SandboxConsoleCapture.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Auth/CookieAuthenticationStateProviderTests.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/Auth/SiteScopeServiceTests.cs diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 444e226..fef0446 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 | 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 +`` 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` 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 diff --git a/src/ScadaLink.CentralUI/Auth/CookieAuthenticationStateProvider.cs b/src/ScadaLink.CentralUI/Auth/CookieAuthenticationStateProvider.cs index 80e78ec..361cd09 100644 --- a/src/ScadaLink.CentralUI/Auth/CookieAuthenticationStateProvider.cs +++ b/src/ScadaLink.CentralUI/Auth/CookieAuthenticationStateProvider.cs @@ -7,23 +7,37 @@ namespace ScadaLink.CentralUI.Auth; /// /// Bridges ASP.NET Core cookie authentication with Blazor Server's auth state. -/// The cookie middleware has already validated and decrypted the cookie by the time -/// the Blazor circuit is established, so we just read HttpContext.User. +/// +/// The cookie middleware validates and decrypts the cookie during the initial +/// HTTP request that establishes the Blazor circuit. This provider is registered +/// Scoped, so it is constructed within that request's DI scope while +/// is still valid. We snapshot +/// the authenticated principal once in the constructor and serve that +/// snapshot for the lifetime of the circuit. +/// +/// +/// We must NOT read on every +/// call (CentralUI-004): for the +/// lifetime of a long-lived SignalR circuit HttpContext is null +/// (or, worse, a stale/foreign context), so a later re-evaluation — +/// e.g. <AuthorizeView> re-rendering — would otherwise see an +/// unauthenticated principal and render the wrong UI. +/// /// public class CookieAuthenticationStateProvider : ServerAuthenticationStateProvider { - private readonly IHttpContextAccessor _httpContextAccessor; + private readonly Task _circuitAuthState; public CookieAuthenticationStateProvider(IHttpContextAccessor httpContextAccessor) { - _httpContextAccessor = httpContextAccessor; + // Snapshot the principal at circuit-construction time. HttpContext is + // valid here (initial HTTP request) and will not be afterwards. + var user = httpContextAccessor.HttpContext?.User + ?? new ClaimsPrincipal(new ClaimsIdentity()); + + _circuitAuthState = Task.FromResult(new AuthenticationState(user)); } public override Task GetAuthenticationStateAsync() - { - var user = _httpContextAccessor.HttpContext?.User - ?? new ClaimsPrincipal(new ClaimsIdentity()); - - return Task.FromResult(new AuthenticationState(user)); - } + => _circuitAuthState; } diff --git a/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs b/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs new file mode 100644 index 0000000..44ba11d --- /dev/null +++ b/src/ScadaLink.CentralUI/Auth/SiteScopeService.cs @@ -0,0 +1,93 @@ +using Microsoft.AspNetCore.Components.Authorization; +using ScadaLink.Commons.Entities.Sites; +using ScadaLink.Security; + +namespace ScadaLink.CentralUI.Auth; + +/// +/// Resolves the set of sites the current user is permitted to operate on, from +/// the SiteId claims attached at login (CentralUI-002). +/// +/// The design (Component-CentralUI, CLAUDE.md "Security & Auth") makes the +/// Deployment role site-scoped: a Deployment user mapped through an LDAP group +/// with site-scope rules carries one +/// claim per permitted site (the claim value is the integer Site.Id). +/// A Deployment user with no SiteId claim — and any Admin/Design user — is +/// system-wide. +/// +/// +/// Deployment and Monitoring pages must filter every site/instance list through +/// and re-check +/// before any cross-site command, so a scoped user cannot view or act on sites +/// outside their grant. +/// +/// +public sealed class SiteScopeService +{ + private readonly AuthenticationStateProvider _authStateProvider; + private (bool IsSystemWide, IReadOnlySet Sites)? _cached; + + public SiteScopeService(AuthenticationStateProvider authStateProvider) + { + _authStateProvider = authStateProvider; + } + + /// + /// True when the user is not restricted to a site subset (no SiteId + /// claims). System-wide users see and act on every site. + /// + public async Task IsSystemWideAsync() + => (await ResolveAsync()).IsSystemWide; + + /// + /// The set of Site.Id values the user may operate on. Empty for a + /// system-wide user (callers should consult + /// or use the filter/allowed helpers, which already account for that). + /// + public async Task> PermittedSiteIdsAsync() + => (await ResolveAsync()).Sites; + + /// + /// Returns the subset of the user is permitted to + /// see. A system-wide user gets the full list back unchanged. + /// + public async Task> FilterSitesAsync(IEnumerable sites) + { + var (isSystemWide, allowed) = await ResolveAsync(); + if (isSystemWide) + return sites.ToList(); + return sites.Where(s => allowed.Contains(s.Id)).ToList(); + } + + /// + /// True when the user may operate on the site with the given Site.Id. + /// Must be re-checked server-side before any mutating cross-site command. + /// + public async Task IsSiteAllowedAsync(int siteId) + { + var (isSystemWide, allowed) = await ResolveAsync(); + return isSystemWide || allowed.Contains(siteId); + } + + private async Task<(bool IsSystemWide, IReadOnlySet Sites)> ResolveAsync() + { + if (_cached is { } cached) + return cached; + + var state = await _authStateProvider.GetAuthenticationStateAsync(); + var siteClaims = state.User.FindAll(JwtTokenService.SiteIdClaimType); + + var ids = new HashSet(); + foreach (var claim in siteClaims) + { + if (int.TryParse(claim.Value, out var id)) + ids.Add(id); + } + + // No SiteId claims => system-wide. This mirrors SiteScopeAuthorizationHandler: + // absence of scope rules means an unrestricted deployer. + var result = (IsSystemWide: ids.Count == 0, Sites: (IReadOnlySet)ids); + _cached = result; + return result; + } +} diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor index c0a0a4e..38fb1ca 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/DebugView.razor @@ -11,6 +11,7 @@ @attribute [Authorize(Policy = AuthorizationPolicies.RequireDeployment)] @inject ITemplateEngineRepository TemplateEngineRepository @inject ISiteRepository SiteRepository +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @inject DebugStreamService DebugStreamService @inject IJSRuntime JS @implements IDisposable @@ -296,7 +297,9 @@ { try { - _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); + // Site scoping (CentralUI-002): a scoped Deployment user may only + // debug sites they are permitted on. + _sites = await SiteScope.FilterSitesAsync(await SiteRepository.GetAllSitesAsync()); } catch (Exception ex) { @@ -358,6 +361,14 @@ _siteInstances.Clear(); _selectedInstanceId = 0; if (_selectedSiteId == 0) return; + // Site scoping (CentralUI-002): re-check the claim server-side — a query + // string or stale localStorage value could name a site outside the grant. + if (!await SiteScope.IsSiteAllowedAsync(_selectedSiteId)) + { + _selectedSiteId = 0; + _toast.ShowError("You are not permitted to debug instances on that site."); + return; + } try { _siteInstances = (await TemplateEngineRepository.GetInstancesBySiteIdAsync(_selectedSiteId)) diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor index 32f6c76..6e1b3f1 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Deployments.razor @@ -7,6 +7,7 @@ @attribute [Authorize(Policy = AuthorizationPolicies.RequireDeployment)] @inject IDeploymentManagerRepository DeploymentManagerRepository @inject ITemplateEngineRepository TemplateEngineRepository +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @implements IDisposable
@@ -245,13 +246,23 @@ _errorMessage = null; try { - _records = (await DeploymentManagerRepository.GetAllDeploymentRecordsAsync()) - .OrderByDescending(r => r.DeployedAt) - .ToList(); - - // Build instance name lookup + // Build instance lookups first — site scoping (CentralUI-002) filters + // deployment records by the site of their instance. var instances = await TemplateEngineRepository.GetAllInstancesAsync(); _instanceNames = instances.ToDictionary(i => i.Id, i => i.UniqueName); + var instanceSiteIds = instances.ToDictionary(i => i.Id, i => i.SiteId); + + var systemWide = await SiteScope.IsSystemWideAsync(); + var permittedSiteIds = systemWide + ? null + : await SiteScope.PermittedSiteIdsAsync(); + + _records = (await DeploymentManagerRepository.GetAllDeploymentRecordsAsync()) + .Where(r => permittedSiteIds == null + || (instanceSiteIds.TryGetValue(r.InstanceId, out var sid) + && permittedSiteIds.Contains(sid))) + .OrderByDescending(r => r.DeployedAt) + .ToList(); _totalPages = Math.Max(1, (int)Math.Ceiling(_records.Count / (double)PageSize)); if (_currentPage > _totalPages) _currentPage = 1; diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor index 7f8531f..ce46136 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceConfigure.razor @@ -11,6 +11,7 @@ @attribute [Authorize(Policy = AuthorizationPolicies.RequireDeployment)] @inject ITemplateEngineRepository TemplateEngineRepository @inject ISiteRepository SiteRepository +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @inject InstanceService InstanceService @inject IFlatteningPipeline FlatteningPipeline @inject AuthenticationStateProvider AuthStateProvider @@ -377,6 +378,17 @@ return; } + // Site scoping (CentralUI-002): a scoped Deployment user must not be + // able to configure or deploy an instance on a site outside their + // grant by navigating straight to its URL. + if (!await SiteScope.IsSiteAllowedAsync(_instance.SiteId)) + { + _instance = null; + _errorMessage = "You are not permitted to manage instances on this site."; + _loading = false; + return; + } + // Identity var template = await TemplateEngineRepository.GetTemplateByIdAsync(_instance.TemplateId); _templateName = template?.Name ?? $"#{_instance.TemplateId}"; diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor index ea767db..035b5a3 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/InstanceCreate.razor @@ -8,6 +8,7 @@ @attribute [Authorize(Policy = AuthorizationPolicies.RequireDeployment)] @inject ITemplateEngineRepository TemplateEngineRepository @inject ISiteRepository SiteRepository +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @inject InstanceService InstanceService @inject AuthenticationStateProvider AuthStateProvider @inject NavigationManager NavigationManager @@ -93,7 +94,9 @@ try { _templates = (await TemplateEngineRepository.GetAllTemplatesAsync()).ToList(); - _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); + // Site scoping (CentralUI-002): a scoped Deployment user may only + // create instances on sites they are permitted on. + _sites = await SiteScope.FilterSitesAsync(await SiteRepository.GetAllSitesAsync()); _allAreas.Clear(); foreach (var site in _sites) @@ -124,6 +127,13 @@ if (string.IsNullOrWhiteSpace(_createName)) { _formError = "Instance name is required."; return; } if (_createTemplateId == 0) { _formError = "Select a template."; return; } if (_createSiteId == 0) { _formError = "Select a site."; return; } + // Site scoping (CentralUI-002): re-check server-side before the mutating + // command, independent of what the site dropdown was populated with. + if (!await SiteScope.IsSiteAllowedAsync(_createSiteId)) + { + _formError = "You are not permitted to create instances on the selected site."; + return; + } try { diff --git a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor index 1b92367..729ea22 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Deployment/Topology.razor @@ -17,6 +17,7 @@ @inject AreaService AreaService @inject InstanceService InstanceService @inject AuthenticationStateProvider AuthStateProvider +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @inject NavigationManager NavigationManager @inject IJSRuntime JSRuntime @inject IDialogService Dialog @@ -225,8 +226,13 @@ _errorMessage = null; try { - _allInstances = (await TemplateEngineRepository.GetAllInstancesAsync()).ToList(); - _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); + // Site scoping (CentralUI-002): a scoped Deployment user only sees the + // sites — and therefore the areas/instances — they are permitted on. + _sites = await SiteScope.FilterSitesAsync(await SiteRepository.GetAllSitesAsync()); + var permittedSiteIds = _sites.Select(s => s.Id).ToHashSet(); + _allInstances = (await TemplateEngineRepository.GetAllInstancesAsync()) + .Where(i => permittedSiteIds.Contains(i.SiteId)) + .ToList(); _templates = (await TemplateEngineRepository.GetAllTemplatesAsync()).ToList(); _allAreas.Clear(); diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor index 3ea3a2d..b229703 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor @@ -5,6 +5,7 @@ @using ScadaLink.Commons.Messages.RemoteQuery @using ScadaLink.Communication @inject ISiteRepository SiteRepository +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @inject CommunicationService CommunicationService
@@ -212,9 +213,16 @@ protected override async Task OnInitializedAsync() { - _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); + // Site scoping (CentralUI-002): a scoped Deployment user may only query + // event logs for the sites they are permitted on. + _sites = await SiteScope.FilterSitesAsync(await SiteRepository.GetAllSitesAsync()); } + // _sites is already filtered, so membership IS the scope check. + private bool SelectedSiteIsPermitted => + !string.IsNullOrEmpty(_selectedSiteId) + && _sites.Any(s => s.SiteIdentifier == _selectedSiteId); + private async Task Search() { _entries = new(); @@ -237,6 +245,14 @@ { _searching = true; _errorMessage = null; + // Site scoping (CentralUI-002): re-check before querying — the dropdown is + // filtered, but the selection must not be trusted on its own. + if (!SelectedSiteIsPermitted) + { + _errorMessage = "You are not permitted to view event logs for that site."; + _searching = false; + return; + } try { var request = new EventLogQueryRequest( diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor index 97df4b9..a7db209 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/ParkedMessages.razor @@ -6,6 +6,7 @@ @using ScadaLink.Commons.Types.Enums @using ScadaLink.Communication @inject ISiteRepository SiteRepository +@inject ScadaLink.CentralUI.Auth.SiteScopeService SiteScope @inject CommunicationService CommunicationService @inject IJSRuntime JS @inject IDialogService Dialog @@ -360,9 +361,17 @@ protected override async Task OnInitializedAsync() { - _sites = (await SiteRepository.GetAllSitesAsync()).ToList(); + // Site scoping (CentralUI-002): a scoped Deployment user may only inspect + // and act on parked messages for the sites they are permitted on. + _sites = await SiteScope.FilterSitesAsync(await SiteRepository.GetAllSitesAsync()); } + // True only when the currently selected SiteIdentifier is one this user is + // permitted on. _sites is already filtered, so membership IS the scope check. + private bool SelectedSiteIsPermitted => + !string.IsNullOrEmpty(_selectedSiteId) + && _sites.Any(s => s.SiteIdentifier == _selectedSiteId); + private async Task OnSiteChanged(ChangeEventArgs e) { _selectedSiteId = e.Value?.ToString() ?? string.Empty; @@ -393,6 +402,15 @@ { _searching = true; _errorMessage = null; + // Site scoping (CentralUI-002): re-check before querying — the dropdown is + // filtered, but the selection must not be trusted on its own. + if (!SelectedSiteIsPermitted) + { + _errorMessage = "You are not permitted to view parked messages for that site."; + _messages = null; + _searching = false; + return; + } try { var request = new ParkedMessageQueryRequest( @@ -557,6 +575,7 @@ { var ids = _selectedIds.ToList(); if (ids.Count == 0) return; + if (!SelectedSiteIsPermitted) { _toast.ShowError("Not permitted for this site."); return; } var confirmed = await Dialog.ConfirmAsync( "Retry parked messages", @@ -587,6 +606,7 @@ { var ids = _selectedIds.ToList(); if (ids.Count == 0) return; + if (!SelectedSiteIsPermitted) { _toast.ShowError("Not permitted for this site."); return; } var confirmed = await Dialog.ConfirmAsync( "Discard parked messages", @@ -618,6 +638,7 @@ private async Task RetrySingle(ParkedMessageEntry msg) { + if (!SelectedSiteIsPermitted) { _toast.ShowError("Not permitted for this site."); return; } _actionInProgress = true; _activeAction = "Retry"; try @@ -638,6 +659,7 @@ private async Task DiscardSingle(ParkedMessageEntry msg) { + if (!SelectedSiteIsPermitted) { _toast.ShowError("Not permitted for this site."); return false; } var confirmed = await Dialog.ConfirmAsync( "Discard parked message", $"Permanently discard message {ShortId(msg.MessageId)}? This cannot be undone.", diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/SandboxConsoleCapture.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/SandboxConsoleCapture.cs new file mode 100644 index 0000000..97e4d1c --- /dev/null +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/SandboxConsoleCapture.cs @@ -0,0 +1,106 @@ +using System.Text; + +namespace ScadaLink.CentralUI.ScriptAnalysis; + +/// +/// Per-call console capture for the Test Run sandbox. +/// +/// Sandbox scripts use System.Console.WriteLine for ad-hoc output. The +/// sandbox needs to capture that output per execution. Console.Out is, +/// however, process-global: redirecting it with Console.SetOut for +/// the duration of one run corrupts any other run executing concurrently — +/// outputs interleave, and whichever run finishes first restores +/// Console.Out while the others are still writing (CentralUI-003). +/// +/// +/// This writer is installed into Console.Out/Console.Error +/// exactly once (see ) and never removed. Each +/// concurrent run pushes its own buffer onto an +/// scope via ; writes on that run's logical call-tree +/// land in that run's buffer only. Writes made on threads with no active +/// capture scope (i.e. genuine host-process console output) fall through to the +/// original writer. No process-global mutation happens per run. +/// +/// +internal sealed class SandboxConsoleCapture : TextWriter +{ + private static readonly object InstallLock = new(); + private static SandboxConsoleCapture? _outInstance; + private static SandboxConsoleCapture? _errorInstance; + + private readonly TextWriter _fallback; + private readonly AsyncLocal _current = new(); + + private SandboxConsoleCapture(TextWriter fallback) => _fallback = fallback; + + public override Encoding Encoding => _fallback.Encoding; + + /// + /// Installs the routing writers into and + /// once for the process. Idempotent and + /// thread-safe. Subsequent calls return the already-installed instances. + /// + public static (SandboxConsoleCapture Out, SandboxConsoleCapture Error) Install() + { + if (_outInstance != null && _errorInstance != null) + return (_outInstance, _errorInstance); + + lock (InstallLock) + { + if (_outInstance == null) + { + _outInstance = new SandboxConsoleCapture(Console.Out); + Console.SetOut(_outInstance); + } + + if (_errorInstance == null) + { + _errorInstance = new SandboxConsoleCapture(Console.Error); + Console.SetError(_errorInstance); + } + } + + return (_outInstance, _errorInstance); + } + + /// + /// Begins a capture scope on the current logical (async) call-tree. All + /// console writes from this point until the returned scope is disposed are + /// routed into instead of the original writer. + /// The scope is restored on dispose, so nesting and concurrent scopes on + /// other call-trees are unaffected. + /// + public CaptureScope BeginCapture(StringWriter buffer) + { + var previous = _current.Value; + _current.Value = buffer; + return new CaptureScope(this, previous); + } + + public override void Write(char value) => Target.Write(value); + + public override void Write(string? value) => Target.Write(value); + + public override void Write(char[] buffer, int index, int count) => + Target.Write(buffer, index, count); + + public override void WriteLine() => Target.WriteLine(); + + public override void WriteLine(string? value) => Target.WriteLine(value); + + private TextWriter Target => _current.Value ?? _fallback; + + internal readonly struct CaptureScope : IDisposable + { + private readonly SandboxConsoleCapture _owner; + private readonly StringWriter? _previous; + + internal CaptureScope(SandboxConsoleCapture owner, StringWriter? previous) + { + _owner = owner; + _previous = previous; + } + + public void Dispose() => _owner._current.Value = _previous; + } +} diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs index 4176c26..c9f8cf3 100644 --- a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs @@ -165,8 +165,10 @@ public class ScriptAnalysisService /// because a shared script has no template siblings in this context. /// For the SandboxInboundScriptHost surface, every Route call throws /// because cross-site routing needs a deployed site. - /// Console.Out / Console.Error are redirected per-call so writes from - /// the script land in the result. + /// Console.Out / Console.Error are captured per-call via an AsyncLocal + /// scope (see ) so writes from the script + /// land in the result without mutating process-global Console state — two + /// concurrent Test Runs do not interfere with each other. /// public async Task RunInSandboxAsync(SandboxRunRequest request, CancellationToken ct) { @@ -377,16 +379,19 @@ public class ScriptAnalysisService Instance = instanceContext, }; - var originalOut = Console.Out; - var originalError = Console.Error; + // Console capture is routed per-call via an AsyncLocal scope (CentralUI-003). + // Console.Out is process-global, so it must NOT be redirected per run — two + // concurrent Test Runs would interleave output and the first to finish would + // restore Console.Out while the other is still writing. SandboxConsoleCapture + // installs routing writers once and scopes capture to this call-tree only. + var (captureOut, captureError) = SandboxConsoleCapture.Install(); var captured = new StringWriter(); + using var outScope = captureOut.BeginCapture(captured); + using var errorScope = captureError.BeginCapture(captured); var stopwatch = Stopwatch.StartNew(); try { - Console.SetOut(captured); - Console.SetError(captured); - // Run on a thread-pool thread with no SynchronizationContext: a // bound script's Instance.SetAttribute / Attributes[...] block // synchronously on cross-site I/O (the API surface is sync by @@ -437,11 +442,9 @@ public class ScriptAnalysisService $"{inner.GetType().Name}: {inner.Message}", SandboxErrorKind.RuntimeError, stopwatch.ElapsedMilliseconds, null); } - finally - { - Console.SetOut(originalOut); - Console.SetError(originalError); - } + // outScope / errorScope are disposed by their `using` declarations when the + // method returns, restoring the previous capture scope on this call-tree + // without touching process-global Console state. } private static Dictionary ConvertJsonParameters( diff --git a/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs b/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs index e307444..0caf98c 100644 --- a/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs @@ -18,6 +18,10 @@ public static class ServiceCollectionExtensions services.AddScoped(); services.AddCascadingAuthenticationState(); + // Resolves the current user's permitted site set from their SiteId claims + // so Deployment/Monitoring pages can enforce site scoping (CentralUI-002). + services.AddScoped(); + // Centralised dialog service: pages inject IDialogService and a single // in MainLayout renders the active dialog. See // Components/Shared/IDialogService.cs. diff --git a/tests/ScadaLink.CentralUI.Tests/Auth/CookieAuthenticationStateProviderTests.cs b/tests/ScadaLink.CentralUI.Tests/Auth/CookieAuthenticationStateProviderTests.cs new file mode 100644 index 0000000..d992814 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Auth/CookieAuthenticationStateProviderTests.cs @@ -0,0 +1,79 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using ScadaLink.CentralUI.Auth; + +namespace ScadaLink.CentralUI.Tests.Auth; + +/// +/// Regression tests for CentralUI-004. The provider used to read +/// on every call; once the Blazor +/// circuit is established that context is gone, so later re-evaluations saw an +/// unauthenticated principal. The provider must snapshot the principal once at +/// construction (during the initial HTTP request) and serve it for the circuit. +/// +public class CookieAuthenticationStateProviderTests +{ + private static ClaimsPrincipal AuthenticatedUser(string name) + { + var identity = new ClaimsIdentity( + new[] { new Claim(ClaimTypes.Name, name) }, + authenticationType: "TestCookie"); + return new ClaimsPrincipal(identity); + } + + [Fact] + public async Task GetAuthenticationStateAsync_ReturnsAuthenticatedUser_WhenHttpContextPresent() + { + var accessor = new HttpContextAccessor + { + HttpContext = new DefaultHttpContext { User = AuthenticatedUser("alice") } + }; + + var provider = new CookieAuthenticationStateProvider(accessor); + var state = await provider.GetAuthenticationStateAsync(); + + Assert.True(state.User.Identity?.IsAuthenticated); + Assert.Equal("alice", state.User.Identity?.Name); + } + + [Fact] + public async Task GetAuthenticationStateAsync_StillReturnsUser_AfterHttpContextIsGone() + { + // The circuit is built during the HTTP request: HttpContext is valid then. + var accessor = new HttpContextAccessor + { + HttpContext = new DefaultHttpContext { User = AuthenticatedUser("bob") } + }; + var provider = new CookieAuthenticationStateProvider(accessor); + + // After the request completes, IHttpContextAccessor.HttpContext is null for + // the life of the long-lived SignalR circuit. + accessor.HttpContext = null; + + var state = await provider.GetAuthenticationStateAsync(); + + // The pre-fix implementation returned an anonymous principal here. + Assert.True(state.User.Identity?.IsAuthenticated); + Assert.Equal("bob", state.User.Identity?.Name); + } + + [Fact] + public async Task GetAuthenticationStateAsync_IsStableAcrossCalls_IgnoringStaleForeignContext() + { + var accessor = new HttpContextAccessor + { + HttpContext = new DefaultHttpContext { User = AuthenticatedUser("carol") } + }; + var provider = new CookieAuthenticationStateProvider(accessor); + + // A stale/foreign context leaking through the AsyncLocal accessor must NOT + // change what this circuit's provider reports. + accessor.HttpContext = new DefaultHttpContext { User = AuthenticatedUser("intruder") }; + + var first = await provider.GetAuthenticationStateAsync(); + var second = await provider.GetAuthenticationStateAsync(); + + Assert.Equal("carol", first.User.Identity?.Name); + Assert.Equal("carol", second.User.Identity?.Name); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/Auth/SiteScopeServiceTests.cs b/tests/ScadaLink.CentralUI.Tests/Auth/SiteScopeServiceTests.cs new file mode 100644 index 0000000..35579f3 --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Auth/SiteScopeServiceTests.cs @@ -0,0 +1,93 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Components.Authorization; +using ScadaLink.CentralUI.Auth; +using ScadaLink.Commons.Entities.Sites; +using ScadaLink.Security; + +namespace ScadaLink.CentralUI.Tests.Auth; + +/// +/// Regression tests for CentralUI-002. Site-scoped Deployment permissions are +/// written as SiteId claims at login but were never read — Deployment +/// pages listed and acted on every site. is the +/// shared helper that reads those claims; these tests pin its behaviour. +/// +public class SiteScopeServiceTests +{ + private sealed class StubAuthStateProvider : AuthenticationStateProvider + { + private readonly ClaimsPrincipal _user; + public StubAuthStateProvider(ClaimsPrincipal user) => _user = user; + public override Task GetAuthenticationStateAsync() + => Task.FromResult(new AuthenticationState(_user)); + } + + private static SiteScopeService ForUser(params Claim[] claims) + { + var identity = new ClaimsIdentity(claims, authenticationType: "TestCookie"); + return new SiteScopeService(new StubAuthStateProvider(new ClaimsPrincipal(identity))); + } + + private static Claim Role(string role) => new(JwtTokenService.RoleClaimType, role); + private static Claim SiteClaim(int id) => new(JwtTokenService.SiteIdClaimType, id.ToString()); + + private static List Sites(params int[] ids) + => ids.Select(id => new Site($"Site{id}", $"SITE-{id}") { Id = id }).ToList(); + + [Fact] + public async Task DeploymentUserWithNoSiteClaims_IsSystemWide() + { + var svc = ForUser(Role("Deployment")); + + Assert.True(await svc.IsSystemWideAsync()); + Assert.Empty(await svc.PermittedSiteIdsAsync()); + } + + [Fact] + public async Task SystemWideUser_FilterSites_ReturnsAllSites() + { + var svc = ForUser(Role("Deployment")); + + var filtered = await svc.FilterSitesAsync(Sites(1, 2, 3)); + + Assert.Equal(new[] { 1, 2, 3 }, filtered.Select(s => s.Id)); + } + + [Fact] + public async Task ScopedUser_FilterSites_ReturnsOnlyPermittedSites() + { + // Regression: a Deployment user scoped to sites 1 and 3 must NOT see site 2. + var svc = ForUser(Role("Deployment"), SiteClaim(1), SiteClaim(3)); + + var filtered = await svc.FilterSitesAsync(Sites(1, 2, 3, 4)); + + Assert.Equal(new[] { 1, 3 }, filtered.Select(s => s.Id).OrderBy(x => x)); + } + + [Fact] + public async Task ScopedUser_IsSiteAllowed_OnlyForGrantedSites() + { + var svc = ForUser(Role("Deployment"), SiteClaim(5)); + + Assert.True(await svc.IsSiteAllowedAsync(5)); + Assert.False(await svc.IsSiteAllowedAsync(6)); + } + + [Fact] + public async Task ScopedUser_IsNotSystemWide_AndReportsItsPermittedIds() + { + var svc = ForUser(Role("Deployment"), SiteClaim(7), SiteClaim(9)); + + Assert.False(await svc.IsSystemWideAsync()); + Assert.Equal(new[] { 7, 9 }, (await svc.PermittedSiteIdsAsync()).OrderBy(x => x)); + } + + [Fact] + public async Task SystemWideUser_IsSiteAllowed_ForAnySite() + { + var svc = ForUser(Role("Deployment")); + + Assert.True(await svc.IsSiteAllowedAsync(1)); + Assert.True(await svc.IsSiteAllowedAsync(999)); + } +} diff --git a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs index 3f4f51f..9b88a64 100644 --- a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs +++ b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -465,4 +465,89 @@ public class ScriptAnalysisServiceTests Assert.True(result.Success); Assert.Equal("42", result.ReturnValueJson); } + + [Fact] + public async Task RunInSandbox_CapturesConsoleOutput() + { + var result = await _svc.RunInSandboxAsync( + new SandboxRunRequest( + "System.Console.WriteLine(\"hello-sandbox\"); return 1;", + Parameters: null, + TimeoutSeconds: null), + CancellationToken.None); + + Assert.True(result.Success); + Assert.Contains("hello-sandbox", result.ConsoleOutput); + } + + [Fact] + public async Task RunInSandbox_ConcurrentRuns_DoNotCrossContaminateConsoleOutput() + { + // Regression test for CentralUI-003. RunInSandboxAsync used to redirect the + // process-global Console.Out/Error to a per-call StringWriter. While one run + // is mid-flight, any concurrent run's `finally` restores Console.Out to the + // ORIGINAL writer — so the long run loses every Console.WriteLine it makes + // after that point, and short runs cross-contaminate each other. The fix + // routes capture per-call via an AsyncLocal writer without mutating + // process-global Console state. + + // A long-running script: writes its tag, then burns CPU, then writes again, + // repeatedly. While it spins, many short runs start and finish around it. + async Task RunLong() + { + var code = @" + for (int i = 0; i < 40; i++) + { + System.Console.WriteLine(""LONG""); + long acc = 0; + for (long j = 0; j < 2_000_000; j++) acc += j; + System.Console.WriteLine(""LONG"" + acc); + } + return 0;"; + var r = await _svc.RunInSandboxAsync( + new SandboxRunRequest(code, Parameters: null, TimeoutSeconds: 30), + CancellationToken.None); + Assert.True(r.Success, r.Error); + return r.ConsoleOutput; + } + + async Task RunShort(int id) + { + var code = $"for (int i = 0; i < 30; i++) System.Console.WriteLine(\"S{id}\"); return 0;"; + var r = await _svc.RunInSandboxAsync( + new SandboxRunRequest(code, Parameters: null, TimeoutSeconds: 30), + CancellationToken.None); + Assert.True(r.Success, r.Error); + return r.ConsoleOutput; + } + + var longTask = RunLong(); + var shortTasks = new List>(); + for (var round = 0; round < 12; round++) + { + for (var k = 0; k < 4; k++) + shortTasks.Add(RunShort(round * 4 + k)); + await Task.Yield(); + } + + var longOut = await longTask; + var shortOuts = await Task.WhenAll(shortTasks); + + // The long run must have captured ALL 80 of its own writes (40 plain + 40 acc). + var longLines = longOut.Split('\n', StringSplitOptions.RemoveEmptyEntries) + .Count(l => l.StartsWith("LONG")); + Assert.Equal(80, longLines); + + // No short run's output must have leaked into the long run's capture. + for (var i = 0; i < shortOuts.Length; i++) + Assert.DoesNotContain($"S{i}", longOut); + + // Each short run captured exactly its own 30 lines and nothing else. + for (var i = 0; i < shortOuts.Length; i++) + { + var lines = shortOuts[i].Split('\n', StringSplitOptions.RemoveEmptyEntries); + Assert.Equal(30, lines.Length); + Assert.All(lines, l => Assert.Equal($"S{i}", l.Trim())); + } + } } diff --git a/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs b/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs index bcf74d7..9cf343c 100644 --- a/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs +++ b/tests/ScadaLink.CentralUI.Tests/TopologyPageTests.cs @@ -65,6 +65,10 @@ public class TopologyPageTests : BunitContext // DI registration still has to satisfy the [Inject]. Services.AddScoped(); + // Site scoping (CentralUI-002): Topology injects SiteScopeService to + // filter the tree by the user's permitted sites. + Services.AddScoped(); + // TreeView persists expansion state via JS interop. Stub the calls so render doesn't throw. JSInterop.Setup("treeviewStorage.load", _ => true).SetResult(null); JSInterop.SetupVoid("treeviewStorage.save", _ => true); @@ -194,6 +198,52 @@ public class TopologyPageTests : BunitContext Assert.Contains(dimmedNodes, n => n.TextContent.Contains("Boilers")); } + [Fact] + public void SiteScoping_ScopedDeploymentUser_OnlySeesPermittedSites() + { + // Regression test for CentralUI-002. The SiteId claims issued at login were + // never read, so a Deployment user scoped to one site could view (and act + // on) every site's topology. Topology now filters the tree by the user's + // permitted sites via SiteScopeService. + var scopedUser = new ClaimsPrincipal(new ClaimsIdentity(new[] + { + new Claim("Username", "scoped-tester"), + new Claim(ScadaLink.Security.JwtTokenService.RoleClaimType, "Deployment"), + // Permitted on site 1 only. + new Claim(ScadaLink.Security.JwtTokenService.SiteIdClaimType, "1"), + }, "TestAuth")); + // Last AuthenticationStateProvider registration wins on resolution. + Services.AddSingleton(new TestAuthStateProvider(scopedUser)); + + SeedRepos(sites: new[] + { + new Site("Plant-A", "plant-a") { Id = 1 }, + new Site("Plant-B", "plant-b") { Id = 2 }, + }); + + var cut = Render(); + + // The permitted site is rendered; the non-permitted site is not. + Assert.Contains("Plant-A", cut.Markup); + Assert.DoesNotContain("Plant-B", cut.Markup); + } + + [Fact] + public void SiteScoping_SystemWideDeploymentUser_SeesAllSites() + { + // A Deployment user with no SiteId claims is system-wide and sees every site. + SeedRepos(sites: new[] + { + new Site("Plant-A", "plant-a") { Id = 1 }, + new Site("Plant-B", "plant-b") { Id = 2 }, + }); + + var cut = Render(); + + Assert.Contains("Plant-A", cut.Markup); + Assert.Contains("Plant-B", cut.Markup); + } + [Fact] public void DoubleClick_OnAreaLabel_EntersRenameMode() {