Skip to content
45 changes: 41 additions & 4 deletions router-tests/automatic_persisted_queries_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,43 @@ func TestAutomaticPersistedQueries(t *testing.T) {
})
})

t.Run("SHA with non-matching query fails", func(t *testing.T) {
Comment thread
StarpTech marked this conversation as resolved.
t.Parallel()

testenv.Run(t, &testenv.Config{
RouterOptions: []core.Option{
// This ensures that no CDN client for persistent operations is created, so we can verify that
// APQ alone (without persistent operation support setup) works as expected.
core.WithGraphApiToken(""),
},
ApqConfig: config.AutomaticPersistedQueriesConfig{
Enabled: true,
Cache: config.AutomaticPersistedQueriesCacheConfig{
Size: 1024 * 1024,
},
},
}, func(t *testing.T, xEnv *testenv.Environment) {
header := make(http.Header)
header.Add("graphql-client-name", "my-client")

// Should not work
res1, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
Query: `{__typename}`,
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "85d996c3662d12de4f4abc17ba6f7aa696c1e760c7ed482a8ae64c49a7d68773"}}`),
Header: header,
})
require.NoError(t, err)
require.Equal(t, `{"errors":[{"message":"persistedQuery sha256 hash does not match query body"}]}`, res1.Body)

// Ensure the bad body is not added to APQ
res2 := xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "85d996c3662d12de4f4abc17ba6f7aa696c1e760c7ed482a8ae64c49a7d68773"}}`),
Header: header,
})
require.Equal(t, `{"errors":[{"message":"PersistedQueryNotFound","extensions":{"code":"PERSISTED_QUERY_NOT_FOUND"}}]}`, res2.Body)
})
})

t.Run("query is deleted after ttl expires", func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -484,15 +521,15 @@ query B ($id: Int!) {
res := xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{
OperationName: []byte(`"A"`),
Query: document,
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "6248b42bc35ecebe0d5e95cb1090e44e514b89edf483b592f90883b478b65b2e"}}`),
Header: header,
})
require.Equal(t, "MISS", res.Response.Header.Get(core.NormalizationCacheHeader))
require.Equal(t, `{"data":{"a":{"id":1,"details":{"pets":null}}}}`, res.Body)

res = xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{
OperationName: []byte(`"A"`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "6248b42bc35ecebe0d5e95cb1090e44e514b89edf483b592f90883b478b65b2e"}}`),
Header: header,
})
require.Equal(t, "HIT", res.Response.Header.Get(core.NormalizationCacheHeader))
Expand All @@ -504,7 +541,7 @@ query B ($id: Int!) {
OperationName: []byte(`"B"`),
Query: document,
Variables: []byte(`{"id": 3}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "6248b42bc35ecebe0d5e95cb1090e44e514b89edf483b592f90883b478b65b2e"}}`),
Header: header,
})
require.Equal(t, "MISS", res.Response.Header.Get(core.NormalizationCacheHeader))
Expand All @@ -513,7 +550,7 @@ query B ($id: Int!) {
res = xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{
OperationName: []byte(`"B"`),
Variables: []byte(`{"id": 3}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "6248b42bc35ecebe0d5e95cb1090e44e514b89edf483b592f90883b478b65b2e"}}`),
Header: header,
})
require.Equal(t, "HIT", res.Response.Header.Get(core.NormalizationCacheHeader))
Expand Down
6 changes: 3 additions & 3 deletions router-tests/persisted_operations_over_get_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func TestAutomatedPersistedQueriesOverGET(t *testing.T) {
}
}`,
Variables: []byte(`{"criteria": {"nationality": "GERMAN" }}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "f74d2ed949afd9f9567805c22e7f927745494cb1a469425cf65064283251cb42"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "e33580cf6276de9a75fb3b1c4b7580fec2a1c8facd13f3487bf6c7c3f854f7e3"}}`),
Header: header,
})
require.NoError(t, err)
Expand All @@ -182,7 +182,7 @@ func TestAutomatedPersistedQueriesOverGET(t *testing.T) {

res2, err2 := xEnv.MakeGraphQLRequestOverGET(testenv.GraphQLRequest{
Variables: []byte(`{"criteria": {"nationality": "GERMAN" }}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "f74d2ed949afd9f9567805c22e7f927745494cb1a469425cf65064283251cb42"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "e33580cf6276de9a75fb3b1c4b7580fec2a1c8facd13f3487bf6c7c3f854f7e3"}}`),
Header: header,
})
require.NoError(t, err2)
Expand Down Expand Up @@ -210,7 +210,7 @@ func TestAutomatedPersistedQueriesOverGET(t *testing.T) {
id
}
}`,
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "51804ed57938b6104d812ca352741ff69d7a8a30d67f240fba2b5a1e97793f9e"}}`),
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "49a2f7dd56b06f620c7d040dd9d562a1c16eadf7c149be5decdd62cfc92e1b12"}}`),
Header: header,
})
require.NoError(t, err)
Expand Down
6 changes: 5 additions & 1 deletion router/core/graph_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1371,7 +1371,11 @@ func (s *graphServer) buildGraphMux(
Enabled: s.securityConfiguration.BlockNonPersistedOperations.Enabled,
Condition: s.securityConfiguration.BlockNonPersistedOperations.Condition,
},
PersistedOperationsDisabled: s.persistedOperationsConfig.Disabled,
BlockPersisted: BlockPersistedOptions{
Enabled: s.securityConfiguration.BlockPersistedOperations.Enabled,
Condition: s.securityConfiguration.BlockPersistedOperations.Condition,
},
Comment on lines +1374 to +1377
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.

