hardening: migrate RLIKE to db_qstr and improve input handling by somethingwithproof · Pull Request #762 · Cacti/plugin_thold

somethingwithproof · 2026-03-20T09:10:38Z

Summary

Hardening

Convert all RLIKE string interpolation to db_qstr() (11 locations)
Migrate notify_lists.php bulk operations from db_execute with string
concatenation to db_execute_prepared with parameter binding
Convert DELETE plugin_thold_threshold_contact to prepared statement
Add intval() guards to get_request_var() values concatenated into
WHERE clauses in thold.php and thold_graph.php (data_template_id,
thold_template_id, site_id, host_id)
Use sanitize_unserialize_selected_items for form data deserialization
Apply html_escape to drp_action hidden form fields

CI

Convert test stub indentation from spaces to tabs (php-cs-fixer)
Drop unreachable --min=80 coverage threshold (plugin source files
cannot be loaded without the Cacti framework)

No behavioral changes. Defense-in-depth hardening only.

TheWitness · 2026-03-23T13:53:09Z

@somethingwithproof, please fix the automation and this is ready to merge.

somethingwithproof · 2026-03-24T06:05:45Z

Ran local pre-commit checks with PHP 8.4:

PHP lint: All files pass
PHPStan: 0 production code errors (27 test-only findings: missing iterable type specs in stubs, Pest todo() method)

SQL hardening: - Convert all db_execute with $selected_items concatenation to db_execute_prepared - Migrate RLIKE filter values from raw interpolation to db_qstr() - Add intval() guards on integer interpolations in thold.php and thold_graph.php - Replace cacti_unserialize(stripslashes()) with sanitize_unserialize_selected_items() - Fix indentation in notify_lists.php form_actions block - Pin shivammathur/setup-php to full commit SHA in CI Tests and tooling: - Add Pest test framework with PHPStan level 6 - Add XSS output escaping tests - Add prepared statement source-verification tests - Add security audit documentation (SECURITY-AUDIT.md, BACKLOG.md) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

Copilot

Pull request overview

This PR hardens the thold plugin against injection-style risks by tightening SQL construction (quoting RLIKE inputs, using prepared statements, and coercing numeric request values), and adds a lightweight security-focused CI/testing toolchain to validate the hardened call sites without requiring a full Cacti runtime.

Changes:

Replace interpolated RLIKE patterns with db_qstr() quoting and add intval() guards for request vars used in SQL.
Convert bulk DB operations in notify_lists.php to db_execute_prepared / db_fetch_*_prepared and adopt safer unserialization.
Introduce Pest/PHPUnit, PHPStan, Infection configs plus a GitHub Actions workflow and security audit docs.

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
thold.php	Adds `intval()` guards for numeric filters and quotes `RLIKE` input via `db_qstr()`.
thold_webapi.php	Switches serialized form input handling to `sanitize_unserialize_selected_items()` and adds a failure guard.
thold_graph.php	Hardens multiple SQL filters: `RLIKE` now uses `db_qstr()` and numeric filters use `intval()`.
notify_lists.php	Migrates concatenated SQL to prepared statements; adds parameter arrays for filters.
tests/Security/XssOutputTest.php	Adds security-oriented tests around output escaping and documents future seams.
tests/Security/PreparedStatementTest.php	Adds source-verification tests asserting hardened call sites exist.
tests/Helpers/GlobalStubs.php	Provides global stubs for Cacti functions for test execution.
tests/Helpers/CactiStubs.php	Adds an additional stubs helper (currently namespaced).
SECURITY.md	Adds a security policy and reporting guidance.
SECURITY-AUDIT.md	Adds a security audit backlog of findings and remediation notes.
phpunit.xml	Adds PHPUnit configuration and sets a bootstrap for test stubs.
phpstan.neon	Adds PHPStan configuration (strict rules, level 6).
pest.php	Adds Pest grouping configuration for tests.
infection.json	Adds Infection mutation testing configuration.
composer.json	Introduces Composer tooling/deps for tests and static analysis.
composer.lock	Locks the dev dependencies for repeatable CI installs.
BACKLOG.md	Adds a backlog derived from the security audit findings.
.github/workflows/ci.yml	Adds CI running Composer install, Pest (coverage), and PHPStan.

