fix(cors): consider using `allow_origins_by_regex` only when it is not `nil` by chenjinfeng0505 · Pull Request #9028 · apache/apisix

chenjinfeng0505 · 2023-03-07T12:02:00Z

Description

Fixes #9027
-- If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only.

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

…rigins_by_regex only

qihaiyan · 2023-03-07T15:39:30Z

-    if not match_origins(req_origin, allow_origins) then
-        allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+    allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+    if not match_origins(req_origin, allow_origins) and conf.allow_origins_by_regex == nil then


With cors plugin's default config, when a request doesn't have a origin header, the match_origins method will return true, then the following process_with_allow_origins method won't be executed.

spacewander · 2023-03-10T03:14:01Z

+
+
+
+=== TEST 36: set route ( regex specified and allow_origins is default value )


Let's add new tests to the other test file.

When the test file is too large, for example > 800 lines, you should split it to a new file. Please take a look at t/plugin/limit-conn.t and t/plugin/limit-conn2.t.

https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#check-code-style-and-test-case-style

spacewander · 2023-03-10T03:14:39Z

 function _M.header_filter(conf, ctx)
    local req_origin =  ctx.original_request_origin
-    -- Try allow_origins first, if mismatched, try allow_origins_by_regex.
+    -- If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only


Let's update docs for the new try order.

Let's update docs for the new try order.

Thank you for reminding. I have modified it.

add final newline

qihaiyan · 2023-03-14T01:12:36Z

Is this PR ready to merge? @spacewander

spacewander · 2023-03-14T03:11:46Z

-    allow_origins = process_with_allow_origins(conf.allow_origins, ctx, req_origin)
-    if not match_origins(req_origin, allow_origins) then
-        allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)
+    allow_origins = process_with_allow_origins_by_regex(conf, ctx, req_origin)


Look like we can skip this function call by checking the conf.allow_origins_by_regex == nil first. For example,

if conf.allow_origins_by_regex ~= nil then

This function named process_with_allow_origins_by_regex has checked whether allow_origins_by_regex is nil.
Does it need to be checked again before the method invocation?

Yes. We can move it out, and avoid the repeated and conf.allow_origins_by_regex == nil check below.

Thanks and I has changed it.

spacewander · 2023-03-17T02:42:50Z

Let's merge master to make CI pass.

spacewander · 2023-03-20T02:17:23Z

https://github.com/apache/apisix/actions/runs/4446285829/jobs/7839374950?pr=9028

#   Failed test 't/plugin/cors2.t TEST 5: regex specified match - header Vary ok'
#   at /apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1011.
#          got: 'Via'
#     expected: 'Via, Origin'
# Looks like you failed 1 test of 35.
[02:10:05] t/plugin/cors2.t ..

Please make the CI pass, thanks!

update TEST 5

chenjinfeng0505 · 2023-03-20T03:03:34Z

@spacewander I have just updated it. Thanks

qihaiyan · 2023-03-24T01:50:33Z

Merging needs another approving of reviewer with write access. @spacewander

KID-G · 2023-03-27T08:43:47Z

Hi, @Qiuqiu0505
Thank you for your contribution to Apache APISIX!
Can you tell me your email address? We have prepared electronic certificates for new contributors.

chenjinfeng0505 · 2023-03-27T09:00:50Z

@KID-G Hi, my email address is [email protected].
I am so lucky to receive certificate. Thanks.

