WebApi: if the HTTP request contains a cookie with an invalid character in the name, all cookies are ignored

[RobSiklos]

Let's say you have a cookie header like the following:

Cookie: c1=value1; c2=value2; invalid:cook=should_not_have_colon_in_name

The last cookie is invalid, because it has a colon character (:) in the name. At least according to https://gist.github.com/jeremiahlee/ff7dfb60572211dce306.

The problem is that when trying to read the cookies for such a request, GetCookies() (System.Net.Http.HttpRequestHeadersExtensions.GetCookies()) returns an empty set (with no error/warning message explaining why).

I would expect that at least the valid cookies would be returned (as is the case when trying to read the cookies from MVC, via HttpRequestBase.Cookies). Also, there should be a way to figure out why the cookies are missing.

[RobSiklos]

I think maybe the return false; on

AspNetWebStack/src/System.Net.Http.Formatting/Headers/CookieHeaderValue.cs

Line 173 in a18ca4a

return false;

should be changed to continue;.

[mkArtakMSFT]

@dougbu, is there a reason for this behavior?

[dougbu]

@mkArtakMSFT not sure. Would take either @Eilon remembering a deep dark secret or some searching through Web API to see if this "give up on first bad header" behaviour is the general case.

[mkArtakMSFT]

Thanks for reporting this issue. We agree that this seems to be a bug and we've assigned one of our team members to look into this based on its priority.

[Eilon]

I would guess that this behavior likely precedes our team ever owning this code, so no one around would know the history or reasoning.

I've given this a fair bit of thought and I think it's reasonable to switch the code to a continue;.

And, of course, cc @Tratcher because there might be some good learnings from what we do in ASP.NET Core that we can apply here.

[Tratcher]

In Core we added a non-strict mode that skips individual invalid values:
https://github.com/aspnet/HttpAbstractions/blob/2ce2d8b6c5c4d3111f5bbfe642c478f229bd955f/src/Microsoft.Net.Http.Headers/HttpHeaderParser.cs#L78-L97

[Eilon]

@Tratcher ah interesting. So it looks like, in some sense, non-strict (e.g. ignore garbage) is the "default," and one would have to opt in to get strict behavior.

I think that if we include this fix in a patch for WSR, we might want to leave it as-is by default, but have a flag to enable it (e.g. Quirks Mode). Definitely don't want to regress anything in a project that's many years old.