GraphQLSchema does not completely validate names

[ab-pm]

The validateName function that is used to validate lots of schema parts is currently only checking that names are not reserved:

graphql-js/src/type/validate.ts

Lines 206 to 217 in 6b253e7

	function validateName(
	context: SchemaValidationContext,
	node: { readonly name: string; readonly astNode: Maybe<ASTNode> },
	): void {
	// Ensure names are valid, however introspection types opt out.
	if (node.name.startsWith('__')) {
	context.reportError(
	`Name "${node.name}" must not begin with "__", which is reserved by GraphQL introspection.`,
	node.astNode,
	);
	}
	}

It should also check that the name complies with https://spec.graphql.org/October2021/#Name, e.g. by testing against the regex /^(?!__)[A-Za-z_][A-Za-z0-9_]*$/.
Otherwise it's possible to construct schemas (via the constructor, not by parsing) that upon printing would lead to invalid syntax, or fields which could never be queried. (Not the case, see below)

[benjie]

Hi @ab-pm! Are you using the validateSchema() function to validate your schema has no naming issues or other problems?

[ab-pm]

I personally don't, I just thought it would be useful.
(What I'd need is print calling validateName on AST tokens, since I deal with potentially bogus/malicious AST inputs for query documents - but I understand that in print this might have a noticeable performance impact and it's not really the responsibility of a printer to do validation.)

I just looked through the source of graphql-js (as the GraphQL reference implementation) to see whether there is something useful to validate strings as Names, and found this function which (in the comment) claims to "Ensure names are valid" but doesn't really live up to that.
In a schema-first style of development this will never happen, but I could see some code-first (or even generated) schemas being constructed with invalid names, and an early error might be helpful for their authors. I admit this hasn't happened to me, so feel free to close this if you consider this a non-issue.

[yaacovCR]

Just looking this over, the strange naming of this function seems to be an artifact of refactoring, see #3288

We moved most of the validation of naming into the type constructors, so that except for the underscoring, conformity to the spec is guaranteed.

There is no context in that PR as to why it is preferable to fail at schema construction for name errors (rather than leaving that to the validation step with well formatted errors).

Totally guessing => perhaps @IvanGoncharov was motivated by a potential security concern with prototype pollution and so wanted to make sure that even non-validated schemas wouldn't have this issue?

Things to do:

1. clarify motivation of above PR
2. rename validateName function or add comment explaining hx

[ab-pm]

@ab-pm: I just looked through the source of graphql-js (as the GraphQL reference implementation) to see whether there is something useful to validate strings as Names, and found this function

@yaacovCR: We moved most of the validation of naming into the type constructors

Ah, that's great, I didn't see assertName and it's usage in (i.a.)

graphql-js/src/type/definition.ts

Line 870 in a7db60b

name: assertName(fieldName),

before, I guess that's what I had been looking for. And it's even exported!

So I guess you could close the issue as resolved regarding my doubt

it's possible to construct schemas (via the constructor, not by parsing) that upon printing would lead to invalid syntax, or fields which could never be queried.

however the todos you mention seem very sensible. I might add

3. update the documentation at https://www.graphql-js.org/api-v16/graphql/ and https://www.graphql-js.org/api-v16/type/ to account for

   graphql-js/src/type/index.ts

   Lines 36 to 53 in a7db60b

   	// Assertions
   	assertType,
   	assertScalarType,
   	assertObjectType,
   	assertInterfaceType,
   	assertUnionType,
   	assertEnumType,
   	assertInputObjectType,
   	assertListType,
   	assertNonNullType,
   	assertInputType,
   	assertOutputType,
   	assertLeafType,
   	assertCompositeType,
   	assertAbstractType,
   	assertWrappingType,
   	assertNullableType,
   	assertNamedType,

   and

   graphql-js/src/index.ts

   Lines 111 to 139 in a7db60b

   	// Assertions
   	assertSchema,
   	assertDirective,
   	assertType,
   	assertScalarType,
   	assertObjectType,
   	assertInterfaceType,
   	assertUnionType,
   	assertEnumType,
   	assertInputObjectType,
   	assertListType,
   	assertNonNullType,
   	assertInputType,
   	assertOutputType,
   	assertLeafType,
   	assertCompositeType,
   	assertAbstractType,
   	assertWrappingType,
   	assertNullableType,
   	assertNamedType,
   	// Un-modifiers
   	getNullableType,
   	getNamedType,
   	// Validate GraphQL schema.
   	validateSchema,
   	assertValidSchema,
   	// Upholds the spec rules about naming.
   	assertName,
   	assertEnumValueName,

[ab-pm]

to do: clarify motivation of above PR

There is no context in that PR as to why it is preferable to fail at schema construction for name errors (rather than leaving that to the validation step with well formatted errors).

I think this is good design to have constructors validate the arguments and not allow invalid instances to be constructed in the first place. The schema construction does not need to validate that again, it only needs to check the invariants across the whole schema.

As for "well formatted errors", if I construct types with invalid names in my code I'd rather have that fail via an exception with a stack trace than at some later step. And for reporting the error locations in a schema file, this doesn't really apply anyway since the parser would have already rejected these names. If anything, you could consider adding an optional astNode parameter to assertName (that would be added to any thrown GraphQLError) and pass the respective NameNode (from astNode?.name etc. in each constructor), but I doubt this is very useful.

Edit: interestingly, #3288 more or less undid #1132. I still think it's good - the latter was motivated by #1080 ("split up these steps to ensure we can do just the validation when we know the schema is valid.") but this is futile if the constructors are exported as the public interface. Non-validating constructors would need to be private (package-scoped).

[ab-pm]

Actually, there's also a problem with the deprecation notice by @IvanGoncharov in

graphql-js/src/utilities/assertValidName.ts

Lines 8 to 12 in a7db60b

	/**
	* Upholds the spec rules about naming.
	* @deprecated Please use `assertName` instead. Will be removed in v17
	*/
	export function assertValidName(name: string): string {

assertValidName does check for the name not to be reserved (start with __), while assertName does not. This is needed e.g. in

graphql-js/src/type/introspection.ts

Lines 207 to 208 in a7db60b

	export const __Type: GraphQLObjectType = new GraphQLObjectType({
	name: '__Type',

[yaacovCR]

At least one motivation for doing validation separately within validateSchema() is that if we know the entire schema is valid, including the names, we can skip the call to validateSchema(), and schema construction will be faster.

On the other hand, you mention the tension, that there is a benefit in fast-failing for common developer errors where the call to validateSchema() may happen at runtime (or potentially in some integration test step, but still late vis a vis the individual developer). And you suggest a boundary in terms of computational complexity: we should validate individual arguments to constructors for GraphQL Schema Elements in terms of correctness within constructors, whereas Schema Element interaction across the schema with its greater complexity should be deferred to validateSchema().

I see the tension, but find myself falling on the other side of it => I want schema construction at runtime to be as fast as possible. If it were up to me, I would vote for moving this validation back out of the constructor.