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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
