Need Urgent fix for CookieHeaderParserShared throws exception when last cookie contains invalid character in .NET7

[kchinburarat]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

#45127 related to this issue Fix CookieHeaderParserShared throws exception when last cookie contains invalid character
and this #45014

I confirm that there is an issue in NET 7 and that the Asp.net core code has to be fixed. We cannot wait for the release of NET 8.

Expected Behavior

CookieHeaderParserShared.CookieHeaderParserShared Shoul not return true from this line
https://github.com/dotnet/aspnetcore/blob/v7.0.1/src/Http/Shared/CookieHeaderParserShared.cs#L75

because it will make this code thrown the exception

Steps To Reproduce

Change ParseManyCookies unit test to below code

  [Fact]
    public void ParseManyCookies()
    {
        var cookies = RequestCookieCollection.Parse(new StringValues(new[] { "errorcookie=dd,:(\"sa;" }));

        Assert.Equal(12, cookies.Count);
    }

Exceptions (if any)

unit test : https://github.com/dotnet/aspnetcore/blob/v7.0.1/src/Http/Http/test/RequestCookiesCollectionTests.cs#L43

.NET Version

7.0

Anything else?

No response

[kchinburarat]

CC: @cnblogs-dudu

[kchinburarat]

This null checking is still present in.NET 6.
if (parsedName != null && parsedValue != null)
https://github.com/dotnet/aspnetcore/blob/v6.0.12/src/Http/Shared/CookieHeaderParserShared.cs#L34

[cnblogs-dudu]

We found many cookies with nullable object must have a value came from baidu analytics.

"Hm_lvt_866c9be12d4a814454792b1fd0fed295=1640531828,1641040391,1642083522,1642236263;"

[cnblogs-dudu]

We have applied the following workaround in the dedicated middleware.

public class CheckCookiesMiddleware
{
    private readonly RequestDelegate _next;

    public CheckCookiesMiddleware(RequestDelegate next)
    {
        _next = next;
    }

    public async Task Invoke(HttpContext httpContext)
    {
        try
        {
            var cookies = httpContext.Request.Cookies;
        }
        catch (Exception ex) when (ex.Message.Contains("Nullable object must have a value"))
        {
            var logger = httpContext.RequestServices.GetRequiredService<ILogger<CheckCookiesMiddleware>>();
            var cookies = httpContext.Request.Headers[HeaderNames.Cookie];
            logger.LogInformation("Removing invalid Cookies: \n{cookies}", cookies);

            httpContext.Request.Headers[HeaderNames.Cookie] = string.Empty;
        }

        await _next(httpContext);
    }
}

[kchinburarat]

@cnblogs-dudu In my case, it is exactly the same. :)

[Tratcher]

Moving the discussion from #45014

Noisy error logs are not sufficient justification for a patch. There needs to be a functional business impact.

A) What impact is this having on your business?
B) Is there a viable workaround? Does the one shared above work for you?

It should be possible to work around this by checking the cookie header first for known invalid data and removing it or terminating the request.

[kchinburarat]

@Tratcher
A) What impact is this having on your business?
This problem may have an impact on our website ranking or marketing-related traffic because we observed in our log system that the bulk of the failed requests originated from crawler requests such as Google bot, which is the most concerning for our company.

B) Is there a viable workaround? Does the one shared above work for you?
The code above just prevents a request from failing, but we will lose all cookie information, which is undesirable.

It should be possible to work around this by checking the cookie header first for known invalid data and removing it or terminating the request.

IMO,Our code is web gateway If we use a cookie sanitizer, our performance may suffer

[kchinburarat]

@Tratcher I believe it would be beneficial if you could restore this code.
if (parsedName != null && parsedValue != null)

[kchinburarat]

@davidfowl @Tratcher
I found this code was remove from dotnet bot here
#35547 (comment)

FYI.
We are decided to roll back the production version from .NET 7 to.NET6

[davidfowl]

@Tratcher if this is common, we might want to revert this change. I’m thinking about the YARP scenario and people using ASP.NET Core as a gateway

[Tratcher]

@kchinburarat good find, I hadn't realized this was a regression. That does qualify for a patch then. I'll get it started.