Mitigation of wrong cookie value separator (#451)

src/ReverseProxy/Service/Proxy/HttpProxy.cs

@@ -370,8 +370,8 @@ private static void CopyRequestHeaders(HttpContext context, HttpRequestMessage d
                 foreach (var header in context.Request.Headers)
                 {
                     var headerName = header.Key;
-                    var value = header.Value;
-                    if (StringValues.IsNullOrEmpty(value))
+                    var headerValue = header.Value;
+                    if (StringValues.IsNullOrEmpty(headerValue))
                     {
                         continue;
                     }
@@ -385,14 +385,14 @@ private static void CopyRequestHeaders(HttpContext context, HttpRequestMessage d
                     if (transforms.RequestHeaderTransforms.TryGetValue(headerName, out var transform))
                     {
                         (transformsRun ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase)).Add(headerName);
-                        value = transform.Apply(context, value);
-                        if (StringValues.IsNullOrEmpty(value))
+                        headerValue = transform.Apply(context, headerValue);
+                        if (StringValues.IsNullOrEmpty(headerValue))
                         {
                             continue;
                         }
                     }
 
-                    AddHeader(destination, headerName, value);
+                    AddHeader(destination, headerName, headerValue);
                 }
             }
 
@@ -416,6 +416,15 @@ private static void CopyRequestHeaders(HttpContext context, HttpRequestMessage d
             // as long as they appear in one (and only one, otherwise they would be duplicated).
             static void AddHeader(HttpRequestMessage request, string headerName, StringValues value)
             {
+                // HttpClient wrongly uses comma (",") instead of semi-colon (";") as a separator for Cookie headers.
+                // To mitigate this, we concatenate them manually and put them back as a single header value.
+                // A multi-header cookie header is invalid, but we get one because of
+                // https://github.com/dotnet/aspnetcore/issues/26461
+                if (string.Equals(headerName, HeaderNames.Cookie, StringComparison.OrdinalIgnoreCase) && value.Count > 1)
+                {
+                    value = String.Join("; ", value);
+                }
+
                 if (value.Count == 1)
                 {
                     string headerValue = value;

test/ReverseProxy.FunctionalTests/HttpProxyCookieTests.cs

@@ -0,0 +1,213 @@
+using System;
+using System.IO;
+using System.Net;
+using System.Net.Http;
+using System.Net.Sockets;
+using System.Text;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Server.Kestrel.Core;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Primitives;
+using Microsoft.Net.Http.Headers;
+using Microsoft.ReverseProxy.Abstractions;
+using Xunit;
+
+namespace Microsoft.ReverseProxy
+{
+    public abstract class HttpProxyCookieTests
+    {
+        public const string CookieAKey = "testA";
+        public const string CookieAValue = "A_Cookie";
+        public const string CookieBKey = "testB";
+        public const string CookieBValue = "B_Cookie";
+
+        public static readonly string CookieA = $"{CookieAKey}={CookieAValue}";
+        public static readonly string CookieB = $"{CookieBKey}={CookieBValue}";
+        public static readonly string Cookies = $"{CookieA}; {CookieB}";
+
+        public abstract HttpProtocols HttpProtocol { get; }
+        public abstract Task ProcessHttpRequest(Uri proxyHostUri);
+
+        [Fact]
+        public async Task ProxyAsync_RequestWithCookieHeaders()
+        {
+            var tcs = new TaskCompletionSource<StringValues>(TaskCreationOptions.RunContinuationsAsynchronously);
+
+            using var destinationHost = CreateDestinationHost(
+                context =>
+                {
+                    if (context.Request.Headers.TryGetValue(HeaderNames.Cookie, out var cookieHeaders))
+                    {
+                        tcs.SetResult(cookieHeaders);
+                    }
+                    else
+                    {
+                        tcs.SetException(new Exception("Missing 'Cookie' header in request"));
+                    }
+                    return Task.CompletedTask;
+                });
+
+            await destinationHost.StartAsync();
+            var destinationHostUrl = destinationHost.GetAddress();
+
+            using var proxyHost = CreateReverseProxyHost(HttpProtocol, destinationHostUrl);
+            await proxyHost.StartAsync();
+            var proxyHostUri = new Uri(proxyHost.GetAddress());
+
+            await ProcessHttpRequest(proxyHostUri);
+
+            Assert.True(tcs.Task.IsCompleted);
+            var cookieHeaders = await tcs.Task;
+            var cookies = Assert.Single(cookieHeaders);
+            Assert.Equal(Cookies, cookies);
+
+            await destinationHost.StopAsync();
+            await proxyHost.StopAsync();
+        }
+
+        private IHost CreateReverseProxyHost(HttpProtocols httpProtocol, string destinationHostUrl)
+        {
+            return CreateHost(httpProtocol,
+                services =>
+                {
+                    var proxyRoute = new ProxyRoute
+                    {
+                        RouteId = "route1",
+                        ClusterId = "cluster1",
+                        Match = { Path = "/{**catchall}" }
+                    };
+
+                    var cluster = new Cluster
+                    {
+                        Id = "cluster1",
+                        Destinations =
+                        {
+                            { "cluster1",  new Destination() { Address = destinationHostUrl } }
+                        }
+                    };
+
+                    services.AddReverseProxy().LoadFromMemory(new[] { proxyRoute }, new[] { cluster });
+                },
+                app =>
+                {
+                    app.UseMiddleware<CheckCookieHeaderMiddleware>();
+                    app.UseRouting();
+                    app.UseEndpoints(builder =>
+                    {
+                        builder.MapReverseProxy();
+                    });
+                });
+        }
+
+        private IHost CreateDestinationHost(RequestDelegate getDelegate)
+        {
+            return CreateHost(HttpProtocols.Http1AndHttp2,
+                services =>
+                {
+                    services.AddRouting();
+                },
+                app =>
+                {
+                    app.UseRouting();
+                    app.UseEndpoints(endpoints => endpoints.MapGet("/", getDelegate));
+                });
+        }
+
+        private IHost CreateHost(HttpProtocols httpProtocols, Action<IServiceCollection> configureServices, Action<IApplicationBuilder> configureApp)
+        {
+            return new HostBuilder()
+               .ConfigureWebHost(webHostBuilder =>
+               {
+                   webHostBuilder
+                   .ConfigureServices(configureServices)
+                   .UseKestrel(kestrel => 
+                   {
+                       kestrel.Listen(IPAddress.Loopback, 0, listenOptions =>
+                       {
+                           listenOptions.Protocols = httpProtocols;
+                       });
+                   })
+                   .Configure(configureApp);
+               }).Build();
+        }
+
+        private class CheckCookieHeaderMiddleware
+        {
+            private readonly RequestDelegate _next;
+
+            public CheckCookieHeaderMiddleware(RequestDelegate next)
+            {
+                _next = next;
+            }
+
+            public async Task Invoke(HttpContext context)
+            {
+                // Ensure that CookieA is the first and CookieB the last.
+                Assert.True(context.Request.Headers.TryGetValue(HeaderNames.Cookie, out var headerValues));
+
+                if (context.Request.Protocol == "HTTP/1.1")
+                {
+                    Assert.Single(headerValues);
+                    Assert.Equal(Cookies, headerValues);
+                }
+                else if (context.Request.Protocol == "HTTP/2")
+                {
+                    Assert.Equal(2, headerValues.Count);
+                    Assert.Equal(CookieA, headerValues[0]);
+                    Assert.Equal(CookieB, headerValues[1]);
+                }
+                else
+                {
+                    Assert.True(false, $"Unexpected HTTP protocol '{context.Request.Protocol}'");
+                }
+
+                await _next.Invoke(context);
+            }
+        }
+    }
+
+    public class HttpProxyCookieTests_Http1 : HttpProxyCookieTests
+    {
+        public override HttpProtocols HttpProtocol => HttpProtocols.Http1;
+
+        public override async Task ProcessHttpRequest(Uri proxyHostUri)
+        {
+            using var client = new HttpClient();            
+            using var message = new HttpRequestMessage(HttpMethod.Get, proxyHostUri);
+            message.Headers.Add(HeaderNames.Cookie, Cookies);
+            using var response = await client.SendAsync(message);
+            response.EnsureSuccessStatusCode();
+        }
+    }
+
+#if NET5_0
+    public class HttpProxyCookieTests_Http2 : HttpProxyCookieTests
+    {
+        public override HttpProtocols HttpProtocol => HttpProtocols.Http2;
+
+        // HttpClient for H/2 will use different header frames for cookies from a container and message headers.
+        // It will first send message header cookie and than the container one and we expect them in the order of cookieA;cookieB.
+        public override async Task ProcessHttpRequest(Uri proxyHostUri)
+        {
+            using var handler = new HttpClientHandler();
+            handler.CookieContainer.Add(new System.Net.Cookie(CookieBKey, CookieBValue, path: "/", domain: proxyHostUri.Host));
+            using var client = new HttpClient(handler);
+            using var message = new HttpRequestMessage(HttpMethod.Get, proxyHostUri);
+            message.Version = HttpVersion.Version20;
+            message.VersionPolicy = HttpVersionPolicy.RequestVersionExact;
+            message.Headers.Add(HeaderNames.Cookie, CookieA);
+            using var response = await client.SendAsync(message);
+            response.EnsureSuccessStatusCode();
+        }
+    }
+#elif NETCOREAPP3_1
+// Do not test HTTP/2 on .NET Core 3.1
+#else
+#error A target framework was added to the project and needs to be added to this condition. 
+#endif
+}

test/ReverseProxy.Tests/Service/Proxy/HttpProxyTests.cs

@@ -585,6 +585,41 @@ public async Task ProxyAsync_RequetsWithBodies_HasHttpContent(string method, str
             Assert.Equal(StatusCodes.Status200OK, httpContext.Response.StatusCode);
         }
 
+        [Fact]
+        public async Task ProxyAsync_RequestWithCookieHeaders()
+        {
+            // This is an invalid format per spec but may happen due to https://github.com/dotnet/aspnetcore/issues/26461
+            var cookies = new [] { "testA=A_Cookie", "testB=B_Cookie", "testC=C_Cookie" };
+            var httpContext = new DefaultHttpContext();
+            httpContext.Request.Method = "GET";
+            httpContext.Request.Headers.Add(HeaderNames.Cookie, cookies);
+
+            var destinationPrefix = "https://localhost/";
+            var sut = CreateProxy();
+            var client = MockHttpHandler.CreateClient(
+                (HttpRequestMessage request, CancellationToken cancellationToken) =>
+                {
+                    // "testA=A_Cookie; testB=B_Cookie; testC=C_Cookie"
+                    var expectedCookieString = String.Join("; ", cookies);
+
+                    Assert.Equal(new Version(2, 0), request.Version);
+                    Assert.Equal("GET", request.Method.Method, StringComparer.OrdinalIgnoreCase);
+                    Assert.Null(request.Content);
+                    Assert.True(request.Headers.TryGetValues(HeaderNames.Cookie, out var cookieHeaders));
+                    Assert.NotNull(cookieHeaders);
+                    var cookie = Assert.Single(cookieHeaders);
+                    Assert.Equal(expectedCookieString, cookie);
+
+                    var response = new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(Array.Empty<byte>()) };
+                    return Task.FromResult(response);
+                });
+
+            await sut.ProxyAsync(httpContext, destinationPrefix, client, new RequestProxyOptions());
+
+            Assert.Null(httpContext.Features.Get<IProxyErrorFeature>());
+            Assert.Equal(StatusCodes.Status200OK, httpContext.Response.StatusCode);
+        }
+
         [Fact]
         public async Task ProxyAsync_UnableToConnect_Returns502()
         {