Stop passing raw API query options into the expression parser (#766)

Author: KrzysztofPajak

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;
     }

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;
+
+/// <summary>
+///     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.
+/// </summary>
+public class ApiQueryOptionException(string message) : Exception(message);
+
+/// <summary>
+///     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.
+/// </summary>
+public static class ApiQueryOptions
+{
+    /// <summary>
+    ///     Long enough for a realistic filter, short enough that a pathological expression cannot make
+    ///     the parser the slow part of the request.
+    /// </summary>
+    public const int MaxFilterLength = 512;
+
+    public const int MaxFields = 50;
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    private static readonly HashSet<string> AllowedMethods = new(StringComparer.OrdinalIgnoreCase) {
+        "Contains", "StartsWith", "EndsWith", "ToLower", "ToUpper", "Trim", "Length", "Equals"
+    };
+
+    /// <summary>
+    ///     Operators and literals the parser understands that are not members of the model.
+    /// </summary>
+    private static readonly HashSet<string> 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);
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    private static readonly Regex StringLiteral = new(@"""(?:[^""\\]|\\.)*""|'(?:[^'\\]|\\.)*'", RegexOptions.Compiled);
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    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()
+    };
+
+    /// <summary>
+    ///     $select is rebuilt from validated field names, so `new` has to be available for that one
+    ///     projection - and for nothing else.
+    /// </summary>
+    public static ParsingConfig SelectConfig { get; } = new() {
+        AreContextKeywordsEnabled = false,
+        AllowNewToEvaluateAnyType = false,
+        ResolveTypesBySimpleName = false,
+        SupportCastingToFullyQualifiedTypeAsString = false,
+        LoadAdditionalAssembliesFromCurrentDomainBaseDirectory = false,
+        AllowEqualsAndToStringMethodsOnObject = false,
+        RestrictOrderByToPropertyOrField = true,
+        CustomTypeProvider = new NoCustomTypesProvider()
+    };
+
+    /// <summary>
+    ///     Checks that a filter names nothing outside the model it runs against.
+    /// </summary>
+    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}");
+        }
+    }
+
+    /// <summary>
+    ///     Reads "Field asc, Other desc" and gives it back with every field checked against the model.
+    /// </summary>
+    public static string ParseOrderBy(string orderBy, Type elementType)
+    {
+        var members = MemberNames(elementType);
+        var parts = SplitFields(orderBy, "$orderby");
+        var ordering = new List<string>(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);
+    }
+
+    /// <summary>
+    ///     Reads a field list and builds the projection itself, so no client text reaches the parser.
+    /// </summary>
+    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<string> 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<string> MemberNames(Type elementType)
+    {
+        return new HashSet<string>(
+            elementType.GetProperties(BindingFlags.Public | BindingFlags.Instance).Select(x => x.Name),
+            StringComparer.OrdinalIgnoreCase);
+    }
+
+    /// <summary>
+    ///     Leaves the parser with no types to resolve at all.
+    /// </summary>
+    private class NoCustomTypesProvider : IDynamicLinqCustomTypeProvider
+    {
+        public HashSet<Type> GetCustomTypes()
+        {
+            return [];
+        }
+
+        public Dictionary<Type, List<MethodInfo>> GetExtensionMethods()
+        {
+            return [];
+        }
+
+        public Type ResolveType(string typeName)
+        {
+            return null;
+        }
+
+        public Type ResolveTypeBySimpleName(string simpleTypeName)
+        {
+            return null;
+        }
+    }
+}

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);
+    }
+
+    [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")]
+    [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<ApiQueryOptionException>(() => ApiQueryOptions.ValidateFilter(filter, ElementType));
+    }
+
+    [TestMethod]
+    public void Filter_RejectsAnOverlongExpression()
+    {
+        var filter = string.Join(" or ", Enumerable.Repeat("Name == \"x\"", 100));
+
+        var ex = Assert.ThrowsExactly<ApiQueryOptionException>(
+            () => ApiQueryOptions.ValidateFilter(filter, ElementType));
+
+        StringAssert.Contains(ex.Message, "characters");
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    [TestMethod]
+    public void FilterConfig_RefusesToResolveTypes()
+    {
+        var source = new[] { new ProductDto { Name = "shirt" } }.AsQueryable();
+
+        Assert.ThrowsExactly<System.Linq.Dynamic.Core.Exceptions.ParseException>(
+            () => 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));
+    }
+
+    [TestMethod]
+    [DataRow("PasswordHash")]
+    [DataRow("Name sideways")]
+    [DataRow("Name asc extra")]
+    [DataRow("")]
+    public void OrderBy_RejectsWhatItCannotVerify(string orderBy)
+    {
+        Assert.ThrowsExactly<ApiQueryOptionException>(() => ApiQueryOptions.ParseOrderBy(orderBy, ElementType));
+    }
+
+    [TestMethod]
+    public void Select_BuildsTheProjectionFromCheckedFields()
+    {
+        Assert.AreEqual("new(Id, Name)", ApiQueryOptions.ParseSelect("Id, Name", ElementType));
+    }
+
+    [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")]
+    public void Select_RejectsAnythingThatIsNotAPlainField(string select)
+    {
+        Assert.ThrowsExactly<ApiQueryOptionException>(() => ApiQueryOptions.ParseSelect(select, ElementType));
+    }
+
+    [TestMethod]
+    public void Select_RejectsMoreFieldsThanTheLimit()
+    {
+        var select = string.Join(",", Enumerable.Range(0, ApiQueryOptions.MaxFields + 1).Select(_ => "Name"));
+
+        Assert.ThrowsExactly<ApiQueryOptionException>(() => ApiQueryOptions.ParseSelect(select, ElementType));
+    }
+}