From 0c9da188e5f3620e00b5529730e0571c98e44497 Mon Sep 17 00:00:00 2001 From: Ramon Smits Date: Tue, 19 May 2026 09:51:10 +0200 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20Add=20failing=20test:=20Sort?= =?UTF-8?q?InfoModelBinder=20drops=20time=5Fof=5Ffailure/modified?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Primary instance global SortInfoModelBinder allowlist is missing the 'time_of_failure' and 'modified' sort keys. When ServicePulse requests the failed-message list sorted by Time of failure (sort=time_of_failure), the binder silently rewrites the token to 'time_sent', so the RavenDB layer — which does support TimeOfFailure ordering — never sees it. The list ends up ordered by Time sent regardless of the user's choice. This test reproduces the defect at the binder level (RED). --- .../WebApi/SortInfoModelBinderTests.cs | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs diff --git a/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs b/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs new file mode 100644 index 0000000000..01a40a5be9 --- /dev/null +++ b/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs @@ -0,0 +1,52 @@ +namespace ServiceControl.UnitTests.Infrastructure.WebApi +{ + using System.Collections.Generic; + using System.Threading.Tasks; + using Microsoft.AspNetCore.Http; + using Microsoft.AspNetCore.Mvc; + using Microsoft.AspNetCore.Mvc.ModelBinding; + using Microsoft.Extensions.Primitives; + using NUnit.Framework; + using Persistence.Infrastructure; + using ServiceControl.Infrastructure.WebApi; + + [TestFixture] + public class SortInfoModelBinderTests + { + // Failed-message endpoints (/api/errors) support these sort fields in the + // RavenDB layer. The model binder must not silently rewrite them away. + [TestCase("time_of_failure")] + [TestCase("modified")] + public async Task Preserves_valid_failed_message_sort_field(string sort) + { + var sortInfo = await Bind(sort, "asc"); + + Assert.That(sortInfo.Sort, Is.EqualTo(sort)); + } + + static async Task Bind(string sort, string direction) + { + var httpContext = new DefaultHttpContext + { + Request = + { + Query = new QueryCollection(new Dictionary + { + ["sort"] = sort, + ["direction"] = direction, + }) + } + }; + + var bindingContext = new DefaultModelBindingContext + { + ActionContext = new ActionContext { HttpContext = httpContext }, + ModelName = "sortInfo", + }; + + await new SortInfoModelBinder().BindModelAsync(bindingContext); + + return (SortInfo)bindingContext.Result.Model; + } + } +} From 6bbabc3cd220747e7ccb9875ecfff787ea6ae765 Mon Sep 17 00:00:00 2001 From: Ramon Smits Date: Tue, 19 May 2026 09:53:02 +0200 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20Allow=20time=5Fof=5Ffailure/?= =?UTF-8?q?modified=20sort=20for=20failed=20messages?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the two missing sort keys to the Primary instance SortInfoModelBinder allowlist so that sort=time_of_failure (and sort=modified) survive model binding and reach the RavenDB query, which already maps them to the TimeOfFailure / LastModified index fields. Fixes the ServicePulse failed-message list ordering by 'Time of failure', which previously fell back to 'Time sent'. (GREEN) --- .../Infrastructure/WebApi/SortInfoModelBinder.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs b/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs index 0e45358a62..c78414947f 100644 --- a/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs +++ b/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs @@ -41,6 +41,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext) "delivery_time", "processing_time", "status", - "message_id" + "message_id", + "modified", + "time_of_failure" }.ToFrozenSet(); } \ No newline at end of file From c433ffaed57c93a9016879b4f6401105669ba0e9 Mon Sep 17 00:00:00 2001 From: Ramon Smits Date: Tue, 19 May 2026 09:58:44 +0200 Subject: [PATCH 3/3] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Single=20source=20of?= =?UTF-8?q?=20truth=20for=20allowed=20sort=20options?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web-layer SortInfoModelBinder kept its own hardcoded allowlist, independent of the persistence layer's sort handling. The two drifted apart, which is how the Primary 'time_of_failure'/'modified' bug was introduced and went unnoticed. Make each instance's SortInfo own the canonical accepted-token set: - Primary: SortInfo.AllowedSortOptions (union of message + failed-message sortable fields). Consumed by SortInfoModelBinder and by the RavenDB AsyncDocumentQuery sort, whose duplicate private set is removed. No behaviour change: tokens unknown to a given endpoint still fall back to TimeSent in the persistence switch exactly as before. - Audit: SortInfo.AllowedSortOptions consumed by its SortInfoModelBinder. Audit had no functional defect (its allowlist already matched the query), but shared the same drift-prone duplicated-literal anti-pattern. Add Audit binder characterization tests and extend the Primary binder tests (valid message + failed-message fields preserved, unknown -> time_sent) to lock the invariant so the lists cannot silently diverge again. (REFACTOR) --- .../Infrastructure/SortInfo.cs | 20 +++++++ .../WebApi/SortInfoModelBinderTests.cs | 58 +++++++++++++++++++ .../WebApi/SortInfoModelBinder.cs | 17 +----- .../RavenQueryExtensions.cs | 13 +---- .../Infrastructure/SortInfo.cs | 25 ++++++++ .../WebApi/SortInfoModelBinderTests.cs | 10 ++++ .../WebApi/SortInfoModelBinder.cs | 19 +----- 7 files changed, 116 insertions(+), 46 deletions(-) create mode 100644 src/ServiceControl.Audit.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs diff --git a/src/ServiceControl.Audit.Persistence/Infrastructure/SortInfo.cs b/src/ServiceControl.Audit.Persistence/Infrastructure/SortInfo.cs index 88fdd9d5b9..456568ed4c 100644 --- a/src/ServiceControl.Audit.Persistence/Infrastructure/SortInfo.cs +++ b/src/ServiceControl.Audit.Persistence/Infrastructure/SortInfo.cs @@ -1,5 +1,8 @@ namespace ServiceControl.Audit.Infrastructure { + using System.Collections.Frozen; + using System.Collections.Generic; + public class SortInfo { public string Direction { get; } @@ -10,5 +13,22 @@ public SortInfo(string sort, string direction) Sort = sort; Direction = direction; } + + // Single source of truth for the audit sort tokens the API accepts. + // Consumed by SortInfoModelBinder so the web allowlist cannot drift + // from the fields the audit message query actually sorts by (anything + // not listed here falls back to TimeSent in the persistence layer). + public static readonly FrozenSet AllowedSortOptions = new HashSet + { + "id", + "message_id", + "message_type", + "status", + "time_sent", + "processed_at", + "critical_time", + "delivery_time", + "processing_time" + }.ToFrozenSet(); } } \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs b/src/ServiceControl.Audit.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs new file mode 100644 index 0000000000..7bd249fcd3 --- /dev/null +++ b/src/ServiceControl.Audit.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs @@ -0,0 +1,58 @@ +namespace ServiceControl.UnitTests.Infrastructure.WebApi; + +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Primitives; +using NUnit.Framework; +using ServiceControl.Audit.Infrastructure; +using ServiceControl.Audit.Infrastructure.WebApi; + +[TestFixture] +public class SortInfoModelBinderTests +{ + [TestCase("processed_at")] + [TestCase("critical_time")] + [TestCase("message_type")] + public async Task Preserves_valid_audit_sort_field(string sort) + { + var sortInfo = await Bind(sort, "asc"); + + Assert.That(sortInfo.Sort, Is.EqualTo(sort)); + } + + [Test] + public async Task Falls_back_to_time_sent_for_unknown_sort_field() + { + var sortInfo = await Bind("not_a_real_field", "asc"); + + Assert.That(sortInfo.Sort, Is.EqualTo("time_sent")); + } + + static async Task Bind(string sort, string direction) + { + var httpContext = new DefaultHttpContext + { + Request = + { + Query = new QueryCollection(new Dictionary + { + ["sort"] = sort, + ["direction"] = direction, + }) + } + }; + + var bindingContext = new DefaultModelBindingContext + { + ActionContext = new ActionContext { HttpContext = httpContext }, + ModelName = "sortInfo", + }; + + await new SortInfoModelBinder().BindModelAsync(bindingContext); + + return (SortInfo)bindingContext.Result.Model; + } +} diff --git a/src/ServiceControl.Audit/Infrastructure/WebApi/SortInfoModelBinder.cs b/src/ServiceControl.Audit/Infrastructure/WebApi/SortInfoModelBinder.cs index abe7e50615..de8b6c0290 100644 --- a/src/ServiceControl.Audit/Infrastructure/WebApi/SortInfoModelBinder.cs +++ b/src/ServiceControl.Audit/Infrastructure/WebApi/SortInfoModelBinder.cs @@ -1,8 +1,6 @@ namespace ServiceControl.Audit.Infrastructure.WebApi; using System; -using System.Collections.Frozen; -using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc.ModelBinding; @@ -21,7 +19,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext) bindingContext.HttpContext.Request.Query.TryGetValue("sort", out var sortValue); var sort = sortValue.ToString(); - if (!AllowableSortOptions.Contains(sort)) + if (!SortInfo.AllowedSortOptions.Contains(sort)) { sort = "time_sent"; } @@ -29,17 +27,4 @@ public Task BindModelAsync(ModelBindingContext bindingContext) bindingContext.Result = ModelBindingResult.Success(new SortInfo(sort, direction)); return Task.CompletedTask; } - - static readonly FrozenSet AllowableSortOptions = new HashSet - { - "processed_at", - "id", - "message_type", - "time_sent", - "critical_time", - "delivery_time", - "processing_time", - "status", - "message_id" - }.ToFrozenSet(); } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.RavenDB/RavenQueryExtensions.cs b/src/ServiceControl.Persistence.RavenDB/RavenQueryExtensions.cs index 47afe4dcd1..e9b86c43ec 100644 --- a/src/ServiceControl.Persistence.RavenDB/RavenQueryExtensions.cs +++ b/src/ServiceControl.Persistence.RavenDB/RavenQueryExtensions.cs @@ -82,7 +82,7 @@ public static IAsyncDocumentQuery Sort(this IAsyncDocumentQuer } var sort = sortInfo.Sort; - if (!AsyncDocumentQuerySortOptions.Contains(sort)) + if (!SortInfo.AllowedSortOptions.Contains(sort)) { sort = "time_sent"; } @@ -207,17 +207,6 @@ public static IAsyncDocumentQuery FilterByQueueAddress(this IAsyncDocument return source; } - static HashSet AsyncDocumentQuerySortOptions = - [ - "id", - "message_id", - "message_type", - "time_sent", - "status", - "modified", - "time_of_failure" - ]; - static string[] SplitChars = { "..." diff --git a/src/ServiceControl.Persistence/Infrastructure/SortInfo.cs b/src/ServiceControl.Persistence/Infrastructure/SortInfo.cs index 0a410f278c..e50a1a407d 100644 --- a/src/ServiceControl.Persistence/Infrastructure/SortInfo.cs +++ b/src/ServiceControl.Persistence/Infrastructure/SortInfo.cs @@ -1,5 +1,7 @@ namespace ServiceControl.Persistence.Infrastructure { + using System.Collections.Frozen; + using System.Collections.Generic; using System.Diagnostics; [DebuggerDisplay("{Sort} {Direction}")] @@ -7,5 +9,28 @@ public class SortInfo(string sort = null, string direction = null) { public string Direction { get; } = string.IsNullOrWhiteSpace(direction) ? "desc" : direction; public string Sort { get; } = string.IsNullOrWhiteSpace(sort) ? "time_sent" : sort; + + // Single source of truth for the sort tokens the API accepts. The model + // binder gates incoming requests against this set and the persistence + // layer resolves each accepted token to an index field per endpoint + // (anything it does not recognise falls back to TimeSent). It is the + // union of the fields sortable by the message endpoints (processed_at, + // critical_time, delivery_time, processing_time) and the failed-message + // endpoints (time_of_failure, modified), so the web allowlist and the + // persistence query can no longer drift apart. + public static readonly FrozenSet AllowedSortOptions = new HashSet + { + "id", + "message_id", + "message_type", + "status", + "time_sent", + "modified", + "time_of_failure", + "processed_at", + "critical_time", + "delivery_time", + "processing_time" + }.ToFrozenSet(); } } \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs b/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs index 01a40a5be9..67e454c1fb 100644 --- a/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs +++ b/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs @@ -17,6 +17,8 @@ public class SortInfoModelBinderTests // RavenDB layer. The model binder must not silently rewrite them away. [TestCase("time_of_failure")] [TestCase("modified")] + [TestCase("message_type")] + [TestCase("processed_at")] public async Task Preserves_valid_failed_message_sort_field(string sort) { var sortInfo = await Bind(sort, "asc"); @@ -24,6 +26,14 @@ public async Task Preserves_valid_failed_message_sort_field(string sort) Assert.That(sortInfo.Sort, Is.EqualTo(sort)); } + [Test] + public async Task Falls_back_to_time_sent_for_unknown_sort_field() + { + var sortInfo = await Bind("not_a_real_field", "asc"); + + Assert.That(sortInfo.Sort, Is.EqualTo("time_sent")); + } + static async Task Bind(string sort, string direction) { var httpContext = new DefaultHttpContext diff --git a/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs b/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs index c78414947f..0dbe13ecf3 100644 --- a/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs +++ b/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs @@ -1,8 +1,6 @@ namespace ServiceControl.Infrastructure.WebApi; using System; -using System.Collections.Frozen; -using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc.ModelBinding; using Persistence.Infrastructure; @@ -22,7 +20,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext) bindingContext.HttpContext.Request.Query.TryGetValue("sort", out var sortValue); var sort = sortValue.ToString(); - if (!AllowableSortOptions.Contains(sort)) + if (!SortInfo.AllowedSortOptions.Contains(sort)) { sort = "time_sent"; } @@ -30,19 +28,4 @@ public Task BindModelAsync(ModelBindingContext bindingContext) bindingContext.Result = ModelBindingResult.Success(new SortInfo(sort, direction)); return Task.CompletedTask; } - - static readonly FrozenSet AllowableSortOptions = new HashSet - { - "processed_at", - "id", - "message_type", - "time_sent", - "critical_time", - "delivery_time", - "processing_time", - "status", - "message_id", - "modified", - "time_of_failure" - }.ToFrozenSet(); } \ No newline at end of file