fix(security): consolidated hardening — SQL injection, XSS, unserialize, trigger_cmd by somethingwithproof · Pull Request #773 · Cacti/plugin_thold

somethingwithproof · 2026-05-17T07:28:38Z

Consolidates the hardening from #766, #767, #769, #770, #771 and #772 into one diff.

Security

Bulk form actions in notify_lists.php and notify_queue.php use prepared statements with IN (?,?,?) placeholders instead of array_to_sql_or() and string concatenation.
get_allowed_thresholds() and get_allowed_threshold_logs() bind $graph_id rather than interpolating it. Callers supply values for any ? in their own $sql_where via a new $sql_params argument, appended before the $graph_id placeholder.
The rfilter value goes through db_qstr_rlike() where Cacti provides it. That helper is core's remediation for GHSA-69gg-xrh3-gp82 and bounds the operand as well as quoting it; it arrived in 1.2.31, so the plugin falls back to plain quoting on the 1.2.25 it declares support for.
Values substituted into trigger commands are quoted with cacti_escapeshellarg().
page, id and drp_action are escaped where they are printed into hidden inputs, and AJAX filter parameters are wrapped in encodeURIComponent().
The RPN evaluator no longer uses eval(). Operators dispatch through a switch; operands were already validated numeric and the operator set was already closed, so the results are unchanged.

Bugs this uncovered

Writing tests that run the code rather than grep it turned up four defects, all fixed here:

Every bulk action on the Notification Lists page did nothing. get_filter_request_var() stores drp_action as an int while the allowlist held strings, so the strict in_array() rejected all of them and each action redirected without touching the database. This was introduced by 9eee922 on this branch.
Bulk writes were discarded on MySQL. Cacti's db_commit_transaction() gates the commit on SELECT @@in_transaction, which only MariaDB defines; on MySQL the query errors and the commit is skipped, so the open transaction is rolled back at disconnect. Verified against mysql:8 and mariadb:10.6. The plugin now issues the transaction statements directly, which behaves identically on both. Worth a separate core fix, since these helpers have no other caller in 1.2.31.
0 0 / corrupted the RPN stack. The zero-divided-by-zero case broke out of the operator switch before pushing its result, so the stack lost two entries and gained none.
% by zero was a fatal, and SQRT of a negative or LOG of zero pushed NAN/-INF. Those compare false against every bound, so a breach went unnoticed. Both now raise the expression's error flag.
Inline trigger commands logged nothing. thold_process_command_output() was passed the topic 'thold', which matches none of its branches.

Deleting a notification list also no longer skips soft-deleted devices, so a device that is later restored cannot come back pointing at a list that no longer exists.

Tests

The previous suite read the plugin's source as text and asserted on substrings, so it could not tell a working fix from a comment; every one of the bugs above passed it. It is replaced with tests that run the code against a recording stand-in for the Cacti framework functions.

145 tests. Every line this branch changes is covered, which CI enforces per pull request — whole-file coverage would be meaningless when most of the plugin only runs inside a live Cacti.

The suite runs on PHP 8.1 in Docker, matching the oldest interpreter the integration matrix covers. composer test:docker runs locally exactly what CI runs.

Compatibility

Checked against release/1.2.31 and release/1.2.25: no PHP 8-only syntax, and every core function, constant and signature the diff relies on exists in both.

…dening RLIKE SQL injection: rfilter request variable was concatenated directly into RLIKE patterns across thold_graph.php (4 instances), thold.php, and notify_lists.php (3 instances including the notification list filter). Replaced with db_qstr() which SQL-escapes and quotes the value. XSS: get_request_var('page') was printed raw into hidden input value attributes. Wrapped with html_escape(). Unserialize: thold_webapi.php called cacti_unserialize(stripslashes(...)) on POST selected_graphs_array. Replaced with sanitize_unserialize_selected_items() which validates the result is an array of integers only. trigger_cmd: thold_set_environ() was called with trigger_cmd_high in both the low-breach and norm-restoration branches. Corrected to trigger_cmd_low and trigger_cmd_norm respectively. No intval() casts added: host_id/site_id go through FILTER_VALIDATE_INT in the request validation arrays; adding casts after validated request vars is redundant per Cacti convention. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

25 tests covering RLIKE injection, XSS output escaping, unserialize hardening, and trigger_cmd variable confusion. All tests are source-level static checks — no Cacti bootstrap or database required. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Split the combined OR-match in TriggerCmdRegressionTest into two independent preg_match assertions so a revert of either branch is caught independently. Add FILTER_VALIDATE_IS_REGEX test to RlikeInjectionTest documenting that rfilter goes through regex validation before any RLIKE clause, mitigating ReDoS at the MySQL engine level. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

notify_lists.php: replace array_to_sql_or() and direct $selected_items concatenation with db_execute_prepared() + IN (?,?,?) placeholders for all bulk delete, associate, and disassociate operations. Also replace count() with cacti_sizeof() per Cacti 1.2.x idiom. thold_functions.php: parameterise $graph_id in get_allowed_thresholds() and get_allowed_threshold_logs() using ? placeholders; switch to db_fetch_assoc_prepared() and db_fetch_cell_prepared(). Add PreparedStatementTest.php, Php74CompatibilityTest.php, and Smoke/PhpSyntaxTest.php to cover these patterns. 41 tests pass. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Prevents open redirect via URL manipulation in JS filter forms. Affects thold.php, thold_graph.php, notify_lists.php, notify_queue.php, thold_templates.php, and thold_webapi.php. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

PhpSyntaxTest.php used str_starts_with() (PHP 8.0+) which would fatal under PHP 7.4 before any assertion ran. Use strncmp() instead. Also remove redundant (int) cast and double in_array check on drp_action in notify_lists.php: get_filter_request_var() already validated the value; the second clause was dead and confusing. Replace count() with cacti_sizeof() in setup.php bulk loop. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

array_keys($actions + $assoc_actions) returns integer keys; POST values are always strings. Without the strval() cast, in_array(..., true) with strict comparison always fails, making every bulk form action unreachable. Adds regression test asserting the strval() cast is present. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…ndlers All three action-save blocks (save_associate, save_templates, save_tholds) now call get_filter_request_var() for id, notification_action, notification_warning_action, and notification_alert_action before consuming those values via get_request_var() in prepared-statement params. Add inline comment on all RLIKE db_qstr() sites documenting the dual guard: FILTER_VALIDATE_IS_REGEX pre-validates; db_qstr() SQL-escapes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR hardens the Thold plugin against multiple security issues (SQL injection via RLIKE concatenation and bulk-action concatenation, XSS via unescaped request vars in URLs and HTML, and unsafe deserialization) and corrects a couple of variable-name bugs in thold_command_execution(). It also introduces a Pest-based test suite with stubs/bootstrap for running outside Cacti, plus PHP lint and PHP 7.4 compatibility smoke tests.

Changes:

Replace raw RLIKE/SQL string concatenation with db_qstr() and parameterized queries (db_execute_prepared, db_fetch_assoc_prepared, db_fetch_cell_prepared), and add $sql_params to get_allowed_thresholds/get_allowed_threshold_logs.
Wrap URL parameters in encodeURIComponent, escape get_request_var('page') with html_escape in hidden inputs, replace cacti_unserialize(stripslashes(...)) with sanitize_unserialize_selected_items, and add drp_action whitelist + per-branch input validation in notify_lists.php.
Fix two thold_set_environ calls that incorrectly passed trigger_cmd_high in the low/norm branches; add Pest test suite, bootstrap stubs, composer.json, and phpunit.xml.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
notify_lists.php	Convert bulk SQL to prepared statements; add drp_action whitelist and request var filters; encode URL params
thold_functions.php	Add `$sql_params`, switch to prepared SELECTs; fix trigger_cmd_low/norm environ args; annotate eval/exec with nosemgrep
thold_graph.php	Use `db_qstr` for RLIKE; `html_escape` page var; `encodeURIComponent` URL params
thold.php	Use `db_qstr` for RLIKE; `encodeURIComponent` URL params
thold_templates.php	`encodeURIComponent` on filter
thold_webapi.php	Switch to `sanitize_unserialize_selected_items`; encode URL params
notify_queue.php	`encodeURIComponent` URL params
setup.php	Use `cacti_sizeof` instead of `count`
tests/*	New Pest test suite, bootstrap stubs, smoke/security tests
composer.json, phpunit.xml	Test tooling config

Comments suppressed due to low confidence (1)

notify_lists.php:21

$actions and $assoc_actions are not visible in this diff hunk, but the existing code branches further below dispatch on drp_action == '1' / '2' for at least three different contexts (lists, associate, templates, tholds) which may use overlapping numeric keys. Combining them with $actions + $assoc_actions only validates membership in the union of keys, which is fine, but please confirm that all four save_* sections (save_list, save_associate, save_templates, save_tholds) only ever receive drp_action values present in one of those two arrays — otherwise legitimate actions will hit raise_message(40) and redirect. If save_templates/save_tholds use a different action map, they will be rejected here.

 | http://www.cacti.net/                                                   |

Verifies that poller_thold.php, setup.php, thold.php, and thold_graph.php contain no single-line raw db_*() calls with interpolated variables. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

get_total_row_data accepts $sql_params since Cacti 1.2.x (lib/auth.php:3120). Both call sites now carry a comment referencing the function signature so future callers understand the API assumption. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

All four form_actions() bulk write blocks now use db_begin_transaction() / db_commit_transaction() so a partial failure cannot leave notification routing state inconsistent across plugin_notification_lists, host, thold_data, and thold_template tables. Adds db_begin/commit/rollback_transaction stubs to test bootstrap and a regression test asserting all four blocks carry transaction guards. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Track $ok across all db_execute_prepared calls in each of the four bulk action blocks. Call db_rollback_transaction() when any statement returns false instead of committing a partial write. Tests: add rollback-count assertion (4), $ok-flag presence check. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…r commit - Add if (!ok) { break; } at end of each per-item loop in save_associate, save_templates, and save_tholds so failed iterations halt immediately. - Move thold_template_update_thresholds calls to after db_commit_transaction() so the cascade does not participate in the transaction boundary. - Apply html_escape() to get_filter_request_var('page') output in thold.php hidden input (mirrors thold_graph.php pattern). - Add tests: loop-break guard count, template-cascade-after-commit, thold.php page XSS fix. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

…notes escapeshellarg PHP_BINARY path in PhpSyntaxTest shell command to handle paths with spaces. Note in thold_functions that thold_set_environ uses putenv side-effects that exec() inherits. Note in bootstrap stub that sanitize_unserialize_selected_items must stay in sync with Cacti core. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…tract Use named variables for the pre-fix vulnerable pattern strings in RlikeInjectionTest to make them easier to verify. Add comments above get_allowed_thresholds/logs noting that sql_params must be supplied when sql_where contains ? placeholders. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Single-quoted string literals with embedded single quotes produced invalid PHP syntax. Nowdoc avoids quoting issues entirely. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…t-circuit delete block PreparedStatementTest regex [^)]+ stopped at first ) inside argument lists and never matched the && $ok chain. Replaced with substr_count. Delete block now chains all eight db_execute_prepared calls with && so the first failure stops execution rather than running all subsequent statements. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…tags RPN math evaluator: replace the two eval() sinks in thold_expression_math_rpn() with native switch dispatch. The binary operator case now computes results directly (+ 0 coercion and (int) casts preserve eval()'s integer semantics for % and ^); the unary-function case gains a missing is_numeric($v1) guard before dispatching to sin()/cos()/sqrt()/etc. Operands were already validated numeric and operators/functions were already whitelisted, so behavior is unchanged for legitimate RPN expressions while the code-execution sink is eliminated. trigger_cmd execution: thold_replace_threshold_tags() gains a $shell flag. When set (only the three trigger_cmd_high/low/norm exec() callers pass true), device/user-derived string tags (<DESCRIPTION>, <HOSTNAME>, <LOCATION>, <SITE>, <THRESHOLDNAME>, <DSNAME>, <NOTES>, <DNOTES>, <DEVICENOTE>, <EXTERNALID>) are escaped with cacti_escapeshellarg() so they cannot break out of the admin-configured command line. Email/HTML callers keep the default $shell=false and are unaffected. Numeric/computed tags remain unescaped.

somethingwithproof · 2026-07-29T03:37:55Z

Re-verified: branch is up to date with develop (no rebase needed), all 4 Integration Test checks currently pass. All 8 Copilot review threads on this PR are already marked resolved. No changes needed.

TheWitness

Use Cacti's composer.json in testing.

The previous suite read the plugin's source as text and asserted on substrings, so it could not distinguish a working fix from a comment. This runs the code instead: tests/Support/CactiStub.php records and programs the Cacti framework functions the plugin calls, and PHPUnit runs against PHP 8.1 in Docker to match the oldest interpreter the CI matrix covers. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Modulo by zero raised an uncaught DivisionByZeroError, and SQRT of a negative or LOG of zero pushed NAN or -INF, which compares false against every bound so the breach went unnoticed. Zero divided by zero broke out of the operator switch before pushing its result, leaving the stack short by two. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

db_qstr_rlike() is core's remediation for GHSA-69gg-xrh3-gp82: on top of quoting it bounds the operand and strips the alternation characters that made the pattern a denial-of-service vector. It arrived in 1.2.31, so the plugin falls back to plain quoting on the 1.2.25 it still declares support for. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

get_filter_request_var() stores drp_action as an int while the allowlist held strings, so the strict in_array() rejected every action and each one redirected without touching the database. The delete also bound $selected_items with its submitted keys intact, which PDO reads as named parameters, and committed through Cacti's db_commit_transaction(), which tests a MariaDB-only system variable and so never commits on MySQL. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

The tests that need Cacti to provide db_qstr_rlike() or get_total_row_data() run in their own process, so defining those functions does not change which branch the rest of the suite takes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

thold_process_command_output() dispatches on the topic it is passed, and 'thold' matched none of its branches, so a trigger command run outside the notification queue recorded neither its exit status nor its output. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

…sembled Also covers the four exit-status and output combinations the command result logging distinguishes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

TheWitness

No composer.json in plugins.

Adopts the conventions from Cacti core: tests/bootstrap-unit.php, tests/Helpers for the stubs, a phpunit.xml carrying error_reporting -1 and CACTI_TEST_BOOTSTRAP, and composer lint / test / test:coverage scripts so CI runs the same commands a developer does. The dev toolchain is Cacti's, pinned to the same platform php 8.1.0. Cacti core runs Pest and this suite does not, because pest ^2 does not resolve on PHP 8.1 -- the platform Cacti's own composer.json pins. Releases up to v2.36.0 conflict with phpunit 10.5.62 and later, every earlier 10.x release is blocked by advisory PKSA-z3gr-8qht-p93v, and v2.36.1, which does resolve, requires PHP 8.2. The stack installs on 8.2 and above; 8.1 is the floor this plugin's CI matrix targets. The tests are written in the plain PHPUnit class style that Cacti's tests/Pest.php explicitly supports, so they run unchanged under Pest wherever it is installable. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Same harness as Cacti#773 and Cacti#788, so whichever lands first the others merge cleanly. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Same harness as Cacti#773 and Cacti#788, with gmp added to the image so the 64-bit counter arithmetic can be tested exactly. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof added 9 commits May 16, 2026 23:32

test(security): add encodeURIComponent regression tests for AJAX filters

f8ca477

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

Copilot AI review requested due to automatic review settings May 17, 2026 07:28

Copilot AI reviewed May 17, 2026

View reviewed changes

somethingwithproof added 5 commits May 17, 2026 00:32

test(security): add PreparedStatementConsistencyTest from Cacti#769

a21d6d0

Verifies that poller_thold.php, setup.php, thold.php, and thold_graph.php contain no single-line raw db_*() calls with interpolated variables. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof and others added 6 commits May 17, 2026 01:29

docs(changelog): document security hardening entries for develop

246c61b

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

fix(test): use nowdoc for RLIKE vulnerable-pattern strings

7a4ded8

Single-quoted string literals with embedded single quotes produced invalid PHP syntax. Nowdoc avoids quoting issues entirely. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof requested review from TheWitness and netniV July 13, 2026 16:59

Merge branch 'develop' into security/consolidated-hardening-20260516

b0e1f05

TheWitness requested review from bmfmancini and browniebraun August 17, 2026 00:04

TheWitness requested changes Aug 17, 2026

View reviewed changes

somethingwithproof added 12 commits August 16, 2026 17:47

docs(changelog): record the hardening and the bugs it uncovered

496f9b4

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

Merge branch 'develop' into security/consolidated-hardening-20260516

b175787

ci: leave the Apache PHP package change to its own pull request

383387c

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

fix(thold): quote substituted values before the trigger command is as…

a4541a8

…sembled Also covers the four exit-status and output combinations the command result logging distinguishes. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

test: move the Cacti library fixture out of an ignored path

a5ce046

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

style: apply the project php-cs-fixer configuration

cfba047

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

somethingwithproof mentioned this pull request Aug 17, 2026

fix(rpn): correct the expression evaluator against rrdtool semantics #788

Open

bmfmancini previously approved these changes Aug 17, 2026

View reviewed changes

TheWitness requested changes Aug 17, 2026

View reviewed changes

This was referenced Aug 17, 2026

fix(thold): keep zero readings and stop falling back to the wrong data source #790

Open

fix(thold): correct counter delta, wrap modulus and percent denominator #791

Open

test: pin the threshold evaluator's behaviour before restructuring it #799

Open

somethingwithproof dismissed bmfmancini’s stale review via 2e91187 August 17, 2026 03:18

somethingwithproof force-pushed the security/consolidated-hardening-20260516 branch from 2e91187 to 917b1e0 Compare August 17, 2026 03:56

browniebraun approved these changes Aug 17, 2026

View reviewed changes

Merge branch 'develop' into security/consolidated-hardening-20260516

27e965a

Navigation Menu

Uh oh!

fix(security): consolidated hardening — SQL injection, XSS, unserialize, trigger_cmd - #773

fix(security): consolidated hardening — SQL injection, XSS, unserialize, trigger_cmd#773
somethingwithproof wants to merge 44 commits into
Cacti:developfrom
somethingwithproof:security/consolidated-hardening-20260516

somethingwithproof commented May 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

somethingwithproof commented Jul 29, 2026

Uh oh!

TheWitness left a comment

Uh oh!

TheWitness left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

somethingwithproof commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security

Bugs this uncovered

Tests

Compatibility

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

somethingwithproof commented Jul 29, 2026

Uh oh!

TheWitness left a comment

Choose a reason for hiding this comment

Uh oh!

TheWitness left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

somethingwithproof commented May 17, 2026 •

edited

Loading