[Mono-dev] Patch for CookieContainer.SetCookies

[Sebastien Pouliot]

Hello again,

Sorry I hit send adding my comments on the patch itself.

> Index: mcs/class/System/System.Net/CookieContainer.cs
> ===================================================================
> --- mcs/class/System/System.Net/CookieContainer.cs      (revision 149764)
> +++ mcs/class/System/System.Net/CookieContainer.cs      (working copy)
> @@ -36,6 +36,7 @@
>  using System.Globalization;
>  using System.Runtime.Serialization;
>  using System.Text;
> +using System.Text.RegularExpressions;
>  
>  namespace System.Net 
>  {
> @@ -53,7 +54,7 @@
>                 int perDomainCapacity = DefaultPerDomainCookieLimit;
>                 int maxCookieSize = DefaultCookieLengthLimit;
>                 CookieCollection cookies;
> -                               
> +
>                 // ctors
>                 public CookieContainer ()
>                 { 
> @@ -146,7 +147,16 @@
>                         if (cookie.Value.Length > maxCookieSize)
>                                 throw new CookieException ("value is larger than MaxCookieSize.");
>  
> -                       AddCookie (cookie);
> +                       // .NET's Add (Cookie) is fundamentally broken and does not copy properties
> +                       // like Secure, HttpOnly and Expires so we clone the parts that .NET
> +                       // does keep before calling AddCookie
> +                       Cookie c = new Cookie (cookie.Name, cookie.Value);
> +                       c.Path = (cookie.Path.Length == 0) ? "/" : cookie.Path;
> +                       c.Domain = cookie.Domain;
> +                       c.ExactDomain = cookie.ExactDomain;
> +                       c.Version = cookie.Version;
> +                       
> +                       AddCookie (c);
>                 }
>  
>                 void AddCookie (Cookie cookie)
> @@ -169,6 +179,16 @@
>                         c.Domain = cookie.Domain;
>                         c.ExactDomain = cookie.ExactDomain;
>                         c.Version = cookie.Version;
> +                       c.Expires = cookie.Expires;
> +                       c.CommentUri = cookie.CommentUri;
> +                       c.Comment = cookie.Comment;
> +                       c.Discard = c.Discard;

				^ that's unlikely to be correct ;-)

> +                       if (cookie.HttpOnly) {
> +                               c.HttpOnly = true;
> +                       }
> +                       if (cookie.Secure) {
> +                               c.Secure = true;
> +                       }

why not simply 
			c.HttpOnly = cookie.HttpOnly;
			c.Secure = cookie.Secure;
?

if there's a problem with this simpler code then it does not get caught
by your new unit tests.

>  
>                         cookies.Add (c);
>                         CheckExpiration ();
> @@ -373,10 +393,21 @@
>                                 return;
>                         
>                         // Cookies must be separated by ',' (like documented on MSDN)
> -                       string [] jar = cookieHeader.Split (',');
> -                       foreach (string cookie in jar) {
> +                       // but expires uses DAY, DD-MMM-YYYY HH:MM:SS GMT, so simple ',' search is wrong.
> +                       // See http://msdn.microsoft.com/en-us/library/aa384321%28VS.85%29.aspx
> +                       string[] jar = cookieHeader.Split(',');
> +                       string tmpCookie;
> +                       for (int i = 0; i < jar.Length; i++) {
> +                               tmpCookie = jar[i];
> +
> +                               if (jar.Length > i + 1
> +                                       && Regex.IsMatch (jar[i], @".*expires\s*=\s*(Mon|Tue|Wed|Thu|Fri|Sat|Sun)", RegexOptions.IgnoreCase) 
> +                                       && Regex.IsMatch (jar[i+1], @"\s\d{2}-(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)-\d{4} \d{2}:\d{2}:\d{2} GMT", RegexOptions.IgnoreCase)) {
> +                                       tmpCookie = new StringBuilder(tmpCookie).Append(",").Append(jar[++i]).ToString();
> +                               }
> +
>                                 try {
> -                                       Cookie c = Parse (cookie);
> +                                       Cookie c = Parse (tmpCookie);
>  
>                                         // add default values from URI if missing from the string
>                                         if (c.Path.Length == 0) {
> @@ -392,7 +423,7 @@
>                                                 c.ExactDomain = true;
>                                         }
>  
> -                                       Add (c);
> +                                       AddCookie (c);
>                                 }
>                                 catch (Exception e) {
>                                         string msg = String.Format ("Could not parse cookies for '{0}'.", uri);
> @@ -430,6 +461,17 @@
>                                                 c.ExactDomain = false;
>                                         }
>                                         break;
> +                               case "expires":
> +                               case "$expires":
> +                                       if (c.Expires == DateTime.MinValue)
> +                                               c.Expires = DateTime.SpecifyKind(DateTime.ParseExact(value, @"ddd, dd-MMM-yyyy HH:mm:ss G\MT", CultureInfo.InvariantCulture), DateTimeKind.Utc);
> +                                               break;
> +                               case "httponly":
> +                                       c.HttpOnly = true;
> +                                       break;
> +                               case "secure":
> +                                       c.Secure = true;
> +                                       break;

Your additional unit tests do not cover the "secure" case above (i.e.
please add test for all new code). 

I suspect the "expires" case is the buggy one wrt your unit test
(previous email).

Sebastien