-
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
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.
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.
Few small questions, otherwise LGTM.
@@ -67,7 +67,7 @@ public sealed class Cookie | |||
private string m_value = string.Empty; // Do not rename (binary serialization) | |||
private int m_version; // Do not rename (binary serialization) | |||
|
|||
private string m_domainKey = string.Empty; // Do not rename (binary serialization) | |||
private string? m_domainKey; // Do not rename (binary serialization) |
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.
Why making this nullable? What was wrong with string.Empty
? Does string.Empty
now have special meaning?
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.
string.Empty
is a "valid" domain according to our logic. It would only match a string.Empty
host though, not sure if such a hostname can show up in practice. string? m_domainKey = null
means that m_domainKey
has not been initialized yet which is IMO a better state after Cookie
construction than if we assigned an arbitrary value. Setting an explicit Domain
or running VerifyAndSetDefaults()
should assign a m_domainKey
.
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
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.