Mitigation of wrong cookie value separator #451
Conversation
Tratcher
left a comment
Tratcher
left a comment
There was a problem hiding this comment.
Looks like a good start. Most of my comments are about simplifying the logic and tests.
ManickaP
force-pushed
the
mapichov/437_cookie_separator
branch
from
6b227ba to
65708d0
Compare
ManickaP
force-pushed
the
mapichov/437_cookie_separator
branch
from
65708d0 to
3b590b5
Compare
Tratcher
left a comment
Tratcher
left a comment
There was a problem hiding this comment.
The fix looks good. Some minor comments left on the tests.
| 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. |
There was a problem hiding this comment.
| // To mitigate this, we concatenate them manually and put them back as a single header value. | |
| // 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 |
There was a problem hiding this comment.
I don't see this change in your PR?
There was a problem hiding this comment.
Sorry, my bad, should be there now.
| } | ||
| public class HttpProxyCookieTests_Http1 : HttpProxyCookieTests |
There was a problem hiding this comment.
| } | |
| public class HttpProxyCookieTests_Http1 : HttpProxyCookieTests | |
| } | |
| public class HttpProxyCookieTests_Http1 : HttpProxyCookieTests |
| // Ensure that CookieA is the first and CookieB the last. | ||
| Assert.True(context.Request.Headers.TryGetValue(HeaderNames.Cookie, out var headerValues)); | ||
| Assert.StartsWith(CookieA, headerValues); | ||
| Assert.EndsWith(CookieB, headerValues); |
There was a problem hiding this comment.
Careful what you're asserting here, you're expecting different results for HTTP/1 and HTTP/2.
headerValues is a StringValues, but you're passing it to StartsWith which takes a string. There's an implicit converter from StringValues to string which concatenates with commas (just like the original bug). Rather than using StartsWith, you want to check the count and value of each StringValues entry to make sure it's what you expect. E.g. Count should be 1 for HTTP/1 and 2 for HTTP/2.
There was a problem hiding this comment.
Added different set of asserts per context.Request.Protocol.
| { | ||
| public override HttpProtocols HttpProtocol => HttpProtocols.Http1; | ||
|
|
||
| // Using simple TcpClient since HttpClient will always concatenate cookies with comma separator. |
There was a problem hiding this comment.
| // Using simple TcpClient since HttpClient will always concatenate cookies with comma separator. |
| { | ||
| using var client = new HttpClient(); | ||
| using var message = new HttpRequestMessage(HttpMethod.Get, proxyHostUri); | ||
| message.Headers.Add(HeaderNames.Cookie, $"{Cookies}"); |
There was a problem hiding this comment.
| message.Headers.Add(HeaderNames.Cookie, $"{Cookies}"); | |
| message.Headers.Add(HeaderNames.Cookie, Cookies); |
2 participants
HttpClientwrongly uses comma (,) as a separator for cookie header values: dotnet/runtime#42856. As well as Kestrel probably doesn't merge cookie header frames for HTTP/2: dotnet/aspnetcore#26461.This PR mitigates this by collecting all cookie headers while copying them. And concatenating them manually with semi-colon (
;) before adding it as a single header to the destination request.Fixes #437