Harden Cookie Parsing (#62681)

* Resolve conflict

* Update HeaderDictionaryTypeExtensionsTest.cs

---------

Co-authored-by: Brennan Conroy <[email protected]>

src/Http/Headers/test/CookieHeaderValueTest.cs

@@ -75,7 +75,7 @@ public class CookieHeaderValueTest
         }
     }
 
-    public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfCookieHeaderDataSet
+    public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfStrictCookieHeaderDataSet
     {
         get
         {
@@ -94,19 +94,30 @@ public class CookieHeaderValueTest
 
             dataset.Add(new[] { header1 }.ToList(), new[] { string1 });
             dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, string1 });
-            dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, null, "", " ", ";", " , ", string1 });
             dataset.Add(new[] { header2 }.ToList(), new[] { string2 });
             dataset.Add(new[] { header1, header2 }.ToList(), new[] { string1, string2 });
-            dataset.Add(new[] { header1, header2 }.ToList(), new[] { string1 + ", " + string2 });
             dataset.Add(new[] { header2, header1 }.ToList(), new[] { string2 + "; " + string1 });
             dataset.Add(new[] { header1, header2, header3, header4 }.ToList(), new[] { string1, string2, string3, string4 });
-            dataset.Add(new[] { header1, header2, header3, header4 }.ToList(), new[] { string.Join(",", string1, string2, string3, string4) });
             dataset.Add(new[] { header1, header2, header3, header4 }.ToList(), new[] { string.Join(";", string1, string2, string3, string4) });
 
             return dataset;
         }
     }
 
+    public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfCookieHeaderDataSet
+    {
+        get
+        {
+            var header1 = new CookieHeaderValue("name1", "n1=v1&n2=v2&n3=v3");
+            var string1 = "name1=n1=v1&n2=v2&n3=v3";
+
+            var dataset = new TheoryData<IList<CookieHeaderValue>, string?[]>();
+            dataset.Concat(ListOfStrictCookieHeaderDataSet);
+            dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, null, "", " ", ";", " , ", string1 });
+            return dataset;
+        }
+    }
+
     public static TheoryData<IList<CookieHeaderValue>?, string?[]> ListWithInvalidCookieHeaderDataSet
     {
         get
@@ -127,18 +138,19 @@ public class CookieHeaderValueTest
             dataset.Add(new[] { header1 }.ToList(), new[] { validString1, invalidString1 });
             dataset.Add(new[] { header1 }.ToList(), new[] { validString1, null, "", " ", ";", " , ", invalidString1 });
             dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1, null, "", " ", ";", " , ", validString1 });
-            dataset.Add(new[] { header1 }.ToList(), new[] { validString1 + ", " + invalidString1 });
-            dataset.Add(new[] { header2 }.ToList(), new[] { invalidString1 + ", " + validString2 });
+            dataset.Add(null, new[] { validString1 + ", " });
+            dataset.Add(null, new[] { invalidString1 + ", " + validString2 });
             dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1 + "; " + validString1 });
             dataset.Add(new[] { header2 }.ToList(), new[] { validString2 + "; " + invalidString1 });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { invalidString1, validString1, validString2, validString3 });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { validString1, invalidString1, validString2, validString3 });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { validString1, validString2, invalidString1, validString3 });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { validString1, validString2, validString3, invalidString1 });
-            dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", invalidString1, validString1, validString2, validString3) });
-            dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", validString1, invalidString1, validString2, validString3) });
-            dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", validString1, validString2, invalidString1, validString3) });
-            dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", validString1, validString2, validString3, invalidString1) });
+            dataset.Add(null, new[] { string.Join(",", invalidString1, validString1, validString2, validString3) });
+            dataset.Add(null, new[] { string.Join(",", validString1, invalidString1, validString2, validString3) });
+            dataset.Add(null, new[] { string.Join(",", validString1, validString2, invalidString1, validString3) });
+            dataset.Add(null, new[] { string.Join(",", validString1, validString2, validString3, invalidString1) });
+            dataset.Add(null, new[] { string.Join(",", validString1, validString2, validString3) });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(";", invalidString1, validString1, validString2, validString3) });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(";", validString1, invalidString1, validString2, validString3) });
             dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(";", validString1, validString2, invalidString1, validString3) });
@@ -248,7 +260,7 @@ public class CookieHeaderValueTest
     }
 
     [Theory]
-    [MemberData(nameof(ListOfCookieHeaderDataSet))]
+    [MemberData(nameof(ListOfStrictCookieHeaderDataSet))]
     public void CookieHeaderValue_ParseStrictList_AcceptsValidValues(IList<CookieHeaderValue> cookies, string[] input)
     {
         var results = CookieHeaderValue.ParseStrictList(input);
@@ -267,7 +279,7 @@ public class CookieHeaderValueTest
     }
 
     [Theory]
