HTTP Headers are enumerated in the wrong order when added via a mix of parsed and unparsed APIs

[geoffkizer]

Repro code:

request.Headers.Add("Accept", "text/foo");
request.Headers.TryAddWithoutValidation("Accept", "text/bar");

Console.WriteLine("NonValidated before parsing:");
PrintHeadersNonValidated(request);

// Force parsing
_ = request.Headers.Accept.Count;

Console.WriteLine("NonValidated after parsing:");
PrintHeadersNonValidated(request);

static void PrintHeadersNonValidated(HttpRequestMessage request)
{
    foreach (var header in request.Headers.NonValidated)
    {
        Console.WriteLine($"  {header.Key}:");
        foreach (var value in header.Value)
        {
            Console.WriteLine($"    {value}");
        }
    }

    Console.WriteLine();
}

Output:

NonValidated before parsing (incorrect):
  Accept:
    text/bar
    text/foo

NonValidated after parsing (correct):
  Accept:
    text/foo
    text/bar

Note that this behavior is not limited to NonValidated -- it affects how the request headers are written to the wire when the request is sent.

Mixing typed Add with untyped TryAddWithoutValidation is not a common thing to do, so the impact here is small.

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Repro code:

request.Headers.Add("Accept", "text/foo");
request.Headers.TryAddWithoutValidation("Accept", "text/bar");

Console.WriteLine("NonValidated before parsing:");
PrintHeadersNonValidated(request);

// Force parsing
_ = request.Headers.Accept.Count;

Console.WriteLine("NonValidated after parsing:");
PrintHeadersNonValidated(request);

static void PrintHeadersNonValidated(HttpRequestMessage request)
{
    foreach (var header in request.Headers.NonValidated)
    {
        Console.WriteLine($"  {header.Key}:");
        foreach (var value in header.Value)
        {
            Console.WriteLine($"    {value}");
        }
    }

    Console.WriteLine();
}

Output:

NonValidated before parsing (incorrect):
  Accept:
    text/bar
    text/foo

NonValidated after parsing (correct):
  Accept:
    text/foo
    text/bar

Note that this behavior is not limited to NonValidated -- it affects how the request headers are written to the wire when the request is sent.

Mixing typed Add with untyped TryAddWithoutValidation is not a common thing to do, so the impact here is small.

Author:	geoffkizer
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[stephentoub]

in the wrong order

Do we define/require a specific order today? Is this more about a new constraint we'd like to enable?

[ManickaP]

Not that this answers the question but just for a reference RFC 2616 says:

Multiple message-header fields with the same field-name MAY be
present in a message if and only if the entire field-value for that
header field is defined as a comma-separated list [i.e., #(values)].
It MUST be possible to combine the multiple header fields into one
"field-name: field-value" pair, without changing the semantics of the
message, by appending each subsequent field-value to the first, each
separated by a comma. The order in which header fields with the same
field-name are received is therefore significant to the
interpretation of the combined field value, and thus a proxy MUST NOT
change the order of these field values when a message is forwarded.

[geoffkizer]

Ordering of header values is significant for many multi-valued headers. In general we need to preserve the ordering as specified when adding these values. We do preserve this if you only add via parsed APIs, or only add via TryAddWithoutValidation. It's the mixing of these that leads to trouble.

There are really two issues here:

First: If you do Add and then TryAddWithoutValidation, then enumerate, you'll see the header that was added second get enumerated before the header that was added first.

It could be argued that mixing Add and TryAddWithoutValidation like this will not guarantee ordering across the two; for example, we could say that headers added via TryAddWithoutValidation always come first.

However, headers added with TryAddWithoutValidation get transformed into parsed headers whenever anything forces parsing, and thus parsing causes the headers to be reordered. That's the second issue, as shown above: forcing the headers to be parsed should not change their order.

[karelz]

Triage: Should be fairly simple to change the order in which we enumerate. Not critical, but we should be closer to RFC with the fix.