Skip to content

Relaxed Vary header defaults#674

Open
seun-ja wants to merge 5 commits into
tower-rs:mainfrom
seun-ja:539-CORS-module-excessively-sets-the-Vary-response-header-
Open

Relaxed Vary header defaults#674
seun-ja wants to merge 5 commits into
tower-rs:mainfrom
seun-ja:539-CORS-module-excessively-sets-the-Vary-response-header-

Conversation

@seun-ja
Copy link
Copy Markdown
Contributor

@seun-ja seun-ja commented May 9, 2026

Motivation

Fixes #539

Relaxes the default Vary header to an empty list.

Solution

Instead of the custom Default implementation adding some header values, it now simply returns the out-of-the-box Default, which is an empty Vector.

Also, I altered the test to reflect the new change and an additional test.

seun-ja added 2 commits May 9, 2026 11:57
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
@jplatte
Copy link
Copy Markdown
Member

jplatte commented May 9, 2026

This is worse than the current state of things IMO. Has the current default caused some specific issue for you?

@seun-ja
Copy link
Copy Markdown
Contributor Author

seun-ja commented May 9, 2026

No @jplatte.

I haven't had any issue using it in this context.

I'm just fixing because of the issue raised in #539 and seems it was ok to fix as you were ok with PR on it

@jlizen
Copy link
Copy Markdown
Member

jlizen commented May 12, 2026

++ to @jplatte 's feedback that this will break a lot of users since we are shifting from "conservative" defaults (include vary headers unnecessarily), to empty as a default, meaning we force anybody with dynamic CORS to manually set the header or risk caching correctness problems. Definitely a breaking change, probably not a justifiable one.

Would it work to instead have permissive() implicitly set .vary(Vary::list([])) rather than shifting defaults globally?

Or are there other overly conservative cases you are concerned with @seun-ja ?

@jplatte
Copy link
Copy Markdown
Member

jplatte commented May 12, 2026

So, what I was thinking of when writing the referenced comment was having the various methods like allow_origin subtract from the vary list (only if not customized), if and only if the input is one that doesn't result in different header values for different requests. For AllowOrigin this would be any and exact (AllowOriginInner::Const). For AllowHeaders it would be any and list (AllowHeadersInner::Const).

seun-ja added 2 commits May 13, 2026 22:23
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
Copy link
Copy Markdown
Member

@jlizen jlizen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach of eagerly stripping the vary headers based on permissive() creates a problem:

CorsLayer::permissive().allow_origin(AllowOrigin::predicate(...))

This would produce no vary: origin header, when in fact we might have dynamic CORS that has multiple allowed origins. That could result in a stale cache improperly rejecting origins that DO match the predicate.

The proper way to handle this would be, like jplatte@ suggested, doing the stripping on the explicit allow_* methods.

And then, we also would need an extra Is_custom_vary parameter in the builder (or something), to make sure that we don't overwrite the user's explicit configuration.

Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
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 module excessively sets the Vary response header

3 participants