Skip to content

Commit 8b4ecfa

Browse files
GHG-bguestbcmyguest
authored andcommitted
fix: deduplicate fields in OverlappingFieldsCanBeMerged to prevent DoS
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
1 parent 56d4338 commit 8b4ecfa

3 files changed

Lines changed: 85 additions & 3 deletions

File tree

src/graphql/validation/rules/overlapping_fields_can_be_merged.py

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -539,8 +539,11 @@ def collect_conflicts_within(
539539
# (except to itself). If the list only has one item, nothing needs to be
540540
# compared.
541541
if len(fields) > 1:
542-
for i, field in enumerate(fields):
543-
for other_field in fields[i + 1 :]:
542+
# Deduplicate structurally identical fields to avoid quadratic blowup
543+
# when a query repeats the same field many times.
544+
unique_fields = deduplicate_fields(fields)
545+
for i, field in enumerate(unique_fields):
546+
for other_field in unique_fields[i + 1 :]:
544547
conflict = find_conflict(
545548
context,
546549
cached_fields_and_fragment_spreads,
@@ -558,6 +561,36 @@ def collect_conflicts_within(
558561
conflicts.append(conflict)
559562

560563

564+
def deduplicate_fields(fields: list[NodeAndDef]) -> list[NodeAndDef]:
565+
"""Deduplicate structurally identical fields.
566+
567+
Fields that are structurally identical (same parent type, field name, arguments,
568+
directives and selection set) can never conflict with each other, so only one of
569+
them needs to take part in the pairwise conflict comparison.
570+
"""
571+
unique: list[NodeAndDef] = []
572+
seen: set[str] = set()
573+
for field in fields:
574+
key = field_fingerprint(field)
575+
if key not in seen:
576+
seen.add(key)
577+
unique.append(field)
578+
return unique
579+
580+
581+
def field_fingerprint(field: NodeAndDef) -> str:
582+
"""Build a fingerprint identifying the structure of a given field."""
583+
parent_type, node, _ = field
584+
parent_type_id = id(parent_type) if parent_type else ""
585+
selection_set_id = id(node.selection_set) if node.selection_set else ""
586+
fingerprint = f"{parent_type_id}:{node.name.value}:{selection_set_id}"
587+
for argument in node.arguments or ():
588+
fingerprint += f":{argument.name.value}={print_ast(argument.value)}"
589+
for directive in node.directives or ():
590+
fingerprint += f":{print_ast(directive)}"
591+
return fingerprint
592+
593+
561594
def collect_conflicts_between(
562595
context: ValidationContext,
563596
conflicts: list[Conflict],
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import pytest
2+
3+
from graphql import (
4+
GraphQLField,
5+
GraphQLObjectType,
6+
GraphQLSchema,
7+
GraphQLString,
8+
parse,
9+
validate,
10+
)
11+
12+
schema = GraphQLSchema(
13+
query=GraphQLObjectType(
14+
name="Query",
15+
fields={"hello": GraphQLField(GraphQLString)},
16+
)
17+
)
18+
19+
20+
@pytest.mark.parametrize("count", [100, 500, 1000, 2000, 3000])
21+
def test_validate_many_repeated_fields(benchmark, count):
22+
document = parse(f"{{ {'hello ' * count}}}")
23+
result = benchmark(lambda: validate(schema, document))
24+
assert result == []

tests/validation/test_overlapping_fields_can_be_merged.py

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
from functools import partial
22

3+
from graphql import parse, validate
34
from graphql.utilities import build_schema
45
from graphql.validation import OverlappingFieldsCanBeMergedRule
56

6-
from .harness import assert_validation_errors
7+
from .harness import assert_validation_errors, test_schema
78

89
assert_errors = partial(assert_validation_errors, OverlappingFieldsCanBeMergedRule)
910

@@ -1565,6 +1566,30 @@ def does_not_infinite_loop_on_transitively_recursive_fragments():
15651566
"""
15661567
)
15671568

1569+
def many_repeated_fields_do_not_cause_quadratic_blowup():
1570+
repeated_fields = "name " * 3000
1571+
assert_valid(
1572+
f"""
1573+
fragment manyRepeatedFields on Dog {{
1574+
{repeated_fields}
1575+
}}
1576+
"""
1577+
)
1578+
1579+
def many_repeated_fields_with_conflict_still_detected():
1580+
repeated_fields = "name " * 100
1581+
doc = parse(
1582+
f"""
1583+
fragment conflictsAmongMany on Dog {{
1584+
{repeated_fields}
1585+
name: nickname
1586+
}}
1587+
"""
1588+
)
1589+
errors = validate(test_schema, doc, [OverlappingFieldsCanBeMergedRule])
1590+
assert errors
1591+
assert "'name' and 'nickname' are different fields" in errors[0].message
1592+
15681593
def finds_invalid_case_even_with_immediately_recursive_fragment():
15691594
assert_errors(
15701595
"""

0 commit comments

Comments
 (0)