Cookie doesn't get replaced in CookieContainer

[radischevo]

Recently I ran into an issue communicating with a remote website that sets cookies with inconsistent Domain attributes.
Consider the following repro:

var uri = new Uri("https://example.com/", UriKind.Absolute);
var container = new CookieContainer();

// First HTTP response...
container.SetCookies(uri, "foo=bar; Path=/; Domain=example.com");

// Second HTTP response...
container.SetCookies(uri, "foo=baz; Path=/; Domain=.example.com");

Console.WriteLine("Got {0} cookies, expected 1. ", container.GetCookies(uri).Count);

I expected that the second cookie would overwrite the first one, but there are now two cookies in the container instead. When using curl, the cookie gets replaced though, the same way browsers behave.

The issue reproduces on both .NET Core 5.0.402 (Mac) and .NET Framework 4.8 (Win).

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Recently I ran into an issue communicating with a remote website that sets cookies with inconsistent Domain attributes.
Consider the following repro:

var uri = new Uri("https://example.com/", UriKind.Absolute);
var container = new CookieContainer();

// First HTTP response...
container.SetCookies(uri, "foo=bar; Path=/; Domain=example.com");

// Second HTTP response...
container.SetCookies(uri, "foo=baz; Path=/; Domain=.example.com");

Console.WriteLine("Got {0} cookies, expected 1. ", container.GetCookies(uri).Count);

I expected that the second cookie would overwrite the first one, but there are now two cookies in the container instead. When using curl, the cookie gets replaced though, the same way browsers behave.

The issue reproduces on both .NET Core 5.0.402 (Mac) and .NET Framework 4.8 (Win).

Author:	radischevo
Assignees:	-
Labels:	area-System.Net, untriaged
Milestone:	-

[karelz]

Triage: We should look at what browsers do, and potentially look at the spec.
The domains seem to be of different scope at first look.

[Laiteux]

I ran accross the exact same issue today, and it took me a while to figure out why.

I'm glad I stumbled upon this issue, so I can confirm this bug, which happens with the use of CookieContainer.SetCookies.

A hacky fix would've been to use http://.example.com as uri with SetCookies, however this will throw an exception since it is not an actually valid URI.

---

This is how I was able to fix it on my side, so that the CookieContainer will end up handling future duplicates and remove them, just like I originally wanted it to:

foreach (string cookie in cookieHeader.Split("; "))
{
    string[] cookieSplit = cookie.Split('=', 2);

    cookieContainer.Add(new Cookie(cookieSplit[0], cookieSplit[1], "/", ".example.com"));
}

Note that this is a personal solution to my own problem and might not work for you depending on your case.
I'm just leaving this here as a note for future users who might encounter the same issue, as well as myself 😉

[radischevo]

@Laiteux, Yes, I also fixed it. I was writing a tool that reveals shortened URLs, so I had to develop a universal workaround. Now I allocate a temporary CookieContainer for each request to avoid parsing cookies manually, and then I copy the relevant cookies to the main container, adding a leading dot to their domains if necessary. So now all cookies in the main container have domains with a leading dot.

The real-world example is https://a.aliexpress.com/_A2dGGE:

1. GET https://a.aliexpress.com/_A2dGGE — HTTP 302
2. GET https://aliexpress.ru/item/32842673737.html?[truncated] — HTTP 302, and sets the xman_f cookie with aliexpress.ru domain (NO leading dot).
3. GET https://login.aliexpress.com/sync_cookie_read.htm?[truncated] — HTTP 302
4. GET https://login.aliexpress.ru/sync_cookie_write.htm?[truncated] — HTTP 302, and sets the xman_f cookie (twice for some reason), but now the domain is .aliexpress.ru (WITH leading dot).
5. GET https://aliexpress.ru/item/32842673737.html — responds with HTTP 200 if the xman_f was handled correctly; otherwise, the redirect chain will repeat from the step three.

As mentioned before, I tested this in Safari 14.1.2 and with curl, and both were able to handle these redirects.

[zryogi]

According to RFC6265 4.1.2.3:

(Note that a leading %x2E ("."), if present,
is ignored even though that character is not permitted, but a
trailing %x2E ("."), if present, will cause the user agent to ignore
the attribute.)

It is also the implementation in curl:

https://github.com/curl/curl/blob/e7416cfd2bd58d256b8524f31ef22a43aa23a970/lib/cookie.c#L133-L145

Is this the correct approach to follow? If it is, may be I can try and look into it.

[zryogi]

My take is that additional logic needs to be added at

runtime/src/libraries/Common/src/System/Net/CookieComparer.cs

Line 17 in a3b186b

if ((result = string.Compare(left.Domain, right.Domain, StringComparison.OrdinalIgnoreCase)) != 0)

So when Domain names are compared if the domain contains a leading "." it should be ignored so ".domain" and "domain" are considered equal.

This will allow InternalAdd to replace the cookie if isStrict is set to true.

[zryogi]

Just tested my approach and seems to be working alright:

The following code:

using System.Net;

Console.WriteLine("Waiting to attach");
Console.ReadLine();

var uri = new Uri("https://example.com/", UriKind.Absolute);
var container = new CookieContainer();

// First HTTP response...
container.SetCookies(uri, "foo=bar; Path=/; Domain=example.com");

// Second HTTP response...
container.SetCookies(uri, "foo=baz; Path=/; Domain=.example.com");

Console.WriteLine("Got {0} cookies, expected 1. ", container.GetCookies(uri).Count);
Console.WriteLine("Got {0} cookies, expected 1. ", container.GetCookies(uri)[0].Domain);

Yields:

Adding the following method to CookieComparer.cs:

internal static string? RemoveLeadingDot(string? s)
{
    if (string.IsNullOrEmpty(s))
    {
        return s;
    }

    if (s[0] == '.')
    {
        return s[1..];
    }

    return s;
}

An changing the Compare method:

internal static int Compare(Cookie left, Cookie right)
{
    int result;

    if ((result = string.Compare(left.Name, right.Name, StringComparison.OrdinalIgnoreCase)) != 0)
    {
        return result;
    }

    string? leftCookieDomain = RemoveLeadingDot(left.Domain);
    string? rightCookieDomain = RemoveLeadingDot(right.Domain);

    if ((result = string.Compare(leftCookieDomain, rightCookieDomain, StringComparison.OrdinalIgnoreCase)) != 0)
    {
        return result;
    }

    // NB: Only the path is case sensitive as per spec. However, many Windows applications assume
    //     case-insensitivity.
    return string.Compare(left.Path, right.Path, StringComparison.Ordinal);
}

I also have ready unit testing. If @karelz has no objections I can go ahead with the PR.

[radischevo]

@ZeroRyogi, looks good, but if the domain begins with a dot, an extra string will be allocated per each RemoveLeadingDot call. I'd suggest you to use a string.Compare overload, that does not require allocation at all (in our case). So the comparer could look like this:

internal static int Compare(Cookie left, Cookie right)
{
    int result;
	
    if ((result = string.Compare(left.Name, right.Name, StringComparison.OrdinalIgnoreCase)) != 0)
    {
        return result;
    }

    if ((result = CompareDomain(left.Domain, right.Domain)) != 0)
    {
        return result;
    }

    // NB: Only the path is case sensitive as per spec. However, many Windows applications assume
    //     case-insensitivity.
    return string.Compare(left.Path, right.Path, StringComparison.Ordinal);
}
	
private static int CompareDomain(string? left, string? right) 
{
    if (left is null) 
    {
        return right is null ? 0 : -1;
    }
	
    if (right is null) 
    {
        return 1;
    }
	
    var indexLeft = GetComparisonStartIndex(left);
    var indexRight = GetComparisonStartIndex(right);

    return string.Compare(left, indexLeft, right, indexRight, left.Length - indexLeft);
}

private static int GetComparisonStartIndex(string domain) 
    => domain.Length != 0 && domain[0] == '.' ? 1 : 0;

[zryogi]

Thank you! good thinking. Feel like this should have been a Code Review already 😄. Will improve and wait for the response to check if this is the correct approach in order to send the PR.

[radischevo]

Well, I did some thinking, and I should say that while tweaking the CookieComparer definitely solves the problem, I'm not sure that we now comply with the RFC well. If we are told to ignore the leading dot in the domain, probably it's the Cookie class itself, that should trim the leading dot on assignment? That's not the simplest solution, 'cause it requires much more logic to be changed or at least reviewed, but it could be the right one. What do you think?