Skip to content

Commit

Permalink
Skip empty lines between requests. (dotnet#43228)
Browse files Browse the repository at this point in the history
  • Loading branch information
@adityamandaleeka
adityamandaleeka authored Aug 12, 2022
1 parent b16f634 commit 38ebb51
Show file tree
Hide file tree
Showing 4 changed files with 194 additions and 57 deletions.
9 changes: 9 additions & 0 deletions src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ internal partial class Http1Connection : HttpProtocol, IRequestProcessor, IHttpO
{
internal static ReadOnlySpan<byte> Http2GoAwayHttp11RequiredBytes => new byte[17] { 0, 0, 8, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 13 };

private const byte ByteCR = (byte)'\r';
private const byte ByteLF = (byte)'\n';
private const byte ByteAsterisk = (byte)'*';
private const byte ByteForwardSlash = (byte)'/';
private const string Asterisk = "*";
Expand Down Expand Up @@ -151,6 +153,13 @@ 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;
Expand Down
8 changes: 0 additions & 8 deletions src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@ public HttpParser(bool showErrorDetails)
/// </summary>
public bool ParseRequestLine(TRequestHandler handler, ref SequenceReader<byte> reader)
{
// Skip any leading \r or \n on the request line. This is not technically allowed,
// but apparently there are enough clients relying on this that it's worth allowing.
// 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.TryReadTo(out ReadOnlySpan<byte> requestLine, ByteLF, advancePastDelimiter: true))
{
ParseRequestLine(handler, requestLine);
Expand Down
49 changes: 0 additions & 49 deletions src/Servers/Kestrel/Core/test/StartLineTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -517,55 +517,6 @@ public void AuthorityForms(string rawTarget, string path, string query)
DifferentFormsWorkTogether();
}

public static IEnumerable<object[]> GetCrLfAndMethodCombinations()
{
// HTTP methods to test
var methods = new string[] {
HttpMethods.Connect,
HttpMethods.Delete,
HttpMethods.Get,
HttpMethods.Head,
HttpMethods.Options,
HttpMethods.Patch,
HttpMethods.Post,
HttpMethods.Put,
HttpMethods.Trace
};

// Prefixes to test
var crLfPrefixes = new string[] {
"\r",
"\n",
"\r\r\r\r\r",
"\r\n",
"\n\r"
};

foreach (var method in methods)
{
foreach (var prefix in crLfPrefixes)
{
yield return new object[] { prefix, method };
}
}
}

[Theory]
[MemberData(nameof(GetCrLfAndMethodCombinations))]
public void LeadingCrLfAreAllowed(string startOfRequestLine, string httpMethod)
{
var rawTarget = "http://localhost/path1?q=123&w=xyzw";
Http1Connection.Reset();
// RawTarget, Path, QueryString are null after reset
Assert.Null(Http1Connection.RawTarget);
Assert.Null(Http1Connection.Path);
Assert.Null(Http1Connection.QueryString);

var ros = new ReadOnlySequence<byte>(Encoding.ASCII.GetBytes($"{startOfRequestLine}{httpMethod} {rawTarget} HTTP/1.1\r\n"));
var reader = new SequenceReader<byte>(ros);
Assert.True(Parser.ParseRequestLine(ParsingHandler, ref reader));
}

public StartLineTests()
{
MemoryPool = PinnedBlockMemoryPoolFactory.Create();
Expand Down
185 changes: 185 additions & 0 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
using Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests.TestTransport;
using Microsoft.AspNetCore.Testing;
using Microsoft.AspNetCore.WebUtilities;
using Microsoft.Extensions.Logging;
using Moq;
using Xunit;
Expand Down Expand Up @@ -293,6 +294,190 @@ private async Task TestBadRequest(string request, string expectedResponseStatusC
Assert.Contains(expectedExceptionMessage, exceptionString);
}

[Theory]
[InlineData("\r")]
[InlineData("\n")]
[InlineData("\r\n")]
[InlineData("\n\r")]
[InlineData("\r\n\r\n")]
[InlineData("\r\r\r\r\r")]
public async Task ExtraLinesBetweenRequestsIgnored(string extraLines)
{
BadHttpRequestException loggedException = null;

TestSink.MessageLogged += context =>
{
if (context.EventId.Name == "ConnectionBadRequest" && context.Exception is BadHttpRequestException ex)
{
loggedException = ex;
}
};

// Set up a listener to catch the BadRequest event
var diagListener = new DiagnosticListener("NotBadRequestTestsDiagListener");
var badRequestEventListener = new BadRequestEventListener(diagListener, (pair) => { });

await using (var server = new TestServer(context => context.Request.Body.DrainAsync(default), new TestServiceContext(LoggerFactory) { DiagnosticSource = diagListener }))
{
using (var connection = server.CreateConnection())
{
await connection.SendAll(
"POST / HTTP/1.1",
"Host:",
"Content-Length: 5",
"",
"funny",
extraLines);

await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 0",
$"Date: {server.Context.DateHeaderValue}",
"",
"");

await connection.SendAll(
"POST / HTTP/1.1",
"Host:",
"Content-Length: 5",
"",
"funny");

await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 0",
$"Date: {server.Context.DateHeaderValue}",
"",
"");

connection.ShutdownSend();

await connection.ReceiveEnd();
}
}

Assert.Null(loggedException);
// Verify DiagnosticSource event for bad request
Assert.False(badRequestEventListener.EventFired);
}

[Fact]
public async Task ExtraLinesIgnoredBetweenAdjacentRequests()
{
BadHttpRequestException loggedException = null;

TestSink.MessageLogged += context =>
{
if (context.EventId.Name == "ConnectionBadRequest" && context.Exception is BadHttpRequestException ex)
{
loggedException = ex;
}
};

// Set up a listener to catch the BadRequest event
var diagListener = new DiagnosticListener("NotBadRequestTestsDiagListener");
var badRequestEventListener = new BadRequestEventListener(diagListener, (pair) => { });

await using (var server = new TestServer(context => context.Request.Body.DrainAsync(default), new TestServiceContext(LoggerFactory) { DiagnosticSource = diagListener }))
{
using (var connection = server.CreateConnection())
{
await connection.SendAll(
"POST / HTTP/1.1",
"Host:",
"Content-Length: 5",
"",
"funny",
"",
"",
"",
"POST /"); // Split the request line

await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 0",
$"Date: {server.Context.DateHeaderValue}",
"",
"");

await connection.SendAll(
" HTTP/1.1",
"Host:",
"Content-Length: 5",
"",
"funny");

await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 0",
$"Date: {server.Context.DateHeaderValue}",
"",
"");

connection.ShutdownSend();

await connection.ReceiveEnd();
}
}

Assert.Null(loggedException);
// Verify DiagnosticSource event for bad request
Assert.False(badRequestEventListener.EventFired);
}

[Theory]
[InlineData("\r")]
[InlineData("\n")]
[InlineData("\r\n")]
[InlineData("\n\r")]
[InlineData("\r\n\r\n")]
[InlineData("\r\r\r\r\r")]
public async Task ExtraLinesAtEndOfConnectionIgnored(string extraLines)
{
BadHttpRequestException loggedException = null;

TestSink.MessageLogged += context =>
{
if (context.EventId.Name == "ConnectionBadRequest" && context.Exception is BadHttpRequestException ex)
{
loggedException = ex;
}
};

// Set up a listener to catch the BadRequest event
var diagListener = new DiagnosticListener("NotBadRequestTestsDiagListener");
var badRequestEventListener = new BadRequestEventListener(diagListener, (pair) => { });

await using (var server = new TestServer(context => context.Request.Body.DrainAsync(default), new TestServiceContext(LoggerFactory) { DiagnosticSource = diagListener }))
{
using (var connection = server.CreateConnection())
{
await connection.SendAll(
"POST / HTTP/1.1",
"Host:",
"Content-Length: 5",
"",
"funny",
extraLines);

await connection.Receive(
"HTTP/1.1 200 OK",
"Content-Length: 0",
$"Date: {server.Context.DateHeaderValue}",
"",
"");

connection.ShutdownSend();

await connection.ReceiveEnd();
}
}

Assert.Null(loggedException);
// Verify DiagnosticSource event for bad request
Assert.False(badRequestEventListener.EventFired);
}

private async Task ReceiveBadRequestResponse(InMemoryConnection connection, string expectedResponseStatusCode, string expectedDateHeaderValue, string expectedAllowHeader = null)
{
var lines = new[]
Expand Down

0 comments on commit 38ebb51

Please sign in to comment.