Skip to content

fix(graphql): memoize fragment depth to bound depth-limit walk - #698

Draft
JiahuiWho wants to merge 2 commits into
mainfrom
fix/graphql-depth-limit-fragment-memoization
Draft

fix(graphql): memoize fragment depth to bound depth-limit walk#698
JiahuiWho wants to merge 2 commits into
mainfrom
fix/graphql-depth-limit-fragment-memoization

Conversation

@JiahuiWho

@JiahuiWho JiahuiWho commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

What

Cache each fragment's computed depth by name in selectionSetDepth so a fragment is expanded at most once per operation, instead of being re-expanded on every path that reaches it.

Why

selectionSetDepth re-expanded a fragment every time it was spread. Since a fragment spread adds no level of its own, that work is redundant, and when fragments fan out into one another (each spreading the next more than once), it compounds into work that grows exponentially with the fragment count, even for a small document with shallow actual depth. Since the reported depth stays low, neither the depth nor complexity limit rejects it.

Memoizing each fragment's depth by name bounds the walk to O(fragments + selections) and returns the identical depth value.

Known limitations

N/A

Issue that this PR addresses

https://hackerone.com/reports/3959812

Checklist

PR Structure

  • It is not possible to break this PR down into smaller PRs.
  • This PR does not mix refactoring changes with feature changes.
  • This PR's title starts with name of package that is most changed in the PR, or all if the changes are broad or impact many packages.

Thoroughness

  • This PR adds tests for the new functionality or fixes.
  • All updated queries have been tested (refer to this check if the data set returned by the updated query is expected to be same as the original one).

Release

  • This is not a breaking change.
  • This is ready to be tested in development.
  • The new functionality is gated with a feature flag if this is not ready for production.

@JiahuiWho
JiahuiWho force-pushed the fix/graphql-depth-limit-fragment-memoization branch from 5f74669 to 6b415aa Compare August 22, 2026 20:03
@JiahuiWho
JiahuiWho requested a balanced review from Copilot August 22, 2026 20:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Memoizes GraphQL fragment-depth calculations to mitigate fragment fan-out DoS attacks.

Changes:

  • Adds per-operation fragment-depth caching.
  • Adds regression coverage for repeated fragment fan-out.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/serve/graphql/depth_limit.go Implements memoized depth traversal.
internal/serve/graphql/depth_limit_test.go Tests depth correctness and fan-out termination.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/serve/graphql/depth_limit.go Outdated
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 this pull request may close these issues.

2 participants