fix: deduplicate fields in OverlappingFieldsCanBeMerged to prevent DoS#270
Open
bcmyguest wants to merge 3 commits into
Open
fix: deduplicate fields in OverlappingFieldsCanBeMerged to prevent DoS#270bcmyguest wants to merge 3 commits into
bcmyguest wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR mitigates a denial-of-service vector in OverlappingFieldsCanBeMergedRule by deduplicating structurally identical fields before performing pairwise conflict checks, reducing worst-case validation time for queries with many repeated fields.
Changes:
- Deduplicate identical fields within a response-name bucket before pairwise conflict comparison.
- Add regression tests to ensure repeated fields don’t trigger quadratic blowup and that conflicts among many repeats are still detected.
- Add a validation-focused benchmark covering 100–3000 repeated fields.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/graphql/validation/rules/overlapping_fields_can_be_merged.py |
Adds field deduplication helpers and applies them in collect_conflicts_within to reduce quadratic comparisons. |
tests/validation/test_overlapping_fields_can_be_merged.py |
Adds regression tests for repeated fields performance and conflict detection among many repeats. |
tests/benchmarks/test_validate_repeated_fields.py |
Introduces a benchmark for validation performance on repeated fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bcmyguest
force-pushed
the
fix-overlapping-fields-dos
branch
from
4b6e1da to
8b4ecfa
Compare
bcmyguest
force-pushed
the
fix-overlapping-fields-dos
branch
from
8b4ecfa to
8d8a02a
Compare
The validation rule had O(n²) time complexity when processing queries
with many repeated fields sharing the same response name. A query like
`{ hello hello hello ... }` with thousands of fields caused excessive
CPU usage, exploitable as a denial of service vector.
Fields that are structurally identical (same parent type, field name,
arguments, directives, and selection set) can never conflict with each
other, so we deduplicate them before pairwise comparison.
This ports the graphql-php fix for GHSA-68jq-c3rv-pcrr:
webonyx/graphql-php@b5e7c99
Assisted-by: Claude:claude-fable-5
bcmyguest
force-pushed
the
fix-overlapping-fields-dos
branch
from
8d8a02a to
bd41cf7
Compare
field_fingerprint used id(node.selection_set) and printed arguments and directives in source order, so repeated composite fields, and fields whose arguments, directives, or nested arguments were merely reordered, failed to deduplicate. That left the O(n^2) pairwise-comparison DoS shape exploitable by reordering semantically irrelevant parts of an otherwise repeated field. Build the dedup key from a canonical nested tuple of the field's AST: arguments (including directive and nested-field arguments) sorted by name with normalized values, directives and sibling selections in a stable order, and selection sets compared structurally. This matches the equivalence relation used by find_conflict, so reordering can no longer defeat dedup. A nested tuple is used as the key so shared sub-selections are referenced rather than repeatedly copied into a string. Add regression tests for repeated composite fields and for reordered top-level and nested arguments. Assisted-by: Claude:claude-fable-5
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
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
The
OverlappingFieldsCanBeMergedRulevalidation rule has O(n²) time complexity when processing queries with many repeated fields sharing the same response name. A query like{ hello hello hello ... }with thousands of fields causes excessive CPU usage, exploitable as a denial-of-service vector.Fields that are structurally identical (same parent type, field name, arguments, directives, and selection set) can never conflict with each other, so this deduplicates them before the pairwise comparison in
collect_conflicts_within.This ports the equivalent graphql-php fix, webonyx/graphql-php@b5e7c99, published as security advisory GHSA-68jq-c3rv-pcrr.
One deliberate difference from the PHP fingerprint: printed directives are included, because graphql-core additionally checks stream-directive conflicts (
same_streams); without them,nameandname @stream(...)would deduplicate together and that conflict would be missed.Validating a query with 3000 repeated fields drops from ~5.2s to ~0.11s on my machine.
Changes
src/graphql/validation/rules/overlapping_fields_can_be_merged.py: deduplicate structurally identical fields incollect_conflicts_within(newdeduplicate_fields/field_fingerprinthelpers)tests/validation/test_overlapping_fields_can_be_merged.py: regression tests — 3000 repeated fields validate without quadratic blowup, and a conflict among many repeated fields is still detectedtests/benchmarks/test_validate_repeated_fields.py: validation-only benchmark over 100–3000 repeated fields, mirroring the upstreamOverlappingFieldsCanBeMergedBenchTesting
pytest tests/validation/— 693 passedpytest tests/benchmarks/test_validate_repeated_fields.py --benchmark-disable— 5 passedAI Disclosure
Used Claude:fable-5 to generate this code.