💡 Verification agent

🧩 Analysis chain

Wiring for conditional persisted-op blocking looks correct

Passing BlockPersisted from securityConfiguration.BlockPersistedOperations into the blocker aligns with the new config model.

Please verify:

  • The pre-handler invokes the operation blocker before any persisted-operation/APQ storage lookups so blocked requests are short-circuited and a nil PersistedOperationClient is safe.
  • OperationProcessor gracefully handles a nil PersistedOperationClient (no dereference).

You can run:


🏁 Script executed:

#!/usr/bin/env bash
# Ensure blocker is consulted in prehandler before PO/APQ paths
rg -n -C4 'OperationBlocker|ErrPersistedOperationBlocked|BlockPersisted' router/core/graphql_prehandler.go

# Inspect use-sites of PersistedOperationClient for nil-guards
rg -n -C4 'PersistedOperationClient' router/core

Length of output: 3972


🏁 Script executed:

#!/usr/bin/env bash
# Locate the handleOperation method in graphql_prehandler.go
rg -n -C5 'func \(.*\) handleOperation' router/core/graphql_prehandler.go

# If found, dump a larger context around its invocation of OperationBlocker and persisted operations
rg -n -C10 'handleOperation' -A200 router/core/graphql_prehandler.go | sed -n '1,200p'

Length of output: 9419


🏁 Script executed:

#!/usr/bin/env bash
# Locate OperationBlocker definition and its options
rg -n -C5 'type OperationBlocker' -g 'router/core/*'

# Show methods on OperationBlocker to see how blocking is enforced
rg -n -C3 'func \(.*OperationBlocker\)' -g 'router/core/*'

# Search for any call sites to OperationBlocker within PreHandler.handleOperation
rg -n -C5 'operationBlocker' router/core/graphql_prehandler.go

Length of output: 7178


🏁 Script executed:

#!/usr/bin/env bash
# Inspect usage of persistedOperationClient within OperationProcessor methods
rg -n -C5 'persistedOperationClient' router/core/operation_processor.go

Length of output: 3857


Move persisted-op blocking before storage lookup

  • In router/core/graphql_prehandler.go’s handleOperation, h.operationBlocker.OperationIsBlocked is invoked after FetchPersistedOperation (around line 704), so blocked persisted operations still hit the client/cache. Relocate the blocker call to immediately before the shouldFetchPersistedOperation check (around line 594) to short-circuit early.
  • OperationProcessor.FetchPersistedOperation already guards against a nil persistedOperationClient (it returns an httpGraphqlError instead of panic).
