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

[somethingwithproof]

What

Consolidates security hardening from PRs #766, #767, #769, #770, #771, #772 into a single reviewable diff.

SQL injection — bulk form actions (notify_lists.php)

• Replaced array_to_sql_or() and direct $selected_items[$i] string concatenation with db_execute_prepared() and IN (?,?,?) placeholders built via implode(',', array_fill(0, cacti_sizeof($selected_items), '?')).
• drp_action allowlist check used array_keys() (returns ints) with in_array(..., true) (strict); added array_map('strval', ...) to align types.
• All four bulk action blocks are wrapped in db_begin_transaction() / db_commit_transaction(). Every db_execute_prepared() return is tracked in $ok; false calls db_rollback_transaction() and breaks per-item loops immediately.
• The flat delete block chains all eight statements with && so the first failure short-circuits the rest rather than running all statements before rollback.
• thold_template_update_thresholds() moved after commit so the cascade does not participate in the transaction boundary.
• Original code had SET thold_host_email = 0 AND deleted="" (MySQL parsed AND deleted="" as part of the SET expression, a bug). Corrected: deleted = "" moved to WHERE clause.

SQL injection — graph filter (thold_functions.php)

• get_allowed_thresholds() and get_allowed_threshold_logs(): $graph_id was interpolated directly into the WHERE clause. Changed to gl.id = ? placeholder with $sql_params[] accumulator. Both db_fetch_assoc_prepared() and db_fetch_cell_prepared() now receive bound params.

RLIKE injection (notify_lists.php, thold.php, thold_graph.php)

• rfilter validated via FILTER_VALIDATE_IS_REGEX before reaching any SQL. db_qstr() escapes the already-validated value for use in RLIKE clauses.

XSS — hidden inputs and AJAX URL params

• get_request_var('page') in thold.php and thold_graph.php hidden inputs wrapped with html_escape().
• AJAX filter URL parameters wrapped with encodeURIComponent() across thold.php, thold_graph.php, notify_lists.php, thold_templates.php, thold_webapi.php.

Object injection (thold_webapi.php)

• sanitize_unserialize_selected_items() applied to selected_graphs_array; raw unserialize() call removed.

PHP 7.4 compatibility (setup.php)

• count($selected_items) changed to cacti_sizeof($selected_items).

Why

All six original PRs addressed overlapping sets of vulnerabilities without complete test coverage or atomic transaction boundaries. Pre-authentication SQL injection via bulk form actions was the highest-severity issue: array_to_sql_or() constructed WHERE clauses from unsanitized POST data. The RLIKE injection via rfilter and the XSS via the page parameter were secondary. Atomicity was missing entirely — partial deletes across 8 tables could leave orphaned host references.

Test plan

• vendor/bin/pest tests/ — 53 tests, all passing
• PreparedStatementTest.php — prepared-statement usage, transaction count, rollback presence, $ok tracking, loop break guards, template cascade position
• RlikeInjectionTest.php — FILTER_VALIDATE_IS_REGEX presence, db_qstr() usage, no raw $rfilter interpolation
• XssEscapingTest.php — html_escape() on page inputs, encodeURIComponent() in AJAX filters
• UnserializeHardeningTest.php — sanitize_unserialize_selected_items() applied, object injection blocked
• Php74CompatibilityTest.php — no PHP 8.0+ syntax across 9 plugin files
• PreparedStatementConsistencyTest.php — no single-line interpolated raw db_*() calls in hardened files
• PhpSyntaxTest.php (smoke) — all plugin PHP files lint clean

[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/                                                   |