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

Validation errors can be used to get schema details #2247

Open
ravangen opened this issue Nov 5, 2019 · 9 comments
Open

Validation errors can be used to get schema details #2247

ravangen opened this issue Nov 5, 2019 · 9 comments

Comments

@ravangen
Copy link

ravangen commented Nov 5, 2019

Similar to introspection, another way to probe a server for details about its schema is to submit invalid GraphQL documents so that the default validation rules provide data back. A variety of rules use didYouMean.js to give the developer suggestions about possible changes to get a valid document. This is ideal when developing against an API, but also is a channel for leaking information (like prototype features) with validation rules like FieldsOnCorrectType.

In production, some companies with internal schemas elect to disable introspection, where internally they can acquire their schema via other channels. Seemingly security through obscurity.

The validation rules still need to run, but it would be ideal if we could toggle the ability to provide suggestions as part of the error message. The ValidationContext does provide onError handler, but it seems a bit reactive and wasteful to then remove the computed suggestionList. This also ties into some ideas outlined in #2074.

For example, Apollo Server provides a convenient constructor option introspection flag to determine if an introspection query should be allowed. It would be an improved developer experience if this flag could inform the underlying GraphQL.js validation rules to restrict its error messaging without any additional configuration required.

@glasser
Copy link
Contributor

glasser commented Apr 6, 2021

We've gotten more requests for this from users of Apollo Server. Would graphql-js be open to a PR that adds a flag to validate's options allowing you to disable the didYouMean suggestions? The alternatives would be to provide alternate versions of all of the specified rules that provide suggestions, or manually stripping didYouMean from returned errors, neither of which feels particularly maintainable.

@leebyron
Copy link
Contributor

leebyron commented Apr 6, 2021

Based on the OP intent, should there be a single flag to disable introspection that also disables didYouMean?

@ravangen
Copy link
Author

ravangen commented Apr 7, 2021

I favour a more granular mechanism to disable didYouMean functionality if desired, whether for the introspection angle, or if there is a performance angle for large schemas (I haven't done any profiling).

@glasser
Copy link
Contributor

glasser commented Apr 7, 2021

@leebyron I think that would satisfy the particular use cases we've heard recommended, though it might also be nice to have the more granular version (eg, for people using validation rules exported by graphql-js directly).

For what it's worth, Apollo Server implements "no introspection" by adding the following validation rule:

const NoIntrospection = (context: ValidationContext) => ({
  Field(node: FieldDefinitionNode) {
    if (node.name.value === '__schema' || node.name.value === '__type') {
      context.reportError(
        new GraphQLError(
          'GraphQL introspection is not allowed by Apollo Server, but the query contained __schema or __type. To enable introspection, pass introspection: true to ApolloServer in production',
          [node],
        ),
      );
    }
  },
});

Is your proposal a flag to validate that would add a rule like the above and change the behavior of didYouMean rules?

@leebyron
Copy link
Contributor

leebyron commented Apr 8, 2021

I don't really have a crisp proposal - just trying to clarify the need.

That seems like a reasonable way to implement introspection blocking, though I would worry about a theoretical misuse that creates a security risk where a query skips validation in some way and is still executed and exposes introspection. It might be nice if this was implemented within GraphQL.js that in addition to the clear error, it also simply wasn't possible to execute introspection queries on a schema with introspection disabled.

My concern with granular controls is that a user would disable introspection but not consider disabling didYouMean and inadvertently open this internal information leak. I would expect that a schema with introspection disabled would also disable didYouMean.

I can't think of a reason why you would want to disable introspection but enable didYouKnow or vice-versa.

@glasser
Copy link
Contributor

glasser commented Apr 8, 2021

Yes, I would expect that Apollo Server would take its introspection option and use it to control both of these features if they are separate. So for the AS use case it doesn't really matter too much if it's one feature or two... though if the features were separate we might consider still using our own NoIntrospection rule just to keep the behavior that the error message tells the user what option to pass to our API to control it?

Re your theoretical risk, I do feel that if disableIntrospection were an option to validate, then structurally it would be pretty clear that you need to run validate in order to disable introspection? So my inclination would be to add disableIntrospection as an option to validate and allow that to be enough.

If it were an execute option too then I guess the behavior of resolving an introspection field without validation would be to just leave the __schema field out of the response, just as it appears execution currently does for unknown fields without validation?

@sahanatroam
Copy link

sahanatroam commented Nov 6, 2021

I'm happy to help here and I do agree with @leebyron where if introspection is set to false didYouMean method should not return any suggestions.

Can the options object be used here to accept the introspection value from the consumer?

export function validate(
  schema: GraphQLSchema,
  documentAST: DocumentNode,
  rules: ReadonlyArray<ValidationRule> = specifiedRules,
  options?: { maxErrors?: number },

  /** @deprecated will be removed in 17.0.0 */
  typeInfo: TypeInfo = new TypeInfo(schema),
)

In addition to this we could also create a method getSpecifiedRules({ introspection: false }) so that consumers could pass in option so that errors returned would respect the options passed in.

Please let me know if this needs to go through an RFC process and gather feedback.

@pechenoha
Copy link

Hi guys, any updates?

@abhishek-parative
Copy link

Any updates in 2024?

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

No branches or pull requests

7 participants