diff --git a/docs/migration/migration8.md b/docs/migration/migration8.md index 7837cc4d..0a6eb3d2 100644 --- a/docs/migration/migration8.md +++ b/docs/migration/migration8.md @@ -20,6 +20,11 @@ the `CsrfProtectionHeaders` property on the same class. See the readme for more details. - Form POST requests are disabled by default; to enable them, set the `ReadFormOnPost` setting to `true`. +- Validation errors such as authentication errors may now be returned with a 'preferred' status + code instead of a 400 status code. This occurs when (1) the response would otherwise contain + a 400 status code (e.g. the execution of the document has not yet begun), and (2) all errors + in the response prefer the same status code. For practical purposes, this means that the included + errors triggered by the authorization validation rule will now return 401 or 403 when appropriate. ## Other changes diff --git a/src/Transports.AspNetCore/Errors/HttpMethodValidationError.cs b/src/Transports.AspNetCore/Errors/HttpMethodValidationError.cs index 11e08fb2..6fb7f479 100644 --- a/src/Transports.AspNetCore/Errors/HttpMethodValidationError.cs +++ b/src/Transports.AspNetCore/Errors/HttpMethodValidationError.cs @@ -4,11 +4,14 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors; /// Represents a validation error indicating that the requested operation is not valid /// for the type of HTTP request. /// -public class HttpMethodValidationError : ValidationError +public class HttpMethodValidationError : ValidationError, IHasPreferredStatusCode { /// public HttpMethodValidationError(GraphQLParser.ROM originalQuery, ASTNode node, string message) : base(originalQuery, null!, message, node) { } + + /// + public HttpStatusCode PreferredStatusCode { get; set; } = HttpStatusCode.MethodNotAllowed; } diff --git a/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs b/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs index 5d493710..55337936 100644 --- a/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs +++ b/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs @@ -584,12 +584,26 @@ protected virtual async Task HandleRequestAsync( var userContext = await BuildUserContextAsync(context, null); var result = await ExecuteRequestAsync(context, gqlRequest, context.RequestServices, userContext); HttpStatusCode statusCode = HttpStatusCode.OK; + // when the request fails validation (this logic does not apply to execution errors) if (!result.Executed) { + // always return 405 Method Not Allowed when applicable, as this is a transport problem, not really a validation error, + // even though it occurs during validation (because the query text must be parsed to know if the request is a query or a mutation) if (result.Errors?.Any(e => e is HttpMethodValidationError) == true) + { statusCode = HttpStatusCode.MethodNotAllowed; + } + // otherwise use 4xx error codes when configured to do so else if (_options.ValidationErrorsReturnBadRequest) + { statusCode = HttpStatusCode.BadRequest; + // if all errors being returned prefer the same status code, use that + if (result.Errors?.Count > 0 && result.Errors[0] is IHasPreferredStatusCode initialError) + { + if (result.Errors.All(e => e is IHasPreferredStatusCode e2 && e2.PreferredStatusCode == initialError.PreferredStatusCode)) + statusCode = initialError.PreferredStatusCode; + } + } } await WriteJsonResponseAsync(context, statusCode, result); } diff --git a/src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs b/src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs index b9ab9acb..93c76596 100644 --- a/src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs +++ b/src/Transports.AspNetCore/GraphQLHttpMiddlewareOptions.cs @@ -43,8 +43,8 @@ public class GraphQLHttpMiddlewareOptions : IAuthorizationOptions public bool ExecuteBatchedRequestsInParallel { get; set; } = true; /// - /// When enabled, GraphQL requests with validation errors - /// have the HTTP status code set to 400 Bad Request. + /// When enabled, GraphQL requests with validation errors have the HTTP status code + /// set to 400 Bad Request or the error status code dictated by the error. /// GraphQL requests with execution errors are unaffected. ///

