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: https://github.com/graphql/graphql-js/blob/6b253e7159a6a6c64c05bcc6bb863fef5a42eeb7/src/type/validate.ts#L206-L217 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 https://github.com/graphql/graphql-js/pull/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.) https://github.com/graphql/graphql-js/blob/a7db60beb22b0e417f9ed12215945c7fcb43246e/src/type/definition.ts#L870 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 https://github.com/graphql/graphql-js/blob/a7db60beb22b0e417f9ed12215945c7fcb43246e/src/type/index.ts#L36-L53 and https://github.com/graphql/graphql-js/blob/a7db60beb22b0e417f9ed12215945c7fcb43246e/src/index.ts#L111-L139

[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 https://github.com/graphql/graphql-js/blob/a7db60beb22b0e417f9ed12215945c7fcb43246e/src/utilities/assertValidName.ts#L8-L12

assertValidName does check for the name not to be reserved (start with __), while assertName does not. This is needed e.g. in https://github.com/graphql/graphql-js/blob/a7db60beb22b0e417f9ed12215945c7fcb43246e/src/type/introspection.ts#L207-L208

[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 https://github.com/microsoft/TypeScript/issues/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.

[yaacovCR]

Thanks for the reply!

I decided to (A) add a separate argument to the schema element constructors instead of (B) adding another property on the config objects because of exactly the issue you raise, that nested schema element creation (fields by object types, for example) would require copying the nested user provided field config and adding the flag, which seemed ugly and potentially slow.

But then you seem to suggest another solution? Or maybe not?

I want to ensure that there is a way to skip name validation for fields/arguments/everything when calling buildASTSchema or buildClientSchema with assumeValidNames set to true in the options, but it sounds like you are saying the user should have to set assumeValidNames explicitly on each config object? But buildASTSchema and buildClientSchema end up creating the config objects, so in that sense we are the user and have to choose (A) or (B) to pass down the flag?

Or maybe you are ok with (A) or (B) or both but were just adding some additional points?

Another unrelated wrinkle is that we should really move double underscore name validation to the constructors as well to be consistent but parsing allows the double underscore, so we cannot assume a properly parsed schema has valid names in totality.

[yaacovCR]

@ab-pm thanks again btw for bringing this issue up in the first place! I think after exploring some of the alternatives, I’m starting to get how we landed where we did!

[yaacovCR]

I tried another approach in:

• https://github.com/graphql/graphql-js/pull/4431

Building on the env work in:

• #4426

[yaacovCR]

We still haven’t worked out exactly how the default environment is set up. We wanted the default to be production, see further discussion in #4426

[ab-pm]

nested schema element creation (fields by object types, for example) would require copying the nested user provided field config and adding the flag, which seemed ugly and potentially slow

I'm saying (after learning more about how the library is implemented) that with (B) we shouldn't actually pass down the flag to nested creation, and rather require the flag to be independently set on every config object.

I want to ensure that there is a way to skip name validation for fields/arguments/everything when calling buildASTSchema or buildClientSchema with assumeValidNames set to true in the options

I agree with that. But since …

… buildASTSchema and buildClientSchema end up creating the config objects, so in that sense we are the user …

… and we can put the assumeValidName flag on each of them anyway, without any uglyness, and without requiring the nested constructors to pass anything down.

It should only be the buildASTSchema/buildClientSchema/extendSchemaImpl functions that pass their options.assumeValidNames (note the plural) into the config.assumeValidName (singular) option for each element. The constructors (GraphQLSchema, GraphQLObject, GraphQLField, GraphQLArgument) should not do any forwarding, they should only skip validation of their own name.

---

That said, I also very much like your new approach with the global functions. This looks very much optimal regarding the overhead of repeated flag condition checks. The no-op should inline very nicely if the environment never changes. Then again, I rather dislike mutable globals and making functions impure (having the behaviour of anything depend on some environment options set elsewhere). I'd rather keep the assumeValid flag for the schema building functions, and have them set the globals to no-op only for the duration of their run, then restore the values in a finally clause.

[yaacovCR]

… and we can put the assumeValidName flag on each of them anyway, without any uglyness, and without requiring the nested constructors to pass anything down.

Ah, I see that now, that is a good call and an option if we feel it's important to preserve the assumeValidNames on the config/object (rather than the current way #4427 is implemented where assumeValid is just passed as a separate argument.

That said, I also very much like your new approach with the global functions. This looks very much optimal regarding the overhead of repeated flag condition checks. The no-op should inline very nicely if the environment never changes.

I am liking that option a bit better now...

Then again, I rather dislike mutable globals and making functions impure (having the behaviour of anything depend on some environment options set elsewhere).

...But I guess it depends whether we go with the general approach in #4426 for instanceOf of having these mutable global additional checks.

I'd rather keep the assumeValid flag for the schema building functions, and have them set the globals to no-op only for the duration of their run, then restore the values in a finally clause.

I agree with you that mutable globals are less than desirable, but it sounds like this suggestion would preserve them, and have them interact with disparate object constructors. Taking the approach of #4431 and #4426 introduce mutable globals that are modified only by setEnv and function variables that hold no-op functions or additional checks, without the need for any resetting, finally blocks, etc, that is to me a safer (more organized?) way to "pen in" this mutation.

Overall #4431 needs some polish, I would like the assertions within the constructors in dev mode and the assertions in validation to use the exact same functions. But I am pausing work on it to see how #4426 pans out. @ab-pm => maybe you might want to comment there on the overall approach if you are interested?

[yaacovCR]

Just cross-referencing #4424 . It seems that within the programmatic API, we do not validate field names unless type.getFields() is called, because the fields are built lazily. #4424 would make enum values work the same way.