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.

Sign up for GitHub

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

Jump to bottom

Accept CSRF on CORS routes #31807

Merged
merged 1 commit into from
Sep 21, 2022
Merged

Accept CSRF on CORS routes #31807

merged 1 commit into from
Sep 21, 2022

Conversation

jotoeri
Copy link
Member

@jotoeri jotoeri commented Apr 2, 2022

On Forms we got the request to allow CORS on our (OCS-)API routes. However, if i add the CORS Annotation to the OCS-Controller Methods, our internal calls to the API (with CSRF) do not work anymore.

This PR now enables to allow both, CSRF and the classical CORS requests on the OCS-routes.

Basically, this PR includes #31698 and takes a part out of #19354, since both is necessary to make it work. The latter imo initially had a similar intention like the current PR, but has been closed due to the discussion on another topic. 😉

@jotoeri jotoeri added the 3. to review Waiting for reviews label Apr 2, 2022
@jotoeri
Copy link
Member Author

jotoeri commented Apr 2, 2022

/backport to stable23

@jotoeri
Copy link
Member Author

jotoeri commented Apr 2, 2022

/backport to stable22

@nickvergessen
Copy link
Member

@juliushaertl since you did one of the previous PRs do you have any insight on the implication?

@juliushaertl
Copy link
Member

Seems like a legit addition also after reading up a bit on the potential risks and discussion in regards to OCS/CORS/CSRF in owncloud/core#15894

Still some additional pair of eyes maybe from @PVince81 would be good I think.

@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
@jotoeri
Copy link
Member Author

jotoeri commented Apr 25, 2022

Any opinion, @PVince81 ? 😉

@come-nc come-nc removed their request for review May 5, 2022 14:52
This was referenced Aug 12, 2022
Merged
Merged
This was referenced Aug 24, 2022
Merged
Merged
This was referenced Sep 6, 2022
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
@jotoeri
Copy link
Member Author

jotoeri commented Sep 15, 2022 •
edited
Loading

Anybody wanna have a look, before it only gets shifted further and further? 😢
Would be nice to have it in 25, so we can use it without waiting for 3 major versions!

ChristophWurst
ChristophWurst approved these changes Sep 15, 2022
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense

@jotoeri
Copy link
Member Author

jotoeri commented Sep 20, 2022

@nickvergessen @juliushaertl @eneiluj @miaulalala Anybody else here? I'm really sorry for asking again, but afaik rc1 is planned for thursday, no? And as we cannot restrict for patch minversion in apps, it'd be good to have it in there.

Or are there any concerns, which block any reactions currently?

@blizzz blizzz mentioned this pull request Sep 20, 2022
Closed
CarlSchwan
CarlSchwan approved these changes Sep 21, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Make sense

@nickvergessen
Copy link
Member

/rebase

@jotoeri @juliushaertl
@everlanes @nickvergessen
Allow CSRF on CORS routes
c8b7a23 
Co-authored-by: Julius Härtl <jus@bitgrid.net>
Co-authored-by: Andreas Brinner <andreas@everlanes.net>
Signed-off-by: Jonas Rittershofer <jotoeri@users.noreply.github.com>
@nickvergessen
Copy link
Member

Error unrelated

@nickvergessen nickvergessen merged commit d0ead94 into master Sep 21, 2022
@nickvergessen nickvergessen deleted the fix/cors_csrf branch September 21, 2022 11:51
@blizzz blizzz mentioned this pull request Sep 22, 2022
Merged
2 tasks
@jotoeri jotoeri mentioned this pull request Dec 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@artonge artonge Awaiting requested review from artonge

@come-nc come-nc Awaiting requested review from come-nc

@PVince81 PVince81 Awaiting requested review from PVince81

@julien-nc julien-nc Awaiting requested review from julien-nc

@miaulalala miaulalala Awaiting requested review from miaulalala

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
7 participants
@jotoeri @nickvergessen @juliushaertl @ChristophWurst @CarlSchwan @blizzz @skjnldsv