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

docs(code-review): resolve InboundAPI-026/027/028/029 + record InboundAPI-030

Browse Source 
Records the remediation in commit b3c90143: Database helper authorized + secured
(parameterized, writes allowed, async/deadline-bound), WaitForAttribute bound by the
wait timeout, and the newly-surfaced compile-surface-mirror gap (030) fixed. InboundAPI
drops to 0 open findings; aggregate README regenerated (0 pending / 568 total).
This commit is contained in:
Joseph Doherty
2026-06-23 22:03:47 -04:00
parent b3c9014379
commit 1f9de8a2b5
2 changed files with 128 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/InboundAPI/findings.md
+117 -9
View File
@@ -8,7 +8,7 @@
| Last reviewed | 2026-06-20 |
| Reviewer | claude-agent |
| Commit reviewed | `4307c381` |
| Open findings | 4 |
| Open findings | 0 |
## Summary
@@ -1336,7 +1336,7 @@ already wired.
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs:13-70`, `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs:267-281`, `:387`; design doc `docs/requirements/Component-InboundAPI.md:202-215` |
**Description**
@@ -1393,7 +1393,30 @@ read-only is enforced (see InboundAPI-028).
**Resolution**
_Unresolved._
Resolved 2026-06-23 (commit `b3c90143`). Design-owner decision (recorded with the user):
**keep the helper, authorize it in the design doc, protect against SQL injection, and
allow writes** (option (a), with writes permitted rather than SELECT-only). Changes:
1. **Design doc reconciled.** `Component-InboundAPI.md`'s "No direct database access" block
was replaced with a "Database Access" section authorizing the curated, scoped `Database`
helper as the explicit design change the prior text demanded — documenting that scripts
are still never handed a raw connection, named connections only, parameterized values,
reads + writes, deadline-bound.
2. **SQL-injection protection made explicit.** The injection vector is closed by parameter
binding: every request-derived value is passed via `parameters` and bound as a named
`@`-prefixed SQL parameter (`AddParameters`), never concatenated into the statement text.
The false "read-only" XML summary + call-site comment were rewritten to describe the
actual contract (parameterized, named-connection-scoped, reads + writes). Regression test
`ParameterValues_are_bound_not_concatenated` proves an injection payload supplied as a
value matches nothing (would have matched the seeded row if concatenated).
3. **Writes authorized.** A dedicated `ExecuteAsync(connectionName, sql, parameters)`
(→ `ExecuteNonQueryAsync`, returns rows affected) gives scripts a first-class, still-
parameterized write path. Test `ExecuteAsync_performs_write_and_returns_rows_affected`.
Connection reach stays scoped to the connections configured on the central
`IDatabaseGateway` (a script cannot supply an arbitrary connection string). The
sync-over-async + token issue is fixed under InboundAPI-027; the compile-surface mirror
that must accept the authorized API is fixed under the newly-surfaced InboundAPI-030.
### InboundAPI-027 — `InboundDatabaseHelper` is sync-over-async and ignores the method-deadline token — thread-pool starvation and an unbounded slow query
@@ -1401,7 +1424,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Performance & resource management |
| Status | Open |
| Status | Resolved |
| Location | `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs:25-33`, `:40-44` |
**Description**
@@ -1440,7 +1463,19 @@ with the token and a command timeout so a slow query cannot run unbounded.
**Resolution**
_Unresolved._
Resolved 2026-06-23 (commit `b3c90143`): took the recommended async path end-to-end.
`InboundDatabaseHelper` is now fully asynchronous — `QuerySingleAsync<T>` / `QueryAsync`
/ `ExecuteAsync` `await _gateway.GetConnectionAsync(...)` then `await
cmd.ExecuteScalarAsync(_ct)` / `ExecuteReaderAsync(_ct)` (+ `ReadAsync(_ct)`) /
`ExecuteNonQueryAsync(_ct)` — no `.GetAwaiter().GetResult()` blocking a pool thread, and
the method-deadline token `_ct` is forwarded to every command so a slow query is cancelled
when the method timeout fires. `InboundScriptExecutor` now passes the method `timeout` to
the helper, which sets a `CommandTimeout` backstop (`Math.Ceiling(timeout.TotalSeconds)`)
in case a provider does not honour mid-statement cancellation. Scripts `await` the helper
(globals run under `CSharpScript.RunAsync`, which supports top-level await). Regression test
`QuerySingleAsync_honours_cancellation_token_on_execute` opens the connection with a
non-cancelling token but a pre-cancelled deadline, proving the EXECUTE path now observes
the token (where the old synchronous `ExecuteScalar()` ignored it).
### InboundAPI-028 — `InboundDatabaseHelper` has no negative-path tests; `Database`/`WaitForAttribute` are not exercised end-to-end through the endpoint
@@ -1448,7 +1483,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Status | Resolved |
| Location | `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundDatabaseHelperTests.cs:34-67`, `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs` |
**Description**
@@ -1479,7 +1514,20 @@ the executor→context→helper wiring is covered through the real endpoint.
**Resolution**
_Unresolved._
Resolved 2026-06-23 (commit `b3c90143`). `InboundDatabaseHelperTests` was rewritten for the
async API and gained the missing negative paths: `QuerySingleAsync_null_gateway_throws`,
`QueryAsync_null_gateway_throws`, `ExecuteAsync_null_gateway_throws` (the
`InvalidOperationException("Database is not available …")` path); `ParameterValues_are_bound_not_concatenated`
(SQL-injection protection); `ExecuteAsync_performs_write_and_returns_rows_affected` (the
newly-authorized write path); and `QuerySingleAsync_honours_cancellation_token_on_execute`
(the InboundAPI-027 deadline-cancellation path). A write/DDL-rejection test is intentionally
NOT added — the design decision (InboundAPI-026) is to permit writes, so there is nothing to
reject. End-to-end coverage was added in `EndpointExtensionsTests`:
`Script_UsingDatabase_RunsEndToEndThroughEndpoint` drives a method script calling
`await Database.QuerySingleAsync(...)` through the real `POST /api/{methodName}` flow (proving
executor→DI-scope→`IDatabaseGateway`→helper wiring), and
`Script_UsingWaitForAttribute_RunsEndToEndThroughEndpoint` drives
`await Route.To(...).WaitForAttribute(...)` through the endpoint.
### InboundAPI-029 — Routed `WaitForAttribute` is cancelled by the method-level deadline, contradicting spec §6 (wait bounded by the wait timeout, not the method timeout)
@@ -1487,7 +1535,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs:220-247`, `:302-303`; design doc `docs/requirements/Component-InboundAPI.md:192` |
**Description**
@@ -1524,4 +1572,64 @@ deadline-token *inheritance*, not the timeout *ordering* conflict).
**Resolution**
_Unresolved._
Resolved 2026-06-23 (commit `b3c90143`). Design-owner decision (recorded with the user):
**the wait timeout wins** (option (b)) — the routed wait must honour spec §6, not be cut
short by the generic method deadline. `RouteTarget.WaitForAttribute` no longer resolves its
token via `Effective(...)` (which fell back to the method deadline). It now builds a per-wait
`CancellationTokenSource(timeout + WaitResponseGrace)` linked with an explicit caller token
and the raw client-disconnect token — deliberately **excluding** the method deadline — so a
wait longer than the method timeout runs to its full wait timeout (the site enforces
`timeout` and returns `Matched=false`; the small grace keeps the local backstop from
pre-empting the site's timed-out response). The raw client-abort token is threaded separately
via the new `RouteHelper.WithRequestAborted(...)` (the executor passes the request token), so
a client disconnect still cancels the wait. `Component-InboundAPI.md:192` was updated to state
the wait is bounded by its own `timeout`, may legitimately outlive the method timeout, and is
the deliberate exception to the InboundAPI-016 rule that routed calls inherit the method
deadline. The stale `WaitForAttribute_WithNoExplicitToken_InheritsMethodDeadlineToken` test
was replaced with `WaitForAttribute_IsNotBoundByMethodDeadline_WaitTimeoutGoverns` (proves an
already-cancelled method deadline does NOT cut the wait short), plus
`WaitForAttribute_ExplicitToken_IsHonoured` and `WaitForAttribute_ClientDisconnect_CancelsTheWait`.
### InboundAPI-030 — Central UI compile-surface mirror omits the `Database` member, so the authorized script DB API fails the design-time gate
| | |
|--|--|
| Severity | Medium |
| Category | Design-document adherence |
| Status | Resolved |
| Location | `src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/InboundScriptHost.cs`, `src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxInboundScriptHost.cs` |
**Description**
Surfaced 2026-06-23 while resolving InboundAPI-026. The Central UI script-analysis
compile surfaces — `InboundScriptHost` (the globals type Roslyn uses for inbound-method
**diagnostics**) and `SandboxInboundScriptHost` (the globals type for a **Test Run**) — are
documented to "mirror the surface the runtime exposes" (`InboundScriptContext` + `RouteHelper`).
But when the runtime `Database` helper was added (2026-06-16, the InboundAPI-026 helper),
neither mirror was updated to expose a `Database` member. Per CLAUDE.md the design-time deploy
gate is **authoritative** (real semantic compile), so an inbound method whose script uses
`Database.*` would fail Central UI diagnostics (and a Test Run compile) with a `CS` error —
`'InboundScriptHost' does not contain a definition for 'Database'` — even though the runtime
exposes it. Authorizing the helper (InboundAPI-026) without fixing the mirror would leave an
authorized API that the gate rejects.
**Recommendation**
Add a `Database` member to both compile-surface hosts whose method signatures exactly match
the runtime `InboundDatabaseHelper` (so the same user code type-checks against runtime,
diagnostics, and Test Run). The diagnostics mirror returns dummy values (never invoked — only
its signatures are read); the Test-Run sandbox mirror should throw `ScriptSandboxException`,
consistent with how its `Route` accessor throws. Add an analysis test asserting a
`Database`-using script diagnoses clean.
**Resolution**
Resolved 2026-06-23 (commit `b3c90143`): added a `DatabaseAccessor Database` member to both
`InboundScriptHost` and `SandboxInboundScriptHost`, with `QuerySingleAsync<T>` / `QueryAsync`
/ `ExecuteAsync` signatures identical to the runtime `InboundDatabaseHelper`. The diagnostics
mirror returns dummy completed tasks; the Test-Run sandbox mirror throws `ScriptSandboxException`
("…not available in Test Run — database access needs the central configuration / machine-data
databases."), mirroring how `RouteAccessor` throws on cross-site routing. Regression test
`InboundScript_Database_DiagnosesClean` (CentralUI.Tests) drives a script using all three
`Database` methods through `ScriptAnalysisService.Diagnose(... ScriptKind.InboundApi)` and
asserts no `CS`/`SCADA` markers — alongside the existing `InboundScript_WaitForAttribute_DiagnosesClean`.