You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "boekkooi-lengoo (via GitHub)" <gi...@apache.org> on 2023/03/06 07:39:40 UTC
[GitHub] [apisix] boekkooi-lengoo opened a new pull request, #9010: Ensure Vary header is set when using CORS with non wildcard origin
boekkooi-lengoo opened a new pull request, #9010:
URL: https://github.com/apache/apisix/pull/9010
### 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
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1128845436
##########
apisix/plugins/cors.lua:
##########
@@ -37,7 +38,7 @@ local metadata_schema = {
type = "object",
additionalProperties = {
type = "string",
- pattern = origins_pattern
+ pattern = origins_metadata_pattern
Review Comment:
an empty string is not a valid origin. We can keep the same regex in metadata_schema and the others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] boekkooi-lengoo commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "boekkooi-lengoo (via GitHub)" <gi...@apache.org>.
boekkooi-lengoo commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1127524333
##########
apisix/plugins/cors.lua:
##########
@@ -37,7 +38,7 @@ local metadata_schema = {
type = "object",
additionalProperties = {
type = "string",
- pattern = origins_pattern
+ pattern = origins_metadata_pattern
Review Comment:
Because else the tests for the `metadata_schema` started failing and I don't understand what `metadata_schema` is used for so I wanted to save and not sorry.
However if you are fine saying that an empty string is a valid origin we can avoid this and I can fix the text.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] navendu-pottekkat commented on pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "navendu-pottekkat (via GitHub)" <gi...@apache.org>.
navendu-pottekkat commented on PR #9010:
URL: https://github.com/apache/apisix/pull/9010#issuecomment-1455968943
@boekkooi-lengoo Thanks for opening the PR. Tagging @spacewander for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] boekkooi-lengoo commented on pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "boekkooi-lengoo (via GitHub)" <gi...@apache.org>.
boekkooi-lengoo commented on PR #9010:
URL: https://github.com/apache/apisix/pull/9010#issuecomment-1463534185
@spacewander I have update the PR to only include the Vary header fix now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] boekkooi-lengoo commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "boekkooi-lengoo (via GitHub)" <gi...@apache.org>.
boekkooi-lengoo commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1127525273
##########
apisix/plugins/cors.lua:
##########
@@ -178,6 +179,9 @@ function _M.check_schema(conf, schema_type)
end
end
end
+ if conf.allow_origins == "" and (not conf.allow_origins_by_regex or next(conf.allow_origins_by_regex) == nil) then
Review Comment:
Thanks @spacewander I knew there should be a better way but my lua foo is not very good :slightly_smiling_face:
I will update this once we are clear on the `metadata_schema` item.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1127313637
##########
apisix/plugins/cors.lua:
##########
@@ -178,6 +179,9 @@ function _M.check_schema(conf, schema_type)
end
end
end
+ if conf.allow_origins == "" and (not conf.allow_origins_by_regex or next(conf.allow_origins_by_regex) == nil) then
Review Comment:
As allow_origins_by_regex is an array, we can use `#` to check if it's empty
##########
apisix/plugins/cors.lua:
##########
@@ -37,7 +38,7 @@ local metadata_schema = {
type = "object",
additionalProperties = {
type = "string",
- pattern = origins_pattern
+ pattern = origins_metadata_pattern
Review Comment:
Why change this pattern?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1131867572
##########
apisix/plugins/cors.lua:
##########
@@ -37,7 +38,7 @@ local metadata_schema = {
type = "object",
additionalProperties = {
type = "string",
- pattern = origins_pattern
+ pattern = origins_metadata_pattern
Review Comment:
I notice there is a fix recently: https://github.com/apache/apisix/pull/9028
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] boekkooi-lengoo commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "boekkooi-lengoo (via GitHub)" <gi...@apache.org>.
boekkooi-lengoo commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1129073158
##########
apisix/plugins/cors.lua:
##########
@@ -37,7 +38,7 @@ local metadata_schema = {
type = "object",
additionalProperties = {
type = "string",
- pattern = origins_pattern
+ pattern = origins_metadata_pattern
Review Comment:
I think this is incorrect as the allow_origins can be empty or unspecified if a regex is allow_origins_by_regex is specified.
However I didn't find any way to make `allow_origins` optional as it has a default value of `*`. Other then allowing an empty string.
Do you have any suggestions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] boekkooi-lengoo commented on a diff in pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "boekkooi-lengoo (via GitHub)" <gi...@apache.org>.
boekkooi-lengoo commented on code in PR #9010:
URL: https://github.com/apache/apisix/pull/9010#discussion_r1129073158
##########
apisix/plugins/cors.lua:
##########
@@ -37,7 +38,7 @@ local metadata_schema = {
type = "object",
additionalProperties = {
type = "string",
- pattern = origins_pattern
+ pattern = origins_metadata_pattern
Review Comment:
I think this is incorrect as the allow_origins can be empty or unspecified if a regex is allow_origins_by_regex is specified.
However I didn't find any way to make `allow_origins` optional as it has a default value of `*`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #9010: fix: Non wildcard origin in CORS should sent Vary header
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander merged PR #9010:
URL: https://github.com/apache/apisix/pull/9010
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org