Allow CORS wildcard headers

[StaticBR]

Currently it is not possible to set wildcard header values for some CORS Headers.

This bug concerns:

• Access-Control-Allow-Headers
• Access-Control-Allow-Methods
• Access-Control-Expose-Headers

All of which can, according to the specs set to "*".
Setting this value in the quarkus properies, results in quarkus not setting the header field. This is not correct, since the default values are not "*".

This PR fixes this behavior.

[sberyozkin]

@StaticBR Can you clarify please why is it necessary ?

Quarkus would set Access-Control-Allow-Headers and Access-Control-Allow-Methods to concrete values. In your test, there are no Access-Control-Requst-Headers and Access-Control-Request-Methods.

As far as Access-Control-Exposed-Headers is concerned, it really requires a user to think and make a decision which headers should be exposable to Java scripts.

[StaticBR]

@sberyozkin
I try to explain where I am coming from:
I wanted to set the Access-Control-Expose-Headers to * in quarkus.
So i set the configuration property: quarkus.http.cors.exposed-headers=*
Which resulted in no change at all. So i searched why this configuration is not working ... which resulted in this PR.

I think that if a Developer explicitly set a configuration property to *, which is a valid value. This is what the header should be set to.

To your points:

Quarkus would set 'Access-Control-Allow-Headers' and 'Access-Control-Allow-Methods' to concrete values.

This is a suitable default. But if the config property is set, why use the default?

In your test, there are no Access-Control-Requst-Headers and Access-Control-Request-Methods.

Sorry i do not understand what you mean by that. These headers are in the test case.

As far as Access-Control-Exposed-Headers is concerned, it really requires a user to think and make a decision which headers should be exposable to Java scripts.

Yes, and if my decision is to expose all headers for an origin, I have currently no possibility to set this via the configuration property.

[sberyozkin]

@StaticBR

In your test, there are no Access-Control-Requst-Headers and Access-Control-Request-Methods.

Sorry i do not understand what you mean by that. These headers are in the test case.

Your test case has *Allow* variations in the response, but not input headers, like Access-Control-Request-Headers

which is when

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

is expected to be returned: This header is required if the preflight request contains [Access-Control-Request-Headers](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Request-Headers).

[StaticBR]

@sberyozkin
Okay, now I'm more confused. I just changed the preprocessing of the configuration property. So this change is what the test case verifies.
I did not alter under which circumstances the header is returned. So i assume this is already checked by another test.

Long story short:
What do you need, so you will be able to merge this?

[StaticBR]

@sberyozkin ^ ping.

[sberyozkin]

@StaticBR Apologies for a delay.

So, first of all, I'm a bit generally cautions now about letting users just to solve all CORS concerns with * alone, CORS can be truly annoying for users who want to make things done, and the easiest path to it is just set wildcards. At one point of time CORS origins were * even by default.

That was my question, why exactly do you need this optimization ? I agree it is simpler, but while, with Origins, it can be hard in devmode to list all the origins in Kubernetes, in other deployments, where * can help, what is a real problem with listing for example, POST,PUT,DELETE - it is clear, concrete and obvious.
Similar to headers, there is a limited number of important headers that the application may need to work with.
I'm not convinced right now it is a real dev experience issue.

And also, looks like one of us, possibly me, is confused about the test. I believe these headers like Access-Control-Allow-Methods is returned in the preflight response, or in any case, whenever Access-Control-Request-Header is present - but it is not in your test... But then if this request header is present, why return *...

I've noticed the Fetch spec says something about * in this context but it is not clear at all how is it relevant if at all.

So I'd like to have much more clarity for us to progress on this PR if it proves possible.

CC @cescoffier

[StaticBR]

@sberyozkin
All right here we go:

So, first of all, I'm a bit generally cautions now about letting users just to solve all CORS concerns with * alone, CORS can be truly annoying for users who want to make things done, and the easiest path to it is just set wildcards. At one point of time CORS origins were * even by default.

