fix(parser): limit filter nesting depth and length, plus small fixes and doc cleanups - #109
Merged
Merged
Conversation
pdevito3
force-pushed
the
fm/qk-dos-limits
branch
from
September 29, 2026 18:18
a1b497b to
bdd01d7
Compare
pdevito3
changed the title
…gth limit Deeply nested parentheses in a filter string can exhaust CPU and memory during parsing, or overflow the call stack, before any grammar rule runs. A crafted filter of a few thousand nested groups can hang a process indefinitely. Check the input length and parenthesis nesting depth before the parser sees the string, and reject input over a limit. The limits are configurable through QueryKitSettings, with safe defaults enabled out of the box (depth 32, length 5000).
…nknown-property tests Bogus can generate the word "id", a real property on TestingPerson, about once every 182 runs. When it does, the test asserts an UnknownFilterPropertyException that never gets thrown, and the test fails at random. Use a fixed name that can never collide with a real property.
The HasType operator (^$) raised an error message that named DoesNotHaveType instead of itself, copied from the operator below it.
No code in the library or the tests calls this method.
Use ToDisplayString() instead of ToString() so the assertion still sees the literal value after PR 110 parameterized filter values.
Use eqi (matching the configured appendix) instead of eq$ so the example actually parses.
The filter example used a malformed verbatim-interpolated string, and the sort example mixed interpolation with an unclosed raw string. Both examples now compile and run as written.
The build step used --configuration Release, but the test step had no configuration flag, so it built and ran a fresh Debug build instead. The test step now passes --configuration Release, so CI tests the same build that dotnet pack ships.
Title == salt does not throw a ParsingException. QueryKit reads the unquoted word as the literal text salt.
pdevito3
force-pushed
the
fm/qk-dos-limits
branch
from
September 29, 2026 18:33
a3b719e to
175df75
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(characters) crashed the process with unbounded memory growth and CPU use, before any real parsing began. Both limits are enabled by default, and both are configurable throughQueryKitSettings/QueryKitConfiguration, matching the existingMaxPropertyDepthpattern. A limit violation throwsQueryKitNestingDepthExceededExceptionorQueryKitInputLengthExceededException(bothQueryKitExceptionsubtypes) before parsing starts.ComparisonOperator.HasType's error message: it said"DoesNotHaveType is only supported for collections"(copy-paste error). It now correctly says"HasType is only supported for collections".ArithmeticOperator.FromSymbolmethod.Console.WriteLinedebug statement left in aHasConversiontest.HasConversiontests that were skipped as "out of date." Verified live that each one fails against current, correct behavior, and thatHasConversionTests.csalready covers the same scenarios correctly.faker.Lorem.Word()for an "unknown property" case, which can occasionally collide with a real property name. Both now use a fixed, clearly-fake property name.CLAUDE.md: update the supported-TFM list to include net10.0.README.md: fix a broken case-insensitive-appendix example (eq$should beeqifor the shown config), and fix two code snippets that did not compile (malformed string literals).Verification
Reproduced the DoS with the
verify-querykitskill before the fix: a filter with 2,000 nested parentheses drove the driver process to 3.4 GB RSS and 76.7% CPU with no return. After the fix, the same input throwsQueryKitNestingDepthExceededExceptionin under 1 second.Every commit leaves
QueryKit.UnitTests(213 tests) andQueryKit.IntegrationTests(212 tests, real Postgres via Testcontainers) fully green.Out of scope
This PR intentionally does not include:
PreventSort/PreventFiltersilent-drop policy question (needs an owner decision).These come from the same review pass and are tracked separately for follow-up PRs.
Test plan
dotnet test QueryKit.UnitTests/— 213 passed, 0 skippeddotnet test QueryKit.IntegrationTests/— 212 passed, 0 skippedverify-querykit