🤖 Prompt for AI Agents
In router/core/graph_server.go around lines 1374 to 1377 and specifically in
router/core/graphql_prehandler.go (handleOperation) move the call to
h.operationBlocker.OperationIsBlocked so it runs immediately before the
shouldFetchPersistedOperation check (around line ~594) instead of after
FetchPersistedOperation (~704); this short-circuits blocked persisted operations
before any client/cache storage lookup. Update the control flow to call
OperationIsBlocked with the same inputs and return its error/path as before if
blocked, leaving FetchPersistedOperation unchanged (it already handles a nil
persistedOperationClient).


Comment thread
endigma marked this conversation as resolved.
SafelistEnabled: s.persistedOperationsConfig.Safelist.Enabled,
LogUnknownOperationsEnabled: s.persistedOperationsConfig.LogUnknown,
exprManager: exprManager,
Expand Down
30 changes: 22 additions & 8 deletions router/core/graphql_prehandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -472,14 +472,25 @@ func (h *PreHandler) Handler(next http.Handler) http.Handler {
}

func (h *PreHandler) shouldComputeOperationSha256(operationKit *OperationKit) bool {
// If forced, always compute the hash
if h.computeOperationSha256 {
return true
}

hasPersistedHash := operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash()

// If it has a hash already AND a body, we need to compute the hash again to ensure it matches the persisted hash
if hasPersistedHash && operationKit.parsedOperation.Request.Query != "" {
return true
}

// If it already has a persisted hash attached to the request, then there is no need for us to compute it anew.
// Otherwise, we only want to compute the hash (an expensive operation) if we're safelisting or logging unknown persisted operations
return !hasPersistedHash && (h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled)
if !hasPersistedHash && (h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled) {
return true
}

return false
}
Comment thread
endigma marked this conversation as resolved.
Comment thread
endigma marked this conversation as resolved.

// shouldFetchPersistedOperation determines if we should fetch a persisted operation. The most intuitive case is if the
Expand Down Expand Up @@ -556,6 +567,16 @@ func (h *PreHandler) handleOperation(req *http.Request, variablesParser *astjson
}
}

// Ensure if request has both hash and query, that the hash matches the query
if operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() && operationKit.parsedOperation.Request.Query != "" {
if operationKit.parsedOperation.Sha256Hash != operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash {
return &httpGraphqlError{
message: "persistedQuery sha256 hash does not match query body",
statusCode: http.StatusBadRequest,
}
}
}

