Skip to content

Add non-throwing parser path for bad request handling #65256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adityamandaleeka merged 11 commits into from
Feb 5, 2026
Merged

Add non-throwing parser path for bad request handling #65256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a099102
Add non-throwing parser path for bad request handling.
adityamandaleeka Jan 25, 2026
9b33013
Fix detail logging.
adityamandaleeka Jan 29, 2026
4aa24e4
Add non-throwing multi-span header parsing path.
adityamandaleeka Jan 30, 2026
f76d662
Metrics parity.
adityamandaleeka Feb 3, 2026
e081175
Use finally for buffer cleanup.
adityamandaleeka Feb 3, 2026
42185f8
Renames.
adityamandaleeka Feb 3, 2026
f988567
Remove dead code.
adityamandaleeka Feb 4, 2026
ededf0b
wip
adityamandaleeka Feb 4, 2026
7adb422
fb
adityamandaleeka Feb 5, 2026
d82f6f0
fb
adityamandaleeka Feb 5, 2026
447e840
fb
adityamandaleeka Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Servers/Kestrel/Core/src/CoreStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@
<data name="BadRequest_InvalidRequestHeadersNoCRLF" xml:space="preserve">
<value>Invalid request headers: missing final CRLF in header fields.</value>
</data>
<data name="BadRequest_InvalidRequestHeader" xml:space="preserve">
<value>Invalid request header.</value>
</data>
<data name="BadRequest_InvalidRequestHeader_Detail" xml:space="preserve">
<value>Invalid request header: '{detail}'</value>
</data>
Expand Down Expand Up @@ -193,6 +196,9 @@
<value>Unexpected end of request content.</value>
</data>
<data name="BadRequest_UnrecognizedHTTPVersion" xml:space="preserve">
<value>Unrecognized HTTP version.</value>
</data>
<data name="BadRequest_UnrecognizedHTTPVersion_Detail" xml:space="preserve">
<value>Unrecognized HTTP version: '{detail}'</value>
</data>
<data name="FallbackToIPv4Any" xml:space="preserve">
Expand Down
267 changes: 206 additions & 61 deletions src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Internal;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;

Expand All @@ -27,6 +28,7 @@ internal partial class Http1Connection : HttpProtocol, IRequestProcessor, IHttpO
private readonly HttpConnectionContext _context;
private readonly IHttpParser<Http1ParsingHandler> _parser;
private readonly Http1OutputProducer _http1Output;
private readonly bool _showErrorDetails;

private volatile bool _requestTimedOut;
private uint _requestCount;
Expand All @@ -52,6 +54,7 @@ public Http1Connection(HttpConnectionContext context)

_context = context;
_parser = ServiceContext.HttpParser;
_showErrorDetails = Log.IsEnabled(LogLevel.Information);

_http1Output = new Http1OutputProducer(
_context.Transport.Output,
Expand Down Expand Up @@ -164,51 +167,6 @@ public void HandleReadDataRateTimeout()
SendTimeoutResponse();
}

public bool ParseRequest(ref SequenceReader<byte> reader)
{
switch (_requestProcessingStatus)
{
case RequestProcessingStatus.RequestPending:
// Skip any empty lines (\r or \n) between requests.
// Peek first as a minor performance optimization; it's a quick inlined check.
if (reader.TryPeek(out byte b) && (b == ByteCR || b == ByteLF))
{
reader.AdvancePastAny(ByteCR, ByteLF);
}

if (reader.End)
{
break;
}

TimeoutControl.ResetTimeout(ServerOptions.Limits.RequestHeadersTimeout, TimeoutReason.RequestHeaders);

_requestProcessingStatus = RequestProcessingStatus.ParsingRequestLine;
goto case RequestProcessingStatus.ParsingRequestLine;
case RequestProcessingStatus.ParsingRequestLine:
if (TakeStartLine(ref reader))
{
_requestProcessingStatus = RequestProcessingStatus.ParsingHeaders;
goto case RequestProcessingStatus.ParsingHeaders;
}
else
{
break;
}
case RequestProcessingStatus.ParsingHeaders:
if (TakeMessageHeaders(ref reader, trailers: false))
{
_requestProcessingStatus = RequestProcessingStatus.AppStarted;
// Consumed preamble
return true;
}
break;
}

// Haven't completed consuming preamble
return false;
}

