From 60a138c93d696797178e1a534f4ec2d388d834d1 Mon Sep 17 00:00:00 2001 From: Chris R Date: Mon, 28 Dec 2020 16:41:46 -0800 Subject: [PATCH 1/2] 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 - .../test/FunctionalTests/UpgradeTests.cs | 59 ++++++++++++------- 5 files changed, 45 insertions(+), 33 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs b/src/Servers/Kestrel/Core/src/BadHttpRequestException.cs index 033c8d9e1b8c..a29936b57c72 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 2b0326e1d73d..adfdbe110c22 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 7aeea58f3489..a323a6b611bb 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs @@ -271,13 +271,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); - } - return new ForUpgrade(context); } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs b/src/Servers/Kestrel/Core/src/Internal/Http/RequestRejectionReason.cs index ee27b5cb9634..e1c96f203fd9 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 @@ public enum RequestRejectionReason MissingHostHeader, MultipleHostHeaders, InvalidHostHeader, - UpgradeRequestCannotHavePayload, RequestBodyExceedsContentLength } } diff --git a/src/Servers/Kestrel/test/FunctionalTests/UpgradeTests.cs b/src/Servers/Kestrel/test/FunctionalTests/UpgradeTests.cs index 2b4027d37cf3..97ed5aa2650c 100644 --- a/src/Servers/Kestrel/test/FunctionalTests/UpgradeTests.cs +++ b/src/Servers/Kestrel/test/FunctionalTests/UpgradeTests.cs @@ -1,15 +1,17 @@ -// Copyright (c) .NET Foundation. All rights reserved. +// 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.IO; using System.Threading.Tasks; +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.Tests; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Logging.Testing; +using Microsoft.Net.Http.Headers; using Xunit; namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests @@ -146,9 +148,15 @@ await connection.Receive("HTTP/1.1 101 Switching Protocols", } [Fact] - public async Task RejectsRequestWithContentLengthAndUpgrade() + public async Task AcceptsRequestWithContentLengthAndUpgrade() { - using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory))) + using (var server = new TestServer(async context => + { + var feature = context.Features.Get(); + Assert.False(feature.IsUpgradableRequest); + Assert.Equal(1, context.Request.ContentLength); + Assert.Equal(1, await context.Request.Body.ReadAsync(new byte[10], 0, 10)); + }, new TestServiceContext(LoggerFactory))) using (var connection = server.CreateConnection()) { await connection.Send("POST / HTTP/1.1", @@ -156,22 +164,27 @@ await connection.Send("POST / HTTP/1.1", "Content-Length: 1", "Connection: Upgrade", "", - ""); + "A"); - await connection.ReceiveForcedEnd( - "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() { - using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory))) + 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()) { @@ -193,9 +206,18 @@ await connection.Send("POST / HTTP/1.1", } [Fact] - public async Task RejectsRequestWithChunkedEncodingAndUpgrade() + public async Task AcceptsRequestWithChunkedEncodingAndUpgrade() { - using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory))) + using (var server = new TestServer(async context => + { + var feature = context.Features.Get(); + Assert.Null(context.Request.ContentLength); + Assert.False(feature.IsUpgradableRequest); + Assert.True(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)); + }, new TestServiceContext(LoggerFactory))) using (var connection = server.CreateConnection()) { await connection.Send("POST / HTTP/1.1", @@ -203,14 +225,11 @@ await connection.Send("POST / HTTP/1.1", "Transfer-Encoding: chunked", "Connection: Upgrade", "", - ""); - await connection.ReceiveForcedEnd( - "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"); } } From 62df65e894b5d6c300788aa6419175a1954e293a Mon Sep 17 00:00:00 2001 From: Chris R Date: Tue, 29 Dec 2020 09:54:09 -0800 Subject: [PATCH 2/2] Update patchconfig.props --- eng/PatchConfig.props | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/eng/PatchConfig.props b/eng/PatchConfig.props index 271e6a24713d..48f0a6d658e8 100644 --- a/eng/PatchConfig.props +++ b/eng/PatchConfig.props @@ -83,4 +83,9 @@ Later on, this will be checked using this condition: Microsoft.AspNetCore.DataProtection.AzureStorage; + + + Microsoft.AspNetCore.Server.Kestrel.Core; + +