Skip to content

Commit b668e44

Browse files
BrennanConroywtgodbe
BrennanConroy
authored and
wtgodbe
committed
Merged PR 50638: Harden Cookie parsing
Harden Cookie parsing ---- #### AI description (iteration 1) #### PR Classification Bug fix focused on enforcing stricter cookie parsing rules in accordance with RFC 6265. #### PR Summary This pull request refines cookie parsing by updating test datasets and parser logic to reject improperly formatted cookie headers. The changes include: - **`src/Http/Headers/test/CookieHeaderValueTest.cs`**: Renamed and modified test datasets to use strict parsing (e.g., renaming to ListOfStrictCookieHeaderDataSet) and update expected results for invalid cookie combinations. - **`src/Http/Http/test/RequestCookiesCollectionTests.cs`**: Revised inline test cases to expect null outcomes for malformed cookies and added new cases for additional invalid formats. - **`src/Http/Shared/CookieHeaderParserShared.cs`**: Enhanced parsing logic to skip invalid cookie segments by enforcing semicolon delimiters and incorporating detailed RFC comment documentation. <!-- GitOpsUserAgent=GitOps.Apps.Server.pullrequestcopilot -->
1 parent 5beb109 commit b668e44

File tree

3 files changed

+72
-19
lines changed

3 files changed

+72
-19
lines changed

‎src/Http/Headers/test/CookieHeaderValueTest.cs

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public static TheoryData<string> InvalidCookieValues
7575
}
7676
}
7777

78-
public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfCookieHeaderDataSet
78+
public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfStrictCookieHeaderDataSet
7979
{
8080
get
8181
{
@@ -94,19 +94,30 @@ public static TheoryData<string> InvalidCookieValues
9494

9595
dataset.Add(new[] { header1 }.ToList(), new[] { string1 });
9696
dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, string1 });
97-
dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, null, "", " ", ";", " , ", string1 });
9897
dataset.Add(new[] { header2 }.ToList(), new[] { string2 });
9998
dataset.Add(new[] { header1, header2 }.ToList(), new[] { string1, string2 });
100-
dataset.Add(new[] { header1, header2 }.ToList(), new[] { string1 + ", " + string2 });
10199
dataset.Add(new[] { header2, header1 }.ToList(), new[] { string2 + "; " + string1 });
102100
dataset.Add(new[] { header1, header2, header3, header4 }.ToList(), new[] { string1, string2, string3, string4 });
103-
dataset.Add(new[] { header1, header2, header3, header4 }.ToList(), new[] { string.Join(",", string1, string2, string3, string4) });
104101
dataset.Add(new[] { header1, header2, header3, header4 }.ToList(), new[] { string.Join(";", string1, string2, string3, string4) });
105102

106103
return dataset;
107104
}
108105
}
109106

107+
public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfCookieHeaderDataSet
108+
{
109+
get
110+
{
111+
var header1 = new CookieHeaderValue("name1", "n1=v1&n2=v2&n3=v3");
112+
var string1 = "name1=n1=v1&n2=v2&n3=v3";
113+
114+
var dataset = new TheoryData<IList<CookieHeaderValue>, string?[]>();
115+
dataset.Concat(ListOfStrictCookieHeaderDataSet);
116+
dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, null, "", " ", ";", " , ", string1 });
117+
return dataset;
118+
}
119+
}
120+
110121
public static TheoryData<IList<CookieHeaderValue>?, string?[]> ListWithInvalidCookieHeaderDataSet
111122
{
112123
get
@@ -127,18 +138,19 @@ public static TheoryData<string> InvalidCookieValues
127138
dataset.Add(new[] { header1 }.ToList(), new[] { validString1, invalidString1 });
128139
dataset.Add(new[] { header1 }.ToList(), new[] { validString1, null, "", " ", ";", " , ", invalidString1 });
129140
dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1, null, "", " ", ";", " , ", validString1 });
130-
dataset.Add(new[] { header1 }.ToList(), new[] { validString1 + ", " + invalidString1 });
131-
dataset.Add(new[] { header2 }.ToList(), new[] { invalidString1 + ", " + validString2 });
141+
dataset.Add(null, new[] { validString1 + ", " });
142+
dataset.Add(null, new[] { invalidString1 + ", " + validString2 });
132143
dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1 + "; " + validString1 });
133144
dataset.Add(new[] { header2 }.ToList(), new[] { validString2 + "; " + invalidString1 });
134145
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { invalidString1, validString1, validString2, validString3 });
135146
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { validString1, invalidString1, validString2, validString3 });
136147
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { validString1, validString2, invalidString1, validString3 });
137148
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { validString1, validString2, validString3, invalidString1 });
138-
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", invalidString1, validString1, validString2, validString3) });
139-
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", validString1, invalidString1, validString2, validString3) });
140-
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", validString1, validString2, invalidString1, validString3) });
141-
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(",", validString1, validString2, validString3, invalidString1) });
149+
dataset.Add(null, new[] { string.Join(",", invalidString1, validString1, validString2, validString3) });
150+
dataset.Add(null, new[] { string.Join(",", validString1, invalidString1, validString2, validString3) });
151+
dataset.Add(null, new[] { string.Join(",", validString1, validString2, invalidString1, validString3) });
152+
dataset.Add(null, new[] { string.Join(",", validString1, validString2, validString3, invalidString1) });
153+
dataset.Add(null, new[] { string.Join(",", validString1, validString2, validString3) });
142154
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(";", invalidString1, validString1, validString2, validString3) });
143155
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(";", validString1, invalidString1, validString2, validString3) });
144156
dataset.Add(new[] { header1, header2, header3 }.ToList(), new[] { string.Join(";", validString1, validString2, invalidString1, validString3) });
@@ -248,7 +260,7 @@ public void CookieHeaderValue_ParseList_AcceptsValidValues(IList<CookieHeaderVal
248260
}
249261

250262
[Theory]
251-
[MemberData(nameof(ListOfCookieHeaderDataSet))]
263+
[MemberData(nameof(ListOfStrictCookieHeaderDataSet))]
252264
public void CookieHeaderValue_ParseStrictList_AcceptsValidValues(IList<CookieHeaderValue> cookies, string[] input)
253265
{
254266
var results = CookieHeaderValue.ParseStrictList(input);
@@ -267,7 +279,7 @@ public void CookieHeaderValue_TryParseList_AcceptsValidValues(IList<CookieHeader
267279
}
268280

269281
[Theory]
270-
[MemberData(nameof(ListOfCookieHeaderDataSet))]
282+
[MemberData(nameof(ListOfStrictCookieHeaderDataSet))]
271283
public void CookieHeaderValue_TryParseStrictList_AcceptsValidValues(IList<CookieHeaderValue> cookies, string[] input)
272284
{
273285
var result = CookieHeaderValue.TryParseStrictList(input, out var results);

‎src/Http/Http/test/RequestCookiesCollectionTests.cs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,22 @@ public void ParseManyCookies()
3333
[Theory]
3434
[InlineData(",", null)]
3535
[InlineData(";", null)]
36-
[InlineData("er=dd,cc,bb", new[] { "dd" })]
37-
[InlineData("er=dd,err=cc,errr=bb", new[] { "dd", "cc", "bb" })]
38-
[InlineData("errorcookie=dd,:(\"sa;", new[] { "dd" })]
36+
[InlineData("er=dd,cc,bb", null)]
37+
[InlineData("er=dd,err=cc,errr=bb", null)]
38+
[InlineData("errorcookie=dd,:(\"sa;", null)]
3939
[InlineData("s;", null)]
40+
[InlineData("a@a=a;", null)]
41+
[InlineData("a@ a=a;", null)]
42+
[InlineData("a a=a;", null)]
43+
[InlineData(",a=a;", null)]
44+
[InlineData(",a=a", null)]
45+
[InlineData("a=a;,b=b", new []{ "a" })] // valid cookie followed by invalid cookie
46+
[InlineData(",a=a;b=b", new[] { "b" })] // invalid cookie followed by valid cookie
4047
public void ParseInvalidCookies(string cookieToParse, string[] expectedCookieValues)
4148
{
4249
var cookies = RequestCookieCollection.Parse(new StringValues(new[] { cookieToParse }));
4350

44-
if(expectedCookieValues == null)
51+
if (expectedCookieValues == null)
4552
{
4653
Assert.Equal(0, cookies.Count);
4754
return;

‎src/Http/Shared/CookieHeaderParserShared.cs

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,17 @@ public static bool TryParseValue(StringSegment value, ref int index, bool suppor
8383

8484
if (!TryGetCookieLength(value, ref current, out parsedName, out parsedValue))
8585
{
86+
var separatorIndex = value.IndexOf(';', current);
87+
if (separatorIndex > 0)
88+
{
89+
// Skip the invalid values and keep trying.
90+
index = separatorIndex;
91+
}
92+
else
93+
{
94+
// No more separators, so we're done.
95+
index = value.Length;
96+
}
8697
return false;
8798
}
8899

@@ -91,6 +102,17 @@ public static bool TryParseValue(StringSegment value, ref int index, bool suppor
91102
// If we support multiple values and we've not reached the end of the string, then we must have a separator.
92103
if ((separatorFound && !supportsMultipleValues) || (!separatorFound && (current < value.Length)))
93104
{
105+
var separatorIndex = value.IndexOf(';', current);
106+
if (separatorIndex > 0)
107+
{
108+
// Skip the invalid values and keep trying.
109+
index = separatorIndex;
110+
}
111+
else
112+
{
113+
// No more separators, so we're done.
114+
index = value.Length;
115+
}
94116
return false;
95117
}
96118

@@ -106,7 +128,7 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta
106128
separatorFound = false;
107129
var current = startIndex + HttpRuleParser.GetWhitespaceLength(input, startIndex);
108130

109-
if ((current == input.Length) || (input[current] != ',' && input[current] != ';'))
131+
if (current == input.Length || input[current] != ';')
110132
{
111133
return current;
112134
}
@@ -119,8 +141,8 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta
119141

120142
if (skipEmptyValues)
121143
{
122-
// Most headers only split on ',', but cookies primarily split on ';'
123-
while ((current < input.Length) && ((input[current] == ',') || (input[current] == ';')))
144+
// Cookies are split on ';'
145+
while (current < input.Length && input[current] == ';')
124146
{
125147
current++; // skip delimiter.
126148
current = current + HttpRuleParser.GetWhitespaceLength(input, current);
@@ -130,6 +152,18 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta
130152
return current;
131153
}
132154

155+
/*
156+
* https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1
157+
* cookie-pair = cookie-name "=" cookie-value
158+
* cookie-name = token
159+
* token = 1*<any CHAR except CTLs or separators>
160+
separators = "(" | ")" | "<" | ">" | "@"
161+
| "," | ";" | ":" | "\" | <">
162+
| "/" | "[" | "]" | "?" | "="
163+
| "{" | "}" | SP | HT
164+
CTL = <any US-ASCII control character
165+
(octets 0 - 31) and DEL (127)>
166+
*/
133167
// name=value; name="value"
134168
internal static bool TryGetCookieLength(StringSegment input, ref int offset, [NotNullWhen(true)] out StringSegment? parsedName, [NotNullWhen(true)] out StringSegment? parsedValue)
135169
{

0 commit comments

Comments
 (0)