Skip to content

Commit a5d5f16

Browse files
adityamandaleekaadityamandaleeka
authored
[6.0] Skip empty lines between requests (#43229)
* [6.0] Skip empty lines between requests. * Add test case.
1 parent 4bcc04f commit a5d5f16

File tree

4 files changed

+195
-58
lines changed

4 files changed

+195
-58
lines changed

‎src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
1616
{
1717
internal partial class Http1Connection : HttpProtocol, IRequestProcessor, IHttpOutputAborter
1818
{
19+
private const byte ByteCR = (byte)'\r';
20+
private const byte ByteLF = (byte)'\n';
1921
private const byte ByteAsterisk = (byte)'*';
2022
private const byte ByteForwardSlash = (byte)'/';
2123
private const string Asterisk = "*";
@@ -146,6 +148,13 @@ public bool ParseRequest(ref SequenceReader<byte> reader)
146148
switch (_requestProcessingStatus)
147149
{
148150
case RequestProcessingStatus.RequestPending:
151+
// Skip any empty lines (\r or \n) between requests.
152+
// Peek first as a minor performance optimization; it's a quick inlined check.
153+
if (reader.TryPeek(out byte b) && (b == ByteCR || b == ByteLF))
154+
{
155+
reader.AdvancePastAny(ByteCR, ByteLF);
156+
}
157+
149158
if (reader.End)
150159
{
151160
break;

‎src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,6 @@ private static bool CheckAllowSpaceAfterRequestLine()
5252

5353
public bool ParseRequestLine(TRequestHandler handler, ref SequenceReader<byte> reader)
5454
{
55-
// Skip any leading \r or \n on the request line. This is not technically allowed,
56-
// but apparently there are enough clients relying on this that it's worth allowing.
57-
// Peek first as a minor performance optimization; it's a quick inlined check.
58-
if (reader.TryPeek(out byte b) && (b == ByteCR || b == ByteLF))
59-
{
60-
reader.AdvancePastAny(ByteCR, ByteLF);
61-
}
62-
6355
if (reader.TryReadTo(out ReadOnlySpan<byte> requestLine, ByteLF, advancePastDelimiter: true))
6456
{
6557
if (_allowSpaceAfterRequestLine)

‎src/Servers/Kestrel/Core/test/StartLineTests.cs

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -517,55 +517,6 @@ public void AuthorityForms(string rawTarget, string path, string query)
517517
DifferentFormsWorkTogether();
518518
}
519519

520-
public static IEnumerable<object[]> GetCrLfAndMethodCombinations()
521-
{
522-
// HTTP methods to test
523-
var methods = new string[] {
524-
HttpMethods.Connect,
525-
HttpMethods.Delete,
526-
HttpMethods.Get,
527-
HttpMethods.Head,
528-
HttpMethods.Options,
529-
HttpMethods.Patch,
530-
HttpMethods.Post,
531-
HttpMethods.Put,
532-
HttpMethods.Trace
533-
};
534-
535-
// Prefixes to test
536-
var crLfPrefixes = new string[] {
537-
"\r",
538-
"\n",
539-
"\r\r\r\r\r",
540-
"\r\n",
541-
"\n\r"
542-
};
543-
544-
foreach (var method in methods)
545-
{
546-
foreach (var prefix in crLfPrefixes)
547-
{
548-
yield return new object[] { prefix, method };
549-
}
550-
}
551-
}
552-
553-
[Theory]
554-
[MemberData(nameof(GetCrLfAndMethodCombinations))]
555-
public void LeadingCrLfAreAllowed(string startOfRequestLine, string httpMethod)
556-
{
557-
var rawTarget = "http://localhost/path1?q=123&w=xyzw";
558-
Http1Connection.Reset();
559-
// RawTarget, Path, QueryString are null after reset
560-
Assert.Null(Http1Connection.RawTarget);
561-
Assert.Null(Http1Connection.Path);
562-
Assert.Null(Http1Connection.QueryString);
563-
564-
var ros = new ReadOnlySequence<byte>(Encoding.ASCII.GetBytes($"{startOfRequestLine}{httpMethod} {rawTarget} HTTP/1.1\r\n"));
565-
var reader = new SequenceReader<byte>(ros);
566-
Assert.True(Parser.ParseRequestLine(ParsingHandler, ref reader));
567-
}
568-
569520
public StartLineTests()
570521
{
571522
MemoryPool = PinnedBlockMemoryPoolFactory.Create();

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs

Lines changed: 186 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
88
using Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests.TestTransport;
99
using Microsoft.AspNetCore.Testing;
10+
using Microsoft.AspNetCore.WebUtilities;
1011
using Microsoft.Extensions.Logging;
1112
using Microsoft.Extensions.Primitives;
1213
using Moq;
13-
using Xunit;
1414
using BadHttpRequestException = Microsoft.AspNetCore.Http.BadHttpRequestException;
1515

1616
namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests
@@ -323,6 +323,191 @@ private async Task TestBadRequest(string request, string expectedResponseStatusC
323323
Assert.Contains(expectedExceptionMessage, exceptionString);
324324
}
325325

326+
[Theory]
327+
[InlineData("\r")]
328+
[InlineData("\n")]
329+
[InlineData("\r\n")]
330+
[InlineData("\n\r")]
331+
[InlineData("\r\n\r\n")]
332+
[InlineData("\r\r\r\r\r")]
333+
public async Task ExtraLinesBetweenRequestsIgnored(string extraLines)
334+
{
335+
BadHttpRequestException loggedException = null;
336+
337+
TestSink.MessageLogged += context =>
338+
{
339+
if (context.EventId.Name == "ConnectionBadRequest" && context.Exception is BadHttpRequestException ex)
340+
{
341+
loggedException = ex;
342+
}
343+
};
344+
345+
// Set up a listener to catch the BadRequest event
346+
var diagListener = new DiagnosticListener("NotBadRequestTestsDiagListener");
347+
var badRequestEventListener = new BadRequestEventListener(diagListener, (pair) => { });
348+
349+
await using (var server = new TestServer(context => context.Request.Body.DrainAsync(default), new TestServiceContext(LoggerFactory) { DiagnosticSource = diagListener }))
350+
{
351+
using (var connection = server.CreateConnection())
352+
{
353+
await connection.SendAll(
354+
"POST / HTTP/1.1",
355+
"Host:",
356+
"Content-Length: 5",
357+
"",
358+
"funny",
359+
extraLines);
360+
361+
await connection.Receive(
362+
"HTTP/1.1 200 OK",
363+
"Content-Length: 0",
364+
$"Date: {server.Context.DateHeaderValue}",
365+
"",
366+
"");
367+
368+
await connection.SendAll(
369+
"POST / HTTP/1.1",
370+
"Host:",
371+
"Content-Length: 5",
372+
"",
373+
"funny");
374+
375+
await connection.Receive(
376+
"HTTP/1.1 200 OK",
377+
"Content-Length: 0",
378+
$"Date: {server.Context.DateHeaderValue}",
379+
"",
380+
"");
381+
382+
connection.ShutdownSend();
383+
384+
await connection.ReceiveEnd();
385+
}
386+
}
387+
388+
Assert.Null(loggedException);
389+
// Verify DiagnosticSource event for bad request
390+
Assert.False(badRequestEventListener.EventFired);
391+
}
392+
393+
[Fact]
394+
public async Task ExtraLinesIgnoredBetweenAdjacentRequests()
395+
{
396+
BadHttpRequestException loggedException = null;
397+
398+
TestSink.MessageLogged += context =>
399+
{
400+
if (context.EventId.Name == "ConnectionBadRequest" && context.Exception is BadHttpRequestException ex)
401+
{
402+
loggedException = ex;
403+
}
404+
};
405+
406+
// Set up a listener to catch the BadRequest event
407+
var diagListener = new DiagnosticListener("NotBadRequestTestsDiagListener");
408+
var badRequestEventListener = new BadRequestEventListener(diagListener, (pair) => { });
409+
410+
await using (var server = new TestServer(context => context.Request.Body.DrainAsync(default), new TestServiceContext(LoggerFactory) { DiagnosticSource = diagListener }))
411+
{
412+
using (var connection = server.CreateConnection())
413+
{
414+
await connection.SendAll(
415+
"POST / HTTP/1.1",
416+
"Host:",
417+
"Content-Length: 5",
418+
"",
419+
"funny",
420+
"",
421+
"",
422+
"",
423+
"POST /"); // Split the request line
424+
425+
await connection.Receive(
426+
"HTTP/1.1 200 OK",
427+
"Content-Length: 0",
428+
$"Date: {server.Context.DateHeaderValue}",
429+
"",
430+
"");
431+
432+
await connection.SendAll(
433+
" HTTP/1.1",
434+
"Host:",
435+
"Content-Length: 5",
436+
"",
437+
"funny");
438+
439+
await connection.Receive(
440+
"HTTP/1.1 200 OK",
441+
"Content-Length: 0",
442+
$"Date: {server.Context.DateHeaderValue}",
443+
"",
444+
"");
445+
446+
connection.ShutdownSend();
447+
448+
await connection.ReceiveEnd();
449+
}
450+
}
451+
452+
Assert.Null(loggedException);
453+
// Verify DiagnosticSource event for bad request
454+
Assert.False(badRequestEventListener.EventFired);
455+
}
456+
457+
[Theory]
458+
[InlineData("\r")]
459+
[InlineData("\n")]
460+
[InlineData("\r\n")]
461+
[InlineData("\n\r")]
462+
[InlineData("\n\n")]
463+
[InlineData("\r\n\r\n")]
464+
[InlineData("\r\r\r\r\r")]
465+
public async Task ExtraLinesAtEndOfConnectionIgnored(string extraLines)
466+
{
467+
BadHttpRequestException loggedException = null;
468+
469+
TestSink.MessageLogged += context =>
470+
{
471+
if (context.EventId.Name == "ConnectionBadRequest" && context.Exception is BadHttpRequestException ex)
472+
{
473+
loggedException = ex;
474+
}
475+
};
476+
477+
// Set up a listener to catch the BadRequest event
478+
var diagListener = new DiagnosticListener("NotBadRequestTestsDiagListener");
479+
var badRequestEventListener = new BadRequestEventListener(diagListener, (pair) => { });
480+
481+
await using (var server = new TestServer(context => context.Request.Body.DrainAsync(default), new TestServiceContext(LoggerFactory) { DiagnosticSource = diagListener }))
482+
{
483+
using (var connection = server.CreateConnection())
484+
{
485+
await connection.SendAll(
486+
"POST / HTTP/1.1",
487+
"Host:",
488+
"Content-Length: 5",
489+
"",
490+
"funny",
491+
extraLines);
492+
493+
await connection.Receive(
494+
"HTTP/1.1 200 OK",
495+
"Content-Length: 0",
496+
$"Date: {server.Context.DateHeaderValue}",
497+
"",
498+
"");
499+
500+
connection.ShutdownSend();
501+
502+
await connection.ReceiveEnd();
503+
}
504+
}
505+
506+
Assert.Null(loggedException);
507+
// Verify DiagnosticSource event for bad request
508+
Assert.False(badRequestEventListener.EventFired);
509+
}
510+
326511
private async Task ReceiveBadRequestResponse(InMemoryConnection connection, string expectedResponseStatusCode, string expectedDateHeaderValue, string expectedAllowHeader = null)
327512
{
328513
var lines = new[]

0 commit comments

Comments
 (0)