Continuation of encoding in Cookie discussion

[aidapsibr]

Refers to #535 & PR #546

Also open on Katana: http://katanaproject.codeplex.com/workitem/442

@Tratcher

Forcefully encoding and forcefully decoding breaks interoperability with existing solutions other than Microsoft.Owin. Furthermore cookie handling is now non-symmetrical in operations. Both the write and read use Uri.EscapeDataString, but the read operation does extra work which invalidates Base64.

public static RequestCookieCollection Parse(IList<string> values)
        {
            if (values.Count == 0)
            {
                return Empty;
            }

            IList<CookieHeaderValue> cookies;
            if (CookieHeaderValue.TryParseList(values, out cookies))
            {
                if (cookies.Count == 0)
                {
                    return Empty;
                }

                var store = new Dictionary<string, string>(cookies.Count);
                for (var i = 0; i < cookies.Count; i++)
                {
                    var cookie = cookies[i];
                    var name = Uri.UnescapeDataString(cookie.Name.Replace('+', ' '));
                    var value = Uri.UnescapeDataString(cookie.Value.Replace('+', ' '));
                    store[name] = value;
                }

                return new RequestCookieCollection(store);
            }
            return Empty;
        }

[aidapsibr]

#515 is related too it seems as is #390

[aidapsibr]

An example of Base64 cookie interop with ASPNET without OWIN and with as was needed in Microsoft.OWIN:

            app.UseCookieAuthentication(
                new CookieAuthenticationOptions
                {
                    AuthenticationType = WsFederationAuthenticationDefaults.AuthenticationType,
                    CookieDomain = ".domain.com",
                    CookieName = "PreExistingCookieName",
                    TicketDataFormat = new WifTokenFormat(), //Replaces ' ' with '+' cause yeah.
                    SlidingExpiration = true,
                    Provider = new CookieAuthenticationProvider
                    {
                        OnResponseSignedIn = context =>
                        {
                            context.Response.Headers["Set-Cookie"] = Uri.UnescapeDataString(context.Response.Headers["Set-Cookie"]
                                .Replace(' ', '+'));
                        }
                    }
                });

[aidapsibr]

Would it be possible with the new DI services in ASPNET to inject a CookieEncodingService instead of hard-coding an impl, I think that would satisfy both camps.

[osudude]

@psibernetic Great idea!

[Tratcher]

#515 is unrelated, it's a parsing failure, so it never even gets to the un-escaping step.

[Tratcher]

@psibernetic please add sample data inputs and outputs for both the request and response headers.

[aidapsibr]

I have created a sample project using Katana to demonstrate the functionality that was ported over that is causing issues, it should adequately show the bug.

The example data I used was:

//Had to come up with a string that would output a + in the encoded result
// xml seems to do the trick.
const string Base64ExampleText = @"<tag attribute=""><childtag></tag>";

Raw base64 output and what is in my cookie:

PHRhZyBhdHRyaWJ1dGU9Ij48Y2hpbGR0YWc+PC90YWc+

IOwinContext.Request.Cookies output:

PHRhZyBhdHRyaWJ1dGU9Ij48Y2hpbGR0YWc PC90YWc 

Which throws Invalid length for a Base-64 char array or string.

https://github.com/psibernetic/KatanaCookieBase64IssueExample

I haven't had time to use the ASPNET 5 implementation yet, but the code was ported over from Katana.

Additionally,
RequestCookieCollection and ResponseCookies decode differently.

@Tratcher

Thanks.

[aidapsibr]

Adding a link to RFC 4648 which describes base64url encoding scheme. I believe this should be the default strategy moving forward, but should be configurable for each cookie for interoperability. ExpressJS does this through a Middleware option function called encode for setting cookies, but makes no assumption on how cookies are to be read.

[Tratcher]

I went digging through the Katana code to figure out why .Replace('+', ' ') was there in the first place.
http://katanaproject.codeplex.com/SourceControl/latest#src/Microsoft.Owin/Infrastructure/OwinHelpers.cs
I found that ParseDelimited was shared between forms, queries, and cookies, and I think the + replacement was intended for queries.

We no longer share parsers between these components so I will remove this special case from the cookie code path. This will allow us to read base64 cookies as-is. This does not affect any of the other encoding or decoding.

[aidapsibr]

Thanks for the the resolution @Tratcher.

[BrandonClapp]

@psibernetic @Tratcher

In addition to +, the / character also seems to still be getting url encoded to %2F. Both + and / are valid Base64 characters.

In ResponseCookies.Append(string key, string value), it appears that we're still URL escaping the values before they are appended to the cookie.

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

Should the / also be left unescaped? Currently, it's breaking interop with our existing Base64 encoded cookies. In order to get around this, we are having to manually decode these characters in the cookie after the Append method URL encodes it.

[Tratcher]

@BrandonClapp Please open a new issue and include example values.

[LordRhialto]

We are running Microsoft.Owin v3.1.0 and are still experiencing this problem:

String identityCookie = Context.Request.Cookies[identityCookieName];

returns a string value with base64 encoded data in it (here, only a part has been shown) eKUiOV0mSV6Kzk 4G4R8w/sgh6BbO instead of a space there should be a +.

[Tratcher]

@LordRhialto Please report Microsoft.Owin issues to https://github.com/aspnet/aspnetkatana