Send 'Connection: close' in all 400 responses to HTTP/1.1 requests (#840).

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

@@ -30,7 +30,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
         private static readonly byte[] _bytesConnectionClose = Encoding.ASCII.GetBytes("\r\nConnection: close");
         private static readonly byte[] _bytesConnectionKeepAlive = Encoding.ASCII.GetBytes("\r\nConnection: keep-alive");
         private static readonly byte[] _bytesTransferEncodingChunked = Encoding.ASCII.GetBytes("\r\nTransfer-Encoding: chunked");
-        private static readonly byte[] _bytesHttpVersion1_1 = Encoding.ASCII.GetBytes("HTTP/1.1 ");
+        private static readonly byte[] _bytesHttpVersion11 = Encoding.ASCII.GetBytes("HTTP/1.1 ");
         private static readonly byte[] _bytesContentLengthZero = Encoding.ASCII.GetBytes("\r\nContent-Length: 0");
         private static readonly byte[] _bytesSpace = Encoding.ASCII.GetBytes(" ");
         private static readonly byte[] _bytesEndHeaders = Encoding.ASCII.GetBytes("\r\n\r\n");
@@ -87,29 +87,30 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
         {
             get
             {
-                if (_httpVersion == HttpVersionType.Http1_1)
+                if (_httpVersion == HttpVersionType.Http11)
                 {
                     return "HTTP/1.1";
                 }
-                if (_httpVersion == HttpVersionType.Http1_0)
+                if (_httpVersion == HttpVersionType.Http10)
                 {
                     return "HTTP/1.0";
                 }
+
                 return string.Empty;
             }
             set
             {
                 if (value == "HTTP/1.1")
                 {
-                    _httpVersion = HttpVersionType.Http1_1;
+                    _httpVersion = HttpVersionType.Http11;
                 }
                 else if (value == "HTTP/1.0")
                 {
-                    _httpVersion = HttpVersionType.Http1_0;
+                    _httpVersion = HttpVersionType.Http10;
                 }
                 else
                 {
-                    _httpVersion = HttpVersionType.Unknown;
+                    _httpVersion = HttpVersionType.Unset;
                 }
             }
         }
@@ -229,7 +230,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
             PathBase = null;
             Path = null;
             QueryString = null;
-            _httpVersion = HttpVersionType.Unknown;
+            _httpVersion = HttpVersionType.Unset;
             StatusCode = 200;
             ReasonPhrase = null;
 
@@ -512,7 +513,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
             }
 
             StringValues expect;
-            if (_httpVersion == HttpVersionType.Http1_1 &&
+            if (_httpVersion == HttpVersionType.Http11 &&
                 RequestHeaders.TryGetValue("Expect", out expect) &&
                 (expect.FirstOrDefault() ?? "").Equals("100-continue", StringComparison.OrdinalIgnoreCase))
             {
@@ -595,6 +596,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
                 {
                     // 400 Bad Request
                     StatusCode = 400;
+                    _keepAlive = false;
                 }
                 else
                 {
@@ -712,7 +714,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
                     //
                     // A server MUST NOT send a response containing Transfer-Encoding unless the corresponding
                     // request indicates HTTP/1.1 (or later).
-                    if (_httpVersion == HttpVersionType.Http1_1)
+                    if (_httpVersion == HttpVersionType.Http11)
                     {
                         _autoChunk = true;
                         responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked);
@@ -724,16 +726,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
                 }
             }
 
-            if (_keepAlive == false && responseHeaders.HasConnection == false && _httpVersion == HttpVersionType.Http1_1)
+            if (!_keepAlive && !responseHeaders.HasConnection && _httpVersion != HttpVersionType.Http10)
             {
                 responseHeaders.SetRawConnection("close", _bytesConnectionClose);
             }
-            else if (_keepAlive && responseHeaders.HasConnection == false && _httpVersion == HttpVersionType.Http1_0)
+            else if (_keepAlive && !responseHeaders.HasConnection && _httpVersion == HttpVersionType.Http10)
             {
                 responseHeaders.SetRawConnection("keep-alive", _bytesConnectionKeepAlive);
             }
 
-            end.CopyFrom(_bytesHttpVersion1_1);
+            end.CopyFrom(_bytesHttpVersion11);
             end.CopyFrom(statusBytes);
             responseHeaders.CopyTo(ref end);
             end.CopyFrom(_bytesEndHeaders, 0, _bytesEndHeaders.Length);
@@ -838,13 +840,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
                     }
                     else if (httpVersion != "HTTP/1.0" && httpVersion != "HTTP/1.1")
                     {
-                        RejectRequest("Malformed request.");
+                        RejectRequest("Unrecognized HTTP version.");
                     }
                 }
 
