Skip to content

Conversation

@anonymoususer72041
Copy link
Contributor

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.

@anonymoususer72041
Copy link
Contributor Author

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.

@RussH
Copy link
Member

RussH commented Jan 8, 2026

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!

@anonymoususer72041
Copy link
Contributor Author

Thanks @RussH – understood on the CI migration.

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.

In the current master codebase SameSite=Strict is not actually applied. lib/Session.php defines a $samesite = 'Strict' variable, but the cookie is set via the legacy setcookie() signature without a SameSite attribute and there is no session.cookie_samesite / session_set_cookie_params(... 'samesite' => ...) configuration in the repo.

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.

@anonymoususer72041 anonymoususer72041 marked this pull request as draft January 9, 2026 15:16
@anonymoususer72041
Copy link
Contributor Author

Marking this PR as draft because the test pipeline is not working yet.

@anonymoususer72041
Copy link
Contributor Author

I’ve pushed the PHPUnit unit tests covering the CSRF token core logic (getCSRFToken(), rotateCSRFToken(), isCSRFTokenValid()). That should give us solid regression coverage for the most security-critical part.

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:

  1. Minimal / representative:

    • 2–4 Behat scenarios asserting:

      • state-changing actions reject GET (POST-only)
      • POST without/invalid CSRF is rejected
      • POST with valid CSRF succeeds
    • One "module" action (e.g. candidate delete) + one AJAX action (e.g. ajax/deleteActivity.php) as representatives.

  2. Broader security coverage:

    • Same assertions as (1), but across multiple entity types (candidate/company/contact/activity), using DB state assertions.
  3. Very broad UI coverage:

    • A @javascript smoke test that navigates many pages and checks every form[method=post] has a non-empty csrfToken after page load.

Do you have a preference on:

  • how broad Behat coverage should be,
  • whether @javascript/Mink tests are desired at all,
  • and where you’d like these tests to live (existing suite vs separate suite)?

Once aligned, I’m happy to implement the Behat part accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants