Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 13, 2024

Conversation

jplatte
Copy link
Collaborator

@jplatte jplatte commented Aug 17, 2023

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
Copy link
Contributor

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
Copy link
Collaborator Author

jplatte commented Aug 17, 2023

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
Copy link
Collaborator Author

jplatte commented Oct 1, 2023

CI failure to be fixed by #418.

@jplatte jplatte closed this Oct 1, 2023
@jplatte jplatte reopened this Oct 1, 2023
@jplatte jplatte changed the title cors: Don't overwrite headers set by the inner service cors: Don't overwrite vary header set by the inner service Oct 1, 2023
@jplatte
Copy link
Collaborator Author

jplatte commented Oct 1, 2023

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

@jplatte jplatte merged commit 43b7196 into main Jan 13, 2024
11 checks passed
@jplatte jplatte deleted the jplatte/cors-header-map branch January 13, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CORS middleware blocks other Vary headers from being set
3 participants