Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove OpenApi CORS default support #29666

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

sberyozkin
Copy link
Member

Fixes #28397

Users should set CORS Filter properties themselves (example, quarkus.http.cors.enabled=true and quarkus.http.cors.origins=* or quarkus.http.cors.origins=http://localhost:8080,http://quarkus.io, etc) if they want to access an OpenApi endpoint which requires CORS protection.
Migration note will follow

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 16, 2022

@maxandersen FYI, if a wildcard open api CORS access is really needed, quarkus.http.cors=true and quarkus.http.cors.origins=* will do - IMHO it makes sense to have a single source where CORS in managed (CORS filter) as opposed to individual extensions choosing to do some specific CORS simplifications

@sberyozkin sberyozkin force-pushed the openapi_remove_cors_wildcard branch from fe719f2 to ef303ec Compare December 16, 2022 12:53
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 16, 2022

Failing Jobs - Building ef303ec

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@sberyozkin
Copy link
Member Author

sberyozkin commented Dec 21, 2022

@stuartwdouglas Can you have a look please if you can get a chance ? This PR is consistent with #29692. It is worth trying to get it into 2.16.0.CR1 to let users check if no real side-effects are introduced, and eliminate confusing warning as mentioned by @yrodiere, and make SAST scanners happy too which don't like a wildcard hardcoded.

@sberyozkin
Copy link
Member Author

Hi @gsmet @phillip-kruger Can you please consider reviewing it ? FYI, in 2.16.CR1, the default Vert.x HTTP CORSFilter does not enable a wildcard by default, therefore, if the application requires CORS support in 2.16 CR1, they will have to enable Vert.x HTTP CORS even if someone wants a CORS Origin wildcard.

Therefore the code in OpenApiHandler which allows a wildcard CORS is effectively a dead code in 2.16 CR1 - the situation where the application itself does not need CORS but OpenAPI does is unrealistic IMHO. It is a good opportunity to clean it up for 2.16.Final

Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gsmet gsmet merged commit 37ba3ed into quarkusio:main Jan 30, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Jan 30, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 30, 2023
@gsmet
Copy link
Member

gsmet commented Jan 30, 2023

@sberyozkin could you please add a note to the 2.17 migration guide?

Also please do not advise to use *.

@sberyozkin sberyozkin deleted the openapi_remove_cors_wildcard branch April 3, 2023 13:11
@sberyozkin
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smallrye-OpenAPI: Don't produce any CORS headers by default in production when CORS is disabled
3 participants