Skip to content

Harden Cookie Parsing #62681

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 11, 2025
Merged

Harden Cookie Parsing #62681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
924518d
Resolve conflict
BrennanConroy Jun 6, 2025
5d1725f
Update HeaderDictionaryTypeExtensionsTest.cs
wtgodbe Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions src/Http/Headers/test/CookieHeaderValueTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public static TheoryData<string> InvalidCookieValues
}
}

public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfCookieHeaderDataSet
public static TheoryData<IList<CookieHeaderValue>, string?[]> ListOfStrictCookieHeaderDataSet
{
get
{
Expand All @@ -94,19 +94,30 @@ public static TheoryData<string> InvalidCookieValues

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);
Copy link

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to dataset.Concat(...) returns a new enumerable and does not modify dataset, so the strict dataset items aren’t actually added. Use a loop or a method like dataset.AddRange(...) (or iterate and dataset.Add(...)) to include those entries.

Suggested change
dataset.Concat(ListOfStrictCookieHeaderDataSet);
foreach (var item in ListOfStrictCookieHeaderDataSet)
{
dataset.Add(item);
}

Copilot uses AI. Check for mistakes.

dataset.Add(new[] { header1, header1 }.ToList(), new[] { string1, null, "", " ", ";", " , ", string1 });
return dataset;
}
}

public static TheoryData<IList<CookieHeaderValue>?, string?[]> ListWithInvalidCookieHeaderDataSet
{
get
Expand All @@ -127,18 +138,19 @@ public static TheoryData<string> InvalidCookieValues
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) });
Expand Down Expand Up @@ -248,7 +260,7 @@ public void CookieHeaderValue_ParseList_AcceptsValidValues(IList<CookieHeaderVal
}

[Theory]
[MemberData(nameof(ListOfCookieHeaderDataSet))]
[MemberData(nameof(ListOfStrictCookieHeaderDataSet))]
public void CookieHeaderValue_ParseStrictList_AcceptsValidValues(IList<CookieHeaderValue> cookies, string[] input)
{
var results = CookieHeaderValue.ParseStrictList(input);
Expand All @@ -267,7 +279,7 @@ public void CookieHeaderValue_TryParseList_AcceptsValidValues(IList<CookieHeader
}

[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);
Expand Down
2 changes: 1 addition & 1 deletion src/Http/Http.Extensions/test/HeaderDictionaryTypeExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ public void GetListT_StringWithQualityHeaderValidValue_Success()
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);

Expand Down
13 changes: 10 additions & 3 deletions src/Http/Http/test/RequestCookiesCollectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,18 @@ public void ParseManyCookies()
[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 }));
Expand Down
40 changes: 37 additions & 3 deletions src/Http/Shared/CookieHeaderParserShared.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ public static bool TryParseValue(StringSegment value, ref int index, bool suppor

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;
}
Comment on lines +92 to +102
Copy link

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This logic for finding the next semicolon and updating index is duplicated in two places. Consider extracting it into a helper method (e.g., SkipToNextSeparator) to reduce duplication and simplify maintenance.

Suggested change
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;
}
index = SkipToNextSeparator(value, current);

Copilot uses AI. Check for mistakes.

return false;
}

Expand All @@ -97,6 +108,17 @@ public static bool TryParseValue(StringSegment value, ref int index, bool suppor
// 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;
}

Expand All @@ -112,7 +134,7 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta
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;
}
Expand All @@ -125,8 +147,8 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta

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);
Expand All @@ -136,6 +158,18 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta
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)
{
Expand Down
Loading