docs: update architecture review with independent findings and verification
This commit is contained in:
Joseph Doherty
@@ -1,19 +1,24 @@
|
||||
# ScadaBridge — architectural / code review (whole system)
|
||||
|
||||
**Date:** 2026-09-09 · **Status:** REVIEW REPORT (no code changed by this review)
|
||||
**Scope:** all 26 `src/` projects + rig config (`docker/`, `docker-env2/`) + docs, at `main` @ `d8229916`
|
||||
(Flow Debugger v1.4 merge). The last whole-system review was 2026-08-14
|
||||
(`2026-08-14-arch-review-remediation-plan.md`); 511 commits have landed since — the remediation
|
||||
waves, LocalDb 0.3.x, Lever 1, inbound standby relay, the shared inbound key store, the template /
|
||||
instance pickers, create-instance bindings, and Flow Debugger v1.0–v1.4. Those ranges got the
|
||||
closest read; the standing design commitments were re-verified as well.
|
||||
**Method:** eight parallel read-only reviewers, one per area (Site Runtime + S&F + event log;
|
||||
Communication + Cluster + Host; Management + Deployment + Templates + ScriptAnalysis; Central UI +
|
||||
Security; edge integrations + audit backbone; Commons + ConfigurationDatabase + Transport; DCL + CLI +
|
||||
DelmiaNotifier + docker scripts; cross-cutting), each required to trace every claim through source
|
||||
and tag it CONFIRMED or PLAUSIBLE. The orchestrator independently re-verified every High and the
|
||||
security-boundary Mediums in source (marked *orch-verified*), and ran the full solution build + test
|
||||
suite (§7). Findings are numbered by area prefix; severities follow the 2026-09-06 review's scale.
|
||||
**Date:** 2026-09-09 · **Status:** SECOND-PASS REVIEW REPORT (documentation only; no production code changed)
|
||||
**Reviewed revision:** `main` @ `c078bc8f`. Its only difference from the first review's
|
||||
`d8229916` is the original findings document; production source/configuration is identical.
|
||||
**Scope:** all 26 `src/` projects, 32 test/support projects, both Docker rig configurations,
|
||||
composition roots, contracts, and relevant design documentation.
|
||||
**Method:** three parallel read-only subagents reviewed (1) Site Runtime / DCL / S&F / event
|
||||
logging / CLI / notifier, (2) Management / Deployment / Templates / ScriptAnalysis / UI / Security,
|
||||
and (3) Commons / ConfigurationDatabase / Transport / edge integrations / audit / health.
|
||||
The primary reviewer covered Communication / ClusterInfrastructure / Host / configuration,
|
||||
rechecked all High source paths, ran build/test/format/package checks, and executed isolated
|
||||
probes. Each area revalidated its existing IDs and searched for additional defects. Source-traced
|
||||
mechanisms, runtime probes, latent risks, and unproven operational impact are distinguished below.
|
||||
This is a focused whole-system review, not a claim that every line or production topology was
|
||||
exercised. No rig rebuild, device writes, failover drill, or remote QA/production inspection ran.
|
||||
|
||||
**Second-pass changes:** 11 new findings; UI-3 withdrawn as unsupported; XC-6 merged into CM-5;
|
||||
UI-4/6/7 reclassified as Low hardening gaps. Existing IDs are preserved for traceability. The
|
||||
first report counted the same dead RPCs twice and overstated several triggers and fixes; the
|
||||
corrections here supersede that wording. §8 records the review decisions and validation limits.
|
||||
|
||||
Severity: **High** = data loss / security boundary / outage; **Medium** = wrong behaviour an operator
|
||||
will hit, or a documented property that does not hold; **Low** = quality, consistency, narrow edge.
|
||||
@@ -22,35 +27,43 @@ will hit, or a documented property that does not hold; **Low** = quality, consis
|
||||
|
||||
## 1. Verdict
|
||||
|
||||
The architecture is intact: every design commitment in `CLAUDE.md` that has a code anchor still
|
||||
holds (§6.H — active-node definition, single ActorSystem name, UTC everywhere, no ClusterClient,
|
||||
The component structure remains intact. The twelve structural/runtime commitments catalogued
|
||||
in the first pass retain their code anchors (§6.H — active-node definition, single ActorSystem name, UTC everywhere, no ClusterClient,
|
||||
auto-down emitted, replication exclusions, metadata resolver on all four compile surfaces, no
|
||||
health publisher, no central buffering for unreachable sites, single trust boundary, additive
|
||||
proto evolution). The dependency graph is an acyclic DAG rooted at Commons with zero project
|
||||
references; every mapped gRPC service is behind a fail-closed constant-time interceptor; all
|
||||
contract changes since August are additive; all eight migrations are sound and the model snapshot
|
||||
matches. Test coverage is broad (≈ 7 600 test attributes, 9 231 passing in the run below).
|
||||
matches. The test suite is broad; fresh execution evidence and its limits are in §7.
|
||||
|
||||
Underneath that, the review found **83 findings: 8 High, 33 Medium, 42 Low**. The Highs cluster
|
||||
in three places the recent waves did not touch:
|
||||
The updated register has **92 unique active findings: 13 High, 33 Medium, 46 Low**.
|
||||
This includes low-priority test/documentation gaps and latent risks, not just reproduced failures.
|
||||
The High-priority findings are:
|
||||
|
||||
| # | Sev | Finding | Area |
|
||||
|---|---|---|---|
|
||||
| DC-1 | **High** | **Nothing in production ever unsubscribes a tag or alarm source** — the DCL leaks device subscriptions for the life of the node; dead Instance Actor refs get a `TagValueUpdate` per change | DCL / Site Runtime |
|
||||
| DC-2 | **High** | **Releasing a "redundant" MxGateway handle tears down the LIVE subscription** — an ordinary double-subscribe during resolution silently freezes the tag at its last Good value until the next reconnect | DCL |
|
||||
| DC-3 | **High** | **Device writes are stashed across a reconnect and replayed minutes later**, after the script was told the write failed — the doc says there is no buffering of setpoints | DCL |
|
||||
| SR-1 | **High** | **The synchronous `Attributes[key]` accessor blocks a script-pool worker on a continuation queued to the same pool** — eight concurrent sync writes stall every script on the node until the 60 s watchdog detaches them | Site Runtime |
|
||||
| CM-1 | **High** | **No gRPC message-size limit anywhere**; a 256-row site audit batch of 64 KB error rows exceeds the 4 MB default and, because the drain is deterministic oldest-first, the site's audit forward stalls **permanently** and its audit DB grows unbounded | Communication / AuditLog |
|
||||
| SR-1 | **High** | **The synchronous `Attributes[key]` accessor blocks a script-pool worker on a continuation queued to the same pool** — saturation can stall the script pool until watchdog recovery (timing is configuration-dependent) | Site Runtime |
|
||||
| CM-1 | **High** | **No explicit gRPC message-size configuration**; a 256-row site audit batch of 64 KB error rows exceeds the 4 MB default and, because the drain is deterministic oldest-first, the site's audit forwarding can repeatedly stall on the same oversized window and its audit DB grows unbounded | Communication / AuditLog |
|
||||
| MG-1 | **High** | **Script trust deny-list omits `System.Runtime.CompilerServices` (`Unsafe`, `RuntimeHelpers`) and `System.Runtime.Loader`** — type-punning and constructor bypass pass the deploy gate and the site backstop | ScriptAnalysis |
|
||||
| UI-1 | **High** | **Every configuration edit made in the Central UI bypasses the configuration audit log** (17 pages save straight through the repositories); the same edits via CLI are audited | Central UI |
|
||||
| UI-2 | **High** | **The 30-minute idle timeout never fires while a tab is open** — the session-expiry ping slides the cookie every minute | Security |
|
||||
| UI-1 | **High** | **Many Central UI configuration mutations bypass the configuration audit log** (16 page files save through repositories; service-mediated template/shared-script edits are audited) | Central UI |
|
||||
| UI-2 | **High** | **Passive session pings keep an unattended tab authenticated** — sliding renewal and periodic role-refresh renewal advance the session | Security |
|
||||
| SR-11 | **High** | **A raw transaction exposes the underlying database connection**, allowing commands to bypass the audit wrapper | Site Runtime |
|
||||
| DC-12 | **High** | **OPC UA snapshot-end markers clear unrelated sources' active alarms** because per-feed boundaries are broadcast connection-wide | DCL / Site Runtime |
|
||||
| CM-9 | **High** | **A site-A key can request site-B reconciliation tokens** — authenticated metadata is never bound to payload ownership | Communication / Security |
|
||||
| CM-10 | **High** | **Cross-node retry treats every `Unavailable` as unsent**, repeating non-idempotent writes after an ambiguous response loss | Communication |
|
||||
| UI-13 | **High** | **Open Blazor circuits retain revoked roles and site scopes**, even after the HTTP cookie has refreshed | Central UI / Security |
|
||||
|
||||
The Mediums split into five themes worth reading as groups: **audit completeness** (EI-1/2/3/5,
|
||||
The remaining findings group into five themes: **audit completeness** (EI-1/2/3/5,
|
||||
UI-1), **authorization scope** (MG-2, UI-6/7/8, XC-1, XC-8), **stale/duplicate-apply under
|
||||
failover** (MG-3/4, EI-4/9, DL-1/6, ORCH-1), **subscription/stream lifecycle** (DC-4/5, SR-3/5,
|
||||
CM-2), and **config that lies** (XC-2/3/7). §5 groups everything into a remediation program.
|
||||
CM-2), and **inert configuration** (XC-2/3/7). §5 groups everything into a remediation program.
|
||||
|
||||
Nothing here requires a redesign. Every High has a small-to-medium fix and a clear test to pin it.
|
||||
Most changes fit the current component structure. Reliable script containment and safe retries
|
||||
need explicit runtime/idempotency boundaries; they cannot be fixed by a timeout or status-code
|
||||
check alone. The immediate sequence is revised in §5.
|
||||
|
||||
---
|
||||
|
||||
@@ -61,7 +74,7 @@ Each finding: where (file:line), what the code does, why it matters, fix, effort
|
||||
### 2.1 Site Runtime / StoreAndForward / SiteEventLogging (SR)
|
||||
|
||||
**SR-1 — Synchronous `Attributes[key]` blocks a script-pool worker on a continuation queued to that same pool `[High][CONFIRMED, orch-verified]`**
|
||||
`Scripts/ScopeAccessors.cs:58-59` (`GetAwaiter().GetResult()`); `Scripts/ScriptRuntimeContext.cs:704, 914` (the Asks inside `GetAttribute`/`SetAttribute` have no `ConfigureAwait(false)`); `Scripts/ScriptRunLauncher.cs:110-123` (the run body is started with `Task.Factory.StartNew(..., executionScheduler)`). Inside the run, `TaskScheduler.Current` is the `ScriptExecutionScheduler`, so the awaiter captures it and the continuation that completes the Ask is `QueueTask`'d onto the same bounded pool whose worker is blocked in `GetResult()`. Pool floor is 8; eight concurrent runs each doing `Instance.Attributes["Setpoint"] = 42` (the documented, compile-gated API — the rig's own Delmia stand-in is a per-attribute write script) block all 8 workers; no script or alarm body on the node progresses until `ScriptExecutionTimeoutSeconds` + `StuckScriptGraceMs` (30 s + 30 s) elapse and the watchdog detaches/replaces each worker, and every affected run reports a spurious timeout. Below saturation it silently halves effective concurrency. The class comment at `ScopeAccessors.cs:12-21` asserts the opposite. **Fix:** `.ConfigureAwait(false)` on the awaits in `GetAttribute`, `SetAttribute`, `WaitInternal`, `WriteBatchAndWait` (AsyncLocal `TraceStepScope` flows with `ExecutionContext`, not the scheduler, so attribution is unaffected); correct the comment; add a test running `poolSize` concurrent bodies each doing one sync `Attributes["x"]` on a scheduler of that size. Small.
|
||||
`Scripts/ScopeAccessors.cs:58-59` (`GetAwaiter().GetResult()`); `Scripts/ScriptRuntimeContext.cs:704, 914` (the Asks inside `GetAttribute`/`SetAttribute` have no `ConfigureAwait(false)`); `Scripts/ScriptRunLauncher.cs:110-123` (the run body is started with `Task.Factory.StartNew(..., executionScheduler)`). Inside the run, `TaskScheduler.Current` is the `ScriptExecutionScheduler`, so the awaiter captures it and the continuation that completes the Ask is `QueueTask`'d onto the same bounded pool whose worker is blocked in `GetResult()`. Pool floor is 8; eight concurrent runs each doing `Instance.Attributes["Setpoint"] = 42` (the documented, compile-gated API — the rig's own Delmia stand-in is a per-attribute write script) block all 8 workers; script/alarm execution can stall until the watchdog detaches/replaces blocked workers. The nominal 30 s timeout + 30 s grace is not a guaranteed recovery time; below saturation these synchronous paths consume additional worker capacity. The class comment at `ScopeAccessors.cs:12-21` asserts the opposite. **Fix:** `.ConfigureAwait(false)` on the awaits in `GetAttribute`, `SetAttribute`, `WaitInternal`, `WriteBatchAndWait` (AsyncLocal `TraceStepScope` flows with `ExecutionContext`, not the scheduler, so attribution is unaffected); correct the comment; add a test running `poolSize` concurrent bodies each doing one sync `Attributes["x"]` on a scheduler of that size. Small.
|
||||
|
||||
**SR-2 — Alarm on-trigger runs have no integration surface `[Medium][CONFIRMED]`**
|
||||
`ScriptRunLauncher.cs:701-725` (`RunAlarmScriptAsync`) never creates a DI scope; `_externalSystemClient`, `_databaseGateway`, `_storeAndForward`, `_operationTrackingStore`, `_cachedForwarder`, `_auditWriter`, `_siteId`, `_sourceNode` are null, so `ExternalSystem`/`Database`/`Notify`/`Tracking` throw (`ScriptRuntimeContext.cs:1422/1485/2145/2195/2760`). The deploy gate compiles alarm on-trigger scripts against the full `ScriptCompileSurface`, so `Notify.To("Ops").Send(...)` deploys cleanly and throws the first time the alarm fires. `Component-SiteRuntime.md:482` says the full API is available to alarm on-trigger runs. **Fix:** wire the scope + dependencies exactly as `RunScriptAsync` does (small), or narrow the doc and gate it at design time (medium).
|
||||
@@ -69,26 +82,68 @@ Each finding: where (file:line), what the code does, why it matters, fix, effort
|
||||
**SR-3 — `NativeAlarmActor` subscribe retry has no in-flight guard, no backoff, no correlation match `[Medium][CONFIRMED]`**
|
||||
`Actors/NativeAlarmActor.cs:147-165, 320-340`: fresh correlation id per send, discarded; fixed-interval re-send; `HandleSubscribeResponse` checks only `Success`. This is the amplifier Lever 1 (2026-08-28) closed on `InstanceActor` and left as a follow-up; a late `Success` from a superseded request also cancels the timer armed for the current one. **Fix:** port `TagSubscribeState` (in-flight id + ceiling, `NextTagSubscribeInterval`, drop non-matching replies). Small.
|
||||
|
||||
**Second-pass clarification:** Lost replies retry at twice the configured interval; explicit failures use the configured interval. DCL does have `_alarmSubscribesInFlight` deduplication, so repeated requests are not proof that every retry opens another physical feed. Correlation and stale-response handling remain defective.
|
||||
|
||||
**SR-4 — `AuditingDbConnection` forwards a settable `ConnectionString` while audit rows keep the construction-time name `[Medium][CONFIRMED]`**
|
||||
`Scripts/AuditingDbConnection.cs:113-120` vs `:35`; `AuditingDbCommand.cs:426`. A script can `Close()`, assign an arbitrary connection string, `Open()`, and every `DbWrite` row still says `target = "MES-Prod"`; with `Persist Security Info=true` the getter returns the password. The audit log is the compensating control for the raw-ADO escape hatch. **Fix:** override `ConnectionString` (set → throw, get → empty) and `ChangeDatabase` (throw). Small.
|
||||
`Scripts/AuditingDbConnection.cs:113-120` vs `:35`; `AuditingDbCommand.cs:426`. A script can `Close()`, assign an arbitrary connection string, `Open()`, and every `DbWrite` row still says `target = "MES-Prod"`; with `Persist Security Info=true` the getter returns the password. The audit log is the compensating control for the raw-ADO escape hatch. **Fix:** reject connection retargeting and redact the getter, then close the raw-transaction escape in SR-11; the class is already sealed. Small plus SR-11.
|
||||
|
||||
**SR-5 — `InstanceActor.PostStop` blocks the default dispatcher for up to 5 s per instance `[Medium][CONFIRMED]`**
|
||||
`Actors/InstanceActor.cs:129, 443-444, 693-708` (`flush.Wait(FinalStaticOverrideFlushTimeout)`); no `WithDispatcher` anywhere in SiteRuntime. A bulk redeploy landing during an `EventLogPurgeService` tick (1000-row `DELETE` + `incremental_vacuum` under `SiteEventLogger._writeLock`, `EventLogPurgeService.cs:519-552`) stops N actors concurrently, each burning a dispatcher thread on `SQLITE_BUSY` backoff — node-wide, since DCL, Deployment Manager, Script/Alarm actors and the stream relay share it. **Fix:** gate the redeploy clear on an explicit "flush committed" ack piped from the persist task instead of `Terminated` + a blocking wait; or ≤ 1 s timeout + cap concurrent stops. Medium.
|
||||
`Actors/InstanceActor.cs:129, 443-444, 693-708` (`flush.Wait(FinalStaticOverrideFlushTimeout)`); no `WithDispatcher` anywhere in SiteRuntime. A bulk redeploy landing during an `EventLogPurgeService` tick (1000-row `DELETE` + `incremental_vacuum` under `SiteEventLogger._writeLock`, `EventLogPurgeService.cs:519-552`) stops N actors concurrently, each burning a dispatcher thread on `SQLITE_BUSY` backoff — node-wide, since DCL, Deployment Manager, Script/Alarm actors and the stream relay share it. **Fix:** gate the redeploy clear on an explicit "flush committed" ack piped from the persist task instead of `Terminated` + a blocking wait; with bounded stop fanout. A shorter blocking timeout alone does not establish safe flush/reset ordering. Medium.
|
||||
|
||||
**SR-6 — Trace arm is consumed by any subsequent value change, not the traced write's echo `[Low][CONFIRMED]`** — `InstanceActor.cs:185, 775-780, 1085-1092`; a periodic device publish inside the 30 s window is stamped with the writer's execution id and the real echo goes untraced. Debug fidelity only. Fix: correlate on the `WriteTagResponse` id or written value, or narrow the window. Small.
|
||||
**SR-6 — Trace arm is consumed by any subsequent value change, not the traced write's echo `[Low][CONFIRMED]`** — `InstanceActor.cs:185, 775-780, 1085-1092`; a periodic device publish inside the 30 s window is stamped with the writer's execution id and the real echo goes untraced. Debug fidelity only. Fix: define explicit echo-matching semantics or mark linkage inferred; device updates carry no write-request id, and clamped/coerced values need deliberate handling. Narrowing the window alone is only a heuristic. Small.
|
||||
|
||||
**SR-7 — `EmitTrace`/`Collect` log their own failures through the decorated logger `[Low][CONFIRMED]`** — `ScriptRuntimeContext.cs:415-419, 551, 562, 568` use `_logger` (the `TraceCapturingLogger` on a traced run) where `EmitLog` (:590-645) correctly uses `_innerLogger`; each failed trace write becomes a `ScriptLog` row plus a second write attempt (bounded at depth 2). Fix: `_innerLogger`. Small.
|
||||
|
||||
**SR-8 — `ScriptExecutionScheduler.TryExecuteTaskInline` keys on the thread-name prefix, not identity `[Low][CONFIRMED]`** — `:385-393` vs naming at `:331-335`; co-resident schedulers (the #18 seam) cross-inline, defeating the bound and detach bookkeeping. Latent. Fix: `[ThreadStatic] CurrentScheduler` + `ReferenceEquals`. Small.
|
||||
|
||||
**SR-9 — `TriggerEvalGate.Shared` lacks the disposed re-create guard its comment claims to mirror `[Low][CONFIRMED]`** — `TriggerEvalGate.cs:55-64` vs `ScriptExecutionScheduler.cs:125-137`; a disposed shared gate makes every Expression trigger on the node evaluate false forever (caught as "expression false"). Fix: `IsDisposed` guard. Small.
|
||||
**SR-9 — Shared trigger gate does not recreate a disposed instance `[Low][CONFIRMED latent lifecycle gap]`**
|
||||
`SiteRuntime/Scripts/TriggerEvalGate.cs:55-64` returns the existing shared instance after disposal,
|
||||
unlike the scheduler's disposed guard. No production call disposing that shared gate was found;
|
||||
this is not an established current node-outage path. **Fix:** either recreate after disposal or
|
||||
make process-lifetime ownership explicit; test Shared→Dispose→Shared if that lifecycle is supported.
|
||||
Small.
|
||||
|
||||
**SR-10 — Script helper properties allocate a fresh helper + guard closure per access `[Low][CONFIRMED]`** — `ScriptRuntimeContext.cs:1070, 1092, 1130, 1150, 1213`. Fix: lazily cached fields. Small.
|
||||
|
||||
**SR-11 — A raw transaction exposes the underlying connection and bypasses DB auditing `[High][CONFIRMED, isolated reproduction, orch-verified]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/AuditingDbConnection.cs:149-150` returns the
|
||||
provider's transaction. `AuditingDbCommand.cs:172-175` also exposes its raw transaction. A script
|
||||
can call `connection.BeginTransaction().Connection`, create a command on that raw connection,
|
||||
and execute it without `AuditingDbCommand`; abstract ADO types are permitted by the trust policy.
|
||||
This also bypasses any ConnectionString/ChangeDatabase hardening proposed under SR-4.
|
||||
An in-memory SQLite probe constructed the internal wrapper through a test harness, then used
|
||||
ordinary public ADO APIs: normal command audit count **1**, transaction connection identity
|
||||
**the original provider connection**, escaped write rows **1**, escaped audit count **0**.
|
||||
Reflection was only harness setup, not part of the script escape. **Fix:** wrap transactions,
|
||||
return the auditing connection from transaction.Connection, and unwrap only internally when
|
||||
assigning a provider command transaction. Cover sync/async starts and Transaction readback.
|
||||
Test exactly-one audit through every public connection/transaction/command path. Medium.
|
||||
|
||||
**SR-12 — Late native-alarm rehydration can resurrect already resolved conditions `[Medium][CONFIRMED source path]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/NativeAlarmActor.cs:125-128` starts an asynchronous
|
||||
storage read and immediately subscribes. `HandleRehydration:167-175` guards only current
|
||||
`_alarms.ContainsKey`; :219 inserts old state. A resolved live transition (:297-306) or completed
|
||||
empty snapshot (:251-282) removes that key. If the old storage read arrives afterward, the prior
|
||||
active condition is restored and emitted. **Fix:** fence rehydration against the first authoritative
|
||||
snapshot and track live-touched keys including deletions. Test delayed read completion after both
|
||||
a resolved live event and an empty snapshot; neither may resurrect the old alarm. Small.
|
||||
|
||||
**SR-13 — An in-progress S&F sweep starts further deliveries after node demotion `[Medium][CONFIRMED source path]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.StoreAndForward/StoreAndForwardService.cs:828-846` checks the active
|
||||
gate once before loading a batch. Lanes wait at :864 and start each remaining delivery at :873
|
||||
without rechecking; :917 calls the external handler. A demotion during the first slow delivery
|
||||
leaves the old node starting additional work while the new active can drain replicated rows.
|
||||
This exceeds a single already-in-flight delivery window. **Fix:** recheck a fail-closed gate before
|
||||
each new attempt, including after lane waits, and abandon the remaining batch on demotion.
|
||||
Already-sent operations still need idempotency/fencing for ambiguous outcomes. Test a blocked first
|
||||
handler, flip the gate, release it, and assert no second attempt starts on the old node. Small.
|
||||
|
||||
### 2.2 Communication / ClusterInfrastructure / Host (CM)
|
||||
|
||||
**CM-1 — No gRPC message-size limits anywhere; the audit batch arithmetic can exceed the 4 MB defaults and head-of-line-stalls the site audit pipeline permanently `[High][CONFIRMED, orch-verified]`**
|
||||
`Host/Program.cs:291-294` (central `AddGrpc`) and `:825-829` (site) set no `MaxReceiveMessageSize`; neither do `CentralChannelProvider.cs:111-120`, `SitePairChannelProvider.cs:424-437`, `SiteStreamGrpcClient.cs:87-95`; a repo-wide grep is empty. Defaults: receive 4 MB both ends, send unlimited. `SiteAuditTelemetryOptions.BatchSize = 256` × `AuditLogOptions.ErrorCapBytes = 65 536` = 16 MB possible; ~63 large error rows in one window cross 4 MB. `SqliteAuditWriter.cs:659-661` serves a deterministic oldest-first window and `SiteCommunicationAuditClient.cs:78-82` deliberately does not catch, so the same 256 rows fail every tick forever. `PurgeExpiredAsync` (:1170-1176) never purges `Pending`, so the site audit DB on `/app/data` grows unbounded behind the stuck head. Same arithmetic on the reconciliation pull (`BatchSize = 256`, site → central, 4 MB client receive) and on `DeployArtifactsCommand` (unlimited central send, 4 MB site receive). **Fix:** explicit `MaxReceiveMessageSize` (e.g. 32 MB) on both `AddGrpc` calls and every channel, **and** a byte budget on the drain/pull mirroring LocalDb's `MaxBatchBytes`; at minimum treat `StatusCode.ResourceExhausted` in `SiteAuditTelemetryActor` as "halve the batch". Test: serialize a 256-row batch of `ErrorCapBytes` rows and assert it fits the configured limit. Medium.
|
||||
**CM-1 — No explicit gRPC message-size configuration; the audit batch arithmetic can exceed the 4 MB defaults and can repeatedly stall the same oversized audit window `[High][CONFIRMED, orch-verified]`**
|
||||
`Host/Program.cs:291-294` (central `AddGrpc`) and `:825-829` (site) set no `MaxReceiveMessageSize`; neither do `CentralChannelProvider.cs:111-120`, `SitePairChannelProvider.cs:424-437`, `SiteStreamGrpcClient.cs:87-95`; a repo-wide grep is empty. Defaults: receive 4 MB both ends, send unlimited. `SiteAuditTelemetryOptions.BatchSize = 256` × `AuditLogOptions.ErrorCapBytes = 65 536` = 16 MB possible; 64 error-only rows at the cap already exceed 4 MiB once protobuf framing is included. `SqliteAuditWriter.cs:659-661` serves a deterministic oldest-first window and `SiteCommunicationAuditClient.cs:78-82` deliberately does not catch, so an unchanged oversized window fails again on each tick. `PurgeExpiredAsync` (:1170-1176) never purges `Pending`, so the site audit DB on `/app/data` grows unbounded behind the stuck head. Same arithmetic on the reconciliation pull (`BatchSize = 256`, site → central, 4 MB client receive) and on `DeployArtifactsCommand` (unlimited central send, 4 MB site receive). **Fix:** explicit `MaxReceiveMessageSize` (e.g. 32 MB) on both `AddGrpc` calls and every channel, **and** a byte budget on the drain/pull mirroring LocalDb's `MaxBatchBytes`; at minimum treat `StatusCode.ResourceExhausted` in `SiteAuditTelemetryActor` as "halve the batch". Test: serialize a 256-row batch of `ErrorCapBytes` rows and assert it fits the configured limit. Medium.
|
||||
|
||||
**Second-pass clarification:** The isolated serializer probe measured **16,779,264 bytes** for 256 rows containing only a 65,536-byte ErrorDetail, versus the configured-by-framework **4,194,304-byte** receive default. Caps apply per field, not per whole row; account for all fields/framing and single oversize rows. Ordinary pending reads are at `AuditLog/Site/SqliteAuditWriter.cs:587-620`; the original :659 anchor is the cached lane. The stall persists while the same oversized windows remain and neither a smaller reconciliation window nor operator action drains them; no live disk-exhaustion experiment ran.
|
||||
|
||||
**CM-2 — `docker-env2` runs the same simultaneous `--force-recreate` deploy that split site pairs twice, with the bootstrap guard OFF `[Medium][CONFIRMED]`** — `docker-env2/deploy.sh:24` is byte-identical to `docker/deploy.sh:25`; env2's four node appsettings have no `BootstrapGuard` block (`ClusterOptions.cs:144` default false). A deploy there can produce two 1-node clusters that never merge, and env2 is the second real environment gating Transport work. Fix: add the block to all four files. Small.
|
||||
|
||||
@@ -96,7 +151,7 @@ Each finding: where (file:line), what the code does, why it matters, fix, effort
|
||||
|
||||
**CM-4 — `ClusterBootstrapCoordinator` can issue `JoinSeedNodes` during host shutdown `[Low][CONFIRMED]`** — `Host/Actors/ClusterBootstrapCoordinator.cs:186-188` swallows the shutdown cancellation as "partner unreachable", so `RunAsync` (:118-126) self-first-joins with no `ct` re-check. Fix: check `ct` before the join / rethrow. Small.
|
||||
|
||||
**CM-5 — Site-hosted `IngestAuditEvents` / `IngestCachedTelemetry` are permanently inert `[Low][CONFIRMED]`** — `SetAuditIngestActor` (`SiteStreamGrpcServer.cs:159-162`) is called only from `AkkaHostedService.cs:569-570` inside `RegisterCentralActors`, but `SiteStreamGrpcServer` is Site-branch-only (`Program.cs:830, 868`); null on central, never set on site. Both RPCs answer an empty ack + Warning on every real node, reachable on the PSK-gated listener; the comment at `AkkaHostedService.cs:565-568` reads as if live. (XC-6 is the same finding from the RPC-inventory side.) Fix: delete both handlers, the setter and the call site (production path is `CentralControlService`), or return `Unimplemented`. Small.
|
||||
**CM-5 — Site-hosted `IngestAuditEvents` / `IngestCachedTelemetry` are permanently inert `[Low][CONFIRMED]`** — `SetAuditIngestActor` (`SiteStreamGrpcServer.cs:159-162`) is called only from `AkkaHostedService.cs:569-570` inside `RegisterCentralActors`, but `SiteStreamGrpcServer` is Site-branch-only (`Program.cs:830, 868`); null on central, never set on site. Both RPCs answer an empty ack + Warning on every real node, reachable on the PSK-gated listener; the comment at `AkkaHostedService.cs:565-568` reads as if live. (XC-6 is the same finding from the RPC-inventory side.) Fix: remove inert host wiring and return `Unimplemented` from retired handlers while preserving additive proto compatibility; production ingest uses `CentralControlService`. Small.
|
||||
|
||||
**CM-6 — Central Kestrel sets no HTTP/2 keepalive, and the optional metrics listener exposes the whole routing table `[Low][CONFIRMED]`** — `Program.cs:231-263` (no `Limits.Http2.KeepAlivePing*`, unlike `SiteKestrelSetup.cs:51-52`); `:254-262` binds `MetricsPort` as `Http1AndHttp2` on the shared pipeline. Defensible today (unary-only `CentralControlService`), undocumented. Fix: apply the site keepalive on central; document the metrics-port surface. Small. (See XC-1 for the larger form of this.)
|
||||
|
||||
@@ -104,6 +159,39 @@ Each finding: where (file:line), what the code does, why it matters, fix, effort
|
||||
|
||||
**CM-8 — `SiteStreamGrpcClient.ConnectedHeaderTimeout` is a mutable static test seam `[Low][CONFIRMED]`** — `:460`, mutated by `SiteStreamGrpcClientTests.cs:440, 474` while `SiteAlarmAggregatorActorTests` (:865) depends on the default; a parallel-run flake source of the register-row-118 kind. Fix: instance value from `CommunicationOptions`. Small.
|
||||
|
||||
**CM-9 — Per-site authentication is not bound to payload site ownership `[High][CONFIRMED source path and isolated gate probe, orch-verified]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.Host/CentralControlAuthInterceptor.cs:149-188` authenticates the key
|
||||
for the metadata `x-scadabridge-site`, but neither records an authoritative site principal nor
|
||||
validates the decoded payload. `Communication/Grpc/CentralControlGrpcService.cs:258-268` forwards
|
||||
`ReconcileSiteRequestDto` unchanged through `CentralControlDtoMapper.cs:381`.
|
||||
`Communication/ReconcileService.cs:64-79,110-161` resolves the payload SiteIdentifier and returns
|
||||
fresh/existing deployment fetch tokens for that site's snapshots. Thus a caller holding only
|
||||
site-A's PSK can name site-B in its request and obtain B's deployment fetch capability, subject to
|
||||
B having deployments. Health/audit source-site fields also retain untrusted payload attribution.
|
||||
The isolated real interceptor+mapper probe accepted site-A metadata/key with a site-B reconciliation
|
||||
payload. No production secrets were fetched. **Fix:** carry verified site identity into dispatch,
|
||||
reject mismatched reconciliation/health/notification ownership, validate every audit/telemetry row
|
||||
against authenticated site policy, and authorize status queries by stored ownership. Preserve
|
||||
legitimate cross-site execution links without letting a caller forge the owning site. Test a
|
||||
cross-site request through the actual interceptor+service+repository path and prove no token or
|
||||
mutation is produced. Medium.
|
||||
|
||||
**CM-10 — `Unavailable` triggers blind replay of non-idempotent site commands `[High][CONFIRMED retry mechanism, orch-verified]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.Communication/Grpc/SitePairChannelProvider.cs:226-237` retries every
|
||||
`RpcException(Unavailable)` on the second node, asserting the call never reached a server.
|
||||
`GrpcSiteTransport.cs:125-175` uses this for lifecycle, tag writes, and `RouteToCall` commands.
|
||||
A response-side connection loss can occur after application; gRPC does not guarantee that
|
||||
`Unavailable` means unsent ([gRPC error guide](https://grpc.io/docs/guides/error/),
|
||||
[HTTP/2 protocol](https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md)).
|
||||
An isolated provider probe applied a synthetic effect then threw Unavailable: the provider applied
|
||||
it again on the peer and returned success (**2 applications**). This proves retry behavior under
|
||||
an injected ambiguous outcome, not a live-device duplicate. `GrpcCentralTransport.cs:176-186`
|
||||
shares the incorrect premise, although notification/audit identity dedup protects some operations.
|
||||
**Fix:** classify operations and guarantee durable/shared deduplication before retrying mutations,
|
||||
or return an explicit uncertain outcome after a possibly committed send. Restrict unprotected
|
||||
retry to provable pre-dispatch failures; merely checking status is insufficient. Test response-loss
|
||||
after commit and assert one effect across the pair. Medium, with cross-layer operation identity.
|
||||
|
||||
### 2.3 ManagementService / DeploymentManager / TemplateEngine / ScriptAnalysis (MG)
|
||||
|
||||
**MG-1 — Script trust deny-list omits `System.Runtime.CompilerServices` and `System.Runtime.Loader` `[High][CONFIRMED, orch-verified]`**
|
||||
@@ -112,81 +200,229 @@ Each finding: where (file:line), what the code does, why it matters, fix, effort
|
||||
**MG-2 — `/api/audit/tree` and `FlowDebugReplayCommand` apply no site scope `[Medium][CONFIRMED, orch-verified]`**
|
||||
`AuditEndpoints.cs:280-305` (`HandleTree`) authenticates and role-checks but never calls `ApplySiteScope` (which `HandleQuery`/`HandleExport` do at :584); `ManagementActor.cs:423` passes only `sp, cmd` to `FlowDebugRunner.ReplayAsync` (:538), which takes no `AuthenticatedUser`; `AuditLogRepository.cs:946-1077` walks the whole `(ExecutionId, ParentExecutionId)` chain with no `SourceSiteId` predicate. A site-A-scoped operator picks any `ExecutionId` from their (correctly scoped) query and receives site-B's `ScriptRun`/`AttributeWrite`/`DbOutbound` rows (SQL text + bound values) from a multi-site chain — the boundary rows 98/99 and P6-4/P6-5 hardened on every other read path. The three Live run commands DO enforce scope. **Fix:** optional permitted-`SourceSiteId` set on `GetExecutionTreeAsync`/`GetTraceEventsAsync` (NULL = central rows, always visible); thread the user into `ReplayAsync`; stop-gap: refuse both for scoped non-admins. Medium.
|
||||
|
||||
**MG-3 — `StaleInstanceProbe`'s process-static memo can outlive a standby period and report "up to date" for a changed instance `[Medium][CONFIRMED]`**
|
||||
`DeploymentManager/StaleInstanceProbe.cs:45, 120-140` (static `ConcurrentDictionary` keyed on an in-memory, process-local `ITemplateGraphWatermark`); `DeploymentService.cs:1162-1211`. A active → standby (Traefik drops it, its watermark freezes) → B edits templates → A re-promoted: A's memo is still "valid" by its own unchanged watermark and serves the pre-edit hash, so `DeploySiteAsync` skips genuinely changed instances as `up-to-date` with no signal; only `--force` recovers. The class comment (:28-33, "can only ever cause EXTRA work, never stale work") covers only a cold process. **Fix:** `ClearMemos()` (exists as a test hook) on the active-node edge, the same `ActiveNodeEvaluator` edge that arms `IActiveNodeGate`. Small.
|
||||
**MG-3 — Process-local stale-probe invalidation misses configuration edits on the other central `[Medium][CONFIRMED, corrected trigger]`**
|
||||
`DeploymentManager/StaleInstanceProbe.cs:45,120-140` caches against an in-process watermark;
|
||||
`ConfigurationDatabase/Repositories/TemplateEngineRepository.cs:772-774,888-897` advances only
|
||||
that process's injected watermark after writes. `ManagementService/ManagementActor.cs:343-350`
|
||||
accepts configuration handlers without an active-node gate. Warm A's memo, then edit the shared
|
||||
configuration DB through an authenticated direct connection to B: A's cached revision remains
|
||||
valid by its own unchanged watermark, so `DeploymentService.cs:1178` can skip a changed instance.
|
||||
The earlier A→standby→A scenario was not established: planned failover leaves/restarts the process,
|
||||
which clears static memory. **Fix:** authoritative shared revisions or reliable cross-node cache
|
||||
invalidation; alternatively remove memoized hashes from deployment skip decisions. Activation-only
|
||||
clearing is insufficient while the active process remains active. Test two independently owned
|
||||
watermarks against a shared store and a peer-originated edit. Medium.
|
||||
|
||||
**MG-4 — `MgmtDeploySiteCommand`'s worst-case runtime exceeds its own Ask timeout by construction `[Medium][CONFIRMED]`**
|
||||
`ManagementEndpoints.cs:44-77` (`LongRunningCommandTimeout` 5 min); `DeploymentManagerOptions.cs:34, 42` (parallelism 4 × 120 s per instance → `ceil(N/4) × 120 s`; N = 11 is already 360 s, before serial prepare and finalize). The caller gets `504` while the batch keeps applying; a redeploy is not idempotent at the site (resets overrides, clears native alarm state), so the retry re-applies live instances and its prepare loses the 5 s lock timeout on still-held locks. **Fix:** derive the bulk Ask timeout from the batch, or make bulk deploy submit-and-poll; at minimum validate at startup and document. Medium.
|
||||
`ManagementEndpoints.cs:44-77` (`LongRunningCommandTimeout` 5 min); `DeploymentManagerOptions.cs:34, 42` (parallelism 4 × 120 s per instance → `ceil(N/4) × 120 s`; N = 9 already needs three waves (360 s), before serial prepare and finalize). The caller gets `504` while the batch keeps applying; a redeploy is not idempotent at the site (resets overrides, clears native alarm state), so the retry re-applies live instances and its prepare loses the 5 s lock timeout on still-held locks. **Fix:** derive the bulk Ask timeout from the batch, or make bulk deploy submit-and-poll with stable operation identity. Propagate the overall budget through CLI/HTTP as well; a longer inner Ask alone cannot prevent caller timeout. Medium.
|
||||
|
||||
**MG-5 — `FlowDebugRunner.MaxWaitSeconds` equals the Ask timeout it must stay under, unvalidated `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:56-63` (300 s) vs `ManagementServiceOptionsValidator.cs:26-28` (only `> 0`); a `--wait-seconds 300` run 504s the caller while the runner keeps a scoped provider and polls the site. Fix: clamp to `LongRunningCommandTimeout − grace` + validator rule. Small.
|
||||
**MG-5 — `FlowDebugRunner.MaxWaitSeconds` equals the Ask timeout it must stay under, unvalidated `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:56-63` (300 s) vs `ManagementServiceOptionsValidator.cs:26-28` (only `> 0`); a `--wait-seconds 300` run 504s the caller while the runner keeps a scoped provider and polls the site. Fix: establish a total deadline at command entry and pass remaining budget through the initial write, audit, and every pull; reserve response grace and validate configuration. A poll-only clamp ignores time already spent before polling. Small.
|
||||
|
||||
**MG-6 — `WriteAndPollAsync` re-fetches a fixed oldest page every second; register row 117 confirmed, plus a silent 64-id truncation `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:501` (`since` never advances), `:510` (`ids.Take(64)` drops descendant ids beyond `MaxExecutionIdsPerRead` with no diagnostic); the site serves `ORDER BY OccurredAtUtc ASC LIMIT` (`SqliteAuditWriter.cs:899-908`), so > 2000 rows in the window makes the awaited run unreachable and up to 300 polls re-transfer the same page. Fix: advance `since` (dedup by known ids) or exclude `ScriptLog` from the awaited poll; warn when `ids.Count > 64`. Small.
|
||||
**MG-6 — `WriteAndPollAsync` re-fetches a fixed oldest page every second; register row 117 confirmed, plus a silent 64-id truncation `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:501` (`since` never advances), `:510` (`ids.Take(64)` drops descendant ids beyond `MaxExecutionIdsPerRead` with no diagnostic); the site serves `ORDER BY OccurredAtUtc ASC LIMIT` (`SqliteAuditWriter.cs:899-908`), so > 2000 rows in the window makes the awaited run unreachable and up to 300 polls re-transfer the same page. Fix: use timestamp-plus-event-id keyset paging and explicit per-execution batching/truncation. Advancing only a timestamp can lose ties and older rows belonging to newly discovered descendants; test both plus more than 64 ids. Small.
|
||||
|
||||
**MG-7 — The value-change / write-kind Flow Debugger result bypasses the `FlowTraceCollector` caps `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:525-526` returns all of `known` with `Truncated: false` hardcoded, vs the capped paths at `:190-197`/`:416-418`; 200 log lines × a cascade is an unbounded reply through `SerializeResult` and the Blazor circuit. Fix: feed `known` through a collector (null redactor — rows were redacted at the site). Small. (Same class as DL-3.)
|
||||
|
||||
**MG-8 — Shared scripts never reach the design-time trust/compile gate on create/update `[Low][CONFIRMED]`** — `TemplateEngine/SharedScriptService.cs:60, 109, 178-206` run only `CSharpDelimiterScanner`; `ValidationService.cs:276-293` compiles `configuration.Scripts` only; `ArtifactDeploymentService.cs:172-179` ships them fleet-wide; only `BundleImporter` (:912, 4658) gates them. A forbidden-API shared script is rejected at the site's `ScriptCompilationService.Compile`, at run time. Fix: `ScriptCompiler.TryCompile` in create/update (memoised by `ScriptCompileVerdictCache`). Small.
|
||||
|
||||
**MG-9 — `ManagementActor.Page` turns an out-of-range `Take` into unlimited `[Low][CONFIRMED]`** — `:685-691` (`take is > 0 and <= 1000 ? take : int.MaxValue`; comment says clamped). Fix: `Math.Min(take, 1000)`. Small.
|
||||
**MG-9 — `ManagementActor.Page` turns an out-of-range `Take` into unlimited `[Low][CONFIRMED]`** — `:685-691` (`take is > 0 and <= 1000 ? take : int.MaxValue`; comment says clamped). Fix: clamp positive takes to 1000 while explicitly preserving/documenting null and nonpositive semantics; test every boundary. Small.
|
||||
|
||||
**MG-10 — `Component-ManagementService.md` documents none of the four `FlowDebug*` commands `[Low][CONFIRMED]`** — zero occurrences, though `RequiredRoleMatrixTests.cs:16-18` names the catalog as its cross-check. Fix: add the subsection. Small.
|
||||
|
||||
### 2.4 Central UI / Security (UI)
|
||||
|
||||
**UI-1 — Central UI configuration edits bypass `IAuditService`: the Configuration Audit Log misses every change made from the UI `[High][CONFIRMED, orch-verified]`**
|
||||
Seventeen page files mutate entities through the repositories and call `SaveChangesAsync()` with no `LogAsync`: `DataConnectionForm.razor:337, 347, 349`; `LdapMappingForm.razor:170, 175, 178, 200, 223`; `SiteForm.razor:169, 186, 189`; `Sites.razor:224`; `SmtpConfiguration.razor:359, 372`; `SmsConfiguration.razor:264, 287`; `ExternalSystemForm.razor:149, 160`; `DbConnectionForm.razor:103, 112`; `ApiMethodForm.razor:311, 323`; `NotificationListForm.razor:191, 199, 241, 267`; deletes in `DataConnections.razor:420`, `ExternalSystems.razor:192, 265, 340`, `LdapMappings.razor:150-153`; `TemplateEdit.razor:1088-1119`; `InstanceConfigure.razor:1326, 1764-1788`. Only `TransportImport.razor.cs:86` injects `IAuditService`. The `ManagementActor` path for the same entities audits every create/update/delete (`AuditAsync` at :1849 … :3615). An Administrator who grants themselves Administrator through an LDAP mapping in the UI leaves no `AuditLogEntry`; the same act via CLI does. The UI path also skips `ConfigSecretScrubber.MergeSentinels` and the server-side delete/move guards. `Component-CentralUI.md` §Configuration Audit Log Viewer promises before/after per change (and `AuditLogEntry` only has `AfterStateJson` anyway). **Fix:** route these pages through `ManagementActorHolder` envelopes as `SecuredWriteService`/`DataConnectionMoveService`/`FlowDebugService` already do (large); stop-gap: inject `IAuditService` and `LogAsync` before `SaveChangesAsync` in the same transaction (medium); add a sweep test that no `Components/Pages/**` file calls `Repository.(Add|Update|Delete)*Async` without an audit.
|
||||
**UI-1 — Many Central UI configuration mutations bypass configuration auditing `[High][CONFIRMED, corrected scope, orch-verified]`**
|
||||
Sixteen page files call repository `SaveChangesAsync` directly: `NotificationLists`,
|
||||
`NotificationListForm`, `SmtpConfiguration`, `SmsConfiguration`, `ExternalSystemForm`, `SiteForm`,
|
||||
`LdapMappingForm`, `ApiMethodForm`, `TemplateEdit`, `LdapMappings`, `DbConnectionForm`,
|
||||
`DataConnectionForm`, `ExternalSystems`, `Sites`, `DataConnections`, and `InstanceConfigure`.
|
||||
Examples under `CentralUI/Components/Pages/`: `Admin/LdapMappingForm.razor:170-178,200-223`,
|
||||
`Design/DataConnectionForm.razor:337-349`, `Notifications/NotificationLists.razor:168-169`, and
|
||||
`Design/TemplateEdit.razor:1088-1119` (native alarms). Role/scope grants and connection changes
|
||||
can commit without the configuration audit row that the corresponding management path writes.
|
||||
Repository bypasses also miss application-level scrubber/validation/delete guards.
|
||||
The original “every edit” claim was incorrect: TemplateEdit's ordinary template/script/attribute
|
||||
changes call audited `TemplateService` (`TemplateService.cs:144,410,802`), and
|
||||
`SharedScriptService.cs:77,118,141` audits its mutations. **Fix:** route the bypass paths through
|
||||
audited application services/management envelopes. A temporary audit integration must use the
|
||||
same transaction, scrub secrets, and handle generated IDs correctly. Test actual UI create/update/
|
||||
delete success and rollback, including LDAP mappings; a source-string sweep alone is insufficient.
|
||||
Large.
|
||||
|
||||
**UI-2 — The documented 30-minute idle timeout never fires while a Central UI tab is open `[High][CONFIRMED, orch-verified]`**
|
||||
`Components/Shared/SessionExpiry.razor:37, 49, 64, 80-100` (mounted unconditionally at `MainLayout.razor:40`) fetches `/auth/ping` every minute with no user-interaction signal; `/auth/ping` (`Auth/AuthEndpoints.cs:186-206`) is a normal request, so the cookie middleware's `SlidingExpiration` renews the window and `CookieSessionValidator.TryRefreshAsync` (:169-206) advances `zb:lastactivity`. The component's own comment (:24-26) concedes "the ping itself slides the cookie". `Component-Security.md:54` says an idle session is rejected; on an unattended HMI workstation with the browser open it never is. **Fix:** `OnCheckSlidingExpiration` → `ShouldRenew = false` for `/auth/ping`, and skip the `LastActivity` advance on that path; optionally a browser idle detector that sends `?active=1` only after real input. Test: advance the clock past `IdleTimeoutMinutes` while pings continue. Small.
|
||||
**UI-2 — Passive session checks keep an unattended tab authenticated `[High][CONFIRMED, orch-verified]`**
|
||||
`CentralUI/Components/Shared/SessionExpiry.razor:37,49,64,80-100` pings every minute without an
|
||||
interaction signal. Normal cookie sliding renewal applies, and
|
||||
`Security/CookieSessionValidator.cs:177-210` periodically rebuilds the principal with a fresh
|
||||
LastActivity anchor when role mapping refresh succeeds. `Security/ServiceCollectionExtensions.cs:274-275`
|
||||
explicitly renews that replacement too. With healthy mapping, an unattended tab can remain logged
|
||||
in indefinitely. **Fix:** distinguish passive validity checks from actual activity: disable passive
|
||||
sliding and Replace-driven activity renewal, preserve role refresh without advancing idle time,
|
||||
and supply real interaction tracking including Blazor input. Merely stopping ping renewal can also
|
||||
expire actively used circuits because SignalR events are not new cookie-authenticated HTTP requests.
|
||||
Test idle pings beyond the timeout and genuine interaction over the same interval; separately fix
|
||||
UI-13's stale circuit authority. Medium.
|
||||
|
||||
**UI-3 — Flow Debugger's pending-refresh loop mutates component state off the renderer dispatcher `[Medium][CONFIRMED]`** — `FlowDebugger.razor.cs:611-632` calls `MergeRefreshedGraph` (:657-680, writes `_raw`/`_graph`/`_selected` non-atomically) on a thread-pool continuation before `InvokeAsync(StateHasChanged)`; every other poller mutates inside `InvokeAsync` (`DataConnections.razor:263-276`, `AlarmSummary.razor:459-471`). Torn state / blank detail pane during the 2-min poll; concurrent use of the scoped service. Fix: `InvokeAsync(() => { Merge; StateHasChanged(); })`. Small.
|
||||
|
||||
**UI-4 — Health / Alarm Summary poll timers can strand `PollGate`, silently killing auto-refresh `[Medium][CONFIRMED]`** — `Health.razor:719-734, 999-1002`; `AlarmSummary.razor:394-411`: `_pollGate.Exit()` lives only inside the inner `InvokeAsync` lambda, so a throwing `InvokeAsync` (`ObjectDisposedException` is expected elsewhere, `DebugView.razor:806-816`) leaves `_inFlight = 1` forever; `Health.Dispose` has no `_disposed` flag. Fix: the `DataConnections` shape (volatile `_disposed`, early return, outer try/catch/finally). Small.
|
||||
**UI-3 — Alleged off-renderer Flow Debugger refresh mutation `[WITHDRAWN — unsupported]`**
|
||||
`CentralUI/Components/Pages/Deployment/FlowDebugger.razor.cs:543-609,612-625` starts the refresh
|
||||
from UI event paths and ordinarily awaits both Delay and ReplayAsync. Those caller awaits capture
|
||||
the Blazor renderer synchronization context. A service's internal `ConfigureAwait(false)` does
|
||||
not remove the caller's context capture. No background entry point establishing the alleged
|
||||
cross-thread mutation was found. Explicit `InvokeAsync` could be defensive, but this is not a
|
||||
confirmed defect and is excluded from the active tally.
|
||||
|
||||
**UI-5 — A non-cooperative Test Run script permanently burns a static `SandboxRunGate` slot; four kill Test Run process-wide `[Medium][CONFIRMED]`** — `ScriptAnalysis/ScriptAnalysisService.cs:216-222` (static, 4 slots), `:470-479` (`await Task.Run(() => script.RunAsync(host, linkedCts.Token))`), `:524-527` (release in `finally` only after completion). `while (true) { }` never returns; four such runs disable Test Run for every user until restart, plus four leaked threads. Fix: `WhenAny(run, Delay(timeout))`, release on timeout, observe the abandoned task's fault, log/metric. Small.
|
||||
**UI-4 — Poll-gate cleanup depends on a posted renderer callback running `[Low][CONFIRMED mechanism; live outage not established]`**
|
||||
`CentralUI/Components/Pages/Monitoring/Health.razor:719-734` and `AlarmSummary.razor:394-411`
|
||||
acquire the gate outside the posted callback but release inside it. A rejected callback can leave
|
||||
the gate held and its unobserved task faulted. Rejection normally occurs during disposal, when
|
||||
refresh should stop anyway; the previous continuing-component outage claim was not reproduced.
|
||||
**Fix:** observe dispatch in an outer try/finally, release the gate there, and guard disposed/queued
|
||||
callbacks. Test dispatcher rejection and disposal races. Small hardening change.
|
||||
|
||||
**UI-6 — Manual cluster failover has no server-side role check `[Medium][CONFIRMED]`** — `Components/Health/CentralFailoverControl.razor:5, 11, 20, 99, 114` is gated only by `<AuthorizeView Policy=RequireAdmin>` on a bare-`[Authorize]` page (`Health.razor:292`); `Host/Health/AkkaManualFailoverService.cs` has no role check, unlike `BrowseService.cs:49, 104`, `EndpointVerificationService.cs:51`, `BindingTester.cs:43`, `CertManagementService.cs:51, 87, 119`. Not exploitable today (Blazor validates handler ids) but the most destructive UI action with zero defence-in-depth. Fix: `HasClaim(Administrator)` inside the service. Small.
|
||||
**UI-5 — Noncooperative Test Run scripts permanently occupy admission slots `[Medium][CONFIRMED; remediation corrected]`**
|
||||
`CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:207-214,471-481,524-527` has four static slots,
|
||||
awaits actual Roslyn execution, and releases only when it completes. A cancellation-ignoring loop
|
||||
retains one worker/slot indefinitely; four exhaust Test Run capacity. **Do not release the slot
|
||||
merely because a logical timeout fired:** the work is still running, and admitting replacements
|
||||
permits unbounded runaway CPU/memory and continued side effects through retained gateways.
|
||||
**Fix:** a bounded pool of isolated, killable workers with confirmed termination before reuse.
|
||||
Interim timeout responses must observe/quarantine unfinished work and retain its capacity charge
|
||||
until actual exit. Test noncooperative code only in a disposable child process, including no further
|
||||
operations after termination. Medium; robust containment is larger than the original small fix.
|
||||
|
||||
**UI-7 — No fallback authorization policy and no route→policy sweep test `[Medium][CONFIRMED]`** — `AuthorizationPolicies.AddScadaBridgeAuthorization` sets no `FallbackPolicy`; `Routes.razor` `AuthorizeRouteView` enforces only what a component declares. All 43 pages carry an attribute today (table in §3), but nothing enforces it, and `MapCentralUI` also routes `ZB.MOM.WW.Secrets.Ui`'s pages (`EndpointExtensions.cs:30`). The CLI has a frozen authz matrix test; the UI has none. Fix: `FallbackPolicy = RequireAuthenticatedUser` (Login already has `[AllowAnonymous]`) and/or a reflection sweep with a frozen table. Small.
|
||||
**UI-6 — Manual failover lacks service-level authorization `[Low][CONFIRMED defense-in-depth gap]`**
|
||||
`Host/Health/AkkaManualFailoverService.cs:59,89` has no authoritative caller check; its current
|
||||
component entry is protected by an Admin `AuthorizeView`. No unauthorized invocation of that
|
||||
handler was demonstrated. **Fix:** an authenticated application facade/management command must
|
||||
verify the Administrator role before calling the host service; respect scoped circuit versus
|
||||
singleton service lifetimes. Test denial at that facade. UI-13 separately covers real stale-grant
|
||||
access through an existing circuit. Small.
|
||||
|
||||
**UI-8 — Flow Debugger's parked-node site-scope check fails open on an unresolvable site `[Low][CONFIRMED]`** — `FlowDebugger.razor.cs:811-834` (`if (site is null) return true;` twice); `FlowDebugService.RelayAsync` (:284-345) has no scope check of its own. Fix: deny for scoped principals; move the check into `RelayAsync` (shared with the Site Calls page). Small.
|
||||
**UI-7 — No complete UI route-policy guard `[Low][CONFIRMED prevention gap]`**
|
||||
`Security/AuthorizationPolicies.cs:195-244` defines no fallback policy, and no sweep freezes every
|
||||
routable component's required role. Current application routes do carry authorization attributes;
|
||||
no anonymous existing-page bypass was shown. **Fix:** reflection-based route/policy tests including
|
||||
the external secrets assembly. A global HTTP fallback is useful only after preserving intended
|
||||
auth/bootstrap endpoints and does not itself enforce each interactive component's role policy.
|
||||
Small.
|
||||
|
||||
**UI-9 — `TemplateFolderTree` rebuilds the whole forest on every parameter set `[Low][CONFIRMED]`** — `:99-103` vs the `InstancePicker.razor.cs:157-166` reference-equality guard. Fix: same guard. Small.
|
||||
**UI-8 — Flow Debugger's parked-node site-scope check fails open on an unresolvable site `[Low][CONFIRMED]`** — `FlowDebugger.razor.cs:816-838` (lookup ultimately permits a null site); `FlowDebugService.RelayAsync` (:284-345) has no scope check of its own. Fix: deny for scoped principals; move the check into `RelayAsync` (shared with the Site Calls page). Small.
|
||||
|
||||
**UI-10 — `/auth/login` and `/auth/token` disable antiforgery (login CSRF) `[Low][CONFIRMED]`** — `AuthEndpoints.cs:110, 171`; an attacker with valid directory credentials can sign a victim's browser into the attacker's account so later actions audit under the attacker. Fix: `<AntiforgeryToken/>` in `Login.razor` + drop the override on `/auth/login`; document or content-type-gate `/auth/token`. Small.
|
||||
**UI-9 — `TemplateFolderTree` rebuilds the whole forest on every parameter set `[Low][CONFIRMED]`** — `:99-103` vs the `InstancePicker.razor.cs:157-166` reference-equality guard. Fix: reference guards only under an explicit immutable/reference-change contract covering every tree-affecting parameter; otherwise in-place changes would disappear. Small.
|
||||
|
||||
**UI-10 — Cookie login accepts cross-site form submission without antiforgery validation `[Low][CONFIRMED, narrowed]`**
|
||||
`CentralUI/Auth/AuthEndpoints.cs:25-110` manually reads a form, sets the authentication cookie,
|
||||
and disables antiforgery. An attacker with valid credentials can establish their session in a
|
||||
victim browser, causing later actions to be attributed to that account. `/auth/token:112-171`
|
||||
returns JSON and sets no cookie; the same browser-session claim does not apply there.
|
||||
**Fix:** render an antiforgery token in Login and explicitly validate it or attach required
|
||||
validation metadata to the manual form handler. Removing `DisableAntiforgery` alone is insufficient.
|
||||
Test missing/invalid token rejection and retain legitimate CLI bearer-token exchange. Small.
|
||||
|
||||
**UI-11 — Topology loads every instance and template into the circuit, unpaged `[Low][CONFIRMED]`** — `Topology.razor:346-349`; the report pages page at 50. Fix: lazy per expanded site/area. Medium.
|
||||
|
||||
**UI-12 — Empty "Notifications" nav heading for a Viewer; Flow Debugger unreachable from the nav for the roles its policy admits `[Low][CONFIRMED]`** — `NavMenu.razor:55-58` (false premise: a Viewer holds none of Admin/Design/Deployment) and `:48` (link inside the Deployer section though `RequireFlowDebugger` admits Viewer — the F3 auditor intent is half-served). Fix: any-of gate; own `AuthorizeView`. Small.
|
||||
|
||||
**UI-13 — Existing circuits retain revoked roles and site scopes `[High][CONFIRMED source path, orch-verified]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/CookieAuthenticationStateProvider.cs:42,46-47` serves
|
||||
one captured principal for the circuit lifetime; `Auth/SiteScopeService.cs:95-98,128-129` caches
|
||||
scope too. HTTP cookie refresh (`Security/ServiceCollectionExtensions.cs:274-275`) updates neither.
|
||||
`CentralUI/Auth/AuthEndpoints.cs:214-218` returns 200 while the replacement principal is still
|
||||
authenticated, so the ping never signals a role/site revocation. `Services/FlowDebugService.cs:581-591`
|
||||
builds ManagementEnvelope authority from the frozen principal. An open circuit can therefore keep
|
||||
issuing formerly allowed mutations after an administrator narrows its grants. Fixing UI-2 alone
|
||||
does not revoke grants on an actively used circuit. **Fix:** server-side circuit revalidation and
|
||||
permission-cache invalidation, with authoritative checks at mutation time; publish changed auth
|
||||
state or terminate/re-establish the circuit on authorization-version changes. Repeatedly reading
|
||||
IHttpContextAccessor is not a valid circuit-lifetime solution. Test revoke role and site while the
|
||||
same circuit stays authenticated, then assert no site command is sent. Medium.
|
||||
|
||||
**UI-14 — Sandbox output is bounded only after unbounded capture allocation `[Medium][CONFIRMED source path]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:460` uses an ordinary
|
||||
StringWriter; :488 and error/timeout branches call `captured.ToString()` before :579-582 truncates
|
||||
to the 32k display ceiling. `SandboxConsoleCapture.cs:117-129` forwards every write without a byte/
|
||||
character budget. A finite or cancellation-cooperative script can allocate a large buffer, then a
|
||||
second full string, in the central process. Four slots and a time limit are not memory bounds.
|
||||
**Fix:** a synchronized bounded writer that discards/counts excess without materializing it, including
|
||||
error/timeout paths. Test many writes of a reused large string and concurrent child output with
|
||||
bounded retained memory. Review return-value serialization under the same output budget. Small.
|
||||
|
||||
### 2.5 InboundAPI / ESG / Notifications / SiteCallAudit / AuditLog / KpiHistory / HealthMonitoring (EI)
|
||||
|
||||
**EI-1 — A relayed inbound request that no peer serves is audited nowhere `[Medium][CONFIRMED]`** — `InboundApiEndpointFilter.cs:95-100` sets `Items[RelayedToActiveItemKey] = true` before `ForwardAsync` and never clears it; `AuditWriteMiddleware.cs:236, 251` skips the row when set; `HttpStandbyRequestForwarder.cs:160-163` answers `503 ACTIVE_NODE_UNREACHABLE`. During exactly the failover/partition window operators need to reconstruct, inbound calls leave zero `ApiInbound` rows. `Component-InboundAPI.md:366` says "audited once, where it ran". Fix: set the flag only on a real peer response (or clear it on `Unreachable()`). Small.
|
||||
**EI-1 — Relay failure with all peers unreachable leaves no inbound audit row `[Medium][CONFIRMED, narrowed]`**
|
||||
`InboundAPI/InboundApiEndpointFilter.cs:95-99` sets `RelayedToActiveItemKey` before forwarding;
|
||||
`Middleware/AuditWriteMiddleware.cs:236,251` then suppresses the local row even when
|
||||
`HttpStandbyRequestForwarder.cs:160-163` returns `ACTIVE_NODE_UNREACHABLE`. If every peer is
|
||||
unreachable, none can audit. A reachable standby refusal may audit, so “no peer executes” does
|
||||
not universally mean zero rows; multiple refusal attempts can also create multiple rows.
|
||||
**Fix:** explicit relay-attempt/serving-peer audit ownership and a correlated local failure row when
|
||||
no serving peer accepts. A response alone is not proof of persisted audit. Test all-unreachable,
|
||||
all-standby, successful relay, and active method failure through middleware plus filter. Small.
|
||||
|
||||
**EI-2 — `SkipBodyCapture` does not suppress `errorDetail`, which routinely quotes the body `[Medium][CONFIRMED]`** — `AuditWriteMiddleware.cs:195, 243-249` vs `:335-341`; `InboundScriptExecutor.cs:601-604` (`ErrorDetail: ex.ToString()`); `EndpointExtensions.cs:288-291`. F11 closed this for the `RouteCall` row (`RouteHelper.cs:422`); the failure path re-opens it for every failing call, traced or not (an `ArgumentException` quoting a lot number, a `JsonException` quoting the payload, a `SqlException` echoing a key). Fix: gate the stashed detail on the same `skipBody` verdict (keep `thrown?.Message`), or run per-target redactors over it. Small.
|
||||
**EI-2 — `SkipBodyCapture` does not suppress `errorDetail`, which routinely quotes the body `[Medium][CONFIRMED]`** — `AuditWriteMiddleware.cs:195, 243-249` vs `:335-341`; `InboundScriptExecutor.cs:601-604` (`ErrorDetail: ex.ToString()`); `EndpointExtensions.cs:288-291`. F11 closed this for the `RouteCall` row (`RouteHelper.cs:422`); the failure path re-opens it for every failing call, traced or not (an `ArgumentException` quoting a lot number, a `JsonException` quoting the payload, a `SqlException` echoing a key). Fix: if this option is intended to prevent body-derived persistence, suppress both free-text error message and detail or replace them with a fixed error category; either can quote the payload. The current option contract names request/response summaries only, so document the strengthened privacy semantics and test complete stored rows. Small.
|
||||
|
||||
**EI-3 — Per-target SQL-parameter redaction is not applied to `DbOutbound` error rows `[Medium][CONFIRMED]`** — `ScadaBridgeAuditRedactor.cs:217-231` redacts only the request summary; `InboundDatabaseHelper.cs:251` and `AuditingDbCommand.cs:516` stamp `errorDetail: thrown?.ToString()`, and SQL Server error text carries parameter values (`The duplicate key value is (…)`). Fix: apply the configured regex over `errorDetail`/`errorMessage` in the `DbOutbound` branch. Small.
|
||||
**EI-3 — Per-target SQL-parameter redaction is not applied to `DbOutbound` error rows `[Medium][CONFIRMED]`** — `ScadaBridgeAuditRedactor.cs:217-231` redacts only the request summary; `InboundDatabaseHelper.cs:251` and `AuditingDbCommand.cs:516` stamp `errorDetail: thrown?.ToString()`, and SQL Server error text carries parameter values (`The duplicate key value is (…)`). Fix: the SQL regex matches parameter NAMES, so applying it to free-text errors cannot reliably remove VALUES. Suppress sensitive free-text SQL exceptions (retain error category/number), or redact the matched parameter values at emission plus configured body patterns. Include all error fields; see EI-11. Small.
|
||||
|
||||
**EI-4 — `StandbyForwardTimeout` is never checked against per-method timeouts; expiry duplicates side effects `[Medium][CONFIRMED]`** — `InboundApiOptionsValidator.cs:35-37` (only `> 0`); `HttpStandbyRequestForwarder.cs:112-113, 147-152` (`CancelAfter(60 s)`); `ApiMethod.TimeoutSeconds` unbounded (`EndpointExtensions.cs:260-262`). A 120 s method relayed from the standby → `503` at 60 s while the active node runs it to completion → the caller's documented retry executes it twice. Fix: startup warning; attempt deadline `max(StandbyForwardTimeout, methodTimeout + grace)`. Small.
|
||||
**EI-4 — Relay timeout can expire before method deadline and cause ambiguous replay `[Medium][CONFIRMED mechanism; duplicate impact conditional]`**
|
||||
`InboundAPI/HttpStandbyRequestForwarder.cs:112-113,147-152` times out at the relay budget and tries
|
||||
another peer; method timeouts can be longer (`EndpointExtensions.cs:260-262`). The executor does
|
||||
link `RequestAborted` (`InboundScriptExecutor.cs:342-349,525`), so a cooperative method need not
|
||||
continue. Already-committed side effects, queued operations, and noncooperative code can survive;
|
||||
replaying after a lost response may therefore repeat effects. **Fix:** align an end-to-end deadline
|
||||
and use stable idempotency keys/result deduplication for retriable operations, or return an
|
||||
explicit uncertain outcome instead of automatically replaying an ambiguous send. Increasing the
|
||||
timeout alone does not establish at-most-once behavior. Test both cooperative cancellation and
|
||||
commit-before-response-loss against two peers. Medium.
|
||||
|
||||
**EI-5 — Relayed calls lose the caller's IP in the audit row `[Medium][CONFIRMED]`** — `AuditWriteMiddleware.cs:344` records `ctx.Connection.RemoteIpAddress` (= the standby node); `HttpStandbyRequestForwarder.cs:166-192` adds no `X-Forwarded-For`. Fix: append `X-Forwarded-For`/`-Host`; prefer it in `EmitInboundAudit` only when `X-ScadaBridge-Forwarded` is present. Small.
|
||||
**EI-5 — Relayed audit rows lose verified caller IP `[Medium][CONFIRMED; proposed trust rule corrected]`**
|
||||
`InboundAPI/HttpStandbyRequestForwarder.cs:166-192` supplies no authenticated address provenance;
|
||||
`Middleware/AuditWriteMiddleware.cs:344` records the standby's remote address. The
|
||||
`X-ScadaBridge-Forwarded` marker is caller-controlled loop prevention, not peer authentication.
|
||||
**Fix:** have the standby overwrite verified client provenance and accept it only through a
|
||||
configured trusted-proxy chain or authenticated peer protocol with bounded hops. Reuse the
|
||||
trusted forwarded-header setup in `Host/Program.cs:669-675` where appropriate. Never trust XFF
|
||||
solely because the loop marker exists. Test forged direct headers and a genuine trusted relay.
|
||||
Small.
|
||||
|
||||
**EI-6 — Per-channel retention purge is an unbounded loop on the maintenance path `[Low][CONFIRMED]`** — `AuditLogRepository.cs:699-735` (`while (true) { DELETE TOP … }`) driven serially per channel from `AuditLogPurgeActor.cs:262-325`; a `ScriptTrace`@30 d backlog after `TraceAllCalls` holds the actor and starves later channels. Fix: per-tick row/time budget like `MaxPurgedRowsPerTick`. Small.
|
||||
|
||||
**EI-7 — A hung KPI source silently stops all KPI history for the process lifetime `[Low][CONFIRMED]`** — `KpiHistoryRecorderActor.cs:315-342` awaits `CollectAsync` with only the shutdown token; the in-flight guard stays raised forever. Fix: per-source linked CTS (half the sample interval) + log. Small.
|
||||
**EI-7 — A hung KPI source can stall KPI sampling until actor restart `[Low][CONFIRMED]`** — `KpiHistoryRecorderActor.cs:315-342` awaits `CollectAsync` with only the shutdown token; the in-flight guard stays raised forever. Fix: bound the actual wait, not just a cooperative cancellation token; keep abandoned work/scopes bounded and do not start unlimited replacement tasks. Test a cancellation-ignoring source and later healthy sources. No live production hang was observed. Small.
|
||||
|
||||
**EI-8 — The standby relay replays only status + `Content-Type`, while `Accept-Encoding` passes through `[Low][CONFIRMED]`** — `HttpStandbyRequestForwarder.cs:131-141, 206-236` vs `:59-63`; the day response compression is enabled, callers get gzip labelled `application/json`. Fix: strip `Accept-Encoding` or copy response headers minus hop-by-hop. Small.
|
||||
|
||||
**EI-9 — Outbox dispatch has no row-level claim; dual-active duplicates every notification `[Low][CONFIRMED]`** — `NotificationOutboxRepository.cs:140-158` (plain read), status written only after delivery (`NotificationOutboxActor.cs:601-637`); under the accepted auto-down dual-active state both singletons deliver everything. `Component-NotificationOutbox.md:112` describes "one notification, rare failover". Fix: widen the doc; optionally an `UPDATE`-claim with a lease. Small (doc) / medium.
|
||||
**EI-9 — Outbox dispatch has no row-level claim; dual-active can repeatedly duplicate notifications `[Low][CONFIRMED]`** — `NotificationOutboxRepository.cs:140-158` (plain read), status written only after delivery (`NotificationOutboxActor.cs:601-637`); under the accepted auto-down dual-active state both singletons can pick overlapping batches and repeatedly deliver the same notifications. `Component-NotificationOutbox.md:112` describes "one notification, rare failover". Fix: widen the doc; optionally an `UPDATE`-claim with a lease. Small (doc) / medium.
|
||||
|
||||
**EI-10 — Audit body pre-read disables the configured chunked-request size limit `[Medium][CONFIRMED source path and isolated Kestrel reproduction]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.Host/Program.cs:701-708` mounts audit middleware before endpoint
|
||||
execution. `InboundAPI/Middleware/AuditWriteMiddleware.cs:203-205,495-496` reads body bytes before
|
||||
`InboundApiEndpointFilter.cs:80-84` attempts to set IHttpMaxRequestBodySizeFeature. Kestrel has
|
||||
already made it read-only, so chunked requests without Content-Length skip the intended limit and
|
||||
fall back to the listener/proxy cap. The JSON handler and standby body copy then consume the rest.
|
||||
An isolated real-Kestrel reproduction of this ordering used global limit 1000, desired limit 8,
|
||||
and a 100-byte chunked request: **HTTP 200; IsReadOnly=True; actual cap=1000; accepted bytes=100**.
|
||||
This demonstrates the ordering mechanism, not the entire application pipeline. **Fix:** install the
|
||||
route-specific cap before any audit buffering/read. Test chunked and HTTP/2 unknown-length JSON
|
||||
over the configured cap with audit on, asserting 413 and no handler/relay dispatch; include
|
||||
SkipBodyCapture and declared-length variants. Small.
|
||||
|
||||
**EI-11 — Normal audit redaction leaves ErrorMessage unredacted and uncapped `[Medium][CONFIRMED source path, orch-verified]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.AuditLog/Redaction/ScadaBridgeAuditRedactor.cs:192-214,235-247` redacts
|
||||
and caps request, response, ErrorDetail, and Extra while preserving `d.ErrorMessage` unchanged.
|
||||
`InboundAPI/InboundDatabaseHelper.cs:250` and other emitters put exception.Message there. A global
|
||||
or target secret pattern can scrub the stack detail yet leave the same value in the short error
|
||||
message; long exception messages also bypass the advertised field cap. This is broader than EI-3's
|
||||
SQL-parameter-name policy. **Fix:** apply normal-path body redaction and caps to ErrorMessage too,
|
||||
and verify the entire serialized event contains no secret marker. Test all error fields with
|
||||
global/per-target patterns and oversized messages, not just ErrorDetail. Small.
|
||||
|
||||
### 2.6 Commons / ConfigurationDatabase / Transport (DL)
|
||||
|
||||
**DL-1 — `Instance.UniqueName` is assumed globally unique but the DB enforces only `(SiteId, UniqueName)` `[Medium][CONFIRMED, orch-verified]`** — `InstanceConfiguration.cs:61`; `InstanceLocator.cs:25-26` (what `Route.To` uses to pick the **site**) is `FirstOrDefault(i => i.UniqueName == name)` with no site qualifier or ordering; also `DeploymentManagerRepository.cs:516`, `TemplateEngineRepository.cs:468`. Global uniqueness is an app-side check-then-act (`InstanceService.cs:67-70`) on a per-node `ManagementActor`, so two operators on the two centrals can both commit; `BundleImporter` Rename (:4205-4215) does no check. A duplicate makes `Route.To` route setpoints to the wrong plant with nothing logged. Fix: migration adding a unique index on `UniqueName` alone. Small.
|
||||
**DL-1 — `Instance.UniqueName` is assumed globally unique but the DB enforces only `(SiteId, UniqueName)` `[Medium][CONFIRMED, orch-verified]`** — `InstanceConfiguration.cs:61`; `InstanceLocator.cs:25-26` (what `Route.To` uses to pick the **site**) is `FirstOrDefault(i => i.UniqueName == name)` with no site qualifier or ordering; also `DeploymentManagerRepository.cs:516`, `TemplateEngineRepository.cs:468`. Global uniqueness is an app-side check-then-act (`InstanceService.cs:67-70`) on a per-node `ManagementActor`, so two operators on the two centrals can both commit; `BundleImporter` Rename (:4205-4215) does no check. A duplicate makes `Route.To` route setpoints to the wrong plant with nothing logged. Fix: inventory and resolve existing duplicates, then add a global unique index and translate concurrent/import collisions into domain conflicts. Wrong-site execution remains subject to the caller’s existing site-scope checks. Small.
|
||||
|
||||
**DL-2 — `DataConnection.Primary/BackupConfiguration` hold protocol credentials in plaintext at rest, unlike every sibling secret column `[Medium][CONFIRMED, orch-verified]`** — `ScadaBridgeDbContext.cs:334-353` encrypts exactly four columns (SMTP credentials, SMS auth token, ESG auth config, DB connection string); `ManagementActor.cs:1936` itself says the DataConnection JSON "can embed OPC UA user and certificate passwords"; `Component-Transport.md:98` puts them in the encrypted `SecretsBlock`; `GuardSecretWritesHaveAKeyRing` (:258-268) omits the entity. Caveat: the key ring is `PersistKeysToDbContext` in the same DB without `ProtectKeysWith*` (`ServiceCollectionExtensions.cs:109-110`), so the existing encryption is defence-in-depth, not a hard boundary — but this is the one column class an operator would assume covered. Fix: add the converter to both properties + the guard switch + a one-off re-encrypt migration; or record the deferral in the register and the doc. Medium.
|
||||
|
||||
**DL-3 — Flow Debugger Replay reads an unbounded audit-row set into memory and over the envelope `[Medium][CONFIRMED]`** — `AuditLogRepository.cs:1182-1230` (`GetTraceEventsAsync`, no `Take`); `FlowDebugRunner.cs:544-553` returns every row. `PullTraceEventsMax = 2000` governs only the site RPC; the Live path is capped 500 events / 1 MB. v1.4's 200 `ScriptLog` rows per execution × a tree, `ApiInbound` rows up to 1 MiB each, and the page polls Replay every 5 s for 2 min. Fix: `.Take(PullTraceEventsMax + 1)` → `FlowGraph.Truncated` (the page already renders the badge). Small.
|
||||
**DL-3 — Flow Debugger Replay reads an unbounded audit-row set into memory and over the envelope `[Medium][CONFIRMED]`** — `AuditLogRepository.cs:1182-1230` (`GetTraceEventsAsync`, no `Take`); `FlowDebugRunner.cs:544-553` returns every row. `PullTraceEventsMax = 2000` governs only the site RPC; the Live path is capped 500 events / 1 MB. v1.4's 200 `ScriptLog` rows per execution × a tree, `ApiInbound` rows up to 1 MiB each, and the page polls Replay every 5 s for 2 min. Fix: bounded row paging AND a byte budget, plus truthful `FlowGraph.Truncated` and separately retained root metadata. A 2001-row limit alone can still produce a very large response. Small.
|
||||
|
||||
**DL-4 — `FlowGraphBuilder` emits two edges between a write and the wait it satisfied `[Low][CONFIRMED]`** — `:150-152` + `:219-224`; `FlowGraphView` draws both, obscuring the dashed echo edge; the test at `FlowGraphBuilderTests.cs:90` asserts `Single(External)`, not `(From, To)` uniqueness. Fix: skip the External edge when the write is the chain predecessor. Small.
|
||||
**DL-4 — `FlowGraphBuilder` emits two edges between a write and the wait it satisfied `[Low][CONFIRMED]`** — `:150-152` + `:219-224`; `FlowGraphView` draws both, obscuring the dashed echo edge; the test at `FlowGraphBuilderTests.cs:90` asserts `Single(External)`, not `(From, To)` uniqueness. Fix: coalesce by endpoints with explicit priority for the dashed External echo edge, or omit the ordinary chain edge when it is replaced. Test both unique endpoints and preserved echo semantics. Small.
|
||||
|
||||
**DL-5 — Refresh merge violates the builder's input-order contract for late-arriving executions `[Low][CONFIRMED mechanism / PLAUSIBLE impact]`** — `FlowDebugger.razor.cs:707-714` (known-first, then fresh) vs the F5 rule (`FlowGraphBuilder.cs:84-117`); a trigger-cascaded child whose rows arrive by telemetry lands at the tail regardless of when it ran. Fix: base on the (complete) Replay order, append live-only rows. Small.
|
||||
|
||||
@@ -196,28 +432,51 @@ Seventeen page files mutate entities through the repositories and call `SaveChan
|
||||
|
||||
**DL-8 — Replay's root execution id is picked by timestamp order, which F5 declared untrustworthy `[Low][CONFIRMED]`** — `FlowDebugRunner.cs:552` (`events[0]`) though `ResolveChainAnchorAsync` (`AuditLogRepository.cs:945-1004`) already computes the true root. Fix: return `ChainAnchor.RootExecutionId`. Small.
|
||||
|
||||
**DL-9 — `Types/Debugging/` is absent from the Commons spec; `FlowTraceCollector` sits outside REQ-COM-6's carve-out `[Low][CONFIRMED]`** — `Component-Commons.md:171-208, 309-327`. Fix: doc update. Small.
|
||||
**DL-9 — Commons documentation omits debugging types and collector ownership `[Low][CONFIRMED documentation gap]`**
|
||||
`docs/requirements/Component-Commons.md:171-208` omits `Types/Debugging/`. Its REQ-COM-6
|
||||
`:319-325` already permits self-contained helpers with owned state, so `FlowTraceCollector`
|
||||
being mutable does not by itself violate the architecture. It collects bounded in-memory data
|
||||
and accepts a redactor abstraction. **Fix:** document the namespace and per-run collector
|
||||
ownership/redaction contract; no relocation is justified merely by stateful fields. Small.
|
||||
|
||||
**DL-10 — No enum lock-in tests for `FlowLane`/`FlowStepKind`/`FlowStepOutcome` `[Low][CONFIRMED]`** — `AuditEnumTests` covers only the four audit enums; `KindRank`'s display-order contract is unguarded. Fix: member-set assertions. Small.
|
||||
**DL-10 — No enum lock-in tests for `FlowLane`/`FlowStepKind`/`FlowStepOutcome` `[Low][CONFIRMED]`** — `AuditEnumTests` covers only the four audit enums; `KindRank`'s display-order contract is unguarded. Fix: member/value contract assertions where serialization requires them, plus a separate `KindRank` display-order test. Existing functional tests already exercise these enums. Small.
|
||||
|
||||
**DL-11 — Folded outbound steps retain the first exception detail instead of the final one `[Low][CONFIRMED source path]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging/FlowGraphBuilder.cs:424-440` creates a folded
|
||||
step from the first lifecycle row and replaces latest Outcome/ErrorMessage but omits ErrorDetail.
|
||||
`FlowStepProjector.cs:46` does project each row's detail; lifecycle rows (:82-93) do not retain it.
|
||||
A submit with null detail followed by a failed terminal event displays no terminal stack, while an
|
||||
earlier failure's detail can remain after later success. **Fix:** carry latest ErrorDetail with
|
||||
latest ErrorMessage, with explicit lifecycle detail semantics if needed. Test null-submit→failure
|
||||
and failure→success. Small.
|
||||
|
||||
### 2.7 DataConnectionLayer / CLI / DelmiaNotifier / docker scripts (DC)
|
||||
|
||||
**DC-1 — Nothing in production ever sends `UnsubscribeTagsRequest` / `UnsubscribeAlarmsRequest`; the DCL leaks subscriptions for the life of the node `[High][CONFIRMED, orch-verified]`**
|
||||
Handlers `Actors/DataConnectionActor.cs:1799, 3138`; routing `DataConnectionManagerActor.cs:153, 178`; a repo-wide grep finds senders only in tests — `InstanceActor.cs` and `NativeAlarmActor.cs` contain zero references, and `DataConnectionActor` has no `Context.Watch` at all. The connection actor is never stopped on redeploy (`HandleReconfigureConnection` :118 forwards; `HandleCreateConnection` :75 short-circuits on an existing name). Disable, delete, or redeploy an instance with a narrower attribute set and: the removed tags stay advised on the device (OPC UA monitored items / MxAccess advises never released), `ReSubscribeAll` re-issues them on every reconnect forever, `TotalSubscribedTagCount`/`ResolvedTagCount` never drop, `_subscribers[instanceName]` keeps a dead `IActorRef` that receives a `TagValueUpdate` per value change (dead-letter flood at tag rate), and `_alarmSourceSubscribers` never empties. On a churny site this is unbounded device-side subscription growth. `Component-DataConnectionLayer.md:447, 455` describe the unsubscribe as a live interaction. **Fix:** send both unsubscribes from `InstanceActor.PostStop` / `NativeAlarmActor.PostStop` and on undeploy/disable; belt-and-braces `Context.Watch` each subscriber in `HandleSubscribe`/`HandleSubscribeAlarms` and treat `Terminated` as an unsubscribe. Test: stop an `InstanceActor` and expect an `UnsubscribeTagsRequest` at the DCL probe. Medium.
|
||||
Handlers `Actors/DataConnectionActor.cs:1799, 3138`; routing `DataConnectionManagerActor.cs:153, 178`; a repo-wide grep finds senders only in tests — `InstanceActor.cs` and `NativeAlarmActor.cs` contain zero references, and `DataConnectionActor` has no `Context.Watch` at all. The connection actor is never stopped on redeploy (`HandleReconfigureConnection` :118 forwards; `HandleCreateConnection` :75 short-circuits on an existing name). Disable, delete, or redeploy an instance with a narrower attribute set and: the removed tags stay advised on the device (OPC UA monitored items / MxAccess advises never released), `ReSubscribeAll` re-issues them on every reconnect forever, `TotalSubscribedTagCount`/`ResolvedTagCount` never drop, `_subscribers[instanceName]` keeps a dead `IActorRef` that receives a `TagValueUpdate` per value change (dead-letter flood at tag rate), and `_alarmSourceSubscribers` never empties. On a churny site this is unbounded device-side subscription growth. `Component-DataConnectionLayer.md:447, 455` describe the unsubscribe as a live interaction. **Fix:** owner-aware cleanup on stop/undeploy plus DCL DeathWatch. Fence tag removal by actor identity/generation: the current handler at `DataConnectionActor.cs:1804` keys only by instance name, so a late old actor unsubscribe must not remove its same-name replacement. Preserve actual subscriber identity on alarm cleanup too. Test stop cleanup and old-owner cleanup arriving after replacement subscription. Medium.
|
||||
|
||||
**DC-2 — Releasing a "redundant" MxGateway handle tears down the LIVE subscription for that tag `[High][CONFIRMED, orch-verified code path]`**
|
||||
`DataConnectionActor.cs:1642-1653` + `:1703` (`_ = UnsubscribeIdsAsync(msg.Adapter, redundantIds)`), same in `:2500, 2509`. On MxGateway the "subscription id" **is** the MXAccess item handle (`RealMxGatewayClient.cs:193`, `reply.ItemHandle.ToString(...)`), per tag, so the redundant id is normally the live handle — and both removals are identity-blind anyway (`MxGatewayDataConnection.cs:294-304` `_tagToSub.TryRemove(s.TagPath)`; `RealMxGatewayClient.cs:219` `_tagToHandle.TryRemove(tag)`), deleting the mapping the event loop routes on. The trigger is ordinary: `HandleSubscribe`'s dedup partition (:1151) checks `_subscriptionIds` and `_subscribesInFlight` but not `_resolutionInFlight`, so a second instance subscribing to an unresolved tag while its resolution probe (:2277) is in flight double-subscribes. Result: the tag silently stops updating — the attribute freezes at its last Good value, no quality change, nothing in the health report — until the next reconnect. Same class as the QA Z28061 frozen-value incident. **Fix:** (a) include `_resolutionInFlight` in the partition; (b) identity-checked removals (`if (cur == id)`); (c) skip the release when `result.SubscriptionId == _subscriptionIds[tag]`. Test: subscribe one tag twice against the Mx adapter, release the "redundant" id, assert the tag still routes. Small.
|
||||
`DataConnectionActor.cs:1642-1653` + `:1703` (`_ = UnsubscribeIdsAsync(msg.Adapter, redundantIds)`), same in `:2500, 2509`. On MxGateway the "subscription id" **is** the MXAccess item handle (`RealMxGatewayClient.cs:193`, `reply.ItemHandle.ToString(...)`), per tag, so the redundant id is normally the live handle — and both removals are identity-blind anyway (`MxGatewayDataConnection.cs:294-304` `_tagToSub.TryRemove(s.TagPath)`; `RealMxGatewayClient.cs:219` `_tagToHandle.TryRemove(tag)`), deleting the mapping the event loop routes on. The trigger is ordinary: `HandleSubscribe`'s dedup partition (:1151) checks `_subscriptionIds` and `_subscribesInFlight` but not `_resolutionInFlight`, so a second instance subscribing to an unresolved tag while its resolution probe (:2277) is in flight double-subscribes. Result: the tag silently stops updating — the attribute freezes at its last Good value, no quality change, nothing in the health report — until the next reconnect. Same class as the QA Z28061 frozen-value incident. **Fix:** (a) include `_resolutionInFlight` in the partition; (b) identity-checked removals (`if (cur == id)`); (c) skip the release when `result.SubscriptionId == _subscriptionIds[tag]`. Test the DCL duplicate-completion path: it must never issue an unsubscribe for the current live handle; separately verify atomic identity-checked reverse-map removal for distinct old/new handles. A direct unsubscribe of the live handle is supposed to remove it. Same-handle behavior was source-traced, not reproduced against the live gateway. Small.
|
||||
|
||||
**DC-3 — Device writes are stashed across a reconnect and applied minutes later, contradicting the "no buffered setpoints" invariant `[High][CONFIRMED, orch-verified]`**
|
||||
`DataConnectionActor.cs:417-422` (Connecting) and `:793-796` (Reconnecting) stash `WriteTagRequest`/`WriteTagBatchRequest`; `:494` `UnstashAll` on `BecomeConnected`. `Component-DataConnectionLayer.md:421`: "There is **no store-and-forward for device writes** … buffering stale setpoints would be dangerous." The script's 30 s Ask (`InstanceActor.cs:787`) times out and the script is told the write failed; the reply dead-letters; the value is still written to the device when the link returns. A 10-minute outage ends with a burst of stale setpoints hitting equipment whose scripts may already have compensated. Secondarily the stash is unbounded (no `stash-capacity` configured) and Lever 1's `SubscribeTagsRequest` re-sends (every ≤ 60 s during the outage) are all stashed and replayed, each a full `SeedTagsAsync`. **Fix:** answer writes in Connecting/Reconnecting with an immediate failed `WriteTagResponse` (what the doc describes and the script already handles), or drop stashed writes older than `WriteTimeout` at unstash; dedupe stashed subscribes per instance / set `stash-capacity`. Small.
|
||||
`DataConnectionActor.cs:417-422` (Connecting) and `:793-796` (Reconnecting) stash `WriteTagRequest`/`WriteTagBatchRequest`; `:494` `UnstashAll` on `BecomeConnected`. `Component-DataConnectionLayer.md:421`: "There is **no store-and-forward for device writes** … buffering stale setpoints would be dangerous." The script's 30 s Ask (`InstanceActor.cs:787`) times out and the script is told the write failed; the reply dead-letters; the value is still written to the device when the link returns. A 10-minute outage ends with a burst of stale setpoints hitting equipment whose scripts may already have compensated. Secondarily the stash is unbounded (no `stash-capacity` configured) and Lever 1's `SubscribeTagsRequest` re-sends (every ≤ 60 s during the outage) are all stashed and replayed, each a full `SeedTagsAsync`. **Fix:** answer writes in Connecting/Reconnecting with an immediate failed response appropriate to single/batch requests (what the no-buffering contract requires); dedupe stashed subscribes per instance / set `stash-capacity`. Small.
|
||||
|
||||
**DC-4 — OPC UA silently downgrades to an unsecured endpoint when the requested `SecurityMode` is unavailable or discovery fails `[Medium][CONFIRMED]`** — `RealOpcUaClient.cs:151-158` (`?? endpoints.FirstOrDefault()`), `:386-392` (catch → `new EndpointDescription(url)`, mode None); no post-connect assertion. A `SignAndEncrypt` connection connects unencrypted and reports green; Verify has the same fallback. Fix: fail the connect when `preferred != None` and no match / discovery threw, naming requested vs available. Small.
|
||||
|
||||
**DC-5 — A new MxGateway alarm source covered by the live stream prefix never receives its snapshot `[Medium][CONFIRMED]`** — `MxGatewayDataConnection.cs:319-322` short-circuits when `AlarmFilterPrefix.Covers(prefix, source)`; `Covers` is true for an empty prefix (`AlarmFilterPrefix.cs:57-58`) and `LongestCommonPrefix` collapses to `""` on the first differing character, so every later source takes the no-restart path — the only path without a snapshot. `NativeAlarmActor` populates `_alarms` only on `SnapshotComplete` (:232-237), so standing alarms on a newly deployed instance are invisible until the next reconnect (OPC UA is unaffected: `TriggerConditionRefreshAsync` per handle). Fix: drop the `Covers` short-circuit (the prefix is documented as a bandwidth optimisation only). Small.
|
||||
**DC-5 — New alarm subscribers can miss standing-state snapshots on both adapters `[Medium][CONFIRMED, expanded]`**
|
||||
`DataConnectionLayer/Adapters/MxGatewayDataConnection.cs:319-322` skips a fresh source snapshot
|
||||
when the existing prefix covers it. Independently, `Actors/DataConnectionActor.cs:2881-2886`
|
||||
returns Success to a new subscriber of an already active source without replaying its state;
|
||||
that path affects OPC UA too. A newly deployed instance can remain blind to standing conditions
|
||||
until a later transition/reconnect. The original “OPC UA unaffected” claim was incorrect.
|
||||
**Fix:** deliver an initial snapshot to each subscriber, using a DCL state replay or a correctly
|
||||
scoped refresh; handle the Mx prefix optimization separately. Fix DC-12's marker scoping before
|
||||
increasing refresh traffic. Test distinct covered sources and a second instance sharing one
|
||||
source on each adapter. Medium.
|
||||
|
||||
**DC-6 — `deploy site` (and every bulk command) exits 0 when every instance fails `[Medium][CONFIRMED]`** — `CLI/Commands/CommandHelpers.cs:169-193` returns 0 for any 2xx; `DeployCommands.cs:68-84`. A CI `scadabridge deploy site … || rollback` never fires; same for `bundle import` and `flow-debug run-*`. Fix: an `onSuccess` handler inspecting `failureCount` → exit 1; document. Small.
|
||||
**DC-6 — `deploy site` exits 0 when its domain result reports failed instances `[Medium][CONFIRMED]`** — `CLI/Commands/CommandHelpers.cs:169-193` returns 0 for any 2xx; `DeployCommands.cs:68-84`. A CI `scadabridge deploy site … || rollback` never fires; other command-specific success payloads need separate contract checks; do not assume every JSON command has the same failure shape. Fix: an `onSuccess` handler inspecting `failureCount` → exit 1; document. Small.
|
||||
|
||||
**DC-7 — DelmiaNotifier treats a standby central's 503 as final and never fails over `[Medium][CONFIRMED]`** — `DelmiaNotifier/Notifier.cs:31-52` rolls over only on `ConnectFailed`. Its HA therefore depends entirely on `StandbyForwardPeerEndpoints` being configured on both centrals — which QA (`vq03`/`vq04`) and prod (`vd03`) still lack (CLAUDE.md). Fix: fail over on 503 (it is "this node cannot serve you", not a business rejection), or add the prerequisite to the README + rollout record. Small.
|
||||
**DC-7 — DelmiaNotifier treats a standby central's 503 as final and never fails over `[Medium][CONFIRMED]`** — `DelmiaNotifier/Notifier.cs:31-52` rolls over only on `ConnectFailed`. Its HA therefore depends entirely on `StandbyForwardPeerEndpoints` being configured on both centrals — which QA (`vq03`/`vq04`) and prod (`vd03`) still lack (CLAUDE.md). Fix: distinguish a known pre-execution standby rejection from an ambiguous post-send failure before retrying; generic retry-on-any-503 can duplicate effects. Document and test the notifier HA contract. QA/prod peer settings quoted here were not remotely rechecked in this pass. Small.
|
||||
|
||||
**DC-8 — `secured-write` is a shipping CLI group absent from the README and from `CommandTreeTests` `[Low][CONFIRMED]`** — `CLI/Program.cs:42`; `CommandTreeTests.cs:26-45, 63-71` asserts 18 groups vs 19 in `Program.cs` and builds `data-connection` without `ConnectionBrowseCommands.AddTo`, so browse/search/verify-endpoint/certs are uncovered too. Fix: add to the test (19), `AddTo` in the test build, README section. Small.
|
||||
|
||||
@@ -225,11 +484,23 @@ Handlers `Actors/DataConnectionActor.cs:1799, 3138`; routing `DataConnectionMana
|
||||
|
||||
**DC-10 — Per-item shard planning allocates a list per monitored item on the subscribe hot path `[Low][CONFIRMED]`** — `RealOpcUaClient.cs:765-772` from the loop at `:686`; ~75 k short-lived objects on a 37.5 k-tag reconnect. Fix: hoist / plan per chunk. Small.
|
||||
|
||||
**DC-11 — DelmiaNotifier aborts the whole failover loop on a malformed `BaseUrls` entry `[Low][CONFIRMED]`** — `HttpRecipeSender.cs:20-45` catches only `HttpRequestException`/`OperationCanceledException`; a `UriFormatException` on the first URL escapes to `Program`'s outer catch. Fix: map any exception to `ConnectFailed`, or `Uri.TryCreate` in `ConfigLoader.SplitBaseUrls`. Small.
|
||||
**DC-11 — DelmiaNotifier aborts the whole failover loop on a malformed `BaseUrls` entry `[Low][CONFIRMED]`** — `HttpRecipeSender.cs:20-45` catches only `HttpRequestException`/`OperationCanceledException`; a `UriFormatException` on the first URL escapes to `Program`'s outer catch. Fix: validate every BaseUrls entry as an absolute HTTP(S) URI during config load, with a clear diagnostic or explicit invalid-endpoint skip policy. Do not map arbitrary exceptions/cancellation to connect failure. Small.
|
||||
|
||||
**DC-12 — OPC UA per-feed snapshot completion clears unrelated active alarms `[High][CONFIRMED source path, orch-verified; live multi-feed gate pending]`**
|
||||
`src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Adapters/RealOpcUaClient.cs:867-909` registers
|
||||
per-source monitored callbacks. `HandleAlarmEvent:1165-1168` emits an unscoped SnapshotComplete
|
||||
for each feed's RefreshEnd. `Actors/DataConnectionActor.cs:3021-3027` broadcasts each marker to
|
||||
all alarm subscribers. `SiteRuntime/Actors/NativeAlarmActor.cs:251-282` swaps/clears its snapshot
|
||||
buffer on every marker. After A commits its snapshot, B's subsequent completion reaches A with
|
||||
an empty buffer, generating false return-to-normal/removal of still-active conditions. Multiple
|
||||
feeds are ordinary configuration; the scope is lost inside the application. **Fix:** preserve
|
||||
feed/source identity and refresh epoch on snapshot boundaries, keeping Mx connection-wide stream
|
||||
semantics separate. Test interleaved A/B snapshot/end delivery and assert completing B cannot
|
||||
change A, then validate two real OPC UA alarm sources on the rig. Medium.
|
||||
|
||||
### 2.8 Cross-cutting (XC) and orchestrator (ORCH)
|
||||
|
||||
**XC-1 — Central's entire HTTP endpoint graph is served on the site-facing h2c gRPC listener, bypassing Traefik `[Medium][CONFIRMED, orch-verified]`** — `Host/Program.cs:228-262` binds the HTTP ports (`Http1AndHttp2`) and `centralGrpcPort` (`Http2`) on one `WebApplication`; endpoint routing is not listener-scoped and there is no `RequireHost`/`MapWhen` anywhere in `src/`. Every endpoint mapped at `:711-748` — Central UI, `/management`, `/api/*`, `/api/audit`, `/metrics`, `/health/*`, the SignalR hub — is also served on `:8083`, the port every site dials and the rig publishes (`docker-compose.yml:131, 198`). `curl --http2-prior-knowledge http://central-a:8083/metrics` reaches metrics and health **anonymously**, and the authenticated surfaces with credentials, on a port not behind Traefik — so the `/health/active` active-node routing is bypassed. Register row 56 deliberately made central `/metrics` opt-in on a separate port for this reason. Fix: `.RequireHost($"*:{httpPort}")` on the HTTP-only groups, or an endpoint filter that 404s non-control-plane paths on `centralGrpcPort`; pin with a Host.Tests case. Small.
|
||||
**XC-1 — Central's entire HTTP endpoint graph is served on the site-facing h2c gRPC listener, bypassing Traefik `[Medium][CONFIRMED, orch-verified]`** — `Host/Program.cs:228-262` binds the HTTP ports (`Http1AndHttp2`) and `centralGrpcPort` (`Http2`) on one `WebApplication`; endpoint routing is not listener-scoped and there is no `RequireHost`/`MapWhen` anywhere in `src/`. Every endpoint mapped at `:711-748` — Central UI, `/management`, `/api/*`, `/api/audit`, `/metrics`, `/health/*`, the SignalR hub — is also served on `:8083`, the port every site dials and the rig publishes (`docker-compose.yml:131, 198`). `curl --http2-prior-knowledge http://central-a:8083/metrics` reaches metrics and health **anonymously**, and the authenticated surfaces with credentials, on a port not behind Traefik — so the `/health/active` active-node routing is bypassed. Register row 56 deliberately made central `/metrics` opt-in on a separate port for this reason. Fix: enforce listener ownership using the actual `HttpContext.Connection.LocalPort` or separate pipelines/listeners, rejecting non-control-plane endpoints before dispatch. `RequireHost` matches caller-supplied Host/authority and is not a port-isolation boundary. Test an h2c call to the gRPC socket with authority set to the HTTP port, and the inverse; preserve intentional metrics/health access. The original curl example is a reproduction recipe, not a live probe run in this pass. Small.
|
||||
|
||||
**XC-2 — Four SMTP keys in all four rig central appsettings bind to nothing `[Medium][CONFIRMED, orch-verified]`** — `docker/central-node-*/appsettings.Central.json:84-89` and env2 set `Notification:{SmtpServer, SmtpPort, AuthMode, FromAddress}`; `NotificationOptions` (`NotificationService/NotificationOptions.cs:13-25`) has only `ConnectionTimeoutSeconds` and `MaxConcurrentConnections`; zero code hits. Real SMTP config is the DB `SmtpConfiguration` row. An operator editing the host/port here changes nothing, silently. Fix: delete the keys, leave a `_comment` pointing at `notification smtp` / `/notifications/smtp`. Small.
|
||||
|
||||
@@ -239,7 +510,10 @@ Handlers `Actors/DataConnectionActor.cs:1799, 3138`; routing `DataConnectionMana
|
||||
|
||||
**XC-5 — README, HighLevelReqs, Component-ManagementService and the components skill still describe ClusterClient as the live transport `[Medium][CONFIRMED]`** — `README.md:83, 96`; `HighLevelReqs.md:49-51`; `Component-ManagementService.md:5, 16, 21, 48, 50, 301, 307` (a whole "ClusterClientReceptionist Registration" section, contradicted by line 27 of the same file); `Component-NotificationOutbox.md:31`; `Component-Commons.md:292`; `.claude/skills/scadabridge-components/SKILL.md` entries 5 and 18. These are the entry-point documents an agent loads first. Fix: rewrite to gRPC both directions + `SiteStreamService` + token-gated HTTP config fetch; delete the receptionist section. Small.
|
||||
|
||||
**XC-6 — `SiteStreamService.IngestAuditEvents` / `IngestCachedTelemetry` are dead RPCs kept on the site's authenticated surface `[Medium][CONFIRMED]`** — the RPC-inventory view of CM-5 (`sitestream.proto:14-15`; `SiteStreamGrpcServer.cs:539, 595`); no client outside `Host.Tests`. Fix as CM-5. Small.
|
||||
**XC-6 — Dead site-hosted ingest RPCs `[MERGED into CM-5 — not separately counted]`**
|
||||
This was the same pair of handlers already recorded under CM-5, assigned a different severity by
|
||||
a second reviewer. Retain CM-5 as the single Low-priority implementation finding. Preserve proto
|
||||
compatibility when retiring handlers; return `Unimplemented` rather than silently acknowledging.
|
||||
|
||||
**XC-7 — Migration-only legacy keys still shipped in every rig site appsettings `[Low][CONFIRMED]`** — `ScadaBridge:Database:SiteDbPath` and `ScadaBridge:StoreAndForward:SqliteDbPath` in all 8 site files (both rigs), which CLAUDE.md says to delete once migrated; the other two named keys are already gone. Fix: delete. Small.
|
||||
|
||||
@@ -249,17 +523,17 @@ Handlers `Actors/DataConnectionActor.cs:1799, 3138`; routing `DataConnectionMana
|
||||
|
||||
**XC-10 — `OpcUaGlobalOptions` / `MxGatewayGlobalOptions` are the only bound options with neither `ValidateOnStart` nor a validator `[Low][CONFIRMED]`** — `DataConnectionLayer/ServiceCollectionExtensions.cs:32-36`; every sibling chains both (arch-review 08 §1.5). A bad timeout surfaces as a hang at first connect, not at boot; masked today because neither section is set on either rig. Fix: validators + `ValidateOnStart`. Small.
|
||||
|
||||
**XC-11 — Umbrella index is two features behind, and `main` is 46 commits unpushed `[Low][CONFIRMED]`** — `scadaproj/CLAUDE.md:41` pins `main @ 789377ef` (2026-09-06); local `main` = `d8229916`; the instance picker and v1.4 step logs are unrecorded; `origin/main..HEAD` = 46. Fix: push; update the row (short, per the index's own rule). Small.
|
||||
**XC-11 — Umbrella index is two features behind, and `main` is 47 commits ahead of the local origin/main ref `[Low][CONFIRMED]`** — `scadaproj/CLAUDE.md:41` pins `main @ 789377ef` (2026-09-06); local `main` = `c078bc8f`; the instance picker and v1.4 step logs are unrecorded; `origin/main..HEAD` = 47 against the locally cached ref (no remote fetch in this review). Fix during a separately authorized integration/push: update the row (short, per the index's own rule). Small.
|
||||
|
||||
**XC-12 — `RequiredSingletonsHealthCheck`'s doc miscounts the central singletons `[Low][CONFIRMED]`** — `Host/Health/RequiredSingletonsHealthCheck.cs:32-38` says "all five … unconditional"; there are seven (`AkkaHostedService.cs:518, 551, 619, 664, 687, 718, 744`), two excluded as best-effort — a reason the comment's two-branch rule would get wrong. Fix: amend the remark. Small.
|
||||
|
||||
**ORCH-1 — `OperationTrackingStore.Dispose()` disposes the write connection under a live pump transaction `[Medium][CONFIRMED, observed in the run]`** — `SiteRuntime/Tracking/OperationTrackingStore.cs:616-634` completes the channel then `_writeConnection.Dispose()` immediately, while the `Task.Run(PumpAsync)` pump may be inside `using var tx = _writeConnection.BeginTransaction()` (:388, :407). `SqliteConnection.Dispose` rolls back the live transaction on the disposing thread while the pump concurrently completes it → `NullReferenceException` inside `SqliteTransaction.Complete()`. Reproduced in this review's suite run: `OperationTrackingStoreTests.SR024_SyncDispose_DoesNotDeadlock_WhenInvokedFromFreshThread` failed with exactly that stack (§7). Production disposes the host singleton through `DisposeAsync` (which drains), so this is a latent hazard on the sync path plus a flaky test, not an outage. Fix: on the sync path, complete the writer and wait for the pump with a short bounded timeout (not the gate) before disposing the connection, or take the write gate synchronously with a timeout; pin with the existing test. Small.
|
||||
**ORCH-1 — `OperationTrackingStore.Dispose()` disposes the write connection under a live pump transaction `[Medium][CONFIRMED, observed in the run]`** — `SiteRuntime/Tracking/OperationTrackingStore.cs:616-634` completes the channel then `_writeConnection.Dispose()` immediately, while the `Task.Run(PumpAsync)` pump may be inside `using var tx = _writeConnection.BeginTransaction()` (:388, :407). `SqliteConnection.Dispose` rolls back the live transaction on the disposing thread while the pump concurrently completes it → `NullReferenceException` inside `SqliteTransaction.Complete()`. Recorded in the **first-pass** suite run: `OperationTrackingStoreTests.SR024_SyncDispose_DoesNotDeadlock_WhenInvokedFromFreshThread` failed with exactly that stack (§7). Production disposes the host singleton through `DisposeAsync` (which drains), so this is a latent hazard on the sync path plus a flaky test, not an outage. Fix: complete/cancel and join the pump before disposing its resources. If a bounded wait expires, defer disposal until completion; never close the connection/gate beneath a live pump. Pin the race with a controlled pump barrier and include sync-context safety; a passing timing-sensitive test does not disprove the source race. Small.
|
||||
|
||||
---
|
||||
|
||||
## 3. Verified correct
|
||||
## 3. Verified strengths and retained first-pass inventories
|
||||
|
||||
The reviewers recorded what they checked and found holding; the significant ones, by area:
|
||||
These inventories originate in the first pass. Production source has not changed, and the second pass rechecked the implicated boundaries plus the dependency graph. This is not a second exhaustive rerun of every historical inventory check. Qualifications below and the current findings take precedence:
|
||||
|
||||
- **Site Runtime:** a stop does not cancel an in-flight run; deadlines armed at enqueue and shed at dequeue on every path; watchdog detach/replace bookkeeping (`ScriptExecutionScheduler.cs:275-379`); `MaxConcurrentRunsPerScript` counter cannot leak; **no awaited audit write on any script path** (eight SiteRuntime sites + the forwarder, all enqueue-and-observe on `TaskScheduler.Default`); `TraceCapturingLogger` double-wrap guard and the shared-script logger sharing; `TraceStepScope` restore; log caps + `FlushDropped` once + collector reserve arithmetic; Lever 1 in full; `OperationTrackingStore` read-your-writes barrier; `Notify.Send` `maxRetries: 0` + the `MaxRetries > 0` park test; purge pacing pure and faulted ticks don't arm catch-up; trigger eval off-pool with coalescing.
|
||||
- **Communication / Host:** every mapped gRPC service is behind a fail-closed `FixedTimeEquals` interceptor (`CentralControlAuthInterceptor`, `SecretsHubAuthInterceptor` via the shared `UsesGrpcHub` predicate, `ControlPlaneAuthInterceptor` on stream + command, `LocalDbSyncAuthInterceptor`); proto diff vs `ee193cd2` is additive-only with vendored C# in sync and `regen-proto.sh` restoring the csproj on every exit path; batching negotiation via the proto3 default; `ActiveNodeEvaluator` is the sole active definition (`State.Leader` appears only in two "never use" comments); `BuildHocon` emits `AutoDowning`, `min-nr-of-members = 1` pinned, empty seed list under the guard; seed self-first enforced and true in all 14 appsettings; channel park-then-sweep, `(site, endpoint)` factory key, `TryGet` on teardown; reconnect on OK completion wired through both consumers; single-endpoint sites collapse correctly; bounded DropOldest channels; options validated on start everywhere but XC-10; correlation-id path-element guard; Traefik probes `/health/active`.
|
||||
@@ -280,7 +554,7 @@ The reviewers recorded what they checked and found holding; the significant ones
|
||||
| `Component-StoreAndForward.md:79` | retry-exhausted notifications park | `maxRetries: 0`, park test `MaxRetries > 0`; contradicts line 52 | — |
|
||||
| `Component-StoreAndForward.md` sweep section | ordered `created_at ASC` drain | per-`(Category, Target)` lanes at `SweepTargetParallelism = 4`, lane abandoned on first transient failure — undocumented | — |
|
||||
| `Component-DataConnectionLayer.md:421` vs `:382/384` | no buffering of device writes / writes are stashed | stashed and replayed with no TTL | DC-3 |
|
||||
| `Component-DataConnectionLayer.md:447, 455, 358` | Site Runtime sends unsubscribes | nothing does | DC-1 |
|
||||
| `Component-DataConnectionLayer.md:447, 455, 358` | Site Runtime sends unsubscribes | no production sender | DC-1 |
|
||||
| `Component-DataConnectionLayer.md:412, 318, 323` | `volatile bool`; alarm requests stashed while Reconnecting; prefix restart costs bandwidth only | `int`+`Interlocked`; unsubscribe handled; also costs the snapshot | DC-9, DC-5 |
|
||||
| `Component-ScriptAnalysis.md:38` | reflection forbidden entirely | `RuntimeHelpers`/`Unsafe`/`AssemblyLoadContext` allowed | MG-1 |
|
||||
| `Component-ManagementService.md` | command catalog | missing all four `FlowDebug*` | MG-10 |
|
||||
@@ -288,40 +562,47 @@ The reviewers recorded what they checked and found holding; the significant ones
|
||||
| `Component-InboundAPI.md:366`; `CLAUDE.md` | relayed call audited once; peer response replayed unchanged | zero rows when no peer served; headers dropped | EI-1, EI-8 |
|
||||
| `Component-NotificationOutbox.md:112` | one duplicate on a rare failover | continuous duplication under dual-active | EI-9 |
|
||||
| `Component-CentralUI.md:89, 285, 311, 317` | data-connection pages Admin; audit pages Admin/Audit; LDAP mapping three roles | `RequireDesign`; `OperationalAudit` = {Admin, Viewer}; six roles | — |
|
||||
| `Component-CentralUI.md:311-315` | configuration audit shows before/after per change | UI writes no rows; only `AfterStateJson` exists | UI-1 |
|
||||
| `Component-CentralUI.md:311-315` | configuration audit shows before/after per change | many direct UI mutations lack rows; audited service paths exist; only `AfterStateJson` exists | UI-1 |
|
||||
| `Component-Security.md:54`; `SessionExpiry.razor:24-26` | idle session rejected; ping never keeps a session alive | ping slides the cookie every minute | UI-2 |
|
||||
| `Component-CentralUI.md` | — | Test Run / sandbox surface undocumented | — |
|
||||
| `Component-Commons.md:171-208, 309-327` | `Types/` subfolder list; pure helpers only | `Types/Debugging/` missing; `FlowTraceCollector` stateful | DL-9 |
|
||||
| `Component-Commons.md:171-208, 309-327` | `Types/` subfolder list; pure helpers only | `Types/Debugging/` missing; `FlowTraceCollector` per-run state needs documentation, already compatible with the owned-state carve-out | DL-9 |
|
||||
| `Component-ConfigurationDatabase.md` | — | encrypted vs plaintext secret columns never enumerated | DL-2 |
|
||||
| `Component-Transport.md:298, 306-315` | `UniqueName` identity key; FK rewiring list | not DB-enforced globally; `SiteId` rewrite omitted | DL-1, DL-6 |
|
||||
| `Component-Communication.md:114` | `IngestCachedTelemetry` live | dead | CM-5 |
|
||||
| `docker/*/appsettings*` | SMTP keys; 02:00 cron purge; legacy DB paths | none bound | XC-2, XC-3, XC-7 |
|
||||
| `RequiredSingletonsHealthCheck.cs:32-38`; `ScopeAccessors.cs:12-21`; `TriggerEvalGate.cs:25-26`; `StaleInstanceProbe.cs:28-33`; `FlowDebugRunner.cs:56-63`; `ManagementActor.cs:685-686`; `SitePairChannelProvider.cs:57-62`; `AkkaHostedService.cs:565-568`; `CommandTreeTests.cs:22-24` | code comments | each contradicted by the code beside it | XC-12, SR-1, SR-9, MG-3, MG-5, MG-9, CM-7, CM-5, DC-8 |
|
||||
| `scadaproj/CLAUDE.md:41` | `main @ 789377ef` | `d8229916`, two features later, 46 commits unpushed | XC-11 |
|
||||
| `scadaproj/CLAUDE.md:41` | `main @ 789377ef` | `c078bc8f`, two features later, 47 commits ahead of cached origin/main | XC-11 |
|
||||
|
||||
---
|
||||
|
||||
## 5. Recommended remediation program
|
||||
|
||||
Grouped so each work package is file-disjoint enough to run as a parallel subagent, in the shape
|
||||
of `2026-08-14-arch-review-remediation-plan.md`. Suggested order is by risk, not by count.
|
||||
Grouped in the shape of `2026-08-14-arch-review-remediation-plan.md`. Packages now overlap
|
||||
in shared wrappers, actors, and security paths; assign file ownership and sequence those changes
|
||||
before parallel implementation. Package numbers remain stable, but the second-pass order is risk-based: start WP1/WP2 and
|
||||
CM-9 (WP4), CM-10 (WP6), UI-13 (WP5) before lower-severity hygiene. Scope WP5 sandbox
|
||||
containment explicitly; do not implement its withdrawn release-on-timeout shortcut.
|
||||
|
||||
| WP | Findings | Theme | Effort |
|
||||
|---|---|---|---|
|
||||
| **WP1 — DCL subscription lifecycle** | DC-1, DC-2, DC-3, DC-5, SR-3, DC-4 | unsubscribe on stop/undeploy + `Context.Watch`; identity-checked handle release + `_resolutionInFlight` in the partition; fail writes while disconnected; snapshot on every new alarm source; port Lever 1 to `NativeAlarmActor`; no silent security downgrade | medium — needs a rig rebuild + the Lever-1-style live gate (subscribe count on a redeploy, frozen-tag probe) |
|
||||
| **WP2 — Script pool + trust** | SR-1, MG-1, SR-4, MG-8, SR-7/8/9/10 | `ConfigureAwait(false)` on the accessor paths; three deny-list entries; seal `AuditingDbConnection`; gate shared scripts at save | small — pure unit-testable |
|
||||
| **WP1 — DCL subscription lifecycle** | DC-1, DC-2, DC-3, DC-5, DC-12, SR-3, SR-5, SR-12, DC-4, DC-10 | owner-fenced unsubscribe + DeathWatch; source-scoped alarm snapshots + rehydration fence; async stop/flush ordering; profile shard-planning allocation; identity-checked handle release + `_resolutionInFlight` in the partition; fail writes while disconnected; snapshot on every new alarm source; port Lever 1 to `NativeAlarmActor`; no silent security downgrade | medium — needs a rig rebuild + the Lever-1-style live gate (subscribe count on a redeploy, frozen-tag probe) |
|
||||
| **WP2 — Script pool + trust** | SR-1, SR-2, MG-1, SR-4, SR-11, MG-8, SR-7, SR-8, SR-9, SR-10 | `ConfigureAwait(false)` on the accessor paths; three deny-list entries; close connection/transaction escape surfaces; wire alarm integration scope; gate shared scripts at save | small/medium; unit and integration tests |
|
||||
| **WP3 — gRPC sizing** | CM-1, CM-3, XC-4, XC-8 | explicit receive limits both roots + channels; byte budget on the audit drain/pull; batch-size ceilings; proto↔C# guard test; interceptor coverage test | medium |
|
||||
| **WP4 — Audit completeness + scope** | UI-1, EI-1, EI-2, EI-3, EI-5, MG-2, UI-8 | UI mutations through the management envelope (or the `IAuditService` stop-gap); relay audit-once fix; `SkipBodyCapture` + per-target redaction over `errorDetail`; caller IP; site scope on tree/Replay/relay | large (UI-1) + small ×6 |
|
||||
| **WP5 — Session + UI hygiene** | UI-2, UI-3, UI-4, UI-5, UI-6, UI-7, UI-10, UI-12, UI-9, UI-11 | non-sliding ping; dispatcher-marshalled merge; `PollGate` shape; sandbox gate release on timeout; failover role check; fallback policy + sweep test; login antiforgery | small ×8, medium ×2 |
|
||||
| **WP6 — Failover-safe apply** | MG-3, MG-4, EI-4, DL-1, DL-6, ORCH-1, EI-9 | clear the stale-probe memo on the active edge; batch-derived bulk-deploy deadline or submit-and-poll; relay timeout vs method timeout; global `UniqueName` index; block cross-site Overwrite; sync-dispose ordering; outbox doc/claim | small ×5, medium ×2, one migration |
|
||||
| **WP7 — Flow Debugger follow-ups** | DL-3, MG-7, MG-6, MG-5, DL-4, DL-5, DL-7, DL-8, DL-10, SR-6 | cap Replay + write-kind results; page the site pull (closes register row 117); root id from the anchor; edge dedupe; order-aware merge; parse once; enum tests | small ×10 |
|
||||
| **WP8 — Central listener + config hygiene** | XC-1, XC-2, XC-3, XC-7, CM-2, CM-6, XC-10, XC-9, CM-5/XC-6, CM-4, CM-7, CM-8, EI-6, EI-7, EI-8, DC-6, DC-7, DC-11 | `RequireHost` on the HTTP groups; delete inert keys; env2 bootstrap guard; delete dead RPCs; validators; SqlClient pin; CLI exit codes; notifier 503 failover | small ×18 — mostly one-liners plus a rig config sweep |
|
||||
| **WP9 — Docs sweep** | XC-5, XC-11, XC-12, MG-10, DC-8, DC-9, DL-9, SR-2 doc half, every row of §4 | ClusterClient purge from the entry-point docs; register rows for anything deferred (DL-2 if deferred, EI-9); umbrella index; push | small |
|
||||
| **WP4 — Audit completeness + scope** | UI-1, EI-1, EI-2, EI-3, EI-5, EI-11, MG-2, CM-9, UI-8 | UI mutations through the management envelope (or the `IAuditService` stop-gap); relay audit ownership; redact/suppress all sensitive error fields; trusted caller IP; bind PSK identity to payload ownership; site scope on tree/Replay/relay | large UI boundary work plus small/medium security fixes |
|
||||
| **WP5 — Session + UI hygiene** | UI-2, UI-13, UI-14, UI-4, UI-5, UI-6, UI-7, UI-10, UI-12, UI-9, UI-11 | real activity tracking; circuit grant revalidation; bounded output; observed poll cleanup; isolated/quarantined sandbox work; service auth; route sweep; explicit login antiforgery | mixed; sandbox isolation requires its own design |
|
||||
| **WP6 — Failover-safe apply** | MG-3, MG-4, CM-10, EI-4, SR-13, DL-1, DL-6, ORCH-1, EI-9, DL-2 | cross-node/shared revision invalidation; batch-derived bulk-deploy deadline or submit-and-poll; aligned deadlines + operation deduplication; recheck S&F gate per attempt; global `UniqueName` index; block cross-site Overwrite; sync-dispose ordering; outbox doc/claim; credential encryption migration or explicit DL-2 deferral | mixed; includes one migration and operation-idempotency work |
|
||||
| **WP7 — Flow Debugger follow-ups** | DL-3, MG-7, MG-6, MG-5, DL-4, DL-5, DL-7, DL-8, DL-10, DL-11, SR-6 | cap Replay + write-kind results; page the site pull (closes register row 117); root id from anchor; preserve echo edge; order-aware merge; final exception detail; parse once; enum tests | small changes across the listed paths |
|
||||
| **WP8 — Central listener + config hygiene** | XC-1, EI-10, XC-2, XC-3, XC-7, CM-2, CM-6, XC-10, XC-9, CM-5 (alias XC-6), CM-4, CM-7, CM-8, MG-9, EI-6, EI-7, EI-8, DC-6, DC-7, DC-11 | actual local-port enforcement + body cap before audit reads; delete inert keys; env2 bootstrap guard; retire dead handlers compatibly; validators; positive page-size clamp; SqlClient pin; CLI exit codes; notifier safe pre-dispatch failover | mostly small changes plus integration tests and a rig config sweep |
|
||||
| **WP9 — Docs sweep** | XC-5, XC-11, XC-12, MG-10, DC-8, DC-9, DL-9, SR-2 doc half, every row of §4 | ClusterClient purge from the entry-point docs; register rows for anything deferred (DL-2 if deferred, EI-9); umbrella index/integration status review | small |
|
||||
|
||||
Phase gate after WP1–WP3 and WP6: full solution build + test, `bash docker/deploy.sh`, then the
|
||||
existing live probes (`failover-drill.sh`, `verify-delmia-notifier.sh` A–J) plus two new ones — a
|
||||
redeploy that narrows an instance's attribute set must drop the device subscription count, and a
|
||||
write issued during a forced reconnect must be refused, not replayed.
|
||||
existing live probes (`failover-drill.sh`, `verify-delmia-notifier.sh` A–J) plus these required new regression gates: narrowed redeploy drops subscriptions without removing
|
||||
same-name replacement ownership; disconnected writes never replay; two OPC UA alarm feeds retain
|
||||
independent snapshots; cross-site payload with another site's credential yields no fetch token;
|
||||
response loss after write produces no second effect; revoked grants cannot act from an existing
|
||||
circuit; transaction-derived commands remain audited; chunked oversized input is rejected before
|
||||
handler/relay dispatch. These are remediation acceptance gates, **not tests claimed to have run
|
||||
against the current rig**.
|
||||
|
||||
---
|
||||
|
||||
@@ -399,7 +680,7 @@ Required keys not checked by `StartupValidator`: `LocalDb:Path` (delegated to `A
|
||||
|
||||
Open: 92, 94, 107, 109, 110, 112, 113, 115, 116, 117, 118 (114 never issued). None should be closed; every open premise re-verified. Row 112 should be re-worded (central now has `Truncated` + `LogsTruncated`; the site half is still unflagged). Row 117 confirmed and extended by MG-6. No dangling row citations in code. New rows to mint from this review if deferred: DL-2, EI-9, XC-3 (if not implemented).
|
||||
|
||||
### 6.F Test-suite shape
|
||||
### 6.F Test-suite shape (first-pass inventory; fresh execution in §7)
|
||||
|
||||
≈ 7 579 `[Fact]`/`[Theory]`/skippable attributes across 32 test projects, all in the slnx; zero static `Skip=`; runtime `Skip.If` concentrated in the MSSQL-fixture and Playwright suites (environment-conditional, not visible in a count); no retry/flaky markers; 56 assertion-less candidates all sampled to throwing helpers — no genuine no-op test found.
|
||||
|
||||
@@ -409,13 +690,134 @@ All twelve checked HOLD with a code anchor: active-node definition (`ActiveNodeE
|
||||
|
||||
---
|
||||
|
||||
## 7. Test evidence
|
||||
## 7. Verification evidence
|
||||
|
||||
`dotnet build ZB.MOM.WW.ScadaBridge.slnx` — **0 warnings, 0 errors**.
|
||||
`dotnet test … --filter "FullyQualifiedName!~PlaywrightTests"` — 29 assemblies, **9 231 passed, 1 failed**:
|
||||
### 7.A Fresh second-pass commands at `c078bc8f`
|
||||
|
||||
`SiteRuntime.Tests.Tracking.OperationTrackingStoreTests.SR024_SyncDispose_DoesNotDeadlock_WhenInvokedFromFreshThread`
|
||||
— `NullReferenceException` at `SqliteTransaction.Complete()` ← `SqliteConnection.Dispose` ←
|
||||
`OperationTrackingStore.Dispose()` (`OperationTrackingStore.cs:635`). This is ORCH-1, a real
|
||||
dispose-vs-pump race, not an environmental flake. The register-row-118 bUnit flake did not fire in
|
||||
this run. Playwright suites need the live rig and were excluded.
|
||||
All commands ran from the repository root on 2026-09-09. Build/test used existing restored
|
||||
assets; package listing queried the configured feeds. No production source, tests, package pins,
|
||||
or rig configuration were changed by this review.
|
||||
|
||||
| Check | Exact command | Result |
|
||||
|---|---|---|
|
||||
| Build | `dotnet build ZB.MOM.WW.ScadaBridge.slnx --no-restore` | exit 0; **0 warnings, 0 errors**, 34.02 s |
|
||||
| Selected solution tests | `dotnet test ZB.MOM.WW.ScadaBridge.slnx --no-build --no-restore --filter 'FullyQualifiedName!~PlaywrightTests' --logger trx --results-directory /tmp/scadabridge-review-tests` | exit 0; **9,232 passed, 0 failed, 0 skipped** in 29 nonempty assemblies; 30 TRX files include the empty filtered Playwright assembly |
|
||||
| Formatting | `dotnet format ZB.MOM.WW.ScadaBridge.slnx --verify-no-changes --no-restore --verbosity quiet` | exit 2; **1,290 WHITESPACE diagnostics across 60 files**; no formatter mutations |
|
||||
| Vulnerabilities | `dotnet package list --project ZB.MOM.WW.ScadaBridge.slnx --include-transitive --vulnerable --format json --no-restore` | exit 0; 58 projects inspected; one reported dependency: **AngleSharp 1.1.1, Moderate, test-only CentralUI.Tests** |
|
||||
| Deprecations | `dotnet package list --project ZB.MOM.WW.ScadaBridge.slnx --deprecated --format json --no-restore` | exit 0; **xunit 2.9.3 marked Legacy in 30 test projects**; feed suggests xunit.v3 |
|
||||
| Dependency graph | XML parsing of all source `.csproj` ProjectReference elements, DFS cycle check | **26 nodes, 91 edges, no cycles; Commons has zero project references** |
|
||||
| Report integrity | parse finding headings, verify active tally/unique IDs, `git diff --check`, inspect final diff | 92 active IDs; 13 High / 33 Medium / 46 Low; UI-3 and XC-6 excluded |
|
||||
|
||||
AngleSharp's [advisory](https://github.com/advisories/GHSA-pgww-w46g-26qg) is already explicitly
|
||||
suppressed only in `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests.csproj:64`,
|
||||
with compatibility rationale recorded in that file and `Directory.Packages.props`. This pass
|
||||
reports that accepted test-only exposure; it does not assert current patched bUnit compatibility
|
||||
or recommend silently overriding its dependency. xUnit deprecation is a migration-planning signal,
|
||||
not proof of a current runtime defect. No version changes were made.
|
||||
|
||||
The first pass recorded **9,231 passed / 1 failed**, with a NullReferenceException in
|
||||
`OperationTrackingStoreTests.SR024_SyncDispose_DoesNotDeadlock_WhenInvokedFromFreshThread`.
|
||||
That same test **passed in this fresh run**. ORCH-1 remains a source-confirmed race; this pass did
|
||||
not reproduce the historical stack and does not claim it is fixed. Passing tests also do not
|
||||
cover the newly identified unsafe paths unless explicitly noted below.
|
||||
|
||||
Raw evidence is local to this review session: `/tmp/scadabridge-review-build.log`,
|
||||
`/tmp/scadabridge-review-test.log`, `/tmp/scadabridge-review-tests/`,
|
||||
`/tmp/scadabridge-review-format.log`, `/tmp/scadabridge-review-vulnerable.json`, and
|
||||
`/tmp/scadabridge-review-deprecated.json`. The results and reproduction recipes below are retained
|
||||
in this document; temporary files are not durable repository artifacts.
|
||||
|
||||
### 7.B Isolated behavior probes
|
||||
|
||||
These probes used local compiled assemblies or real loopback Kestrel with synthetic data. They
|
||||
were created under `/tmp`, not added as regression tests. They neither touched live devices nor
|
||||
fetched production credentials.
|
||||
|
||||
| Finding | Executed probe / observation | Scope of proof |
|
||||
|---|---|---|
|
||||
| MG-1 | `ScriptTrustValidator.FindViolations` returned **0** for RuntimeHelpers.GetUninitializedObject, managed Unsafe.As, and new AssemblyLoadContext snippets | Real policy permits each referenced API; not a full sandbox escape exploit |
|
||||
| CM-1 | `AuditEventBatch.CalculateSize()` for 256 ErrorDetail-only rows of 65,536 ASCII bytes returned **16,779,264**; actual `AddGrpc` options resolve receive default **4,194,304** | Real protobuf size and framework default; no disk-growth or end-to-end drain experiment |
|
||||
| CM-9 | Real central auth interceptor with site-A metadata/key accepted reconciliation payload `SiteIdentifier=site-b`; real mapper preserved B | Auth/request binding gap, followed in source to token staging; no cross-site secret fetch executed |
|
||||
| CM-10 | Provider callback applied a synthetic effect, threw Unavailable, then succeeded on peer: **applications=2**, returned `ok` | Fault-injected retry path, not physical network/device duplication |
|
||||
| SR-11 | In-memory SQLite: normal wrapped write audit count **1**; `tx.Connection` was original provider, not wrapper; raw write affected **1** row with **0** audit emissions, persisted value 42 | Actual ADO escape and missing audit; reflection only instantiated internal wrapper in harness, not used by escape; not SQL Server/Roslyn end-to-end |
|
||||
| EI-10 | Real Kestrel read-before-cap pattern: desired cap 8, listener cap 1000, chunked body 100 → **HTTP 200, IsReadOnly=True, acceptedBytes=100** | Real framework middleware-order mechanism, not full app/Traefik deployment |
|
||||
|
||||
Probe sources/output: `/tmp/scadabridge-review-probes/Program.cs` and
|
||||
`/tmp/scadabridge-review-probes.log`; `/tmp/scadabridge-sr11-probe/Program.cs`;
|
||||
`/tmp/scadabridge-cap-probe/Program.cs` and `/tmp/scadabridge-cap-probe/output.txt`.
|
||||
Repeatable regression recipes are attached to each finding and the §5 acceptance gates.
|
||||
|
||||
## 8. Second-pass review decisions and architecture fitness
|
||||
|
||||
### 8.A Corrections that change remediation
|
||||
|
||||
| IDs | Decision |
|
||||
|---|---|
|
||||
| UI-3 | Withdraw: caller awaits retain renderer synchronization context; no demonstrated off-dispatcher entry |
|
||||
| XC-6 / CM-5 | One defect, retain CM-5 Low; XC-6 is a historical alias |
|
||||
| UI-1 | Many bypass paths in 16 files, not every UI edit; template/shared-script service paths already audit |
|
||||
| MG-3 | Use peer-originated DB edit/local-watermark mismatch as trigger; activation-only invalidation is insufficient |
|
||||
| UI-4/6/7 | Low hardening/prevention gaps; no continuing-circuit refresh outage or unauthorized handler exploit demonstrated |
|
||||
| UI-2 / UI-13 | Passive cookie renewal and stale circuit authority are separate failures; both need server-side handling |
|
||||
| UI-5 / UI-14 | Timeout is not termination or a memory bound; never recycle an admission slot while runaway work remains |
|
||||
| DC-1/2/5/12 | Cleanup must fence old owners and live handles; snapshots must cover every subscriber without broadcasting per-feed completion to unrelated sources |
|
||||
| SR-4 / SR-11 | Retargeting restrictions must cover transaction-derived connections; the class is already sealed |
|
||||
| EI-2/3/5/11 | Free-text messages may contain secrets; SQL name patterns do not redact values; caller-controlled forwarding markers are not identity proof |
|
||||
| EI-4 / CM-10 / DC-7 | Align deadlines but distinguish safe pre-dispatch rejection from an uncertain already-applied request |
|
||||
| ORCH-1 / EI-7 | A timeout cannot justify disposing resources still owned by unfinished work or admitting unlimited abandoned tasks |
|
||||
| MG-6 / DL-3/4/8/11 | Stable keysets + bytes + true root + echo-edge priority + latest detail are separate correctness requirements |
|
||||
| DL-9 | Document bounded owned state; statefulness alone is not a Commons architecture violation |
|
||||
| XC-1 | Use actual listener/local port, not spoofable Host/authority matching, for port isolation |
|
||||
|
||||
All previous SR/DC/MG/UI/DL/EI/CM/XC IDs and ORCH-1 were source-revisited or, for inventory/doc
|
||||
observations, checked against their current files. Historical production/QA statements, performance
|
||||
estimates, and migration/deferred-register inventories are not newly measured simply because
|
||||
source is unchanged. The original twelve design commitments describe implementation structure;
|
||||
they are not evidence that the script trust gate is a complete security sandbox or that per-site
|
||||
keys enforce payload ownership.
|
||||
|
||||
### 8.B CheckIt matrix
|
||||
|
||||
The intended design is a modular Akka hub-and-spoke system with a framework-light Commons core,
|
||||
not a framework-free application layer. Checks are interpreted against that declared architecture.
|
||||
A `pass` is limited to the cited check, not a blanket claim that the component is bug-free.
|
||||
|
||||
| Check | Status | Evidence / interpretation |
|
||||
|---|---|---|
|
||||
| CA-01 dependency direction | pass | Commons defines entities/ports, no project references; EF/UI/Host remain outside core; §6.A graph |
|
||||
| CA-02 framework independence | pass (Commons scope) | Commons directly references only shared Audit; no EF/ASP.NET/Akka core imports found; actor application components intentionally depend on Akka |
|
||||
| CA-03 ports/adapters | pass | External gateway/DB/device/notification ports and test doubles remain; SR-11 is an adapter escape defect, not absence of ports |
|
||||
| CA-04 boundary data shape | pass (structural) | Typed message/proto DTO mapping and additive contracts retained; CM-9 shows shape alone does not validate ownership |
|
||||
| CA-05 rule placement | fail | UI-1 bypasses audited application mutation boundaries and their guards |
|
||||
| CA-06 cycles | pass | Fresh DFS: 26 source projects / 91 edges / zero cycles |
|
||||
| CA-07 testability | pass | Runtime, management, adapter, and transport suites use probes/test doubles; selected solution tests run locally |
|
||||
| BP-01 separation of concerns | fail | UI-1 duplicates mutation/audit responsibility outside audited service path |
|
||||
| BP-02 explicit dependencies / lifetime | fail | MG-3 process-global memo, CM-8 mutable static seam, SR-2 omitted alarm dependencies, ORCH-1 resource lifetime race |
|
||||
| BP-03 repeatable self-checking tests | fail | Current run passes; ORCH-1 historical failure + CM-8 shared static test seam still lack deterministic guards |
|
||||
| BP-04 integration boundaries | pass | Separate Transport.IntegrationTests and host/communication boundary suites; new negative gates needed for reported defects |
|
||||
| BP-05 static-analysis health | pass (executed build diagnostics only) | Warnings-as-errors configured; build 0/0; no separate security/static-analysis engine run, format fails below |
|
||||
| BP-06 automated style | fail | No checked-in root .editorconfig or CI workflow found; formatter reports 1,290 diagnostics in 60 files |
|
||||
| BP-07 dependency-risk process | pass with documented exception | Fresh vulnerable/deprecated queries completed; known test-only AngleSharp suppression has rationale; xUnit Legacy recorded; XC-9 version skew remains |
|
||||
| BP-08 security baseline | fail | MG-1, MG-2, UI-1/2/13, SR-11, CM-9, EI-10/11 show concrete gaps despite existing auth/redaction controls |
|
||||
| BP-09 continuous architecture fitness | fail | Role/dispatch/compile-contract/captive-dependency tests exist; whole-graph, mapped-service-auth, proto-source sync and route-policy guards are incomplete; no checked-in CI workflow found |
|
||||
| LOG-01 organization logging baseline | fail | Host uses Serilog (`Program.cs:181,795`) and structured node/trace enrichment (`LoggerConfigurationFactory.cs:85-95`); actors also use Akka ILoggingAdapter (`CentralCommunicationActor.cs:27`). No application LogContext push/FromLogContext or request/user/operation scope setup found; only TraceCapturingLogger delegates BeginScope. Thus the stricter ILogger + Serilog + LogContext baseline is not fully implemented. Existing trace IDs are not absent, and this is not a new security defect count |
|
||||
|
||||
### 8.C Fitness functions to add
|
||||
|
||||
1. Actual mapped gRPC services must have an authentication gate; verified principal ownership must
|
||||
match every request/row site. Combine prefix coverage with adversarial metadata/payload tests.
|
||||
2. Source project graph must stay acyclic and Commons must not acquire outer-layer references;
|
||||
freeze deliberate exceptions, not incidental transitive package names.
|
||||
3. Generated descriptors must match proto messages, fields, types, oneofs, and service signatures.
|
||||
Field-number-only regex checking is a useful start but misses renamed/retyped/service drift.
|
||||
4. Every UI mutation goes through an audited application boundary and uses current grants. Test
|
||||
representative create/update/delete rollback and revocation on a live circuit; don't rely only
|
||||
on file-name/string sweeps or role attributes.
|
||||
5. Every bounded surface enforces its limit before allocation/read: audit serialized batches,
|
||||
Replay payloads, sandbox writers, and chunked HTTP bodies. Include single oversize rows.
|
||||
6. Lifecycle tests control the race: owner replacement/unsubscribe, interleaved source snapshots,
|
||||
late rehydration, demotion between deliveries, pump shutdown, and response loss after commit.
|
||||
|
||||
Add these to the repository's chosen CI system with existing build/test gates; keep formatting
|
||||
cleanup and framework/package migration in separate maintenance work so they do not obscure the
|
||||
correctness/security remediation.
|
||||
|
||||
Reference in New Issue
Block a user