fix(filter): count the nesting depth in the grammar

.agents/skills/verify-querykit/features/configuration.md

@@ -16,6 +16,7 @@ A developer passes a `QueryKitConfiguration` to change how QueryKit reads the in
 - `config-parameterized` sends filter values as SQL parameters with `ParameterizeFilterValues = true`.
 - `config-remove-ignored` drops an ignored clause instead of replacing it with `True == True`, with `IgnoredClauseBehavior = IgnoredClauseBehavior.Remove`.
 - `config-small-limits` lowers the parse limits with `MaxInputLength` and `MaxNestingDepth`.
+- `config-nesting-depth` counts each parenthesized group against `MaxNestingDepth`. A `(` or `)` inside a quoted value does not change the count.
 
 ## How to get to it (user POV)
 
@@ -28,7 +29,7 @@ Preconditions:
 
 - A run is up and `qk doctor` prints only `ok` lines.
 - The seed data matches `features/README.md`.
-- `qk configs` lists `aliases`, `loose-names`, `derived`, `custom-operation`, `word-operators`, `hidden-price`, `allow-unknown`, `max-depth-0`, `upper`, `parameterized`, `remove-ignored`, and `small-limits`.
+- `qk configs` lists `aliases`, `loose-names`, `derived`, `custom-operation`, `word-operators`, `hidden-price`, `allow-unknown`, `max-depth-0`, `upper`, `parameterized`, `remove-ignored`, `small-limits`, and `depth-10`.
 
 - **Query names.** Run `qk run configuration-query-name --config aliases --filter 'chef == "Julia Child" && name _= "S"'`. Both targets give `["Salt Bread"]`.
 - **Query names that are not identifiers.** Run `qk run configuration-loose-query-names --config loose-names --filter 'recipe-title == "Pancakes" || _stars > 4 || chef name == "Gordon Ramsay"'`. Both targets give `["Pancakes", "Beef Stew"]`.
@@ -43,6 +44,7 @@ Preconditions:
 - **Parameterized values.** Run `qk run configuration-parameterized --config parameterized --filter 'Title == "Pancakes"'`. Both targets give `["Pancakes"]`. The `sql` contains a `@` parameter instead of the literal `'Pancakes'`.
 - **Remove ignored clauses.** Run `qk run configuration-remove-ignored --config remove-ignored --filter 'Rating > 1 && Nope == 1'`. Exit `0`. Both targets give all four recipes. The `expression` has no `True == True`.
 - **Small parse limits.** Run `qk run configuration-small-limits --config small-limits --filter '((((Title == "Pancakes"))))'`. Exit `2`. Both targets have `error.type` `QueryKit.Exceptions.QueryKitNestingDepthExceededException`.
+- **Quoted parentheses and the nesting depth.** Run `qk run configuration-nesting-depth --config depth-10 --filter 'Title == "))))))))))))))))))))" || ((((((((((((((((((((Title == "Pancakes"))))))))))))))))))))'`. Exit `2`. Both targets have `error.type` `QueryKit.Exceptions.QueryKitNestingDepthExceededException` with the message `The filter has a nesting depth of 11, which exceeds the maximum allowed depth of 10.`
 
 ## Gotchas
 

.agents/skills/verify-querykit/harness/Driver/Configs.cs

@@ -94,5 +94,8 @@ public static class Configs
                 s.MaxInputLength = 100;
                 s.MaxNestingDepth = 3;
             })),
+
+        ["depth-10"] = ("MaxNestingDepth = 10.",
+            () => new QueryKitConfiguration(s => s.MaxNestingDepth = 10)),
     };
 }

QueryKit.UnitTests/ParseLimitsTests.cs

@@ -130,6 +130,110 @@ public void configuration_that_implements_the_parse_limits_uses_its_own_limits()
             .WithMessage("*depth of 3*maximum allowed depth of 2*");
     }
 
