Allow/ignore upgrades with bodies #17081 (#28907)

Author: Tratcher

src/Servers/Kestrel/Core/src/BadHttpRequestException.cs

@@ -111,9 +111,6 @@ internal static BadHttpRequestException GetException(RequestRejectionReason reas
                 case RequestRejectionReason.InvalidHostHeader:
                     ex = new BadHttpRequestException(CoreStrings.BadRequest_InvalidHostHeader, StatusCodes.Status400BadRequest, reason);
                     break;
-                case RequestRejectionReason.UpgradeRequestCannotHavePayload:
-                    ex = new BadHttpRequestException(CoreStrings.BadRequest_UpgradeRequestCannotHavePayload, StatusCodes.Status400BadRequest, reason);
-                    break;
                 default:
                     ex = new BadHttpRequestException(CoreStrings.BadRequest, StatusCodes.Status400BadRequest, reason);
                     break;

src/Servers/Kestrel/Core/src/CoreStrings.resx

@@ -198,9 +198,6 @@
   <data name="BadRequest_UnrecognizedHTTPVersion" xml:space="preserve">
     <value>Unrecognized HTTP version: '{detail}'</value>
   </data>
-  <data name="BadRequest_UpgradeRequestCannotHavePayload" xml:space="preserve">
-    <value>Requests with 'Connection: Upgrade' cannot have content in the request body.</value>
-  </data>
   <data name="FallbackToIPv4Any" xml:space="preserve">
     <value>Failed to bind to http://[::]:{port} (IPv6Any). Attempting to bind to http://0.0.0.0:{port} instead.</value>
   </data>

src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs

@@ -127,13 +127,13 @@ public static MessageBody For(
                 keepAlive = (connectionOptions & ConnectionOptions.KeepAlive) == ConnectionOptions.KeepAlive;
             }
 
-            if (upgrade)
+            // Ignore upgrades if the request has a body. Technically it's possible to support, but we'd have to add a lot
+            // more logic to allow reading/draining the normal body before the connection could be fully upgraded.
+            // See https://tools.ietf.org/html/rfc7230#section-6.7, https://tools.ietf.org/html/rfc7540#section-3.2
+            if (upgrade
+                && headers.ContentLength.GetValueOrDefault() == 0
+                && headers.HeaderTransferEncoding.Count == 0)
             {
-                if (headers.HeaderTransferEncoding.Count > 0 || (headers.ContentLength.HasValue && headers.ContentLength.Value != 0))
-                {
-                    BadHttpRequestException.Throw(RequestRejectionReason.UpgradeRequestCannotHavePayload);
-                }
-
                 context.OnTrailersComplete(); // No trailers for these.
                 return new Http1UpgradeMessageBody(context);
             }

src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs

@@ -32,7 +32,6 @@ internal enum RequestRejectionReason
         MissingHostHeader,
         MultipleHostHeaders,
         InvalidHostHeader,
-        UpgradeRequestCannotHavePayload,
         RequestBodyExceedsContentLength
     }
 }

src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs

@@ -3,17 +3,17 @@
 
 using System;
 using System.IO;
-using System.IO.Pipelines;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Connections.Features;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests.TestTransport;
 using Microsoft.AspNetCore.Server.Kestrel.Tests;
 using Microsoft.AspNetCore.Testing;
 using Microsoft.Extensions.Logging.Testing;
+using Microsoft.Net.Http.Headers;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests
@@ -154,9 +154,24 @@ await connection.Receive("HTTP/1.1 101 Switching Protocols",
         }
 
         [Fact]
-        public async Task RejectsRequestWithContentLengthAndUpgrade()
+        public async Task AcceptsRequestWithContentLengthAndUpgrade()
         {
-            await using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory)))
+            await using (var server = new TestServer(async context =>
+            {
+                var feature = context.Features.Get<IHttpUpgradeFeature>();
+
+                if (HttpMethods.IsPost(context.Request.Method))
+                {
+                    Assert.False(feature.IsUpgradableRequest);
+                    Assert.Equal(1, context.Request.ContentLength);
+                    Assert.Equal(1, await context.Request.Body.ReadAsync(new byte[10], 0, 10));
+                }
+                else
+                {
+                    Assert.True(feature.IsUpgradableRequest);
+                }
+            },
+            new TestServiceContext(LoggerFactory)))
             {
                 using (var connection = server.CreateConnection())
                 {
@@ -165,23 +180,28 @@ await connection.Send("POST / HTTP/1.1",
                         "Content-Length: 1",
                         "Connection: Upgrade",
                         "",
-                        "");
+                        "A");
 
-                    await connection.ReceiveEnd(
-                        "HTTP/1.1 400 Bad Request",
-                        "Connection: close",
-                        $"Date: {server.Context.DateHeaderValue}",
-                        "Content-Length: 0",
-                        "",
-                        "");
+                    await connection.Receive("HTTP/1.1 200 OK");
                 }
             }
         }
 
         [Fact]
         public async Task AcceptsRequestWithNoContentLengthAndUpgrade()
         {
-            await using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory)))
+            await using (var server = new TestServer(async context =>
+            {
+                var feature = context.Features.Get<IHttpUpgradeFeature>();
+                Assert.True(feature.IsUpgradableRequest);
+
+                if (HttpMethods.IsPost(context.Request.Method))
+                {
+                    Assert.Equal(0, context.Request.ContentLength);
+                }
+                Assert.Equal(0, await context.Request.Body.ReadAsync(new byte[10], 0, 10));
+            },
+            new TestServiceContext(LoggerFactory)))
             {
                 using (var connection = server.CreateConnection())
                 {
@@ -203,9 +223,26 @@ await connection.Send("POST / HTTP/1.1",
         }
 
         [Fact]
-        public async Task RejectsRequestWithChunkedEncodingAndUpgrade()
+        public async Task AcceptsRequestWithChunkedEncodingAndUpgrade()
         {
-            await using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory)))
+            await using (var server = new TestServer(async context =>
+            {
+                var feature = context.Features.Get<IHttpUpgradeFeature>();
+
+                Assert.Null(context.Request.ContentLength);
+
+                if (HttpMethods.IsPost(context.Request.Method))
+                {
+                    Assert.False(feature.IsUpgradableRequest);
+                    Assert.Equal("chunked", context.Request.Headers[HeaderNames.TransferEncoding]);
+                    Assert.Equal(11, await context.Request.Body.ReadAsync(new byte[12], 0, 12));
+                }
+                else
+                {
+                    Assert.True(feature.IsUpgradableRequest);
+                }
+            },
+            new TestServiceContext(LoggerFactory)))
             {
                 using (var connection = server.CreateConnection())
                 {
@@ -214,14 +251,11 @@ await connection.Send("POST / HTTP/1.1",
                         "Transfer-Encoding: chunked",
                         "Connection: Upgrade",
                         "",
-                        "");
-                    await connection.ReceiveEnd(
-                        "HTTP/1.1 400 Bad Request",
-                        "Connection: close",
-                        $"Date: {server.Context.DateHeaderValue}",
-                        "Content-Length: 0",
+                        "B", "Hello World",
+                        "0",
                         "",
                         "");
+                    await connection.Receive("HTTP/1.1 200 OK");
                 }
             }
         }