diff --git a/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json b/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json index fc4a1f24..ee93e3e8 100644 --- a/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json +++ b/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json @@ -1,13 +1,13 @@ { "planPath": "docs/plans/2026-06-15-stillpending-m2-implementation.md", "tasks": [ - {"id": 32, "ref": "M2.0", "subject": "M2.0 #32: EF model/snapshot drift (PendingModelChangesWarning)", "class": "high-risk", "status": "pending"}, - {"id": 33, "ref": "M2.1", "subject": "M2.1 #22: native-alarm capability validation wired into deploy pipeline", "class": "standard", "status": "pending"}, - {"id": 34, "ref": "M2.2", "subject": "M2.2 #10: connection-level diff surfaced in deployment diff", "class": "standard", "status": "pending"}, - {"id": 35, "ref": "M2.3", "subject": "M2.3 #7: Database.CachedWrite transient/permanent SQL classification", "class": "high-risk", "status": "pending"}, - {"id": 36, "ref": "M2.4", "subject": "M2.4 #8: alarm conditionFilter applied (OPC UA WhereClause + client routing)", "class": "high-risk", "status": "pending"}, - {"id": 37, "ref": "M2.5", "subject": "M2.5 #9: per-script execution timeout (entity+migration+flatten+actor)", "class": "standard", "status": "pending", "blockedBy": [32]}, - {"id": 38, "ref": "M2.6", "subject": "M2.6 #13: nested Object/List extended-type validation", "class": "standard", "status": "pending"}, + {"id": 32, "ref": "M2.0", "subject": "M2.0 #32: EF model/snapshot drift (PendingModelChangesWarning)", "class": "high-risk", "status": "completed", "commits": ["2fb608f"]}, + {"id": 33, "ref": "M2.1", "subject": "M2.1 #22: native-alarm capability validation wired into deploy pipeline", "class": "standard", "status": "completed", "commits": ["d690920", "41d828e"]}, + {"id": 34, "ref": "M2.2", "subject": "M2.2 #10: connection-level diff surfaced in deployment diff", "class": "standard", "status": "completed", "commits": ["e9a84ba", "198770f"]}, + {"id": 35, "ref": "M2.3", "subject": "M2.3 #7: Database.CachedWrite transient/permanent SQL classification", "class": "high-risk", "status": "completed", "commits": ["d052706", "de375ff"]}, + {"id": 36, "ref": "M2.4", "subject": "M2.4 #8: alarm conditionFilter applied (OPC UA WhereClause + client routing)", "class": "high-risk", "status": "completed", "commits": ["8825df5", "00304a2"]}, + {"id": 37, "ref": "M2.5", "subject": "M2.5 #9: per-script execution timeout (entity+migration+flatten+actor)", "class": "standard", "status": "completed", "blockedBy": [32], "commits": ["3edef09", "3032faa"]}, + {"id": 38, "ref": "M2.6", "subject": "M2.6 #13: nested Object/List extended-type validation", "class": "standard", "status": "in_progress"}, {"id": 39, "ref": "M2.7", "subject": "M2.7 #20+#21: return-type + argument-type compatibility checks", "class": "standard", "status": "pending"}, {"id": 40, "ref": "M2.8", "subject": "M2.8 #23: binding-completeness Error + name-exists-at-site", "class": "standard", "status": "pending"}, {"id": 41, "ref": "M2.9", "subject": "M2.9 #17: MachineDataDb fail-fast (reverts Host-008)", "class": "small", "status": "pending"}, @@ -26,5 +26,10 @@ {"ref": "#16", "subject": "Transport stale-instance enumeration", "to": "M8 (Transport)"}, {"ref": "#19", "subject": "script started/completed events", "status": "done in M1.8"} ], + "followups": [ + {"id": 52, "subject": "Investigate 2 partition-purge E2E test failures (AuditLogPurgeActor/PartitionPurge)", "from": "M2.0", "status": "pending"}, + {"id": 53, "subject": "Dedup alarm-capable protocol predicate (3 copies → AlarmCapableProtocols)", "from": "M2.1", "status": "pending"}, + {"id": 54, "subject": "Expose ExecutionTimeoutSeconds (+ MinTimeBetweenRuns) in CLI + UI script authoring", "from": "M2.5", "status": "pending"} + ], "lastUpdated": "2026-06-15" } diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/InboundApi/InboundApiSchema.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/InboundApi/InboundApiSchema.cs index d797d41d..5ee21547 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/InboundApi/InboundApiSchema.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/InboundApi/InboundApiSchema.cs @@ -39,7 +39,15 @@ public sealed class InboundApiSchema /// For = array: the schema every element must satisfy; null means element type was not declared (shape-only). public InboundApiSchema? Items { get; init; } - private static readonly JsonDocumentOptions DocOptions = default; + /// Maximum allowed schema nesting depth for both Parse and Validate recursion. + private const int MaxDepth = 32; + + // Allow the JSON reader to parse schemas up to ~3× our structural ceiling so + // the application-level ParseSchema depth guard (MaxDepth = 32) fires before + // the System.Text.Json reader ceiling. Each structural level contributes + // roughly 3 JSON-reader nesting levels (object → properties-object → value), + // so 128 reader levels comfortably accommodates 32+ structural levels. + private static readonly JsonDocumentOptions DocOptions = new() { MaxDepth = 128 }; /// /// Parses a stored definition string into an . @@ -51,7 +59,7 @@ public sealed class InboundApiSchema /// /// The definition JSON; null/whitespace yields null. /// The parsed schema, or null when the input is empty. - /// The input is non-empty but not valid JSON, or is a JSON scalar/null at the root. + /// The input is non-empty but not valid JSON, is a JSON scalar/null at the root, or the schema nesting exceeds . public static InboundApiSchema? Parse(string? json) { if (string.IsNullOrWhiteSpace(json)) @@ -62,14 +70,19 @@ public sealed class InboundApiSchema using var doc = JsonDocument.Parse(json, DocOptions); return doc.RootElement.ValueKind switch { - JsonValueKind.Object => ParseSchema(doc.RootElement), + JsonValueKind.Object => ParseSchema(doc.RootElement, depth: 0), JsonValueKind.Array => ParseLegacyArray(doc.RootElement), _ => throw new JsonException("Type definition must be a JSON object (JSON Schema) or legacy parameter array."), }; } - private static InboundApiSchema ParseSchema(JsonElement el) + private static InboundApiSchema ParseSchema(JsonElement el, int depth) { + if (depth > MaxDepth) + { + throw new JsonException($"Schema nesting exceeds the maximum allowed depth of {MaxDepth}."); + } + var type = el.TryGetProperty("type", out var t) && t.ValueKind == JsonValueKind.String ? NormalizeType(t.GetString()) : "string"; @@ -79,7 +92,7 @@ public sealed class InboundApiSchema InboundApiSchema? items = null; if (el.TryGetProperty("items", out var itemsEl) && itemsEl.ValueKind == JsonValueKind.Object) { - items = ParseSchema(itemsEl); + items = ParseSchema(itemsEl, depth + 1); } return new InboundApiSchema { Type = "array", Items = items }; @@ -109,7 +122,7 @@ public sealed class InboundApiSchema foreach (var prop in props.EnumerateObject()) { var schema = prop.Value.ValueKind == JsonValueKind.Object - ? ParseSchema(prop.Value) + ? ParseSchema(prop.Value, depth + 1) : new InboundApiSchema { Type = "string" }; fields.Add(new InboundApiSchemaField(prop.Name, requiredSet.Contains(prop.Name), schema)); } @@ -142,7 +155,18 @@ public sealed class InboundApiSchema } var rawType = TryGetMember(item, "type", out var t) ? t.GetString() : "string"; - var required = !TryGetMember(item, "required", out var rq) || rq.ValueKind != JsonValueKind.False; + + // A field is optional only when "required" is explicitly false. + // The SQL migration uses a string comparison (LOWER(...) <> 'false'), + // so we must also accept the string "false" (case-insensitive) here — + // not only the JSON boolean false — to stay consistent with legacy rows + // that stored "required":"false" as a string. + var required = !TryGetMember(item, "required", out var rq) + || (rq.ValueKind != JsonValueKind.False + && !string.Equals( + rq.ValueKind == JsonValueKind.String ? rq.GetString() : null, + "false", + StringComparison.OrdinalIgnoreCase)); var normalized = NormalizeType(rawType); InboundApiSchema schema; @@ -198,6 +222,9 @@ public sealed class InboundApiSchema "boolean" or "bool" => "boolean", "integer" or "int" or "int32" or "int64" => "integer", "number" or "float" or "double" or "decimal" => "number", + // datetime→string is intentional: the legacy migration's SQL + // normalization function maps "datetime" to "string" (no separate + // datetime wire type in the extended type system), so C# must match. "string" or "datetime" => "string", "object" => "object", "array" or "list" => "array", @@ -215,9 +242,18 @@ public sealed class InboundApiSchema /// The path prefix for the value being validated (empty for the root). /// Accumulator the validator appends path-qualified messages to. public void Validate(JsonElement value, string path, List errors) + => ValidateCore(value, path, errors, depth: 0); + + private void ValidateCore(JsonElement value, string path, List errors, int depth) { ArgumentNullException.ThrowIfNull(errors); + if (depth > MaxDepth) + { + errors.Add($"{Describe(path)}: schema nesting too deep (max {MaxDepth})"); + return; + } + // A null value satisfies any declared type — a present-but-null field is // allowed; a MISSING required field is reported by the enclosing object. if (value.ValueKind == JsonValueKind.Null) @@ -260,11 +296,11 @@ public sealed class InboundApiSchema break; case "object": - ValidateObject(value, path, errors); + ValidateObject(value, path, errors, depth); break; case "array": - ValidateArray(value, path, errors); + ValidateArray(value, path, errors, depth); break; default: @@ -273,7 +309,7 @@ public sealed class InboundApiSchema } } - private void ValidateObject(JsonElement value, string path, List errors) + private void ValidateObject(JsonElement value, string path, List errors, int depth) { if (value.ValueKind != JsonValueKind.Object) { @@ -303,7 +339,7 @@ public sealed class InboundApiSchema var fieldPath = JoinField(path, field.Name); if (value.TryGetProperty(field.Name, out var fieldValue)) { - field.Schema.Validate(fieldValue, fieldPath, errors); + field.Schema.ValidateCore(fieldValue, fieldPath, errors, depth + 1); } else if (field.Required) { @@ -312,7 +348,7 @@ public sealed class InboundApiSchema } } - private void ValidateArray(JsonElement value, string path, List errors) + private void ValidateArray(JsonElement value, string path, List errors, int depth) { if (value.ValueKind != JsonValueKind.Array) { @@ -329,7 +365,7 @@ public sealed class InboundApiSchema var index = 0; foreach (var element in value.EnumerateArray()) { - Items.Validate(element, $"{path}[{index}]", errors); + Items.ValidateCore(element, $"{path}[{index}]", errors, depth + 1); index++; } } diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ReturnValueValidator.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ReturnValueValidator.cs index 62acc5b9..03995c96 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ReturnValueValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ReturnValueValidator.cs @@ -63,6 +63,18 @@ public static class ReturnValueValidator return ReturnValidationResult.Valid(); } + // INTENTIONAL asymmetry with ParameterValidator: + // + // ParameterValidator has an early-return guard for "schema.Type != object" + // because method parameters are ALWAYS a top-level JSON object (flat map of + // name→value); a non-object parameter schema is treated as unconstrained. + // + // ReturnValueValidator does NOT guard on schema.Type here. A method may + // declare a scalar return type (e.g. {"type":"string"} or {"type":"integer"}) + // and the script is expected to return exactly that scalar JSON value. + // Guarding on type == "object" would silently bypass validation for scalar + // and array return schemas — do NOT add that guard here. + if (string.IsNullOrWhiteSpace(resultJson)) { return ReturnValidationResult.Invalid( diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ParameterValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ParameterValidatorTests.cs index e3c63747..de384994 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ParameterValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ParameterValidatorTests.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using ZB.MOM.WW.ScadaBridge.Commons.Types.InboundApi; namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; @@ -364,4 +365,76 @@ public class ParameterValidatorTests Assert.False(bad.IsValid); Assert.Contains("'count'", bad.ErrorMessage); } + + // FIX 1: legacy "required":"false" string → field is optional ───────────── + + [Theory] + [InlineData("""[{"name":"opt","type":"String","required":"false"}]""")] + [InlineData("""[{"name":"opt","type":"String","required":"False"}]""")] + [InlineData("""[{"name":"opt","type":"String","required":"FALSE"}]""")] + public void LegacyFlatArray_RequiredStringFalse_FieldIsOptional(string def) + { + // An absent field whose "required" is the string "false" (any case) + // must be treated as optional — consistent with the SQL migration's + // LOWER(...) <> 'false' comparison that produced these rows. + var result = ParameterValidator.Validate(null, def); + Assert.True(result.IsValid, $"Expected optional field to be valid when absent; error: {result.ErrorMessage}"); + } + + [Fact] + public void LegacyFlatArray_RequiredStringFalse_FieldPresentAndTypedCorrectly_Passes() + { + const string def = """[{"name":"opt","type":"String","required":"false"}]"""; + + var result = ParameterValidator.Validate(Body("{\"opt\":\"hello\"}"), def); + Assert.True(result.IsValid); + } + + // FIX 2: recursion depth guard on Parse ─────────────────────────────────── + + /// + /// Builds a JSON Schema string with levels of nested + /// object-in-properties nesting. Each level wraps the previous in an object + /// with a single property "a". The result exceeds the Parse ceiling when + /// depth > 32. + /// + private static string BuildDeeplyNestedSchema(int depth) + { + // Inner-most: a scalar + var schema = "{\"type\":\"string\"}"; + for (var i = 0; i < depth; i++) + { + schema = "{\"type\":\"object\",\"properties\":{\"a\":" + schema + "}}"; + } + return schema; + } + + [Fact] + public void SchemaAtDepthCeiling_ParsesSuccessfully() + { + // Exactly 32 levels of nesting should parse without throwing. + var def = BuildDeeplyNestedSchema(32); + var schema = InboundApiSchema.Parse(def); + Assert.NotNull(schema); + } + + [Fact] + public void SchemaExceedingDepthCeiling_ThrowsJsonException_NotStackOverflow() + { + // 33 levels exceeds the ceiling → JsonException (clean 400 via the + // caller's try/catch), NOT a StackOverflowException. + var def = BuildDeeplyNestedSchema(33); + Assert.Throws(() => InboundApiSchema.Parse(def)); + } + + [Fact] + public void SchemaExceedingDepthCeiling_ParameterValidator_ReturnsInvalid() + { + // End-to-end: ParameterValidator wraps Parse in try/catch(JsonException) + // → the caller gets Invalid rather than an unhandled exception. + var def = BuildDeeplyNestedSchema(33); + var result = ParameterValidator.Validate(Body("{\"a\":\"x\"}"), def); + Assert.False(result.IsValid); + Assert.Contains("Invalid parameter definitions", result.ErrorMessage); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ReturnValueValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ReturnValueValidatorTests.cs index eed2b7fe..cacd1dd3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ReturnValueValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ReturnValueValidatorTests.cs @@ -1,3 +1,5 @@ +using ZB.MOM.WW.ScadaBridge.Commons.Types.InboundApi; + namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; /// @@ -304,4 +306,92 @@ public class ReturnValueValidatorTests Assert.False(bad.IsValid); Assert.Contains("totalUnits", bad.ErrorMessage); } + + // FIX 3: scalar return schema validates scalar return values ────────────── + // (Guards the intentional ParameterValidator/ReturnValueValidator asymmetry: + // ReturnValueValidator must NOT short-circuit on non-object schema types.) + + [Fact] + public void ScalarStringReturnSchema_ValidatesScalarStringReturn() + { + // A {"type":"string"} return schema must accept a bare JSON string. + var result = ReturnValueValidator.Validate("\"hello\"", """{"type":"string"}"""); + Assert.True(result.IsValid); + } + + [Fact] + public void ScalarIntegerReturnSchema_ValidatesScalarIntegerReturn() + { + var result = ReturnValueValidator.Validate("42", """{"type":"integer"}"""); + Assert.True(result.IsValid); + } + + [Fact] + public void ScalarStringReturnSchema_RejectsIntegerReturn() + { + var result = ReturnValueValidator.Validate("42", """{"type":"string"}"""); + Assert.False(result.IsValid); + Assert.Contains("String", result.ErrorMessage); + } + + [Fact] + public void ScalarBooleanReturnSchema_ValidatesBooleanReturn() + { + var result = ReturnValueValidator.Validate("true", """{"type":"boolean"}"""); + Assert.True(result.IsValid); + } + + // FIX 2: recursion depth guard on Validate ───────────────────────────────── + + [Fact] + public void ValidateExceedingDepthCeiling_AddsDepthError_DoesNotThrow() + { + // Build a schema programmatically (bypassing Parse) with 34 levels of + // nesting to exceed the ceiling of 32. Validate must add an error and + // return, NOT stack overflow. + // + // Parse prevents creating a >32-level schema from stored JSON, but + // InboundApiSchema is a public type constructable in code, so Validate + // must guard independently. + var deepSchema = BuildProgrammaticSchema(34); + + var json = BuildDeeplyNestedValue(34); + using var doc = System.Text.Json.JsonDocument.Parse(json); + + var errors = new System.Collections.Generic.List(); + // Must not throw — adds a depth error to the list instead. + deepSchema.Validate(doc.RootElement, string.Empty, errors); + + Assert.NotEmpty(errors); + Assert.Contains("nesting too deep", errors[0], StringComparison.OrdinalIgnoreCase); + } + + /// + /// Constructs an with + /// levels of object-nesting programmatically (bypassing Parse) to + /// exercise the Validate depth ceiling independently of the Parse ceiling. + /// + private static InboundApiSchema BuildProgrammaticSchema(int depth) + { + InboundApiSchema inner = new() { Type = "string" }; + for (var i = 0; i < depth; i++) + { + inner = new InboundApiSchema + { + Type = "object", + Fields = [new InboundApiSchemaField("a", Required: false, inner)], + }; + } + return inner; + } + + private static string BuildDeeplyNestedValue(int depth) + { + var value = "\"leaf\""; + for (var i = 0; i < depth; i++) + { + value = "{\"a\":" + value + "}"; + } + return value; + } }