Skip to content

Commit 3b590b5

Browse files
ManickaPManickaP
committed
Addressed feedback.
1 parent 0c4f4f1 commit 3b590b5

File tree

4 files changed

+232
-234
lines changed

4 files changed

+232
-234
lines changed

‎src/ReverseProxy/Service/Proxy/HttpProxy.cs‎

Lines changed: 9 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -379,11 +379,6 @@ private static void CopyRequestHeaders(HttpContext context, HttpRequestMessage d
379379
{
380380
// Transforms that were run in the first pass.
381381
HashSet<string> transformsRun = null;
382-
383-
// HttpClient wrongly uses comma (",") instead of semi-colon (";") as a separator for Cookie headers.
384-
// To mitigate this, we collect them, concatenate them manually and put them back as a single header value.
385-
List<string> cookies = null;
386-
387382
if (transforms.CopyRequestHeaders ?? true)
388383
{
389384
foreach (var header in context.Request.Headers)
@@ -411,10 +406,7 @@ private static void CopyRequestHeaders(HttpContext context, HttpRequestMessage d
411406
}
412407
}
413408

414-
if (!CatchCookies(headerName, headerValue, ref cookies))
415-
{
416-
AddHeader(destination, headerName, headerValue);
417-
}
409+
AddHeader(destination, headerName, headerValue);
418410
}
419411
}
420412

@@ -427,48 +419,24 @@ private static void CopyRequestHeaders(HttpContext context, HttpRequestMessage d
427419
headerValue = transform.Apply(context, headerValue);
428420
if (!StringValues.IsNullOrEmpty(headerValue))
429421
{
430-
if (!CatchCookies(headerName, headerValue, ref cookies))
431-
{
432-
AddHeader(destination, headerName, headerValue);
433-
}
422+
AddHeader(destination, headerName, headerValue);
434423
}
435424
}
436425
}
437426

438-
if (cookies != null && cookies.Count > 0)
439-
{
440-
AddHeader(destination, "Cookie", String.Join("; ", cookies));
441-
}
442-
443-
// Returns true when a cookie header is collected and therefore shouldn't be added to the destination yet.
444-
static bool CatchCookies(string headerName, StringValues value, ref List<string> cookies)
445-
{
446-
if (headerName != "Cookie")
447-
{
448-
return false;
449-
}
450-
451-
cookies ??= new List<string>();
452-
if (value.Count == 1)
453-
{
454-
string headerValue = value;
455-
cookies.Add(headerValue);
456-
}
457-
else
458-
{
459-
string[] headerValues = value;
460-
cookies.AddRange(headerValues);
461-
}
462-
463-
return true;
464-
}
465-
466427
// Note: HttpClient.SendAsync will end up sending the union of
467428
// HttpRequestMessage.Headers and HttpRequestMessage.Content.Headers.
468429
// We don't really care where the proxied headers appear among those 2,
469430
// as long as they appear in one (and only one, otherwise they would be duplicated).
470431
static void AddHeader(HttpRequestMessage request, string headerName, StringValues value)
471432
{
433+
// HttpClient wrongly uses comma (",") instead of semi-colon (";") as a separator for Cookie headers.
434+
// To mitigate this, we concatenate them manually and put them back as a single header value.
435+
if (headerName == HeaderNames.Cookie && value.Count > 1)
436+
{
437+
value = String.Join("; ", value);
438+
}
439+
472440
if (value.Count == 1)
473441
{
474442
string headerValue = value;

‎test/ReverseProxy.FunctionalTests/HttpProxyCookieTests.cs‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
using System;
2+
using System.IO;
3+
using System.Net;
4+
using System.Net.Http;
5+
using System.Net.Sockets;
6+
using System.Text;
7+
using System.Threading.Tasks;
8+
using Microsoft.AspNetCore.Builder;
9+
using Microsoft.AspNetCore.Hosting;
10+
using Microsoft.AspNetCore.Http;
11+
using Microsoft.AspNetCore.Server.Kestrel.Core;
12+
using Microsoft.Extensions.DependencyInjection;
13+
using Microsoft.Extensions.Hosting;
14+
using Microsoft.Extensions.Logging;
15+
using Microsoft.Extensions.Primitives;
16+
using Microsoft.Net.Http.Headers;
17+
using Microsoft.ReverseProxy.Abstractions;
18+
using Xunit;
19+
20+
namespace Microsoft.ReverseProxy
21+
{
22+
public abstract class HttpProxyCookieTests
23+
{
24+
public const string CookieAKey = "testA";
25+
public const string CookieAValue = "A_Cookie";
26+
public const string CookieBKey = "testB";
27+
public const string CookieBValue = "B_Cookie";
28+
29+
public static readonly string CookieA = $"{CookieAKey}={CookieAValue}";
30+
public static readonly string CookieB = $"{CookieBKey}={CookieBValue}";
31+
public static readonly string Cookies = $"{CookieA}; {CookieB}";
32+
33+
public abstract HttpProtocols HttpProtocol { get; }
34+
public abstract Task ProcessHttpRequest(Uri proxyHostUri);
35+
36+
[Fact]
37+
public async Task ProxyAsync_RequestWithCookieHeaders()
38+
{
39+
var tcs = new TaskCompletionSource<StringValues>();
40+
41+
using var destinationHost = CreateDestinationHost(
42+
context =>
43+
{
44+
if (context.Request.Headers.TryGetValue(HeaderNames.Cookie, out var cookieHeaders))
45+
{
46+
tcs.SetResult(cookieHeaders);
47+
}
48+
else
49+
{
50+
tcs.SetException(new Exception("Missing 'Cookie' header in request"));
51+
}
52+
return Task.CompletedTask;
53+
});
54+
55+
await destinationHost.StartAsync();
56+
var destinationHostUrl = destinationHost.GetAddress();
57+
58+
using var proxyHost = CreateReverseProxyHost(HttpProtocol, destinationHostUrl);
59+
await proxyHost.StartAsync();
60+
var proxyHostUri = new Uri(proxyHost.GetAddress());
61+
62+
await ProcessHttpRequest(proxyHostUri);
63+
64+
Assert.True(tcs.Task.IsCompleted);
65+
var cookieHeaders = await tcs.Task;
66+
var cookies = Assert.Single(cookieHeaders);
67+
Assert.Equal(Cookies, cookies);
68+
69+
await destinationHost.StopAsync();
70+
await proxyHost.StopAsync();
71+
}
72+
73+
private IHost CreateReverseProxyHost(HttpProtocols httpProtocol, string destinationHostUrl)
74+
{
75+
return CreateHost(httpProtocol,
76+
services =>
77+
{
78+
var proxyRoute = new ProxyRoute
79+
{
80+
RouteId = "route1",
81+
ClusterId = "cluster1",
82+
Match = { Path = "/{**catchall}" }
83+
};
84+
85+
var cluster = new Cluster
86+
{
87+
Id = "cluster1",
88+
Destinations =
89+
{
90+
{ "cluster1", new Destination() { Address = destinationHostUrl } }
91+
}
92+
};
93+
94+
services.AddReverseProxy().LoadFromMemory(new[] { proxyRoute }, new[] { cluster });
95+
},
96+
app =>
97+
{
98+
app.UseMiddleware<CheckCookieHeaderMiddleware>();
99+
app.UseRouting();
100+
app.UseEndpoints(builder =>
101+
{
102+
builder.MapReverseProxy();
103+
});
104+
});
105+
}
106+
107+
private IHost CreateDestinationHost(RequestDelegate getDelegate)
108+
{
109+
return CreateHost(HttpProtocols.Http1AndHttp2,
110+
services =>
111+
{
112+
services.AddRouting();
113+
},
114+
app =>
115+
{
116+
app.UseRouting();
117+
app.UseEndpoints(endpoints => endpoints.MapGet("/", getDelegate));
118+
});
119+
}
120+
121+
private IHost CreateHost(HttpProtocols httpProtocols, Action<IServiceCollection> configureServices, Action<IApplicationBuilder> configureApp)
122+
{
123+
return new HostBuilder()
124+
.ConfigureWebHost(webHostBuilder =>
125+
{
126+
webHostBuilder
127+
.ConfigureServices(configureServices)
128+
.UseKestrel(kestrel =>
129+
{
130+
kestrel.Listen(IPAddress.Loopback, 0, listenOptions =>
131+
{
132+
listenOptions.Protocols = httpProtocols;
133+
});
134+
})
135+
.Configure(configureApp);
136+
}).Build();
137+
}
138+
139+
private class CheckCookieHeaderMiddleware
140+
{
141+
private readonly RequestDelegate _next;
142+
143+
public CheckCookieHeaderMiddleware(RequestDelegate next)
144+
{
145+
_next = next;
146+
}
147+
148+
public async Task Invoke(HttpContext context)
149+
{
150+
// Ensure that CookieA is the first and CookieB the last.
151+
Assert.True(context.Request.Headers.TryGetValue(HeaderNames.Cookie, out var headerValues));
152+
Assert.StartsWith(CookieA, headerValues);
153+
Assert.EndsWith(CookieB, headerValues);
154+
155+
await _next.Invoke(context);
156+
}
157+
}
158+
}
159+
public class HttpProxyCookieTests_Http1 : HttpProxyCookieTests
160+
{
161+
public override HttpProtocols HttpProtocol => HttpProtocols.Http1;
162+
163+
// Using simple TcpClient since HttpClient will always concatenate cookies with comma separator.
164+
public override async Task ProcessHttpRequest(Uri proxyHostUri)
165+
{
166+
using var client = new TcpClient(proxyHostUri.Host, proxyHostUri.Port);
167+
using var stream = client.GetStream();
168+
using var writer = new StreamWriter(stream, Encoding.ASCII);
169+
using var reader = new StreamReader(stream, Encoding.ASCII);
170+
171+
await writer.WriteAsync($"GET / HTTP/1.1\r\n");
172+
await writer.WriteAsync($"Host: {proxyHostUri.Authority}\r\n");
173+
await writer.WriteAsync($"Cookie: {Cookies}\r\n");
174+
await writer.WriteAsync($"Connection: close\r\n");
175+
await writer.WriteAsync($"\r\n");
176+
await writer.FlushAsync();
177+
178+
string line = null;
179+
string statusLine = null;
180+
while ((line = await reader.ReadLineAsync()) != null)
181+
{
182+
statusLine ??= line;
183+
}
184+
185+
Assert.Equal($"HTTP/1.1 200 OK", statusLine);
186+
}
187+
}
188+
189+
#if NET5_0
190+
public class HttpProxyCookieTests_Http2 : HttpProxyCookieTests
191+
{
192+
public override HttpProtocols HttpProtocol => HttpProtocols.Http2;
193+
194+
// HttpClient for H/2 will use different header frames for cookies from a container and message headers.
195+
// It will first send message header cookie and than the container one and we expect them in the order of cookieA;cookieB.
196+
public override async Task ProcessHttpRequest(Uri proxyHostUri)
197+
{
198+
var handler = new HttpClientHandler();
199+
handler.CookieContainer.Add(new System.Net.Cookie(CookieBKey, CookieBValue, path: "/", domain: proxyHostUri.Host));
200+
var client = new HttpClient(handler);
201+
var message = new HttpRequestMessage(HttpMethod.Get, proxyHostUri);
202+
message.Version = HttpVersion.Version20;
203+
message.VersionPolicy = HttpVersionPolicy.RequestVersionExact;
204+
message.Headers.Add(HeaderNames.Cookie, $"{CookieA}");
205+
using var response = await client.SendAsync(message);
206+
response.EnsureSuccessStatusCode();
207+
}
208+
}
209+
#elif NETCOREAPP3_1
210+
// Do not test HTTP/2 on .NET Core 3.1
211+
#else
212+
#error A target framework was added to the project and needs to be added to this condition.
213+
#endif
214+
}

0 commit comments

Comments
 (0)