From 9ed720b68675b9c05cf4847ad64d082ec3a07d53 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Sat, 15 Feb 2025 20:40:31 +0100 Subject: [PATCH 1/3] Modernize Cookie domain-matching --- .../Common/src/System/Net/CookieComparer.cs | 7 +- .../src/System.Net.Primitives.csproj | 2 - .../src/System/Net/Cookie.cs | 215 +++++---------- .../src/System/Net/CookieContainer.cs | 248 +++--------------- .../tests/FunctionalTests/CookieTest.cs | 5 + .../CookieTest/CookieContainerTest.cs | 33 --- .../tests/FunctionalTests/IPAddressParsing.cs | 7 + .../tests/UnitTests/CookieContainerTest.cs | 159 ++++++++--- .../tests/UnitTests/Fakes/HostInformation.cs | 16 -- ...stem.Net.Primitives.UnitTests.Tests.csproj | 9 +- 10 files changed, 257 insertions(+), 444 deletions(-) delete mode 100644 src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/HostInformation.cs diff --git a/src/libraries/Common/src/System/Net/CookieComparer.cs b/src/libraries/Common/src/System/Net/CookieComparer.cs index a2980c5c8746ec..5a1e06f292362b 100644 --- a/src/libraries/Common/src/System/Net/CookieComparer.cs +++ b/src/libraries/Common/src/System/Net/CookieComparer.cs @@ -23,11 +23,8 @@ internal static bool Equals(Cookie left, Cookie right) } internal static bool EqualDomains(ReadOnlySpan left, ReadOnlySpan right) - { - if (left.StartsWith('.')) left = left.Slice(1); - if (right.StartsWith('.')) right = right.Slice(1); + => StripLeadingDot(left).Equals(StripLeadingDot(right), StringComparison.OrdinalIgnoreCase); - return left.Equals(right, StringComparison.OrdinalIgnoreCase); - } + internal static ReadOnlySpan StripLeadingDot(ReadOnlySpan s) => s.StartsWith('.') ? s[1..] : s; } } diff --git a/src/libraries/System.Net.Primitives/src/System.Net.Primitives.csproj b/src/libraries/System.Net.Primitives/src/System.Net.Primitives.csproj index 78b92c9adb6008..81f974b735bb8b 100644 --- a/src/libraries/System.Net.Primitives/src/System.Net.Primitives.csproj +++ b/src/libraries/System.Net.Primitives/src/System.Net.Primitives.csproj @@ -76,8 +76,6 @@ Link="Common\System\Net\SocketAddressExtensions.cs" /> - host, ReadOnlySpan domain) + { + if (!host.EndsWith(domain, StringComparison.Ordinal)) + { + return false; + } + + // The last character of the 'host' that is not included in the domain + int idxOfSeparator = host.Length - domain.Length - 1; + if (idxOfSeparator < 0) + { + // 'host' and 'domain' are equal + Debug.Assert(idxOfSeparator == -1); + return true; + } + + return host[idxOfSeparator] is '.' // The last character of 'host' that is not included in 'domain' is a "." + && domain.Contains('.') // In case of single-label domains, there should be an exact match. + && !IPAddress.IsValid(host); // If host is an IP address, there should be an exact match. } // According to spec we must assume default values for attributes but still @@ -323,27 +360,23 @@ private static bool IsDomainEqualToHost(string domain, string host) // // Afterwards, the function can be called many times with other URIs and // setDefault == false to check whether this cookie matches given uri - internal bool VerifySetDefaults(CookieVariant variant, Uri uri, bool isLocalDomain, string localDomain, bool setDefault, bool shouldThrow) + internal void VerifyAndSetDefaults(CookieVariant variant, Uri uri) { string host = uri.Host; int port = uri.Port; string path = uri.AbsolutePath; - bool valid = true; - if (setDefault) + // Set Variant. If version is zero => reset cookie to Version0 style + if (Version == 0) { - // Set Variant. If version is zero => reset cookie to Version0 style - if (Version == 0) - { - variant = CookieVariant.Plain; - } - else if (Version == 1 && variant == CookieVariant.Unknown) - { - // Since we don't expose Variant to an app, set it to Default - variant = CookieVariant.Default; - } - m_cookieVariant = variant; + variant = CookieVariant.Plain; + } + else if (Version == 1 && variant == CookieVariant.Unknown) + { + // Since we don't expose Variant to an app, set it to Default + variant = CookieVariant.Default; } + m_cookieVariant = variant; // Check the name if (string.IsNullOrEmpty(m_name) || @@ -352,149 +385,50 @@ internal bool VerifySetDefaults(CookieVariant variant, Uri uri, bool isLocalDoma m_name.EndsWith(' ') || m_name.AsSpan().ContainsAny(s_reservedToNameChars)) { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, "Name", m_name ?? "")); - } - return false; + throw new CookieException(SR.Format(SR.net_cookie_attribute, "Name", m_name ?? "")); } // Check the value if (m_value == null || (!(m_value.Length > 2 && m_value.StartsWith('\"') && m_value.EndsWith('\"')) && m_value.AsSpan().ContainsAny(';', ','))) { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, "Value", m_value ?? "")); - } - return false; + throw new CookieException(SR.Format(SR.net_cookie_attribute, "Value", m_value ?? "")); } // Check Comment syntax if (Comment != null && !(Comment.Length > 2 && Comment.StartsWith('\"') && Comment.EndsWith('\"')) && (Comment.AsSpan().ContainsAny(';', ','))) { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.CommentAttributeName, Comment)); - } - return false; + throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.CommentAttributeName, Comment)); } // Check Path syntax if (Path != null && !(Path.Length > 2 && Path.StartsWith('\"') && Path.EndsWith('\"')) && (Path.AsSpan().ContainsAny(';', ','))) { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.PathAttributeName, Path)); - } - return false; + throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.PathAttributeName, Path)); } // Check/set domain // // If domain is implicit => assume a) uri is valid, b) just set domain to uri hostname. - if (setDefault && m_domain_implicit) + if (m_domain_implicit) { m_domain = host; + InitDomainKey(); } else { - if (!m_domain_implicit) - { - // Forwarding note: If Uri.Host is of IP address form then the only supported case - // is for IMPLICIT domain property of a cookie. - // The code below (explicit cookie.Domain value) will try to parse Uri.Host IP string - // as a fqdn and reject the cookie. - - // Aliasing since we might need the KeyValue (but not the original one). - string domain = m_domain; - - // Syntax check for Domain charset plus empty string. - if (!DomainCharsTest(domain)) - { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.DomainAttributeName, domain ?? "")); - } - return false; - } + Debug.Assert(m_domainKey is not null); - // Domain must start with '.' if set explicitly. - if (domain[0] != '.') - { - domain = '.' + domain; - } - - int host_dot = host.IndexOf('.'); - - // First quick check is for pushing a cookie into the local domain. - if (isLocalDomain && string.Equals(localDomain, domain, StringComparison.OrdinalIgnoreCase)) - { - valid = true; - } - else if (domain.IndexOf('.', 1, domain.Length - 2) == -1) - { - // A single label domain is valid only if the domain is exactly the same as the host specified in the URI. - if (!IsDomainEqualToHost(domain, host)) - { - valid = false; - } - } - else if (variant == CookieVariant.Plain) - { - // We distinguish between Version0 cookie and other versions on domain issue. - // According to Version0 spec a domain must be just a substring of the hostname. - - if (!IsDomainEqualToHost(domain, host)) - { - if (host.Length <= domain.Length || - (string.Compare(host, host.Length - domain.Length, domain, 0, domain.Length, StringComparison.OrdinalIgnoreCase) != 0)) - { - valid = false; - } - } - } - else if (host_dot == -1 || - domain.Length != host.Length - host_dot || - (string.Compare(host, host_dot, domain, 0, domain.Length, StringComparison.OrdinalIgnoreCase) != 0)) - { - // Starting from the first dot, the host must match the domain. - // - // For null hosts, the host must match the domain exactly. - if (!IsDomainEqualToHost(domain, host)) - { - valid = false; - } - } - - if (valid) - { - m_domainKey = domain.ToLowerInvariant(); - } - } - else + if (!IsValidDomainName(m_domainKey) || !HostMatchesDomain(host, m_domainKey)) { - // For implicitly set domain AND at the set_default == false time - // we simply need to match uri.Host against m_domain. - if (!string.Equals(host, m_domain, StringComparison.OrdinalIgnoreCase)) - { - valid = false; - } - } - if (!valid) - { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.DomainAttributeName, m_domain)); - } - return false; + throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.DomainAttributeName, m_domain)); } } // Check/Set Path - if (setDefault && m_path_implicit) + if (m_path_implicit) { // This code assumes that the URI path is always valid and contains at least one '/'. switch (m_cookieVariant) @@ -532,7 +466,7 @@ internal bool VerifySetDefaults(CookieVariant variant, Uri uri, bool isLocalDoma } // Set the default port if Port attribute was present but had no value. - if (setDefault && (m_port_implicit == false && m_port.Length == 0)) + if (m_port_implicit == false && m_port.Length == 0) { m_port_list = new int[1] { port }; } @@ -540,7 +474,7 @@ internal bool VerifySetDefaults(CookieVariant variant, Uri uri, bool isLocalDoma if (m_port_implicit == false) { // Port must match against the one from the uri. - valid = false; + bool valid = false; foreach (int p in m_port_list!) { if (p == port) @@ -551,21 +485,16 @@ internal bool VerifySetDefaults(CookieVariant variant, Uri uri, bool isLocalDoma } if (!valid) { - if (shouldThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.PortAttributeName, m_port)); - } - return false; + throw new CookieException(SR.Format(SR.net_cookie_attribute, CookieFields.PortAttributeName, m_port)); } } - return true; } // Very primitive test to make sure that the name does not have illegal characters // as per RFC 952 (relaxed on first char could be a digit and string can have '_'). - private static bool DomainCharsTest(string name) => - !string.IsNullOrEmpty(name) && - !name.AsSpan().ContainsAnyExcept(s_domainChars); + private static bool IsValidDomainName(ReadOnlySpan name) => + !name.IsEmpty && + !name.ContainsAnyExcept(s_domainChars); [AllowNull] public string Port @@ -676,7 +605,7 @@ internal string DomainKey { get { - return m_domain_implicit ? Domain : m_domainKey; + return m_domain_implicit ? Domain : m_domainKey!; } } @@ -713,7 +642,7 @@ public override int GetHashCode() StringComparer.OrdinalIgnoreCase.GetHashCode(Name), StringComparer.Ordinal.GetHashCode(Value), StringComparer.Ordinal.GetHashCode(Path), - StringComparer.OrdinalIgnoreCase.GetHashCode(Domain), + StringComparer.OrdinalIgnoreCase.GetHashCode(DomainKey), Version); } diff --git a/src/libraries/System.Net.Primitives/src/System/Net/CookieContainer.cs b/src/libraries/System.Net.Primitives/src/System/Net/CookieContainer.cs index cd1d96b571c7d2..c61076fe894908 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/CookieContainer.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/CookieContainer.cs @@ -4,7 +4,6 @@ using System.Collections; using System.Collections.Generic; using System.Diagnostics; -using System.Net.NetworkInformation; using System.Text; // Relevant cookie specs: @@ -28,30 +27,7 @@ // // Cookies without an explicit Domain attribute will only match a potential uri that matches the original // uri from where the cookie came from. -// -// For explicit Domain attribute in the cookie, the following rules apply: -// -// Version=0 (Netscape, RFC6265) allows the Domain attribute of the cookie to match any tail substring -// of the host uri. -// -// Version=1 related cookie specs only allows the Domain attribute to match the host uri based on a -// more restricted set of rules. -// -// According to RFC2109/RFC2965, the cookie will be rejected for matching if: -// * The value for the Domain attribute contains no embedded dots or does not start with a dot. -// * The value for the request-host does not domain-match the Domain attribute. -// " The request-host is a FQDN (not IP address) and has the form HD, where D is the value of the Domain -// attribute, and H is a string that contains one or more dots. -// -// Examples: -// * A cookie from request-host y.x.foo.com for Domain=.foo.com would be rejected, because H is y.x -// and contains a dot. -// -// * A cookie from request-host x.foo.com for Domain=.foo.com would be accepted. -// -// * A cookie with Domain=.com or Domain=.com., will always be rejected, because there is no embedded dot. -// -// * A cookie with Domain=ajax.com will be rejected because the value for Domain does not begin with a dot. +// For explicit Domain attribute in the cookie, see the rules defined in Cookie.HostMatchesDomain(). namespace System.Net { @@ -94,7 +70,6 @@ public class CookieContainer public const int DefaultPerDomainCookieLimit = 20; public const int DefaultCookieLengthLimit = 4096; - private static readonly string s_fqdnMyDomain = CreateFqdnMyDomain(); private static readonly HeaderVariantInfo[] s_headerInfo = { new HeaderVariantInfo(HttpKnownHeaderNames.SetCookie, CookieVariant.Rfc2109), new HeaderVariantInfo(HttpKnownHeaderNames.SetCookie2, CookieVariant.Rfc2965) @@ -105,7 +80,9 @@ public class CookieContainer private int m_maxCookies = DefaultCookieLimit; // Do not rename (binary serialization) private int m_maxCookiesPerDomain = DefaultPerDomainCookieLimit; // Do not rename (binary serialization) private int m_count; // Do not rename (binary serialization) - private readonly string m_fqdnMyDomain = s_fqdnMyDomain; // Do not rename (binary serialization) +#pragma warning disable CA1823 // Avoid unused private fields + private readonly string m_fqdnMyDomain = string.Empty; +#pragma warning restore CA1823 // Avoid unused private fields public CookieContainer() { @@ -128,14 +105,6 @@ public CookieContainer(int capacity, int perDomainCapacity, int maxCookieSize) : m_maxCookieSize = maxCookieSize; } - private static string CreateFqdnMyDomain() - { - string domain = HostInformation.DomainName; - return domain != null && domain.Length > 1 ? - '.' + domain : - string.Empty; - } - // NOTE: after shrinking the capacity, Count can become greater than Capacity. public int Capacity { @@ -252,24 +221,20 @@ public void Add(Cookie cookie) // We don't know cookie verification status, so re-create the cookie and verify it. Cookie new_cookie = cookie.Clone(); - new_cookie.VerifySetDefaults(new_cookie.Variant, uri, IsLocalDomain(uri.Host), m_fqdnMyDomain, true, true); + new_cookie.VerifyAndSetDefaults(new_cookie.Variant, uri); - Add(new_cookie, true); + InternalAdd(new_cookie); } // This method is called *only* when cookie verification is done, so unlike with public // Add(Cookie cookie) the cookie is in a reasonable condition. - internal void Add(Cookie cookie, bool throwOnError) + internal void InternalAdd(Cookie cookie) { PathList? pathList; if (cookie.Value.Length > m_maxCookieSize) { - if (throwOnError) - { - throw new CookieException(SR.Format(SR.net_cookie_size, cookie, m_maxCookieSize)); - } - return; + throw new CookieException(SR.Format(SR.net_cookie_size, cookie, m_maxCookieSize)); } try @@ -340,10 +305,7 @@ internal void Add(Cookie cookie, bool throwOnError) } catch (Exception e) { - if (throwOnError) - { - throw new CookieException(SR.net_container_add_cookie, e); - } + throw new CookieException(SR.net_container_add_cookie, e); } } @@ -580,85 +542,15 @@ public void Add(CookieCollection cookies) } } - // This will try (if needed) get the full domain name of the host given the Uri. - // NEVER call this function from internal methods with 'fqdnRemote' == null. - // Since this method counts security issue for DNS and hence will slow - // the performance. - internal bool IsLocalDomain(string host) - { - int dot = host.IndexOf('.'); - if (dot == -1) - { - // No choice but to treat it as a host on the local domain. - // This also covers 'localhost' and 'loopback'. - return true; - } - - // Quick test for typical cases: loopback addresses for IPv4 and IPv6. - if ((host == "127.0.0.1") || (host == "::1") || (host == "0:0:0:0:0:0:0:1")) - { - return true; - } - - // Test domain membership. - if (string.Compare(m_fqdnMyDomain, 0, host, dot, m_fqdnMyDomain.Length, StringComparison.OrdinalIgnoreCase) == 0) - { - return true; - } - - // Test for "127.###.###.###" without using regex. - ReadOnlySpan hostSpan = host; - Span ipParts = stackalloc Range[5]; - ipParts = ipParts.Slice(0, hostSpan.Split(ipParts, '.')); - if (ipParts.Length == 4 && hostSpan[ipParts[0]] is "127") - { - int i; - for (i = 1; i < ipParts.Length; i++) - { - ReadOnlySpan part = hostSpan[ipParts[i]]; - switch (part.Length) - { - case 3: - if (!char.IsAsciiDigit(part[2])) - { - break; - } - goto case 2; - - case 2: - if (!char.IsAsciiDigit(part[1])) - { - break; - } - goto case 1; - - case 1: - if (!char.IsAsciiDigit(part[0])) - { - break; - } - continue; - } - break; - } - if (i == 4) - { - return true; - } - } - - return false; - } - public void Add(Uri uri, Cookie cookie) { ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(cookie); Cookie new_cookie = cookie.Clone(); - new_cookie.VerifySetDefaults(new_cookie.Variant, uri, IsLocalDomain(uri.Host), m_fqdnMyDomain, true, true); + new_cookie.VerifyAndSetDefaults(new_cookie.Variant, uri); - Add(new_cookie, true); + InternalAdd(new_cookie); } public void Add(Uri uri, CookieCollection cookies) @@ -666,18 +558,17 @@ public void Add(Uri uri, CookieCollection cookies) ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(cookies); - bool isLocalDomain = IsLocalDomain(uri.Host); foreach (Cookie c in cookies) { Cookie new_cookie = c.Clone(); - new_cookie.VerifySetDefaults(new_cookie.Variant, uri, isLocalDomain, m_fqdnMyDomain, true, true); - Add(new_cookie, true); + new_cookie.VerifyAndSetDefaults(new_cookie.Variant, uri); + InternalAdd(new_cookie); } } - internal CookieCollection CookieCutter(Uri uri, string? headerName, string setCookieHeader, bool isThrow) + internal CookieCollection CookieCutter(Uri uri, string? headerName, string setCookieHeader) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"uri:{uri} headerName:{headerName} setCookieHeader:{setCookieHeader} isThrow:{isThrow}"); + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"uri:{uri} headerName:{headerName} setCookieHeader:{setCookieHeader}"); CookieCollection cookies = new CookieCollection(); CookieVariant variant = CookieVariant.Unknown; @@ -696,7 +587,6 @@ internal CookieCollection CookieCutter(Uri uri, string? headerName, string setCo } } - bool isLocalDomain = IsLocalDomain(uri.Host); try { CookieParser parser = new CookieParser(setCookieHeader); @@ -717,20 +607,12 @@ internal CookieCollection CookieCutter(Uri uri, string? headerName, string setCo // Parser marks invalid cookies this way if (string.IsNullOrEmpty(cookie.Name)) { - if (isThrow) - { - throw new CookieException(SR.net_cookie_format); - } - // Otherwise, ignore (reject) cookie - continue; + throw new CookieException(SR.net_cookie_format); } // This will set the default values from the response URI // AND will check for cookie validity - if (!cookie.VerifySetDefaults(variant, uri, isLocalDomain, m_fqdnMyDomain, true, isThrow)) - { - continue; - } + cookie.VerifyAndSetDefaults(variant, uri); // If many same cookies arrive we collapse them into just one, hence setting // parameter isStrict = true below cookies.InternalAdd(cookie, true); @@ -742,16 +624,13 @@ internal CookieCollection CookieCutter(Uri uri, string? headerName, string setCo } catch (Exception e) { - if (isThrow) - { - throw new CookieException(SR.Format(SR.net_cookie_parse_header, uri.AbsoluteUri), e); - } + throw new CookieException(SR.Format(SR.net_cookie_parse_header, uri.AbsoluteUri), e); } int cookiesCount = cookies.Count; for (int i = 0; i < cookiesCount; i++) { - Add((Cookie)cookies[i], isThrow); + InternalAdd((Cookie)cookies[i]); } return cookies; @@ -801,74 +680,33 @@ public CookieCollection GetAllCookies() int port = uri.Port; CookieCollection? cookies = null; - var domainAttributeMatchAnyCookieVariant = new System.Collections.Generic.List(); - System.Collections.Generic.List? domainAttributeMatchOnlyCookieVariantPlain = null; - - string fqdnRemote = uri.Host; - - // Add initial candidates to match Domain attribute of possible cookies. - // For these Domains, cookie can have any CookieVariant enum value. - domainAttributeMatchAnyCookieVariant.Add(fqdnRemote); - domainAttributeMatchAnyCookieVariant.Add("." + fqdnRemote); - - int dot = fqdnRemote.IndexOf('.'); - if (dot == -1) + List matchingDomainKeys = [uri.Host]; + ReadOnlySpan host = uri.Host; + int lastDot = host.LastIndexOf('.'); + while (lastDot > 0) { - // DNS.resolve may return short names even for other inet domains ;-( - // We _don't_ know what the exact domain is, so try also grab short hostname cookies. - // Grab long name from the local domain - if (!string.IsNullOrEmpty(m_fqdnMyDomain)) + int dot = host[..lastDot].LastIndexOf('.'); + if (dot > 0) { - domainAttributeMatchAnyCookieVariant.Add(fqdnRemote + m_fqdnMyDomain); - // Grab the local domain itself - domainAttributeMatchAnyCookieVariant.Add(m_fqdnMyDomain); + string match = host[(dot + 1)..].ToString(); + matchingDomainKeys.Add(match); } - } - else - { - // Grab the host domain - domainAttributeMatchAnyCookieVariant.Add(fqdnRemote.Substring(dot)); - - // The following block is only for compatibility with Version0 spec. - // Still, we'll add only Plain-Variant cookies if found under below keys - if (fqdnRemote.Length > 2) - { - // We ignore the '.' at the end on the name - int last = fqdnRemote.LastIndexOf('.', fqdnRemote.Length - 2); - // AND keys with <2 dots inside. - if (last > 0) - { - last = fqdnRemote.LastIndexOf('.', last - 1); - } - if (last != -1) - { - while ((dot < last) && (dot = fqdnRemote.IndexOf('.', dot + 1)) != -1) - { - // These candidates can only match CookieVariant.Plain cookies. - domainAttributeMatchOnlyCookieVariantPlain ??= new System.Collections.Generic.List(); - domainAttributeMatchOnlyCookieVariantPlain.Add(fqdnRemote.Substring(dot)); - } - } - } - } - BuildCookieCollectionFromDomainMatches(uri, isSecure, port, ref cookies, domainAttributeMatchAnyCookieVariant, false); - if (domainAttributeMatchOnlyCookieVariantPlain != null) - { - BuildCookieCollectionFromDomainMatches(uri, isSecure, port, ref cookies, domainAttributeMatchOnlyCookieVariantPlain, true); + lastDot = dot; } + BuildCookieCollectionFromDomainMatches(uri, isSecure, port, ref cookies, matchingDomainKeys); return cookies; } - private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int port, ref CookieCollection? cookies, System.Collections.Generic.List domainAttribute, bool matchOnlyPlainCookie) + private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int port, ref CookieCollection? cookies, List matchingDomainKeys) { - for (int i = 0; i < domainAttribute.Count; i++) + for (int i = 0; i < matchingDomainKeys.Count; i++) { PathList pathList; lock (m_domainTable.SyncRoot) { - pathList = (PathList)m_domainTable[domainAttribute[i]]!; + pathList = (PathList)m_domainTable[matchingDomainKeys[i]]!; if (pathList == null) { continue; @@ -886,7 +724,7 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int { CookieCollection cc = (CookieCollection)list.GetByIndex(e)!; cc.TimeStamp(CookieCollection.Stamp.Set); - MergeUpdateCollections(ref cookies, cc, port, isSecure, matchOnlyPlainCookie); + MergeUpdateCollections(ref cookies, uri.Host, cc, port, isSecure); } } } @@ -896,7 +734,7 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int { lock (m_domainTable.SyncRoot) { - m_domainTable.Remove(domainAttribute[i]); + m_domainTable.Remove(matchingDomainKeys[i]); } } } @@ -922,7 +760,7 @@ private static bool PathMatch(string requestPath, string cookiePath) requestPath[cookiePath.Length] == '/'; } - private void MergeUpdateCollections(ref CookieCollection? destination, CookieCollection source, int port, bool isSecure, bool isPlainOnly) + private void MergeUpdateCollections(ref CookieCollection? destination, string host, CookieCollection source, int port, bool isSecure) { lock (source) { @@ -942,13 +780,7 @@ private void MergeUpdateCollections(ref CookieCollection? destination, CookieCol } else { - // Add only if port does match to this request URI - // or was not present in the original response. - if (isPlainOnly && cookie.Variant != CookieVariant.Plain) - { - ; // Don't add - } - else if (cookie.PortList != null) + if (cookie.PortList != null) { foreach (int p in cookie.PortList) { @@ -971,6 +803,12 @@ private void MergeUpdateCollections(ref CookieCollection? destination, CookieCol to_add = false; } + // For implicit domains exact match is needed + if (cookie.DomainImplicit && !string.Equals(host, cookie.Domain, StringComparison.OrdinalIgnoreCase)) + { + to_add = false; + } + if (to_add) { // In 'source' are already ordered. @@ -1025,7 +863,7 @@ public void SetCookies(Uri uri, string cookieHeader) ArgumentNullException.ThrowIfNull(uri); ArgumentNullException.ThrowIfNull(cookieHeader); - CookieCutter(uri, null, cookieHeader, true); // Will throw on error + CookieCutter(uri, null, cookieHeader); // Will throw on error } } diff --git a/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest.cs b/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest.cs index 11fa3ba37026ee..6a31011d3ca77a 100644 --- a/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest.cs +++ b/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest.cs @@ -294,6 +294,8 @@ public static void Equals_Compare_Success() Cookie c14 = new Cookie("name", "value", "path", "domain") { Version = 5 }; Cookie c15 = new Cookie("name", "value", "path", "domain") { Version = 100 }; + Cookie c9dot = new Cookie("name", "value", "path", ".domain"); + Assert.False(c2.Equals(null)); Assert.False(c2.Equals("")); @@ -329,6 +331,9 @@ public static void Equals_Compare_Success() Assert.NotEqual(c13, c15); Assert.Equal(c13.GetHashCode(), c14.GetHashCode()); Assert.NotEqual(c13.GetHashCode(), c15.GetHashCode()); + + Assert.Equal(c9, c9dot); + Assert.Equal(c9.GetHashCode(), c9dot.GetHashCode()); } [Fact] diff --git a/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest/CookieContainerTest.cs b/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest/CookieContainerTest.cs index 9f5333c1c3d73e..bd3bb3fcacf123 100644 --- a/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest/CookieContainerTest.cs +++ b/src/libraries/System.Net.Primitives/tests/FunctionalTests/CookieTest/CookieContainerTest.cs @@ -93,39 +93,6 @@ public void GetCookies_AddCookieVersion0WithExplicitDomain_CookieReturnedForDoma Assert.Equal(1, cookies.Count); } - [Fact] - public void GetCookies_AddCookieVersion1WithExplicitDomain_CookieReturnedForDomainAndOneLevelSubDomain() - { - const string SchemePrefix = "http://"; - const string OriginalDomain = "contoso.com"; - const string OriginalDomainWithLeadingDot = "." + OriginalDomain; - - var container = new CookieContainer(); - var cookie1 = new Cookie(CookieName1, CookieValue1) { Domain = OriginalDomainWithLeadingDot, Version = 1 }; - container.Add(new Uri(SchemePrefix + OriginalDomain), cookie1); - - var uri = new Uri(SchemePrefix + OriginalDomain); - var cookies = container.GetCookies(uri); - Assert.Equal(1, cookies.Count); - Assert.Equal(OriginalDomainWithLeadingDot, cookies[CookieName1].Domain); - - uri = new Uri(SchemePrefix + "www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(1, cookies.Count); - - uri = new Uri(SchemePrefix + "x.www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(0, cookies.Count); - - uri = new Uri(SchemePrefix + "y.x.www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(0, cookies.Count); - - uri = new Uri(SchemePrefix + "z.y.x.www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(0, cookies.Count); - } - [Fact] public void GetAllCookies_Empty_ReturnsEmptyCollection() { diff --git a/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs b/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs index 3e326de9236c9a..d084f44de7ac04 100644 --- a/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs +++ b/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs @@ -643,5 +643,12 @@ private static void TestIsValid(string address, bool expectedValid) Assert.Equal(expectedValid, IPAddress.IsValid(address)); Assert.Equal(expectedValid, IPAddress.IsValidUtf8(Encoding.UTF8.GetBytes(address))); } + + [Fact] + public void _OmgOmg() + { + bool valid = IPAddress.IsValid("0.url1.com"); + Assert.False(valid); + } } } diff --git a/src/libraries/System.Net.Primitives/tests/UnitTests/CookieContainerTest.cs b/src/libraries/System.Net.Primitives/tests/UnitTests/CookieContainerTest.cs index 06ee2c214ee858..050024fd0a0592 100644 --- a/src/libraries/System.Net.Primitives/tests/UnitTests/CookieContainerTest.cs +++ b/src/libraries/System.Net.Primitives/tests/UnitTests/CookieContainerTest.cs @@ -467,39 +467,6 @@ public void GetCookies_AddCookieVersion0WithExplicitDomain_CookieReturnedForDoma Assert.Equal(1, cookies.Count); } - [Fact] - public void GetCookies_AddCookieVersion1WithExplicitDomain_CookieReturnedForDomainAndOneLevelSubDomain() - { - const string SchemePrefix = "http://"; - const string OriginalDomain = "contoso.com"; - const string OriginalDomainWithLeadingDot = "." + OriginalDomain; - - var container = new CookieContainer(); - var cookie1 = new Cookie(CookieName1, CookieValue1) { Domain = OriginalDomainWithLeadingDot, Version = 1 }; - container.Add(new Uri(SchemePrefix + OriginalDomain), cookie1); - - var uri = new Uri(SchemePrefix + OriginalDomain); - CookieCollection cookies = container.GetCookies(uri); - Assert.Equal(1, cookies.Count); - Assert.Equal(OriginalDomainWithLeadingDot, cookies[CookieName1].Domain); - - uri = new Uri(SchemePrefix + "www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(1, cookies.Count); - - uri = new Uri(SchemePrefix + "x.www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(0, cookies.Count); - - uri = new Uri(SchemePrefix + "y.x.www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(0, cookies.Count); - - uri = new Uri(SchemePrefix + "z.y.x.www." + OriginalDomain); - cookies = container.GetCookies(uri); - Assert.Equal(0, cookies.Count); - } - [Fact] public void Ctor_Capacity_Success() { @@ -590,13 +557,20 @@ public void Add_SameCookieDifferentVairants_OverridesOlderVariant() { Uri uri = new Uri("http://domain.com"); + Cookie c0 = new Cookie("name1", "value", "", "domain.com"); Cookie c1 = new Cookie("name1", "value", "", ".domain.com"); // Variant = Plain Cookie c2 = new Cookie("name1", "value", "", ".domain.com") { Port = "\"80\"" }; // Variant = RFC2965 (should override) Cookie c3 = new Cookie("name1", "value", "", ".domain.com") { Port = "\"80, 90\"" }; // Variant = RFC2965 (should override) Cookie c4 = new Cookie("name1", "value", "", ".domain.com") { Version = 1 }; // Variant = RFC2109 (should be rejected) CookieContainer cc = new CookieContainer(); + + cc.Add(c0); + Assert.Equal("domain.com", cc.GetCookies(uri)[0].Domain); + cc.Add(c1); + Assert.Equal(1, cc.Count); + Assert.Equal(".domain.com", cc.GetCookies(uri)[0].Domain); // Adding a newer variant should override an older one cc.Add(c2); @@ -614,6 +588,24 @@ public void Add_SameCookieDifferentVairants_OverridesOlderVariant() Assert.Equal(1, cc.Count); } + [Fact] + public static void Add_SetCookies_SameCookieDifferentVairants_OverridesOlderVariant() + { + Uri uri = new Uri("http://domain.com"); + CookieContainer cc = new(); + Cookie a = new() + { + Domain = "domain.com", + Name = "lol", + Value = "0" + }; + cc.Add(uri, a); + cc.SetCookies(uri, "lol=42"); + + Assert.Equal(1, cc.Count); + Assert.Equal("42", cc.GetCookies(uri).Single().Value); + } + [Fact] public void Add_Cookie_Invalid() { @@ -781,10 +773,10 @@ private void VerifyGetCookies(CookieContainer cc1, Uri uri, Cookie[] expected) for (int i = 0; i < expected.Length; i++) { - Cookie c1 = expected[i]; Cookie c2 = cc2[i]; - Assert.Equal(c1.Name, c2.Name); // Primitive check for equality - Assert.Equal(c1.Value, c2.Value); + Cookie c1 = expected.Single(c => c.Name == c2.Name); + + Assert.Equal(c1.Value, c2.Value); // Primitive check for equality } } @@ -983,5 +975,100 @@ public void GetCookies_PathMatchingFollowsRfc6265(bool useDefaultPath, string[] CookieCollection collection = container.GetCookies(requestUri); Assert.Equal(expectedMatches, collection.Count); } + + [Fact] + public void Add_ImplicitDomainOfIPv6Hostname_Success() + { + CookieContainer container = new CookieContainer(); + Cookie cookie = new Cookie("lol", "haha"); + Uri uri = new Uri("https://[::FFFF:192.168.0.1]/test"); + container.Add(uri, cookie); + Assert.Equal(uri.Host, container.GetCookies(uri).Single().Domain); + } + + public static TheoryData DomainMatching_WhenMatches_Success_Data = new TheoryData() + { + { "https://q", "q" }, + { "https://localhost/", "localhost" }, + { "https://test.com", "test.com" }, + { "https://test.COM", "tEsT.com" }, + { "https://test.com", ".test.com" }, + { "https://yay.test.com", "yay.test.com" }, + { "https://yay.test.com", ".yay.test.com" }, + { "https://yay.test.com", ".test.com" }, + { "https://42.aaaa.bbb.cc.d.test.com", "d.test.com" }, + { "https://yay.test.com/foo/bar", "test.com" }, + { "https://127.0.1.1", "127.0.1.1" }, + { "https://42.42.100.100", "42.42.100.100" }, + }; + + [Theory] + [MemberData(nameof(DomainMatching_WhenMatches_Success_Data))] + public void DomainMatching_WhenMatches_Success(string uriString, string domain) + { + CookieContainer container = new CookieContainer(); + Cookie cookie = new Cookie("lol", "haha") + { + Domain = domain + }; + + Uri uri = new Uri(uriString); + container.Add(uri, cookie); + Assert.Equal(1, container.Count); + + CookieCollection collection = container.GetCookies(uri); + Assert.Equal(1, collection.Count); + } + + [Fact] + public void DomainMatching_ExplicitDomain_MatchesMultiple() + { + CookieContainer container = new CookieContainer(); + Cookie a = new Cookie("a", "aa", null, "a.com"); + Cookie b = new Cookie("b", "bb", null, "b.a.com"); + Cookie c = new Cookie("c", "cc", null, "c.b.a.com"); + + container.Add(new Uri("http://a.com"), a); + container.Add(new Uri("http://b.a.com"), b); + container.Add(new Uri("http://c.b.a.com"), c); + + CookieCollection matches = container.GetCookies(new Uri("http://c.b.a.com")); + Assert.Equal(3, matches.Count); + } + + public static TheoryData DomainMatching_WhenDoesNotMatch_ThrowsCookieException_Data = new TheoryData() + { + { "https://test.com", "test.co" }, // Domain is not a suffix + { "https://test.com", "x.test.com" }, // Domain is not a suffix (extra chars at start) + { "https://test.com", "ttest.com" }, // Suffix but not separated by dot + { "https://test.com", "test.com." }, // Trailing dot + { "https://test.com", "..test.com" }, // 2 leading dots + { "https://foo.test.com", "yay.test.com" }, // subdomain mismatch + { "https://42.42.100.100", "41.42.100.100" }, // different IP + + // Single label domain without a full match. + { "https://test.com", ".com" }, + { "https://test.com", "com" }, + + // If Host is an IP address, it should be equal to the domain. + // See https://issues.chromium.org/issues/40126142 and the last condition in + // https://datatracker.ietf.org/doc/html/rfc6265#section-5.1.3 + { "https://1.2.3.4", ".2.3.4" } + }; + + + [Theory] + [MemberData(nameof(DomainMatching_WhenDoesNotMatch_ThrowsCookieException_Data))] + public void DomainMatching_WhenDoesNotMatch_ThrowsCookieException(string uriString, string domain) + { + CookieContainer container = new CookieContainer(); + Cookie cookie = new Cookie("lol", "haha") + { + Domain = domain + }; + + Uri uri = new Uri(uriString); + Assert.Throws(() => container.Add(uri, cookie)); + } } } diff --git a/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/HostInformation.cs b/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/HostInformation.cs deleted file mode 100644 index 277b450b24158d..00000000000000 --- a/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/HostInformation.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -namespace System.Net.NetworkInformation -{ - internal static class HostInformation - { - public static string DomainName - { - get - { - return "contoso.com"; - } - } - } -} diff --git a/src/libraries/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj b/src/libraries/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj index 1d9db5b2762863..133bf304cde91a 100644 --- a/src/libraries/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj +++ b/src/libraries/System.Net.Primitives/tests/UnitTests/System.Net.Primitives.UnitTests.Tests.csproj @@ -1,4 +1,4 @@ - + true $(NoWarn);169;649 @@ -28,6 +28,10 @@ Link="ProductionCode\System\Net\Sockets\SocketError.cs" /> + + - - - Date: Sat, 15 Feb 2025 21:41:25 +0100 Subject: [PATCH 2/3] delete dummy test --- .../tests/FunctionalTests/IPAddressParsing.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs b/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs index d084f44de7ac04..3e326de9236c9a 100644 --- a/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs +++ b/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs @@ -643,12 +643,5 @@ private static void TestIsValid(string address, bool expectedValid) Assert.Equal(expectedValid, IPAddress.IsValid(address)); Assert.Equal(expectedValid, IPAddress.IsValidUtf8(Encoding.UTF8.GetBytes(address))); } - - [Fact] - public void _OmgOmg() - { - bool valid = IPAddress.IsValid("0.url1.com"); - Assert.False(valid); - } } } From b4d33d30cc237437dda267e26d365007adb032c2 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Wed, 12 Mar 2025 19:47:24 +0100 Subject: [PATCH 3/3] review feedback --- .../src/System/Net/Cookie.cs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs b/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs index b14a4af8f4eb40..d4341314bb1163 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs @@ -177,11 +177,8 @@ public string Domain } set { - m_domain = value ?? string.Empty; + SetDomainAndKey(value ?? string.Empty); m_domain_implicit = false; - - // Given Domain is explicit now, it is necessary to initialize DomainKey for correct GetHashCode() behavior. - InitDomainKey(); } } @@ -310,8 +307,9 @@ internal Cookie Clone() return clonedCookie; } - private void InitDomainKey() + private void SetDomainAndKey(string domain) { + m_domain = domain; m_domainKey = CookieComparer.StripLeadingDot(m_domain).ToString().ToLowerInvariant(); } @@ -356,10 +354,7 @@ private static bool HostMatchesDomain(ReadOnlySpan host, ReadOnlySpan assume a) uri is valid, b) just set domain to uri hostname. if (m_domain_implicit) { - m_domain = host; - InitDomainKey(); + SetDomainAndKey(host); } else {