6 Commits

30 changed files with 1727 additions and 220 deletions
+31 -7
View File
@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 13 |
| Open findings | 10 |
## Summary
@@ -53,7 +53,7 @@ are High severity and should be addressed before production use.
|--|--|
| Severity | High |
| Category | Concurrency & thread safety |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:17`, `:32`, `:40`, `:89`, `:123-128` |
**Description**
@@ -76,7 +76,12 @@ compile at most once.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): replaced the plain `Dictionary` handler
cache with a `ConcurrentDictionary`; `RemoveHandler` now uses `TryRemove`; the
lazy-compile path in `ExecuteAsync` compiles outside the cache and inserts atomically
via `GetOrAdd` so concurrent first-callers share one handler. Regression tests
`ConcurrentLazyCompile_SameMethod_DoesNotCorruptCache` and
`ConcurrentRegisterAndExecute_DoesNotThrow` added.
### InboundAPI-002 — Lazy compilation is a check-then-act race with no atomicity
@@ -114,7 +119,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs:22-23`, consumed by `src/ScadaLink.InboundAPI/ApiKeyValidator.cs:33` |
**Description**
@@ -138,7 +143,17 @@ match-position timing.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): `ApiKeyValidator` no longer calls the
secret-equality lookup `GetApiKeyByValueAsync` (the SQL `WHERE KeyValue = @secret`
timing oracle). It now fetches all keys via `GetAllApiKeysAsync` and matches the
secret in-process with `CryptographicOperations.FixedTimeEquals` over the UTF-8 bytes,
scanning every candidate so neither match position nor secret length is observable.
Regression tests `ValidateAsync_DoesNotUseSecretEqualityLookup`,
`ValidateAsync_WrongKey_ConstantTimePath_Returns401`, and
`ValidateAsync_KeyOfDifferentLength_Returns401` added. Note: the timing-oracle method
`GetApiKeyByValueAsync` remains on `IInboundApiRepository` (it is outside this module);
removing it from the repository is left as separate follow-up since the validator no
longer depends on it.
### InboundAPI-004 — Client disconnect is misreported as a script timeout
@@ -178,7 +193,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:56-93` |
**Description**
@@ -205,7 +220,16 @@ forbidden-API checker so the trust model is enforced consistently. Reject the me
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): added `ForbiddenApiChecker`, a Roslyn
`CSharpSyntaxWalker` that statically rejects scripts referencing forbidden namespaces
(`System.IO`, `System.Diagnostics`, `System.Threading` except `Tasks`,
`System.Reflection`, `System.Net`, `System.Runtime.InteropServices`, `Microsoft.Win32`)
whether reached via a `using` directive or a fully-qualified name. `CompileAndRegister`
now runs the check before Roslyn compilation and refuses to register (and logs) a
violating method; `ExecuteAsync`'s lazy-compile path is gated by the same check.
Regression tests `CompileAndRegister_ForbiddenApi_RejectsScript` (theory),
`ExecuteAsync_ForbiddenApiScript_DoesNotRunAndReturnsFailure`, and
`CompileAndRegister_PermittedScript_StillRegisters` added.
### InboundAPI-006 — No request body size limit on the inbound endpoint
+35 -7
View File
@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 13 |
| Open findings | 10 |
## Summary
@@ -51,7 +51,7 @@ authorization bypass with no workaround.
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ManagementService/ManagementActor.cs:1465`, `:1481`, `:1493`, `:641`, `:649` |
**Description**
@@ -76,7 +76,16 @@ is already loaded — call `EnforceSiteScope(user, instance.SiteId)` (which requ
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). Threaded `AuthenticatedUser` into
`HandleQueryEventLogs`, `HandleQueryParkedMessages`, `HandleRetryParkedMessage`,
`HandleDiscardParkedMessage`, and `HandleDebugSnapshot`; added an
`EnforceSiteScopeForIdentifier` helper that resolves the site by identifier and applies
`EnforceSiteScope`. `HandleDebugSnapshot` enforces against the already-loaded instance's
`SiteId`. Regression tests: `QueryEventLogs_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`QueryParkedMessages_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`RetryParkedMessage_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`DiscardParkedMessage_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`DebugSnapshot_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`.
### ManagementService-002 — Single-entity query handlers leak data across site scope
@@ -84,7 +93,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ManagementService/ManagementActor.cs:510`, `:673`, `:733`, `:774`, `:631`, `:624` |
**Description**
@@ -106,7 +115,16 @@ the resolved site ID in `HandleGetSite`, `HandleListAreas`, and `HandleGetDataCo
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). `HandleGetInstance`, `HandleGetSite`,
`HandleGetDataConnection` now take `AuthenticatedUser` and call `EnforceSiteScope` against
the resolved entity's site ID (instance `SiteId`, site `Id`, data-connection `SiteId`);
`HandleListAreas` enforces against the requested `SiteId` before querying. Regression tests:
`GetInstance_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`GetInstance_InScopeForSiteScopedUser_ReturnsSuccess`,
`GetSite_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`GetSite_OutOfScopeForAdminUser_ReturnsSuccess`,
`ListAreas_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`,
`GetDataConnection_OutOfScopeForSiteScopedUser_ReturnsUnauthorized`.
### ManagementService-003 — DebugStreamHub.SubscribeInstance performs no per-instance authorization
@@ -114,7 +132,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ManagementService/DebugStreamHub.cs:104` |
**Description**
@@ -135,7 +153,17 @@ established in `OnConnectedAsync` must be persisted on the connection (e.g. in
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). `OnConnectedAsync` now persists the resolved
roles and `PermittedSiteIds` in `Context.Items`. `SubscribeInstance` resolves the
instance's site via `ITemplateEngineRepository` and rejects the subscription (sending
`OnStreamTerminated`) when the new pure `DebugStreamHub.IsInstanceAccessAllowed` check
fails. The check grants access for the Admin role or system-wide Deployment (empty
permitted set) and otherwise requires the instance's site in the permitted set. Regression
tests: `IsInstanceAccessAllowed_SiteScopedUser_OutOfScopeInstance_Denied`,
`IsInstanceAccessAllowed_SiteScopedUser_InScopeInstance_Allowed`,
`IsInstanceAccessAllowed_SystemWideDeployment_AnySiteAllowed`,
`IsInstanceAccessAllowed_AdminRole_BypassesSiteScope`,
`IsInstanceAccessAllowed_AdminRoleCheck_IsCaseInsensitive`.
### ManagementService-004 — Actor offloads work to Task.Run instead of using PipeTo
+28 -7
View File
@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 11 |
| Open findings | 8 |
## Summary
@@ -82,7 +82,7 @@ path. Fixed by the commit whose message references `NotificationService-001`.
|--|--|
| Severity | High |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:157-167` |
**Description**
@@ -95,7 +95,14 @@ Re-throw `OperationCanceledException`/`TaskCanceledException` when `cancellation
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). Classification was rewritten around a typed
`ClassifySmtpError` helper: a caller-requested cancellation (`OperationCanceledException`/
`TaskCanceledException` while `cancellationToken.IsCancellationRequested`) now propagates
out of both `SendAsync` and `DeliverAsync` via dedicated `catch` filters instead of being
buffered. The broad `IOException` catch-all was dropped — only MailKit's typed exceptions
plus `SocketException`/`TimeoutException` are treated as transient. Regression tests
`Send_CancellationRequested_PropagatesAndDoesNotBuffer` and
`Send_TaskCanceledException_WithCancellation_Propagates`.
### NotificationService-003 — Error classification by substring matching on exception messages is fragile
@@ -103,7 +110,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:144-147`, `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:163-166` |
**Description**
@@ -116,7 +123,16 @@ Classify on MailKit's typed exceptions and `SmtpCommandException.StatusCode` (4x
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). All `ex.Message.Contains(...)` checks were
removed. The new `ClassifySmtpError` helper inspects `SmtpCommandException.StatusCode`
(numeric SMTP code: 4xx → transient, 5xx → permanent) and treats `SmtpProtocolException`,
`ServiceNotConnectedException`, `SocketException` and `TimeoutException` as transient;
anything else is `Unknown` and propagates unclassified rather than being guessed. The
permanent-promotion `catch` block in `DeliverAsync` now keys off this classification.
Regression tests `Send_Smtp5xxCommandException_ClassifiedPermanent`,
`Send_Smtp4xxCommandException_ClassifiedTransientAndBuffered`,
`Send_SmtpProtocolException_ClassifiedTransient`, and
`Send_NonSmtpExceptionWith5xxLookalikeText_NotPromotedToPermanent`.
### NotificationService-004 — `DeliverAsync` constructs two SMTP clients and leaks the used one
@@ -124,7 +140,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Performance & resource management |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.NotificationService/NotificationDeliveryService.cs:118-119` |
**Description**
@@ -143,7 +159,12 @@ Create exactly one client and dispose the one that is actually used:
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). `DeliverAsync` now invokes `_smtpClientFactory()`
exactly once and disposes the client actually used via `using var disposable = smtp as
IDisposable;`. The previous code created two `MailKitSmtpClientWrapper` instances per send
and disposed the unused one while leaking the connected one. Regression test
`Send_CreatesExactlyOneSmtpClient_AndDisposesIt` verifies the factory is invoked once and
the resulting client is disposed.
### NotificationService-005 — Non-TLS path uses `SecureSocketOptions.Auto`, contradicting the requested mode
+8 -24
View File
@@ -40,10 +40,10 @@ module file and counted in **Total**.
| Severity | Open findings |
|----------|---------------|
| Critical | 0 |
| High | 28 |
| High | 12 |
| Medium | 100 |
| Low | 89 |
| **Total** | **217** |
| **Total** | **201** |
## Module Status
@@ -60,11 +60,11 @@ module file and counted in **Total**.
| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-16 | `9c60592` | 0/0/7/4 | 11 | 14 |
| [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-16 | `9c60592` | 0/0/5/5 | 10 | 12 |
| [Host](Host/findings.md) | 2026-05-16 | `9c60592` | 0/0/3/7 | 10 | 11 |
| [InboundAPI](InboundAPI/findings.md) | 2026-05-16 | `9c60592` | 0/3/5/5 | 13 | 13 |
| [ManagementService](ManagementService/findings.md) | 2026-05-16 | `9c60592` | 0/3/5/5 | 13 | 13 |
| [NotificationService](NotificationService/findings.md) | 2026-05-16 | `9c60592` | 0/3/5/3 | 11 | 12 |
| [Security](Security/findings.md) | 2026-05-16 | `9c60592` | 0/3/4/4 | 11 | 11 |
| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-16 | `9c60592` | 0/4/4/3 | 11 | 11 |
| [InboundAPI](InboundAPI/findings.md) | 2026-05-16 | `9c60592` | 0/0/5/5 | 10 | 13 |
| [ManagementService](ManagementService/findings.md) | 2026-05-16 | `9c60592` | 0/0/5/5 | 10 | 13 |
| [NotificationService](NotificationService/findings.md) | 2026-05-16 | `9c60592` | 0/0/5/3 | 8 | 12 |
| [Security](Security/findings.md) | 2026-05-16 | `9c60592` | 0/0/4/4 | 8 | 11 |
| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-16 | `9c60592` | 0/0/4/3 | 7 | 11 |
| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-16 | `9c60592` | 0/3/8/5 | 16 | 16 |
| [StoreAndForward](StoreAndForward/findings.md) | 2026-05-16 | `9c60592` | 0/2/4/6 | 12 | 14 |
| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-16 | `9c60592` | 0/5/5/4 | 14 | 14 |
@@ -80,28 +80,12 @@ description, location, recommendation — lives in the module's `findings.md`.
_None open._
### High (28)
### High (12)
| ID | Module | Title |
|----|--------|-------|
| ClusterInfrastructure-001 | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | Module implements none of its documented responsibilities |
| DeploymentManager-006 | [DeploymentManager](DeploymentManager/findings.md) | Query-the-site-before-redeploy idempotency requirement not implemented |
| InboundAPI-001 | [InboundAPI](InboundAPI/findings.md) | Singleton script handler cache mutated without synchronization |
| InboundAPI-003 | [InboundAPI](InboundAPI/findings.md) | API key compared with non-constant-time string equality |
| InboundAPI-005 | [InboundAPI](InboundAPI/findings.md) | Compiled API scripts run with no script-trust-model enforcement |
| ManagementService-001 | [ManagementService](ManagementService/findings.md) | Remote-query and debug-snapshot handlers bypass site-scope enforcement |
| ManagementService-002 | [ManagementService](ManagementService/findings.md) | Single-entity query handlers leak data across site scope |
| ManagementService-003 | [ManagementService](ManagementService/findings.md) | DebugStreamHub.SubscribeInstance performs no per-instance authorization |
| NotificationService-002 | [NotificationService](NotificationService/findings.md) | `TimeoutException`/`OperationCanceledException` misclassified as transient |
| NotificationService-003 | [NotificationService](NotificationService/findings.md) | Error classification by substring matching on exception messages is fragile |
| NotificationService-004 | [NotificationService](NotificationService/findings.md) | `DeliverAsync` constructs two SMTP clients and leaks the used one |
| Security-001 | [Security](Security/findings.md) | StartTLS upgrade path is unreachable dead code |
| Security-002 | [Security](Security/findings.md) | Authentication cookie is not marked `Secure` |
| Security-003 | [Security](Security/findings.md) | JWT signing key length is never validated |
| SiteEventLogging-001 | [SiteEventLogging](SiteEventLogging/findings.md) | `PRAGMA incremental_vacuum` is a no-op; storage cap cannot reclaim space |
| SiteEventLogging-002 | [SiteEventLogging](SiteEventLogging/findings.md) | Storage-cap purge deletes the entire table when space is not reclaimed |
| SiteEventLogging-003 | [SiteEventLogging](SiteEventLogging/findings.md) | Shared `SqliteConnection` used by purge and query without the write lock |
| SiteEventLogging-004 | [SiteEventLogging](SiteEventLogging/findings.md) | Event-log handler runs as a cluster singleton that can land on the standby node |
| SiteRuntime-001 | [SiteRuntime](SiteRuntime/findings.md) | `Instance.SetAttribute` never writes to the Data Connection Layer |
| SiteRuntime-002 | [SiteRuntime](SiteRuntime/findings.md) | `RouteInboundApiSetAttributes` always treats writes as static overrides |
| SiteRuntime-003 | [SiteRuntime](SiteRuntime/findings.md) | Redeployment relies on a fixed 500 ms reschedule and can collide on the child actor name |
+24 -7
View File
@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 11 |
| Open findings | 8 |
## Summary
@@ -51,7 +51,7 @@ defects that should be fixed before any production deployment.
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Security/LdapAuthService.cs:37-47` |
**Description**
@@ -74,7 +74,12 @@ session is encrypted before binding. Remove the unreachable conditional.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). Added an explicit `LdapTransport` enum
(`Ldaps`/`StartTls`/`None`); `SecureSocketLayer` is set only for LDAPS, and the
StartTLS branch now connects in plaintext, calls `StartTls()`, and verifies
`connection.Tls` before binding. `LdapUseTls` is retained as a compatibility shim
mapping onto the enum. Regression tests `AuthenticateAsync_StartTlsTransport_AttemptsConnection`
and `AuthenticateAsync_NoTlsTransport_RejectedWithoutAllowInsecure`.
### Security-002 — Authentication cookie is not marked `Secure`
@@ -82,7 +87,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Security/ServiceCollectionExtensions.cs:16-23` |
**Description**
@@ -102,7 +107,12 @@ the documented 15-minute JWT / 30-minute idle policy.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). Confirmed the cookie is configured in this
module (`ServiceCollectionExtensions.AddSecurity`), not CentralUI — no misattribution.
Added `options.Cookie.SecurePolicy = CookieSecurePolicy.Always` so the JWT-bearing
cookie is never sent over plain HTTP. Regression test
`AddSecurity_AuthCookie_IsMarkedSecureAlways`. (`ExpireTimeSpan`/`SlidingExpiration`
tuning left as a separate, lower-priority improvement.)
### Security-003 — JWT signing key length is never validated
@@ -110,7 +120,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Security/JwtTokenService.cs:33`, `src/ScadaLink.Security/SecurityOptions.cs:42` |
**Description**
@@ -131,7 +141,14 @@ constructor so a weak key is rejected before any token is issued.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`). The `JwtTokenService` constructor now
fails fast with an `InvalidOperationException` when `JwtSigningKey` is empty or
shorter than 32 bytes (`SecurityOptions.MinJwtSigningKeyBytes`), so a weak key is
rejected before any token is issued. The misleading `SecurityOptions` XML doc was
corrected to state the requirement. Regression tests
`JwtTokenService_EmptySigningKey_ThrowsAtConstruction`,
`JwtTokenService_ShortSigningKey_ThrowsAtConstruction`, and
`JwtTokenService_AdequateSigningKey_ConstructsSuccessfully`.
### Security-004 — Search filter uses `uid=` while fallback DN construction uses `cn=`
+62 -10
View File
@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 11 |
| Open findings | 7 |
## Summary
@@ -51,7 +51,7 @@ the requirement strictly, and several documentation/maintainability issues.
|--|--|
| Severity | High |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs:100-102`, `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:36-55` |
**Description**
@@ -78,7 +78,12 @@ deletes, or measure logical data size (e.g. `page_count - freelist_count` times
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): `InitializeSchema` now sets
`PRAGMA auto_vacuum = INCREMENTAL` before any table is created, and
`GetDatabaseSizeBytes` measures logical size as `(page_count - freelist_count) *
page_size` so reclaimed pages no longer mask the size drop. The cap-purge loop now
reliably observes the database shrinking. Regression test
`PurgeByStorageCap_StopsWhenUnderCap_DoesNotEmptyTable`.
### SiteEventLogging-002 — Storage-cap purge deletes the entire table when space is not reclaimed
@@ -86,7 +91,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs:87-105` |
**Description**
@@ -111,7 +116,14 @@ removed.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): with the size measurement fixed
(SiteEventLogging-001) the cap loop terminates when the file is genuinely under the
cap. An additional guard stops the loop if the on-disk size fails to decrease across
an iteration, so a cap that can never be met no longer empties the whole table.
Regression tests `PurgeByStorageCap_StopsWhenUnderCap_DoesNotEmptyTable` (asserts the
table is not emptied and the file ends under a realistic non-zero cap) and
`PurgeByStorageCap_RemovesOldestEventsFirst` (asserts only the oldest events are
removed).
### SiteEventLogging-003 — Shared `SqliteConnection` used by purge and query without the write lock
@@ -119,7 +131,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Concurrency & thread safety |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.SiteEventLogging/EventLogPurgeService.cs:64,90,100,110,114`, `src/ScadaLink.SiteEventLogging/EventLogQueryService.cs:36`, `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:34,72` |
**Description**
@@ -145,15 +157,24 @@ makes this safer). Do not share one `SqliteConnection` across threads.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): the raw `internal Connection` property was
removed from `SiteEventLogger` and replaced with lock-guarded `WithConnection(...)`
overloads that hold the existing `_writeLock` for the duration of the caller's
delegate. `EventLogPurgeService`, `EventLogQueryService`, and `LogEventAsync` all now
access the connection exclusively through `WithConnection`, so the purge thread,
query thread, and recording threads are serialised on a single lock. `Dispose` was
also brought under the lock to avoid a dispose/use race. Regression test
`PurgeByStorageCap_ConcurrentWritesDoNotCorruptConnection` exercises purge running
concurrently with multiple writer threads.
### SiteEventLogging-004 — Event-log handler runs as a cluster singleton that can land on the standby node
| | |
|--|--|
| Severity | High |
| Severity | Low |
| Original severity | High (re-triaged down to Low on 2026-05-16 — see Re-triage note) |
| Category | Design-document adherence |
| Status | Open |
| Status | Won't Fix |
| Location | `src/ScadaLink.Host/Actors/AkkaHostedService.cs:313-336`, `src/ScadaLink.SiteEventLogging/EventLogHandlerActor.cs:21-25` |
**Description**
@@ -179,9 +200,40 @@ the active node explicitly (the node owning the Deployment Manager singleton), o
handler are guaranteed co-located. Reconcile the design doc and the inline comment
with whichever model is chosen.
**Re-triage note (2026-05-16)**
The finding's central claim — that a remote query "can be served by the standby
node and read that node's near-empty database" — is incorrect for the query path.
In `AkkaHostedService.cs` the `event-log-handler` `ClusterSingletonManager` and the
`deployment-manager` `ClusterSingletonManager` are created with the **same role**
(`siteRole`) in the **same cluster**. Akka.NET pins every cluster singleton of a
given role to the *oldest member of that role* — so all same-role singletons in a
cluster co-locate on one node. The "active node" in this codebase is, by definition,
the node hosting the `deployment-manager` singleton; the event-log query singleton
is therefore *guaranteed* to run on that same node. A `ClusterClient` query cannot
land on the standby. The inline comment in `AkkaHostedService.cs` is accurate, not
"the opposite of what happens".
A real but distinct concern exists: the *writer* (`SiteEventLogger`) is registered
as a plain per-node DI singleton (`AddSiteEventLogging`), so it records to a local
SQLite file on **every** node, including the standby. That wastes storage on the
standby but does **not** cause the query-returns-nothing symptom the finding
describes, because the query singleton always reads the *active* node's (populated)
database. Gating the writer to the active node would be a `ScadaLink.Host` wiring
change, outside this module's scope, and is a minor optimisation rather than a
correctness defect.
Re-triaged from High to Low and closed as **Won't Fix**: the High-severity
correctness claim does not hold. Any residual cleanup (gate the standby-node writer;
the comment needs no change) can be raised as a fresh Low finding against
`ScadaLink.Host` if desired.
**Resolution**
_Unresolved._
Won't Fix — 2026-05-16 (commit `<pending>`). Re-triaged: the asserted defect (query
served by standby returning an empty log) cannot occur because the event-log query
singleton and the deployment-manager singleton share a role and so always co-locate
on the active node. No code change made; see the re-triage note above.
### SiteEventLogging-005 — `LogEventAsync` performs synchronous disk I/O on the caller's thread
+1 -1
View File
@@ -45,7 +45,7 @@ def parse_findings(module):
fid = m.group(1).strip()
title = m.group(2).strip().lstrip("—–-").strip().replace("|", "\\|")
sev = re.search(r"\|\s*Severity\s*\|\s*([A-Za-z]+)", block)
status = re.search(r"\|\s*Status\s*\|\s*([A-Za-z ]+?)\s*\|", block)
status = re.search(r"\|\s*Status\s*\|\s*([A-Za-z' ]+?)\s*\|", block)
if not sev or not status:
raise SystemExit(f"{module}/findings.md: {fid} is missing a Severity or Status field")
findings.append((module, fid, sev.group(1), title, status.group(1).strip()))
+35 -1
View File
@@ -1,3 +1,5 @@
using System.Security.Cryptography;
using System.Text;
using ScadaLink.Commons.Entities.InboundApi;
using ScadaLink.Commons.Interfaces.Repositories;
@@ -30,7 +32,14 @@ public class ApiKeyValidator
return ApiKeyValidationResult.Unauthorized("Missing X-API-Key header");
}
var apiKey = await _repository.GetApiKeyByValueAsync(apiKeyValue, cancellationToken);
// InboundAPI-003: do NOT resolve the key with a secret-equality lookup
// (GetApiKeyByValueAsync translates to a SQL "WHERE KeyValue = @secret" early-exit
// comparison — a timing side-channel). Fetch all keys and match the secret
// in-process with a constant-time comparison so neither match position nor
// secret length is observable to a network attacker.
var apiKey = FindKeyConstantTime(
await _repository.GetAllApiKeysAsync(cancellationToken),
apiKeyValue);
if (apiKey == null || !apiKey.IsEnabled)
{
return ApiKeyValidationResult.Unauthorized("Invalid or disabled API key");
@@ -53,6 +62,31 @@ public class ApiKeyValidator
return ApiKeyValidationResult.Valid(apiKey, method);
}
/// <summary>
/// InboundAPI-003: Finds the key whose value matches <paramref name="candidate"/>
/// using <see cref="CryptographicOperations.FixedTimeEquals"/> over the UTF-8 bytes.
/// Every candidate row is compared so that the running time does not depend on the
/// match position; length mismatches return false without leaking length timing.
/// </summary>
private static ApiKey? FindKeyConstantTime(IEnumerable<ApiKey> keys, string candidate)
{
var candidateBytes = Encoding.UTF8.GetBytes(candidate);
ApiKey? match = null;
foreach (var key in keys)
{
var keyBytes = Encoding.UTF8.GetBytes(key.KeyValue);
if (CryptographicOperations.FixedTimeEquals(candidateBytes, keyBytes))
{
// Do not break — continuing keeps the loop's timing independent of
// where (or whether) a match is found.
match = key;
}
}
return match;
}
}
/// <summary>
@@ -0,0 +1,121 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
namespace ScadaLink.InboundAPI;
/// <summary>
/// InboundAPI-005: Enforces the ScadaLink script trust model on inbound API method
/// scripts before they are compiled into executable handlers.
///
/// The trust model (CLAUDE.md, Akka.NET conventions) forbids scripts from reaching
/// <c>System.IO</c>, <c>System.Diagnostics.Process</c>, <c>System.Threading</c>,
/// <c>System.Reflection</c>, and raw network APIs. Roslyn scripting performs no
/// API allow/deny-listing — restricting default imports is a convenience, not a
/// sandbox — so a script can fully-qualify any referenced type. This static check
/// walks the script syntax tree and rejects any reference to a forbidden namespace,
/// whether reached through a <c>using</c> directive or a fully-qualified name.
/// </summary>
public static class ForbiddenApiChecker
{
/// <summary>
/// Namespace prefixes the trust model forbids. A script segment matches if it is
/// equal to one of these or is a child namespace of it.
/// </summary>
private static readonly string[] ForbiddenNamespaces =
{
"System.IO",
"System.Diagnostics", // covers Process
"System.Threading", // Task/Tasks is explicitly re-allowed below
"System.Reflection",
"System.Net", // raw network (Sockets, HttpClient, etc.)
"System.Runtime.InteropServices",
"Microsoft.Win32",
};
/// <summary>
/// Namespaces that would otherwise be caught by a forbidden prefix but are
/// required for normal async script authoring and carry no host-access risk.
/// </summary>
private static readonly string[] AllowedExceptions =
{
"System.Threading.Tasks",
};
/// <summary>
/// Analyses the script source and returns the list of trust-model violations.
/// An empty list means the script is acceptable.
/// </summary>
public static IReadOnlyList<string> FindViolations(string scriptCode)
{
if (string.IsNullOrWhiteSpace(scriptCode))
return Array.Empty<string>();
var tree = CSharpSyntaxTree.ParseText(
scriptCode,
new CSharpParseOptions(kind: SourceCodeKind.Script));
var walker = new ForbiddenApiWalker();
walker.Visit(tree.GetRoot());
return walker.Violations;
}
private static bool IsForbidden(string dottedName)
{
foreach (var allowed in AllowedExceptions)
{
if (dottedName == allowed || dottedName.StartsWith(allowed + ".", StringComparison.Ordinal))
return false;
}
foreach (var forbidden in ForbiddenNamespaces)
{
if (dottedName == forbidden || dottedName.StartsWith(forbidden + ".", StringComparison.Ordinal))
return true;
}
return false;
}
private sealed class ForbiddenApiWalker : CSharpSyntaxWalker
{
private readonly List<string> _violations = new();
public IReadOnlyList<string> Violations => _violations;
public override void VisitUsingDirective(UsingDirectiveSyntax node)
{
if (node.Name is not null && IsForbidden(node.Name.ToString()))
_violations.Add($"forbidden namespace import '{node.Name}'");
base.VisitUsingDirective(node);
}
public override void VisitQualifiedName(QualifiedNameSyntax node)
{
// Check the longest qualified name; do not descend so a single
// System.IO.File reference is reported once, not three times.
var text = node.ToString();
if (IsForbidden(text))
{
_violations.Add($"forbidden type reference '{text}'");
return;
}
base.VisitQualifiedName(node);
}
public override void VisitMemberAccessExpression(MemberAccessExpressionSyntax node)
{
// Catches fully-qualified expressions such as System.IO.File.Delete(...).
var text = node.ToString();
if (IsForbidden(text))
{
_violations.Add($"forbidden API access '{text}'");
return;
}
base.VisitMemberAccessExpression(node);
}
}
}
@@ -1,3 +1,4 @@
using System.Collections.Concurrent;
using System.Text.Json;
using Microsoft.CodeAnalysis.CSharp.Scripting;
using Microsoft.CodeAnalysis.Scripting;
@@ -14,7 +15,11 @@ namespace ScadaLink.InboundAPI;
public class InboundScriptExecutor
{
private readonly ILogger<InboundScriptExecutor> _logger;
private readonly Dictionary<string, Func<InboundScriptContext, Task<object?>>> _scriptHandlers = new();
// InboundAPI-001: this executor is registered as a singleton and its handler cache
// is read and written from concurrent ASP.NET request threads. A plain Dictionary is
// not safe for concurrent read/write, so a ConcurrentDictionary is used throughout.
private readonly ConcurrentDictionary<string, Func<InboundScriptContext, Task<object?>>> _scriptHandlers = new();
private readonly IServiceProvider _serviceProvider;
@@ -37,18 +42,45 @@ public class InboundScriptExecutor
/// </summary>
public void RemoveHandler(string methodName)
{
_scriptHandlers.Remove(methodName);
_scriptHandlers.TryRemove(methodName, out _);
}
/// <summary>
/// Compiles and registers a single API method script.
/// Compiles and registers a single API method script. Returns <c>false</c> if the
/// script is empty, fails Roslyn compilation, or violates the script trust model.
/// </summary>
public bool CompileAndRegister(ApiMethod method)
=> Compile(method) is { } handler && Register(method.Name, handler);
private bool Register(string methodName, Func<InboundScriptContext, Task<object?>> handler)
{
_scriptHandlers[methodName] = handler;
return true;
}
/// <summary>
/// Compiles a single API method script into an executable handler. Returns
/// <c>null</c> when the script is missing, fails to compile, or violates the
/// script trust model (InboundAPI-005). Does not mutate the handler cache.
/// </summary>
private Func<InboundScriptContext, Task<object?>>? Compile(ApiMethod method)
{
if (string.IsNullOrWhiteSpace(method.Script))
{
_logger.LogWarning("API method {Method} has no script code", method.Name);
return false;
return null;
}
// InboundAPI-005: enforce the script trust model before compiling. Roslyn
// scripting performs no API allow/deny-listing, so forbidden namespaces must
// be rejected statically or the script could reach the host process.
var violations = ForbiddenApiChecker.FindViolations(method.Script);
if (violations.Count > 0)
{
_logger.LogWarning(
"API method {Method} script rejected — trust model violation(s): {Violations}",
method.Name, string.Join("; ", violations));
return null;
}
try
@@ -83,22 +115,20 @@ public class InboundScriptExecutor
_logger.LogWarning(
"API method {Method} script compilation failed: {Errors}",
method.Name, string.Join("; ", errors));
return false;
return null;
}
_scriptHandlers[method.Name] = async ctx =>
_logger.LogInformation("API method {Method} script compiled", method.Name);
return async ctx =>
{
var state = await compiled.RunAsync(ctx, ctx.CancellationToken);
return state.ReturnValue;
};
_logger.LogInformation("API method {Method} script compiled and registered", method.Name);
return true;
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to compile API method {Method} script", method.Name);
return false;
return null;
}
}
@@ -119,15 +149,18 @@ public class InboundScriptExecutor
var context = new InboundScriptContext(parameters, route, cts.Token);
object? result;
if (!_scriptHandlers.TryGetValue(method.Name, out var handler))
{
// Lazy compile on first request (handles methods created after startup)
if (!CompileAndRegister(method))
// Lazy compile on first request (handles methods created after startup).
// Compile outside the cache so a failed compile is not stored, then add
// atomically so concurrent first-callers share a single handler instance.
var compiled = Compile(method);
if (compiled == null)
return new InboundScriptResult(false, null, "Script compilation failed for this method");
handler = _scriptHandlers[method.Name];
handler = _scriptHandlers.GetOrAdd(method.Name, compiled);
}
result = await handler(context).WaitAsync(cts.Token);
var result = await handler(context).WaitAsync(cts.Token);
var resultJson = result != null
? JsonSerializer.Serialize(result)
@@ -2,6 +2,7 @@ using System.Text;
using Microsoft.AspNetCore.SignalR;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using ScadaLink.Commons.Interfaces.Repositories;
using ScadaLink.Commons.Messages.DebugView;
using ScadaLink.Commons.Messages.Streaming;
using ScadaLink.Communication;
@@ -17,6 +18,26 @@ namespace ScadaLink.ManagementService;
public class DebugStreamHub : Hub
{
private const string SessionIdKey = "DebugStreamSessionId";
private const string RolesKey = "DebugStreamRoles";
private const string PermittedSiteIdsKey = "DebugStreamPermittedSiteIds";
/// <summary>
/// Pure site-scope authorization check for a debug-stream subscription.
/// Returns true when the caller may subscribe to a debug stream for an instance
/// belonging to <paramref name="instanceSiteId"/>.
/// Admin role, or an empty <paramref name="permittedSiteIds"/> (system-wide
/// Deployment), grants access to any site; otherwise the instance's site must be
/// in the permitted set.
/// </summary>
public static bool IsInstanceAccessAllowed(
IReadOnlyCollection<string> roles,
IReadOnlyCollection<string> permittedSiteIds,
int instanceSiteId)
{
if (roles.Contains("Admin", StringComparer.OrdinalIgnoreCase)) return true;
if (permittedSiteIds.Count == 0) return true; // system-wide deployment
return permittedSiteIds.Contains(instanceSiteId.ToString());
}
private readonly DebugStreamService _debugStreamService;
private readonly IHubContext<DebugStreamHub> _hubContext;
@@ -93,6 +114,11 @@ public class DebugStreamHub : Hub
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();
_logger.LogInformation("DebugStreamHub connection established for {Username}", username);
await base.OnConnectedAsync();
}
@@ -108,6 +134,41 @@ public class DebugStreamHub : Hub
var connectionId = Context.ConnectionId;
// Per-instance site-scope enforcement: a site-scoped Deployment user must not
// be able to stream an instance belonging to a site outside their scope.
var httpContext = Context.GetHttpContext();
if (httpContext == null)
{
_logger.LogWarning("DebugStreamHub: {ConnectionId} subscribe rejected — no HTTP context", connectionId);
await Clients.Caller.SendAsync("OnStreamTerminated", "Authorization context unavailable.");
return;
}
var roles = Context.Items.TryGetValue(RolesKey, out var rolesObj) && rolesObj is string[] r
? r : Array.Empty<string>();
var permittedSiteIds = Context.Items.TryGetValue(PermittedSiteIdsKey, out var sitesObj) && sitesObj is string[] s
? s : Array.Empty<string>();
var instanceRepo = httpContext.RequestServices.GetRequiredService<ITemplateEngineRepository>();
var instance = await instanceRepo.GetInstanceByIdAsync(instanceId);
if (instance == null)
{
_logger.LogWarning("DebugStreamHub: {ConnectionId} subscribe rejected — instance {InstanceId} not found",
connectionId, instanceId);
await Clients.Caller.SendAsync("OnStreamTerminated", $"Instance {instanceId} not found.");
return;
}
if (!IsInstanceAccessAllowed(roles, permittedSiteIds, instance.SiteId))
{
_logger.LogWarning(
"DebugStreamHub: {ConnectionId} subscribe to instance {InstanceId} denied — site {SiteId} outside permitted scope",
connectionId, instanceId, instance.SiteId);
await Clients.Caller.SendAsync("OnStreamTerminated",
$"Access denied: instance {instanceId} belongs to a site outside your Deployment scope.");
return;
}
try
{
// Use IHubContext for callbacks — the hub instance is transient (disposed after method returns),
@@ -164,7 +164,7 @@ public class ManagementActor : ReceiveActor
// Instances
ListInstancesCommand cmd => await HandleListInstances(sp, cmd, user),
GetInstanceCommand cmd => await HandleGetInstance(sp, cmd),
GetInstanceCommand cmd => await HandleGetInstance(sp, cmd, user),
CreateInstanceCommand cmd => await HandleCreateInstance(sp, cmd, user),
MgmtDeployInstanceCommand cmd => await HandleDeployInstance(sp, cmd, user),
MgmtEnableInstanceCommand cmd => await HandleEnableInstance(sp, cmd, user),
@@ -179,18 +179,18 @@ public class ManagementActor : ReceiveActor
// Sites
ListSitesCommand => await HandleListSites(sp, user),
GetSiteCommand cmd => await HandleGetSite(sp, cmd),
GetSiteCommand cmd => await HandleGetSite(sp, cmd, user),
CreateSiteCommand cmd => await HandleCreateSite(sp, cmd, user.Username),
UpdateSiteCommand cmd => await HandleUpdateSite(sp, cmd, user.Username),
DeleteSiteCommand cmd => await HandleDeleteSite(sp, cmd, user.Username),
ListAreasCommand cmd => await HandleListAreas(sp, cmd),
ListAreasCommand cmd => await HandleListAreas(sp, cmd, user),
CreateAreaCommand cmd => await HandleCreateArea(sp, cmd, user.Username),
DeleteAreaCommand cmd => await HandleDeleteArea(sp, cmd, user.Username),
UpdateAreaCommand cmd => await HandleUpdateArea(sp, cmd, user.Username),
// Data Connections
ListDataConnectionsCommand cmd => await HandleListDataConnections(sp, cmd),
GetDataConnectionCommand cmd => await HandleGetDataConnection(sp, cmd),
GetDataConnectionCommand cmd => await HandleGetDataConnection(sp, cmd, user),
CreateDataConnectionCommand cmd => await HandleCreateDataConnection(sp, cmd, user.Username),
UpdateDataConnectionCommand cmd => await HandleUpdateDataConnection(sp, cmd, user.Username),
DeleteDataConnectionCommand cmd => await HandleDeleteDataConnection(sp, cmd, user.Username),
@@ -263,11 +263,11 @@ public class ManagementActor : ReceiveActor
GetSiteHealthCommand cmd => HandleGetSiteHealth(sp, cmd),
// Remote Queries
QueryEventLogsCommand cmd => await HandleQueryEventLogs(sp, cmd),
QueryParkedMessagesCommand cmd => await HandleQueryParkedMessages(sp, cmd),
RetryParkedMessageCommand cmd => await HandleRetryParkedMessage(sp, cmd),
DiscardParkedMessageCommand cmd => await HandleDiscardParkedMessage(sp, cmd),
DebugSnapshotCommand cmd => await HandleDebugSnapshot(sp, cmd),
QueryEventLogsCommand cmd => await HandleQueryEventLogs(sp, cmd, user),
QueryParkedMessagesCommand cmd => await HandleQueryParkedMessages(sp, cmd, user),
RetryParkedMessageCommand cmd => await HandleRetryParkedMessage(sp, cmd, user),
DiscardParkedMessageCommand cmd => await HandleDiscardParkedMessage(sp, cmd, user),
DebugSnapshotCommand cmd => await HandleDebugSnapshot(sp, cmd, user),
// Role resolution (for CLI LDAP auth)
ResolveRolesCommand cmd => await HandleResolveRoles(sp, cmd),
@@ -329,6 +329,21 @@ public class ManagementActor : ReceiveActor
EnforceSiteScope(user, instance.SiteId);
}
/// <summary>
/// Resolves a site by its string identifier and enforces site-scope.
/// Used by remote-query handlers that key off the site identifier rather than its ID.
/// </summary>
private static async Task EnforceSiteScopeForIdentifier(IServiceProvider sp, AuthenticatedUser user, string siteIdentifier)
{
if (user.PermittedSiteIds.Length == 0) return;
if (user.Roles.Contains("Admin", StringComparer.OrdinalIgnoreCase)) return;
var repo = sp.GetRequiredService<ISiteRepository>();
var site = await repo.GetSiteByIdentifierAsync(siteIdentifier);
if (site != null)
EnforceSiteScope(user, site.Id);
}
/// <summary>
/// Helper to log an audit entry after a successful mutation.
/// </summary>
@@ -507,10 +522,13 @@ public class ManagementActor : ReceiveActor
return instances;
}
private static async Task<object?> HandleGetInstance(IServiceProvider sp, GetInstanceCommand cmd)
private static async Task<object?> HandleGetInstance(IServiceProvider sp, GetInstanceCommand cmd, AuthenticatedUser user)
{
var repo = sp.GetRequiredService<ITemplateEngineRepository>();
return await repo.GetInstanceByIdAsync(cmd.InstanceId);
var instance = await repo.GetInstanceByIdAsync(cmd.InstanceId);
if (instance != null)
EnforceSiteScope(user, instance.SiteId);
return instance;
}
private static async Task<object?> HandleCreateInstance(IServiceProvider sp, CreateInstanceCommand cmd, AuthenticatedUser user)
@@ -638,16 +656,18 @@ public class ManagementActor : ReceiveActor
: throw new InvalidOperationException(result.Error);
}
private static async Task<object?> HandleRetryParkedMessage(IServiceProvider sp, RetryParkedMessageCommand cmd)
private static async Task<object?> HandleRetryParkedMessage(IServiceProvider sp, RetryParkedMessageCommand cmd, AuthenticatedUser user)
{
await EnforceSiteScopeForIdentifier(sp, user, cmd.SiteIdentifier);
var commService = sp.GetRequiredService<CommunicationService>();
var request = new Commons.Messages.RemoteQuery.ParkedMessageRetryRequest(
Guid.NewGuid().ToString("N"), cmd.SiteIdentifier, cmd.MessageId, DateTimeOffset.UtcNow);
return await commService.RetryParkedMessageAsync(cmd.SiteIdentifier, request);
}
private static async Task<object?> HandleDiscardParkedMessage(IServiceProvider sp, DiscardParkedMessageCommand cmd)
private static async Task<object?> HandleDiscardParkedMessage(IServiceProvider sp, DiscardParkedMessageCommand cmd, AuthenticatedUser user)
{
await EnforceSiteScopeForIdentifier(sp, user, cmd.SiteIdentifier);
var commService = sp.GetRequiredService<CommunicationService>();
var request = new Commons.Messages.RemoteQuery.ParkedMessageDiscardRequest(
Guid.NewGuid().ToString("N"), cmd.SiteIdentifier, cmd.MessageId, DateTimeOffset.UtcNow);
@@ -670,10 +690,13 @@ public class ManagementActor : ReceiveActor
return sites;
}
private static async Task<object?> HandleGetSite(IServiceProvider sp, GetSiteCommand cmd)
private static async Task<object?> HandleGetSite(IServiceProvider sp, GetSiteCommand cmd, AuthenticatedUser user)
{
var repo = sp.GetRequiredService<ISiteRepository>();
return await repo.GetSiteByIdAsync(cmd.SiteId);
var site = await repo.GetSiteByIdAsync(cmd.SiteId);
if (site != null)
EnforceSiteScope(user, site.Id);
return site;
}
private static async Task<object?> HandleCreateSite(IServiceProvider sp, CreateSiteCommand cmd, string user)
@@ -730,8 +753,9 @@ public class ManagementActor : ReceiveActor
return true;
}
private static async Task<object?> HandleListAreas(IServiceProvider sp, ListAreasCommand cmd)
private static async Task<object?> HandleListAreas(IServiceProvider sp, ListAreasCommand cmd, AuthenticatedUser user)
{
EnforceSiteScope(user, cmd.SiteId);
var repo = sp.GetRequiredService<ICentralUiRepository>();
return await repo.GetAreaTreeBySiteIdAsync(cmd.SiteId);
}
@@ -771,10 +795,13 @@ public class ManagementActor : ReceiveActor
return await repo.GetAllDataConnectionsAsync();
}
private static async Task<object?> HandleGetDataConnection(IServiceProvider sp, GetDataConnectionCommand cmd)
private static async Task<object?> HandleGetDataConnection(IServiceProvider sp, GetDataConnectionCommand cmd, AuthenticatedUser user)
{
var repo = sp.GetRequiredService<ISiteRepository>();
return await repo.GetDataConnectionByIdAsync(cmd.DataConnectionId);
var conn = await repo.GetDataConnectionByIdAsync(cmd.DataConnectionId);
if (conn != null)
EnforceSiteScope(user, conn.SiteId);
return conn;
}
private static async Task<object?> HandleCreateDataConnection(IServiceProvider sp, CreateDataConnectionCommand cmd, string user)
@@ -1462,8 +1489,9 @@ public class ManagementActor : ReceiveActor
// Remote Query handlers
// ========================================================================
private static async Task<object?> HandleQueryEventLogs(IServiceProvider sp, QueryEventLogsCommand cmd)
private static async Task<object?> HandleQueryEventLogs(IServiceProvider sp, QueryEventLogsCommand cmd, AuthenticatedUser user)
{
await EnforceSiteScopeForIdentifier(sp, user, cmd.SiteIdentifier);
var commService = sp.GetRequiredService<CommunicationService>();
var request = new EventLogQueryRequest(
Guid.NewGuid().ToString("N"),
@@ -1478,8 +1506,9 @@ public class ManagementActor : ReceiveActor
return await commService.QueryEventLogsAsync(cmd.SiteIdentifier, request);
}
private static async Task<object?> HandleQueryParkedMessages(IServiceProvider sp, QueryParkedMessagesCommand cmd)
private static async Task<object?> HandleQueryParkedMessages(IServiceProvider sp, QueryParkedMessagesCommand cmd, AuthenticatedUser user)
{
await EnforceSiteScopeForIdentifier(sp, user, cmd.SiteIdentifier);
var commService = sp.GetRequiredService<CommunicationService>();
var request = new ParkedMessageQueryRequest(
Guid.NewGuid().ToString("N"),
@@ -1490,12 +1519,14 @@ public class ManagementActor : ReceiveActor
return await commService.QueryParkedMessagesAsync(cmd.SiteIdentifier, request);
}
private static async Task<object?> HandleDebugSnapshot(IServiceProvider sp, DebugSnapshotCommand cmd)
private static async Task<object?> HandleDebugSnapshot(IServiceProvider sp, DebugSnapshotCommand cmd, AuthenticatedUser user)
{
var instanceRepo = sp.GetRequiredService<ITemplateEngineRepository>();
var instance = await instanceRepo.GetInstanceByIdAsync(cmd.InstanceId)
?? throw new InvalidOperationException($"Instance {cmd.InstanceId} not found.");
EnforceSiteScope(user, instance.SiteId);
var siteRepo = sp.GetRequiredService<ISiteRepository>();
var site = await siteRepo.GetSiteByIdAsync(instance.SiteId)
?? throw new InvalidOperationException($"Site {instance.SiteId} not found.");
@@ -1,4 +1,7 @@
using System.Net.Sockets;
using System.Text.Json;
using MailKit;
using MailKit.Net.Smtp;
using Microsoft.Extensions.Logging;
using ScadaLink.Commons.Entities.Notifications;
using ScadaLink.Commons.Interfaces.Repositories;
@@ -76,7 +79,12 @@ public class NotificationDeliveryService : INotificationDeliveryService
_logger.LogError(ex, "Permanent SMTP failure sending to list {List}", listName);
return new NotificationResult(false, $"Permanent SMTP error: {ex.Message}");
}
catch (Exception ex) when (IsTransientSmtpError(ex))
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
// NS-002: a caller-requested cancellation propagates; it is not buffered.
throw;
}
catch (Exception ex) when (IsTransientSmtpError(ex, cancellationToken))
{
// WP-12: Transient SMTP failure — hand to S&F
_logger.LogWarning(ex, "Transient SMTP failure sending to list {List}, buffering for retry", listName);
@@ -172,8 +180,9 @@ public class NotificationDeliveryService : INotificationDeliveryService
string body,
CancellationToken cancellationToken)
{
using var client = _smtpClientFactory() as IDisposable;
// NS-004: create exactly one client and dispose the one actually used.
var smtp = _smtpClientFactory();
using var disposable = smtp as IDisposable;
try
{
@@ -195,32 +204,80 @@ public class NotificationDeliveryService : INotificationDeliveryService
await smtp.DisconnectAsync(cancellationToken);
}
catch (Exception ex) when (ex is not SmtpPermanentException && !IsTransientSmtpError(ex))
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
// Classify unrecognized SMTP exceptions
if (ex.Message.Contains("5.", StringComparison.Ordinal) ||
ex.Message.Contains("550", StringComparison.Ordinal) ||
ex.Message.Contains("553", StringComparison.Ordinal) ||
ex.Message.Contains("554", StringComparison.Ordinal))
{
throw new SmtpPermanentException(ex.Message, ex);
}
// Default: treat as transient
// NS-002: A deliberately cancelled token must propagate as a cancellation,
// not be misclassified as a transient SMTP failure and buffered for retry.
throw;
}
catch (Exception ex) when (ClassifySmtpError(ex, cancellationToken) == SmtpErrorClass.Permanent
&& ex is not SmtpPermanentException)
{
// NS-003: Permanent SMTP failure (5xx) — surface a typed permanent exception.
throw new SmtpPermanentException(ex.Message, ex);
}
// Transient and SmtpPermanentException both propagate unchanged: SendAsync's
// catch filters (SmtpPermanentException / IsTransientSmtpError) handle them.
}
private static bool IsTransientSmtpError(Exception ex)
private enum SmtpErrorClass
{
return ex is TimeoutException
or OperationCanceledException
or System.Net.Sockets.SocketException
or IOException
|| ex.Message.Contains("4.", StringComparison.Ordinal)
|| ex.Message.Contains("421", StringComparison.Ordinal)
|| ex.Message.Contains("450", StringComparison.Ordinal)
|| ex.Message.Contains("451", StringComparison.Ordinal);
/// <summary>Cancellation or an unrecognised exception — caller decides.</summary>
Unknown,
/// <summary>Retryable failure (4xx, connection/socket/protocol error, timeout).</summary>
Transient,
/// <summary>Non-retryable failure (5xx) — must be returned to the script.</summary>
Permanent,
}
/// <summary>
/// NS-002/NS-003: Classifies an SMTP failure using MailKit's typed exceptions and
/// the numeric <see cref="SmtpStatusCode"/> rather than locale-dependent substring
/// matching on the exception message. A cancellation requested by the caller is
/// never treated as a transient SMTP error.
/// </summary>
private static SmtpErrorClass ClassifySmtpError(Exception ex, CancellationToken cancellationToken)
{
// A deliberate cancellation is not an SMTP error at all.
if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested)
{
return SmtpErrorClass.Unknown;
}
// MailKit reports SMTP command failures with the real status code; the
// SmtpStatusCode enum's underlying value is the numeric SMTP reply code.
if (ex is SmtpCommandException command)
{
var code = (int)command.StatusCode;
if (code >= 400 && code < 500)
{
return SmtpErrorClass.Transient;
}
if (code >= 500 && code < 600)
{
return SmtpErrorClass.Permanent;
}
return SmtpErrorClass.Unknown;
}
// Protocol errors, a dropped/unavailable service, socket failures and
// timeouts are all retryable — the message has not been rejected.
if (ex is SmtpProtocolException
or ServiceNotConnectedException
or SocketException
or TimeoutException)
{
return SmtpErrorClass.Transient;
}
return SmtpErrorClass.Unknown;
}
private static bool IsTransientSmtpError(Exception ex, CancellationToken cancellationToken)
{
return ClassifySmtpError(ex, cancellationToken) == SmtpErrorClass.Transient;
}
}
+13
View File
@@ -22,6 +22,19 @@ public class JwtTokenService
{
_options = options?.Value ?? throw new ArgumentNullException(nameof(options));
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
// Fail fast: a missing or short signing key produces trivially forgeable tokens.
// HMAC-SHA256 requires a key of at least 256 bits (32 bytes).
var keyByteLength = string.IsNullOrEmpty(_options.JwtSigningKey)
? 0
: Encoding.UTF8.GetByteCount(_options.JwtSigningKey);
if (keyByteLength < SecurityOptions.MinJwtSigningKeyBytes)
{
throw new InvalidOperationException(
$"SecurityOptions.JwtSigningKey must be at least {SecurityOptions.MinJwtSigningKeyBytes} bytes " +
$"(256 bits) for HMAC-SHA256; the configured key is {keyByteLength} byte(s). " +
"Configure a strong signing key before starting the service.");
}
}
public string GenerateToken(
+11 -3
View File
@@ -24,7 +24,7 @@ public class LdapAuthService
return new LdapAuthResult(false, null, null, null, "Password is required.");
// Enforce TLS unless explicitly allowed for dev/test
if (!_options.LdapUseTls && !_options.AllowInsecureLdap)
if (_options.LdapTransport == LdapTransport.None && !_options.AllowInsecureLdap)
{
return new LdapAuthResult(false, null, null, null,
"Insecure LDAP connections are not allowed. Enable TLS or set AllowInsecureLdap for dev/test.");
@@ -34,16 +34,24 @@ public class LdapAuthService
{
using var connection = new LdapConnection();
if (_options.LdapUseTls)
// LDAPS: TLS negotiated at connection time. StartTLS: connect plaintext,
// then upgrade the session before any credentials are sent.
if (_options.LdapTransport == LdapTransport.Ldaps)
{
connection.SecureSocketLayer = true;
}
await Task.Run(() => connection.Connect(_options.LdapServer, _options.LdapPort), ct);
if (_options.LdapUseTls && !connection.SecureSocketLayer)
if (_options.LdapTransport == LdapTransport.StartTls)
{
await Task.Run(() => connection.StartTls(), ct);
if (!connection.Tls)
{
return new LdapAuthResult(false, null, null, null,
"StartTLS upgrade did not produce an encrypted session.");
}
}
// Resolve the user's actual DN, then bind with their credentials
+26
View File
@@ -0,0 +1,26 @@
namespace ScadaLink.Security;
/// <summary>
/// Transport security mode for the LDAP connection. The design requires either
/// LDAPS or StartTLS in production; <see cref="None"/> is for dev/test only and
/// must be paired with <see cref="SecurityOptions.AllowInsecureLdap"/>.
/// </summary>
public enum LdapTransport
{
/// <summary>
/// LDAPS — TLS negotiated at connection time (typically port 636).
/// </summary>
Ldaps,
/// <summary>
/// StartTLS — connect in plaintext (typically port 389), then upgrade the
/// session to TLS before binding.
/// </summary>
StartTls,
/// <summary>
/// No transport security. Dev/test only — requires
/// <see cref="SecurityOptions.AllowInsecureLdap"/> to be true.
/// </summary>
None
}
+27 -1
View File
@@ -4,7 +4,24 @@ public class SecurityOptions
{
public string LdapServer { get; set; } = string.Empty;
public int LdapPort { get; set; } = 389;
public bool LdapUseTls { get; set; } = true;
/// <summary>
/// Transport security mode for the LDAP connection. Defaults to LDAPS.
/// Use <see cref="LdapTransport.StartTls"/> to connect on the plaintext port
/// and upgrade the session before binding.
/// </summary>
public LdapTransport LdapTransport { get; set; } = LdapTransport.Ldaps;
/// <summary>
/// True when the configured transport provides encryption (LDAPS or StartTLS).
/// Retained for backward compatibility: assigning a value maps onto
/// <see cref="LdapTransport"/> (true =&gt; LDAPS, false =&gt; None).
/// </summary>
public bool LdapUseTls
{
get => LdapTransport != LdapTransport.None;
set => LdapTransport = value ? LdapTransport.Ldaps : LdapTransport.None;
}
/// <summary>
/// Allow insecure (non-TLS) LDAP connections. ONLY for dev/test with GLAuth.
@@ -39,7 +56,16 @@ public class SecurityOptions
/// </summary>
public string LdapGroupAttribute { get; set; } = "memberOf";
/// <summary>
/// Symmetric HMAC-SHA256 signing key for cookie-embedded JWTs. Must be at least
/// 32 bytes (256 bits) — validated at <see cref="JwtTokenService"/> construction.
/// </summary>
public string JwtSigningKey { get; set; } = string.Empty;
/// <summary>
/// Minimum signing-key length in bytes required for HMAC-SHA256 (256 bits).
/// </summary>
public const int MinJwtSigningKeyBytes = 32;
public int JwtExpiryMinutes { get; set; } = 15;
public int IdleTimeoutMinutes { get; set; } = 30;
@@ -20,6 +20,9 @@ public static class ServiceCollectionExtensions
options.Cookie.Name = "ScadaLink.Auth";
options.Cookie.HttpOnly = true;
options.Cookie.SameSite = Microsoft.AspNetCore.Http.SameSiteMode.Strict;
// The cookie carries the embedded JWT (a bearer credential); never
// transmit it over plain HTTP. Design: "HttpOnly and Secure (requires HTTPS)".
options.Cookie.SecurePolicy = Microsoft.AspNetCore.Http.CookieSecurePolicy.Always;
});
services.AddScadaLinkAuthorization();
@@ -1,4 +1,3 @@
using Microsoft.Data.Sqlite;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
@@ -12,6 +11,9 @@ namespace ScadaLink.SiteEventLogging;
/// </summary>
public class EventLogPurgeService : BackgroundService
{
/// <summary>Number of events deleted per cap-purge batch.</summary>
private const int CapPurgeBatchSize = 1000;
private readonly SiteEventLogger _eventLogger;
private readonly SiteEventLogOptions _options;
private readonly ILogger<EventLogPurgeService> _logger;
@@ -21,7 +23,7 @@ public class EventLogPurgeService : BackgroundService
IOptions<SiteEventLogOptions> options,
ILogger<EventLogPurgeService> logger)
{
// We need the concrete type to access the connection
// We need the concrete type to funnel access through its shared lock.
_eventLogger = (SiteEventLogger)eventLogger;
_options = options.Value;
_logger = logger;
@@ -61,10 +63,13 @@ public class EventLogPurgeService : BackgroundService
{
var cutoff = DateTimeOffset.UtcNow.AddDays(-_options.RetentionDays).ToString("o");
using var cmd = _eventLogger.Connection.CreateCommand();
cmd.CommandText = "DELETE FROM site_events WHERE timestamp < $cutoff";
cmd.Parameters.AddWithValue("$cutoff", cutoff);
var deleted = cmd.ExecuteNonQuery();
var deleted = _eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = "DELETE FROM site_events WHERE timestamp < $cutoff";
cmd.Parameters.AddWithValue("$cutoff", cutoff);
return cmd.ExecuteNonQuery();
});
if (deleted > 0)
{
@@ -74,8 +79,8 @@ public class EventLogPurgeService : BackgroundService
private void PurgeByStorageCap()
{
var currentSizeBytes = GetDatabaseSizeBytes();
var capBytes = (long)_options.MaxStorageMb * 1024 * 1024;
var currentSizeBytes = GetDatabaseSizeBytes();
if (currentSizeBytes <= capBytes)
return;
@@ -84,37 +89,77 @@ public class EventLogPurgeService : BackgroundService
"Event log size {Size:F1} MB exceeds cap {Cap} MB — purging oldest events",
currentSizeBytes / (1024.0 * 1024.0), _options.MaxStorageMb);
// Delete oldest events in batches until under the cap
// Delete the oldest events in batches until the database is under the cap.
// The loop also stops if the on-disk size fails to decrease across an
// iteration (e.g. if vacuum cannot reclaim space), so a cap that can never
// be met does not silently empty the entire table.
while (currentSizeBytes > capBytes)
{
using var cmd = _eventLogger.Connection.CreateCommand();
cmd.CommandText = """
DELETE FROM site_events WHERE id IN (
SELECT id FROM site_events ORDER BY id ASC LIMIT 1000
)
""";
var deleted = cmd.ExecuteNonQuery();
if (deleted == 0) break;
var previousSizeBytes = currentSizeBytes;
// Reclaim space
using var vacuumCmd = _eventLogger.Connection.CreateCommand();
vacuumCmd.CommandText = "PRAGMA incremental_vacuum";
vacuumCmd.ExecuteNonQuery();
var deleted = _eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = $"""
DELETE FROM site_events WHERE id IN (
SELECT id FROM site_events ORDER BY id ASC LIMIT {CapPurgeBatchSize}
)
""";
var rows = cmd.ExecuteNonQuery();
// Reclaim free pages so page_count/freelist measurement reflects the
// delete. Effective because auto_vacuum = INCREMENTAL is set at schema
// creation; harmless otherwise.
using var vacuumCmd = connection.CreateCommand();
vacuumCmd.CommandText = "PRAGMA incremental_vacuum";
vacuumCmd.ExecuteNonQuery();
return rows;
});
if (deleted == 0)
break;
currentSizeBytes = GetDatabaseSizeBytes();
if (currentSizeBytes >= previousSizeBytes)
{
// Size is not shrinking despite deletes — stop rather than wipe the
// whole table. This should not happen now that logical size is
// measured, but guards against any future regression.
_logger.LogWarning(
"Event log size did not decrease after a cap-purge batch ({Size:F1} MB); " +
"stopping to avoid emptying the log",
currentSizeBytes / (1024.0 * 1024.0));
break;
}
}
}
/// <summary>
/// Returns the logical size of the database in bytes — only pages that hold live
/// data, excluding free pages on the freelist. Measuring logical size (rather than
/// the raw file size from <c>page_count</c>) means the storage-cap loop observes
/// space being reclaimed even if free pages have not yet been returned to the OS.
/// </summary>
internal long GetDatabaseSizeBytes()
{
using var pageCountCmd = _eventLogger.Connection.CreateCommand();
pageCountCmd.CommandText = "PRAGMA page_count";
var pageCount = (long)pageCountCmd.ExecuteScalar()!;
return _eventLogger.WithConnection(connection =>
{
using var pageCountCmd = connection.CreateCommand();
pageCountCmd.CommandText = "PRAGMA page_count";
var pageCount = (long)pageCountCmd.ExecuteScalar()!;
using var pageSizeCmd = _eventLogger.Connection.CreateCommand();
pageSizeCmd.CommandText = "PRAGMA page_size";
var pageSize = (long)pageSizeCmd.ExecuteScalar()!;
using var freeListCmd = connection.CreateCommand();
freeListCmd.CommandText = "PRAGMA freelist_count";
var freeListCount = (long)freeListCmd.ExecuteScalar()!;
return pageCount * pageSize;
using var pageSizeCmd = connection.CreateCommand();
pageSizeCmd.CommandText = "PRAGMA page_size";
var pageSize = (long)pageSizeCmd.ExecuteScalar()!;
var usedPages = Math.Max(0, pageCount - freeListCount);
return usedPages * pageSize;
});
}
}
@@ -33,7 +33,6 @@ public class EventLogQueryService : IEventLogQueryService
{
var pageSize = request.PageSize > 0 ? request.PageSize : _options.QueryPageSize;
using var cmd = _eventLogger.Connection.CreateCommand();
var whereClauses = new List<string>();
var parameters = new List<SqliteParameter>();
@@ -84,32 +83,42 @@ public class EventLogQueryService : IEventLogQueryService
? "WHERE " + string.Join(" AND ", whereClauses)
: "";
// Fetch pageSize + 1 to determine if there are more results
cmd.CommandText = $"""
SELECT id, timestamp, event_type, severity, instance_id, source, message, details
FROM site_events
{whereClause}
ORDER BY id ASC
LIMIT $limit
""";
cmd.Parameters.AddWithValue("$limit", pageSize + 1);
foreach (var p in parameters)
cmd.Parameters.Add(p);
var entries = new List<EventLogEntry>();
using var reader = cmd.ExecuteReader();
while (reader.Read())
// Run the read against the shared connection under the logger's write
// lock — the connection is not thread-safe and is also used by the
// recorder and the purge service on other threads.
var entries = _eventLogger.WithConnection(connection =>
{
entries.Add(new EventLogEntry(
Id: reader.GetInt64(0),
Timestamp: DateTimeOffset.Parse(reader.GetString(1)),
EventType: reader.GetString(2),
Severity: reader.GetString(3),
InstanceId: reader.IsDBNull(4) ? null : reader.GetString(4),
Source: reader.GetString(5),
Message: reader.GetString(6),
Details: reader.IsDBNull(7) ? null : reader.GetString(7)));
}
using var cmd = connection.CreateCommand();
// Fetch pageSize + 1 to determine if there are more results
cmd.CommandText = $"""
SELECT id, timestamp, event_type, severity, instance_id, source, message, details
FROM site_events
{whereClause}
ORDER BY id ASC
LIMIT $limit
""";
cmd.Parameters.AddWithValue("$limit", pageSize + 1);
foreach (var p in parameters)
cmd.Parameters.Add(p);
var rows = new List<EventLogEntry>();
using var reader = cmd.ExecuteReader();
while (reader.Read())
{
rows.Add(new EventLogEntry(
Id: reader.GetInt64(0),
Timestamp: DateTimeOffset.Parse(reader.GetString(1)),
EventType: reader.GetString(2),
Severity: reader.GetString(3),
InstanceId: reader.IsDBNull(4) ? null : reader.GetString(4),
Source: reader.GetString(5),
Message: reader.GetString(6),
Details: reader.IsDBNull(7) ? null : reader.GetString(7)));
}
return rows;
});
var hasMore = entries.Count > pageSize;
if (hasMore)
@@ -9,6 +9,11 @@ namespace ScadaLink.SiteEventLogging;
/// Only the active node generates events. Not replicated to standby.
/// On failover, the new active node starts a fresh log.
/// </summary>
/// <remarks>
/// A single <see cref="SqliteConnection"/> is owned here and is NOT thread-safe.
/// All access — recording, querying, purging — must be funnelled through
/// <see cref="WithConnection"/>, which serialises callers on a shared lock.
/// </remarks>
public class SiteEventLogger : ISiteEventLogger, IDisposable
{
private readonly SqliteConnection _connection;
@@ -31,10 +36,50 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
InitializeSchema();
}
internal SqliteConnection Connection => _connection;
/// <summary>
/// Runs <paramref name="action"/> against the shared connection while holding the
/// write lock, so purge / query / record callers on different threads never use
/// the non-thread-safe <see cref="SqliteConnection"/> concurrently.
/// Returns <see langword="false"/> without invoking the action if the logger has
/// been disposed.
/// </summary>
internal bool WithConnection(Action<SqliteConnection> action)
{
ArgumentNullException.ThrowIfNull(action);
lock (_writeLock)
{
if (_disposed) return false;
action(_connection);
return true;
}
}
/// <summary>
/// Runs <paramref name="func"/> against the shared connection while holding the
/// write lock. Throws <see cref="ObjectDisposedException"/> if the logger has
/// been disposed (callers that need a result cannot proceed without the database).
/// </summary>
internal T WithConnection<T>(Func<SqliteConnection, T> func)
{
ArgumentNullException.ThrowIfNull(func);
lock (_writeLock)
{
ObjectDisposedException.ThrowIf(_disposed, this);
return func(_connection);
}
}
private void InitializeSchema()
{
// auto_vacuum must be set before any table is created for it to take effect
// on a fresh database. With INCREMENTAL mode, PRAGMA incremental_vacuum can
// later reclaim free pages so the storage-cap purge can shrink the file.
using (var pragmaCmd = _connection.CreateCommand())
{
pragmaCmd.CommandText = "PRAGMA auto_vacuum = INCREMENTAL";
pragmaCmd.ExecuteNonQuery();
}
using var cmd = _connection.CreateCommand();
cmd.CommandText = """
CREATE TABLE IF NOT EXISTS site_events (
@@ -69,13 +114,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
var timestamp = DateTimeOffset.UtcNow.ToString("o");
lock (_writeLock)
try
{
if (_disposed) return Task.CompletedTask;
try
WithConnection(connection =>
{
using var cmd = _connection.CreateCommand();
using var cmd = connection.CreateCommand();
cmd.CommandText = """
INSERT INTO site_events (timestamp, event_type, severity, instance_id, source, message, details)
VALUES ($timestamp, $event_type, $severity, $instance_id, $source, $message, $details)
@@ -88,11 +131,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
cmd.Parameters.AddWithValue("$message", message);
cmd.Parameters.AddWithValue("$details", (object?)details ?? DBNull.Value);
cmd.ExecuteNonQuery();
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to record event: {EventType} from {Source}", eventType, source);
}
});
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to record event: {EventType} from {Source}", eventType, source);
}
return Task.CompletedTask;
@@ -100,8 +143,11 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable
public void Dispose()
{
if (_disposed) return;
_disposed = true;
_connection.Dispose();
lock (_writeLock)
{
if (_disposed) return;
_disposed = true;
_connection.Dispose();
}
}
}
@@ -37,7 +37,7 @@ public class ApiKeyValidatorTests
[Fact]
public async Task InvalidApiKey_Returns401()
{
_repository.GetApiKeyByValueAsync("bad-key").Returns((ApiKey?)null);
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey>());
var result = await _validator.ValidateAsync("bad-key", "testMethod");
Assert.False(result.IsValid);
@@ -48,7 +48,7 @@ public class ApiKeyValidatorTests
public async Task DisabledApiKey_Returns401()
{
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = false };
_repository.GetApiKeyByValueAsync("valid-key").Returns(key);
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
var result = await _validator.ValidateAsync("valid-key", "testMethod");
Assert.False(result.IsValid);
@@ -59,7 +59,7 @@ public class ApiKeyValidatorTests
public async Task ValidKey_MethodNotFound_Returns400()
{
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true };
_repository.GetApiKeyByValueAsync("valid-key").Returns(key);
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
_repository.GetMethodByNameAsync("nonExistent").Returns((ApiMethod?)null);
var result = await _validator.ValidateAsync("valid-key", "nonExistent");
@@ -73,7 +73,7 @@ public class ApiKeyValidatorTests
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true };
var method = new ApiMethod("testMethod", "return 1;") { Id = 10 };
_repository.GetApiKeyByValueAsync("valid-key").Returns(key);
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
_repository.GetMethodByNameAsync("testMethod").Returns(method);
_repository.GetApprovedKeysForMethodAsync(10).Returns(new List<ApiKey>());
@@ -88,7 +88,7 @@ public class ApiKeyValidatorTests
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true };
var method = new ApiMethod("testMethod", "return 1;") { Id = 10 };
_repository.GetApiKeyByValueAsync("valid-key").Returns(key);
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
_repository.GetMethodByNameAsync("testMethod").Returns(method);
_repository.GetApprovedKeysForMethodAsync(10).Returns(new List<ApiKey> { key });
@@ -98,4 +98,50 @@ public class ApiKeyValidatorTests
Assert.Equal(key, result.ApiKey);
Assert.Equal(method, result.Method);
}
// --- InboundAPI-003: API key must not be matched with a non-constant-time
// (timing-oracle) secret-equality lookup. ---
[Fact]
public async Task ValidateAsync_DoesNotUseSecretEqualityLookup()
{
// GetApiKeyByValueAsync translates to a SQL "WHERE KeyValue = @secret" early-exit
// comparison — a timing side-channel. The validator must not call it.
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true };
var method = new ApiMethod("testMethod", "return 1;") { Id = 10 };
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
_repository.GetMethodByNameAsync("testMethod").Returns(method);
_repository.GetApprovedKeysForMethodAsync(10).Returns(new List<ApiKey> { key });
await _validator.ValidateAsync("valid-key", "testMethod");
await _repository.DidNotReceive()
.GetApiKeyByValueAsync(Arg.Any<string>(), Arg.Any<CancellationToken>());
}
[Fact]
public async Task ValidateAsync_WrongKey_ConstantTimePath_Returns401()
{
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true };
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
var result = await _validator.ValidateAsync("wrong-key", "testMethod");
Assert.False(result.IsValid);
Assert.Equal(401, result.StatusCode);
}
[Fact]
public async Task ValidateAsync_KeyOfDifferentLength_Returns401()
{
// FixedTimeEquals over UTF-8 bytes must reject length mismatches without leaking.
var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true };
_repository.GetAllApiKeysAsync().Returns(new List<ApiKey> { key });
var result = await _validator.ValidateAsync("valid-key-with-extra", "testMethod");
Assert.False(result.IsValid);
Assert.Equal(401, result.StatusCode);
}
}
@@ -132,4 +132,111 @@ public class InboundScriptExecutorTests
Assert.True(result.Success);
Assert.Contains("ScadaLink", result.ResultJson!);
}
// --- InboundAPI-001: concurrent lazy-compile must not corrupt the handler cache ---
[Fact]
public async Task ConcurrentLazyCompile_SameMethod_DoesNotCorruptCache()
{
// Many concurrent first-callers of an uncompiled method race the lazy-compile
// path. With an unsynchronized Dictionary this can throw or return a torn/null
// handler; all calls must succeed and produce the same result.
var method = new ApiMethod("concurrent", "return 7;") { Id = 1, TimeoutSeconds = 10 };
var tasks = Enumerable.Range(0, 64).Select(_ => Task.Run(() =>
_executor.ExecuteAsync(
method,
new Dictionary<string, object?>(),
_route,
TimeSpan.FromSeconds(10)))).ToArray();
var results = await Task.WhenAll(tasks);
Assert.All(results, r =>
{
Assert.True(r.Success, r.ErrorMessage);
Assert.Equal("7", r.ResultJson);
});
}
[Fact]
public async Task ConcurrentRegisterAndExecute_DoesNotThrow()
{
// RegisterHandler/RemoveHandler racing ExecuteAsync must not crash the process
// with an InvalidOperationException from concurrent Dictionary mutation.
var method = new ApiMethod("racy", "return 1;") { Id = 1, TimeoutSeconds = 10 };
var writers = Enumerable.Range(0, 32).Select(i => Task.Run(() =>
{
for (var n = 0; n < 50; n++)
{
_executor.RegisterHandler("racy", async ctx => { await Task.CompletedTask; return i; });
_executor.RemoveHandler("racy");
}
}));
var readers = Enumerable.Range(0, 32).Select(_ => Task.Run(async () =>
{
for (var n = 0; n < 50; n++)
{
await _executor.ExecuteAsync(
method, new Dictionary<string, object?>(), _route, TimeSpan.FromSeconds(10));
}
}));
// Should complete without an unhandled concurrency exception.
await Task.WhenAll(writers.Concat(readers));
}
// --- InboundAPI-005: compiled scripts must not bypass the script trust model ---
[Theory]
[InlineData("System.IO.File.Delete(\"/tmp/x\"); return null;")]
[InlineData("System.Diagnostics.Process.Start(\"/bin/sh\"); return null;")]
[InlineData("var t = System.Reflection.Assembly.GetExecutingAssembly(); return null;")]
[InlineData("new System.Threading.Thread(() => {}).Start(); return null;")]
[InlineData("var s = new System.Net.Sockets.Socket(System.Net.Sockets.AddressFamily.InterNetwork, System.Net.Sockets.SocketType.Stream, System.Net.Sockets.ProtocolType.Tcp); return null;")]
public void CompileAndRegister_ForbiddenApi_RejectsScript(string script)
{
var method = new ApiMethod("forbidden", script) { Id = 1, TimeoutSeconds = 10 };
var registered = _executor.CompileAndRegister(method);
Assert.False(registered);
}
[Fact]
public async Task ExecuteAsync_ForbiddenApiScript_DoesNotRunAndReturnsFailure()
{
// A fully-qualified forbidden API call must be rejected at compile/register time
// so the script never executes.
var marker = System.IO.Path.Combine(
System.IO.Path.GetTempPath(), $"scadalink-pwned-{Guid.NewGuid():N}");
System.IO.File.Delete(marker);
var method = new ApiMethod("evil", $"System.IO.File.WriteAllText(@\"{marker}\", \"x\"); return 1;")
{
Id = 1,
TimeoutSeconds = 10
};
var result = await _executor.ExecuteAsync(
method, new Dictionary<string, object?>(), _route, TimeSpan.FromSeconds(10));
Assert.False(result.Success);
Assert.False(System.IO.File.Exists(marker));
}
[Fact]
public void CompileAndRegister_PermittedScript_StillRegisters()
{
// The trust-model check must not reject legitimate scripts.
var method = new ApiMethod("ok", "var list = new List<int> { 1, 2, 3 }; return list.Sum();")
{
Id = 1,
TimeoutSeconds = 10
};
Assert.True(_executor.CompileAndRegister(method));
}
}
@@ -0,0 +1,66 @@
using ScadaLink.ManagementService;
namespace ScadaLink.ManagementService.Tests;
/// <summary>
/// Tests for <see cref="DebugStreamHub"/> per-instance site-scope authorization
/// (finding ManagementService-003).
/// </summary>
public class DebugStreamHubTests
{
[Fact]
public void IsInstanceAccessAllowed_SiteScopedUser_InScopeInstance_Allowed()
{
var allowed = DebugStreamHub.IsInstanceAccessAllowed(
roles: new[] { "Deployment" },
permittedSiteIds: new[] { "1", "2" },
instanceSiteId: 2);
Assert.True(allowed);
}
[Fact]
public void IsInstanceAccessAllowed_SiteScopedUser_OutOfScopeInstance_Denied()
{
var allowed = DebugStreamHub.IsInstanceAccessAllowed(
roles: new[] { "Deployment" },
permittedSiteIds: new[] { "1", "2" },
instanceSiteId: 99);
Assert.False(allowed);
}
[Fact]
public void IsInstanceAccessAllowed_SystemWideDeployment_AnySiteAllowed()
{
// Empty permitted set == system-wide Deployment.
var allowed = DebugStreamHub.IsInstanceAccessAllowed(
roles: new[] { "Deployment" },
permittedSiteIds: Array.Empty<string>(),
instanceSiteId: 99);
Assert.True(allowed);
}
[Fact]
public void IsInstanceAccessAllowed_AdminRole_BypassesSiteScope()
{
var allowed = DebugStreamHub.IsInstanceAccessAllowed(
roles: new[] { "Admin", "Deployment" },
permittedSiteIds: new[] { "1" },
instanceSiteId: 99);
Assert.True(allowed);
}
[Fact]
public void IsInstanceAccessAllowed_AdminRoleCheck_IsCaseInsensitive()
{
var allowed = DebugStreamHub.IsInstanceAccessAllowed(
roles: new[] { "admin" },
permittedSiteIds: new[] { "1" },
instanceSiteId: 99);
Assert.True(allowed);
}
}
@@ -44,6 +44,10 @@ public class ManagementActorTests : TestKit, IDisposable
new(new AuthenticatedUser("testuser", "Test User", roles, Array.Empty<string>()),
command, Guid.NewGuid().ToString("N"));
private static ManagementEnvelope ScopedEnvelope(object command, string[] permittedSiteIds, params string[] roles) =>
new(new AuthenticatedUser("scopeduser", "Scoped User", roles, permittedSiteIds),
command, Guid.NewGuid().ToString("N"));
void IDisposable.Dispose()
{
Shutdown();
@@ -478,4 +482,190 @@ public class ManagementActorTests : TestKit, IDisposable
Assert.Equal("COMMAND_FAILED", response.ErrorCode);
Assert.Contains("Connection refused", response.Error);
}
// ========================================================================
// Site-scope enforcement tests (findings ManagementService-001 / -002)
//
// A site-scoped Deployment user (PermittedSiteIds set, no Admin role) must
// not be able to read or act on entities belonging to a site outside their
// scope. The handlers must throw SiteScopeViolationException, which the
// actor maps to ManagementUnauthorized.
// ========================================================================
private void AddSiteRepoWithSite(int siteId, string identifier)
{
var siteRepo = Substitute.For<ISiteRepository>();
siteRepo.GetSiteByIdAsync(siteId, Arg.Any<CancellationToken>())
.Returns(new Commons.Entities.Sites.Site($"Site{siteId}", identifier) { Id = siteId });
siteRepo.GetSiteByIdentifierAsync(identifier, Arg.Any<CancellationToken>())
.Returns(new Commons.Entities.Sites.Site($"Site{siteId}", identifier) { Id = siteId });
_services.AddScoped(_ => siteRepo);
}
[Fact]
public void GetInstance_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
// Instance belongs to site 2; user is scoped to site 1.
_templateRepo.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>())
.Returns(new Instance("Pump7") { Id = 7, SiteId = 2 });
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetInstanceCommand(7), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void GetInstance_InScopeForSiteScopedUser_ReturnsSuccess()
{
_templateRepo.GetInstanceByIdAsync(7, Arg.Any<CancellationToken>())
.Returns(new Instance("Pump7") { Id = 7, SiteId = 1 });
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetInstanceCommand(7), new[] { "1" }, "Deployment");
actor.Tell(envelope);
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
}
[Fact]
public void GetSite_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetSiteCommand(2), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void ListAreas_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
AddSiteRepoWithSite(2, "SITE2");
var uiRepo = Substitute.For<ICentralUiRepository>();
uiRepo.GetAreaTreeBySiteIdAsync(2, Arg.Any<CancellationToken>())
.Returns(new List<Commons.Entities.Instances.Area>());
_services.AddScoped(_ => uiRepo);
var actor = CreateActor();
var envelope = ScopedEnvelope(new ListAreasCommand(2), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void GetDataConnection_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
var siteRepo = Substitute.For<ISiteRepository>();
siteRepo.GetDataConnectionByIdAsync(5, Arg.Any<CancellationToken>())
.Returns(new Commons.Entities.Sites.DataConnection("Conn5", "OpcUa", 2) { Id = 5 });
_services.AddScoped(_ => siteRepo);
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetDataConnectionCommand(5), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void GetSite_OutOfScopeForAdminUser_ReturnsSuccess()
{
// Admin role bypasses site scoping even when PermittedSiteIds is set.
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new GetSiteCommand(2), new[] { "1" }, "Admin");
actor.Tell(envelope);
ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
}
[Fact]
public void QueryEventLogs_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
// Site SITE2 has Id 2; user scoped to site 1.
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new QueryEventLogsCommand("SITE2"), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void QueryParkedMessages_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new QueryParkedMessagesCommand("SITE2"), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void RetryParkedMessage_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new RetryParkedMessageCommand("SITE2", "msg-1"), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void DiscardParkedMessage_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new DiscardParkedMessageCommand("SITE2", "msg-1"), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
[Fact]
public void DebugSnapshot_OutOfScopeForSiteScopedUser_ReturnsUnauthorized()
{
// Instance 9 belongs to site 2; user scoped to site 1.
_templateRepo.GetInstanceByIdAsync(9, Arg.Any<CancellationToken>())
.Returns(new Instance("Pump9") { Id = 9, SiteId = 2 });
AddSiteRepoWithSite(2, "SITE2");
var actor = CreateActor();
var envelope = ScopedEnvelope(new DebugSnapshotCommand(9), new[] { "1" }, "Deployment");
actor.Tell(envelope);
var response = ExpectMsg<ManagementUnauthorized>(TimeSpan.FromSeconds(5));
Assert.Equal(envelope.CorrelationId, response.CorrelationId);
}
}
@@ -1,3 +1,5 @@
using MailKit;
using MailKit.Net.Smtp;
using Microsoft.Extensions.Logging.Abstractions;
using NSubstitute;
using NSubstitute.ExceptionExtensions;
@@ -225,4 +227,182 @@ public class NotificationDeliveryServiceTests
Assert.False(delivered); // permanent — the S&F engine parks the message
}
// ── NotificationService-002: cancellation must not be misclassified as transient ──
/// <summary>
/// Like <see cref="SetupHappyPath"/> but matches any <see cref="CancellationToken"/>,
/// so tests that pass an already-cancelled token still resolve the list/recipients.
/// </summary>
private void SetupHappyPathAnyToken()
{
var list = new NotificationList("ops-team") { Id = 1 };
var recipients = new List<NotificationRecipient>
{
new("Alice", "alice@example.com") { Id = 1, NotificationListId = 1 }
};
var smtpConfig = new SmtpConfiguration("smtp.example.com", "basic", "noreply@example.com")
{
Id = 1, Port = 587, Credentials = "user:pass", TlsMode = "starttls"
};
_repository.GetListByNameAsync("ops-team", Arg.Any<CancellationToken>()).Returns(list);
_repository.GetRecipientsByListIdAsync(1, Arg.Any<CancellationToken>()).Returns(recipients);
_repository.GetAllSmtpConfigurationsAsync(Arg.Any<CancellationToken>())
.Returns(new List<SmtpConfiguration> { smtpConfig });
}
[Fact]
public async Task Send_CancellationRequested_PropagatesAndDoesNotBuffer()
{
SetupHappyPathAnyToken();
using var cts = new CancellationTokenSource();
cts.Cancel();
_smtpClient.SendAsync(Arg.Any<string>(), Arg.Any<IEnumerable<string>>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
.Throws(new OperationCanceledException(cts.Token));
var sfService = await CreateSfServiceAsync();
var service = CreateService(sf: sfService);
await Assert.ThrowsAnyAsync<OperationCanceledException>(
() => service.SendAsync("ops-team", "Alert", "Body", cancellationToken: cts.Token));
// The cancellation propagated instead of being buffered for retry.
var depth = await sfService.GetBufferDepthAsync();
depth.TryGetValue(ScadaLink.Commons.Types.Enums.StoreAndForwardCategory.Notification, out var count);
Assert.Equal(0, count);
}
[Fact]
public async Task Send_TaskCanceledException_WithCancellation_Propagates()
{
SetupHappyPathAnyToken();
using var cts = new CancellationTokenSource();
cts.Cancel();
_smtpClient.SendAsync(Arg.Any<string>(), Arg.Any<IEnumerable<string>>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
.Throws(new TaskCanceledException());
var service = CreateService(sf: null);
await Assert.ThrowsAnyAsync<OperationCanceledException>(
() => service.SendAsync("ops-team", "Alert", "Body", cancellationToken: cts.Token));
}
// ── NotificationService-003: classify on MailKit typed exceptions / status codes ──
[Fact]
public async Task Send_Smtp5xxCommandException_ClassifiedPermanent()
{
SetupHappyPath();
// 550 MailboxUnavailable — a real permanent rejection.
_smtpClient.SendAsync(Arg.Any<string>(), Arg.Any<IEnumerable<string>>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
.Throws(new SmtpCommandException(
SmtpErrorCode.RecipientNotAccepted, SmtpStatusCode.MailboxUnavailable, "rejected"));
var service = CreateService();
var result = await service.SendAsync("ops-team", "Alert", "Body");
Assert.False(result.Success);
Assert.Contains("Permanent SMTP error", result.ErrorMessage);
}
[Fact]
public async Task Send_Smtp4xxCommandException_ClassifiedTransientAndBuffered()
{
SetupHappyPath();
// 450 MailboxBusy — a real transient failure.
_smtpClient.SendAsync(Arg.Any<string>(), Arg.Any<IEnumerable<string>>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
.Throws(new SmtpCommandException(
SmtpErrorCode.MessageNotAccepted, SmtpStatusCode.MailboxBusy, "try again"));
var sfService = await CreateSfServiceAsync();
var service = CreateService(sf: sfService);
var result = await service.SendAsync("ops-team", "Alert", "Body");
Assert.True(result.Success);
Assert.True(result.WasBuffered);
}
[Fact]
public async Task Send_NonSmtpExceptionWith5xxLookalikeText_NotPromotedToPermanent()
{
// NS-003: the old classifier promoted ANY exception whose message contained
// "5." / "550" / etc. to a permanent SMTP error — so an unrelated failure
// referencing a host like "smtp5.example.com" was silently swallowed as a
// clean permanent NotificationResult. Classification now uses MailKit's
// typed exceptions only, so a non-SMTP exception is no longer misclassified.
SetupHappyPath();
_smtpClient.SendAsync(Arg.Any<string>(), Arg.Any<IEnumerable<string>>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
.Throws(new InvalidOperationException("internal error talking to smtp5.example.com"));
var service = CreateService();
// The exception is not classified at all (not a typed SMTP failure); it
// surfaces rather than being mistaken for a permanent 5xx rejection.
await Assert.ThrowsAsync<InvalidOperationException>(
() => service.SendAsync("ops-team", "Alert", "Body"));
}
[Fact]
public async Task Send_SmtpProtocolException_ClassifiedTransient()
{
SetupHappyPath();
_smtpClient.SendAsync(Arg.Any<string>(), Arg.Any<IEnumerable<string>>(), Arg.Any<string>(), Arg.Any<string>(), Arg.Any<CancellationToken>())
.Throws(new SmtpProtocolException("protocol error"));
var sfService = await CreateSfServiceAsync();
var service = CreateService(sf: sfService);
var result = await service.SendAsync("ops-team", "Alert", "Body");
Assert.True(result.Success);
Assert.True(result.WasBuffered);
}
// ── NotificationService-004: DeliverAsync must create exactly one client and dispose it ──
private sealed class TrackingSmtpClient : ISmtpClientWrapper, IDisposable
{
public bool Disposed { get; private set; }
public Task ConnectAsync(string host, int port, bool useTls, CancellationToken cancellationToken = default)
=> Task.CompletedTask;
public Task AuthenticateAsync(string authType, string? credentials, CancellationToken cancellationToken = default)
=> Task.CompletedTask;
public Task SendAsync(string from, IEnumerable<string> bccRecipients, string subject, string body, CancellationToken cancellationToken = default)
=> Task.CompletedTask;
public Task DisconnectAsync(CancellationToken cancellationToken = default)
=> Task.CompletedTask;
public void Dispose() => Disposed = true;
}
[Fact]
public async Task Send_CreatesExactlyOneSmtpClient_AndDisposesIt()
{
SetupHappyPath();
var created = new List<TrackingSmtpClient>();
var service = new NotificationDeliveryService(
_repository,
() =>
{
var c = new TrackingSmtpClient();
created.Add(c);
return c;
},
NullLogger<NotificationDeliveryService>.Instance);
var result = await service.SendAsync("ops-team", "Alert", "Body");
Assert.True(result.Success);
Assert.Single(created); // NS-004: factory invoked once, not twice
Assert.True(created[0].Disposed); // the client actually used is disposed
}
private static async Task<StoreAndForwardService> CreateSfServiceAsync()
{
var dbName = $"file:sf_test_{Guid.NewGuid():N}?mode=memory&cache=shared";
var storage = new StoreAndForwardStorage(
$"Data Source={dbName}", NullLogger<StoreAndForwardStorage>.Instance);
await storage.InitializeAsync();
return new StoreAndForwardService(
storage, new StoreAndForwardOptions(), NullLogger<StoreAndForwardService>.Instance);
}
}
@@ -10,6 +10,7 @@
<ItemGroup>
<PackageReference Include="coverlet.collector" />
<PackageReference Include="MailKit" />
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="NSubstitute" />
<PackageReference Include="xunit" />
+118
View File
@@ -357,6 +357,124 @@ public class RoleMapperTests : IDisposable
#endregion
#region Code Review Regression Tests
/// <summary>
/// Regression tests for Security-001 (StartTLS dead code), Security-002 (cookie not
/// marked Secure), and Security-003 (JWT signing key length never validated).
/// </summary>
public class SecurityReviewRegressionTests
{
// --- Security-003: JWT signing key length validation ---
private static SecurityOptions JwtOptions(string signingKey) => new()
{
JwtSigningKey = signingKey,
JwtExpiryMinutes = 15,
IdleTimeoutMinutes = 30,
JwtRefreshThresholdMinutes = 5
};
[Fact]
public void JwtTokenService_EmptySigningKey_ThrowsAtConstruction()
{
var ex = Assert.Throws<InvalidOperationException>(() =>
new JwtTokenService(Options.Create(JwtOptions("")), NullLogger<JwtTokenService>.Instance));
Assert.Contains("JwtSigningKey", ex.Message);
}
[Fact]
public void JwtTokenService_ShortSigningKey_ThrowsAtConstruction()
{
// 31 bytes — one short of the 256-bit HMAC-SHA256 minimum.
var shortKey = new string('k', 31);
var ex = Assert.Throws<InvalidOperationException>(() =>
new JwtTokenService(Options.Create(JwtOptions(shortKey)), NullLogger<JwtTokenService>.Instance));
Assert.Contains("32", ex.Message);
}
[Fact]
public void JwtTokenService_AdequateSigningKey_ConstructsSuccessfully()
{
var key = new string('k', 32);
var service = new JwtTokenService(Options.Create(JwtOptions(key)), NullLogger<JwtTokenService>.Instance);
var token = service.GenerateToken("User", "user", new[] { "Admin" }, null);
Assert.False(string.IsNullOrEmpty(token));
}
// --- Security-002: authentication cookie must be marked Secure ---
[Fact]
public void AddSecurity_AuthCookie_IsMarkedSecureAlways()
{
var services = new ServiceCollection();
services.AddLogging();
services.AddDataProtection();
services.AddSecurity();
using var provider = services.BuildServiceProvider();
var cookieOptions = provider
.GetRequiredService<IOptionsMonitor<Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationOptions>>()
.Get(Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationDefaults.AuthenticationScheme);
Assert.Equal(
Microsoft.AspNetCore.Http.CookieSecurePolicy.Always,
cookieOptions.Cookie.SecurePolicy);
Assert.True(cookieOptions.Cookie.HttpOnly);
}
// --- Security-001: StartTLS transport must be reachable ---
[Fact]
public void SecurityOptions_LdapTransport_DefaultsToLdaps()
{
var options = new SecurityOptions();
Assert.Equal(LdapTransport.Ldaps, options.LdapTransport);
}
[Fact]
public async Task AuthenticateAsync_StartTlsTransport_AttemptsConnection()
{
// With StartTLS selected the service must not be blocked by the insecure-LDAP
// guard and must reach the connection stage (which fails against a dead host).
// This proves the StartTLS path is reachable rather than dead code.
var options = new SecurityOptions
{
LdapServer = "nonexistent.invalid",
LdapPort = 389,
LdapTransport = LdapTransport.StartTls,
LdapSearchBase = "dc=example,dc=com"
};
var service = new LdapAuthService(Options.Create(options), NullLogger<LdapAuthService>.Instance);
var result = await service.AuthenticateAsync("user", "password");
// Connection fails (host invalid) — but crucially NOT with the insecure-LDAP message.
Assert.False(result.Success);
Assert.DoesNotContain("Insecure LDAP", result.ErrorMessage ?? string.Empty);
}
[Fact]
public async Task AuthenticateAsync_NoTlsTransport_RejectedWithoutAllowInsecure()
{
var options = new SecurityOptions
{
LdapServer = "ldap.example.com",
LdapPort = 389,
LdapTransport = LdapTransport.None,
AllowInsecureLdap = false
};
var service = new LdapAuthService(Options.Create(options), NullLogger<LdapAuthService>.Instance);
var result = await service.AuthenticateAsync("user", "password");
Assert.False(result.Success);
Assert.Contains("Insecure LDAP", result.ErrorMessage);
}
}
#endregion
#region WP-9: Authorization Policy Tests
public class AuthorizationPolicyTests
@@ -40,20 +40,26 @@ public class EventLogPurgeServiceTests : IDisposable
private void InsertEventWithTimestamp(DateTimeOffset timestamp)
{
using var cmd = _eventLogger.Connection.CreateCommand();
cmd.CommandText = """
INSERT INTO site_events (timestamp, event_type, severity, source, message)
VALUES ($ts, 'script', 'Info', 'Test', 'Test message')
""";
cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o"));
cmd.ExecuteNonQuery();
_eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = """
INSERT INTO site_events (timestamp, event_type, severity, source, message)
VALUES ($ts, 'script', 'Info', 'Test', 'Test message')
""";
cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o"));
cmd.ExecuteNonQuery();
});
}
private long GetEventCount()
{
using var cmd = _eventLogger.Connection.CreateCommand();
cmd.CommandText = "SELECT COUNT(*) FROM site_events";
return (long)cmd.ExecuteScalar()!;
return _eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = "SELECT COUNT(*) FROM site_events";
return (long)cmd.ExecuteScalar()!;
});
}
[Fact]
@@ -116,4 +122,155 @@ public class EventLogPurgeServiceTests : IDisposable
Assert.True(size > 0);
}
private void InsertBulkEvents(int count)
{
// Each event carries a sizeable details payload so the database grows
// measurably and the storage cap can be exercised against a realistic file.
var details = new string('x', 2000);
_eventLogger.WithConnection(connection =>
{
for (int i = 0; i < count; i++)
{
using var cmd = connection.CreateCommand();
cmd.CommandText = """
INSERT INTO site_events (timestamp, event_type, severity, source, message, details)
VALUES ($ts, 'script', 'Info', 'Test', 'Bulk event', $details)
""";
cmd.Parameters.AddWithValue("$ts", DateTimeOffset.UtcNow.ToString("o"));
cmd.Parameters.AddWithValue("$details", details);
cmd.ExecuteNonQuery();
}
});
}
private long MinEventId()
{
return _eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = "SELECT MIN(id) FROM site_events";
var result = cmd.ExecuteScalar();
return result is long l ? l : 0;
});
}
[Fact]
public void PurgeByStorageCap_StopsWhenUnderCap_DoesNotEmptyTable()
{
// Regression test for SiteEventLogging-001 / -002:
// a realistic non-zero cap must trim the oldest events to the budget,
// not delete the entire table.
InsertBulkEvents(3000);
var purge = CreatePurgeService();
var totalSize = purge.GetDatabaseSizeBytes();
// Cap at roughly half the current database size — purge must keep some rows.
var capBytes = totalSize / 2;
var capOptions = new SiteEventLogOptions
{
DatabasePath = _dbPath,
RetentionDays = 30,
MaxStorageMb = (int)Math.Max(1, capBytes / (1024 * 1024))
};
var cappedPurge = CreatePurgeService(capOptions);
cappedPurge.RunPurge();
var remaining = GetEventCount();
Assert.True(remaining > 0, "Storage-cap purge must not delete the entire table.");
Assert.True(remaining < 3000, "Storage-cap purge must remove some events when over cap.");
// The database must actually be back under the cap after purge.
var finalSize = cappedPurge.GetDatabaseSizeBytes();
var finalCapBytes = (long)capOptions.MaxStorageMb * 1024 * 1024;
Assert.True(finalSize <= finalCapBytes,
$"Database size {finalSize} must be at or below cap {finalCapBytes} after purge.");
}
[Fact]
public void PurgeByStorageCap_RemovesOldestEventsFirst()
{
// Regression test for SiteEventLogging-002: only the oldest events
// (lowest ids) should be removed when trimming to the cap.
InsertBulkEvents(3000);
var purge = CreatePurgeService();
var totalSize = purge.GetDatabaseSizeBytes();
var capOptions = new SiteEventLogOptions
{
DatabasePath = _dbPath,
RetentionDays = 30,
MaxStorageMb = (int)Math.Max(1, (totalSize / 2) / (1024 * 1024))
};
var minIdBefore = MinEventId();
var cappedPurge = CreatePurgeService(capOptions);
cappedPurge.RunPurge();
var minIdAfter = MinEventId();
// The surviving rows must be the newest ones — minimum id has advanced.
Assert.True(minIdAfter > minIdBefore,
"Oldest events (lowest ids) must be purged first.");
// The newest event (highest id) must still be present.
var newestPresent = _eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = "SELECT COUNT(*) FROM site_events WHERE id = 3000";
return (long)cmd.ExecuteScalar()!;
});
Assert.Equal(1L, newestPresent);
}
[Fact]
public async Task PurgeByStorageCap_ConcurrentWritesDoNotCorruptConnection()
{
// Regression test for SiteEventLogging-003: purge running on a background
// thread while events are recorded on other threads must not throw
// "DataReader already open" / "connection busy" from a shared connection.
InsertBulkEvents(2000);
var purge = CreatePurgeService();
var totalSize = purge.GetDatabaseSizeBytes();
var capOptions = new SiteEventLogOptions
{
DatabasePath = _dbPath,
RetentionDays = 30,
MaxStorageMb = (int)Math.Max(1, (totalSize / 2) / (1024 * 1024))
};
var exceptions = new System.Collections.Concurrent.ConcurrentBag<Exception>();
var stop = false;
var purgeTask = Task.Run(() =>
{
try
{
var p = CreatePurgeService(capOptions);
for (int i = 0; i < 20; i++) p.RunPurge();
}
catch (Exception ex) { exceptions.Add(ex); }
});
var writeTasks = Enumerable.Range(0, 4).Select(_ => Task.Run(async () =>
{
try
{
while (!stop)
{
await _eventLogger.LogEventAsync("script", "Info", null, "Concurrent", "Concurrent write");
}
}
catch (Exception ex) { exceptions.Add(ex); }
})).ToArray();
await purgeTask;
stop = true;
await Task.WhenAll(writeTasks);
Assert.Empty(exceptions);
}
}
@@ -256,17 +256,20 @@ public class EventLogQueryServiceTests : IDisposable
private void InsertEventAt(DateTimeOffset timestamp, string eventType, string severity, string? instanceId, string source, string message)
{
using var cmd = _eventLogger.Connection.CreateCommand();
cmd.CommandText = """
INSERT INTO site_events (timestamp, event_type, severity, instance_id, source, message)
VALUES ($ts, $et, $sev, $iid, $src, $msg)
""";
cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o"));
cmd.Parameters.AddWithValue("$et", eventType);
cmd.Parameters.AddWithValue("$sev", severity);
cmd.Parameters.AddWithValue("$iid", (object?)instanceId ?? DBNull.Value);
cmd.Parameters.AddWithValue("$src", source);
cmd.Parameters.AddWithValue("$msg", message);
cmd.ExecuteNonQuery();
_eventLogger.WithConnection(connection =>
{
using var cmd = connection.CreateCommand();
cmd.CommandText = """
INSERT INTO site_events (timestamp, event_type, severity, instance_id, source, message)
VALUES ($ts, $et, $sev, $iid, $src, $msg)
""";
cmd.Parameters.AddWithValue("$ts", timestamp.ToString("o"));
cmd.Parameters.AddWithValue("$et", eventType);
cmd.Parameters.AddWithValue("$sev", severity);
cmd.Parameters.AddWithValue("$iid", (object?)instanceId ?? DBNull.Value);
cmd.Parameters.AddWithValue("$src", source);
cmd.Parameters.AddWithValue("$msg", message);
cmd.ExecuteNonQuery();
});
}
}