Add OPTION and HEAD to quarkus.http.cors.methods#3941
Conversation
| | `quarkus.http.limits.max-body-size` | `10240K` | Define the HTTP max body size limit. | | ||
| | `quarkus.http.cors.enabled` | `false` | Enable the HTTP CORS filter. Must be set to `true` for any other CORS property to take effect. | | ||
| | `quarkus.http.cors.origins` | | Define the HTTP CORS origins. | | ||
| | `quarkus.http.cors.methods` | `PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS` | Define the HTTP CORS covered methods. | |
There was a problem hiding this comment.
What "Default Value" means here is ambiguous. I would rename the column to "Default Value in Polaris" to clarify things.
|
|
||
| quarkus.http.cors.origins=http://localhost:8080 | ||
| quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT | ||
| quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS |
There was a problem hiding this comment.
That is the default value in Quarkus, so I wonder if it's simpler to just comment this out?
Or better yet: I think we should include quarkus.http.cors.enabled (it's probably an oversight), and comment out all the rest:
quarkus.http.cors.enabled=false
#quarkus.http.cors.origins=http://example.com:8080
#quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, OPTIONS, HEAD
#quarkus.http.cors.headers=*
#quarkus.http.cors.exposed-headers=*
#quarkus.http.cors.access-control-max-age=PT10M
#quarkus.http.cors.access-control-allow-credentials=trueWDYT?
There was a problem hiding this comment.
Hmm, should we disable CORS filters? The rational would be that CORS on REST endpoints it isn't less critical, than let's say on a website?
There was a problem hiding this comment.
They are disabled by default. The default value of quarkus.http.cors.enabled is false.
There was a problem hiding this comment.
Oh okay true, the doc also says that. Yes, in this case better to comment out comment out those configs. I think we should also remove those from configuring-polaris.md too, as Quarkus defaults are used then.
There was a problem hiding this comment.
I think we should also remove those from
configuring-polaris.mdtoo, as Quarkus defaults are used then.
Well this section attempts to present common configs that most users would want to configure. I think CORS deserves to be there, wdyt?
There was a problem hiding this comment.
It does, but only those, which have some non-default values no? If we don't set any default for quarkus.http.cors.*, just add quarkus.http.cors.enabled=false, then I think it is sufficient do document only quarkus.http.cors.enabled no?
I'm wondering why quarkus.http.cors.* has any non-default value in our application properties, when cors is turned off? Is it because if someone turns it on, then we think that these values are the safest option?
There was a problem hiding this comment.
quarkus.http.cors.* values were added for Polaris UI, IIRC.
Yet, if we switch to Quarkus default (which have wider scope), it should not hurt UI, I think.
I think it is sufficient to document only what we explicitly change (or instruct users to change) in Polaris compared to Quarkus defaults. Interested users can always go to Quarkus config docs (which are pretty easy to navigate in my experience).
There was a problem hiding this comment.
I'm wondering why
quarkus.http.cors.*has any non-default value in our application properties, when cors is turned off? Is it because if someone turns it on, then we think that these values are the safest option?
No, imho these values were meant as (dummy) examples.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
This change looks backward-compatible to me and worth having in 1.4.0, so I'm going to merge now. Happy to reconsider if concerns are raised after merging. |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thanks for the fix, @nandorKollar ! |
3 participants
Fixes #3938
Add HEAD and OPTIONS to default allowed method list for CORS.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)