Skip to content

hardening: enforce POST+CSRF for purge syslog devices utility - #318

Open
somethingwithproof wants to merge 19 commits into
Cacti:developfrom
somethingwithproof:fix/259-csrf-purge-utility
Open

hardening: enforce POST+CSRF for purge syslog devices utility#318
somethingwithproof wants to merge 19 commits into
Cacti:developfrom
somethingwithproof:fix/259-csrf-purge-utility

Conversation

@somethingwithproof

@somethingwithproof somethingwithproof commented Apr 11, 2026

Copy link
Copy Markdown
Member

Summary

Enforces POST and CSRF validation before the purge utility can delete unused Syslog host records.

Security behavior

  • Non-POST requests are logged and rejected.
  • Invalid CSRF tokens are logged and rejected.
  • If the CSRF helper is unavailable, the action intentionally fails closed: it logs and rejects the request rather than attempting an unsafe manual token fallback.
  • User-facing errors remain generic; logs retain the diagnostic reason.
  • The confirmation dialog posts the Cacti CSRF token and uses script-context-safe JSON encoding.

Validation

  • PHP syntax checks
  • tests/regression/issue259_csrf_purge_test.php

Compatibility

Uses existing Cacti plugin conventions and PHP 7.4-compatible syntax.

Copilot AI review requested due to automatic review settings April 11, 2026 08:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the Syslog plugin’s destructive purge_syslog_hosts utilities action to mitigate CSRF risk by requiring a CSRF-protected POST, and updates the UI flow to submit a POST (instead of a GET link). It also adds a regression script to guard against reintroducing the insecure behavior.

Changes:

  • Enforce POST and validate CSRF via csrf_check(false) before executing the purge deletes.
  • Replace the utilities GET link with a jQuery UI confirmation dialog that submits a POST including __csrf_magic.
  • Add a regression script asserting the hardening behavior and related UI/encoding expectations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
setup.php Adds POST+CSRF enforcement for purge action; replaces purge link with a dialog-driven POST flow including CSRF token.
tests/regression/issue259_csrf_purge_test.php Adds a regression script that scans setup.php for required hardening snippets and safe UI/JS encoding patterns.

Comment thread setup.php Outdated
Comment thread setup.php Outdated
Comment thread tests/regression/issue259_csrf_purge_test.php Outdated
@somethingwithproof
somethingwithproof force-pushed the fix/259-csrf-purge-utility branch from f595568 to 75e4ad6 Compare April 11, 2026 08:38
@somethingwithproof

Copy link
Copy Markdown
Member Author

@TheWitness could you take a look? This is the re-opened CSRF+POST hardening for the purge devices utility (original #265), now rebased onto develop. All merge conflicts resolved.

@TheWitness TheWitness left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That whole json_encode is it really necessary if you are using the __esc() function? If so, we need simplify it in code. Something a big simpler. Some thing like:

<?php print json_safe(__('Devices Selected', 'syslog')); ?>

Comment thread syslog.php Outdated
somethingwithproof and others added 19 commits September 3, 2026 17:26
Refs Cacti#259

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…eader

- Distinct raise_message IDs per failure mode (syslog_method_error,
  syslog_csrf_error, syslog_csrf_unavailable) so log triage can
  differentiate non-POST, invalid token, and missing-helper paths
- Add cacti_log entry on the non-POST rejection path so the audit
  trail is symmetric with the other two fail-closed branches
- Document csrf_check($fatal=false) arg semantics inline so future
  readers see the helper contract
- Rename the regression test comment block to call out explicitly
  that it is a source-scan lint, not a behavioral test; flag follow-up
  for real behavioral coverage once a DB-backed test harness exists

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Three defects, all failing open or failing at setup:

- the plugin syntax check redirected find's own output rather than php's, so
  PHP errors never reached the grep testing for them; the step could not fail
- MYSQL_AUTH_USR carried a literal tilde, because parameter expansion happens
  after tilde expansion, so MySQL was handed a path it could not resolve
- the Cacti checkout took the default branch, which is 1.3 in development and
  whose CLI installer currently fatals with an undefined __()

plugin_syslog additionally installed libapache2-mod-php${{ matrix.php }},
which Ubuntu does not package, so apt exited 100 before Cacti was reached.

Verified with actionlint, which is clean on the result.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Drops the typed signature to match the rest of functions.php, where no
function declares parameter or return types.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The install form in this file already appends header=false, and the redirect
targets need it too or the XHR renders a full page into the content tab.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The issue#279 XSS fix is unrelated to the purge CSRF guard and moves to its
own branch.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Every assertion passed while the purge was broken, and two of them rejected
correct fixes; the guard and the fragment are now exercised for real.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Every other close tag in the tree is '; ?>'; these five were the only
exceptions.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
somethingwithproof force-pushed the fix/259-csrf-purge-utility branch from 0d234f1 to ccf0c99 Compare September 4, 2026 00:36
@somethingwithproof

Copy link
Copy Markdown
Member Author

Renamed to syslog_json_safe().

On whether the json_encode is needed given __esc(): it is, but only inside the <script> block. __esc() HTML-escapes, and a browser does not HTML-decode inside a script element, so the entities would render literally and a </script> in the value would still break out. The value= attribute and the <p> text still use __esc(), which is correct for HTML context.

Also in this push: the purge post and its redirects now carry header=false following setup.php:1100, the regression test drives the guard against stubs instead of grepping source, issue#259 is in the changelog, and the nav bar encoding work moved off this branch.

@somethingwithproof
somethingwithproof dismissed TheWitness’s stale review September 5, 2026 01:23

Addressed in the syslog_json_safe() helper, used as syslog_json_safe(__('Devices Selected', 'syslog')).

@somethingwithproof somethingwithproof self-assigned this Sep 6, 2026
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.

3 participants