-                // HttpVersion must be set here to send correct response when request is rejected
-                HttpVersion = httpVersion;
-
                 scan.Take();
                 var next = scan.Take();
                 if (next == -1)
@@ -879,6 +878,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
                 Method = method;
                 RequestUri = requestUrlPath;
                 QueryString = queryString;
+                HttpVersion = httpVersion;
 
                 bool caseMatches;
 
@@ -1099,9 +1099,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http
 
         private enum HttpVersionType
         {
-            Unknown = -1,
+            Unset = -1,
-            Http1_0 = 0,
+            Http10 = 0,
-            Http1_1 = 1
+            Http11 = 1
         }
 
         protected enum RequestLineStatus

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

@@ -1,7 +1,6 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using System;
 using System.Threading.Tasks;
 using Xunit;
 
@@ -9,64 +8,107 @@ namespace Microsoft.AspNetCore.Server.KestrelTests
 {
     public class BadHttpRequestTests
     {
-        [Theory(Skip = "This test fails intermittently and needs to be investigated.")]
+        // Don't send more data than necessary to fail, otherwise the test throws trying to
-        [InlineData("/ HTTP/1.1\r\n\r\n")]
+        // send data after the server has already closed the connection. This would cause the
-        [InlineData(" / HTTP/1.1\r\n\r\n")]
+        // test to fail on Windows, due to a winsock limitation: after the error when trying
-        [InlineData("  / HTTP/1.1\r\n\r\n")]
+        // to write to the socket closed by the server, winsock disposes all resources used
-        [InlineData("GET  / HTTP/1.1\r\n\r\n")]
+        // by that socket. The test then fails when we try to read the expected response
-        [InlineData("GET  /  HTTP/1.1\r\n\r\n")]
+        // from the server because, although it would have been buffered, it got discarded
-        [InlineData("GET  HTTP/1.1\r\n\r\n")]
+        // by winsock on the send() error.
+        // The solution for this is for the client to always try to receive before doing
+        // any sends, that way it can detect that the connection has been closed by the server
+        // and not try to send() on the closed connection, triggering the error that would cause
+        // any buffered received data to be lost.
+        // We do not deem necessary to mitigate this issue in TestConnection, since it would only
+        // be ensuring that we have a properly implemented HTTP client that can handle the
+        // winsock issue. There is nothing to be verified in Kestrel in this situation.
+        [Theory]
+        // Incomplete request lines
+        [InlineData("G")]
+        [InlineData("GE")]
+        [InlineData("GET")]
+        [InlineData("GET ")]
         [InlineData("GET /")]
         [InlineData("GET / ")]
         [InlineData("GET / H")]
+        [InlineData("GET / HT")]
+        [InlineData("GET / HTT")]
+        [InlineData("GET / HTTP")]
+        [InlineData("GET / HTTP/")]
+        [InlineData("GET / HTTP/1")]
         [InlineData("GET / HTTP/1.")]
+        [InlineData("GET / HTTP/1.1")]
+        [InlineData("GET / HTTP/1.1\r")]
+        [InlineData("GET / HTTP/1.0")]
+        [InlineData("GET / HTTP/1.0\r")]
+        // Missing method
+        [InlineData(" ")]
+        // Missing second space
+        [InlineData("/ HTTP/1.1\r\n\r\n")]
         [InlineData("GET /\r\n")]
-        [InlineData("GET / \r\n")]
+        // Missing target
+        [InlineData("GET  ")]
+        // Missing version
+        [InlineData("GET / \r")]
+        // Missing CR
         [InlineData("GET / \n")]
-        [InlineData("GET / http/1.0\r\n\r\n")]
+        // Unrecognized HTTP version
-        [InlineData("GET / http/1.1\r\n\r\n")]
+        [InlineData("GET / http/1.0\r")]
-        [InlineData("GET / HTTP/1.1 \r\n\r\n")]
+        [InlineData("GET / http/1.1\r")]
-        [InlineData("GET / HTTP/1.1a\r\n\r\n")]
+        [InlineData("GET / HTTP/1.1 \r")]
-        [InlineData("GET / HTTP/1.0\n\r\n")]
+        [InlineData("GET / HTTP/1.1a\r")]
-        [InlineData("GET / HTTP/3.0\r\n\r\n")]
+        [InlineData("GET / HTTP/1.0\n\r")]
-        [InlineData("GET / H\r\n\r\n")]
+        [InlineData("GET / HTTP/1.2\r")]
-        [InlineData("GET / HTTP/1.\r\n\r\n")]
+        [InlineData("GET / HTTP/3.0\r")]
-        [InlineData("GET / hello\r\n\r\n")]
+        [InlineData("GET / H\r")]
-        [InlineData("GET / 8charact\r\n\r\n")]
+        [InlineData("GET / HTTP/1.\r")]
-        public async Task TestBadRequests(string request)
+        [InlineData("GET / hello\r")]
+        [InlineData("GET / 8charact\r")]
+        // Missing LF
+        [InlineData("GET / HTTP/1.0\rA")]
+        public async Task TestBadRequestLines(string request)
         {
             using (var server = new TestServer(context => { return Task.FromResult(0); }))
             {
                 using (var connection = new TestConnection(server.Port))
                 {
-                    var receiveTask = Task.Run(async () =>
+                    await connection.SendEnd(request);
-                    {
+                    await ReceiveBadRequestResponse(connection);
-                        await connection.Receive(
-                            "HTTP/1.1 400 Bad Request",
-                            "");
-                        await connection.ReceiveStartsWith("Date: ");
-                        await connection.ReceiveForcedEnd(
-                            "Content-Length: 0",
-                            "Server: Kestrel",
-                            "",
-                            "");
-                    });
-
-                    try
-                    {
-                        await connection.SendEnd(request).ConfigureAwait(false);
-                    }
-                    catch (Exception)
-                    {
-                        // TestConnection.SendEnd will start throwing while sending characters
-                        // in cases where the server rejects the request as soon as it
-                        // determines the request line is malformed, even though there
-                        // are more characters following.
-                    }
-
-                    await receiveTask;
                 }
             }
         }
+
+        [Theory]
+        [InlineData(" ")]
+        [InlineData("GET  ")]
+        [InlineData("GET / HTTP/1.2\r")]
+        [InlineData("GET / HTTP/1.0\rA")]
+        public async Task ServerClosesConnectionAsSoonAsBadRequestLineIsDetected(string request)
+        {
+            using (var server = new TestServer(context => { return Task.FromResult(0); }))
+            {
+                using (var connection = new TestConnection(server.Port))
+                {
+                    await connection.Send(request);
+                    await ReceiveBadRequestResponse(connection);
+                }
+            }
+        }
+
+        private async Task ReceiveBadRequestResponse(TestConnection connection)
+        {
+            await connection.Receive(
+                "HTTP/1.1 400 Bad Request",
+                "");
+            await connection.Receive(
+                "Connection: close",
+                "");
+            await connection.ReceiveStartsWith("Date: ");
+            await connection.ReceiveEnd(
+                "Content-Length: 0",
+                "Server: Kestrel",
+                "",
+                "");
+        }
     }
 }

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

