Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parsing SDL allows true, false, and null for enum value names #3221

Closed
Yogu opened this issue Aug 3, 2021 · 3 comments · Fixed by #3223
Closed

Parsing SDL allows true, false, and null for enum value names #3221

Yogu opened this issue Aug 3, 2021 · 3 comments · Fixed by #3223

Comments

@Yogu
Copy link
Contributor

Yogu commented Aug 3, 2021

There is a schema validator that forbids true, false, and null as enum value names sinde https://github.com/graphql/graphql-js/pull/812/files. The parse function together with the specifiedSDLRules rules however allow these strings to be used as enum value names/values (they're the same there). This means you can parse and successfully validate an SDL which generates a validation error when creating the schema.

I'm not quite sure whether this is a bug in the parser or a missing SDL validator. parseEnumValueDefinition uses parseName and states that the EnumValue non-terminal is equivalent to the name non-terminal:

  /**
   * EnumValueDefinition : Description? EnumValue Directives[Const]?
   *
   * EnumValue : Name
   */
  ;

  _proto.parseEnumValueDefinition = function parseEnumValueDefinition() {
    var start = this._lexer.token;
    var description = this.parseDescription();
    var name = this.parseName();
    var directives = this.parseDirectives(true);
    return {
      kind: Kind.ENUM_VALUE_DEFINITION,
      description: description,
      name: name,
      directives: directives,
      loc: this.loc(start)
    };
  }

There also is an EnumValue non-terminal which is defined as follows:

EnumValue : Name but not true, false or null

So an enum declaration either should switch from EnumName to EnumValue, or there should be an SDL validator that rejects the three forbidden words when used as enum value names/values.

@renatobenks
Copy link

renatobenks commented Aug 3, 2021

what a catch, sounds like a missing thing, interesting 🤔

I had a problem related a couple years ago, using schema-stitching from graphql-tools, which he was re-creating the entire schema, and the enums became syntactically problematic, probably because of this.

@Yogu
Copy link
Contributor Author

Yogu commented Aug 4, 2021

I looked into the specification. It's actually a bug in the parser. Section 3.9 defines the EnumValueDefinition as follows:

EnumValueDefinition :
Description[opt] EnumValue Directives[Const][opt]

And section 2.9.6 defines EnumValue:

EnumValue :
Name but not true or false or null

I'll create a PR for this fix.

Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
@IvanGoncharov
Copy link
Member

@Yogu Thanks for reporting and especially for working on a fix 👍
Closing and we will track progress in #3223

Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Yogu added a commit to Yogu/graphql-js that referenced this issue Aug 4, 2021
The specification disallows these in the SDL grammar definition, and using one of these enum values would result in a schema that would throw validation errors on creation.

Fixes graphql#3221.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants