diff --git a/docker-env2/central-node-a/appsettings.Central.json b/docker-env2/central-node-a/appsettings.Central.json index 63697f15..e59703ab 100644 --- a/docker-env2/central-node-a/appsettings.Central.json +++ b/docker-env2/central-node-a/appsettings.Central.json @@ -35,7 +35,12 @@ "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, "RequireHttpsCookie": false, - "CookieName": "ZB.MOM.WW.ScadaBridge.Auth.env2" + "CookieName": "ZB.MOM.WW.ScadaBridge.Auth.env2", + "ForwardedHeaders": { + "_comment": "Traefik fronts this node on the external scadabridge-net docker network. Trust X-Forwarded-For from the docker bridge address pool so LoginThrottle keys on the real client IP (arch-review R2 N2). Narrow to the Traefik container IP if the network is pinned.", + "Enabled": true, + "KnownNetworks": [ "172.16.0.0/12" ] + } }, "Communication": { "DeploymentTimeout": "00:02:00", diff --git a/docker-env2/central-node-b/appsettings.Central.json b/docker-env2/central-node-b/appsettings.Central.json index 556955b5..11ddfdba 100644 --- a/docker-env2/central-node-b/appsettings.Central.json +++ b/docker-env2/central-node-b/appsettings.Central.json @@ -35,7 +35,12 @@ "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, "RequireHttpsCookie": false, - "CookieName": "ZB.MOM.WW.ScadaBridge.Auth.env2" + "CookieName": "ZB.MOM.WW.ScadaBridge.Auth.env2", + "ForwardedHeaders": { + "_comment": "Traefik fronts this node on the external scadabridge-net docker network. Trust X-Forwarded-For from the docker bridge address pool so LoginThrottle keys on the real client IP (arch-review R2 N2). Narrow to the Traefik container IP if the network is pinned.", + "Enabled": true, + "KnownNetworks": [ "172.16.0.0/12" ] + } }, "Communication": { "DeploymentTimeout": "00:02:00", diff --git a/docker/central-node-a/appsettings.Central.json b/docker/central-node-a/appsettings.Central.json index bd39ffe2..66799ef1 100644 --- a/docker/central-node-a/appsettings.Central.json +++ b/docker/central-node-a/appsettings.Central.json @@ -38,7 +38,12 @@ "JwtSigningKey": "scadabridge-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, - "RequireHttpsCookie": false + "RequireHttpsCookie": false, + "ForwardedHeaders": { + "_comment": "Traefik fronts this node on the external scadabridge-net docker network. Trust X-Forwarded-For from the docker bridge address pool so LoginThrottle keys on the real client IP (arch-review R2 N2). Narrow to the Traefik container IP if the network is pinned.", + "Enabled": true, + "KnownNetworks": [ "172.16.0.0/12" ] + } }, "Communication": { "DeploymentTimeout": "00:02:00", diff --git a/docker/central-node-b/appsettings.Central.json b/docker/central-node-b/appsettings.Central.json index bf0d092e..95c4aa9d 100644 --- a/docker/central-node-b/appsettings.Central.json +++ b/docker/central-node-b/appsettings.Central.json @@ -38,7 +38,12 @@ "JwtSigningKey": "scadabridge-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, - "RequireHttpsCookie": false + "RequireHttpsCookie": false, + "ForwardedHeaders": { + "_comment": "Traefik fronts this node on the external scadabridge-net docker network. Trust X-Forwarded-For from the docker bridge address pool so LoginThrottle keys on the real client IP (arch-review R2 N2). Narrow to the Traefik container IP if the network is pinned.", + "Enabled": true, + "KnownNetworks": [ "172.16.0.0/12" ] + } }, "Communication": { "DeploymentTimeout": "00:02:00", diff --git a/docs/requirements/Component-CentralUI.md b/docs/requirements/Component-CentralUI.md index 5853d690..48c8fe52 100644 --- a/docs/requirements/Component-CentralUI.md +++ b/docs/requirements/Component-CentralUI.md @@ -191,7 +191,7 @@ Per-leaf alarm rendering (leaf nodes are individual conditions for native alarms - **Live updates** — the page is driven by a **transient, per-site central live alarm cache** (`ISiteAlarmLiveCache`, owned by the Communication component; see [Component-Communication](Component-Communication.md)). On site select the page subscribes to the cache; the cache runs one shared, reference-counted per-site aggregator that **seeds** from the snapshot fan-out and then stays warm on a single **site-wide, alarm-only** `SubscribeSite` gRPC stream (seed-then-stream, dedup by `(InstanceUniqueName, AlarmName, SourceReference)`). Applied deltas raise an in-process change event (mirroring `IDeploymentStatusNotifier`) that the Blazor circuit pushes to the browser via `StateHasChanged()` — no new SignalR hub. `AlarmSummaryService.BuildFromLiveAlarms` rebuilds the roll-up + rows from the cache's current alarm set. The cache is **purely in-memory on the active central node** — there is still **no persisted central alarm store**; on a NodeA↔NodeB failover the new active node re-seeds from scratch. - **View** — roll-up tiles (total active, worst severity, unacked count, per-`AlarmKind` counts) plus a flat, sortable, filterable table. Filters cover instance, `AlarmKind` (Computed / NativeOpcUa / NativeMxAccess), state, acked/unacked, severity threshold, and name search. - **Read-only** — there are no ack / shelve / suppress controls (native alarms remain read-only by design). -- **Refresh** — manual refresh button plus the 15s poll timer (mirroring the Health dashboard), now retained as a **fallback + `NotReporting` authority** behind the live cache: when the cache reports `IsLive`, the page renders live-cache state; when a stream is unhealthy or a site has not yet seeded, the poll keeps the page fresh so a stream failure never blanks it. (Aggregated live stream **delivered 2026-07-10** — see `docs/plans/2026-07-10-aggregated-live-alarm-stream-plan.md`.) +- **Refresh** — manual refresh button plus the 15s poll timer (mirroring the Health dashboard), now retained as a **fallback + `NotReporting` authority** behind the live cache: when the cache reports `IsLive`, the page renders live-cache state; when a stream is unhealthy, **the aggregator has died (deathwatch resets `IsLive`)**, or a site has not yet seeded, the poll keeps the page fresh so a stream failure never blanks it. When live, the poll updates only the `NotReporting` list and leaves the row set to the delta path, so a slow fan-out can never momentarily revert a fresher live delta (R2 N5). (Aggregated live stream **delivered 2026-07-10** — see `docs/plans/2026-07-10-aggregated-live-alarm-stream-plan.md`.) - **Reuse** — the alarm badge/formatter markup is factored out of Debug View into a shared `AlarmStateBadges` component consumed by both Debug View and this page. ### Parked Message Management (Deployment Role) diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index 3fc88241..6a59f0fe 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -138,7 +138,7 @@ Set in a local or docker-dev environment via the environment variable `ScadaBrid - Approve or reject a pending secured write from the Secured Writes page — the *approving* half of the two-person write workflow. - **Purpose**: The approving counterpart to **Operator**. Separation of duties is enforced **server-side**: the ManagementActor rejects any approval where the approving user equals the submitting Operator (no self-approval), so the two roles must be held by distinct principals for a write to execute. (See Component-ManagementService.md and Component-CentralUI.md.) -> **Two-person secured-write workflow.** `Operator` and `Verifier` are deliberately separate global roles so a single principal cannot both initiate and approve a write through the MxAccess Gateway. Both are coarse global roles like the others; any site scoping is layered on at the LDAP-mapping level. +> **Two-person secured-write workflow.** `Operator` and `Verifier` are deliberately separate global roles so a single principal cannot both initiate and approve a write through the MxAccess Gateway. Both are coarse global roles like the others. **Site scoping from the LDAP mapping is enforced server-side on every secured-write command** (arch-review R2 N3): submit checks the target site, approve/reject check the row's site (before the TTL/self-approval/CAS chain), and the list constrains a scoped caller to their permitted sites — mirroring the deployment commands' `EnforceSiteScope` rule. Administrators and system-wide principals (empty permitted-site set) are unrestricted. > > **Deployment-configuration hazard — never grant one principal both roles.** The whole control collapses if the same identity (or LDAP group) maps to *both* `Operator` and `Verifier`: server-side no-self-approval blocks approving the *exact* write you submitted, but a dual-role principal can still trivially pair-approve with a second submission, so a single compromised or careless account regains unilateral write. Keep `SCADA-Operators` and `SCADA-Verifiers` group membership disjoint. The dev `DisableLogin` caveat is the extreme of this: with `DisableLogin` on, the auto-login principal holds **all** roles, so the two-person flow cannot be exercised end-to-end by a single identity — which is exactly why `DisableLogin=true` is refused outside Development (see the `DisableLoginGuard` note above). No-self-approval is covered by handler tests; real two-person use requires two real, distinct-role identities. > @@ -205,7 +205,8 @@ Role checks are expressed as named ASP.NET Core authorization policies (in `Auth To blunt online password-spray and brute-force against the directory, every LDAP-bind surface consults a shared in-memory throttle (`LoginThrottle`, a `ZB.MOM.WW.ScadaBridge.Security` singleton) before attempting a bind, and records the bind outcome afterwards. -- **Scope — every LDAP-bind surface**: the Central UI interactive login (`POST /auth/login`) and CLI token issue (`POST /auth/token`), plus the HTTP-Basic management/CLI surfaces fronted by `ManagementAuthenticator` — `POST /management`, the audit REST endpoints, and the debug-stream hub. No bind happens outside these paths, so throttling coverage is complete. The dev/test `DisableLogin` bypass sits **above** the throttle: a login-disabled deployment never binds, so it is never throttled. -- **Key and window**: counters are keyed per `{username}|{IP}` pair (lower-cased username; remote IP from the connection). A fixed window of `LoginFailureWindowMinutes` (default 5) opens on the first failure; reaching `MaxLoginFailuresPerWindow` failures (default 5) within it locks that key out for `LoginLockoutMinutes` (default 5). A successful bind clears the key; an expired window resets the count. Setting `MaxLoginFailuresPerWindow` to `0` disables throttling entirely. +- **Scope — every LDAP-bind surface**: the Central UI interactive login (`POST /auth/login`) and CLI token issue (`POST /auth/token`), plus the HTTP-Basic management/CLI surfaces fronted by `ManagementAuthenticator` — `POST /management`, the audit REST endpoints, **and the debug-stream hub (`DebugStreamHub.OnConnectedAsync` delegates its whole credential path to `ManagementAuthenticator.AuthenticateAsync`, closing the round-2 N1 gap where the hub bound LDAP directly and bypassed the throttle)**. No bind happens outside these paths. The dev/test `DisableLogin` bypass sits **above** the throttle: a login-disabled deployment never binds, so it is never throttled. +- **Key and window**: counters are keyed per `{username}|{IP}` pair (lower-cased username; the IP half is the **forwarded-header-resolved client IP** when `ScadaBridge:Security:ForwardedHeaders` names a trusted proxy, otherwise the raw connection peer). A fixed window of `LoginFailureWindowMinutes` (default 5) opens on the first failure; reaching `MaxLoginFailuresPerWindow` failures (default 5) within it locks that key out for `LoginLockoutMinutes` (default 5). A successful bind clears the key; an expired window resets the count. Setting `MaxLoginFailuresPerWindow` to `0` disables throttling entirely. +- **Proxy topologies and the username-lockout DoS.** The key is deliberately `{username}|{IP}`, not `{username}` alone: keying on username alone would let any network-adjacent actor lock any operator out of a SCADA control surface with five wrong passwords, repeatable indefinitely. That isolation only exists when the throttle sees real client IPs — behind a proxy **without** ForwardedHeaders trust, all clients collapse onto the proxy's IP and the DoS returns. Mitigations: (a) the shipped Traefik topologies enable trusted-proxy ForwardedHeaders (so per-IP isolation is real in the documented deployment); (b) the lockout window is short by default (`LoginLockoutMinutes` = 5) and never touches the directory account itself; (c) residual risk — an attacker spraying from their *own* IP locking out a victim username *at that IP only* — is the throttle working as designed. A success-path bypass ("a correct password unlocks") was considered and rejected: it would let an attacker who has the password bypass the lockout entirely, and it converts the throttle into a password oracle during the lockout window. - **Behaviour when locked**: the surface refuses the bind without contacting LDAP. The Basic-Auth surfaces (`ManagementAuthenticator`) and `POST /auth/token` return HTTP `429` with `code = "AUTH_THROTTLED"`; `POST /auth/login` redirects to `/login` with a "Too many failed attempts. Try again later." message. - **Best-effort, per-node**: the throttle is in-memory with no external state. The two central nodes maintain independent counters (a spray hitting both simply burns its budget on each). Memory is bounded — writes prune expired keys and cap the tracked-key count. This is a rate-limit guard, not an account-lockout policy; it never disables the directory account itself. diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/AlarmSummary.razor b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/AlarmSummary.razor index 536bdf1c..fc140b7b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/AlarmSummary.razor +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Monitoring/AlarmSummary.razor @@ -291,9 +291,27 @@ try { var result = await AlarmSummaryService.GetSiteAlarmsAsync(siteId); - _rows = result.Alarms; + // Stale-site guard (arch-review R2 N4): the operator may have switched sites + // while this fan-out was in flight — drop the result rather than labeling + // site A's alarms under site B's picker. Mirrors OnLiveAlarmsChanged (:374). + if (_selectedSiteId != siteId) + { + return; + } + + // _notReporting is the poll's unique authority — the alarm-only live cache + // cannot compute it — so it is always refreshed. _notReporting = result.NotReportingInstances; - _rollup = AlarmSummaryService.ComputeRollup(_rows); + + // While the cache is live, the live deltas own the row set: a poll whose fan-out + // started BEFORE a delta must not land after it and momentarily revert the alarm + // state (arch-review R2 N5). When not live (pre-seed / degraded stream / dead + // aggregator — see R2 N6), the poll remains the full-rebuild safety net. + if (!LiveAlarmCache.IsLive(siteId)) + { + _rows = result.Alarms; + _rollup = AlarmSummaryService.ComputeRollup(_rows); + } RecomputeVisibleRows(); } catch @@ -341,10 +359,11 @@ // aggregated alarm set changes. We rebuild _rows/_rollup/_visibleRows from the // immutable live snapshot — but deliberately DO NOT touch _notReporting, since // the alarm-only live cache can't compute it. - // • The 15s poll (RefreshAsync) still runs untouched: it is the authority for - // _notReporting and the safety net when the cache is not live (pre-seed or a - // degraded/failed stream — IsLive == false). When live, the poll simply - // re-affirms the same snapshot; the live path just makes updates arrive sooner. + // • The 15s poll (RefreshAsync) is the authority for _notReporting and the + // full-rebuild safety net ONLY while the cache is not live (pre-seed or a + // degraded/failed stream — IsLive == false); when live it deliberately leaves + // _rows to the delta path, so a slow fan-out can never revert a fresher live + // delta (arch-review R2 N5). // • Both paths mutate shared state only via the Blazor dispatcher (the poll via // its InvokeAsync callback, the live delta via OnLiveChanged's InvokeAsync), so // they are serialized and never race. Each rebuild is an idempotent snapshot, so @@ -363,15 +382,20 @@ _liveSubscription = null; } + // N7: set BEFORE teardown so a live callback racing Dispose is dropped both + // before the InvokeAsync marshal and inside it (mirrors DebugView.razor). + private volatile bool _disposed; + // Raised on the aggregator's thread — marshal onto the circuit before touching state. private void OnLiveAlarmsChanged(int siteId) { + if (_disposed) return; _ = InvokeAsync(() => { // Drop stale callbacks for a site we've since navigated away from, and // let the poll drive until the aggregator has actually seeded (so we never // clobber a good poll snapshot with an empty pre-seed list). - if (_selectedSiteId != siteId || !LiveAlarmCache.IsLive(siteId)) + if (_disposed || _selectedSiteId != siteId || !LiveAlarmCache.IsLive(siteId)) { return; } @@ -506,6 +530,7 @@ public void Dispose() { + _disposed = true; StopTimer(); DisposeLiveSubscription(); } diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/ISecuredWriteRepository.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/ISecuredWriteRepository.cs index fe431e10..52f7c71f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/ISecuredWriteRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/ISecuredWriteRepository.cs @@ -46,6 +46,11 @@ public interface ISecuredWriteRepository /// Site id filter; null matches every site. /// Number of rows to skip (offset paging). /// Maximum number of rows to return. + /// When non-null, only rows whose SiteId (site + /// identifier) is in the set match; null matches every site. Applied IN ADDITION + /// to . Used by the ManagementActor to constrain an unfiltered + /// list to a site-scoped caller's permitted sites at the query level (arch-review R2 N3), + /// so paging and totals stay correct. /// Cancellation token. /// A task that resolves to a page of matching rows, newest submission first. Task> QueryAsync( @@ -53,6 +58,7 @@ public interface ISecuredWriteRepository string? siteId, int skip, int take, + IReadOnlyCollection? permittedSiteIds = null, CancellationToken ct = default); /// @@ -106,7 +112,16 @@ public interface ISecuredWriteRepository /// /// Status filter; null matches every status. /// Site id filter; null matches every site. + /// When non-null, only rows whose SiteId (site + /// identifier) is in the set match; null matches every site. Applied IN ADDITION + /// to . Used by the ManagementActor to constrain an unfiltered + /// list to a site-scoped caller's permitted sites at the query level (arch-review R2 N3), + /// so paging and totals stay correct. /// Cancellation token. /// A task that resolves to the count of matching rows. - Task CountAsync(string? status, string? siteId, CancellationToken ct = default); + Task CountAsync( + string? status, + string? siteId, + IReadOnlyCollection? permittedSiteIds = null, + CancellationToken ct = default); } diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/ISiteAlarmLiveCache.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/ISiteAlarmLiveCache.cs index 8fe7eebc..5d094a47 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/ISiteAlarmLiveCache.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/ISiteAlarmLiveCache.cs @@ -60,6 +60,6 @@ public interface ISiteAlarmLiveCache /// cache is authoritative. /// /// The numeric site id. - /// true once the site's aggregator has seeded and published at least once. + /// true while a living aggregator has seeded and published; reverts to false if the aggregator terminates (R2 N6) — a frozen snapshot is never reported as live. bool IsLive(int siteId); } diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/SiteAlarmLiveCacheService.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/SiteAlarmLiveCacheService.cs index f27d50a7..c3fbb7f3 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/SiteAlarmLiveCacheService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/SiteAlarmLiveCacheService.cs @@ -239,6 +239,14 @@ public sealed class SiteAlarmLiveCacheService : ISiteAlarmLiveCache TimeSpan.FromSeconds(60))); // stability window — former StabilityWindow static default entry.Actor = system.ActorOf(props, $"site-alarm-aggregator-{entry.SiteId}-{Guid.NewGuid():N}"); + + var aggregator = entry.Actor; + // Deathwatch (arch-review R2 N6): if the aggregator terminates for any reason, + // liveness must reset — a dead feed must never keep IsLive == true, or the page + // grafts a frozen snapshot over fresh poll data for up to 15 s. + system.ActorOf(Props.Create(() => new AggregatorTerminationWatcher( + aggregator!, () => OnAggregatorTerminated(entry.SiteId, aggregator!)))); + entry.StartRetryTimer?.Dispose(); entry.StartRetryTimer = null; _logger.LogInformation("Started live alarm aggregator for site {SiteId} ({SiteIdentifier})", @@ -421,6 +429,57 @@ public sealed class SiteAlarmLiveCacheService : ISiteAlarmLiveCache } } + // ── Deathwatch (R2 N6) ────────────────────────────────────────────────────── + + /// Watches one aggregator and fires a callback exactly once on Terminated. + private sealed class AggregatorTerminationWatcher : ReceiveActor + { + public AggregatorTerminationWatcher(IActorRef watched, Action onTerminated) + { + Context.Watch(watched); + Receive(_ => + { + onTerminated(); + Context.Stop(Self); + }); + } + } + + /// + /// Terminated hook (R2 N6). A DELIBERATE stop (linger fired, entry already removed + /// from _sites) is a no-op — the ReferenceEquals guard also ignores a stale watcher + /// firing after a newer aggregator replaced the dead one. A crash-death with live + /// viewers resets liveness (IsLive → false, snapshot cleared, so the page's poll is + /// authoritative again) and arms the existing bounded start-retry so the feature + /// self-heals, mirroring StartAggregatorAsync's transient-failure path. + /// + private void OnAggregatorTerminated(int siteId, IActorRef deadActor) + { + lock (_lock) + { + if (!_sites.TryGetValue(siteId, out var entry) || !ReferenceEquals(entry.Actor, deadActor)) + return; + + entry.Actor = null; + entry.HasPublished = false; + entry.Current = Empty; + + if (entry.Subscribers.Count > 0 && !entry.Starting) + { + entry.Starting = true; + entry.StartRetryTimer?.Dispose(); + entry.StartRetryTimer = new Timer( + _ => { _ = StartAggregatorAsync(entry); }, + state: null, + dueTime: _options.LiveAlarmCacheReconcileInterval, + period: Timeout.InfiniteTimeSpan); + } + } + _logger.LogWarning( + "Live alarm aggregator for site {SiteId} terminated unexpectedly; liveness reset " + + "(page falls back to polling) and a restart is armed while viewers remain", siteId); + } + // ── Nested types ──────────────────────────────────────────────────────────── /// Per-site bookkeeping. All mutable fields are guarded by the service's _lock. diff --git a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SecuredWriteRepository.cs b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SecuredWriteRepository.cs index cab9d433..1ad871e1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SecuredWriteRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/SecuredWriteRepository.cs @@ -57,6 +57,7 @@ public class SecuredWriteRepository : ISecuredWriteRepository string? siteId, int skip, int take, + IReadOnlyCollection? permittedSiteIds = null, CancellationToken ct = default) { IQueryable query = _context.Set() @@ -72,6 +73,11 @@ public class SecuredWriteRepository : ISecuredWriteRepository query = query.Where(p => p.SiteId == siteId); } + if (permittedSiteIds is not null) + { + query = query.Where(p => permittedSiteIds.Contains(p.SiteId)); + } + return await query .OrderByDescending(p => p.SubmittedAtUtc) .ThenByDescending(p => p.Id) @@ -128,7 +134,11 @@ WHERE Id = {id} } /// - public async Task CountAsync(string? status, string? siteId, CancellationToken ct = default) + public async Task CountAsync( + string? status, + string? siteId, + IReadOnlyCollection? permittedSiteIds = null, + CancellationToken ct = default) { IQueryable query = _context.Set() .AsNoTracking(); @@ -143,6 +153,11 @@ WHERE Id = {id} query = query.Where(p => p.SiteId == siteId); } + if (permittedSiteIds is not null) + { + query = query.Where(p => permittedSiteIds.Contains(p.SiteId)); + } + return await query.CountAsync(ct); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index 69fb8252..2be86d1d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -337,6 +337,18 @@ try } // Middleware pipeline + + // Trusted-proxy forwarded headers (arch-review R2 N2): behind Traefik every client + // shares the proxy's IP, collapsing the LoginThrottle's {username}|{ip} keys onto + // one address (per-IP isolation gone + cheap username-lockout DoS). When enabled, + // X-Forwarded-For from the CONFIGURED proxies only is honored. MUST run first — + // everything downstream keys on Connection.RemoteIpAddress. + var forwardedOptions = builder.Configuration + .GetSection(ZB.MOM.WW.ScadaBridge.Security.ForwardedHeadersSecurityOptions.SectionName) + .Get() ?? new(); + if (ZB.MOM.WW.ScadaBridge.Security.ForwardedHeadersSetup.Build(forwardedOptions) is { } fho) + app.UseForwardedHeaders(fho); + app.UseWebSockets(); app.UseRouting(); app.UseAuthentication(); diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ConfigSecretScrubber.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ConfigSecretScrubber.cs index 4cc28c94..6ecdbe2d 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ConfigSecretScrubber.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ConfigSecretScrubber.cs @@ -18,7 +18,7 @@ internal static class ConfigSecretScrubber private static readonly string[] SecretNameFragments = ["password", "secret", "token", "apikey", "credential", "passphrase"]; - private static bool IsSecretName(string propertyName) + internal static bool IsSecretName(string propertyName) { foreach (var fragment in SecretNameFragments) { @@ -127,6 +127,15 @@ internal static class ConfigSecretScrubber /// The client-submitted config JSON, possibly containing sentinel values. /// The previously stored config JSON to source real secret values from. /// The incoming JSON with sentinel values replaced by the corresponding stored values. + /// + /// Array limitation (arch-review R2 N9): sentinel restoration inside arrays + /// pairs incoming[i] with stored[i] BY INDEX. A client that reorders an array of + /// credentialed objects while round-tripping sentinels grafts the wrong stored + /// secret into each slot — a silent misconfiguration (never a disclosure). Clients + /// must not reorder arrays that carry sentinels (the CLI round-trip does not); + /// key-based matching is a possible future refinement if a config type ever nests + /// credentialed arrays. + /// public static string? MergeSentinels(string? incoming, string? stored) { if (string.IsNullOrEmpty(incoming)) diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs index baa71751..429cda0e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs @@ -1,4 +1,3 @@ -using System.Text; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -6,7 +5,6 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Messages.DebugView; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Streaming; using ZB.MOM.WW.ScadaBridge.Communication; -using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.ManagementService; @@ -75,73 +73,35 @@ public class DebugStreamHub : Hub return; } - // Dev/test: when login is disabled, auto-authenticate the debug stream as the configured - // dev user (ALL roles incl. Deployer, system-wide) — mirroring the cookie/management - // bypass so the CLI debug stream is usable in a login-disabled (e.g. no-LDAP) deployment. - // See ManagementAuthenticator. - var bypassUser = ManagementAuthenticator.TryDisableLoginUser(httpContext); - if (bypassUser is not null) + // Authenticate via the SAME shared Basic→LDAP flow as POST /management and the + // audit REST (arch-review R2 N1): ManagementAuthenticator applies the DisableLogin + // bypass, the LoginThrottle lockout check BEFORE the bind, and RecordFailure/ + // RecordSuccess bookkeeping — so hub connection attempts can no longer be used as + // an unthrottled LDAP-bind oracle, and hub failures feed the shared lockout. + var outcome = await ManagementAuthenticator.AuthenticateAsync(httpContext); + if (outcome.User is null) { - Context.Items[RolesKey] = bypassUser.Roles; - Context.Items[PermittedSiteIdsKey] = bypassUser.PermittedSiteIds; - _logger.LogInformation( - "DebugStreamHub connection established for {Username} (login disabled)", bypassUser.Username); - await base.OnConnectedAsync(); - return; - } - - // Extract Basic Auth credentials - var authHeader = httpContext.Request.Headers.Authorization.ToString(); - if (string.IsNullOrEmpty(authHeader) || !authHeader.StartsWith("Basic ", StringComparison.OrdinalIgnoreCase)) - { - _logger.LogWarning("DebugStreamHub connection rejected: missing Basic Auth header"); + _logger.LogWarning("DebugStreamHub connection rejected: authentication failed or throttled"); Context.Abort(); return; } + var user = outcome.User; - string username, password; - try + // Role check — Deployer role required (unchanged policy). + if (!user.Roles.Contains(Roles.Deployer)) { - var decoded = Encoding.UTF8.GetString(Convert.FromBase64String(authHeader["Basic ".Length..])); - var colon = decoded.IndexOf(':'); - if (colon < 0) throw new FormatException(); - username = decoded[..colon]; - password = decoded[(colon + 1)..]; - } - catch - { - _logger.LogWarning("DebugStreamHub connection rejected: malformed Basic Auth"); - Context.Abort(); - return; - } - - // LDAP authentication - var ldapAuth = httpContext.RequestServices.GetRequiredService(); - var authResult = await ldapAuth.AuthenticateAsync(username, password, Context.ConnectionAborted); - if (!authResult.Succeeded) - { - _logger.LogWarning("DebugStreamHub connection rejected: LDAP auth failed for {Username}", username); - Context.Abort(); - return; - } - - // Role check — Deployer role required - var roleMapper = httpContext.RequestServices.GetRequiredService(); - var mappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups, Context.ConnectionAborted); - - if (!mappingResult.Roles.Contains(Roles.Deployer)) - { - _logger.LogWarning("DebugStreamHub connection rejected: {Username} lacks Deployer role", username); + _logger.LogWarning("DebugStreamHub connection rejected: {Username} lacks Deployer role", user.Username); Context.Abort(); return; } // Persist the resolved identity on the connection so per-instance site-scope - // enforcement can be applied to SubscribeInstance calls. - Context.Items[RolesKey] = mappingResult.Roles.ToArray(); - Context.Items[PermittedSiteIdsKey] = mappingResult.PermittedSiteIds.ToArray(); + // enforcement can be applied to SubscribeInstance calls. PermittedSiteIds is the + // authenticator's normalized form (empty == system-wide), same as every other surface. + Context.Items[RolesKey] = user.Roles; + Context.Items[PermittedSiteIdsKey] = user.PermittedSiteIds; - _logger.LogInformation("DebugStreamHub connection established for {Username}", username); + _logger.LogInformation("DebugStreamHub connection established for {Username}", user.Username); await base.OnConnectedAsync(); } diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 71e692e4..1fe68472 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -467,7 +467,7 @@ public class ManagementActor : ReceiveActor SubmitSecuredWriteCommand cmd => await HandleSubmitSecuredWrite(sp, cmd, user), ApproveSecuredWriteCommand cmd => await HandleApproveSecuredWrite(sp, cmd, user), RejectSecuredWriteCommand cmd => await HandleRejectSecuredWrite(sp, cmd, user), - ListSecuredWritesCommand cmd => await HandleListSecuredWrites(sp, cmd), + ListSecuredWritesCommand cmd => await HandleListSecuredWrites(sp, cmd, user), // Transport bundle operations ExportBundleCommand cmd => await HandleExportBundle(sp, cmd, user.Username), @@ -1247,6 +1247,11 @@ public class ManagementActor : ReceiveActor var site = await siteRepo.GetSiteByIdentifierAsync(cmd.SiteId) ?? throw new ManagementCommandException($"Site '{cmd.SiteId}' not found."); + // Site scope (arch-review R2 N3): an LDAP-mapping-scoped Operator may only + // submit device writes to their permitted sites — same rule C2 applies to + // DeployArtifacts, on a higher-consequence path. Checked before the row exists. + EnforceSiteScope(user, site.Id); + var connections = await siteRepo.GetDataConnectionsBySiteIdAsync(site.Id); var conn = connections.FirstOrDefault(c => string.Equals(c.Name, cmd.ConnectionName, StringComparison.Ordinal)) @@ -1293,6 +1298,11 @@ public class ManagementActor : ReceiveActor throw new ManagementCommandException( $"Secured write {cmd.Id} is '{row.Status}', not Pending; it cannot be approved."); + // Site scope (arch-review R2 N3): the approving Verifier must be permitted on + // the row's target site — checked BEFORE the TTL/self-approval/CAS chain so an + // out-of-scope verifier can neither consume the Pending transition nor relay. + await EnforceSiteScopeForIdentifier(sp, user, row.SiteId); + // Server-side TTL: a Pending write older than the configured age is transitioned // to Expired and can never be approved — the stale setpoint is never relayed // (arch-review S2). Checked BEFORE the self-approval/CAS so an overdue row never @@ -1450,6 +1460,10 @@ public class ManagementActor : ReceiveActor throw new ManagementCommandException( $"Secured write {cmd.Id} is '{entity.Status}', not Pending; it cannot be rejected."); + // Site scope (arch-review R2 N3): the rejecting Verifier must be permitted on + // the row's target site — checked BEFORE the TTL/self-approval chain. + await EnforceSiteScopeForIdentifier(sp, user, entity.SiteId); + // Server-side TTL: an overdue Pending write expires here too, so a reject on a // stale row deterministically reports the expiry rather than recording a // human decision (arch-review S2). @@ -1479,16 +1493,36 @@ public class ManagementActor : ReceiveActor } private static async Task HandleListSecuredWrites( - IServiceProvider sp, ListSecuredWritesCommand cmd) + IServiceProvider sp, ListSecuredWritesCommand cmd, AuthenticatedUser user) { + // Site scoping (arch-review R2 N3): an explicit SiteId filter is scope-checked + // like every other identifier-keyed command; an UNFILTERED list from a + // site-scoped non-admin caller is constrained to their permitted sites at the + // query level (rows AND totalCount both scoped — see ISecuredWriteRepository). + IReadOnlyCollection? permittedIdentifiers = null; + if (cmd.SiteId is not null) + { + await EnforceSiteScopeForIdentifier(sp, user, cmd.SiteId); + } + else if (user.PermittedSiteIds.Length > 0 + && !user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) + { + var siteRepo = sp.GetRequiredService(); + var permitted = new HashSet(user.PermittedSiteIds); + permittedIdentifiers = (await siteRepo.GetAllSitesAsync()) + .Where(s => permitted.Contains(s.Id.ToString())) + .Select(s => s.SiteIdentifier) + .ToList(); + } + var repo = sp.GetRequiredService(); // Offset paging (arch-review P3): clamp untrusted paging inputs so a crafted // command can neither request a negative offset nor an unbounded page. var take = Math.Clamp(cmd.Take, 1, 500); var skip = Math.Max(0, cmd.Skip); - var rows = await repo.QueryAsync(cmd.Status, cmd.SiteId, skip, take); - var totalCount = await repo.CountAsync(cmd.Status, cmd.SiteId); + var rows = await repo.QueryAsync(cmd.Status, cmd.SiteId, skip, take, permittedIdentifiers); + var totalCount = await repo.CountAsync(cmd.Status, cmd.SiteId, permittedIdentifiers); // Opportunistic expiry sweep (arch-review S2): every time the list is read, walk // the page and expire any overdue Pending row. TryExpireIfStaleAsync self-filters diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/ForwardedHeadersSetup.cs b/src/ZB.MOM.WW.ScadaBridge.Security/ForwardedHeadersSetup.cs new file mode 100644 index 00000000..8e85de61 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Security/ForwardedHeadersSetup.cs @@ -0,0 +1,64 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.HttpOverrides; +using System.Net; + +namespace ZB.MOM.WW.ScadaBridge.Security; + +/// +/// Config shape for trusted-proxy forwarded-header handling +/// (ScadaBridge:Security:ForwardedHeaders). Disabled by default: a deployment +/// with no reverse proxy must NOT honor X-Forwarded-For (a client could spoof its +/// throttle key). The documented Traefik topologies enable it with the docker +/// network range so LoginThrottle keys on the REAL client IP (arch-review R2 N2). +/// +public sealed class ForwardedHeadersSecurityOptions +{ + /// Configuration section binding this options class. + public const string SectionName = "ScadaBridge:Security:ForwardedHeaders"; + + /// Whether X-Forwarded-For from trusted proxies is honored. + public bool Enabled { get; set; } + + /// Exact proxy IPs allowed to supply X-Forwarded-For. + public string[] KnownProxies { get; set; } = []; + + /// CIDR networks allowed to supply X-Forwarded-For (e.g. "172.16.0.0/12"). + public string[] KnownNetworks { get; set; } = []; +} + +/// +/// Builds the for app.UseForwardedHeaders. +/// Pure and fail-fast: malformed entries throw at startup rather than silently +/// trusting nothing/everything. Defaults (loopback) are CLEARED — only the +/// explicitly configured proxies/networks are trusted, and ForwardLimit=1 means +/// only the entry the trusted proxy itself appended is honored. +/// +public static class ForwardedHeadersSetup +{ + /// + /// Builds the forwarded-headers options, or when disabled. + /// + /// The bound . + /// The configured , or when disabled. + public static ForwardedHeadersOptions? Build(ForwardedHeadersSecurityOptions options) + { + if (!options.Enabled) return null; + var built = new ForwardedHeadersOptions + { + ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto, + ForwardLimit = 1, + }; + built.KnownProxies.Clear(); + // KnownIPNetworks (System.Net.IPNetwork) is the net10 successor to the now-deprecated + // KnownNetworks; UseForwardedHeaders reads it identically. + built.KnownIPNetworks.Clear(); + foreach (var proxy in options.KnownProxies) + built.KnownProxies.Add(IPAddress.Parse(proxy)); + foreach (var network in options.KnownNetworks) + { + var parts = network.Split('/'); + built.KnownIPNetworks.Add(new(IPAddress.Parse(parts[0]), int.Parse(parts[1]))); + } + return built; + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs index 8d2a46bd..164d6b65 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/LoginThrottle.cs @@ -41,6 +41,18 @@ public sealed class LoginThrottle /// public const int MaxTrackedKeys = 10_000; + /// Amortization cadence: a full prune sweep runs every Nth failure write + /// (plus immediately whenever the map exceeds ), so a + /// spray does not pay an O(n) scan — and a possible full sort — on every failed + /// request (arch-review R2 N8). Lockout SEMANTICS are unaffected: IsLockedOut reads + /// window/lockout expiry directly and never depends on pruning. + internal const int PruneEveryNFailures = 64; + + /// Diagnostics/test accessor: number of currently tracked keys. + internal int TrackedKeyCount => _entries.Count; + + private int _failureWrites; + private readonly TimeProvider _clock; private readonly IOptions _options; @@ -118,7 +130,13 @@ public sealed class LoginThrottle return Arm(existing with { Count = existing.Count + 1 }, opts, now, lockout); }); - Prune(now); + // Amortized prune (R2 N8): every Nth failure, or immediately when over the hard + // cap — the cap is the memory-safety invariant and must never wait for the cadence. + if (Interlocked.Increment(ref _failureWrites) % PruneEveryNFailures == 0 + || _entries.Count > MaxTrackedKeys) + { + Prune(now); + } } /// diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Monitoring/AlarmSummaryRenderTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Monitoring/AlarmSummaryRenderTests.cs index 51cfefc6..1c117079 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Monitoring/AlarmSummaryRenderTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Monitoring/AlarmSummaryRenderTests.cs @@ -208,6 +208,100 @@ public class AlarmSummaryRenderTests : BunitContext Assert.True(_liveCache.DisposeCount >= 1); } + // ── arch-review R2 N4: stale-site poll guard ─────────────────────────────── + + [Fact] + public void InFlightPollForPreviousSite_DoesNotOverwriteNewSitesRows() + { + // Site 1's fan-out hangs on a TCS; site 2 answers immediately (arch-review R2 N4). + var site1Tcs = new TaskCompletionSource(); + _summary.GetSiteAlarmsAsync(1, Arg.Any()).Returns(site1Tcs.Task); + _summary.GetSiteAlarmsAsync(2, Arg.Any()).Returns(Task.FromResult( + new AlarmSummaryResult( + new List + { + new("Site2Instance", new AlarmStateChanged("Site2Instance", "S2-alarm", AlarmState.Active, 100, DateTimeOffset.UtcNow)), + }, + Array.Empty()))); + + var cut = Render(); + cut.Find("[data-test='alarm-summary-site']").Change("1"); // poll in flight, hung + cut.Find("[data-test='alarm-summary-site']").Change("2"); // site 2 renders + cut.WaitForAssertion(() => Assert.Equal("Site2Instance", FirstRowInstance(cut))); + + // The stale site-1 fan-out now completes — it must be dropped, not rendered. + site1Tcs.SetResult(new AlarmSummaryResult( + new List + { + new("StaleSite1", new AlarmStateChanged("StaleSite1", "S1-alarm", AlarmState.Active, 999, DateTimeOffset.UtcNow)), + }, + new[] { "StaleNotReporting" })); + + cut.WaitForAssertion(() => + { + Assert.Equal("Site2Instance", FirstRowInstance(cut)); + Assert.DoesNotContain("StaleNotReporting", cut.Markup); + }); + } + + // ── arch-review R2 N5: while live, the poll updates only _notReporting ────── + + [Fact] + public void PollWhileLive_UpdatesNotReportingOnly_DoesNotRegressLiveRows() + { + // Poll snapshot = Zeta/Alpha (constructor stub) + a not-reporting instance. + _summary.GetSiteAlarmsAsync(Arg.Any(), Arg.Any()) + .Returns(ci => Task.FromResult(new AlarmSummaryResult( + new List + { + new("Zeta", new AlarmStateChanged("Zeta", "Z-alarm", AlarmState.Active, 900, DateTimeOffset.UtcNow)), + }, + new[] { "OfflineInstance" }))); + + var cut = RenderWithSiteSelected(); + + // Go live with a fresher delta: Gamma only (arch-review R2 N5). + _liveCache.PushAlarms(new List + { + new("Gamma", "G-alarm", AlarmState.Active, 300, DateTimeOffset.UtcNow), + }); + cut.WaitForAssertion(() => Assert.Equal("Gamma", FirstRowInstance(cut))); + + // A poll now completes: it owns _notReporting but must NOT rebuild the rows. + cut.Find("[data-test='alarm-summary-refresh']").Click(); + + cut.WaitForAssertion(() => + { + Assert.Contains("OfflineInstance", cut.Markup); // notReporting refreshed + Assert.Equal("Gamma", FirstRowInstance(cut)); // live rows NOT regressed + Assert.Single(cut.FindAll("tr[data-test='alarm-summary-row']")); + }); + } + + // ── arch-review R2 N7: disposal guard on the live callback ───────────────── + + [Fact] + public void LiveCallbackRacingDispose_IsDropped_NoRebuildNoThrow() + { + var cut = RenderWithSiteSelected(); + _liveCache.PushAlarms(new List + { + new("Gamma", "G-alarm", AlarmState.Active, 300, DateTimeOffset.UtcNow), + }); + cut.WaitForAssertion(() => Assert.Equal("Gamma", FirstRowInstance(cut))); + + // Snapshot the callback BEFORE disposal — this models a delta thread that + // already read the delegate when Dispose ran (arch-review R2 N7). + var inFlight = _liveCache.CapturedCallback!; + cut.Instance.Dispose(); + + _liveCache.PushAlarms(new List()); // mutate the fake's snapshot + var ex = Record.Exception(inFlight); + Assert.Null(ex); + // The disposed page must not have re-applied the (now empty) live snapshot. + Assert.Equal("Gamma", FirstRowInstance(cut)); + } + /// /// Controllable in-memory fake of : records /// subscribe/dispose counts and lets a test push a live snapshot (which flips @@ -223,6 +317,7 @@ public class AlarmSummaryRenderTests : BunitContext public int SubscribeCount { get; private set; } public int DisposeCount { get; private set; } public int? LastSubscribedSite { get; private set; } + public Action? CapturedCallback => _onChanged; public IDisposable Subscribe(int siteId, Action onChanged) { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs index eec76ba1..533e203c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/SiteAlarmLiveCacheServiceTests.cs @@ -302,4 +302,40 @@ public class SiteAlarmLiveCacheServiceTests : TestKit AwaitCondition(() => service.IsLive(3), TimeSpan.FromSeconds(5)); Assert.True(factory.GetOrCreateCount >= 1); // a client was created for the single endpoint } + + // ── R2 T10: deathwatch un-sticks IsLive on aggregator death (N6) ── + + [Fact] + public async Task Aggregator_Death_Resets_IsLive_And_Clears_The_Snapshot() + { + var service = CreateService(TimeSpan.FromMinutes(5), out _); + using var sub = service.Subscribe(SiteId, () => { }); + AwaitAssert(() => Assert.True(service.IsLive(SiteId)), TimeSpan.FromSeconds(10)); + + // Kill the aggregator out from under the service (crash-death, NOT a linger stop). + var aggregator = await Sys.ActorSelection($"/user/site-alarm-aggregator-{SiteId}-*") + .ResolveOne(TimeSpan.FromSeconds(5)); + Sys.Stop(aggregator); + + AwaitAssert(() => + { + Assert.False(service.IsLive(SiteId)); // sticky no more (R2 N6) + Assert.Empty(service.GetCurrentAlarms(SiteId)); // frozen snapshot never grafted + }, TimeSpan.FromSeconds(10)); + } + + [Fact] + public void Linger_Stop_Does_Not_Resurrect_The_Aggregator() + { + // Last viewer leaves, linger fires, entry removed: the deathwatch on the + // deliberately-stopped actor must be a no-op (entry gone), not a restart. + var service = CreateService(TimeSpan.FromMilliseconds(50), out var factory); + var sub = service.Subscribe(SiteId, () => { }); + AwaitAssert(() => Assert.True(service.IsLive(SiteId)), TimeSpan.FromSeconds(10)); + sub.Dispose(); + AwaitAssert(() => Assert.False(service.IsLive(SiteId)), TimeSpan.FromSeconds(10)); + var startsAfterStop = factory.GetOrCreateCount; + Thread.Sleep(300); + Assert.Equal(startsAfterStop, factory.GetOrCreateCount); // no self-heal restart fired + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecuredWriteRepositoryTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecuredWriteRepositoryTests.cs index 3ccff9fa..2788fac8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecuredWriteRepositoryTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/SecuredWriteRepositoryTests.cs @@ -164,6 +164,72 @@ public class SecuredWriteRepositoryTests : IClassFixture Assert.Equal(0, await repo.CountAsync(status: "Executed", siteId: siteId)); } + [SkippableFact] + public async Task QueryAsync_PermittedSiteIds_ReturnsOnlyPermittedRows() + { + Skip.IfNot(_fixture.Available, _fixture.SkipReason); + + // Distinct per-run site identifiers stand in for "SITE1"/"SITE2"/"SITE3" so the + // permitted-set filter is asserted deterministically against a shared MSSQL. + var site1 = NewSiteId(); + var site2 = NewSiteId(); + var site3 = NewSiteId(); + await using var context = CreateContext(); + var repo = new SecuredWriteRepository(context); + + await repo.AddAsync(NewRow(site1, status: "Pending")); + await repo.AddAsync(NewRow(site1, status: "Pending")); + await repo.AddAsync(NewRow(site2, status: "Pending")); + await repo.AddAsync(NewRow(site3, status: "Pending")); + + var rows = await repo.QueryAsync(status: null, siteId: null, skip: 0, take: 100, + permittedSiteIds: new[] { site1, site3 }); + Assert.Equal(3, rows.Count); + Assert.DoesNotContain(rows, r => r.SiteId == site2); + } + + [SkippableFact] + public async Task CountAsync_PermittedSiteIds_CountsOnlyPermittedRows() + { + Skip.IfNot(_fixture.Available, _fixture.SkipReason); + + var site1 = NewSiteId(); + var site2 = NewSiteId(); + var site3 = NewSiteId(); + await using var context = CreateContext(); + var repo = new SecuredWriteRepository(context); + + await repo.AddAsync(NewRow(site1, status: "Pending")); + await repo.AddAsync(NewRow(site1, status: "Pending")); + await repo.AddAsync(NewRow(site2, status: "Pending")); + await repo.AddAsync(NewRow(site3, status: "Pending")); + + var count = await repo.CountAsync(status: null, siteId: null, + permittedSiteIds: new[] { site1, site3 }); + Assert.Equal(3, count); + } + + [SkippableFact] + public async Task QueryAsync_NullPermittedSiteIds_IsUnfiltered_BackCompat() + { + Skip.IfNot(_fixture.Available, _fixture.SkipReason); + + // 4 rows on a single unique site; null permittedSiteIds must not filter them + // out — exactly today's behavior. + var siteId = NewSiteId(); + await using var context = CreateContext(); + var repo = new SecuredWriteRepository(context); + + await repo.AddAsync(NewRow(siteId, status: "Pending")); + await repo.AddAsync(NewRow(siteId, status: "Pending")); + await repo.AddAsync(NewRow(siteId, status: "Executed")); + await repo.AddAsync(NewRow(siteId, status: "Rejected")); + + var rows = await repo.QueryAsync(status: null, siteId: siteId, skip: 0, take: 100, + permittedSiteIds: null); + Assert.Equal(4, rows.Count); + } + // --- helpers ------------------------------------------------------------ private ScadaBridgeDbContext CreateContext() diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ConfigSecretScrubberTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ConfigSecretScrubberTests.cs index 75e65e75..e5157a3c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ConfigSecretScrubberTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ConfigSecretScrubberTests.cs @@ -42,4 +42,47 @@ public class ConfigSecretScrubberTests Assert.Contains("\"real\"", merged); Assert.DoesNotContain(ConfigSecretScrubber.Sentinel, merged); } + + // ── arch-review R2 N9: frozen scrubber fragment coverage ─────────────────── + + // Frozen non-secret allowlist (arch-review R2 N9), mirroring RequiredRoleMatrixTests: + // every string-typed property on the DataConnections config types must EITHER match + // the scrubber's secret fragments (it gets elided) OR be listed here after a + // deliberate "this is not a secret" decision. A new string property with neither + // fails EveryConfigStringProperty_IsScrubbedOrDeliberatelyNonSecret, so a future + // "SigningKey"/"Pin" cannot silently leak through List/Get/audit. + private static readonly Dictionary KnownNonSecretStringProperties = new() + { + [typeof(ZB.MOM.WW.ScadaBridge.Commons.Types.DataConnections.OpcUaEndpointConfig)] = ["EndpointUrl", "SubscriptionDisplayName"], + [typeof(ZB.MOM.WW.ScadaBridge.Commons.Types.DataConnections.OpcUaUserIdentityConfig)] = ["Username", "CertificatePath"], + [typeof(ZB.MOM.WW.ScadaBridge.Commons.Types.DataConnections.MxGatewayEndpointConfig)] = ["Endpoint", "ClientName", "CaFile", "ServerName"], + [typeof(ZB.MOM.WW.ScadaBridge.Commons.Types.DataConnections.OpcUaHeartbeatConfig)] = ["TagPath"], + [typeof(ZB.MOM.WW.ScadaBridge.Commons.Types.DataConnections.OpcUaDeadbandConfig)] = [], + }; + + [Fact] + public void EveryConfigStringProperty_IsScrubbedOrDeliberatelyNonSecret() + { + foreach (var (type, allowlist) in KnownNonSecretStringProperties) + { + var stringProps = type.GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance) + .Where(p => p.PropertyType == typeof(string)) + .Select(p => p.Name); + foreach (var name in stringProps) + { + var scrubbed = ConfigSecretScrubber.IsSecretName(name); + var allowlisted = allowlist.Contains(name); + Assert.True(scrubbed ^ allowlisted, + $"{type.Name}.{name}: must be EITHER fragment-scrubbed OR explicitly " + + "allowlisted as non-secret (and never both) — see arch-review R2 N9."); + } + } + } + + [Theory] + [InlineData("Password")] + [InlineData("CertificatePassword")] + [InlineData("ApiKey")] + public void KnownSecretProperties_MatchTheFragmentList(string name) + => Assert.True(ConfigSecretScrubber.IsSecretName(name)); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/DebugStreamHubTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/DebugStreamHubTests.cs index e9e0236d..76957af2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/DebugStreamHubTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/DebugStreamHubTests.cs @@ -1,4 +1,19 @@ +using System.Net; +using System.Text; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Connections.Features; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.SignalR; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using NSubstitute; +using ZB.MOM.WW.Auth.Abstractions.Ldap; +using ZB.MOM.WW.ScadaBridge.Communication; +using ZB.MOM.WW.ScadaBridge.Communication.Grpc; using ZB.MOM.WW.ScadaBridge.ManagementService; +using ZB.MOM.WW.ScadaBridge.Security; +using ZB.MOM.WW.ScadaBridge.Security.Auth; namespace ZB.MOM.WW.ScadaBridge.ManagementService.Tests; @@ -63,4 +78,83 @@ public class DebugStreamHubTests Assert.True(allowed); } + + // --- OnConnectedAsync delegates to ManagementAuthenticator (arch-review R2 N1) ------------ + + private readonly ILdapAuthService _ldap = Substitute.For(); + private readonly ServiceProvider _provider; + private readonly LoginThrottle _throttle; + + public DebugStreamHubTests() + { + var services = new ServiceCollection(); + services.AddSingleton>( + Options.Create(new AuthDisableLoginOptions { DisableLogin = false })); + services.AddSingleton>(Options.Create(new SecurityOptions())); + services.AddSingleton(TimeProvider.System); + services.AddSingleton(); + services.AddSingleton(_ldap); + services.AddLogging(); + _provider = services.BuildServiceProvider(); + _throttle = _provider.GetRequiredService(); + } + + private async Task ConnectAsync(string username, string password) + { + var http = new DefaultHttpContext { RequestServices = _provider }; + var creds = Convert.ToBase64String(Encoding.UTF8.GetBytes($"{username}:{password}")); + http.Request.Headers.Authorization = $"Basic {creds}"; + http.Connection.RemoteIpAddress = IPAddress.Parse("10.0.0.1"); + + var debugStreamService = new DebugStreamService( + new CommunicationService(Options.Create(new CommunicationOptions()), NullLogger.Instance), + new ServiceCollection().BuildServiceProvider(), + new SiteStreamGrpcClientFactory(NullLoggerFactory.Instance), + NullLogger.Instance); + var hubContext = Substitute.For>(); + var hub = new DebugStreamHub(debugStreamService, hubContext, NullLogger.Instance); + + var ctx = new TestHubCallerContext(http); + hub.Context = ctx; + await hub.OnConnectedAsync(); + return ctx; + } + + [Fact] + public async Task OnConnectedAsync_FailedBinds_FeedTheSharedLoginThrottle() + { + // 5 failed hub connects for alice@10.0.0.1 must arm the same lockout the + // /management and /auth surfaces consult (arch-review R2 N1). + _ldap.AuthenticateAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(LdapAuthResult.Fail(LdapAuthFailure.BadCredentials)); + for (var i = 0; i < 5; i++) await ConnectAsync("alice", "wrong"); // each Aborted + Assert.True(_throttle.IsLockedOut("alice", "10.0.0.1")); + } + + [Fact] + public async Task OnConnectedAsync_LockedOutKey_AbortsWithoutContactingLdap() + { + for (var i = 0; i < 5; i++) _throttle.RecordFailure("alice", "10.0.0.1"); + _ldap.ClearReceivedCalls(); + var ctx = await ConnectAsync("alice", "whatever"); + Assert.True(ctx.Aborted); + await _ldap.DidNotReceiveWithAnyArgs().AuthenticateAsync(default!, default!, default); + } + + private sealed class TestHubCallerContext : HubCallerContext + { + private readonly FeatureCollection _features = new(); + public TestHubCallerContext(HttpContext http) => + _features.Set(new TestHttpContextFeature { HttpContext = http }); + public bool Aborted { get; private set; } + public override string ConnectionId => "test-conn"; + public override string? UserIdentifier => null; + public override System.Security.Claims.ClaimsPrincipal? User => null; + public override IDictionary Items { get; } = new Dictionary(); + public override IFeatureCollection Features => _features; + public override CancellationToken ConnectionAborted => CancellationToken.None; + public override void Abort() => Aborted = true; + + private sealed class TestHttpContextFeature : IHttpContextFeature { public HttpContext? HttpContext { get; set; } } + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index b52d42e5..9a336316 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -145,7 +145,7 @@ public class ManagementActorTests : TestKit, IDisposable var securedWriteRepo = Substitute.For(); securedWriteRepo.QueryAsync( Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any()) + Arg.Any?>(), Arg.Any()) .Returns(Task.FromResult>( Array.Empty())); _services.AddScoped(_ => securedWriteRepo); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs index 4d2199f4..05c6dc22 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs @@ -151,6 +151,9 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable new(new AuthenticatedUser(username, username, roles, Array.Empty()), command, Guid.NewGuid().ToString("N")); + private static ManagementEnvelope ScopedEnvelope(object command, string username, string[] permittedSiteIds, params string[] roles) => + new(new AuthenticatedUser(username, username, roles, permittedSiteIds), command, Guid.NewGuid().ToString("N")); + void IDisposable.Dispose() => Shutdown(); /// Wires a site whose single connection uses the given protocol. @@ -346,7 +349,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable [Fact] public void List_FiltersByStatusAndSite() { - _securedWriteRepo.QueryAsync("Pending", "SITE1", 0, 200, Arg.Any()) + _securedWriteRepo.QueryAsync("Pending", "SITE1", 0, 200, Arg.Any?>(), Arg.Any()) .Returns(new List { new() @@ -369,13 +372,13 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable Assert.Equal(envelope.CorrelationId, response.CorrelationId); Assert.Contains("\"id\":3", response.JsonData); Assert.Contains("alice", response.JsonData); - _securedWriteRepo.Received(1).QueryAsync("Pending", "SITE1", 0, 200, Arg.Any()); + _securedWriteRepo.Received(1).QueryAsync("Pending", "SITE1", 0, 200, Arg.Any?>(), Arg.Any()); } [Fact] public void List_NoFilters_PassesNullsToRepository() { - _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any()) + _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any?>(), Arg.Any()) .Returns(new List()); var actor = CreateActor(); @@ -385,7 +388,49 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable actor.Tell(envelope); ExpectMsg(TimeSpan.FromSeconds(5)); - _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, Arg.Any()); + _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, Arg.Any?>(), Arg.Any()); + } + + [Fact] + public void List_SiteScopedReader_ExplicitOutOfScopeSiteFilter_Unauthorized() + { + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ListSecuredWritesCommand(null, "SITE1"), "bob", ["3"], "Verifier")); + ExpectMsg(TimeSpan.FromSeconds(5)); + } + + [Fact] + public void List_SiteScopedReader_Unfiltered_QueriesOnlyPermittedIdentifiers() + { + _siteRepo.GetAllSitesAsync(Arg.Any()).Returns(new List + { + new("Site1", "SITE1") { Id = 1 }, + new("Site3", "SITE3") { Id = 3 }, + }); + _securedWriteRepo.QueryAsync(null, null, 0, 200, + Arg.Any?>(), Arg.Any()) + .Returns(new List()); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ListSecuredWritesCommand(null, null), "bob", ["3"], "Verifier")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, + Arg.Is?>(s => s != null && s.Single() == "SITE3"), + Arg.Any()); + } + + [Fact] + public void List_SystemWideReader_PassesNullPermittedFilter() + { + _securedWriteRepo.QueryAsync(null, null, 0, 200, + Arg.Any?>(), Arg.Any()) + .Returns(new List()); + var actor = CreateActor(); + actor.Tell(Envelope(new ListSecuredWritesCommand(null, null), "carol", "Operator")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.Received(1).QueryAsync(null, null, 0, 200, + Arg.Is?>(s => s == null), + Arg.Any()); } // ------------------------------------------------------------------------ @@ -620,16 +665,16 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable [Fact] public void List_WithSkipTake_ForwardsPagingAndReturnsTotalCount() { - _securedWriteRepo.QueryAsync(null, null, 10, 25, Arg.Any()) + _securedWriteRepo.QueryAsync(null, null, 10, 25, Arg.Any?>(), Arg.Any()) .Returns(new List()); - _securedWriteRepo.CountAsync(null, null, Arg.Any()).Returns(87); + _securedWriteRepo.CountAsync(null, null, Arg.Any?>(), Arg.Any()).Returns(87); var actor = CreateActor(); actor.Tell(Envelope(new ListSecuredWritesCommand(null, null, Skip: 10, Take: 25), "carol", "Operator")); var resp = ExpectMsg(TimeSpan.FromSeconds(5)); - _securedWriteRepo.Received(1).QueryAsync(null, null, 10, 25, Arg.Any()); - _securedWriteRepo.Received(1).CountAsync(null, null, Arg.Any()); + _securedWriteRepo.Received(1).QueryAsync(null, null, 10, 25, Arg.Any?>(), Arg.Any()); + _securedWriteRepo.Received(1).CountAsync(null, null, Arg.Any?>(), Arg.Any()); Assert.Contains("\"totalCount\":87", resp.JsonData); } @@ -652,7 +697,7 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable ValueJson = "false", ValueType = "Boolean", Status = "Pending", OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow.AddHours(-25) // TTL default 24h }; - _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any()) + _securedWriteRepo.QueryAsync(null, null, 0, 200, Arg.Any?>(), Arg.Any()) .Returns(new List { fresh, stale }); _securedWriteRepo.TryMarkExpiredAsync(2, Arg.Any(), Arg.Any()) .Returns(true); @@ -899,4 +944,86 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable var execute = SingleOfKind(captured, AuditKind.SecuredWriteExecute); Assert.Equal(ZB.MOM.WW.Audit.AuditOutcome.Failure, execute.Outcome); } + + // ------------------------------------------------------------------------ + // Site scope on submit / approve / reject (arch-review R2 N3) + // ------------------------------------------------------------------------ + + [Fact] + public void Submit_SiteScopedOperator_OutOfScopeSite_Unauthorized_NoRowInserted() + { + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope( + new SubmitSecuredWriteCommand("SITE1", "Gw1", "Tag.A", "true", "Boolean", "go"), + "alice", ["3"], "Operator")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.DidNotReceiveWithAnyArgs().AddAsync(default!, default); + } + + [Fact] + public void Submit_SiteScopedOperator_InScopeSite_Succeeds() + { + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope( + new SubmitSecuredWriteCommand("SITE1", "Gw1", "Tag.A", "true", "Boolean", "go"), + "alice", ["1"], "Operator")); + ExpectMsg(TimeSpan.FromSeconds(5)); + } + + [Fact] + public void Approve_SiteScopedVerifier_OutOfScopeSite_Unauthorized_NeverRelays() + { + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + _securedWriteRepo.GetAsync(42, Arg.Any()).Returns(new PendingSecuredWrite + { + Id = 42, SiteId = "SITE1", ConnectionName = "Gw1", TagPath = "Tag.A", + ValueJson = "true", ValueType = "Boolean", Status = "Pending", + OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow, + }); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ApproveSecuredWriteCommand(42, "ok"), "bob", ["3"], "Verifier")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkApprovedAsync(default, default!, default, default, default); + Assert.Equal(0, _comms.CallCount); // the device write is NEVER relayed + } + + [Fact] + public void Reject_SiteScopedVerifier_OutOfScopeSite_Unauthorized() + { + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + _securedWriteRepo.GetAsync(42, Arg.Any()).Returns(new PendingSecuredWrite + { + Id = 42, SiteId = "SITE1", ConnectionName = "Gw1", TagPath = "Tag.A", + ValueJson = "true", ValueType = "Boolean", Status = "Pending", + OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow, + }); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new RejectSecuredWriteCommand(42, "no"), "bob", ["3"], "Verifier")); + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default); + } + + [Fact] + public void Approve_ScopedAdministrator_BypassesSiteScope() + { + // A scoped Verifier who is ALSO an Administrator bypasses site scope + // (EnforceSiteScope admin bypass, ManagementActor.cs:499) — the approve + // proceeds past the scope gate rather than returning ManagementUnauthorized. + SeedSiteWithConnection(1, "SITE1", "Gw1", "MxGateway"); + _securedWriteRepo.GetAsync(42, Arg.Any()).Returns(new PendingSecuredWrite + { + Id = 42, SiteId = "SITE1", ConnectionName = "Gw1", TagPath = "Tag.A", + ValueJson = "true", ValueType = "Boolean", Status = "Pending", + OperatorUser = "alice", SubmittedAtUtc = DateTime.UtcNow, + }); + _securedWriteRepo.TryMarkApprovedAsync( + 42, Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(true); + var actor = CreateActor(); + actor.Tell(ScopedEnvelope(new ApproveSecuredWriteCommand(42, "ok"), "carol", ["3"], "Verifier", "Administrator")); + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.IsNotType(response); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ForwardedHeadersSetupTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ForwardedHeadersSetupTests.cs new file mode 100644 index 00000000..e5107601 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ForwardedHeadersSetupTests.cs @@ -0,0 +1,36 @@ +using Microsoft.AspNetCore.HttpOverrides; +using ZB.MOM.WW.ScadaBridge.Security; +using Xunit; + +namespace ZB.MOM.WW.ScadaBridge.Security.Tests; + +public class ForwardedHeadersSetupTests +{ + [Fact] + public void Build_Disabled_ReturnsNull() + => Assert.Null(ForwardedHeadersSetup.Build(new ForwardedHeadersSecurityOptions { Enabled = false })); + + [Fact] + public void Build_Enabled_ParsesProxiesAndNetworks_AndClearsDefaults() + { + var built = ForwardedHeadersSetup.Build(new ForwardedHeadersSecurityOptions + { + Enabled = true, + KnownProxies = ["10.5.0.9"], + KnownNetworks = ["172.16.0.0/12"], + })!; + Assert.Equal(ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto, built.ForwardedHeaders); + Assert.Equal(1, built.ForwardLimit); + Assert.Single(built.KnownProxies); // ONLY the configured proxy — + Assert.DoesNotContain(System.Net.IPAddress.IPv6Loopback, // the loopback default is cleared + built.KnownProxies); + Assert.Single(built.KnownIPNetworks); // net10 successor to the deprecated KnownNetworks + } + + [Theory] + [InlineData("not-an-ip", "172.16.0.0/12")] + [InlineData("10.5.0.9", "not-a-cidr")] + public void Build_MalformedEntry_ThrowsAtStartup(string proxy, string network) + => Assert.ThrowsAny(() => ForwardedHeadersSetup.Build( + new ForwardedHeadersSecurityOptions { Enabled = true, KnownProxies = [proxy], KnownNetworks = [network] })); +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs index c25c6679..91ed4b1c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/LoginThrottleTests.cs @@ -25,6 +25,12 @@ public class LoginThrottleTests private static LoginThrottle Create(TimeProvider clock, SecurityOptions? options = null) => new(clock, Options.Create(options ?? new SecurityOptions())); + private static (LoginThrottle throttle, TestTimeProvider clock) Create() + { + var clock = new TestTimeProvider(Start); + return (Create(clock), clock); + } + [Fact] public void FiveFailuresInWindow_LocksOut_ThenWindowExpiryUnlocks() { @@ -34,7 +40,7 @@ public class LoginThrottleTests for (var i = 0; i < 5; i++) throttle.RecordFailure("alice", "10.0.0.1"); Assert.True(throttle.IsLockedOut("alice", "10.0.0.1")); - Assert.False(throttle.IsLockedOut("alice", "10.0.0.2")); // per username+IP key + Assert.False(throttle.IsLockedOut("alice", "10.0.0.2")); // per username+IP key — real isolation in production requires the trusted-proxy ForwardedHeaders config (R2 N2); without it all clients share the proxy IP Assert.False(throttle.IsLockedOut("bob", "10.0.0.1")); clock.Advance(TimeSpan.FromMinutes(6)); // lockout window passed @@ -110,4 +116,29 @@ public class LoginThrottleTests Assert.False(throttle.IsLockedOut("alice", "10.0.0.1")); } + + // ── arch-review R2 N8: amortized prune ───────────────────────────────────── + + [Fact] + public void KeySpray_MapStaysBounded_AtTheHardCap() + { + var (throttle, _) = Create(); // existing helper: throttle over a fake clock + // Spray far past the cap: the over-cap eviction must run on EVERY write once + // the cap is exceeded, regardless of the amortized cadence (arch-review R2 N8). + for (var i = 0; i < LoginThrottle.MaxTrackedKeys + 500; i++) + throttle.RecordFailure($"user{i}", "10.0.0.1"); + Assert.True(throttle.TrackedKeyCount <= LoginThrottle.MaxTrackedKeys); + } + + [Fact] + public void ExpiredEntries_AreEventuallySwept_OnTheAmortizedCadence() + { + var (throttle, clock) = Create(); + throttle.RecordFailure("stale", "10.0.0.1"); + clock.Advance(TimeSpan.FromMinutes(10)); // window fully expired + for (var i = 0; i < LoginThrottle.PruneEveryNFailures; i++) + throttle.RecordFailure($"fresh{i}", "10.0.0.1"); // cadence tick fires within these + Assert.False(throttle.IsLockedOut("stale", "10.0.0.1")); + Assert.True(throttle.TrackedKeyCount <= LoginThrottle.PruneEveryNFailures); // "stale" swept + } }