Merge Vary header

[sstepanchuk]

Is this okay Vary header don't merge with Vary header from response (other layers)? each layer should add Vary header?

Here layers

let middleware_stack = ServiceBuilder::new()
        .layer(
            CorsLayer::new()
                .allow_origin(AllowOrigin::list(vec![
                    "http://test.com".parse::<HeaderValue>().unwrap(),
                    "http://example.com".parse::<HeaderValue>().unwrap(),
                ]))
                .allow_methods([Method::GET, Method::POST])
        )
        .layer(
            CompressionLayer::new()
                .quality(CompressionLevel::Best)
        );

Here resposne

HTTP/1.1 200 OK
content-type: application/json
vary: origin, access-control-request-method, access-control-request-headers
vary: accept-encoding
content-length: 109
date: Wed, 05 Feb 2025 17:45:27 GMT

[samgiles]

I think this is an issue in hyper, rather than a tower specific issue. Note it's not invalid according to the HTTP spec in this case.

We can probably achieve this with a Tower layer. Here's what I'm now using in my side project having stumbled upon this issue:

https://gist.github.com/samgiles/eacdbc35dc5ff1d2ec4ce8d2aef54f67 <- would be happy to see this contributed if it's useful .

[jplatte]

This is definitely not related to hyper. Where did you find that this is not valid? I'm pretty sure MDN said it is valid when I wrote this code.

[samgiles]

@jplatte I said:

Note it's not invalid

I think you misread 😅

This is definitely not related to hyper

In my testing last night it was the serialization of the HeaderMap onto the wire that caused this duplicate. See the link to hyper (I’m using hyper and I’ve guessed OP is too).

This crate isn’t handling the protocol itself right?

[jplatte]

This crate is adding the header to an http::Response. It doesn't encode that response, but it has the ability to look up an existing header and update that instead of adding its own to the header multimap.

[samgiles]

it has the ability to look up an existing header

Ah right, I have a failing test in a branch, perhaps if you can point me to where this is happening I can propose a fix in a PR, I couldn't find this before

[jplatte]

Well first, why "fix" what's not broken?

[samgiles]

It’s not ideal behaviour according to the spec. RFC7230 is perhaps a little ambiguous.

RFC 7230, Section 3.2.2, Field Order

A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).

But then goes on to say:

A recipient MAY combine multiple header fields with the same field
name into one "field-name: field-value" pair, without changing the
semantics of the message, by appending each subsequent field value to
the combined field value in order, 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; a proxy MUST NOT change the order of these field values when
forwarding a message.

I interpret this as - please don’t send messages like this but clients can handle it if they want to, but they don’t need to. So I see fixing this as helping to conform closer to the spec.

[jplatte]

I don't interpret it the same way. A recipient being allowed to coalesce multiple vary header values into a single one before further processing it is one thing, but that doesn't mean that it should just ignore one of the headers if it does not first coalesce all into one.

The main reason I am hesitant about this is that the http crate doesn't really provide a good API to modify (concatenate) HeaderValues. Currently, what the CORS middleware does is it internally builds up the vary header value as BytesMut (which can easily be modified), then does one conversion step to HeaderValue which is known to never fail because all bytes ever added to the BytesMut buffer are valid header values. If we wanted to update an existing header value, AFAIK with https current API, we'd first have to convert the existing value to BytesMut, which (a) incurs extra allocation costs and (b) is fallible.