fix(cors): avoid overwriting `Access-Control-Expose-Headers` response header by VanLiuZhi · Pull Request #11136 · apache/apisix

VanLiuZhi · 2024-04-09T12:55:40Z

1. Problem Description

I used version APISIX 3.5 before and found that the code for the expose_headers setting in the core plugin is incorrect, which has not been addressed in the latest branch. This PR is intended to rectify this issue.

The original relevant code is as follows:

expose_headers = {
    description =
        "you can use '*' to expose all headers when no credentials," ..
        "'**' to allow forcefully (it will bring some security risks, be careful)," ..
        "multiple headers use ',' to split. default: *.",
    type = "string",
    default = "*"
},

core.response.set_header("Access-Control-Expose-Headers", conf.expose_headers)

This code describes the expose_headers configuration, suggesting that setting it to ** would expose all headers, but this is incorrect. The HTTP protocol does not have such a specification. You can refer to the description on MDN:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Expose-Headers

* (wildcard)
The value "*" only counts as a special wildcard value for requests without credentials
 (requests without HTTP cookies or HTTP authentication information). 
In requests with credentials, it is treated as the literal header name "*".

So ** is the wrong description and configuration, and setting it to * is also conditional.

2. Impact

If I enable this plugin globally and set allow_methods, allow_origins, and other configurations, not setting expose_headers using the default value * or setting ** will cause problems because the plugin's request header will override the Access-Control-Expose-Headers set by the upstream service, causing the upstream service to be unable to expose request headers.

I can only hard-code the:

expose_headers: Content-Encoding, My-Revision, My-Ikun

This is unreasonable. If my upstream service needs to expose a new header Project-App, I have to configure it, and let the plugin configure it as:

expose_headers: Content-Encoding, My-Revision, My-Ikun, Project-App

3. Recommended Solution

In most cases, expose_headers does not need to be set globally and can be managed by the upstream service.

I have used Kong before, and the strategy of the Kong core plugin is to remove Access-Control-Expose-Headers from the response header if the user does not configure it.

I suggest adopting a similar approach where the core plugin does not add Access-Control-Expose-Headers to the response header if the user does not configure it. This allows the upstream service to control the exposure of headers independently.

So I made changes to the code:

expose_headers = {
    description =
            "multiple header use ',' to split." ..
            "If not specified, no custom headers are exposed.",
    type = "string"
},

if conf.expose_headers ~= nil and conf.expose_headers ~= "" then
    core.response.set_header("Access-Control-Expose-Headers", conf.expose_headers)
end

The plugin will modify the Access-Control-Expose-Headers header and override the upstream service's configuration only when a value is explicitly set by the user.
If no value is set, the plugin will not modify the header, allowing the upstream service to expose its own headers as needed.

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)

shreemaan-abhishek · 2024-04-11T05:20:01Z

can this whole problem be avoided if configuring the value as ** is avoided?

shreemaan-abhishek · 2024-04-11T05:20:23Z

you will also have to support this fix with test cases.

…atch)

VanLiuZhi · 2024-04-12T06:54:31Z

can this whole problem be avoided if configuring the value as ** is avoided?

Cannot set **. In fact, most of the core-related configurations are not supported to be set ** in the HTTP protocol. The setting of ** is handled by the APISIX code. I guess the expose_headers part was not paid attention to during development.

Our requirement is for the upstream service to customize the Access-Control-Expose-Headers, which can be read by the browser or client. To meet this requirement, the core plugin can only allow all Access-Control-Expose-Headers.

However, setting Access-Control-Expose-Headers: ** is invalid in the HTTP protocol. Setting Access-Control-Expose-Headers: * only works when credentials are not passed, so removing the response header is the best practice.

VanLiuZhi · 2024-04-12T06:57:27Z

you will also have to support this fix with test cases.

I submitted a test case to verify whether the removal of expose_headers is effective when no value is set.

…aders match)

shreemaan-abhishek · 2024-04-12T13:25:20Z

@ShiningRush was it intentional (by design) to allow setting ** for allow_origins?

ShiningRush · 2024-04-12T14:23:53Z

@ShiningRush was it intentional (by design) to allow setting ** for allow_origins?

Yes, ** is designed for allow_origins and allow_headers first. But I don't know why the expose_header also supports this now.
I think ** is equal to empty string when it handles expose_headers, so @VanLiuZhi 's PR is LGTM.

VanLiuZhi · 2024-04-13T14:17:44Z

@ShiningRush Yes, ** is designed for allow_origins and allow_headers first. But I don't know why the expose_header also supports this now. I think ** is equal to empty string when it handles expose_headers, so @VanLiuZhi 's PR is LGTM.

Thank you! I have noticed that since the default values were removed, many of the previous test cases have failed. Do I need to modify these test cases?

shreemaan-abhishek · 2024-04-15T15:26:03Z

okay, makes sense now. Sorry for the hassle, I am not much familiar with cors 😂 . But your PR does two things:

removes comment about ** (but infact, user can still configure ** as the value for expose_headers but it they do it will be their problem
removes * as the default value.

so now the default behaviour is that if this field is not configured, Access-Control-Expose-Headers will remain unaffected.

Please consider this and update the PR title and add more details in the comment/docs about the changing behaviour.

…-plugins-cors

VanLiuZhi · 2024-04-17T09:33:41Z

okay, makes sense now. Sorry for the hassle, I am not much familiar with cors 😂 . But your PR does two things:

removes comment about ** (but infact, user can still configure ** as the value for expose_headers but it they do it will be their problem

removes * as the default value.

so now the default behaviour is that if this field is not configured, Access-Control-Expose-Headers will remain unaffected.

Please consider this and update the PR title and add more details in the comment/docs about the changing behaviour.

@shreemaan-abhishek 👌Thank you for your correction. I have updated the PR description and title. The documentation related to CORS has been revised.

fix(cors): expose_headers cannot be set to **

aabaffc

l26880 added 2 commits April 12, 2024 13:49

fix(cors): expose_headers description update, add test case

137549f

fix(cors): update test cases(TEST 29: Access-Control-Expose-Headers m…

3f088f6

…atch)

fix(cors): update test cases(TEST 27: remove Access-Control-Expose-He…

24fa509

…aders match)

fix(cors): update test cases(cors.t cors3.t)

931b1fa

VanLiuZhi changed the title fix(cors): expose_headers cannot be set to ** fix(cors): when expose_headers is set to **, allowing any header becomes ineffective, which will prevent the upstream service from exposing its own headers Apr 17, 2024

l26880 added 2 commits April 17, 2024 15:47

fix(cors): update cors docs

6b7c60e

Merge remote-tracking branch 'origin/master-plugins-cors' into master…

21b6eb2

…-plugins-cors

shreemaan-abhishek changed the title ~~fix(cors): when expose_headers is set to **, allowing any header becomes ineffective, which will prevent the upstream service from exposing its own headers~~ fix(cors): avoid overwriting Access-Control-Expose-Headers response header Apr 18, 2024

shreemaan-abhishek reviewed Apr 18, 2024

View reviewed changes

Comment thread docs/en/latest/plugins/cors.md

Comment thread docs/zh/latest/plugins/cors.md

fix(cors): update cors docs, remove extra whitespaces

bc6bd17

shreemaan-abhishek approved these changes Apr 18, 2024

View reviewed changes

shreemaan-abhishek added the approved label Apr 22, 2024

Revolyssup approved these changes Apr 23, 2024

View reviewed changes

shreemaan-abhishek merged commit 53661ea into apache:master Apr 23, 2024

membphis mentioned this pull request May 12, 2026

docs: CHANGELOG audit findings for v3.x — completeness, wording, and process gaps #13359

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cors): avoid overwriting `Access-Control-Expose-Headers` response header#11136

fix(cors): avoid overwriting `Access-Control-Expose-Headers` response header#11136
shreemaan-abhishek merged 8 commits into
apache:masterfrom
VanLiuZhi:master-plugins-cors

VanLiuZhi commented Apr 9, 2024 •

edited

Loading

Uh oh!

shreemaan-abhishek commented Apr 11, 2024

Uh oh!

shreemaan-abhishek commented Apr 11, 2024

Uh oh!

VanLiuZhi commented Apr 12, 2024 •

edited

Loading

Uh oh!

VanLiuZhi commented Apr 12, 2024 •

edited

Loading

Uh oh!

shreemaan-abhishek commented Apr 12, 2024

Uh oh!

ShiningRush commented Apr 12, 2024

Uh oh!

VanLiuZhi commented Apr 13, 2024

Uh oh!

shreemaan-abhishek commented Apr 15, 2024

Uh oh!

VanLiuZhi commented Apr 17, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

VanLiuZhi commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Problem Description

2. Impact

3. Recommended Solution

Checklist

Uh oh!

shreemaan-abhishek commented Apr 11, 2024

Uh oh!

shreemaan-abhishek commented Apr 11, 2024

Uh oh!

VanLiuZhi commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VanLiuZhi commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shreemaan-abhishek commented Apr 12, 2024

Uh oh!

ShiningRush commented Apr 12, 2024

Uh oh!

VanLiuZhi commented Apr 13, 2024

Uh oh!

shreemaan-abhishek commented Apr 15, 2024

Uh oh!

VanLiuZhi commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VanLiuZhi commented Apr 9, 2024 •

edited

Loading

VanLiuZhi commented Apr 12, 2024 •

edited

Loading

VanLiuZhi commented Apr 12, 2024 •

edited

Loading

VanLiuZhi commented Apr 17, 2024 •

edited

Loading