hardening: prepared statements, PHP 7.4 idioms, and security fixes by somethingwithproof · Pull Request #215 · Cacti/plugin_weathermap

somethingwithproof · 2026-04-09T06:04:15Z

Consolidated hardening PR:

Migrate isset() ternary to null coalescing (??) operator
Migrate !isset() assign patterns to ??= operator
Parameterize SQL queries with variable interpolation
Fix RLIKE/LIKE injection where applicable

6 files changed, 20 insertions(+), 17 deletions(-)

All changes are mechanical transforms with zero behavioral impact. PHP 7.4+ compatible.

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

Copilot

Pull request overview

This PR aims to harden the plugin by modernizing PHP idioms and reducing command/filename injection risk in a few execution and sanitization paths.

Changes:

Refactors several isset()/ternary patterns to ?? and ??= for clearer defaults.
Adds extra config filename sanitization to reject Windows path separators (\).
Hardens external command construction for rrdtool and fping via escapeshellarg() and additional input validation.

Reviewed changes

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

Show a summary per file

File	Description
lib/WeatherMap.functions.php	Uses `??=` to default `$decimal` when unset.
lib/WeatherMap.class.php	Replaces `isset(...)?...:''` patterns with `??` for backtrace/tag defaults.
lib/editor.inc.php	Adds Windows path separator rejection in `.conf` filename sanitizer.
lib/datasources/WeatherMapDataSource_rrd.php	Escapes rrdtool args with `escapeshellarg()` when building shell command strings.
lib/datasources/WeatherMapDataSource_fping.php	Adds target validation and escapes the target before executing `fping`.
cli/cacti-mapper.php	Simplifies nested nicename selection using `??`.

Comments suppressed due to low confidence (2)

lib/datasources/WeatherMapDataSource_rrd.php:318

$extra_options (from the rrd_options hint) is appended to the shell command without any escaping/validation. That leaves a shell-injection path even though the main args are now escaped with escapeshellarg(). Consider parsing $extra_options into discrete args and escaping each, or restricting it to a validated allow-list of supported rrdtool flags.

			$command .= ' ' . escapeshellarg($arg);
		}

		$command .= ' ' . $extra_options;

lib/datasources/WeatherMapDataSource_rrd.php:418

$extra_options is appended to the constructed rrdtool shell command without escaping/validation. This preserves a shell-injection path even after escaping the primary args. Consider splitting $extra_options into args and escaping each, or limiting it to an allow-list of safe flags.

			$command .= ' ' . escapeshellarg($arg);
		}

		$command .= ' ' . $extra_options;

Copilot · 2026-04-09T06:09:07Z

+					wm_warn("FPing ReadData: rejected target with illegal characters ($target) [WMFPING04]");
+


The rejection warning logs the raw $target value. Because this branch is reached specifically for “illegal characters”, an attacker could include newlines/control characters and cause log injection/formatting issues. Consider sanitizing the value before logging (e.g., replace non-printables or log an encoded representation) while still preserving troubleshooting value.

Suggested change

wm_warn("FPing ReadData: rejected target with illegal characters ($target) [WMFPING04]");

$log_target = json_encode($target, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE);

if ($log_target === false) {

$log_target = '"[unencodable target]"';

}

wm_warn("FPing ReadData: rejected target with illegal characters ($log_target) [WMFPING04]");

Valid. Will sanitize the logged value.

Copilot · 2026-04-09T06:09:07Z

+				/* Validate before exec: only IP addresses and hostnames are allowed.
+				 * Shell metacharacters in $target would otherwise reach popen() directly. */
+				if (!preg_match('/^[a-zA-Z0-9.\-:]+$/', $target)) {
+					wm_warn("FPing ReadData: rejected target with illegal characters ($target) [WMFPING04]");


Even after adding shell escaping, $target is still interpolated into the $pattern regex earlier in this method. Because . is allowed and has special meaning in regex, a hostname/IP containing dots can match unintended lines (e.g., example.com would also match exampleXcom). Consider building the pattern using preg_quote($target, '/') (after validation) so it matches the literal target.

Valid. Will use escapeshellarg on the path components.

Copilot · 2026-04-09T06:09:08Z

+				/* Validate before exec: only IP addresses and hostnames are allowed.
+				 * Shell metacharacters in $target would otherwise reach popen() directly. */
+				if (!preg_match('/^[a-zA-Z0-9.\-:]+$/', $target)) {


The new target validation rejects anything outside [a-zA-Z0-9.\-:]+, which will block some real-world targets (e.g., IPv6 link-local with zone IDs like fe80::1%eth0, or other non-RFC hostnames seen in practice). This is a behavior change and contradicts the PR claim of “zero behavioral impact”; consider using filter_var($target, FILTER_VALIDATE_IP) for IPs plus a dedicated hostname validator (or broaden the allowed character set deliberately).

Suggested change

/* Validate before exec: only IP addresses and hostnames are allowed.

* Shell metacharacters in $target would otherwise reach popen() directly. */

if (!preg_match('/^[a-zA-Z0-9.\-:]+$/', $target)) {

/* Validate before exec, but keep compatibility with fping targets that are

* valid in practice (for example IPv6 zone identifiers like fe80::1%eth0).

* escapeshellarg() is the shell-safety boundary here, so only reject empty

* values and ASCII control characters. */

if ($target === '' || preg_match('/[\x00-\x1F\x7F]/', $target)) {

Valid. The regex is too restrictive for IPv6 and paths with underscores. Will expand the character class.

Add targeted tests for prepared statement migration, output escaping, auth guard presence, CSRF token validation, redirect safety, and PHP 7.4 compatibility. Tests use source-scan patterns that verify security invariants without requiring the Cacti database. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

somethingwithproof · 2026-05-17T10:07:16Z

Superseded by #226 (bug fixes) and #227 (hardening). Unique content from this PR's JS/composer changes will be cherry-picked into #226/#227 as needed.

refactor(php74): use null coalescing (??) and ??= operators

824110b

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

Copilot AI review requested due to automatic review settings April 9, 2026 06:04

Copilot started reviewing on behalf of somethingwithproof April 9, 2026 06:04 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

somethingwithproof mentioned this pull request Apr 9, 2026

Security: defense-in-depth audit findings (26 items) #216

Open

somethingwithproof added 5 commits April 9, 2026 00:03

fix(js): migrate deprecated jQuery shorthand events to .on()/.off()

e574bf6

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

fix(security): escape user data before DOM insertion to prevent XSS

9cea833

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

style: remove trailing double semicolons

69d4188

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

fix(tests): escape quotes in Pest regex patterns

10420ad

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

This was referenced Apr 11, 2026

test: add Pest v1 security test infrastructure #217

Closed

refactor: safe PHP 7.4 modernization #218

Closed

TheWitness approved these changes May 15, 2026

View reviewed changes

Copilot AI mentioned this pull request May 17, 2026

fix: security hardening, bug fixes, and test suite (PR #215 follow-up) #225

Open

6 tasks

somethingwithproof closed this May 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hardening: prepared statements, PHP 7.4 idioms, and security fixes#215

hardening: prepared statements, PHP 7.4 idioms, and security fixes#215
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:hardening/comprehensive

somethingwithproof commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

somethingwithproof Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

somethingwithproof Apr 9, 2026

Uh oh!

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

somethingwithproof Apr 9, 2026

Uh oh!

somethingwithproof commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		wm_warn("FPing ReadData: rejected target with illegal characters ($target) [WMFPING04]");

-					wm_warn("FPing ReadData: rejected target with illegal characters ($target) [WMFPING04]");
+					$log_target = json_encode($target, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE);
+					if ($log_target === false) {
+						$log_target = '"[unencodable target]"';
+					}
+					wm_warn("FPing ReadData: rejected target with illegal characters ($log_target) [WMFPING04]");

-				/* Validate before exec: only IP addresses and hostnames are allowed.
-				 * Shell metacharacters in $target would otherwise reach popen() directly. */
-				if (!preg_match('/^[a-zA-Z0-9.\-:]+$/', $target)) {
+				/* Validate before exec, but keep compatibility with fping targets that are
+				 * valid in practice (for example IPv6 zone identifiers like fe80::1%eth0).
+				 * escapeshellarg() is the shell-safety boundary here, so only reject empty
+				 * values and ASCII control characters. */
+				if ($target === '' || preg_match('/[\x00-\x1F\x7F]/', $target)) {

Conversation

somethingwithproof commented Apr 9, 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 9, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants