Comma's in cookie value

[martijnboland]

I've had an issue where I wanted to store a serialized json array in a cookie. This is not allowed anymore because of the comma that appears in the serialized representation (e.g. "["value 1","value 2"]").
Comma's are indeed not supported in cookies, but in the past (System.Web, Owin), these were always encoded and this does not seem to be the case anymore.

I wonder, should we encode and decode these values ourselves or should the framework do that automatically.

[Tratcher]

This is for httpContext.Response.Cookies.Append(key, value); correct?

[martijnboland]

Yes

[muratg]

Moving this to Backlog as we will be in RC2 ask mode very soon. If you feel strongly about this issue, please ping me.

[brockallen]

Yes, this seems like an important issue to understand why it's happening and get fixed.

[muratg]

Yep, looks like a bug. Though we are crammed on time and we will revisit backlog bugs after RC2.

[CrispinH]

This bug is affecting my use of IdentityServer4 and presumably everyone else's too. Given Dominick Baier's blog post paragraph: "One year ago the ASP.NET team decided to discontinue that middleware and rather focus on consuming tokens instead. They also asked us if IdentityServer can be the replacement going forward." - this is needs fixing.

[Tratcher]

@leastprivilege, can you elaborate on how this affects IdentiyServer?

[brockallen]

We put the client id the user has logged into into a cookie. This is used for single sign out.

[Tratcher]

In a json format? or does the client id happen to contain a comma?

[brockallen]

We serailize to json and it's an array of strings. It's the exact same code in IdentityServer3 and that works under katana :)

Basically, the APIs to add the cookie seem to automatically URL encode the value. In ASP.NET Core it's odd that the value is encoded, but there's a "," left in the middle.

[brockallen]

Here's a link with the sample text that's being put into the cookie: IdentityServer/IdentityServer3#2446 (comment)

[Tratcher]

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http/ResponseCookies.cs#L65

[brockallen]

Right, so why is the default url encoder putting a comma in there? I can try to test it in better isolation.

[brockallen]

oh or are you showing that this was added recently?

[Tratcher]

I was just citing the problematic line of code.

[brockallen]

hmm, ok. i'll see if i can repro in a stand alone project

[brockallen]

This program:

public static void Main(string[] args)
{
    var data = new string[] { "foo", "bar" };
    var json = JsonConvert.SerializeObject(data);
    var value = UrlEncoder.Default.UrlEncode(json);
    Console.WriteLine(value);
}

Emits this (notice the comma):
%5B%22foo%22,%22bar%22%5D

The correct value should be:
%5B%22foo%22%2C%22bar%22%5D

So it's a bug in the UrlEncoder.Default for certain.

[Tratcher]

Commas are actually allowed in urls for some reason, so this is more of an issue that we're using an encoder that doesn't quite fit our needs. Adding an extra .Replace(",", "%2C") in there wouldn't be too bad.

[brockallen]

How did Katana do this? Perhaps there's precedent?

[Tratcher]

Uri.EscapeDataString http://katanaproject.codeplex.com/SourceControl/latest#src/Microsoft.Owin/ResponseCookieCollection.cs, which encodes anything except unreserved characters. I guess we could also try that.

[aidapsibr]

The reason the Uri.Escape and Unescape are used is there was an attempt at sharing the ParseDelimited method between Query string, form values, and cookies. Except Cookies have no requirement to be encoded and in fact that breaks base64 for interop between normal frameworks. The real solution here is to remove the Uri.Escape and Unescape and + with ' ' replacement from cookies at all. A middleware could optionally do that if desired, but baking that requirement into the framework makes no sense.

http://katanaproject.codeplex.com/workitem/442

[Tratcher]

No, the reason there's encoding here is because people pass in arbitrary values without an knowledge of what's allowed in the spec and expect it to work. See the user's original (invalid) data input.

[BrennanConroy]

Fixed via 765a520

[brockallen]

Very happy -- thank you all!