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.
RouteHelper/RouteTarget carry a new captureBodies flag (default true) threaded
through every With* copy, same shape as the site-scope field Task 3 added.
InboundScriptExecutor resolves IOptionsMonitor<AuditLogOptions> from the
per-execution DI scope and looks up PerTargetOverrides[method.Name]?.SkipBodyCapture
exactly as AuditWriteMiddleware.cs:183-187 does for the inbound row, then calls
.WithBodyCapture(!skip) when building the script's Route helper. When set, the
always-on ScriptTrace.RouteCall row omits its request/response summary, so an
operator who suppressed a method's body from the audit log doesn't get it back
via the routed call's arguments (derived from that same body).
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
A method-entry Live run had no site-scope check at all. RunValueChangeAsync
calls EnforceSiteScope on its named instance, but a method run has no single
target site up front — the operator picks a METHOD, and where its script routes
is only known once it runs — so a site-scoped Deployer could reach any site the
method happened to route to.
The scope is therefore pushed into RouteHelper and enforced at route time:
- RouteHelper.WithSiteScope(IReadOnlySet<string>?) carries a set of site
IDENTIFIERS (null = unscoped, the state of every ordinary inbound HTTP
request; an EMPTY set permits nothing, the fail-closed outcome for a scoped
user whose sites no longer exist). Threaded through every With* copy and To().
- The gate lives in RouteTarget.ResolveSiteAsync — the single choke point that
Call, GetAttributes, WaitForAttribute and SetAttributes all pass through — so
it fires BEFORE any site traffic and throws the new RouteSiteScopeException.
- InboundScriptExecutor maps it to error code SITE_SCOPE_DENIED. The type is
InboundAPI's own (that assembly cannot reference ManagementService), and it
derives from InvalidOperationException so base-type callers are unaffected.
- FlowDebugRunner.RunMethodAsync resolves the user's PermittedSiteIds (numeric)
to identifiers via ISiteRepository and converts SITE_SCOPE_DENIED back to
SiteScopeViolationException (-> ManagementUnauthorized), AFTER writing the
inbound audit row so a denied attempt is still recorded against the operator.
Administrators and unscoped users get a null scope and are unaffected, exactly
as EnforceSiteScope treats them. Every contract change is a trailing optional
parameter. Mutation-checked: disabling the gate fails 3 of the 5 new RouteHelper
tests and 1 of the runner tests, while the in-scope/unscoped positives still pass.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
InboundDatabaseHelper gains trailing optional auditWriter / nodeIdentity /
executionId / parentExecutionId / methodName / traceSink parameters; every
QuerySingle/Query/Execute writes one DbOutbound/DbWrite row (Delivered/Failed,
Target = connection name, Actor = 'api:{method}', DurationMs, SQL + parameters
as RequestSummary, Extra in the site's {op, rowsAffected|rowsReturned}
vocabulary, SourceNode) via ICentralAuditWriter — best-effort, never into the
script; the same instance goes to the RouteHelper trace sink first. Closes the
audit-scope gap Component-AuditLog.md already claimed (plan D7). Redaction and
the 8 KB cap apply at write time in the shared redactor, as for site rows.
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
Flow Debugger (design 2026-09-05 §5.2): InboundScriptExecutor.ExecuteAsync gains a trailing
trace parameter that builds the script's Route helper WithTrace(trace); the inbound endpoint
passes method.TraceAllCalls so a flagged integration method is traced on every real call.
Claude-Session: https://claude.ai/code/session_01GSLkkHKz9JrAtmJjmQK3cP
Flow Debugger (design 2026-09-05 §5.1/§5.2/§7.1): RouteTarget.Call emits one
ScriptTrace.RouteCall central-direct-write row per routed call (ExecutionId = the inbound
parent, Extra.childExecutionId from the reply, best-effort, never throws into the script)
whether or not the run is traced. WithTrace(bool) stamps Trace on RouteToCallRequest and
RouteToSetAttributesRequest; WithTraceSink hands the row + the reply's inline site events to a
callback so a Live run renders without touching the store. DI supplies ICentralAuditWriter /
INodeIdentityProvider as optional ctor args; new RouteHelper(locator, router) still compiles.
Claude-Session: https://claude.ai/code/session_01GSLkkHKz9JrAtmJjmQK3cP
A caller addressing a standby central node directly (or hitting Traefik inside its
/health/active probe window after a failover) used to get 503 STANDBY_NODE. The
standby now relays the request verbatim to the active peer over HTTP
(IStandbyRequestForwarder / HttpStandbyRequestForwarder) and replays the response;
method scripts and Route.To() still execute only on the active node.
- ScadaBridge:InboundApi:StandbyForwardPeerEndpoints (static list of the OTHER
central node's HTTP base URL; empty = old 503 behaviour) + StandbyForwardTimeout
(60s), both startup-validated.
- Peers tried in order: peer 503 stamped X-ScadaBridge-Standby, connection failure
or timeout → next peer; all fail → 503 ACTIVE_NODE_UNREACHABLE. A 503 without
the marker is the active node's own answer and is replayed.
- One-hop loop guard via X-ScadaBridge-Forwarded; 413 body cap now runs BEFORE
the active-node check so a standby never buffers/relays an oversized body.
- AuditWriteMiddleware skips its own ApiInbound row for a relayed request so a
call is audited once, on the node that executed it.
- Rig + env2 central nodes configured; spec (Component-InboundAPI.md → Standby
forwarding), Component-Host.md options row, CLAUDE.md updated.
- 20 new tests (filter decision + relay mechanics + validator).
Claude-Session: https://claude.ai/code/session_019ZxXT5sxZs5XrtdwYzJDPV
WP2.6 (arch-review remediation, cross-cutting misc):
- SiteExternalSystemRepository: name/ID-indexed ExternalSystemDefinitionCache replaces
the fetch-all + reverse-map scan on every by-ID/method lookup; loaded once per
redeploy, invalidated by DeploymentManagerActor after HandleDeployArtifacts applies
external-system changes. Static JsonSerializerOptions for method-list parsing.
- Inbound API: short-TTL ApiMethodCache fronts the per-request ApiMethod repository
fetch; invalidated by name via the existing ScriptArtifactChangeSubscriber/
IScriptArtifactChangeBus pipeline, self-healing via TTL for changes the bus
doesn't cover (e.g. Management API edits).
- StoreAndForward: the cached-call audit-observer queue — the one unbounded channel
left in the system — is now bounded (ObserverQueueCapacity, default 10,000) with
DropOldest overflow and a dropped-notification counter.
- SiteStreamManager: alarm state changes now travel a dedicated publish
source/broadcast hub, isolated from the (far higher-volume) attribute path, so an
attribute storm can no longer evict a pending alarm transition; the alarm hand-off
queue is bounded with a drop counter surfaced on the site health report
(SiteStreamAlarmDropCount via the new SiteStreamAlarmDropReporter), and publishing
is skipped entirely at zero subscribers on either path.
- CLI ManagementHttpClient: explicit 30s HttpClient.Timeout on the shared
construction (was the 100s framework default), overridable via
SCADABRIDGE_HTTP_TIMEOUT_SECONDS.
Deviation: the failback-probe heartbeat item is NOT included — its only viable
surface (CentralChannelProvider.cs / heartbeat consumers) lives entirely in the
Communication project, explicitly off-limits to this work package this phase.
Tests: SiteRuntime.Tests (550), InboundAPI.Tests (278), StoreAndForward.Tests (133),
CLI.Tests (390), HealthMonitoring.Tests (97) — all green after full solution build.
The NuGetAuditSuppress in Directory.Packages.props was masking a LIVE high-severity
vulnerability, not documenting an accepted one. Only the Host project resolved a
patched SQLitePCLRaw.lib.e_sqlite3 2.1.12 (transitively, via ZB.MOM.WW.Auth.ApiKeys).
Every other SQLite consumer - AuditLog, SiteRuntime, StoreAndForward, SiteEventLogging
and 11 test projects - still resolved the vulnerable 2.1.11.
The suppression's rationale was factually wrong: it claimed 'the only patched native
lib is the SQLitePCLRaw 3.x line'. 2.1.12 patches this advisory within the 2.1.x line,
so the feared risky force-override of the whole family to 3.x is unnecessary.
Fix: explicit PackageReference to the patched 2.1.12 in each SQLite-consuming project,
plus the central PackageVersion row. Suppression removed, so the advisory is audited
again rather than silenced.
Rejected alternative: CentralPackageTransitivePinningEnabled. It clears the advisory in
one line but makes every central version a ceiling for transitive resolution, demanding
bumps to Google.Protobuf, Grpc.Net.Client, Microsoft.Data.SqlClient and Newtonsoft.Json.
That is a gRPC/data-access change to a production SCADA platform and deserves its own
reviewed commit.
Verified: forced full-solution restore reports no NU1903 (only the pre-existing,
unrelated AngleSharp NU1902 in CentralUI.Tests); all four previously-vulnerable src
projects now resolve 2.1.12; 55 projects build 0 warnings / 0 errors; 1108 tests pass
across the SQLite layer (AuditLog 354, ConfigurationDatabase 357, Security 181,
StoreAndForward 152, SiteEventLogging 64).
Also updates the now-obsolete CompileAndRegister_NonCompilingUpdate test
(InboundScriptExecutorTests.cs, not in the plan's Files list) to assert the
deliberate DB-authoritative behavior: a broken save now 500s consistently on
every node instead of the stale delegate silently serving.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
CompileAndRegister only swaps the cached handler on a successful compile, so a
management (or DB) save of a non-compiling inbound script silently kept the
previously-registered handler serving — stale results, still HTTP 200 — while
UpdateApiMethod reported success. A broken save thus looked like a no-op, with
the real Roslyn error only in the central log.
Surface the diagnostics instead: a new CompileAndRegister(method, out errors)
overload reports the compile/trust-model errors (the bool overload is kept for
existing callers), and HandleCreate/UpdateApiMethod return them as a top-level
compileWarning on the management result (null when the script compiled). The
save still persists and the previously registered version keeps serving — the
warning just stops a non-compiling save from looking like a success.
- InboundScriptExecutor: Compile returns its diagnostics alongside the handler.
- ManagementActor: TryCompileAndDescribe + flat ApiMethodResult projection so
compileWarning rides at the top level without polluting the ApiMethod POCO.
- Component-InboundAPI.md: document the non-fatal-but-surfaced contract.
- Tests: executor diagnostics + non-compiling-update-keeps-previous-handler,
plus an end-to-end ManagementActor test asserting compileWarning is returned.
Fixes the 8 findings from the 2026-06-24 re-review (commit c42bb485), with a
regression test per Medium finding:
- DataConnectionLayer-029 (Med): HandleAlarmSubscribeCompleted now mirrors the
tag-path re-check — if a feed is already stored for the source, release the
redundant just-created subscription instead of overwriting + leaking the first
one (the double-subscribe window DCL-023 reopened). +regression test.
- InboundAPI-031 (Med): remove WaitForAttribute's local 5s grace backstop (tighter
than the CommunicationService Ask's timeout+IntegrationTimeout round-trip budget,
so a slow-but-valid timed-out 'false' got cancelled into a 500). Link only the
client-abort + explicit caller tokens; the lower layer owns the backstop. +test.
- SiteRuntime-032 (Med): derive the deployed count from an authoritative set of
deployed config names (HashSet) instead of a map-presence-gated int, so deleting
a DISABLED instance decrements correctly (SiteRuntime-029's gate leaked it).
+deploy->disable->delete regression test.
- StoreAndForward-028 (Med): reset _bufferedCount in StopAsync alongside the
register-guard so a same-instance Stop->Start re-seeds from a clean base (no ~2N
gauge double-count). +restart regression test.
- AuditLog-017 (Low): test the OnIngestAsync scope-resolution guard (actor survives,
replies empty, counts the failure) — no longer unpinned.
- CentralUI-037 / ScriptAnalysis-009 / SiteRuntime-033 (Low): doc-comment + spec
fixes (Database-throws in the inbound sandbox; baseReferences param wording;
native-alarm cap return-to-normal + per-condition NativeAlarmDropped eviction).
Targeted suites green: SiteRuntime 5, StoreAndForward 6, InboundAPI 31,
DataConnectionLayer 10, AuditLog 5, ScriptAnalysis 40, CentralUI ScriptAnalysis 52.
Resolves InboundAPI-026/027/028/029 (+ newly-surfaced -030).
- 026: authorize the scoped Database helper in the design doc; SQL-injection
protection is parameter binding (values never concatenated); allow writes via
ExecuteAsync; drop the false 'read-only' claim. Named connections only.
- 027: async ADO.NET end-to-end (no .GetAwaiter().GetResult()); honour the method
deadline token on ExecuteScalarAsync/ExecuteReaderAsync/ExecuteNonQueryAsync +
a CommandTimeout backstop derived from the method timeout.
- 028: negative-path tests (null-gateway, deadline cancellation, parameterization)
+ e2e Database + WaitForAttribute cases through the real endpoint.
- 029: WaitForAttribute is bounded by its WAIT timeout (per-wait CTS + client-abort
+ explicit token), NOT the method deadline (spec §6) — a long wait may outlive the
method timeout; WithRequestAborted threads the raw client-abort token separately.
- 030: Central UI compile-surface mirrors (InboundScriptHost / SandboxInboundScriptHost)
gained the Database member (drifted since the runtime helper was added) so the
authorized async API type-checks at the design-time gate.
1. Request headers in Extra JSON (AuditWriteMiddleware): adds a `requestHeaders`
object to the existing Extra JSON alongside remoteIp/userAgent; headers whose
names appear in AuditLogOptions.HeaderRedactList (Authorization, X-Api-Key,
Cookie, Set-Cookie by default) are replaced with "<redacted>" using
OrdinalIgnoreCase matching — same policy as ScadaBridgeAuditRedactor.
2. AuditInboundCeilingHits counter: new IAuditInboundCeilingHitsCounter interface
+ NoOpAuditInboundCeilingHitsCounter default; AuditCentralHealthSnapshot
implements the interface (Interlocked field, thread-safe) and exposes
AuditInboundCeilingHits on IAuditCentralHealthSnapshot; AddAuditLog registers
the NoOp default, AddAuditLogCentralMaintenance forwards to the snapshot;
AuditWriteMiddleware accepts the counter as an optional ctor arg and increments
it once per request where either the request or response body hit the cap.
3. Per-method SkipBodyCapture opt-out: adds SkipBodyCapture bool to
PerTargetRedactionOverride; AuditWriteMiddleware consults the per-target
override map at the start of InvokeAsync (before EnableBuffering) and, when
set, skips body read + capture entirely — the audit row still emits with
headers/metadata but null RequestSummary/ResponseSummary; truncation flags
are also cleared so the ceiling-hits counter is not bumped for opted-out methods.
- Add WhitespaceAuthorization_ValidXApiKey_Returns200: pins the IsNullOrWhiteSpace
fall-through — a present-but-blank Authorization header is treated as absent so a
valid X-API-Key still authenticates (200).
- Remove MissingBearer_Returns401 (added in 510559e): identical path to
NeitherHeader_Returns401 (no Authorization + no X-API-Key → 401); keep the
descriptively-named NeitherHeader variant.
- Change "legacy 'X-API-Key'" -> "alternate 'X-API-Key'" in EndpointExtensions.cs and
the BuildPostWithApiKeyHeader/HappyPath doc comments to avoid implying Bearer is
the older transport (Bearer was itself introduced by the prior auth re-arch).
Object/List parameters and return values were shape-validated only (object vs
array), with no field-level/nested type checks — type-wrong nested data passed
inbound validation and failed only at script runtime. Add recursive type
validation (declared Object field types, List element type, scalars at any depth)
with path-qualified errors, symmetric across ParameterValidator and ReturnValueValidator.
Both validators now parse the canonical JSON Schema definition format (the
Central UI / MigrateParametersToJsonSchema output) via a shared recursive engine,
Commons.Types.InboundApi.InboundApiSchema, instead of the legacy flat
[{name,type}] array which they could not even deserialize from migrated rows.
The legacy flat-array form is still accepted on read for transition safety.
Undeclared fields are rejected at every level (consistent with the existing
top-level unexpected-parameter rejection); a present-but-null value satisfies
any type, only absence of a required field is an error.
Joseph Doherty