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

fix: Non wildcard origin in CORS should sent Vary header #9010

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

boekkooi-lengoo
Copy link
Contributor

Description

When CORS requirements are more complicated than setting Access-Control-Allow-Origin to * then we set the Vary to Origin. This avoids caching the wrong response.

Last week I was debugging an issue where requests where failing with CORS errors.
After some digging it became clear that when the first request to the endpoint did not match the allowed origins the request was cached and this cached resolve was then used later on for Origins that should be allowed. This turned out to be because the Vary header did not contain Origin.

Secondly I also resolved an issue where I was unable to use only allow_origins_by_regex in standalone mode by allowing allow_origins to be an empty string.
I'm not sure what metadata_schema is used for so this maybe wrong.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)
@boekkooi-lengoo boekkooi-lengoo changed the title Ensure Vary header is set when using CORS with non wildcard origin Mar 6, 2023
@boekkooi-lengoo boekkooi-lengoo changed the title Fix - Non wildcard origin in CORS should sent Vary header Mar 6, 2023
@pottekkat
Copy link
Contributor

@boekkooi-lengoo Thanks for opening the PR. Tagging @spacewander for review.

apisix/plugins/cors.lua Outdated Show resolved Hide resolved
apisix/plugins/cors.lua Outdated Show resolved Hide resolved
When CORS requirements are more complicated than setting `Access-Control-Allow-Origin` to `*` then we set the `Vary` to `Origin`.
This avoids caching the wrong response.
@boekkooi-lengoo
Copy link
Contributor Author

@spacewander I have update the PR to only include the Vary header fix now.

@spacewander spacewander merged commit e41cf45 into apache:master Mar 16, 2023
hongbinhsu added a commit to fitphp/apix that referenced this pull request Mar 17, 2023
* upstream/master: (46 commits)
  fix(consumer): work if the etcd connection failed during starting (apache#9077)
  ci: fix low disk space error when loading saved docker images (apache#9080)
  change: change the default router from radixtree uri to radixtree hos… (apache#9047)
  chore(deps): bump dubbo from 2.7.18 to 2.7.21 in /t/lib/dubbo-backend/dubbo-backend-provider (apache#9041)
  fix: cli test on master (apache#9075)
  fix: Non wildcard origin in CORS should sent Vary header (apache#9010)
  feat: bump lua-resty-ldap version for ldap-auth (apache#9037)
  fix: invalidate cache in core.request.add_haeder and fix some calls (apache#8824)
  docs: remove unnecessary getting-started.md (apache#9054)
  docs: contribute Getting Started tutorials (apache#9046)
  docs: replace full-width quotation mark with half-width quotation mark (apache#8887)
  docs: fix typo and grammar (apache#9008)
  feat: ready to release 2.15.3 (apache#9021)
  ci: ensure the test can run with different repo name (apache#8832)
  chore(deps): bump golang.org/x/net from 0.0.0-20220722155237-a158d28d115b to 0.7.0 in /ci/pod/openfunction/function-example/test-uri (apache#9018)
  docs: improved SEO & fixed typo and localization issues (apache#8993)
  feat: release APISIX 3.2.0 (apache#8988)
  docs: fix grammar (apache#8935)
  docs: fix indent in encrypted-storage-fields (apache#8876)
  docs: k8s discovery: state the restriction on the use of port number (apache#8969)
  ...
AlinsRan pushed a commit to AlinsRan/apisix that referenced this pull request Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants