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

fix(inbound-api): resolve InboundAPI-009,010,011,013 — cache failed compiles, reject unknown body fields, close enumeration oracle, drop misnamed factory; InboundAPI-007,012 flagged

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 22:24:03 -04:00
parent 8664cdf940
commit 858fe24add
7 changed files with 255 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 6 |
| Open findings | 2 |
## Summary
@@ -386,7 +386,7 @@ actually enforced in production; until then the endpoint defaults to "allow".
|--|--|
| Severity | Low |
| Category | Performance & resource management |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:123-128` |
**Description**
@@ -406,7 +406,16 @@ updated via `CompileAndRegister`.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending): confirmed the root cause — a failed `Compile`
stored nothing in `_scriptHandlers`, so every subsequent request re-entered the
lazy-compile branch and re-ran Roslyn. Added a `_knownBadMethods` `ConcurrentDictionary`
of method names whose compilation failed; `ExecuteAsync`'s lazy-compile path
short-circuits before Roslyn when the method is already known-bad, and records the
failure on a fresh failed compile. `CompileAndRegister` also records failures and
clears the record on a successful (re)compile, so a fixed method definition is
re-evaluated. Regression tests `FailedCompilation_IsNotRetriedOnEveryRequest`
(asserts the compile-failure log fires exactly once across 5 requests) and
`FailedCompilation_RecompilesAfterCompileAndRegisterCalledAgain` added.
### InboundAPI-010 — `ParameterValidator` ignores extra body fields and cannot validate Object/List element types
@@ -414,7 +423,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/ParameterValidator.cs:64-90`, `:112-118` |
**Description**
@@ -438,7 +447,20 @@ shape — and update the design doc to match.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending): both gaps addressed along the lines the
recommendation offers. (1) `ParameterValidator.Validate` now enumerates the request
body's top-level fields and returns an Invalid result naming any field that does not
match a defined parameter (`"Unexpected parameter(s): ..."`), so a typo'd parameter
name is reported instead of silently ignored. (2) For `Object`/`List`, recursive
field/element-level type validation is **deliberately not** added — it requires a
richer nested `ParameterDefinition` schema, a design decision; instead the
shape-only behaviour is now explicitly documented in the `CoerceValue` XML comment so
the code's contract is unambiguous. Re-triage note: the design doc
(`Component-InboundAPI.md` line 43) lists only Boolean/Integer/Float/String as method
parameter types — the Object/List extended types are a CLAUDE.md decision; reconciling
the design doc is out of this module's editable scope and left as a doc-owner
follow-up. Regression tests `UnexpectedBodyField_ReturnsInvalid` and
`OnlyDefinedFields_StillValid` added.
### InboundAPI-011 — Method-existence check leaks to unapproved callers (enumeration oracle)
@@ -446,7 +468,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/ApiKeyValidator.cs:39-52` |
**Description**
@@ -466,7 +488,18 @@ raw caller input in error bodies, or sanitize it.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending): confirmed — `ValidateAsync` returned 400
`Method '{methodName}' not found` for a missing method but 403
`API key not approved for this method` for an existing-but-unapproved one, an
enumeration oracle, and echoed the caller-supplied method name verbatim. Both cases
now return an identical response: HTTP 403 with the single shared message
`API key not approved for this method` (a `NotApprovedMessage` constant); the
method name is no longer interpolated into any error body, removing both the
existence oracle and the reflected-input concern. Regression tests
`ValidKey_MethodNotFound_IsIndistinguishableFromNotApproved` and
`ValidKey_MethodNotFound_ErrorMessageDoesNotEchoMethodName` added; the pre-existing
`ValidKey_MethodNotFound_Returns400` test was updated to assert the new
indistinguishable contract.
### InboundAPI-012 — `ParameterDefinition` POCO declared in the component project, not Commons
@@ -495,7 +528,18 @@ components that work with method definitions.
**Resolution**
_Unresolved._
_Unresolved — left Open; the fix crosses this module's editable boundary._
Re-triage 2026-05-16: confirmed against the source — `ParameterDefinition`
(`ParameterValidator.cs:128-133`) is indeed an API-contract-shaped POCO declared in
the component project. However the recommended fix is to **create the type in
`ScadaLink.Commons`** (and update the validator plus any other consumers to reference
it), which edits files outside this module's editable scope
(`src/ScadaLink.InboundAPI`, `tests/`, this file only). It also touches a shared
contract type: a Commons namespace placement and a likely-paired return-definition
type are a cross-component code-organization decision. **Surface to the design/Commons
owner:** add `ParameterDefinition` (and a return-definition counterpart) under a
`ScadaLink.Commons` InboundApi types namespace, then repoint `ParameterValidator` and
any other consumers at it.
### InboundAPI-013 — `ApiKeyValidationResult.NotFound` factory returns HTTP 400, contradicting its name
@@ -503,7 +547,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/ApiKeyValidator.cs:78-79` |
**Description**
@@ -523,4 +567,14 @@ does not list it.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit pending): the misnamed `NotFound` factory (which built a
`StatusCode = 400` result) was the only producer of the "method not found" result,
and the InboundAPI-011 fix made "method not found" return 403 via the existing
`Forbidden` factory instead. The misleading `NotFound` factory is therefore now
**removed entirely** — it has no remaining callers in or out of the module
(`ApiKeyValidationResult` is InboundAPI-internal), eliminating the name/behaviour
contradiction. No separate regression test is needed: the InboundAPI-011 tests cover
the new method-not-found status, and removing dead code cannot regress. Doc-owner
follow-up: `Component-InboundAPI.md`'s Error Handling section still does not list a
"method not found" status; it should note that it is reported as 403 (indistinguishable
from "key not approved"), but that doc edit is outside this module's editable scope.