-    [MemberData(nameof(ListOfCookieHeaderDataSet))]
+    [MemberData(nameof(ListOfStrictCookieHeaderDataSet))]
     public void CookieHeaderValue_TryParseStrictList_AcceptsValidValues(IList<CookieHeaderValue> cookies, string[] input)
     {
         var result = CookieHeaderValue.TryParseStrictList(input, out var results);

src/Http/Http.Extensions/test/HeaderDictionaryTypeExtensionsTest.cs

@@ -214,7 +214,7 @@ public class HeaderDictionaryTypeExtensionsTest
     public void GetListT_CookieHeaderValue_Success()
     {
         var context = new DefaultHttpContext();
-        context.Request.Headers.Cookie = "cookie1=a,cookie2=b";
+        context.Request.Headers.Cookie = "cookie1=a;cookie2=b";
 
         var result = context.Request.GetTypedHeaders().GetList<CookieHeaderValue>(HeaderNames.Cookie);
 

src/Http/Http/test/RequestCookiesCollectionTests.cs

@@ -33,11 +33,18 @@ public class RequestCookiesCollectionTests
     [Theory]
     [InlineData(",", null)]
     [InlineData(";", null)]
-    [InlineData("er=dd,cc,bb", new[] { "dd" })]
-    [InlineData("er=dd,err=cc,errr=bb", new[] { "dd", "cc", "bb" })]
-    [InlineData("errorcookie=dd,:(\"sa;", new[] { "dd" })]
+    [InlineData("er=dd,cc,bb", null)]
+    [InlineData("er=dd,err=cc,errr=bb", null)]
+    [InlineData("errorcookie=dd,:(\"sa;", null)]
     [InlineData("s;", null)]
     [InlineData("er=;,err=,errr=\\,errrr=\"", null)]
+    [InlineData("a@a=a;", null)]
+    [InlineData("a@ a=a;", null)]
+    [InlineData("a a=a;", null)]
+    [InlineData(",a=a;", null)]
+    [InlineData(",a=a", null)]
+    [InlineData("a=a;,b=b", new []{ "a" })] // valid cookie followed by invalid cookie
+    [InlineData(",a=a;b=b", new[] { "b" })] // invalid cookie followed by valid cookie
     public void ParseInvalidCookies(string cookieToParse, string[] expectedCookieValues)
     {
         var cookies = RequestCookieCollection.Parse(new StringValues(new[] { cookieToParse }));

src/Http/Shared/CookieHeaderParserShared.cs

@@ -89,6 +89,17 @@ internal static class CookieHeaderParserShared
 
         if (!TryGetCookieLength(value, ref current, out parsedName, out parsedValue))
         {
+            var separatorIndex = value.IndexOf(';', current);
+            if (separatorIndex > 0)
+            {
+                // Skip the invalid values and keep trying.
+                index = separatorIndex;
+            }
+            else
+            {
+                // No more separators, so we're done.
+                index = value.Length;
+            }
             return false;
         }
 
@@ -97,6 +108,17 @@ internal static class CookieHeaderParserShared
         // If we support multiple values and we've not reached the end of the string, then we must have a separator.
         if ((separatorFound && !supportsMultipleValues) || (!separatorFound && (current < value.Length)))
         {
+            var separatorIndex = value.IndexOf(';', current);
+            if (separatorIndex > 0)
+            {
+                // Skip the invalid values and keep trying.
+                index = separatorIndex;
+            }
+            else
+            {
+                // No more separators, so we're done.
+                index = value.Length;
+            }
             return false;
         }
 
@@ -112,7 +134,7 @@ internal static class CookieHeaderParserShared
         separatorFound = false;
         var current = startIndex + HttpRuleParser.GetWhitespaceLength(input, startIndex);
 
-        if ((current == input.Length) || (input[current] != ',' && input[current] != ';'))
+        if (current == input.Length || input[current] != ';')
         {
             return current;
         }
@@ -125,8 +147,8 @@ internal static class CookieHeaderParserShared
 
         if (skipEmptyValues)
         {
-            // Most headers only split on ',', but cookies primarily split on ';'
-            while ((current < input.Length) && ((input[current] == ',') || (input[current] == ';')))
+            // Cookies are split on ';'
+            while (current < input.Length && input[current] == ';')
             {
                 current++; // skip delimiter.
                 current = current + HttpRuleParser.GetWhitespaceLength(input, current);
@@ -136,6 +158,18 @@ internal static class CookieHeaderParserShared
         return current;
     }
 
+    /*
+     * https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1
+     * cookie-pair       = cookie-name "=" cookie-value
+     * cookie-name       = token
+     * token          = 1*<any CHAR except CTLs or separators>
+       separators     = "(" | ")" | "<" | ">" | "@"
+                      | "," | ";" | ":" | "\" | <">
+                      | "/" | "[" | "]" | "?" | "="
+                      | "{" | "}" | SP | HT
+       CTL            = <any US-ASCII control character
+                        (octets 0 - 31) and DEL (127)>
+     */
     // name=value; name="value"
     internal static bool TryGetCookieLength(StringSegment input, ref int offset, [NotNullWhen(true)] out StringSegment? parsedName, [NotNullWhen(true)] out StringSegment? parsedValue)
     {