+    [Fact]
+    public void quoted_close_parentheses_before_a_group_do_not_lower_the_nesting_depth()
+    {
+        var input = $"""Title == "{new string(')', 20)}" || """ + new string('(', 20) + """Title == "salt" """ + new string(')', 20);
+
+        var act = () => FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(10));
+        act.Should().Throw<QueryKitNestingDepthExceededException>()
+            .WithMessage("*depth of 11*maximum allowed depth of 10*");
+    }
+
+    [Fact]
+    public void quoted_close_parentheses_inside_a_group_do_not_lower_the_nesting_depth()
+    {
+        var input = new string('(', 8) + $"""Title == "{new string(')', 8)}" && """
+            + new string('(', 8) + """Title == "salt" """ + new string(')', 16);
+
+        var act = () => FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(10));
+        act.Should().Throw<QueryKitNestingDepthExceededException>()
+            .WithMessage("*depth of 11*maximum allowed depth of 10*");
+    }
+
+    [Fact]
+    public void repeated_quoted_close_parentheses_do_not_lower_the_nesting_depth()
+    {
+        var segment = """((((Title == "))))" && """;
+        var input = string.Concat(Enumerable.Repeat(segment, 5)) + """Title == "salt" """ + new string(')', 20);
+
+        var act = () => FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(10));
+        act.Should().Throw<QueryKitNestingDepthExceededException>()
+            .WithMessage("*depth of 11*maximum allowed depth of 10*");
+    }
+
+    [Fact]
+    public void quoted_parentheses_within_the_nesting_depth_keep_their_value()
+    {
+        var input = """"((Title == ")))" || Title == "(((" || Title == """((("""))"""";
+
+        var filterExpression = FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(2));
+        filterExpression.Compile().Invoke(new TestingPerson { Title = ")))" }).Should().BeTrue();
+        filterExpression.Compile().Invoke(new TestingPerson { Title = "(((" }).Should().BeTrue();
+        filterExpression.Compile().Invoke(new TestingPerson { Title = "salt" }).Should().BeFalse();
+    }
+
+    [Fact]
+    public void quoted_open_parentheses_do_not_count_toward_the_nesting_depth()
+    {
+        var parentheses = new string('(', 33);
+        var input = $$""""Title == "{{parentheses}}" || Title == """{{parentheses}}""" """";
+
+        var filterExpression = FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(1));
+        filterExpression.Compile().Invoke(new TestingPerson { Title = parentheses }).Should().BeTrue();
+    }
+
+    [Fact]
+    public void arithmetic_groups_count_toward_the_nesting_depth()
+    {
+        var input = """((Age + (Rating * 2)) > 3)""";
+
+        FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(3)).Should().NotBeNull();
+        var act = () => FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(2));
+        act.Should().Throw<QueryKitNestingDepthExceededException>()
+            .WithMessage("*depth of 3*maximum allowed depth of 2*");
+    }
+
+    [Fact]
+    public void property_list_groups_count_toward_the_nesting_depth()
+    {
+        var input = """((Title, FirstName) == "salt")""";
+
+        FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(2)).Should().NotBeNull();
+        var act = () => FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(1));
+        act.Should().Throw<QueryKitNestingDepthExceededException>()
+            .WithMessage("*depth of 2*maximum allowed depth of 1*");
+    }
+
+    [Fact]
+    public void deep_filter_with_quoted_close_parentheses_throws_instead_of_overflowing_the_stack()
+    {
+        // Without the grammar count, this filter overflows a 1 MB stack and stops the test process
+        const int depth = 20_000;
+        var input = $"""Title == "{new string(')', depth)}" || """ + new string('(', depth) + """Title == "salt" """ + new string(')', depth);
+
+        Exception? thrown = null;
+        var thread = new Thread(() =>
+        {
+            try
+            {
+                FilterParser.ParseFilter<TestingPerson>(input, DepthLimit(10));
+            }
+            catch (Exception e)
+            {
+                thrown = e;
+            }
+        }, maxStackSize: 1024 * 1024);
+        thread.Start();
+        thread.Join();
+
+        thrown.Should().BeOfType<QueryKitNestingDepthExceededException>()
+            .Which.Message.Should().Contain("depth of 11");
+    }
+
+    private static QueryKitConfiguration DepthLimit(int maxNestingDepth)
+        => new(settings => settings.MaxNestingDepth = maxNestingDepth);
+
     private sealed class InterfaceOnlyConfigurationWithLimits : FilterBehaviorInterfaceTests.InterfaceOnlyConfiguration, IQueryKitParseLimits
     {
         public int MaxNestingDepth { get; set; }

QueryKit/Configuration/IQueryKitParseLimits.cs

@@ -1,7 +1,7 @@
 namespace QueryKit.Configuration;
 
 /// <summary>
-/// The limits that the filter parser applies before it reads a filter. A configuration that does not
+/// The limits that the filter parser applies to a filter. A configuration that does not
 /// implement this interface uses <see cref="QueryKitSettings.DefaultMaxInputLength"/> and
 /// <see cref="QueryKitSettings.DefaultMaxNestingDepth"/>.
 /// </summary>

QueryKit/FilterParser.cs

@@ -20,7 +20,7 @@ public static class FilterParser
     /// <returns>Returns a Func delegate that represents a lambda expression that applies the filter defined by the input parameter.</returns>
     public static Expression<Func<T, bool>> ParseFilter<T>(string input, IQueryKitConfiguration? config = null)
     {
-        EnsureWithinParseLimits(input, config);
+        EnsureWithinInputLength(input, config);
 
         input = config?.ReplaceLogicalAliases(input) ?? input;
         input = config?.ReplaceComparisonAliases(input) ?? input;
@@ -30,6 +30,10 @@ public static Expression<Func<T, bool>> ParseFilter<T>(string input, IQueryKitCo
         Expression expr;
         var parameterizeBefore = FilterValue.Parameterize;
         FilterValue.Parameterize = config is IQueryKitFilterBehavior { ParameterizeFilterValues: true };
+        var maxNestingDepthBefore = _maxNestingDepth;
+        var nestingDepthBefore = _nestingDepth;
+        _maxNestingDepth = (config as IQueryKitParseLimits)?.MaxNestingDepth ?? QueryKitSettings.DefaultMaxNestingDepth;
+        _nestingDepth = 0;
         try
         {
             expr = ExprParser<T>(parameter, config).End().Parse(input);
@@ -53,6 +57,8 @@ public static Expression<Func<T, bool>> ParseFilter<T>(string input, IQueryKitCo
         finally
         {
             FilterValue.Parameterize = parameterizeBefore;
+            _maxNestingDepth = maxNestingDepthBefore;
+            _nestingDepth = nestingDepthBefore;
         }
 
         return Expression.Lambda<Func<T, bool>>(expr, parameter);
@@ -68,38 +74,46 @@ private static Expression ReplaceDerivedProperties(Expression expr, IQueryKitCon
         return new ParameterReplacer(parameter).Visit(expr);
     }
 
-    // Runs before the grammar sees the input, so a hostile filter (deeply nested parentheses,
-    // or an oversized `in` list) is rejected with a QueryKitException instead of overflowing the
-    // call stack or exhausting CPU and memory during parsing.
-    private static void EnsureWithinParseLimits(string input, IQueryKitConfiguration? config)
+    // Runs before the grammar sees the input, so an oversized filter is rejected with a
+    // QueryKitException instead of exhausting CPU and memory during parsing.
+    private static void EnsureWithinInputLength(string input, IQueryKitConfiguration? config)
     {
         var maxLength = (config as IQueryKitParseLimits)?.MaxInputLength ?? QueryKitSettings.DefaultMaxInputLength;
         if (input.Length > maxLength)
         {
             throw new QueryKitInputLengthExceededException(input.Length, maxLength);
         }
+    }
+
+    // The nesting depth limit of the parse on this thread, and the number of parenthesized groups
+    // that the parser is in now. Parsing is synchronous, so the values belong to the thread that parses.
+    [ThreadStatic] private static int _maxNestingDepth;
+    [ThreadStatic] private static int _nestingDepth;
 
-        // Counts every '(' and ')', including ones inside quoted values. QueryKit supports several
-        // quoting styles (plain and raw-string style with 3+ quote marks), so a scanner that tries
-        // to skip "quoted" spans could misjudge one of them and undercount real nesting. Counting
-        // everything can only reject too much, never too little.
-        var maxDepth = (config as IQueryKitParseLimits)?.MaxNestingDepth ?? QueryKitSettings.DefaultMaxNestingDepth;
-        var depth = 0;
-        foreach (var c in input)
+    // Parses '(' inner ')' and counts the group against MaxNestingDepth. The grammar does the count,
+    // so a '(' or ')' inside a quoted value cannot change it. The parser recurses once for each group,
+    // so the limit also limits the depth of the call stack.
+    private static Parser<TResult> Grouped<TResult>(Parser<TResult> inner)
+    {
+        Parser<TResult> counted = input =>
         {
-            if (c == '(')
+            try
             {
-                depth++;
-                if (depth > maxDepth)
+                _nestingDepth++;
+                if (_nestingDepth > _maxNestingDepth)
                 {
-                    throw new QueryKitNestingDepthExceededException(depth, maxDepth);
+                    throw new QueryKitNestingDepthExceededException(_nestingDepth, _maxNestingDepth);
                 }
+
+                return inner(input);
             }
-            else if (c == ')')
+            finally
             {
-                depth--;
+                _nestingDepth--;
             }
-        }
+        };
+
+        return counted.Contained(Parse.Char('('), Parse.Char(')'));
     }
 
     private static readonly Parser<string> Identifier =
@@ -113,10 +127,7 @@ from rest in Parse.LetterOrDigit.XOr(Parse.Char('_')).Many()
     private static Parser<IEnumerable<string>> PropertyListParser(Parser<string> propertyPathParser)
     {
         var propertiesParser = propertyPathParser.Token().DelimitedBy(Parse.Char(',').Token());
-        return from openParen in Parse.Char('(')
-               from properties in propertiesParser
-               from closeParen in Parse.Char(')')
-               select properties;
+        return Grouped(propertiesParser);
     }
 
     // Each parser is built once. A parser in a second or later `from` clause is built in a lambda
@@ -320,7 +331,7 @@ from trailingSpaces in Parse.WhiteSpace.Many()
     private static readonly Parser<ArithmeticExpression> ArithmeticTermParser =
         PropertyArithmeticParser
             .Or(LiteralArithmeticParser)
-            .Or(Parse.Ref(() => ArithmeticExpressionParser).Contained(Parse.Char('('), Parse.Char(')')).Select(expr => new GroupedArithmeticExpression(expr)));
+            .Or(Grouped(Parse.Ref(() => ArithmeticExpressionParser)).Select(expr => new GroupedArithmeticExpression(expr)));
 
     private static readonly Parser<ArithmeticExpression> ArithmeticFactorParser =
         Parse.ChainOperator(
@@ -672,7 +683,7 @@ private static Parser<Expression> ArithmeticComparisonExprParser<T>(ParameterExp
         var rightSideValueParser = RightSideValueParser.Token();
 
         // Only parse arithmetic expressions that are in parentheses and contain arithmetic operators
-        var parenthesizedArithmetic = ArithmeticExpressionParser.Contained(Parse.Char('('), Parse.Char(')')).Token();
+        var parenthesizedArithmetic = Grouped(ArithmeticExpressionParser).Token();
 
         // Ensure the arithmetic expression contains actual arithmetic operators
         var validArithmeticExpr = parenthesizedArithmetic.Where(expr => ContainsArithmeticOperator(expr));
@@ -1201,7 +1212,7 @@ private static Parser<Expression> PropertyListComparisonExprParser<T>(
 
     private static Parser<Expression> AtomicExprParser<T>(ParameterExpression parameter, IQueryKitConfiguration? config = null)
         => ComparisonExprParser<T>(parameter, config)
-            .Or(Parse.Ref(() => ExprParser<T>(parameter, config)).Contained(Parse.Char('('), Parse.Char(')')));
+            .Or(Grouped(Parse.Ref(() => ExprParser<T>(parameter, config))));
 
     private static Parser<Expression> ExprParser<T>(ParameterExpression parameter, IQueryKitConfiguration? config = null)
         => OrExprParser<T>(parameter, config);

README.md

@@ -823,7 +823,7 @@ var filterExpression = FilterParser.ParseFilter<Recipe>(input, config);
 
 #### Parse Limits
 
-`IQueryKitParseLimits` caps how much a filter string can do before QueryKit parses it, through `MaxInputLength` (a number of characters) and `MaxNestingDepth` (a number of levels of parentheses). Both limits are off by default. If your app sends user input to QueryKit, turn both limits on. A filter string with a few thousand nested parentheses can overflow the call stack and stop the process. `QueryKitConfiguration` implements this interface, so a custom configuration class can implement `IQueryKitParseLimits` directly instead. A filter string that goes over `MaxInputLength` throws a `QueryKitInputLengthExceededException`. A filter string that goes over `MaxNestingDepth` throws a `QueryKitNestingDepthExceededException`. Both exceptions throw before parsing starts. These limits apply only to filter strings. Sort strings have no limit, and the number of items in an in-list has no limit. The nesting-depth check counts every `(` character, including a `(` inside a quoted value.
+`IQueryKitParseLimits` caps how much a filter string can do, through `MaxInputLength` (a number of characters) and `MaxNestingDepth` (a number of levels of parentheses). Both limits are off by default. If your app sends user input to QueryKit, turn both limits on. A filter string with a few thousand nested parentheses can overflow the call stack and stop the process. `QueryKitConfiguration` implements this interface, so a custom configuration class can implement `IQueryKitParseLimits` directly instead. A filter string that goes over `MaxInputLength` throws a `QueryKitInputLengthExceededException` before parsing starts. A filter string that goes over `MaxNestingDepth` throws a `QueryKitNestingDepthExceededException` when the parser enters the group that goes over the limit. These limits apply only to filter strings. Sort strings have no limit, and the number of items in an in-list has no limit. The nesting depth counts each parenthesized group in the filter: a logical group, an arithmetic group, and a property list. A `(` or `)` inside a quoted value is part of the value, so it does not change the depth. `MaxNestingDepth` does not limit a long flat chain of `&&` or `||` clauses, and a very long chain can also overflow the call stack. Keep `MaxInputLength` small to limit such a chain.
 
 ```csharp
 var config = new QueryKitConfiguration(config =>