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

Limiting the query depth, blocks `IntrospectionQuery` too making graphiQL inconsistent #1055

Closed
themanojshukla opened this issue May 20, 2018 · 6 comments

Comments

@themanojshukla
Copy link

@themanojshukla themanojshukla commented May 20, 2018

Hi,

I tried to limit the graphql query depth to 5, it blocks graphiQL's IntrospectionQuery from execution which makes it inconsistent. Meaning, we can't see the Schema, Documentation and Types in graphiQL.

After doing some debugging, I found that IntrospectionQuery is having depth size as 13. So, exceeding the depth comparison.

As a tool, graphiQL must be allowed to do introspection, as it helps to explore the existing endpoints/queries, Type Checking, Autocomplete, etc. But, the User Queries must be checked for depth limit to avoid malicious request over server.

I couldn't find a way to handle this, but today got some workaround by overriding
the MaxQueryDepthInstrumentation but not sure if this is the elegant solution.

Please add a feature to easy configuration to set the depth limit and allow IntrospectionQuery.
This brings another feature request to give a handler to allow WhiteListed queries defined by the user.

Thanks & Regards,
Manoj

@bbakerman

This comment has been minimized.

Copy link
Member

@bbakerman bbakerman commented May 22, 2018

The tricky part here is that Introspection is an open ended query. It can be just `__typename`` right up to the default complicated Introspection query.

query IntrospectionQuery {
    __schema {
      queryType { name }
      mutationType { name }
      subscriptionType { name }
      types {
        ...FullType
      }
      directives {
        name
        description
        locations
        args {
          ...InputValue
        }
      }
    }
  }

  fragment FullType on __Type {
    kind
    name
    description
    fields(includeDeprecated: true) {
      name
      description
      args {
        ...InputValue
      }
      type {
        ...TypeRef
      }
      isDeprecated
      deprecationReason
    }
    inputFields {
      ...InputValue
    }
    interfaces {
      ...TypeRef
    }
    enumValues(includeDeprecated: true) {
      name
      description
      isDeprecated
      deprecationReason
    }
    possibleTypes {
      ...TypeRef
    }
  }

  fragment InputValue on __InputValue {
    name
    description
    type { ...TypeRef }
    defaultValue
  }

  fragment TypeRef on __Type {
    kind
    name
    ofType {
      kind
      name
      ofType {
        kind
        name
        ofType {
          kind
          name
        }
      }
    }
  }

So how would the library make a sensible decision on whether its an introspection query or not? The presence of __xxx`` say? This would allow people to whack __typename` in their expensive queries to subvert your depth checking.

This is a tricky problem and at first glance I cant think of an easy way to know its Introspection of not.

Hmmmm...

@themanojshukla

This comment has been minimized.

Copy link
Author

@themanojshukla themanojshukla commented May 22, 2018

But, we could have something to White-List those queries which are exclusively specified by the users (could be list of allowed queries).

Can't we have some regex to identify the valid IntrospectionQuery format.?

@bbakerman

This comment has been minimized.

Copy link
Member

@bbakerman bbakerman commented May 22, 2018

Yes it could however there is NO one canonical Introspection query. A client is free to make them up.

So it would need to be a hueristic to "guess" if the max depth should be allowed or not.

@themanojshukla

This comment has been minimized.

Copy link
Author

@themanojshukla themanojshukla commented Jun 3, 2018

I got a workaround for this, but I want your guidance over my approach.

I've tokenized the introspection query, and created a simple array of token of type String.
Here is what I found as possible && unique tokens..

String[] validTokens = {
 "", "query", "IntrospectionQuery", "schema", "queryType", "name",  "mutationType", "subscriptionType",
 "types", "FullType", "directives", "description", "locations", "args", "InputValue", "fragment", "on", "Type",
 "kind", "fields", "includeDeprecated", "true", "type", "TypeRef", "isDeprecated", "deprecationReason",
 "inputFields", "interfaces", "enumValues", "possibleTypes", "defaultValue", "ofType"
};

Now, I will tokenize each request (only unique tokens, no duplicates) and now can match if the current query contains only IntrospectionTokens or not.

If it contains only IntrospectionTokens I would by pass it from the depth checker, but if any token in current query is out of this allowed strings, I would make it pass through the depth checker as normal process.

Now, in future, if anything new comes into the valid IntrospectionQuery, I would just add that new token to the list of validTokens. And all would be as fine as it were.

I've not seen any kind of prevention in any implementation of GraphQL specs. Following/modifying this approach we can make it available as @PublicApi or configurable to Back-end Developers.

I tried this on my local project, It works perfectly fine. Need your thoughts on this.!!

@bbakerman

This comment has been minimized.

Copy link
Member

@bbakerman bbakerman commented Jun 24, 2018

Right so basically you have written a heuristic that checks if a query looks Introspection like.

This is likely to work but it CANT be guaranteed to work for all queries because the introspection queries are NOT fixed. Its is however a pretty good heuristic.

In short I think this works for you but I struggle to immediately see how we can make it generic for all users.

That said it makes sense that one would want to not depth check valid introspection queries yet do it on the other queries.

I am in a bit of a bind on how to solve this challenge

@themanojshukla

This comment has been minimized.

Copy link
Author

@themanojshukla themanojshukla commented Jul 7, 2018

Can we configure/modify/change the default introspection query written in GraphiQL tool? If yes, we can have a mechanism to avoid the above problem.

As far as I'm aware, the very first introspection query is made by THAT GraphiQL interface WHICH IS connected to our GraphQL server (what we add as dependency in our server project), so the Introspection Query Stored in GraphiQL is always hit whenever somebody opens /graphiql endpoint.

If anyhow, we can configure THAT stored query, then we can easily allow those Introspections which we created/configured and have agreed that it (fully or partially) is sufficient to explore whole (or required) schema with documentation. Doing so, we can bypass the depth checking for our CUSTOM Introspection and all other queries will have to pass through the depth checker (if depth limit is enabled).

@bbakerman bbakerman closed this Sep 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.