bug: bound GraphQL query depth and complexity to prevent unauthenticated DoS - #1066
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @Bean | ||
| public Instrumentation maxQueryDepthInstrumentation( | ||
| @Value("${graphql.limit.max-query-depth:10}") int maxQueryDepth) { | ||
| return new MaxQueryDepthInstrumentation(maxQueryDepth); | ||
| } | ||
|
|
||
| @Bean | ||
| public Instrumentation maxQueryComplexityInstrumentation( | ||
| @Value("${graphql.limit.max-query-complexity:50000}") int maxQueryComplexity) { | ||
| return new MaxQueryComplexityInstrumentation(maxQueryComplexity, pageSizeAwareCalculator()); |
There was a problem hiding this comment.
📝 Info: Both limits reach production execution
DGS 4.9.21 collects every Instrumentation bean into one chain. Both independently tested limits therefore run on production requests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| static FieldComplexityCalculator pageSizeAwareCalculator() { | ||
| return (FieldComplexityEnvironment environment, int childComplexity) -> | ||
| 1 + pageSize(environment.getArguments()) * childComplexity; |
There was a problem hiding this comment.
📝 Info: Variable page sizes remain accounted
graphql-java supplies coerced variable values through FieldComplexityEnvironment. $first and $last therefore receive the same multiplier as inline values.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return (FieldComplexityEnvironment environment, int childComplexity) -> | ||
| 1 + pageSize(environment.getArguments()) * childComplexity; |
There was a problem hiding this comment.
🟥 Complexity overflow bypasses query limits
Large first or last values overflow pageSizeAwareCalculator below the limit. Anonymous requests can execute the expensive queries this protection targets.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
/graphqlispermitAlland the schema is cyclic (Article.comments->CommentEdge.node: Comment->Comment.article: Article!), while no graphql-java instrumentation was registered anywhere. A linear-sized anonymous query could therefore fan out exponentially — each nesting level ofcomments(first: 1000)triggers a paginated comment query plus onefindByIdper comment inArticleDatafetcher.getCommentArticle— exhausting the DB pool and heap.New
GraphQLQueryLimitConfigregisters twoInstrumentationbeans, which DGS's autoconfiguration folds into itsChainedInstrumentation, so the limits apply to every operation regardless of resolver:MaxQueryDepthInstrumentation(graphql.limit.max-query-depth, default 10)— a fullcomments -> edges -> node -> articlecycle costs 4 levels, so recursion terminates after ~2 cycles.MaxQueryComplexityInstrumentation(graphql.limit.max-query-complexity, default 50000, pageSizeAwareCalculator).The custom calculator is the important part: graphql-java's default counts 1 per field and is blind to the
first/lastfan-out, so afirst: 1000connection would cost the same asfirst: 1. Instead each field multiplies its children by its requested page size:So
articles(first: 1000) { ... comments(first: 1000) { ... } }costs ~1000 * 1000 * fields and is rejected, while realistic paging (articles(first: 20)withcomments(first: 20)~= 5.6k) stays well under the cap. Both limits are overridable viaapplication.properties.Not changed (out of scope for this finding):
/graphqlstays anonymous — the RealWorld spec requires unauthenticated reads — andCursorPageParameter.MAX_LIMITstays at 1000, since aggregate cost is now bounded by the complexity cap.Tests:
GraphQLQueryLimitConfigTestbuilds a minimal SDL reproducing the Article/Comment cycle and asserts the deep cyclic query and the wide 1000x1000 query are rejected, while a realistic 20x20 query passes both instrumentations.Verification caveat: this environment cannot resolve Gradle plugins/dependencies (Maven Central and plugins.gradle.org are off the session network allowlist and the Gradle cache is empty), so
compileJava/test/spotlessJavaCheckcould not be run locally; the repo also has no CI workflow. Formatting was matched to google-java-format by hand.Written by Devin
Devin-Org: engineering