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

fix(inbound-api): resolve InboundAPI-001/003/005 — concurrent handler cache, constant-time API key compare, script trust-model enforcement

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 19:47:17 -04:00
parent d30ded7e72
commit 6f4efdfa2e
6 changed files with 393 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 13 |
| Open findings | 10 |
## Summary
@@ -53,7 +53,7 @@ are High severity and should be addressed before production use.
|--|--|
| Severity | High |
| Category | Concurrency & thread safety |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:17`, `:32`, `:40`, `:89`, `:123-128` |
**Description**
@@ -76,7 +76,12 @@ compile at most once.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): replaced the plain `Dictionary` handler
cache with a `ConcurrentDictionary`; `RemoveHandler` now uses `TryRemove`; the
lazy-compile path in `ExecuteAsync` compiles outside the cache and inserts atomically
via `GetOrAdd` so concurrent first-callers share one handler. Regression tests
`ConcurrentLazyCompile_SameMethod_DoesNotCorruptCache` and
`ConcurrentRegisterAndExecute_DoesNotThrow` added.
### InboundAPI-002 — Lazy compilation is a check-then-act race with no atomicity
@@ -114,7 +119,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs:22-23`, consumed by `src/ScadaLink.InboundAPI/ApiKeyValidator.cs:33` |
**Description**
@@ -138,7 +143,17 @@ match-position timing.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): `ApiKeyValidator` no longer calls the
secret-equality lookup `GetApiKeyByValueAsync` (the SQL `WHERE KeyValue = @secret`
timing oracle). It now fetches all keys via `GetAllApiKeysAsync` and matches the
secret in-process with `CryptographicOperations.FixedTimeEquals` over the UTF-8 bytes,
scanning every candidate so neither match position nor secret length is observable.
Regression tests `ValidateAsync_DoesNotUseSecretEqualityLookup`,
`ValidateAsync_WrongKey_ConstantTimePath_Returns401`, and
`ValidateAsync_KeyOfDifferentLength_Returns401` added. Note: the timing-oracle method
`GetApiKeyByValueAsync` remains on `IInboundApiRepository` (it is outside this module);
removing it from the repository is left as separate follow-up since the validator no
longer depends on it.
### InboundAPI-004 — Client disconnect is misreported as a script timeout
@@ -178,7 +193,7 @@ _Unresolved._
|--|--|
| Severity | High |
| Category | Security |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.InboundAPI/InboundScriptExecutor.cs:56-93` |
**Description**
@@ -205,7 +220,16 @@ forbidden-API checker so the trust model is enforced consistently. Reject the me
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `<pending>`): added `ForbiddenApiChecker`, a Roslyn
`CSharpSyntaxWalker` that statically rejects scripts referencing forbidden namespaces
(`System.IO`, `System.Diagnostics`, `System.Threading` except `Tasks`,
`System.Reflection`, `System.Net`, `System.Runtime.InteropServices`, `Microsoft.Win32`)
whether reached via a `using` directive or a fully-qualified name. `CompileAndRegister`
now runs the check before Roslyn compilation and refuses to register (and logs) a
violating method; `ExecuteAsync`'s lazy-compile path is gated by the same check.
Regression tests `CompileAndRegister_ForbiddenApi_RejectsScript` (theory),
`ExecuteAsync_ForbiddenApiScript_DoesNotRunAndReturnsFailure`, and
`CompileAndRegister_PermittedScript_StillRegisters` added.
### InboundAPI-006 — No request body size limit on the inbound endpoint