From 36f863316f065fb9bab4760b5809cae7bbd01c6c Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Thu, 31 Aug 2023 16:19:24 +1000 Subject: [PATCH 1/9] Enable builds on 21.x --- .github/workflows/pull_request.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 006a2c915d..8b8f17b005 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -7,6 +7,7 @@ on: pull_request: branches: - master + - 21.x - 20.x - 19.x - 18.x From 92e67fbe4db3fa952ce5d98ef6544c5f1881c53e Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 22 Aug 2023 08:59:54 +0000 Subject: [PATCH 2/9] Merge pull request #3278 from graphql-java/3276_variable_validation_bug Bug 3276 - default values not being used correctly during validation (cherry picked from commit ba589a81bae36da045996e764bfec0b8637c65ac) --- .../graphql/validation/TraversalContext.java | 52 +++++-- .../graphql/validation/ValidationContext.java | 11 +- .../validation/rules/VariableTypesMatch.java | 27 ++-- .../rules/VariablesTypesMatcher.java | 28 +++- src/main/resources/i18n/Validation.properties | 2 +- .../resources/i18n/Validation_de.properties | 2 +- .../execution/ValuesResolverE2ETest.groovy | 136 ++++++++++++++++++ .../rules/VariableTypesMatchTest.groovy | 104 ++++++++++++-- .../rules/VariablesTypesMatcherTest.groovy | 17 ++- 9 files changed, 337 insertions(+), 42 deletions(-) create mode 100644 src/test/groovy/graphql/execution/ValuesResolverE2ETest.groovy diff --git a/src/main/java/graphql/validation/TraversalContext.java b/src/main/java/graphql/validation/TraversalContext.java index dfd3e4920d..93bbdb62e6 100644 --- a/src/main/java/graphql/validation/TraversalContext.java +++ b/src/main/java/graphql/validation/TraversalContext.java @@ -32,6 +32,7 @@ import graphql.schema.GraphQLType; import graphql.schema.GraphQLUnionType; import graphql.schema.GraphQLUnmodifiedType; +import graphql.schema.InputValueWithState; import java.util.ArrayList; import java.util.List; @@ -47,6 +48,7 @@ public class TraversalContext implements DocumentVisitor { private final List outputTypeStack = new ArrayList<>(); private final List parentTypeStack = new ArrayList<>(); private final List inputTypeStack = new ArrayList<>(); + private final List defaultValueStack = new ArrayList<>(); private final List fieldDefStack = new ArrayList<>(); private final List nameStack = new ArrayList<>(); private GraphQLDirective directive; @@ -155,6 +157,7 @@ private void enterImpl(Argument argument) { } addInputType(argumentType != null ? argumentType.getType() : null); + addDefaultValue(argumentType != null ? argumentType.getArgumentDefaultValue() : null); this.argument = argumentType; } @@ -165,23 +168,30 @@ private void enterImpl(ArrayValue arrayValue) { inputType = (GraphQLInputType) unwrapOne(nullableType); } addInputType(inputType); + // List positions never have a default value. See graphql-js impl for inspiration + addDefaultValue(null); } private void enterImpl(ObjectField objectField) { GraphQLUnmodifiedType objectType = unwrapAll(getInputType()); GraphQLInputType inputType = null; + GraphQLInputObjectField inputField = null; if (objectType instanceof GraphQLInputObjectType) { GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) objectType; - GraphQLInputObjectField inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName()); - if (inputField != null) + inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName()); + if (inputField != null) { inputType = inputField.getType(); + } } addInputType(inputType); + addDefaultValue(inputField != null ? inputField.getInputFieldDefaultValue() : null); } private GraphQLArgument find(List arguments, String name) { for (GraphQLArgument argument : arguments) { - if (argument.getName().equals(name)) return argument; + if (argument.getName().equals(name)) { + return argument; + } } return null; } @@ -190,29 +200,32 @@ private GraphQLArgument find(List arguments, String name) { @Override public void leave(Node node, List ancestors) { if (node instanceof OperationDefinition) { - outputTypeStack.remove(outputTypeStack.size() - 1); + pop(outputTypeStack); } else if (node instanceof SelectionSet) { - parentTypeStack.remove(parentTypeStack.size() - 1); + pop(parentTypeStack); } else if (node instanceof Field) { leaveName(((Field) node).getName()); - fieldDefStack.remove(fieldDefStack.size() - 1); - outputTypeStack.remove(outputTypeStack.size() - 1); + pop(fieldDefStack); + pop(outputTypeStack); } else if (node instanceof Directive) { directive = null; } else if (node instanceof InlineFragment) { - outputTypeStack.remove(outputTypeStack.size() - 1); + pop(outputTypeStack); } else if (node instanceof FragmentDefinition) { leaveName(((FragmentDefinition) node).getName()); - outputTypeStack.remove(outputTypeStack.size() - 1); + pop(outputTypeStack); } else if (node instanceof VariableDefinition) { inputTypeStack.remove(inputTypeStack.size() - 1); } else if (node instanceof Argument) { argument = null; - inputTypeStack.remove(inputTypeStack.size() - 1); + pop(inputTypeStack); + pop(defaultValueStack); } else if (node instanceof ArrayValue) { - inputTypeStack.remove(inputTypeStack.size() - 1); + pop(inputTypeStack); + pop(defaultValueStack); } else if (node instanceof ObjectField) { - inputTypeStack.remove(inputTypeStack.size() - 1); + pop(inputTypeStack); + pop(defaultValueStack); } } @@ -249,10 +262,16 @@ private void addOutputType(GraphQLOutputType type) { } private T lastElement(List list) { - if (list.size() == 0) return null; + if (list.isEmpty()) { + return null; + } return list.get(list.size() - 1); } + private T pop(List list) { + return list.remove(list.size() - 1); + } + /** * @return can be null if the parent is not a CompositeType */ @@ -267,11 +286,18 @@ private void addParentType(GraphQLCompositeType compositeType) { public GraphQLInputType getInputType() { return lastElement(inputTypeStack); } + public InputValueWithState getDefaultValue() { + return lastElement(defaultValueStack); + } private void addInputType(GraphQLInputType graphQLInputType) { inputTypeStack.add(graphQLInputType); } + private void addDefaultValue(InputValueWithState defaultValue) { + defaultValueStack.add(defaultValue); + } + public GraphQLFieldDefinition getFieldDef() { return lastElement(fieldDefStack); } diff --git a/src/main/java/graphql/validation/ValidationContext.java b/src/main/java/graphql/validation/ValidationContext.java index 2646afcdbb..18fe678177 100644 --- a/src/main/java/graphql/validation/ValidationContext.java +++ b/src/main/java/graphql/validation/ValidationContext.java @@ -14,6 +14,7 @@ import graphql.schema.GraphQLInputType; import graphql.schema.GraphQLOutputType; import graphql.schema.GraphQLSchema; +import graphql.schema.InputValueWithState; import java.util.LinkedHashMap; import java.util.List; @@ -41,8 +42,10 @@ public ValidationContext(GraphQLSchema schema, Document document, I18n i18n) { } private void buildFragmentMap() { - for (Definition definition : document.getDefinitions()) { - if (!(definition instanceof FragmentDefinition)) continue; + for (Definition definition : document.getDefinitions()) { + if (!(definition instanceof FragmentDefinition)) { + continue; + } FragmentDefinition fragmentDefinition = (FragmentDefinition) definition; fragmentDefinitionMap.put(fragmentDefinition.getName(), fragmentDefinition); } @@ -72,6 +75,10 @@ public GraphQLInputType getInputType() { return traversalContext.getInputType(); } + public InputValueWithState getDefaultValue() { + return traversalContext.getDefaultValue(); + } + public GraphQLFieldDefinition getFieldDef() { return traversalContext.getFieldDef(); } diff --git a/src/main/java/graphql/validation/rules/VariableTypesMatch.java b/src/main/java/graphql/validation/rules/VariableTypesMatch.java index de395850af..ef7c4069f4 100644 --- a/src/main/java/graphql/validation/rules/VariableTypesMatch.java +++ b/src/main/java/graphql/validation/rules/VariableTypesMatch.java @@ -59,24 +59,25 @@ public void checkVariable(VariableReference variableReference) { if (variableType == null) { return; } - GraphQLInputType expectedType = getValidationContext().getInputType(); - Optional schemaDefault = Optional.ofNullable(getValidationContext().getArgument()).map(v -> v.getArgumentDefaultValue()); - Value schemaDefaultValue = null; - if (schemaDefault.isPresent() && schemaDefault.get().isLiteral()) { - schemaDefaultValue = (Value) schemaDefault.get().getValue(); - } else if (schemaDefault.isPresent() && schemaDefault.get().isSet()) { - schemaDefaultValue = ValuesResolver.valueToLiteral(schemaDefault.get(), expectedType, getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale()); - } - if (expectedType == null) { - // we must have a unknown variable say to not have a known type + GraphQLInputType locationType = getValidationContext().getInputType(); + Optional locationDefault = Optional.ofNullable(getValidationContext().getDefaultValue()); + if (locationType == null) { + // we must have an unknown variable say to not have a known type return; } - if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType) && - !variablesTypesMatcher.doesVariableTypesMatch(variableType, schemaDefaultValue, expectedType)) { + Value locationDefaultValue = null; + if (locationDefault.isPresent() && locationDefault.get().isLiteral()) { + locationDefaultValue = (Value) locationDefault.get().getValue(); + } else if (locationDefault.isPresent() && locationDefault.get().isSet()) { + locationDefaultValue = ValuesResolver.valueToLiteral(locationDefault.get(), locationType, getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale()); + } + boolean variableDefMatches = variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), locationType, locationDefaultValue); + if (!variableDefMatches) { GraphQLType effectiveType = variablesTypesMatcher.effectiveType(variableType, variableDefinition.getDefaultValue()); String message = i18n(VariableTypeMismatch, "VariableTypesMatchRule.unexpectedType", + variableDefinition.getName(), GraphQLTypeUtil.simplePrint(effectiveType), - GraphQLTypeUtil.simplePrint(expectedType)); + GraphQLTypeUtil.simplePrint(locationType)); addError(VariableTypeMismatch, variableReference.getSourceLocation(), message); } } diff --git a/src/main/java/graphql/validation/rules/VariablesTypesMatcher.java b/src/main/java/graphql/validation/rules/VariablesTypesMatcher.java index 8208eba393..0b47fcec78 100644 --- a/src/main/java/graphql/validation/rules/VariablesTypesMatcher.java +++ b/src/main/java/graphql/validation/rules/VariablesTypesMatcher.java @@ -9,16 +9,38 @@ import static graphql.schema.GraphQLNonNull.nonNull; import static graphql.schema.GraphQLTypeUtil.isList; import static graphql.schema.GraphQLTypeUtil.isNonNull; +import static graphql.schema.GraphQLTypeUtil.unwrapNonNull; import static graphql.schema.GraphQLTypeUtil.unwrapOne; @Internal public class VariablesTypesMatcher { - public boolean doesVariableTypesMatch(GraphQLType variableType, Value variableDefaultValue, GraphQLType expectedType) { - return checkType(effectiveType(variableType, variableDefaultValue), expectedType); + /** + * This method and variable naming was inspired from the reference graphql-js implementation + * + * @param varType the variable type + * @param varDefaultValue the default value for the variable + * @param locationType the location type where the variable was encountered + * @param locationDefaultValue the default value for that location + * + * @return true if the variable matches ok + */ + public boolean doesVariableTypesMatch(GraphQLType varType, Value varDefaultValue, GraphQLType locationType, Value locationDefaultValue) { + if (isNonNull(locationType) && !isNonNull(varType)) { + boolean hasNonNullVariableDefaultValue = + varDefaultValue != null && !(varDefaultValue instanceof NullValue); + boolean hasLocationDefaultValue = locationDefaultValue != null; + if (!hasNonNullVariableDefaultValue && !hasLocationDefaultValue) { + return false; + } + GraphQLType nullableLocationType = unwrapNonNull(locationType); + return checkType(varType, nullableLocationType); + } + return checkType(varType, locationType); } - public GraphQLType effectiveType(GraphQLType variableType, Value defaultValue) { + + public GraphQLType effectiveType(GraphQLType variableType, Value defaultValue) { if (defaultValue == null || defaultValue instanceof NullValue) { return variableType; } diff --git a/src/main/resources/i18n/Validation.properties b/src/main/resources/i18n/Validation.properties index 9733f79738..4a6a551fdf 100644 --- a/src/main/resources/i18n/Validation.properties +++ b/src/main/resources/i18n/Validation.properties @@ -76,7 +76,7 @@ VariableDefaultValuesOfCorrectType.badDefault=Validation error ({0}) : Bad defau # VariablesAreInputTypes.wrongType=Validation error ({0}) : Input variable ''{1}'' type ''{2}'' is not an input type # -VariableTypesMatchRule.unexpectedType=Validation error ({0}) : Variable type ''{1}'' does not match expected type ''{2}'' +VariableTypesMatchRule.unexpectedType=Validation error ({0}) : Variable ''{1}'' of type ''{2}'' used in position expecting type ''{3}'' # UniqueObjectFieldName.duplicateFieldName=Validation Error ({0}) : There can be only one field named ''{1}'' # diff --git a/src/main/resources/i18n/Validation_de.properties b/src/main/resources/i18n/Validation_de.properties index def39f94ea..c15e3bb550 100644 --- a/src/main/resources/i18n/Validation_de.properties +++ b/src/main/resources/i18n/Validation_de.properties @@ -76,7 +76,7 @@ VariableDefaultValuesOfCorrectType.badDefault=Validierungsfehler ({0}) : Ung # VariablesAreInputTypes.wrongType=Validierungsfehler ({0}) : Eingabevariable ''{1}'' Typ ''{2}'' ist kein Eingabetyp # -VariableTypesMatchRule.unexpectedType=Validierungsfehler ({0}) : Der Variablentyp ''{1}'' stimmt nicht mit dem erwarteten Typ ''{2}'' überein +VariableTypesMatchRule.unexpectedType=Validierungsfehler ({0}) : Variable ''{1}'' vom Typ ''{2}'' verwendet in Position, die Typ ''{3}'' erwartet # # These are used but IDEA cant find them easily as being called # diff --git a/src/test/groovy/graphql/execution/ValuesResolverE2ETest.groovy b/src/test/groovy/graphql/execution/ValuesResolverE2ETest.groovy new file mode 100644 index 0000000000..590d84e277 --- /dev/null +++ b/src/test/groovy/graphql/execution/ValuesResolverE2ETest.groovy @@ -0,0 +1,136 @@ +package graphql.execution + +import graphql.ExecutionInput +import graphql.ExecutionResult +import graphql.GraphQL +import graphql.Scalars +import graphql.TestUtil +import graphql.schema.DataFetcher +import graphql.schema.DataFetchingEnvironment +import graphql.schema.FieldCoordinates +import graphql.schema.GraphQLCodeRegistry +import graphql.schema.GraphQLInputObjectType +import graphql.schema.GraphQLList +import graphql.schema.GraphQLObjectType +import graphql.schema.GraphQLSchema +import spock.lang.Specification + +class ValuesResolverE2ETest extends Specification { + + def "issue 3276 - reported bug on validation problems as SDL"() { + def sdl = ''' + type Query { + items(pagination: Pagination = {limit: 10, offset: 0}): [String] + } + + input Pagination { + limit: Int + offset: Int + } + ''' + DataFetcher df = { DataFetchingEnvironment env -> + def pagination = env.getArgument("pagination") as Map + def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value } + return strings + } + def schema = TestUtil.schema(sdl, [Query: [items: df]]) + def graphQL = GraphQL.newGraphQL(schema).build() + + when: + def ei = ExecutionInput.newExecutionInput(''' + query Items($limit: Int, $offset: Int) { + items(pagination: {limit: $limit, offset: $offset}) + } + ''').variables([limit: 5, offset: 0]).build() + def er = graphQL.execute(ei) + then: + er.errors.isEmpty() + er.data == [items : ["limit=5", "offset=0"]] + } + + def "issue 3276 - reported bug on validation problems as reported code"() { + DataFetcher dataFetcher = { env -> + def pagination = env.getArgument("pagination") as Map + def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value } + return strings + } + GraphQLSchema schema = GraphQLSchema.newSchema() + .query(GraphQLObjectType.newObject() + .name("Query") + .field(items -> items + .name("items") + .type(GraphQLList.list(Scalars.GraphQLString)) + .argument(pagination -> pagination + .name("pagination") + //skipped adding the default limit/offset values as it doesn't change anything + .defaultValueProgrammatic(new HashMap<>()) + .type(GraphQLInputObjectType.newInputObject() + .name("Pagination") + .field(limit -> limit + .name("limit") + .type(Scalars.GraphQLInt)) + .field(offset -> offset + .name("offset") + .type(Scalars.GraphQLInt)) + .build()))) + .build()) + .codeRegistry(GraphQLCodeRegistry.newCodeRegistry() + .dataFetcher(FieldCoordinates.coordinates("Query", "items"), dataFetcher) + .build()) + .build() + + GraphQL gql = GraphQL.newGraphQL(schema).build() + + Map vars = new HashMap<>() + vars.put("limit", 5) + vars.put("offset", 0) + + ExecutionInput ei = ExecutionInput.newExecutionInput() + .query("query Items( \$limit: Int, \$offset: Int) {\n" + + " items(\n" + + " pagination: {limit: \$limit, offset: \$offset} \n" + + " )\n" + + "}") + .variables(vars) + .build() + + when: + ExecutionResult result = gql.execute( ei) + then: + result.errors.isEmpty() + result.data == [items : ["limit=5", "offset=0"]] + } + + def "issue 3276 - should end up in validation errors because location defaults are not present"() { + def sdl = ''' + type Query { + items(pagination: Pagination = {limit: 1, offset: 1}): [String] + } + input Pagination { + limit: Int! #non-null this time, no default value + offset: Int! #non-null this time, no default value + } + ''' + DataFetcher df = { DataFetchingEnvironment env -> + def pagination = env.getArgument("pagination") as Map + def strings = pagination.entrySet().collect { entry -> entry.key + "=" + entry.value } + return strings + } + def schema = TestUtil.schema(sdl, [Query: [items: df]]) + def graphQL = GraphQL.newGraphQL(schema).build() + + when: + def ei = ExecutionInput.newExecutionInput(''' + query Items( $limit: Int, $offset: Int) { + items( + pagination: {limit: $limit, offset: $offset} + ) + } + ''').variables([limit: 5, offset: null]).build() + def er = graphQL.execute(ei) + then: + er.errors.size() == 2 + er.errors[0].message == "Validation error (VariableTypeMismatch@[items]) : Variable 'limit' of type 'Int' used in position expecting type 'Int!'" + er.errors[1].message == "Validation error (VariableTypeMismatch@[items]) : Variable 'offset' of type 'Int' used in position expecting type 'Int!'" + } +} diff --git a/src/test/groovy/graphql/validation/rules/VariableTypesMatchTest.groovy b/src/test/groovy/graphql/validation/rules/VariableTypesMatchTest.groovy index dab3510daa..5ccaff13dd 100644 --- a/src/test/groovy/graphql/validation/rules/VariableTypesMatchTest.groovy +++ b/src/test/groovy/graphql/validation/rules/VariableTypesMatchTest.groovy @@ -2,8 +2,10 @@ package graphql.validation.rules import graphql.StarWarsSchema +import graphql.TestUtil import graphql.i18n.I18n import graphql.parser.Parser +import graphql.schema.GraphQLSchema import graphql.validation.LanguageTraversal import graphql.validation.RulesVisitor import graphql.validation.ValidationContext @@ -14,10 +16,10 @@ import spock.lang.Specification class VariableTypesMatchTest extends Specification { ValidationErrorCollector errorCollector = new ValidationErrorCollector() - def traverse(String query) { + def traverse(String query, GraphQLSchema schema = StarWarsSchema.starWarsSchema) { def document = Parser.parse(query) I18n i18n = I18n.i18n(I18n.BundleType.Validation, Locale.ENGLISH) - def validationContext = new ValidationContext(StarWarsSchema.starWarsSchema, document, i18n) + def validationContext = new ValidationContext(schema, document, i18n) def variableTypesMatchRule = new VariableTypesMatch(validationContext, errorCollector) def languageTraversal = new LanguageTraversal() languageTraversal.traverse(document, new RulesVisitor(validationContext, [variableTypesMatchRule])) @@ -56,7 +58,7 @@ class VariableTypesMatchTest extends Specification { then: errorCollector.containsValidationError(ValidationErrorType.VariableTypeMismatch) // #991: describe which types were mismatched in error message - errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[human]) : Variable type 'String' does not match expected type 'String!'" + errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[human]) : Variable 'id' of type 'String' used in position expecting type 'String!'" } def "invalid variables in fragment spread"() { @@ -78,7 +80,7 @@ class VariableTypesMatchTest extends Specification { then: errorCollector.containsValidationError(ValidationErrorType.VariableTypeMismatch) - errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable type 'String' does not match expected type 'String!'" + errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable 'xid' of type 'String' used in position expecting type 'String!'" } def "mixed validity operations, valid first"() { @@ -104,7 +106,7 @@ class VariableTypesMatchTest extends Specification { then: errorCollector.containsValidationError(ValidationErrorType.VariableTypeMismatch) - errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable type 'String' does not match expected type 'String!'" + errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable 'id' of type 'String' used in position expecting type 'String!'" } def "mixed validity operations, invalid first"() { @@ -130,7 +132,7 @@ class VariableTypesMatchTest extends Specification { then: errorCollector.containsValidationError(ValidationErrorType.VariableTypeMismatch) - errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable type 'String' does not match expected type 'String!'" + errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable 'id' of type 'String' used in position expecting type 'String!'" } def "multiple invalid operations"() { @@ -158,11 +160,97 @@ class VariableTypesMatchTest extends Specification { errorCollector.getErrors().size() == 2 errorCollector.errors.any { it.validationErrorType == ValidationErrorType.VariableTypeMismatch && - it.message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable type 'String' does not match expected type 'String!'" + it.message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable 'id' of type 'String' used in position expecting type 'String!'" } errorCollector.errors.any { it.validationErrorType == ValidationErrorType.VariableTypeMismatch && - it.message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable type 'Boolean' does not match expected type 'String!'" + it.message == "Validation error (VariableTypeMismatch@[QueryType/human]) : Variable 'id' of type 'Boolean' used in position expecting type 'String!'" } } + + + def "issue 3276 - invalid variables in object field values with no defaults in location"() { + + def sdl = ''' + type Query { + items(pagination: Pagination = {limit: 1, offset: 1}): [String] + } + input Pagination { + limit: Int! + offset: Int! + } + ''' + def schema = TestUtil.schema(sdl) + given: + def query = ''' + query Items( $limit: Int, $offset: Int) { + items( + pagination: {limit: $limit, offset: $offset} + ) + } + ''' + + when: + traverse(query, schema) + + then: + errorCollector.containsValidationError(ValidationErrorType.VariableTypeMismatch) + errorCollector.errors[0].message == "Validation error (VariableTypeMismatch@[items]) : Variable 'limit' of type 'Int' used in position expecting type 'Int!'" + } + + def "issue 3276 - valid variables because of schema defaults with nullable variable"() { + + def sdl = ''' + type Query { + items(pagination: Pagination! = {limit: 1, offset: 1}): [String] + } + input Pagination { + limit: Int! + offset: Int! + } + ''' + def schema = TestUtil.schema(sdl) + given: + def query = ''' + query Items( $var : Pagination) { + items( + pagination: $var + ) + } + ''' + + when: + traverse(query, schema) + + then: + errorCollector.errors.isEmpty() + } + + def "issue 3276 - valid variables because of variable defaults"() { + + def sdl = ''' + type Query { + items(pagination: Pagination!): [String] + } + input Pagination { + limit: Int! + offset: Int! + } + ''' + def schema = TestUtil.schema(sdl) + given: + def query = ''' + query Items( $var : Pagination = {limit: 1, offset: 1}) { + items( + pagination: $var + ) + } + ''' + + when: + traverse(query, schema) + + then: + errorCollector.errors.isEmpty() + } } diff --git a/src/test/groovy/graphql/validation/rules/VariablesTypesMatcherTest.groovy b/src/test/groovy/graphql/validation/rules/VariablesTypesMatcherTest.groovy index c6bead4ef6..b05e425090 100644 --- a/src/test/groovy/graphql/validation/rules/VariablesTypesMatcherTest.groovy +++ b/src/test/groovy/graphql/validation/rules/VariablesTypesMatcherTest.groovy @@ -1,6 +1,7 @@ package graphql.validation.rules import graphql.language.BooleanValue +import graphql.language.StringValue import spock.lang.Specification import spock.lang.Unroll @@ -18,7 +19,7 @@ class VariablesTypesMatcherTest extends Specification { def "#variableType with default value #defaultValue and expected #expectedType should result: #result "() { expect: - typesMatcher.doesVariableTypesMatch(variableType, defaultValue, expectedType) == result + typesMatcher.doesVariableTypesMatch(variableType, defaultValue, expectedType, null) == result where: variableType | defaultValue | expectedType || result @@ -27,4 +28,18 @@ class VariablesTypesMatcherTest extends Specification { nonNull(GraphQLBoolean) | new BooleanValue(true) | GraphQLBoolean || true nonNull(GraphQLString) | null | list(GraphQLString) || false } + + @Unroll + def "issue 3276 - #variableType with default value #defaultValue and expected #expectedType with #locationDefaultValue should result: #result "() { + + expect: + typesMatcher.doesVariableTypesMatch(variableType, defaultValue, expectedType, locationDefaultValue) == result + + where: + variableType | defaultValue | expectedType | locationDefaultValue || result + GraphQLString | null | nonNull(GraphQLString) | null || false + GraphQLString | null | nonNull(GraphQLString) | StringValue.of("x") || true + GraphQLString | StringValue.of("x") | nonNull(GraphQLString) | StringValue.of("x") || true + GraphQLString | StringValue.of("x") | nonNull(GraphQLString) | null || true + } } From e11e254fc38d4a09d8f0f47725c6bb451bc0e3ee Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Mon, 21 Aug 2023 06:32:53 +0000 Subject: [PATCH 3/9] Merge pull request #3286 from graphql-java/3285_default_value_problems deprecated default value method produces bad results (cherry picked from commit a54bb43936a3b68fe44ee55032e407c8a703c263) --- .../execution/ValuesResolverConversion.java | 8 +- .../introspection/IntrospectionTest.groovy | 86 ++++++++++++------- .../schema/idl/SchemaPrinterTest.groovy | 18 ++++ 3 files changed, 79 insertions(+), 33 deletions(-) diff --git a/src/main/java/graphql/execution/ValuesResolverConversion.java b/src/main/java/graphql/execution/ValuesResolverConversion.java index 83f5285b0d..eff4d1cef1 100644 --- a/src/main/java/graphql/execution/ValuesResolverConversion.java +++ b/src/main/java/graphql/execution/ValuesResolverConversion.java @@ -69,11 +69,17 @@ static Object valueToLiteralImpl(GraphqlFieldVisibility fieldVisibility, if (valueMode == NORMALIZED) { return assertShouldNeverHappen("can't infer normalized structure"); } - return ValuesResolverLegacy.valueToLiteralLegacy( + Value value = ValuesResolverLegacy.valueToLiteralLegacy( inputValueWithState.getValue(), type, graphqlContext, locale); + // + // the valueToLiteralLegacy() nominally cant know if null means never set or is set to a null value + // but this code can know - its is SET to a value so, it MUST be a Null Literal + // this method would assert at the end of it if inputValueWithState.isNotSet() were true + // + return value == null ? NullValue.of() : value; } if (inputValueWithState.isLiteral()) { return inputValueWithState.getValue(); diff --git a/src/test/groovy/graphql/introspection/IntrospectionTest.groovy b/src/test/groovy/graphql/introspection/IntrospectionTest.groovy index 712ac8048c..bee6b161e8 100644 --- a/src/test/groovy/graphql/introspection/IntrospectionTest.groovy +++ b/src/test/groovy/graphql/introspection/IntrospectionTest.groovy @@ -1,5 +1,6 @@ package graphql.introspection + import graphql.TestUtil import graphql.schema.DataFetcher import graphql.schema.FieldCoordinates @@ -438,7 +439,7 @@ class IntrospectionTest extends Specification { def "test AST printed introspection query is equivalent to original string"() { when: - def oldIntrospectionQuery = "\n" + + def oldIntrospectionQuery = "\n" + " query IntrospectionQuery {\n" + " __schema {\n" + " queryType { name }\n" + @@ -540,12 +541,11 @@ class IntrospectionTest extends Specification { " }\n" + "\n" - def newIntrospectionQuery = IntrospectionQuery.INTROSPECTION_QUERY; + def newIntrospectionQuery = IntrospectionQuery.INTROSPECTION_QUERY then: - oldIntrospectionQuery.replaceAll("\\s+","").equals( - newIntrospectionQuery.replaceAll("\\s+","") - ) + oldIntrospectionQuery.replaceAll("\\s+", "") == + newIntrospectionQuery.replaceAll("\\s+", "") } def "test parameterized introspection queries"() { @@ -582,44 +582,66 @@ class IntrospectionTest extends Specification { def parseExecutionResult = { [ - it.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "notDeprecated"}["description"] != null, // descriptions is true - it.data["__schema"]["types"].find{it["name"] == "UUID"}["specifiedByURL"] != null, // specifiedByUrl is true - it.data["__schema"]["directives"].find{it["name"] == "repeatableDirective"}["isRepeatable"] != null, // directiveIsRepeatable is true - it.data["__schema"]["description"] != null, // schemaDescription is true - it.data["__schema"]["types"].find { it['name'] == 'InputType' }["inputFields"].find({ it["name"] == "inputField" }) != null // inputValueDeprecation is true + it.data["__schema"]["types"].find { it["name"] == "Query" }["fields"].find { it["name"] == "notDeprecated" }["description"] != null, // descriptions is true + it.data["__schema"]["types"].find { it["name"] == "UUID" }["specifiedByURL"] != null, // specifiedByUrl is true + it.data["__schema"]["directives"].find { it["name"] == "repeatableDirective" }["isRepeatable"] != null, // directiveIsRepeatable is true + it.data["__schema"]["description"] != null, // schemaDescription is true + it.data["__schema"]["types"].find { it['name'] == 'InputType' }["inputFields"].find({ it["name"] == "inputField" }) != null // inputValueDeprecation is true ] } when: - def allFalseExecutionResult = graphQL.execute( + def allFalseExecutionResult = graphQL.execute( IntrospectionQueryBuilder.build( - IntrospectionQueryBuilder.Options.defaultOptions() - .descriptions(false) - .specifiedByUrl(false) - .directiveIsRepeatable(false) - .schemaDescription(false) - .inputValueDeprecation(false) - .typeRefFragmentDepth(5) + IntrospectionQueryBuilder.Options.defaultOptions() + .descriptions(false) + .specifiedByUrl(false) + .directiveIsRepeatable(false) + .schemaDescription(false) + .inputValueDeprecation(false) + .typeRefFragmentDepth(5) ) - ) + ) then: - !parseExecutionResult(allFalseExecutionResult).any() - allFalseExecutionResult.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "tenDimensionalList"}["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 5 + !parseExecutionResult(allFalseExecutionResult).any() + allFalseExecutionResult.data["__schema"]["types"].find { it["name"] == "Query" }["fields"].find { it["name"] == "tenDimensionalList" }["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 5 when: - def allTrueExecutionResult = graphQL.execute( + def allTrueExecutionResult = graphQL.execute( IntrospectionQueryBuilder.build( - IntrospectionQueryBuilder.Options.defaultOptions() - .descriptions(true) - .specifiedByUrl(true) - .directiveIsRepeatable(true) - .schemaDescription(true) - .inputValueDeprecation(true) - .typeRefFragmentDepth(7) + IntrospectionQueryBuilder.Options.defaultOptions() + .descriptions(true) + .specifiedByUrl(true) + .directiveIsRepeatable(true) + .schemaDescription(true) + .inputValueDeprecation(true) + .typeRefFragmentDepth(7) ) - ) + ) + then: + parseExecutionResult(allTrueExecutionResult).every() + allTrueExecutionResult.data["__schema"]["types"].find { it["name"] == "Query" }["fields"].find { it["name"] == "tenDimensionalList" }["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 7 + } + + def "issue 3285 - deprecated defaultValue on programmatic args prints AST literal as expected"() { + def queryObjType = newObject().name("Query") + .field(newFieldDefinition().name("f").type(GraphQLString) + .argument(newArgument().name("arg").type(GraphQLString).defaultValue(null))) + .build() + def schema = newSchema().query(queryObjType).build() + def graphQL = newGraphQL(schema).build() + + + when: + def executionResult = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) then: - parseExecutionResult(allTrueExecutionResult).every() - allTrueExecutionResult.data["__schema"]["types"].find{it["name"] == "Query"}["fields"].find{it["name"] == "tenDimensionalList"}["type"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"]["ofType"] == null // typeRefFragmentDepth is 7 + executionResult.errors.isEmpty() + + def types = executionResult.data['__schema']['types'] as List + def queryType = types.find { it['name'] == 'Query' } + def fField = (queryType['fields'] as List)[0] + def arg = (fField['args'] as List)[0] + arg['name'] == "arg" + arg['defaultValue'] == "null" // printed AST } } diff --git a/src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy index e946a06278..cff7a5e6e0 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy @@ -2247,6 +2247,24 @@ type Query { type TestObjectB { field: String } +''' + } + + def "issue 3285 - deprecated defaultValue on programmatic args prints as expected"() { + def queryObjType = newObject().name("Query") + .field(newFieldDefinition().name("f").type(GraphQLString) + .argument(newArgument().name("arg").type(GraphQLString).defaultValue(null))) + .build() + def schema = GraphQLSchema.newSchema().query(queryObjType).build() + + + when: + def options = defaultOptions().includeDirectiveDefinitions(false) + def sdl = new SchemaPrinter(options).print(schema) + then: + sdl == '''type Query { + f(arg: String = null): String +} ''' } } \ No newline at end of file From bd0b6e661dcebb54a1056325bfb61b752f08e7f0 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 4 Sep 2023 13:43:04 +1000 Subject: [PATCH 4/9] guava version fix for 21.x (#3322) --- build.gradle | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/build.gradle b/build.gradle index d545b92572..4768218f76 100644 --- a/build.gradle +++ b/build.gradle @@ -50,6 +50,7 @@ def reactiveStreamsVersion = '1.0.3' def slf4jVersion = '2.0.7' def releaseVersion = System.env.RELEASE_VERSION def antlrVersion = '4.11.1' // https://mvnrepository.com/artifact/org.antlr/antlr4-runtime +def guavaVersion = '32.1.1-jre' version = releaseVersion ? releaseVersion : getDevelopmentVersion() group = 'com.graphql-java' @@ -89,7 +90,7 @@ dependencies { api 'com.graphql-java:java-dataloader:3.2.0' api 'org.reactivestreams:reactive-streams:' + reactiveStreamsVersion antlr 'org.antlr:antlr4:' + antlrVersion - implementation 'com.google.guava:guava:32.1.1-jre' + implementation 'com.google.guava:guava:' + guavaVersion testImplementation group: 'junit', name: 'junit', version: '4.13.2' testImplementation 'org.spockframework:spock-core:2.0-groovy-3.0' testImplementation 'org.codehaus.groovy:groovy:3.0.18' @@ -124,7 +125,7 @@ shadowJar { } relocate('org.antlr.v4.runtime', 'graphql.org.antlr.v4.runtime') dependencies { - include(dependency('com.google.guava:guava:32.1.1-jre')) + include(dependency('com.google.guava:guava:' + guavaVersion)) include(dependency('org.antlr:antlr4-runtime:' + antlrVersion)) } from "LICENSE.md" From fc5d4e521026d05d6735d2e1b03fa71f54922199 Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Mon, 18 Mar 2024 15:52:40 +1100 Subject: [PATCH 5/9] Add cherry pick of PR 3525 and minor adjustments --- .../java/graphql/execution/Execution.java | 1 + .../graphql/execution/ExecutionContext.java | 5 + .../graphql/execution/ExecutionStrategy.java | 29 +++- .../java/graphql/execution/FetchedValue.java | 2 +- .../graphql/execution/FieldValueInfo.java | 2 +- .../graphql/execution/ResultNodesInfo.java | 55 +++++++ src/test/groovy/graphql/GraphQLTest.groovy | 141 ++++++++++++++++++ 7 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 src/main/java/graphql/execution/ResultNodesInfo.java diff --git a/src/main/java/graphql/execution/Execution.java b/src/main/java/graphql/execution/Execution.java index 916bc64659..4014cd71f8 100644 --- a/src/main/java/graphql/execution/Execution.java +++ b/src/main/java/graphql/execution/Execution.java @@ -95,6 +95,7 @@ public CompletableFuture execute(Document document, GraphQLSche .executionInput(executionInput) .build(); + executionContext.getGraphQLContext().put(ResultNodesInfo.RESULT_NODES_INFO, executionContext.getResultNodesInfo()); InstrumentationExecutionParameters parameters = new InstrumentationExecutionParameters( executionInput, graphQLSchema, instrumentationState diff --git a/src/main/java/graphql/execution/ExecutionContext.java b/src/main/java/graphql/execution/ExecutionContext.java index a517f5eea9..1cc615fe46 100644 --- a/src/main/java/graphql/execution/ExecutionContext.java +++ b/src/main/java/graphql/execution/ExecutionContext.java @@ -55,6 +55,7 @@ public class ExecutionContext { private final ValueUnboxer valueUnboxer; private final ExecutionInput executionInput; private final Supplier queryTree; + private final ResultNodesInfo resultNodesInfo = new ResultNodesInfo(); ExecutionContext(ExecutionContextBuilder builder) { this.graphQLSchema = builder.graphQLSchema; @@ -282,4 +283,8 @@ public ExecutionContext transform(Consumer builderConsu builderConsumer.accept(builder); return builder.build(); } + + public ResultNodesInfo getResultNodesInfo() { + return resultNodesInfo; + } } diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 4ed0f1e644..01b57dea6d 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -62,6 +62,7 @@ import static graphql.execution.FieldValueInfo.CompleteValueType.NULL; import static graphql.execution.FieldValueInfo.CompleteValueType.OBJECT; import static graphql.execution.FieldValueInfo.CompleteValueType.SCALAR; +import static graphql.execution.ResultNodesInfo.MAX_RESULT_NODES; import static graphql.execution.instrumentation.SimpleInstrumentationContext.nonNullCtx; import static graphql.schema.DataFetchingEnvironmentImpl.newDataFetchingEnvironment; import static graphql.schema.GraphQLTypeUtil.isEnum; @@ -239,7 +240,23 @@ protected CompletableFuture fetchField(ExecutionContext executionC MergedField field = parameters.getField(); GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); GraphQLFieldDefinition fieldDef = getFieldDef(executionContext.getGraphQLSchema(), parentType, field.getSingleField()); - GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); + return fetchField(fieldDef, executionContext, parameters); + } + + private CompletableFuture fetchField(GraphQLFieldDefinition fieldDef, ExecutionContext executionContext, ExecutionStrategyParameters parameters) { + + int resultNodesCount = executionContext.getResultNodesInfo().incrementAndGetResultNodesCount(); + + Integer maxNodes; + if ((maxNodes = executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) != null) { + if (resultNodesCount > maxNodes) { + executionContext.getResultNodesInfo().maxResultNodesExceeded(); + return CompletableFuture.completedFuture(new FetchedValue(null, Collections.emptyList(), null, null)); + } + } + + MergedField field = parameters.getField(); + GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType(); // if the DF (like PropertyDataFetcher) does not use the arguments or execution step info then dont build any @@ -274,6 +291,7 @@ protected CompletableFuture fetchField(ExecutionContext executionC .queryDirectives(queryDirectives) .build(); }); + GraphQLCodeRegistry codeRegistry = executionContext.getGraphQLSchema().getCodeRegistry(); DataFetcher dataFetcher = codeRegistry.getDataFetcher(parentType, fieldDef); Instrumentation instrumentation = executionContext.getInstrumentation(); @@ -568,6 +586,15 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext, List fieldValueInfos = new ArrayList<>(size.orElse(1)); int index = 0; for (Object item : iterableValues) { + int resultNodesCount = executionContext.getResultNodesInfo().incrementAndGetResultNodesCount(); + Integer maxNodes; + if ((maxNodes = executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) != null) { + if (resultNodesCount > maxNodes) { + executionContext.getResultNodesInfo().maxResultNodesExceeded(); + return new FieldValueInfo(NULL, completedFuture(null), fieldValueInfos); + } + } + ResultPath indexedPath = parameters.getPath().segment(index); ExecutionStepInfo stepInfoForListElement = executionStepInfoFactory.newExecutionStepInfoForListElement(executionStepInfo, indexedPath); diff --git a/src/main/java/graphql/execution/FetchedValue.java b/src/main/java/graphql/execution/FetchedValue.java index 28d2ce6da6..8cc520cb28 100644 --- a/src/main/java/graphql/execution/FetchedValue.java +++ b/src/main/java/graphql/execution/FetchedValue.java @@ -19,7 +19,7 @@ public class FetchedValue { private final Object localContext; private final ImmutableList errors; - private FetchedValue(Object fetchedValue, Object rawFetchedValue, ImmutableList errors, Object localContext) { + FetchedValue(Object fetchedValue, Object rawFetchedValue, ImmutableList errors, Object localContext) { this.fetchedValue = fetchedValue; this.rawFetchedValue = rawFetchedValue; this.errors = errors; diff --git a/src/main/java/graphql/execution/FieldValueInfo.java b/src/main/java/graphql/execution/FieldValueInfo.java index 168ffab735..c889e98b34 100644 --- a/src/main/java/graphql/execution/FieldValueInfo.java +++ b/src/main/java/graphql/execution/FieldValueInfo.java @@ -25,7 +25,7 @@ public enum CompleteValueType { private final CompletableFuture fieldValue; private final List fieldValueInfos; - private FieldValueInfo(CompleteValueType completeValueType, CompletableFuture fieldValue, List fieldValueInfos) { + FieldValueInfo(CompleteValueType completeValueType, CompletableFuture fieldValue, List fieldValueInfos) { assertNotNull(fieldValueInfos, () -> "fieldValueInfos can't be null"); this.completeValueType = completeValueType; this.fieldValue = fieldValue; diff --git a/src/main/java/graphql/execution/ResultNodesInfo.java b/src/main/java/graphql/execution/ResultNodesInfo.java new file mode 100644 index 0000000000..afc366f6be --- /dev/null +++ b/src/main/java/graphql/execution/ResultNodesInfo.java @@ -0,0 +1,55 @@ +package graphql.execution; + +import graphql.Internal; +import graphql.PublicApi; + +import java.util.concurrent.atomic.AtomicInteger; + +/** + * This class is used to track the number of result nodes that have been created during execution. + * After each execution the GraphQLContext contains a ResultNodeInfo object under the key {@link ResultNodesInfo#RESULT_NODES_INFO} + *

