Resolve all 33 medium-severity architecture review findings
Second slice of the 2026-09-09 whole-system review remediation
(docs/plans/2026-09-09-arch-review.md §10), on top of the High slice 146a1323.
Runtime/devices: SR-2 alarm on-trigger integration scope; SR-3 correlated NativeAlarmActor
subscribe retry; SR-4 immutable audited DB target; SR-5 ordered InstanceActor stop/flush;
SR-12 rehydration fence; SR-13 S&F gate recheck per delivery; DC-4 no OPC UA security
downgrade; DC-5 alarm snapshot for every new subscriber; ORCH-1 sync-dispose joins the pump.
Management/UI: MG-2 site scope on audit tree + Flow Debugger Replay; MG-3 stale-probe memo
removed; MG-4 server-owned bulk-deploy deadline; UI-5 sandbox runs in a killable child
process (new CentralUI.SandboxWorker project); UI-14 bounded sandbox output/IPC.
Edge/audit: EI-1 relay audit ownership; EI-2 SkipBodyCapture suppresses error text; EI-3/EI-11
error-field redaction + caps; EI-4 single relay deadline, pre-execution-only retry; EI-5
authenticated relay provenance; EI-10 body cap before audit read.
Persistence/contracts: DL-1 global Instance.UniqueName unique index + conflict translation;
DL-2 DataConnection protocol config encrypted at rest (migration
20260909203046_EnforceGlobalInstanceNamesAndProtectProtocolConfiguration — coordinated
central upgrade, old binaries cannot read protected rows); DL-3 bounded Replay paging;
DL-6 cross-site Overwrite of a deployed instance rejected; XC-4 proto descriptor contract test.
Config/hygiene: XC-1 central listener isolation by local port; XC-2/XC-3 inert SMTP + cron
keys removed from rig settings; CM-2 env2 bootstrap guard enabled; DC-6 deploy site exit code;
DC-7 notifier retries only explicit standby rejection; XC-5 ClusterClient purged from docs.
Gates recorded in the report: build 0/0, 9 397 passed / 0 failed, rig redeployed on all
eight nodes with the migration applied. 46 Low findings remain open.
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# ScadaBridge — architectural / code review (whole system)
|
||||
|
||||
**Date:** 2026-09-09 · **Status:** ALL 13 HIGH FINDINGS COMPLETED (see §9)
|
||||
**Date:** 2026-09-09 · **Status:** ALL 13 HIGH AND 33 MEDIUM FINDINGS COMPLETED (see §§9–10)
|
||||
**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,
|
||||
@@ -14,7 +14,7 @@ probes. Each area revalidated its existing IDs and searched for additional defec
|
||||
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. The review phase ran no rig rebuild, device writes, failover drill, or remote QA/production inspection.
|
||||
Sections 1–8 preserve that reviewed baseline; §9 records the subsequent implementation and local rig verification.
|
||||
Sections 1–8 preserve that reviewed baseline; §§9–10 record subsequent implementation and local rig verification.
|
||||
|
||||
**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
|
||||
@@ -38,8 +38,8 @@ contract changes since August are additive; all eight migrations are sound and t
|
||||
matches. The test suite is broad; fresh execution evidence and its limits are in §7.
|
||||
|
||||
The reviewed baseline contained **92 unique findings: 13 High, 33 Medium, 46 Low**.
|
||||
**Remediation status: all 13 High findings completed; 79 findings remain open (33 Medium, 46 Low).**
|
||||
Completion evidence and limits are recorded in §9.
|
||||
**Remediation status: all 13 High and 33 Medium findings completed; 46 Low findings remain open.**
|
||||
Completion evidence and limits are recorded in §§9–10.
|
||||
This includes low-priority test/documentation gaps and latent risks, not just reproduced failures.
|
||||
The reviewed High-priority findings were:
|
||||
|
||||
@@ -59,7 +59,7 @@ The reviewed High-priority findings were:
|
||||
| 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 remaining findings group into five themes: **audit completeness** (EI-1/2/3/5,
|
||||
The other reviewed findings grouped 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 **inert configuration** (XC-2/3/7). §5 groups everything into a remediation program.
|
||||
@@ -743,7 +743,7 @@ fetched production credentials.
|
||||
| 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 |
|
||||
| EI-10 | Implemented; verification pending | Early bounded body reader applies even with auditing disabled; real Kestrel gate pending. |
|
||||
|
||||
Probe sources/output: `/tmp/scadabridge-review-probes/Program.cs` and
|
||||
`/tmp/scadabridge-review-probes.log`; `/tmp/scadabridge-sr11-probe/Program.cs`;
|
||||
@@ -759,7 +759,7 @@ Repeatable regression recipes are attached to each finding and the §5 acceptanc
|
||||
| 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 |
|
||||
| MG-3 | Implemented; verification pending | Removed stale deployment probe memoization; peer-change regressions passed. |
|
||||
| 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 |
|
||||
@@ -896,3 +896,91 @@ No device writes, failover drill, remote deployment, commit or push was performe
|
||||
Medium/Low register and baseline formatting debt remain. The local rig has an existing
|
||||
`zztest-conn-5d93ea81` test connection retrying unreachable `localhost:4840`; healthy cluster
|
||||
readiness does not assert that every configured device endpoint is reachable.
|
||||
|
||||
## 10. Medium-severity remediation completion
|
||||
|
||||
**All 33 Medium findings are completed and verified.** The High fixes were committed first as
|
||||
`146a1323` (`Resolve all high-severity architecture review findings`). Medium implementation,
|
||||
tests and documentation remain uncommitted on `codex/resolve-high-review-findings` in
|
||||
`/Users/dohertj2/Desktop/ScadaBridge-high-findings`; no push was requested for this phase.
|
||||
Parallel subagents handled runtime/devices, management/UI and edge integrations; the primary
|
||||
agent handled persistence, listener/contracts, configuration and final integration. Independent
|
||||
cross-reviews rechecked lifecycle, sandbox containment, relay provenance and migration safety.
|
||||
|
||||
| Finding | Status | Progress / evidence |
|
||||
|---|---|---|
|
||||
| SR-2 | Completed | Fresh alarm execution scope supplies database and tracking dependencies; runtime focused and full solution suites passed. |
|
||||
| SR-3 | Completed | Correlated bounded retry restores failed alarm subscriptions; full solution suite passed. |
|
||||
| SR-4 | Completed | Implemented; immutable audited target regression passed. |
|
||||
| SR-5 | Completed | Ordered persistence drain and same-name stop barriers; independent review passed; runtime suite 102 passed including locked delete/disable and queued cancellation. |
|
||||
| SR-12 | Completed | Implemented; delayed rehydration fencing regression passed. |
|
||||
| SR-13 | Completed | Authority rechecked after each lane admission and before every delivery; StoreAndForward suite 38 passed. |
|
||||
| CM-2 | Completed | Implemented; all four env2 node configurations enable deterministic bootstrap; parsed configuration validation passed. |
|
||||
| MG-2 | Completed | Mandatory fresh caller scope filters audit trees and replay; management regressions and full UI suite passed. |
|
||||
| MG-3 | Completed | Removed stale deployment probe memoization; peer-change regressions passed. |
|
||||
| MG-4 | Completed | Server-owned bulk deadline stops subsequent dispatch and returns unfinished failures; targeted deployment tests passed. |
|
||||
| UI-5 | Completed | Isolated child processes killed and joined before admission reuse; broker-deadline and runaway-worker tests included in 97 passing UI cases. Published Linux image contains worker DLL/deps/runtimeconfig; deployed browser termination/recovery check passed. |
|
||||
| UI-14 | Completed | Console, return serialization and IPC bounded; oversized-output regressions passed in the full UI suite and deployed browser. |
|
||||
| EI-1 | Completed | Relay ownership and uncertain-outcome audit completion implemented; InboundAPI suite passed. |
|
||||
| EI-2 | Completed | Body-capture opt-out also suppresses free-form failure details and companion events; InboundAPI suite passed. |
|
||||
| EI-3 | Completed | SQL exception details respect sensitive-parameter redaction policy; audit redaction regressions passed. |
|
||||
| EI-4 | Completed | Single relay deadline and explicit pre-execution-only retry; ambiguous outcomes are not replayed; InboundAPI suite passed. |
|
||||
| EI-5 | Completed | Authenticated request-bound relay provenance replaces trust in caller-supplied forwarding headers; independent review and InboundAPI suite passed. |
|
||||
| EI-10 | Completed | Early body cap applies with auditing disabled; real Kestrel oversized/chunked cases included in six passing Host regressions. |
|
||||
| EI-11 | Completed | All retained error-message/detail fields are capped and redacted; targeted audit suite passed. |
|
||||
| DL-1 | Completed | Global unique index, domain conflict translation and duplicate preflight migration; real SQL concurrency/upgrade guard tests passed. |
|
||||
| DL-2 | Completed | Protected primary/backup configuration with per-provider model cache and idempotent bounded startup upgrade; real SQL and SQLite regressions passed. |
|
||||
| DL-3 | Completed | Eight-row keyset paging, 500-event ceiling, UTF-8 budget and preserved root/truncation metadata; all six real SQL regressions passed. |
|
||||
| DL-6 | Completed | Deployed cross-site overwrite rejected; both real import cases passed after a valid failing baseline. |
|
||||
| DC-4 | Completed | Exact requested OPC UA security mode enforced during discovery/connect; focused device suite passed. |
|
||||
| DC-5 | Completed | Shared alarm cache merges live events with snapshots and fences stale epochs; independent review and 34 device regressions passed. |
|
||||
| DC-6 | Completed | CLI returns failure for nonzero bulk failureCount and malformed responses; all three actual command/socket regressions passed. |
|
||||
| DC-7 | Completed | Notifier retries only explicit standby rejection; notifier suite passed. |
|
||||
| XC-1 | Completed | Actual socket and gRPC metadata enforce endpoint ownership despite forged authority; six combined Host/Kestrel regressions passed. |
|
||||
| XC-2 | Completed | Implemented; removed four ignored SMTP keys from all four central rig settings; database configuration guidance replaces them. |
|
||||
| XC-3 | Completed | Implemented; removed ignored cron key from all eight site settings; documents interval-based purge. |
|
||||
| XC-4 | Completed | Actual protoc descriptors match vendored message/service graphs; four tests passed including three name/type/service drift controls. Implicit JSON names normalized using effective runtime descriptors. |
|
||||
| XC-5 | Completed | Implemented; README, high-level/component requirements and component skill now describe actual gRPC/HTTP transport. |
|
||||
| ORCH-1 | Completed | Synchronous disposal joins context-free persistence drain; runtime 102-case suite passed. |
|
||||
|
||||
### Final verification
|
||||
|
||||
| Gate | Result / evidence |
|
||||
|---|---|
|
||||
| Full solution build | **0 warnings, 0 errors**; `/tmp/medium-full-build2.log`. Final deployed-browser project build also passed 0/0 after its test-only update (`/tmp/medium-browser-build.log`). |
|
||||
| Selected full solution | `dotnet test ZB.MOM.WW.ScadaBridge.slnx --no-restore --filter 'FullyQualifiedName!~PlaywrightTests' --logger trx --results-directory /tmp/scadabridge-medium-full`: **9,397 passed, 0 failed, 0 skipped**, exit 0, 30 result files. `/tmp/medium-full-tests.log`. Includes 1,399 UI, 596 Host, 125 Transport integration and 117 cross-component integration cases plus 13 performance checks. |
|
||||
| Deployed browser | **2 passed, 0 failed, 0 skipped**: Linux sandbox normal execution, bounded console/return output, noncooperative infinite-loop timeout and subsequent successful execution; trusted/passive session input and antiforgery. `/tmp/medium-browser-final.log`. No child worker remained afterward on either central node. |
|
||||
| Real persistence | Global-name concurrency and duplicate-upgrade rejection, primary/backup encryption and idempotent conversion passed against SQL Server; key-ring isolation and guards passed against SQLite. `/tmp/medium-persistence-final.log`. Final replay keyset/Unicode/oversized-root SQL checks also passed and are included in the full solution run. |
|
||||
| Transport contract | Six real Kestrel listener/body-cap cases passed (`/tmp/medium-host-green.log`); actual protoc descriptor comparison and three drift controls passed (`/tmp/medium-proto-green2.log`). |
|
||||
| Local rig | Image `sha256:b8d158309b5d850fc01a3d32e7325e2238611b039e37f5fa0d51e7a488ddee36` on all **eight nodes**; all ready, one active per pair, two members per pair, zero unreachable members or restarts. Site-A replication connected, backlog 0. `/tmp/scadabridge-medium-postdeploy-health.json`. |
|
||||
| Upgrade | Coordinated central-pair stop, successful COPY_ONLY/CHECKSUM backup and RESTORE VERIFYONLY, then migration `20260909203046_EnforceGlobalInstanceNamesAndProtectProtocolConfiguration`. All 4 protocol rows retained; 3 non-null primary values protected, 0 unprotected values, existing 1-entry key ring preserved. `/tmp/medium-rig-backup.log`, `/tmp/medium-rig-before-counts.log`, `/tmp/medium-rig-after-counts.log`. Live Site-A OPC UA connection reconnected after upgrade. |
|
||||
| Configuration/docs | All 12 rig JSON files parse; four env2 bootstrap guards enabled; obsolete SMTP/cron keys removed; transport entry-point documentation corrected. Component requirements and umbrella project index updated. |
|
||||
|
||||
### Operational notes and verification limits
|
||||
|
||||
- The SQL backup remains inside the existing SQL Server data volume at
|
||||
`/var/opt/mssql/backup/ScadaBridgeConfig_before_medium_20260909_211100.bak`.
|
||||
Protocol encryption is a coordinated central upgrade: old binaries cannot read the new values.
|
||||
A rollback requires the coordinated pre-upgrade database/key-ring backup, as documented in
|
||||
`Component-ConfigurationDatabase.md`; the migration refuses a downgrade over protected rows.
|
||||
- The image was built from the implementation worktree and deployed using the original
|
||||
repository's canonical Compose file. Existing configuration/log bind mounts and named data
|
||||
volumes were preserved. Env2 bootstrap settings were verified statically; env2 containers
|
||||
remain untouched. No device writes, live failover drill or remote deployment was performed.
|
||||
- Sandbox isolation provides a process resource/lifetime boundary, not a separate OS identity.
|
||||
The worker is killed at the deadline. If a parent-side broker ignores cancellation, its slot
|
||||
and scope remain owned until that operation actually completes; no orphan broker operation
|
||||
or early capacity reuse is introduced. Already accepted remote work cannot be undone by
|
||||
killing its client. Console retains 32,768 characters plus a fixed truncation marker; return
|
||||
serialization is capped at 32 KiB bytes, IPC at 1 MiB, managed worker heap at 512 MiB.
|
||||
- Replay is capped at 500 events and a 1 MiB serialized UTF-8 budget, with eight-row keyset
|
||||
pages bounding EF buffering. Oversized events are omitted whole with truthful truncation
|
||||
and separately retained root metadata. Shared snapshot/lifecycle races are covered by
|
||||
deterministic tests, not a live raised-alarm scenario. The pre-existing unreachable
|
||||
`zztest-conn-5d93ea81` test connection remains outside this remediation.
|
||||
- The first proto comparison exposed implicit JSON-name representation differences, not
|
||||
wire drift. It now compares effective runtime names against compiler output while preserving
|
||||
explicit field/service metadata. Earlier fixture/build failures were corrected without
|
||||
weakening production guards: audit test doubles accept caller scope, and protocol-writing
|
||||
fixtures use stable ephemeral key rings. These are not claimed as before-fix reproductions.
|
||||
- The 46 Low findings remain open. Focused test counts overlap the full suite and are not additive.
|
||||
|
||||
Reference in New Issue
Block a user