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 new file mode 100644 index 0000000000..67e454c1fb --- /dev/null +++ b/src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs @@ -0,0 +1,62 @@ +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")] + [TestCase("message_type")] + [TestCase("processed_at")] + public async Task Preserves_valid_failed_message_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/Infrastructure/WebApi/SortInfoModelBinder.cs b/src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs index 0e45358a62..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,17 +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" - }.ToFrozenSet(); } \ No newline at end of file