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

fix(AppFramework): Correctly implement CSRF checks #43699

Closed
wants to merge 6 commits into from
Closed

fix(AppFramework): Correctly implement CSRF checks #43699

wants to merge 6 commits into from

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Feb 20, 2024 •
edited
Loading

Summary

These changes ensure that actually all the CSRF protection methods can be correctly used.
I improved the existing codes and allowed requests with OCS-APIRequest headers to pass which was not possible before and a bug.
As far as I can see there were no security problems before and this does not open any new ones ™️

Checklist

@provokateurin provokateurin added bug 3. to review Waiting for reviews labels Feb 20, 2024
@provokateurin provokateurin added this to the Nextcloud 29 milestone Feb 20, 2024
nickvergessen
nickvergessen requested changes Feb 21, 2024
View reviewed changes
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.

Seems to break cypress?

@come-nc
Copy link
Contributor

come-nc commented Feb 22, 2024

Code looks good but I do not know the situation well enough to say if it’s okay to require only one method to pass.

@provokateurin provokateurin marked this pull request as draft February 28, 2024 16:07
This was referenced Mar 12, 2024
Merged
Merged
@provokateurin provokateurin force-pushed the fix/appframework/csrf-request-checks branch from 978b2c2 to 5fca3cc Compare March 14, 2024 12:26
@provokateurin provokateurin marked this pull request as ready for review March 14, 2024 12:26
@provokateurin
Copy link
Member Author

I rewrote this PR from scratch with the new information I had. This should be much easier to review. The main problem before was concerning the cookie checks which I didn't understand correctly, but now I also added documentation for the next people who will look at it...

come-nc
come-nc reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

In the end what is the behavior change brought by this PR?

@provokateurin
Copy link
Member Author

It allows using the OCS-APIRequest header to bypass CSRF checks. For clients this is necessary, so they don't have to deal with CSRF tokens. Both methods are safe, but the former was not possible until now.

@nickvergessen
Copy link
Member

So nothing for 29 during freeze 🙊

@provokateurin
Copy link
Member Author

I guess so :/

@provokateurin
Copy link
Member Author

Since the branch-off is done, can I get reviews to get it into Nextcloud 30?

@provokateurin
Copy link
Member Author

@nickvergessen @ChristophWurst Can I get your reviews?

Altahrim
Altahrim reviewed May 3, 2024
View reviewed changes
@provokateurin
refactor(AppFramework): Improve strict and lax cookie checks
c9479c6 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
refactor(AppFramework): Improve and document cookie check requirements
86bd47c 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
fix(AppFramework): Allow requests with OCS-APIRequest header to pass …
0f953c9 
…CSRF checks

Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin
docs(AppFramework): Document CSRF check requirements
a57f9ca 
Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin force-pushed the fix/appframework/csrf-request-checks branch from 5fca3cc to 22a6959 Compare May 5, 2024 06:32
@provokateurin provokateurin requested a review from Altahrim May 5, 2024 06:33
@provokateurin provokateurin force-pushed the fix/appframework/csrf-request-checks branch from 22a6959 to a77ef60 Compare May 6, 2024 04:18
@provokateurin provokateurin mentioned this pull request Jul 25, 2024
Merged
4 tasks
@provokateurin
Copy link
Member Author

I made a minimal version of these changes that only include the actual fix and leaves out all the refactoring and documentation: #46760

@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Altahrim Altahrim Altahrim approved these changes

@bigcat88 bigcat88 bigcat88 approved these changes

@come-nc come-nc come-nc approved these changes

@miaulalala miaulalala Awaiting requested review from miaulalala

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

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

Successfully merging this pull request may close these issues.
7 participants
@provokateurin @come-nc @nickvergessen @ChristophWurst @Altahrim @bigcat88 @skjnldsv