From c58ab2a6941450d428990a892a416264daa7755a Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Tue, 19 May 2026 09:16:47 +1000 Subject: [PATCH] Reject indirect directive definition cycles --- .../idl/SchemaTypeDirectivesChecker.java | 119 +++++++++++++++++- .../DirectiveIllegalReferenceError.java | 9 +- .../schema/idl/SchemaGeneratorTest.groovy | 41 ++++++ .../SchemaTypeDirectivesCheckerTest.groovy | 45 +++++++ 4 files changed, 212 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java b/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java index 4c3e373e37..b5d4ce13da 100644 --- a/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java +++ b/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java @@ -30,11 +30,17 @@ import graphql.schema.idl.errors.MissingTypeError; import graphql.schema.idl.errors.NotAnInputTypeError; +import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; +import static graphql.Assert.assertNotNull; import static graphql.introspection.Introspection.DirectiveLocation.ARGUMENT_DEFINITION; import static graphql.introspection.Introspection.DirectiveLocation.ENUM; import static graphql.introspection.Introspection.DirectiveLocation.ENUM_VALUE; @@ -182,6 +188,10 @@ private static boolean isNoNullArgWithoutDefaultValue(InputValueDefinition defin } private void commonCheck(Collection directiveDefinitions, List errors) { + List directiveDefinitionsList = new ArrayList<>(directiveDefinitions); + Map directiveDefinitionsByName = getByName(directiveDefinitionsList, DirectiveDefinition::getName, mergeFirst()); + Map> directiveReferencesByName = directiveReferencesByName(directiveDefinitionsByName); + directiveDefinitions.forEach(directiveDefinition -> { assertTypeName(directiveDefinition, errors); directiveDefinition.getInputValueDefinitions().forEach(inputValueDefinition -> { @@ -192,6 +202,113 @@ private void commonCheck(Collection directiveDefinitions, L } }); }); + checkIndirectDirectiveCycles(directiveDefinitionsByName, directiveReferencesByName, errors); + } + + private static Map> directiveReferencesByName( + Map directiveDefinitionsByName) { + Map> result = new LinkedHashMap<>(); + directiveDefinitionsByName.forEach((name, directiveDefinition) -> result.put(name, directiveReferences(directiveDefinition))); + return result; + } + + private static Map directiveReferences(DirectiveDefinition directiveDefinition) { + Map result = new LinkedHashMap<>(); + for (InputValueDefinition inputValueDefinition : directiveDefinition.getInputValueDefinitions()) { + recordDirectiveReferences(directiveDefinition, result, inputValueDefinition); + } + return result; + } + + private static void recordDirectiveReferences(DirectiveDefinition directiveDefinition, + Map result, + InputValueDefinition inputValueDefinition) { + for (Directive directive : inputValueDefinition.getDirectives()) { + if (directive.getName().equals(directiveDefinition.getName())) { + continue; + } + result.putIfAbsent(directive.getName(), inputValueDefinition); + } + } + + private static void checkIndirectDirectiveCycles( + Map directiveDefinitionsByName, + Map> directiveReferencesByName, + List errors) { + Set checked = new LinkedHashSet<>(); + Set visiting = new LinkedHashSet<>(); + List path = new ArrayList<>(); + for (String directiveName : directiveDefinitionsByName.keySet()) { + checkIndirectDirectiveCycles(directiveName, directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors); + } + } + + private static void checkIndirectDirectiveCycles(String directiveName, + Map directiveDefinitionsByName, + Map> directiveReferencesByName, + Set checked, + Set visiting, + List path, + List errors) { + if (checked.contains(directiveName)) { + return; + } + + visiting.add(directiveName); + path.add(directiveName); + checkIndirectDirectiveCycleReferences(directiveName, directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors); + path.remove(path.size() - 1); + visiting.remove(directiveName); + checked.add(directiveName); + } + + private static void checkIndirectDirectiveCycleReferences(String directiveName, + Map directiveDefinitionsByName, + Map> directiveReferencesByName, + Set checked, + Set visiting, + List path, + List errors) { + Map references = directiveReferencesByName.getOrDefault(directiveName, Collections.emptyMap()); + for (Map.Entry entry : references.entrySet()) { + checkIndirectDirectiveCycleReference(entry.getKey(), entry.getValue(), directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors); + } + } + + private static void checkIndirectDirectiveCycleReference(String referencedDirectiveName, + InputValueDefinition inputValueDefinition, + Map directiveDefinitionsByName, + Map> directiveReferencesByName, + Set checked, + Set visiting, + List path, + List errors) { + if (visiting.contains(referencedDirectiveName)) { + addIndirectDirectiveCycleError(referencedDirectiveName, inputValueDefinition, directiveDefinitionsByName, path, errors); + return; + } + if (!checked.contains(referencedDirectiveName)) { + checkIndirectDirectiveCycles(referencedDirectiveName, directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors); + } + } + + private static void addIndirectDirectiveCycleError(String repeatedDirectiveName, + InputValueDefinition inputValueDefinition, + Map directiveDefinitionsByName, + List path, + List errors) { + List cyclePath = directiveCyclePath(repeatedDirectiveName, path); + String cyclePathString = String.join(" -> ", cyclePath); + + DirectiveDefinition directiveDefinition = assertNotNull(directiveDefinitionsByName.get(repeatedDirectiveName)); + errors.add(new DirectiveIllegalReferenceError(directiveDefinition, inputValueDefinition, cyclePathString)); + } + + private static List directiveCyclePath(String repeatedDirectiveName, List path) { + int cycleStart = path.indexOf(repeatedDirectiveName); + List cyclePath = new ArrayList<>(path.subList(cycleStart, path.size())); + cyclePath.add(repeatedDirectiveName); + return cyclePath; } private static void assertTypeName(NamedNode node, List errors) { @@ -224,4 +341,4 @@ private static TypeDefinition findTypeDefFromRegistry(String typeName, TypeDe } return typeRegistry.scalars().get(typeName); } -} \ No newline at end of file +} diff --git a/src/main/java/graphql/schema/idl/errors/DirectiveIllegalReferenceError.java b/src/main/java/graphql/schema/idl/errors/DirectiveIllegalReferenceError.java index 44fb541e53..2bcd34db39 100644 --- a/src/main/java/graphql/schema/idl/errors/DirectiveIllegalReferenceError.java +++ b/src/main/java/graphql/schema/idl/errors/DirectiveIllegalReferenceError.java @@ -12,4 +12,11 @@ public DirectiveIllegalReferenceError(DirectiveDefinition directive, NamedNode l directive.getName(), location.getName(), lineCol(location) )); } -} \ No newline at end of file + + public DirectiveIllegalReferenceError(DirectiveDefinition directive, NamedNode location, String cyclePath) { + super(directive, + String.format("'%s' must not reference itself via directive cycle '%s' on '%s''%s'", + directive.getName(), cyclePath, location.getName(), lineCol(location) + )); + } +} diff --git a/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy index 5c268ebd0f..b6d084e5c4 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy @@ -25,6 +25,7 @@ import graphql.schema.GraphQLType import graphql.schema.GraphQLTypeUtil import graphql.schema.GraphQLUnionType import graphql.schema.GraphqlTypeComparatorRegistry +import graphql.schema.idl.errors.DirectiveIllegalReferenceError import graphql.schema.idl.errors.NotAnInputTypeError import graphql.schema.idl.errors.NotAnOutputTypeError import graphql.schema.idl.errors.SchemaProblem @@ -2270,6 +2271,46 @@ class SchemaGeneratorTest extends Specification { schema != null } + def "#4201 indirect cyclical directive definitions are rejected without stack overflow - #name"() { + given: + def registry = new SchemaParser().parse(sdl) + + when: + UnExecutableSchemaGenerator.makeUnExecutableSchema(registry) + + then: + def e = thrown(SchemaProblem) + e.errors.size() == 1 + e.errors.get(0) instanceof DirectiveIllegalReferenceError + e.errors.get(0).getMessage().contains(cycleMessage) + + where: + name << ["two directives", "three directives"] + sdl << [ + ''' + directive @foo(x: Int @bar(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + directive @bar(y: Int @foo(x: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + + type Query { + field: String @foo(x: 10) @bar(y: 20) + } + ''', + ''' + directive @dirA(x: Int @dirB(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + directive @dirB(y: Int @dirC(z: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + directive @dirC(z: Int @dirA(x: 3)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + + type Query { + field: String @dirA(x: 10) @dirB(y: 20) @dirC(z: 30) + } + ''' + ] + cycleMessage << [ + "'foo' must not reference itself via directive cycle 'foo -> bar -> foo'", + "'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'" + ] + } + def "code registry default data fetcher is respected"() { def sdl = ''' type Query { diff --git a/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy index 887fe97f5c..4debd015b9 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy @@ -232,6 +232,51 @@ class SchemaTypeDirectivesCheckerTest extends Specification { errors.get(0).getMessage() == "'invalidExample' must not reference itself on 'arg''[@2:39]'" } + def "directive must not indirectly reference itself"() { + given: + def spec = ''' + directive @foo(arg: String @bar) on ARGUMENT_DEFINITION + directive @bar(arg: String @foo) on ARGUMENT_DEFINITION + + type Query { + f1 : String + } + ''' + def registry = parse(spec) + def errors = [] + + when: + new SchemaTypeDirectivesChecker(registry, RuntimeWiring.newRuntimeWiring().build()).checkTypeDirectives(errors) + + then: + errors.size() == 1 + errors.get(0) instanceof DirectiveIllegalReferenceError + errors.get(0).getMessage().contains("'foo' must not reference itself via directive cycle 'foo -> bar -> foo'") + } + + def "directive must not indirectly reference itself through a longer cycle"() { + given: + def spec = ''' + directive @dirA(x: Int @dirB(y: 1)) on ARGUMENT_DEFINITION + directive @dirB(y: Int @dirC(z: 2)) on ARGUMENT_DEFINITION + directive @dirC(z: Int @dirA(x: 3)) on ARGUMENT_DEFINITION + + type Query { + f1 : String + } + ''' + def registry = parse(spec) + def errors = [] + + when: + new SchemaTypeDirectivesChecker(registry, RuntimeWiring.newRuntimeWiring().build()).checkTypeDirectives(errors) + + then: + errors.size() == 1 + errors.get(0) instanceof DirectiveIllegalReferenceError + errors.get(0).getMessage().contains("'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'") + } + def "directive must not begin with '__'"() { given: def spec = '''