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

[VanLiuZhi]

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]

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

[shreemaan-abhishek]

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

[VanLiuZhi]

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]

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.

[shreemaan-abhishek]

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

[ShiningRush]

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

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

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.

[VanLiuZhi]

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.