From d1dce65e40e568b56337a5027416860e78cf9a35 Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Fri, 5 Jun 2026 14:58:51 +0900 Subject: [PATCH] fix(metrics): return 400 not 500 for empty or malformed GraphQL request body The Prometheus MeasureGraphQLResponseDuration middleware read the request body and ran its own json.Unmarshal to extract the operationName metric label. On any unmarshal error it short-circuited the request with HTTP 500 before the GraphQL handler ran, so an empty or malformed body to /query returned 500 'unexpected end of JSON input' instead of 400 Bad Request. Validating the request payload is the GraphQL server's job, and it already answers a bad body with 400. The metrics middleware now parses the operation name best-effort and forwards the request downstream regardless of whether the body is valid JSON, so the handler can return the correct status. A bad body that has no parseable operation simply gets an empty operationName label. Adds an end-to-end test (server behind the metrics middleware) asserting 400 for empty and malformed bodies and 200 for a valid query, plus a middleware unit test that it no longer short-circuits and forwards the body unchanged. Fixes #2195 Signed-off-by: Arpit Jain --- .../server/server_badrequest_test.go | 85 +++++++++++++++++++ pkg/metrics/measure_graphql_test.go | 77 +++++++++++++++++ pkg/metrics/prometheus.go | 20 ++--- 3 files changed, 172 insertions(+), 10 deletions(-) create mode 100644 pkg/assembler/server/server_badrequest_test.go create mode 100644 pkg/metrics/measure_graphql_test.go diff --git a/pkg/assembler/server/server_badrequest_test.go b/pkg/assembler/server/server_badrequest_test.go new file mode 100644 index 0000000000..d6a4e597dd --- /dev/null +++ b/pkg/assembler/server/server_badrequest_test.go @@ -0,0 +1,85 @@ +// +// Copyright 2024 The GUAC Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package server + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + _ "github.com/guacsec/guac/pkg/assembler/backends/keyvalue" + + "github.com/guacsec/guac/internal/testing/stablememmap" + "github.com/guacsec/guac/pkg/assembler/backends" + "github.com/guacsec/guac/pkg/metrics" +) + +// TestQueryEndpointBadRequest covers issue #2195: posting an empty or malformed +// JSON body to the /query endpoint must return 400 Bad Request, not 500 +// Internal Server Error. The handler is exercised together with the Prometheus +// metrics middleware because that middleware (not the GraphQL server) was the +// source of the original 500 response. +func TestQueryEndpointBadRequest(t *testing.T) { + ctx := metrics.WithMetrics(context.Background(), "guac_query_test") + + store := stablememmap.GetStore() + backend, err := backends.Get("keyvalue", ctx, store) + if err != nil { + t.Fatalf("Error getting backend: %v", err) + } + + srv := GetGraphqlServer(ctx, backend) + collector := metrics.FromContext(ctx, "guac_query_test") + handler := collector.MeasureGraphQLResponseDuration(srv) + + tests := []struct { + name string + body string + wantStatus int + }{ + { + name: "empty body", + body: "", + wantStatus: http.StatusBadRequest, + }, + { + name: "malformed json", + body: "{not json", + wantStatus: http.StatusBadRequest, + }, + { + name: "valid query", + body: `{"query":"{ __typename }"}`, + wantStatus: http.StatusOK, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/query", strings.NewReader(tc.body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + handler.ServeHTTP(rr, req) + + if rr.Code != tc.wantStatus { + t.Fatalf("POST /query with %s body: got status %d, want %d (body: %q)", tc.name, rr.Code, tc.wantStatus, rr.Body.String()) + } + }) + } +} diff --git a/pkg/metrics/measure_graphql_test.go b/pkg/metrics/measure_graphql_test.go new file mode 100644 index 0000000000..3c5fe34f31 --- /dev/null +++ b/pkg/metrics/measure_graphql_test.go @@ -0,0 +1,77 @@ +// +// Copyright 2024 The GUAC Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package metrics + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestMeasureGraphQLResponseDurationDoesNotShortCircuit guards against a +// regression of issue #2195: an empty or malformed request body used to make +// this metrics middleware return 500 before the request ever reached the +// GraphQL handler. The middleware must not decide request validity; it should +// pass the request through to the downstream handler (which answers with the +// correct status code for the payload) regardless of whether the body is valid +// JSON. +func TestMeasureGraphQLResponseDurationDoesNotShortCircuit(t *testing.T) { + ctx := WithMetrics(context.Background(), "guac_graphql_test") + collector := FromContext(ctx, "guac_graphql_test") + + // A downstream handler that echoes the body it received so we can assert + // the middleware forwarded it unchanged. + var sawBody string + downstream := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + sawBody = string(b) + w.WriteHeader(http.StatusOK) + }) + + handler := collector.MeasureGraphQLResponseDuration(downstream) + + tests := []struct { + name string + body string + }{ + {name: "empty body", body: ""}, + {name: "malformed json", body: "{not json"}, + {name: "valid json with operation name", body: `{"operationName":"Q","query":"{ __typename }"}`}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + sawBody = "" + req := httptest.NewRequest(http.MethodPost, "/query", strings.NewReader(tc.body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + handler.ServeHTTP(rr, req) + + // The middleware must reach the downstream handler instead of + // short-circuiting with a 500. + if rr.Code != http.StatusOK { + t.Fatalf("middleware short-circuited bad body: got status %d, want %d (request should reach downstream handler)", rr.Code, http.StatusOK) + } + if sawBody != tc.body { + t.Fatalf("downstream received body %q, want %q (body must be forwarded unchanged)", sawBody, tc.body) + } + }) + } +} diff --git a/pkg/metrics/prometheus.go b/pkg/metrics/prometheus.go index 7b1e939618..bd61917e1a 100644 --- a/pkg/metrics/prometheus.go +++ b/pkg/metrics/prometheus.go @@ -276,7 +276,8 @@ func (pc *prometheusCollector) MeasureGraphQLResponseDuration(next http.Handler) return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() - // Create a copy of the request body + // Create a copy of the request body so we can both inspect it here and + // still hand the original bytes to the downstream GraphQL handler. body, err := io.ReadAll(r.Body) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) @@ -284,18 +285,17 @@ func (pc *prometheusCollector) MeasureGraphQLResponseDuration(next http.Handler) } r.Body = io.NopCloser(bytes.NewBuffer(body)) // Replace the request body with a copy - // Create another copy for JSON unmarshalling - bodyCopy := make([]byte, len(body)) - copy(bodyCopy, body) - - // Parse the operation name from the request body copy + // Best-effort parse of the operation name for the metric label. An empty + // or malformed body must not be turned into an error here: validating the + // request payload is the GraphQL server's job, and it already responds + // with 400 Bad Request for bad input. Returning 500 from this metrics + // middleware short-circuited that and was misleading (see issue #2195), + // so we only record the operation name when the body parses and otherwise + // let the request flow through unchanged. var graphqlRequest struct { OperationName string `json:"operationName"` } - if err := json.Unmarshal(bodyCopy, &graphqlRequest); err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } + _ = json.Unmarshal(body, &graphqlRequest) rec := statusRecorder{w, http.StatusOK}