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}