fix: deduplicate fields in OverlappingFieldsCanBeMerged to prevent DoS#270
fix: deduplicate fields in OverlappingFieldsCanBeMerged to prevent DoS#270bcmyguest wants to merge 5 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>
The rule compares overlapping fields pairwise, which is quadratic in the number
of fields sharing a response name. Two query shapes trigger this:
- Identical repeated fields ({ name name name ... }): handled by deduplicating
structurally identical fields, as graphql-php did (CVE-2026-40476). This also
extends that dedup to the "between fragments" comparison path, which was still
quadratic (and, with the budget below, could falsely reject valid queries).
- Distinct conflicting fields ({ x:f(a:0) x:f(a:1) ... }): cannot be deduped
because they genuinely differ, so a shared comparison budget bounds them,
reporting one "too complex to validate" error. Overridable via max_comparisons.
Valid queries do not approach the budget in practice: identical fields dedup,
and distinct fields under one response name conflict (so are already invalid).
The budget idea is not from graphql-php (dedup only); it follows the reporter's
proof-of-concept.
Assisted-by: Claude:claude-opus-4-8
bcmyguest
force-pushed
the
fix-overlapping-fields-dos
branch
from
ebba2ab to
3aad157
Compare
| @pytest.mark.timeout(5) | ||
| def many_repeated_fields_do_not_cause_quadratic_blowup(): | ||
| repeated_fields = "name " * 3000 |
| @pytest.mark.timeout(5) | ||
| def many_fields_with_differing_arguments_are_rejected_as_too_complex(): | ||
| # Fields differing only in their arguments genuinely conflict, so they |
| # Sorted so that reordering sibling selections does not change the result. | ||
| return tuple(sorted(map(_canonical_selection, selection_set.selections))) |
The fingerprint sorted sibling selections but kept duplicates, so fields whose
sub-selections differ only by redundant repeats (e.g. `f { a }` vs `f { a a }`)
got distinct fingerprints and failed to deduplicate. A valid query using many
such variants under one response name could then exhaust the comparison budget
and be wrongly rejected as "too complex". Repeating an identical sibling is
redundant and never conflicts, so de-duplicate the canonical selections to match
the equivalence find_conflict uses.
Reported by GitHub Copilot review.
Assisted-by: Claude:claude-opus-4-8
|
Hi @bcmyguest. Thanks for reporting this. Just so you're not left wondering: I've seen it, it's a real issue, and it's actively being worked on. I've been developing a fix along similar lines to where your PR ended up: a comparison budget that bounds the worst case, since deduplication alone doesn't cover fields with genuinely differing arguments (as you found). Because graphql-core is a close port of graphql-js and the same rule is affected upstream, it's always best to fix it upstream first and keep deviations and custom fixes in graphql-core very small and contained. I'd like to coordinate with the graphql-js maintainers on how best to handle it before I finalize, so we don't diverge from upstream more than necessary, likely a few more days. I'll follow up here once it's settled. One thing for the future: for security-sensitive issues like this, a private heads-up by email, or via GitHub's "Report a vulnerability" (private advisory) on the repo before a public PR gives me room to prepare a fix and coordinate with upstream before it's out in the open. graphql-core is volunteer-maintained, so that bit of slack really helps. I believe in this case it did no harm since the problem has already been reported elsewhere anyway, just something to keep in mind. Thanks again for the careful report and the work on the PR. |
|
Sorry about that. |
|
No worries. Luckily, 3.3 has no yet been released, so it will contain the fix, and I will also backport it to 3.2. If you have any other fixes that should go into 3.3 let me know. |
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.