Return 400 on JSON parse failure for application/json (#9784)

Author: glen-84

dictionary.txt

@@ -282,6 +282,7 @@ Trimmable
 Tzdb
 unittests
 unlisten
+unparseable
 unpublish
 unserialized
 Unsubscriber

src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/Formatters/DefaultHttpResponseFormatter.cs

@@ -34,6 +34,7 @@ public class DefaultHttpResponseFormatter : IHttpResponseFormatter
     private readonly FormatInfo _eventStreamFormat;
     private readonly FormatInfo _jsonLinesFormat;
     private readonly FormatInfo _legacyFormat;
+    private readonly bool _isLegacyTransport;
     private readonly IncrementalDeliveryFormat _incrementalDeliveryDefaultFormat;
 
     /// <summary>
@@ -122,7 +123,8 @@ public DefaultHttpResponseFormatter(
             ContentType.JsonLines,
             ResponseContentType.JsonLines,
             jsonLinesResultFormatter);
-        _defaultFormat = options.HttpTransportVersion is HttpTransportVersion.Legacy
+        _isLegacyTransport = options.HttpTransportVersion is HttpTransportVersion.Legacy;
+        _defaultFormat = _isLegacyTransport
             ? _legacyFormat
             : _graphqlResponseFormat;
 
@@ -400,11 +402,23 @@ protected virtual HttpStatusCode OnDetermineStatusCode(
         FormatInfo format,
         HttpStatusCode? proposedStatusCode)
     {
-        // the current spec proposal strongly recommends to always return OK
-        // when using the legacy application/json response content-type.
         if (format.Kind is ResponseContentType.Json)
         {
-            return HttpStatusCode.OK;
+            // the legacy transport preserves the pre-spec behavior of always returning
+            // 200 for the application/json response content-type.
+            if (_isLegacyTransport)
+            {
+                return HttpStatusCode.OK;
+            }
+
+            // per graphql-over-http §6.4.1, the application/json response content-type
+            // should return 200 for every well-formed request regardless of errors
+            // raised. the only 4xx is 400 for requests the server cannot interpret
+            // (§6.4.1.1.1 JSON parse, §6.4.1.1.2 invalid parameters). honor a proposed
+            // 400; everything else, including an unexpected 500, stays 200.
+            return proposedStatusCode is HttpStatusCode.BadRequest
+                ? HttpStatusCode.BadRequest
+                : HttpStatusCode.OK;
         }
 
         // if we are sending a single result with the multipart/mixed header or

src/HotChocolate/AspNetCore/src/AspNetCore.Pipeline/HttpPostMiddlewareBase.cs

@@ -97,10 +97,16 @@ protected async Task HandleRequestAsync(HttpContext context, ExecutorSession ses
             }
             catch (GraphQLRequestException ex)
             {
-                // A GraphQL request exception is thrown if the HTTP request body couldn't be
-                // parsed. In this case we will return HTTP status code 400 and return a
-                // GraphQL error result.
-                statusCode = HttpStatusCode.BadRequest;
+                // request-interpretation failures (invalid JSON, missing query, etc.) propose
+                // 400 per graphql-over-http §6.4.1.1.1. GraphQL document syntax errors
+                // are document-parsing failures and leave the proposed status unset so the
+                // formatter applies the per-content-type rule (200 for application/json
+                // per §6.4.1.1.3, 400 for application/graphql-response+json).
+                //
+                // classification uses the original parser errors: error filters run by
+                // session.Handle can rewrite the error code and would otherwise misclassify
+                // the failure. the handled errors are used only for the response body.
+                statusCode = IsDocumentSyntaxError(ex.Errors) ? null : HttpStatusCode.BadRequest;
                 var errors = session.Handle(ex.Errors);
                 result = OperationResult.FromError([.. errors]);
                 session.DiagnosticEvents.ParserErrors(context, errors);
@@ -261,6 +267,24 @@ await session.ParseRequestAsync(
         return requests;
     }
 
+    private static bool IsDocumentSyntaxError(IReadOnlyList<IError> errors)
+    {
+        if (errors.Count == 0)
+        {
+            return false;
+        }
+
+        for (var i = 0; i < errors.Count; i++)
+        {
+            if (!string.Equals(errors[i].Code, ErrorCodes.Server.SyntaxError, StringComparison.Ordinal))
+            {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     private static bool TryParseOperations(
         string operationNameString,
         [NotNullWhen(true)] out IReadOnlyList<string>? operationNames)

src/HotChocolate/AspNetCore/test/AspNetCore.Tests/GraphQLOverHttpSpecTests.cs

@@ -121,7 +121,7 @@ public async Task SingleResult_MultipartAcceptHeader(string acceptHeader)
     [InlineData("*/*", Legacy, OK, ContentType.Json)]
     [InlineData("application/*", Latest, BadRequest, ContentType.GraphQLResponse)]
     [InlineData("application/*", Legacy, OK, ContentType.Json)]
-    [InlineData(ContentType.Json, Latest, OK, ContentType.Json)]
+    [InlineData(ContentType.Json, Latest, BadRequest, ContentType.Json)]
     [InlineData(ContentType.Json, Legacy, OK, ContentType.Json)]
     [InlineData(ContentType.GraphQLResponse, Latest, BadRequest, ContentType.GraphQLResponse)]
     [InlineData(ContentType.GraphQLResponse, Legacy, BadRequest, ContentType.GraphQLResponse)]