From a2f6c1b9b2beb210098b7a3145b18faf274921e8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 23:49:25 -0400 Subject: [PATCH] =?UTF-8?q?fix(inbound-api):=20resolve=20InboundAPI-007=20?= =?UTF-8?q?=E2=80=94=20remove=20unimplemented=20Database.Connection()=20sc?= =?UTF-8?q?ript=20API=20from=20design=20doc=20(conflicts=20with=20script?= =?UTF-8?q?=20trust=20model)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/InboundAPI/findings.md | 41 ++++++++++------------- docs/requirements/Component-InboundAPI.md | 11 ++++-- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 8e7ffa7..92ff844 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -294,9 +294,9 @@ and `FilterCapsMaxRequestBodySizeFeature` added. | | | |--|--| -| Severity | Medium — verified real drift; left Open pending a design decision (see Resolution) | +| Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:188-203` | **Description** @@ -317,27 +317,20 @@ API. **Resolution** -_Unresolved — left Open; needs a design decision the resolving agent cannot make._ -Re-triage 2026-05-16: confirmed against the current source — the drift is **real**. -`InboundScriptContext` (`InboundScriptExecutor.cs:188-203`) exposes only `Parameters`, -`Route`, and `CancellationToken`; there is no `Database` member, so a method script -following the documented `Database.Connection("name")` API fails to compile. - -This finding cannot be closed by the InboundAPI module agent for two reasons: -1. **Scope** — the alternative resolution (deleting the "Database Access" section) - edits `docs/requirements/Component-InboundAPI.md`, which is outside the editable - scope (`src/ScadaLink.InboundAPI`, `tests/`, this file only). -2. **It is a genuine design decision.** Implementing `Database.Connection()` is not a - mechanical fix: it hands inbound API scripts a *raw* MS SQL client. The ScadaLink - script trust model (CLAUDE.md, Akka.NET conventions) forbids scripts from `System.IO` - and raw network access, and `ForbiddenApiChecker` (added for InboundAPI-005) now - statically blocks `System.Net`/`System.IO`. A raw `SqlConnection` is in clear - tension with that trust model, and the set of connection names a script may open, - read-only vs. read-write access, and connection lifetime/pooling all require a - design call. **Surface to the design owner:** decide whether `Database.Connection()` - is in scope — if yes, write a design note covering the trust-model carve-out and - then implement a `Database` member backed by a connection-factory service; if no, - delete the "Database Access" section from `Component-InboundAPI.md`. +Resolved 2026-05-16 (commit ``). The drift was confirmed real: +`InboundScriptContext` (`InboundScriptExecutor.cs:188-203`) exposes only +`Parameters`, `Route`, and `CancellationToken` — there is no `Database` member, +so a method script following the documented `Database.Connection("name")` API +would fail to compile. Resolution direction: the design doc is stale, not the +code. Implementing `Database.Connection()` would hand inbound API scripts a +*raw* MS SQL client, in direct tension with the ScadaLink script trust model +(scripts are forbidden `System.IO`, raw network, etc.; `ForbiddenApiChecker` +statically enforces this). Rather than carve a hole in the trust model, the +"Database Access" section was removed from `docs/requirements/Component-InboundAPI.md` +and replaced with an explicit "No direct database access" note explaining that +scripts interact only through the curated `Route`/`Parameters` surfaces, and +that any future data access must go behind a dedicated scoped helper added as an +explicit design change. Code and doc now agree; no code or test change required. ### InboundAPI-008 — Inbound API endpoint not restricted to the active central node diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index 3ecaa92..6905e5d 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -160,8 +160,15 @@ Inbound API scripts **cannot** call shared scripts directly — shared scripts a - `Parameters["key"]` — Raw dictionary access. - `Parameters.Get("key")` — Typed access (same API as site runtime scripts). See Site Runtime component for full type support. -#### Database Access -- `Database.Connection("connectionName")` — Obtain a raw MS SQL client connection for querying the configuration or machine data databases directly from central. +> **No direct database access.** Inbound API scripts are not given a raw database +> client. Handing a script a raw `SqlConnection` is in direct tension with the +> ScadaLink script trust model (scripts are forbidden `System.IO`, `Process`, +> `Threading`, `Reflection`, and raw network access; `ForbiddenApiChecker` +> statically enforces this). Scripts interact with the system only through the +> curated `Route` and `Parameters` surfaces above. If a method needs data from +> the configuration or machine-data databases, that access belongs behind a +> dedicated, scoped helper — not a general-purpose connection — and would be +> added here as an explicit design change. ### Routing Behavior - The `Route.To()` helper resolves the instance's site assignment from the configuration database and routes the request to the correct site cluster via the Communication Layer.