Skip to content

Commit c58ab2a

Browse files
andimarekandimarek
committed
Reject indirect directive definition cycles
1 parent bd529fb commit c58ab2a

4 files changed

Lines changed: 212 additions & 2 deletions

File tree

‎src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java‎

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,17 @@
3030
import graphql.schema.idl.errors.MissingTypeError;
3131
import graphql.schema.idl.errors.NotAnInputTypeError;
3232

33+
import java.util.ArrayList;
3334
import java.util.Collection;
35+
import java.util.Collections;
36+
import java.util.LinkedHashMap;
37+
import java.util.LinkedHashSet;
3438
import java.util.List;
3539
import java.util.Map;
3640
import java.util.Optional;
41+
import java.util.Set;
3742

43+
import static graphql.Assert.assertNotNull;
3844
import static graphql.introspection.Introspection.DirectiveLocation.ARGUMENT_DEFINITION;
3945
import static graphql.introspection.Introspection.DirectiveLocation.ENUM;
4046
import static graphql.introspection.Introspection.DirectiveLocation.ENUM_VALUE;
@@ -182,6 +188,10 @@ private static boolean isNoNullArgWithoutDefaultValue(InputValueDefinition defin
182188
}
183189

184190
private void commonCheck(Collection<DirectiveDefinition> directiveDefinitions, List<GraphQLError> errors) {
191+
List<DirectiveDefinition> directiveDefinitionsList = new ArrayList<>(directiveDefinitions);
192+
Map<String, DirectiveDefinition> directiveDefinitionsByName = getByName(directiveDefinitionsList, DirectiveDefinition::getName, mergeFirst());
193+
Map<String, Map<String, InputValueDefinition>> directiveReferencesByName = directiveReferencesByName(directiveDefinitionsByName);
194+
185195
directiveDefinitions.forEach(directiveDefinition -> {
186196
assertTypeName(directiveDefinition, errors);
187197
directiveDefinition.getInputValueDefinitions().forEach(inputValueDefinition -> {
@@ -192,6 +202,113 @@ private void commonCheck(Collection<DirectiveDefinition> directiveDefinitions, L
192202
}
193203
});
194204
});
205+
checkIndirectDirectiveCycles(directiveDefinitionsByName, directiveReferencesByName, errors);
206+
}
207+
208+
private static Map<String, Map<String, InputValueDefinition>> directiveReferencesByName(
209+
Map<String, DirectiveDefinition> directiveDefinitionsByName) {
210+
Map<String, Map<String, InputValueDefinition>> result = new LinkedHashMap<>();
211+
directiveDefinitionsByName.forEach((name, directiveDefinition) -> result.put(name, directiveReferences(directiveDefinition)));
212+
return result;
213+
}
214+
215+
private static Map<String, InputValueDefinition> directiveReferences(DirectiveDefinition directiveDefinition) {
216+
Map<String, InputValueDefinition> result = new LinkedHashMap<>();
217+
for (InputValueDefinition inputValueDefinition : directiveDefinition.getInputValueDefinitions()) {
218+
recordDirectiveReferences(directiveDefinition, result, inputValueDefinition);
219+
}
220+
return result;
221+
}
222+
223+
private static void recordDirectiveReferences(DirectiveDefinition directiveDefinition,
224+
Map<String, InputValueDefinition> result,
225+
InputValueDefinition inputValueDefinition) {
226+
for (Directive directive : inputValueDefinition.getDirectives()) {
227+
if (directive.getName().equals(directiveDefinition.getName())) {
228+
continue;
229+
}
230+
result.putIfAbsent(directive.getName(), inputValueDefinition);
231+
}
232+
}
233+
234+
private static void checkIndirectDirectiveCycles(
235+
Map<String, DirectiveDefinition> directiveDefinitionsByName,
236+
Map<String, Map<String, InputValueDefinition>> directiveReferencesByName,
237+
List<GraphQLError> errors) {
238+
Set<String> checked = new LinkedHashSet<>();
239+
Set<String> visiting = new LinkedHashSet<>();
240+
List<String> path = new ArrayList<>();
241+
for (String directiveName : directiveDefinitionsByName.keySet()) {
242+
checkIndirectDirectiveCycles(directiveName, directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors);
243+
}
244+
}
245+
246+
private static void checkIndirectDirectiveCycles(String directiveName,
247+
Map<String, DirectiveDefinition> directiveDefinitionsByName,
248+
Map<String, Map<String, InputValueDefinition>> directiveReferencesByName,
249+
Set<String> checked,
250+
Set<String> visiting,
251+
List<String> path,
252+
List<GraphQLError> errors) {
253+
if (checked.contains(directiveName)) {
254+
return;
255+
}
256+
257+
visiting.add(directiveName);
258+
path.add(directiveName);
259+
checkIndirectDirectiveCycleReferences(directiveName, directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors);
260+
path.remove(path.size() - 1);
261+
visiting.remove(directiveName);
262+
checked.add(directiveName);
263+
}
264+
265+
private static void checkIndirectDirectiveCycleReferences(String directiveName,
266+
Map<String, DirectiveDefinition> directiveDefinitionsByName,
267+
Map<String, Map<String, InputValueDefinition>> directiveReferencesByName,
268+
Set<String> checked,
269+
Set<String> visiting,
270+
List<String> path,
271+
List<GraphQLError> errors) {
272+
Map<String, InputValueDefinition> references = directiveReferencesByName.getOrDefault(directiveName, Collections.emptyMap());
273+
for (Map.Entry<String, InputValueDefinition> entry : references.entrySet()) {
274+
checkIndirectDirectiveCycleReference(entry.getKey(), entry.getValue(), directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors);
275+
}
276+
}
277+
278+
private static void checkIndirectDirectiveCycleReference(String referencedDirectiveName,
279+
InputValueDefinition inputValueDefinition,
280+
Map<String, DirectiveDefinition> directiveDefinitionsByName,
281+
Map<String, Map<String, InputValueDefinition>> directiveReferencesByName,
282+
Set<String> checked,
283+
Set<String> visiting,
284+
List<String> path,
285+
List<GraphQLError> errors) {
286+
if (visiting.contains(referencedDirectiveName)) {
287+
addIndirectDirectiveCycleError(referencedDirectiveName, inputValueDefinition, directiveDefinitionsByName, path, errors);
288+
return;
289+
}
290+
if (!checked.contains(referencedDirectiveName)) {
291+
checkIndirectDirectiveCycles(referencedDirectiveName, directiveDefinitionsByName, directiveReferencesByName, checked, visiting, path, errors);
292+
}
293+
}
294+
295+
private static void addIndirectDirectiveCycleError(String repeatedDirectiveName,
296+
InputValueDefinition inputValueDefinition,
297+
Map<String, DirectiveDefinition> directiveDefinitionsByName,
298+
List<String> path,
299+
List<GraphQLError> errors) {
300+
List<String> cyclePath = directiveCyclePath(repeatedDirectiveName, path);
301+
String cyclePathString = String.join(" -> ", cyclePath);
302+
303+
DirectiveDefinition directiveDefinition = assertNotNull(directiveDefinitionsByName.get(repeatedDirectiveName));
304+
errors.add(new DirectiveIllegalReferenceError(directiveDefinition, inputValueDefinition, cyclePathString));
305+
}
306+
307+
private static List<String> directiveCyclePath(String repeatedDirectiveName, List<String> path) {
308+
int cycleStart = path.indexOf(repeatedDirectiveName);
309+
List<String> cyclePath = new ArrayList<>(path.subList(cycleStart, path.size()));
310+
cyclePath.add(repeatedDirectiveName);
311+
return cyclePath;
195312
}
196313