public bool TakeStartLine(ref SequenceReader<byte> reader)
Copy link
Member

@BrennanConroy BrennanConroy Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete method? Also delete TakeMessageHeaders? (will need to update Http1ChunkedEncodingMessageBody)

Copy link
Member Author

@adityamandaleeka adityamandaleeka Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do this as followup once this PR is in.

{
// Make sure the buffer is limited
Expand Down Expand Up @@ -287,6 +245,130 @@ bool TrimAndTakeMessageHeaders(ref SequenceReader<byte> reader, bool trailers)
}
}

/// <summary>
/// Non-throwing version of ParseRequest. Returns HttpParseResult instead of throwing.
/// </summary>
private HttpParseResult TryParseRequestCore(ref SequenceReader<byte> reader)
{
switch (_requestProcessingStatus)
{
case RequestProcessingStatus.RequestPending:
// Skip any empty lines (\r or \n) between requests.
// Peek first as a minor performance optimization; it's a quick inlined check.
if (reader.TryPeek(out byte b) && (b == ByteCR || b == ByteLF))
{
reader.AdvancePastAny(ByteCR, ByteLF);
}

if (reader.End)
{
return HttpParseResult.Incomplete;
}

TimeoutControl.ResetTimeout(ServerOptions.Limits.RequestHeadersTimeout, TimeoutReason.RequestHeaders);
_requestProcessingStatus = RequestProcessingStatus.ParsingRequestLine;
goto case RequestProcessingStatus.ParsingRequestLine;

case RequestProcessingStatus.ParsingRequestLine:
var startLineResult = TryTakeStartLineCore(ref reader);
if (startLineResult.HasError)
{
return startLineResult;
}
if (startLineResult.IsComplete)
{
_requestProcessingStatus = RequestProcessingStatus.ParsingHeaders;
goto case RequestProcessingStatus.ParsingHeaders;
}
return HttpParseResult.Incomplete;

case RequestProcessingStatus.ParsingHeaders:
var headersResult = TryTakeMessageHeadersCore(ref reader, trailers: false);
if (headersResult.HasError)
{
return headersResult;
}
if (headersResult.IsComplete)
{
_requestProcessingStatus = RequestProcessingStatus.AppStarted;
return HttpParseResult.Complete;
}
return HttpParseResult.Incomplete;
}

return HttpParseResult.Incomplete;
}

private HttpParseResult TryTakeStartLineCore(ref SequenceReader<byte> reader)
{
if (reader.Remaining >= ServerOptions.Limits.MaxRequestLineSize)
{
return TryTrimAndTakeStartLineCore(ref reader);
}

return _parser.TryParseRequestLine(new Http1ParsingHandler(this), ref reader);

HttpParseResult TryTrimAndTakeStartLineCore(ref SequenceReader<byte> reader)
{
var trimmedBuffer = reader.Sequence.Slice(reader.Position, ServerOptions.Limits.MaxRequestLineSize);
var trimmedReader = new SequenceReader<byte>(trimmedBuffer);

var result = _parser.TryParseRequestLine(new Http1ParsingHandler(this), ref trimmedReader);
if (result.HasError)
{
return result;
}
if (!result.IsComplete)
{
return HttpParseResult.Error(RequestRejectionReason.RequestLineTooLong);
}

reader.Advance(trimmedReader.Consumed);
return HttpParseResult.Complete;
}
}