I totally agree, we have to be cautions. And i also always argue for sensible default values. But why should quarkus be more restrictive as the W3C Standard? Why would you force a developer to be not able to use one specific value?

That was my question, why exactly do you need this optimization ? I agree it is simpler, but while, with Origins, it can be hard in devmode to list all the origins in Kubernetes, in other deployments, where * can help, what is a real problem with listing for example, POST,PUT,DELETE - it is clear, concrete and obvious.

I agree here, since the HTTP commands are a finite list. I changed this just for consistent behavior of the parameters.

Similar to headers, there is a limited number of important headers that the application may need to work with. I'm not convinced right now it is a real dev experience issue.

This is the one i disagree with you, and was also my point when i stumbled upon this.
Lets make a example: you have an API that returns user defined header metadata for a file download. After this change you can simply return all headers via CORS and prevent misuse by an origin filter.

And also, looks like one of us, possibly me, is confused about the test. I believe these headers like Access-Control-Allow-Methods is returned in the preflight response, or in any case, whenever Access-Control-Request-Header is present - but it is not in your test... But then if this request header is present, why return *...

My change only changed the value of following headers:

Access-Control-Allow-Headers & Access-Control-Allow-Methods
You are right normally these are preflight header. Currently quarkus also returns them on the request it self, which is not required. Therefore you are also correct, even if the test works I tested for wrong condition. I fixed the test by splitting it.

Access-Control-Expose-Headers
This is not a preflight header.

I've noticed the Fetch spec says something about * in this context but it is not clear at all how is it relevant if at all.

Maybe because the fetch request will filter all headers that are not listed within the Access-Control-Expose-Headers?

So I'd like to have much more clarity for us to progress on this PR if it proves possible.

Hope you have more clarity now 😁.

[sberyozkin]

@StaticBR, thanks

So, first of all, I'm a bit generally cautions now about letting users just to solve all CORS concerns with * alone, CORS can be truly annoying for users who want to make things done, and the easiest path to it is just set wildcards. At one point of time CORS origins were * even by default.

I totally agree, we have to be cautions. And i also always argue for sensible default values. But why should quarkus be more restrictive as the W3C Standard?

Because IMHO we can't afford following every standard's security text blindly but offer an opinionated treatment of some parts of such texts, anything related to wildcards is certainly one such part.

Why would you force a developer to be not able to use one specific value?

Because the practice shows the developers would embrace wildcards to push CORS concerns aside at dev time and very likely keep them in prod. Also, wildcard is not really a specific value.

As far as the rest of the comments are concerned, they indeed make things clearer to me, thanks for that.
Your real use case is supporting situations where allowed headers are quite dynamic in nature, which does look like a case which is worth supporting.

I'm not too concerned about not doing the same for allowed methods because IMHO, it is a small, restricted set, it is not hard to specify. Also, IMHO we should not be touching exposed headers because this is really about letting scripts access known, concrete headers, which users should configure.

As far as allowed methods and exposed headers are concerned, personally, I'd be open to allowing wildcards in devmode only for developers to start fast, in prod - concrete values should be set. Please don't be concerned about it too much right now, let's deal with your case first, and can continue talking about the rest of them in Discussions. Let's also see what @cescoffier and others think.

Back to the allowed headers. I think the way you changed the code is not correct - because if , during the preflight request, Access-Control-Request-Headers includes concrete values - then the same values should be echoed back, not Access-Control-Allow-Headers=* - which is only returned for regular CORS requests - there should be a test case confirming it, even if it already works as expected.

[StaticBR]

@sberyozkin

Because IMHO we can't afford following every standard's security text blindly but offer an opinionated treatment of some parts of such texts, anything related to wildcards is certainly one such part.
Because the practice shows the developers would embrace wildcards to push CORS concerns aside at dev time and very likely keep them in prod. Also, wildcard is not really a specific value.

Unfortunately I think we two have a different design philosophy there. I would argue that an developer, who explicitly sets a value, knows what he is doing. And therefor a framework should not prohibit him to do so. But like I said, different styles. If it is intentional diverging from the W3C standard, it would be nice to add this to the documentation.

