Open atifaziz opened 5 years ago
cc: @davidsh
Cross-referencing dotnet/runtime#27764 since it also seems to be related to differences in handling of cookies between .NET Core and .NET Framework when using HttpWebRequest
/HttpWebResponse
API family.
Re-opening due to accidental close of the wrong issue. Sorry.
This seems like a regression from the fact the implementation of HttpWebRequest is different. It is technical breaking change, but semantically the behavior is correct. Given we have only 1 complaint, this does not seem to be high-value enough to fix it. Closing.
but semantically the behavior is correct.
How is it semantically correct?
Given we have only 1 complaint
How many do you need? It's a shame that I took quite some trouble, I think, to provide a comprehensive issue report with code to reproduce the regression and it gets closed after 2 months as not important enough. If you're not going to fix it then, at the very least, it would have been good to:
HttpWebResponse.Headers
documentation. /cc @KathleenDollard How is it semantically correct?
According to RFC 6265, there is no order dependencies of cookies received by 'Set-Cookie' headers. So, a user-agent (client) should process the cookies the same way regardless of the order they appear within one or more 'Set-Cookie' response headers.
This seems like a regression from the fact the implementation of HttpWebRequest is different.
In .NET Core, the HttpWebRequest API is built on top of the HttpClient API. HttpClient coalesces all the 'Set-Cookie' response headers into a single array of cookies. And that is why it appears as a single 'Set-Cookie' header as viewed by the HttpWebRequest API. This is different from the .NET Framework implementation of HttpWebRequest. But in practice, we haven't seen any broken applications due to this since according to the RFC, a client shouldn't expect the cookies to be ordered in any particular way from the server.
Add a compatibility note to the HttpWebResponse.Headers documentation. /cc @KathleenDollard
This is a good point. Feel free to open an issue in https://github.com/dotnet/dotnet-api-docs/issues. Or you can even submit a PR to change the documentation to add more info about this. The 'Remarks' section of the API docs is where we currently put compatibility notes like this.
In .NET Core, the HttpWebRequest API is built on top of the HttpClient API. HttpClient coalesces all the 'Set-Cookie' response headers into a single array of cookies. And that is why it appears as a single 'Set-Cookie' header as viewed by the HttpWebRequest API.
Right, that's the explanation and what I suspected was happening.
This is different from the .NET Framework implementation of HttpWebRequest.
That's the only issue being reported here. It's not about ordering.
But in practice, we haven't seen any broken applications due to this since according to the RFC
Well, it certainly broke our applications.
a client shouldn't expect the cookies to be ordered in any particular way from the server.
Again, it's not about ordering. It's simply that the same collection is delivering Set-Cookie
as a single string (multiple Set-Cookie
folded into one and separated by commas) and the other delivering multiple values for the same header. The latter requires less parsing. Returning the Set-Cookie
header as a single string is, in fact incorrect, because the header value syntax is invalid per RFC 6265:
4.1.1. Syntax
Informally, the Set-Cookie response header contains the header name
"Set-Cookie" followed by a ":" and a cookie. Each cookie begins with
a name-value-pair, followed by zero or more attribute-value pairs.
Servers SHOULD NOT send Set-Cookie headers that fail to conform to
the following grammar:
set-cookie-header = "Set-Cookie:" SP set-cookie-string
set-cookie-string = cookie-pair *( ";" SP cookie-av )
cookie-pair = cookie-name "=" cookie-value
What's worse, the same collection returns different results depending on the GetValues
overload used! For example, try the following:
var request = WebRequest.CreateHttp("https://my.visualstudio.com/");
using var response = request.GetResponse();
var headers = response.Headers;
var i = Array.IndexOf(headers.AllKeys, "Set-Cookie");
if (i < 0)
throw new Exception("Set-Cookie header is absent.");
foreach (var v in headers.GetValues(i)) // returns all cookies in one string
Console.WriteLine("Set-Cookie: " + v);
Console.WriteLine();
foreach (var v in headers.GetValues("Set-Cookie")) // returns cookies as separate strings
Console.WriteLine("Set-Cookie: " + v);
The output should be as follows:
Set-Cookie: buid=AQABAAEAAAAP0wLlqdLVToOpA4kwzSnxB9ifhMzWnEktRTgnB23g5k0aFCzDcvv_M1GLFDswsPBG15PkjNZPK1EZ_ZRhFPABtvQIkPetS-ikrW1MdjdGhN9fDn_UO5VRnUrI_4oZgT0gAA; expires=Wed, 16-Oct-2019 12:02:10 GMT; path=/; secure; HttpOnly, fpc=Atc8k94GbTlPs266cvYW0e7dicmqAQAAAMJwEdUOAAAA; expires=Wed, 16-Oct-2019 12:02:10 GMT; path=/; secure; HttpOnly, esctx=AQABAAAAAAAP0wLlqdLVToOpA4kwzSnxLej6lcOGTIwDU1w0V4yZP3cj4JEUSzfg2K7MI5yoD_muzd2Q7Uj7PvIdSUiuVMqaYyR3Wmhl4Ly86EkDJC4s0yvhbQrveTFisym6WNTz-k9txMoFCYZtlrRxdXOEyJA_gkc8pS5GYMvZQqIuigd89HvWDKZrblUAZVk2kwmec30gAA; domain=.login.microsoftonline.com; path=/; secure; HttpOnly, x-ms-gateway-slice=prod; path=/; secure; HttpOnly, stsservicecookie=ests; path=/; secure; HttpOnly
Set-Cookie: buid=AQABAAEAAAAP0wLlqdLVToOpA4kwzSnxB9ifhMzWnEktRTgnB23g5k0aFCzDcvv_M1GLFDswsPBG15PkjNZPK1EZ_ZRhFPABtvQIkPetS-ikrW1MdjdGhN9fDn_UO5VRnUrI_4oZgT0gAA; expires=Wed, 16-Oct-2019 12:02:10 GMT; path=/; secure; HttpOnly
Set-Cookie: fpc=Atc8k94GbTlPs266cvYW0e7dicmqAQAAAMJwEdUOAAAA; expires=Wed, 16-Oct-2019 12:02:10 GMT; path=/; secure; HttpOnly
Set-Cookie: esctx=AQABAAAAAAAP0wLlqdLVToOpA4kwzSnxLej6lcOGTIwDU1w0V4yZP3cj4JEUSzfg2K7MI5yoD_muzd2Q7Uj7PvIdSUiuVMqaYyR3Wmhl4Ly86EkDJC4s0yvhbQrveTFisym6WNTz-k9txMoFCYZtlrRxdXOEyJA_gkc8pS5GYMvZQqIuigd89HvWDKZrblUAZVk2kwmec30gAA; domain=.login.microsoftonline.com; path=/; secure; HttpOnly
Set-Cookie: x-ms-gateway-slice=prod; path=/; secure; HttpOnly
Set-Cookie: stsservicecookie=ests; path=/; secure; HttpOnly
So the API is even inconsistent with itself, which is more than a regression bug!
@mairaw A compatibility note in the docs would be useful on this.
Returning the Set-Cookie header as a single string is, in fact incorrect, because the header value syntax is invalid per RFC 6265.
and separated by commas
You are correct that using comma as the delimiter between cookies in the single 'Set-Cookie' header is incorrect. The delimiter in that case should be a semicolon.
We will investigate if we can at least correct the delimiter problem even if we still have to have a single 'Set-Cookie' header.
What exactly do we want to say in the docs for this? I can talk about the different implementation on which each framework is built on top of, that ordering is not guaranteed, but what about the bug he's reporting? Do we need the investigation to be concluded before we update the docs?
Do we need the investigation to be concluded before we update the docs?
@mairaw Yes. Let's wait on any doc changes for now until we finish investigating. We will open a separate doc issue in the dotnet/dotnet-api-docs repo once that is done.
@davidsh Thanks for reconsidering this.
The delimiter in that case should be a semicolon.
This won't be helpful as semi-colon (;
) is already taken in the cookie syntax to delimit attribute-value pairs (per section 4.1.1 of RFC 6252):
set-cookie-string = cookie-pair *( ";" SP cookie-av )
We will investigate if we can at least correct the delimiter problem even if we still have to have a single 'Set-Cookie' header.
Why try so hard to return a single header when GetValues(header)
does the right thing already and returns each Set-Cookie
header separately as an array of strings? It's just GetValues(index)
that's the problem. Even if the docs add a compatibility note (thanks @KathleenDollard and @mairaw for taking note), no one in their right mind would use the overload with the regression.
I don't think we're trying hard to keep it the same way -- it is more that we try hard to not make changes without fully understanding their scope.
The current implementation is clearly broken, so I think we need to make some change that will break anyone depending on a comma being there.
I'm leaning towards reverting to the old behavior of returning separately.
I don't think we're trying hard to keep it the same way -- it is more that we try hard to not make changes without fully understanding their scope.
I can completely appreciate that.
I'm leaning towards reverting to the old behavior of returning separately.
Can't say I don't second that. 😉
Just in case, as a workaround: Implementation of HttpResponseMessage doesn't have this issue and can be used instead of HttpWebResponse. So you have two choices:
HttpClient client = new HttpClient();
var resp = await client.GetAsync(your_url);
var headerValues = resp.Headers.GetValues("Set-Cookie");
internal static IEnumerable<string> GetSetCookieHeaderValues(HttpWebResponse response)
{
if (response == null || !response.Headers.AllKeys.Contains("set-cookie", StringComparer.OrdinalIgnoreCase))
{
return null;
}
string headerName = response.Headers.AllKeys.First(k => string.Equals(k, "set-cookie", StringComparison.OrdinalIgnoreCase));
BindingFlags bindFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic
| BindingFlags.Static;
FieldInfo field = response.GetType().GetField("_httpResponseMessage", bindFlags);
return (field.GetValue(response) as HttpResponseMessage)?.Headers.GetValues(headerName);
}
Apparently this issue breaks Azure Function Proxies: https://github.com/Azure/azure-functions-host/issues/4486 Browsers (e.g. currently latest Chrome and Firefox) ignore all other cookies but the first one. This is not a small issue.
Ouchie. Trying to get a .NET 6 V4 http triggered Azure Function to return multiple set-cookie headers and nothing is working, including using a HttpResponseMessage
return type. It works locally, but not when deployed. This is painful.
This is still a relevant issue.
Right now, we're thinking of adding a proxy in front of our services that restores the cookie functionality. The proxy will be written in something else than C#, unfortunately.
I'm amazed that the dotnet core team is okay with all Azure functions having a broken functionality .. I'd love to write a PR for this, but I don't know where to start, tbh.
Looking at the kestrel code naively, I would probably implement this cookie handling on these three lines https://github.com/dotnet/aspnetcore/blob/fe0fbff040c9f129ba3f5ee8cf15c8f6d96fb50e/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L959 https://github.com/dotnet/aspnetcore/blob/fe0fbff040c9f129ba3f5ee8cf15c8f6d96fb50e/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L971 https://github.com/dotnet/aspnetcore/blob/fe0fbff040c9f129ba3f5ee8cf15c8f6d96fb50e/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs#L978
Under .NET Framework,
HttpWebResponse.Headers
can deliver theSet-Cookie
header value as multiple values, where each value represents one cookie.HttpWebResponse.Headers
is aWebHeaderCollection
and invokingGetValues("Set-Cookie")
returns an array of strings where each string is a single cookie. In .NET Core, however, the same returns the entire header as a single string; that isGetValues("Set-Cookie")
always returns an array of one string with comma-separated cookies. This seems to be a compatibility bug that yields different results at run-time when the same code is executed under .NET Framework and .NET Core.I have created a self-contained program to demonstrate the issue:
When run as a .NET Core 2.2 application, this program will do the following:
foo=bar
andbar=baz
).dotnet run
but with the--framework net471
option.The output of the program shows the difference in behaviour:
Specifically, under .NET Core, we see a single
Set-Cookie
line with both cookies:whereas under .NET Framework, we see two, one per cookie:
I have uploaded a ZIP archive with the full project:
📎
CookiesBugDemo.zip
Simply unzip and execute the
run.cmd
batch script included.More Information
dotnet --info
says: