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 2020/07/31 13:02:45 UTC
[GitHub] [incubator-apisix] ShiningRush opened a new pull request #1963: feat: add option for cors
ShiningRush opened a new pull request #1963:
URL: https://github.com/apache/incubator-apisix/pull/1963
fixed with #1915 #1704
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1963: feat: add option for cors
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1963:
URL: https://github.com/apache/incubator-apisix/pull/1963#discussion_r463683726
##########
File path: t/plugin/cors.t
##########
@@ -491,3 +491,134 @@ Access-Control-Max-Age: 5
Access-Control-Allow-Credentials:
--- no_error_log
[error]
+
+
+Access-Control-Expose-Headers: e-headers
+--- no_error_log
+[error]
+
+
+
+=== TEST 17: set route(overwrite upstream)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "cors": {
+ "allow_origins": "**",
+ "allow_methods": "**",
+ "allow_headers": "*",
+ "expose_headers": "*",
+ "allow_credential": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/headers"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 18: overwrite upstream
+--- request
+GET /headers?Access-Control-Allow-Origin=https://sub.domain.com HTTP/1.1
+--- more_headers
+Origin: https://sub.domain.com
+--- response_body
+/headers
+--- response_headers
+Access-Control-Allow-Origin: https://sub.domain.com
+--- no_error_log
+[error]
+
+
+
+=== TEST 19: overwrite upstream(Access-Control-Allow-Methods)
+--- request
+GET /headers?Access-Control-Allow-Methods=methods HTTP/1.1
+--- more_headers
+Origin: https://sub.domain.com
+--- response_body
+/headers
+--- response_headers
+Access-Control-Allow-Methods: GET,POST,PUT,DELETE,PATCH,HEAD,OPTIONS,CONNECT,TRACE
+--- no_error_log
+[error]
+
+
+
+=== TEST 20: overwrite upstream(Access-Control-Allow-Headers)
+--- request
+GET /headers?Access-Control-Allow-Headers=a-headers HTTP/1.1
+--- more_headers
+Origin: https://sub.domain.com
+--- response_body
+/headers
+--- response_headers
+Access-Control-Allow-Headers: *
+--- no_error_log
+[error]
+
+
+
+=== TEST 21: overwrite upstream(Access-Control-Expose-Headers)
+--- request
+GET /headers?Access-Control-Expose-Headers=e-headers HTTP/1.1
+--- more_headers
+Origin: https://sub.domain.com
+--- response_body
+/headers
+--- response_headers
+Access-Control-Expose-Headers: *
+--- no_error_log
+[error]
+
+
+
+=== TEST 22: overwrite upstream(Access-Control-Max-Age)
+--- request
+GET /headers?Access-Control-Max-Age=10 HTTP/1.1
+--- more_headers
+Origin: https://sub.domain.com
+--- response_body
+/headers
+--- response_headers
+Access-Control-Max-Age: 5
+--- no_error_log
+[error]
+
+
+
+=== TEST 23: not overwrite upstream(Access-Control-Allow-Credentials)
+--- request
+GET /headers?Access-Control-Allow-Credentials=false HTTP/1.1
+--- more_headers
+Origin: https://sub.domain.com
+--- response_body
+/headers
+--- response_headers
+Access-Control-Allow-Credentials: true
+--- no_error_log
+[error]
Review comment:
need one blank line at the end of file
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-apisix] membphis commented on a change in pull request #1963: feat: add option for cors
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1963:
URL: https://github.com/apache/incubator-apisix/pull/1963#discussion_r463681866
##########
File path: apisix/plugins/cors.lua
##########
@@ -146,10 +146,16 @@ function _M.rewrite(conf, ctx)
ctx.cors_allow_origins = allow_origins
set_cors_headers(conf, ctx)
+end
+function _M.rewrite(conf, ctx)
if ctx.var.request_method == "OPTIONS" then
return 200
end
end
+function _M.header_filter(conf, ctx)
Review comment:
two blank lines between different functions.
##########
File path: apisix/plugins/cors.lua
##########
@@ -114,17 +114,17 @@ local function set_cors_headers(conf, ctx)
allow_methods = "GET,POST,PUT,DELETE,PATCH,HEAD,OPTIONS,CONNECT,TRACE"
end
- ngx.header["Access-Control-Allow-Origin"] = ctx.cors_allow_origins
- ngx.header["Access-Control-Allow-Methods"] = allow_methods
- ngx.header["Access-Control-Allow-Headers"] = conf.allow_headers
- ngx.header["Access-Control-Max-Age"] = conf.max_age
+ core.response.set_header("Access-Control-Allow-Origin", ctx.cors_allow_origins)
+ core.response.set_header("Access-Control-Allow-Methods", allow_methods)
+ core.response.set_header("Access-Control-Allow-Headers", conf.allow_headers)
+ core.response.set_header("Access-Control-Max-Age", conf.max_age)
+ core.response.set_header("Access-Control-Expose-Headers", conf.expose_headers)
if conf.allow_credential then
- ngx.header["Access-Control-Allow-Credentials"] = true
+ core.response.set_header("Access-Control-Allow-Credentials", true)
end
- ngx.header["Access-Control-Expose-Headers"] = conf.expose_headers
end
-function _M.rewrite(conf, ctx)
+local function set_cors(conf, ctx)
Review comment:
https://github.com/apache/incubator-apisix/blob/8d2e758e1e60413f57338c7c54e5e5c405f229a5/apisix/plugins/cors.lua#L128-L148
we can copy them into `header_filter` and remove function `set_cors`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org