-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
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);
}
…time into cookie-domain-02
// For implicit domains exact match is needed | ||
if (cookie.DomainImplicit && !string.Equals(host, cookie.Domain, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
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.
With #39781 and #64038 we shifted the .NET
Cookie
/CookieContainer
behavior towards RFC 6265 for handling the leading dot in theDomain
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:
GetCookies(uri)
does not apply the RFC 6265 domain-matching for implicit domains. An implicit domain string has to be identical to host in order to match it. (This is codified in the testGetCookies_AddCookiesWithImplicitDomain_CookiesReturnedOnlyForExactDomainMatch
).The change results in the deletion of a significant amount of obsolete logic, and the following behavioral changes:
Domain="test.com"
andDomain=".test.com"
are now fetching the same cookies. This is a desired fix for CookieContainer doesn't replace Cookie with same name #84820.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: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.