Skip to content

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jul 11, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 11, 2025 16:10
@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jul 11, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances cookie parsing by skipping over invalid segments and tightening separator logic, while expanding test scenarios for strict and lenient parsing.

  • Added logic in TryParseValue to skip past invalid cookie segments up to the next semicolon.
  • Updated GetNextNonEmptyOrWhitespaceIndex to treat only ; as a separator.
  • Expanded invalid-cookie test cases in RequestCookiesCollectionTests and restructured test datasets in CookieHeaderValueTest.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Http/Shared/CookieHeaderParserShared.cs Skip invalid cookie values by advancing index to next ;.
src/Http/Http/test/RequestCookiesCollectionTests.cs Added more invalid-cookie scenarios and expected outcomes.
src/Http/Headers/test/CookieHeaderValueTest.cs Renamed and restructured theory datasets; introduced strict set.

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.

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

@wtgodbe wtgodbe enabled auto-merge (squash) July 11, 2025 19:05
@wtgodbe wtgodbe merged commit 0608937 into main Jul 11, 2025
27 of 28 checks passed
@wtgodbe wtgodbe deleted the wtgodbe/CookieParsing branch July 11, 2025 19:31
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview7 milestone Jul 11, 2025
@TimmeeNz
Copy link

TimmeeNz commented Sep 4, 2025

Can I please ask why this change was made? Why is it not associated with any issue? It seems like a breaking change for anyone using a comma as a separator for cookies . This seems to have been included in .Net 8.0.19 was this the intention? At the very least it should be called out in some release notes? We just spent a couple of days trying to work out what was going on with a client calling our API that was using cookies for authentication, with it working in some environments and not working in others. We finally determined it was a difference between dot net 8 versions and then tracked down this change. Very frustrating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants