Skip to content

Commit 8c8ee15

Browse files
author
Cesar Blum Silveira
committed
Send 'Connection: close' in all 400 responses to HTTP/1.1 requests (#840).
1 parent 3e841cc commit 8c8ee15

File tree

4 files changed

+109
-79
lines changed

4 files changed

+109
-79
lines changed

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

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public abstract partial class Frame : FrameContext, IFrameControl
3030
private static readonly byte[] _bytesConnectionClose = Encoding.ASCII.GetBytes("\r\nConnection: close");
3131
private static readonly byte[] _bytesConnectionKeepAlive = Encoding.ASCII.GetBytes("\r\nConnection: keep-alive");
3232
private static readonly byte[] _bytesTransferEncodingChunked = Encoding.ASCII.GetBytes("\r\nTransfer-Encoding: chunked");
33-
private static readonly byte[] _bytesHttpVersion1_1 = Encoding.ASCII.GetBytes("HTTP/1.1 ");
33+
private static readonly byte[] _bytesHttpVersion11 = Encoding.ASCII.GetBytes("HTTP/1.1 ");
3434
private static readonly byte[] _bytesContentLengthZero = Encoding.ASCII.GetBytes("\r\nContent-Length: 0");
3535
private static readonly byte[] _bytesSpace = Encoding.ASCII.GetBytes(" ");
3636
private static readonly byte[] _bytesEndHeaders = Encoding.ASCII.GetBytes("\r\n\r\n");
@@ -87,29 +87,30 @@ public string HttpVersion
8787
{
8888
get
8989
{
90-
if (_httpVersion == HttpVersionType.Http1_1)
90+
if (_httpVersion == HttpVersionType.Http11)
9191
{
9292
return "HTTP/1.1";
9393
}
94-
if (_httpVersion == HttpVersionType.Http1_0)
94+
if (_httpVersion == HttpVersionType.Http10)
9595
{
9696
return "HTTP/1.0";
9797
}
98+
9899
return string.Empty;
99100
}
100101
set
101102
{
102103
if (value == "HTTP/1.1")
103104
{
104-
_httpVersion = HttpVersionType.Http1_1;
105+
_httpVersion = HttpVersionType.Http11;
105106
}
106107
else if (value == "HTTP/1.0")
107108
{
108-
_httpVersion = HttpVersionType.Http1_0;
109+
_httpVersion = HttpVersionType.Http10;
109110
}
110111
else
111112
{
112-
_httpVersion = HttpVersionType.Unknown;
113+
_httpVersion = HttpVersionType.Unset;
113114
}
114115
}
115116
}
@@ -229,7 +230,7 @@ public void Reset()
229230
PathBase = null;
230231
Path = null;
231232
QueryString = null;
232-
_httpVersion = HttpVersionType.Unknown;
233+
_httpVersion = HttpVersionType.Unset;
233234
StatusCode = 200;
234235
ReasonPhrase = null;
235236

@@ -512,7 +513,7 @@ public void ProduceContinue()
512513
}
513514

