feat: add allow_origins_by_regex to cors plugin by batman-ezio · Pull Request #3839 · apache/apisix

batman-ezio · 2021-03-16T03:05:36Z

What this PR does / why we need it:

Pre-submission checklist:

[ x] Did you explain what problem does this PR solve? Or what new features have been added?
[ x] Have you added corresponding test cases?
[ x] Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

tokers · 2021-03-17T12:18:46Z

@batman-ezio Corresponding test cases are required to verify these changes.

spacewander · 2021-03-18T04:31:51Z

 | 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  |


"which origin is"

spacewander · 2021-03-18T06:46:52Z


    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


If the origin is matched by regex, it should not be * as this is only available in allow_origins.

this function set_cors_headers can call not only matched by regex,.
if it matched by allow_origins = '**' then it will go here and needs to add Vary Origin header

spacewander · 2021-03-18T06:47:21Z

-    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")


Can we pass req_origin as an argument to avoid getting it repeatedly?

spacewander · 2021-03-18T06:52:07Z

+
+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


Better to check if the configuration is empty in the schema, like

apisix/apisix/plugins/ip-restriction.lua

Line 37 in c3e03bb

minItems = 1

ok. thanks.

spacewander · 2021-03-18T06:53:12Z

+end
+
+local function process_with_allow_origins_by_regex(conf, ctx)
+    local allow_origins_by_regex = conf.allow_origins_by_regex or {}


There is no need to create a temporary {} if conf.allow_origins_by_regex is nil.

spacewander · 2021-03-18T07:02:43Z

+
+
+
+=== TEST 30: set route (regex specified not match)


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.

test cases added

spacewander · 2021-03-19T01:00:02Z


    core.response.set_header("Access-Control-Allow-Origin", ctx.cors_allow_origins)
-    if ctx.cors_allow_origins ~= "*" then
+    if ctx.cors_allow_origins ~= "*" or conf.allow_origins_by_regex ~= nil then


I still don't understand why we need or conf.allow_origins_by_regex ~= nil.
If allow_origins_by_regex is given but not matched, it can't go into this function.
If allow_origins_by_regex is given but matched, the cors_allow_origins can't be * as * is just a mark used in allow_origins.

if allow_origins_by_regex is matched, the cors_allow_origins will be the matched domain. not *
for example. it we use [".*.test.com"] and the request is from a.test.com
then the cors_allow_origins will set to a.test.com. not *

conf.allow_origins_by_regex ~= nil means we use regex to match many domains.
the Vary must set as Origin
f a request may contain a Access-Control-Allow-Origin with different values, then the CDN should always respond with Vary: Origin,
check
https://stackoverflow.com/questions/25329405/why-isnt-vary-origin-response-set-on-a-cors-miss

Yes.

if allow_origins_by_regex is matched, the cors_allow_origins will be the matched domain. not *

So ctx.cors_allow_origins ~= "*" will be true.
Therefore there is no need to add an additional condition.

juzhiyuan · 2021-03-22T06:51:59Z

ping @tokers to review.

feat: add allow_origins_by_regex to cors plugin

c648694

spacewander requested changes Mar 16, 2021

View reviewed changes

Comment thread apisix/plugins/cors.lua Outdated

ezioruan added 3 commits March 16, 2021 12:13

feat: add allow_origins_by_regex to cors plugin

7798a28

feat: add allow_origins_by_regex to cors plugin

14cd781

feat: add allow_origins_by_regex to cors plugin

e60a7c7

tokers reviewed Mar 17, 2021

View reviewed changes

Comment thread apisix/plugins/cors.lua Outdated

Comment thread apisix/plugins/cors.lua

Comment thread apisix/plugins/cors.lua Outdated

ezioruan added 4 commits March 17, 2021 20:54

feat: add allow_origins_by_regex to cors plugin

eb54f7c

feat: add allow_origins_by_regex to cors plugin

58b45ed

feat: add allow_origins_by_regex to cors plugin

dc87949

feat: add allow_origins_by_regex to cors plugin

02978ea

batman-ezio requested a review from spacewander March 18, 2021 06:02

spacewander reviewed Mar 18, 2021

View reviewed changes

ezioruan added 2 commits March 19, 2021 08:11

feat: add allow_origins_by_regex to cors plugin

57a8e20

feat: add allow_origins_by_regex to cors plugin

7b408fe

spacewander reviewed Mar 19, 2021

View reviewed changes

feat: add allow_origins_by_regex to cors plugin

95cc60c

spacewander approved these changes Mar 19, 2021

View reviewed changes

batman-ezio requested a review from tokers March 19, 2021 07:47

tokers approved these changes Mar 22, 2021

View reviewed changes

spacewander merged commit bb95f7a into apache:master Mar 22, 2021

Uh oh!

Conversation

batman-ezio commented Mar 16, 2021 • edited by spacewander Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tokers commented Mar 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juzhiyuan commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

batman-ezio commented Mar 16, 2021 •

edited by spacewander

Loading