As far as allowed methods and exposed headers are concerned, personally, I'd be open to allowing wildcards in devmode only for developers to start fast, in prod - concrete values should be set.

I would argue against that behavior. Handling of this headers should not be different in devmode. Otherwise the Developer will think the solution also works in production. This will only cause frustration. Either we change it completely or not at all.

Back to the allowed headers. I think the way you changed the code is not correct - because if , during the preflight request, Access-Control-Request-Headers includes concrete values - then the same values should be echoed back, not Access-Control-Allow-Headers=* - which is only returned for regular CORS requests - there should be a test case confirming it, even if it already works as expected.

Okay I will make one last try to convince you why I changed it this way.
Let's make a concrete Example:
I develop a file storage API. The client can upload and download blobs. This blobs can have some client-defined metadata assigned to it and is called via CORS. In order do this in one request and do not have to convert the blob in base64, we use the binary data as HTTP content and the metadata will be transmitted in the header as key-value pairs.

Client upload Data (POST):
The Browser will make a preflight request to check if POST is allowed and also sends the header fields in Access-Control-Request-Headers.
To handle this behavior on server side I currently have to implement an own handler for the preflight request. Because there is not an option for the server allow every header. This could be solved by allowing to answer with Access-Control-Allow-Headers=*.

Client download Data GET:
Same behavior here: Client sends a preflight. Okay GET is allowed then it will send the request it self.
The server wants to return all the client-defined metadata with the blob content to the client.
But for the client to be able to use the header fields, the server has to again develop around the CORS plugin and handle the Access-Control-Expose-Headers it self, by setting the response header somewhere within the code. Instead of just saying Access-Control-Expose-Headers=* via the CORS property.

Anyway if you are still convinced this is a bad idea, or does not fit your design philosophy please feel free to close this pull request.

[StaticBR]

@sberyozkin ^ ping.

[StaticBR]

@sberyozkin ^ ping

[sberyozkin]

@StaticBR I missed your pings, sorry, but one ping is enough :-)

Client upload Data (POST):
The Browser will make a preflight request to check if POST is allowed and also sends the header fields in Access-Control-Request-Headers.
To handle this behavior on server side I currently have to implement an own handler for the preflight request. Because there is not an option for the server allow every header. This could be solved by allowing to answer with Access-Control-Allow-Headers=*

What I'm saying is that, since you have the browser sending the concrete header values in Access-Control-Request-Headers, the server should echo exactly the same values instead of opening it up with *, when you have configured quarkus.http.cors.headers=*, right ?

Same behavior here: Client sends a preflight. Okay GET is allowed then it will send the request it self.
The server wants to return all the client-defined metadata with the blob content to the client.
But for the client to be able to use the header fields, the server has to again develop around the CORS plugin and handle the Access-Control-Expose-Headers it self, by setting the response header somewhere within the code. Instead of just saying Access-Control-Expose-Headers=* via the CORS property.

I think this use case is valid.

[sberyozkin]

@StaticBR

Because IMHO we can't afford following every standard's security text blindly but offer an opinionated treatment of some parts of such texts, anything related to wildcards is certainly one such part. Because the practice shows the developers would embrace wildcards to push CORS concerns aside at dev time and very likely keep them in prod. Also, wildcard is not really a specific value.

Unfortunately I think we two have a different design philosophy there.

It is not about the design philosophy, but about us trying to be defensive against some optimistic spec texts and also minimizing the risk of users going with wildcards into prod without quite understanding the possible consequences. We've had some issues with wildcards so this is why there has to be a very convincing case to allow them.

IMHO your exposed headers case fits this requirement. The same for dynamic headers - but like I said, your implementation gives more to the client than what was actually asked for, the configured wildcard should allow echoing concrete requested header names back.

There is no dynamism in the allowed methods case as far as the server implementation is concerned, so IMHO we should avoid wildcards here.

Thanks