From 1f9de8a2b58c1aa69c582e2ccf91599af4d30525 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 23 Jun 2026 22:03:47 -0400 Subject: [PATCH] docs(code-review): resolve InboundAPI-026/027/028/029 + record InboundAPI-030 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). --- code-reviews/InboundAPI/findings.md | 126 ++++++++++++++++++++++++++-- code-reviews/README.md | 29 +++---- 2 files changed, 128 insertions(+), 27 deletions(-) diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index d7109c81..e623e5b8 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -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` / `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` / `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`. diff --git a/code-reviews/README.md b/code-reviews/README.md index 7869ab63..49c1c4da 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,10 +40,10 @@ module file and counted in **Total**. | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 1 | -| Medium | 1 | -| Low | 2 | -| **Total** | **4** | +| High | 0 | +| Medium | 0 | +| Low | 0 | +| **Total** | **0** | ## Module Status @@ -61,7 +61,7 @@ module file and counted in **Total**. | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 26 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 25 | | [Host](Host/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 26 | -| [InboundAPI](InboundAPI/findings.md) | 2026-06-20 | `4307c381` | 0/1/1/2 | 4 | 29 | +| [InboundAPI](InboundAPI/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 30 | | [KpiHistory](KpiHistory/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 6 | | [ManagementService](ManagementService/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 26 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 13 | @@ -86,21 +86,14 @@ description, location, recommendation — lives in the module's `findings.md`. _None open._ -### High (1) +### High (0) -| ID | Module | Title | -|----|--------|-------| -| InboundAPI-026 | [InboundAPI](InboundAPI/findings.md) | `InboundDatabaseHelper` gives inbound scripts arbitrary raw SQL (not read-only); contradicts the design doc's "No direct database access" decision and regresses InboundAPI-007 | +_None open._ -### Medium (1) +### Medium (0) -| ID | Module | Title | -|----|--------|-------| -| InboundAPI-027 | [InboundAPI](InboundAPI/findings.md) | `InboundDatabaseHelper` is sync-over-async and ignores the method-deadline token — thread-pool starvation and an unbounded slow query | +_None open._ -### Low (2) +### Low (0) -| ID | Module | Title | -|----|--------|-------| -| InboundAPI-028 | [InboundAPI](InboundAPI/findings.md) | `InboundDatabaseHelper` has no negative-path tests; `Database`/`WaitForAttribute` are not exercised end-to-end through the endpoint | -| InboundAPI-029 | [InboundAPI](InboundAPI/findings.md) | Routed `WaitForAttribute` is cancelled by the method-level deadline, contradicting spec §6 (wait bounded by the wait timeout, not the method timeout) | +_None open._