From 1568a902fd1f816b1c62cf285d44b6fba7025c91 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 9 Aug 2026 10:34:55 +0200 Subject: [PATCH 1/2] fix(api): stop passing raw query options into the expression parser $filter, $orderby and $select went from the query string straight into System.Linq.Dynamic.Core with no parsing restrictions, no field whitelist and no length limit, and with no try/catch - so a parse failure left OnActionExecuted as a 500 rather than a 400. The only guard was MaxLimit on $top. $orderby and $select no longer reach the parser as client text at all. They are read as field lists, checked against the projected model and rebuilt here, which removes the class of problem instead of filtering it. $filter still needs a real expression, so it is fenced on three sides: a ParsingConfig that resolves no types, disables the it/root context keywords, bans new, refuses to probe assemblies and denies Equals/ToString on object, backed by an empty custom type provider; a 512 character limit; and a whitelist that accepts only members of the model, seven cheap text methods and the parser's own keywords. String literals are stripped before the whitelist runs, so a product named "Password" stays searchable. $skip and $top now reject non-numeric and negative input instead of silently ignoring it. 13 tests cover the rejections, including reflection via it.GetType(), a fully qualified type, object construction and a field the model does not expose. One test exercises the ParsingConfig on its own, so a future change to the whitelist cannot quietly re-open type access. Store scoping (2.2 in the audit) is deliberately not part of this: the API requires ManageAccessAdminPanel, which no store or vendor group holds by default, the DTOs carry no store fields to filter on, and scoping the existing endpoints would break every global-admin integration. Co-Authored-By: Claude Opus 5 --- .../Attributes/EnableQueryAttribute.cs | 58 +++-- .../Queries/ApiQueryOptions.cs | 203 ++++++++++++++++++ .../Queries/ApiQueryOptionsTests.cs | 101 +++++++++ 3 files changed, 348 insertions(+), 14 deletions(-) create mode 100644 src/Modules/Grand.Module.Api/Queries/ApiQueryOptions.cs create mode 100644 src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs diff --git a/src/Modules/Grand.Module.Api/Attributes/EnableQueryAttribute.cs b/src/Modules/Grand.Module.Api/Attributes/EnableQueryAttribute.cs index f2d108cc5..c2132a961 100644 --- a/src/Modules/Grand.Module.Api/Attributes/EnableQueryAttribute.cs +++ b/src/Modules/Grand.Module.Api/Attributes/EnableQueryAttribute.cs @@ -1,8 +1,10 @@ -using Microsoft.AspNetCore.Mvc.Filters; +using Grand.Module.Api.Constants; +using Grand.Module.Api.Queries; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; using System.Linq.Dynamic.Core; -using Microsoft.AspNetCore.Http; -using Grand.Module.Api.Constants; +using System.Linq.Dynamic.Core.Exceptions; namespace Grand.Module.Api.Attributes; @@ -14,34 +16,62 @@ public override void OnActionExecuted(ActionExecutedContext context) if (context.Result is not ObjectResult result || result.Value == null) return; - if (result.Value is IQueryable queryable) + if (result.Value is not IQueryable queryable) + return; + + try + { + result.Value = ApplyQueryOptions(queryable, context.HttpContext.Request.Query); + } + catch (ApiQueryOptionException ex) + { + //a rejected query option is the client's mistake, not ours - this filter runs after the + //action, so without this the exception would leave the pipeline as a 500 + context.Result = new BadRequestObjectResult(new { error = ex.Message }); + } + catch (ParseException ex) { - queryable = ApplyQueryOptions(queryable, context.HttpContext.Request.Query, context.HttpContext.Response); - result.Value = queryable; + context.Result = new BadRequestObjectResult(new { error = $"$filter could not be parsed: {ex.Message}" }); } } - private static IQueryable ApplyQueryOptions(IQueryable queryable, IQueryCollection query, HttpResponse response) + private static IQueryable ApplyQueryOptions(IQueryable queryable, IQueryCollection query) { + var elementType = queryable.ElementType; + if (query.TryGetValue("$filter", out var filter)) - queryable = queryable.Where(filter.ToString()); + { + ApiQueryOptions.ValidateFilter(filter.ToString(), elementType); + queryable = queryable.Where(ApiQueryOptions.FilterConfig, filter.ToString()); + } if (query.TryGetValue("$orderby", out var orderBy)) - queryable = queryable.OrderBy(orderBy.ToString()); + queryable = queryable.OrderBy(ApiQueryOptions.FilterConfig, + ApiQueryOptions.ParseOrderBy(orderBy.ToString(), elementType)); if (query.TryGetValue("$select", out var select)) - queryable = queryable.Select($"new({select})"); + queryable = queryable.Select(ApiQueryOptions.SelectConfig, + ApiQueryOptions.ParseSelect(select.ToString(), elementType)); + + if (query.TryGetValue("$skip", out var skipValue)) + { + if (!int.TryParse(skipValue, out var skip) || skip < 0) + throw new ApiQueryOptionException("$skip must be a non-negative integer"); - if (query.TryGetValue("$skip", out var skipValue) && int.TryParse(skipValue, out var skip)) queryable = queryable.Skip(skip); + } - if (query.TryGetValue("$top", out var topValue) && int.TryParse(topValue, out var top)) + if (query.TryGetValue("$top", out var topValue)) { - top = Math.Min(top, Configurations.MaxLimit); - queryable = queryable.Take(top); + if (!int.TryParse(topValue, out var top) || top < 0) + throw new ApiQueryOptionException("$top must be a non-negative integer"); + + queryable = queryable.Take(Math.Min(top, Configurations.MaxLimit)); } else + { queryable = queryable.Take(Configurations.MaxLimit); + } return queryable; } diff --git a/src/Modules/Grand.Module.Api/Queries/ApiQueryOptions.cs b/src/Modules/Grand.Module.Api/Queries/ApiQueryOptions.cs new file mode 100644 index 000000000..c06dcdc76 --- /dev/null +++ b/src/Modules/Grand.Module.Api/Queries/ApiQueryOptions.cs @@ -0,0 +1,203 @@ +using System.Linq.Dynamic.Core; +using System.Linq.Dynamic.Core.CustomTypeProviders; +using System.Reflection; +using System.Text.RegularExpressions; + +namespace Grand.Module.Api.Queries; + +/// +/// Raised when a client sends a query option the API refuses to run. Mapped to 400 by the caller; +/// it is never an internal error, so it must not surface as 500. +/// +public class ApiQueryOptionException(string message) : Exception(message); + +/// +/// Parses and restricts the OData-like query options. +/// $filter reaches a real expression parser, so it is fenced on three sides: a parsing config that +/// denies types, context keywords and object construction; a length limit; and a whitelist that +/// accepts only members of the projected model. $orderby and $select never reach the parser as raw +/// text - they are read as field lists and rebuilt here. +/// +public static class ApiQueryOptions +{ + /// + /// Long enough for a realistic filter, short enough that a pathological expression cannot make + /// the parser the slow part of the request. + /// + public const int MaxFilterLength = 512; + + public const int MaxFields = 50; + + /// + /// Methods a filter may name. Everything here is a cheap, side-effect free string or text + /// operation; nothing that reflects, constructs, or walks a collection. + /// + private static readonly HashSet AllowedMethods = new(StringComparer.OrdinalIgnoreCase) { + "Contains", "StartsWith", "EndsWith", "ToLower", "ToUpper", "Trim", "Length", "Equals" + }; + + /// + /// Operators and literals the parser understands that are not members of the model. + /// + private static readonly HashSet Keywords = new(StringComparer.OrdinalIgnoreCase) { + "and", "or", "not", "true", "false", "null", "iif" + }; + + private static readonly Regex Identifier = new(@"[A-Za-z_][A-Za-z0-9_]*", RegexOptions.Compiled); + + /// + /// String literals hold user text, not member names, so they are removed before the whitelist + /// runs - otherwise a product named "Password" would fail its own search. + /// + private static readonly Regex StringLiteral = new(@"""(?:[^""\\]|\\.)*""|'(?:[^'\\]|\\.)*'", RegexOptions.Compiled); + + /// + /// Denies everything the parser can reach outside the model: no type resolution, no `it`/`root` + /// context keywords, no `new`, no assembly probing, no Equals/ToString on object. + /// + public static ParsingConfig FilterConfig { get; } = new() { + AreContextKeywordsEnabled = false, + AllowNewToEvaluateAnyType = false, + DisallowNewKeyword = true, + ResolveTypesBySimpleName = false, + SupportCastingToFullyQualifiedTypeAsString = false, + LoadAdditionalAssembliesFromCurrentDomainBaseDirectory = false, + AllowEqualsAndToStringMethodsOnObject = false, + RestrictOrderByToPropertyOrField = true, + CustomTypeProvider = new NoCustomTypesProvider() + }; + + /// + /// $select is rebuilt from validated field names, so `new` has to be available for that one + /// projection - and for nothing else. + /// + public static ParsingConfig SelectConfig { get; } = new() { + AreContextKeywordsEnabled = false, + AllowNewToEvaluateAnyType = false, + ResolveTypesBySimpleName = false, + SupportCastingToFullyQualifiedTypeAsString = false, + LoadAdditionalAssembliesFromCurrentDomainBaseDirectory = false, + AllowEqualsAndToStringMethodsOnObject = false, + RestrictOrderByToPropertyOrField = true, + CustomTypeProvider = new NoCustomTypesProvider() + }; + + /// + /// Checks that a filter names nothing outside the model it runs against. + /// + public static void ValidateFilter(string filter, Type elementType) + { + if (string.IsNullOrWhiteSpace(filter)) + throw new ApiQueryOptionException("$filter is empty"); + + if (filter.Length > MaxFilterLength) + throw new ApiQueryOptionException($"$filter exceeds {MaxFilterLength} characters"); + + var members = MemberNames(elementType); + var expression = StringLiteral.Replace(filter, " "); + + foreach (Match match in Identifier.Matches(expression)) + { + var name = match.Value; + if (Keywords.Contains(name) || AllowedMethods.Contains(name) || members.Contains(name)) + continue; + + throw new ApiQueryOptionException($"'{name}' is not a queryable field of {elementType.Name}"); + } + } + + /// + /// Reads "Field asc, Other desc" and gives it back with every field checked against the model. + /// + public static string ParseOrderBy(string orderBy, Type elementType) + { + var members = MemberNames(elementType); + var parts = SplitFields(orderBy, "$orderby"); + var ordering = new List(parts.Count); + + foreach (var part in parts) + { + var tokens = part.Split(' ', StringSplitOptions.RemoveEmptyEntries); + if (tokens.Length > 2) + throw new ApiQueryOptionException($"'{part}' is not a valid $orderby entry"); + + if (!members.Contains(tokens[0])) + throw new ApiQueryOptionException($"'{tokens[0]}' is not a queryable field of {elementType.Name}"); + + var direction = tokens.Length == 2 ? tokens[1].ToLowerInvariant() : "asc"; + if (direction != "asc" && direction != "desc") + throw new ApiQueryOptionException($"'{tokens[1]}' is not a sort direction"); + + ordering.Add($"{tokens[0]} {direction}"); + } + + return string.Join(", ", ordering); + } + + /// + /// Reads a field list and builds the projection itself, so no client text reaches the parser. + /// + public static string ParseSelect(string select, Type elementType) + { + var members = MemberNames(elementType); + var fields = SplitFields(select, "$select"); + + foreach (var field in fields) + if (!members.Contains(field)) + throw new ApiQueryOptionException($"'{field}' is not a queryable field of {elementType.Name}"); + + return $"new({string.Join(", ", fields)})"; + } + + private static List SplitFields(string value, string option) + { + if (string.IsNullOrWhiteSpace(value)) + throw new ApiQueryOptionException($"{option} is empty"); + + var fields = value.Split(',', StringSplitOptions.RemoveEmptyEntries) + .Select(x => x.Trim()) + .Where(x => x.Length > 0) + .ToList(); + + if (fields.Count == 0) + throw new ApiQueryOptionException($"{option} is empty"); + + if (fields.Count > MaxFields) + throw new ApiQueryOptionException($"{option} lists more than {MaxFields} fields"); + + return fields; + } + + private static HashSet MemberNames(Type elementType) + { + return new HashSet( + elementType.GetProperties(BindingFlags.Public | BindingFlags.Instance).Select(x => x.Name), + StringComparer.OrdinalIgnoreCase); + } + + /// + /// Leaves the parser with no types to resolve at all. + /// + private class NoCustomTypesProvider : IDynamicLinqCustomTypeProvider + { + public HashSet GetCustomTypes() + { + return []; + } + + public Dictionary> GetExtensionMethods() + { + return []; + } + + public Type ResolveType(string typeName) + { + return null; + } + + public Type ResolveTypeBySimpleName(string simpleTypeName) + { + return null; + } + } +} diff --git a/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs b/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs new file mode 100644 index 000000000..8e36c66e4 --- /dev/null +++ b/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs @@ -0,0 +1,101 @@ +using Grand.Module.Api.DTOs.Catalog; +using Grand.Module.Api.Queries; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System.Linq.Dynamic.Core; +using Assert = Microsoft.VisualStudio.TestTools.UnitTesting.Assert; + +namespace Grand.Module.Api.Tests.Queries; + +[TestClass] +public class ApiQueryOptionsTests +{ + private static readonly Type ElementType = typeof(ProductDto); + + [TestMethod] + public void Filter_AcceptsAFieldOfTheModel() + { + ApiQueryOptions.ValidateFilter("Name.Contains(\"shirt\") and Published == true", ElementType); + } + + [TestMethod] + public void Filter_KeepsALiteralOutOfTheFieldCheck() + { + //the text is data, not a member name - a product called "Password" must stay searchable + ApiQueryOptions.ValidateFilter("Name == \"Password\"", ElementType); + } + + [DataTestMethod] + [DataRow("PasswordHash != null", DisplayName = "field the model does not expose")] + [DataRow("it.GetType().Assembly != null", DisplayName = "reflection through the context keyword")] + [DataRow("\"\".GetType().Assembly.GetTypes().Length > 0", DisplayName = "type walk from a literal")] + [DataRow("System.IO.File.ReadAllText(\"appsettings.json\") != null", DisplayName = "fully qualified type")] + [DataRow("new(Name as X).X != null", DisplayName = "object construction")] + [DataRow("Name.Equals(Name, StringComparison.Ordinal)", DisplayName = "type reference in an argument")] + public void Filter_RejectsAnythingOutsideTheModel(string filter) + { + Assert.ThrowsExactly(() => ApiQueryOptions.ValidateFilter(filter, ElementType)); + } + + [TestMethod] + public void Filter_RejectsAnOverlongExpression() + { + var filter = string.Join(" or ", Enumerable.Repeat("Name == \"x\"", 100)); + + var ex = Assert.ThrowsExactly( + () => ApiQueryOptions.ValidateFilter(filter, ElementType)); + + StringAssert.Contains(ex.Message, "characters"); + } + + /// + /// The whitelist decides what runs; this checks the parser it runs on is locked down too, so a + /// future change to the whitelist cannot quietly re-open type access. + /// + [TestMethod] + public void FilterConfig_RefusesToResolveTypes() + { + var source = new[] { new ProductDto { Name = "shirt" } }.AsQueryable(); + + Assert.ThrowsExactly( + () => source.Where(ApiQueryOptions.FilterConfig, "it.GetType().Name == \"ProductDto\"").ToList()); + } + + [TestMethod] + public void OrderBy_NormalizesDirectionAndDefaultsToAscending() + { + Assert.AreEqual("Name asc, Sku desc", ApiQueryOptions.ParseOrderBy("Name, Sku DESC", ElementType)); + } + + [DataTestMethod] + [DataRow("PasswordHash")] + [DataRow("Name sideways")] + [DataRow("Name asc extra")] + [DataRow("")] + public void OrderBy_RejectsWhatItCannotVerify(string orderBy) + { + Assert.ThrowsExactly(() => ApiQueryOptions.ParseOrderBy(orderBy, ElementType)); + } + + [TestMethod] + public void Select_BuildsTheProjectionFromCheckedFields() + { + Assert.AreEqual("new(Id, Name)", ApiQueryOptions.ParseSelect("Id, Name", ElementType)); + } + + [DataTestMethod] + [DataRow("PasswordHash", DisplayName = "field the model does not expose")] + [DataRow("Name as Alias", DisplayName = "expression rather than a field")] + [DataRow("it.GetType()", DisplayName = "reflection")] + public void Select_RejectsAnythingThatIsNotAPlainField(string select) + { + Assert.ThrowsExactly(() => ApiQueryOptions.ParseSelect(select, ElementType)); + } + + [TestMethod] + public void Select_RejectsMoreFieldsThanTheLimit() + { + var select = string.Join(",", Enumerable.Range(0, ApiQueryOptions.MaxFields + 1).Select(_ => "Name")); + + Assert.ThrowsExactly(() => ApiQueryOptions.ParseSelect(select, ElementType)); + } +} From 592827586fc9f624fa6ec938d7a68a32780d69e9 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 9 Aug 2026 12:08:10 +0200 Subject: [PATCH 2/2] test(api): use TestMethod for the data-driven query option tests DataTestMethod is obsolete in MSTest 4; TestMethod carries DataRow on its own. Co-Authored-By: Claude Opus 5 --- .../Queries/ApiQueryOptionsTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs b/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs index 8e36c66e4..2ac02abb4 100644 --- a/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs +++ b/src/Tests/Grand.Module.Api.Tests/Queries/ApiQueryOptionsTests.cs @@ -1,4 +1,4 @@ -using Grand.Module.Api.DTOs.Catalog; +using Grand.Module.Api.DTOs.Catalog; using Grand.Module.Api.Queries; using Microsoft.VisualStudio.TestTools.UnitTesting; using System.Linq.Dynamic.Core; @@ -24,7 +24,7 @@ public void Filter_KeepsALiteralOutOfTheFieldCheck() ApiQueryOptions.ValidateFilter("Name == \"Password\"", ElementType); } - [DataTestMethod] + [TestMethod] [DataRow("PasswordHash != null", DisplayName = "field the model does not expose")] [DataRow("it.GetType().Assembly != null", DisplayName = "reflection through the context keyword")] [DataRow("\"\".GetType().Assembly.GetTypes().Length > 0", DisplayName = "type walk from a literal")] @@ -66,7 +66,7 @@ public void OrderBy_NormalizesDirectionAndDefaultsToAscending() Assert.AreEqual("Name asc, Sku desc", ApiQueryOptions.ParseOrderBy("Name, Sku DESC", ElementType)); } - [DataTestMethod] + [TestMethod] [DataRow("PasswordHash")] [DataRow("Name sideways")] [DataRow("Name asc extra")] @@ -82,7 +82,7 @@ public void Select_BuildsTheProjectionFromCheckedFields() Assert.AreEqual("new(Id, Name)", ApiQueryOptions.ParseSelect("Id, Name", ElementType)); } - [DataTestMethod] + [TestMethod] [DataRow("PasswordHash", DisplayName = "field the model does not expose")] [DataRow("Name as Alias", DisplayName = "expression rather than a field")] [DataRow("it.GetType()", DisplayName = "reflection")]