-
Notifications
You must be signed in to change notification settings - Fork 282
Security: CSRF protection for state-changing requests (high risk) #693
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
base: master
Are you sure you want to change the base?
Security: CSRF protection for state-changing requests (high risk) #693
Conversation
|
Security heads-up @RussH: this affects all deployments and can allow unauthorized write actions in an authenticated session. Please prioritize review and consider a patch release after merge. |
|
Thanks for putting the work into this.. I definitely agree that hardening the project against CSRF is a high priority. I did want to mention that SameSite=Strict cookie protections are already in place, but I recognize that moving actions from GET to POST and adding proper tokens is an enhancement. I’m currently in the middle of migrating our CI/CD from Travis-CI over to GitHub Actions. Since this PR touches so many files and changes how the app handles requests, it’s almost certainly going to break the current automated tests. I need to get the new GitHub Actions pipeline stable first so I have a reliable way to test these changes. Once that's up and running, I'll be able to check the build status here and see what needs to be tweaked to get this merged. I appreciate the patience while I get the infrastructure sorted out! |
|
Thanks @RussH – understood on the CI migration.
In the current master codebase SameSite=Strict is not actually applied. Also, Strict has real UX trade-offs compared to Lax: users coming in via external links (email, chat, ticketing systems, bookmarks) or certain SSO/OAuth redirect flows can appear "logged out" because the session cookie won’t be sent on that cross-site entry. Since this PR migrates legacy state-changing GET endpoints to POST and enforces server-side CSRF token validation on write actions, I’d suggest using SameSite=Lax as the default baseline. |
|
Marking this PR as draft because the test pipeline is not working yet. |
|
I’ve pushed the PHPUnit unit tests covering the CSRF token core logic ( Before I invest time into Behat coverage, I’d like to align with what you expect here, @RussH – this can range from a minimal set of representative scenarios to a much broader (and potentially flaky/slow) UI smoke suite. Options I see:
Do you have a preference on:
Once aligned, I’m happy to implement the Behat part accordingly. |
Summary
This PR adds CSRF protection for state-changing actions by introducing a CSRF token mechanism and enforcing server-side validation on write operations (including relevant AJAX endpoints).
As part of this work, legacy endpoints that performed state changes via GET were migrated to POST to make the write surface explicit and allow consistent token validation.
Motivation
OpenCATS includes multiple workflows where authenticated users trigger actions that modify data. Without CSRF protection, any installation is affected (including internal-only and local deployments): if a user is logged in, a crafted third-party page, link, or even an HTML-capable email/attachment opened in a browser context can trigger unintended state-changing requests in the user's session.
This is a high-impact issue because it requires no access to the OpenCATS instance itself – only that a legitimate user visits attacker-controlled content while authenticated. Adding CSRF defenses (and migrating legacy state-changing GET actions to POST where needed) significantly reduces the risk of unauthorized changes and brings OpenCATS in line with modern baseline web security expectations.
Given the potential for unauthorized write actions (including changes to privacy-sensitive candidate data and – depending on the victim's privileges – administrative settings), I suggest prioritizing review and publishing a patch release after merge.