dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(inbound-api): resolve InboundAPI-014..017 — return-value validation, reflection-gateway hardening, deadline-bound routed calls, RouteHelper test coverage

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-17 03:18:33 -04:00
parent aca65e85bb
commit 73a393076a
12 changed files with 993 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

85
code-reviews/InboundAPI/findings.md
View File

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-17 |
| Reviewer | claude-agent |
| Commit reviewed | `39d737e` |
| Open findings | 4 |
| Open findings | 0 |
## Summary
@@ -606,7 +606,7 @@ from "key not approved"), but that doc edit is outside this module's editable sc
|--|--|
| Severity | Medium |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:201-205`, `src/ScadaLink.Commons/Entities/InboundApi/ApiMethod.cs:10` |
**Description**
@@ -639,7 +639,24 @@ value serialized as-is. Code and design doc must be reconciled.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit `<pending>`): root cause confirmed — `ApiMethod` carries
`ReturnDefinition` but the executor did a blind `JsonSerializer.Serialize(result)`,
so a script returning the wrong shape silently emitted a malformed 200. Took
option (a): added `ReturnValueValidator`, the response-side mirror of
`ParameterValidator`. It parses `ReturnDefinition` (a JSON array of `{name,type}`
field definitions, same extended-type set as parameters), validates the serialized
script result against it — declared fields must be present with a compatible JSON
type, primitives type-checked, `Object`/`List` shape-checked — and a `null`/non-object
result is rejected when a structure is declared. `InboundScriptExecutor.ExecuteAsync`
now runs the validator after serialization and, on mismatch, logs and returns a
script failure (`"Method return value did not match its return definition"`, → 500)
instead of a malformed 200. A method with no `ReturnDefinition` stays unconstrained
(backward compatible). Doc-owner follow-up (outside this module's editable scope):
the `Component-InboundAPI.md` "Response Format" section may note that return shaping
is validation-only (no coercion). Regression tests: `ReturnValueValidatorTests`
(12 cases) plus executor-level `ReturnValue_MatchingReturnDefinition_Succeeds`,
`ReturnValue_NotMatchingReturnDefinition_ReturnsFailureNotMalformed200`, and
`ReturnValue_NoReturnDefinition_IsUnconstrained`.
### InboundAPI-015 — `ForbiddenApiChecker` is purely textual and is bypassable via reflection reachable without a forbidden namespace token
@@ -647,7 +664,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/ForbiddenApiChecker.cs:63-119`, `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:109-126` |
**Description**
@@ -691,7 +708,29 @@ that the current check is best-effort and does not stop a determined script.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit `<pending>`): the specific reflection-via-permitted-member
vector was confirmed and the textual checker materially hardened against it (full
sandboxing remains a separate, larger design effort — see below). `ForbiddenApiWalker`
now, in addition to the namespace deny-list, rejects a curated set of reflection-gateway
**member names** (`GetType`, `GetTypeInfo`, `Assembly`, `Module`, `CreateInstance`,
`InvokeMember`, `GetMethod(s)`, `GetConstructor(s)`, `GetField(s)`, `GetProperty(ies)`,
`GetMember(s)`, `GetRuntimeMethod(s)`, `MethodHandle`, `TypeHandle`) regardless of the
receiver expression — so `typeof(string).Assembly.GetType("System.IO.File")` is now
caught because `.Assembly` and `.GetType` appear as accessed member names. It also
rejects a bare `Activator` identifier and the `dynamic` keyword (which widens
late-bound access the static walker cannot see through). `Invoke` is deliberately
**not** flagged so legitimate `Action`/`Func` delegate invocation still compiles —
the reflection `MethodInfo.Invoke` path is cut off by rejecting the `GetMethod` that
produces the `MethodInfo`. **Documented limitation:** this is hardened defence-in-depth,
not a true sandbox — a determined author may still find a vector the syntax walker
cannot see (e.g. via `Microsoft.CSharp.RuntimeBinder` internals or generics tricks).
Genuine containment needs a runtime boundary (restricted `AssemblyLoadContext` /
curated reference set that does not expose reflection-to-arbitrary-type / out-of-process
sandbox); that is tracked as a future design change and noted in the `ForbiddenApiChecker`
XML summary. Regression tests: new `ForbiddenApiCheckerTests` suite (19 cases) covering
the `Assembly`/`GetType`/`Type.GetType`/`Activator.CreateInstance`/`InvokeMember`/
`GetMethod`/`GetTypeInfo`/`dynamic` bypass vectors plus permitted-script and
namespace-deny-list regression guards.
### InboundAPI-016 — Routed `Route.To().Call()` invocations are not bound by the method timeout
@@ -699,7 +738,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/RouteHelper.cs:59-152`, `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:177`, `:199` |
**Description**
@@ -738,7 +777,24 @@ and abandon the in-flight request when it fires.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit `<pending>`): root cause confirmed — every `RouteTarget`
method took `CancellationToken cancellationToken = default`, so a natural script
`Route.To("inst").Call("doWork", p)` routed with `CancellationToken.None` and was not
bound by the method timeout at all. `RouteHelper` now carries the executing method's
deadline token: `InboundScriptExecutor.ExecuteAsync` calls the new
`RouteHelper.WithDeadline(cts.Token)` when it builds the script context, so the route
helper handed to the script is bound to the method-level timeout CTS. Each
`RouteTarget` method resolves an *effective* token — the explicitly-supplied token if
the caller passed one (tighter bound preserved), otherwise the method deadline — and
forwards it into both `IInstanceLocator` site resolution and the routed call. The
deadline token therefore flows through to `CommunicationService.RouteTo*Async`, so
an in-flight routed call observes cancellation when the method timeout fires instead
of running orphaned. Regression tests (in the new `RouteHelperTests`):
`Call_WithNoExplicitToken_InheritsMethodDeadlineToken`,
`Call_WhenMethodDeadlineCancelled_RoutedCallObservesCancellation`,
`Call_ExplicitToken_OverridesDeadlineToken`,
`GetAttributes_WithNoExplicitToken_InheritsMethodDeadlineToken`,
`SetAttributes_WithNoExplicitToken_InheritsMethodDeadlineToken`.
### InboundAPI-017 — `RouteHelper` / `RouteTarget` has no test coverage
@@ -746,7 +802,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/RouteHelper.cs:1-165`, `tests/ScadaLink.InboundAPI.Tests/` |
**Description**
@@ -776,4 +832,15 @@ wiring is added.
**Resolution**
_Unresolved._
Resolved 2026-05-17 (commit `<pending>`): confirmed — `ScadaLink.InboundAPI.Tests` had
no file exercising `RouteHelper`/`RouteTarget`. To make the surface testable without a
live actor system, an `IInstanceRouter` seam was introduced in the module (the routing
transport `RouteHelper` depends on); the production `CommunicationServiceInstanceRouter`
delegates to `CommunicationService` and is registered by `AddInboundAPI`. `RouteHelper`
now depends on `IInstanceLocator` + `IInstanceRouter` (both substitutable). Added the
`RouteHelperTests` suite (15 cases) covering: the happy path of `Call`/`GetAttribute(s)`/
`SetAttribute(s)`, correlation-ID generation, the unresolved-instance
`InvalidOperationException` path, the `!Success` → `InvalidOperationException` mapping
for each routed method, `GetAttribute` delegating to the batch `GetAttributes` and
returning `null` for an absent key, `SetAttribute` delegating to `SetAttributes`, and
the InboundAPI-016 deadline-token inheritance behaviour. All 15 pass.