Skip to content

Commit ebba2ab

Browse files
committed
fix: bound OverlappingFieldsCanBeMerged comparisons to prevent DoS
Field deduplication only collapses structurally identical fields, so a query placing many fields with differing arguments under one response name still forces a quadratic number of pairwise comparisons (e.g. `x:f(a:0) x:f(a:1) ...`), each of which cannot be deduplicated because the fields genuinely conflict. Add a shared, cumulative comparison budget (ComparisonBudget) charged once per find_conflict call. When exhausted it raises ComparisonBudgetExceededError, caught by the rule, which reports a single "too complex to validate" error and stops. This bounds validation of hostile queries (roughly sub-second) across both within-set and between-fragment comparisons. Valid queries never approach the limit: distinct-argument fields under one response name always conflict and are therefore invalid, and identical fields deduplicate to one. The limit is exposed as the overridable class attribute max_comparisons. Assisted-by: Claude:claude-opus-4-8
1 parent 82a9884 commit ebba2ab

2 files changed

Lines changed: 132 additions & 9 deletions

File tree

src/graphql/validation/rules/overlapping_fields_can_be_merged.py

Lines changed: 97 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from __future__ import annotations
44

55
from itertools import chain
6-
from typing import TYPE_CHECKING, Any, NamedTuple, TypeAlias, cast
6+
from typing import TYPE_CHECKING, Any, ClassVar, NamedTuple, TypeAlias, cast
77

