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

Allow CORS same origin requests #29626

Merged
merged 3 commits into from Dec 16, 2022

Conversation

stuartwdouglas
Copy link
Member

No description provided.

@quarkus-bot

This comment has been minimized.

Also clean up the slow path
@quarkus-bot

This comment has been minimized.

@sberyozkin sberyozkin self-requested a review December 2, 2022 09:47
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @stuartwdouglas, it does look like it will offer a more optimal way of comparing Origin and the request URI, and if necessary, we can follow up with adding a flag making the same origin check optional

Copy link
Contributor

@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 blindly allowing same origin hosts by default. You are basically trusting the client to speak the truth allowing to trick Quarkus into accept from any host name (not just localhost) which is problematic.

we should check what other fwks do here - the only reason it worked before was due to another CORS filter bug.

@sberyozkin sberyozkin marked this pull request as draft December 2, 2022 14:56
@stuartwdouglas
Copy link
Member Author

-1 on blindly allowing same origin hosts by default. You are basically trusting the client to speak the truth allowing to trick Quarkus into accept from any host name (not just localhost) which is problematic.

we should check what other fwks do here - the only reason it worked before was due to another CORS filter bug.

That is the point.

CORS is intended to prevent trusted clients (i.e. browsers implementing the web security model) from being tricked by a malicious site into sending requests to other sites.

If the client is compromised then CORS is useless, the client can send any origin it wants so no matter what is configured the client can send something that would be expected. This type of issue is prevented by requiring authentication.

@sberyozkin sberyozkin marked this pull request as ready for review December 6, 2022 16:00
@sberyozkin
Copy link
Member

Re-opening for another round of reviews

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 6, 2022

Failing Jobs - Building 2ce7349

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
✔️ Maven Tests - JDK 11
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

Copy link
Contributor

@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 after reviewing and discussion on enabling same origin even when additional hosts specified for cors.

@sberyozkin
Copy link
Member

Thanks @stuartwdouglas @maxandersen

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.

None yet

4 participants