dohertj2/ScadaBridge
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(validation): close Theme 3 — 11 input-validation / unbounded-input findings

Browse Source 
Each finding is a focused validation guard or upper bound at a trust boundary.
Highlights:
- Commons-015: EncryptionMetadata ctor now validates Algorithm (AES-256-GCM
  only), Kdf (PBKDF2-SHA256 only), Iterations ([100k, 10M]), non-null Salt/IV.
- Transport-004: new BundleUnlockRateLimiter (sliding-window, per-key,
  singleton) wired into BundleImporter.LoadAsync; over-budget callers see
  BundleUnlockRateLimitedException. Per-bundle 3-strike + per-window cap.
- ESG-022: ExternalSystemClient.InvokeHttpAsync allow-lists the documented
  GET/POST/PUT/PATCH/DELETE set (case-insensitive); unknown verbs throw.
- SEL-015: SiteEventLogger queue now bounded (10k cap, DropOldest); dropped
  events fault their Task and increment FailedWriteCount so the drop is
  observable instead of an unbounded memory growth.
- SEL-017: EventLogQueryService clamps caller-supplied PageSize to a new
  MaxQueryPageSize cap (default 500) so int.MaxValue can't OOM the host.
- SEL-020: LogEventAsync rejects severities outside {Info, Warning, Error}
  (matches SQLite BINARY-collation query filter).
- InboundAPI-020: ContentType "json" check now case-insensitive
  (application/JSON no longer slips through as not-json).
- InboundAPI-024: _knownBadMethods capped at 1000 entries (drops new entries
  once full); per-request DB lookup remains the correctness path.
- SR-025: HandleSetStaticAttribute validates the attribute name against the
  deployed config; unknown names now return Success=false instead of
  leaking orphan override rows into the SQLite store.
- TE-021: MoveTemplateAsync runs the sibling-name-collision check at the
  destination, mirroring TemplateFolderService.MoveFolderAsync.
- TE-022: LockEnforcer's once-locked-stays-locked rule now also covers
  LockedInDerived (was previously only IsLocked).

New regression tests across 8 test projects (EncryptionMetadata, rate
limiter, ESG client allow-list, SEL bounded channel / PageSize clamp /
severity validation, InboundAPI ContentType + bad-methods cap, SiteRT
unknown-attribute, TemplateEngine MoveTemplate + LockedInDerived).
Build clean; affected suites all green. README regenerated: 93 open (was 104).

Note: a separate manual re-run was needed for the SiteEventLogging hunk
because its initial subagent's source edits never landed on disk despite
reporting success (file-collision-style failure mode).
This commit is contained in:
Joseph Doherty
2026-05-28 06:58:25 -04:00
parent 344379a40a
commit 819f1b4665
35 changed files with 1457 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Commons/findings.md
+4 -2
View File
@@ -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
code-reviews/ExternalSystemGateway/findings.md
+4 -2
View File
@@ -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<string>` 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:
code-reviews/InboundAPI/findings.md
+22 -2
View File
@@ -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<string, byte>`
code-reviews/README.md
+12 -23
View File
@@ -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 |
code-reviews/SiteEventLogging/findings.md
+27 -4
View File
@@ -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<PendingEvent>` 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.
code-reviews/SiteRuntime/findings.md
+14 -2
View File
@@ -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
'<name>'")`; 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
code-reviews/TemplateEngine/findings.md
+32 -7
View File
@@ -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).
code-reviews/Transport/findings.md
+4 -2
View File
@@ -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<string, Queue<DateTimeOffset>>` 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