Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Commit 7858479

Browse files
author
Cesar Blum Silveira
committed
Separate request rejection from bad request state setting.
1 parent f1071de commit 7858479

File tree

3 files changed

+130
-59
lines changed

3 files changed

+130
-59
lines changed

‎src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ public abstract partial class Frame : IFrameControl
4747
private readonly object _onStartingSync = new Object();
4848
private readonly object _onCompletedSync = new Object();
4949

50-
protected bool _requestRejected;
5150
private Streams _frameStreams;
5251

5352
protected Stack<KeyValuePair<Func<object, Task>, object>> _onStarting;
@@ -64,6 +63,7 @@ public abstract partial class Frame : IFrameControl
6463
private bool _canHaveBody;
6564
private bool _autoChunk;
6665
protected Exception _applicationException;
66+
private BadHttpRequestException _requestRejectedException;
6767

6868
protected HttpVersion _httpVersion;
6969

@@ -717,7 +717,7 @@ private void ProduceStart(bool appCompleted)
717717

718718
protected Task TryProduceInvalidRequestResponse()
719719
{
720-
if (_requestRejected)
720+
if (_requestRejectedException != null)
721721
{
722722
if (FrameRequestHeaders == null || FrameResponseHeaders == null)
723723
{
@@ -732,7 +732,7 @@ protected Task TryProduceInvalidRequestResponse()
732732

733733
protected Task ProduceEnd()
734734
{
735-
if (_requestRejected || _applicationException != null)
735+
if (_requestRejectedException != null || _applicationException != null)
736736
{
737737
if (HasResponseStarted)
738738
{
@@ -741,8 +741,13 @@ protected Task ProduceEnd()
741741
return TaskCache.CompletedTask;
742742
}
743743

744-
// If the request was rejected, the error state has already been set by SetBadRequestState
745-
if (!_requestRejected)
744+
// If the request was rejected, the error state has already been set by SetBadRequestState and
745+
// that should take precedence.
746+
if (_requestRejectedException != null)
747+
{
748+
SetErrorResponseHeaders(statusCode: _requestRejectedException.StatusCode);
749+
}
750+
else
746751
{
747752
// 500 Internal Server Error
748753
SetErrorResponseHeaders(statusCode: 500);
@@ -1394,24 +1399,19 @@ private void ThrowResponseAbortedException()
13941399
_applicationException);
13951400
}
13961401

1397-
public void RejectRequest(string message)
1402+
public void RejectRequest(RequestRejectionReason reason)
13981403
{
1399-
throw new ObjectDisposedException(
1400-
"The response has been aborted due to an unhandled application exception.",
1401-
_applicationException);
1404+
RejectRequest(BadHttpRequestException.GetException(reason));
14021405
}
14031406

1404-
public void RejectRequest(RequestRejectionReason reason)
1407+
public void RejectRequest(RequestRejectionReason reason, string value)
14051408
{
1406-
var ex = BadHttpRequestException.GetException(reason);
1407-
SetBadRequestState(ex);
1408-
throw ex;
1409+
RejectRequest(BadHttpRequestException.GetException(reason, value));
14091410
}
14101411

1411-
public void RejectRequest(RequestRejectionReason reason, string value)
1412+
private void RejectRequest(BadHttpRequestException ex)
14121413
{
1413-
var ex = BadHttpRequestException.GetException(reason, value);
1414-
SetBadRequestState(ex);
1414+
Log.ConnectionBadRequest(ConnectionId, ex);
14151415
throw ex;
14161416
}
14171417

@@ -1422,17 +1422,14 @@ public void SetBadRequestState(RequestRejectionReason reason)
14221422

14231423
public void SetBadRequestState(BadHttpRequestException ex)
14241424
{
1425-
// Setting status code will throw if response has already started
14261425
if (!HasResponseStarted)
14271426
{
1428-
SetErrorResponseHeaders(statusCode: ex.StatusCode);
1427+
SetErrorResponseHeaders(ex.StatusCode);
14291428
}
14301429

14311430
_keepAlive = false;
14321431
_requestProcessingStopping = true;
1433-
_requestRejected = true;
1434-
1435-
Log.ConnectionBadRequest(ConnectionId, ex);
1432+
_requestRejectedException = ex;
14361433
}
14371434

14381435
protected void ReportApplicationError(Exception ex)

‎src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs

Lines changed: 36 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,11 @@ public override async Task RequestProcessingAsync()
100100
catch (Exception ex)
101101
{
102102
ReportApplicationError(ex);
103+
104+
if (ex is BadHttpRequestException)
105+
{
106+
throw;
107+
}
103108
}
104109
finally
105110
{
@@ -118,31 +123,38 @@ public override async Task RequestProcessingAsync()
118123
{
119124
await FireOnCompleted();
120125
}
126+
}
121127

122-
// If _requestAbort is set, the connection has already been closed.
123-
if (Volatile.Read(ref _requestAborted) == 0)
124-
{
125-
ResumeStreams();
126-
127-
if (_keepAlive)
128-
{
129-
// Finish reading the request body in case the app did not.
130-
await messageBody.Consume();
131-
}
132-
133-
// ProduceEnd() must be called before _application.DisposeContext(), to ensure
134-
// HttpContext.Response.StatusCode is correctly set when
135-
// IHttpContextFactory.Dispose(HttpContext) is called.
136-
await ProduceEnd();
137-
}
138-
else if (!HasResponseStarted)
128+
// If _requestAbort is set, the connection has already been closed.
129+
if (Volatile.Read(ref _requestAborted) == 0)
130+
{
131+
ResumeStreams();
132+
133+
if (_keepAlive)
139134
{
140-
// If the request was aborted and no response was sent, there's no
141-
// meaningful status code to log.
142-
StatusCode = 0;
135+
// Finish reading the request body in case the app did not.
136+
await messageBody.Consume();
143137
}
138+
139+
// ProduceEnd() must be called before _application.DisposeContext(), to ensure
140+
// HttpContext.Response.StatusCode is correctly set when
141+
// IHttpContextFactory.Dispose(HttpContext) is called.
142+
await ProduceEnd();
143+
}
144+
else if (!HasResponseStarted)
145+
{
146+
// If the request was aborted and no response was sent, there's no
147+
// meaningful status code to log.
148+
StatusCode = 0;
144149
}
145150
}
151+
catch (BadHttpRequestException ex)
152+
{
153+
// Handle BadHttpRequestException thrown during app execution or remaining message body consumption.
154+
// This has to be caught here so StatusCode is set properly before disposing the HttpContext
155+
// (DisposeContext logs StatusCode).
156+
SetBadRequestState(ex);
157+
}
146158
finally
147159
{
148160
_application.DisposeContext(context, _applicationException);
@@ -169,11 +181,9 @@ public override async Task RequestProcessingAsync()
169181
}
170182
catch (BadHttpRequestException ex)
171183
{
172-
if (!_requestRejected)
173-
{
174-
// SetBadRequestState logs the error.
175-
SetBadRequestState(ex);
176-
}
184+
// Handle BadHttpRequestException thrown during request line or header parsing.
185+
// SetBadRequestState logs the error.
186+
SetBadRequestState(ex);
177187
}
178188
catch (Exception ex)
179189
{
@@ -183,11 +193,10 @@ public override async Task RequestProcessingAsync()
183193
{
184194
try
185195
{
186-
await TryProduceInvalidRequestResponse();
187-
188196
// If _requestAborted is set, the connection has already been closed.
189197
if (Volatile.Read(ref _requestAborted) == 0)
190198
{
199+
await TryProduceInvalidRequestResponse();
191200
ConnectionControl.End(ProduceEndType.SocketShutdown);
192201
}
193202
}

‎test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs

Lines changed: 76 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5-
using System.IO;
65
using System.Linq;
76
using System.Net;
87
using System.Net.Http;
@@ -239,6 +238,38 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformed()
239238
sendMalformedRequest: true);
240239
}
241240

241+
[Fact]
242+
public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedRead()
243+
{
244+
return ResponseStatusCodeSetBeforeHttpContextDispose(
245+
async context =>
246+
{
247+
await context.Request.Body.ReadAsync(new byte[1], 0, 1);
248+
},
249+
expectedClientStatusCode: null,
250+
expectedServerStatusCode: HttpStatusCode.BadRequest,
251+
sendMalformedRequest: true);
252+
}
253+
254+
[Fact]
255+
public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIgnored()
256+
{
257+
return ResponseStatusCodeSetBeforeHttpContextDispose(
258+
async context =>
259+
{
260+
try
261+
{
262+
await context.Request.Body.ReadAsync(new byte[1], 0, 1);
263+
}
264+
catch (BadHttpRequestException)
265+
{
266+
}
267+
},
268+
expectedClientStatusCode: null,
269+
expectedServerStatusCode: HttpStatusCode.BadRequest,
270+
sendMalformedRequest: true);
271+
}
272+
242273
private static async Task ResponseStatusCodeSetBeforeHttpContextDispose(
243274
RequestDelegate handler,
244275
HttpStatusCode? expectedClientStatusCode,
@@ -730,14 +761,11 @@ await connection.Receive(
730761
[Fact]
731762
public async Task HeadResponseCanContainContentLengthHeader()
732763
{
733-
var testLogger = new TestApplicationErrorLogger();
734-
var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) };
735-
736764
using (var server = new TestServer(httpContext =>
737765
{
738766
httpContext.Response.ContentLength = 42;
739767
return TaskCache.CompletedTask;
740-
}, serviceContext))
768+
}, new TestServiceContext()))
741769
{
742770
using (var connection = server.CreateConnection())
743771
{
@@ -746,7 +774,7 @@ await connection.SendEnd(
746774
"",
747775
"");
748776
await connection.ReceiveEnd(
749-
$"HTTP/1.1 200 OK",
777+
"HTTP/1.1 200 OK",
750778
$"Date: {server.Context.DateHeaderValue}",
751779
"Content-Length: 42",
752780
"",
@@ -758,14 +786,11 @@ await connection.ReceiveEnd(
758786
[Fact]
759787
public async Task HeadResponseCanContainContentLengthHeaderButBodyNotWritten()
760788
{
761-
var testLogger = new TestApplicationErrorLogger();
762-
var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) };
763-
764789
using (var server = new TestServer(async httpContext =>
765790
{
766791
httpContext.Response.ContentLength = 12;
767792
await httpContext.Response.WriteAsync("hello, world");
768-
}, serviceContext))
793+
}, new TestServiceContext()))
769794
{
770795
using (var connection = server.CreateConnection())
771796
{
@@ -774,7 +799,7 @@ await connection.SendEnd(
774799
"",
775800
"");
776801
await connection.ReceiveEnd(
777-
$"HTTP/1.1 200 OK",
802+
"HTTP/1.1 200 OK",
778803
$"Date: {server.Context.DateHeaderValue}",
779804
"Content-Length: 12",
780805
"",
@@ -783,6 +808,46 @@ await connection.ReceiveEnd(
783808
}
784809
}
785810

811+
[Fact]
812+
public async Task AppCanWriteOwnBadRequestResponse()
813+
{
814+
var expectedResponse = string.Empty;
815+
var responseWrittenTcs = new TaskCompletionSource<object>();
816+
817+
using (var server = new TestServer(async httpContext =>
818+
{
819+
try
820+
{
821+
await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1);
822+
}
823+
catch (BadHttpRequestException ex)
824+
{
825+
expectedResponse = ex.Message;
826+
httpContext.Response.StatusCode = 400;
827+
httpContext.Response.ContentLength = ex.Message.Length;
828+
await httpContext.Response.WriteAsync(ex.Message);
829+
responseWrittenTcs.SetResult(null);
830+
}
831+
}, new TestServiceContext()))
832+
{
833+
using (var connection = server.CreateConnection())
834+
{
835+
await connection.SendEnd(
836+
"POST / HTTP/1.1",
837+
"Transfer-Encoding: chunked",
838+
"",
839+
"bad");
840+
await responseWrittenTcs.Task;
841+
await connection.ReceiveEnd(
842+
"HTTP/1.1 400 Bad Request",
843+
$"Date: {server.Context.DateHeaderValue}",
844+
$"Content-Length: {expectedResponse.Length}",
845+
"",
846+
expectedResponse);
847+
}
848+
}
849+
}
850+
786851
public static TheoryData<string, StringValues, string> NullHeaderData
787852
{
788853
get

0 commit comments

Comments
 (0)