Skip to content

Fixed parser denial of service through deep nesting and nested list types - #626

Open
xperiandri wants to merge 2 commits into
validation-dos-fixesfrom
parser-dos-fixes
Open

xperiandri wants to merge 2 commits into
validation-dos-fixesfrom
parser-dos-fixes

Conversation

@xperiandri

Copy link
Copy Markdown
Collaborator

Stacked on the validation DoS fix in validation-dos-fixes, which should be reviewed first; this PR only shows the parser commits.

Problem

Three requests could take a server down through Parser.tryParse, which the ASP.NET Core handler calls before anything else:

  • Deep nesting. Selection sets, list or object values, or list types nested about 1 000 deep overflow the stack in FParsec, which recurses once per level, and terminate the process.
  • Nested list types. A list type nested 100 deep in a variable definition, such as [[…Int…]], never finishes parsing. The type parser tried NonNullType first and backtracked, which parsed every nested level twice.
  • Out-of-range integers. An integer outside the 64-bit range threw OverflowException out of tryParse, so the server answered with an unhandled exception.

Changes

  • Nesting scan. parse/tryParse first run a linear scan, tryFindNestingViolation. It rejects braces, brackets and parentheses nested deeper than DocumentLimitsDefaults.MaxNestingDepth (128) outside of strings and comments. The error is an ordinary syntax error with the line and column, in FParsec's Error in Ln: L Col: C form.
    • The scan has to split comments and strings exactly as the grammar does. Otherwise a bracket the scan skips but the grammar parses would escape the limit.
    • The grammar and the scan now share one set of line terminators, \n, \r, U+2028 and U+2029.
    • The scan has no block strings, because the grammar has none.
    • An adversarial review found both of these gaps in the first version and reproduced a stack overflow through each. The second commit fixes them, with regression tests.
  • List types. A list type is parsed once and then checked for !, which is linear.
  • Integers. Int64.TryParse with the invariant culture replaces the conversion that threw; an out-of-range integer is now a syntax error.
  • Test helper. runOnSmallStack moved to Helpers.fs, so both the validation and the parser tests use it.

Tests

ParserLimitsTests.fs (xUnit) runs every case on a 1 MiB thread stack with a timeout:

  • nesting to the limit for selection sets, list and object values, list types and inline fragments, and nesting beyond it (129, 1 000, 5 000, 20 000) for selection sets;
  • 20 000 unclosed braces;
  • brackets in strings, escaped quotes and comments;
  • runs of quotes, and comments ended by U+2028 or U+2029;
  • line and column reporting;
  • 64-bit range edges and overflow;
  • type parsing unchanged for Int!, [Int!]! and [Int].

The parser and validation limit tests pass in both Debug and Release. The whole unit test project passes: 810 tests passed, 5 skipped as before.

Stacked PRs get no CI run, because the workflow only runs for PRs into master and dev.

🤖 Generated with Claude Code

xperiandri and others added 2 commits October 3, 2026 02:30
…ypes

- `Parser.parse` and `Parser.tryParse` scan the document first and reject braces, brackets and parentheses nested deeper than `DocumentLimitsDefaults.MaxNestingDepth` (128) outside of strings and comments, instead of overflowing the stack
- List types are parsed once before looking for `!`, instead of backtracking from a non-null attempt, which took exponential time in the nesting depth
- An integer out of the 64-bit range is a syntax error instead of an `OverflowException` escaping `tryParse`
- Moved the test helper running code on a 1 MiB stack into `Helpers.fs`

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ings

The scan ended comments only at `\n` and `\r` and skipped `"""` block strings, while the grammar also ends comments and strings at U+2028 and U+2029 and has no block strings, reading `""""` as two empty strings. Brackets after such a separator or a run of quotes were parsed but not counted, so deep nesting still overflowed the stack.

- The scan and the grammar share one set of line terminators
- The scan reads strings exactly as the grammar does, without block strings
- Regression tests go through `Parser.tryParse`

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@xperiandri
xperiandri added this pull request to stack #627 October 3, 2026 01:04
Copilot AI balanced review requested due to automatic review settings October 3, 2026 01:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The security-sensitive parser changes appear coherent, but the stacked dependency and unavailable CI warrant final human review.

Review effort: Balanced
Findings: None

What changed in this PR

Adds parser-level protections against denial-of-service inputs while preserving normal GraphQL parsing behavior.

Changes:

  • Enforces a nesting-depth limit before parsing.
  • Makes nested list-type parsing linear and integer overflow recoverable.
  • Adds focused parser-limit tests and a shared small-stack test helper.
File Description
src/​FSharp.Data.GraphQL.Shared/​Parser.fs Implements nesting scanning and parser fixes.
src/​FSharp.Data.GraphQL.Shared/​DocumentLimits.fs Documents parser and validation limits.
tests/​FSharp.Data.GraphQL.Tests/​ParserLimitsTests.fs Covers nesting, list types, strings, comments, and integers.
tests/​FSharp.Data.GraphQL.Tests/​Helpers.fs Adds the shared small-stack runner.
tests/​FSharp.Data.GraphQL.Tests/​ValidationDoSTests.fs Uses the shared test helper.
tests/​FSharp.Data.GraphQL.Tests/​FSharp.Data.GraphQL.Tests.fsproj Includes the new test file.
RELEASE_NOTES.md Records security fixes and the breaking limit.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Test Results

    9 files  ± 0      9 suites  ±0   14m 39s ⏱️ +21s
  923 tests +22    918 ✅ +22   5 💤 ±0  0 ❌ ±0 
2 769 runs  +66  2 754 ✅ +66  15 💤 ±0  0 ❌ ±0 

Results for commit f9077f9. ± Comparison against base commit df09a89.

♻️ This comment has been updated with latest results.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants