Fix CookieHeaderParserShared throws exception when last cookie contains invalid character #45014

[korteksz]

Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong which contain separator characters. Reported by this issue: #45014

Fix CookieHeaderParserShared throws exception when last cookie contains invalid character #45014

• [x] You've read the Contributor Guide and Code of Conduct.
• [x] You've included unit or integration tests for your change, where applicable.
• [x] You've included inline docs for your change, where applicable.
• [x] There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong (partly) which contain separator characters.

More details about the problem: https://github.com/dotnet/aspnetcore/issues/45014#issuecomment-1312540487

Fixes #45014

[ghost]

Thanks for your PR, @korteksz. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[dnfadmin]

All CLA requirements met.

[Tratcher]

Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong (partly) which contain separator characters.

I don't think this is the right aproach, it seems to have caused many cookie related failures in the SignalR tests. https://dev.azure.com/dnceng-public/public/_build/results?buildId=85624&view=ms.vss-test-web.build-test-results-tab&runId=1791430&paneView=debug&resultId=100695

supportsMultipleValues means that a single header string might contain multiple cookies. That's fine, especially since it's common for cookie headers to be concatenated incorrectly.

I think there's a lower issue where something is returning true but null, where it should be returning false. Probably here: https://github.com/dotnet/aspnetcore/blob/860b12b64d49aa7ffae534601844dd54d0363ebf/src/Http/Shared/CookieHeaderParserShared.cs#L29

[BrennanConroy]

Can you please review and sign the CLA? https://github.com/dotnet/aspnetcore/pull/45127#issuecomment-1317296994

[korteksz]

@BrennanConroy @dnfadmin I have already signed the CLA from my mobile phone and when I try to do it from the desktop, then I got MongoTopologyClosedError: Topology is closed error even if I click the recheck link below. When I open the link from that comment then I get this one Error There is no CLA to sign for dotnet/aspnetcore. Can you help me what should I do in this case?

Edit Now I tried it again and it was working this time, só it is signed now

[korteksz]

Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong (partly) which contain separator characters.

I don't think this is the right aproach, it seems to have caused many cookie related failures in the SignalR tests. https://dev.azure.com/dnceng-public/public/_build/results?buildId=85624&view=ms.vss-test-web.build-test-results-tab&runId=1791430&paneView=debug&resultId=100695

supportsMultipleValues means that a single header string might contain multiple cookies. That's fine, especially since it's common for cookie headers to be concatenated incorrectly.

I think there's a lower issue where something is returning true but null, where it should be returning false. Probably here:

https://github.com/dotnet/aspnetcore/blob/860b12b64d49aa7ffae534601844dd54d0363ebf/src/Http/Shared/CookieHeaderParserShared.cs#L29

@Tratcher Thanks for pointing this out. I assumed that this won't be the right solution. That changeset is revoked now and I've got a different one commited and the pipelines are green as well.

The problem is that if you have a cookie like errorcookie=dd,:("sa; then it gets parsed currently like: Key: errorcookie Value: dd -> because it parses the cookie until it doesn't find a non-cookie character, in this case the ',' char. Then the while loop keeps iterating further until the last character which is a ';'. As far as this is a separator and we are at the end of the input value and because supportsMultipleValues is true we return true at the end, however our parsedName and parsedValue is null. To avoid that I added a check to ignore separators at the end of the cookie value. Hope this one is better than the previous :)

[cnblogs-dudu]

This PR disappeared in the release .NET 7.0.1, please see CookieHeaderParserShared.cs#L29

[ghost]

Hi @cnblogs-dudu. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[Tratcher]

/backport to release/7.0

[github-actions[bot]]

Started backporting to release/7.0: https://github.com/dotnet/aspnetcore/actions/runs/3751657045

[github-actions[bot]]

@Tratcher backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong which contain separator characters. Reported by this issue: #45014
Using index info to reconstruct a base tree...
M	src/Http/Http/src/Internal/RequestCookieCollection.cs
M	src/Http/Http/test/RequestCookiesCollectionTests.cs
M	src/Http/Shared/CookieHeaderParserShared.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Http/Shared/CookieHeaderParserShared.cs
CONFLICT (content): Merge conflict in src/Http/Shared/CookieHeaderParserShared.cs
Auto-merging src/Http/Http/test/RequestCookiesCollectionTests.cs
Auto-merging src/Http/Http/src/Internal/RequestCookieCollection.cs
CONFLICT (content): Merge conflict in src/Http/Http/src/Internal/RequestCookieCollection.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong which contain separator characters. Reported by this issue: #45014
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions[bot]]

@Tratcher an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

[ogxd]

I believe this pull request introduced a small breaking change. I did not test on this repo source directly but from my own code. Here is a repro:

[Fact]
public void Repro()
{
  var type = Type.GetType("Microsoft.Net.Http.Headers.CookieHeaderParserShared, Microsoft.Net.Http.Headers");
  var method = type.GetMethod("TryParseValues", BindingFlags.Public | BindingFlags.Static);
  var strValue = new StringValues("cookie1=123;cookie2="); // <- last cookie has no value
  var dic = new Dictionary<string, string>();
  method.Invoke(null, [strValue, dic, true]);
  Assert.True(dic.Count == 2);
}

This code passes in .NET 6 and .NET 7, but does not on .NET 8. Since this PR (I believe), the last cookie with an empty value is ignored, and the dictionary count is now 1.

I am not sure what the behaviour should be to match the W3C as close as possible but:

• It is a breaking change, likely collateral to this PR
• If we ever want to create a cookie with an empty value, it is not possible anymore (but should this be a thing in the first place? I don't know).

[Tratcher]

Comments on closed issues are not tracked, please open a new issue with the details for your scenario.