security: CSRF protection for state-changing requests (high risk)

[anonymoususer72041]

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]

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]

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]

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]

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 and
• whether @javascript/Mink tests are desired at all?

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

[anonymoususer72041]

@RussH would you be open to prioritizing this PR and #697 ahead of other fixes, features, or refactors, so we can release a short-term security patch for the highest-risk issues?

Prioritizing these high-risk items would be a big win for all OpenCATS instances.

[RussH]

It's definitely going to be a massive improvement over current. However it's not quite right yet - there's some oddities, I’m seeing a regression with bulk actions after this change.
In Candidates, selecting multiple rows and choosing “Add to list” no longer shows the modal; instead the request errors out.
I’m getting a 500 on: index.php?m=lists&a=addToListFromDatagridModal... and Apache shows memory / DataGrid warnings around the same time.
My web PHP memory_limit is 128M. The request params include maxResults=100000000 and p={...} (JSON) plus a dynamicArgument... serialized selection.

Q? : are we now sending p as JSON in some paths where the receiver expects serialized params? That could explain foreach() warnings / huge “allow all” queries and the 500.
Happy to grab more logs / stack trace if you want, but it feels like the POST quick-action path may be breaking modal flows like addToListFromDatagridModal.

[anonymoususer72041]

I was able to trace this to inconsistent DataGrid parameter encoding in bulk/quick-action flows. In particular, the "selected rows" payload (dynamicArgument/exportIDs) could be interpreted incorrectly, which then caused the DataGrid to fall back to an "allow all" query with maxResults=100000000, leading to warnings and eventually a 500 under a 128M memory_limit.

I pushed a fix that:

• Fixes the exportIDs handling in the DataGrid request parsing (including a comparison bug that caused the exportIDs branch to run incorrectly)
• Normalizes the selection payload so the server receives JSON consistently (instead of a serialized selection in some paths)
• Ensures the modal endpoint (lists/addToListFromDatagridModal) sees the expected DataGrid params and doesn't expand into a huge result set

[anonymoususer72041]

Once #715 lands into master, CI should not fail anymore.

[RussH]

@anonymoususer72041
I’m unable to push updates to the PR branch (fork). Could you please rebase/merge the latest master into your PR branch and push, so GitHub Actions runs with the current CI config? Once it’s updated I’ll review the test results.

[anonymoususer72041]

@RussH done

[RussH]

While testing this PR locally I hit what looks like a related regression with bulk “add to list” flows:

JobOrders datagrid → select multiple → “Add to static list”: the popup opens but no lists are offered (empty list selector). Feels similar to the earlier multi-select/list modal issues we’ve seen.

Also seeing a bunch of noise in error_log during normal navigation:

[Tue Feb 17 14:41:28.524893 2026] [php7:warn] [pid 5533:tid 5533] [client 127.0.0.1:56026] PHP Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, bool given in /srv/http/lib/DatabaseConnection.php on line 321, referer: http://127.0.0.1/
[Tue Feb 17 14:41:28.525154 2026] [php7:warn] [pid 5533:tid 5533] [client 127.0.0.1:56026] PHP Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, bool given in /srv/http/lib/DatabaseConnection.php on line 321, referer: http://127.0.0.1/
[Tue Feb 17 14:41:28.525501 2026] [php7:warn] [pid 5533:tid 5533] [client 127.0.0.1:56026] PHP Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, bool given in /srv/http/lib/DatabaseConnection.php on line 321, referer: http://127.0.0.1/
[Tue Feb 17 14:41:28.525784 2026] [php7:warn] [pid 5533:tid 5533] [client 127.0.0.1:56026] PHP Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, bool given in /srv/http/lib/DatabaseConnection.php on line 321, referer: http://127.0.0.1/
[Tue Feb 17 14:41:44.738091 2026] [php7:warn] [pid 5536:tid 5536] [client 127.0.0.1:53404] PHP Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, bool given in /srv/http/lib/DatabaseConnection.php on line 321, referer: http://127.0.0.1/index.php?m=home
[Tue Feb 17 14:41:44.738355 2026] [php7:warn] [pid 5536:tid 5536] [client 127.0.0.1:53404] PHP Warning: mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, bool given in /srv/http/lib/DatabaseConnection.php on line 321, referer: http://127.0.0.1/index.php?m=home
[Tue Feb 17 14:41:47.610444 2026] [php7:notice] [pid 5536:tid 5536] [client 127.0.0.1:53404] PHP Notice: session_start(): ps_files_cleanup_dir: opendir(/var/lib/php74/sessions) failed: Permission denied (13) in /srv/http/index.php on line 75, referer: http://127.0.0.1/index.php?m=activity

The session warning may be a red herring, but the mysqli_fetch_assoc(bool) warnings look like a query failing somewhere and returning false.

Can you take a look at the JobOrders “add to list” popup (multi-select) and the query failure warnings and see if they can be addressed in this PR before merging?

I need to add some behat testing for the lists, we seem to trip over this frequently - CI/CD tests pass, but a little real world testing fails.