Skip to content

[TT-17945] Fix stack overflow on cyclic fragment spreads - #448

Open
ermirizio wants to merge 2 commits into
masterfrom
TT-17945/fix-fragment-spread-cycle-stack-overflow
Open

[TT-17945] Fix stack overflow on cyclic fragment spreads#448
ermirizio wants to merge 2 commits into
masterfrom
TT-17945/fix-fragment-spread-cycle-stack-overflow

Conversation

@ermirizio

Copy link
Copy Markdown

Summary

  • Fixes an unrecoverable fatal error: stack overflow in astnormalization.(*nestedDepthCalc): calculateNestedDepth and depthForFragment were mutually recursive with no cycle detection, so a GraphQL document whose fragments spread each other in a cycle (e.g. fragment A { ...B } / fragment B { ...A }) recursed forever during Request.Normalize().
  • This is remotely triggerable by a single crafted request against any consumer (e.g. Tyk Gateway) that exposes GraphQL, and crashes the whole process — Go's stack-overflow fatal error cannot be caught by recover().
  • Fix tracks fragment names currently being resolved on the active call path in depthForFragment; if a name is re-entered while still being resolved, it reports operationreport.ErrFragmentSpreadFormsCycle instead of recursing. fragmentSpreadInlineVisitor.EnterDocument already stops the walk on report errors, so no other call site needed to change.
  • Applied the identical hardening to the v2 module (v2/pkg/astnormalization/fragmentspread_depth.go). FragmentSpreadDepth is currently dead code there (v2's fragment inliner doesn't call it, so this specific crash isn't reachable through v2 today) but it's exported, byte-for-byte identical to the vulnerable v1 code, and cheap to harden preemptively.

Jira: https://tyktech.atlassian.net/browse/TT-17945
Related: TykTechnologies/tyk#8567 (defense-in-depth reorder in the gateway itself)

Test plan

  • Added TestRealDepthCalculator_CyclicFragmentSpread to both pkg/astnormalization and v2/pkg/astnormalization, each building a two-fragment cycle and asserting FragmentSpreadDepth.Get returns promptly with a "forms fragment cycle" error rather than hanging/crashing (guarded by a 5s timeout so a regression fails the test instead of hanging CI).
  • Verified the new test reproduces the original crash: temporarily reverted the v1 fix and confirmed the test times out with the goroutine stuck in the old recursive calculateNestedDepth/depthForFragment call chain.
  • go test ./pkg/astnormalization/... (both modules) — pass
  • go test ./pkg/graphql/... — pass
  • go vet ./pkg/astnormalization/... (both modules) — clean

🤖 Generated with Claude Code

ermirizio and others added 2 commits August 11, 2026 15:01
nestedDepthCalc.calculateNestedDepth and depthForFragment in
astnormalization were mutually recursive with no cycle protection. A
GraphQL document whose fragments spread each other in a cycle (e.g.
"fragment A { ...B }" / "fragment B { ...A }") made this recurse
forever, crashing the whole process with an unrecoverable
"fatal error: stack overflow" - a remotely triggerable DoS against any
gateway exposing GraphQL.

Track fragment names currently being resolved on the active call
path; if depthForFragment is asked to resolve a name it's already in
the middle of resolving, report a normal fragment-cycle error instead
of recursing. fragmentSpreadInlineVisitor.EnterDocument already stops
the walk on report errors, so this is enough to turn the crash into a
handled validation-style error.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… recursion

Apply the same fix as the v1 module: FragmentSpreadDepth's depth
calculation is currently unused by v2's fragment inliner (dead code),
so this isn't reachable through the same crash path today, but it's
exported and identical to the vulnerable v1 code, so harden it too
before anything starts calling it again.
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
32.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

1 participant