+ * The number of result can be limited (and should be for security reasons) by setting the maximum number of result nodes + * in the GraphQLContext under the key {@link ResultNodesInfo#MAX_RESULT_NODES} to an Integer + *

+ */ +@PublicApi +public class ResultNodesInfo { + + public static final String MAX_RESULT_NODES = "__MAX_RESULT_NODES"; + public static final String RESULT_NODES_INFO = "__RESULT_NODES_INFO"; + + private volatile boolean maxResultNodesExceeded = false; + private final AtomicInteger resultNodesCount = new AtomicInteger(0); + + @Internal + public int incrementAndGetResultNodesCount() { + return resultNodesCount.incrementAndGet(); + } + + @Internal + public void maxResultNodesExceeded() { + this.maxResultNodesExceeded = true; + } + + /** + * The number of result nodes created. + * Note: this can be higher than max result nodes because + * a each node that exceeds the number of max nodes is set to null, + * but still is a result node (with value null) + * + * @return number of result nodes created + */ + public int getResultNodesCount() { + return resultNodesCount.get(); + } + + /** + * If the number of result nodes has exceeded the maximum allowed numbers. + * + * @return true if the number of result nodes has exceeded the maximum allowed numbers + */ + public boolean isMaxResultNodesExceeded() { + return maxResultNodesExceeded; + } +} diff --git a/src/test/groovy/graphql/GraphQLTest.groovy b/src/test/groovy/graphql/GraphQLTest.groovy index ec2523d3f8..d994ad9a0f 100644 --- a/src/test/groovy/graphql/GraphQLTest.groovy +++ b/src/test/groovy/graphql/GraphQLTest.groovy @@ -13,6 +13,7 @@ import graphql.execution.ExecutionId import graphql.execution.ExecutionIdProvider import graphql.execution.ExecutionStrategyParameters import graphql.execution.MissingRootTypeException +import graphql.execution.ResultNodesInfo import graphql.execution.SubscriptionExecutionStrategy import graphql.execution.ValueUnboxer import graphql.execution.instrumentation.ChainedInstrumentation @@ -49,6 +50,7 @@ import static graphql.ExecutionInput.Builder import static graphql.ExecutionInput.newExecutionInput import static graphql.Scalars.GraphQLInt import static graphql.Scalars.GraphQLString +import static graphql.execution.ResultNodesInfo.MAX_RESULT_NODES import static graphql.schema.GraphQLArgument.newArgument import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition import static graphql.schema.GraphQLInputObjectField.newInputObjectField @@ -1440,4 +1442,143 @@ many lines'''] then: !er.errors.isEmpty() } + + def "max result nodes not breached"() { + given: + def sdl = ''' + + type Query { + hello: String + } + ''' + def df = { env -> "world" } as DataFetcher + def fetchers = ["Query": ["hello": df]] + def schema = TestUtil.schema(sdl, fetchers) + def graphQL = GraphQL.newGraphQL(schema).build() + + def query = "{ hello h1: hello h2: hello h3: hello } " + def ei = newExecutionInput(query).build() + ei.getGraphQLContext().put(MAX_RESULT_NODES, 4); + + when: + def er = graphQL.execute(ei) + def rni = ei.getGraphQLContext().get(ResultNodesInfo.RESULT_NODES_INFO) as ResultNodesInfo + then: + !rni.maxResultNodesExceeded + rni.resultNodesCount == 4 + er.data == [hello: "world", h1: "world", h2: "world", h3: "world"] + } + + def "max result nodes breached"() { + given: + def sdl = ''' + + type Query { + hello: String + } + ''' + def df = { env -> "world" } as DataFetcher + def fetchers = ["Query": ["hello": df]] + def schema = TestUtil.schema(sdl, fetchers) + def graphQL = GraphQL.newGraphQL(schema).build() + + def query = "{ hello h1: hello h2: hello h3: hello } " + def ei = newExecutionInput(query).build() + ei.getGraphQLContext().put(MAX_RESULT_NODES, 3); + + when: + def er = graphQL.execute(ei) + def rni = ei.getGraphQLContext().get(ResultNodesInfo.RESULT_NODES_INFO) as ResultNodesInfo + then: + rni.maxResultNodesExceeded + rni.resultNodesCount == 4 + er.data == [hello: "world", h1: "world", h2: "world", h3: null] + } + + def "max result nodes breached with list"() { + given: + def sdl = ''' + + type Query { + hello: [String] + } + ''' + def df = { env -> ["w1", "w2", "w3"] } as DataFetcher + def fetchers = ["Query": ["hello": df]] + def schema = TestUtil.schema(sdl, fetchers) + def graphQL = GraphQL.newGraphQL(schema).build() + + def query = "{ hello}" + def ei = newExecutionInput(query).build() + ei.getGraphQLContext().put(MAX_RESULT_NODES, 3); + + when: + def er = graphQL.execute(ei) + def rni = ei.getGraphQLContext().get(ResultNodesInfo.RESULT_NODES_INFO) as ResultNodesInfo + then: + rni.maxResultNodesExceeded + rni.resultNodesCount == 4 + er.data == [hello: null] + } + + def "max result nodes breached with list 2"() { + given: + def sdl = ''' + + type Query { + hello: [Foo] + } + type Foo { + name: String + } + ''' + def df = { env -> [[name: "w1"], [name: "w2"], [name: "w3"]] } as DataFetcher + def fetchers = ["Query": ["hello": df]] + def schema = TestUtil.schema(sdl, fetchers) + def graphQL = GraphQL.newGraphQL(schema).build() + + def query = "{ hello {name}}" + def ei = newExecutionInput(query).build() + // we have 7 result nodes overall + ei.getGraphQLContext().put(MAX_RESULT_NODES, 6); + + when: + def er = graphQL.execute(ei) + def rni = ei.getGraphQLContext().get(ResultNodesInfo.RESULT_NODES_INFO) as ResultNodesInfo + then: + rni.resultNodesCount == 7 + rni.maxResultNodesExceeded + er.data == [hello: [[name: "w1"], [name: "w2"], [name: null]]] + } + + def "max result nodes not breached with list"() { + given: + def sdl = ''' + + type Query { + hello: [Foo] + } + type Foo { + name: String + } + ''' + def df = { env -> [[name: "w1"], [name: "w2"], [name: "w3"]] } as DataFetcher + def fetchers = ["Query": ["hello": df]] + def schema = TestUtil.schema(sdl, fetchers) + def graphQL = GraphQL.newGraphQL(schema).build() + + def query = "{ hello {name}}" + def ei = newExecutionInput(query).build() + // we have 7 result nodes overall + ei.getGraphQLContext().put(MAX_RESULT_NODES, 7); + + when: + def er = graphQL.execute(ei) + def rni = ei.getGraphQLContext().get(ResultNodesInfo.RESULT_NODES_INFO) as ResultNodesInfo + then: + !rni.maxResultNodesExceeded + rni.resultNodesCount == 7 + er.data == [hello: [[name: "w1"], [name: "w2"], [name: "w3"]]] + } + } From f1c4069b582b9363ffe3a666bc6a162734a2d833 Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Mon, 18 Mar 2024 16:39:49 +1100 Subject: [PATCH 6/9] Backport PR 3526 with minor adjustments --- .../execution/AsyncExecutionStrategy.java | 8 ++ .../AsyncSerialExecutionStrategy.java | 9 ++ .../graphql/introspection/Introspection.java | 82 ++++++++++++ .../IntrospectionDisabledError.java | 35 +++++ ...NoIntrospectionGraphqlFieldVisibility.java | 5 + .../introspection/IntrospectionTest.groovy | 124 +++++++++++++++++- 6 files changed, 260 insertions(+), 3 deletions(-) create mode 100644 src/main/java/graphql/introspection/IntrospectionDisabledError.java diff --git a/src/main/java/graphql/execution/AsyncExecutionStrategy.java b/src/main/java/graphql/execution/AsyncExecutionStrategy.java index 6608fba0f3..68b2072e2f 100644 --- a/src/main/java/graphql/execution/AsyncExecutionStrategy.java +++ b/src/main/java/graphql/execution/AsyncExecutionStrategy.java @@ -5,8 +5,10 @@ import graphql.execution.instrumentation.ExecutionStrategyInstrumentationContext; import graphql.execution.instrumentation.Instrumentation; import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters; +import graphql.introspection.Introspection; import java.util.List; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.function.BiConsumer; @@ -44,6 +46,12 @@ public CompletableFuture execute(ExecutionContext executionCont MergedSelectionSet fields = parameters.getFields(); List fieldNames = fields.getKeys(); + + Optional isNotSensible = Introspection.isIntrospectionSensible(executionContext.getGraphQLContext(),fields); + if (isNotSensible.isPresent()) { + return CompletableFuture.completedFuture(isNotSensible.get()); + } + Async.CombinedBuilder futures = Async.ofExpectedSize(fields.size()); for (String fieldName : fieldNames) { MergedField currentField = fields.getSubField(fieldName); diff --git a/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java b/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java index fc2dde0980..c2d0cf1db7 100644 --- a/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java +++ b/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java @@ -6,8 +6,10 @@ import graphql.execution.instrumentation.Instrumentation; import graphql.execution.instrumentation.InstrumentationContext; import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters; +import graphql.introspection.Introspection; import java.util.List; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import static graphql.execution.instrumentation.SimpleInstrumentationContext.nonNullCtx; @@ -39,6 +41,13 @@ public CompletableFuture execute(ExecutionContext executionCont MergedSelectionSet fields = parameters.getFields(); ImmutableList fieldNames = ImmutableList.copyOf(fields.keySet()); + // this is highly unlikely since Mutations cant do introspection BUT in theory someone could make the query strategy this code + // so belts and braces + Optional isNotSensible = Introspection.isIntrospectionSensible(executionContext.getGraphQLContext(), fields); + if (isNotSensible.isPresent()) { + return CompletableFuture.completedFuture(isNotSensible.get()); + } + CompletableFuture> resultsFuture = Async.eachSequentially(fieldNames, (fieldName, prevResults) -> { MergedField currentField = fields.getSubField(fieldName); ResultPath fieldPath = parameters.getPath().segment(mkNameForPath(currentField)); diff --git a/src/main/java/graphql/introspection/Introspection.java b/src/main/java/graphql/introspection/Introspection.java index 9b874f7f1b..fe0268d19a 100644 --- a/src/main/java/graphql/introspection/Introspection.java +++ b/src/main/java/graphql/introspection/Introspection.java @@ -3,9 +3,12 @@ import com.google.common.collect.ImmutableSet; import graphql.Assert; +import graphql.ExecutionResult; import graphql.GraphQLContext; import graphql.Internal; import graphql.PublicApi; +import graphql.execution.MergedField; +import graphql.execution.MergedSelectionSet; import graphql.execution.ValuesResolver; import graphql.language.AstPrinter; import graphql.schema.FieldCoordinates; @@ -32,6 +35,7 @@ import graphql.schema.GraphQLSchema; import graphql.schema.GraphQLUnionType; import graphql.schema.InputValueWithState; +import org.jetbrains.annotations.NotNull; import java.util.ArrayList; import java.util.HashSet; @@ -39,7 +43,9 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Optional; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Function; import java.util.stream.Collectors; @@ -58,8 +64,83 @@ import static graphql.schema.GraphQLTypeUtil.unwrapAllAs; import static graphql.schema.GraphQLTypeUtil.unwrapOne; +/** + * GraphQl has a unique capability called Introspection that allow + * consumers to inspect the system and discover the fields and types available and makes the system self documented. + *

+ * Some security recommendations such as OWASP + * recommend that introspection be disabled in production. The {@link Introspection#enabledJvmWide(boolean)} method can be used to disable + * introspection for the whole JVM or you can place {@link Introspection#INTROSPECTION_DISABLED} into the {@link GraphQLContext} of a request + * to disable introspection for that request. + */ @PublicApi public class Introspection { + + + /** + * Placing a boolean value under this key in the per request {@link GraphQLContext} will enable + * or disable Introspection on that request. + */ + public static final String INTROSPECTION_DISABLED = "INTROSPECTION_DISABLED"; + private static final AtomicBoolean INTROSPECTION_ENABLED_STATE = new AtomicBoolean(true); + + /** + * This static method will enable / disable Introspection at a JVM wide level. + * + * @param enabled the flag indicating the desired enabled state + * + * @return the previous state of enablement + */ + public static boolean enabledJvmWide(boolean enabled) { + return INTROSPECTION_ENABLED_STATE.getAndSet(enabled); + } + + /** + * @return true if Introspection is enabled at a JVM wide level or false otherwise + */ + public static boolean isEnabledJvmWide() { + return INTROSPECTION_ENABLED_STATE.get(); + } + + /** + * This will look in to the field selection set and see if there are introspection fields, + * and if there is,it checks if introspection should run, and if not it will return an errored {@link ExecutionResult} + * that can be returned to the user. + * + * @param mergedSelectionSet the fields to be executed + * + * @return an optional error result + */ + public static Optional isIntrospectionSensible(GraphQLContext graphQLContext, MergedSelectionSet mergedSelectionSet) { + MergedField schemaField = mergedSelectionSet.getSubField(SchemaMetaFieldDef.getName()); + if (schemaField != null) { + if (!isIntrospectionEnabled(graphQLContext)) { + return mkDisabledError(schemaField); + } + } + MergedField typeField = mergedSelectionSet.getSubField(TypeMetaFieldDef.getName()); + if (typeField != null) { + if (!isIntrospectionEnabled(graphQLContext)) { + return mkDisabledError(typeField); + } + } + // later we can put a good faith check code here to check the fields make sense + return Optional.empty(); + } + + @NotNull + private static Optional mkDisabledError(MergedField schemaField) { + IntrospectionDisabledError error = new IntrospectionDisabledError(schemaField.getSingleField().getSourceLocation()); + return Optional.of(ExecutionResult.newExecutionResult().addError(error).build()); + } + + private static boolean isIntrospectionEnabled(GraphQLContext graphQlContext) { + if (!isEnabledJvmWide()) { + return false; + } + return !graphQlContext.getOrDefault(INTROSPECTION_DISABLED, false); + } + private static final Map> introspectionDataFetchers = new LinkedHashMap<>(); private static void register(GraphQLFieldsContainer parentType, String fieldName, IntrospectionDataFetcher introspectionDataFetcher) { @@ -623,6 +704,7 @@ public enum DirectiveLocation { return environment.getGraphQLSchema().getType(name); }; + // __typename is always available public static final IntrospectionDataFetcher TypeNameMetaFieldDefDataFetcher = environment -> simplePrint(environment.getParentType()); @Internal diff --git a/src/main/java/graphql/introspection/IntrospectionDisabledError.java b/src/main/java/graphql/introspection/IntrospectionDisabledError.java new file mode 100644 index 0000000000..cbd7e077a3 --- /dev/null +++ b/src/main/java/graphql/introspection/IntrospectionDisabledError.java @@ -0,0 +1,35 @@ +package graphql.introspection; + +import graphql.ErrorClassification; +import graphql.ErrorType; +import graphql.GraphQLError; +import graphql.Internal; +import graphql.language.SourceLocation; + +import java.util.Collections; +import java.util.List; + +@Internal +public class IntrospectionDisabledError implements GraphQLError { + + private final List locations; + + public IntrospectionDisabledError(SourceLocation sourceLocation) { + locations = sourceLocation == null ? Collections.emptyList() : Collections.singletonList(sourceLocation); + } + + @Override + public String getMessage() { + return "Introspection has been disabled for this request"; + } + + @Override + public List getLocations() { + return locations; + } + + @Override + public ErrorClassification getErrorType() { + return ErrorClassification.errorClassification("IntrospectionDisabled"); + } +} diff --git a/src/main/java/graphql/schema/visibility/NoIntrospectionGraphqlFieldVisibility.java b/src/main/java/graphql/schema/visibility/NoIntrospectionGraphqlFieldVisibility.java index 604e794114..62aa16bbe0 100644 --- a/src/main/java/graphql/schema/visibility/NoIntrospectionGraphqlFieldVisibility.java +++ b/src/main/java/graphql/schema/visibility/NoIntrospectionGraphqlFieldVisibility.java @@ -12,10 +12,15 @@ * This field visibility will prevent Introspection queries from being performed. Technically this puts your * system in contravention of the specification * but some production systems want this lock down in place. + * + * @deprecated This is no longer the best way to prevent Introspection - {@link graphql.introspection.Introspection#enabledJvmWide(boolean)} + * can be used instead */ @PublicApi +@Deprecated(since = "2024-03-16") public class NoIntrospectionGraphqlFieldVisibility implements GraphqlFieldVisibility { + @Deprecated(since = "2024-03-16") public static NoIntrospectionGraphqlFieldVisibility NO_INTROSPECTION_FIELD_VISIBILITY = new NoIntrospectionGraphqlFieldVisibility(); diff --git a/src/test/groovy/graphql/introspection/IntrospectionTest.groovy b/src/test/groovy/graphql/introspection/IntrospectionTest.groovy index bee6b161e8..e7a1a647b2 100644 --- a/src/test/groovy/graphql/introspection/IntrospectionTest.groovy +++ b/src/test/groovy/graphql/introspection/IntrospectionTest.groovy @@ -1,7 +1,8 @@ package graphql.introspection - +import graphql.ExecutionInput import graphql.TestUtil +import graphql.execution.AsyncSerialExecutionStrategy import graphql.schema.DataFetcher import graphql.schema.FieldCoordinates import graphql.schema.GraphQLCodeRegistry @@ -22,6 +23,14 @@ import static graphql.schema.GraphQLSchema.newSchema class IntrospectionTest extends Specification { + def setup() { + Introspection.enabledJvmWide(true) + } + + def cleanup() { + Introspection.enabledJvmWide(true) + } + def "bug 1186 - introspection depth check"() { def spec = ''' type Query { @@ -544,8 +553,9 @@ class IntrospectionTest extends Specification { def newIntrospectionQuery = IntrospectionQuery.INTROSPECTION_QUERY then: - oldIntrospectionQuery.replaceAll("\\s+", "") == - newIntrospectionQuery.replaceAll("\\s+", "") + def oldQuery = oldIntrospectionQuery.replaceAll("\\s+", "") + def newQuery = newIntrospectionQuery.replaceAll("\\s+", "") + oldQuery == newQuery } def "test parameterized introspection queries"() { @@ -644,4 +654,112 @@ class IntrospectionTest extends Specification { arg['name'] == "arg" arg['defaultValue'] == "null" // printed AST } + + def "jvm wide enablement"() { + def graphQL = TestUtil.graphQL("type Query { f : String } ").build() + + when: + def er = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) + + then: + er.errors.isEmpty() + + when: + Introspection.enabledJvmWide(false) + er = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) + + then: + er.errors[0] instanceof IntrospectionDisabledError + er.errors[0].getErrorType().toString() == "IntrospectionDisabled" + + when: + Introspection.enabledJvmWide(true) + er = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) + + then: + er.errors.isEmpty() + } + + def "per request enablement"() { + def graphQL = TestUtil.graphQL("type Query { f : String } ").build() + + when: + // null context + def ei = ExecutionInput.newExecutionInput(IntrospectionQuery.INTROSPECTION_QUERY) + .build() + def er = graphQL.execute(ei) + + then: + er.errors.isEmpty() + + when: + ei = ExecutionInput.newExecutionInput(IntrospectionQuery.INTROSPECTION_QUERY) + .graphQLContext(Map.of(Introspection.INTROSPECTION_DISABLED, false)).build() + er = graphQL.execute(ei) + + then: + er.errors.isEmpty() + + when: + ei = ExecutionInput.newExecutionInput(IntrospectionQuery.INTROSPECTION_QUERY) + .graphQLContext(Map.of(Introspection.INTROSPECTION_DISABLED, true)).build() + er = graphQL.execute(ei) + + then: + er.errors[0] instanceof IntrospectionDisabledError + er.errors[0].getErrorType().toString() == "IntrospectionDisabled" + } + + def "mixed schema and other fields stop early"() { + def graphQL = TestUtil.graphQL("type Query { normalField : String } ").build() + + def query = """ + query goodAndBad { + normalField + __schema{ types{ fields { name }}} + } + """ + + when: + def er = graphQL.execute(query) + + then: + er.errors.isEmpty() + + when: + Introspection.enabledJvmWide(false) + er = graphQL.execute(query) + + then: + er.errors[0] instanceof IntrospectionDisabledError + er.errors[0].getErrorType().toString() == "IntrospectionDisabled" + er.data == null // stops hard + } + + def "AsyncSerialExecutionStrategy with jvm wide enablement"() { + def graphQL = TestUtil.graphQL("type Query { f : String } ") + .queryExecutionStrategy(new AsyncSerialExecutionStrategy()).build() + + when: + def er = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) + + then: + er.errors.isEmpty() + + when: + Introspection.enabledJvmWide(false) + er = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) + + then: + er.errors[0] instanceof IntrospectionDisabledError + er.errors[0].getErrorType().toString() == "IntrospectionDisabled" + + when: + Introspection.enabledJvmWide(true) + er = graphQL.execute(IntrospectionQuery.INTROSPECTION_QUERY) + + then: + er.errors.isEmpty() + } + } From 498f1eb9fafc072ebe75ce1ec6925186b3a7965a Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Tue, 19 Mar 2024 09:44:18 +1100 Subject: [PATCH 7/9] Cherry pick GoodFaithIntrospection #3527 --- .../execution/AsyncExecutionStrategy.java | 2 +- .../AsyncSerialExecutionStrategy.java | 2 +- .../introspection/GoodFaithIntrospection.java | 122 ++++++++++++++++ .../graphql/introspection/Introspection.java | 10 +- ...ithIntrospectionInstrumentationTest.groovy | 136 ++++++++++++++++++ ...ormalizedOperationToAstCompilerTest.groovy | 48 ++++--- 6 files changed, 299 insertions(+), 21 deletions(-) create mode 100644 src/main/java/graphql/introspection/GoodFaithIntrospection.java create mode 100644 src/test/groovy/graphql/introspection/GoodFaithIntrospectionInstrumentationTest.groovy diff --git a/src/main/java/graphql/execution/AsyncExecutionStrategy.java b/src/main/java/graphql/execution/AsyncExecutionStrategy.java index 68b2072e2f..2ca2e08942 100644 --- a/src/main/java/graphql/execution/AsyncExecutionStrategy.java +++ b/src/main/java/graphql/execution/AsyncExecutionStrategy.java @@ -47,7 +47,7 @@ public CompletableFuture execute(ExecutionContext executionCont MergedSelectionSet fields = parameters.getFields(); List fieldNames = fields.getKeys(); - Optional isNotSensible = Introspection.isIntrospectionSensible(executionContext.getGraphQLContext(),fields); + Optional isNotSensible = Introspection.isIntrospectionSensible(fields, executionContext); if (isNotSensible.isPresent()) { return CompletableFuture.completedFuture(isNotSensible.get()); } diff --git a/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java b/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java index c2d0cf1db7..06ddc4dca2 100644 --- a/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java +++ b/src/main/java/graphql/execution/AsyncSerialExecutionStrategy.java @@ -43,7 +43,7 @@ public CompletableFuture execute(ExecutionContext executionCont // this is highly unlikely since Mutations cant do introspection BUT in theory someone could make the query strategy this code // so belts and braces - Optional isNotSensible = Introspection.isIntrospectionSensible(executionContext.getGraphQLContext(), fields); + Optional isNotSensible = Introspection.isIntrospectionSensible(fields, executionContext); if (isNotSensible.isPresent()) { return CompletableFuture.completedFuture(isNotSensible.get()); } diff --git a/src/main/java/graphql/introspection/GoodFaithIntrospection.java b/src/main/java/graphql/introspection/GoodFaithIntrospection.java new file mode 100644 index 0000000000..7e853016ac --- /dev/null +++ b/src/main/java/graphql/introspection/GoodFaithIntrospection.java @@ -0,0 +1,122 @@ +package graphql.introspection; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import graphql.ErrorClassification; +import graphql.ExecutionResult; +import graphql.GraphQLContext; +import graphql.GraphQLError; +import graphql.PublicApi; +import graphql.execution.ExecutionContext; +import graphql.language.SourceLocation; +import graphql.normalized.ExecutableNormalizedField; +import graphql.normalized.ExecutableNormalizedOperation; +import graphql.schema.FieldCoordinates; + +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; + +import static graphql.schema.FieldCoordinates.coordinates; + +/** + * This {@link graphql.execution.instrumentation.Instrumentation} ensure that a submitted introspection query is done in + * good faith. + *

+ * There are attack vectors where a crafted introspection query can cause the engine to spend too much time + * producing introspection data. This is especially true on large schemas with lots of types and fields. + *

+ * Schemas form a cyclic graph and hence it's possible to send in introspection queries that can reference those cycles + * and in large schemas this can be expensive and perhaps a "denial of service". + *

+ * This instrumentation only allows one __schema field or one __type field to be present, and it does not allow the `__Type` fields + * to form a cycle, i.e., that can only be present once. This allows the standard and common introspection queries to work + * so tooling such as graphiql can work. + */ +@PublicApi +public class GoodFaithIntrospection { + + /** + * Placing a boolean value under this key in the per request {@link GraphQLContext} will enable + * or disable Good Faith Introspection on that request. + */ + public static final String GOOD_FAITH_INTROSPECTION_DISABLED = "GOOD_FAITH_INTROSPECTION_DISABLED"; + + private static final AtomicBoolean ENABLED_STATE = new AtomicBoolean(true); + + /** + * @return true if good faith introspection is enabled + */ + public static boolean isEnabledJvmWide() { + return ENABLED_STATE.get(); + } + + /** + * This allows you to disable good faith introspection, which is on by default. + * + * @param flag the desired state + * + * @return the previous state + */ + public static boolean enabledJvmWide(boolean flag) { + return ENABLED_STATE.getAndSet(flag); + } + + private static final Map ALLOWED_FIELD_INSTANCES = Map.of( + coordinates("Query", "__schema"), 1 + , coordinates("Query", "__type"), 1 + + , coordinates("__Type", "fields"), 1 + , coordinates("__Type", "inputFields"), 1 + , coordinates("__Type", "interfaces"), 1 + , coordinates("__Type", "possibleTypes"), 1 + ); + + public static Optional checkIntrospection(ExecutionContext executionContext) { + if (isIntrospectionEnabled(executionContext.getGraphQLContext())) { + ExecutableNormalizedOperation operation = executionContext.getNormalizedQueryTree().get(); + ImmutableListMultimap coordinatesToENFs = operation.getCoordinatesToNormalizedFields(); + for (Map.Entry entry : ALLOWED_FIELD_INSTANCES.entrySet()) { + FieldCoordinates coordinates = entry.getKey(); + Integer allowSize = entry.getValue(); + ImmutableList normalizedFields = coordinatesToENFs.get(coordinates); + if (normalizedFields.size() > allowSize) { + BadFaithIntrospectionError error = new BadFaithIntrospectionError(coordinates.toString()); + return Optional.of(ExecutionResult.newExecutionResult().addError(error).build()); + } + } + } + return Optional.empty(); + } + + private static boolean isIntrospectionEnabled(GraphQLContext graphQlContext) { + if (!isEnabledJvmWide()) { + return false; + } + return !graphQlContext.getOrDefault(GOOD_FAITH_INTROSPECTION_DISABLED, false); + } + + public static class BadFaithIntrospectionError implements GraphQLError { + private final String message; + + public BadFaithIntrospectionError(String qualifiedField) { + this.message = String.format("This request is not asking for introspection in good faith - %s is present too often!", qualifiedField); + } + + @Override + public String getMessage() { + return message; + } + + @Override + public ErrorClassification getErrorType() { + return ErrorClassification.errorClassification("BadFaithIntrospection"); + } + + @Override + public List getLocations() { + return null; + } + } +} diff --git a/src/main/java/graphql/introspection/Introspection.java b/src/main/java/graphql/introspection/Introspection.java index fe0268d19a..37bf624e7b 100644 --- a/src/main/java/graphql/introspection/Introspection.java +++ b/src/main/java/graphql/introspection/Introspection.java @@ -7,6 +7,7 @@ import graphql.GraphQLContext; import graphql.Internal; import graphql.PublicApi; +import graphql.execution.ExecutionContext; import graphql.execution.MergedField; import graphql.execution.MergedSelectionSet; import graphql.execution.ValuesResolver; @@ -108,10 +109,12 @@ public static boolean isEnabledJvmWide() { * that can be returned to the user. * * @param mergedSelectionSet the fields to be executed + * @param executionContext the execution context in play * * @return an optional error result */ - public static Optional isIntrospectionSensible(GraphQLContext graphQLContext, MergedSelectionSet mergedSelectionSet) { + public static Optional isIntrospectionSensible(MergedSelectionSet mergedSelectionSet, ExecutionContext executionContext) { + GraphQLContext graphQLContext = executionContext.getGraphQLContext(); MergedField schemaField = mergedSelectionSet.getSubField(SchemaMetaFieldDef.getName()); if (schemaField != null) { if (!isIntrospectionEnabled(graphQLContext)) { @@ -124,7 +127,10 @@ public static Optional isIntrospectionSensible(GraphQLContext g return mkDisabledError(typeField); } } - // later we can put a good faith check code here to check the fields make sense + if (schemaField != null || typeField != null) + { + return GoodFaithIntrospection.checkIntrospection(executionContext); + } return Optional.empty(); } diff --git a/src/test/groovy/graphql/introspection/GoodFaithIntrospectionInstrumentationTest.groovy b/src/test/groovy/graphql/introspection/GoodFaithIntrospectionInstrumentationTest.groovy new file mode 100644 index 0000000000..f1ffc2c570 --- /dev/null +++ b/src/test/groovy/graphql/introspection/GoodFaithIntrospectionInstrumentationTest.groovy @@ -0,0 +1,136 @@ +package graphql.introspection + +import graphql.ExecutionInput +import graphql.ExecutionResult +import graphql.TestUtil +import spock.lang.Specification + +class GoodFaithIntrospectionInstrumentationTest extends Specification { + + def graphql = TestUtil.graphQL("type Query { normalField : String }").build() + + def setup() { + GoodFaithIntrospection.enabledJvmWide(true) + } + def cleanup() { + GoodFaithIntrospection.enabledJvmWide(true) + } + + def "test asking for introspection in good faith"() { + + when: + ExecutionResult er = graphql.execute(IntrospectionQuery.INTROSPECTION_QUERY) + then: + er.errors.isEmpty() + } + + def "test asking for introspection in bad faith"() { + + when: + ExecutionResult er = graphql.execute(query) + then: + !er.errors.isEmpty() + er.errors[0] instanceof GoodFaithIntrospection.BadFaithIntrospectionError + + where: + query | _ + // long attack + """ + query badActor{__schema{types{fields{type{fields{type{fields{type{fields{type{name}}}}}}}}}}} + """ | _ + // a case for __Type interfaces + """ query badActor { + __schema { types { interfaces { fields { type { interfaces { name } } } } } } + } + """ | _ + // a case for __Type inputFields + """ query badActor { + __schema { types { inputFields { type { inputFields { name }}}}} + } + """ | _ + // a case for __Type possibleTypes + """ query badActor { + __schema { types { inputFields { type { inputFields { name }}}}} + } + """ | _ + // a case leading from __InputValue + """ query badActor { + __schema { types { fields { args { type { name fields { name }}}}}} + } + """ | _ + // a case leading from __Field + """ query badActor { + __schema { types { fields { type { name fields { name }}}}} + } + """ | _ + // a case for __type + """ query badActor { + __type(name : "t") { name } + alias1 : __type(name : "t1") { name } + } + """ | _ + // a case for schema repeated - dont ask twice + """ query badActor { + __schema { types { name} } + alias1 : __schema { types { name} } + } + """ | _ + } + + def "mixed general queries and introspections will be stopped anyway"() { + def query = """ + query goodAndBad { + normalField + __schema{types{fields{type{fields{type{fields{type{fields{type{name}}}}}}}}}} + } + """ + + when: + ExecutionResult er = graphql.execute(query) + then: + !er.errors.isEmpty() + er.errors[0] instanceof GoodFaithIntrospection.BadFaithIntrospectionError + er.data == null // it stopped hard - it did not continue to normal business + } + + def "can be disabled"() { + when: + def currentState = GoodFaithIntrospection.isEnabledJvmWide() + + then: + currentState + + when: + def prevState = GoodFaithIntrospection.enabledJvmWide(false) + + then: + prevState + + when: + ExecutionResult er = graphql.execute("query badActor{__schema{types{fields{type{fields{type{fields{type{fields{type{name}}}}}}}}}}}") + + then: + er.errors.isEmpty() + } + + def "can be disabled per request"() { + when: + def context = [(GoodFaithIntrospection.GOOD_FAITH_INTROSPECTION_DISABLED): true] + ExecutionInput executionInput = ExecutionInput.newExecutionInput("query badActor{__schema{types{fields{type{fields{type{fields{type{fields{type{name}}}}}}}}}}}") + .graphQLContext(context).build() + ExecutionResult er = graphql.execute(executionInput) + + then: + er.errors.isEmpty() + + when: + context = [(GoodFaithIntrospection.GOOD_FAITH_INTROSPECTION_DISABLED): false] + executionInput = ExecutionInput.newExecutionInput("query badActor{__schema{types{fields{type{fields{type{fields{type{fields{type{name}}}}}}}}}}}") + .graphQLContext(context).build() + er = graphql.execute(executionInput) + + then: + !er.errors.isEmpty() + er.errors[0] instanceof GoodFaithIntrospection.BadFaithIntrospectionError + } +} diff --git a/src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy b/src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy index 726a98eb63..6568880118 100644 --- a/src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy +++ b/src/test/groovy/graphql/normalized/ExecutableNormalizedOperationToAstCompilerTest.groovy @@ -1324,17 +1324,10 @@ class ExecutableNormalizedOperationToAstCompilerTest extends Specification { ''' } - def "introspection query can be printed"() { + def "introspection query can be printed __schema"() { def sdl = ''' type Query { - foo1: Foo - } - interface Foo { - test: String - } - type AFoo implements Foo { - test: String - aFoo: String + f: String } ''' def query = ''' @@ -1346,14 +1339,7 @@ class ExecutableNormalizedOperationToAstCompilerTest extends Specification { } } } - - __type(name: "World") { - name - fields { - name - } - } - } + } ''' GraphQLSchema schema = mkSchema(sdl) @@ -1370,6 +1356,34 @@ class ExecutableNormalizedOperationToAstCompilerTest extends Specification { } } } +} +''' + } + + def "introspection query can be printed __type"() { + def sdl = ''' + type Query { + f: String + } + ''' + def query = ''' + query introspection_query { + __type(name: "World") { + name + fields { + name + } + } + } + ''' + + GraphQLSchema schema = mkSchema(sdl) + def fields = createNormalizedFields(schema, query) + when: + def result = compileToDocument(schema, QUERY, null, fields, noVariables) + def documentPrinted = AstPrinter.printAst(new AstSorter().sort(result.document)) + then: + documentPrinted == '''{ __type(name: "World") { fields { name From 81b41b0b5cba4dcf9508aeeff65899d0fd06b32b Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Tue, 19 Mar 2024 11:37:26 +1100 Subject: [PATCH 8/9] Fix typo --- src/main/java/graphql/execution/ExecutionStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 01b57dea6d..22fa0262ee 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -251,7 +251,7 @@ private CompletableFuture fetchField(GraphQLFieldDefinition fieldD if ((maxNodes = executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) != null) { if (resultNodesCount > maxNodes) { executionContext.getResultNodesInfo().maxResultNodesExceeded(); - return CompletableFuture.completedFuture(new FetchedValue(null, Collections.emptyList(), null, null)); + return CompletableFuture.completedFuture(new FetchedValue(null, null, ImmutableKit.emptyList(), null)); } } From bfd6478d8caad46d50e462093d742996b39c33d4 Mon Sep 17 00:00:00 2001 From: dondonz <13839920+dondonz@users.noreply.github.com> Date: Tue, 19 Mar 2024 11:48:10 +1100 Subject: [PATCH 9/9] Fix hanging test - must return completed ExecutionResult, not null --- src/main/java/graphql/execution/ExecutionStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 22fa0262ee..73d5000677 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -591,7 +591,7 @@ protected FieldValueInfo completeValueForList(ExecutionContext executionContext, if ((maxNodes = executionContext.getGraphQLContext().get(MAX_RESULT_NODES)) != null) { if (resultNodesCount > maxNodes) { executionContext.getResultNodesInfo().maxResultNodesExceeded(); - return new FieldValueInfo(NULL, completedFuture(null), fieldValueInfos); + return new FieldValueInfo(NULL, completedFuture(ExecutionResult.newExecutionResult().build()), fieldValueInfos); } }