[Cookie] CookieCollection.GetCookies() does not retrieve cookies prefixed with a "."

[robertmclaws]

Scope

Modernize the CookieContainer to fix a 7 year old bug concerning the ability to return ALL cookies for a given domain.

Rationale

A well documented (and entirely too long-standing) bug in the CookieContainer is that CookieContainer.GetCookies() only accepts a Uri, not a string. This means you cannot properly retrieve cookies that are prefixed with a dot. This is because URIs prefixed with a dot are invalid.

Previously-provided solutions involve using Reflection, but these are not ideal for various reasons... not to mention that the .NET Core version has slightly different internals, so the old solutions no longer apply.

I've provided a complete solution that functions on .NET Core here as an extension method, but it again requires reflection, and has not been vetted for proper security.

A proper solution would be to provide a version of GetCookies() that accepts a string domain, and is more intelligent about matching that domain, to return all of the Cookies that the user would expect in a given situation.

Usage

Here's a pseudocode example of how this would work.

public Class SomeClass
{
    private static HttpClient HttpClient;
    private static CookieContainer Cookies;

    static SomeClass()
    {
        Cookies = new CookieContainer();
        HttpClient = new HttpClient(
            new HttpClientHandler
            {
                CookieContainer = Cookies,
                AutomaticDecompression = DecompressionMethods.GZip,
                AllowAutoRedirect = false
            }
        );
    }
     /// <summary>
    /// 
    /// </summary>
    /// <returns></returns>
    public async Task<AccessToken> GetAccessToken()
    {
        AuthTicket authTicket;
        var cookies = Cookies.GetCookies("sso.somewebsite.com");
        // The "CASTGC" in this case is actually in the ".sso.somewebsite.com" domain and would not
        // be returned in the current codebase. 
        if (!cookies.Any(c => c.Name == "CASTGC") || cookies.Count == 0)
        {
            authTicket = await Login();
        }
        accessToken = await HttpClient.GetStringAsync("http://somewebsite.com");
        return accessToken;
    }

Implementation

Based on my Extension Method implementation previously mentioned, the implementation would likely look something like this:

        /// <summary>
        /// Returns all of the <see cref="Cookie">Cookies</see> where <see cref="Cookie.Domain"/> 
        /// contains part of the specified string. Will return cookies for any subdomain, as well as dotted-prefix cookies. 
        /// </summary>
        /// <param name="domain">The string that contains part of the domain you want to extract cookies for.</param>
        /// <returns></returns>
        public static IEnumerable<Cookie> GetCookies(string domain)
        {
            foreach (var entry in _domainTable)
            {
                if (entry.Key.Contains(domain))
                {
                    foreach (var li in entry.Value._list)
                    {
                        foreach (Cookie cookie in li.Value)
                        {
                            yield return cookie;
                        }
                    }
                }
            }
        }

Obstacles

There may be security implications with this design that I am not aware of, so the final implementation would likely need to pass a security review from @blowdart's team. Might also want to get @benaadams to review the entire CookieCollection code to make sure it is optimized for performance.

[blowdart]

Oh how interesting, I didn't even know this bug existed :)

[CIPop]

/cc @davidsh

My understanding is that there are two proposals:

1. the one in dotnet/corefx#14674 proposes that the cookie filtering should be changed to respect the latest RFC. This has app-compat risk.
2. the one in this issue is proposing a new API surface that will accept a domain (string) and would have the latest RFC behavior.

I lean towards having the new API and ensuring that new versions of components such as HttpClient use it internally (the solution presented in this issue). Not only this is less risky from an app-compat standpoint but it also allows for more flexibility as Uri doesn't appear to be the right type to encode the Cookie scope.

[mconnew]

@CIPop, Adding a new api doesn't fix issue dotnet/corefx#14674 as the cookie container throws when trying to add a cookie which conforms to the latest RFC or even the immediately prior RFC version. Adding a new api will still fail to retrieve the cookie as it doesn't even get into the collection in the first place.
Also, I don't understand how what the app-compat risk is? CookieContainer might have some extra legitimate cookies in it if you fix the parsing. If you add a new api, then you could make the prior rejected cookies only available by the new api which would mean zero behavior difference for existing clients.

[robertmclaws]

When @CIPop says "propose a new API" I think he's referring to some version of my Extension method that accepts a string and functions according to the spec. That would fix THIS issue, though not the one @mconnew is referring to. I also agree that System.Uri is the wrong type to hold the Cookie scope, especially given they can start with a period.

Truth be told, my personal opinion is that the entire CookieContainer implementation is garbage and needs to be rewritten from the ground up. Hopefully you guys are tackling that in dotnet/corefx#19138, and that you've made it simpler, instead of just rearranging the deck chairs (It does appear to be cleaner, though I have not dug through it completely).

I also feel like, if .NET Core 2.0 is a major version release, that it should be OK to break compatibility if the implementation is a fundamentally broken as it appears to be.

HTH :)

[caesar-chen]

From RFC 6265:

5.2.3.  The Domain Attribute
   ...
   If the first character of the attribute-value string is %x2E ("."):

      Let cookie-domain be the attribute-value without the leading %x2E
      (".") character.

   ...

If we update the code to support RFC 6265, the cookie-domain stored should not start with a ".".

[caesar-chen]

I have tried the repro attached in the original SO page with .NET Core 2.1 and .NET Framework 4.7.1. For test data "http://example.com", it correctly returns 2 cookies on both Core & Framework. I also inspected those 2 cookies, and they are the ones we are expecting (Test2=val, Test3=val) Probably the original issue has been resolved, if so, this API proposal is not necessary since we can correctly retrieve cookies (for any subdomain, as well as dotted-prefix cookies) with URI. (The second cookie Test2=val is with domain .example.com, with current codebase we have no trouble retrieving it).

@robertmclaws Do you still observe this issue, and think a new API has to be introduced? Are you able to do, like Cookies.GetCookies("http://sso.somewebsite.com") to retrieve cookies in the ".sso.somewebsite.com" domain?

Here is the test code I used (modified from SO description):

using System;
using System.Net;

namespace CookieTest
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            Test();
            Console.ReadLine();
        }

        static CookieContainer getContainer()
        {
            CookieContainer result = new CookieContainer();

            Uri uri = new Uri("http://sub.example.com");
            string cookieH = @"Test1=val; domain=sub.example.com; path=/";
            result.SetCookies(uri, cookieH);

            cookieH = @"Test2=val; domain=.example.com; path=/";
            result.SetCookies(uri, cookieH);

            cookieH = @"Test3=val; domain=example.com; path=/";
            result.SetCookies(uri, cookieH);

            return result;
        }

        static void Test()
        {
            string result = "";

            CookieContainer cookie = getContainer();
            result = "<br>Total cookies count: " + cookie.Count + "; expected: 3";
            Console.WriteLine(result);

            Uri uri = new Uri("http://sub.example.com");
            CookieCollection coll = cookie.GetCookies(uri);
            result = "<br>For " + uri + " Cookie count: " + coll.Count + "; expected: 3";
            Console.WriteLine(result);

            uri = new Uri("http://other.example.com");
            coll = cookie.GetCookies(uri);
            result = "<br>For " + uri + " Cookie count: " + coll.Count + "; expected: 2";
            Console.WriteLine(result);

            uri = new Uri("http://example.com");
            coll = cookie.GetCookies(uri);
            result = "<br>For " + uri + " Cookie count: " + coll.Count + "; expected: 2";
            Console.WriteLine(result);
        }
    }
}

[karelz]

Triage: Is it going to be part of the general RFC we are about to look at in Uri space?