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

Do not support any Origin by default if CORS is enabled #29692

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

sberyozkin
Copy link
Member

If CORS is enabled then the users have to take an action and enable the wildcard if they really need to, as opposed to Quarkus doing it by default. This update will hopefully encourage users take CORS configuration more seriously.
DevUI is not expected to be affected.

Note I'm adding the wildcard to the tests to have them passing again, which also shows that the users who need a wildcard will only have to add one more property.
The migration guide update will follow.

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member Author

Sorry, will clean it up, I thought I knew which tests could be affected

@sberyozkin sberyozkin force-pushed the cors_wildcard_disabled branch from fb24934 to cf3c09a Compare December 6, 2022 13:45
@quarkus-bot

This comment has been minimized.

@sberyozkin sberyozkin marked this pull request as draft December 6, 2022 16:57
@sberyozkin sberyozkin force-pushed the cors_wildcard_disabled branch from cf3c09a to 5608d91 Compare December 6, 2022 17:34
@sberyozkin sberyozkin force-pushed the cors_wildcard_disabled branch from 5608d91 to ca0d4c8 Compare December 6, 2022 23:55
@sberyozkin sberyozkin marked this pull request as ready for review December 7, 2022 10:50
@quarkus-bot

This comment has been minimized.

Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

+1 on the principle here; Enabling CORS should not have * by default given we apply #29626 so same origin still works.

Would prefer if @stuartwdouglas or @cescoffier verifies the implementation beyond just me.

@sberyozkin
Copy link
Member Author

Thanks @maxandersen, sure, as commented in the other issue, Stuart's same origin check fix is a better version of * for such a check. But yes, let Stuart/Clement double check, rebasing this PR now

@sberyozkin sberyozkin force-pushed the cors_wildcard_disabled branch from ca0d4c8 to cb74a62 Compare December 16, 2022 12:30
@quarkus-bot

This comment has been minimized.

@sberyozkin sberyozkin force-pushed the cors_wildcard_disabled branch from cb74a62 to e85bea0 Compare December 20, 2022 11:38
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 20, 2022

Failing Jobs - Building e85bea0

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 sberyozkin dismissed stuartwdouglas’s stale review December 21, 2022 23:41

Boolean all origins value is introduced as requested

@sberyozkin
Copy link
Member Author

Hi @stuartwdouglas I'll go ahead with the merge tomorrow if you don't have other comments, thanks

@sberyozkin sberyozkin merged commit b32cf7e into quarkusio:main Dec 22, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 22, 2022
@sberyozkin sberyozkin deleted the cors_wildcard_disabled branch December 22, 2022 12:33
@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.

4 participants