From cffd762ae579f21ba53d655272672a07610cc598 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 14 Aug 2026 00:00:22 +0100 Subject: [PATCH] fix(graphql): guard fragment spread resolution against cycles The parser does not reject fragment cycles. A 60-byte query with a self-referential fragment caused unbounded recursion and a stack overflow, which recover cannot catch. This was a remote DoS when BLOCK_SCHEMA_INTROSPECTION was enabled. Thread a visited set through checkSelections so each fragment is entered at most once per operation. Tests cover self cycles, mutual cycles, and a deep acyclic chain that must still detect introspection. --- graphql.go | 29 ++++++++++++----- graphql_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 9 deletions(-) diff --git a/graphql.go b/graphql.go index a1fa964..a527279 100644 --- a/graphql.go +++ b/graphql.go @@ -379,7 +379,7 @@ func parseGraphQLQuery(c fiber.Ctx) *parseGraphQLQueryResult { processDirectives(oper, res) // Check for introspection queries if they're blocked - if cfg.Security.BlockIntrospection && checkSelections(c, oper.GetSelectionSet().Selections, fragments) { + if cfg.Security.BlockIntrospection && checkSelections(c, oper.GetSelectionSet().Selections, fragments, make(map[string]bool)) { _ = c.Status(403).SendString("Introspection queries are not allowed") res.shouldBlock = true return res @@ -430,8 +430,17 @@ func processDirectives(oper *ast.OperationDefinition, res *parseGraphQLQueryResu // checkSelections recursively checks if any selection is an introspection query that should be blocked. // fragments indexes top-level named FragmentDefinition nodes so that a FragmentSpread (which only -// carries a name) can be resolved to its actual selection set. -func checkSelections(c fiber.Ctx, selections []ast.Selection, fragments map[string]*ast.FragmentDefinition) bool { +// carries a name) can be resolved to its actual selection set. visited tracks fragment names already +// entered anywhere in this top-level call's traversal so a fragment that spreads itself (directly or via +// a cycle, e.g. A -> B -> A) cannot recurse forever; graphql-go's parser does not reject fragment cycles, +// only its (unused) validator does. Skipping an already-visited fragment is safe in both situations the +// visited map covers. For a true cycle, the fragment is already being checked higher up the call stack, +// so re-entering it cannot discover anything new. For a diamond (DAG) reuse - the same fragment spread +// from two separate, non-nested branches, with no cycle involved - the fragment was already fully +// checked earlier in this traversal; since visited only ever grows (it is never cleared within a call), +// a later traversal of that same fragment could only re-walk a subset of what the earlier, complete +// traversal already covered, so skipping it loses nothing. +func checkSelections(c fiber.Ctx, selections []ast.Selection, fragments map[string]*ast.FragmentDefinition, visited map[string]bool) bool { if len(selections) == 0 { return false } @@ -463,7 +472,7 @@ func checkSelections(c fiber.Ctx, selections []ast.Selection, fragments map[stri // Check nested selections if present if sel.SelectionSet != nil && len(sel.GetSelectionSet().Selections) > 0 { - if checkSelections(c, sel.GetSelectionSet().Selections, fragments) { + if checkSelections(c, sel.GetSelectionSet().Selections, fragments, visited) { return true } } @@ -471,7 +480,7 @@ func checkSelections(c fiber.Ctx, selections []ast.Selection, fragments map[stri case *ast.InlineFragment: // Check nested selections in fragments if sel.SelectionSet != nil && len(sel.GetSelectionSet().Selections) > 0 { - if checkSelections(c, sel.GetSelectionSet().Selections, fragments) { + if checkSelections(c, sel.GetSelectionSet().Selections, fragments, visited) { return true } } @@ -480,9 +489,13 @@ func checkSelections(c fiber.Ctx, selections []ast.Selection, fragments map[stri // A named fragment spread carries no selection set itself; resolve it // to its top-level FragmentDefinition so introspection inside a named // fragment (e.g. `...F` where F selects __schema) cannot bypass the block. - if sel.Name != nil { + // Skip a name already visited to break self- and mutually-recursive + // fragment cycles (e.g. `fragment A on Query { ...A }`), which the + // parser does not reject. + if sel.Name != nil && !visited[sel.Name.Value] { if frag, ok := fragments[sel.Name.Value]; ok && frag.SelectionSet != nil { - if checkSelections(c, frag.SelectionSet.Selections, fragments) { + visited[sel.Name.Value] = true + if checkSelections(c, frag.SelectionSet.Selections, fragments, visited) { return true } } @@ -536,7 +549,7 @@ func checkIfContainsIntrospection(c fiber.Ctx, query string) bool { for _, def := range p.Definitions { if op, ok := def.(*ast.OperationDefinition); ok { if op.SelectionSet != nil { - blocked = checkSelections(c, op.GetSelectionSet().Selections, fragments) + blocked = checkSelections(c, op.GetSelectionSet().Selections, fragments, make(map[string]bool)) break } } diff --git a/graphql_test.go b/graphql_test.go index 5bb25f1..006597e 100644 --- a/graphql_test.go +++ b/graphql_test.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" "testing" + "time" "github.com/goccy/go-json" fiber "github.com/gofiber/fiber/v3" @@ -526,6 +527,87 @@ func (suite *Tests) Test_DeepIntrospectionQueries() { } } +// Test_FragmentCycleDoesNotHang guards against a stack-overflow DoS: graphql-go's +// parser.Parse does not reject fragment spread cycles (cycle detection lives only +// in the unused validator), so a self-referential or mutually-recursive named +// fragment must be caught by checkSelections' visited set instead of recursing +// until the process aborts. +func (suite *Tests) Test_FragmentCycleDoesNotHang() { + tests := []struct { + name string + query string + }{ + { + name: "self-referential fragment", + query: "query Q { ...A } fragment A on Query { ...A }", + }, + { + name: "mutually recursive fragments A -> B -> A", + query: "query Q { ...A } fragment A on Query { ...B } fragment B on Query { ...A }", + }, + } + + for _, tt := range tests { + suite.Run(tt.name, func() { + cfg.Security.BlockIntrospection = true + cfg.Security.IntrospectionAllowed = nil + introspectionAllowedQueries = make(map[string]struct{}) + + body := map[string]any{"query": tt.query} + bodyBytes, _ := json.Marshal(body) + ctx := fiber.New().AcquireCtx(&fasthttp.RequestCtx{}) + ctx.Request().SetBody(bodyBytes) + + done := make(chan struct{}) + go func() { + parseGraphQLQuery(ctx) + close(done) + }() + + select { + case <-done: + // Neither cycle ever reaches an introspection field, so the + // request must complete and must not be blocked. + suite.Equal(200, ctx.Response().StatusCode()) + case <-time.After(2 * time.Second): + suite.FailNow("checkSelections did not return promptly for a cyclic fragment spread") + } + }) + } +} + +// Test_DeepAcyclicFragmentChainDetectsIntrospection verifies the visited-set fix +// for fragment cycles does not break detection of introspection buried at the +// end of a long, legitimate (acyclic) chain of distinct named fragments. +func (suite *Tests) Test_DeepAcyclicFragmentChainDetectsIntrospection() { + const chainLength = 50 + + var b strings.Builder + b.WriteString("query Q { ...F0 }") + for i := 0; i < chainLength; i++ { + fmt.Fprintf(&b, " fragment F%d on Query { ", i) + if i == chainLength-1 { + b.WriteString("__schema { queryType { name } }") + } else { + fmt.Fprintf(&b, "...F%d", i+1) + } + b.WriteString(" }") + } + + cfg.Security.BlockIntrospection = true + cfg.Security.IntrospectionAllowed = nil + introspectionAllowedQueries = make(map[string]struct{}) + + body := map[string]any{"query": b.String()} + bodyBytes, _ := json.Marshal(body) + ctx := fiber.New().AcquireCtx(&fasthttp.RequestCtx{}) + ctx.Request().SetBody(bodyBytes) + + parseGraphQLQuery(ctx) + + suite.Equal(403, ctx.Response().StatusCode(), "introspection at the end of a 50-fragment acyclic chain must still be detected") +} + func TestIntrospectionQueryHandling(t *testing.T) { tests := []struct { name string @@ -624,7 +706,7 @@ func TestIntrospectionQueryHandling(t *testing.T) { } for _, def := range p.Definitions { if op, ok := def.(*ast.OperationDefinition); ok { - blocked = checkSelections(ctx, op.GetSelectionSet().Selections, fragments) + blocked = checkSelections(ctx, op.GetSelectionSet().Selections, fragments, make(map[string]bool)) break } }