From 7991a9a2e26fedf6678298ea374b426011d408a6 Mon Sep 17 00:00:00 2001 From: Chris Ross Date: Wed, 13 Jan 2021 10:23:53 -0800 Subject: [PATCH] Allow/ignore upgrades with bodies #17081 (#28896) --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 3 - .../src/Internal/Http/Http1MessageBody.cs | 12 +-- .../Internal/Http/RequestRejectionReason.cs | 1 - ...n.cs => KestrelBadHttpRequestException.cs} | 3 - .../InMemory.FunctionalTests/UpgradeTests.cs | 76 ++++++++++++++----- 5 files changed, 61 insertions(+), 34 deletions(-) rename src/Servers/Kestrel/Core/src/{KestralBadHttpRequestException.cs => KestrelBadHttpRequestException.cs} (97%) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index 293d84df90..8f932cae80 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -198,9 +198,6 @@ Unrecognized HTTP version: '{detail}' - - Requests with 'Connection: Upgrade' cannot have content in the request body. - Failed to bind to http://[::]:{port} (IPv6Any). Attempting to bind to http://0.0.0.0:{port} instead. diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs index af2afd6eeb..01432d159f 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs @@ -130,13 +130,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http keepAlive = keepAlive && (connectionOptions & ConnectionOptions.Close) == 0; } - 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)) - { - KestrelBadHttpRequestException.Throw(RequestRejectionReason.UpgradeRequestCannotHavePayload); - } - context.OnTrailersComplete(); // No trailers for these. return new Http1UpgradeMessageBody(context, keepAlive); } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs index 23dc6c67c6..0bdb50c5a1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs @@ -33,7 +33,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http MissingHostHeader, MultipleHostHeaders, InvalidHostHeader, - UpgradeRequestCannotHavePayload, RequestBodyExceedsContentLength } } diff --git a/src/Servers/Kestrel/Core/src/KestralBadHttpRequestException.cs b/src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs similarity index 97% rename from src/Servers/Kestrel/Core/src/KestralBadHttpRequestException.cs rename to src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs index 0921e41e77..cb55e3d059 100644 --- a/src/Servers/Kestrel/Core/src/KestralBadHttpRequestException.cs +++ b/src/Servers/Kestrel/Core/src/KestrelBadHttpRequestException.cs @@ -88,9 +88,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core 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; diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs index 0432a9bcdf..4f71c1e0e4 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs @@ -3,16 +3,16 @@ 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.Net.Http.Headers; using Xunit; namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests @@ -153,9 +153,24 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests } [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(); + + 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()) { @@ -164,15 +179,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests "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"); } } } @@ -180,7 +189,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests [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(); + 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()) { @@ -202,9 +222,26 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests } [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(); + + 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()) { @@ -213,14 +250,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests "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"); } } }