diff --git a/src/index.ts b/src/index.ts index 0500d695fe..ea06e75169 100644 --- a/src/index.ts +++ b/src/index.ts @@ -444,6 +444,7 @@ export { UniqueFieldDefinitionNamesRule, UniqueArgumentDefinitionNamesRule, UniqueDirectiveNamesRule, + NoDirectiveDefinitionCyclesRule, PossibleTypeExtensionsRule, // Custom validation rules NoDeprecatedCustomRule, diff --git a/src/utilities/__tests__/extendSchema-test.ts b/src/utilities/__tests__/extendSchema-test.ts index c6b86ede00..9a0c04eaea 100644 --- a/src/utilities/__tests__/extendSchema-test.ts +++ b/src/utilities/__tests__/extendSchema-test.ts @@ -315,7 +315,7 @@ describe('extendSchema', () => { someScalar(arg: SomeScalar): SomeScalar } - directive @foo(arg: SomeScalar) on SCALAR + directive @foo on SCALAR input FooInput { foo: SomeScalar diff --git a/src/validation/__tests__/NoDirectiveDefinitionCyclesRule-test.ts b/src/validation/__tests__/NoDirectiveDefinitionCyclesRule-test.ts new file mode 100644 index 0000000000..64d9cc8894 --- /dev/null +++ b/src/validation/__tests__/NoDirectiveDefinitionCyclesRule-test.ts @@ -0,0 +1,266 @@ +import { describe, it } from 'mocha'; + +import { expectJSON } from '../../__testUtils__/expectJSON.js'; + +import { parse } from '../../language/parser.js'; + +import type { GraphQLSchema } from '../../type/schema.js'; + +import { buildSchema } from '../../utilities/buildASTSchema.js'; + +import { NoDirectiveDefinitionCyclesRule } from '../rules/NoDirectiveDefinitionCyclesRule.js'; +import { validateSDL } from '../validate.js'; + +function expectErrors( + sdlStr: string, + schema?: GraphQLSchema, + parseOptions?: { experimentalDirectivesOnDirectiveDefinitions?: boolean }, +) { + const doc = parse(sdlStr, parseOptions); + const errors = validateSDL(doc, schema, [NoDirectiveDefinitionCyclesRule]); + return expectJSON(errors); +} + +function expectValid( + sdlStr: string, + schema?: GraphQLSchema, + parseOptions?: { experimentalDirectivesOnDirectiveDefinitions?: boolean }, +) { + expectErrors(sdlStr, schema, parseOptions).toDeepEqual([]); +} + +describe('Validate: No directive definition cycles', () => { + it('single reference is valid', () => { + expectValid(` + directive @a(arg: String @b) on FIELD_DEFINITION + directive @b on ARGUMENT_DEFINITION + `); + }); + + it('does not false positive on unknown directive', () => { + expectValid(` + directive @a(arg: String @unknown) on FIELD_DEFINITION + `); + }); + + it('rejects a self-referential directive definition', () => { + expectErrors(` + directive @self(arg: String @self) on FIELD_DEFINITION + `).toDeepEqual([ + { + message: 'Cannot reference directive "@self" within itself.', + locations: [{ line: 2, column: 35 }], + }, + ]); + }); + + it('rejects directive definitions with circular references', () => { + expectErrors(` + directive @a(arg: String @b) on FIELD_DEFINITION + directive @b(arg: String @a) on FIELD_DEFINITION + `).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of directive applications: "@b", "@a".', + locations: [ + { line: 2, column: 32 }, + { line: 3, column: 32 }, + ], + }, + ]); + }); + + it('rejects directive definitions with overlapping circular references', () => { + expectErrors(` + directive @a(arg: String @b) on FIELD_DEFINITION + directive @b(arg: String @c) on FIELD_DEFINITION + directive @c(first: String @a, second: String @d) on FIELD_DEFINITION + directive @d(arg: String @b) on FIELD_DEFINITION + `).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of directive applications: "@b", "@c", "@a".', + locations: [ + { line: 2, column: 32 }, + { line: 3, column: 32 }, + { line: 4, column: 34 }, + ], + }, + { + message: + 'Cannot reference directive "@b" within itself through a series of directive applications: "@c", "@d", "@b".', + locations: [ + { line: 3, column: 32 }, + { line: 4, column: 53 }, + { line: 5, column: 32 }, + ], + }, + ]); + }); + + it('rejects directive definitions that recurse through a referenced type', () => { + expectErrors(` + directive @a(arg: InputObject) on FIELD_DEFINITION + + input InputObject { + value: String @a + } + `).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of references: "InputObject", "@a".', + locations: [ + { line: 2, column: 25 }, + { line: 5, column: 23 }, + ], + }, + ]); + }); + + it('does not duplicate cycles through recursive referenced types', () => { + expectErrors(` + directive @a(arg: InputObject) on INPUT_FIELD_DEFINITION + input InputObject { + self: InputObject @a + } + `).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of references: "InputObject", "@a".', + locations: [ + { line: 2, column: 25 }, + { line: 4, column: 27 }, + ], + }, + ]); + }); + + it('rejects type extensions that create cycles with existing directives', () => { + const schema = buildSchema( + ` + directive @a(arg: InputObject) on INPUT_FIELD_DEFINITION + input InputObject { + value: String + } + `, + { noLocation: true }, + ); + + expectErrors( + ` + extend input InputObject { + recursive: String @a + } + `, + schema, + ).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of references: "InputObject", "@a".', + locations: [{ line: 3, column: 29 }], + }, + ]); + }); + + it('rejects directives on directive definitions when the syntax exists', () => { + expectErrors( + ` + directive @a @b on DIRECTIVE_DEFINITION + directive @b @a on DIRECTIVE_DEFINITION + `, + undefined, + { + experimentalDirectivesOnDirectiveDefinitions: true, + }, + ).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of directive applications: "@b", "@a".', + locations: [ + { line: 2, column: 22 }, + { line: 3, column: 22 }, + ], + }, + ]); + }); + + it('rejects directive extensions with circular references', () => { + const schema = buildSchema( + ` + directive @a on DIRECTIVE_DEFINITION + directive @b on DIRECTIVE_DEFINITION + `, + { + noLocation: true, + experimentalDirectivesOnDirectiveDefinitions: true, + }, + ); + + expectErrors( + ` + extend directive @a @b + extend directive @b @a + `, + schema, + { + experimentalDirectivesOnDirectiveDefinitions: true, + }, + ).toDeepEqual([ + { + message: + 'Cannot reference directive "@a" within itself through a series of directive applications: "@b", "@a".', + locations: [ + { line: 2, column: 29 }, + { line: 3, column: 29 }, + ], + }, + ]); + }); + + it('ignores directive applications from the schema being extended', () => { + const schema = buildSchema( + ` + directive @a @b on DIRECTIVE_DEFINITION + directive @b on DIRECTIVE_DEFINITION + `, + { + noLocation: true, + experimentalDirectivesOnDirectiveDefinitions: true, + }, + ); + + expectValid( + ` + extend directive @b @a + `, + schema, + { + experimentalDirectivesOnDirectiveDefinitions: true, + }, + ); + }); + + it('only considers applied directives from the SDL document being validated', () => { + const schema = buildSchema(` + directive @a(arg: InputObject) on INPUT_FIELD_DEFINITION + input InputObject { + field: String @b + } + directive @b on INPUT_FIELD_DEFINITION + `); + + // `schemaToExtend` still contributes the directive argument reference + // from `@a` to `InputObject`, but the already-applied `@b` on + // `InputObject.field` is outside the current SDL document's validation + // surface. Counting it would incorrectly report `@b -> @a -> InputObject -> @b`. + expectValid( + ` + extend directive @b @a + `, + schema, + { + experimentalDirectivesOnDirectiveDefinitions: true, + }, + ); + }); +}); diff --git a/src/validation/index.ts b/src/validation/index.ts index b1998b5300..49afca59a7 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -109,6 +109,7 @@ export { UniqueEnumValueNamesRule } from './rules/UniqueEnumValueNamesRule.js'; export { UniqueFieldDefinitionNamesRule } from './rules/UniqueFieldDefinitionNamesRule.js'; export { UniqueArgumentDefinitionNamesRule } from './rules/UniqueArgumentDefinitionNamesRule.js'; export { UniqueDirectiveNamesRule } from './rules/UniqueDirectiveNamesRule.js'; +export { NoDirectiveDefinitionCyclesRule } from './rules/NoDirectiveDefinitionCyclesRule.js'; export { PossibleTypeExtensionsRule } from './rules/PossibleTypeExtensionsRule.js'; // Optional rules not defined by the GraphQL Specification diff --git a/src/validation/rules/NoDirectiveDefinitionCyclesRule.ts b/src/validation/rules/NoDirectiveDefinitionCyclesRule.ts new file mode 100644 index 0000000000..6df79edad8 --- /dev/null +++ b/src/validation/rules/NoDirectiveDefinitionCyclesRule.ts @@ -0,0 +1,281 @@ +import type { ObjMap } from '../../jsutils/ObjMap.js'; + +import { GraphQLError } from '../../error/GraphQLError.js'; + +import type { + DirectiveDefinitionNode, + DirectiveExtensionNode, + DirectiveNode, + InputValueDefinitionNode, + NamedTypeNode, + TypeDefinitionNode, + TypeExtensionNode, + TypeNode, +} from '../../language/ast.js'; +import { Kind } from '../../language/kinds.js'; +import type { ASTVisitor } from '../../language/visitor.js'; + +import type { SDLValidationContext } from '../ValidationContext.js'; + +type ReferenceNode = DirectiveNode | NamedTypeNode; +type ReferenceOwnerNode = + | DirectiveDefinitionNode + | DirectiveExtensionNode + | TypeDefinitionNode + | TypeExtensionNode; + +interface Reference { + readonly key: string; + readonly node: ReferenceNode; + readonly isFromDocument: boolean; +} + +/** + * No directive definition cycles + * + * The graph of directives used within directive definitions must not form any + * cycles including referencing itself. This includes directives used on + * directive arguments and, when the experimental syntax is enabled, directives + * applied directly to directive definitions and extensions. + * + * See https://spec.graphql.org/draft/#sec-Type-System.Directives + */ +export function NoDirectiveDefinitionCyclesRule( + context: SDLValidationContext, +): ASTVisitor { + const visitedDirectives: ObjMap = Object.create(null); + const referencePath: Array = []; + const referencePathIndexByKey: ObjMap = + Object.create(null); + + const referencesByKey: ObjMap> = Object.create(null); + let currentKey: string | undefined; + + const schema = context.getSchema(); + if (schema != null) { + // Existing applied directives are not part of the document being validated; + // only persisted schema structure can contribute references here. + for (const directive of schema.getDirectives()) { + const key = '@' + directive.name; + for (const node of [directive.astNode, ...directive.extensionASTNodes]) { + if (node != null) { + addDirectiveDefinitionReferences(key, node); + } + } + } + + for (const type of Object.values(schema.getTypeMap())) { + for (const node of [type.astNode, ...type.extensionASTNodes]) { + if (node != null) { + addTypeDefinitionReferences(type.name, node); + } + } + } + } + + const referenceOwnerVisitor = { + enter: enterReferenceOwner, + leave: leaveReferenceOwner, + }; + + return { + DirectiveDefinition: referenceOwnerVisitor, + DirectiveExtension: referenceOwnerVisitor, + ScalarTypeDefinition: referenceOwnerVisitor, + ScalarTypeExtension: referenceOwnerVisitor, + ObjectTypeDefinition: referenceOwnerVisitor, + ObjectTypeExtension: referenceOwnerVisitor, + InterfaceTypeDefinition: referenceOwnerVisitor, + InterfaceTypeExtension: referenceOwnerVisitor, + UnionTypeDefinition: referenceOwnerVisitor, + UnionTypeExtension: referenceOwnerVisitor, + EnumTypeDefinition: referenceOwnerVisitor, + EnumTypeExtension: referenceOwnerVisitor, + InputObjectTypeDefinition: referenceOwnerVisitor, + InputObjectTypeExtension: referenceOwnerVisitor, + Directive: collectReference, + NamedType: collectReference, + Document: { + leave() { + for (const key of Object.keys(referencesByKey)) { + if (key.startsWith('@')) { + detectCycleRecursive(key); + } + } + }, + }, + }; + + function enterReferenceOwner(node: ReferenceOwnerNode): void { + currentKey = + node.kind === Kind.DIRECTIVE_DEFINITION || + node.kind === Kind.DIRECTIVE_EXTENSION + ? '@' + node.name.value + : node.name.value; + } + + function leaveReferenceOwner(): void { + currentKey = undefined; + } + + function collectReference(node: ReferenceNode): false { + if (currentKey !== undefined) { + addReference(currentKey, node, true); + } + return false; + } + + function detectCycleRecursive(key: string): void { + if (key.startsWith('@')) { + if (visitedDirectives[key]) { + return; + } + visitedDirectives[key] = true; + } + + referencePathIndexByKey[key] = referencePath.length; + + for (const reference of referencesByKey[key] ?? []) { + const cycleIndex = referencePathIndexByKey[reference.key]; + + referencePath.push(reference); + if (cycleIndex === undefined) { + detectCycleRecursive(reference.key); + } else if (reference.key.startsWith('@')) { + const cyclePath = referencePath.slice(cycleIndex); + if (cyclePath.some((cycleReference) => cycleReference.isFromDocument)) { + reportCycle( + reference.key.slice(1), + cyclePath.map((cycleReference) => cycleReference.node), + ); + } + } + referencePath.pop(); + } + + referencePathIndexByKey[key] = undefined; + } + + function addDirectiveDefinitionReferences( + key: string, + node: DirectiveDefinitionNode | DirectiveExtensionNode, + ): void { + if (node.kind === Kind.DIRECTIVE_DEFINITION) { + addInputValueDefinitionReferences(key, node.arguments); + } + } + + function addTypeDefinitionReferences( + key: string, + node: TypeDefinitionNode | TypeExtensionNode, + ): void { + if ( + node.kind === Kind.OBJECT_TYPE_DEFINITION || + node.kind === Kind.OBJECT_TYPE_EXTENSION || + node.kind === Kind.INTERFACE_TYPE_DEFINITION || + node.kind === Kind.INTERFACE_TYPE_EXTENSION + ) { + addNamedTypeReferences(key, node.interfaces); + } + + switch (node.kind) { + case Kind.SCALAR_TYPE_DEFINITION: + case Kind.SCALAR_TYPE_EXTENSION: + break; + + case Kind.OBJECT_TYPE_DEFINITION: + case Kind.OBJECT_TYPE_EXTENSION: + case Kind.INTERFACE_TYPE_DEFINITION: + case Kind.INTERFACE_TYPE_EXTENSION: + for (const field of node.fields ?? []) { + addInputValueDefinitionReferences(key, field.arguments); + addTypeReference(key, field.type); + } + break; + + case Kind.UNION_TYPE_DEFINITION: + case Kind.UNION_TYPE_EXTENSION: + addNamedTypeReferences(key, node.types); + break; + + case Kind.ENUM_TYPE_DEFINITION: + case Kind.ENUM_TYPE_EXTENSION: + break; + + case Kind.INPUT_OBJECT_TYPE_DEFINITION: + case Kind.INPUT_OBJECT_TYPE_EXTENSION: + addInputValueDefinitionReferences(key, node.fields); + break; + } + } + + function addInputValueDefinitionReferences( + key: string, + inputValues: ReadonlyArray | undefined, + ): void { + for (const inputValue of inputValues ?? []) { + addTypeReference(key, inputValue.type); + } + } + + function addNamedTypeReferences( + key: string, + nodes: ReadonlyArray | undefined, + ): void { + for (const node of nodes ?? []) { + addReference(key, node, false); + } + } + + function addTypeReference(key: string, typeNode: TypeNode): void { + let namedType = typeNode; + while ( + namedType.kind === Kind.LIST_TYPE || + namedType.kind === Kind.NON_NULL_TYPE + ) { + namedType = namedType.type; + } + + addReference(key, namedType, false); + } + + function addReference( + key: string, + node: ReferenceNode, + isFromDocument: boolean, + ): void { + const referenceKey = + node.kind === Kind.DIRECTIVE ? '@' + node.name.value : node.name.value; + + referencesByKey[key] ??= []; + referencesByKey[key].push({ key: referenceKey, node, isFromDocument }); + } + + function reportCycle( + directiveName: string, + cyclePath: ReadonlyArray, + ): void { + const viaPath = cyclePath.slice(0, -1).map(formatReference).join(', '); + const referencesDescription = cyclePath.some( + (referenceNode) => referenceNode.kind === Kind.NAMED_TYPE, + ) + ? ' through a series of references' + : ' through a series of directive applications'; + + context.reportError( + new GraphQLError( + `Cannot reference directive "@${directiveName}" within itself` + + (viaPath !== '' + ? `${referencesDescription}: ${viaPath}, "@${directiveName}".` + : '.'), + { nodes: cyclePath }, + ), + ); + } + + function formatReference(referenceNode: ReferenceNode): string { + return referenceNode.kind === Kind.DIRECTIVE + ? '"@' + referenceNode.name.value + '"' + : '"' + referenceNode.name.value + '"'; + } +} diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index 225b87e3fc..ef26c591dc 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -29,6 +29,8 @@ import { LoneAnonymousOperationRule } from './rules/LoneAnonymousOperationRule.j import { LoneSchemaDefinitionRule } from './rules/LoneSchemaDefinitionRule.js'; // TODO: Spec Section import { MaxIntrospectionDepthRule } from './rules/MaxIntrospectionDepthRule.js'; +// Spec Section: "Directives" +import { NoDirectiveDefinitionCyclesRule } from './rules/NoDirectiveDefinitionCyclesRule.js'; // Spec Section: "Fragments must not form cycles" import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule.js'; // Spec Section: "All Variable Used Defined" @@ -142,6 +144,7 @@ export const specifiedSDLRules: ReadonlyArray = UniqueDirectiveNamesRule, KnownTypeNamesRule, KnownDirectivesRule, + NoDirectiveDefinitionCyclesRule, UniqueDirectivesPerLocationRule, PossibleTypeExtensionsRule, KnownArgumentNamesOnDirectivesRule,