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

Rejecting costly or malicious queries #291

Closed
herojan opened this Issue Jan 3, 2017 · 9 comments

Comments

Projects
None yet
7 participants
@herojan
Copy link
Contributor

herojan commented Jan 3, 2017

Is there currently a way to determine how costly a query will be to run? I have been looking at the code and I think there isn't, correct me and close this issue if I'm wrong!

It would be important to add some protections against this kind of thing. When thinking of ways to do this I found that I settled on a way that sangria is already doing. i.e. assign a complexity score to each field and add up the scores, rejecting the query if it's too complex.

With this in mind I'd propose the following:

  1. Add an optional cost/complexity function to the GraphQLFieldDefinition and its inner Builder class. The interface would be something like:
public interface ComplexityFunction {
   Double calculate(ValidationContext validationContext, List<GraphQLArgument> args, Double childScore);
}

with java 8 usage like

newFieldDefinition()
.name("field")
.dataFetcher(env -> "result")
.complexity((validationContext, fieldDef, childScore) -> 2.0 * getArgument("size", args) * childScore)
  1. Add a way to specify the complexity limit on queries to run. This will likely take the form of adding a List of validations as an argument to the graphql.execute method. It would be nice to be able to use the RulesVisitor and another validation for this but I'm not sure it can be done that way yet.

What do you think? I'm willing to implement this feature myself, so I'd like to know if this is wanted and if this is a good approach.

@dminkovsky

This comment has been minimized.

Copy link
Contributor

dminkovsky commented Jan 5, 2017

Thank you for reporting @herojan.

@timtebeek mentions this might also be related https://github.com/oembedler/spring-graphql-common#protection-against-malicious-queries

@herojan

This comment has been minimized.

Copy link
Contributor

herojan commented Jan 5, 2017

@dminkovsky Ah, I wasn't looking for a solution to a problem I was having, I already have support for this in my own project. I was making this issue to discuss whether or not this feature should be added to graphql-java. Actually adding it isn't hard.

I had seen that Spring project before but I didn't know it does this. It looks like they based their implementation on Sangria's too, since the documentation structure and headings are the same. The expression language is quite nice.

The idea was to add this to GraphQLFieldDefinition and its Builder class, in the same way as the dataFetcher is added. I modified the example above for it to be a bit clearer.

I do believe it would make sense here because it is a simple pre-execution analysis similar to the validation rules, and because no graphql interface could ever be exposed without supporting this kind of thing.

@bbakerman

This comment has been minimized.

Copy link
Member

bbakerman commented Jan 13, 2017

I had a go with this PR #299 to add constraints. It only does depth for the moment but I think it can be extended to be the place for "field complexity"

@Macroz

This comment has been minimized.

Copy link
Contributor

Macroz commented Jan 17, 2017

See also issue #75

@andimarek andimarek added this to the 4.0.0 milestone May 20, 2017

@andimarek

This comment has been minimized.

Copy link
Member

andimarek commented May 20, 2017

I would like to see some kind of solution for malicious queries in the next version: the question what to do about it comes up all the time and I think graphql-java needs to have some kind of answer to that.

@kaqqao

This comment has been minimized.

Copy link
Contributor

kaqqao commented May 28, 2017

I'm almost done implementing static complexity analysis in graphql-spqr, similar to what Sangria and Absinthe have. Implemented an AST walker, that basically does what FieldCollector does, but for the entire tree (not just one level) and accumulates the scores for the encountererd nodes, taking the worst outcome in case of conditional fragments.
Both the function retrieveing the complexity expressions and the expression evaluator and replacable.

Whatever ends up in graphql-java itself, I'd argue it's of utmost importance to make it extensible/customizable, so that different behaviors can be plugged in easily. Then projects and libraries can provide concrete implementations and/or extensions.

@ieugen

This comment has been minimized.

Copy link

ieugen commented May 29, 2017

FYI, Github v4 api is GraphQL based and it's interesting to see how they enforce limits [1].

They have a node limit and a rate limit:

Clients must supply a first or last argument on any connection.
Values of first and last must be within 1-100.
Individual calls cannot request more than 11,250 total nodes.

[1] https://developer.github.com/v4/guides/resource-limitations/

@kaqqao

This comment has been minimized.

Copy link
Contributor

kaqqao commented Jun 16, 2017

Just in case someone cares, my implementation is ready. It's in this package, the main class being ComplexityAnalyzer. It is completely implemented using the existing API (instrumentations being the critical part).

Functionally, it replicates Sangria's approach, It uses JavaScript expressions for the dynamic formulas (so no dependency on any expression language), but this is easily customizable, so other languages (SpEL, OGNL, JUEL etc), calculation methods or sources of info (think of a DB with live performance analytics) are pluggable.

@andimarek

This comment has been minimized.

Copy link
Member

andimarek commented Aug 29, 2017

this is now solved with #655: based on query depth and/or query complexity you can now prevent the execution of certain queries.

@andimarek andimarek closed this Aug 29, 2017

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