diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs index e2c0d88b68..0953726458 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs @@ -47,7 +47,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http private readonly object _onStartingSync = new Object(); private readonly object _onCompletedSync = new Object(); - protected bool _requestRejected; private Streams _frameStreams; protected Stack, object>> _onStarting; @@ -64,6 +63,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http private bool _canHaveBody; private bool _autoChunk; protected Exception _applicationException; + private BadHttpRequestException _requestRejectedException; protected HttpVersion _httpVersion; @@ -717,7 +717,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http protected Task TryProduceInvalidRequestResponse() { - if (_requestRejected) + if (_requestRejectedException != null) { if (FrameRequestHeaders == null || FrameResponseHeaders == null) { @@ -732,7 +732,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http protected Task ProduceEnd() { - if (_requestRejected || _applicationException != null) + if (_requestRejectedException != null || _applicationException != null) { if (HasResponseStarted) { @@ -741,8 +741,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http return TaskCache.CompletedTask; } - // If the request was rejected, the error state has already been set by SetBadRequestState - if (!_requestRejected) + // If the request was rejected, the error state has already been set by SetBadRequestState and + // that should take precedence. + if (_requestRejectedException != null) + { + SetErrorResponseHeaders(statusCode: _requestRejectedException.StatusCode); + } + else { // 500 Internal Server Error SetErrorResponseHeaders(statusCode: 500); @@ -1394,24 +1399,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http _applicationException); } - public void RejectRequest(string message) - { - throw new ObjectDisposedException( - "The response has been aborted due to an unhandled application exception.", - _applicationException); - } - public void RejectRequest(RequestRejectionReason reason) { - var ex = BadHttpRequestException.GetException(reason); - SetBadRequestState(ex); - throw ex; + RejectRequest(BadHttpRequestException.GetException(reason)); } public void RejectRequest(RequestRejectionReason reason, string value) { - var ex = BadHttpRequestException.GetException(reason, value); - SetBadRequestState(ex); + RejectRequest(BadHttpRequestException.GetException(reason, value)); + } + + private void RejectRequest(BadHttpRequestException ex) + { + Log.ConnectionBadRequest(ConnectionId, ex); throw ex; } @@ -1422,17 +1422,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http public void SetBadRequestState(BadHttpRequestException ex) { - // Setting status code will throw if response has already started if (!HasResponseStarted) { - SetErrorResponseHeaders(statusCode: ex.StatusCode); + SetErrorResponseHeaders(ex.StatusCode); } _keepAlive = false; _requestProcessingStopping = true; - _requestRejected = true; - - Log.ConnectionBadRequest(ConnectionId, ex); + _requestRejectedException = ex; } protected void ReportApplicationError(Exception ex) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs index b71f94dbd3..cfdc1fc556 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs @@ -100,6 +100,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http catch (Exception ex) { ReportApplicationError(ex); + + if (ex is BadHttpRequestException) + { + throw; + } } finally { @@ -118,30 +123,37 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { await FireOnCompleted(); } - - // If _requestAbort is set, the connection has already been closed. - if (Volatile.Read(ref _requestAborted) == 0) - { - ResumeStreams(); - - if (_keepAlive) - { - // Finish reading the request body in case the app did not. - await messageBody.Consume(); - } - - // ProduceEnd() must be called before _application.DisposeContext(), to ensure - // HttpContext.Response.StatusCode is correctly set when - // IHttpContextFactory.Dispose(HttpContext) is called. - await ProduceEnd(); - } - else if (!HasResponseStarted) - { - // If the request was aborted and no response was sent, there's no - // meaningful status code to log. - StatusCode = 0; - } } + + // If _requestAbort is set, the connection has already been closed. + if (Volatile.Read(ref _requestAborted) == 0) + { + ResumeStreams(); + + if (_keepAlive) + { + // Finish reading the request body in case the app did not. + await messageBody.Consume(); + } + + // ProduceEnd() must be called before _application.DisposeContext(), to ensure + // HttpContext.Response.StatusCode is correctly set when + // IHttpContextFactory.Dispose(HttpContext) is called. + await ProduceEnd(); + } + else if (!HasResponseStarted) + { + // If the request was aborted and no response was sent, there's no + // meaningful status code to log. + StatusCode = 0; + } + } + catch (BadHttpRequestException ex) + { + // Handle BadHttpRequestException thrown during app execution or remaining message body consumption. + // This has to be caught here so StatusCode is set properly before disposing the HttpContext + // (DisposeContext logs StatusCode). + SetBadRequestState(ex); } finally { @@ -169,11 +181,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } catch (BadHttpRequestException ex) { - if (!_requestRejected) - { - // SetBadRequestState logs the error. - SetBadRequestState(ex); - } + // Handle BadHttpRequestException thrown during request line or header parsing. + // SetBadRequestState logs the error. + SetBadRequestState(ex); } catch (Exception ex) { @@ -183,11 +193,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { try { - await TryProduceInvalidRequestResponse(); - // If _requestAborted is set, the connection has already been closed. if (Volatile.Read(ref _requestAborted) == 0) { + await TryProduceInvalidRequestResponse(); ConnectionControl.End(ProduceEndType.SocketShutdown); } } diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs index 336be582ee..6140e6ea0a 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.IO; using System.Linq; using System.Net; using System.Net.Http; @@ -239,6 +238,38 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests sendMalformedRequest: true); } + [Fact] + public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedRead() + { + return ResponseStatusCodeSetBeforeHttpContextDispose( + async context => + { + await context.Request.Body.ReadAsync(new byte[1], 0, 1); + }, + expectedClientStatusCode: null, + expectedServerStatusCode: HttpStatusCode.BadRequest, + sendMalformedRequest: true); + } + + [Fact] + public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIgnored() + { + return ResponseStatusCodeSetBeforeHttpContextDispose( + async context => + { + try + { + await context.Request.Body.ReadAsync(new byte[1], 0, 1); + } + catch (BadHttpRequestException) + { + } + }, + expectedClientStatusCode: null, + expectedServerStatusCode: HttpStatusCode.BadRequest, + sendMalformedRequest: true); + } + private static async Task ResponseStatusCodeSetBeforeHttpContextDispose( RequestDelegate handler, HttpStatusCode? expectedClientStatusCode, @@ -730,14 +761,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests [Fact] public async Task HeadResponseCanContainContentLengthHeader() { - var testLogger = new TestApplicationErrorLogger(); - var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) }; - using (var server = new TestServer(httpContext => { httpContext.Response.ContentLength = 42; return TaskCache.CompletedTask; - }, serviceContext)) + }, new TestServiceContext())) { using (var connection = server.CreateConnection()) { @@ -746,7 +774,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests "", ""); await connection.ReceiveEnd( - $"HTTP/1.1 200 OK", + "HTTP/1.1 200 OK", $"Date: {server.Context.DateHeaderValue}", "Content-Length: 42", "", @@ -758,14 +786,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests [Fact] public async Task HeadResponseCanContainContentLengthHeaderButBodyNotWritten() { - var testLogger = new TestApplicationErrorLogger(); - var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) }; - using (var server = new TestServer(async httpContext => { httpContext.Response.ContentLength = 12; await httpContext.Response.WriteAsync("hello, world"); - }, serviceContext)) + }, new TestServiceContext())) { using (var connection = server.CreateConnection()) { @@ -774,7 +799,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests "", ""); await connection.ReceiveEnd( - $"HTTP/1.1 200 OK", + "HTTP/1.1 200 OK", $"Date: {server.Context.DateHeaderValue}", "Content-Length: 12", "", @@ -783,6 +808,46 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests } } + [Fact] + public async Task AppCanWriteOwnBadRequestResponse() + { + var expectedResponse = string.Empty; + var responseWrittenTcs = new TaskCompletionSource(); + + using (var server = new TestServer(async httpContext => + { + try + { + await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1); + } + catch (BadHttpRequestException ex) + { + expectedResponse = ex.Message; + httpContext.Response.StatusCode = 400; + httpContext.Response.ContentLength = ex.Message.Length; + await httpContext.Response.WriteAsync(ex.Message); + responseWrittenTcs.SetResult(null); + } + }, new TestServiceContext())) + { + using (var connection = server.CreateConnection()) + { + await connection.SendEnd( + "POST / HTTP/1.1", + "Transfer-Encoding: chunked", + "", + "bad"); + await responseWrittenTcs.Task; + await connection.ReceiveEnd( + "HTTP/1.1 400 Bad Request", + $"Date: {server.Context.DateHeaderValue}", + $"Content-Length: {expectedResponse.Length}", + "", + expectedResponse); + } + } + } + public static TheoryData NullHeaderData { get