514515
StringValues expect;
515-
if (_httpVersion == HttpVersionType.Http1_1 &&
516+
if (_httpVersion == HttpVersionType.Http11 &&
516517
RequestHeaders.TryGetValue("Expect", out expect) &&
517518
(expect.FirstOrDefault() ?? "").Equals("100-continue", StringComparison.OrdinalIgnoreCase))
518519
{
@@ -595,6 +596,7 @@ protected Task ProduceEnd()
595596
{
596597
// 400 Bad Request
597598
StatusCode = 400;
599+
_keepAlive = false;
598600
}
599601
else
600602
{
@@ -712,7 +714,7 @@ private void CreateResponseHeader(
712714
//
713715
// A server MUST NOT send a response containing Transfer-Encoding unless the corresponding
714716
// request indicates HTTP/1.1 (or later).
715-
if (_httpVersion == HttpVersionType.Http1_1)
717+
if (_httpVersion == HttpVersionType.Http11)
716718
{
717719
_autoChunk = true;
718720
responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked);
@@ -724,16 +726,16 @@ private void CreateResponseHeader(
724726
}
725727
}
726728

727-
if (_keepAlive == false && responseHeaders.HasConnection == false && _httpVersion == HttpVersionType.Http1_1)
729+
if (!_keepAlive && !responseHeaders.HasConnection && _httpVersion != HttpVersionType.Http10)
728730
{
729731
responseHeaders.SetRawConnection("close", _bytesConnectionClose);
730732
}
731-
else if (_keepAlive && responseHeaders.HasConnection == false && _httpVersion == HttpVersionType.Http1_0)
733+
else if (_keepAlive && !responseHeaders.HasConnection && _httpVersion == HttpVersionType.Http10)
732734
{
733735
responseHeaders.SetRawConnection("keep-alive", _bytesConnectionKeepAlive);
734736
}
735737

736-
end.CopyFrom(_bytesHttpVersion1_1);
738+
end.CopyFrom(_bytesHttpVersion11);
737739
end.CopyFrom(statusBytes);
738740
responseHeaders.CopyTo(ref end);
739741
end.CopyFrom(_bytesEndHeaders, 0, _bytesEndHeaders.Length);
@@ -838,13 +840,10 @@ protected RequestLineStatus TakeStartLine(SocketInput input)
838840
}
839841
else if (httpVersion != "HTTP/1.0" && httpVersion != "HTTP/1.1")
840842
{
841-
RejectRequest("Malformed request.");
843+
RejectRequest("Unrecognized HTTP version.");
842844
}
843845
}
844846

845-
// HttpVersion must be set here to send correct response when request is rejected
846-
HttpVersion = httpVersion;
847-
848847
scan.Take();
849848
var next = scan.Take();
850849
if (next == -1)
@@ -879,6 +878,7 @@ protected RequestLineStatus TakeStartLine(SocketInput input)
879878
Method = method;
880879
RequestUri = requestUrlPath;
881880
QueryString = queryString;
881+
HttpVersion = httpVersion;
882882

883883
bool caseMatches;
884884

@@ -1099,9 +1099,9 @@ protected void ReportApplicationError(Exception ex)
10991099

11001100
private enum HttpVersionType
11011101
{
1102-
Unknown = -1,
1103-
Http1_0 = 0,
1104-
Http1_1 = 1
1102+
Unset = -1,
1103+
Http10 = 0,
1104+
Http11 = 1
11051105
}
11061106

11071107
protected enum RequestLineStatus

‎test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs

Lines changed: 87 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,72 +1,114 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4-
using System;
54
using System.Threading.Tasks;
65
using Xunit;
76

87
namespace Microsoft.AspNetCore.Server.KestrelTests
98
{
109
public class BadHttpRequestTests
1110
{
12-
[Theory(Skip = "This test fails intermittently and needs to be investigated.")]
13-
[InlineData("/ HTTP/1.1\r\n\r\n")]
14-
[InlineData(" / HTTP/1.1\r\n\r\n")]
15-
[InlineData(" / HTTP/1.1\r\n\r\n")]
16-
[InlineData("GET / HTTP/1.1\r\n\r\n")]
17-
[InlineData("GET / HTTP/1.1\r\n\r\n")]
18-
[InlineData("GET HTTP/1.1\r\n\r\n")]
11+
// Don't send more data than necessary to fail, otherwise the test throws trying to
12+
// send data after the server has already closed the connection. This would cause the
13+
// test to fail on Windows, due to a winsock limitation: after the error when trying
14+
// to write to the socket closed by the server, winsock disposes all resources used
15+
// by that socket. The test then fails when we try to read the expected response
16+
// from the server because, although it would have been buffered, it got discarded
17+
// by winsock on the send() error.
18+
// The solution for this is for the client to always try to receive before doing
19+
// any sends, that way it can detect that the connection has been closed by the server
20+
// and not try to send() on the closed connection, triggering the error that would cause
21+
// any buffered received data to be lost.
22+
// We do not deem necessary to mitigate this issue in TestConnection, since it would only
23+
// be ensuring that we have a properly implemented HTTP client that can handle the
24+
// winsock issue. There is nothing to be verified in Kestrel in this situation.
25+
[Theory]
26+
// Incomplete request lines
27+
[InlineData("G")]
28+
[InlineData("GE")]
29+
[InlineData("GET")]
30+
[InlineData("GET ")]
1931
[InlineData("GET /")]
2032
[InlineData("GET / ")]
2133
[InlineData("GET / H")]
34+
[InlineData("GET / HT")]
35+
[InlineData("GET / HTT")]
36+
[InlineData("GET / HTTP")]
37+
[InlineData("GET / HTTP/")]
38+
[InlineData("GET / HTTP/1")]
2239
[InlineData("GET / HTTP/1.")]
40+
[InlineData("GET / HTTP/1.1")]
41+
[InlineData("GET / HTTP/1.1\r")]
42+
[InlineData("GET / HTTP/1.0")]
43+
[InlineData("GET / HTTP/1.0\r")]
44+
// Missing method
45+
[InlineData(" ")]
46+
// Missing second space
47+
[InlineData("/ HTTP/1.1\r\n\r\n")]
2348
[InlineData("GET /\r\n")]
24-
[InlineData("GET / \r\n")]
49+
// Missing target
50+
[InlineData("GET ")]
51+
// Missing version
52+
[InlineData("GET / \r")]
53+
// Missing CR
2554
[InlineData("GET / \n")]
26-
[InlineData("GET / http/1.0\r\n\r\n")]
27-
[InlineData("GET / http/1.1\r\n\r\n")]
28-
[InlineData("GET / HTTP/1.1 \r\n\r\n")]
29-
[InlineData("GET / HTTP/1.1a\r\n\r\n")]
30-
[InlineData("GET / HTTP/1.0\n\r\n")]
31-
[InlineData("GET / HTTP/3.0\r\n\r\n")]
32-
[InlineData("GET / H\r\n\r\n")]
33-
[InlineData("GET / HTTP/1.\r\n\r\n")]
34-
[InlineData("GET / hello\r\n\r\n")]
35-
[InlineData("GET / 8charact\r\n\r\n")]
36-
public async Task TestBadRequests(string request)
55+
// Unrecognized HTTP version
56+
[InlineData("GET / http/1.0\r")]
57+
[InlineData("GET / http/1.1\r")]
58+
[InlineData("GET / HTTP/1.1 \r")]
59+
[InlineData("GET / HTTP/1.1a\r")]
60+
[InlineData("GET / HTTP/1.0\n\r")]
61+
[InlineData("GET / HTTP/1.2\r")]
62+
[InlineData("GET / HTTP/3.0\r")]
63+
[InlineData("GET / H\r")]
64+
[InlineData("GET / HTTP/1.\r")]
65+
[InlineData("GET / hello\r")]
66+
[InlineData("GET / 8charact\r")]
67+
// Missing LF
68+
[InlineData("GET / HTTP/1.0\rA")]
69+
public async Task TestBadRequestLines(string request)
3770
{
3871
using (var server = new TestServer(context => { return Task.FromResult(0); }))
3972
{
4073
using (var connection = new TestConnection(server.Port))
4174
{
42-
var receiveTask = Task.Run(async () =>
43-
{
44-
await connection.Receive(
45-
"HTTP/1.1 400 Bad Request",
46-
"");
47-
await connection.ReceiveStartsWith("Date: ");
48-
await connection.ReceiveForcedEnd(
49-
"Content-Length: 0",
50-
"Server: Kestrel",
51-
"",
52-
"");
53-
});
54-
55-
try
56-
{
57-
await connection.SendEnd(request).ConfigureAwait(false);
58-
}
59-
catch (Exception)
60-
{
61-
// TestConnection.SendEnd will start throwing while sending characters
62-
// in cases where the server rejects the request as soon as it
63-
// determines the request line is malformed, even though there
64-
// are more characters following.
65-
}
75+
await connection.SendEnd(request);
76+
await ReceiveBadRequestResponse(connection);
77+
}
78+
}
79+
}
6680

67-
await receiveTask;
81+
[Theory]
82+
[InlineData(" ")]
83+
[InlineData("GET ")]
84+
[InlineData("GET / HTTP/1.2\r")]
85+
[InlineData("GET / HTTP/1.0\rA")]
86+
public async Task ServerClosesConnectionAsSoonAsBadRequestLineIsDetected(string request)
87+
{
88+
using (var server = new TestServer(context => { return Task.FromResult(0); }))
89+
{
90+
using (var connection = new TestConnection(server.Port))
91+
{
92+
await connection.Send(request);
93+
await ReceiveBadRequestResponse(connection);
6894
}
6995
}
7096
}
97+
98+
private async Task ReceiveBadRequestResponse(TestConnection connection)
99+
{
100+
await connection.Receive(
101+
"HTTP/1.1 400 Bad Request",
102+
"");
103+
await connection.Receive(
104+
"Connection: close",
105+
"");
106+
await connection.ReceiveStartsWith("Date: ");
107+
await connection.ReceiveEnd(
108+
"Content-Length: 0",
109+
"Server: Kestrel",
110+
"",
111+
"");
112+
}
71113
}
72114
}

‎test/Microsoft.AspNetCore.Server.KestrelTests/ChunkedRequestTests.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,7 @@ await connection.Send(
371371

372372
await connection.Receive(
373373
"HTTP/1.1 400 Bad Request",
374+
"Connection: close",
374375
"");
375376
await connection.ReceiveStartsWith("Date:");
376377
await connection.ReceiveForcedEnd(
@@ -415,6 +416,7 @@ await connection.Send(
415416

416417
await connection.Receive(
417418
"HTTP/1.1 400 Bad Request",
419+
"Connection: close",
418420
"");
419421
await connection.ReceiveStartsWith("Date:");
420422
await connection.ReceiveForcedEnd(

‎test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -724,21 +724,6 @@ public async Task ConnectionClosesWhenFinReceivedBeforeRequestCompletes(ServiceC
724724
{
725725
using (var server = new TestServer(AppChunked, testContext))
726726
{
727-
using (var connection = new TestConnection(server.Port))
728-
{
729-
await connection.SendEnd(
730-
"GET /");
731-
await connection.Receive(
732-
"HTTP/1.1 400 Bad Request",
733-
"");
734-
await connection.ReceiveStartsWith("Date:");
735-
await connection.ReceiveForcedEnd(
736-
"Content-Length: 0",
737-
"Server: Kestrel",
738-
"",
739-
"");
740-
}
741-
742727
using (var connection = new TestConnection(server.Port))
743728
{
744729
await connection.SendEnd(
@@ -750,6 +735,7 @@ await connection.Receive(
750735
"Content-Length: 0",
751736
"",
752737
"HTTP/1.1 400 Bad Request",
738+
"Connection: close",
753739
"");
754740
await connection.ReceiveStartsWith("Date:");
755741
await connection.ReceiveForcedEnd(

0 commit comments

Comments
 (0)