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

Commit 15dae1a

Browse files
author
Cesar Blum Silveira
committed
Separate request rejection from bad request state setting.
1 parent a35a00f commit 15dae1a

File tree

3 files changed

+82
-48
lines changed

3 files changed

+82
-48
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: 32 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,35 @@ 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+
SetBadRequestState(ex);
154+
}
146155
finally
147156
{
148157
_application.DisposeContext(context, _applicationException);
@@ -169,11 +178,8 @@ public override async Task RequestProcessingAsync()
169178
}
170179
catch (BadHttpRequestException ex)
171180
{
172-
if (!_requestRejected)
173-
{
174-
// SetBadRequestState logs the error.
175-
SetBadRequestState(ex);
176-
}
181+
// SetBadRequestState logs the error.
182+
SetBadRequestState(ex);
177183
}
178184
catch (Exception ex)
179185
{
@@ -183,11 +189,10 @@ public override async Task RequestProcessingAsync()
183189
{
184190
try
185191
{
186-
await TryProduceInvalidRequestResponse();
187-
188192
// If _requestAborted is set, the connection has already been closed.
189193
if (Volatile.Read(ref _requestAborted) == 0)
190194
{
195+
await TryProduceInvalidRequestResponse();
191196
ConnectionControl.End(ProduceEndType.SocketShutdown);
192197
}
193198
}

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,38 @@ public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformed()
239239
sendMalformedRequest: true);
240240
}
241241

242+
[Fact]
243+
public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedRead()
244+
{
245+
return ResponseStatusCodeSetBeforeHttpContextDispose(
246+
async context =>
247+
{
248+
await context.Request.Body.ReadAsync(new byte[1], 0, 1);
249+
},
250+
expectedClientStatusCode: null,
251+
expectedServerStatusCode: HttpStatusCode.BadRequest,
252+
sendMalformedRequest: true);
253+
}
254+
255+
[Fact]
256+
public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIgnored()
257+
{
258+
return ResponseStatusCodeSetBeforeHttpContextDispose(
259+
async context =>
260+
{
261+
try
262+
{
263+
await context.Request.Body.ReadAsync(new byte[1], 0, 1);
264+
}
265+
catch (BadHttpRequestException)
266+
{
267+
}
268+
},
269+
expectedClientStatusCode: null,
270+
expectedServerStatusCode: HttpStatusCode.BadRequest,
271+
sendMalformedRequest: true);
272+
}
273+
242274
private static async Task ResponseStatusCodeSetBeforeHttpContextDispose(
243275
RequestDelegate handler,
244276
HttpStatusCode? expectedClientStatusCode,

0 commit comments

Comments
 (0)