fix: security hardening, bug fixes, and test suite (PR #215 follow-up) by somethingwithproof · Pull Request #225 · Cacti/plugin_weathermap

somethingwithproof · 2026-05-17T03:34:05Z

Summary

Security fixes (all confirmed exploitable or close to it):

Shell injection (fping): target validated with allowlist regex before use, cacti_escapeshellarg() applied at exec boundary (was unescaped)
Shell injection (rrd): $extra_options split by whitespace and each token individually cacti_escapeshellarg()'d (replaced manual strchr quoting that missed embedded quotes)
SQL injection (cacti-mapper): static db_fetch_assoc converted to db_fetch_assoc_prepared
SQL injection (group_move): raw multi-step UPDATE replaced with two db_execute_prepared calls
Settings never saved: REPLACE INFO typo fixed to REPLACE INTO — all map settings were silently discarded since this function was added
XSS: html_escape() added on $maptitle at viewer output (lines 407, 658) and config file preview in mgmt

Logic bugs:

dir($orig_cwd) → chdir($orig_cwd) at image-serving paths (returned a Directory object, did not change cwd)
Cron field destructuring was [$minute, $hour, $wday, $day, $month]; fixed to [$minute, $hour, $day, $month, $wday]
render_colour() sentinel checks used $col[1] in the third operand instead of $col[2] — 'none'/'copy'/'contrast' sentinels fired incorrectly for mixed channels
Division by zero when max_bandwidth_out/max_bandwidth_in is 0 (unset on a link)
ReadData base returned 2 elements; engine destructures 3 — PHP notice + wrong $time
ColourFromPercent had print ... exit() debug statements preventing all scale colour rendering

Cleanup:

Dead escapeHtml function removed from editor.js (DOM-based fix was applied, JS version unused)
Semgrep nosemgrep annotations for pre-existing audit findings in plugin loader (argv[0]-relative path for CLI; bounded by admin-controlled config dir)

Test plan

./vendor/bin/pest passes — 113 tests, 0 failures
Unit: RenderColourTest (sentinel + mutation guards), BandwidthPercentageTest (zero guard, half-duplex), CronFieldOrderTest
Handoff: DataSourceContractTest (3-element ReadData return), XssEscapingHandoffTest
Integration: SettingsPersistenceTest (REPLACE INTO SQL, prepared statements in mgmt)
Mutation: FixedBugRegressionTest (5 regression suites, one per fixed bug)
Smoke: PluginFilesSyntaxTest (in-process parse, hook functions, datasource discovery)

🤖 Generated with Claude Code

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

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>

Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

…ity/consolidated-modernization-20260412

- fping: tighten IPv6/DNS hostname regex, use preg_quote, log via json_encode - rrd: split extra_options by whitespace and escapeshellarg each token - WeatherMap.class.php: annotate pre-existing audit findings (plugin loader uses argv[0]-relative path; tainted-object-instantiation and unlink in cache eviction are bounded by admin-controlled config dir) - WeatherMap.class.php: remove debug print+exit left in ColourFromPercent - WeatherMap.class.php: guard bandwidth percentage division against zero max - WeatherMap.class.php: fix base ReadData return to include third element - WeatherMap.functions.php: fix render_colour copy-paste; third condition was checking col[1] instead of col[2] for none/copy/contrast sentinels - poller-common.php: fix cron field order (wday was in month slot) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

- weathermap-cacti-plugin.php: html_escape maptitle at lines 407 and 658; fix dir() vs chdir() at image-serving paths; correct liveview flat-array access ($map['configfile'] not $map[0]['configfile']); add mime_map for Content-Type header - weathermap-cacti-plugin-mgmt.php: fix REPLACE INFO -> REPLACE INTO (settings were silently never persisted); replace raw SQL in group_move with db_execute_prepared; html_escape config preview output; remove stray $name='' that clobbered the fetched map name - js/editor.js: remove dead escapeHtml function (unused after DOM-based fix) - tests/Security/AuthGuardTest.php: rewrite to test actual web entry points - tests/Security/PreparedStatementConsistencyTest.php: tighten to toBe(0) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Unit: - RenderColourTest: sentinel values and mutation guards for col[1]→col[2] fix - BandwidthPercentageTest: zero-max guard, half-duplex, and over-limit cases - CronFieldOrderTest: field order regression for minute/hour/day/month/wday fix Handoff: - DataSourceContractTest: ReadData base returns 3-element [-1,-1,0] array - XssEscapingHandoffTest: html_escape at maptitle and config preview outputs Integration: - SettingsPersistenceTest: REPLACE INTO SQL and prepared statements in mgmt Mutation: - FixedBugRegressionTest: 5 fixed-bug regression suites (colour sentinel, bandwidth zero, ReadData return, REPLACE INFO typo, dir vs chdir) Smoke: - PluginFilesSyntaxTest: in-process parse check, hook function existence, datasource discovery, extends WeatherMapDataSource assertion Infrastructure: - bootstrap.php: add describe() polyfill for Pest v1 (no native describe) - phpunit.xml: suppress E_DEPRECATED via <ini> to avoid Pest v1/PHP 8.4 noise - cli/cacti-mapper.php: convert static db_fetch_assoc to _prepared - SetupStructureTest.php: fix pre-existing INI-based version assertion - BandwidthPercentageTest.php: round() to avoid float representation noise - PluginFilesSyntaxTest.php: remove phantom second needle from toContain call Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Replace bare escapeshellarg() with cacti_escapeshellarg() per Cacti idiom. The wrapper handles cross-platform quoting (Windows CMD/PowerShell) and is the required form in all Cacti plugin code. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot

Pull request overview

This PR is a follow-up to #215 that bundles security hardening (shell-injection escaping in fping/rrd datasources, SQL-injection conversion to prepared statements, XSS escaping on $maptitle and config preview), logic-bug fixes (dir()→chdir(), swapped cron field destructuring, render_colour sentinel using $col[1] instead of $col[2], division-by-zero on bandwidth %, base ReadData 2-vs-3 element return, REPLACE INFO→REPLACE INTO, debug print/exit left in ColourFromPercent), and a large amount of incidental cleanup (jQuery .click()→.on('click'), declare(strict_types=1); added to every PHP file, editor action allowlist, and a new Pest test suite that is largely source-text matching).

Changes:

Real security/logic fixes in lib/WeatherMap.class.php, datasources, lib/poller-common.php, lib/editor.inc.php, and the two main weathermap-cacti-plugin*.php entry points.
declare(strict_types=1); added to ~40 files (engine, CLI, locale stubs, image-dir stubs) plus jQuery event-binding modernisation in two JS files.
New tests/ tree (Pest), phpunit.xml, phpstan.neon, composer.json, SECURITY.md, SECURITY-AUDIT.md, BACKLOG.md, and Semgrep nosemgrep annotations on pre-existing flagged sites.

Reviewed changes

Copilot reviewed 90 out of 91 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
weathermap-cacti-plugin.php	`dir()`→`chdir()`, mime map, `html_escape($maptitle)`, `$map[0]`→`$map`, jQuery `.on()`
weathermap-cacti-plugin-mgmt.php	`REPLACE INFO`→`REPLACE INTO`, prepared statements in `weathermap_group_move`, `html_escape($buffer)`, jQuery `.on()`
weathermap-cacti-plugin-editor.php	`wm_editor_sanitize_action` allowlist for the action var
lib/WeatherMap.class.php	`render_colour` sentinel fix, bandwidth zero-guard, base `ReadData` 3-element return, `ColourFromPercent` debug removed, nosemgrep annotations
lib/WeatherMap.functions.php	`render_colour` `$col[2]` fix, comment touch-ups
lib/datasources/WeatherMapDataSource_fping.php	Target allowlist + `escapeshellarg`; `null,null,0` rejection path
lib/datasources/WeatherMapDataSource_rrd.php	`escapeshellarg` per arg, split `$extra_options`
lib/poller-common.php	Cron field order fix (`day`/`month`/`wday`), `$bgfile`→`$objfile` writable check
lib/editor.inc.php	`wm_editor_sanitize_action` helper, backslash check in conffile sanitizer
cli/cacti-mapper.php	`db_fetch_assoc_prepared`, nicename null-coalesce
js/editor.js, js/map-cycle.js, js/jquery.ddslick.js	DOM-safe option construction; `.click`/`.keyup` → `.on()`
~40 other PHP files	`declare(strict_types=1);` added; otherwise unchanged
tests/**	New Pest suite (most tests are `file_get_contents` + `toContain` source-text checks)
phpunit.xml, phpstan.neon, composer.json, infection.json	New tooling config
SECURITY.md, SECURITY-AUDIT.md, BACKLOG.md	New documentation

+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
+         bootstrap="tests/Helpers/GlobalStubs.php"


+					$mime_map = ['png' => 'image/png', 'jpg' => 'image/jpeg', 'jpeg' => 'image/jpeg', 'gif' => 'image/gif'];
+					header('Content-type: ' . ($mime_map[$imageformat] ?? 'image/png'));


+
+declare(strict_types=1);


phpunit.xml: point bootstrap at tests/bootstrap.php, which contains all Cacti function stubs. GlobalStubs.php was only a comment block and would cause tests to fail when run via plain phpunit. WeatherMapDataSource_fping.php: return [-1, -1, 0] on rejected target, not [null, null, 0]. The engine treats -1 as the "no valid data" sentinel; null on this one rejection path made the contract inconsistent with every other datasource failure path. WeatherMap.class.php: fix bracket/paren transposition in commented-out debug line ($dir)] to $dir])). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Raw $ping_count interpolated into the popen command string after $target was escaped. A map config PINGCOUNT value containing shell metacharacters would bypass the cacti_escapeshellarg guard on $target and inject into the adjacent unquoted argument. Add (int) cast to eliminate the injection surface. Add two source-level regression tests asserting the cast is present and that no raw variable interpolation remains on that command line. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: TheWitness <thewitness@cacti.net>

somethingwithproof added 17 commits April 8, 2026 23:01

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

824110b

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

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>

refactor: add strict typing and clean up standalone infra

78b6aa1

refactor: safe PHP 7.4 modernization (arrays, null coalescing)

2d851b3

fix: restore corrupted function calls from refactor tool

fc4086b

Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix: restore corrupted array access and is_array calls

cc9bf96

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

style: remove trailing double semicolons

90f2f62

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>

fix(security): normalize editor actions at entrypoint

83e7cf6

Merge remote-tracking branch 'fork/refactor/modernization' into secur…

c455244

…ity/consolidated-modernization-20260412

Copilot AI review requested due to automatic review settings May 17, 2026 03:34

Copilot started reviewing on behalf of somethingwithproof May 17, 2026 03:34 View session

Copilot AI reviewed May 17, 2026

View reviewed changes

somethingwithproof added 2 commits May 16, 2026 22:42

somethingwithproof mentioned this pull request May 17, 2026

test: add Pest v1 security test infrastructure #217

Closed

2 tasks

TheWitness added 2 commits May 18, 2026 10:56

Fix query fetching SNMP details from the database

c6e3ccd

Signed-off-by: TheWitness <thewitness@cacti.net>

Merge branch 'develop' into security/consolidated-modernization-20260412

243e25e

Signed-off-by: TheWitness <thewitness@cacti.net>

TheWitness previously approved these changes May 18, 2026

View reviewed changes

Merge branch 'develop' into security/consolidated-modernization-20260412

b346736

Signed-off-by: TheWitness <thewitness@cacti.net>

TheWitness dismissed their stale review via b346736 May 19, 2026 01:13

Remove is_executable check for fping_cmd

1d73ed0

Signed-off-by: TheWitness <thewitness@cacti.net>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

fix: security hardening, bug fixes, and test suite (PR #215 follow-up)#225
somethingwithproof wants to merge 23 commits into
Cacti:developfrom
somethingwithproof:security/consolidated-modernization-20260412

somethingwithproof commented May 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		$mime_map = ['png' => 'image/png', 'jpg' => 'image/jpeg', 'jpeg' => 'image/jpeg', 'gif' => 'image/gif'];
		header('Content-type: ' . ($mime_map[$imageformat] ?? 'image/png'));


		declare(strict_types=1);

Conversation

somethingwithproof commented May 17, 2026

Summary

Test plan

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants