Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize CookieContainer domain-matching #112604

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Feb 15, 2025

With #39781 and #64038 we shifted the .NET Cookie/CookieContainer behavior towards RFC 6265 for handling the leading dot in the Domain attribute, however there were gaps left primarily because the the cookie code is complicated and full of logic that handles legacy requirements from old RFC-s related to now obsolete attributes. The existing obscure logic results in weird unintended behaviors under the hood and the bug #84820.

This PR is addressing this problem by adapting the RFC 6265 domain matching algorithm with a few modifications in order to avoid breaking changes:

The change results in the deletion of a significant amount of obsolete logic, and the following behavioral changes:

  1. Leading dots in Domain are now ignored everywhere, Domain="test.com" and Domain=".test.com" are now fetching the same cookies. This is a desired fix for CookieContainer doesn't replace Cookie with same name #84820.
  2. Handling of obsolete attributes in Domain-related path. With RFC 6265 being 14 years old and all modern browsers are ignoring these attributes, I think the impact of this change should be negligible.
  3. Deleted logic from CookieContainer that local domain in certain domain-matching edge cases. As far as I was able to figure out, it was responsible to provide the following (untested!) behavior:
// IPGlobalProperties.GetIPGlobalProperties().DomainName is "contoso.com"
CookieContainer cc = new();
cc.Add(new Uri("http://test.contoso.com"), new Cookie("a", "b"));
CookieCollection coll = cc.GetCookies(new Uri("http://test")); // GetCookies appends contoso.com to the uri so it's going to match
Assert.Equal(1, coll.Count);

There should be no significant behavioral changes beyond the ones I listed.

Fixes #84820
Fixes #112602
Contributes to #40328 (addressing point 2)


A final note: the PR does not attempt to go beyond RFC 6265 and doesn't attempt to emulate browser behavior. If we want to move to this direction, we should be looking at the current draft RFC https://datatracker.ietf.org/doc/draft-ietf-httpbis-rfc6265bis/, which introduces further tweaks to domain-matching, most importantly the rejection of public suffixes.

@Copilot Copilot bot review requested due to automatic review settings February 15, 2025 20:23

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • src/libraries/System.Net.Primitives/src/System.Net.Primitives.csproj: Language not supported
  • src/libraries/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj: Language not supported
  • src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/HostInformation.cs: Evaluated as low risk
  • src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest.cs: Evaluated as low risk
  • src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest/CookieContainerTest.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs:333

  • Ensure that the new method HostMatchesDomain is covered by tests to verify its behavior.
private static bool HostMatchesDomain(ReadOnlySpan<char> host, ReadOnlySpan<char> domain)

src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs:647

  • [nitpick] The test method name '_OmgOmg' is not descriptive. It should be renamed to 'Test_InvalidIPAddressWithDomainName'.
[Fact]
public void _OmgOmg()
{
bool valid = IPAddress.IsValid("0.url1.com");
Assert.False(valid);
}
Comment on lines +806 to +807
// For implicit domains exact match is needed
if (cookie.DomainImplicit && !string.Equals(host, cookie.Domain, StringComparison.OrdinalIgnoreCase))
Copy link
Member Author

@antonfirsov antonfirsov Feb 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the thing that makes GetCookies_AddCookiesWithImplicitDomain_CookiesReturnedOnlyForExactDomainMatch pass. On main it happens by an accident: the domains passed to the method BuildCookieCollectionFromDomainMatches mismatch the host because of a leading dot difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants