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

CORS should only be bypassed on PublicPage if not logged in #36396

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 26, 2023

Summary

To prevent CSRF attack vectors CORS should only be bypassed if not logged in on @PublicPage.
If you add the CORS annotation you would expect all users to be validated even on @PublicPage, e.g. for APIs where anonymous users can submit data but also logged in users, currently there is no protection against CSRF on those API endpoints.

Checklist

@szaimen szaimen added this to the Nextcloud 26 milestone Jan 26, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, icewind1991, blizzz and nickvergessen and removed request for a team January 26, 2023 20:49
@jotoeri jotoeri mentioned this pull request Jan 27, 2023
4 tasks
@blizzz blizzz mentioned this pull request Feb 1, 2023
@susnux
Copy link
Contributor Author

susnux commented Feb 8, 2023

Any update on this one? Something I can help with?

@nickvergessen nickvergessen removed the request for review from LukasReschke February 8, 2023 22:10
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

LGTM, but not too deep in the topic

@susnux susnux requested a review from juliusknorr February 13, 2023 16:18
@juliusknorr
Copy link
Member

Restarted drone since the history was gone

@nickvergessen
Copy link
Member

  1. Test\AppFramework\Middleware\Security\CORSMiddlewareTest::testCORSShouldNeverAllowCookieAuth
    OC\AppFramework\Middleware\Security\Exceptions\SecurityException: CORS requires basic auth

seems related :/

… in to prevent CSRF attack vectors

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Feb 16, 2023

seems related :/

The exception is expected, fixed the new test.

@juliusknorr juliusknorr merged commit 90d2cb0 into master Feb 17, 2023
@juliusknorr juliusknorr deleted the fix/cors branch February 17, 2023 08:42
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
susnux added a commit to nextcloud/forms that referenced this pull request Mar 20, 2024
This was fixed in Nextcloud 26+, ref: nextcloud/server#36396

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
susnux added a commit to nextcloud/forms that referenced this pull request Mar 22, 2024
This was fixed in Nextcloud 26+, ref: nextcloud/server#36396

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
susnux added a commit to nextcloud/forms that referenced this pull request Mar 22, 2024
This was fixed in Nextcloud 26+, ref: nextcloud/server#36396

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Chartman123 pushed a commit to nextcloud/forms that referenced this pull request Mar 22, 2024
This was fixed in Nextcloud 26+, ref: nextcloud/server#36396

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants