Always set Vary: Origin when reflecting the request Origin - #267
Open
arpitjain099 wants to merge 1 commit into
Open
Always set Vary: Origin when reflecting the request Origin#267arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
When the CORS middleware echoes the caller's Origin back into Access-Control-Allow-Origin (with a single configured origin, or an AllowedOriginValidator, which is the usual credentialed multi-origin setup), the response depends on the request Origin. But Vary: Origin was only emitted when more than one static origin was configured, so most reflected responses told shared caches they do not vary by Origin. A cache in front of the app can then serve one origin's credentialed CORS response to a request from another origin. Set Vary: Origin whenever a non-wildcard Origin is reflected, matching rs/cors and go-chi/cors. The wildcard "*" answer is identical for every origin and still gets no Vary: Origin, so the case reverted in gorilla#122 is unaffected. Preflight responses also derive Allow-Methods/Allow-Headers from the request's Access-Control-Request-* headers, so those are added to Vary as well. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the CORS middleware reflects the caller's
Originback intoAccess-Control-Allow-Origin, the response depends on that origin, so a shared cache in front of the app needsVary: Originto key on it. Right now that header is only set when more than one static origin is configured (len(ch.allowedOrigins) > 1). The common credentialed setups do not hit that branch:AllowedOrigins([]string{"https://app.example.com"})AllowedOriginValidator(the usual way to allow a set of dynamic/subdomain origins with credentials)In both cases the middleware echoes the request
Origin(often alongsideAccess-Control-Allow-Credentials: true) but sends noVary. A cache sitting in front of the app can then store one origin's response and hand it to a request from a different origin, which is the classic CORS cache-poisoning shape.The fix sets
Vary: Originwhenever a non-wildcard origin is reflected. rs/cors and go-chi/cors both do this on every reflected response. The wildcard*answer is identical for every origin, so it still gets noVary: Origin; that keeps the behavior that #114 tried to change and #122 deliberately reverted. Preflight responses also deriveAccess-Control-Allow-Methods/Access-Control-Allow-Headersfrom the request'sAccess-Control-Request-*headers, so those are added toVaryon the OPTIONS path too.Tests: added cases asserting
Vary: Originis present for the single-origin and validator (credentialed) configs and that preflight varies on the request method/headers, plus a case asserting the wildcard answer still does not addVary: Origin. The existing multi-origin and star tests still pass.go test ./...is green; without the fix the three new reflected-origin tests fail becauseVaryis empty.