197314
private static void assertTypeName(NamedNode<?> node, List<GraphQLError> errors) {
@@ -224,4 +341,4 @@ private static TypeDefinition<?> findTypeDefFromRegistry(String typeName, TypeDe
224341
}
225342
return typeRegistry.scalars().get(typeName);
226343
}
227-
}
344+
}

‎src/main/java/graphql/schema/idl/errors/DirectiveIllegalReferenceError.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,11 @@ public DirectiveIllegalReferenceError(DirectiveDefinition directive, NamedNode l
1212
directive.getName(), location.getName(), lineCol(location)
1313
));
1414
}
15-
}
15+
16+
public DirectiveIllegalReferenceError(DirectiveDefinition directive, NamedNode location, String cyclePath) {
17+
super(directive,
18+
String.format("'%s' must not reference itself via directive cycle '%s' on '%s''%s'",
19+
directive.getName(), cyclePath, location.getName(), lineCol(location)
20+
));
21+
}
22+
}

‎src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import graphql.schema.GraphQLType
2525
import graphql.schema.GraphQLTypeUtil
2626
import graphql.schema.GraphQLUnionType
2727
import graphql.schema.GraphqlTypeComparatorRegistry
28+
import graphql.schema.idl.errors.DirectiveIllegalReferenceError
2829
import graphql.schema.idl.errors.NotAnInputTypeError
2930
import graphql.schema.idl.errors.NotAnOutputTypeError
3031
import graphql.schema.idl.errors.SchemaProblem
@@ -2270,6 +2271,46 @@ class SchemaGeneratorTest extends Specification {
22702271
schema != null
22712272
}
22722273
2274+
def "#4201 indirect cyclical directive definitions are rejected without stack overflow - #name"() {
2275+
given:
2276+
def registry = new SchemaParser().parse(sdl)
2277+
2278+
when:
2279+
UnExecutableSchemaGenerator.makeUnExecutableSchema(registry)
2280+
2281+
then:
2282+
def e = thrown(SchemaProblem)
2283+
e.errors.size() == 1
2284+
e.errors.get(0) instanceof DirectiveIllegalReferenceError
2285+
e.errors.get(0).getMessage().contains(cycleMessage)
2286+
2287+
where:
2288+
name << ["two directives", "three directives"]
2289+
sdl << [
2290+
'''
2291+
directive @foo(x: Int @bar(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION
2292+
directive @bar(y: Int @foo(x: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION
2293+
2294+
type Query {
2295+
field: String @foo(x: 10) @bar(y: 20)
2296+
}
2297+
''',
2298+
'''
2299+
directive @dirA(x: Int @dirB(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION
2300+
directive @dirB(y: Int @dirC(z: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION
2301+
directive @dirC(z: Int @dirA(x: 3)) on FIELD_DEFINITION | ARGUMENT_DEFINITION
2302+
2303+
type Query {
2304+
field: String @dirA(x: 10) @dirB(y: 20) @dirC(z: 30)
2305+
}
2306+
'''
2307+
]
2308+
cycleMessage << [
2309+
"'foo' must not reference itself via directive cycle 'foo -> bar -> foo'",
2310+
"'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'"
2311+
]
2312+
}
2313+
22732314
def "code registry default data fetcher is respected"() {
22742315
def sdl = '''
22752316
type Query {

‎src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,51 @@ class SchemaTypeDirectivesCheckerTest extends Specification {
232232
errors.get(0).getMessage() == "'invalidExample' must not reference itself on 'arg''[@2:39]'"
233233
}
234234

235+
def "directive must not indirectly reference itself"() {
236+
given:
237+
def spec = '''
238+
directive @foo(arg: String @bar) on ARGUMENT_DEFINITION
239+
directive @bar(arg: String @foo) on ARGUMENT_DEFINITION
240+
241+
type Query {
242+
f1 : String
243+
}
244+
'''
245+
def registry = parse(spec)
246+
def errors = []
247+
248+
when:
249+
new SchemaTypeDirectivesChecker(registry, RuntimeWiring.newRuntimeWiring().build()).checkTypeDirectives(errors)
250+
251+
then:
252+
errors.size() == 1
253+
errors.get(0) instanceof DirectiveIllegalReferenceError
254+
errors.get(0).getMessage().contains("'foo' must not reference itself via directive cycle 'foo -> bar -> foo'")
255+
}
256+
257+
def "directive must not indirectly reference itself through a longer cycle"() {
258+
given:
259+
def spec = '''
260+
directive @dirA(x: Int @dirB(y: 1)) on ARGUMENT_DEFINITION
261+
directive @dirB(y: Int @dirC(z: 2)) on ARGUMENT_DEFINITION
262+
directive @dirC(z: Int @dirA(x: 3)) on ARGUMENT_DEFINITION
263+
264+
type Query {
265+
f1 : String
266+
}
267+
'''
268+
def registry = parse(spec)
269+
def errors = []
270+
271+
when:
272+
new SchemaTypeDirectivesChecker(registry, RuntimeWiring.newRuntimeWiring().build()).checkTypeDirectives(errors)
273+
274+
then:
275+
errors.size() == 1
276+
errors.get(0) instanceof DirectiveIllegalReferenceError
277+
errors.get(0).getMessage().contains("'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'")
278+
}
279+
235280
def "directive must not begin with '__'"() {
236281
given:
237282
def spec = '''

0 commit comments

Comments
 (0)