private HttpParseResult TryTakeMessageHeadersCore(ref SequenceReader<byte> reader, bool trailers)
{
if (reader.Remaining > _remainingRequestHeadersBytesAllowed)
{
return TryTrimAndTakeMessageHeadersCore(ref reader, trailers);
}

var alreadyConsumed = reader.Consumed;
var result = _parser.TryParseHeaders(new Http1ParsingHandler(this, trailers), ref reader);
_remainingRequestHeadersBytesAllowed -= reader.Consumed - alreadyConsumed;
Copy link
Member

@BrennanConroy BrennanConroy Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous code put this in a finally for some reason. I think all cases where TryParseHeaders throws we would mark endConnection and so wouldn't care about the value of _remainingRequestHeadersBytesAllowed anymore. Just want to call this out in case I'm wrong.

Copy link
Member Author

@adityamandaleeka adityamandaleeka Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. When an error occurs, endConnection is set to true and the connection is closed, so the value of _remainingRequestHeadersBytesAllowed doesn't matter after that point.


if (result.IsComplete)
{
TimeoutControl.CancelTimeout();
}

return result;

HttpParseResult TryTrimAndTakeMessageHeadersCore(ref SequenceReader<byte> reader, bool trailers)
{
var trimmedBuffer = reader.Sequence.Slice(reader.Position, _remainingRequestHeadersBytesAllowed);
var trimmedReader = new SequenceReader<byte>(trimmedBuffer);

var result = _parser.TryParseHeaders(new Http1ParsingHandler(this, trailers), ref trimmedReader);
_remainingRequestHeadersBytesAllowed -= trimmedReader.Consumed;

if (result.HasError)
{
return result;
}
if (!result.IsComplete)
{
return HttpParseResult.Error(RequestRejectionReason.HeadersExceedMaxTotalSize);
}

TimeoutControl.CancelTimeout();
reader.Advance(trimmedReader.Consumed);
return HttpParseResult.Complete;
}
}

public void OnStartLine(HttpVersionAndMethod versionAndMethod, TargetOffsetPathLength targetPath, Span<byte> startLine)
{
// Null characters are not allowed and should have been checked by HttpParser before calling this method
Expand Down Expand Up @@ -722,29 +804,40 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio
{
var reader = new SequenceReader<byte>(result.Buffer);
var isConsumed = false;

// The parser itself doesn't throw, but handler callbacks (OnStartLine, OnHeader)
// can still throw BadHttpRequestException for semantic errors (e.g., invalid request target).
// We catch those here to ensure metrics are recorded via OnBadRequest.
HttpParseResult parseResult;
try
{
isConsumed = ParseRequest(ref reader);
}
catch (InvalidOperationException) when (_requestProcessingStatus == RequestProcessingStatus.ParsingHeaders)
{
KestrelBadHttpRequestException.Throw(RequestRejectionReason.MalformedRequestInvalidHeaders);
throw;
parseResult = TryParseRequestCore(ref reader);

// Handle parse errors without exceptions
if (parseResult.HasError)
{
// Create exception and call HandleBadRequest BEFORE finally runs AdvanceTo,
// as that may invalidate the buffer
var ex = CreateBadRequestException(parseResult, result.Buffer);
HandleBadRequest(result.Buffer, ex);
endConnection = true;
return true;
}

isConsumed = parseResult.IsComplete;
}
#pragma warning disable CS0618 // Type or member is obsolete
catch (BadHttpRequestException ex)
{
OnBadRequest(result.Buffer, ex);

// Avoid re-throwing - handle the bad request state directly here
// to eliminate exception propagation overhead through async state machines
SetBadRequestState(ex);
// HandleBadRequest must be called before finally runs AdvanceTo
HandleBadRequest(result.Buffer, ex);
endConnection = true;
return true;
}
#pragma warning restore CS0618 // Type or member is obsolete
catch (Exception)
{
// Record OtherError for unexpected exceptions that escape the parser
KestrelMetrics.AddConnectionEndReason(MetricsContext, ConnectionEndReason.OtherError);
throw;
Copy link
Member

@BrennanConroy BrennanConroy Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also set the bad request state, end connection, then return instead of throwing?

Copy link
Member Author

@adityamandaleeka adityamandaleeka Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe... but I think we should keep this as a rethrow. This catch block is for truly unexpected exceptions (OOM, etc.) which we can't "handle" and send a bad request exception, or InvalidOperationException from invalid header characters (null/CR/LF). The outer catch in ProcessRequestsAsync will log them appropriately. We could potentially convert InvalidOperationException to a bad request, but that would be a behavioral change - the current behavior of logging and closing the connection is reasonable.

}
Expand All @@ -761,11 +854,13 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio
endConnection = true;
return true;
case RequestProcessingStatus.ParsingRequestLine:
KestrelBadHttpRequestException.Throw(RequestRejectionReason.InvalidRequestLine);
break;
HandleBadRequest(result.Buffer, RequestRejectionReason.InvalidRequestLine);
endConnection = true;
return true;
case RequestProcessingStatus.ParsingHeaders:
KestrelBadHttpRequestException.Throw(RequestRejectionReason.MalformedRequestInvalidHeaders);
break;
HandleBadRequest(result.Buffer, RequestRejectionReason.MalformedRequestInvalidHeaders);
endConnection = true;
return true;
}
}
else if (!_keepAlive && _requestProcessingStatus == RequestProcessingStatus.RequestPending)
Expand All @@ -779,7 +874,9 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio
{
// In this case, there is an ongoing request but the start line/header parsing has timed out, so send
// a 408 response.
KestrelBadHttpRequestException.Throw(RequestRejectionReason.RequestHeadersTimeout);
HandleBadRequest(result.Buffer, RequestRejectionReason.RequestHeadersTimeout);
endConnection = true;
return true;
}

