You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/09/15 03:35:29 UTC
[GitHub] [apisix] bzp2010 opened a new pull request, #7923: fix(cors): allow origins and regex conflict
bzp2010 opened a new pull request, #7923:
URL: https://github.com/apache/apisix/pull/7923
### Description
The default value of the current `allow_origins` configuration conflicts with the value of `allow_origins_by_regex`, causing the value entered by the user to be ignored.
This PR will proactively remove the default value of `allow_origins` when the `allow_origins_by_regex` configuration exists. A new test case has been added and no changes have been made to the old one, which is now fully compatible.
### 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] github-actions[bot] closed pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #7923: fix(cors): allow origins and regex conflict
URL: https://github.com/apache/apisix/pull/7923
--
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] tokers commented on a diff in pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #7923:
URL: https://github.com/apache/apisix/pull/7923#discussion_r971753879
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
@bzp2010 Then we should also show the relationship between these two fields in the doc.
--
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] bzp2010 commented on a diff in pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7923:
URL: https://github.com/apache/apisix/pull/7923#discussion_r971721464
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
@spacewander
In my opinion, `allow_origins_by_regex` is a more advanced form of `allow_origins`. Once the user decides to use `allow_origins_by_regex`, then we should not use `allow_origins` at same time. With regular expressions, the user can implement whatever he wants to implement with `allow_origins`.
So my implementation is as you can see, I fully believe that we should not mix these two patterns, it will bring extra mental cost to the user, the user needs to understand in particular, how the execution order of the text pattern and the regular pattern is.
Even if possible, I would like to add mandatory checks directly via jsonschema to make sure users don't set them at the same time. What do you think?
--
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 #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7923:
URL: https://github.com/apache/apisix/pull/7923#discussion_r971771627
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
> Once the user decides to use allow_origins_by_regex, then we should not use allow_origins at same time.
This will be a break change. Note that we are going to backport this fix. Maybe we can discuss this issue in the mail list.
--
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] bzp2010 commented on a diff in pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7923:
URL: https://github.com/apache/apisix/pull/7923#discussion_r971721464
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
@spacewander
In my opinion, `allow_origins_by_regex` is a more advanced form of allow_origins. Once the user decides to use `allow_origins_by_regex`, then we should not use `allow_origins` at same time. With regular expressions, the user can implement whatever he wants to implement with allow_origins.
So my implementation is as you can see, I fully believe that we should not mix these two patterns, it will bring extra mental cost to the user, the user needs to understand in particular, how the execution order of the text pattern and the regular pattern is.
Even if possible, I would like to add mandatory checks directly via jsonschema to make sure users don't set them at the same time. What do you think?
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
@spacewander
In my opinion, `allow_origins_by_regex` is a more advanced form of `allow_origins`. Once the user decides to use `allow_origins_by_regex`, then we should not use `allow_origins` at same time. With regular expressions, the user can implement whatever he wants to implement with allow_origins.
So my implementation is as you can see, I fully believe that we should not mix these two patterns, it will bring extra mental cost to the user, the user needs to understand in particular, how the execution order of the text pattern and the regular pattern is.
Even if possible, I would like to add mandatory checks directly via jsonschema to make sure users don't set them at the same time. What do you think?
--
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 #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7923:
URL: https://github.com/apache/apisix/pull/7923#discussion_r971585055
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
Maybe we should only clear when the value is `*`?
Otherwise, conf like `allow_origins = "apple', allow_origins_by_regex = "bana*"` will not work.
--
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] Qiuqiu0505 commented on pull request #7923: fix(cors): allow origins and regex conflict
Posted by "Qiuqiu0505 (via GitHub)" <gi...@apache.org>.
Qiuqiu0505 commented on PR #7923:
URL: https://github.com/apache/apisix/pull/7923#issuecomment-1430829873
Hi，I also met the same preblem and are there any new conclusions now? Thanks
--
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] github-actions[bot] commented on pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7923:
URL: https://github.com/apache/apisix/pull/7923#issuecomment-1375366692
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] bzp2010 commented on a diff in pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7923:
URL: https://github.com/apache/apisix/pull/7923#discussion_r971995733
##########
apisix/plugins/cors.lua:
##########
@@ -164,13 +164,11 @@ function _M.check_schema(conf, schema_type)
if not ok then
return false, err
end
- if conf.allow_credential then
- if conf.allow_origins == "*" or conf.allow_methods == "*" or
- conf.allow_headers == "*" or conf.expose_headers == "*" then
- return false, "you can not set '*' for other option when 'allow_credential' is true"
- end
- end
+
+ -- When allow_origins_by_regex is present we need to clear the default value of allow_origins
+ -- first to avoid errors in the check in allow_credential below.
if conf.allow_origins_by_regex then
+ conf.allow_origins = nil
Review Comment:
OK, I will start a discussion in the mail list.
--
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] github-actions[bot] commented on pull request #7923: fix(cors): allow origins and regex conflict
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7923:
URL: https://github.com/apache/apisix/pull/7923#issuecomment-1345510133
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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