Compare commits
6 Commits
d30ded7e72
...
1ae11d1135
| Author | SHA1 | Date | |
|---|---|---|---|
| 1ae11d1135 | |||
| 0529cf2d40 | |||
| 0d9363766d | |||
| 393172f169 | |||
| b249ca3bf7 | |||
| 6f4efdfa2e |
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
@@ -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 |
|
||||
|
||||
@@ -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=`
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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()))
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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 => LDAPS, false => 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" />
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user