From 6e358eb26cdfd707a17470e63ee41ebccb967e10 Mon Sep 17 00:00:00 2001 From: Daniel Tschinder Date: Sun, 11 Feb 2018 17:58:48 +0100 Subject: [PATCH] Fix infinite loop on invalid queries in OverlappingFields `OverlappingFieldsCanBeMerged` would infinite loop when passed something like ```graphql fragment A on User { name ...A } ``` It's not `OverlappingFieldsCanBeMerged`'s responsibility to detect that validation error, but we still would ideally avoid infinite looping. This detects that case, and pretends that the infinite spread wasn't there for the purposes of this validation step. Also, by memoizing and checking for self-references this removes duplicate reports. ref: graphql/graphql-js#1111 --- .../Rules/OverlappingFieldsCanBeMerged.php | 123 +++++++++++------- .../OverlappingFieldsCanBeMergedTest.php | 60 ++++++++- 2 files changed, 137 insertions(+), 46 deletions(-) diff --git a/src/Validator/Rules/OverlappingFieldsCanBeMerged.php b/src/Validator/Rules/OverlappingFieldsCanBeMerged.php index 13f5fcf..0867be6 100644 --- a/src/Validator/Rules/OverlappingFieldsCanBeMerged.php +++ b/src/Validator/Rules/OverlappingFieldsCanBeMerged.php @@ -49,7 +49,7 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule * dramatically improve the performance of this validator. * @var PairSet */ - private $comparedFragments; + private $comparedFragmentPairs; /** * A cache for the "field map" and list of fragment names found in any given @@ -62,7 +62,7 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule public function getVisitor(ValidationContext $context) { - $this->comparedFragments = new PairSet(); + $this->comparedFragmentPairs = new PairSet(); $this->cachedFieldsAndFragmentNames = new \SplObjectStorage(); return [ @@ -174,29 +174,34 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule $fieldMap ); - // (B) Then collect conflicts between these fields and those represented by - // each spread fragment name found. + $fragmentNamesLength = count($fragmentNames); - for ($i = 0; $i < $fragmentNamesLength; $i++) { - $this->collectConflictsBetweenFieldsAndFragment( - $context, - $conflicts, - false, - $fieldMap, - $fragmentNames[$i] - ); - // (C) Then compare this fragment with all other fragments found in this - // selection set to collect conflicts between fragments spread together. - // This compares each item in the list of fragment names to every other item - // in that same list (except for itself). - for ($j = $i + 1; $j < $fragmentNamesLength; $j++) { - $this->collectConflictsBetweenFragments( + if ($fragmentNamesLength !== 0) { + // (B) Then collect conflicts between these fields and those represented by + // each spread fragment name found. + $comparedFragments = []; + for ($i = 0; $i < $fragmentNamesLength; $i++) { + $this->collectConflictsBetweenFieldsAndFragment( $context, $conflicts, + $comparedFragments, false, - $fragmentNames[$i], - $fragmentNames[$j] + $fieldMap, + $fragmentNames[$i] ); + // (C) Then compare this fragment with all other fragments found in this + // selection set to collect conflicts between fragments spread together. + // This compares each item in the list of fragment names to every other item + // in that same list (except for itself). + for ($j = $i + 1; $j < $fragmentNamesLength; $j++) { + $this->collectConflictsBetweenFragments( + $context, + $conflicts, + false, + $fragmentNames[$i], + $fragmentNames[$j] + ); + } } } @@ -209,6 +214,7 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule * * @param ValidationContext $context * @param array $conflicts + * @param array $comparedFragments * @param bool $areMutuallyExclusive * @param array $fieldMap * @param string $fragmentName @@ -216,10 +222,16 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule private function collectConflictsBetweenFieldsAndFragment( ValidationContext $context, array &$conflicts, + array &$comparedFragments, $areMutuallyExclusive, array $fieldMap, $fragmentName ) { + if (isset($comparedFragments[$fragmentName])) { + return; + } + $comparedFragments[$fragmentName] = true; + $fragment = $context->getFragment($fragmentName); if (!$fragment) { return; @@ -230,6 +242,10 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule $fragment ); + if ($fieldMap === $fieldMap2) { + return; + } + // (D) First collect any conflicts between the provided collection of fields // and the collection of fields represented by the given fragment. $this->collectConflictsBetween( @@ -247,6 +263,7 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule $this->collectConflictsBetweenFieldsAndFragment( $context, $conflicts, + $comparedFragments, $areMutuallyExclusive, $fieldMap, $fragmentNames2[$i] @@ -271,24 +288,32 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule $fragmentName1, $fragmentName2 ) { - $fragment1 = $context->getFragment($fragmentName1); - $fragment2 = $context->getFragment($fragmentName2); - if (!$fragment1 || !$fragment2) { - return; - } - // No need to compare a fragment to itself. - if ($fragment1 === $fragment2) { + if ($fragmentName1 === $fragmentName2) { return; } // Memoize so two fragments are not compared for conflicts more than once. if ( - $this->comparedFragments->has($fragmentName1, $fragmentName2, $areMutuallyExclusive) + $this->comparedFragmentPairs->has( + $fragmentName1, + $fragmentName2, + $areMutuallyExclusive + ) ) { return; } - $this->comparedFragments->add($fragmentName1, $fragmentName2, $areMutuallyExclusive); + $this->comparedFragmentPairs->add( + $fragmentName1, + $fragmentName2, + $areMutuallyExclusive + ); + + $fragment1 = $context->getFragment($fragmentName1); + $fragment2 = $context->getFragment($fragmentName2); + if (!$fragment1 || !$fragment2) { + return; + } list($fieldMap1, $fragmentNames1) = $this->getReferencedFieldsAndFragmentNames( $context, @@ -382,27 +407,35 @@ class OverlappingFieldsCanBeMerged extends AbstractValidationRule // (I) Then collect conflicts between the first collection of fields and // those referenced by each fragment name associated with the second. $fragmentNames2Length = count($fragmentNames2); - for ($j = 0; $j < $fragmentNames2Length; $j++) { - $this->collectConflictsBetweenFieldsAndFragment( - $context, - $conflicts, - $areMutuallyExclusive, - $fieldMap1, - $fragmentNames2[$j] - ); + if ($fragmentNames2Length !== 0) { + $comparedFragments = []; + for ($j = 0; $j < $fragmentNames2Length; $j++) { + $this->collectConflictsBetweenFieldsAndFragment( + $context, + $conflicts, + $comparedFragments, + $areMutuallyExclusive, + $fieldMap1, + $fragmentNames2[$j] + ); + } } // (I) Then collect conflicts between the second collection of fields and // those referenced by each fragment name associated with the first. $fragmentNames1Length = count($fragmentNames1); - for ($i = 0; $i < $fragmentNames2Length; $i++) { - $this->collectConflictsBetweenFieldsAndFragment( - $context, - $conflicts, - $areMutuallyExclusive, - $fieldMap2, - $fragmentNames1[$i] - ); + if ($fragmentNames1Length !== 0) { + $comparedFragments = []; + for ($i = 0; $i < $fragmentNames2Length; $i++) { + $this->collectConflictsBetweenFieldsAndFragment( + $context, + $conflicts, + $comparedFragments, + $areMutuallyExclusive, + $fieldMap2, + $fragmentNames1[$i] + ); + } } // (J) Also collect conflicts between any fragment names by the first and diff --git a/tests/Validator/OverlappingFieldsCanBeMergedTest.php b/tests/Validator/OverlappingFieldsCanBeMergedTest.php index 7301473..399af6a 100644 --- a/tests/Validator/OverlappingFieldsCanBeMergedTest.php +++ b/tests/Validator/OverlappingFieldsCanBeMergedTest.php @@ -454,7 +454,7 @@ class OverlappingFieldsCanBeMergedTest extends TestCase deeperField { x: b } - }, + } deepField { deeperField { y @@ -969,6 +969,64 @@ class OverlappingFieldsCanBeMergedTest extends TestCase $this->assertStringEndsWith($hint, $error); } + /** + * @it does not infinite loop on recursive fragment + */ + public function testDoesNotInfiniteLoopOnRecursiveFragment() + { + $this->expectPassesRule(new OverlappingFieldsCanBeMerged, ' + fragment fragA on Human { name, relatives { name, ...fragA } } + '); + } + + /** + * @it does not infinite loop on immediately recursive fragment + */ + public function testDoesNotInfiniteLoopOnImmeditelyRecursiveFragment() + { + $this->expectPassesRule(new OverlappingFieldsCanBeMerged, ' + fragment fragA on Human { name, ...fragA } + '); + } + + /** + * @it does not infinite loop on transitively recursive fragment + */ + public function testDoesNotInfiniteLoopOnTransitivelyRecursiveFragment() + { + $this->expectPassesRule(new OverlappingFieldsCanBeMerged, ' + fragment fragA on Human { name, ...fragB } + fragment fragB on Human { name, ...fragC } + fragment fragC on Human { name, ...fragA } + '); + } + + /** + * @it find invalid case even with immediately recursive fragment + */ + public function testFindInvalidCaseEvenWithImmediatelyRecursiveFragment() + { + $this->expectFailsRule(new OverlappingFieldsCanBeMerged, ' + fragment sameAliasesWithDifferentFieldTargets on Dob { + ...sameAliasesWithDifferentFieldTargets + fido: name + fido: nickname + } + ', + [ + FormattedError::create( + OverlappingFieldsCanBeMerged::fieldsConflictMessage( + 'fido', + 'name and nickname are different fields' + ), + [ + new SourceLocation(4, 9), + new SourceLocation(5, 9), + ] + ) + ]); + } + private function getSchema() { $StringBox = null;