endConnection = false;
Expand Down Expand Up @@ -836,6 +933,54 @@ internal static ConnectionEndReason GetConnectionEndReason(Microsoft.AspNetCore.
}
}

#pragma warning disable CS0618 // Type or member is obsolete
/// <summary>
/// Helper method to handle bad request: records metrics, sets bad request state, and ends the connection.
/// </summary>
private void HandleBadRequest(ReadOnlySequence<byte> requestData, BadHttpRequestException ex)
{
OnBadRequest(requestData, ex);
SetBadRequestState(ex);
}

/// <summary>
/// Helper method to handle bad request by rejection reason: creates exception, records metrics, sets bad request state.
/// </summary>
private void HandleBadRequest(ReadOnlySequence<byte> requestData, RequestRejectionReason reason)
{
var ex = KestrelBadHttpRequestException.GetException(reason);
HandleBadRequest(requestData, ex);
}
#pragma warning restore CS0618 // Type or member is obsolete

#pragma warning disable CS0618 // Type or member is obsolete
/// <summary>
/// Creates a BadHttpRequestException with error detail extracted from the buffer.
/// </summary>
private BadHttpRequestException CreateBadRequestException(HttpParseResult parseResult, ReadOnlySequence<byte> buffer)
{
// Some error reasons don't use detail
if (parseResult.ErrorReason == RequestRejectionReason.InvalidRequestHeadersNoCRLF)
{
return KestrelBadHttpRequestException.GetException(parseResult.ErrorReason);
}

// Extract error detail from buffer if available, but only when _showErrorDetails is enabled
// to avoid leaking internal details in production.
if (_showErrorDetails &&
parseResult.ErrorLength > 0 &&
parseResult.ErrorOffset + parseResult.ErrorLength <= buffer.Length)
{
var errorSlice = buffer.Slice(parseResult.ErrorOffset, parseResult.ErrorLength);
var errorBytes = errorSlice.IsSingleSegment ? errorSlice.FirstSpan : errorSlice.ToArray();
var detail = errorBytes.GetAsciiStringEscaped(Constants.MaxExceptionDetailSize);
return KestrelBadHttpRequestException.GetException(parseResult.ErrorReason, detail);
}

return KestrelBadHttpRequestException.GetException(parseResult.ErrorReason);
}
#pragma warning restore CS0618 // Type or member is obsolete

#pragma warning disable CS0618 // Type or member is obsolete
private void OnBadRequest(ReadOnlySequence<byte> requestData, BadHttpRequestException ex)
#pragma warning restore CS0618 // Type or member is obsolete
Expand Down
Loading
Loading