diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index 5c8eb09c..c4783a2a 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 8 | +| Open findings | 7 | ## Summary @@ -732,9 +732,11 @@ describe the corrupt-typed-row branch. Regression tests added in |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Types/Transport/EncryptionMetadata.cs:3-8` | +**Resolution (2026-05-28):** Converted `EncryptionMetadata` to a non-positional `record` with an explicit constructor that enforces invariants at the type boundary: `Algorithm` must equal `"AES-256-GCM"`, `Kdf` must equal `"PBKDF2-SHA256"`, `Iterations` must lie in `[MinPbkdf2Iterations=100_000, MaxPbkdf2Iterations=10_000_000]`, and `SaltB64`/`IvB64` must be non-null (empty permitted for the BundleSerializer.Pack seed pattern). Invalid values throw `ArgumentException` (or `ArgumentNullException`) naming the offending field. Added `EncryptionMetadataTests` covering valid construction, unknown algorithm/KDF (including case sensitivity), out-of-range iteration counts (including both boundaries), and null salt/IV. Updated `BundleSecretEncryptorTests` / `BundleSerializerTests` to use `MinPbkdf2Iterations` instead of the prior 10_000 placeholder. + **Description** `EncryptionMetadata` is a positional record that carries the bundle's encryption parameters diff --git a/code-reviews/ExternalSystemGateway/findings.md b/code-reviews/ExternalSystemGateway/findings.md index 1d445288..38ba22ec 100644 --- a/code-reviews/ExternalSystemGateway/findings.md +++ b/code-reviews/ExternalSystemGateway/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 6 | +| Open findings | 5 | ## Summary @@ -1224,9 +1224,11 @@ ever leaked in the warning text would close the test gap as well. |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:233` | +**Resolution (2026-05-28):** Added a `ValidateHttpMethod` helper called at the top of `InvokeHttpAsync` that rejects any verb outside the documented `GET/POST/PUT/PATCH/DELETE` allowlist (matching ESG-023's design-doc reconciliation) with a clear `ArgumentException` naming the offending verb. Allowlist is a `HashSet` with `OrdinalIgnoreCase` so the operator-authored entity column is case-insensitive. Regression tests `Call_UnsupportedHttpMethod_ThrowsArgumentException` (Theory: FOO/DLETE/GIT/OPTIONS/HEAD) and `Call_DocumentedHttpMethod_IsAccepted` (Theory: GET/get/Post/PATCH/delete) cover the rejection and the case-insensitive accept paths. + **Description** `InvokeHttpAsync` constructs the request method directly from the string column: diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 6d7d0003..17d8ca6c 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -985,9 +985,18 @@ bodyless request through the middleware. |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/EndpointExtensions.cs:70` | +**Resolution (2026-05-28):** swapped the case-sensitive `Contains("json")` +substring match for `Contains("json", StringComparison.OrdinalIgnoreCase)` so +`application/JSON` / `Application/Json` / `APPLICATION/JSON` all enter the +JSON-deserialization path. Regression test +`EndpointContentTypeTests.ContentTypeCheck_IsCaseInsensitive_ParsesBodyForAnyCasing` +(theory, 4 casings) drives a TestServer-hosted `MapInboundAPI` end-to-end with +a required Integer parameter — proving body parsing actually ran by asserting +the parameter reaches the handler. + **Description** `HandleInboundApiRequest` parses the JSON body only when @@ -1164,9 +1173,20 @@ resolved key name after successful auth, but is absent on auth failures). |--|--| | Severity | Low | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:30`, `:77`, `:223`, `:233` | +**Resolution (2026-05-28):** capped the cache at `KnownBadMethodsCap = 1000` +entries via a new `TryRecordBadMethod` helper that short-circuits when the cap +is reached — both `CompileAndRegister` and the `ExecuteAsync` lazy-compile path +now route through it. Once full, new bad-method records are dropped; the cache +is just a fast-fail optimisation and the per-request DB lookup remains the +correctness path. Regression tests +`KnownBadMethodsCache_SizeNeverExceedsCap_UnderUniqueNameFlood` and +`KnownBadMethodsCache_LazyCompilePath_AlsoCappedUnderUniqueNameFlood` flood +`cap + 500` / `cap + 250` unique broken methods and assert the cache size never +exceeds the cap. Internal `KnownBadMethodCount` exposed for testability only. + **Description** The InboundAPI-009 fix introduced `_knownBadMethods`, a `ConcurrentDictionary` diff --git a/code-reviews/README.md b/code-reviews/README.md index c17efc89..454f47e1 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,9 +41,9 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 37 | -| Low | 67 | -| **Total** | **104** | +| Medium | 32 | +| Low | 61 | +| **Total** | **93** | ## Module Status @@ -53,25 +53,25 @@ module file and counted in **Total**. | [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 23 | | [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/5 | 5 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 14 | -| [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/5 | 6 | 23 | +| [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/5 | 5 | 23 | | [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/4 | 5 | 22 | | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/2 | 5 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/4 | 5 | 24 | -| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 23 | +| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/3 | 4 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/5 | 6 | 22 | -| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 25 | +| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 25 | | [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 23 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 10 | | [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 25 | | [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 6 | -| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 23 | -| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 26 | +| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 23 | +| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 26 | | [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/3 | 6 | 24 | -| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/1 | 5 | 22 | -| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 12 | +| [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/0 | 3 | 22 | +| [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/3 | 4 | 12 | ## Pending Findings @@ -88,7 +88,7 @@ _None open._ _None open._ -### Medium (37) +### Medium (32) | ID | Module | Title | |----|--------|-------| @@ -97,7 +97,6 @@ _None open._ | AuditLog-005 | [AuditLog](AuditLog/findings.md) | `GetBacklogStatsAsync` holds the SQLite hot-path write lock for the full COUNT+MIN scan | | CLI-017 | [CLI](CLI/findings.md) | `BundleCommands.RunBundleCommandAsync` duplicates `ExecuteCommandAsync` and breaks the auth exit-code contract | | CLI-019 | [CLI](CLI/findings.md) | `bundle export` decodes the entire base64 bundle into memory before writing | -| Commons-015 | [Commons](Commons/findings.md) | `EncryptionMetadata` accepts any algorithm string and any iteration count | | Communication-017 | [Communication](Communication/findings.md) | `_inProgressDeployments` grows unboundedly — successful deployments are never cleaned up | | ConfigurationDatabase-016 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `InboundApiRepository.GetApiKeyByValueAsync` hashes the candidate with the unpeppered `ApiKeyHasher.Default` | | ConfigurationDatabase-017 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | Stub-attach delete on `DeploymentRecord` bypasses optimistic concurrency | @@ -116,8 +115,6 @@ _None open._ | NotificationService-024 | [NotificationService](NotificationService/findings.md) | No test affirms the central-only invariant; the orphaned-path tests give a false coverage signal | | SiteCallAudit-001 | [SiteCallAudit](SiteCallAudit/findings.md) | SupervisorStrategy override is dead code; XML claims Resume that is not enforced | | SiteCallAudit-003 | [SiteCallAudit](SiteCallAudit/findings.md) | `OnUpsertAsync` does not refresh `IngestedAtUtc`; direct-write callers must remember to stamp it | -| SiteEventLogging-015 | [SiteEventLogging](SiteEventLogging/findings.md) | Background write queue is unbounded; can grow without limit under sustained writer slowness | -| SiteEventLogging-017 | [SiteEventLogging](SiteEventLogging/findings.md) | Central client's `PageSize` is unbounded; defeats the "configurable page size" design rationale | | SiteRuntime-021 | [SiteRuntime](SiteRuntime/findings.md) | `HandleDeployArtifacts` updates `DataConnections` in SQLite but never sends `CreateConnectionCommand` to the DCL | | SiteRuntime-022 | [SiteRuntime](SiteRuntime/findings.md) | `AuditingDbCommand.DbConnection.set` uses reflection to read `AuditingDbConnection._inner` | | StoreAndForward-019 | [StoreAndForward](StoreAndForward/findings.md) | Notifications park after `DefaultMaxRetries` exhaustion, contradicting "retried until central acks" | @@ -126,11 +123,9 @@ _None open._ | TemplateEngine-018 | [TemplateEngine](TemplateEngine/findings.md) | `DiffService` reports no entries for added/removed/changed connections | | TemplateEngine-019 | [TemplateEngine](TemplateEngine/findings.md) | `TemplateResolver.BuildInheritanceChain` still uses the `0`-as-no-parent sentinel that was removed from `CycleDetector` | | TemplateEngine-020 | [TemplateEngine](TemplateEngine/findings.md) | `Create*` audit entries are written with `EntityId = "0"` before `SaveChangesAsync` populates the real key | -| TemplateEngine-021 | [TemplateEngine](TemplateEngine/findings.md) | `MoveTemplateAsync` skips folder cycle and sibling-name-collision validation | -| Transport-004 | [Transport](Transport/findings.md) | `MaxUnlockAttemptsPerIpPerHour` option is declared but never enforced | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | -### Low (67) +### Low (61) | ID | Module | Title | |----|--------|-------| @@ -167,7 +162,6 @@ _None open._ | DeploymentManager-023 | [DeploymentManager](DeploymentManager/findings.md) | `BuildDeployArtifactsCommandAsync` re-queries system-wide artifacts once per site | | DeploymentManager-024 | [DeploymentManager](DeploymentManager/findings.md) | Test probe actors hold mutable static state across tests | | ExternalSystemGateway-021 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `ApplyAuth` silently sends an unauthenticated request on unknown `AuthType`, empty `AuthConfiguration`, or malformed Basic config | -| ExternalSystemGateway-022 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `new HttpMethod(method.HttpMethod)` accepts any string at runtime; an invalid HTTP verb fails only at call time | | HealthMonitoring-018 | [HealthMonitoring](HealthMonitoring/findings.md) | Same counter-reset-before-publish hazard in `CentralHealthReportLoop` | | HealthMonitoring-021 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralSiteId = "central"` reserved constant silently collides with a real site named "central" | | HealthMonitoring-022 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralHealthReportLoopTests` uses real-time `PeriodicTimer` + `Task.Delay`; flake-prone on slow CI | @@ -177,9 +171,7 @@ _None open._ | Host-021 | [Host](Host/findings.md) | Microsoft `Logging:LogLevel` section in `appsettings.json` is dead config under Serilog | | Host-022 | [Host](Host/findings.md) | `ParseLevel` silently coerces unrecognised `MinimumLevel` to `Information` | | InboundAPI-019 | [InboundAPI](InboundAPI/findings.md) | `EnableBuffering()` called unconditionally on every request, including bodyless requests | -| InboundAPI-020 | [InboundAPI](InboundAPI/findings.md) | `ContentType.Contains("json")` is case-sensitive; `application/JSON` with no Content-Length skips body parsing | | InboundAPI-023 | [InboundAPI](InboundAPI/findings.md) | `EndpointExtensions.HandleInboundApiRequest` composition wiring has no test coverage | -| InboundAPI-024 | [InboundAPI](InboundAPI/findings.md) | `_knownBadMethods` is unbounded — an attacker can grow the cache by spamming distinct method names against the audit middleware path | | ManagementService-023 | [ManagementService](ManagementService/findings.md) | HandleQueryDeployments unfiltered branch is N+1 on instance lookup | | NotificationOutbox-006 | [NotificationOutbox](NotificationOutbox/findings.md) | `ResolveAdapters` rebuilds the `NotificationType → adapter` dictionary on every dispatch sweep | | NotificationOutbox-008 | [NotificationOutbox](NotificationOutbox/findings.md) | `FallbackMaxRetries` / `FallbackRetryDelay` path is unreachable in production AND untested | @@ -190,14 +182,11 @@ _None open._ | SiteCallAudit-002 | [SiteCallAudit](SiteCallAudit/findings.md) | Singleton failover does not wait for in-flight async upserts | | SiteCallAudit-006 | [SiteCallAudit](SiteCallAudit/findings.md) | Stuck-only paging test does not exercise the multi-page boundary with an interleaved non-stuck row at the cursor | | SiteEventLogging-018 | [SiteEventLogging](SiteEventLogging/findings.md) | `FailedWriteCount` is exposed but never consumed by Health Monitoring | -| SiteEventLogging-020 | [SiteEventLogging](SiteEventLogging/findings.md) | `severity` and `eventType` are unvalidated free-form strings; doc enumerates a set that is not enforced | | SiteEventLogging-022 | [SiteEventLogging](SiteEventLogging/findings.md) | `Cache=Shared` is redundant for a single-connection logger | | SiteEventLogging-023 | [SiteEventLogging](SiteEventLogging/findings.md) | Concurrent-stress test uses a non-volatile `stop` flag | -| SiteRuntime-025 | [SiteRuntime](SiteRuntime/findings.md) | `HandleSetStaticAttribute` persists unknown attribute names as static overrides | | StoreAndForward-022 | [StoreAndForward](StoreAndForward/findings.md) | `NotifyCachedCallObserverAsync` silently drops the entire audit lifecycle when the message id is not a parseable `TrackedOperationId` | | StoreAndForward-023 | [StoreAndForward](StoreAndForward/findings.md) | `siteId` silently defaults to empty when no `IStoreAndForwardSiteContext` is registered, degrading audit telemetry correlation | | StoreAndForward-024 | [StoreAndForward](StoreAndForward/findings.md) | `StopAsync` does not wait for an in-flight retry sweep, so disposed dependencies can be touched after shutdown | -| TemplateEngine-022 | [TemplateEngine](TemplateEngine/findings.md) | `LockEnforcer.ValidateLockChange` enforces "once-locked-stays-locked" for `IsLocked` but not for `LockedInDerived` | | Transport-008 | [Transport](Transport/findings.md) | `PreviewAsync` issues an N+1 `GetTemplateWithChildrenAsync` per matching template name | | Transport-009 | [Transport](Transport/findings.md) | `IAuditCorrelationContext.BundleImportId` is mutated on the same scoped instance the AuditService reads | | Transport-012 | [Transport](Transport/findings.md) | "Bundle Import" filter promised in design doc not surfaced in Configuration Audit Log Viewer UI | diff --git a/code-reviews/SiteEventLogging/findings.md b/code-reviews/SiteEventLogging/findings.md index c3cf5ba2..af2d8cc4 100644 --- a/code-reviews/SiteEventLogging/findings.md +++ b/code-reviews/SiteEventLogging/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 8 | +| Open findings | 5 | ## Summary @@ -753,9 +753,18 @@ background scheduler). |--|--| | Severity | Medium | | Category | Performance & resource management | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:58-63` | +**Resolution (2026-05-28):** Switched `SiteEventLogger._writeQueue` to +`Channel.CreateBounded` with `FullMode = BoundedChannelFullMode.DropOldest`. +Default capacity 10,000 via new `SiteEventLogOptions.WriteQueueCapacity`. The +`itemDropped` callback faults the dropped event's Task with +`InvalidOperationException` and increments `FailedWriteCount` so both awaiting +callers and Health Monitoring observe drops. `DropOldest` preserves the +SiteEventLogging-005 "callers never block" guarantee. Test: +`LogEventAsync_BoundedQueueDropsOldest_AndFaultsDroppedTask`. + **Description** `SiteEventLogger` creates its background-writer feeder as @@ -844,9 +853,16 @@ lexicographic-comparison hazard structurally. |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/EventLogQueryService.cs:55`, `src/ScadaLink.Commons/Messages/RemoteQuery/EventLogQueryRequest.cs:18` | +**Resolution (2026-05-28):** `EventLogQueryService.ExecuteQuery` now clamps +`pageSize` to new `SiteEventLogOptions.MaxQueryPageSize` (default 500, matching +existing `QueryPageSize` default) before issuing the SQL. Silent clamp rather +than reject so misconfigured clients still get a usable response. Test: +`Query_PageSize_IsClampedToMaxQueryPageSize` (asserts a request of 100,000 is +clamped down with `HasMore = true`). + **Description** `EventLogQueryService.ExecuteQuery` resolves the effective page size as @@ -970,9 +986,16 @@ SiteEventLogging.Tests). |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/SiteEventLogger.cs:144-156`, `src/ScadaLink.SiteEventLogging/ISiteEventLogger.cs:14-15` | +**Resolution (2026-05-28):** `LogEventAsync` now validates `severity` against +the closed set `{Info, Warning, Error}` (case-sensitive, matches SQLite +BINARY collation used by the query filter) and throws `ArgumentException` +naming the offending value. `eventType` left intentionally free-form (design +enumerates an open category set). Tests: `LogEventAsync_ThrowsOnUnknownSeverity` +(5 cases) and `LogEventAsync_AcceptsAllDocumentedSeverities` (3 cases). + **Description** `LogEventAsync` validates `eventType` and `severity` only for non-empty/non-whitespace. diff --git a/code-reviews/SiteRuntime/findings.md b/code-reviews/SiteRuntime/findings.md index 3ece6b89..0d60cbf5 100644 --- a/code-reviews/SiteRuntime/findings.md +++ b/code-reviews/SiteRuntime/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 6 | +| Open findings | 5 | ## Summary @@ -1265,9 +1265,21 @@ into a sync path that calls `_connection.Dispose()` directly. |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs:223`, `src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs:246` | +**Resolution (2026-05-28):** `HandleSetStaticAttribute` now rejects writes +whose `command.AttributeName` does not resolve against +`_configuration.Attributes`. The caller receives +`SetStaticAttributeResponse(Success: false, ErrorMessage: "Unknown attribute +''")`; no override is persisted, `_attributes` is not mutated, and no +synthetic `AttributeValueChanged` is published — eliminating the +in-memory-pollution, restart-resurrection, and debug-stream spam vectors. +Regression test +`InstanceActorSetAttributeTests.SetAttribute_UnknownAttribute_ReturnsFailureAndDoesNotPersistOverride` +exercises an inbound `SetStaticAttributeCommand` with an unknown name and +asserts the failure response, no DCL traffic, and an empty override row. + **Description** `HandleSetStaticAttribute` resolves the target attribute against diff --git a/code-reviews/TemplateEngine/findings.md b/code-reviews/TemplateEngine/findings.md index a0fad6c1..0e12fd5b 100644 --- a/code-reviews/TemplateEngine/findings.md +++ b/code-reviews/TemplateEngine/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 6 | +| Open findings | 4 | ## Summary @@ -1067,7 +1067,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/TemplateService.cs:173` | **Description** @@ -1102,9 +1102,21 @@ templates with the same `FolderId == newFolderId` and the same `Name` (case-insensitive), mirroring `TemplateFolderService.MoveFolderAsync` lines 130–142. Add a regression test `MoveTemplate_NameCollisionAtDestination_Fails`. -**Resolution** +**Resolution (2026-05-28):** -_Unresolved._ +Resolved (commit `pending`): `MoveTemplateAsync` now loads `GetAllTemplatesAsync` +on any FolderId-changing move and rejects the move if another template at the +destination shares the moved template's name (case-insensitive), mirroring +`TemplateFolderService.MoveFolderAsync`'s sibling-name uniqueness check; the +FolderId is not written when the check fails. Cycle detection is deliberately +not added — a template move changes only `FolderId` and never touches +`ParentTemplateId`, and templates have no folder-children navigation, so no +inheritance- or folder-graph cycle is reachable through this path (the +finding's own description states this; only the sibling-name check applies). +Regression tests: `MoveTemplate_NameCollisionAtDestination_Fails` (case- +insensitive collision rejected, FolderId untouched, `UpdateTemplateAsync` never +called) and `MoveTemplate_NoCollisionAtDestination_Succeeds` (same-named +template in a *different* folder is not a collision). ### TemplateEngine-022 — `LockEnforcer.ValidateLockChange` enforces "once-locked-stays-locked" for `IsLocked` but not for `LockedInDerived` @@ -1112,7 +1124,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/LockEnforcer.cs:109`, `src/ScadaLink.TemplateEngine/TemplateService.cs:323`, `src/ScadaLink.TemplateEngine/TemplateService.cs:476`, `src/ScadaLink.TemplateEngine/TemplateService.cs:623` | **Description** @@ -1156,7 +1168,20 @@ like `IsLocked`, extend `ValidateLockChange` (or add a sibling intended to be mutable, update the `LockEnforcer` summary to scope the rule to `IsLocked` only. Either way, add a test pinning the chosen behaviour. -**Resolution** +**Resolution (2026-05-28):** -_Unresolved._ +Resolved (commit `pending`): chose option (b) — `LockedInDerived` is now a +one-way ratchet on base templates, matching the design intent that an existing +block on derived overrides cannot be retroactively re-allowed. Added a sibling +`LockEnforcer.ValidateLockedInDerivedChange(originalLockedInDerived, +proposedLockedInDerived, memberName)` and wired it into `UpdateAttributeAsync`, +`UpdateAlarmAsync`, and `UpdateScriptAsync` (only when the owning template is +*not* derived — derived rows never carry an authoritative `LockedInDerived`, +they inherit the base's value). The `LockEnforcer` class XML summary now +explicitly extends the once-locked-stays-locked rule to both `IsLocked` and +`LockedInDerived` so the documentation matches the enforced behaviour. +Regression tests: `LockEnforcerTests.ValidateLockedInDerivedChange_*` (true→ +false rejected, false→true / true→true / false→false accepted) and +`TemplateServiceTests.UpdateAttribute_LockedInDerivedDowngrade_OnBase_Rejected` +(end-to-end on the attribute update path). diff --git a/code-reviews/Transport/findings.md b/code-reviews/Transport/findings.md index d1d26eee..9f1839e3 100644 --- a/code-reviews/Transport/findings.md +++ b/code-reviews/Transport/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 11 | +| Open findings | 10 | ## Summary @@ -173,9 +173,11 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Transport/TransportOptions.cs:12`, `docs/requirements/Component-Transport.md` §11 | +**Resolution (2026-05-28):** Added a new `BundleUnlockRateLimiter` class (in-memory, per-key sliding-window counter via `ConcurrentDictionary>` with per-bucket locking), registered as a singleton in `AddTransport`, and wired into `BundleImporter.LoadAsync` before the decrypt attempt — exceeding the per-window cap throws the new `BundleUnlockRateLimitedException` (429-equivalent). The importer keys the limiter on the bundle's `ContentHash` (it has no `IHttpContext` dependency by design); an IP-aware caller can use the limiter's public `TryRegisterAttempt(clientIp, max)` directly for true per-IP keying. `BundleUnlockRateLimiterTests` covers: N attempts allowed and N+1 rejected; full-window expiry releases the entire budget; partial expiry releases only the aged-out slots (sliding window); per-key isolation; argument validation. + **Description** `TransportOptions.MaxUnlockAttemptsPerIpPerHour` defaults to 10 and is diff --git a/src/ScadaLink.Commons/Types/Transport/EncryptionMetadata.cs b/src/ScadaLink.Commons/Types/Transport/EncryptionMetadata.cs index c4562ebd..e5bc256e 100644 --- a/src/ScadaLink.Commons/Types/Transport/EncryptionMetadata.cs +++ b/src/ScadaLink.Commons/Types/Transport/EncryptionMetadata.cs @@ -1,8 +1,93 @@ namespace ScadaLink.Commons.Types.Transport; -public sealed record EncryptionMetadata( - string Algorithm, // "AES-256-GCM" - string Kdf, // "PBKDF2-SHA256" - int Iterations, - string SaltB64, - string IvB64); +/// +/// AES-GCM encryption envelope metadata for a bundle's content payload. Carried on +/// the bundle manifest (plaintext) so the importer can derive the per-bundle key and +/// initialise the cipher without prior knowledge of the passphrase. +/// +/// Commons-015: invariants are enforced in the constructor so a malformed envelope +/// (unknown algorithm, unsupported KDF, weak iteration count, null salt/IV) is +/// rejected at the type boundary rather than failing inside +/// with a misleading exception. +/// +/// +public sealed record EncryptionMetadata +{ + /// The only AES symmetric algorithm the bundle format supports. + public const string SupportedAlgorithm = "AES-256-GCM"; + + /// The only key-derivation function the bundle format supports. + public const string SupportedKdf = "PBKDF2-SHA256"; + + /// + /// PBKDF2 iteration-count floor — OWASP's documented minimum. The Transport design + /// doc specifies 600_000 as the production value; this constant is the hard + /// reject threshold below which the envelope is treated as malformed. + /// + public const int MinPbkdf2Iterations = 100_000; + + /// + /// PBKDF2 iteration-count ceiling — guards against a hostile bundle declaring an + /// absurd iteration count that would burn CPU on every unlock attempt. + /// + public const int MaxPbkdf2Iterations = 10_000_000; + + /// + /// Initializes a new . Each argument is validated + /// against the documented contract; invalid values throw + /// naming the offending field. + /// + /// Symmetric algorithm name; must equal . + /// Key-derivation function name; must equal . + /// PBKDF2 iteration count; must lie in [, ]. + /// Base64-encoded PBKDF2 salt; must be non-null (may be empty for the seed pattern used by BundleSerializer.Pack). + /// Base64-encoded AES-GCM IV; must be non-null (may be empty for the seed pattern used by BundleSerializer.Pack). + /// Thrown when any field violates the documented contract. + public EncryptionMetadata(string Algorithm, string Kdf, int Iterations, string SaltB64, string IvB64) + { + if (!string.Equals(Algorithm, SupportedAlgorithm, StringComparison.Ordinal)) + { + throw new ArgumentException( + $"{nameof(Algorithm)} must be '{SupportedAlgorithm}'; got '{Algorithm}'.", + nameof(Algorithm)); + } + + if (!string.Equals(Kdf, SupportedKdf, StringComparison.Ordinal)) + { + throw new ArgumentException( + $"{nameof(Kdf)} must be '{SupportedKdf}'; got '{Kdf}'.", + nameof(Kdf)); + } + + if (Iterations < MinPbkdf2Iterations || Iterations > MaxPbkdf2Iterations) + { + throw new ArgumentException( + $"{nameof(Iterations)} must be between {MinPbkdf2Iterations} and {MaxPbkdf2Iterations}; got {Iterations}.", + nameof(Iterations)); + } + + ArgumentNullException.ThrowIfNull(SaltB64); + ArgumentNullException.ThrowIfNull(IvB64); + + this.Algorithm = Algorithm; + this.Kdf = Kdf; + this.Iterations = Iterations; + this.SaltB64 = SaltB64; + this.IvB64 = IvB64; + } + + /// Symmetric algorithm name (always ). + public string Algorithm { get; init; } + + /// Key-derivation function name (always ). + public string Kdf { get; init; } + + /// PBKDF2 iteration count. + public int Iterations { get; init; } + + /// Base64-encoded PBKDF2 salt. + public string SaltB64 { get; init; } + + /// Base64-encoded AES-GCM IV. + public string IvB64 { get; init; } +} diff --git a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs index 260c8c62..2c680f86 100644 --- a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs @@ -242,6 +242,18 @@ public class ExternalSystemClient : IExternalSystemClient IReadOnlyDictionary? parameters, CancellationToken cancellationToken) { + // ExternalSystemGateway-022: validate the verb against the documented set + // (GET/POST/PUT/PATCH/DELETE — per ESG-023's design-doc reconciliation) + // BEFORE constructing the request. `new HttpMethod(string)` accepts any + // token-character string (e.g. "FOO", "DLETE"), and the body-vs-query + // branch below only knows POST/PUT/PATCH and GET/DELETE — so an + // unsupported verb would dispatch silently with parameters sent to + // neither body nor query, and the script would only see a remote 4xx. + // Rejecting at the gateway entry surfaces the misconfiguration with a + // clear ArgumentException naming the offending verb. Case-insensitive + // match: the entity column carries free-form strings. + ValidateHttpMethod(method.HttpMethod); + var client = _httpClientFactory.CreateClient($"ExternalSystem_{system.Name}"); var url = BuildUrl(system.EndpointUrl, method.Path, parameters, method.HttpMethod); @@ -357,6 +369,39 @@ public class ExternalSystemClient : IExternalSystemClient /// private const int MaxErrorBodyChars = 2048; + /// + /// ExternalSystemGateway-022: documented HTTP-verb allowlist. Matches the + /// design doc's enumerated set (GET/POST/PUT/PATCH/DELETE per ESG-023) and + /// the body-vs-query branching above; any addition here must update both. + /// + private static readonly HashSet SupportedHttpMethods = new(StringComparer.OrdinalIgnoreCase) + { + "GET", "POST", "PUT", "PATCH", "DELETE", + }; + + /// + /// Rejects HTTP verbs the gateway does not support. Throws + /// for null/empty input or any string outside + /// the documented allowlist. Case-insensitive — the entity column carries + /// operator-authored strings. + /// + private static void ValidateHttpMethod(string httpMethod) + { + if (string.IsNullOrWhiteSpace(httpMethod)) + { + throw new ArgumentException( + "HTTP method must be one of GET/POST/PUT/PATCH/DELETE; got null or empty.", + nameof(httpMethod)); + } + + if (!SupportedHttpMethods.Contains(httpMethod)) + { + throw new ArgumentException( + $"HTTP method '{httpMethod}' is not supported. Allowed verbs: GET, POST, PUT, PATCH, DELETE.", + nameof(httpMethod)); + } + } + private static string Truncate(string value, int maxChars) { if (string.IsNullOrEmpty(value) || value.Length <= maxChars) diff --git a/src/ScadaLink.InboundAPI/EndpointExtensions.cs b/src/ScadaLink.InboundAPI/EndpointExtensions.cs index a00fd8cc..e4f687b6 100644 --- a/src/ScadaLink.InboundAPI/EndpointExtensions.cs +++ b/src/ScadaLink.InboundAPI/EndpointExtensions.cs @@ -67,7 +67,14 @@ public static class EndpointExtensions JsonElement? body = null; try { - if (httpContext.Request.ContentLength > 0 || httpContext.Request.ContentType?.Contains("json") == true) + // InboundAPI-020: the content-type sniff must be case-insensitive — a + // request with `application/JSON` or `Application/Json` is still JSON + // and must enter the body-parsing path. The previous case-sensitive + // `Contains("json")` silently skipped JSON deserialization for any + // capitalised value, leaving `body = null` and surfacing required + // parameters as 400 "missing" even though the caller sent a valid body. + if (httpContext.Request.ContentLength > 0 + || httpContext.Request.ContentType?.Contains("json", StringComparison.OrdinalIgnoreCase) == true) { using var doc = await JsonDocument.ParseAsync( httpContext.Request.Body, cancellationToken: httpContext.RequestAborted); diff --git a/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs b/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs index 3696de7a..0d9af7fd 100644 --- a/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs +++ b/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs @@ -27,8 +27,38 @@ public class InboundScriptExecutor // request for a broken method re-runs the expensive Roslyn compilation — a CPU // amplification vector since the inbound API has no rate limiting. The entry is // cleared whenever the method is (re)compiled via CompileAndRegister. + // + // InboundAPI-024: bound the cache so a spam attack of unique method names cannot + // grow it without bound. Once the cap is reached new bad-method records are + // dropped — the cache is just a fast-fail optimisation; the per-request DB + // lookup remains the correctness path. + private const int KnownBadMethodsCap = 1000; private readonly ConcurrentDictionary _knownBadMethods = new(); + /// + /// InboundAPI-024 diagnostic helper — returns the current size of the + /// known-bad-methods cache so tests can assert the cap is honoured. Internal + /// so the cache itself stays an implementation detail. + /// + internal int KnownBadMethodCount => _knownBadMethods.Count; + + /// + /// InboundAPI-024: records in the known-bad-methods + /// cache only if the cache has not reached . + /// Once full, new records are dropped (paying the cheap recompile next time + /// rather than leaking memory under a unique-name flood). Existing entries are + /// not touched — they remain capped fast-fail records until cleared on a + /// successful (re)compile in . + /// + private void TryRecordBadMethod(string methodName) + { + if (_knownBadMethods.ContainsKey(methodName)) + return; + if (_knownBadMethods.Count >= KnownBadMethodsCap) + return; + _knownBadMethods.TryAdd(methodName, 0); + } + private readonly IServiceProvider _serviceProvider; /// @@ -73,8 +103,10 @@ public class InboundScriptExecutor if (handler == null) { // InboundAPI-009: record the failure so the lazy-compile path does not - // keep recompiling a broken script on every request. - _knownBadMethods[method.Name] = 0; + // keep recompiling a broken script on every request. InboundAPI-024: + // routed through the capped TryRecordBadMethod helper so the cache + // cannot grow without bound under a flood of unique method names. + TryRecordBadMethod(method.Name); return false; } @@ -230,7 +262,9 @@ public class InboundScriptExecutor if (compiled == null) { // Cache the failure so the next request short-circuits above. - _knownBadMethods[method.Name] = 0; + // InboundAPI-024: routed through TryRecordBadMethod so the + // cache is bounded under a flood of unique method names. + TryRecordBadMethod(method.Name); return new InboundScriptResult(false, null, "Script compilation failed for this method"); } handler = _scriptHandlers.GetOrAdd(method.Name, compiled); diff --git a/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs b/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs index 2b39d801..e7bb1b41 100644 --- a/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs +++ b/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs @@ -53,7 +53,13 @@ public class EventLogQueryService : IEventLogQueryService { try { - var pageSize = request.PageSize > 0 ? request.PageSize : _options.QueryPageSize; + // SiteEventLogging-017: clamp caller-supplied PageSize to a hard upper + // bound so a central client sending int.MaxValue can't force the query + // to materialise the entire log into a single list while holding the + // shared write lock. Silent clamp — misconfigured clients still get a + // usable response. + var requestedSize = request.PageSize > 0 ? request.PageSize : _options.QueryPageSize; + var pageSize = Math.Min(requestedSize, _options.MaxQueryPageSize); var whereClauses = new List(); var parameters = new List(); diff --git a/src/ScadaLink.SiteEventLogging/SiteEventLogOptions.cs b/src/ScadaLink.SiteEventLogging/SiteEventLogOptions.cs index ca30a259..6fa3b5c3 100644 --- a/src/ScadaLink.SiteEventLogging/SiteEventLogOptions.cs +++ b/src/ScadaLink.SiteEventLogging/SiteEventLogOptions.cs @@ -10,6 +10,21 @@ public class SiteEventLogOptions public string DatabasePath { get; set; } = "site_events.db"; /// Maximum number of rows returned per paginated query; default 500. public int QueryPageSize { get; set; } = 500; + /// + /// SiteEventLogging-017: hard upper bound on a caller-supplied PageSize. A + /// misbehaving or hostile central client that requests int.MaxValue would + /// otherwise force the query to materialise the entire log into a single list while + /// holding the shared write lock. Silent clamp; default 500 matches + /// . + /// + public int MaxQueryPageSize { get; set; } = 500; /// Interval between purge runs; default 24 hours. public TimeSpan PurgeInterval { get; set; } = TimeSpan.FromHours(24); + /// + /// SiteEventLogging-015: bound on the background write queue. Default 10 000 events. + /// Overflow uses BoundedChannelFullMode.DropOldest — callers never block; the + /// dropped event's Task is faulted and FailedWriteCount is incremented + /// so the drop is observable. + /// + public int WriteQueueCapacity { get; set; } = 10_000; } diff --git a/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs b/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs index f3c271d4..a27baccb 100644 --- a/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs +++ b/src/ScadaLink.SiteEventLogging/SiteEventLogger.cs @@ -17,12 +17,16 @@ namespace ScadaLink.SiteEventLogging; /// , which serialises callers on a shared lock. /// /// -/// Event recording is offloaded to a dedicated background writer thread (fed by an -/// unbounded ). only validates -/// its arguments and enqueues, so callers — typically Akka actor threads on hot -/// paths — never block on disk I/O or on contention for the write lock. The -/// returned completes once the event is durably persisted and -/// faults if the write fails, so failures are observable rather than swallowed. +/// Event recording is offloaded to a dedicated background writer thread (fed by a +/// bounded ; capacity , +/// default 10 000, overflow ). +/// only validates its arguments and enqueues, so callers — +/// typically Akka actor threads on hot paths — never block on disk I/O or on +/// contention for the write lock. The returned completes once the +/// event is durably persisted and faults if the write fails. SiteEventLogging-015: +/// when a queued event is evicted to make room for a newer one, that event's Task +/// is faulted with and +/// is incremented so the drop is observable. /// /// public class SiteEventLogger : ISiteEventLogger, IDisposable @@ -55,11 +59,26 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable InitializeSchema(); - _writeQueue = Channel.CreateUnbounded(new UnboundedChannelOptions - { - SingleReader = true, - SingleWriter = false, - }); + // SiteEventLogging-015: bounded queue with DropOldest preserves the + // "callers never block" guarantee (SiteEventLogging-005) while putting an + // upper bound on memory under sustained writer slowness. Drops are + // observable — itemDropped faults the evicted Task and increments + // FailedWriteCount. + var capacity = Math.Max(1, options.Value.WriteQueueCapacity); + _writeQueue = Channel.CreateBounded( + new BoundedChannelOptions(capacity) + { + SingleReader = true, + SingleWriter = false, + FullMode = BoundedChannelFullMode.DropOldest, + }, + itemDropped: dropped => + { + Interlocked.Increment(ref _failedWriteCount); + dropped.Completion.TrySetException( + new InvalidOperationException( + $"Event was dropped because the write queue exceeded its bounded capacity ({capacity}).")); + }); _writerLoop = Task.Run(ProcessWriteQueueAsync); } @@ -141,6 +160,16 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable cmd.ExecuteNonQuery(); } + /// + /// SiteEventLogging-020: closed set of allowed severities. Case-sensitive to + /// match the SQLite default BINARY collation used by the query filter — + /// a row stored as "error" would be invisible to a query filtering on + /// "Error", so the contract on the way in must match the contract on + /// the way out. + /// + private static readonly HashSet AllowedSeverities = + new(StringComparer.Ordinal) { "Info", "Warning", "Error" }; + /// public Task LogEventAsync( string eventType, @@ -155,6 +184,15 @@ public class SiteEventLogger : ISiteEventLogger, IDisposable ArgumentException.ThrowIfNullOrWhiteSpace(source); ArgumentException.ThrowIfNullOrWhiteSpace(message); + // SiteEventLogging-020: reject unknown severities so the query-time filter + // (case-sensitive BINARY collation) and the documented enum stay in sync. + if (!AllowedSeverities.Contains(severity)) + { + throw new ArgumentException( + $"Severity '{severity}' is not one of the allowed values: Info, Warning, Error.", + nameof(severity)); + } + var pending = new PendingEvent( DateTimeOffset.UtcNow.ToString("o"), eventType, diff --git a/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs b/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs index bcfc1887..0f8e9557 100644 --- a/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs +++ b/src/ScadaLink.SiteRuntime/Actors/InstanceActor.cs @@ -226,13 +226,37 @@ public class InstanceActor : ReceiveActor var resolved = _configuration?.Attributes .FirstOrDefault(a => a.CanonicalName == command.AttributeName); - var isDataSourced = resolved != null - && !string.IsNullOrEmpty(resolved.DataSourceReference) + // SiteRuntime-025: reject writes targeting an attribute that does not exist + // on the deployed instance. Without this check, an inbound API + // SetAttribute("notARealAttr", ...) would pollute the in-memory + // _attributes dictionary, publish a synthetic AttributeValueChanged to + // debug-view subscribers, and persist a durable static-override row that + // resurrects on every restart. The override row is also outside the + // ClearStaticOverridesAsync window for unknown names. Refuse the write + // and let the caller see the failure, mirroring the script trust model's + // "scripts can only read/write attributes on their own instance" framing. + if (resolved == null) + { + _logger.LogWarning( + "SetAttribute rejected — attribute '{Attribute}' is not defined on instance '{Instance}'", + command.AttributeName, _instanceUniqueName); + Sender.Tell(new SetStaticAttributeResponse( + command.CorrelationId, + _instanceUniqueName, + command.AttributeName, + false, + $"Unknown attribute '{command.AttributeName}'", + DateTimeOffset.UtcNow)); + return; + } + + var isDataSourced = + !string.IsNullOrEmpty(resolved.DataSourceReference) && !string.IsNullOrEmpty(resolved.BoundDataConnectionName); if (isDataSourced) { - HandleSetDataAttribute(command, resolved!); + HandleSetDataAttribute(command, resolved); return; } diff --git a/src/ScadaLink.TemplateEngine/LockEnforcer.cs b/src/ScadaLink.TemplateEngine/LockEnforcer.cs index dbd62ca0..07a14805 100644 --- a/src/ScadaLink.TemplateEngine/LockEnforcer.cs +++ b/src/ScadaLink.TemplateEngine/LockEnforcer.cs @@ -8,7 +8,11 @@ namespace ScadaLink.TemplateEngine; /// Locking rules: /// - Locked members cannot be overridden downstream (child templates or compositions). /// - Any level can lock an unlocked member (intermediate locking). -/// - Once locked, a member stays locked — it cannot be unlocked downstream. +/// - Once locked, a member stays locked — neither +/// nor may be cleared after it has +/// been set. The same one-way ratchet applies to alarms and scripts. This pins +/// the design intent so a base template cannot retroactively re-allow derived +/// overrides that were previously blocked (TemplateEngine-022). /// /// Override granularity: /// - Attributes: Value and Description overridable; DataType and DataSourceReference fixed. @@ -115,4 +119,27 @@ public static class LockEnforcer return null; } + + /// + /// Validates that a (or alarm/script) + /// flag change is legal. LockedInDerived follows the same one-way ratchet + /// as IsLocked — once set on a base template, it cannot be cleared, + /// otherwise derived templates that were previously blocked from overriding the + /// field would become retroactively allowed (TemplateEngine-022). + /// + /// Current LockedInDerived state. + /// Proposed LockedInDerived state. + /// Name of the member being changed, for error messages. + public static string? ValidateLockedInDerivedChange( + bool originalLockedInDerived, + bool proposedLockedInDerived, + string memberName) + { + if (originalLockedInDerived && !proposedLockedInDerived) + { + return $"Member '{memberName}' is locked-in-derived and that lock cannot be cleared."; + } + + return null; + } } diff --git a/src/ScadaLink.TemplateEngine/TemplateService.cs b/src/ScadaLink.TemplateEngine/TemplateService.cs index d7e35cb2..41ce4534 100644 --- a/src/ScadaLink.TemplateEngine/TemplateService.cs +++ b/src/ScadaLink.TemplateEngine/TemplateService.cs @@ -187,6 +187,31 @@ public class TemplateService return Result