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.

[yaacovCR]

@ab-pm I sketched out in #4423 what it would look like to move name validation back into validateSchema.

This is the benchmark improvement on my machine when the schema is assumed valid, ~9%:

Note: when the schema is assumed valid and the only thing done is schema creation, most of the schema construction is deferred anyway secondary to the lazy construction => that's presumably why name validation of the named types is such a large percentage.

[ab-pm]

Hm, as indicated above, I fall on the other side of that tension, and don't really like the approach in that PR. Allowing the construction of invalid objects is an anti-pattern, it is like going against the best practice to make illegal states unrepresentable at the type level.

Also I think when constructing a schema in a code-first approach it's much more useful to get "syntax" validation errors with stack traces from the calls to the constructors, not in some later validateSchema() call. I see your point that skipping (or deferring) name validation in a schema-first approach can improve startup times, but shouldn't that be done explicitly with a config.assumeValid flag that defaults to false? Better err on the side of caution.

Another approach I could think of is to skip the name validation when a config.astNode is passed - this indicates that the name was parsed, and the schema document parser already throws an error on invalid name syntax. Of course that doesn't help when the schema is constructed from a config snapshot, not from a SDL file, but for that I think one really should use assumeValid: true.

[benjie]

Are you suggesting adding assumeValid to every constructor (objects, interfaces, unions, input objects, scalars, enums) so it can choose to bypass name validation?

[ab-pm]

Yes, that's what I meant. Sure it would still incur some overhead (the config property creation and the if condition evaluation) per entity, but in terms of DX this seems like the nicest approach - in cases where you don't construct the schema entity-by-entity, but load a pre-validated config or parse a schema document, it should be rather simple to pass assumeValidName(s): true into the constructor call of each class.

[yaacovCR]

i mocked up what that might look like in #4427 and it does indeed have the same savings as #4423.

so that's good!

But I think the naive approach in #4423 has a few problems:

1. It's a bit unclear to me what a schema with assumeValid: true containing types with assumeValid: false and vice a versa should mean. This could be somewhat fixed by having the object version be just assumeValidNames: true assumeValidNames: false and then even we would document that assumeValidNames takes priority over assumeValid.
2. extendSchema and lexicographicallySortSchema (via the underlying internal mapSchemaConfig function) recreate types; extendSchema carries with it the assumeValid option, and it passes that to let's say assumeValidName arguments, but what it should really do is pass assumeValidName: true if assumeValid: true or if the existing names have been already validated. lexicographicallySortSchema should never revalidated names, because it doesn't change anything. I have to think more about this, because if we end up eventualy exporting mapSchemaConfig and we save the fact that the names have been validated, we would then have to document very clearly that if names are changes (which would be a possible use case) you would have to reset assumeValidName or nameAlreadyValidated, or we would have to check ourselves. It gets complicated.

This seems like it opens up a lot more complexity than I would like.

Thinking again about the initial issue GraphQLSchema does not completely validate name I would just note that the constructors do not enforce and cannot enforce that the "__" prefix is not used for user types, as these are the same constructors as are used for introspection types. So some name validation is going to be deferred anyway. I'm not sure the actual cost of allowing invalid names; at some point, validateSchema should be called and the names corrected, and as far as I'm aware, there is actually nothing that breaks with an invalid name in our implementation, it's just not spec compliant. But I wouldn't say that this is the usual violation of "the best practice to make illegal states unrepresentable at the type level" in that I don't believe there is illegal state. Or, if it is, then the other validation errors that are deferred with all of the validation rules have much more illegal state that affect execution more. In any case, a __ type name will always be allowed through. One could say that those who know the spec won't make that mistake, but you could say that about the other name rules as well.

What would be best is if we could type the name arguments with something tighter than string in TypeScript, but that doesn't seem possible without regex types.

In terms of just skipping validation with ASTs, we need to support hand-rolled ASTs that could have incorrect names by TS constraints.

Just freeflowing some thoughts. Over all, with existing options, I prefer #4427, but I'm hopeful some additional option will open up.

[ab-pm]

(Not sure if I should comment here or on the PR, but let's keep the discussion where it started)

This seems like it opens up a lot more complexity than I would like.

Hm I did not expect that GraphQLSchemaValidationOptions.assumeValid would be carried over into the constructor calls. In a GraphQLSchemaConfig, the constructors already were (thunked to be) called independently, and those calls should each have their individual assumeValidName flag explicitly specified. But I was not familiar with how buildSchema, buildASTSchema and extendSchema were implemented all on top of the same abstraction, and how the assumeValid and assumeValidSDL flags work.

I would not attempt to pass the assumeValidNames flag down (from GraphQLObject into GraphQLField into GraphQLArgument, from GraphQLEnum into GraphQLEnumValue etc). Admittedly that was my first instinct as well, but it makes things more complex (and unnecessarily so). My expectation was that the assumeValidName would become not another (optional) parameter of the constructors, rather that it would become an optional property in the …Config objects. (Looking at the implementation, I quickly found that passing down the flag would have meant copying the nested config objects to add the outer flag value, really ugly and probably not very fast). Really I think one should explicitly specify it for every single name if one makes individual constructor calls. In the library, that would only be a few places though:

• extend the extendSchema Options with a assumeValidNames?: boolean property,
  in extendSchemaImpl add assumeValidName: options.assumeValid === true || options.assumeValidSDL === true || options.assumeValidNames === true in the respective config objects
• in buildASTSchema, extend the buildASTSchema options parameter type by assumeValidNames?: boolean as well,
  then in buildSchema pass assumeValidNames: true (which is guaranteed by the preceding parse)
  (I wouldn't have thought that "we need to support hand-rolled ASTs that could have incorrect names", I would have defaulted assumeValidNames to true when there's any AST, but ok)
• in buildClientSchema, add assumeValidName: options.assumeValid === true in the respective config objects

if names are changed in mapSchemaConfig (which would be a possible use case) you would have to reset [the check] or we would have to check ourselves. It gets complicated.

I think that's actually solved rather trivially:

mappedConfig.assumeValidName = mappedConfig.name === config.name || options.assumeValid === true;

there is actually nothing that breaks with an invalid name in our implementation

It fails the (trivially assumed, but admittedly not documented) invariant that parse(print(schema)) returns the same schema again. But yeah, this is not held for any other schema that violates the spec rules either, if parse does validate these rules.

What would be best is if we could type the name arguments with something tighter than string in TypeScript, but that doesn't seem possible without regex types.

Ah, yes, when one tries to build a template literal type for this you encounter microsoft/TypeScript#44792.
But tbh this would probably break a lot of libraries and applications that do not pass literals but string-typed variables into names; so I'm not sure this is a good idea. It would pass the responsibility to call validateName onto each user of graphql-js.