From 9676d0d21aed4a5a10376e7ab260221e096b3b61 Mon Sep 17 00:00:00 2001 From: Chris R Date: Mon, 28 Dec 2020 15:53:00 -0800 Subject: [PATCH] Allow/ignore upgrades with bodies #17081 --- .../Core/src/BadHttpRequestException.cs | 3 - src/Servers/Kestrel/Core/src/CoreStrings.resx | 3 - .../src/Internal/Http/Http1MessageBody.cs | 12 +-- .../Internal/Http/RequestRejectionReason.cs | 1 - .../InMemory.FunctionalTests/UpgradeTests.cs | 76 ++++++++++++++----- 5 files changed, 61 insertions(+), 34 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs b/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs index 929a4087786f..70afdf98f07c 100644 --- a/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs +++ b/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; diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index 59bd610f1e95..aa6016d0e4f7 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 4b5d5b087259..25740f6ee62d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs +++ b/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); } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs index fce21b621062..1646434699ad 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs @@ -32,7 +32,6 @@ internal enum RequestRejectionReason MissingHostHeader, MultipleHostHeaders, InvalidHostHeader, - UpgradeRequestCannotHavePayload, RequestBodyExceedsContentLength } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs index cfd81233bee2..0ca6b88bb059 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/UpgradeTests.cs @@ -3,10 +3,9 @@ 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; @@ -14,6 +13,7 @@ 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(); + + 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,15 +180,9 @@ 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"); } } } @@ -181,7 +190,18 @@ await connection.ReceiveEnd( [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()) { @@ -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(); + + 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"); } } }