requestContext.operation.extensions = operationKit.parsedOperation.Request.Extensions
requestContext.operation.variables, err = variablesParser.ParseBytes(operationKit.parsedOperation.Request.Variables)
if err != nil {
Expand All @@ -571,13 +592,6 @@ func (h *PreHandler) handleOperation(req *http.Request, variablesParser *astjson
)

if h.shouldFetchPersistedOperation(operationKit) {
if h.operationBlocker.persistedOperationsDisabled {
return &httpGraphqlError{
message: "persisted operations are disabled",
statusCode: http.StatusBadRequest,
}
}

ctx, span := h.tracer.Start(req.Context(), "Load Persisted Operation",
trace.WithSpanKind(trace.SpanKindClient),
trace.WithAttributes(requestContext.telemetry.traceAttrs...),
Expand Down
40 changes: 35 additions & 5 deletions router/core/operation_blocker.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,19 @@ var (
ErrMutationOperationBlocked = errors.New("operation type 'mutation' is blocked")
ErrSubscriptionOperationBlocked = errors.New("operation type 'subscription' is blocked")
ErrNonPersistedOperationBlocked = errors.New("non-persisted operation is blocked")
ErrPersistedOperationBlocked = errors.New("persisted operation is blocked")
)

type OperationBlocker struct {
blockMutations BlockMutationOptions
blockSubscriptions BlockSubscriptionOptions
blockNonPersisted BlockNonPersistedOptions
blockPersisted BlockPersistedOptions
mutationExpr *vm.Program
subscriptionExpr *vm.Program
nonPersistedExpr *vm.Program
persistedExpr *vm.Program

persistedOperationsDisabled bool
safelistEnabled bool
logUnknownOperationsEnabled bool
}
Expand All @@ -44,6 +46,11 @@ type BlockNonPersistedOptions struct {
Condition string
}

type BlockPersistedOptions struct {
Enabled bool
Condition string
}

type SafelistPersistedOptions struct {
Enabled bool
}
Expand All @@ -52,8 +59,8 @@ type OperationBlockerOptions struct {
BlockMutations BlockMutationOptions
BlockSubscriptions BlockSubscriptionOptions
BlockNonPersisted BlockNonPersistedOptions
BlockPersisted BlockPersistedOptions
SafelistEnabled bool
PersistedOperationsDisabled bool
LogUnknownOperationsEnabled bool
exprManager *expr.Manager
}
Expand All @@ -63,8 +70,8 @@ func NewOperationBlocker(opts *OperationBlockerOptions) (*OperationBlocker, erro
blockMutations: opts.BlockMutations,
blockSubscriptions: opts.BlockSubscriptions,
blockNonPersisted: opts.BlockNonPersisted,
blockPersisted: opts.BlockPersisted,

persistedOperationsDisabled: opts.PersistedOperationsDisabled,
safelistEnabled: opts.SafelistEnabled,
logUnknownOperationsEnabled: opts.LogUnknownOperationsEnabled,
}
Expand Down Expand Up @@ -102,13 +109,19 @@ func (o *OperationBlocker) compileExpressions(exprManager *expr.Manager) error {
o.nonPersistedExpr = v
}

if o.blockPersisted.Enabled && o.blockPersisted.Condition != "" {
v, err := exprManager.CompileExpression(o.blockPersisted.Condition, reflect.Bool)
if err != nil {
return fmt.Errorf("failed to compile persisted expression: %w", err)
}
o.persistedExpr = v
}

return nil
}

func (o *OperationBlocker) OperationIsBlocked(requestLogger *zap.Logger, exprContext expr.Context, operation *ParsedOperation) error {

if !operation.IsPersistedOperation && o.blockNonPersisted.Enabled {

// Block all non-persisted operations when no expression is provided
if o.nonPersistedExpr == nil {
return ErrNonPersistedOperationBlocked
Expand All @@ -125,6 +138,23 @@ func (o *OperationBlocker) OperationIsBlocked(requestLogger *zap.Logger, exprCon
}
}

if operation.IsPersistedOperation && o.blockPersisted.Enabled {
// Block all persisted operations when no expression is provided
if o.persistedExpr == nil {
return ErrPersistedOperationBlocked
}

ok, err := expr.ResolveBoolExpression(o.persistedExpr, exprContext)
if err != nil {
requestLogger.Error("failed to resolve persisted block expression", zap.Error(err))
return ErrPersistedOperationBlocked
}

if ok {
return ErrPersistedOperationBlocked
}
}

switch operation.Type {
case "mutation":
if o.blockMutations.Enabled {
Expand Down
69 changes: 43 additions & 26 deletions router/core/operation_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ var (
type OperationProcessorOptions struct {
Executor *Executor
MaxOperationSizeInBytes int64
PersistedOperationClient persistedoperation.SaveClient
PersistedOperationClient *persistedoperation.Client
AutomaticPersistedOperationCacheTtl int

EnablePersistedOperationsCache bool
Expand All @@ -124,7 +124,7 @@ type OperationProcessorOptions struct {
type OperationProcessor struct {
executor *Executor
maxOperationSizeInBytes int64
persistedOperationClient persistedoperation.SaveClient
persistedOperationClient *persistedoperation.Client
operationCache *OperationCache
parseKits map[int]*parseKit
parseKitSemaphore chan int
Expand Down Expand Up @@ -426,7 +426,7 @@ func (o *OperationKit) FetchPersistedOperation(ctx context.Context, clientInfo *
}
}
if fromCache {
if isApq, _ := o.persistedOperationCacheKeyHasTtl(clientInfo.Name, includeOperationName); isApq {
if fromCacheHasTTL, _ := o.persistedOperationCacheKeyHasTtl(clientInfo.Name, includeOperationName); fromCacheHasTTL {
// if it is an APQ request, we need to save it again to renew the TTL expiration
if err = o.operationProcessor.persistedOperationClient.SaveOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash, o.parsedOperation.NormalizedRepresentation); err != nil {
return false, false, err
Expand All @@ -435,34 +435,51 @@ func (o *OperationKit) FetchPersistedOperation(ctx context.Context, clientInfo *
return true, false, nil
}

persistedOperationData, isApq, err := o.operationProcessor.persistedOperationClient.PersistedOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash)
if err != nil {
return false, isApq, err
} else if isApq && persistedOperationData == nil && o.parsedOperation.Request.Query == "" {
// If the client has APQ enabled, throw an error if the operation wasn't attached to the request
return false, isApq, &persistedoperation.PersistentOperationNotFoundError{
ClientName: clientInfo.Name,
Sha256Hash: o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash,
// If APQ is enabled and the query body is in the request, short-circuit
if o.parsedOperation.Request.Query != "" && o.operationProcessor.persistedOperationClient.APQEnabled() {
isAPQ = true

// If the operation was fetched with APQ, save it again to renew the TTL
err := o.operationProcessor.persistedOperationClient.SaveOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash, o.parsedOperation.Request.Query)
if err != nil {
return false, true, err
}
}
// it's important to make a copy of the persisted operation data, because it's used in the cache
// we might modify it later, so we don't want to modify the cached data
if persistedOperationData != nil {
o.parsedOperation.Request.Query = string(persistedOperationData)
// when we have successfully loaded the operation content from the storage,
// but it was passed via body instead of hash, we need to mark operation as persisted
// to populate persisted operation cache
o.parsedOperation.IsPersistedOperation = true
}
} else {
var persistedOperationData []byte
var err error

// If the operation was fetched with APQ, save it again to renew the TTL
if isApq {
if err = o.operationProcessor.persistedOperationClient.SaveOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash, o.parsedOperation.Request.Query); err != nil {
return false, isApq, err
persistedOperationData, isAPQ, err = o.operationProcessor.persistedOperationClient.PersistedOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash)
if err != nil {
return false, isAPQ, err
}

if isAPQ && persistedOperationData == nil && o.parsedOperation.Request.Query == "" {
// If the client has APQ enabled, throw an error if the operation wasn't attached to the request
return false, isAPQ, &persistedoperation.PersistentOperationNotFoundError{
ClientName: clientInfo.Name,
Sha256Hash: o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash,
}
}

// it's important to make a copy of the persisted operation data, because it's used in the cache
// we might modify it later, so we don't want to modify the cached data
if persistedOperationData != nil {
o.parsedOperation.Request.Query = string(persistedOperationData)
// when we have successfully loaded the operation content from the storage,
// but it was passed via body instead of hash, we need to mark operation as persisted
// to populate persisted operation cache
o.parsedOperation.IsPersistedOperation = true
}

// If the operation was fetched with APQ, save it again to renew the TTL
if isAPQ {
if err = o.operationProcessor.persistedOperationClient.SaveOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash, o.parsedOperation.Request.Query); err != nil {
return false, true, err
}
}
}

return false, isApq, nil
return false, isAPQ, nil
}

const (
Expand Down
3 changes: 2 additions & 1 deletion router/core/ratelimiter.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ import (
"encoding/json"
"errors"
"fmt"
rd "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis"
"io"
"reflect"
"sync"

rd "github.com/wundergraph/cosmo/router/internal/rediscloser"

"github.com/expr-lang/expr/vm"
"github.com/go-redis/redis_rate/v10"
"github.com/wundergraph/cosmo/router/internal/expr"
Expand Down
Loading
Loading