fix(metrics): return 400 not 500 for empty or malformed GraphQL request body#3067
Open
arpitjain099 wants to merge 1 commit into
Open
fix(metrics): return 400 not 500 for empty or malformed GraphQL request body#3067arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
…st 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 guacsec#2195 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2195.
Posting an empty or malformed JSON body to the
/queryGraphQL endpoint returned500 Internal Server Error("unexpected end of JSON input") instead of400 Bad Request. As the reporter noted, a 500 is misleading for what is purely a client input problem and sends people hunting for a server-side fault that does not exist.Root cause
The 500 did not come from the GraphQL server itself. The current gqlgen POST transport already answers a bad request body with 400. The 500 came from the Prometheus metrics middleware
MeasureGraphQLResponseDurationinpkg/metrics/prometheus.go, which wraps the GraphQL handler (wired incmd/guacgql/cmd/server.go). That middleware read the body, ran its ownjson.Unmarshalto pull outoperationNamefor a metric label, and on any unmarshal error short-circuited the request withhttp.StatusInternalServerErrorbefore the GraphQL handler ever ran. An empty body is not valid JSON, so it tripped this path.This matches the review feedback on the earlier (now stale-closed) attempt in #2202: a maintainer noted the change there was in the wrong place and that the fix belongs at the GraphQL server side, not bolted onto the metrics path with a second status decision.
Fix
The metrics middleware should observe, not adjudicate. It now best-effort parses the operation name and, regardless of whether the body is valid JSON, forwards the request to the downstream GraphQL handler. The handler then returns the correct status: 400 for an empty or malformed body, 200 for a valid query. The unmarshal error no longer produces a 500. When the body does not parse, the
operationNamemetric label is simply empty, which is the natural outcome for a request that has no parseable operation.This also removes a redundant body copy that the old code made only to feed that unmarshal.
Behavior before / after
Reproduced with a test that posts to the handler.
Before:
After:
Tests
pkg/assembler/server/server_badrequest_test.go: end-to-end test that runs the real GraphQL server behind the metrics middleware and asserts 400 for empty and malformed bodies and 200 for a valid query. This is the direct regression guard for [bug] GraphQL server returns 500 when sending empty json payload #2195.pkg/metrics/measure_graphql_test.go: unit test asserting the middleware no longer short-circuits a bad body and forwards the original bytes downstream unchanged.go build ./...,go vet, andgo test ./pkg/metrics/... ./pkg/assembler/server/...pass.Note on the prior PR
#2202 targeted this same issue but was auto-closed by the stale bot after 90 days of inactivity (it was never rejected on the merits; it had an unrelated merge-conflict artifact and a maintainer question about placement). This PR takes the placement the reviewer pointed to and adds the regression tests that were missing there.