spacewander commented on a change in pull request #3839: URL: https://github.com/apache/apisix/pull/3839#discussion_r596545363
########## File path: docs/en/latest/plugins/cors.md ########## @@ -43,6 +43,7 @@ title: cors | expose_headers | string | optional | "*" | | Which headers are allowed to set in response when access cross-origin resource. Multiple value use `,` to split. | | max_age | integer | optional | 5 | | Maximum number of seconds the results can be cached.. Within this time range, the browser will reuse the last check result. `-1` means no cache. Please note that the maximum value is depended on browser, please refer to [MDN](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age#Directives) for details. | | allow_credential | boolean | optional | false | | Enable request include credential (such as Cookie etc.). According to CORS specification, if you set this option to `true`, you can not use '*' for other options. | +| allow_origins_by_regex | array | optional | nil | | Use regex expressions to match which origins is allowed to enable CORS, for example, [".*\.test.com"] can use to match all subdomain of test.com | Review comment: "which origin is" ########## File path: apisix/plugins/cors.lua ########## @@ -151,16 +176,8 @@ local function set_cors_headers(conf, ctx) end end - -function _M.rewrite(conf, ctx) - if ctx.var.request_method == "OPTIONS" then - return 200 - end -end - - -function _M.header_filter(conf, ctx) - local allow_origins = conf.allow_origins +local function process_with_allow_origins(conf, ctx) + local allow_origins = conf.allow_origins or {} local req_origin = core.request.header(ctx, "Origin") Review comment: Can we pass req_origin as an argument to avoid getting it repeatedly? ########## File path: apisix/plugins/cors.lua ########## @@ -179,8 +196,56 @@ function _M.header_filter(conf, ctx) end end - ctx.cors_allow_origins = allow_origins - set_cors_headers(conf, ctx) + return allow_origins +end + +local function process_with_allow_origins_by_regex(conf, ctx) + local allow_origins_by_regex = conf.allow_origins_by_regex or {} + if next(allow_origins_by_regex) == nil then Review comment: Better to check if the configuration is empty in the schema, like https://github.com/apache/apisix/blob/c3e03bb44a93ffc904812bfcad7eba4f92897fd2/apisix/plugins/ip-restriction.lua#L37 ########## File path: apisix/plugins/cors.lua ########## @@ -179,8 +196,56 @@ function _M.header_filter(conf, ctx) end end - ctx.cors_allow_origins = allow_origins - set_cors_headers(conf, ctx) + return allow_origins +end + +local function process_with_allow_origins_by_regex(conf, ctx) + local allow_origins_by_regex = conf.allow_origins_by_regex or {} Review comment: There is no need to create a temporary `{}` if conf.allow_origins_by_regex is nil. ########## File path: t/plugin/cors.t ########## @@ -757,4 +757,132 @@ GET /t --- response_body eval qr/failed to check the configuration of plugin cors err: you can not/ --- no_error_log + + + +=== TEST 28: set route (regex specified) +--- 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": "http://sub.domain.com,http://sub2.domain.com";, + "allow_methods": "GET,POST", + "allow_headers": "headr1,headr2", + "expose_headers": "ex-headr1,ex-headr2", + "max_age": 50, + "allow_credential": true, + "allow_origins_by_regex":[".*\\.test.com"] + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 29: regex specified +--- request +GET /hello HTTP/1.1 +--- more_headers +Origin: http://a.test.com +resp-vary: Via +--- response_body +hello world +--- response_headers +Access-Control-Allow-Origin: http://a.test.com +Vary: Via, Origin +Access-Control-Allow-Methods: GET,POST +Access-Control-Allow-Headers: headr1,headr2 +Access-Control-Expose-Headers: ex-headr1,ex-headr2 +Access-Control-Max-Age: 50 +Access-Control-Allow-Credentials: true +--- no_error_log +[error] + + + +=== TEST 30: set route (regex specified not match) Review comment: This configuration is a copy of TEST 28? We don't need it. BTW, we need a test with two regexes configured so that we can test the regex concat. ########## File path: apisix/plugins/cors.lua ########## @@ -121,19 +137,28 @@ function _M.check_schema(conf) return false, "you can not set '*' for other option when 'allow_credential' is true" end end + if conf.allow_origins_by_regex then + for i, re_rule in ipairs(conf.allow_origins_by_regex) do + local ok, err = re_compile(re_rule, "j") + if not ok then + return false, err + end + end + end return true end local function set_cors_headers(conf, ctx) local allow_methods = conf.allow_methods + local allow_origins_by_regex = conf.allow_origins_by_regex or {} if allow_methods == "**" then allow_methods = "GET,POST,PUT,DELETE,PATCH,HEAD,OPTIONS,CONNECT,TRACE" end core.response.set_header("Access-Control-Allow-Origin", ctx.cors_allow_origins) - if ctx.cors_allow_origins ~= "*" then + if ctx.cors_allow_origins ~= "*" or next(allow_origins_by_regex) ~= nil then Review comment: If the origin is matched by regex, it should not be `*` as this is only available in `allow_origins`. ---------------------------------------------------------------- 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: [email protected]
