aspnet / AspNetWebStack

ASP.NET MVC 5.x, Web API 2.x, and Web Pages 3.x (not ASP.NET Core)
Other
858 stars 354 forks source link

Crash when deserializing a HttpResponseMessage on .NET Core #313

Closed Daniel-Svensson closed 2 years ago

Daniel-Svensson commented 2 years ago

When a response contains "Expires: -1" the System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsyncCore method fails. The method works as expected on .Net Framework, but fails on .Net Core 2.1+ including NET 6.0

This appears to be a break between .NET Core 2.0 and 2.1. w.r.t. how date headers are parsed. In 2.0, Expires: -1 is treated as valid.

My root cause/issue is that this bug breaks the the only good library for client side Http caching, which seems to have given many people headaches. https://github.com/aliostad/CacheCow/issues/213

The issues has been previously reported: #193 but was closed as external (which was only partly true), there is still code that needs to be fixed here in AspNetWebStack The dotnet/runtime part is fixed https://github.com/dotnet/runtime/issues/27218 and contains both testcase as well as further details.

Based on discussion in runtime repo it seems like the relevant code is: https://github.com/aspnet/AspNetWebStack/blob/main/src/System.Net.Http.Formatting/Formatting/Parsers/InternetMessageFormatHeaderParser.cs#L319-L334

And that validation should always be ignored for the Expires header, since it should be able to contain common values such as -1.

I am unsure about how you want to fix this since it can be changed just for the expires header in that specific method, or if the default value for header validation should change since it is possible to control that when creating the InternetMessageFormatHeaderParser

Stacktrace:

System.Net.Http.dll!System.Net.Http.Headers.HttpHeaderParser.ParseValue(string value, object storeValue, ref int index)
System.Net.Http.dll!System.Net.Http.Headers.HttpHeaders.ParseAndAddValue(System.Net.Http.Headers.HeaderDescriptor descriptor, System.Net.Http.Headers.HttpHeaders.HeaderStoreItemInfo info, string value)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.CurrentHeaderFieldStore.CopyTo(System.Net.Http.Headers.HttpHeaders headers, bool ignoreHeaderValidation)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.ParseHeaderFields(byte[] buffer, int bytesReady, ref int bytesConsumed, ref System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.HeaderFieldState requestHeaderState, int maximumHeaderLength, ref int totalBytesConsumed, System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.CurrentHeaderFieldStore currentField, System.Net.Http.Headers.HttpHeaders headers, bool ignoreHeaderValidation)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.ParseBuffer(byte[] buffer, int bytesReady, ref int bytesConsumed)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.HttpResponseHeaderParser.ParseBuffer(byte[] buffer, int bytesReady, ref int bytesConsumed)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsyncCore(System.Net.Http.HttpContent content, int bufferSize, int maxHeaderSize, System.Threading.CancellationToken cancellationToken)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content, int bufferSize, int maxHeaderSize, System.Threading.CancellationToken cancellationToken)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content, int bufferSize, int maxHeaderSize)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content, int bufferSize)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content)
CacheCow.Client.dll!CacheCow.Client.MessageContentHttpMessageSerializer.DeserializeToResponseAsync(System.IO.Stream stream)
pranavkm commented 2 years ago

We think a fix here might be to update https://github.com/aspnet/AspNetWebStack/blob/main/src/System.Net.Http.Formatting/Formatting/Parsers/HttpResponseHeaderParser.cs#L52 to use the 2nd constructor that turns of HTTP header validation as part of deserialization.

Alternatively a narrower fix might be to special case the Expires header.

pranavkm commented 2 years ago

Looks like this was previously addressed as part of 3.2.8: https://github.com/aspnet/AspNetWebStack/blob/release/3.2.8/src/System.Net.Http.Formatting/Formatting/Parsers/InternetMessageFormatHeaderParser.cs#L321-L326. I hasn't noticed this since main doesn't yet have this change. Marking this issue as resolved.

FYI @dougbu