Copilot · 2026-04-03T21:08:37Z

notify_lists.php

 	if (get_request_var('associated') == 'true') {
-		$sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . '(notify_warning=' . get_request_var('id') . ' OR notify_alert=' . get_request_var('id') . ')';
+		$sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . '(notify_warning = ? OR notify_alert = ?)';
+		$sql_params[] = get_request_var('id');
+		$sql_params[] = get_request_var('id');


In templates(), the prepared WHERE clause binds get_request_var('id') directly. Since this value is logically an integer list id (and elsewhere in this PR you introduce intval() guards), it should be coerced to an int before binding (e.g. store an $safe_id = intval(get_request_var('id')) and bind that) to keep type-safety/defense-in-depth consistent.

Copilot · 2026-04-03T21:08:37Z

tests/Helpers/CactiStubs.php

+<?php
+declare(strict_types=1);
+
+namespace Cacti\PluginThold\Tests\Helpers;
+
+/*


tests/Helpers/CactiStubs.php declares a namespace, so the stub functions defined here would be namespaced (e.g. Cacti\PluginThold\Tests\Helpers\db_execute) rather than the global functions the plugin code calls. The function_exists('db_execute') guard also checks the global symbol, so this file will not behave as intended if the global stubs are not already loaded. Consider removing the namespace (or wrapping stubs in namespace { ... }) and consolidating with GlobalStubs.php to avoid duplicate/conflicting stubs.

Copilot · 2026-04-03T21:08:38Z

tests/Helpers/GlobalStubs.php

+	/* Must match Cacti's own implementation: ENT_QUOTES|ENT_HTML5, UTF-8. */
+	function html_escape(mixed $value): string
+	{
+		return htmlspecialchars((string) $value, ENT_QUOTES, 'UTF-8');


The html_escape() stub comment says it must match Cacti's implementation using ENT_QUOTES|ENT_HTML5, but the actual call uses only ENT_QUOTES. Either update the stub to include ENT_HTML5 or adjust the comment so tests don't encode assumptions that aren't implemented.

Suggested change

return htmlspecialchars((string) $value, ENT_QUOTES, 'UTF-8');

return htmlspecialchars((string) $value, ENT_QUOTES | ENT_HTML5, 'UTF-8');

Copilot · 2026-04-03T21:08:38Z

composer.json

+    "name": "cacti/plugin_thold",
+    "description": "Cacti plugin_thold plugin",
+    "type": "cacti-plugin",
+    "require": {"php": ">=8.4"},


composer.json pins the project to PHP >= 8.4. Repo documentation indicates the plugin targets PHP 8.1+ (see .github/copilot-instructions.md), so this requirement may unnecessarily drop supported environments. Consider lowering the constraint (e.g. >=8.1) or documenting why 8.4 is required, and ensure CI/tooling PHP versions match the chosen support window.

Suggested change

"require": {"php": ">=8.4"},

"require": {"php": ">=8.1"},

Copilot · 2026-04-03T21:08:38Z

.github/workflows/ci.yml

+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+      - uses: shivammathur/setup-php@728c6c6b8cf02c2e48117716a91ee48313958a19 # v2
+        with: {php-version: '8.4', coverage: xdebug}


CI is hard-pinned to PHP 8.4. If the plugin is intended to support PHP 8.1+ (per repo docs), consider running the workflow against the lowest supported PHP version (or a matrix like 8.1/8.2/8.3/8.4) so hardening changes are validated across the supported range.

Suggested change

steps:

- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683

- uses: shivammathur/setup-php@728c6c6b8cf02c2e48117716a91ee48313958a19 # v2

with: {php-version: '8.4', coverage: xdebug}

strategy:

matrix:

php-version: ['8.1', '8.2', '8.3', '8.4']

steps:

- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683

- uses: shivammathur/setup-php@728c6c6b8cf02c2e48117716a91ee48313958a19 # v2

with: {php-version: '${{ matrix.php-version }}', coverage: xdebug}

Copilot · 2026-04-03T21:08:38Z

tests/Security/PreparedStatementTest.php

+     * to prevent regex injection that could exhaust server memory (ReDoS) or
+     * expose data via crafted patterns.


The comment here implies quoting via db_qstr() prevents “regex injection” / ReDoS in RLIKE patterns. db_qstr() helps prevent SQL injection, but it does not constrain regex complexity—an expensive user-supplied regex can still cause backtracking/CPU spikes. Consider adjusting this comment to reflect what the change actually guarantees, or (if ReDoS is a concern) documenting/adding server-side regex complexity limits separately.

Suggested change

* to prevent regex injection that could exhaust server memory (ReDoS) or

* expose data via crafted patterns.

* so it is embedded in the SQL statement as data rather than SQL syntax.

* This verifies SQL-injection hardening for the interpolated RLIKE value;

* it does not, by itself, constrain regex complexity or mitigate ReDoS.

- Remove namespace from CactiStubs.php so stubs are globally reachable - Fix html_escape stub in both helpers to use ENT_QUOTES|ENT_HTML5 - Lower composer.json PHP requirement from >=8.4 to >=8.1 - Expand CI matrix to cover PHP 8.1/8.2/8.3/8.4 - Clarify PreparedStatementTest RLIKE comment: db_qstr prevents SQL injection but does not constrain regex complexity or mitigate ReDoS

somethingwithproof · 2026-04-03T21:50:27Z

CI is now passing. The coverage failure was caused by PHPUnit 10+/Pest v4 requiring a <source> filter in phpunit.xml before processing --coverage output — without it the runner exits non-zero even when all tests pass. Added the filter in ea8c8ce.

Also pushed a follow-up (6b1b19c) with test infrastructure corrections: removed namespace from CactiStubs.php so stubs are globally reachable, corrected the html_escape stub to ENT_QUOTES|ENT_HTML5, lowered the composer.json PHP floor to >=8.1, and expanded the CI matrix to 8.1/8.2/8.3/8.4.

TheWitness previously approved these changes Mar 23, 2026

View reviewed changes

somethingwithproof dismissed TheWitness’s stale review via cb1961b March 23, 2026 14:21

somethingwithproof force-pushed the fix/hardening-prepared-statements branch from ae36a9a to f17c7d1 Compare March 25, 2026 02:32

ci: add phpunit source filter to enable coverage reporting

ea8c8ce

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

Copilot AI review requested due to automatic review settings April 3, 2026 21:02

Copilot started reviewing on behalf of somethingwithproof April 3, 2026 21:03 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hardening: migrate RLIKE to db_qstr and improve input handling#762

hardening: migrate RLIKE to db_qstr and improve input handling#762
somethingwithproof wants to merge 3 commits intoCacti:developfrom
somethingwithproof:fix/hardening-prepared-statements

somethingwithproof commented Mar 20, 2026 •

edited

Loading

Uh oh!

TheWitness commented Mar 23, 2026

Uh oh!

somethingwithproof commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

somethingwithproof commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	return htmlspecialchars((string) $value, ENT_QUOTES, 'UTF-8');
	return htmlspecialchars((string) $value, ENT_QUOTES \| ENT_HTML5, 'UTF-8');

		* to prevent regex injection that could exhaust server memory (ReDoS) or
		* expose data via crafted patterns.

-     * to prevent regex injection that could exhaust server memory (ReDoS) or
-     * expose data via crafted patterns.
+     * so it is embedded in the SQL statement as data rather than SQL syntax.
+     * This verifies SQL-injection hardening for the interpolated RLIKE value;
+     * it does not, by itself, constrain regex complexity or mitigate ReDoS.

Conversation

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

Uh oh!

Summary

Uh oh!

TheWitness commented Mar 23, 2026

Uh oh!

somethingwithproof commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

somethingwithproof commented Mar 20, 2026 •

edited

Loading