* upstream/master: (25 commits) fix: upgrade lua-resty-ldap to 0.2.2 (apache#9254) feat(cli): support bypassing Admin API Auth by configuration (apache#9147) fix(ci): write version into xds first (apache#9274) fix: skip warning log when apisix.data_encryption.enable is false (apache#9057) docs: add-api7-information (apache#9260) docs: Fixed typo (apache#9244) docs: clarify what is client.ca in client-to-apisix-mtls.md (apache#9221) docs: Corrected typos and grammatical errors (apache#9216) docs: updated ssl sni parameter requirement in admin-api.md (apache#9176) fix: check upstream reference in traffic-split plugin when delete upstream (apache#9044) docs: Update proxy-rewrite headers.add docs (apache#9220) feat: suppot header injection for fault-injection plugin (apache#9039) fix: upgrade lua-resty-etcd to 1.10.4 (apache#9235) docs: fix incorrect semantic.yml link (apache#9231) feat: Upstream status report (apache#9151) fix: host_hdr should not be false (apache#9150) docs: remove APISIX base instruction (apache#9117) fix(cli): prevent non-`127.0.0.0/24` to access admin api with empty admin_key (apache#9146) docs: fix 404 link (apache#9160) fix(cors): consider using `allow_origins_by_regex` only when it is not `nil` (apache#9028) ...

…t `nil` (apache#9028)

chenjinfeng0505 added 5 commits March 7, 2023 18:48

If allow_origins_by_regex is not nil, should be considered to allow_o…

b77b53e

…rigins_by_regex only

If allow_origins_by_regex is not nil, should be considered to allow_o…

b207498

…rigins_by_regex only

Merge branch 'master' of github.com:Qiuqiu0505/apisix

33a17ca

Merge branch 'master' of github.com:Qiuqiu0505/apisix

adec375

Merge branch 'master' of github.com:Qiuqiu0505/apisix

453c3fa

qihaiyan previously approved these changes Mar 9, 2023

View reviewed changes

spacewander mentioned this pull request Mar 10, 2023

fix: Non wildcard origin in CORS should sent Vary header #9010

Merged

5 tasks

spacewander reviewed Mar 10, 2023

View reviewed changes

Update cors.t

66674f4

chenjinfeng0505 dismissed qihaiyan’s stale review via 66674f4 March 10, 2023 03:36

chenjinfeng0505 and others added 4 commits March 10, 2023 14:07

Update cors1.t and cors.md

57a613d

Merge branch 'master' of github.com:Qiuqiu0505/apisix

cbccc93

Update cors2.t

94d956b

add final newline

Update cors.lua

27fc87e

spacewander reviewed Mar 14, 2023

View reviewed changes

spacewander added the need backport label Mar 14, 2023

chenjinfeng0505 added 3 commits March 16, 2023 11:11

Update cors.lua

b720d52

Update cors.lua

d503d2d

Update cors.lua

87b212c

chenjinfeng0505 added 2 commits March 17, 2023 10:45

Merge branch 'apache:master' into master

9a2b284

Merge branch 'apache:master' into master

202313b

chenjinfeng0505 added 2 commits March 20, 2023 10:18

Merge branch 'apache:master' into master

75eb8e3

Update cors2.t

e31705f

update TEST 5

chenjinfeng0505 closed this Mar 20, 2023

chenjinfeng0505 reopened this Mar 20, 2023

update cors2.t

68901f5

spacewander approved these changes Mar 21, 2023

View reviewed changes

Merge branch 'apache:master' into master

21fbfb1

qihaiyan approved these changes Mar 21, 2023

View reviewed changes

spacewander approved these changes Mar 22, 2023

View reviewed changes

spacewander requested review from leslie-tsang and soulbird March 24, 2023 03:40

soulbird approved these changes Mar 24, 2023

View reviewed changes

leslie-tsang approved these changes Mar 24, 2023

View reviewed changes

leslie-tsang merged commit 6c9e34a into apache:master Mar 24, 2023

leslie-tsang changed the title ~~fix: If allow_origins_by_regex is not nil, should be considered to allow_origins_by_regex only~~ fix(cors): consider using allow_origins_by_regex only when it is not nil Mar 24, 2023

AlinsRan pushed a commit to AlinsRan/apisix that referenced this pull request Jun 25, 2023

fix(cors): consider using allow_origins_by_regex only when it is no…

e8c1875

…t `nil` (apache#9028)




		=== TEST 36: set route ( regex specified and allow_origins is default value )

Uh oh!

Conversation

chenjinfeng0505 commented Mar 7, 2023 • edited by spacewander Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

qihaiyan Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

spacewander Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

chenjinfeng0505 Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

qihaiyan commented Mar 14, 2023

Uh oh!

spacewander Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

chenjinfeng0505 Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

spacewander Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

chenjinfeng0505 Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

spacewander commented Mar 17, 2023

Uh oh!

spacewander commented Mar 20, 2023

Uh oh!

chenjinfeng0505 commented Mar 20, 2023

Uh oh!

qihaiyan commented Mar 24, 2023

Uh oh!

KID-G commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenjinfeng0505 commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chenjinfeng0505 commented Mar 7, 2023 •

edited by spacewander

Loading

spacewander Mar 10, 2023 •

edited

Loading

KID-G commented Mar 27, 2023 •

edited

Loading