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

fix(AppFramework): Allow requests with OCS-APIRequest header to pass CSRF checks #46760

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Jul 25, 2024

Summary

Minimal fix extracted from #43699.
OCS APIs use the custom header to prevent CSRF attacks as described in https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#employing-custom-request-headers-for-ajaxapi:

Both the synchronizer token and the double-submit cookie are used to prevent forgery of form data, but they can be tricky to implement and degrade usability. Many modern web applications do not use <form> tags to submit data. A user-friendly defense that is particularly well suited for AJAX or API endpoints is the use of a custom request header. No token is needed for this approach.

While it is possible for clients to obtain CSRF tokens, it is a lot easier sending the custom header and not maintaining any state.

Using the same method for non-OCS endpoints is safe and simply allows using endpoints that require CSRF protection at the moment.
Usually external clients do not need to send the CSRF token as the cookie check will skip the token check, but if the client needs to send cookies because it also has to call APIs that are session aware (e.g. Talk room join/leave) or it runs in a browser, where the cookies are sent automatically, this measure is necessary. The existing CORS protection will prevent hijacks as it will requests with the header.

Ideally the two CSRF protections would be aligned and merged into a single implementation as there is no reason to have separate implementations of this protection, but this can be done in the future.

Checklist

@skjnldsv skjnldsv merged commit 0ae83d6 into master Jul 27, 2024
167 checks passed
@skjnldsv skjnldsv deleted the fix/appframework/csrf-custom-header branch July 27, 2024 14:27
@blizzz blizzz mentioned this pull request Jul 30, 2024
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