From 78cc8294da72e8b38ccdb1180ddcce11e85d6ff5 Mon Sep 17 00:00:00 2001 From: MihaZupan Date: Tue, 12 Jan 2021 19:03:42 +0100 Subject: [PATCH 1/2] Drop invalid response headers --- .../Service/Proxy/HttpTransformer.cs | 13 +++--- .../Transforms/StructuredTransformer.cs | 4 +- .../Utilities/RequestUtilities.cs | 23 +++++++++-- .../Service/Proxy/HttpProxyTests.cs | 41 +++++++++++++++++++ 4 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/ReverseProxy/Service/Proxy/HttpTransformer.cs b/src/ReverseProxy/Service/Proxy/HttpTransformer.cs index d40c66de2..f2925e326 100644 --- a/src/ReverseProxy/Service/Proxy/HttpTransformer.cs +++ b/src/ReverseProxy/Service/Proxy/HttpTransformer.cs @@ -68,10 +68,10 @@ public virtual Task TransformRequestAsync(HttpContext httpContext, HttpRequestMe public virtual Task TransformResponseAsync(HttpContext httpContext, HttpResponseMessage proxyResponse) { var responseHeaders = httpContext.Response.Headers; - CopyResponseHeaders(proxyResponse.Headers, responseHeaders); + CopyResponseHeaders(httpContext, proxyResponse.Headers, responseHeaders); if (proxyResponse.Content != null) { - CopyResponseHeaders(proxyResponse.Content.Headers, responseHeaders); + CopyResponseHeaders(httpContext, proxyResponse.Content.Headers, responseHeaders); } return Task.CompletedTask; @@ -93,22 +93,25 @@ public virtual Task TransformResponseTrailersAsync(HttpContext httpContext, Http { // Note that trailers, if any, should already have been declared in Proxy's response // by virtue of us having proxied all response headers in step 6. - CopyResponseHeaders(proxyResponse.TrailingHeaders, outgoingTrailers); + CopyResponseHeaders(httpContext, proxyResponse.TrailingHeaders, outgoingTrailers); } return Task.CompletedTask; } - private static void CopyResponseHeaders(HttpHeaders source, IHeaderDictionary destination) + private static void CopyResponseHeaders(HttpContext httpContext, HttpHeaders source, IHeaderDictionary destination) { + var isHttp2OrGreater = ProtocolHelper.IsHttp2OrGreater(httpContext.Request.Protocol); + foreach (var header in source) { var headerName = header.Key; - if (RequestUtilities.ResponseHeadersToSkip.Contains(headerName)) + if (RequestUtilities.ShouldSkipResponseHeader(headerName, isHttp2OrGreater)) { continue; } + destination.Append(headerName, header.Value.ToArray()); } } diff --git a/src/ReverseProxy/Service/RuntimeModel/Transforms/StructuredTransformer.cs b/src/ReverseProxy/Service/RuntimeModel/Transforms/StructuredTransformer.cs index 4066a6c22..973445373 100644 --- a/src/ReverseProxy/Service/RuntimeModel/Transforms/StructuredTransformer.cs +++ b/src/ReverseProxy/Service/RuntimeModel/Transforms/StructuredTransformer.cs @@ -168,10 +168,12 @@ public override Task TransformResponseTrailersAsync(HttpContext context, HttpRes private static void CopyResponseHeaders(HttpResponseMessage response, HttpHeaders source, HttpContext context, IHeaderDictionary destination, IReadOnlyDictionary transforms, ref HashSet transformsRun) { + var isHttp2OrGreater = ProtocolHelper.IsHttp2OrGreater(context.Request.Protocol); + foreach (var header in source) { var headerName = header.Key; - if (RequestUtilities.ResponseHeadersToSkip.Contains(headerName)) + if (RequestUtilities.ShouldSkipResponseHeader(headerName, isHttp2OrGreater)) { continue; } diff --git a/src/ReverseProxy/Utilities/RequestUtilities.cs b/src/ReverseProxy/Utilities/RequestUtilities.cs index 48209d31a..65a6e0654 100644 --- a/src/ReverseProxy/Utilities/RequestUtilities.cs +++ b/src/ReverseProxy/Utilities/RequestUtilities.cs @@ -14,11 +14,26 @@ namespace Microsoft.ReverseProxy.Utilities { internal static class RequestUtilities { - // TODO: this list only contains "Transfer-Encoding" because that messes up Kestrel. If we don't need to add any more here then it would be more efficient to - // check for the single value directly. What about connection headers? - internal static readonly HashSet ResponseHeadersToSkip = new HashSet(StringComparer.OrdinalIgnoreCase) + internal static bool ShouldSkipResponseHeader(string headerName, bool isHttp2OrGreater) { - HeaderNames.TransferEncoding + if (isHttp2OrGreater) + { + // This check can be dropped in .NET 6.0+ as these headers will be removed by Kestrel + return _invalidH2H3ResponseHeaders.Contains(headerName); + } + else + { + return headerName.Equals(HeaderNames.TransferEncoding, StringComparison.OrdinalIgnoreCase); + } + } + + private static readonly HashSet _invalidH2H3ResponseHeaders = new(StringComparer.OrdinalIgnoreCase) + { + HeaderNames.Connection, + HeaderNames.TransferEncoding, + HeaderNames.KeepAlive, + HeaderNames.Upgrade, + "Proxy-Connection" }; /// diff --git a/test/ReverseProxy.Tests/Service/Proxy/HttpProxyTests.cs b/test/ReverseProxy.Tests/Service/Proxy/HttpProxyTests.cs index bb05953cc..da670a0de 100644 --- a/test/ReverseProxy.Tests/Service/Proxy/HttpProxyTests.cs +++ b/test/ReverseProxy.Tests/Service/Proxy/HttpProxyTests.cs @@ -1538,6 +1538,47 @@ await Assert.ThrowsAsync(() => proxy.ProxyAsync(httpContext, destinationPrefix, httpClient, requestOptions, transforms)); } + [Theory] + [InlineData("Foo", "HTTP/1.1", false)] + [InlineData("Foo", "HTTP/2", false)] + [InlineData("Transfer-Encoding", "HTTP/1.1", true)] + [InlineData("Transfer-Encoding", "HTTP/2", true)] + [InlineData("Connection", "HTTP/1.1", false)] + [InlineData("Connection", "HTTP/2", true)] + [InlineData("Keep-Alive", "HTTP/1.1", false)] + [InlineData("Keep-Alive", "HTTP/2", true)] + [InlineData("Upgrade", "HTTP/1.1", false)] + [InlineData("Upgrade", "HTTP/2", true)] + [InlineData("Proxy-Connection", "HTTP/1.1", false)] + [InlineData("Proxy-Connection", "HTTP/2", true)] + public async Task ProxyAsync_DropsInvalidResponseHeaders(string responseHeaderName, string requestProtocol, bool shouldDrop) + { + var events = TestEventListener.Collect(); + + var httpContext = new DefaultHttpContext(); + httpContext.Request.Method = "GET"; + httpContext.Request.Protocol = requestProtocol; + + var destinationPrefix = "https://localhost:123/a/b/"; + var sut = CreateProxy(); + var client = MockHttpHandler.CreateClient( + async (HttpRequestMessage request, CancellationToken cancellationToken) => + { + await Task.Yield(); + var response = new HttpResponseMessage(HttpStatusCode.OK); + response.Content = new StringContent("Foo"); + response.Headers.Add(responseHeaderName, "Bar"); + return response; + }); + + await sut.ProxyAsync(httpContext, destinationPrefix, client); + + Assert.Equal(!shouldDrop, httpContext.Response.Headers.ContainsKey(responseHeaderName)); + + AssertProxyStartStop(events, destinationPrefix, httpContext.Response.StatusCode); + events.AssertContainProxyStages(hasRequestContent: false); + } + private static void AssertProxyStartStop(List events, string destinationPrefix, int statusCode) { AssertProxyStartFailedStop(events, destinationPrefix, statusCode, error: null); From 623d3501cc235cb6d5e3af7448f5403be689bade Mon Sep 17 00:00:00 2001 From: MihaZupan Date: Thu, 14 Jan 2021 17:57:06 +0100 Subject: [PATCH 2/2] Remove comment about .NET 6+ --- src/ReverseProxy/Utilities/RequestUtilities.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ReverseProxy/Utilities/RequestUtilities.cs b/src/ReverseProxy/Utilities/RequestUtilities.cs index 65a6e0654..835d39be1 100644 --- a/src/ReverseProxy/Utilities/RequestUtilities.cs +++ b/src/ReverseProxy/Utilities/RequestUtilities.cs @@ -18,7 +18,6 @@ internal static bool ShouldSkipResponseHeader(string headerName, bool isHttp2OrG { if (isHttp2OrGreater) { - // This check can be dropped in .NET 6.0+ as these headers will be removed by Kestrel return _invalidH2H3ResponseHeaders.Contains(headerName); } else