@@ -371,6 +371,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests
 
                     await connection.Receive(
                         "HTTP/1.1 400 Bad Request",
+                        "Connection: close",
                         "");
                     await connection.ReceiveStartsWith("Date:");
                     await connection.ReceiveForcedEnd(
@@ -415,6 +416,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests
 
                     await connection.Receive(
                         "HTTP/1.1 400 Bad Request",
+                        "Connection: close",
                         "");
                     await connection.ReceiveStartsWith("Date:");
                     await connection.ReceiveForcedEnd(

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

@@ -724,21 +724,6 @@ namespace Microsoft.AspNetCore.Server.KestrelTests
         {
             using (var server = new TestServer(AppChunked, testContext))
             {
-                using (var connection = new TestConnection(server.Port))
-                {
-                    await connection.SendEnd(
-                        "GET /");
-                    await connection.Receive(
-                        "HTTP/1.1 400 Bad Request",
-                        "");
-                    await connection.ReceiveStartsWith("Date:");
-                    await connection.ReceiveForcedEnd(
-                        "Content-Length: 0",
-                        "Server: Kestrel",
-                        "",
-                        "");
-                }
-
                 using (var connection = new TestConnection(server.Port))
                 {
                     await connection.SendEnd(
@@ -750,6 +735,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests
                         "Content-Length: 0",
                         "",
                         "HTTP/1.1 400 Bad Request",
+                        "Connection: close",
                         "");
                     await connection.ReceiveStartsWith("Date:");
                     await connection.ReceiveForcedEnd(