Use TryAddWithoutValidation to prevent parsing potentially illegal headers

[onyxmaster]

Hi!

Currently, when automatic decompression is used, the headers are copied between instances of HttpContent to facilitate the decompression. Unfortunately, the HttpContentHeaders.Add method used in HttpContentHeadersExtensions performs header parsing, which tests whether the headers are valid. This leads to exceptions like "The format of value '0' is invalid." triggered by .NET Framework's DateHeaderParser being too strict when parsing the Expires: 0 header, while RFC 9111 explicitly mentions this:

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

Also, this can lead to a misinterpretation of other header fields that do not perfectly roundtrip by a parse->convert step of HttpContentHeaders.Add. I believe that decompression should not intervene into header processing and pass content headers as-is.

To improve this, I propose using HttpContentHeaders.TryAddWithoutValidation that will copy the headers without any other processing. I'm not sure whether the result value should be checked and acted upon by, for example, throwing some kind of exception since this might defeat the original purpose.

Thanks for looking into this.

[TalAloni]

Sorry for the delay.
This project is a fork, it's based on .NET Core SocketsHttpHandler with some fixes / enhancements taken from later .NET versions, Ideally we want to port fixes rather than making our own fixes, can you provide a reference to .NET runtime equivalent change?

[onyxmaster]

I believe that AddHeaders method in question was added by you to polyfill the internal HttpHeaders.AddHeaders which in its original state was implemented differently, so, unfortunately, I cannot point you to the "fix" in the .NET runtime, because the original code from this repository does not exactly match the original code as it was open sourced in 2016.
Still, if one would take a look at the current version, it has a special path to handle raw values, cloning them unchanged. So even if "Expires: 0" fails to be parsed by the header parser in modern code, it would still be passed on via the ParsedAndInvalidValues unchanged instead of throwing an exception on cloning the headers.

[onyxmaster]

I think that proper fix would entail implementing HttpHeaders.AddHeaders from scratch, but it looks that implementation in current .NET is completely incompatible with what's in there for HttpHeaders in netstandard2.0 TFM, so I don't believe it's feasible, so it looks like emulating the functionality is the best we can do.

[TalAloni]

Thanks,
I do vaguely recall I was not very happy with the AddHeaders extension method.
I do recall copying a bunch of HTTP header names and trying to implement something closer to the actual HttpHeaders.AddHeaders as implemented in .NET Core 2.2 only to give up when realizing I need to add more and more code.
I had more time and motivation to get it right back then than I do now, I'm not sold on the suggestion that no validation is better than too much validation.

Also - I don't recall any problem in my many tests so maybe the issue is limited to very special circumstances with specific server values that are uncommon - and not a common problem.

At this point I have to ask myself whether the use case is too niche for me to spend time on, given that a perfect solution is not going to be easy.

[onyxmaster]

I believe that it's at least surprising that enabling support for compression in client breaks communication with servers that use documented header of "Expires: 0". Yes, REST API servers usually don't do that, but when downloading dynamically-generated content, it is quite common.

As for the "lack of the validation" -- there is none in the first place in the current version of the code, it already skips validation of received headers.

So the proposed change does not remove the validation that has any robustness or correctness value, it prevents accidental and unexpected breakage of already tested and working code when compression is enabled that is entirely dependent on the quirks of the server.

[TalAloni]

You make a good case, I'll take another look.

[TalAloni]

I'm seeing that you also added dotnet/corefx#37391 - are you sure we must have both changes or none? why are those 2 in the same pull request?

[onyxmaster]

I'm seeing that you also added dotnet/corefx#37391 - are you sure we must have both changes or none? why are those 2 in the same pull request?

This is an example of my stupidity =)
I forgot to create the feature branch for this PR and pushed a "2.2" branch. I was also preparing another PR which is not yet ready, and pushed it in the same branch so it went into this PR.
I force-pushed the branch which now does not contain extra commits.
I'm sorry for the confusion.

[TalAloni]

Thanks, and thanks for taking the time to explain. this change is included in v2.2.0.10