HttpMessageContent breaks on dotnet core 2.1

[aliostad]

Hi,

[NOT ENTIRELY SURE IF THIS IS ASP.NET OR COREFX]

I have an HTTP Caching library for .NET and I use HttpMessageContent class to help me serialise and deseralise the messages. This has been working throughout including .NET Core 2.0 but it seems to have been broken by .NET Core 2.1 on Mac.

Here is the repro code. Works with netcoreapp2.0 but breaks with netcoreapp2.1.

Project file:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
    <LangVersion>latest</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNet.WebApi.Client" Version="5.2.6" />
  </ItemGroup>
</Project>

Program.cs:

using System;
using System.IO;
using System.Net.Http;
using System.Threading.Tasks;

namespace CacheCowCrashRepro
{
    class Program
    {
        static async Task Main(string[] args)
        {
            try
            {
                var serializer = new MessageContentHttpMessageSerializer();
                var client = new HttpClient();
                var request = new HttpRequestMessage(HttpMethod.Get, new Uri("https://google.com"));
                var response = await client.SendAsync(request);
                var ms = new MemoryStream();
                await serializer.SerializeAsync(response, ms);
                ms.Position = 0;
                // var bytes = ms.ToArray();
                // File.WriteAllBytes("response.bin", bytes); // to store 
                var r2 = await serializer.DeserializeToResponseAsync(ms);
                Console.WriteLine(response);
            }
            catch (Exception e)
            {
                Console.WriteLine(e.ToString());
            }
        }
    }

    public class MessageContentHttpMessageSerializer
    {
        private bool _bufferContent;

        public MessageContentHttpMessageSerializer()
            : this(true)
        {

        }

        public MessageContentHttpMessageSerializer(bool bufferContent)
        {
            _bufferContent = bufferContent;
        }

        public async Task SerializeAsync(HttpResponseMessage response, Stream stream)
        {
            if (response.Content != null)
            {
                if (_bufferContent)
                    await response.Content.LoadIntoBufferAsync();
            }

            var httpMessageContent = new HttpMessageContent(response);
            var buffer = await httpMessageContent.ReadAsByteArrayAsync();
            stream.Write(buffer, 0, buffer.Length);
        }

        public async Task SerializeAsync(HttpRequestMessage request, Stream stream)
        {
            if (request.Content != null && _bufferContent)
            {
                await request.Content.LoadIntoBufferAsync();
            }

            var httpMessageContent = new HttpMessageContent(request);
            var buffer = await httpMessageContent.ReadAsByteArrayAsync();
            stream.Write(buffer, 0, buffer.Length);
        }

        public async Task<HttpResponseMessage> DeserializeToResponseAsync(Stream stream)
        {
            var response = new HttpResponseMessage();
            response.Content = new StreamContent(stream);
            response.Content.Headers.Add("Content-Type", "application/http;msgtype=response");
            var responseMessage = await response.Content.ReadAsHttpResponseMessageAsync();
            if (responseMessage.Content != null && _bufferContent)
                await responseMessage.Content.LoadIntoBufferAsync();
            return responseMessage;
        }

        public async Task<HttpRequestMessage> DeserializeToRequestAsync(Stream stream)
        {
            var request = new HttpRequestMessage();
            request.Content = new StreamContent(stream);
            request.Content.Headers.Add("Content-Type", "application/http;msgtype=request");
            var requestMessage = await request.Content.ReadAsHttpRequestMessageAsync();
            if (requestMessage.Content != null && _bufferContent)
                await requestMessage.Content.LoadIntoBufferAsync();
            return requestMessage;
        }
    }
}

Here is the response I get which is nothing special. The only thing I notice is that there is no ContentLength header and encoding is chunked but looking at the message, I could not see a chunked encoding, the response is all in one block - maybe I missed.

response.bin

[davidsh]

HttpMessageContent is not part of .NET Core. It is a class provided by System.Net.Http.Formatting which is owned by ASP.NET WebAPI.

The PackageReference for WebAPI is bringing in System.Net.Http.Formatting:

<PackageReference Include="Microsoft.AspNet.WebApi.Client" Version="5.2.6" />

https://apisof.net/catalog/System.Net.Http.HttpMessageContent

cc: @Tratcher

[aliostad]

Should I open this issue under aspnet/home ?

[aliostad]

It is true that it is part of the asp.net but it breaks with netcoreapp2.1 and works with netcoreapp2.0.
It is a wild guess but I think possibly changes for HTTP2 broke it - regardless, it seems underlying stuff - to use the technical phrase - has been changed.

