From 57d7b110869ca23f8f40fd7ebd36953c11bd793a Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Mon, 9 Oct 2023 20:07:10 +0100 Subject: [PATCH 1/5] Forbid @skip and @include directives in subscription root selection --- src/execution/collectFields.ts | 60 ++++++++++++++++--- .../SingleFieldSubscriptionsRule-test.ts | 42 +++++++++++++ .../rules/SingleFieldSubscriptionsRule.ts | 38 ++++++++---- 3 files changed, 119 insertions(+), 21 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 1d0341b4cc..073d9fc905 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -5,6 +5,7 @@ import { isSameSet } from '../jsutils/isSameSet.js'; import type { ObjMap } from '../jsutils/ObjMap.js'; import type { + DirectiveNode, FieldNode, FragmentDefinitionNode, FragmentSpreadNode, @@ -26,7 +27,7 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -import { getDirectiveValues } from './values.js'; +import { getArgumentValues, getDirectiveValues } from './values.js'; export interface DeferUsage { label: string | undefined; @@ -60,6 +61,7 @@ export interface CollectFieldsResult { groupedFieldSet: GroupedFieldSet; newGroupedFieldSetDetails: Map; newDeferUsages: ReadonlyArray; + forbiddenDirectiveInstances: ReadonlyArray; } interface CollectFieldsContext { @@ -72,6 +74,7 @@ interface CollectFieldsContext { fieldsByTarget: Map>; newDeferUsages: Array; visitedFragmentNames: Set; + forbiddenDirectiveInstances: Array; } /** @@ -100,6 +103,7 @@ export function collectFields( targetsByKey: new Map(), newDeferUsages: [], visitedFragmentNames: new Set(), + forbiddenDirectiveInstances: [], }; collectFieldsImpl(context, operation.selectionSet); @@ -107,9 +111,20 @@ export function collectFields( return { ...buildGroupedFieldSets(context.targetsByKey, context.fieldsByTarget), newDeferUsages: context.newDeferUsages, + forbiddenDirectiveInstances: context.forbiddenDirectiveInstances, }; } +/** + * This variable is the empty variables used during the validation phase (where + * no variables exist) for field collection; if a `@skip` or `@include` + * directive is ever seen when `variableValues` is set to this, it should + * throw. + */ +export const VALIDATION_PHASE_EMPTY_VARIABLES: { + [variable: string]: any; +} = Object.freeze(Object.create(null)); + /** * Given an array of field nodes, collects all of the subfields of the passed * in fields, and returns them at the end. @@ -139,6 +154,7 @@ export function collectSubfields( targetsByKey: new Map(), newDeferUsages: [], visitedFragmentNames: new Set(), + forbiddenDirectiveInstances: [], }; for (const fieldDetails of fieldGroup.fields) { @@ -155,6 +171,7 @@ export function collectSubfields( fieldGroup.targets, ), newDeferUsages: context.newDeferUsages, + forbiddenDirectiveInstances: context.forbiddenDirectiveInstances, }; } @@ -179,7 +196,7 @@ function collectFieldsImpl( for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - if (!shouldIncludeNode(variableValues, selection)) { + if (!shouldIncludeNode(context, variableValues, selection)) { continue; } const key = getFieldEntryKey(selection); @@ -200,7 +217,7 @@ function collectFieldsImpl( } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(variableValues, selection) || + !shouldIncludeNode(context, variableValues, selection) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -232,7 +249,7 @@ function collectFieldsImpl( case Kind.FRAGMENT_SPREAD: { const fragName = selection.name.value; - if (!shouldIncludeNode(variableValues, selection)) { + if (!shouldIncludeNode(context, variableValues, selection)) { continue; } @@ -304,19 +321,44 @@ function getDeferValues( * directives, where `@skip` has higher precedence than `@include`. */ function shouldIncludeNode( + context: CollectFieldsContext, variableValues: { [variable: string]: unknown }, node: FragmentSpreadNode | FieldNode | InlineFragmentNode, ): boolean { - const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues); + const skipDirectiveNode = node.directives?.find( + (directive) => directive.name.value === GraphQLSkipDirective.name, + ); + if ( + skipDirectiveNode && + variableValues === VALIDATION_PHASE_EMPTY_VARIABLES + ) { + context.forbiddenDirectiveInstances.push(skipDirectiveNode); + return false; + } + const skip = skipDirectiveNode + ? getArgumentValues(GraphQLSkipDirective, skipDirectiveNode, variableValues) + : undefined; if (skip?.if === true) { return false; } - const include = getDirectiveValues( - GraphQLIncludeDirective, - node, - variableValues, + const includeDirectiveNode = node.directives?.find( + (directive) => directive.name.value === GraphQLIncludeDirective.name, ); + if ( + includeDirectiveNode && + variableValues === VALIDATION_PHASE_EMPTY_VARIABLES + ) { + context.forbiddenDirectiveInstances.push(includeDirectiveNode); + return false; + } + const include = includeDirectiveNode + ? getArgumentValues( + GraphQLIncludeDirective, + includeDirectiveNode, + variableValues, + ) + : undefined; if (include?.if === false) { return false; } diff --git a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts index 7e0a227d07..7e301b3720 100644 --- a/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts +++ b/src/validation/__tests__/SingleFieldSubscriptionsRule-test.ts @@ -286,6 +286,48 @@ describe('Validate: Subscriptions with single field', () => { ]); }); + it('fails with @skip or @include directive', () => { + expectErrors(` + subscription RequiredRuntimeValidation($bool: Boolean!) { + newMessage @include(if: $bool) { + body + sender + } + disallowedSecondRootField @skip(if: $bool) + } + `).toDeepEqual([ + { + message: + 'Subscription "RequiredRuntimeValidation" must not use `@skip` or `@include` directives in the top level selection.', + locations: [ + { line: 3, column: 20 }, + { line: 7, column: 35 }, + ], + }, + ]); + }); + + it('fails with @skip or @include directive in anonymous subscription', () => { + expectErrors(` + subscription ($bool: Boolean!) { + newMessage @include(if: $bool) { + body + sender + } + disallowedSecondRootField @skip(if: $bool) + } + `).toDeepEqual([ + { + message: + 'Anonymous Subscription must not use `@skip` or `@include` directives in the top level selection.', + locations: [ + { line: 3, column: 20 }, + { line: 7, column: 35 }, + ], + }, + ]); + }); + it('skips if not subscription type', () => { const emptySchema = buildSchema(` type Query { diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index c0d1031103..aeb76af51a 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -11,7 +11,10 @@ import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; import type { FieldGroup } from '../../execution/collectFields.js'; -import { collectFields } from '../../execution/collectFields.js'; +import { + collectFields, + VALIDATION_PHASE_EMPTY_VARIABLES, +} from '../../execution/collectFields.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -23,7 +26,8 @@ function toNodes(fieldGroup: FieldGroup): ReadonlyArray { * Subscriptions must only include a non-introspection field. * * A GraphQL subscription is valid only if it contains a single root field and - * that root field is not an introspection field. + * that root field is not an introspection field. `@skip` and `@include` + * directives are forbidden. * * See https://spec.graphql.org/draft/#sec-Single-root-field */ @@ -37,9 +41,7 @@ export function SingleFieldSubscriptionsRule( const subscriptionType = schema.getSubscriptionType(); if (subscriptionType) { const operationName = node.name ? node.name.value : null; - const variableValues: { - [variable: string]: any; - } = Object.create(null); + const variableValues = VALIDATION_PHASE_EMPTY_VARIABLES; const document = context.getDocument(); const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { @@ -47,13 +49,25 @@ export function SingleFieldSubscriptionsRule( fragments[definition.name.value] = definition; } } - const { groupedFieldSet } = collectFields( - schema, - fragments, - variableValues, - subscriptionType, - node, - ); + const { groupedFieldSet, forbiddenDirectiveInstances } = + collectFields( + schema, + fragments, + variableValues, + subscriptionType, + node, + ); + if (forbiddenDirectiveInstances.length > 0) { + context.reportError( + new GraphQLError( + operationName != null + ? `Subscription "${operationName}" must not use \`@skip\` or \`@include\` directives in the top level selection.` + : 'Anonymous Subscription must not use `@skip` or `@include` directives in the top level selection.', + { nodes: forbiddenDirectiveInstances }, + ), + ); + return; + } if (groupedFieldSet.size > 1) { const fieldGroups = [...groupedFieldSet.values()]; const extraFieldGroups = fieldGroups.slice(1); From 6b658cf47830d28b01b95331ddc66f52228d365e Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Wed, 26 Feb 2025 15:14:36 +0000 Subject: [PATCH 2/5] Be more explicit about forbidding skip/include --- src/execution/collectFields.ts | 37 +++++++++---------- .../rules/SingleFieldSubscriptionsRule.ts | 9 ++--- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index d8bac290c2..06df3e4948 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -78,6 +78,7 @@ export function collectFields( runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, hideSuggestions: boolean, + forbidSkipInclude = false, ): { groupedFieldSet: GroupedFieldSet; newDeferUsages: ReadonlyArray; @@ -95,7 +96,15 @@ export function collectFields( forbiddenDirectiveInstances: [], }; - collectFieldsImpl(context, selectionSet, groupedFieldSet, newDeferUsages); + collectFieldsImpl( + context, + selectionSet, + groupedFieldSet, + newDeferUsages, + undefined, + undefined, + forbidSkipInclude, + ); return { groupedFieldSet, newDeferUsages, @@ -103,16 +112,6 @@ export function collectFields( }; } -/** - * This variable is the empty variables used during the validation phase (where - * no variables exist) for field collection; if a `@skip` or `@include` - * directive is ever seen when `variableValues` is set to this, it should - * throw. - */ -export const VALIDATION_PHASE_EMPTY_VARIABLES: VariableValues = Object.freeze( - Object.create(null), -); - /** * Given an array of field nodes, collects all of the subfields of the passed * in fields, and returns them at the end. @@ -134,6 +133,7 @@ export function collectSubfields( ): { groupedFieldSet: GroupedFieldSet; newDeferUsages: ReadonlyArray; + forbiddenDirectiveInstances: ReadonlyArray; } { const context: CollectFieldsContext = { schema, @@ -177,6 +177,7 @@ function collectFieldsImpl( newDeferUsages: Array, deferUsage?: DeferUsage, fragmentVariableValues?: VariableValues, + forbidSkipInclude = false, ): void { const { schema, @@ -197,6 +198,7 @@ function collectFieldsImpl( variableValues, fragmentVariableValues, hideSuggestions, + forbidSkipInclude, ) ) { continue; @@ -216,6 +218,7 @@ function collectFieldsImpl( variableValues, fragmentVariableValues, hideSuggestions, + forbidSkipInclude, ) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { @@ -263,6 +266,7 @@ function collectFieldsImpl( variableValues, fragmentVariableValues, hideSuggestions, + forbidSkipInclude, ) ) { continue; @@ -364,14 +368,12 @@ function shouldIncludeNode( variableValues: VariableValues, fragmentVariableValues: VariableValues | undefined, hideSuggestions: Maybe, + forbidSkipInclude: boolean, ): boolean { const skipDirectiveNode = node.directives?.find( (directive) => directive.name.value === GraphQLSkipDirective.name, ); - if ( - skipDirectiveNode && - variableValues === VALIDATION_PHASE_EMPTY_VARIABLES - ) { + if (skipDirectiveNode && forbidSkipInclude) { context.forbiddenDirectiveInstances.push(skipDirectiveNode); return false; } @@ -391,10 +393,7 @@ function shouldIncludeNode( const includeDirectiveNode = node.directives?.find( (directive) => directive.name.value === GraphQLIncludeDirective.name, ); - if ( - includeDirectiveNode && - variableValues === VALIDATION_PHASE_EMPTY_VARIABLES - ) { + if (includeDirectiveNode && forbidSkipInclude) { context.forbiddenDirectiveInstances.push(includeDirectiveNode); return false; } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 7fdf2972f2..5d8bdb615d 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -10,10 +10,8 @@ import type { FieldDetailsList, FragmentDetails, } from '../../execution/collectFields.js'; -import { - collectFields, - VALIDATION_PHASE_EMPTY_VARIABLES, -} from '../../execution/collectFields.js'; +import { collectFields } from '../../execution/collectFields.js'; +import type { VariableValues } from '../../execution/values.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -40,7 +38,7 @@ export function SingleFieldSubscriptionsRule( const subscriptionType = schema.getSubscriptionType(); if (subscriptionType) { const operationName = node.name ? node.name.value : null; - const variableValues = VALIDATION_PHASE_EMPTY_VARIABLES; + const variableValues: VariableValues = Object.create(null); const document = context.getDocument(); const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { @@ -56,6 +54,7 @@ export function SingleFieldSubscriptionsRule( subscriptionType, node.selectionSet, context.hideSuggestions, + true, ); if (forbiddenDirectiveInstances.length > 0) { context.reportError( From 93f71d8ba8d98c6e35784623367ebaaaf4d86a70 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Wed, 26 Feb 2025 15:31:27 +0000 Subject: [PATCH 3/5] Use context instead --- src/execution/collectFields.ts | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 06df3e4948..785116f6af 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -59,6 +59,7 @@ interface CollectFieldsContext { visitedFragmentNames: Set; hideSuggestions: boolean; forbiddenDirectiveInstances: Array; + forbidSkipAndInclude: boolean; } /** @@ -78,7 +79,7 @@ export function collectFields( runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, hideSuggestions: boolean, - forbidSkipInclude = false, + forbidSkipAndInclude = false, ): { groupedFieldSet: GroupedFieldSet; newDeferUsages: ReadonlyArray; @@ -94,17 +95,10 @@ export function collectFields( visitedFragmentNames: new Set(), hideSuggestions, forbiddenDirectiveInstances: [], + forbidSkipAndInclude, }; - collectFieldsImpl( - context, - selectionSet, - groupedFieldSet, - newDeferUsages, - undefined, - undefined, - forbidSkipInclude, - ); + collectFieldsImpl(context, selectionSet, groupedFieldSet, newDeferUsages); return { groupedFieldSet, newDeferUsages, @@ -143,6 +137,7 @@ export function collectSubfields( visitedFragmentNames: new Set(), hideSuggestions, forbiddenDirectiveInstances: [], + forbidSkipAndInclude: false, }; const subGroupedFieldSet = new AccumulatorMap(); const newDeferUsages: Array = []; @@ -177,7 +172,6 @@ function collectFieldsImpl( newDeferUsages: Array, deferUsage?: DeferUsage, fragmentVariableValues?: VariableValues, - forbidSkipInclude = false, ): void { const { schema, @@ -198,7 +192,6 @@ function collectFieldsImpl( variableValues, fragmentVariableValues, hideSuggestions, - forbidSkipInclude, ) ) { continue; @@ -218,7 +211,6 @@ function collectFieldsImpl( variableValues, fragmentVariableValues, hideSuggestions, - forbidSkipInclude, ) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { @@ -266,7 +258,6 @@ function collectFieldsImpl( variableValues, fragmentVariableValues, hideSuggestions, - forbidSkipInclude, ) ) { continue; @@ -368,12 +359,11 @@ function shouldIncludeNode( variableValues: VariableValues, fragmentVariableValues: VariableValues | undefined, hideSuggestions: Maybe, - forbidSkipInclude: boolean, ): boolean { const skipDirectiveNode = node.directives?.find( (directive) => directive.name.value === GraphQLSkipDirective.name, ); - if (skipDirectiveNode && forbidSkipInclude) { + if (skipDirectiveNode && context.forbidSkipAndInclude) { context.forbiddenDirectiveInstances.push(skipDirectiveNode); return false; } @@ -393,7 +383,7 @@ function shouldIncludeNode( const includeDirectiveNode = node.directives?.find( (directive) => directive.name.value === GraphQLIncludeDirective.name, ); - if (includeDirectiveNode && forbidSkipInclude) { + if (includeDirectiveNode && context.forbidSkipAndInclude) { context.forbiddenDirectiveInstances.push(includeDirectiveNode); return false; } From 5053439a86b100054c3a7a89be476f086cad284e Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Wed, 26 Feb 2025 15:54:58 +0000 Subject: [PATCH 4/5] CollectSubfields doesn't need to collect forbidden directive instances --- src/execution/collectFields.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 785116f6af..1d1f65d891 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -127,7 +127,6 @@ export function collectSubfields( ): { groupedFieldSet: GroupedFieldSet; newDeferUsages: ReadonlyArray; - forbiddenDirectiveInstances: ReadonlyArray; } { const context: CollectFieldsContext = { schema, @@ -160,7 +159,6 @@ export function collectSubfields( return { groupedFieldSet: subGroupedFieldSet, newDeferUsages, - forbiddenDirectiveInstances: context.forbiddenDirectiveInstances, }; } From 794e73aff3ab4a4754424617a34b7767b61aa125 Mon Sep 17 00:00:00 2001 From: Benjie Gillam Date: Wed, 26 Feb 2025 15:56:07 +0000 Subject: [PATCH 5/5] hideSuggestions exists on context --- src/execution/collectFields.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 1d1f65d891..bc00b413d8 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -1,5 +1,4 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap.js'; -import type { Maybe } from '../jsutils/Maybe.js'; import type { ObjMap } from '../jsutils/ObjMap.js'; import type { @@ -189,7 +188,6 @@ function collectFieldsImpl( selection, variableValues, fragmentVariableValues, - hideSuggestions, ) ) { continue; @@ -208,7 +206,6 @@ function collectFieldsImpl( selection, variableValues, fragmentVariableValues, - hideSuggestions, ) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { @@ -255,7 +252,6 @@ function collectFieldsImpl( selection, variableValues, fragmentVariableValues, - hideSuggestions, ) ) { continue; @@ -356,7 +352,6 @@ function shouldIncludeNode( node: FragmentSpreadNode | FieldNode | InlineFragmentNode, variableValues: VariableValues, fragmentVariableValues: VariableValues | undefined, - hideSuggestions: Maybe, ): boolean { const skipDirectiveNode = node.directives?.find( (directive) => directive.name.value === GraphQLSkipDirective.name, @@ -371,7 +366,7 @@ function shouldIncludeNode( GraphQLSkipDirective.args, variableValues, fragmentVariableValues, - hideSuggestions, + context.hideSuggestions, ) : undefined; if (skip?.if === true) { @@ -391,7 +386,7 @@ function shouldIncludeNode( GraphQLIncludeDirective.args, variableValues, fragmentVariableValues, - hideSuggestions, + context.hideSuggestions, ) : undefined; if (include?.if === false) {