From 8c8ce5228fcdb2def377a9cd80273888916faea8 Mon Sep 17 00:00:00 2001 From: endigma Date: Mon, 1 Sep 2025 16:22:57 +0100 Subject: [PATCH 01/10] fix(router): validate that user provided hashes match query Resolves ENG-8050 --- router/core/graphql_prehandler.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/router/core/graphql_prehandler.go b/router/core/graphql_prehandler.go index 3c9ca3f976..38a989b5c9 100644 --- a/router/core/graphql_prehandler.go +++ b/router/core/graphql_prehandler.go @@ -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 } // shouldFetchPersistedOperation determines if we should fetch a persisted operation. The most intuitive case is if the @@ -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 { From b3f8d02ac5bbcc9e802b84a4c3295d8b6192eac5 Mon Sep 17 00:00:00 2001 From: endigma Date: Mon, 1 Sep 2025 16:48:12 +0100 Subject: [PATCH 02/10] feat(router): refactor PO blocking, allow disabling PO provider --- router/core/graph_server.go | 6 +- router/core/graphql_prehandler.go | 7 -- router/core/operation_blocker.go | 40 +++++- router/core/router.go | 116 +++++++++--------- router/pkg/config/config.go | 1 + router/pkg/config/config.schema.json | 19 ++- .../pkg/config/testdata/config_defaults.json | 4 + router/pkg/config/testdata/config_full.json | 4 + 8 files changed, 125 insertions(+), 72 deletions(-) diff --git a/router/core/graph_server.go b/router/core/graph_server.go index 875b0969d5..95f66c0ac5 100644 --- a/router/core/graph_server.go +++ b/router/core/graph_server.go @@ -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, + }, + SafelistEnabled: s.persistedOperationsConfig.Safelist.Enabled, LogUnknownOperationsEnabled: s.persistedOperationsConfig.LogUnknown, exprManager: exprManager, diff --git a/router/core/graphql_prehandler.go b/router/core/graphql_prehandler.go index 38a989b5c9..8c934d7244 100644 --- a/router/core/graphql_prehandler.go +++ b/router/core/graphql_prehandler.go @@ -592,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...), diff --git a/router/core/operation_blocker.go b/router/core/operation_blocker.go index 1a5ac26fc3..6bc9900c10 100644 --- a/router/core/operation_blocker.go +++ b/router/core/operation_blocker.go @@ -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 } @@ -44,6 +46,11 @@ type BlockNonPersistedOptions struct { Condition string } +type BlockPersistedOptions struct { + Enabled bool + Condition string +} + type SafelistPersistedOptions struct { Enabled bool } @@ -52,8 +59,8 @@ type OperationBlockerOptions struct { BlockMutations BlockMutationOptions BlockSubscriptions BlockSubscriptionOptions BlockNonPersisted BlockNonPersistedOptions + BlockPersisted BlockPersistedOptions SafelistEnabled bool - PersistedOperationsDisabled bool LogUnknownOperationsEnabled bool exprManager *expr.Manager } @@ -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, } @@ -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 @@ -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 { diff --git a/router/core/router.go b/router/core/router.go index f5bed12df7..11ef708bac 100644 --- a/router/core/router.go +++ b/router/core/router.go @@ -980,69 +980,71 @@ func (r *Router) buildClients() error { var pClient persistedoperation.Client - if provider, ok := cdnProviders[r.persistedOperationsConfig.Storage.ProviderID]; ok { - if r.graphApiToken == "" { - return errors.New("graph token is required to fetch persisted operations from CDN") - } + if !r.persistedOperationsConfig.Disabled { + if provider, ok := cdnProviders[r.persistedOperationsConfig.Storage.ProviderID]; ok { + if r.graphApiToken == "" { + return errors.New("graph token is required to fetch persisted operations from CDN") + } - c, err := cdn.NewClient(provider.URL, r.graphApiToken, cdn.Options{ - Logger: r.logger, - }) - if err != nil { - return err - } - pClient = c + c, err := cdn.NewClient(provider.URL, r.graphApiToken, cdn.Options{ + Logger: r.logger, + }) + if err != nil { + return err + } + pClient = c - r.logger.Info("Use CDN as storage provider for persisted operations", - zap.String("provider_id", provider.ID), - ) - } else if provider, ok := s3Providers[r.persistedOperationsConfig.Storage.ProviderID]; ok { - - c, err := s3.NewClient(provider.Endpoint, &s3.Options{ - AccessKeyID: provider.AccessKey, - SecretAccessKey: provider.SecretKey, - Region: provider.Region, - UseSSL: provider.Secure, - BucketName: provider.Bucket, - ObjectPathPrefix: r.persistedOperationsConfig.Storage.ObjectPrefix, - TraceProvider: r.tracerProvider, - }) - if err != nil { - return err - } - pClient = c + r.logger.Info("Use CDN as storage provider for persisted operations", + zap.String("provider_id", provider.ID), + ) + } else if provider, ok := s3Providers[r.persistedOperationsConfig.Storage.ProviderID]; ok { + + c, err := s3.NewClient(provider.Endpoint, &s3.Options{ + AccessKeyID: provider.AccessKey, + SecretAccessKey: provider.SecretKey, + Region: provider.Region, + UseSSL: provider.Secure, + BucketName: provider.Bucket, + ObjectPathPrefix: r.persistedOperationsConfig.Storage.ObjectPrefix, + TraceProvider: r.tracerProvider, + }) + if err != nil { + return err + } + pClient = c - r.logger.Info("Use S3 as storage provider for persisted operations", - zap.String("provider_id", provider.ID), - ) - } else if provider, ok := fileSystemProviders[r.persistedOperationsConfig.Storage.ProviderID]; ok { - c, err := fs.NewClient(provider.Path, &fs.Options{ - ObjectPathPrefix: r.persistedOperationsConfig.Storage.ObjectPrefix, - }) - if err != nil { - return err - } - pClient = c + r.logger.Info("Use S3 as storage provider for persisted operations", + zap.String("provider_id", provider.ID), + ) + } else if provider, ok := fileSystemProviders[r.persistedOperationsConfig.Storage.ProviderID]; ok { + c, err := fs.NewClient(provider.Path, &fs.Options{ + ObjectPathPrefix: r.persistedOperationsConfig.Storage.ObjectPrefix, + }) + if err != nil { + return err + } + pClient = c - r.logger.Info("Use file system as storage provider for persisted operations", - zap.String("provider_id", provider.ID), - ) - } else if r.graphApiToken != "" { - if r.persistedOperationsConfig.Storage.ProviderID != "" { - return fmt.Errorf("unknown storage provider id '%s' for persisted operations", r.persistedOperationsConfig.Storage.ProviderID) - } + r.logger.Info("Use file system as storage provider for persisted operations", + zap.String("provider_id", provider.ID), + ) + } else if r.graphApiToken != "" { + if r.persistedOperationsConfig.Storage.ProviderID != "" { + return fmt.Errorf("unknown storage provider id '%s' for persisted operations", r.persistedOperationsConfig.Storage.ProviderID) + } - c, err := cdn.NewClient(r.cdnConfig.URL, r.graphApiToken, cdn.Options{ - Logger: r.logger, - }) - if err != nil { - return err - } - pClient = c + c, err := cdn.NewClient(r.cdnConfig.URL, r.graphApiToken, cdn.Options{ + Logger: r.logger, + }) + if err != nil { + return err + } + pClient = c - r.logger.Debug("Default to Cosmo CDN as persisted operations provider", - zap.String("url", r.cdnConfig.URL), - ) + r.logger.Debug("Default to Cosmo CDN as persisted operations provider", + zap.String("url", r.cdnConfig.URL), + ) + } } var kvClient apq.KVClient diff --git a/router/pkg/config/config.go b/router/pkg/config/config.go index beeac69969..a24eadac4d 100644 --- a/router/pkg/config/config.go +++ b/router/pkg/config/config.go @@ -407,6 +407,7 @@ type SecurityConfiguration struct { BlockMutations BlockOperationConfiguration `yaml:"block_mutations" envPrefix:"SECURITY_BLOCK_MUTATIONS_"` BlockSubscriptions BlockOperationConfiguration `yaml:"block_subscriptions" envPrefix:"SECURITY_BLOCK_SUBSCRIPTIONS_"` BlockNonPersistedOperations BlockOperationConfiguration `yaml:"block_non_persisted_operations" envPrefix:"SECURITY_BLOCK_NON_PERSISTED_OPERATIONS_"` + BlockPersistedOperations BlockOperationConfiguration `yaml:"block_persisted_operations" envPrefix:"SECURITY_BLOCK_PERSISTED_OPERATIONS_"` ComplexityCalculationCache *ComplexityCalculationCache `yaml:"complexity_calculation_cache"` ComplexityLimits *ComplexityLimits `yaml:"complexity_limits"` DepthLimit *QueryDepthConfiguration `yaml:"depth_limit"` diff --git a/router/pkg/config/config.schema.json b/router/pkg/config/config.schema.json index 9b58d9ac88..69c5768e55 100644 --- a/router/pkg/config/config.schema.json +++ b/router/pkg/config/config.schema.json @@ -145,7 +145,7 @@ "properties": { "disabled": { "type": "boolean", - "description": "Disables persisted operations. If disabled, all operations sent with a persisted operation in the body are blocked.", + "description": "Disable persisted operations.", "default": false }, "safelist": { @@ -2383,7 +2383,7 @@ "properties": { "enabled": { "type": "boolean", - "description": "Block non-persisted operations (sent with operation ID). If the value is true, all Operations are blocked. You can also specify a condition that is evaluated to determine if the non-persisted operation should be blocked." + "description": "Block non-persisted operations (sent without operation hash). You can also specify a condition that is evaluated to determine if the non-persisted operation should be blocked." }, "condition": { "type": "string", @@ -2391,6 +2391,21 @@ } } }, + "block_persisted_operations": { + "type": "object", + "description": "The configuration for blocking persisted operations.", + "additionalProperties": false, + "properties": { + "enabled": { + "type": "boolean", + "description": "Block persisted operations (sent with operation hash). You can also specify a condition that is evaluated to determine if the persisted operation should be blocked." + }, + "condition": { + "type": "string", + "description": "The expression to evaluate if the persisted operation should be blocked. The expression is specified as a string and needs to evaluate to a boolean. Please see https://expr-lang.org/ for more information." + } + } + }, "complexity_calculation_cache": { "type": "object", "description": "The configuration for the complexity calculation cache. The complexity calculation cache is used to cache the complexity calculation for the queries.", diff --git a/router/pkg/config/testdata/config_defaults.json b/router/pkg/config/testdata/config_defaults.json index 5b8ba74b26..7010c3ae54 100644 --- a/router/pkg/config/testdata/config_defaults.json +++ b/router/pkg/config/testdata/config_defaults.json @@ -318,6 +318,10 @@ "Enabled": false, "Condition": "" }, + "BlockPersistedOperations": { + "Enabled": false, + "Condition": "" + }, "ComplexityCalculationCache": null, "ComplexityLimits": null, "DepthLimit": null, diff --git a/router/pkg/config/testdata/config_full.json b/router/pkg/config/testdata/config_full.json index 1f70e0987a..83fe1d4f3f 100644 --- a/router/pkg/config/testdata/config_full.json +++ b/router/pkg/config/testdata/config_full.json @@ -655,6 +655,10 @@ "Enabled": false, "Condition": "" }, + "BlockPersistedOperations": { + "Enabled": false, + "Condition": "" + }, "ComplexityCalculationCache": { "Enabled": true, "CacheSize": 1024 From 6e8fc0fb6599a7f174e96e0014b0dfdf7d7928b3 Mon Sep 17 00:00:00 2001 From: endigma Date: Mon, 1 Sep 2025 16:48:34 +0100 Subject: [PATCH 03/10] tests: update APQ tests to have correct SHAs how did this even happen --- .../automatic_persisted_queries_test.go | 45 +++++++++++++++++-- .../persisted_operations_over_get_test.go | 6 +-- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/router-tests/automatic_persisted_queries_test.go b/router-tests/automatic_persisted_queries_test.go index 9ba4ebc5b3..fa417096e3 100644 --- a/router-tests/automatic_persisted_queries_test.go +++ b/router-tests/automatic_persisted_queries_test.go @@ -88,6 +88,43 @@ func TestAutomaticPersistedQueries(t *testing.T) { }) }) + t.Run("SHA with non-matching query fails", func(t *testing.T) { + 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() @@ -484,7 +521,7 @@ 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)) @@ -492,7 +529,7 @@ query B ($id: Int!) { 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)) @@ -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)) @@ -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)) diff --git a/router-tests/persisted_operations_over_get_test.go b/router-tests/persisted_operations_over_get_test.go index 9451ee5748..fc9b00075c 100644 --- a/router-tests/persisted_operations_over_get_test.go +++ b/router-tests/persisted_operations_over_get_test.go @@ -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) @@ -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) @@ -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) From b2b2a65dc8d879ade7175575caa47d5c462732fc Mon Sep 17 00:00:00 2001 From: endigma Date: Mon, 1 Sep 2025 17:23:36 +0100 Subject: [PATCH 04/10] chore: add warning to startup for potentially unintended configurations --- router/core/router.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/router/core/router.go b/router/core/router.go index 11ef708bac..7183177b88 100644 --- a/router/core/router.go +++ b/router/core/router.go @@ -469,6 +469,29 @@ func NewRouter(opts ...Option) (*Router, error) { return nil, errors.New("automatic persisted queries and safelist cannot be enabled at the same time (as APQ would permit queries that are not in the safelist)") } + if r.securityConfiguration.BlockPersistedOperations.Enabled && + r.securityConfiguration.BlockNonPersistedOperations.Enabled { + + // Both have no condition, unusable state + if r.securityConfiguration.BlockPersistedOperations.Condition == "" && + r.securityConfiguration.BlockNonPersistedOperations.Condition == "" { + return nil, errors.New("persisted and non-persisted operations are both unconditionally blocked") + } + + // One or both have a condition, could be intentional for edge cases + r.logger.Warn("The security configuration fields 'block_persisted_operations' and 'block_non_persisted_operations' are both enabled. Take care to ensure this is intentional.") + } + + if r.persistedOperationsConfig.Safelist.Enabled && r.securityConfiguration.BlockPersistedOperations.Enabled { + // Both have no condition, unusable state + if r.securityConfiguration.BlockPersistedOperations.Condition == "" { + return nil, errors.New("safelist cannot be enabled while persisted operations are unconditionally blocked") + } + + // Has a condition, could be intentional for edge cases + r.logger.Warn("The security configuration field 'block_persisted_operations' is enabled alongside the persisted operations safelist. Take care to ensure this is intentional. Misconfiguration will result in safelisted queries being blocked.") + } + if r.securityConfiguration.DepthLimit != nil { r.logger.Warn("The security configuration field 'depth_limit' is deprecated, and will be removed. Use 'security.complexity_limits.depth' instead.") From bc0fe784467ac14cd545d22ebe0b0e49ba02c6b0 Mon Sep 17 00:00:00 2001 From: endigma Date: Mon, 1 Sep 2025 21:14:18 +0100 Subject: [PATCH 05/10] short circuit when query is in request --- router/core/operation_processor.go | 55 +++++++++++++++++------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/router/core/operation_processor.go b/router/core/operation_processor.go index 0033b9de72..c862cc1b44 100644 --- a/router/core/operation_processor.go +++ b/router/core/operation_processor.go @@ -435,34 +435,43 @@ 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, + // Short-circuit if the query is present in the request + if o.parsedOperation.Request.Query != "" { + // 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 + } + } else { + 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, + } + } + // 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 } - } - // 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, isApq, err + // 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 ( From f0f54daf9b2e495c59bb13eb329f04a38a53a247 Mon Sep 17 00:00:00 2001 From: endigma Date: Tue, 2 Sep 2025 13:19:01 +0100 Subject: [PATCH 06/10] fix: bug with safelist, improve code structure --- router/core/operation_processor.go | 10 +++--- router/core/ratelimiter.go | 3 +- router/core/router.go | 4 +-- router/core/router_config.go | 4 +-- router/debug.config.yaml | 10 +++++- .../internal/persistedoperation/apq/redis.go | 5 +-- router/internal/persistedoperation/client.go | 31 +++++++++---------- .../operationstorage/cdn/client.go | 17 +++++----- .../operationstorage/fs/client.go | 20 ++++++------ .../operationstorage/s3/client.go | 11 ++++--- .../redis => rdcloser}/rdcloser.go | 7 +++-- .../redis => rdcloser}/rdcloser_test.go | 5 +-- router/pkg/pubsub/redis/adapter.go | 2 +- 13 files changed, 72 insertions(+), 57 deletions(-) rename router/internal/{persistedoperation/operationstorage/redis => rdcloser}/rdcloser.go (99%) rename router/internal/{persistedoperation/operationstorage/redis => rdcloser}/rdcloser_test.go (98%) diff --git a/router/core/operation_processor.go b/router/core/operation_processor.go index c862cc1b44..63037924a1 100644 --- a/router/core/operation_processor.go +++ b/router/core/operation_processor.go @@ -100,7 +100,7 @@ var ( type OperationProcessorOptions struct { Executor *Executor MaxOperationSizeInBytes int64 - PersistedOperationClient persistedoperation.SaveClient + PersistedOperationClient *persistedoperation.Client AutomaticPersistedOperationCacheTtl int EnablePersistedOperationsCache bool @@ -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 @@ -435,12 +435,12 @@ func (o *OperationKit) FetchPersistedOperation(ctx context.Context, clientInfo * return true, false, nil } - // Short-circuit if the query is present in the request - if o.parsedOperation.Request.Query != "" { + // If APQ is enabled and the query body is in the request, short-circuit + if o.parsedOperation.Request.Query != "" && o.operationProcessor.persistedOperationClient.APQEnabled() { // 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 + return false, false, err } } else { persistedOperationData, isAPQ, err := o.operationProcessor.persistedOperationClient.PersistedOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash) diff --git a/router/core/ratelimiter.go b/router/core/ratelimiter.go index 0e47f30b60..a494a7de5a 100644 --- a/router/core/ratelimiter.go +++ b/router/core/ratelimiter.go @@ -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/rdcloser" + "github.com/expr-lang/expr/vm" "github.com/go-redis/redis_rate/v10" "github.com/wundergraph/cosmo/router/internal/expr" diff --git a/router/core/router.go b/router/core/router.go index 7183177b88..623eee0f90 100644 --- a/router/core/router.go +++ b/router/core/router.go @@ -36,8 +36,8 @@ import ( "github.com/wundergraph/cosmo/router/internal/persistedoperation/apq" "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/cdn" "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/fs" - rd "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis" "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/s3" + rd "github.com/wundergraph/cosmo/router/internal/rdcloser" "github.com/wundergraph/cosmo/router/internal/retrytransport" "github.com/wundergraph/cosmo/router/internal/stringsx" "github.com/wundergraph/cosmo/router/pkg/config" @@ -1001,7 +1001,7 @@ func (r *Router) buildClients() error { fileSystemProviders[provider.ID] = provider } - var pClient persistedoperation.Client + var pClient persistedoperation.StorageClient if !r.persistedOperationsConfig.Disabled { if provider, ok := cdnProviders[r.persistedOperationsConfig.Storage.ProviderID]; ok { diff --git a/router/core/router_config.go b/router/core/router_config.go index 2a2a78924d..e3061ebb5a 100644 --- a/router/core/router_config.go +++ b/router/core/router_config.go @@ -8,7 +8,7 @@ import ( nodev1 "github.com/wundergraph/cosmo/router/gen/proto/wg/cosmo/node/v1" "github.com/wundergraph/cosmo/router/internal/graphqlmetrics" "github.com/wundergraph/cosmo/router/internal/persistedoperation" - rd "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis" + rd "github.com/wundergraph/cosmo/router/internal/rdcloser" "github.com/wundergraph/cosmo/router/internal/retrytransport" "github.com/wundergraph/cosmo/router/pkg/config" "github.com/wundergraph/cosmo/router/pkg/controlplane/configpoller" @@ -59,7 +59,7 @@ type Config struct { cacheControlPolicy config.CacheControlPolicy routerConfigPollerConfig *RouterConfigPollerConfig cdnConfig config.CDNConfiguration - persistedOperationClient persistedoperation.SaveClient + persistedOperationClient *persistedoperation.Client persistedOperationsConfig config.PersistedOperationsConfig automaticPersistedQueriesConfig config.AutomaticPersistedQueriesConfig apolloCompatibilityFlags config.ApolloCompatibilityFlags diff --git a/router/debug.config.yaml b/router/debug.config.yaml index 6aa036f9b0..339f903812 100644 --- a/router/debug.config.yaml +++ b/router/debug.config.yaml @@ -8,10 +8,18 @@ version: '1' execution_config: file: path: './__schemas/config.json' - watch: true + # watch: true log_level: debug +# automatic_persisted_queries: +# enabled: true + +persisted_operations: + disabled: false + safelist: + enabled: true + watch_config: enabled: true interval: '10s' diff --git a/router/internal/persistedoperation/apq/redis.go b/router/internal/persistedoperation/apq/redis.go index 8ca2b67735..5f6e0a391a 100644 --- a/router/internal/persistedoperation/apq/redis.go +++ b/router/internal/persistedoperation/apq/redis.go @@ -3,11 +3,12 @@ package apq import ( "context" "errors" + "time" + "github.com/redis/go-redis/v9" - "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis" + rd "github.com/wundergraph/cosmo/router/internal/rdcloser" "github.com/wundergraph/cosmo/router/pkg/config" "go.uber.org/zap" - "time" ) type RedisOptions struct { diff --git a/router/internal/persistedoperation/client.go b/router/internal/persistedoperation/client.go index 457ab7b34c..2028f67941 100644 --- a/router/internal/persistedoperation/client.go +++ b/router/internal/persistedoperation/client.go @@ -24,33 +24,28 @@ func (e PersistentOperationNotFoundError) Error() string { return fmt.Sprintf("operation '%s' for client '%s' not found", e.Sha256Hash, e.ClientName) } -type Client interface { - PersistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, bool, error) +type StorageClient interface { + PersistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, error) Close() } -type SaveClient interface { - Client - SaveOperation(ctx context.Context, clientName, sha256Hash, operationBody string) error -} - type Options struct { // CacheSize indicates the in-memory cache size, in bytes. If 0, no in-memory // cache is used. CacheSize uint64 Logger *zap.Logger - ProviderClient Client + ProviderClient StorageClient ApqClient apq.Client } -type client struct { +type Client struct { cache *operationstorage.OperationsCache - providerClient Client + providerClient StorageClient apqClient apq.Client } -func NewClient(opts *Options) (SaveClient, error) { +func NewClient(opts *Options) (*Client, error) { cacheSize := int64(opts.CacheSize) cache, err := operationstorage.NewOperationsCache(cacheSize) @@ -58,14 +53,14 @@ func NewClient(opts *Options) (SaveClient, error) { return nil, errors.Join(err, fmt.Errorf("initializing CDN cache")) } - return &client{ + return &Client{ providerClient: opts.ProviderClient, cache: cache, apqClient: opts.ApqClient, }, nil } -func (c *client) PersistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, bool, error) { +func (c *Client) PersistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, bool, error) { if c.apqClient != nil && c.apqClient.Enabled() { resp, apqErr := c.apqClient.PersistedOperation(ctx, clientName, sha256Hash) if len(resp) > 0 || apqErr != nil { @@ -86,7 +81,7 @@ func (c *client) PersistedOperation(ctx context.Context, clientName string, sha2 poNotFound *PersistentOperationNotFoundError ) - content, _, err := c.providerClient.PersistedOperation(ctx, clientName, sha256Hash) + content, err := c.providerClient.PersistedOperation(ctx, clientName, sha256Hash) if errors.As(err, &poNotFound) && c.apqClient != nil { // This could well be the first time a client is requesting an APQ operation and the query is attached to the request. Return without error here, and we'll verify the operation later. return content, true, nil @@ -100,7 +95,7 @@ func (c *client) PersistedOperation(ctx context.Context, clientName string, sha2 return content, false, nil } -func (c *client) SaveOperation(ctx context.Context, clientName, sha256Hash, operationBody string) error { +func (c *Client) SaveOperation(ctx context.Context, clientName, sha256Hash, operationBody string) error { if c.apqClient != nil && c.apqClient.Enabled() { return c.apqClient.SaveOperation(ctx, clientName, sha256Hash, []byte(operationBody)) } @@ -108,7 +103,11 @@ func (c *client) SaveOperation(ctx context.Context, clientName, sha256Hash, oper return nil } -func (c *client) Close() { +func (c *Client) APQEnabled() bool { + return c.apqClient != nil && c.apqClient.Enabled() +} + +func (c *Client) Close() { if c.providerClient != nil { c.providerClient.Close() } diff --git a/router/internal/persistedoperation/operationstorage/cdn/client.go b/router/internal/persistedoperation/operationstorage/cdn/client.go index fe1648e516..3867f22dcd 100644 --- a/router/internal/persistedoperation/operationstorage/cdn/client.go +++ b/router/internal/persistedoperation/operationstorage/cdn/client.go @@ -6,6 +6,10 @@ import ( "encoding/json" "errors" "fmt" + "io" + "net/http" + "net/url" + "github.com/wundergraph/cosmo/router/internal/httpclient" "github.com/wundergraph/cosmo/router/internal/jwt" "github.com/wundergraph/cosmo/router/internal/persistedoperation" @@ -14,15 +18,14 @@ import ( semconv "go.opentelemetry.io/otel/semconv/v1.17.0" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" - "io" - "net/http" - "net/url" ) type Options struct { Logger *zap.Logger } +var _ persistedoperation.StorageClient = (*client)(nil) + type client struct { cdnURL *url.URL authenticationToken string @@ -36,13 +39,13 @@ type client struct { logger *zap.Logger } -func (cdn *client) PersistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, bool, error) { +func (cdn *client) PersistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, error) { content, err := cdn.persistedOperation(ctx, clientName, sha256Hash) if err != nil { - return nil, false, err + return nil, err } - return content, false, nil + return content, nil } func (cdn *client) persistedOperation(ctx context.Context, clientName string, sha256Hash string) ([]byte, error) { @@ -124,7 +127,7 @@ func (cdn *client) persistedOperation(ctx context.Context, clientName string, sh // NewClient creates a new CDN client. URL is the URL of the CDN. // Token is the token used to authenticate with the CDN, the same as the GRAPH_API_TOKEN -func NewClient(endpoint string, token string, opts Options) (persistedoperation.Client, error) { +func NewClient(endpoint string, token string, opts Options) (*client, error) { u, err := url.Parse(endpoint) if err != nil { return nil, fmt.Errorf("invalid CDN URL %q: %w", endpoint, err) diff --git a/router/internal/persistedoperation/operationstorage/fs/client.go b/router/internal/persistedoperation/operationstorage/fs/client.go index 6f44a9c36a..6e7181d721 100644 --- a/router/internal/persistedoperation/operationstorage/fs/client.go +++ b/router/internal/persistedoperation/operationstorage/fs/client.go @@ -10,25 +10,25 @@ import ( "github.com/wundergraph/cosmo/router/internal/persistedoperation" ) -type Option func(*Client) - -type Client struct { +type client struct { path string options *Options } +var _ persistedoperation.StorageClient = (*client)(nil) + type Options struct { ObjectPathPrefix string } // NewClient creates a new FileStorage client that can be used to retrieve persisted operations -func NewClient(path string, options *Options) (persistedoperation.Client, error) { +func NewClient(path string, options *Options) (*client, error) { absolutePath, err := filepath.Abs(path) if err != nil { return nil, fmt.Errorf("failed to get absolute storage path: %w", err) } - client := &Client{ + client := &client{ path: absolutePath, options: options, } @@ -36,16 +36,16 @@ func NewClient(path string, options *Options) (persistedoperation.Client, error) return client, nil } -func (c Client) PersistedOperation(ctx context.Context, clientName, sha256Hash string) ([]byte, bool, error) { +func (c client) PersistedOperation(ctx context.Context, clientName, sha256Hash string) ([]byte, error) { content, err := c.persistedOperation(clientName, sha256Hash) if err != nil { - return nil, false, err + return nil, err } - return content, false, nil + return content, nil } -func (c Client) persistedOperation(clientName string, sha256Hash string) ([]byte, error) { +func (c client) persistedOperation(clientName string, sha256Hash string) ([]byte, error) { operationName := fmt.Sprintf("%s.json", sha256Hash) objectPath := filepath.Join(c.path, c.options.ObjectPathPrefix, operationName) @@ -69,4 +69,4 @@ func (c Client) persistedOperation(clientName string, sha256Hash string) ([]byte return []byte(po.Body), nil } -func (c Client) Close() {} +func (c client) Close() {} diff --git a/router/internal/persistedoperation/operationstorage/s3/client.go b/router/internal/persistedoperation/operationstorage/s3/client.go index 58fd5d9742..321819d32e 100644 --- a/router/internal/persistedoperation/operationstorage/s3/client.go +++ b/router/internal/persistedoperation/operationstorage/s3/client.go @@ -32,9 +32,10 @@ type Options struct { TraceProvider *sdktrace.TracerProvider } -// NewClient creates a new S3 client that can be used to retrieve persisted operations -func NewClient(endpoint string, options *Options) (persistedoperation.Client, error) { +var _ persistedoperation.StorageClient = (*Client)(nil) +// NewClient creates a new S3 client that can be used to retrieve persisted operations +func NewClient(endpoint string, options *Options) (*Client, error) { client := &Client{ options: options, tracer: options.TraceProvider.Tracer( @@ -74,13 +75,13 @@ func NewClient(endpoint string, options *Options) (persistedoperation.Client, er return client, nil } -func (c Client) PersistedOperation(ctx context.Context, clientName, sha256Hash string) ([]byte, bool, error) { +func (c Client) PersistedOperation(ctx context.Context, clientName, sha256Hash string) ([]byte, error) { content, err := c.persistedOperation(ctx, clientName, sha256Hash) if err != nil { - return nil, false, err + return nil, err } - return content, false, nil + return content, nil } func (c Client) persistedOperation(ctx context.Context, clientName, sha256Hash string) ([]byte, error) { diff --git a/router/internal/persistedoperation/operationstorage/redis/rdcloser.go b/router/internal/rdcloser/rdcloser.go similarity index 99% rename from router/internal/persistedoperation/operationstorage/redis/rdcloser.go rename to router/internal/rdcloser/rdcloser.go index 46fba0f7c0..7bd370152e 100644 --- a/router/internal/persistedoperation/operationstorage/redis/rdcloser.go +++ b/router/internal/rdcloser/rdcloser.go @@ -1,12 +1,13 @@ -package rd +package rdcloser import ( "context" "fmt" - "github.com/redis/go-redis/v9" - "go.uber.org/zap" "net/url" "strings" + + "github.com/redis/go-redis/v9" + "go.uber.org/zap" ) // RDCloser is an interface that combines the redis.Cmdable and io.Closer interfaces, ensuring that we can close the diff --git a/router/internal/persistedoperation/operationstorage/redis/rdcloser_test.go b/router/internal/rdcloser/rdcloser_test.go similarity index 98% rename from router/internal/persistedoperation/operationstorage/redis/rdcloser_test.go rename to router/internal/rdcloser/rdcloser_test.go index 6bd4cbd807..5db69c2bfa 100644 --- a/router/internal/persistedoperation/operationstorage/redis/rdcloser_test.go +++ b/router/internal/rdcloser/rdcloser_test.go @@ -1,11 +1,12 @@ -package rd +package rdcloser import ( "fmt" + "testing" + "github.com/alicebob/miniredis/v2" "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" - "testing" ) func TestRedisCloser(t *testing.T) { diff --git a/router/pkg/pubsub/redis/adapter.go b/router/pkg/pubsub/redis/adapter.go index 13f0cbb0e2..343141e5a4 100644 --- a/router/pkg/pubsub/redis/adapter.go +++ b/router/pkg/pubsub/redis/adapter.go @@ -7,7 +7,7 @@ import ( "github.com/wundergraph/cosmo/router/pkg/metric" - rd "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/redis" + rd "github.com/wundergraph/cosmo/router/internal/rdcloser" "github.com/wundergraph/cosmo/router/pkg/pubsub/datasource" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/resolve" "go.uber.org/zap" From c07f6a079e38981cf3ea8dcced9ddce86ecaf8db Mon Sep 17 00:00:00 2001 From: endigma Date: Tue, 2 Sep 2025 13:50:10 +0100 Subject: [PATCH 07/10] fix: confusing apq return causing cache issues --- router/core/operation_processor.go | 5 ++++- router/internal/persistedoperation/apq/client.go | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/router/core/operation_processor.go b/router/core/operation_processor.go index 63037924a1..c145df1a59 100644 --- a/router/core/operation_processor.go +++ b/router/core/operation_processor.go @@ -437,10 +437,12 @@ func (o *OperationKit) FetchPersistedOperation(ctx context.Context, clientInfo * // 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, false, err + return false, true, err } } else { persistedOperationData, isAPQ, err := o.operationProcessor.persistedOperationClient.PersistedOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash) @@ -453,6 +455,7 @@ func (o *OperationKit) FetchPersistedOperation(ctx context.Context, clientInfo * 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 { diff --git a/router/internal/persistedoperation/apq/client.go b/router/internal/persistedoperation/apq/client.go index 86a50995bf..278bab1eca 100644 --- a/router/internal/persistedoperation/apq/client.go +++ b/router/internal/persistedoperation/apq/client.go @@ -3,6 +3,7 @@ package apq import ( "context" "errors" + "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage" "github.com/wundergraph/cosmo/router/pkg/config" "go.uber.org/zap" From c23e02a47f26493e7c05c5996f89eeaf3659fe6d Mon Sep 17 00:00:00 2001 From: endigma Date: Tue, 2 Sep 2025 14:11:17 +0100 Subject: [PATCH 08/10] chore: fix shadowed isAPQ variable --- router/core/operation_processor.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/router/core/operation_processor.go b/router/core/operation_processor.go index c145df1a59..eb020a785f 100644 --- a/router/core/operation_processor.go +++ b/router/core/operation_processor.go @@ -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 @@ -445,10 +445,15 @@ func (o *OperationKit) FetchPersistedOperation(ctx context.Context, clientInfo * return false, true, err } } else { - persistedOperationData, isAPQ, err := o.operationProcessor.persistedOperationClient.PersistedOperation(ctx, clientInfo.Name, o.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash) + var persistedOperationData []byte + var err error + + 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 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, From 316a3ada0fd4ccfa9c28c5470e9e9bea45731d99 Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 3 Sep 2025 10:15:12 +0100 Subject: [PATCH 09/10] chore: rename rdcloser => rediscloser --- router/core/ratelimiter.go | 2 +- router/core/router.go | 2 +- router/core/router_config.go | 2 +- router/internal/persistedoperation/apq/redis.go | 2 +- .../{rdcloser/rdcloser.go => rediscloser/rediscloser.go} | 2 +- .../rdcloser_test.go => rediscloser/rediscloser_test.go} | 2 +- router/pkg/pubsub/redis/adapter.go | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) rename router/internal/{rdcloser/rdcloser.go => rediscloser/rediscloser.go} (99%) rename router/internal/{rdcloser/rdcloser_test.go => rediscloser/rediscloser_test.go} (98%) diff --git a/router/core/ratelimiter.go b/router/core/ratelimiter.go index a494a7de5a..c206c3a421 100644 --- a/router/core/ratelimiter.go +++ b/router/core/ratelimiter.go @@ -10,7 +10,7 @@ import ( "reflect" "sync" - rd "github.com/wundergraph/cosmo/router/internal/rdcloser" + rd "github.com/wundergraph/cosmo/router/internal/rediscloser" "github.com/expr-lang/expr/vm" "github.com/go-redis/redis_rate/v10" diff --git a/router/core/router.go b/router/core/router.go index 623eee0f90..3432528a7a 100644 --- a/router/core/router.go +++ b/router/core/router.go @@ -37,7 +37,7 @@ import ( "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/cdn" "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/fs" "github.com/wundergraph/cosmo/router/internal/persistedoperation/operationstorage/s3" - rd "github.com/wundergraph/cosmo/router/internal/rdcloser" + rd "github.com/wundergraph/cosmo/router/internal/rediscloser" "github.com/wundergraph/cosmo/router/internal/retrytransport" "github.com/wundergraph/cosmo/router/internal/stringsx" "github.com/wundergraph/cosmo/router/pkg/config" diff --git a/router/core/router_config.go b/router/core/router_config.go index e3061ebb5a..e7e17ff018 100644 --- a/router/core/router_config.go +++ b/router/core/router_config.go @@ -8,7 +8,7 @@ import ( nodev1 "github.com/wundergraph/cosmo/router/gen/proto/wg/cosmo/node/v1" "github.com/wundergraph/cosmo/router/internal/graphqlmetrics" "github.com/wundergraph/cosmo/router/internal/persistedoperation" - rd "github.com/wundergraph/cosmo/router/internal/rdcloser" + rd "github.com/wundergraph/cosmo/router/internal/rediscloser" "github.com/wundergraph/cosmo/router/internal/retrytransport" "github.com/wundergraph/cosmo/router/pkg/config" "github.com/wundergraph/cosmo/router/pkg/controlplane/configpoller" diff --git a/router/internal/persistedoperation/apq/redis.go b/router/internal/persistedoperation/apq/redis.go index 5f6e0a391a..67093d91bd 100644 --- a/router/internal/persistedoperation/apq/redis.go +++ b/router/internal/persistedoperation/apq/redis.go @@ -6,7 +6,7 @@ import ( "time" "github.com/redis/go-redis/v9" - rd "github.com/wundergraph/cosmo/router/internal/rdcloser" + rd "github.com/wundergraph/cosmo/router/internal/rediscloser" "github.com/wundergraph/cosmo/router/pkg/config" "go.uber.org/zap" ) diff --git a/router/internal/rdcloser/rdcloser.go b/router/internal/rediscloser/rediscloser.go similarity index 99% rename from router/internal/rdcloser/rdcloser.go rename to router/internal/rediscloser/rediscloser.go index 7bd370152e..2bbc7c3459 100644 --- a/router/internal/rdcloser/rdcloser.go +++ b/router/internal/rediscloser/rediscloser.go @@ -1,4 +1,4 @@ -package rdcloser +package rediscloser import ( "context" diff --git a/router/internal/rdcloser/rdcloser_test.go b/router/internal/rediscloser/rediscloser_test.go similarity index 98% rename from router/internal/rdcloser/rdcloser_test.go rename to router/internal/rediscloser/rediscloser_test.go index 5db69c2bfa..79da15f237 100644 --- a/router/internal/rdcloser/rdcloser_test.go +++ b/router/internal/rediscloser/rediscloser_test.go @@ -1,4 +1,4 @@ -package rdcloser +package rediscloser import ( "fmt" diff --git a/router/pkg/pubsub/redis/adapter.go b/router/pkg/pubsub/redis/adapter.go index 343141e5a4..8de962d2b6 100644 --- a/router/pkg/pubsub/redis/adapter.go +++ b/router/pkg/pubsub/redis/adapter.go @@ -7,7 +7,7 @@ import ( "github.com/wundergraph/cosmo/router/pkg/metric" - rd "github.com/wundergraph/cosmo/router/internal/rdcloser" + rd "github.com/wundergraph/cosmo/router/internal/rediscloser" "github.com/wundergraph/cosmo/router/pkg/pubsub/datasource" "github.com/wundergraph/graphql-go-tools/v2/pkg/engine/resolve" "go.uber.org/zap" From 521b8d8434039a15b27ef7a90f71c5e20f474983 Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 3 Sep 2025 10:20:40 +0100 Subject: [PATCH 10/10] chore: undo change to debug.config.yaml --- router/debug.config.yaml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/router/debug.config.yaml b/router/debug.config.yaml index 339f903812..4baf5895a6 100644 --- a/router/debug.config.yaml +++ b/router/debug.config.yaml @@ -12,14 +12,6 @@ execution_config: log_level: debug -# automatic_persisted_queries: -# enabled: true - -persisted_operations: - disabled: false - safelist: - enabled: true - watch_config: enabled: true interval: '10s'