[I closed it to open under ASP.NET but prefer for you to tell me to move it hence re-opened it]

[karelz]

It should be evaluated first higher in the stack. If it is CoreFX problem, then please isolate a repro which does not use ASP.NET components at all.

[dougbu]

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. In 2.1, the parsers throw a FormatException on this header. In particular, the 2.1 exception is thrown from https://github.com/dotnet/corefx/blob/a10890f4ffe0fadf090c922578ba0e606ebdd16c/src/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderParser.cs#L74

The message is "The format of value '-1' is invalid.". I'm not sure what handles -1 in 2.0 or whether the the Expires header was handled as a non-date then.

I don't have time right at the moment to create a smaller repro. But, this core scenario involves a HttpHeaders subclass that overrides nothing (i.e. just make it concrete) and calling headers.Add("Expires", "-1").

[karelz]

Is Expires: -1 a valid date header value?
2.0 and 2.1 have entirely different implementations. We are not bug-by-bug compatible. If the format is allowed in RFC, or is used by wider set of servers, we should recognize it.

[davidsh]

Is Expires: -1 a valid date header value?

According to RFC 7234, we are supposed to handle Expires: -1 as a date that represents an arbitrary time in the past. We should not throw an exception.

A cache recipient MUST interpret invalid date formats, especially the
value "0", as representing a time in the past (i.e., "already
expired").

[Tratcher]

-1 is quite common but invalid per spec.

[davidsh]

More discussion:
https://stackoverflow.com/questions/11357430/http-expires-header-values-0-and-1

http://www.httpwatch.com/httpgallery/headers/

Expires: -1

The Expires header specifies when the content should be considered to be out of date. The value -1
indicates that the content expires immediately and would have to be re-requested before being
displayed again.

[davidsh]

If the format is allowed in RFC, or is used by wider set of servers, we should recognize it.

Based on this, I think we should fix .NET Core to be more resilent with getting "Expires: -1" etc. headers.

[stephentoub]

if it is CoreFX problem, then please isolate a repro

using System.Net.Http.Headers;

class Program
{
    static void Main()
    {
        var headers = new DerivedHttpHeaders();
        headers.Add("Expires", "-1");
    }
}

internal sealed class DerivedHttpHeaders : HttpHeaders { }

[karelz]

Not surprisingly, we do not work correctly even for Expires: 0.
Per spec, we should treat all invalid dates as "expired in the past". I wonder if it would be useful for (server) diagnostic to log invalid values (at least those which are not typical like 0 and -1).

This may be good candidate for 2.1 / 2.2 fix if it impacts more people and/or it impacts someone hard. @aliostad were you able to work around the problem for now?

[stephentoub]

I don't think this is a case of a regression in the handling of "Expires" with regards to the actual content Expires header, e.g. this fails on both .NET Framework and .NET Core (all versions I tested):

using System.Net.Http;

class Program
{
    static void Main()
    {
        var headers = new ByteArrayContent(new byte[0]).Headers;
        headers.Add("Expires", "-1");
    }
}

with:

Unhandled Exception: System.FormatException: The format of value '-1' is invalid.
   at System.Net.Http.Headers.HttpHeaderParser.ParseValue(String value, Object storeValue, Int32& index)
   at System.Net.Http.Headers.HttpHeaders.ParseAndAddValue(String name, HeaderStoreItemInfo info, String value)
   at System.Net.Http.Headers.HttpHeaders.Add(String name, String value)
   at Program.Main()

This fails because it uses the DateTimeParser to parse the value. However, what's changed in .NET Core 2.1 is that this DateTimeParser is now used to parse an arbitrary header named "Expires", i.e. when it's being added to any HttpHeaders collection rather than to an HttpContentHeaders collection.

Previously, the knowledge that "Expires" was associated with a DateTimeParser only came as part of instantiating an HttpContentHeaders instance, with that assocation stored in the HttpContentHeader's parser store:
https://github.com/dotnet/corefx/blob/d6173e069a9bcedfdfd7f4f41e67d23f67157b61/src/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs#L169
which is then passed down to the base instance for use in calls to Add and such as part of its ctor:
https://github.com/dotnet/corefx/blob/d6173e069a9bcedfdfd7f4f41e67d23f67157b61/src/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs#L149-L154

But dotnet/corefx#23091 changed that (cc: @geoffkizer). Now as of that PR, HttpHeaders itself is aware of all known headers and their associated parsers, so whereas previously any value at all could be stored in an "Expires" header added to a custom HttpHeaders-derived type (e.g. this succeeds):

using System.Net.Http.Headers;

class Program
{
    static void Main()
    {
        var headers = new DerivedHttpHeaders();
        headers.Add("Expires", "the best expires value ever");
    }
}

internal sealed class DerivedHttpHeaders : HttpHeaders { }

now such use validates the value for "Expires" with DateTimeParser, and the above of course fails.

So, there are then two questions:

1. Does this particular case matter? i.e. did we fix a bug here (e.g. "Expires" should never have allowed such erroneous values) or did we break something we need to fix?
2. Separate from that, even though "-1" has never been supported, should it be allowed?

[dougbu]

Does this particular case matter?

As @Tratcher said, Expires: -1 is really common. And, @davidsh mentioned any Expires value has a well-known meaning.

even though "-1" has never been supported

Not validating the Expires header (as in 2.0) was technically the right thing to do per RFC 7234. 0 and -1 might be well known but probably shouldn't be special-cased.

[stephentoub]

@dougbu, for (1), I'm taking about the difference between whether HttpHeaders should itself know about Expires or not. Previously it didn't; now it does.

The case that Chris is talking about has never been supported as far as I can tell. That's my (2).

[aliostad]

Thank you guys. It seems the issue has been isolated now. It did appear to me to be related to chunked encoding but that turns out to be a coincidence.

So I understand my other issue https://github.com/dotnet/corefx/issues/31918 in corefx has been re-opened. I will leave it to you whether to close this one.

[davidsh]

The end-to-end scenario needs to be fixed. If an HTTP request receives a response from a server and that response has a response header of "Expires: -1" (or anything invalid), our HTTP stack should ignore the header. We should not throw an exception.

Semantically, our HTTP stack doesn't really do anything with that header because it is really for browsers that cache responses etc. But at the very least, the HTTP request should not fail because it got an response header for "Expires" that has an invalid date format.

[Tratcher]

Ignoring the value is not quite enough, the presence of the header is significant. If the client asks for the value of that header from the strongly typed property they shouldn't get null, they should get DateTime.MinValue or similar.

[aliostad]

If an HTTP request receives a response from a server and that response has a response header of "Expires: -1" (or anything invalid), our HTTP stack should ignore the header. We should not throw an exception.

@davidsh this is not technically true. A cache server or intermediary is free to cache any resource that does not have cache directives. But Expires: "-1" is a cache directive (albeit against spec) which should not be ignored. This means item can be cached but not without validating with the server first.

[davidsh]

@davidsh this is not technically true. A cache server or intermediary is free to cache any resource that does not have cache directives. But Expires: "-1" is a cache directive (albeit against spec) which should not be ignored. This means item can be cached but not without validating with the server first.

The HTTP stack in .NET Core doesn't cache responses. It doesn't function like a browser. So, practically speaking, the HTTP stack doesn't do anything because of the presence of the header.

[davidsh]

Ignoring the value is not quite enough, the presence of the header is significant. If the client asks for the value of that header from the strongly typed property they shouldn't get null, they should get DateTime.MinValue or similar.

Yes, that is probably a good compromise especially since the RFC implies that invalid date formats should be treated as sometime in the "past".

[stephentoub]

The end-to-end scenario needs to be fixed.

That's fine, I just want to make sure it's clear that, as far as I can tell (and I could of course be wrong), that end-to-end scenario never worked with HttpClient, at least not if it's the issue highlighted in the simple repro. We can of course choose to make it work, but that would be fixing something other than the regression for which this issue was opened, which is that previously there was zero validation applied to an Expires header associated with an HttpHeaders instance other than HttpContentHeaders, and now the same validation is applied to all HttpHeaders instances, regardless of whether it's HttpContentHeaders or not.

[xela30]

So, what's the plan of attack, guys? Should we expect it in 2.2?

[stephentoub]

Should we expect it in 2.2?

No

[pietervdvn]

I've have been encountering this issue as well.

It seems that the Server-header is the culprit. Removing this header from the input stream removes all issues.

[geoffkizer]

But dotnet/corefx#23091 changed that (cc: @geoffkizer). Now as of that PR, HttpHeaders itself is aware of all known headers and their associated parsers, so whereas previously any value at all could be stored in an "Expires" header added to a custom HttpHeaders-derived type (e.g. this succeeds):

This is a bug, I believe. I'm not sure why an HttpHeaders-derived type is causing this validation to occur, but it shouldn't be.

[geoffkizer]

The problem seems to be here: https://github.com/dotnet/corefx/pull/23091/files#diff-3b3d828e1c3f9a9d2e926e471d0ca21cR49

For custom HttpHeader derived types, this is causing all known headers to be validated, instead of being treated as custom headers.

[aliostad]

Is this coming in 3.0?

[karelz]

Milestone says 3.0, so it has a chance. We will see.

[aliostad]

It is just I have people complaining using my library, It is broken here, I am at loss why such a critical bug has been open for so long. I am happy to supply a PR, if that makes it quicker.

[karelz]

Why do you think it is that critical? I see only 3 people commenting here and no upvotes on the top post. It does not seem to be affecting that many.

If you can submit PR, that would significantly increase chances to get it fixed in 3.0. We would definitely appreciate that!

[aliostad]

I am just testing this on .NET Core 3.0, expecting it would have been solved but I still get the same error as .NET Core2.1 - Can you please confirm the status of this issue?

As you can see it is broken for 2.1 and 3.0 alike.

[davidsh]

I am just testing this on .NET Core 3.0, expecting it would have been solved but I still get the same error as .NET Core2.1 - Can you please confirm the status of this issue?

It was assumed this issue was fixed in .NET Core 3.0 with PR dotnet/corefx#36908. If that is not the case, please open a new issue and attach your repro. It's possible you are hitting a different bug.

[aliostad]

@davidsh Repro is exactly the same and the error is exactly the same:

System.InvalidOperationException: Error parsing HTTP message header byte 818 of message System.Byte[].
   at System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsyncCore(HttpContent content, Int32 bufferSize, Int32 maxHeaderSize, CancellationToken cancellationToken)
   at CacheCowCrashRepro.MessageContentHttpMessageSerializer.DeserializeToResponseAsync(Stream stream) in C:\Users\aliostad\RiderProjects\ConsoleApp1\Program.cs:line 78
   at CacheCowCrashRepro.Program.Main(String[] args) in C:\Users\aliostad\RiderProjects\ConsoleApp1\Program.cs:line 23

How do you say it is fixed? Simply put the repro code I have supplied and bang it crashes the same way in .NET 3.0 as in .NET 2.1.

I can create a new issue but not sure why.

[davidsh]

Simply put the repro code I have supplied and bang it crashes the same way in .NET 3.0 as in .NET 2.1.

@aliostad Are you referring to your original repro you posted here ?

[aliostad]

Yes please. Just put it in a console app with netcoreapp3.0 and you should see the same error - or maybe I am doing something completely wrong.

[karelz]

@aliostad if you look at the bug history, you can notice there was a PR to fix it: https://github.com/dotnet/corefx/issues/31918#ref-pullrequest-433538984
It is quite possible that it did not fix the problem, or there were multiple problems and it fixed just part of them, etc. Pity we did not get verification before we shipped 3.0.
Either way, filing a new issue will be easier as it will be targeted at 3.0 repro with clean discussion without the noise above. Does it make sense?

[wfurt]

The fix was for underlying parsing issue. Note that HttpContentMessageExtensions and ReadAsHttpResponseMessageAsyncCore are not part of corefx.

The code would need to be updated to use TryAddWithoutValidation()

[davidsh]

@Tratcher At this point, it looks like the bug is now in ASP.NET component as described here

<PackageReference Include="Microsoft.AspNet.WebApi.Client" Version="5.2.6" />

Where is the code source for the HttpMessageContent component? And is there a recommended workaround or newer component in ASP.NET that would help here?

Update:
It seems NuGet.org for the Microsoft.AspNet.WebApi.Client package says the source code lives here. But this package hasn't been updated in a while:

And @danroth27 says the package is still supported for .NET Core.

[davidsh]

I suspect that the bug fix for this needs to made in this source code here to use .TryAddWithoutValidation() instead of .Add():

https://github.com/aspnet/AspNetWebStack/blob/ebef5b7d821b64ed5c48765d0caf6ce8a9bcfaf5/src/System.Net.Http.Formatting/Formatting/Parsers/InternetMessageFormatHeaderParser.cs#L319-L334

[Tratcher]

Indeed, a new bug will need to be opened in https://github.com/aspnet/AspNetWebStack/.

[aliostad]

5.2.7 creates the same problem

[aliostad]

Sorry @Tratcher do you want me to open the bug? I am happy to do that.

[Tratcher]

Yes please, you seem to have the most context on the issue.