diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs index afbbec14..c4907680 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Audit/AuditQueryParamParsers.cs @@ -79,7 +79,8 @@ public static class AuditQueryParamParsers /// the filter dimension stays unconstrained. /// /// - /// Register row 99. Two traps this closes that a bare + /// Register row 99 (closure defect fixed by arch-review 2026-08-19 P6-3). Three + /// traps this closes that a bare /// does not: /// /// Numeric strings. TryParse happily converts "7" — @@ -87,6 +88,17 @@ public static class AuditQueryParamParsers /// is a caller-facing name-based contract, never an ordinal one, so any value /// starting with a digit or sign is rejected outright. Enum member names can /// never start with those characters, so the check costs no legitimate input. + /// Comma-joined tokens. TryParse splits ANY comma-separated + /// string on , and bitwise-ORs the pieces together — regardless of + /// whether is a [Flags] enum. For a dense + /// 0-based enum like AuditStatus that composite almost always collides + /// with a real, but WRONG, member ("Forwarded,Attempted" = 1|2 = 3 = + /// Delivered), and it smuggles an ordinal past the numeric guard above + /// ("Failed,7" = 7 = Skipped). A single token containing , + /// is therefore rejected outright, before TryParse ever sees it — a + /// value that legitimately arrives as multiple tokens (a repeated query + /// parameter, or an already-split list) is unaffected, since each of those + /// tokens is comma-free on its own. /// Undefined composites. /// catches anything that parsed to a value with no declared member. /// @@ -95,7 +107,7 @@ public static class AuditQueryParamParsers /// Raw query-parameter string values to parse; may be null. /// Query-parameter name, used in the error message (e.g. "status"). /// A non-empty list of parsed values, or null if no values were supplied. - /// A supplied value is not a member name of . + /// A supplied value is not a member name of , or a single token contains a comma. public static IReadOnlyList? ParseEnumListStrict( IEnumerable? rawValues, string parameterName) where TEnum : struct, Enum @@ -115,6 +127,7 @@ public static class AuditQueryParamParsers var trimmed = raw.Trim(); if (LooksNumeric(trimmed) + || trimmed.Contains(',') || !Enum.TryParse(trimmed, ignoreCase: true, out var value) || !Enum.IsDefined(value)) { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/AuditQueryParamParsersTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/AuditQueryParamParsersTests.cs index 6f7fc137..0201bff2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/AuditQueryParamParsersTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Types/AuditQueryParamParsersTests.cs @@ -145,6 +145,66 @@ public class AuditQueryParamParsersTests new[] { "", " ", null }, "status")); } + // ======================================================================== + // Comma-joined single token (arch-review 2026-08-19 P6-3, register row 99 + // closure defect): a bare Enum.TryParse splits and bitwise-ORs a comma list + // inside ONE token for ANY enum, not just [Flags] ones. AuditStatus is dense + // and 0-based, so the OR'd composite almost always collides with a REAL, but + // WRONG, member. Values arriving as separate tokens (repeated query param / + // already-split list) must be unaffected — only a comma INSIDE one token is + // rejected. Proof this closes the defect: reverting the fix (`git stash`) + // makes every one of these throw-expecting tests fail, because the call + // instead RETURNS the wrong composite value below. + // ======================================================================== + + [Fact] + public void ParseEnumListStrict_CommaJoinedTwoNames_IsRejected_NotSilentlyOrdedIntoWrongMember() + { + // Forwarded(1) | Attempted(2) = 3 = Delivered — the exact silent-wrong-data + // collision the finding calls out. Pre-fix this returned [Delivered]. + var ex = Assert.Throws( + () => AuditQueryParamParsers.ParseEnumListStrict( + new[] { "Forwarded,Attempted" }, "status")); + Assert.Contains("Forwarded,Attempted", ex.Message); + Assert.Contains("status", ex.Message); + } + + [Fact] + public void ParseEnumListStrict_CommaJoinedNameAndOrdinal_IsRejected_NotSmuggledPastNumericGuard() + { + // Failed(4) | 7 = 7 = Skipped — a comma smuggles an ordinal past the + // LooksNumeric guard because the guard only inspects the WHOLE token, and + // a comma-joined token starts with a letter. Pre-fix this returned [Skipped]. + var ex = Assert.Throws( + () => AuditQueryParamParsers.ParseEnumListStrict( + new[] { "Failed,7" }, "status")); + Assert.Contains("Failed,7", ex.Message); + } + + [Fact] + public void ParseEnumListStrict_CommaJoinedNamesDropAMember_IsRejected() + { + // Submitted(0) | Parked(5) = 5 = Parked — Submitted silently vanishes from + // the composite. Pre-fix this returned [Parked], dropping Submitted entirely. + var ex = Assert.Throws( + () => AuditQueryParamParsers.ParseEnumListStrict( + new[] { "Submitted,Parked" }, "status")); + Assert.Contains("Submitted,Parked", ex.Message); + } + + [Fact] + public void ParseEnumListStrict_RepeatedParamTokensOfValidNames_StillParseIndependently() + { + // The fix targets a comma INSIDE one token — values that legitimately + // arrive as separate tokens (a repeated query param, or an already-split + // list) must keep working exactly as before. + var result = AuditQueryParamParsers.ParseEnumListStrict( + new[] { "Forwarded", "Attempted", "Submitted" }, "status"); + Assert.Equal( + new[] { AuditStatus.Forwarded, AuditStatus.Attempted, AuditStatus.Submitted }, + result); + } + [Fact] public void ParseStringList_NullInput_ReturnsNull() {