From 7ad187245455c464eaf4293f97f4d989aedc1bfb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 19 Aug 2026 16:15:33 -0400 Subject: [PATCH] fix(management): strict enum parsing for DataType/ElementDataType/AlarmTriggerType (P6-10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ManagementActor's ParseDataType/ParseElementDataType (bare Enum.TryParse) and the two AlarmTriggerType parse sites in HandleAddAlarm/HandleUpdateAlarm (raw Enum.Parse) all admitted ordinals ("2"), undefined values ("99" — a deploy-time bomb that saves fine and only detonates at deploy), and comma-joined name lists (bitwise-ORed together regardless of [Flags], e.g. "String,Int32" silently becoming DateType.DateTime). A typo against the raw Enum.Parse sites also surfaced as a masked "An internal error occurred" rather than a curated field-naming error. Adds ParseEnumStrict, mirroring the existing ParseDeploymentStatus guard (names only, defined values only, comma-token rejection per the Medium-wave T8 nuance / AuditQueryParamParsers.ParseEnumListStrict pattern), and routes all four call sites through it. Tests: 22 new ManagementActorTests cases per parse site (ordinal, undefined numeric, comma-joined, and typo all rejected with the curated field-naming error; every defined name still parses through to the service layer). Proof of failure recorded in each rejection test's doc comment; verified live by stashing the ManagementActor.cs fix — exactly the 14 new rejection tests fail pre-fix (13 acceptance tests pass unchanged), confirming the defect and the fix. --- .../ManagementActor.cs | 58 +++- .../ManagementActorTests.cs | 249 ++++++++++++++++++ 2 files changed, 297 insertions(+), 10 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 13b62ae0..6d5f84d1 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -2931,11 +2931,7 @@ public class ManagementActor : ReceiveActor /// be masked as a generic internal error. /// private static Commons.Types.Enums.DataType ParseDataType(string? dataType) - { - if (!Enum.TryParse(dataType, ignoreCase: true, out var parsed)) - throw new ManagementCommandException($"Unrecognised data type '{dataType}'."); - return parsed; - } + => ParseEnumStrict("data type", dataType); /// /// Parses an optional element data type token. Returns null when the token is @@ -2945,9 +2941,51 @@ public class ManagementActor : ReceiveActor private static Commons.Types.Enums.DataType? ParseElementDataType(string? elementDataType) { if (string.IsNullOrWhiteSpace(elementDataType)) return null; - if (!Enum.TryParse(elementDataType, ignoreCase: true, out var parsed)) - throw new ManagementCommandException($"Unrecognised element type '{elementDataType}'."); - return parsed; + return ParseEnumStrict("element type", elementDataType); + } + + /// + /// Parses a single required enum token by NAME ONLY, rejecting ordinals, + /// comma-joined name lists, and defined-but-out-of-range composites; throws a + /// curated naming the field and the + /// valid value set on any rejection. + /// + /// + /// Register row 88 pattern (see ). Plain + /// Enum.TryParse/Enum.Parse admits three traps a wire-contract + /// field must not tolerate: + /// + /// Ordinals. "2" parses to whatever member sits at that + /// position (e.g. DataType.Float) — a caller-facing field is a + /// name-based contract, never a positional one. + /// Undefined ordinals. "99" parses to an undefined enum + /// value that saves without error and only detonates at deploy time. + /// Comma-joined tokens. Enum.TryParse bitwise-ORs a + /// comma-separated string's pieces together regardless of whether the enum + /// is [Flags] — "String,Int32" (4|1) silently becomes + /// DateTime (5) for a dense 0-based enum. Any token containing a + /// comma is rejected outright, before TryParse ever sees it. + /// + /// Mirrors , + /// adapted to a single required value and . + /// + /// The value is not a defined member name of . + private static TEnum ParseEnumStrict(string fieldName, string? raw) + where TEnum : struct, Enum + { + var trimmed = raw?.Trim() ?? string.Empty; + var looksNumeric = trimmed.Length > 0 + && (char.IsAsciiDigit(trimmed[0]) || trimmed[0] == '+' || trimmed[0] == '-'); + if (!looksNumeric + && !trimmed.Contains(',') + && Enum.TryParse(trimmed, ignoreCase: true, out var parsed) + && Enum.IsDefined(parsed)) + { + return parsed; + } + + throw new ManagementCommandException( + $"Unrecognised {fieldName} '{raw}'. Valid values: {string.Join(", ", Enum.GetNames())}."); } /// @@ -3000,7 +3038,7 @@ public class ManagementActor : ReceiveActor var svc = sp.GetRequiredService(); var alarm = new TemplateAlarm(cmd.Name) { - TriggerType = Enum.Parse(cmd.TriggerType, ignoreCase: true), + TriggerType = ParseEnumStrict("alarm trigger type", cmd.TriggerType), PriorityLevel = cmd.PriorityLevel, Description = cmd.Description, TriggerConfiguration = cmd.TriggerConfiguration, @@ -3015,7 +3053,7 @@ public class ManagementActor : ReceiveActor var svc = sp.GetRequiredService(); var alarm = new TemplateAlarm(cmd.Name) { - TriggerType = Enum.Parse(cmd.TriggerType, ignoreCase: true), + TriggerType = ParseEnumStrict("alarm trigger type", cmd.TriggerType), PriorityLevel = cmd.PriorityLevel, Description = cmd.Description, TriggerConfiguration = cmd.TriggerConfiguration, diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 1fdf4d50..e7030b5d 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -765,6 +765,255 @@ public class ManagementActorTests : TestKit, IDisposable Assert.DoesNotContain("internal error", response.Error); } + // ======================================================================== + // P6-10 (register row-88 class): strict single-value enum parsing in + // ParseDataType / ParseElementDataType / the AlarmTriggerType parse sites. + // Raw Enum.Parse/TryParse admits ordinals ("2"), undefined values ("99" — + // a deploy-time bomb that saves fine and only detonates at deploy), and + // (Medium-wave T8 nuance) comma-joined name lists that TryParse/Parse + // bitwise-OR together regardless of [Flags]. ParseEnumStrict rejects all + // three, and — for AlarmTriggerType specifically — replaces a masked + // "An internal error occurred" (raw Enum.Parse's uncaught + // ArgumentException) with a curated, field-naming error. + // ======================================================================== + + [Theory] + [InlineData("2")] // ordinal for a DEFINED member (Float) + [InlineData("99")] // undefined ordinal — the deploy-time-bomb case + [InlineData("String,Int32")] // comma-joined names — TryParse ORs 4|1 into DateTime (5) + public void AddAttribute_WithNonNameDataTypeToken_ReturnsCuratedError(string token) + { + // PROOF OF FAILURE (pre-fix): the unguarded `Enum.TryParse` in the old + // ParseDataType has no numeric guard, no comma guard, and never checks + // Enum.IsDefined. Each of these tokens parsed successfully pre-fix (to + // Float, an undefined value, and DateTime respectively) and the command + // proceeded past the parser into TemplateService, failing later with + // "Template with ID 1 not found." instead of this curated message — + // verified by stashing this fix and re-running (see task report). + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAttributeCommand(1, "Whatever", token, null, null, null, false, null), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("Unrecognised data type", response.Error); + Assert.Contains(token, response.Error); + Assert.Contains("Valid values:", response.Error); + Assert.DoesNotContain("internal error", response.Error); + } + + [Fact] + public void AddAttribute_WithTypoDataType_ReturnsCuratedErrorNamingValidValues() + { + // The pre-fix ParseDataType already curated a plain typo (that part of + // MV-10 was correct), but its message named no valid set — this asserts + // the new "Valid values:" suffix, which is absent pre-fix. + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAttributeCommand(1, "Whatever", "Sting", null, null, null, false, null), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Unrecognised data type 'Sting'", response.Error); + Assert.Contains("Valid values:", response.Error); + Assert.Contains("Boolean", response.Error); + Assert.Contains("List", response.Error); + } + + [Theory] + [InlineData("Boolean")] + [InlineData("Int32")] + [InlineData("Float")] + [InlineData("Double")] + [InlineData("String")] + [InlineData("DateTime")] + [InlineData("Binary")] + public void AddAttribute_WithDefinedDataTypeName_ParsesAndProceedsPastGuard(string name) + { + // No repository stub: GetTemplateByIdAsync returns null (NSubstitute + // default), so a token the guard ACCEPTS reaches AddAttributeAsync's + // "Template not found" failure. A token the guard REJECTS would never + // reach the service at all (see the tests above), so this proves every + // defined name still parses. + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAttributeCommand(1, "Whatever", name, null, null, null, false, null), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Template with ID 1 not found", response.Error); + } + + [Fact] + public void AddAttribute_WithDefinedListDataTypeNameAndElementType_ParsesAndProceedsPastGuard() + { + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAttributeCommand(1, "Whatever", "List", null, null, null, false, "String"), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Template with ID 1 not found", response.Error); + } + + [Fact] + public void UpdateAttribute_WithCommaJoinedDataType_ReturnsCuratedError() + { + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new UpdateTemplateAttributeCommand(5, "Count", "String,Int32", null, null, null, false, null), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Unrecognised data type", response.Error); + Assert.Contains("Valid values:", response.Error); + } + + [Theory] + [InlineData("2")] // ordinal for a DEFINED, valid element type (Float) + [InlineData("99")] // undefined ordinal + [InlineData("String,Int32")] // comma-joined — combines to DateTime (also a valid element type) + public void AddListAttribute_WithNonNameElementTypeToken_ReturnsCuratedError(string token) + { + // PROOF OF FAILURE (pre-fix): each of these tokens parsed successfully + // (Float / an undefined value that fails a LATER, differently-worded + // "requires a valid element type" check / DateTime) rather than being + // rejected at the parse boundary with this message. + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAttributeCommand(1, "Tags", "List", null, null, null, false, token), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("Unrecognised element type", response.Error); + Assert.Contains(token, response.Error); + Assert.Contains("Valid values:", response.Error); + } + + [Fact] + public void AddListAttribute_WithTypoElementType_ReturnsCuratedErrorNamingValidValues() + { + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAttributeCommand(1, "Tags", "List", null, null, null, false, "Sting"), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Unrecognised element type 'Sting'", response.Error); + Assert.Contains("Valid values:", response.Error); + } + + [Theory] + [InlineData("0")] // ordinal for a DEFINED member (ValueMatch) + [InlineData("99")] // undefined ordinal — the deploy-time-bomb case + [InlineData("ValueMatch,RangeViolation")] // comma-joined — Enum.Parse ORs 0|1 into RangeViolation + public void AddAlarm_WithNonNameTriggerTypeToken_ReturnsCuratedError(string token) + { + // PROOF OF FAILURE (pre-fix): AddAlarm/UpdateAlarm built TriggerType via + // raw `Enum.Parse`, which has none of ParseDeploymentStatus's guards. + // Each of these tokens parsed SILENTLY (to ValueMatch, an undefined + // value, and RangeViolation respectively — no exception at all) and the + // command proceeded into TemplateService, failing later with + // "Template with ID 1 not found." instead of this curated message. + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAlarmCommand(1, "AlarmX", token, 5, null, null, false), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("Unrecognised alarm trigger type", response.Error); + Assert.Contains(token, response.Error); + Assert.Contains("Valid values:", response.Error); + } + + [Fact] + public void AddAlarm_WithTypoTriggerType_ReturnsCuratedErrorInsteadOfMaskedInternalError() + { + // PROOF OF FAILURE (pre-fix): unlike ParseDataType (already curated by + // MV-10), the TriggerType sites used raw `Enum.Parse`. A typo's + // ArgumentException is uncaught by any handler-local guard and reaches + // MapFault as an unanticipated fault — masked to a generic + // "An internal error occurred (CorrelationId=...)" message, exactly the + // masking this task's defect description calls out. Post-fix, + // ParseEnumStrict raises a curated ManagementCommandException instead. + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAlarmCommand(1, "AlarmX", "ValueMach", 5, null, null, false), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("Unrecognised alarm trigger type 'ValueMach'", response.Error); + Assert.Contains("Valid values:", response.Error); + Assert.DoesNotContain("internal error", response.Error); + } + + [Theory] + [InlineData("ValueMatch")] + [InlineData("RangeViolation")] + [InlineData("RateOfChange")] + [InlineData("HiLo")] + [InlineData("Expression")] + public void AddAlarm_WithDefinedTriggerTypeName_ParsesAndProceedsPastGuard(string name) + { + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new AddTemplateAlarmCommand(1, "AlarmX", name, 5, null, null, false), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Template with ID 1 not found", response.Error); + } + + [Fact] + public void UpdateAlarm_WithOrdinalTriggerType_ReturnsCuratedError() + { + _services.AddScoped(); + var actor = CreateActor(); + var envelope = Envelope( + new UpdateTemplateAlarmCommand(5, "AlarmX", "0", 5, null, null, false), + "Designer"); + + actor.Tell(envelope); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("Unrecognised alarm trigger type", response.Error); + Assert.Contains("Valid values:", response.Error); + } + [Fact] public void UpdateAttribute_ValidScalar_PersistsValueAndKeepsFixedFields() {