From 352b99da88119d50d37ec9382afc985cd781fe49 Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 11 Mar 2026 17:18:06 +0000 Subject: [PATCH 1/5] fix(router): make computeSha256 independent of metric/access log attributes Convert the mutually exclusive if/else-if chain in graph_server.go:671-689 to independent if blocks so safelist/logUnknown checks always evaluate. Guard the persisted query hash overwrite in graphql_prehandler.go:558-563 to preserve client-provided hashes for "run by ID" requests. Use client-provided hash for telemetry when a request has a hash but no query body. Add tests for safelist + access log SHA256 attributes (the configuration that exposed the bug). Update cache_warmup_test.go cache metrics expectations. --- router-tests/cache_warmup_test.go | 2 + router-tests/safelist_test.go | 84 +++++++++++++++++++++++++++++++ router/core/graph_server.go | 10 ++-- router/core/graphql_prehandler.go | 40 ++++++++++----- 4 files changed, 120 insertions(+), 16 deletions(-) diff --git a/router-tests/cache_warmup_test.go b/router-tests/cache_warmup_test.go index 7692eaa9d4..0987193357 100644 --- a/router-tests/cache_warmup_test.go +++ b/router-tests/cache_warmup_test.go @@ -462,6 +462,8 @@ func TestCacheWarmup(t *testing.T) { ValidationMisses: 1, PlanHits: 4, PlanMisses: 1, + QueryHashMisses: 2, // 2x miss for safelist queries (raw query body hashed for safelist check) + QueryHashHits: 1, // 1x hit for repeated query body hash }, }, }, func(t *testing.T, xEnv *testenv.Environment) { diff --git a/router-tests/safelist_test.go b/router-tests/safelist_test.go index 97cd0e48b0..1647ce48a2 100644 --- a/router-tests/safelist_test.go +++ b/router-tests/safelist_test.go @@ -117,6 +117,90 @@ func TestSafelist(t *testing.T) { }) }) + t.Run("safelist with access log sha256 attribute allows a persisted query to run", func(t *testing.T) { + testenv.Run(t, &testenv.Config{ + AccessLogFields: []config.CustomAttribute{ + { + Key: "operation_sha256", + ValueFrom: &config.CustomDynamicAttribute{ + ContextField: core.ContextFieldOperationSha256, + }, + }, + }, + RouterOptions: []core.Option{ + core.WithPersistedOperationsConfig(config.PersistedOperationsConfig{ + Safelist: config.SafelistConfiguration{Enabled: true}, + }), + }, + }, func(t *testing.T, xEnv *testenv.Environment) { + header := make(http.Header) + header.Add("graphql-client-name", "my-client") + res, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{ + OperationName: []byte(`"Employees"`), + Header: header, + Query: persistedQuery, + }) + require.NoError(t, err) + require.Equal(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, res.Body) + }) + }) + + t.Run("safelist with access log sha256 attribute allows a persisted query run with ID", func(t *testing.T) { + testenv.Run(t, &testenv.Config{ + AccessLogFields: []config.CustomAttribute{ + { + Key: "operation_sha256", + ValueFrom: &config.CustomDynamicAttribute{ + ContextField: core.ContextFieldOperationSha256, + }, + }, + }, + RouterOptions: []core.Option{ + core.WithPersistedOperationsConfig(config.PersistedOperationsConfig{ + Safelist: config.SafelistConfiguration{Enabled: true}, + }), + }, + }, func(t *testing.T, xEnv *testenv.Environment) { + header := make(http.Header) + header.Add("graphql-client-name", "my-client") + res, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{ + OperationName: []byte(`"Employees"`), + Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f"}}`), + Header: header, + }) + require.NoError(t, err) + require.Equal(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, res.Body) + }) + }) + + t.Run("safelist with access log sha256 attribute rejects non persisted query", func(t *testing.T) { + testenv.Run(t, &testenv.Config{ + AccessLogFields: []config.CustomAttribute{ + { + Key: "operation_sha256", + ValueFrom: &config.CustomDynamicAttribute{ + ContextField: core.ContextFieldOperationSha256, + }, + }, + }, + RouterOptions: []core.Option{ + core.WithPersistedOperationsConfig(config.PersistedOperationsConfig{ + Safelist: config.SafelistConfiguration{Enabled: true}, + }), + }, + }, func(t *testing.T, xEnv *testenv.Environment) { + header := make(http.Header) + header.Add("graphql-client-name", "my-client") + res, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{ + OperationName: []byte(`"Employees"`), + Header: header, + Query: queryWithDetails, + }) + require.NoError(t, err) + require.Equal(t, persistedNotFoundResp, res.Body) + }) + }) + t.Run("log unknown operations", func(t *testing.T) { t.Run("logs non persisted query but allows them to continue", func(t *testing.T) { testenv.Run(t, &testenv.Config{ diff --git a/router/core/graph_server.go b/router/core/graph_server.go index 325ec74c25..10a2b90780 100644 --- a/router/core/graph_server.go +++ b/router/core/graph_server.go @@ -668,21 +668,25 @@ func (s *graphMux) buildOperationCaches(srv *graphServer) (computeSha256 bool, e } // Currently, we only support custom attributes from the context for OTLP metrics - if len(srv.metricConfig.Attributes) > 0 { + if !computeSha256 && len(srv.metricConfig.Attributes) > 0 { for _, customAttribute := range srv.metricConfig.Attributes { if customAttribute.ValueFrom != nil && customAttribute.ValueFrom.ContextField == ContextFieldOperationSha256 { computeSha256 = true break } } - } else if srv.accessLogsConfig != nil { + } + + if !computeSha256 && srv.accessLogsConfig != nil { for _, customAttribute := range append(srv.accessLogsConfig.Attributes, srv.accessLogsConfig.SubgraphAttributes...) { if customAttribute.ValueFrom != nil && customAttribute.ValueFrom.ContextField == ContextFieldOperationSha256 { computeSha256 = true break } } - } else if srv.persistedOperationsConfig.Safelist.Enabled || srv.persistedOperationsConfig.LogUnknown { + } + + if srv.persistedOperationsConfig.Safelist.Enabled || srv.persistedOperationsConfig.LogUnknown { // In these case, we'll want to compute the sha256 for every operation, in order to check that the operation // is present in the Persisted Operation cache computeSha256 = true diff --git a/router/core/graphql_prehandler.go b/router/core/graphql_prehandler.go index ee9eb10fc5..11801412e8 100644 --- a/router/core/graphql_prehandler.go +++ b/router/core/graphql_prehandler.go @@ -543,22 +543,36 @@ func (h *PreHandler) handleOperation(req *http.Request, httpOperation *httpOpera // Compute the operation sha256 hash as soon as possible for observability reasons if h.shouldComputeOperationSha256(operationKit, requestContext) { - if err := operationKit.ComputeOperationSha256(); err != nil { - return &httpGraphqlError{ - message: fmt.Sprintf("error hashing operation: %s", err), - statusCode: http.StatusInternalServerError, + if operationKit.parsedOperation.Request.Query == "" && operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() { + // The request has a persisted hash but no query body (run by ID). Use the client-provided + // hash for telemetry instead of hashing an empty string. The hash will be verified against + // the persisted operations store (source of truth) when the operation is fetched. + requestContext.operation.sha256Hash = operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash + requestContext.expressionContext.Request.Operation.Sha256Hash = requestContext.operation.sha256Hash + + setTelemetryAttributes(req.Context(), requestContext, expr.BucketSha256) + + requestContext.telemetry.addCustomMetricStringAttr(ContextFieldOperationSha256, requestContext.operation.sha256Hash) + } else { + if err := operationKit.ComputeOperationSha256(); err != nil { + return &httpGraphqlError{ + message: fmt.Sprintf("error hashing operation: %s", err), + statusCode: http.StatusInternalServerError, + } } - } - requestContext.operation.sha256Hash = operationKit.parsedOperation.Sha256Hash - requestContext.expressionContext.Request.Operation.Sha256Hash = operationKit.parsedOperation.Sha256Hash + requestContext.operation.sha256Hash = operationKit.parsedOperation.Sha256Hash + requestContext.expressionContext.Request.Operation.Sha256Hash = operationKit.parsedOperation.Sha256Hash - setTelemetryAttributes(req.Context(), requestContext, expr.BucketSha256) + setTelemetryAttributes(req.Context(), requestContext, expr.BucketSha256) - requestContext.telemetry.addCustomMetricStringAttr(ContextFieldOperationSha256, requestContext.operation.sha256Hash) - if h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled { - // Set the request hash to the parsed hash, to see if it matches a persisted operation - operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery = &GraphQLRequestExtensionsPersistedQuery{ - Sha256Hash: operationKit.parsedOperation.Sha256Hash, + requestContext.telemetry.addCustomMetricStringAttr(ContextFieldOperationSha256, requestContext.operation.sha256Hash) + if h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled { + if !operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() { + // Set the request hash to the parsed hash, to see if it matches a persisted operation + operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery = &GraphQLRequestExtensionsPersistedQuery{ + Sha256Hash: operationKit.parsedOperation.Sha256Hash, + } + } } } } From de3b807f74ffd8004e31a02bc7da4135afa605de Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 11 Mar 2026 17:21:15 +0000 Subject: [PATCH 2/5] fix(router): simplify comment in prehandler sha256 path --- router/core/graphql_prehandler.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/router/core/graphql_prehandler.go b/router/core/graphql_prehandler.go index 11801412e8..c0098cd3d3 100644 --- a/router/core/graphql_prehandler.go +++ b/router/core/graphql_prehandler.go @@ -544,9 +544,7 @@ func (h *PreHandler) handleOperation(req *http.Request, httpOperation *httpOpera // Compute the operation sha256 hash as soon as possible for observability reasons if h.shouldComputeOperationSha256(operationKit, requestContext) { if operationKit.parsedOperation.Request.Query == "" && operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() { - // The request has a persisted hash but no query body (run by ID). Use the client-provided - // hash for telemetry instead of hashing an empty string. The hash will be verified against - // the persisted operations store (source of truth) when the operation is fetched. + // No query body to hash; use the client-provided persisted hash for telemetry. requestContext.operation.sha256Hash = operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash requestContext.expressionContext.Request.Operation.Sha256Hash = requestContext.operation.sha256Hash From 9c7a30f66fca968e0ca908538606706d1f3d48e9 Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 11 Mar 2026 17:58:57 +0000 Subject: [PATCH 3/5] fix(router): update access log test expectations for persisted hash telemetry Tests previously expected sha256 of empty string for operation_sha256 when a request provided a persisted hash but no query body. Now correctly expects the client-provided persisted hash. --- router-tests/structured_logging_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/router-tests/structured_logging_test.go b/router-tests/structured_logging_test.go index 79477c4234..79c1fbb7fb 100644 --- a/router-tests/structured_logging_test.go +++ b/router-tests/structured_logging_test.go @@ -1818,7 +1818,7 @@ func TestFlakyAccessLogs(t *testing.T) { "service_name": "service-name", // From request header "operation_persisted_hash": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context "operation_hash": "1163600561566987607", // From context - "operation_sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", // From context + "operation_sha256": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context (client-provided persisted hash, no query body to hash) "operation_name": "Employees", // From context "operation_type": "query", // From context } @@ -2062,7 +2062,7 @@ func TestFlakyAccessLogs(t *testing.T) { val, ok := requestContext["operation_sha256_expression"].(string) require.True(t, ok) - require.Equal(t, "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", val) + require.Equal(t, "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", val) }, ) }) From e1fbfd8ddcaefe939699682b208ba453bb8ea088 Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 11 Mar 2026 18:01:14 +0000 Subject: [PATCH 4/5] chore: remove obvious comment --- router-tests/structured_logging_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/router-tests/structured_logging_test.go b/router-tests/structured_logging_test.go index 79c1fbb7fb..2cb0de83cb 100644 --- a/router-tests/structured_logging_test.go +++ b/router-tests/structured_logging_test.go @@ -1818,7 +1818,7 @@ func TestFlakyAccessLogs(t *testing.T) { "service_name": "service-name", // From request header "operation_persisted_hash": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context "operation_hash": "1163600561566987607", // From context - "operation_sha256": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context (client-provided persisted hash, no query body to hash) + "operation_sha256": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context "operation_name": "Employees", // From context "operation_type": "query", // From context } From d0f37201844a2e097dfadad94c9c87424cfe1e25 Mon Sep 17 00:00:00 2001 From: endigma Date: Wed, 11 Mar 2026 18:05:58 +0000 Subject: [PATCH 5/5] fix(router): rename misleading test to reflect no-body request --- router-tests/structured_logging_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/router-tests/structured_logging_test.go b/router-tests/structured_logging_test.go index 2cb0de83cb..8f8625f0b4 100644 --- a/router-tests/structured_logging_test.go +++ b/router-tests/structured_logging_test.go @@ -2029,7 +2029,7 @@ func TestFlakyAccessLogs(t *testing.T) { ) }) - t.Run("validate request.operation.sha256Hash expression with persisted hash and body", func(t *testing.T) { + t.Run("validate request.operation.sha256Hash expression with persisted hash only", func(t *testing.T) { t.Parallel() testenv.Run(t,