88
from ...error import GraphQLError
99
from ...language import (
@@ -63,6 +63,15 @@ class OverlappingFieldsCanBeMergedRule(ValidationRule):
6363
See https://spec.graphql.org/draft/#sec-Field-Selection-Merging
6464
"""
6565

66+
# Maximum number of pairwise field comparisons allowed for a single document.
67+
# Structurally identical fields are deduplicated before comparison, but a query
68+
# can still force a quadratic number of comparisons by placing many fields with
69+
# differing arguments under the same response name. Such queries are always
70+
# invalid (differing arguments conflict), so a valid query never approaches this
71+
# limit; it exists purely to keep validation of hostile queries bounded (roughly
72+
# sub-second). Override on a subclass to trade strictness for headroom.
73+
max_comparisons: ClassVar[int] = 100_000
74+
6675
def __init__(self, context: ValidationContext) -> None:
6776
super().__init__(context)
6877
# A memoization for when fields and a fragment or two fragments are compared
@@ -76,15 +85,36 @@ def __init__(self, context: ValidationContext) -> None:
7685
# times, so this improves the performance of this validator.
7786
self.cached_fields_and_fragment_spreads: dict = {}
7887

88+
# A shared budget bounding the total number of pairwise field comparisons,
89+
# so a hostile query cannot force quadratic work. See ``max_comparisons``.
90+
self.comparison_budget = ComparisonBudget(self.max_comparisons)
91+
self._too_complex = False
92+
7993
def enter_selection_set(self, selection_set: SelectionSetNode, *_args: Any) -> None:
80-
conflicts = find_conflicts_within_selection_set(
81-
self.context,
82-
self.cached_fields_and_fragment_spreads,
83-
self.compared_fields_and_fragment_pairs,
84-
self.compared_fragment_pairs,
85-
self.context.get_parent_type(),
86-
selection_set,
87-
)
94+
if self._too_complex:
95+
return
96+
try:
97+
conflicts = find_conflicts_within_selection_set(
98+
self.context,
99+
self.cached_fields_and_fragment_spreads,
100+
self.compared_fields_and_fragment_pairs,
101+
self.compared_fragment_pairs,
102+
self.comparison_budget,
103+
self.context.get_parent_type(),
104+
selection_set,
105+
)
106+
except ComparisonBudgetExceededError:
107+
# The query forced more field comparisons than allowed. Report a single
108+
# error and stop checking so validation stays bounded.
109+
self._too_complex = True
110+
self.report_error(
111+
GraphQLError(
112+
"Query is too complex to validate for overlapping fields."
113+
" Reduce the number of fields sharing a response name.",
114+
selection_set,
115+
)
116+
)
117+
return
88118
for (reason_name, reason), fields1, fields2 in conflicts:
89119
reason_msg = reason_message(reason)
90120
self.report_error(
@@ -176,6 +206,7 @@ def find_conflicts_within_selection_set(
176206
cached_fields_and_fragment_spreads: dict,
177207
compared_fields_and_fragment_pairs: OrderedPairSet,
178208
compared_fragment_pairs: PairSet,
209+
budget: ComparisonBudget,
179210
parent_type: GraphQLNamedType | None,
180211
selection_set: SelectionSetNode,
181212
) -> list[Conflict]:
@@ -200,6 +231,7 @@ def find_conflicts_within_selection_set(
200231
cached_fields_and_fragment_spreads,
201232
compared_fields_and_fragment_pairs,
202233
compared_fragment_pairs,
234+
budget,
203235
field_map,
204236
)
205237

@@ -213,6 +245,7 @@ def find_conflicts_within_selection_set(
213245
cached_fields_and_fragment_spreads,
214246
compared_fields_and_fragment_pairs,
215247
compared_fragment_pairs,
248+
budget,
216249
False,
217250
field_map,
218251
fragment_spread,
@@ -228,6 +261,7 @@ def find_conflicts_within_selection_set(
228261
cached_fields_and_fragment_spreads,
229262
compared_fields_and_fragment_pairs,
230263
compared_fragment_pairs,
264+
budget,
231265
False,
232266
fragment_spread,
233267
other_fragment_spread,
@@ -242,6 +276,7 @@ def collect_conflicts_between_fields_and_fragment(
242276
cached_fields_and_fragment_spreads: dict,
243277
compared_fields_and_fragment_pairs: OrderedPairSet,
244278
compared_fragment_pairs: PairSet,
279+
budget: ComparisonBudget,
245280
are_mutually_exclusive: bool,
246281
field_map: NodeAndDefCollection,
247282
fragment_spread: FragmentSpread,
@@ -288,6 +323,7 @@ def collect_conflicts_between_fields_and_fragment(
288323
cached_fields_and_fragment_spreads,
289324
compared_fields_and_fragment_pairs,
290325
compared_fragment_pairs,
326+
budget,
291327
are_mutually_exclusive,
292328
field_map,
293329
None,
@@ -304,6 +340,7 @@ def collect_conflicts_between_fields_and_fragment(
304340
cached_fields_and_fragment_spreads,
305341
compared_fields_and_fragment_pairs,
306342
compared_fragment_pairs,
343+
budget,
307344
are_mutually_exclusive,
308345
field_map,
309346
referenced_fragment_spread,
@@ -316,6 +353,7 @@ def collect_conflicts_between_fragments(
316353
cached_fields_and_fragment_spreads: dict,
317354
compared_fields_and_fragment_pairs: OrderedPairSet,
318355
compared_fragment_pairs: PairSet,
356+
budget: ComparisonBudget,
319357
are_mutually_exclusive: bool,
320358
fragment_spread1: FragmentSpread,
321359
fragment_spread2: FragmentSpread,
@@ -387,6 +425,7 @@ def collect_conflicts_between_fragments(
387425
cached_fields_and_fragment_spreads,
388426
compared_fields_and_fragment_pairs,
389427
compared_fragment_pairs,
428+
budget,
390429
are_mutually_exclusive,
391430
field_map1,
392431
fragment_spread1.var_map,
@@ -403,6 +442,7 @@ def collect_conflicts_between_fragments(
403442
cached_fields_and_fragment_spreads,
404443
compared_fields_and_fragment_pairs,
405444
compared_fragment_pairs,
445+
budget,
406446
are_mutually_exclusive,
407447
fragment_spread1,
408448
referenced_fragment_spread2,
@@ -417,6 +457,7 @@ def collect_conflicts_between_fragments(
417457
cached_fields_and_fragment_spreads,
418458
compared_fields_and_fragment_pairs,
419459
compared_fragment_pairs,
460+
budget,
420461
are_mutually_exclusive,
421462
referenced_fragment_spread1,
422463
fragment_spread2,
@@ -428,6 +469,7 @@ def find_conflicts_between_sub_selection_sets(
428469
cached_fields_and_fragment_spreads: dict,
429470
compared_fields_and_fragment_pairs: OrderedPairSet,
430471
compared_fragment_pairs: PairSet,
472+
budget: ComparisonBudget,
431473
are_mutually_exclusive: bool,
432474
parent_type1: GraphQLNamedType | None,
433475
selection_set1: SelectionSetNode,
@@ -466,6 +508,7 @@ def find_conflicts_between_sub_selection_sets(
466508
cached_fields_and_fragment_spreads,
467509
compared_fields_and_fragment_pairs,
468510
compared_fragment_pairs,
511+
budget,
469512
are_mutually_exclusive,
470513
field_map1,
471514
var_map1,
@@ -483,6 +526,7 @@ def find_conflicts_between_sub_selection_sets(
483526
cached_fields_and_fragment_spreads,
484527
compared_fields_and_fragment_pairs,
485528
compared_fragment_pairs,
529+
budget,
486530
are_mutually_exclusive,
487531
field_map1,
488532
fragment_spread2,
@@ -498,6 +542,7 @@ def find_conflicts_between_sub_selection_sets(
498542
cached_fields_and_fragment_spreads,
499543
compared_fields_and_fragment_pairs,
500544
compared_fragment_pairs,
545+
budget,
501546
are_mutually_exclusive,
502547
field_map2,
503548
fragment_spread1,
@@ -514,6 +559,7 @@ def find_conflicts_between_sub_selection_sets(
514559
cached_fields_and_fragment_spreads,
515560
compared_fields_and_fragment_pairs,
516561
compared_fragment_pairs,
562+
budget,
517563
are_mutually_exclusive,
518564
fragment_spread1,
519565
fragment_spread2,
@@ -528,6 +574,7 @@ def collect_conflicts_within(
528574
cached_fields_and_fragment_spreads: dict,
529575
compared_fields_and_fragment_pairs: OrderedPairSet,
530576
compared_fragment_pairs: PairSet,
577+
budget: ComparisonBudget,
531578
field_map: NodeAndDefCollection,
532579
) -> None:
533580
"""Collect all Conflicts "within" one collection of fields."""
@@ -550,6 +597,7 @@ def collect_conflicts_within(
550597
cached_fields_and_fragment_spreads,
551598
compared_fields_and_fragment_pairs,
552599
compared_fragment_pairs,
600+
budget,
553601
# within one collection is never mutually exclusive
554602
False,
555603
response_name,
@@ -661,6 +709,7 @@ def collect_conflicts_between(
661709
cached_fields_and_fragment_spreads: dict,
662710
compared_fields_and_fragment_pairs: OrderedPairSet,
663711
compared_fragment_pairs: PairSet,
712+
budget: ComparisonBudget,
664713
parent_fields_are_mutually_exclusive: bool,
665714
field_map1: NodeAndDefCollection,
666715
var_map1: VarMap,
@@ -689,6 +738,7 @@ def collect_conflicts_between(
689738
cached_fields_and_fragment_spreads,
690739
compared_fields_and_fragment_pairs,
691740
compared_fragment_pairs,
741+
budget,
692742
parent_fields_are_mutually_exclusive,
693743
response_name,
694744
field1,
@@ -705,6 +755,7 @@ def find_conflict(
705755
cached_fields_and_fragment_spreads: dict,
706756
compared_fields_and_fragment_pairs: OrderedPairSet,
707757
compared_fragment_pairs: PairSet,
758+
budget: ComparisonBudget,
708759
parent_fields_are_mutually_exclusive: bool,
709760
response_name: str,
710761
field1: NodeAndDef,
@@ -717,6 +768,10 @@ def find_conflict(
717768
Determines if there is a conflict between two particular fields, including comparing
718769
their sub-fields.
719770
"""
771+
# Charge this comparison against the shared budget. When it is exhausted this
772+
# raises, aborting an otherwise quadratic amount of work on a hostile query.
773+
budget.spend()
774+
720775
parent_type1, node1, def1 = field1
721776
parent_type2, node2, def2 = field2
722777

@@ -778,6 +833,7 @@ def find_conflict(
778833
cached_fields_and_fragment_spreads,
779834
compared_fields_and_fragment_pairs,
780835
compared_fragment_pairs,
836+
budget,
781837
are_mutually_exclusive,
782838
get_named_type(type1),
783839
selection_set1,
@@ -1060,6 +1116,38 @@ def subfield_conflicts(
10601116
return None # no conflict
10611117

10621118

1119+
class ComparisonBudgetExceededError(Exception):
1120+
"""Raised when field comparisons exceed the allowed budget.
1121+
1122+
Signals that a query is too complex to validate for overlapping fields within
1123+
a bounded number of comparisons. Caught by the rule, which reports a single
1124+
error instead of continuing quadratic work.
1125+
"""
1126+
1127+
1128+
class ComparisonBudget:
1129+
"""A shared, decreasing count of allowed pairwise field comparisons.
1130+
1131+
``OverlappingFieldsCanBeMerged`` compares overlapping fields pairwise, which is
1132+
quadratic in the number of fields sharing a response name. Structurally identical
1133+
fields are deduplicated first, but fields differing only in their arguments cannot
1134+
be deduplicated and still force quadratic work. Each comparison spends one unit
1135+
from this budget; :exc:`ComparisonBudgetExceededError` is raised once it is
1136+
exhausted.
1137+
"""
1138+
1139+
__slots__ = ("_remaining",)
1140+
1141+
def __init__(self, limit: int) -> None:
1142+
self._remaining = limit
1143+
1144+
def spend(self) -> None:
1145+
"""Charge a single comparison, raising when the budget is exhausted."""
1146+
if self._remaining <= 0:
1147+
raise ComparisonBudgetExceededError
1148+
self._remaining -= 1
1149+
1150+
10631151
class OrderedPairSet:
10641152
"""Ordered pair set
10651153

tests/validation/test_overlapping_fields_can_be_merged.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1698,3 +1698,38 @@ def does_not_infinite_loop_on_recursive_fragments_separated_by_fields():
16981698
}
16991699
"""
17001700
)
1701+
1702+
@pytest.mark.timeout(5)
1703+
def many_fields_with_differing_arguments_are_rejected_as_too_complex():
1704+
# Fields differing only in their arguments genuinely conflict, so they
1705+
# cannot be deduplicated and would otherwise force a quadratic number of
1706+
# comparisons. The comparison budget bounds this by rejecting the query
1707+
# with a single error instead of validating every pair.
1708+
distinct_fields = " ".join(
1709+
f"p: isAtLocation(x: {i})" for i in range(2000)
1710+
)
1711+
doc = parse(
1712+
f"""
1713+
fragment manyDistinctArguments on Dog {{
1714+
{distinct_fields}
1715+
}}
1716+
"""
1717+
)
1718+
errors = validate(test_schema, doc, [OverlappingFieldsCanBeMergedRule])
1719+
assert errors
1720+
assert "too complex to validate" in errors[0].message
1721+
1722+
def modest_differing_arguments_still_report_the_real_conflict():
1723+
# Below the budget the ordinary, specific conflict must still be reported
1724+
# rather than the "too complex" fallback.
1725+
doc = parse(
1726+
"""
1727+
fragment fewDistinctArguments on Dog {
1728+
p: isAtLocation(x: 0)
1729+
p: isAtLocation(x: 1)
1730+
}
1731+
"""
1732+
)
1733+
errors = validate(test_schema, doc, [OverlappingFieldsCanBeMergedRule])
1734+
assert errors
1735+
assert "they have differing arguments" in errors[0].message

0 commit comments

Comments
 (0)