fix(commons): reject comma-joined enum tokens in strict audit query parsers (arch-review 2026-08-19 P6-3, register row 99 defect)
This commit is contained in:
@@ -79,7 +79,8 @@ public static class AuditQueryParamParsers
|
|||||||
/// the filter dimension stays unconstrained.
|
/// the filter dimension stays unconstrained.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// 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
|
||||||
/// <see cref="Enum.TryParse{TEnum}(string, bool, out TEnum)"/> does not:
|
/// <see cref="Enum.TryParse{TEnum}(string, bool, out TEnum)"/> does not:
|
||||||
/// <list type="number">
|
/// <list type="number">
|
||||||
/// <item><b>Numeric strings.</b> <c>TryParse</c> happily converts <c>"7"</c> —
|
/// <item><b>Numeric strings.</b> <c>TryParse</c> happily converts <c>"7"</c> —
|
||||||
@@ -87,6 +88,17 @@ public static class AuditQueryParamParsers
|
|||||||
/// is a caller-facing name-based contract, never an ordinal one, so any value
|
/// 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
|
/// 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.</item>
|
/// never start with those characters, so the check costs no legitimate input.</item>
|
||||||
|
/// <item><b>Comma-joined tokens.</b> <c>TryParse</c> splits ANY comma-separated
|
||||||
|
/// string on <c>,</c> and bitwise-ORs the pieces together — regardless of
|
||||||
|
/// whether <typeparamref name="TEnum"/> is a <c>[Flags]</c> enum. For a dense
|
||||||
|
/// 0-based enum like <c>AuditStatus</c> that composite almost always collides
|
||||||
|
/// with a real, but WRONG, member (<c>"Forwarded,Attempted"</c> = 1|2 = 3 =
|
||||||
|
/// <c>Delivered</c>), and it smuggles an ordinal past the numeric guard above
|
||||||
|
/// (<c>"Failed,7"</c> = 7 = <c>Skipped</c>). A single token containing <c>,</c>
|
||||||
|
/// is therefore rejected outright, before <c>TryParse</c> 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.</item>
|
||||||
/// <item><b>Undefined composites.</b> <see cref="Enum.IsDefined{TEnum}(TEnum)"/>
|
/// <item><b>Undefined composites.</b> <see cref="Enum.IsDefined{TEnum}(TEnum)"/>
|
||||||
/// catches anything that parsed to a value with no declared member.</item>
|
/// catches anything that parsed to a value with no declared member.</item>
|
||||||
/// </list>
|
/// </list>
|
||||||
@@ -95,7 +107,7 @@ public static class AuditQueryParamParsers
|
|||||||
/// <param name="rawValues">Raw query-parameter string values to parse; may be null.</param>
|
/// <param name="rawValues">Raw query-parameter string values to parse; may be null.</param>
|
||||||
/// <param name="parameterName">Query-parameter name, used in the error message (e.g. <c>"status"</c>).</param>
|
/// <param name="parameterName">Query-parameter name, used in the error message (e.g. <c>"status"</c>).</param>
|
||||||
/// <returns>A non-empty list of parsed values, or <c>null</c> if no values were supplied.</returns>
|
/// <returns>A non-empty list of parsed values, or <c>null</c> if no values were supplied.</returns>
|
||||||
/// <exception cref="AuditFilterParseException">A supplied value is not a member name of <typeparamref name="TEnum"/>.</exception>
|
/// <exception cref="AuditFilterParseException">A supplied value is not a member name of <typeparamref name="TEnum"/>, or a single token contains a comma.</exception>
|
||||||
public static IReadOnlyList<TEnum>? ParseEnumListStrict<TEnum>(
|
public static IReadOnlyList<TEnum>? ParseEnumListStrict<TEnum>(
|
||||||
IEnumerable<string?>? rawValues, string parameterName)
|
IEnumerable<string?>? rawValues, string parameterName)
|
||||||
where TEnum : struct, Enum
|
where TEnum : struct, Enum
|
||||||
@@ -115,6 +127,7 @@ public static class AuditQueryParamParsers
|
|||||||
|
|
||||||
var trimmed = raw.Trim();
|
var trimmed = raw.Trim();
|
||||||
if (LooksNumeric(trimmed)
|
if (LooksNumeric(trimmed)
|
||||||
|
|| trimmed.Contains(',')
|
||||||
|| !Enum.TryParse<TEnum>(trimmed, ignoreCase: true, out var value)
|
|| !Enum.TryParse<TEnum>(trimmed, ignoreCase: true, out var value)
|
||||||
|| !Enum.IsDefined(value))
|
|| !Enum.IsDefined(value))
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -145,6 +145,66 @@ public class AuditQueryParamParsersTests
|
|||||||
new[] { "", " ", null }, "status"));
|
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<AuditFilterParseException>(
|
||||||
|
() => AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(
|
||||||
|
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<AuditFilterParseException>(
|
||||||
|
() => AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(
|
||||||
|
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<AuditFilterParseException>(
|
||||||
|
() => AuditQueryParamParsers.ParseEnumListStrict<AuditStatus>(
|
||||||
|
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<AuditStatus>(
|
||||||
|
new[] { "Forwarded", "Attempted", "Submitted" }, "status");
|
||||||
|
Assert.Equal(
|
||||||
|
new[] { AuditStatus.Forwarded, AuditStatus.Attempted, AuditStatus.Submitted },
|
||||||
|
result);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void ParseStringList_NullInput_ReturnsNull()
|
public void ParseStringList_NullInput_ReturnsNull()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user