/// Does not apply to batched or WebSocket requests. diff --git a/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt index 279e2717..62fe0990 100644 --- a/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt +++ b/tests/ApiApprovalTests/net50+net60+net80/GraphQL.Server.Transports.AspNetCore.approved.txt @@ -220,9 +220,10 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors public FileSizeExceededError() { } public System.Net.HttpStatusCode PreferredStatusCode { get; set; } } - public class HttpMethodValidationError : GraphQL.Validation.ValidationError + public class HttpMethodValidationError : GraphQL.Validation.ValidationError, GraphQL.Server.Transports.AspNetCore.Errors.IHasPreferredStatusCode { public HttpMethodValidationError(GraphQLParser.ROM originalQuery, GraphQLParser.AST.ASTNode node, string message) { } + public System.Net.HttpStatusCode PreferredStatusCode { get; set; } } public interface IHasPreferredStatusCode { diff --git a/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt index e1a2d4c7..485e3fa7 100644 --- a/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt +++ b/tests/ApiApprovalTests/netcoreapp21+netstandard20/GraphQL.Server.Transports.AspNetCore.approved.txt @@ -238,9 +238,10 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors public FileSizeExceededError() { } public System.Net.HttpStatusCode PreferredStatusCode { get; set; } } - public class HttpMethodValidationError : GraphQL.Validation.ValidationError + public class HttpMethodValidationError : GraphQL.Validation.ValidationError, GraphQL.Server.Transports.AspNetCore.Errors.IHasPreferredStatusCode { public HttpMethodValidationError(GraphQLParser.ROM originalQuery, GraphQLParser.AST.ASTNode node, string message) { } + public System.Net.HttpStatusCode PreferredStatusCode { get; set; } } public interface IHasPreferredStatusCode { diff --git a/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt b/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt index 6ed0eab4..c60e890a 100644 --- a/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt +++ b/tests/ApiApprovalTests/netcoreapp31/GraphQL.Server.Transports.AspNetCore.approved.txt @@ -220,9 +220,10 @@ namespace GraphQL.Server.Transports.AspNetCore.Errors public FileSizeExceededError() { } public System.Net.HttpStatusCode PreferredStatusCode { get; set; } } - public class HttpMethodValidationError : GraphQL.Validation.ValidationError + public class HttpMethodValidationError : GraphQL.Validation.ValidationError, GraphQL.Server.Transports.AspNetCore.Errors.IHasPreferredStatusCode { public HttpMethodValidationError(GraphQLParser.ROM originalQuery, GraphQLParser.AST.ASTNode node, string message) { } + public System.Net.HttpStatusCode PreferredStatusCode { get; set; } } public interface IHasPreferredStatusCode { diff --git a/tests/Samples.Authorization.Tests/EndToEndTests.cs b/tests/Samples.Authorization.Tests/EndToEndTests.cs index 1c169d02..ab1daa62 100644 --- a/tests/Samples.Authorization.Tests/EndToEndTests.cs +++ b/tests/Samples.Authorization.Tests/EndToEndTests.cs @@ -21,7 +21,7 @@ public Task GraphQLGet_Success() [Fact] public Task GraphQLGet_AccessDenied() - => new ServerTests().VerifyGraphQLGetAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.BadRequest); + => new ServerTests().VerifyGraphQLGetAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.Unauthorized); [Fact] public Task GraphQLPost_Success() @@ -29,7 +29,7 @@ public Task GraphQLPost_Success() [Fact] public Task GraphQPost_AccessDenied() - => new ServerTests().VerifyGraphQLPostAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.BadRequest); + => new ServerTests().VerifyGraphQLPostAsync("/graphql", ACCESS_DENIED_QUERY, ACCESS_DENIED_RESPONSE, HttpStatusCode.Unauthorized); [Fact] public Task GraphQLWebSocket_Success() diff --git a/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs b/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs index f364b81e..048a1f97 100644 --- a/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs +++ b/tests/Transports.AspNetCore.Tests/AuthorizationTests.cs @@ -765,7 +765,7 @@ public async Task EndToEnd(bool authenticated) using var client = server.CreateClient(); using var response = await client.GetAsync("/graphql?query={ parent { child } }"); - response.StatusCode.ShouldBe(authenticated ? System.Net.HttpStatusCode.OK : System.Net.HttpStatusCode.BadRequest); + response.StatusCode.ShouldBe(authenticated ? System.Net.HttpStatusCode.OK : System.Net.HttpStatusCode.Unauthorized); var actual = await response.Content.ReadAsStringAsync(); if (authenticated) diff --git a/tests/Transports.AspNetCore.Tests/Middleware/GetTests.cs b/tests/Transports.AspNetCore.Tests/Middleware/GetTests.cs index 751ad347..dfe83148 100644 --- a/tests/Transports.AspNetCore.Tests/Middleware/GetTests.cs +++ b/tests/Transports.AspNetCore.Tests/Middleware/GetTests.cs @@ -1,4 +1,6 @@ using System.Net; +using GraphQL.Server.Transports.AspNetCore.Errors; +using GraphQL.Validation; namespace Tests.Middleware; @@ -6,7 +8,7 @@ public class GetTests : IDisposable { private GraphQLHttpMiddlewareOptions _options = null!; private GraphQLHttpMiddlewareOptions _options2 = null!; - private readonly Action _configureExecution = _ => { }; + private Action _configureExecution = _ => { }; private readonly TestServer _server; public GetTests() @@ -55,6 +57,14 @@ private class Query2 { public static string? Ext(IResolveFieldContext context) => context.InputExtensions.TryGetValue("test", out var value) ? value?.ToString() : null; + + public static string? CustomError() => throw new CustomError(); + } + + public class CustomError : ValidationError, IHasPreferredStatusCode + { + public CustomError() : base("Custom error") { } + public HttpStatusCode PreferredStatusCode => HttpStatusCode.NotAcceptable; } public void Dispose() => _server.Dispose(); @@ -250,6 +260,67 @@ public async Task Disabled() response.StatusCode.ShouldBe(HttpStatusCode.NotFound); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredStatusCode_ExecutionErrors(bool badRequest) + { + _options2.ValidationErrorsReturnBadRequest = badRequest; + var client = _server.CreateClient(); + using var response = await client.GetAsync("/graphql2?query={customError}"); + await response.ShouldBeAsync( + HttpStatusCode.OK, + """{"errors":[{"message":"Custom error","locations":[{"line":1,"column":2}],"path":["customError"],"extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}],"data":{"customError":null}}"""); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredStatusCode_ValidationErrors(bool badRequest) + { + _options2.ValidationErrorsReturnBadRequest = badRequest; + var mockRule = new Mock(MockBehavior.Loose); + mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny())).Returns(context => + { + context.ReportError(new CustomError()); + context.ReportError(new CustomError()); + return default; + }); + _configureExecution = o => + { + o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object); + }; + var client = _server.CreateClient(); + using var response = await client.GetAsync("/graphql2?query={__typename}"); + await response.ShouldBeAsync( + badRequest ? HttpStatusCode.NotAcceptable : HttpStatusCode.OK, + """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}]}"""); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredStatusCode_MixedValidationErrors(bool badRequest) + { + _options2.ValidationErrorsReturnBadRequest = badRequest; + var mockRule = new Mock(MockBehavior.Loose); + mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny())).Returns(context => + { + context.ReportError(new CustomError()); + context.ReportError(new ValidationError("test")); + return default; + }); + _configureExecution = o => + { + o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object); + }; + var client = _server.CreateClient(); + using var response = await client.GetAsync("/graphql2?query={__typename}"); + await response.ShouldBeAsync( + badRequest ? HttpStatusCode.BadRequest : HttpStatusCode.OK, + """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"test","extensions":{"code":"VALIDATION_ERROR","codes":["VALIDATION_ERROR"]}}]}"""); + } + [Theory] [InlineData(false)] [InlineData(true)] diff --git a/tests/Transports.AspNetCore.Tests/Middleware/PostTests.cs b/tests/Transports.AspNetCore.Tests/Middleware/PostTests.cs index e64b3ee9..2aff9345 100644 --- a/tests/Transports.AspNetCore.Tests/Middleware/PostTests.cs +++ b/tests/Transports.AspNetCore.Tests/Middleware/PostTests.cs @@ -1,4 +1,6 @@ using System.Net; +using GraphQL.Server.Transports.AspNetCore.Errors; +using GraphQL.Validation; namespace Tests.Middleware; @@ -6,7 +8,7 @@ public class PostTests : IDisposable { private GraphQLHttpMiddlewareOptions _options = null!; private GraphQLHttpMiddlewareOptions _options2 = null!; - private readonly Action _configureExecution = _ => { }; + private Action _configureExecution = _ => { }; private readonly TestServer _server; public PostTests() @@ -65,6 +67,14 @@ private class Query2 public static MyFile File3(MyFileInput arg) => new(arg.File); public static IEnumerable File4(IEnumerable args) => args.Select(x => new MyFile(x.File)); public static IEnumerable File5(MyFileInput2 args) => args.Files.Select(x => new MyFile(x)); + + public static string? CustomError() => throw new CustomError(); + } + + public class CustomError : ValidationError, IHasPreferredStatusCode + { + public CustomError() : base("Custom error") { } + public HttpStatusCode PreferredStatusCode => HttpStatusCode.NotAcceptable; } private record MyFileInput(IFormFile File); @@ -542,6 +552,64 @@ public async Task Disabled() response.StatusCode.ShouldBe(HttpStatusCode.NotFound); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredStatusCode_ExecutionErrors(bool badRequest) + { + _options2.ValidationErrorsReturnBadRequest = badRequest; + using var response = await PostRequestAsync("/graphql2", new() { Query = "{customError}" }); + await response.ShouldBeAsync( + HttpStatusCode.OK, + """{"errors":[{"message":"Custom error","locations":[{"line":1,"column":2}],"path":["customError"],"extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}],"data":{"customError":null}}"""); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredStatusCode_ValidationErrors(bool badRequest) + { + _options2.ValidationErrorsReturnBadRequest = badRequest; + var mockRule = new Mock(MockBehavior.Loose); + mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny())).Returns(context => + { + context.ReportError(new CustomError()); + context.ReportError(new CustomError()); + return default; + }); + _configureExecution = o => + { + o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object); + }; + using var response = await PostRequestAsync("/graphql2", new() { Query = "{__typename}" }); + await response.ShouldBeAsync( + badRequest ? HttpStatusCode.NotAcceptable : HttpStatusCode.OK, + """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}}]}"""); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task PreferredStatusCode_MixedValidationErrors(bool badRequest) + { + _options2.ValidationErrorsReturnBadRequest = badRequest; + var mockRule = new Mock(MockBehavior.Loose); + mockRule.Setup(x => x.GetPreNodeVisitorAsync(It.IsAny())).Returns(context => + { + context.ReportError(new CustomError()); + context.ReportError(new ValidationError("test")); + return default; + }); + _configureExecution = o => + { + o.ValidationRules = (o.ValidationRules ?? DocumentValidator.CoreRules).Append(mockRule.Object); + }; + using var response = await PostRequestAsync("/graphql2", new() { Query = "{__typename}" }); + await response.ShouldBeAsync( + badRequest ? HttpStatusCode.BadRequest : HttpStatusCode.OK, + """{"errors":[{"message":"Custom error","extensions":{"code":"CUSTOM","codes":["CUSTOM"]}},{"message":"test","extensions":{"code":"VALIDATION_ERROR","codes":["VALIDATION_ERROR"]}}]}"""); + } + [Theory] [InlineData(false)] [InlineData(true)]