diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 18f8bc4..8e7ffa7 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 | 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. diff --git a/src/ScadaLink.InboundAPI/ApiKeyValidator.cs b/src/ScadaLink.InboundAPI/ApiKeyValidator.cs index ec34028..f13715f 100644 --- a/src/ScadaLink.InboundAPI/ApiKeyValidator.cs +++ b/src/ScadaLink.InboundAPI/ApiKeyValidator.cs @@ -13,6 +13,10 @@ public class ApiKeyValidator { private readonly IInboundApiRepository _repository; + // InboundAPI-011: the single message used for both "method not found" and + // "key not approved" so the two outcomes are indistinguishable to the caller. + private const string NotApprovedMessage = "API key not approved for this method"; + public ApiKeyValidator(IInboundApiRepository repository) { _repository = repository; @@ -45,10 +49,15 @@ public class ApiKeyValidator return ApiKeyValidationResult.Unauthorized("Invalid or disabled API key"); } + // InboundAPI-011: "method not found" and "key not approved" must produce an + // indistinguishable response. Otherwise a caller holding any valid key could + // enumerate which method names exist by observing the status/message + // difference. Both cases return 403 with the identical message below, and the + // caller-supplied method name is never echoed back into the response. var method = await _repository.GetMethodByNameAsync(methodName, cancellationToken); if (method == null) { - return ApiKeyValidationResult.NotFound($"Method '{methodName}' not found"); + return ApiKeyValidationResult.Forbidden(NotApprovedMessage); } // Check if this key is approved for the method @@ -57,7 +66,7 @@ public class ApiKeyValidator if (!isApproved) { - return ApiKeyValidationResult.Forbidden("API key not approved for this method"); + return ApiKeyValidationResult.Forbidden(NotApprovedMessage); } return ApiKeyValidationResult.Valid(apiKey, method); @@ -108,7 +117,4 @@ public class ApiKeyValidationResult public static ApiKeyValidationResult Forbidden(string message) => new() { IsValid = false, StatusCode = 403, ErrorMessage = message }; - - public static ApiKeyValidationResult NotFound(string message) => - new() { IsValid = false, StatusCode = 400, ErrorMessage = message }; } diff --git a/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs b/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs index 094c482..43ba800 100644 --- a/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs +++ b/src/ScadaLink.InboundAPI/InboundScriptExecutor.cs @@ -21,6 +21,13 @@ public class InboundScriptExecutor // not safe for concurrent read/write, so a ConcurrentDictionary is used throughout. private readonly ConcurrentDictionary>> _scriptHandlers = new(); + // InboundAPI-009: a script that fails to compile (or violates the trust model) + // is recorded here so it is compiled at most once. Without this, every subsequent + // request for a broken method re-runs the expensive Roslyn compilation — a CPU + // amplification vector since the inbound API has no rate limiting. The entry is + // cleared whenever the method is (re)compiled via CompileAndRegister. + private readonly ConcurrentDictionary _knownBadMethods = new(); + private readonly IServiceProvider _serviceProvider; public InboundScriptExecutor(ILogger logger, IServiceProvider serviceProvider) @@ -50,7 +57,21 @@ public class InboundScriptExecutor /// script is empty, fails Roslyn compilation, or violates the script trust model. /// public bool CompileAndRegister(ApiMethod method) - => Compile(method) is { } handler && Register(method.Name, handler); + { + var handler = Compile(method); + if (handler == null) + { + // InboundAPI-009: record the failure so the lazy-compile path does not + // keep recompiling a broken script on every request. + _knownBadMethods[method.Name] = 0; + return false; + } + + // The method definition was (re)compiled successfully — drop any stale + // failure record so a fixed script is no longer treated as bad. + _knownBadMethods.TryRemove(method.Name, out _); + return Register(method.Name, handler); + } private bool Register(string methodName, Func> handler) { @@ -157,12 +178,21 @@ public class InboundScriptExecutor if (!_scriptHandlers.TryGetValue(method.Name, out var handler)) { + // InboundAPI-009: a method already known to fail compilation must not + // be recompiled on every request — short-circuit before Roslyn runs. + if (_knownBadMethods.ContainsKey(method.Name)) + return new InboundScriptResult(false, null, "Script compilation failed for this method"); + // Lazy compile on first request (handles methods created after startup). // Compile outside the cache so a failed compile is not stored, then add // atomically so concurrent first-callers share a single handler instance. var compiled = Compile(method); if (compiled == null) + { + // Cache the failure so the next request short-circuits above. + _knownBadMethods[method.Name] = 0; return new InboundScriptResult(false, null, "Script compilation failed for this method"); + } handler = _scriptHandlers.GetOrAdd(method.Name, compiled); } diff --git a/src/ScadaLink.InboundAPI/ParameterValidator.cs b/src/ScadaLink.InboundAPI/ParameterValidator.cs index df7ed80..dc6269c 100644 --- a/src/ScadaLink.InboundAPI/ParameterValidator.cs +++ b/src/ScadaLink.InboundAPI/ParameterValidator.cs @@ -61,6 +61,19 @@ public static class ParameterValidator var result = new Dictionary(); var errors = new List(); + // InboundAPI-010: report top-level body fields that do not match any defined + // parameter, so a caller learns about a typo'd parameter name instead of + // having the field silently ignored. + var defined = new HashSet(definitions.Select(d => d.Name), StringComparer.Ordinal); + var unexpected = body.Value.EnumerateObject() + .Select(p => p.Name) + .Where(name => !defined.Contains(name)) + .ToList(); + if (unexpected.Count > 0) + { + errors.Add($"Unexpected parameter(s): {string.Join(", ", unexpected)}"); + } + foreach (var def in definitions) { if (body.Value.TryGetProperty(def.Name, out var prop)) @@ -89,6 +102,13 @@ public static class ParameterValidator return ParameterValidationResult.Valid(result); } + /// + /// Coerces a JSON element to the declared parameter type. InboundAPI-010: the + /// Object and List extended types are validated for JSON shape + /// only (object vs. array) — there is no field-level or element-level type + /// validation. A method script that needs a specific nested structure must + /// validate it itself; invalid nested data surfaces as a runtime script error. + /// private static (object? value, string? error) CoerceValue(JsonElement element, string expectedType, string paramName) { return expectedType.ToLowerInvariant() switch diff --git a/tests/ScadaLink.InboundAPI.Tests/ApiKeyValidatorTests.cs b/tests/ScadaLink.InboundAPI.Tests/ApiKeyValidatorTests.cs index be25ccc..784eaea 100644 --- a/tests/ScadaLink.InboundAPI.Tests/ApiKeyValidatorTests.cs +++ b/tests/ScadaLink.InboundAPI.Tests/ApiKeyValidatorTests.cs @@ -56,15 +56,45 @@ public class ApiKeyValidatorTests } [Fact] - public async Task ValidKey_MethodNotFound_Returns400() + public async Task ValidKey_MethodNotFound_IsIndistinguishableFromNotApproved() { + // InboundAPI-011: a "method not found" response must not be observably + // different from a "key not approved" response, or a caller holding any + // valid key could enumerate which method names exist on the central API. var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true }; + var method = new ApiMethod("realMethod", "return 1;") { Id = 10 }; + _repository.GetAllApiKeysAsync().Returns(new List { key }); _repository.GetMethodByNameAsync("nonExistent").Returns((ApiMethod?)null); + _repository.GetMethodByNameAsync("realMethod").Returns(method); + _repository.GetApprovedKeysForMethodAsync(10).Returns(new List()); + + var notFound = await _validator.ValidateAsync("valid-key", "nonExistent"); + var notApproved = await _validator.ValidateAsync("valid-key", "realMethod"); + + Assert.False(notFound.IsValid); + Assert.False(notApproved.IsValid); + // Status code and error message must be identical so existence is not observable. + Assert.Equal(notApproved.StatusCode, notFound.StatusCode); + Assert.Equal(notApproved.ErrorMessage, notFound.ErrorMessage); + Assert.Equal(403, notFound.StatusCode); + } + + [Fact] + public async Task ValidKey_MethodNotFound_ErrorMessageDoesNotEchoMethodName() + { + // InboundAPI-011: the error body must not echo the caller-supplied method + // name back verbatim (reflected-input) and must not confirm non-existence. + var key = new ApiKey("test", "valid-key") { Id = 1, IsEnabled = true }; + _repository.GetAllApiKeysAsync().Returns(new List { key }); + _repository.GetMethodByNameAsync("probe-XYZ").Returns((ApiMethod?)null); + + var result = await _validator.ValidateAsync("valid-key", "probe-XYZ"); - var result = await _validator.ValidateAsync("valid-key", "nonExistent"); Assert.False(result.IsValid); - Assert.Equal(400, result.StatusCode); + Assert.DoesNotContain("probe-XYZ", result.ErrorMessage ?? string.Empty); + Assert.DoesNotContain("not found", result.ErrorMessage ?? string.Empty, + StringComparison.OrdinalIgnoreCase); } [Fact] diff --git a/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs index e564ae5..3dbf375 100644 --- a/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ScadaLink.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using ScadaLink.Commons.Entities.InboundApi; @@ -323,4 +324,63 @@ public class InboundScriptExecutorTests Assert.False(result.Success); Assert.Contains("timed out", result.ErrorMessage); } + + // --- InboundAPI-009: a script that fails to compile must be compiled at most + // once — repeated calls must not re-run the expensive Roslyn compilation. --- + + [Fact] + public async Task FailedCompilation_IsNotRetriedOnEveryRequest() + { + // A broken script compiled once must be remembered as bad: subsequent + // ExecuteAsync calls must NOT recompile (CPU amplification vector — there is + // no rate limiting on the inbound API). Compilation is observed via the + // "compilation failed" log line, which must appear exactly once. + var counter = new CompileLogCounter(); + var executor = new InboundScriptExecutor( + new CountingLogger(counter), + Substitute.For()); + + var method = new ApiMethod("broken", "%%% invalid C# %%%") { Id = 1, TimeoutSeconds = 10 }; + + for (var i = 0; i < 5; i++) + { + var result = await executor.ExecuteAsync( + method, new Dictionary(), _route, TimeSpan.FromSeconds(10)); + Assert.False(result.Success); + } + + Assert.Equal(1, counter.CompilationFailures); + } + + [Fact] + public void FailedCompilation_RecompilesAfterCompileAndRegisterCalledAgain() + { + // The failure cache must not be permanent: when the method definition is + // updated via CompileAndRegister, a now-valid script must register. + var bad = new ApiMethod("fixable", "%%% invalid %%%") { Id = 1, TimeoutSeconds = 10 }; + Assert.False(_executor.CompileAndRegister(bad)); + + var good = new ApiMethod("fixable", "return 1;") { Id = 1, TimeoutSeconds = 10 }; + Assert.True(_executor.CompileAndRegister(good)); + } + + private sealed class CompileLogCounter + { + public int CompilationFailures; + } + + private sealed class CountingLogger(CompileLogCounter counter) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + var message = formatter(state, exception); + if (message.Contains("script compilation failed", StringComparison.OrdinalIgnoreCase)) + Interlocked.Increment(ref counter.CompilationFailures); + } + } } diff --git a/tests/ScadaLink.InboundAPI.Tests/ParameterValidatorTests.cs b/tests/ScadaLink.InboundAPI.Tests/ParameterValidatorTests.cs index 35972dd..0e758de 100644 --- a/tests/ScadaLink.InboundAPI.Tests/ParameterValidatorTests.cs +++ b/tests/ScadaLink.InboundAPI.Tests/ParameterValidatorTests.cs @@ -134,4 +134,40 @@ public class ParameterValidatorTests Assert.False(result.IsValid); Assert.Contains("Unknown parameter type", result.ErrorMessage); } + + // --- InboundAPI-010: unexpected top-level body fields must be reported so + // callers get feedback on typo'd parameter names instead of silent ignore. --- + + [Fact] + public void UnexpectedBodyField_ReturnsInvalid() + { + var definitions = JsonSerializer.Serialize(new[] + { + new { Name = "value", Type = "Integer", Required = true } + }); + + // "valeu" is a typo for "value"; the caller must be told, not ignored. + using var doc = JsonDocument.Parse("{\"value\": 1, \"valeu\": 2}"); + var result = ParameterValidator.Validate(doc.RootElement.Clone(), definitions); + + Assert.False(result.IsValid); + Assert.Contains("valeu", result.ErrorMessage); + } + + [Fact] + public void OnlyDefinedFields_StillValid() + { + // Regression guard: a body containing exactly the defined parameters + // must continue to validate. + var definitions = JsonSerializer.Serialize(new[] + { + new { Name = "value", Type = "Integer", Required = true } + }); + + using var doc = JsonDocument.Parse("{\"value\": 1}"); + var result = ParameterValidator.Validate(doc.RootElement.Clone(), definitions); + + Assert.True(result.IsValid); + Assert.Equal((long)1, result.Parameters["value"]); + } }