tower-rs / tower-http

HTTP specific Tower utilities.
680 stars 159 forks source link

cors: Don't overwrite vary header set by the inner service #398

Closed jplatte closed 8 months ago

jplatte commented 1 year ago

Motivation

Fixes #397.

Solution

For GET requests where we call an inner service and compute headers for it (as opposed to assembling the whole response to be sent to the client), append those computed headers instead of overwriting existing headers set by the inner service.

@AlyoshaVasilieva: can you confirm that two vary headers in the response are okay for this case? With the first commit of this PR, we will no longer create multiple vary headers in the middleware, but with the second commit we will just keep all existing headers from the inner service so the end result in your example will be

< vary: accept, accept-encoding
< vary: origin, access-control-request-method, access-control-request-headers
AlyoshaVasilieva commented 1 year ago

My understanding is that it's permitted, RFC 7230 says https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.2 :

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).

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.

Since Vary is a list, the multiple Vary headers should be treated as a single list.

Random StackOverflow answers appear to agree that caches can handle multiple Vary headers.

I think the exception is that Vary: * can't appear with other Vary headers, as that value isn't a list, but I'm not experienced in this area. I've personally never seen Vary: *.

jplatte commented 1 year ago

Right. The first bit is making me question the append for the other headers though. I think we might have to overwrite some (maybe while logging an error), and only append where multiple values are explicitly allowed.

jplatte commented 11 months ago

CI failure to be fixed by #418.

jplatte commented 11 months ago

Updated to only append vary headers, and overwrite others since they're not allowed to occur multiple times in the output. Maybe it would make sense to also deduplicate vary values, but that can be a separate change (it's basically just an optimization).